Merge suggestions quick wins #2760
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request introduces modifications to the merge suggestions worker, focusing on enhancing the handling of member and organization merge suggestions. The changes include renaming existing functions, adding new functions for managing merge suggestions, and adjusting similarity confidence score thresholds. The modifications aim to provide more granular control over merge suggestion processing, with improved error handling and the ability to track suggestions that should not be merged. Changes
Sequence DiagramsequenceDiagram
participant Workflow as Merge Suggestions Workflow
participant LLM as Language Model
participant Activities as Merge Suggestion Activities
participant Repository as Merge Suggestion Repository
Workflow->>LLM: Request merge suggestion verdict
LLM-->>Workflow: Return verdict
alt Merge Suggested
Workflow->>Activities: Process merge
else No Merge
Workflow->>Activities: Remove suggestion
Activities->>Repository: Add to no-merge list
end
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
services/apps/merge_suggestions_worker/src/workflows/generateMemberMergeSuggestions.ts (1)
Line range hint
53-61: Consider adding metrics collection for filtered suggestionsThe code maintains both raw and filtered suggestions, which is good for analysis. Consider adding metrics to track the ratio of filtered vs raw suggestions to validate this threshold change.
This would help in:
- Monitoring the impact of the increased threshold
- Fine-tuning the threshold based on real-world data
- Detecting potential issues with the similarity calculation
services/libs/data-access-layer/src/member_merge/index.ts (1)
31-31: SQL keyword casing should be uppercase for consistency.The implementation correctly handles duplicate entries, but for SQL convention consistency:
- on conflict ("memberId", "noMergeId") do nothing + ON CONFLICT ("memberId", "noMergeId") DO NOTHINGservices/libs/data-access-layer/src/org_merge/index.ts (1)
22-41: Implementation looks good but needs SQL keyword casing fix.The new function follows the same pattern as member merge handling, which is good for consistency.
- on conflict ("organizationId", "noMergeId") do nothing + ON CONFLICT ("organizationId", "noMergeId") DO NOTHINGservices/apps/merge_suggestions_worker/src/workflows/mergeOrganizationsWithLLM.ts (1)
90-102: Consider enhancing the LLM verdict handling.A few suggestions to make the code more robust:
- Add error handling for
addOrganizationSuggestionToNoMerge- Consider using a constant for the 'true' comparison
+const SAME_ORGANIZATION_VERDICT = 'true'; + - if (llmResult.body.content[0].text === 'true') { + if (llmResult.body.content[0].text === SAME_ORGANIZATION_VERDICT) {Also, consider wrapping the operations in a try-catch block:
} else { console.log( `LLM doesn't think these orgs are the same. Removing from suggestions and adding to no merge: ${suggestion[0]} and ${suggestion[1]}!`, ) + try { await organizationActivitiesProxy.removeOrganizationMergeSuggestions( suggestion, OrganizationMergeSuggestionTable.ORGANIZATION_TO_MERGE_FILTERED, ) await organizationActivitiesProxy.removeOrganizationMergeSuggestions( suggestion, OrganizationMergeSuggestionTable.ORGANIZATION_TO_MERGE_RAW, ) await organizationActivitiesProxy.addOrganizationSuggestionToNoMerge(suggestion) + } catch (error) { + console.error('Failed to process non-matching organizations:', error); + // Consider if we need to retry or handle specific error cases + } }services/libs/data-access-layer/src/old/apps/merge_suggestions_worker/organizationMergeSuggestions.repo.ts (1)
Line range hint
304-324: Fix typo in error message.The method improvements look good, but there's a typo in the error message: "rom" should be "from".
- this.log.error(`Error removing organization suggestions rom ${table}!`, error) + this.log.error(`Error removing organization suggestions from ${table}!`, error)services/apps/merge_suggestions_worker/src/activities/memberMergeSuggestions.ts (1)
366-373: Consider enhancing error handling.While the method correctly validates the array length, it silently returns on invalid input. Consider either:
- Throwing an error for invalid input
- Adding warning level logging instead of debug
export async function addMemberSuggestionToNoMerge(suggestion: string[]): Promise<void> { if (suggestion.length !== 2) { - svc.log.debug(`Suggestions array must have two ids!`) - return + svc.log.warn(`Invalid suggestion array length: ${suggestion.length}. Expected: 2`) + return } const qx = pgpQx(svc.postgres.writer.connection()) await addMemberNoMerge(qx, suggestion[0], suggestion[1]) }services/apps/merge_suggestions_worker/src/workflows/generateOrganizationMergeSuggestions.ts (1)
Line range hint
56-65: Consider adding logging for filtered suggestionsThe code filters suggestions but doesn't log how many were filtered out. This information would be valuable for monitoring the impact of the new threshold.
Consider adding logging before and after filtering:
if (allMergeSuggestions.length > 0) { + console.log(`Found ${allMergeSuggestions.length} total merge suggestions`); await activity.addOrganizationToMerge( allMergeSuggestions, OrganizationMergeSuggestionTable.ORGANIZATION_TO_MERGE_RAW, ) + const filteredSuggestions = allMergeSuggestions.filter((s) => s.similarity > SIMILARITY_CONFIDENCE_SCORE_THRESHOLD); + console.log(`${filteredSuggestions.length} suggestions passed the similarity threshold of ${SIMILARITY_CONFIDENCE_SCORE_THRESHOLD}`); await activity.addOrganizationToMerge( - allMergeSuggestions.filter((s) => s.similarity > SIMILARITY_CONFIDENCE_SCORE_THRESHOLD), + filteredSuggestions, OrganizationMergeSuggestionTable.ORGANIZATION_TO_MERGE_FILTERED, ) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
services/apps/merge_suggestions_worker/src/activities.ts(2 hunks)services/apps/merge_suggestions_worker/src/activities/memberMergeSuggestions.ts(2 hunks)services/apps/merge_suggestions_worker/src/activities/organizationMergeSuggestions.ts(2 hunks)services/apps/merge_suggestions_worker/src/workflows/generateMemberMergeSuggestions.ts(1 hunks)services/apps/merge_suggestions_worker/src/workflows/generateOrganizationMergeSuggestions.ts(1 hunks)services/apps/merge_suggestions_worker/src/workflows/mergeMembersWithLLM.ts(3 hunks)services/apps/merge_suggestions_worker/src/workflows/mergeOrganizationsWithLLM.ts(3 hunks)services/libs/data-access-layer/src/member_merge/index.ts(1 hunks)services/libs/data-access-layer/src/old/apps/merge_suggestions_worker/memberMergeSuggestions.repo.ts(2 hunks)services/libs/data-access-layer/src/old/apps/merge_suggestions_worker/organizationMergeSuggestions.repo.ts(2 hunks)services/libs/data-access-layer/src/org_merge/index.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: lint-format-services
🔇 Additional comments (14)
services/apps/merge_suggestions_worker/src/workflows/generateMemberMergeSuggestions.ts (1)
20-20: Verify the impact of increased similarity thresholdThe similarity threshold increase from 0.5 to 0.75 will make member merge suggestions more conservative. While this reduces false positives, it might also exclude valid merge candidates.
Let's analyze the potential impact:
✅ Verification successful
Similarity threshold change is consistent with codebase
The increase to 0.75 aligns with the organization merge suggestions threshold and the medium confidence score defined in the similarity calculators. This change maintains consistency across the merge suggestion system.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any hardcoded similarity thresholds across the codebase rg -g '!*.{log,md}' -n '(0\.5|0\.75).*similarity' # Look for related configuration or environment variables rg -g '!*.{log,md}' '(SIMILARITY|CONFIDENCE|THRESHOLD|MERGE_SUGGESTION).*=.*[0-9]' # Check for any tests that might need updating fd -e test.ts -e spec.ts | xargs rg 'similarity.*>.*0\.'Length of output: 1670
services/apps/merge_suggestions_worker/src/activities.ts (1)
15-15: Clean refactoring of function names and exports.The renaming improves clarity and the new exports maintain consistency with the implementation.
Also applies to: 19-19, 26-26, 46-46, 48-48, 52-52
services/apps/merge_suggestions_worker/src/workflows/mergeOrganizationsWithLLM.ts (2)
54-61: Good enhancement of error handling with table-specific removals.The granular approach to removing suggestions from both tables ensures data consistency.
3-3: Good addition of type imports.The explicit import of OrganizationMergeSuggestionTable improves type safety.
services/apps/merge_suggestions_worker/src/workflows/mergeMembersWithLLM.ts (3)
3-3: LGTM!The import of
MemberMergeSuggestionTableenum provides type safety for table names used in the code.
66-73: LGTM!Good error handling improvement. The code now ensures proper cleanup by removing invalid suggestions from both raw and filtered tables.
99-111: LGTM!The code now properly handles LLM's negative verdicts by:
- Logging the decision for debugging
- Removing suggestions from both tables
- Adding the pair to no-merge list to prevent future suggestions
services/libs/data-access-layer/src/old/apps/merge_suggestions_worker/memberMergeSuggestions.repo.ts (1)
Line range hint
287-309: LGTM!The method has been improved with:
- Better naming that reflects its broader usage
- Added flexibility through table parameter
- Safe SQL query construction using parameterized values
- Enhanced error logging with table context
services/apps/merge_suggestions_worker/src/activities/memberMergeSuggestions.ts (2)
2-2: LGTM!Clean import of the required function.
355-364: LGTM!The method has been properly updated to match the repository layer changes.
services/apps/merge_suggestions_worker/src/activities/organizationMergeSuggestions.ts (2)
4-4: LGTM!The new import is properly organized and aligns with the new functionality for handling organizations that should not be merged.
432-441: LGTM!The function rename and signature update improve clarity and provide more flexibility in specifying the deletion table.
services/apps/merge_suggestions_worker/src/workflows/generateOrganizationMergeSuggestions.ts (2)
Line range hint
13-78: Well-structured workflow implementation!The implementation follows good practices:
- Efficient pagination using
continueAsNew- Parallel processing with reasonable chunk sizes
- Proper state management
- Good error handling through activity timeouts
20-20: Verify the impact of increasing the similarity thresholdIncreasing the similarity confidence score threshold from 0.5 to 0.75 will make the merge suggestion filtering more strict. This means fewer suggestions will be added to the
ORGANIZATION_TO_MERGE_FILTEREDtable, potentially reducing false positives but also possibly missing valid merge candidates.Let's analyze the impact of this change:
Consider:
- Have you validated this threshold with real-world data?
- Are there tests that need updating with this new threshold?
- Should this threshold be configurable per tenant?
| export async function addOrganizationSuggestionToNoMerge(suggestion: string[]): Promise<void> { | ||
| if (suggestion.length !== 2) { | ||
| svc.log.debug(`Suggestions array must have two ids!`) | ||
| return | ||
| } | ||
| const qx = pgpQx(svc.postgres.writer.connection()) | ||
|
|
||
| await addOrgNoMerge(qx, suggestion[0], suggestion[1]) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling and logging
The function could be improved in several ways:
- Throw an error instead of silently returning on invalid input
- Add error handling for database operations
- Add logging for successful operations
- Add type validation for array elements
Consider this implementation:
export async function addOrganizationSuggestionToNoMerge(suggestion: string[]): Promise<void> {
if (suggestion.length !== 2) {
- svc.log.debug(`Suggestions array must have two ids!`)
- return
+ throw new Error('Suggestions array must have exactly two organization IDs')
}
+
+ if (!suggestion.every(id => typeof id === 'string' && id.length > 0)) {
+ throw new Error('All organization IDs must be non-empty strings')
+ }
+
const qx = pgpQx(svc.postgres.writer.connection())
- await addOrgNoMerge(qx, suggestion[0], suggestion[1])
+ try {
+ await addOrgNoMerge(qx, suggestion[0], suggestion[1])
+ svc.log.info(
+ { primaryId: suggestion[0], secondaryId: suggestion[1] },
+ 'Successfully added organization pair to no-merge list'
+ )
+ } catch (error) {
+ svc.log.error(
+ { error, primaryId: suggestion[0], secondaryId: suggestion[1] },
+ 'Failed to add organization pair to no-merge list'
+ )
+ throw error
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export async function addOrganizationSuggestionToNoMerge(suggestion: string[]): Promise<void> { | |
| if (suggestion.length !== 2) { | |
| svc.log.debug(`Suggestions array must have two ids!`) | |
| return | |
| } | |
| const qx = pgpQx(svc.postgres.writer.connection()) | |
| await addOrgNoMerge(qx, suggestion[0], suggestion[1]) | |
| export async function addOrganizationSuggestionToNoMerge(suggestion: string[]): Promise<void> { | |
| if (suggestion.length !== 2) { | |
| throw new Error('Suggestions array must have exactly two organization IDs') | |
| } | |
| if (!suggestion.every(id => typeof id === 'string' && id.length > 0)) { | |
| throw new Error('All organization IDs must be non-empty strings') | |
| } | |
| const qx = pgpQx(svc.postgres.writer.connection()) | |
| try { | |
| await addOrgNoMerge(qx, suggestion[0], suggestion[1]) | |
| svc.log.info( | |
| { primaryId: suggestion[0], secondaryId: suggestion[1] }, | |
| 'Successfully added organization pair to no-merge list' | |
| ) | |
| } catch (error) { | |
| svc.log.error( | |
| { error, primaryId: suggestion[0], secondaryId: suggestion[1] }, | |
| 'Failed to add organization pair to no-merge list' | |
| ) | |
| throw error | |
| } | |
| } |
Changes proposed ✍️
What
copilot:summary
copilot:poem
Why
How
copilot:walkthrough
Checklist ✅
Feature,Improvement, orBug.Summary by CodeRabbit
New Features
Improvements
Technical Updates