🚨 Lighthouse: [Performance] Consolidate multiple COUNT queries using FILTER#223
🚨 Lighthouse: [Performance] Consolidate multiple COUNT queries using FILTER#223ldsgroups225 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. |
📝 WalkthroughWalkthroughMultiple analytics query functions have been refactored to consolidate separate count queries into single aggregated SELECT statements with filtered count expressions, reducing database calls across Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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: 2
🤖 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 current db.select on students builds baseConditions
but leaves out a required tenant filter, allowing cross-tenant aggregation via
the students fallback; update the query that builds the select (the
db.select(...) that uses total/active/graduated/transferred/withdrawn and
baseConditions) to always include eq(students.schoolId, schoolId) in the WHERE
clause (i.e., push sql/and(eq(students.schoolId, schoolId)) into baseConditions
or combine it with and(...baseConditions)) rather than conditionally omitting it
when schoolId is undefined; if platform-wide (admin) analytics are required,
extract that into a separate explicit admin-only query instead of making
schoolId optional in this function.
- Around line 47-51: The query functions in analytics.ts (e.g., the
db.select(...) that computes total, currentPeriod, previousPeriod) must stop
returning raw Promise results and instead return ResultAsync<T, DatabaseError>
from `@praha/byethrow`; wrap the db call with ResultAsync.fromPromise(...) (or
ResultAsync) and append .mapErr(tapLogErr(logger, 'analytics.<functionName>'))
so errors are converted to DatabaseError and logged consistently; follow the
same pattern used in academic-intentions.ts/discounts.ts/payments.ts and apply
this change to the other query blocks mentioned (lines ~124–128, ~351–357,
~422–425), referencing tapLogErr, ResultAsync, and DatabaseError to locate the
utilities to import and use.
🪄 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: 15ed7888-1631-4f92-80cc-345a1fcdd71b
📒 Files selected for processing (1)
packages/data-ops/src/queries/analytics.ts
| 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.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Promise-returning exports in analytics.ts =="
rg -n --type=ts 'export async function (getAnalyticsOverview|getSchoolsPerformance|getEnrollmentStats|getEnrollmentGrowth)\b|Promise<' packages/data-ops/src/queries/analytics.ts
echo
echo "== ResultAsync / tapLogErr usage elsewhere in data-ops =="
rg -n -C2 --type=ts 'ResultAsync|tapLogErr' packages/data-ops/src -g '!packages/data-ops/src/queries/analytics.ts'Repository: ldsgroups225/Yeko
Length of output: 50374
Wrap analytics.ts query functions in the ResultAsync<T, DatabaseError> pattern with tapLogErr error logging.
All query functions in this file currently return bare Promise<...> types and use raw await db.select(...) chains. Per the package contract, every query function must return ResultAsync<T, DatabaseError> from @praha/byethrow and attach .mapErr(tapLogErr(logger, context)) for consistent error handling and auditability. See academic-intentions.ts, discounts.ts, payments.ts, and other files in packages/data-ops/src/queries/ for the required pattern.
Also applies to: 124–128, 351–357, 422–425
🤖 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 query
functions in analytics.ts (e.g., the db.select(...) that computes total,
currentPeriod, previousPeriod) must stop returning raw Promise results and
instead return ResultAsync<T, DatabaseError> from `@praha/byethrow`; wrap the db
call with ResultAsync.fromPromise(...) (or ResultAsync) and append
.mapErr(tapLogErr(logger, 'analytics.<functionName>')) so errors are converted
to DatabaseError and logged consistently; follow the same pattern used in
academic-intentions.ts/discounts.ts/payments.ts and apply this change to the
other query blocks mentioned (lines ~124–128, ~351–357, ~422–425), referencing
tapLogErr, ResultAsync, and DatabaseError to locate the utilities to import and
use.
| 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.
Don’t keep an unscoped students fallback here.
Line 357 and Line 425 still omit the students.schoolId predicate when schoolId is undefined, which preserves a cross-tenant aggregation path on a school-scoped table. If platform-wide analytics is intentional, please move that into an explicit admin-only query instead of hiding the tenant bypass behind an optional parameter.
Based on learnings, "Every query on school-scoped tables MUST include where(eq(table.schoolId, schoolId)) for multi-tenant isolation."
Also applies to: 422-425
🤖 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
current db.select on students builds baseConditions but leaves out a required
tenant filter, allowing cross-tenant aggregation via the students fallback;
update the query that builds the select (the db.select(...) that uses
total/active/graduated/transferred/withdrawn and baseConditions) to always
include eq(students.schoolId, schoolId) in the WHERE clause (i.e., push
sql/and(eq(students.schoolId, schoolId)) into baseConditions or combine it with
and(...baseConditions)) rather than conditionally omitting it when schoolId is
undefined; if platform-wide (admin) analytics are required, extract that into a
separate explicit admin-only query instead of making schoolId optional in this
function.
1 participant
Impact on performance
COUNTqueries running across four main analytical dashboards down to just 4 aggregated queries.Technical Rationale
Previously, analytics endpoints calculating performance breakdowns—like counting active, inactive, and suspended schools, or finding current versus previous period growths—were running completely separate
db.select({ count: count() })statements usingPromise.all. This forced PostgreSQL to scan the tables multiple times, once for each criteria.By refactoring these
Promise.allstructures to use PostgreSQL's nativeFILTER (WHERE ...)clause (via Drizzle'ssql<number>tag), we combine these independent aggregations. Postgres can now scan the table once and correctly bucket the sums in a single pass. We also added fallback defaults (Number()with|| 0) to ensure type safety matching the old outputs.PR created automatically by Jules for task 3327829088487857982 started by @ldsgroups225
Summary by CodeRabbit