-
Notifications
You must be signed in to change notification settings - Fork 11
fix: Use left join on resource to make it nullable #232
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughThe changes in this pull request involve modifications to the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ResourceRouter
participant Database
Client->>ResourceRouter: Request to get nodes
ResourceRouter->>Database: Select nodes with where clause
Database-->>ResourceRouter: Return nodes
ResourceRouter-->>Client: Send nodes response
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
Documentation and Community
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/api/src/router/resources.ts (2)
505-507: Fix indentation in the join conditionThe indentation of the join condition parameters appears inconsistent with the surrounding code.
- schema.jobResourceRelationship.resourceIdentifier, - schema.resource.identifier, + schema.jobResourceRelationship.resourceIdentifier, + schema.resource.identifier,
521-530: Consider handling large result setsWhile the parallel processing of child nodes is efficient, consider implementing batch processing for large result sets to prevent potential memory issues.
Consider adding a batch size limit:
+ const BATCH_SIZE = 50; const childrenPromises = relationships.map((r) => getNodeDataForResource(db, r.resource.id), ); - const children = await Promise.all(childrenPromises); + const children = []; + for (let i = 0; i < childrenPromises.length; i += BATCH_SIZE) { + const batch = await Promise.all( + childrenPromises.slice(i, i + BATCH_SIZE) + ); + children.push(...batch); + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
packages/api/src/router/resources.ts(1 hunks)
🔇 Additional comments (2)
packages/api/src/router/resources.ts (2)
493-498: LGTM: Clean and efficient job extraction logic
The base case handling and job extraction logic is well-structured, using appropriate array operations for flattening nested data.
499-518: LGTM: Correct implementation of nullable resource relationships
The left join implementation correctly handles nullable resources, aligning with the PR objective. The subsequent filtering ensures type safety.
Summary by CodeRabbit
Bug Fixes
Refactor