🚨 Lighthouse: Optimize multiple COUNT queries#228
🚨 Lighthouse: Optimize multiple COUNT queries#228ldsgroups225 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. |
📝 WalkthroughWalkthroughMultiple analytics and dashboard queries are refactored to consolidate parallel Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 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.
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 `@packages/data-ops/src/queries/analytics.ts`:
- Around line 351-357: The students aggregates (db.select
{...}.from(students)... using count()/sql and the similar block at lines
417-420) currently omit tenant scoping and can aggregate across schools; require
a schoolId filter by ensuring the query includes where(eq(students.schoolId,
schoolId)) (or make schoolId a required parameter) and add this predicate into
baseConditions (or the query's where clause) so every execution against the
students table is scoped to the given schoolId; alternatively, separate this
into an explicit admin/global-only query with proper authorization checks if
global totals are needed.
- Around line 47-51: The db.select aggregation on school-scoped queries is
missing mandatory tenant filtering and the helpers bypass the ResultAsync
error-contract; update the school/scoped queries (the db.select that uses total:
count() and currentPeriod/previousPeriod sql counts and any functions
referencing baseConditions) to always include where(eq(students.schoolId,
schoolId)) instead of optional baseConditions (make schoolId non-optional or
return an error when missing), and refactor the four helper functions
(getEnrollmentStats, getEnrollmentGrowth and the two other student-query helpers
mentioned) to return ResultAsync<T, DatabaseError> by wrapping their async DB
calls and appending .mapErr(tapLogErr(logger, context)) so database errors are
captured and logged rather than thrown.
In `@packages/data-ops/src/queries/dashboard-stats.ts`:
- Around line 19-24: The getDashboardStats function currently returns a
Promise<DashboardStats> from the db.select call; change it to return
ResultAsync<DashboardStats, DatabaseError> by wrapping the database query in
ResultAsync.fromPromise (or equivalent from `@praha/byethrow`) and then attach
.mapErr(tapLogErr(logger, context)) to the resulting ResultAsync; specifically
wrap the db.select(...).from(schools) call that builds
total/active/recent/inactive (references: db.select, schools, thirtyDaysAgo,
DashboardStats) so all DB errors are mapped to DatabaseError and logged via
tapLogErr(logger, context) before returning the ResultAsync from
getDashboardStats.
🪄 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: f2dcd79b-767e-4c52-9143-799eedc20772
📒 Files selected for processing (3)
packages/data-ops/src/queries/analytics.tspackages/data-ops/src/queries/dashboard-stats.tspr-body.txt
| db.select({ | ||
| total: count(), | ||
| currentPeriod: sql<number>`count(*) filter (where ${schools.createdAt} >= ${startDate})`, | ||
| previousPeriod: sql<number>`count(*) filter (where ${schools.createdAt} >= ${previousStartDate} and ${schools.createdAt} < ${startDate})`, | ||
| }).from(schools), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n packages/data-ops/src/queries/analytics.ts | head -60Repository: ldsgroups225/Yeko
Length of output: 2498
🏁 Script executed:
cat -n packages/data-ops/src/queries/analytics.ts | sed -n '40,55p'Repository: ldsgroups225/Yeko
Length of output: 842
🏁 Script executed:
cat -n packages/data-ops/src/queries/analytics.ts | sed -n '115,135p'Repository: ldsgroups225/Yeko
Length of output: 969
🏁 Script executed:
cat -n packages/data-ops/src/queries/analytics.ts | sed -n '345,365p'Repository: ldsgroups225/Yeko
Length of output: 1130
🏁 Script executed:
cat -n packages/data-ops/src/queries/analytics.ts | sed -n '410,425p'Repository: ldsgroups225/Yeko
Length of output: 1095
🏁 Script executed:
cat -n packages/data-ops/src/queries/analytics.ts | sed -n '90,105p'Repository: ldsgroups225/Yeko
Length of output: 476
🏁 Script executed:
cat -n packages/data-ops/src/queries/analytics.ts | sed -n '300,320p'Repository: ldsgroups225/Yeko
Length of output: 838
🏁 Script executed:
cat -n packages/data-ops/src/queries/analytics.ts | sed -n '330,345p'Repository: ldsgroups225/Yeko
Length of output: 520
🏁 Script executed:
cat -n packages/data-ops/src/queries/analytics.ts | sed -n '395,415p'Repository: ldsgroups225/Yeko
Length of output: 862
🏁 Script executed:
cat -n packages/data-ops/src/queries/analytics.ts | sed -n '108,115p'Repository: ldsgroups225/Yeko
Length of output: 457
🏁 Script executed:
rg "export async function getSchoolsPerformance" packages/data-ops/src/queries/analytics.ts -A 3Repository: ldsgroups225/Yeko
Length of output: 238
Add mandatory tenant filtering for school-scoped tables and keep query helpers on the ResultAsync path.
The functions at lines 337-359 and 400-420 query the students table (school-scoped) with optional schoolId parameters. When schoolId is undefined, baseConditions becomes an empty array, causing .where() to evaluate to undefined — the query then executes without any tenant filtering, exposing cross-tenant data.
Additionally, all four touched query helpers run inside raw async functions and bypass the required ResultAsync<T, DatabaseError> + tapLogErr(...) contract. Database errors can throw uncaught.
Changes required:
- Add mandatory
where(eq(students.schoolId, schoolId))forgetEnrollmentStatsandgetEnrollmentGrowth— remove the optional check; ifschoolIdis required, make it non-optional; if optional, you must still filter or return an error. - Wrap all four functions to return
ResultAsync<T, DatabaseError>with.mapErr(tapLogErr(logger, context))on the result chain.
Applies to lines 47–51, 124–128, 351–357, 417–420 and their parent functions.
🤖 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 47 - 51, The
db.select aggregation on school-scoped queries is missing mandatory tenant
filtering and the helpers bypass the ResultAsync error-contract; update the
school/scoped queries (the db.select that uses total: count() and
currentPeriod/previousPeriod sql counts and any functions referencing
baseConditions) to always include where(eq(students.schoolId, schoolId)) instead
of optional baseConditions (make schoolId non-optional or return an error when
missing), and refactor the four helper functions (getEnrollmentStats,
getEnrollmentGrowth and the two other student-query helpers mentioned) to return
ResultAsync<T, DatabaseError> by wrapping their async DB calls and appending
.mapErr(tapLogErr(logger, context)) so database errors are captured and logged
rather than thrown.
| db.select({ | ||
| total: count(), | ||
| active: sql<number>`count(*) filter (where ${students.status} = 'active')`, | ||
| graduated: sql<number>`count(*) filter (where ${students.status} = 'graduated')`, | ||
| transferred: sql<number>`count(*) filter (where ${students.status} = 'transferred')`, | ||
| withdrawn: sql<number>`count(*) filter (where ${students.status} = 'withdrawn')`, | ||
| }).from(students).where(baseConditions.length > 0 ? and(...baseConditions) : undefined), |
There was a problem hiding this comment.
Require schoolId scoping on these students aggregates.
students is a school-scoped table, but both queries drop the tenant filter when schoolId is absent. That leaves a cross-tenant aggregation path for enrollment totals and growth. Make schoolId mandatory here, or split the global/admin case into a separately authorized query.
Based on learnings: Every query on school-scoped tables MUST include where(eq(table.schoolId, schoolId)) for multi-tenant isolation.
Also applies to: 417-420
🤖 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 - 357, The
students aggregates (db.select {...}.from(students)... using count()/sql and the
similar block at lines 417-420) currently omit tenant scoping and can aggregate
across schools; require a schoolId filter by ensuring the query includes
where(eq(students.schoolId, schoolId)) (or make schoolId a required parameter)
and add this predicate into baseConditions (or the query's where clause) so
every execution against the students table is scoped to the given schoolId;
alternatively, separate this into an explicit admin/global-only query with
proper authorization checks if global totals are needed.
| db.select({ | ||
| total: count(), | ||
| active: sql<number>`count(*) filter (where ${schools.status} = 'active')`, | ||
| recent: sql<number>`count(*) filter (where ${schools.createdAt} >= ${thirtyDaysAgo})`, | ||
| inactive: sql<number>`count(*) filter (where ${schools.status} = 'inactive')`, | ||
| }).from(schools), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat packages/data-ops/src/queries/dashboard-stats.tsRepository: ldsgroups225/Yeko
Length of output: 8691
Return ResultAsync with tapLogErr error logging from getDashboardStats function.
The function returns Promise<DashboardStats> directly, but per the coding guidelines for packages/data-ops/src/queries/**/*.ts, it must return ResultAsync<T, DatabaseError> from @praha/byethrow and attach .mapErr(tapLogErr(logger, context)) to all database operations. Currently, any database error thrown by the queries at lines 19-24 will propagate uncaught instead of following the structured error-handling pattern.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/data-ops/src/queries/dashboard-stats.ts` around lines 19 - 24, The
getDashboardStats function currently returns a Promise<DashboardStats> from the
db.select call; change it to return ResultAsync<DashboardStats, DatabaseError>
by wrapping the database query in ResultAsync.fromPromise (or equivalent from
`@praha/byethrow`) and then attach .mapErr(tapLogErr(logger, context)) to the
resulting ResultAsync; specifically wrap the db.select(...).from(schools) call
that builds total/active/recent/inactive (references: db.select, schools,
thirtyDaysAgo, DashboardStats) so all DB errors are mapped to DatabaseError and
logged via tapLogErr(logger, context) before returning the ResultAsync from
getDashboardStats.
1 participant
Impact on performance:
Significantly reduces database load and latency for the dashboard and analytics pages by combining multiple
COUNT(*)queries on the same table (such asschoolsandstudents) into a single database query. This eliminates multiple parallel table scans and DB connection overhead, shifting the conditional logic to Postgres.Technical rationale:
Previously, the code executed several independent
db.select({ count: count() })queries with differentWHEREclauses for different statuses or date ranges usingPromise.all(). Now, it uses a singledb.select()with multiplesql<number>\count(*) filter (where ...)`columns, leveraging PostgreSQL'sFILTER` clause. This represents a substantial query optimization by converting N queries into 1.PR created automatically by Jules for task 8444507562885530474 started by @ldsgroups225
Summary by CodeRabbit
Refactor