fix(webapp): scan every run-ops store for the batches list - #4806
Conversation
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe batch list route passes configured non-aliased Gen-2 shard replicas to Merge Risk: 🔵 Low · up to Batch pages now query every configured owning shard and fail if one is unavailable, preventing silent omissions but expanding page availability dependencies and potentially adding read load to shard writers without replicas. The PR is mergeable with owner awareness and follow-up monitoring for shard latency, failures, and primary load. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description explains the problem, implementation, testing, merge precedence, failure behavior, and boot interlocks. It does not include the template's issue reference, checklist, changelog, or screenshots sections, but the core information is complete.
✨ Finishing Touches 💡 1📝 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/webapp/app/presenters/v3/BatchListPresenter.server.ts (1)
113-136: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winReject configured shards when split reads are disabled
RUN_OPS_SHARDSis parsed independently ofRUN_OPS_SPLIT_ENABLED, and validation allows this combination.selectRunOpsTopology()returns an emptyshardsmap whensplitEnabledis false. The batch presenter then skips shard scans and can omit existing gen-2 batches.Reject this combination at boot.
🧹 Nitpick comments (1)
apps/webapp/test/batchListPresenter.readroute.test.ts (1)
19-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove
nonAliasedShardReplicasto a non-server module.
shardHandles.server.tsimportsrunOpsShardHandlesfrom~/db.server, which imports~/env.serverand resolves shard handles during module initialization. This test therefore indirectly importsenv.server.ts, which violates the webapp test guideline. Re-export the pure helper fromshardHandles.server.ts.Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 573413b1-1ef6-46e7-a975-b1093faa9802
📒 Files selected for processing (5)
apps/webapp/app/presenters/v3/BatchListPresenter.server.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.batches/route.tsxapps/webapp/app/v3/runOpsMigration/shardHandles.server.test.tsapps/webapp/app/v3/runOpsMigration/shardHandles.server.tsapps/webapp/test/batchListPresenter.readroute.test.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.
📜 Review details
⏰ Context from checks skipped due to timeout. (33)
- GitHub Check: report
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (14, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (24, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (11, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (22, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (5, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (6, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (18, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (17, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (8, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (23, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (20, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (15, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (7, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (19, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (10, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (13, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (9, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (12, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (21, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (16, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (3, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (2, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (4, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (1, 24)
- GitHub Check: obsmap / 🧪 Unit Tests: Observability Map
- GitHub Check: e2e-webapp / 🧪 E2E Tests: Webapp (1, 2)
- GitHub Check: e2e-webapp / 🧪 E2E Tests: Webapp (2, 2)
- GitHub Check: runops-guard / runops-guard
- GitHub Check: typecheck / typecheck
- GitHub Check: fk-cascade-guard / fk-cascade-guard
- GitHub Check: code-quality / code-quality
- GitHub Check: Analyze (javascript-typescript)
🧰 Additional context used
📓 Path-based instructions (14)
New code must target Run Engine V2 through the singleton in `app/v3/runEngine.server.ts`; do not reintroduce V1 execution paths. V1 branches may only reject or finalize gracefully with a clean 4xx.
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
Files:
apps/webapp/app/v3/runOpsMigration/shardHandles.server.tsapps/webapp/app/v3/runOpsMigration/shardHandles.server.test.ts
Never use `request.signal` to detect client disconnects. Use `getRequestAbortSignal()` from `app/services/httpAsyncStorage.server.ts`, which is wired to Express response close events.
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
Files:
apps/webapp/app/presenters/v3/BatchListPresenter.server.tsapps/webapp/app/v3/runOpsMigration/shardHandles.server.tsapps/webapp/app/v3/runOpsMigration/shardHandles.server.test.ts
We use vitest exclusively. **Never mock anything** - use testcontainers instead.
📄 CodeRabbit inference engine (AGENTS.md)
Files:
apps/webapp/test/batchListPresenter.readroute.test.tsapps/webapp/app/v3/runOpsMigration/shardHandles.server.test.ts
Test files must not import `app/env.server.ts`; pass configuration as options instead.
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
Files:
apps/webapp/test/batchListPresenter.readroute.test.tsapps/webapp/app/v3/runOpsMigration/shardHandles.server.test.ts
For dashboard changes, visually verify the running Remix app with Chrome DevTools MCP, using snapshots, screenshots, interaction, and console-message checks as appropriate.
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
Files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.batches/route.tsxapps/webapp/app/presenters/v3/BatchListPresenter.server.tsapps/webapp/app/v3/runOpsMigration/shardHandles.server.tsapps/webapp/app/v3/runOpsMigration/shardHandles.server.test.ts
**Prefer static imports over dynamic imports.** Only use dynamic `import()` when:
📄 CodeRabbit inference engine (AGENTS.md)
Files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.batches/route.tsxapps/webapp/app/presenters/v3/BatchListPresenter.server.tsapps/webapp/app/v3/runOpsMigration/shardHandles.server.tsapps/webapp/test/batchListPresenter.readroute.test.tsapps/webapp/app/v3/runOpsMigration/shardHandles.server.test.ts
Add crumbs as you write code — not just when debugging. Mark lines with
📄 CodeRabbit inference engine (AGENTS.md)
Files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.batches/route.tsxapps/webapp/app/presenters/v3/BatchListPresenter.server.tsapps/webapp/app/v3/runOpsMigration/shardHandles.server.tsapps/webapp/test/batchListPresenter.readroute.test.tsapps/webapp/app/v3/runOpsMigration/shardHandles.server.test.ts
Use zod for validation in packages/core and apps/webapp
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.batches/route.tsxapps/webapp/app/presenters/v3/BatchListPresenter.server.tsapps/webapp/app/v3/runOpsMigration/shardHandles.server.tsapps/webapp/test/batchListPresenter.readroute.test.tsapps/webapp/app/v3/runOpsMigration/shardHandles.server.test.ts
Do not import `env.server.ts` directly or indirectly into test files; instead pass environment-dependent values through options/parameters to make code testable
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Files:
apps/webapp/test/batchListPresenter.readroute.test.tsapps/webapp/app/v3/runOpsMigration/shardHandles.server.test.ts
Access environment variables through the `env` export of `env.server.ts` instead of directly accessing `process.env`
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.batches/route.tsxapps/webapp/app/presenters/v3/BatchListPresenter.server.tsapps/webapp/app/v3/runOpsMigration/shardHandles.server.tsapps/webapp/test/batchListPresenter.readroute.test.tsapps/webapp/app/v3/runOpsMigration/shardHandles.server.test.ts
Use vitest for all tests in the Trigger.dev repository
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/test/batchListPresenter.readroute.test.tsapps/webapp/app/v3/runOpsMigration/shardHandles.server.test.ts
Use function declarations instead of default exports
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.batches/route.tsxapps/webapp/app/presenters/v3/BatchListPresenter.server.tsapps/webapp/app/v3/runOpsMigration/shardHandles.server.tsapps/webapp/test/batchListPresenter.readroute.test.tsapps/webapp/app/v3/runOpsMigration/shardHandles.server.test.ts
Use types over interfaces for TypeScript
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.batches/route.tsxapps/webapp/app/presenters/v3/BatchListPresenter.server.tsapps/webapp/app/v3/runOpsMigration/shardHandles.server.tsapps/webapp/test/batchListPresenter.readroute.test.tsapps/webapp/app/v3/runOpsMigration/shardHandles.server.test.ts
When creating or editing OTEL metrics (counters, histograms, gauges), ensure metric attributes have low cardinality by using only enums, booleans, bounded error codes, or bounded shard IDs
📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc)
Files:
apps/webapp/app/presenters/v3/BatchListPresenter.server.tsapps/webapp/app/v3/runOpsMigration/shardHandles.server.tsapps/webapp/test/batchListPresenter.readroute.test.tsapps/webapp/app/v3/runOpsMigration/shardHandles.server.test.ts
🧠 Learnings (2)
📚 Learning: 2026-06-16T09:19:47.637Z
Learnt from: d-cs
Repo: triggerdotdev/trigger.dev PR: 3960
File: apps/webapp/test/prismaInfrastructureErrorCapture.test.ts:0-0
Timestamp: 2026-06-16T09:19:47.637Z
Learning: In this repo’s Vitest setup, `vitest.config.ts` uses `globals: true`, so identifiers like `vi`, `describe`, `it`, and `expect` are available as globals in Vitest test files. During code review, do not flag missing `vi`/`describe`/`it`/`expect` imports as a runtime error or correctness issue when they’re used in `*.test.ts/tsx` or `*.spec.ts/tsx` files. Explicit imports are still preferred for consistency, but they’re not required for runtime behavior.
Applied to files:
apps/webapp/test/batchListPresenter.readroute.test.ts
📚 Learning: 2026-05-12T21:04:05.815Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3542
File: apps/webapp/app/components/sessions/v1/SessionStatus.tsx:1-3
Timestamp: 2026-05-12T21:04:05.815Z
Learning: In this Remix + TypeScript codebase, do not flag a server/client boundary violation when a file imports only types from a module matching `*.server`.
Specifically, it’s safe to import types using `import type { Foo } from "*.server"` or `import { type Foo } from "*.server"` because TypeScript erases type-only imports at compile time and they emit no JavaScript, so they won’t cross the Remix server/client bundle boundary.
Only raise the boundary concern for value imports (e.g., `import { Foo }` without `type`, or `import Foo`), since those produce JavaScript output.
Applied to files:
apps/webapp/test/batchListPresenter.readroute.test.ts
🔇 Additional comments (7)
apps/webapp/app/v3/runOpsMigration/shardHandles.server.ts (3)
13-18: LGTM!Also applies to: 28-29
46-68: LGTM!
70-77: LGTM!apps/webapp/app/v3/runOpsMigration/shardHandles.server.test.ts (1)
2-2: LGTM!Also applies to: 38-62
apps/webapp/app/presenters/v3/BatchListPresenter.server.ts (1)
78-85: LGTM!Also applies to: 117-160, 176-177, 204-226
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.batches/route.tsx (1)
59-59: LGTM!Also applies to: 108-108
apps/webapp/test/batchListPresenter.readroute.test.ts (1)
169-192: LGTM!Also applies to: 628-894
Observability mapAs of Nothing in this pull request moves the report any more. The findings an earlier push reported are gone. The score and findings here are report-only and never gate the merge. Separately, a required test suite keeps this tool's symbol and route lists in sync with the code they name, and can fail a pull request that renames or removes a symbol they reference, or that adds the first route with a segment they anticipate. Each failure names the list to edit. The rules and their reasons: internal-packages/observability-map/README.md. |
b6f3886 to
530a352
Compare
@trigger.dev/build
trigger.dev
@trigger.dev/core
@trigger.dev/python
@trigger.dev/react-hooks
@trigger.dev/redis-worker
@trigger.dev/rsc
@trigger.dev/schema-to-json
@trigger.dev/sdk
commit: |
|
Thanks, both checked against the code. Shards configured while split reads are disabled. The mechanism is accurate: I don't think it's a defect this PR introduces, though. On that same arm A boot-time interlock rejecting the combination sounds reasonable, but it changes startup behaviour and belongs with the config and topology validation rather than a read-path fix, so I'd rather it were raised there than bolted on here. This PR deliberately leaves the single-database passthrough arm untouched. Moving |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Fixed rather than deferred, in You were right that the combination should be rejected at boot. What made it worth closing here is that the silence is specific: the other two ways the split ends up disabled, a missing URL and a sentinel that cannot confirm distinct databases, already refuse to start. Only the flag-off case slipped through, because
On the second point, moving the helper out of the |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/webapp/app/v3/runOpsMigration/splitMode.server.ts (1)
81-95: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd temporary crumbs for the changed shard interlock flow.
Add a
//@Crumbsmarker or a `// `#region` `@crumbsblock while developing this behavior. Strip the instrumentation withagentcrumbs stripbefore merge.
apps/webapp/app/v3/runOpsMigration/splitMode.server.ts#L81-L95: mark the new assertion path.apps/webapp/app/db.server.ts#L621-L626: mark the boot-time assertion call.apps/webapp/test/runOpsSplitMode.test.ts#L159-L186: mark the new test path.As per coding guidelines, “Add crumbs as you write code — not just when debugging.”
Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d770bf91-336c-4911-8caf-0d1069f6814e
📒 Files selected for processing (3)
apps/webapp/app/db.server.tsapps/webapp/app/v3/runOpsMigration/splitMode.server.tsapps/webapp/test/runOpsSplitMode.test.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review.
📜 Review details
⏰ Context from checks skipped due to timeout. (35)
- GitHub Check: report
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (24, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (19, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (17, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (21, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (13, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (16, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (20, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (22, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (15, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (10, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (23, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (7, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (14, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (3, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (18, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (4, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (11, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (5, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (1, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (8, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (2, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (9, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (12, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (6, 24)
- GitHub Check: runops-guard / runops-guard
- GitHub Check: fk-cascade-guard / fk-cascade-guard
- GitHub Check: e2e-webapp / 🧪 E2E Tests: Webapp (2, 2)
- GitHub Check: typecheck / typecheck
- GitHub Check: e2e-webapp / 🧪 E2E Tests: Webapp (1, 2)
- GitHub Check: obsmap / 🧪 Unit Tests: Observability Map
- GitHub Check: code-quality / code-quality
- GitHub Check: audit
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: audit
🧰 Additional context used
📓 Path-based instructions (14)
New code must target Run Engine V2 through the singleton in `app/v3/runEngine.server.ts`; do not reintroduce V1 execution paths. V1 branches may only reject or finalize gracefully with a clean 4xx.
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
Files:
apps/webapp/app/v3/runOpsMigration/splitMode.server.ts
Never use `request.signal` to detect client disconnects. Use `getRequestAbortSignal()` from `app/services/httpAsyncStorage.server.ts`, which is wired to Express response close events.
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
Files:
apps/webapp/app/v3/runOpsMigration/splitMode.server.tsapps/webapp/app/db.server.ts
We use vitest exclusively. **Never mock anything** - use testcontainers instead.
📄 CodeRabbit inference engine (AGENTS.md)
Files:
apps/webapp/test/runOpsSplitMode.test.ts
Test files must not import `app/env.server.ts`; pass configuration as options instead.
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
Files:
apps/webapp/test/runOpsSplitMode.test.ts
For dashboard changes, visually verify the running Remix app with Chrome DevTools MCP, using snapshots, screenshots, interaction, and console-message checks as appropriate.
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
Files:
apps/webapp/app/v3/runOpsMigration/splitMode.server.tsapps/webapp/app/db.server.ts
**Prefer static imports over dynamic imports.** Only use dynamic `import()` when:
📄 CodeRabbit inference engine (AGENTS.md)
Files:
apps/webapp/test/runOpsSplitMode.test.tsapps/webapp/app/v3/runOpsMigration/splitMode.server.tsapps/webapp/app/db.server.ts
Add crumbs as you write code — not just when debugging. Mark lines with
📄 CodeRabbit inference engine (AGENTS.md)
Files:
apps/webapp/test/runOpsSplitMode.test.tsapps/webapp/app/v3/runOpsMigration/splitMode.server.tsapps/webapp/app/db.server.ts
Use zod for validation in packages/core and apps/webapp
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/test/runOpsSplitMode.test.tsapps/webapp/app/v3/runOpsMigration/splitMode.server.tsapps/webapp/app/db.server.ts
Do not import `env.server.ts` directly or indirectly into test files; instead pass environment-dependent values through options/parameters to make code testable
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Files:
apps/webapp/test/runOpsSplitMode.test.ts
Access environment variables through the `env` export of `env.server.ts` instead of directly accessing `process.env`
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Files:
apps/webapp/test/runOpsSplitMode.test.tsapps/webapp/app/v3/runOpsMigration/splitMode.server.tsapps/webapp/app/db.server.ts
Use vitest for all tests in the Trigger.dev repository
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/test/runOpsSplitMode.test.ts
Use function declarations instead of default exports
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/test/runOpsSplitMode.test.tsapps/webapp/app/v3/runOpsMigration/splitMode.server.tsapps/webapp/app/db.server.ts
Use types over interfaces for TypeScript
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/test/runOpsSplitMode.test.tsapps/webapp/app/v3/runOpsMigration/splitMode.server.tsapps/webapp/app/db.server.ts
When creating or editing OTEL metrics (counters, histograms, gauges), ensure metric attributes have low cardinality by using only enums, booleans, bounded error codes, or bounded shard IDs
📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc)
Files:
apps/webapp/test/runOpsSplitMode.test.tsapps/webapp/app/v3/runOpsMigration/splitMode.server.tsapps/webapp/app/db.server.ts
🧠 Learnings (1)
📚 Learning: 2026-06-04T18:16:35.386Z
Learnt from: nicktrn
Repo: triggerdotdev/trigger.dev PR: 3836
File: apps/supervisor/src/backpressure/backpressureMonitor.ts:3-5
Timestamp: 2026-06-04T18:16:35.386Z
Learning: When reviewing TypeScript in this repo, apply the rule “prefer type aliases over interfaces” only to data/object shapes and union/intersection type modeling. If an interface is being used as a behavioral contract for collaborators to implement (e.g., method-shape interfaces that define required behavior, such as `BackpressureLogger` / `BackpressureSignalSource` in `apps/supervisor/src/backpressure/backpressureMonitor.ts`), keep it as an `interface` and do not flag it as a type-alias-vs-interface violation.
Applied to files:
apps/webapp/app/v3/runOpsMigration/splitMode.server.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/webapp/app/db.server.ts (1)
623-626: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd required
@crumbsinstrumentation to the new code paths.
apps/webapp/app/db.server.ts#L623-L626: add a crumb for the boot-time shard interlock.apps/webapp/app/v3/runOpsMigration/splitMode.server.ts#L82-L105: add crumbs for owning-shard filtering and rejection.apps/webapp/test/runOpsSplitMode.test.ts#L160-L208: add crumbs for the new structured descriptor cases.As per coding guidelines: “Add crumbs as you write code — not just when debugging.”
Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6d1ec123-e5c6-46e6-8d0f-1cd78e808501
📒 Files selected for processing (3)
apps/webapp/app/db.server.tsapps/webapp/app/v3/runOpsMigration/splitMode.server.tsapps/webapp/test/runOpsSplitMode.test.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review.
📜 Review details
⏰ Context from checks skipped due to timeout. (34)
- GitHub Check: report
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (14, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (17, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (3, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (19, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (15, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (24, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (12, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (23, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (16, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (7, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (11, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (21, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (4, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (10, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (13, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (8, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (18, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (6, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (22, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (5, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (9, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (20, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (2, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (1, 24)
- GitHub Check: fk-cascade-guard / fk-cascade-guard
- GitHub Check: e2e-webapp / 🧪 E2E Tests: Webapp (1, 2)
- GitHub Check: typecheck / typecheck
- GitHub Check: runops-guard / runops-guard
- GitHub Check: e2e-webapp / 🧪 E2E Tests: Webapp (2, 2)
- GitHub Check: obsmap / 🧪 Unit Tests: Observability Map
- GitHub Check: code-quality / code-quality
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: audit
🧰 Additional context used
📓 Path-based instructions (14)
New code must target Run Engine V2 through the singleton in `app/v3/runEngine.server.ts`; do not reintroduce V1 execution paths. V1 branches may only reject or finalize gracefully with a clean 4xx.
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
Files:
apps/webapp/app/v3/runOpsMigration/splitMode.server.ts
Never use `request.signal` to detect client disconnects. Use `getRequestAbortSignal()` from `app/services/httpAsyncStorage.server.ts`, which is wired to Express response close events.
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
Files:
apps/webapp/app/db.server.tsapps/webapp/app/v3/runOpsMigration/splitMode.server.ts
We use vitest exclusively. **Never mock anything** - use testcontainers instead.
📄 CodeRabbit inference engine (AGENTS.md)
Files:
apps/webapp/test/runOpsSplitMode.test.ts
Test files must not import `app/env.server.ts`; pass configuration as options instead.
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
Files:
apps/webapp/test/runOpsSplitMode.test.ts
For dashboard changes, visually verify the running Remix app with Chrome DevTools MCP, using snapshots, screenshots, interaction, and console-message checks as appropriate.
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
Files:
apps/webapp/app/db.server.tsapps/webapp/app/v3/runOpsMigration/splitMode.server.ts
**Prefer static imports over dynamic imports.** Only use dynamic `import()` when:
📄 CodeRabbit inference engine (AGENTS.md)
Files:
apps/webapp/test/runOpsSplitMode.test.tsapps/webapp/app/db.server.tsapps/webapp/app/v3/runOpsMigration/splitMode.server.ts
Add crumbs as you write code — not just when debugging. Mark lines with
📄 CodeRabbit inference engine (AGENTS.md)
Files:
apps/webapp/test/runOpsSplitMode.test.tsapps/webapp/app/db.server.tsapps/webapp/app/v3/runOpsMigration/splitMode.server.ts
Use zod for validation in packages/core and apps/webapp
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/test/runOpsSplitMode.test.tsapps/webapp/app/db.server.tsapps/webapp/app/v3/runOpsMigration/splitMode.server.ts
Do not import `env.server.ts` directly or indirectly into test files; instead pass environment-dependent values through options/parameters to make code testable
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Files:
apps/webapp/test/runOpsSplitMode.test.ts
Access environment variables through the `env` export of `env.server.ts` instead of directly accessing `process.env`
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Files:
apps/webapp/test/runOpsSplitMode.test.tsapps/webapp/app/db.server.tsapps/webapp/app/v3/runOpsMigration/splitMode.server.ts
Use vitest for all tests in the Trigger.dev repository
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/test/runOpsSplitMode.test.ts
Use function declarations instead of default exports
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/test/runOpsSplitMode.test.tsapps/webapp/app/db.server.tsapps/webapp/app/v3/runOpsMigration/splitMode.server.ts
Use types over interfaces for TypeScript
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/test/runOpsSplitMode.test.tsapps/webapp/app/db.server.tsapps/webapp/app/v3/runOpsMigration/splitMode.server.ts
When creating or editing OTEL metrics (counters, histograms, gauges), ensure metric attributes have low cardinality by using only enums, booleans, bounded error codes, or bounded shard IDs
📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc)
Files:
apps/webapp/test/runOpsSplitMode.test.tsapps/webapp/app/db.server.tsapps/webapp/app/v3/runOpsMigration/splitMode.server.ts
🧠 Learnings (3)
📚 Learning: 2026-06-16T09:19:47.637Z
Learnt from: d-cs
Repo: triggerdotdev/trigger.dev PR: 3960
File: apps/webapp/test/prismaInfrastructureErrorCapture.test.ts:0-0
Timestamp: 2026-06-16T09:19:47.637Z
Learning: In this repo’s Vitest setup, `vitest.config.ts` uses `globals: true`, so identifiers like `vi`, `describe`, `it`, and `expect` are available as globals in Vitest test files. During code review, do not flag missing `vi`/`describe`/`it`/`expect` imports as a runtime error or correctness issue when they’re used in `*.test.ts/tsx` or `*.spec.ts/tsx` files. Explicit imports are still preferred for consistency, but they’re not required for runtime behavior.
Applied to files:
apps/webapp/test/runOpsSplitMode.test.ts
📚 Learning: 2026-06-04T18:16:35.386Z
Learnt from: nicktrn
Repo: triggerdotdev/trigger.dev PR: 3836
File: apps/supervisor/src/backpressure/backpressureMonitor.ts:3-5
Timestamp: 2026-06-04T18:16:35.386Z
Learning: When reviewing TypeScript in this repo, apply the rule “prefer type aliases over interfaces” only to data/object shapes and union/intersection type modeling. If an interface is being used as a behavioral contract for collaborators to implement (e.g., method-shape interfaces that define required behavior, such as `BackpressureLogger` / `BackpressureSignalSource` in `apps/supervisor/src/backpressure/backpressureMonitor.ts`), keep it as an `interface` and do not flag it as a type-alias-vs-interface violation.
Applied to files:
apps/webapp/app/v3/runOpsMigration/splitMode.server.ts
📚 Learning: 2026-05-12T21:04:05.815Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3542
File: apps/webapp/app/components/sessions/v1/SessionStatus.tsx:1-3
Timestamp: 2026-05-12T21:04:05.815Z
Learning: In this Remix + TypeScript codebase, do not flag a server/client boundary violation when a file imports only types from a module matching `*.server`.
Specifically, it’s safe to import types using `import type { Foo } from "*.server"` or `import { type Foo } from "*.server"` because TypeScript erases type-only imports at compile time and they emit no JavaScript, so they won’t cross the Remix server/client bundle boundary.
Only raise the boundary concern for value imports (e.g., `import { Foo }` without `type`, or `import Foo`), since those produce JavaScript output.
Applied to files:
apps/webapp/app/v3/runOpsMigration/splitMode.server.ts
🔇 Additional comments (3)
apps/webapp/app/db.server.ts (1)
31-31: LGTM!apps/webapp/app/v3/runOpsMigration/splitMode.server.ts (1)
10-14: LGTM!apps/webapp/test/runOpsSplitMode.test.ts (1)
6-6: LGTM!
The batches list read exactly two databases, so a batch living on any other configured run-ops store was missing from the page with no error and nothing logged. The scan now covers one leg per store, in ascending precedence order, and the existing keyset merge generalises unchanged: every leg runs the same query, so the merged first page is still the true first page. The empty-state check keeps its existing pair and issues the rest in one round trip. A store that declares itself an alias of another shares its client by reference and contributes no leg, matching how the routing store and the boot sentinels treat one. With no extra store configured the page is byte-identical to today.
The merge was only ever exercised as a union, with each id on exactly one store, so nothing caught a leg-order regression: a union is insensitive to the order its legs are applied in. Seed one id on two stores and assert the higher-authority copy is the one the page shows. Verified by mutation: making the lower-authority insert unconditional, and applying the legs in the wrong order, both fail it. Also corrects a comment that justified the leg order with a claim about duplicate ids being impossible. A single writer routes creates by id shape, but a row can still sit on two stores while data is moved between them, which is why the routing store dedupes batches without alarming.
The precedence test now guards the leg order that one of them warned about.
…ut the split Shard clients are only built on the split-on arm of the topology selector, so a shard configured while the split flag is off was dropped in silence: no client, no fan-out leg, and any row already resident on that database missing from every list with no error. The other two ways the split ends up disabled, a missing URL and a sentinel that cannot confirm distinct databases, already refuse to boot. This closes the one that did not, and names the dropped shards so the misconfiguration is obvious.
An aliased shard owns no database: it shares its target's client by reference, so its rows are still read with the split off and nothing is dropped. Feeding every configured key to the interlock refused an alias-only config for no reason. The exemption now lives inside the interlock rather than at its call site, so it takes the raw descriptors and no caller can forget to apply it. This matches where the distinctness sentinel, the coresidency loop and replication already draw the line.
74d3b05 to
6973ca5
Compare
The probe fails closed, so one store being briefly unreachable collapsed the deployment to single-DB and the boot interlock then refused the boot. With more than two stores configured that turns a transient blip on any one of them into a fleet-wide startup failure. Each target now gets a bounded number of attempts with a short backoff before the probe gives up. Failing closed is unchanged once the budget is exhausted: "distinct" stays a positive claim a failed probe cannot support. A genuine duplicate is a final answer and is never retried, so a misconfigured deployment still fails on the first pass. Retries are per target, so one slow store does not re-probe the stores that already answered.
2 participants
Summary
Batches created on a run-ops store other than the two the list reads were missing from the Batches page. No error, nothing logged: the page just showed fewer batches than exist. This is only reachable once additional run-ops stores are configured, so nothing changes for anyone today.
Fix
The list scanned exactly two databases and merged them by keyset. It now covers one leg per configured store, in ascending precedence order, all issued together.
The existing keyset merge generalises without change. Every leg runs the same query, with the same cursor predicate, ordering and over-fetch, so a row's rank within its own leg is never worse than its global rank, and the merged first page is still the true first page. That argument holds for any number of legs, not just two.
The empty-state check keeps its existing sequential pair, since a project with no batches is the common case for that path, then issues the remaining checks in a single round trip.
A store that declares itself an alias of another shares its client by reference, so it contributes no leg. Scanning it would query the same database twice for rows the other leg already returned. This matches how the routing store and the boot checks treat an alias.
The fan-out deliberately fails the page if any store is unreachable, rather than returning a short page. A tolerant merge would recreate the same silent absence this change removes, with a wider blast radius.
Verification
Covered by container tests against real databases: gen-1, legacy and additional stores merged into one ordered page, paging forward and back across a boundary that spans stores, and the empty-state check.
Also verified end to end against a live environment with a real corpus: the missing rows reproduce with the new leg removed and appear correctly with it present, ordering interleaves across stores as expected, paging across a store boundary loses and repeats nothing, and the page is byte-identical to before when no extra store is configured.
Merge precedence is pinned by its own test: one id seeded on two stores, asserting the higher-authority copy is the one shown. Verified by mutation, since a union-only test passes regardless of leg order.
Boot interlocks
Two related boot checks changed alongside the read path, since configuring an extra store is what makes them reachable.
A store configured while split reads are disabled is dropped in silence: no client is built, no leg is added, and rows already resident there disappear from every list with no error. The other two ways the split ends up disabled already refuse to start; this closes the one that did not, and names the stores it is refusing. A store that declares itself an alias of another owns no database, so it is exempt.
The distinct-database probe fails closed, which meant one store being briefly unreachable collapsed the deployment to single-DB and then refused the boot entirely. Each target now gets a bounded number of attempts with a short backoff before the probe gives up. Failing closed is unchanged once that budget is exhausted, and a genuine duplicate is still a final answer that is never retried.