Skip to content

Conversation

ericallam
Copy link
Member

No description provided.

Copy link

changeset-bot bot commented Sep 12, 2025

⚠️ No Changeset found

Latest commit: b915a84

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Contributor

coderabbitai bot commented Sep 12, 2025

Walkthrough

Adds 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)
Check name Status Explanation Resolution
Description Check ❓ Inconclusive I cannot validate the pull request description because neither the PR's current description text nor the repository's required description_template were provided for comparison; the supplied raw_summary details code changes but is not the PR description/template needed to confirm conformity. Please provide the pull request's current description and the repository's description_template (or update the PR to include the template sections such as summary, motivation, tests, and impact) so I can re-evaluate the description for completeness and conformity.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The title succinctly and accurately summarizes the primary change: adding a BatchTaskRun index to improve performance for the batch list dashboard page, and it is concise, specific, and relevant to the changeset.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch ea-branch-85

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

📥 Commits

Reviewing files that changed from the base of the PR and between 10e7985 and b915a84.

📒 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.

@matt-aitken matt-aitken merged commit 3188dc9 into main Sep 12, 2025
31 checks passed
@matt-aitken matt-aitken deleted the ea-branch-85 branch September 12, 2025 16:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants