-
-
Notifications
You must be signed in to change notification settings - Fork 813
perf(webapp): Add BatchTaskRun index to speed up the batch list dashboard page #2499
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
WalkthroughAdds a new PostgreSQL migration to create an idempotent concurrent index on BatchTaskRun over columns runtimeEnvironmentId and id (DESC), named BatchTaskRun_runtimeEnvironmentId_id_idx. Updates Prisma schema: removes the existing index on dependentTaskAttemptId and introduces a composite index on [runtimeEnvironmentId, id(sort: Desc)] with a comment indicating it supports the batch list dashboard page. Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks (2 passed, 1 inconclusive)❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal-packages/database/prisma/schema.prisma (2)
1581-1583
: Pin Prisma index name to the manual SQL index to prevent drift.Add a map so Prisma keeps using
BatchTaskRun_runtimeEnvironmentId_id_idx
and never tries to recreate/rename it.- @@index([runtimeEnvironmentId, id(sort: Desc)]) + @@index([runtimeEnvironmentId, id(sort: Desc)], map: "BatchTaskRun_runtimeEnvironmentId_id_idx")
1581-1583
: Clarify intent re: dependentTaskAttemptId index.AI summary says it was removed, but it still exists in the model. Keep it if queries rely on it; otherwise consider dropping to save write cost.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal-packages/database/prisma/migrations/20250912155941_add_batch_task_run_dashboard_index/migration.sql
(1 hunks)internal-packages/database/prisma/schema.prisma
(1 hunks)
⏰ 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). (23)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
internal-packages/database/prisma/migrations/20250912155941_add_batch_task_run_dashboard_index/migration.sql (1)
1-2
: LGTM — single CONCURRENTLY index will run non-transactionally.migration file contains only the single CREATE INDEX CONCURRENTLY statement (internal-packages/database/prisma/migrations/20250912155941_add_batch_task_run_dashboard_index/migration.sql); Prisma Migrate does not wrap PostgreSQL migrations in a transaction by default, so a single-statement CONCURRENTLY index is applied outside a transaction. (prisma.io)
Caveat: Prisma has historically wrapped/mishandled multi-statement migrations in some versions (causing CREATE INDEX CONCURRENTLY to fail) — keep this migration single-statement (no extra SET/BEGIN/COMMIT) and ensure CI uses the pinned prisma version in package.json (devDependency prisma: 6.14.0). (github.com)
internal-packages/database/prisma/schema.prisma (1)
1581-1583
: Cannot verify — confirm dashboard query uses WHERE runtimeEnvironmentId = ? and ORDER BY id DESC; run EXPLAIN on the exact query.Repository search returned no matches for prisma.batchTaskRun.findMany (the provided ripgrep commands produced no output), so I couldn't locate the batch list query to confirm ordering/filtering. If the UI orders by createdAt or filters by status, the current index won’t be used; provide the exact query or EXPLAIN output.
No description provided.