fix: keep merge suggestion UI in sync when pairs are rescored - #4506
Conversation
Signed-off-by: Yeganathan S <63534555+skwowet@users.noreply.github.com>
|
Your PR title doesn't contain a Jira issue key. Consider adding it for better traceability. Example:
Projects:
Please add a Jira issue key to your PR title. |
Signed-off-by: Yeganathan S <63534555+skwowet@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Synchronizes raw and UI merge-suggestion tables when pairs are rescored.
Changes:
- Unifies raw/UI writes behind threshold-aware repository methods.
- Removes stale UI suggestions when scores fall below the threshold.
- Updates generation and enrichment callers to use the unified path.
Blocking concerns: Temporal workflow changes require versioning for in-flight executions. The PR title also lacks the required JIRA key.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
organizationMergeSuggestions.repo.ts |
Synchronizes organization suggestion tables. |
memberMergeSuggestions.repo.ts |
Synchronizes member suggestion tables. |
generateOrganizationMergeSuggestions.ts |
Uses one organization write activity. |
generateMemberMergeSuggestions.ts |
Uses one member write activity. |
activities/organizationMergeSuggestions.ts |
Accepts a similarity threshold. |
activities/memberMergeSuggestions.ts |
Accepts a similarity threshold. |
enrichment.ts |
Uses unified organization suggestion writes. |
Suppressed comments (2)
services/apps/merge_suggestions_worker/src/workflows/generateMemberMergeSuggestions.ts:53
- This changes the persisted Temporal command sequence from two activities to one without a patch or worker versioning. An in-flight execution whose history already contains the old second activity will replay this code and emit
continueAsNewinstead, causing nondeterminism; an old activity dispatched after deployment can also pass the former table-name string into the new numeric threshold parameter. Gate the sequence with Temporal versioning and keep the activity argument backward-compatible until old runs drain.
await activity.addMemberToMerge(allMergeSuggestions, SIMILARITY_CONFIDENCE_SCORE_THRESHOLD)
services/apps/merge_suggestions_worker/src/workflows/generateOrganizationMergeSuggestions.ts:59
- This changes the persisted Temporal command sequence from two activities to one without a patch or worker versioning. An in-flight execution whose history already contains the old second activity will replay this code and emit
continueAsNewinstead, causing nondeterminism; an old activity dispatched after deployment can also pass the former table-name string into the new numeric threshold parameter. Gate the sequence with Temporal versioning and keep the activity argument backward-compatible until old runs drain.
await activity.addOrganizationToMerge(
allMergeSuggestions,
SIMILARITY_CONFIDENCE_SCORE_THRESHOLD,
)
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (4)
Previously missed (2) — in code that hasn't changed since the last review.
services/apps/merge_suggestions_worker/src/activities/memberMergeSuggestions.ts:388
- These writes are still non-atomic because the repository receives a plain connection. Concurrent high/low rescoring can interleave between the raw update and the filtered-table delete, recreating a stale UI row; a failure can also leave the tables divergent. Execute the repository call in one pg-promise transaction.
const memberMergeSuggestionsRepo = new MemberMergeSuggestionsRepository(
svc.postgres.writer.connection(),
svc.log,
)
await memberMergeSuggestionsRepo.addToMerge(suggestions, similarityThreshold)
services/apps/merge_suggestions_worker/src/activities/organizationMergeSuggestions.ts:400
- These writes are still non-atomic because the repository receives a plain connection. Concurrent high/low rescoring can interleave between the raw update and the filtered-table delete, recreating a stale UI row; a failure can also leave the tables divergent. Execute the repository call in one pg-promise transaction.
const organizationMergeSuggestionsRepo = new OrganizationMergeSuggestionsRepository(
svc.postgres.writer.connection(),
svc.log,
)
await organizationMergeSuggestionsRepo.addToMerge(suggestions, similarityThreshold)
services/apps/merge_suggestions_worker/src/workflows/generateMemberMergeSuggestions.ts:52
- This comment only narrates the newly introduced behavior, which conflicts with the repository's no-narrative-comments rule in
CLAUDE.md:72-84. Remove it; the activity and repository names should express the behavior.
// Writes raw and UI together so a rescore below the threshold cannot leave a stale UI row.
services/apps/merge_suggestions_worker/src/workflows/generateOrganizationMergeSuggestions.ts:55
- This comment only narrates the newly introduced behavior, which conflicts with the repository's no-narrative-comments rule in
CLAUDE.md:72-84. Remove it; the activity and repository names should express the behavior.
// Writes raw and UI together so a rescore below the threshold cannot leave a stale UI row.
Signed-off-by: Yeganathan S <63534555+skwowet@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Suppressed comments (4)
Previously missed (2) — in code that hasn't changed since the last review.
services/libs/data-access-layer/src/old/apps/merge_suggestions_worker/memberMergeSuggestions.repo.ts:146
- This comment only narrates the immediately following
update; remove it so the code remains self-explanatory.
This issue also appears on line 169 of the same file.
// Update existing rows if they already exist
services/libs/data-access-layer/src/old/apps/merge_suggestions_worker/organizationMergeSuggestions.repo.ts:142
- This comment only narrates the immediately following
update; remove it so the code remains self-explanatory.
This issue also appears on line 164 of the same file.
// Update existing rows if they already exist
services/libs/data-access-layer/src/old/apps/merge_suggestions_worker/memberMergeSuggestions.repo.ts:169
- This comment only narrates the immediately following
insert; remove it so the code remains self-explanatory.
// Insert only new rows and enforce bidirectional uniqueness
services/libs/data-access-layer/src/old/apps/merge_suggestions_worker/organizationMergeSuggestions.repo.ts:164
- This comment only narrates the immediately following
insert; remove it so the code remains self-explanatory.
// insert only new rows and enforce bidirectional uniqueness
Summary
memberToMerge/organizationToMergecould keep an old high similarity after a later run scored the same pair lower in the raw tables, so the UI showed stale merge suggestions.addToMergenow writes raw and UI together: always upsert raw, upsert UI only above the similarity threshold, and delete the UI row when the new score is at or below the cutoff.Changes
addToMergetake an optionalsimilarityThreshold(default0.75) instead of a target table. Generate workflows still pass the threshold; enrichment uses the default.addToMergecalls (all pairs to raw, then a filtered subset to UI).