⚡ Bolt: [Combine multiple COUNT queries into single query using FILTER in getTicketStats]#227
Conversation
…R in getTicketStats]
|
👋 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. |
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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 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: 1
🤖 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/support/read-tickets.ts`:
- Around line 228-255: The aggregate queries against the supportTickets table
(the three db.select blocks building countsResult, avgTimeResult, and
satisfactionResult) are currently executed without guaranteed tenant scoping and
can return data across schools; update getTicketStats (or the surrounding
function) to require a schoolId parameter or otherwise enforce multi-tenant
isolation by adding eq(supportTickets.schoolId, schoolId) into the where clauses
(i.e., include it in baseConditions or explicitly
and(eq(supportTickets.schoolId, schoolId), ...baseConditions)) so each of the
three selects is always filtered to the requesting school's data, or separate
the all-schools path into an admin-only query.
🪄 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: 67e4fcb8-e711-4fba-a81c-8ad42a45e3c0
📒 Files selected for processing (1)
packages/data-ops/src/queries/support/read-tickets.ts
| const [[countsResult], [avgTimeResult], [satisfactionResult]] = await Promise.all([ | ||
| db | ||
| .select({ | ||
| total: count(), | ||
| open: sql<number>`COUNT(*) FILTER (WHERE ${supportTickets.status} = 'open')`, | ||
| inProgress: sql<number>`COUNT(*) FILTER (WHERE ${supportTickets.status} = 'in_progress')`, | ||
| resolved: sql<number>`COUNT(*) FILTER (WHERE ${supportTickets.status} = 'resolved')`, | ||
| closed: sql<number>`COUNT(*) FILTER (WHERE ${supportTickets.status} = 'closed')`, | ||
| }) | ||
| .from(supportTickets) | ||
| .where(baseConditions.length > 0 ? and(...baseConditions) : undefined), | ||
|
|
||
| // Calculate average resolution time (for resolved tickets) | ||
| const [avgTimeResult] = await db | ||
| .select({ | ||
| avg: sql<number>`AVG(EXTRACT(EPOCH FROM (${supportTickets.resolvedAt} - ${supportTickets.createdAt})) / 3600)`, | ||
| }) | ||
| .from(supportTickets) | ||
| .where(and(eq(supportTickets.status, 'resolved'), ...baseConditions)) | ||
| // Calculate average resolution time (for resolved tickets) | ||
| db | ||
| .select({ | ||
| avg: sql<number>`AVG(EXTRACT(EPOCH FROM (${supportTickets.resolvedAt} - ${supportTickets.createdAt})) / 3600)`, | ||
| }) | ||
| .from(supportTickets) | ||
| .where(and(eq(supportTickets.status, 'resolved'), ...baseConditions)), | ||
|
|
||
| // Calculate average satisfaction score | ||
| const [satisfactionResult] = await db | ||
| .select({ | ||
| avg: sql<number>`AVG(${supportTickets.satisfactionRating})`, | ||
| }) | ||
| .from(supportTickets) | ||
| .where( | ||
| and(isNotNull(supportTickets.satisfactionRating), ...baseConditions), | ||
| ) | ||
| // Calculate average satisfaction score | ||
| db | ||
| .select({ | ||
| avg: sql<number>`AVG(${supportTickets.satisfactionRating})`, | ||
| }) | ||
| .from(supportTickets) | ||
| .where( | ||
| and(isNotNull(supportTickets.satisfactionRating), ...baseConditions), |
There was a problem hiding this comment.
Always tenant-scope these aggregate queries.
Line 238, Line 246, and Line 255 still run unscoped when schoolId is missing, so getTicketStats() can aggregate every school's tickets. For a school-scoped table, that's a cross-tenant data leak; make schoolId required here or split the all-schools path into a separate admin-only query.
As per coding guidelines, "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/support/read-tickets.ts` around lines 228 -
255, The aggregate queries against the supportTickets table (the three db.select
blocks building countsResult, avgTimeResult, and satisfactionResult) are
currently executed without guaranteed tenant scoping and can return data across
schools; update getTicketStats (or the surrounding function) to require a
schoolId parameter or otherwise enforce multi-tenant isolation by adding
eq(supportTickets.schoolId, schoolId) into the where clauses (i.e., include it
in baseConditions or explicitly and(eq(supportTickets.schoolId, schoolId),
...baseConditions)) so each of the three selects is always filtered to the
requesting school's data, or separate the all-schools path into an admin-only
query.
1 participant
💡 What: Refactored
getTicketStatsto use a single query with PostgreSQL'sFILTER (WHERE ...)clause instead of five sequentialCOUNT()queries. Also parallelized the remaining average calculations usingPromise.all().🎯 Why: The original implementation executed five separate database queries sequentially to count tickets by different statuses, plus two more queries for averages. This caused unnecessary network round trips and multiple table scans (N+1 query pattern).
📊 Impact: Reduces network round trips from 7 sequential calls to 1 concurrent
Promise.all()containing 3 queries. Reduces table scans for counts from 5 to 1. Expected to significantly improve response time for the support dashboard metrics.🔬 Measurement: Run the support dashboard and observe the network tab for the stats endpoint response time. Also verified via
pnpm testandpnpm typecheck.PR created automatically by Jules for task 9972217565576460988 started by @ldsgroups225
Summary by CodeRabbit