fix mergeWithLLM workflow for members and organizations when it fails find data
#2721
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 WalkthroughThe changes introduce two new functions, Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 2
🧹 Outside diff range and nitpick comments (4)
services/apps/merge_suggestions_worker/src/workflows/mergeOrganizationsWithLLM.ts (1)
52-56: Consider enhancing error logging and monitoringWhile the error handling is good, consider adding structured logging with more context and potentially tracking these cleanup operations for monitoring purposes.
- console.log( - `Failed getting organization data in suggestion. Skipping suggestion: ${suggestion}`, - ) - await organizationActivitiesProxy.removeRawOrganizationMergeSuggestions(suggestion) + console.log({ + level: 'warn', + message: 'Failed getting organization data in suggestion', + suggestion, + organizationsFound: organizations.length, + action: 'removing_suggestion' + }) + await organizationActivitiesProxy.removeRawOrganizationMergeSuggestions(suggestion) + // Consider adding metrics/monitoring hereservices/apps/merge_suggestions_worker/src/workflows/mergeMembersWithLLM.ts (1)
64-68: Consider enhancing error logging and monitoringSimilar to the organization workflow, consider adding structured logging with more context and tracking these cleanup operations.
- console.log(`Failed getting members data in suggestion. Skipping suggestion: ${suggestion}`) - await memberActivitiesProxy.removeRawMemberMergeSuggestions(suggestion) + console.log({ + level: 'warn', + message: 'Failed getting members data in suggestion', + suggestion, + membersFound: members.length, + action: 'removing_suggestion' + }) + await memberActivitiesProxy.removeRawMemberMergeSuggestions(suggestion) + // Consider adding metrics/monitoring hereservices/apps/merge_suggestions_worker/src/activities/memberMergeSuggestions.ts (1)
354-360: Add JSDoc documentation and consistent error handlingThe new function should follow the documentation and error handling patterns established in the file.
+/** + * Removes a raw member merge suggestion from the database + * @param suggestion Array containing two member IDs to remove from merge suggestions + * @throws Error if the suggestion cannot be removed + */ export async function removeRawMemberMergeSuggestions(suggestion: string[]): Promise<void> { const memberMergeSuggestionsRepo = new MemberMergeSuggestionsRepository( svc.postgres.writer.connection(), svc.log, ) - await memberMergeSuggestionsRepo.removeRawMemberSuggestions(suggestion) + try { + await memberMergeSuggestionsRepo.removeRawMemberSuggestions(suggestion) + } catch (err) { + throw new Error(err) + } }services/apps/merge_suggestions_worker/src/activities/organizationMergeSuggestions.ts (1)
431-437: Consider adding input validation and improving parameter naming.While the implementation is functionally correct, consider these improvements:
- Validate the input array is not empty
- Rename parameter from
suggestiontoorganizationIdsfor clarity- Add JSDoc documentation describing the purpose and parameters
Here's a suggested improvement:
+/** + * Removes raw organization merge suggestions for the specified organizations + * @param organizationIds - Array of organization IDs to remove from merge suggestions + * @throws {Error} If the removal operation fails + */ -export async function removeRawOrganizationMergeSuggestions(suggestion: string[]): Promise<void> { +export async function removeRawOrganizationMergeSuggestions(organizationIds: string[]): Promise<void> { + if (!organizationIds?.length) { + throw new Error('Organization IDs array cannot be empty'); + } const organizationMergeSuggestionsRepo = new OrganizationMergeSuggestionsRepository( svc.postgres.writer.connection(), svc.log, ) - await organizationMergeSuggestionsRepo.removeRawOrganizationMergeSuggestions(suggestion) + await organizationMergeSuggestionsRepo.removeRawOrganizationMergeSuggestions(organizationIds) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
services/apps/merge_suggestions_worker/src/activities.ts(3 hunks)services/apps/merge_suggestions_worker/src/activities/memberMergeSuggestions.ts(1 hunks)services/apps/merge_suggestions_worker/src/activities/organizationMergeSuggestions.ts(1 hunks)services/apps/merge_suggestions_worker/src/workflows/mergeMembersWithLLM.ts(1 hunks)services/apps/merge_suggestions_worker/src/workflows/mergeOrganizationsWithLLM.ts(1 hunks)services/libs/data-access-layer/src/old/apps/merge_suggestions_worker/memberMergeSuggestions.repo.ts(1 hunks)services/libs/data-access-layer/src/old/apps/merge_suggestions_worker/organizationMergeSuggestions.repo.ts(1 hunks)
🔇 Additional comments (6)
services/apps/merge_suggestions_worker/src/activities.ts (1)
15-15: LGTM! Clean addition of new activity functions
The new functions removeRawMemberMergeSuggestions and removeRawOrganizationMergeSuggestions are properly imported and exported.
Also applies to: 25-25, 45-48
services/apps/merge_suggestions_worker/src/workflows/mergeOrganizationsWithLLM.ts (1)
54-54: LGTM! Good error handling improvement
The addition of removeRawOrganizationMergeSuggestions ensures invalid suggestions are cleaned up when organization data cannot be retrieved.
services/apps/merge_suggestions_worker/src/workflows/mergeMembersWithLLM.ts (2)
66-66: LGTM! Consistent error handling with organization workflow
The addition of removeRawMemberMergeSuggestions maintains consistency with the organization workflow for cleaning up invalid suggestions.
Line range hint 23-23: Verify the difference in batch sizes between workflows
The member workflow processes 10 suggestions per run while the organization workflow processes 5. Is this difference intentional?
Also applies to: 32-32
✅ Verification successful
The difference in batch sizes appears intentional due to the nature of the entities being processed
The batch size difference between member suggestions (10) and organization suggestions (5) is intentional and reasonable because:
- Organization merges are typically more complex operations that may involve more data and relationships
- Members are generally simpler entities with fewer dependencies
- The codebase consistently shows different batch sizes for organization vs member operations across various services (e.g., in sync services, cleanup operations)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any configuration or documentation that might explain the batch size differences
rg -i "suggestions.*per.*run|batch.*size" --type ts
Length of output: 9199
services/apps/merge_suggestions_worker/src/activities/organizationMergeSuggestions.ts (2)
431-437: LGTM! The function integrates well with the existing workflow.
The new function follows established patterns in the codebase and properly complements the existing getRawOrganizationMergeSuggestions functionality. It correctly uses the writer connection for mutations and maintains consistency with error handling patterns.
431-437: Verify the usage of this new function.
Let's confirm how this function is being used in the mergeWithLLM workflow.
✅ Verification successful
Based on the shell script results, I can now generate the final response:
Function is properly used in the mergeWithLLM workflow
The removeRawOrganizationMergeSuggestions function is correctly integrated and used in the mergeOrganizationsWithLLM.ts workflow. It's specifically used to clean up invalid suggestions where the number of organizations doesn't match the expected count (2), preventing processing of malformed data.
- Used in
services/apps/merge_suggestions_worker/src/workflows/mergeOrganizationsWithLLM.ts - Properly exported in
activities.ts - Implementation matches the repository layer in
organizationMergeSuggestions.repo.ts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find where removeRawOrganizationMergeSuggestions is being used
# Search for direct usage of the function
echo "Searching for direct usage of removeRawOrganizationMergeSuggestions..."
rg -l "removeRawOrganizationMergeSuggestions"
# Search for the workflow file mentioned in the PR
echo "Searching for the mergeWithLLM workflow file..."
fd -t f "mergeOrganizationsWithLLM"
# If found, show the context of usage
echo "Showing usage context if found..."
rg -A 5 -B 5 "removeRawOrganizationMergeSuggestions"
Length of output: 6985
.../libs/data-access-layer/src/old/apps/merge_suggestions_worker/memberMergeSuggestions.repo.ts
Show resolved
Hide resolved
...data-access-layer/src/old/apps/merge_suggestions_worker/organizationMergeSuggestions.repo.ts
Show resolved
Hide resolved
… to find data
Changes proposed ✍️
What
copilot:summary
copilot:poem
Why
How
copilot:walkthrough
Checklist ✅
Feature,Improvement, orBug.Summary by CodeRabbit
New Features
Bug Fixes
Documentation