Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded@BilalG1 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 18 minutes and 9 seconds before requesting another review. ⌛ 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. 📒 Files selected for processing (3)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a CacheEntry DB model and a getOrSetCacheValue helper, wraps several internal metrics loaders with a tenancy-scoped caching layer, replaces per-user reads with batched Prisma queries and CRUD mapping, refactors user last-active mapping, and adds E2E performance tests and SQL seed data for benchmarking. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant MetricsRoute
participant CacheLayer as Cache (CacheEntry)
participant EventsDB as Events
participant Prisma
participant Mapper
Client->>MetricsRoute: GET /internal/metrics
MetricsRoute->>CacheLayer: get cached payload (namespace, key)
alt cache hit & valid
CacheLayer-->>MetricsRoute: cached payload
MetricsRoute-->>Client: Return cached metrics
else cache miss/expired
MetricsRoute->>EventsDB: Query recent events (tenancy-scoped)
EventsDB-->>MetricsRoute: event rows (userIds)
alt events found
MetricsRoute->>Prisma: Batch fetch users by IDs (userFullInclude)
Prisma-->>MetricsRoute: Prisma user records
MetricsRoute->>Mapper: userPrismaToCrud mapping
Mapper-->>MetricsRoute: CRUD-shaped users
MetricsRoute->>CacheLayer: upsert payload with expiresAt
CacheLayer-->>MetricsRoute: upsert OK
MetricsRoute-->>Client: Return computed metrics
else no events
MetricsRoute-->>Client: Return empty result
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
![recurseml[bot]](https://avatars.githubusercontent.com/in/1054195?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review by RecurseML
🔍 Review performed on 8c805a8..463a7d2
✨ No bugs found, your code is sparkling clean
✅ Files analyzed, no issues (5)
• apps/backend/src/app/api/latest/internal/metrics/route.tsx
• apps/backend/src/app/api/latest/users/crud.tsx
• apps/e2e/tests/backend/performance/mock-metric-events.sql
• apps/e2e/tests/backend/performance/mock-users.sql
• apps/e2e/tests/backend/performance/users-list.test.ts
|
Claude finished @BilalG1's task —— View job PR Review: Metrics Endpoint SpeedI've completed my review of this performance optimization PR. Here's my comprehensive feedback: 📊 Overall AssessmentThis is a well-executed performance optimization PR with solid technical improvements. The changes demonstrate good database optimization practices and thorough performance testing setup. ✅ StrengthsSQL Query Optimizations:
Performance Testing Infrastructure:
Code Quality:
|
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Overview
Summary
This PR optimizes the metrics endpoint query performance by refactoring SQL queries to use more efficient patterns. Key changes include:
- Daily Active Users query: Restructured to use CTEs with proper filtering and deduplication at the SQL level, avoiding
DISTINCTon computed expressions - Recently Active Users: Replaced N+1 pattern (iterating through events and calling
usersCrudHandlers.adminReadfor each) with a batch query approach usingfindManyand in-memory mapping - User mapping logic: Extracted common pattern into reusable
mapUserLastActiveAtMillisfunction usingMap<string, number>for O(1) lookups - Test infrastructure: Added comprehensive SQL scripts to generate 10k test users and 300k activity events across 100 countries for realistic performance testing
The optimizations focus on reducing database round trips and improving query efficiency through better use of PostgreSQL features like CTEs and proper grouping.
Confidence Score: 4/5
- This PR is safe to merge with low risk - query optimizations are well-structured and maintain correctness
- Score reflects solid SQL optimization work with good test infrastructure. Minor deduction for using plain object instead of Map in one location and lack of index verification for query performance
- apps/backend/src/app/api/latest/internal/metrics/route.tsx - verify database indexes support the LATERAL join patterns
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| apps/backend/src/app/api/latest/internal/metrics/route.tsx | 4/5 | Refactored SQL queries for better performance using CTEs and extracted user mapping logic, improving Daily Active Users query deduplication |
| apps/backend/src/app/api/latest/users/crud.tsx | 5/5 | Extracted user mapping logic into reusable mapUserLastActiveAtMillis function using Map for efficient lookups |
| apps/e2e/tests/backend/performance/metrics.test.ts | 5/5 | Added skipped performance tests with warmup and multiple iterations for metrics endpoint benchmarking |
Sequence Diagram
sequenceDiagram
participant Client
participant MetricsRoute as Metrics Route Handler
participant PrismaClient as Prisma Client
participant PostgreSQL as PostgreSQL DB
Client->>MetricsRoute: GET /api/v1/internal/metrics
par Parallel Metric Queries
MetricsRoute->>PrismaClient: Count total users
PrismaClient->>PostgreSQL: SELECT COUNT(*) FROM ProjectUser
PostgreSQL-->>PrismaClient: Count result
PrismaClient-->>MetricsRoute: Total users
and
MetricsRoute->>PrismaClient: Load daily active users
PrismaClient->>PostgreSQL: CTE query with filtered_events + unique_daily_users
Note over PostgreSQL: Deduplicates (day, userId) pairs<br/>before aggregation
PostgreSQL-->>PrismaClient: DAU data
PrismaClient-->>MetricsRoute: Daily active users
and
MetricsRoute->>PrismaClient: Load users by country
PrismaClient->>PostgreSQL: LATERAL join for latest country per user
Note over PostgreSQL: Efficient per-user lookup<br/>with LIMIT 1
PostgreSQL-->>PrismaClient: Country distribution
PrismaClient-->>MetricsRoute: Users by country
and
MetricsRoute->>PrismaClient: Load recently active users
PrismaClient->>PostgreSQL: DISTINCT ON (userId) for latest activity
PostgreSQL-->>PrismaClient: Top 5 user IDs + timestamps
MetricsRoute->>PrismaClient: Batch findMany for user details
PrismaClient->>PostgreSQL: SELECT * FROM ProjectUser WHERE id IN (...)
PostgreSQL-->>PrismaClient: User records
Note over MetricsRoute: Map users with lastActiveAt<br/>in memory
PrismaClient-->>MetricsRoute: Recently active users
end
MetricsRoute-->>Client: Aggregated metrics response
Additional Comments (1)
-
apps/backend/src/app/api/latest/internal/metrics/route.tsx, line 55-59 (link)syntax: use
Map<string, number>instead of plain object for dynamic keys to avoid prototype pollution vulnerabilitiesContext Used: Rule from
dashboard- Use Map<A, B> instead of plain objects when using dynamic keys to avoid prototype pollution vulnerab... (source)
5 files reviewed, 1 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
🧹 Nitpick comments (9)
apps/backend/src/app/api/latest/users/crud.tsx (1)
197-216: Add a precondition to guard array alignment; keep fallback robust.mapUserLastActiveAtMillis assumes userIds and userSignedUpAtMillis are index-aligned. Add an assertion to fail fast on mismatch.
const mapUserLastActiveAtMillis = ( events: Array<{ userId: string, lastActiveAt: Date }>, userIds: string[], userSignedUpAtMillis: (number | Date)[], ): number[] => { + if (userIds.length !== userSignedUpAtMillis.length) { + throw new StackAssertionError( + "userIds and userSignedUpAtMillis must have identical length", + { userIdsLen: userIds.length, signedUpLen: userSignedUpAtMillis.length } + ); + } const eventsByUserId = new Map<string, number>(); for (const event of events) { eventsByUserId.set(event.userId, event.lastActiveAt.getTime()); }apps/e2e/tests/backend/performance/mock-users.sql (1)
24-39: Consider idempotency guards to make the seed re-runnable.Re-running will create more users and auth rows. If that’s undesirable for perf runs, add ON CONFLICT DO NOTHING keyed on deterministic IDs or wrap in a DELETE-by-tag prelude. Example patterns:
- Add a seed tag (e.g., displayName prefix) and DELETE FROM ... WHERE displayName LIKE 'Perf Test User %' before inserts.
- Or generate deterministic UUIDs from idx (uuid_generate_v5) and use ON CONFLICT DO NOTHING.
Also applies to: 41-57, 58-79
apps/backend/src/app/api/latest/internal/metrics/route.tsx (2)
92-129: DAU CTE structure looks correct; add indexes to keep it fast.For sustained performance on large Event volumes, ensure functional indexes exist:
- GIN on "systemEventTypeIds"
- BTREE on ("eventStartedAt")
- BTREE on ((data->>'projectId')), ((COALESCE(data->>'branchId','main'))), ((data->>'userId'))
- Partial index for '$user-activity' filter if common
Optional: tighten the time window by passing a truncated date (new Date().toISOString().slice(0,10)) to avoid TZ ambiguity between app and DB.
164-207: Nice N+1 removal; minor O(1) lookup improvement.Build a Map for dbUsers to avoid linear finds (future-proof if LIMIT grows).
- const userObjects = events.map((event) => { - const user = dbUsers.find((user) => user.projectUserId === event.userId); - return user ? userPrismaToCrud(user, event.lastActiveAt.getTime()) : null; - }); + const byId = new Map(dbUsers.map(u => [u.projectUserId, u])); + const userObjects = events.map((event) => { + const user = byId.get(event.userId); + return user ? userPrismaToCrud(user, event.lastActiveAt.getTime()) : null; + });apps/e2e/tests/backend/performance/metrics.test.ts (3)
1-5: Imports OK; consider removing unused types to keep lint happy.If User is unused, drop it to reduce noise.
6-23: Gate perf suite via env flag and add basic assertions/snapshots.
- Replace describe.skip with an ENV-gated helper so devs can opt-in locally/CI.
- Assert non-empty items and snapshot a stable shape.
-describe.skip("/api/v1/users performance", () => { +const RUN_PERF = process.env.RUN_PERF === "1"; +const describeIf = RUN_PERF ? describe : describe.skip; +describeIf("/api/v1/users performance", () => { @@ - const response = await niceBackendFetch("/api/v1/users?limit=200000", { accessType: "server" }); + const response = await niceBackendFetch("/api/v1/users?limit=200000", { accessType: "server" }); const durationMs = performance.now() - start; expect(response.status).toBe(200); + expect(Array.isArray(response.body.items)).toBe(true); + expect(response.body.items.length).toBeGreaterThan(0); + expect(Object.keys(response.body)).toMatchInlineSnapshot([ + "items", + "is_paginated", + "pagination", + ]);As per coding guidelines.
25-48: Same gating and minimal snapshot for metrics.
- Use ENV-gated describe.
- Snapshot stable response keys; keep timings as logs.
-describe.skip("/api/v1/internal/metrics performance", () => { +describeIf("/api/v1/internal/metrics performance", () => { @@ - const response = await niceBackendFetch("/api/v1/internal/metrics", { accessType: "admin" }); + const response = await niceBackendFetch("/api/v1/internal/metrics", { accessType: "admin" }); const durationMs = performance.now() - start; expect(response.status).toBe(200); + expect(Object.keys(response.body)).toMatchInlineSnapshot([ + "total_users", + "daily_users", + "daily_active_users", + "users_by_country", + "recently_registered", + "recently_active", + "login_methods", + ]); return durationMs; };As per coding guidelines.
apps/e2e/tests/backend/performance/mock-metric-events.sql (2)
50-67: Auth-method enrichment is solid; consider idempotency.These blocks generate new AuthMethod/Passkey/OTP/OAuth rows on each run. If seeds may run multiple times, add ON CONFLICT DO NOTHING keyed on ("tenancyId","projectUserId", method kind) or a pre-clean step.
Also applies to: 88-117, 118-139, 140-168, 169-183
189-304: Event/IP data volume and cleanup strategy.
- This creates ~300k Event and 300k EventIpInfo rows (300 users x 1000 events). Confirm this is the intended load for local/CI; consider parameterizing LIMIT and per-user event count via psql variables.
- Since data includes seedTag ('perf-metrics-mock'), add an optional DELETE FROM "Event" WHERE data->>'seedTag'='perf-metrics-mock' prelude to keep runs bounded.
For long-term perf testing, keep a maintenance step to purge old seedRunId batches to control DB size.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/backend/src/app/api/latest/internal/metrics/route.tsx(7 hunks)apps/backend/src/app/api/latest/users/crud.tsx(2 hunks)apps/e2e/tests/backend/performance/metrics.test.ts(1 hunks)apps/e2e/tests/backend/performance/mock-metric-events.sql(1 hunks)apps/e2e/tests/backend/performance/mock-users.sql(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
apps/backend/src/app/api/latest/**
📄 CodeRabbit inference engine (AGENTS.md)
apps/backend/src/app/api/latest/**: Organize backend API routes by resource under /api/latest (e.g., auth at /api/latest/auth/, users at /api/latest/users/, teams at /api/latest/teams/, oauth providers at /api/latest/oauth-providers/)
Use the custom route handler system in the backend to ensure consistent API responses
Files:
apps/backend/src/app/api/latest/users/crud.tsxapps/backend/src/app/api/latest/internal/metrics/route.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Prefer ES6 Map over Record when representing key–value collections
Files:
apps/backend/src/app/api/latest/users/crud.tsxapps/backend/src/app/api/latest/internal/metrics/route.tsxapps/e2e/tests/backend/performance/metrics.test.ts
**/*.test.{ts,tsx,js}
📄 CodeRabbit inference engine (AGENTS.md)
In tests, prefer .toMatchInlineSnapshot where possible; refer to snapshot-serializer.ts for snapshot formatting and handling of non-deterministic values
Files:
apps/e2e/tests/backend/performance/metrics.test.ts
🧬 Code graph analysis (2)
apps/backend/src/app/api/latest/internal/metrics/route.tsx (3)
apps/backend/src/prisma-client.tsx (4)
getPrismaSchemaForTenancy(68-70)globalPrismaClient(33-33)sqlQuoteIdent(376-378)getPrismaClientForTenancy(64-66)apps/backend/src/lib/tenancies.tsx (1)
Tenancy(47-47)apps/backend/src/app/api/latest/users/crud.tsx (2)
userFullInclude(28-47)userPrismaToCrud(87-129)
apps/e2e/tests/backend/performance/metrics.test.ts (2)
apps/e2e/tests/backend/backend-helpers.ts (3)
backendContext(34-56)InternalProjectKeys(75-80)niceBackendFetch(107-171)apps/e2e/tests/helpers.ts (1)
it(11-11)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: build (22.x)
- GitHub Check: build (22.x)
- GitHub Check: restart-dev-and-test
- GitHub Check: setup-tests
- GitHub Check: lint_and_build (latest)
- GitHub Check: docker
- GitHub Check: all-good
- GitHub Check: claude-review
- GitHub Check: docker
- GitHub Check: check_prisma_migrations (22.x)
- GitHub Check: Security Check
🔇 Additional comments (6)
apps/backend/src/app/api/latest/users/crud.tsx (1)
242-242: Refactor LGTM; reduces duplication.Delegating to mapUserLastActiveAtMillis keeps the query/result mapping simple and consistent.
apps/e2e/tests/backend/performance/mock-users.sql (1)
1-4: Ensure pgcrypto availability in CI/dev.CREATE EXTENSION may require elevated privileges. If your CI DB role lacks this, document a pre-step or guard via migration tooling.
apps/backend/src/app/api/latest/internal/metrics/route.tsx (3)
1-1: Imports and reuse from users CRUD: good integration.Also applies to: 8-8
65-65: Type annotation tweak is fine.
137-137: Signature/spacing change: no issues.apps/e2e/tests/backend/performance/mock-metric-events.sql (1)
14-31: Timestamp spreading logic LGTM. Distributes createdAt over last 30 days with hour/minute jitter.
There was a problem hiding this comment.
Review by RecurseML
🔍 Review performed on 463a7d2..0122422
✨ No bugs found, your code is sparkling clean
✅ Files analyzed, no issues (2)
• apps/backend/src/app/api/latest/internal/metrics/route.tsx
• apps/e2e/tests/backend/performance/metrics.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/backend/scripts/db-migrations.ts (2)
12-17: Dangerous in prod: DROP SCHEMA + GRANT ALL TO publicAdd a hard environment guard before any destructive path, and avoid granting ALL to public outside dev/test. Gate these statements behind a prod check.
Example guard usage added below in main comment; also wrap GRANTs with a dev-only conditional.
176-191: Add a hard safety guard before destructive commands; wire--yesPrevent accidental production drops and DB-target mismatch before running reset/generate-migration-file.
- switch (command) { + switch (command) { case 'reset': { - await promptDropDb(); + const force = await assertSafeToReset(args); + await promptDropDb(force); await dropSchema(); await migrate(); await seed(); break; } case 'generate-migration-file': { - await promptDropDb(); + const force = await assertSafeToReset(args); + await promptDropDb(force); await dropSchema(); await migrate(); await generateMigrationFile(); await seed(); break; }Add this helper (place above
generateMigrationFileor just beforemain):+const isProductionLike = () => (process.env.NODE_ENV || '').toLowerCase().includes('prod'); +const parseDb = (raw: string) => { + try { const u = new URL(raw); return { host: u.host, db: u.pathname.replace(/^\//,'') }; } + catch { return null as any; } +}; +async function assertSafeToReset(args: string[]) { + const force = args.includes('--yes') || process.env.FORCE_DB_RESET === '1'; + const dbUrl = process.env.DATABASE_URL || ''; + const directUrl = process.env.STACK_DIRECT_DATABASE_CONNECTION_STRING || dbUrl; + const a = parseDb(dbUrl), b = parseDb(directUrl); + const envUnsafe = isProductionLike(); + const targetMismatch = !!(a && b && (a.host !== b.host || a.db !== b.db)); + if ((envUnsafe || targetMismatch) && !force) { + throw new Error(`Refusing to run destructive command${envUnsafe ? ' in production' : ''}${targetMismatch ? ' with mismatched DB targets' : ''}. Re-run with --yes to override.`); + } + return force; +}
♻️ Duplicate comments (3)
apps/backend/src/app/api/latest/internal/metrics/route.tsx (3)
49-50: Schema qualification still missing for Event and EventIpInfo.As flagged in the previous review, these tables should be schema-qualified with
${sqlQuoteIdent(schema)}."Event"and${sqlQuoteIdent(schema)}."EventIpInfo"for consistency with tenancy-scoped queries elsewhere in the codebase.
118-118: Schema qualification missing for Event table.Similar to the issue in
loadUsersByCountry, this query should use${sqlQuoteIdent(schema)}."Event"for consistency with tenancy-scoped operations.
182-182: Schema qualification missing for Event table.The Event table should be schema-qualified as
${sqlQuoteIdent(schema)}."Event"to match the pattern used in tenancy-scoped queries.
🧹 Nitpick comments (7)
apps/backend/src/lib/cache.tsx (1)
40-42: Cache stampede: concurrent requests can duplicate expensive work.When an expired entry is found (or missing), multiple concurrent requests will all invoke
loader()simultaneously before any result is cached. For expensive computations this defeats the purpose of caching.Consider implementing a locking mechanism (e.g., advisory locks, in-memory promise cache, or optimistic "loading" sentinel) to ensure only one request computes the value while others wait. This is a common pattern in production cache libraries.
apps/backend/scripts/db-migrations.ts (6)
5-8: Optional: prefer node: specifiers for core modulesUse
node:child_process,node:fs,node:path,node:readlinefor clarity and faster resolution.-import { spawnSync } from "child_process"; -import fs from "fs"; -import path from "path"; -import * as readline from "readline"; +import { spawnSync } from "node:child_process"; +import fs from "node:fs"; +import path from "node:path"; +import * as readline from "node:readline"; ```<!-- review_comment_end --> --- `20-33`: **Non-interactive/CI-friendly prompt** Support a `force`/`--yes` path and non‑TTY detection to avoid hanging in CI; also handle Ctrl‑C. ```diff -const askQuestion = (question: string) => { +const askQuestion = (question: string) => { const rl = readline.createInterface({ input: process.stdin, output: process.stdout, }); - return new Promise<string>((resolve) => { + return new Promise<string>((resolve) => { + rl.on('SIGINT', () => { rl.close(); resolve(''); }); rl.question(question, (answer) => { rl.close(); resolve(answer); }); }); }; -const promptDropDb = async () => { - const answer = (await askQuestion( +const promptDropDb = async (force = false) => { + if (force) return; + if (!process.stdout.isTTY) { + console.error('Non-interactive terminal. Re-run with --yes to skip confirmation.'); + process.exit(1); + } + const answer = (await askQuestion( 'Are you sure you want to drop everything in the database? This action cannot be undone. (y/N): ', )).trim(); if (answer.toLowerCase() !== 'y') { console.log('Operation cancelled'); process.exit(0); } }; ```<!-- review_comment_end --> --- `80-97`: **CLI portability** Relying on `pnpm prisma` requires pnpm on PATH. If this script is used outside pnpm contexts, consider `npx prisma` or invoking the local binary via package.json script. <!-- review_comment_end --> --- `118-121`: **Migration summary counts/labels when selecting specific migrations** When `selectedMigrationFiles` is used (e.g., applying the just-generated one), “Total migrations in folder” becomes misleading. Compute folder total separately and label considered vs folder totals. ```diff - const migrationFiles = selectedMigrationFiles ?? getMigrationFiles(MIGRATION_FILES_DIR); - const totalMigrations = migrationFiles.length; + const migrationFiles = selectedMigrationFiles ?? getMigrationFiles(MIGRATION_FILES_DIR); + const folderTotal = getMigrationFiles(MIGRATION_FILES_DIR).length; + const consideredTotal = migrationFiles.length; @@ - console.log(`📁 Total migrations in folder: ${totalMigrations}`); - console.log(`🆕 Newly applied migrations: ${result.newlyAppliedMigrationNames.length}`); - console.log(`✓ Already applied migrations: ${totalMigrations - result.newlyAppliedMigrationNames.length}`); + console.log(`📁 Migrations in folder: ${folderTotal}`); + if (selectedMigrationFiles) console.log(`🎯 Migrations considered: ${consideredTotal}`); + console.log(`🆕 Newly applied migrations: ${result.newlyAppliedMigrationNames.length}`); + console.log(`✓ Already applied migrations: ${consideredTotal - result.newlyAppliedMigrationNames.length}`); ```<!-- review_comment_end --> Also applies to: 133-141 --- `162-169`: **Help text drift** Reflect current behavior and add `--yes` flag. ```diff Commands: - reset Drop all data and recreate the database, then apply migrations and seed - generate-migration-file Generate a new migration file using Prisma, then reset and migrate + reset Drop all data and recreate the database, then apply migrations and seed + generate-migration-file Generate and apply a new migration file from schema changes, then seed seed [Advanced] Run database seeding only init Apply migrations and seed the database migrate Apply migrations help Show this help message + +Flags: + --yes Skip confirmation prompts for destructive operations ```<!-- review_comment_end --> --- `10-10`: **Avoid deep import to /dist if possible** Prefer a stable public entrypoint (e.g., `@stackframe/stack-shared/utils/env`) to reduce coupling to build output. If the package exposes a public export, switch to it; otherwise ignore.<!-- review_comment_end --> <!-- file_end --> </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: CodeRabbit UI **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 01224227ee924a6d76ad3f624375bd8fc3ee2e2c and 4d9be71063ee33be7e11bbeb5ab1256f3f86fe71. </details> <details> <summary>📒 Files selected for processing (6)</summary> * `.github/workflows/check-prisma-migrations.yaml` (0 hunks) * `apps/backend/prisma/migrations/20251020210214_add_cache_entry/migration.sql` (1 hunks) * `apps/backend/prisma/schema.prisma` (2 hunks) * `apps/backend/scripts/db-migrations.ts` (3 hunks) * `apps/backend/src/app/api/latest/internal/metrics/route.tsx` (7 hunks) * `apps/backend/src/lib/cache.tsx` (1 hunks) </details> <details> <summary>💤 Files with no reviewable changes (1)</summary> * .github/workflows/check-prisma-migrations.yaml </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>📓 Path-based instructions (2)</summary> <details> <summary>**/*.{ts,tsx,js,jsx}</summary> **📄 CodeRabbit inference engine (AGENTS.md)** > Prefer ES6 Map over Record when representing key–value collections Files: - `apps/backend/scripts/db-migrations.ts` - `apps/backend/src/lib/cache.tsx` - `apps/backend/src/app/api/latest/internal/metrics/route.tsx` </details> <details> <summary>apps/backend/src/app/api/latest/**</summary> **📄 CodeRabbit inference engine (AGENTS.md)** > `apps/backend/src/app/api/latest/**`: Organize backend API routes by resource under /api/latest (e.g., auth at /api/latest/auth/*, users at /api/latest/users/*, teams at /api/latest/teams/*, oauth providers at /api/latest/oauth-providers/*) > Use the custom route handler system in the backend to ensure consistent API responses Files: - `apps/backend/src/app/api/latest/internal/metrics/route.tsx` </details> </details><details> <summary>🧬 Code graph analysis (3)</summary> <details> <summary>apps/backend/scripts/db-migrations.ts (2)</summary><blockquote> <details> <summary>apps/backend/src/auto-migrations/utils.tsx (2)</summary> * `MIGRATION_FILES_DIR` (5-5) * `getMigrationFiles` (7-30) </details> <details> <summary>packages/stack-shared/src/utils/env.tsx (1)</summary> * `getEnvVariable` (16-58) </details> </blockquote></details> <details> <summary>apps/backend/src/lib/cache.tsx (1)</summary><blockquote> <details> <summary>apps/backend/src/prisma-client.tsx (1)</summary> * `PrismaClientTransaction` (18-18) </details> </blockquote></details> <details> <summary>apps/backend/src/app/api/latest/internal/metrics/route.tsx (4)</summary><blockquote> <details> <summary>apps/backend/src/lib/tenancies.tsx (1)</summary> * `Tenancy` (47-47) </details> <details> <summary>apps/backend/src/prisma-client.tsx (5)</summary> * `PrismaClientTransaction` (18-18) * `getPrismaSchemaForTenancy` (68-70) * `globalPrismaClient` (33-33) * `sqlQuoteIdent` (376-378) * `getPrismaClientForTenancy` (64-66) </details> <details> <summary>apps/backend/src/lib/cache.tsx (1)</summary> * `getOrSetCacheValue` (19-64) </details> <details> <summary>apps/backend/src/app/api/latest/users/crud.tsx (2)</summary> * `userFullInclude` (28-47) * `userPrismaToCrud` (87-129) </details> </blockquote></details> </details> </details> <details> <summary>⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)</summary> * GitHub Check: check_prisma_migrations (22.x) * GitHub Check: docker * GitHub Check: lint_and_build (latest) * GitHub Check: setup-tests * GitHub Check: all-good * GitHub Check: build (22.x) * GitHub Check: build (22.x) * GitHub Check: docker * GitHub Check: restart-dev-and-test * GitHub Check: Security Check </details> <details> <summary>🔇 Additional comments (9)</summary><blockquote> <details> <summary>apps/backend/prisma/migrations/20251020210214_add_cache_entry/migration.sql (1)</summary><blockquote> `1-15`: **Migration looks good.** The CacheEntry table structure correctly implements the caching requirements with appropriate constraints and indexes. </blockquote></details> <details> <summary>apps/backend/src/app/api/latest/internal/metrics/route.tsx (3)</summary><blockquote> `14-25`: **Well-structured caching integration.** The cache key composition correctly isolates metrics by tenancy and anonymous-user inclusion, and the 1-hour TTL is appropriate for analytics use cases. --- `205-220`: **Excellent refactor to batch user loading.** Replacing per-user `adminRead` calls with a single `findMany` query significantly improves performance. The reuse of `userFullInclude` and `userPrismaToCrud` from the CRUD module promotes consistency. --- `269-301`: **Strategic caching of expensive metric queries.** The selective caching of `daily_active_users`, `users_by_country`, and `recently_active_users` appropriately targets the most computationally expensive metrics while leaving simpler queries uncached. </blockquote></details> <details> <summary>apps/backend/prisma/schema.prisma (1)</summary><blockquote> `842-853`: **No duplicates found—the review comment is based on incorrect AI analysis.** The verification confirms only one `CacheEntry` model exists at line 843. The AI summary incorrectly claimed duplicate definitions. The schema is correct; no action required. > Likely an incorrect or invalid review comment. </blockquote></details> <details> <summary>apps/backend/scripts/db-migrations.ts (4)</summary><blockquote> `45-51`: **Migration name sanitizer looks good** <!-- review_comment_end --> --- `52-69`: **Interactive migration name UX is solid** <!-- review_comment_end --> --- `70-70`: **Timestamp prefix OK** <!-- review_comment_end --> --- `72-116`: **The review comment about DATABASE_URL fallback is incorrect and does not apply to this codebase.** The review assumes `DATABASE_URL` is a competing connection string variable, but verification confirms: - `DATABASE_URL` does not exist in the backend codebase (zero occurrences in TypeScript files and .env configuration) - `STACK_DIRECT_DATABASE_CONNECTION_STRING` is the sole database connection variable used - No DB mismatch scenario exists, as there is no second connection target The `getEnvVariable()` function does support an optional `defaultValue` parameter, but proposing `DATABASE_URL` as a fallback is inapplicable when that variable is not configured anywhere in the backend environment. > Likely an incorrect or invalid review comment. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
apps/backend/src/app/api/latest/internal/metrics/route.tsx (3)
108-151: Schema-qualify Event in DAU query. Add indexes (ops).Qualify Event; suggest partial indexes for JSON filters to keep performant at scale.
Apply:
+ const schema = await getPrismaSchemaForTenancy(tenancy); - const res = await globalPrismaClient.$queryRaw<{ day: Date, dau: bigint }[]>` + const res = await globalPrismaClient.$queryRaw<{ day: Date, dau: bigint }[]>` ... - FROM "Event" + FROM ${sqlQuoteIdent(schema)}."Event" ... `;Operational (migration example):
Index on project/branch and event type:
CREATE INDEX CONCURRENTLY IF NOT EXISTS evt_proj_branch_type_started_at
ON ""."Event" ((data->>'projectId'), COALESCE((data->>'branchId'),'main'), "eventStartedAt")
WHERE '$user-activity' = ANY("systemEventTypeIds"::text[]);Optional: expression index on (data->>'userId').
50-71: Schema-qualify Event/EventIpInfo in users-by-country.Unqualified tables risk hitting the wrong schema. Qualify with tenancy schema. This mirrors other routes.
Apply:
+ const schema = await getPrismaSchemaForTenancy(tenancy); const rows = await globalPrismaClient.$queryRaw<{ countryCode: string | null, userCount: bigint }[]>(Prisma.sql` WITH latest_ip AS ( SELECT DISTINCT ON (e."data"->>'userId') e."data"->>'userId' AS "userId", eip."countryCode" AS "countryCode" - FROM "Event" e - JOIN "EventIpInfo" eip + FROM ${sqlQuoteIdent(schema)}."Event" e + JOIN ${sqlQuoteIdent(schema)}."EventIpInfo" eip ON eip.id = e."endUserIpInfoGuessId" WHERE '$user-activity' = ANY(e."systemEventTypeIds"::text[]) AND e."data"->>'projectId' = ${tenancy.project.id} AND COALESCE(e."data"->>'branchId', 'main') = ${tenancy.branchId} AND e."data"->>'userId' = ANY(${userIdArray}) AND e."endUserIpInfoGuessId" IS NOT NULL AND eip."countryCode" IS NOT NULL ORDER BY e."data"->>'userId', e."eventStartedAt" DESC ) SELECT "countryCode", COUNT("userId") AS "userCount" FROM latest_ip GROUP BY "countryCode" ORDER BY "userCount" DESC; `);
179-205: Schema-qualify Event in recently-active query.Same consistency issue; qualify table with tenancy schema.
Apply:
+ const schema = await getPrismaSchemaForTenancy(tenancy); const events = await globalPrismaClient.$queryRaw<{ userId: string, lastActiveAt: Date }[]>` ... - FROM "Event" + FROM ${sqlQuoteIdent(schema)}."Event" ... `;
🧹 Nitpick comments (2)
apps/backend/src/app/api/latest/internal/metrics/route.tsx (2)
15-17: Make TTL configurable (optional).Consider reading TTL from env/config to tune per environment.
191-194: LIMIT 4000 may drop true top-N distinct users under bursty single-user traffic.Either (a) remove this LIMIT and add a time window (e.g., last 7 days), or (b) increase progressively until N distinct users found.
Example change:
- ORDER BY "eventStartedAt" DESC - LIMIT 4000 + WHERE "eventStartedAt" >= ${new Date(Date.now() - 7*24*60*60*1000)}::timestamptz + ORDER BY "eventStartedAt" DESC
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/check-prisma-migrations.yaml(0 hunks)apps/backend/src/app/api/latest/internal/metrics/route.tsx(6 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/check-prisma-migrations.yaml
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use ES6 Maps instead of Records wherever possible in TypeScript code
Files:
apps/backend/src/app/api/latest/internal/metrics/route.tsx
🧬 Code graph analysis (1)
apps/backend/src/app/api/latest/internal/metrics/route.tsx (4)
apps/backend/src/lib/tenancies.tsx (1)
Tenancy(47-47)apps/backend/src/prisma-client.tsx (4)
PrismaClientTransaction(22-22)globalPrismaClient(37-37)getPrismaSchemaForTenancy(72-74)getPrismaClientForTenancy(68-70)apps/backend/src/lib/cache.tsx (1)
getOrSetCacheValue(19-64)apps/backend/src/app/api/latest/users/crud.tsx (2)
userFullInclude(28-47)userPrismaToCrud(87-129)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: check_prisma_migrations (22.x)
- GitHub Check: build (22.x)
- GitHub Check: restart-dev-and-test
- GitHub Check: restart-dev-and-test-with-custom-base-port
- GitHub Check: setup-tests
- GitHub Check: build (22.x)
- GitHub Check: lint_and_build (latest)
- GitHub Check: build (22.x)
- GitHub Check: all-good
- GitHub Check: docker
- GitHub Check: Security Check
🔇 Additional comments (7)
apps/backend/src/app/api/latest/internal/metrics/route.tsx (7)
2-3: Imports: looks good.Nothing to fix here.
Also applies to: 9-11
34-47: Prefiltering users is OK.Fetching candidate userIds up front to honor includeAnonymous is reasonable.
81-105: Total users query: LGTM.Proper schema-qualification and windowed CTE look correct.
153-177: Login methods: LGTM.Already tenancy-qualified; query shape is fine.
221-226: Mapping to CRUD objects: LGTM.Small set, in-memory find is fine here.
274-287: Cache wrapping: LGTM.Good use of includeAnonymous in cache key and tenancy scoping.
18-26: Clarify cache key intent and verify client usage for global CacheEntry.The cache key concern is overstated:
tenancy.idis already a globally unique UUID, preventing cross-branch collisions. However, your suggestion to addprojectIdandbranchIdis reasonable defensive coding for clarity—though not strictly necessary for correctness.More importantly, verify whether a tenant-scoped Prisma client (returned by
getPrismaClientForTenancy) should be querying the globalCacheEntrytable. While the schema is shared across clients, this warrants confirmation to ensure consistency.Consider either:
- If tenant client is correct: Remove the collision concern; the current key is already safe.
- If globalPrismaClient should be used: Pass
globalPrismaClientexplicitly togetOrSetCacheValueinstead of the tenant-scoped client.If you opt to add
projectIdandbranchIdfor defensive clarity, apply the diff; otherwise, no changes needed.
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
2 participants
High-level PR Summary
This PR optimizes the performance of user list and metrics endpoints by refactoring SQL queries to use more efficient patterns. The changes include rewriting queries to use
LATERALjoins and CTEs with proper filtering, extracting common user mapping logic into reusable functions, and adding performance tests with SQL scripts to generate realistic test data (10,000 mock users and activity events across 100 countries).⏱️ Estimated Review Time: 30-90 minutes
💡 Review Order Suggestion
apps/e2e/tests/backend/performance/mock-users.sqlapps/e2e/tests/backend/performance/mock-metric-events.sqlapps/e2e/tests/backend/performance/users-list.test.tsapps/backend/src/app/api/latest/users/crud.tsxapps/backend/src/app/api/latest/internal/metrics/route.tsxImportant
Optimize metrics and user list endpoints with SQL refactoring, caching, and performance tests, adding a
CacheEntrymodel and mock data scripts.route.tsxto useLATERALjoins and CTEs for efficient data retrieval.route.tsxusinggetOrSetCacheValue()to reduce database load.CacheEntrymodel toschema.prismaand create corresponding table and index inmigration.sql.check-prisma-migrations.yaml.metrics.test.tsto benchmark metrics and user endpoints.mock-users.sqlandmock-metric-events.sqlfor testing with 10,000 users and events across 100 countries.db-migrations.tsto include new migration file generation logic.cache.tsxfor caching logic implementation.This description was created by
for 4d9be71. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
New Features
Bug Fixes
Performance
Tests
Chores