🚨 Lighthouse: [Best Practice/Perf Fix] Parallelize analytics queries#87
Conversation
Grouped multiple independent database queries into Promise.all() in packages/data-ops/src/queries/analytics.ts, eliminating sequential await waterfalls in getAnalyticsOverview, getEnrollmentStats, and getEnrollmentGrowth.
|
👋 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. |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe changes refactor analytics query functions to use Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 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 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 333-348: The current Promise.all block and related queries run
against the students table without a tenant filter when schoolId is missing;
ensure tenant isolation by making schoolId mandatory or returning early if it's
absent, and add a where(eq(students.schoolId, schoolId)) filter to every
students query (including the counts using db.select(...).from(students) and the
getEnrollmentTrends(timeRange, schoolId) call) so each query includes
eq(students.schoolId, schoolId); also apply the same guard/filter to the other
block referenced (lines ~402-415) that queries students so no school-scoped
table is queried without the tenant filter.
- Around line 38-52: Wrap the existing Promise.all(...) block that resolves
[totalSchoolsResult, currentPeriodResult, previousPeriodResult] in a
ResultAsync<ResultType, DatabaseError> instead of awaiting it directly: create a
ResultAsync from the Promise.all(db.select(...).from(schools)...) call (using
ResultAsync.fromPromise or the package's standard constructor), preserve the
same Promise.all contents (db.select, schools, gte, and, sql) to keep
parallelism, and ensure you chain .tapLogErr(...) on that ResultAsync before
returning so any Drizzle errors are converted to DatabaseError and logged via
tapLogErr; update the function's return type to ResultAsync<..., DatabaseError>
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c56c89e1-2883-41ab-8be5-5c9d93ff5d60
📒 Files selected for processing (1)
packages/data-ops/src/queries/analytics.ts
| // Get schools stats in parallel | ||
| const [ | ||
| [totalSchoolsResult], | ||
| [currentPeriodResult], | ||
| [previousPeriodResult] | ||
| ] = await Promise.all([ | ||
| db.select({ count: count() }).from(schools), | ||
| db.select({ count: count() }).from(schools).where(gte(schools.createdAt, startDate)), | ||
| db.select({ count: count() }).from(schools).where( | ||
| and( | ||
| gte(schools.createdAt, previousStartDate), | ||
| sql`${schools.createdAt} < ${startDate}`, | ||
| ), | ||
| ) | ||
| ]) |
There was a problem hiding this comment.
Wrap this parallel query bundle in the package ResultAsync flow.
This keeps the latency win, but it still lets Drizzle failures escape as raw exceptions from a query function and skips the required tapLogErr(...) logging path. Please keep the Promise.all, but move it under the standard ResultAsync<T, DatabaseError> wrapper used in @repo/data-ops.
As per coding guidelines, "Every query function MUST return ResultAsync<T, DatabaseError> from @praha/byethrow with error handling via tapLogErr" and "Do NOT skip tapLogErr on ResultAsync chains in query 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 38 - 52, Wrap the
existing Promise.all(...) block that resolves [totalSchoolsResult,
currentPeriodResult, previousPeriodResult] in a ResultAsync<ResultType,
DatabaseError> instead of awaiting it directly: create a ResultAsync from the
Promise.all(db.select(...).from(schools)...) call (using ResultAsync.fromPromise
or the package's standard constructor), preserve the same Promise.all contents
(db.select, schools, gte, and, sql) to keep parallelism, and ensure you chain
.tapLogErr(...) on that ResultAsync before returning so any Drizzle errors are
converted to DatabaseError and logged via tapLogErr; update the function's
return type to ResultAsync<..., DatabaseError> accordingly.
| // Get enrollment stats in parallel | ||
| const [ | ||
| [totalResult], | ||
| [activeResult], | ||
| [graduatedResult], | ||
| [transferredResult], | ||
| [withdrawnResult], | ||
| 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)), | ||
| getEnrollmentTrends(timeRange, schoolId) | ||
| ]) |
There was a problem hiding this comment.
Do not run students analytics without a tenant filter.
When schoolId is missing, both of these Promise.all blocks query students across every school, and getEnrollmentTrends(timeRange, schoolId) does the same. For a school-scoped table, that breaks tenant isolation. Make schoolId mandatory here, or return early before issuing any students query unless the tenant filter is present.
Based on learnings, "Multi-tenant queries on school-scoped tables MUST include where(eq(table.schoolId, schoolId)) filter to enforce data isolation."
Also applies to: 402-415
🤖 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 333 - 348, The
current Promise.all block and related queries run against the students table
without a tenant filter when schoolId is missing; ensure tenant isolation by
making schoolId mandatory or returning early if it's absent, and add a
where(eq(students.schoolId, schoolId)) filter to every students query (including
the counts using db.select(...).from(students) and the
getEnrollmentTrends(timeRange, schoolId) call) so each query includes
eq(students.schoolId, schoolId); also apply the same guard/filter to the other
block referenced (lines ~402-415) that queries students so no school-scoped
table is queried without the tenant filter.
ldsgroups225
deleted the
lighthouse/analytics-parallel-queries-11594021532884556163
branch
2 participants
Impact on performance: Reduced total round-trip latency in analytics data fetching by executing independent Drizzle ORM queries concurrently instead of sequentially.
Technical rationale: Analytics operations often require data from various tables (schools, students, etc.) separated into different independent counts and aggregations. Executing them sequentially with individual
awaitstatements created a waterfall effect where the total latency is the sum of all individual query latencies. By wrapping these inPromise.all(), the total time bound is reduced to the longest-running individual query.PR created automatically by Jules for task 11594021532884556163 started by @ldsgroups225
Summary by CodeRabbit