⚡ Bolt: Combine multiple student count queries#222
⚡ Bolt: Combine multiple student count queries#222ldsgroups225 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 PR documents and implements a database query optimization that consolidates five parallel Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.jules/bolt.md:
- Line 17: Update the phrasing in the sentence that describes the 5 concurrent
`COUNT` queries run via `Promise.all` to use "multiple scans" instead of "N+1
like" (or if you prefer to keep the N+1 phrasing, hyphenate it as "N+1-like");
locate the sentence mentioning "5 concurrent `COUNT` queries on the same table
with different conditions via `Promise.all`" and replace the offending phrase
accordingly.
In `@packages/data-ops/src/queries/analytics.ts`:
- Around line 351-363: The students aggregate query (db.select producing
statsResult and calling getEnrollmentTrends) is missing tenant scoping and can
count across all schools; require or apply school scoping by adding a where
clause that enforces eq(students.schoolId, schoolId) (combined with existing
baseConditions/and(...baseConditions)) or make schoolId a required parameter for
this function and ensure you include where(eq(students.schoolId, schoolId))
before executing db.select so the counts are limited to the intended school; if
you need a cross-school admin view, split this into a separate query with
explicit intent.
- Around line 352-363: Wrap both getEnrollmentStats and getEnrollmentTrends to
return ResultAsync by importing Result as R from `@praha/byethrow` and tapLogErr
from `@repo/logger` and using the project pattern: change function return types to
R.ResultAsync<..., DatabaseError>, wrap the DB Promise.all block and the
getEnrollmentTrends call in R.pipe(...) (or R.fromPromise equivalent used
elsewhere), map successful results with .map(...) and convert errors with
.mapError(tapLogErr('getEnrollmentStats') / tapLogErr('getEnrollmentTrends')) so
errors are logged and the functions return ResultAsync instead of raw Promise;
ensure you reference the existing symbols students, baseConditions, timeRange,
schoolId and the helper getEnrollmentTrends inside the wrapper.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 32c24062-1f03-4701-9b90-f7f99b526594
📒 Files selected for processing (2)
.jules/bolt.mdpackages/data-ops/src/queries/analytics.ts
| **Learning:** Sequential `for...of` loops that `await` independent database queries (like iterating over active school years to fetch report card discovery units) create severe N+1 bottlenecks. | ||
| **Action:** Always replace sequential `await` loops for read-only queries with `Promise.all()` mapped over the array to allow the database driver to process the independent queries concurrently, effectively eliminating the N+1 I/O wait. | ||
| ## 2026-03-30 - [Combine COUNT queries with FILTER] | ||
| **Learning:** Found a pattern of 5 concurrent `COUNT` queries on the same table with different conditions via `Promise.all`. While executed concurrently, this still causes N+1 like query behaviour for database reads, forcing 5 separate scans. |
There was a problem hiding this comment.
Prefer “multiple scans” over “N+1-like” here.
This is a fixed five-query pattern rather than a true N+1, and if you keep the phrase it should be hyphenated (N+1-like).
🧰 Tools
🪛 LanguageTool
[grammar] ~17-~17: Use a hyphen to join words.
Context: ...uted concurrently, this still causes N+1 like query behaviour for database reads,...
(QB_NEW_EN_HYPHEN)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.jules/bolt.md at line 17, Update the phrasing in the sentence that
describes the 5 concurrent `COUNT` queries run via `Promise.all` to use
"multiple scans" instead of "N+1 like" (or if you prefer to keep the N+1
phrasing, hyphenate it as "N+1-like"); locate the sentence mentioning "5
concurrent `COUNT` queries on the same table with different conditions via
`Promise.all`" and replace the offending phrase accordingly.
| // ⚡ Bolt Optimization: Combined 5 separate count queries into a single query using filter conditions | ||
| const [ | ||
| [totalResult], | ||
| [activeResult], | ||
| [graduatedResult], | ||
| [transferredResult], | ||
| [withdrawnResult], | ||
| [statsResult], | ||
| trends, | ||
| ] = await Promise.all([ | ||
| db.select({ count: count() }).from(students).where(baseConditions.length > 0 ? and(...baseConditions) : undefined), | ||
| db.select({ count: count() }).from(students).where(and(eq(students.status, 'active'), ...baseConditions)), | ||
| db.select({ count: count() }).from(students).where(and(eq(students.status, 'graduated'), ...baseConditions)), | ||
| db.select({ count: count() }).from(students).where(and(eq(students.status, 'transferred'), ...baseConditions)), | ||
| db.select({ count: count() }).from(students).where(and(eq(students.status, 'withdrawn'), ...baseConditions)), | ||
| db.select({ | ||
| total: sql<number>`count(*)::int`, | ||
| active: sql<number>`count(*) filter (where ${students.status} = 'active')::int`, | ||
| graduated: sql<number>`count(*) filter (where ${students.status} = 'graduated')::int`, | ||
| transferred: sql<number>`count(*) filter (where ${students.status} = 'transferred')::int`, | ||
| withdrawn: sql<number>`count(*) filter (where ${students.status} = 'withdrawn')::int`, | ||
| }).from(students).where(baseConditions.length > 0 ? and(...baseConditions) : undefined), | ||
| getEnrollmentTrends(timeRange, schoolId), |
There was a problem hiding this comment.
Require schoolId for this students aggregate.
When schoolId is absent, this new aggregate still falls back to counting students across every tenant. Make schoolId mandatory here, or split the cross-school admin view into a separate query. Based on learnings: Applies to packages/data-ops/src/queries/**/*.ts: Every query on school-scoped tables MUST include where(eq(table.schoolId, schoolId)) for multi-tenant isolation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/data-ops/src/queries/analytics.ts` around lines 351 - 363, The
students aggregate query (db.select producing statsResult and calling
getEnrollmentTrends) is missing tenant scoping and can count across all schools;
require or apply school scoping by adding a where clause that enforces
eq(students.schoolId, schoolId) (combined with existing
baseConditions/and(...baseConditions)) or make schoolId a required parameter for
this function and ensure you include where(eq(students.schoolId, schoolId))
before executing db.select so the counts are limited to the intended school; if
you need a cross-school admin view, split this into a separate query with
explicit intent.
| const [ | ||
| [totalResult], | ||
| [activeResult], | ||
| [graduatedResult], | ||
| [transferredResult], | ||
| [withdrawnResult], | ||
| [statsResult], | ||
| trends, | ||
| ] = await Promise.all([ | ||
| db.select({ count: count() }).from(students).where(baseConditions.length > 0 ? and(...baseConditions) : undefined), | ||
| db.select({ count: count() }).from(students).where(and(eq(students.status, 'active'), ...baseConditions)), | ||
| db.select({ count: count() }).from(students).where(and(eq(students.status, 'graduated'), ...baseConditions)), | ||
| db.select({ count: count() }).from(students).where(and(eq(students.status, 'transferred'), ...baseConditions)), | ||
| db.select({ count: count() }).from(students).where(and(eq(students.status, 'withdrawn'), ...baseConditions)), | ||
| db.select({ | ||
| total: sql<number>`count(*)::int`, | ||
| active: sql<number>`count(*) filter (where ${students.status} = 'active')::int`, | ||
| graduated: sql<number>`count(*) filter (where ${students.status} = 'graduated')::int`, | ||
| transferred: sql<number>`count(*) filter (where ${students.status} = 'transferred')::int`, | ||
| withdrawn: sql<number>`count(*) filter (where ${students.status} = 'withdrawn')::int`, | ||
| }).from(students).where(baseConditions.length > 0 ? and(...baseConditions) : undefined), | ||
| getEnrollmentTrends(timeRange, schoolId), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Compare analytics.ts with the standard data-ops query pattern.
echo "Expected pattern in query files:"
rg -n -C2 '@praha/byethrow|ResultAsync|tapLogErr' packages/data-ops/src/queries
echo
echo "Current analytics.ts implementation:"
rg -n -C2 'getEnrollmentStats|Promise<EnrollmentStats>|Promise\.all|db\.select' packages/data-ops/src/queries/analytics.tsRepository: ldsgroups225/Yeko
Length of output: 50374
🏁 Script executed:
# Get the specific lines and context around 352-363 in analytics.ts
wc -l packages/data-ops/src/queries/analytics.tsRepository: ldsgroups225/Yeko
Length of output: 107
🏁 Script executed:
# Extract lines 340-380 to see full context of the Promise.all block and surrounding code
sed -n '340,380p' packages/data-ops/src/queries/analytics.tsRepository: ldsgroups225/Yeko
Length of output: 1800
🏁 Script executed:
# Also check the function signature and return type of getEnrollmentStats
rg -n -B5 -A2 'getEnrollmentStats|async function.*EnrollmentStats' packages/data-ops/src/queries/analytics.ts | head -50Repository: ldsgroups225/Yeko
Length of output: 259
🏁 Script executed:
# Check if getEnrollmentTrends returns ResultAsync
rg -n -B2 'getEnrollmentTrends|function.*getEnrollmentTrends' packages/data-ops/src/queries/analytics.tsRepository: ldsgroups225/Yeko
Length of output: 546
🏁 Script executed:
# Check imports at the top of analytics.ts to see if Result/ResultAsync is being used
head -20 packages/data-ops/src/queries/analytics.tsRepository: ldsgroups225/Yeko
Length of output: 637
🏁 Script executed:
# Check the full getEnrollmentTrends function signature and implementation
sed -n '304,340p' packages/data-ops/src/queries/analytics.tsRepository: ldsgroups225/Yeko
Length of output: 1073
🏁 Script executed:
# Check if there are any Result/ResultAsync imports in analytics.ts
rg '@praha/byethrow|ResultAsync|tapLogErr|Result as R' packages/data-ops/src/queries/analytics.tsRepository: ldsgroups225/Yeko
Length of output: 43
🏁 Script executed:
# Get the complete return statement of getEnrollmentStats to see the full picture
sed -n '342,390p' packages/data-ops/src/queries/analytics.tsRepository: ldsgroups225/Yeko
Length of output: 2038
Wrap both getEnrollmentStats and getEnrollmentTrends functions with ResultAsync and proper error logging.
Both functions currently return raw Promise types without error handling. Per coding guidelines, every query in packages/data-ops/src/queries/**/*.ts must return ResultAsync<T, DatabaseError> with tapLogErr for error logging.
Currently:
getEnrollmentStatsreturnsPromise<EnrollmentStats>and usesPromise.allwith unhandled DB operationsgetEnrollmentTrendsreturnsPromise<EnrollmentTrend[]>without error wrapping
Both need to follow the pattern used in all other query files (e.g., student-fees.ts, transactions.ts):
- Import
Result as Rfrom@praha/byethrowandtapLogErrfrom@repo/logger - Change return types to
R.ResultAsync<T, DatabaseError> - Wrap queries in
R.pipe()with.catch()and.mapError(tapLogErr(...))
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/data-ops/src/queries/analytics.ts` around lines 352 - 363, Wrap both
getEnrollmentStats and getEnrollmentTrends to return ResultAsync by importing
Result as R from `@praha/byethrow` and tapLogErr from `@repo/logger` and using the
project pattern: change function return types to R.ResultAsync<...,
DatabaseError>, wrap the DB Promise.all block and the getEnrollmentTrends call
in R.pipe(...) (or R.fromPromise equivalent used elsewhere), map successful
results with .map(...) and convert errors with
.mapError(tapLogErr('getEnrollmentStats') / tapLogErr('getEnrollmentTrends')) so
errors are logged and the functions return ResultAsync instead of raw Promise;
ensure you reference the existing symbols students, baseConditions, timeRange,
schoolId and the helper getEnrollmentTrends inside the wrapper.
1 participant
💡 What: Combined 5 separate count queries into a single query using filter conditions.
🎯 Why: Replaced multiple database queries with a single query for better performance.
📊 Impact: Decreases database load and network latency by fetching aggregated counts in a single query.
🔬 Measurement: Run
pnpm testin@repo/data-opsto verify functionality.PR created automatically by Jules for task 16570309501137354436 started by @ldsgroups225
Summary by CodeRabbit