fix(webapp): scan every run-ops store for the batches list - #4806
Conversation
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.
|
WalkthroughThe batch list route now passes configured non-aliased Gen-2 shard replicas to Merge Risk: 🟡 Moderate · up to The change makes batch listings query every configured store, but a supported configuration can still omit gen-2 batches when split reads are disabled, and any unavailable store can now fail the entire listing while backend work grows with shard count. Merge should wait for the configuration correctness issue to be fixed or explicitly accepted. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description provides a detailed summary, implementation rationale, testing evidence, failure behavior, and known follow-ups. It does not include the template's issue link, 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
1 participant
WIP. Opening early for visibility; two small follow-ups still to land (see below).
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.
Still to land
Note for CI
gen2MintInertness.test.tscurrently fails onmainand this branch inherits it. It is unrelated to this change and reproduces on a cleanmaincheckout.