🚨 Lighthouse: [Fix N+1 bottleneck in report card validation]#254
🚨 Lighthouse: [Fix N+1 bottleneck in report card validation]#254ldsgroups225 wants to merge 1 commit intomasterfrom
Conversation
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughThe validation logic in Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/data-ops/src/queries/report-cards.ts (1)
430-437: Consider filtering by active status for classes and school years in batch validation.The batch queries in
validateReportCardPayloadsdo not filter byclasses.status = 'active'orschoolYears.isActive = true, while thelistNightlyReportCardDiscoveryUnitsfunction applies these filters elsewhere in the file (lines 922, 1009). This could allow report cards to be validated/created for archived classes or inactive school years, creating an inconsistency in how status is enforced across report card operations.The refactored code correctly preserves the original behavior of
getScopedClassandgetScopedSchoolYear(which also lack status filters), but consider adding status filters in a follow-up PR to align with the filtering patterns used in discovery operations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/data-ops/src/queries/report-cards.ts` around lines 430 - 437, The batch validation queries inside validateReportCardPayloads currently select classes, terms, and schoolYears without filtering on status; update the three db.select calls that query classes, terms (which already innerJoins schoolYears), and schoolYears so they also include filters equivalent to classes.status = 'active' and schoolYears.isActive = true (mirror the filtering used in listNightlyReportCardDiscoveryUnits) to prevent validating report cards for archived classes or inactive school years; ensure the classes query adds the classes.status filter, the terms query keeps its innerJoin(schoolYears) and adds schoolYears.isActive = true, and the schoolYears query adds schoolYears.isActive = true.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/data-ops/src/queries/report-cards.ts`:
- Around line 430-437: The batch validation queries inside
validateReportCardPayloads currently select classes, terms, and schoolYears
without filtering on status; update the three db.select calls that query
classes, terms (which already innerJoins schoolYears), and schoolYears so they
also include filters equivalent to classes.status = 'active' and
schoolYears.isActive = true (mirror the filtering used in
listNightlyReportCardDiscoveryUnits) to prevent validating report cards for
archived classes or inactive school years; ensure the classes query adds the
classes.status filter, the terms query keeps its innerJoin(schoolYears) and adds
schoolYears.isActive = true, and the schoolYears query adds schoolYears.isActive
= true.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2655446b-d732-492f-84c4-1f90a79d0c64
📒 Files selected for processing (1)
packages/data-ops/src/queries/report-cards.ts
1 participant
Impact on performance
This PR eliminates a severe N+1 query problem during report card batch validation. Previously,
validateReportCardPayloadsiterated over arrays of mapped unique IDs and awaited a database hit per student, class, term, etc. This has been replaced with a single bulk fetch using Drizzle'sinArray(), which is correctly mapped back to memory structures.Technical Rationale
inArrayfor better efficiency and Cloudflare connection pooling usage.getScoped*error handling and uniqueness checking.PR created automatically by Jules for task 10152485457501312247 started by @ldsgroups225
Summary by CodeRabbit