feat(webapp,run-store): route run-graph reads and writes through the run-store router#4237
Open
d-cs wants to merge 15 commits into
Open
feat(webapp,run-store): route run-graph reads and writes through the run-store router#4237d-cs wants to merge 15 commits into
d-cs wants to merge 15 commits into
Conversation
Add a type-aware guard (apps/webapp/scripts/runOpsLegacyGuard.ts, wired as `pnpm --filter webapp run guard:runops-legacy` with a checked-in baseline as the CI gate) that flags any run-graph table access made through the control-plane Prisma client. Reroute all existing sites through the run-store router or the dedicated run-ops clients, and add finalizeRun, findManyBatchTaskRunItems, findBatchTaskRunItem, upsertWaitpointTag, and findManyWaitpointTags to RunStore.
- Correct the WaitpointTag residency comments: writes are single-homed on the legacy database today (no tag id is minted), not routed by mint-kind. - cancelAttempt: read the attempt on the owning store's primary (read-your-writes) instead of a replica, matching the other attempt services. - Guard findManyBatchTaskRunItems against an unrouted full-table scan when neither batchTaskRunId nor taskRunId is provided. - Log when the deprecated non-v3 batch-item resume path updates no rows.
Build the legacy run-ops Prisma client from RUN_OPS_LEGACY_DATABASE_URL (and a new optional RUN_OPS_LEGACY_DATABASE_READ_REPLICA_URL) instead of aliasing the control-plane client when the split is enabled. Single-DB and split-without-URLs installs still alias and open no new connection, so their boot is unchanged. Add an advisory boot sentinel that reports whether the legacy and control-plane databases are co-resident (metric run_ops_legacy_control_plane_coresident; degrades to "unknown" if the fingerprint query is denied), with opt-in hard enforcement via RUN_OPS_EXPECT_CONTROL_PLANE_SPLIT. Add a three-database topology test.
- Create the co-residency advisory counter lazily on first emit so it binds to the registered meter provider instead of the no-op provider (this module loads before the tracer registers metrics, so a module-load counter never emitted). - Add unit tests for the co-residency sentinel: the enforcement matrix plus the never-crash-on-probe-error and never-enforce-on-"unknown" guarantees. - Warn at boot when the split is enabled without a legacy read-replica URL, since legacy reads would otherwise silently fall back to the legacy primary.
Stop forwarding a caller's transaction into the legacy run-ops store: routed run-graph writes now run on their own store's client, so a legacy write can never execute against the control-plane connection. No production path relied on cross-graph write atomicity, matching the semantics the dedicated run-ops path has always had. Drop the remaining cross-graph foreign-key constraints between run-graph tables and control-plane tables. Give the runs-legacy and sessions logical-replication slots their own connection strings (RUN_REPLICATION_LEGACY_DATABASE_URL / SESSION_REPLICATION_DATABASE_URL, each falling back to DATABASE_URL), route the admin replication endpoint through the same chain, and add an optional dedicated migrate target (RUN_OPS_LEGACY_DIRECT_URL) so the legacy run-ops database keeps its schema current.
Wrap the legacy run-ops migrate command in a trace-suppressed subshell so `set -x` no longer prints the connection string (with credentials) to container logs. Also correct a now-stale comment that described the removed transaction-forwarding behavior.
…ntrol-plane replica Four read-through call sites passed the control-plane replica as the `legacyReplica` client (batch results, run results, waitpoint token, and span waitpoint endpoints), so legacy-resident reads hit the control-plane database instead of the legacy run-ops database. Harmless while the two share one connection, but returns 404/not-found once the legacy database is pointed at its own instance. Route them through the legacy run-ops replica like the rest of the read-through wiring.
…ough slots Add detector (iii) to the run-ops legacy guard: flag any read-through config slot (newClient/legacyReplica/runOpsNew/runOpsLegacyReplica/newReplica) assigned a control-plane client ($replica/prisma/this._replica/this._prisma). The type-aware detectors cannot catch this — the slot is typed as a run-ops client but the control-plane client is coerced in — which is how the legacy-replica misroute reached several endpoints. controlPlaneReplica is intentionally exempt. Adds self-test fixtures; the real code is clean so the baseline is unchanged.
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📜 Recent review details⏰ Context from checks skipped due to timeout. (23)
WalkthroughThe pull request separates legacy run-ops database clients from control-plane clients and adds environment configuration, topology checks, and co-residency enforcement. RunStore gains residency-aware finalization, batch-item, and waitpoint-tag operations while routed writes omit caller transactions. Presenters, services, and routes migrate run-graph reads to RunStore. Replication DSN fallbacks, legacy migrations, foreign-key removal, static guarding, three-database fixtures, and routing tests are added or updated. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 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}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
The read-through presenter tests injected read clients the old way, so once those reads moved onto the run store they fell through to the default database and only passed where one happened to be reachable. Finish the run-store injection seam on the affected presenters (default unchanged, so behavior is identical) and wire each test to its own test-container stores.
…e, and edge cleanup - Skip the legacy attempt-create when a run-ops-resident run reaches the no-attempt branch during finalization; it is already finalized on its own database, so minting a legacy attempt would orphan a row on the wrong one. - Cancel and finalize the same run by taking the run id from the fetched attempt rather than a caller-supplied id. - When resuming dependent parents, read the owning primary for the run and attempts just finalized in the same operation so replica lag cannot skip a resume, and stop when the environment cannot be resolved instead of treating it as production. - Delete run/waitpoint edges from both stores so a cross-database edge cannot leave a run blocked. - Drop a redundant read client on the bulk-action source-run lookup so it uses the residency-routed replica.
Collaborator
Author
|
Addressing the two outside-diff items from the review:
The two heavier items (batch-item durable scheduling, and waitpoint-tag |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/webapp/test/waitpointListPresenter.readroute.test.ts (1)
44-57: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated
makeRunStoretest helper into a shared module. The sameRoutingRunStore/PostgresRunStoreconstruction (dedicatedNEW leg +legacyleg,as nevercasts) is copy-pasted across all these files (and several other test files per usage graph). The copies are already drifting — some annotate the return asRoutingRunStore, others don't — which is exactly the maintenance hazard duplication causes. Move it to a shared test util (e.g.apps/webapp/test/utils/makeRunStore.ts) and import it everywhere.
apps/webapp/test/waitpointListPresenter.readroute.test.ts#L44-L57: replace the local helper with an import from the shared util.apps/webapp/test/waitpointTagListPresenter.readroute.test.ts#L32-L48: replace the local helper with the shared import.apps/webapp/test/apiWaitpointListPresenter.readroute.test.ts#L44-L57: replace the local helper with the shared import.apps/webapp/test/waitpointPresenter.readthrough.test.ts#L75-L88: replace the local helper with the shared import.apps/webapp/test/waitpointPresenter.dedicatedConnectedRuns.readthrough.test.ts#L70-L83: replace the local helper with the shared import (keep thededicated-variant rationale in the shared doc comment).apps/webapp/app/presenters/v3/ApiRunResultPresenter.server.ts (1)
21-22: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused read-through constructor state.
_readThroughandApiRunResultReadThroughDepsaren’t referenced anywhere inApiRunResultPresenter.server.ts; drop them unless you need to keep the positional constructor shape for callers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 180372de-42a4-49fb-98f4-93f5e7668ab8
📒 Files selected for processing (19)
apps/webapp/app/presenters/v3/ApiRunResultPresenter.server.tsapps/webapp/app/presenters/v3/ApiWaitpointPresenter.server.tsapps/webapp/app/presenters/v3/WaitpointListPresenter.server.tsapps/webapp/app/presenters/v3/WaitpointPresenter.server.tsapps/webapp/app/v3/services/bulk/performBulkAction.server.tsapps/webapp/app/v3/services/cancelAttempt.server.tsapps/webapp/app/v3/services/finalizeTaskRun.server.tsapps/webapp/app/v3/services/resumeDependentParents.server.tsapps/webapp/test/apiRunResultPresenter.readthrough.test.tsapps/webapp/test/apiWaitpointListPresenter.readroute.test.tsapps/webapp/test/apiWaitpointPresenter.readthrough.test.tsapps/webapp/test/batchPresenter.test.tsapps/webapp/test/createCheckpoint.batchReplicaLag.test.tsapps/webapp/test/waitpointCallback.controlPlane.test.tsapps/webapp/test/waitpointListPresenter.readroute.test.tsapps/webapp/test/waitpointPresenter.dedicatedConnectedRuns.readthrough.test.tsapps/webapp/test/waitpointPresenter.readthrough.test.tsapps/webapp/test/waitpointTagListPresenter.readroute.test.tsinternal-packages/run-store/src/runOpsStore.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- apps/webapp/app/v3/services/bulk/performBulkAction.server.ts
- apps/webapp/app/v3/services/finalizeTaskRun.server.ts
- apps/webapp/app/presenters/v3/WaitpointListPresenter.server.ts
- apps/webapp/app/presenters/v3/ApiWaitpointPresenter.server.ts
- apps/webapp/app/v3/services/resumeDependentParents.server.ts
- apps/webapp/app/v3/services/cancelAttempt.server.ts
- internal-packages/run-store/src/runOpsStore.ts
- apps/webapp/app/presenters/v3/WaitpointPresenter.server.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Analyze (javascript-typescript)
🧰 Additional context used
📓 Path-based instructions (9)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Use types over interfaces for TypeScript
Avoid using enums; prefer string unions or const objects instead
Files:
apps/webapp/test/createCheckpoint.batchReplicaLag.test.tsapps/webapp/test/waitpointCallback.controlPlane.test.tsapps/webapp/test/waitpointTagListPresenter.readroute.test.tsapps/webapp/app/presenters/v3/ApiRunResultPresenter.server.tsapps/webapp/test/waitpointPresenter.dedicatedConnectedRuns.readthrough.test.tsapps/webapp/test/apiWaitpointListPresenter.readroute.test.tsapps/webapp/test/waitpointPresenter.readthrough.test.tsapps/webapp/test/waitpointListPresenter.readroute.test.tsapps/webapp/test/batchPresenter.test.tsapps/webapp/test/apiRunResultPresenter.readthrough.test.tsapps/webapp/test/apiWaitpointPresenter.readthrough.test.ts
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use zod for validation in packages/core and apps/webapp
Files:
apps/webapp/test/createCheckpoint.batchReplicaLag.test.tsapps/webapp/test/waitpointCallback.controlPlane.test.tsapps/webapp/test/waitpointTagListPresenter.readroute.test.tsapps/webapp/app/presenters/v3/ApiRunResultPresenter.server.tsapps/webapp/test/waitpointPresenter.dedicatedConnectedRuns.readthrough.test.tsapps/webapp/test/apiWaitpointListPresenter.readroute.test.tsapps/webapp/test/waitpointPresenter.readthrough.test.tsapps/webapp/test/waitpointListPresenter.readroute.test.tsapps/webapp/test/batchPresenter.test.tsapps/webapp/test/apiRunResultPresenter.readthrough.test.tsapps/webapp/test/apiWaitpointPresenter.readthrough.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use function declarations instead of default exports
**/*.{ts,tsx,js,jsx}: Prefer static imports over dynamicimport(); only use dynamic imports when resolving circular dependencies, enabling real code splitting, or conditionally loading a module at runtime.
Always import from@trigger.dev/sdk; never use@trigger.dev/sdk/v3or the deprecatedclient.defineJob.
Files:
apps/webapp/test/createCheckpoint.batchReplicaLag.test.tsapps/webapp/test/waitpointCallback.controlPlane.test.tsapps/webapp/test/waitpointTagListPresenter.readroute.test.tsapps/webapp/app/presenters/v3/ApiRunResultPresenter.server.tsapps/webapp/test/waitpointPresenter.dedicatedConnectedRuns.readthrough.test.tsapps/webapp/test/apiWaitpointListPresenter.readroute.test.tsapps/webapp/test/waitpointPresenter.readthrough.test.tsapps/webapp/test/waitpointListPresenter.readroute.test.tsapps/webapp/test/batchPresenter.test.tsapps/webapp/test/apiRunResultPresenter.readthrough.test.tsapps/webapp/test/apiWaitpointPresenter.readthrough.test.ts
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use vitest for all tests in the Trigger.dev repository
Files:
apps/webapp/test/createCheckpoint.batchReplicaLag.test.tsapps/webapp/test/waitpointCallback.controlPlane.test.tsapps/webapp/test/waitpointTagListPresenter.readroute.test.tsapps/webapp/test/waitpointPresenter.dedicatedConnectedRuns.readthrough.test.tsapps/webapp/test/apiWaitpointListPresenter.readroute.test.tsapps/webapp/test/waitpointPresenter.readthrough.test.tsapps/webapp/test/waitpointListPresenter.readroute.test.tsapps/webapp/test/batchPresenter.test.tsapps/webapp/test/apiRunResultPresenter.readthrough.test.tsapps/webapp/test/apiWaitpointPresenter.readthrough.test.ts
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc)
**/*.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
Do not use high-cardinality attributes in OTEL metrics such as UUIDs/IDs (envId, userId, runId, projectId, organizationId), unbounded integers (itemCount, batchSize, retryCount), timestamps (createdAt, startTime), or free-form strings (errorMessage, taskName, queueName)
When exporting OTEL metrics via OTLP to Prometheus, be aware that the exporter automatically adds unit suffixes to metric names (e.g., 'my_duration_ms' becomes 'my_duration_ms_milliseconds', 'my_counter' becomes 'my_counter_total'). Account for these transformations when writing Grafana dashboards or Prometheus queries
Files:
apps/webapp/test/createCheckpoint.batchReplicaLag.test.tsapps/webapp/test/waitpointCallback.controlPlane.test.tsapps/webapp/test/waitpointTagListPresenter.readroute.test.tsapps/webapp/app/presenters/v3/ApiRunResultPresenter.server.tsapps/webapp/test/waitpointPresenter.dedicatedConnectedRuns.readthrough.test.tsapps/webapp/test/apiWaitpointListPresenter.readroute.test.tsapps/webapp/test/waitpointPresenter.readthrough.test.tsapps/webapp/test/waitpointListPresenter.readroute.test.tsapps/webapp/test/batchPresenter.test.tsapps/webapp/test/apiRunResultPresenter.readthrough.test.tsapps/webapp/test/apiWaitpointPresenter.readthrough.test.ts
apps/webapp/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
apps/webapp/**/*.{ts,tsx}: Access environment variables through theenvexport ofenv.server.tsinstead of directly accessingprocess.env
Use subpath exports from@trigger.dev/corepackage instead of importing from the root@trigger.dev/corepathAlways use
findFirstinstead offindUniquefor Prisma queries.
Files:
apps/webapp/test/createCheckpoint.batchReplicaLag.test.tsapps/webapp/test/waitpointCallback.controlPlane.test.tsapps/webapp/test/waitpointTagListPresenter.readroute.test.tsapps/webapp/app/presenters/v3/ApiRunResultPresenter.server.tsapps/webapp/test/waitpointPresenter.dedicatedConnectedRuns.readthrough.test.tsapps/webapp/test/apiWaitpointListPresenter.readroute.test.tsapps/webapp/test/waitpointPresenter.readthrough.test.tsapps/webapp/test/waitpointListPresenter.readroute.test.tsapps/webapp/test/batchPresenter.test.tsapps/webapp/test/apiRunResultPresenter.readthrough.test.tsapps/webapp/test/apiWaitpointPresenter.readthrough.test.ts
apps/webapp/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Do not import
env.server.tsdirectly or indirectly into test files; instead pass environment-dependent values through options/parameters to make code testable
Files:
apps/webapp/test/createCheckpoint.batchReplicaLag.test.tsapps/webapp/test/waitpointCallback.controlPlane.test.tsapps/webapp/test/waitpointTagListPresenter.readroute.test.tsapps/webapp/test/waitpointPresenter.dedicatedConnectedRuns.readthrough.test.tsapps/webapp/test/apiWaitpointListPresenter.readroute.test.tsapps/webapp/test/waitpointPresenter.readthrough.test.tsapps/webapp/test/waitpointListPresenter.readroute.test.tsapps/webapp/test/batchPresenter.test.tsapps/webapp/test/apiRunResultPresenter.readthrough.test.tsapps/webapp/test/apiWaitpointPresenter.readthrough.test.ts
apps/webapp/**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
In test files, never import
env.server.ts; pass configuration as options instead.
Files:
apps/webapp/test/createCheckpoint.batchReplicaLag.test.tsapps/webapp/test/waitpointCallback.controlPlane.test.tsapps/webapp/test/waitpointTagListPresenter.readroute.test.tsapps/webapp/test/waitpointPresenter.dedicatedConnectedRuns.readthrough.test.tsapps/webapp/test/apiWaitpointListPresenter.readroute.test.tsapps/webapp/test/waitpointPresenter.readthrough.test.tsapps/webapp/test/waitpointListPresenter.readroute.test.tsapps/webapp/test/batchPresenter.test.tsapps/webapp/test/apiRunResultPresenter.readthrough.test.tsapps/webapp/test/apiWaitpointPresenter.readthrough.test.ts
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Place test files next to their source files, e.g.
MyService.ts->MyService.test.ts.
Files:
apps/webapp/test/createCheckpoint.batchReplicaLag.test.tsapps/webapp/test/waitpointCallback.controlPlane.test.tsapps/webapp/test/waitpointTagListPresenter.readroute.test.tsapps/webapp/test/waitpointPresenter.dedicatedConnectedRuns.readthrough.test.tsapps/webapp/test/apiWaitpointListPresenter.readroute.test.tsapps/webapp/test/waitpointPresenter.readthrough.test.tsapps/webapp/test/waitpointListPresenter.readroute.test.tsapps/webapp/test/batchPresenter.test.tsapps/webapp/test/apiRunResultPresenter.readthrough.test.tsapps/webapp/test/apiWaitpointPresenter.readthrough.test.ts
🧠 Learnings (19)
📚 Learning: 2026-03-22T13:26:12.060Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3244
File: apps/webapp/app/components/code/TextEditor.tsx:81-86
Timestamp: 2026-03-22T13:26:12.060Z
Learning: In the triggerdotdev/trigger.dev codebase, do not flag `navigator.clipboard.writeText(...)` calls for `missing-await`/`unhandled-promise` issues. These clipboard writes are intentionally invoked without `await` and without `catch` handlers across the project; keep that behavior consistent when reviewing TypeScript/TSX files (e.g., usages like in `apps/webapp/app/components/code/TextEditor.tsx`).
Applied to files:
apps/webapp/test/createCheckpoint.batchReplicaLag.test.tsapps/webapp/test/waitpointCallback.controlPlane.test.tsapps/webapp/test/waitpointTagListPresenter.readroute.test.tsapps/webapp/app/presenters/v3/ApiRunResultPresenter.server.tsapps/webapp/test/waitpointPresenter.dedicatedConnectedRuns.readthrough.test.tsapps/webapp/test/apiWaitpointListPresenter.readroute.test.tsapps/webapp/test/waitpointPresenter.readthrough.test.tsapps/webapp/test/waitpointListPresenter.readroute.test.tsapps/webapp/test/batchPresenter.test.tsapps/webapp/test/apiRunResultPresenter.readthrough.test.tsapps/webapp/test/apiWaitpointPresenter.readthrough.test.ts
📚 Learning: 2026-03-22T19:24:14.403Z
Learnt from: matt-aitken
Repo: triggerdotdev/trigger.dev PR: 3187
File: apps/webapp/app/v3/services/alerts/deliverErrorGroupAlert.server.ts:200-204
Timestamp: 2026-03-22T19:24:14.403Z
Learning: In the triggerdotdev/trigger.dev codebase, webhook URLs are not expected to contain embedded credentials/secrets (e.g., fields like `ProjectAlertWebhookProperties` should only hold credential-free webhook endpoints). During code review, if you see logging or inclusion of raw webhook URLs in error messages, do not automatically treat it as a credential-leak/secrets-in-logs issue by default—first verify the URL does not contain embedded credentials (for example, no username/password in the URL, no obvious secret/token query params or fragments). If the URL is credential-free per this project’s conventions, allow the logging.
Applied to files:
apps/webapp/test/createCheckpoint.batchReplicaLag.test.tsapps/webapp/test/waitpointCallback.controlPlane.test.tsapps/webapp/test/waitpointTagListPresenter.readroute.test.tsapps/webapp/app/presenters/v3/ApiRunResultPresenter.server.tsapps/webapp/test/waitpointPresenter.dedicatedConnectedRuns.readthrough.test.tsapps/webapp/test/apiWaitpointListPresenter.readroute.test.tsapps/webapp/test/waitpointPresenter.readthrough.test.tsapps/webapp/test/waitpointListPresenter.readroute.test.tsapps/webapp/test/batchPresenter.test.tsapps/webapp/test/apiRunResultPresenter.readthrough.test.tsapps/webapp/test/apiWaitpointPresenter.readthrough.test.ts
📚 Learning: 2026-05-18T08:21:27.694Z
Learnt from: d-cs
Repo: triggerdotdev/trigger.dev PR: 3632
File: apps/webapp/sentry.server.ts:4-21
Timestamp: 2026-05-18T08:21:27.694Z
Learning: When handling Prisma error P1001 ("Can't reach database server") in TypeScript, don’t assume a single error shape. Prisma can surface P1001 via two different error classes/fields: `PrismaClientKnownRequestError` exposes it as `err.code === "P1001"` (common during mid-query connection drops), while `PrismaClientInitializationError` exposes it as `err.errorCode === "P1001"` (common on client startup failure). Therefore, predicates should use `err.code === "P1001" || err.errorCode === "P1001"`. Do not flag `err.code === "P1001"` as “unreachable/never matches,” as it is expected in production.
Applied to files:
apps/webapp/test/createCheckpoint.batchReplicaLag.test.tsapps/webapp/test/waitpointCallback.controlPlane.test.tsapps/webapp/test/waitpointTagListPresenter.readroute.test.tsapps/webapp/app/presenters/v3/ApiRunResultPresenter.server.tsapps/webapp/test/waitpointPresenter.dedicatedConnectedRuns.readthrough.test.tsapps/webapp/test/apiWaitpointListPresenter.readroute.test.tsapps/webapp/test/waitpointPresenter.readthrough.test.tsapps/webapp/test/waitpointListPresenter.readroute.test.tsapps/webapp/test/batchPresenter.test.tsapps/webapp/test/apiRunResultPresenter.readthrough.test.tsapps/webapp/test/apiWaitpointPresenter.readthrough.test.ts
📚 Learning: 2026-05-18T08:21:27.694Z
Learnt from: d-cs
Repo: triggerdotdev/trigger.dev PR: 3632
File: apps/webapp/sentry.server.ts:4-21
Timestamp: 2026-05-18T08:21:27.694Z
Learning: When handling Prisma errors for P1001 ("Can't reach database server"), do not assume it only appears under a single property name. Prisma may surface P1001 via either `PrismaClientKnownRequestError` (`err.code === "P1001"`, e.g., mid-query connection drops) or `PrismaClientInitializationError` (`err.errorCode === "P1001"`, e.g., client startup connection failure). To reliably detect the condition, check `err.code === "P1001" || err.errorCode === "P1001"`, and avoid review rules that would incorrectly flag `err.code === "P1001"` as unreachable/never-matching.
Applied to files:
apps/webapp/test/createCheckpoint.batchReplicaLag.test.tsapps/webapp/test/waitpointCallback.controlPlane.test.tsapps/webapp/test/waitpointTagListPresenter.readroute.test.tsapps/webapp/app/presenters/v3/ApiRunResultPresenter.server.tsapps/webapp/test/waitpointPresenter.dedicatedConnectedRuns.readthrough.test.tsapps/webapp/test/apiWaitpointListPresenter.readroute.test.tsapps/webapp/test/waitpointPresenter.readthrough.test.tsapps/webapp/test/waitpointListPresenter.readroute.test.tsapps/webapp/test/batchPresenter.test.tsapps/webapp/test/apiRunResultPresenter.readthrough.test.tsapps/webapp/test/apiWaitpointPresenter.readthrough.test.ts
📚 Learning: 2026-06-13T19:53:13.759Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3937
File: packages/trigger-sdk/skills/realtime-and-frontend/SKILL.md:258-260
Timestamp: 2026-06-13T19:53:13.759Z
Learning: When reviewing code that uses `trigger.dev/react-hooks`’s `useRealtimeRun`, preserve the call signature where the first argument is the full realtime handle object (not `handle.id`). This is intentional to maintain type-safety and is consistent with the official docs; do not suggest changing the first argument from the handle object to `handle.id`.
Applied to files:
apps/webapp/test/createCheckpoint.batchReplicaLag.test.tsapps/webapp/test/waitpointCallback.controlPlane.test.tsapps/webapp/test/waitpointTagListPresenter.readroute.test.tsapps/webapp/app/presenters/v3/ApiRunResultPresenter.server.tsapps/webapp/test/waitpointPresenter.dedicatedConnectedRuns.readthrough.test.tsapps/webapp/test/apiWaitpointListPresenter.readroute.test.tsapps/webapp/test/waitpointPresenter.readthrough.test.tsapps/webapp/test/waitpointListPresenter.readroute.test.tsapps/webapp/test/batchPresenter.test.tsapps/webapp/test/apiRunResultPresenter.readthrough.test.tsapps/webapp/test/apiWaitpointPresenter.readthrough.test.ts
📚 Learning: 2026-06-17T17:13:49.929Z
Learnt from: matt-aitken
Repo: triggerdotdev/trigger.dev PR: 3948
File: apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.bulk-actions.$bulkActionParam/route.tsx:48-62
Timestamp: 2026-06-17T17:13:49.929Z
Learning: In triggerdotdev/trigger.dev, within `dashboardLoader`/`dashboardAction` (or similar context resolver code) whenever you resolve an organization ID from an organization slug for RBAC/enterprise authorization scope, always read from the primary Prisma client (`prisma`), not `$replica`. Using `$replica` can hit replica-lag and cause the RBAC lookup/authorization to run without the correct org scope (bypassing intended role enforcement). Implement the slug→org lookup with `prisma.organization.findFirst(...)` (or equivalent primary-client query) and add an inline comment documenting why the primary client is required (replica lag could lead to unscoped RBAC checks).
Applied to files:
apps/webapp/test/createCheckpoint.batchReplicaLag.test.tsapps/webapp/test/waitpointCallback.controlPlane.test.tsapps/webapp/test/waitpointTagListPresenter.readroute.test.tsapps/webapp/app/presenters/v3/ApiRunResultPresenter.server.tsapps/webapp/test/waitpointPresenter.dedicatedConnectedRuns.readthrough.test.tsapps/webapp/test/apiWaitpointListPresenter.readroute.test.tsapps/webapp/test/waitpointPresenter.readthrough.test.tsapps/webapp/test/waitpointListPresenter.readroute.test.tsapps/webapp/test/batchPresenter.test.tsapps/webapp/test/apiRunResultPresenter.readthrough.test.tsapps/webapp/test/apiWaitpointPresenter.readthrough.test.ts
📚 Learning: 2026-06-23T13:04:21.413Z
Learnt from: carderne
Repo: triggerdotdev/trigger.dev PR: 4023
File: apps/webapp/app/services/upsertBranch.server.ts:14-18
Timestamp: 2026-06-23T13:04:21.413Z
Learning: In TypeScript, it’s valid to `import { type X }` and then use `typeof X` in a type-only position, e.g. `type Alias = z.infer<typeof X>`. The `type` modifier suppresses the runtime import, but the type checker still has the full exported type so `z.infer<typeof X>` can resolve correctly. In code reviews, don’t flag this as a TypeScript compile error as long as `typeof X` is used in a type context (e.g., with `z.infer`, `type` aliases, generics), not as a runtime value.
Applied to files:
apps/webapp/test/createCheckpoint.batchReplicaLag.test.tsapps/webapp/test/waitpointCallback.controlPlane.test.tsapps/webapp/test/waitpointTagListPresenter.readroute.test.tsapps/webapp/app/presenters/v3/ApiRunResultPresenter.server.tsapps/webapp/test/waitpointPresenter.dedicatedConnectedRuns.readthrough.test.tsapps/webapp/test/apiWaitpointListPresenter.readroute.test.tsapps/webapp/test/waitpointPresenter.readthrough.test.tsapps/webapp/test/waitpointListPresenter.readroute.test.tsapps/webapp/test/batchPresenter.test.tsapps/webapp/test/apiRunResultPresenter.readthrough.test.tsapps/webapp/test/apiWaitpointPresenter.readthrough.test.ts
📚 Learning: 2026-05-07T12:25:18.271Z
Learnt from: d-cs
Repo: triggerdotdev/trigger.dev PR: 3531
File: apps/webapp/test/sentryTraceContext.server.test.ts:9-47
Timestamp: 2026-05-07T12:25:18.271Z
Learning: In the triggerdotdev/trigger.dev webapp test suite, it is acceptable to leave `createInMemoryTracing()` calls that register a global `NodeTracerProvider` without `afterEach`/`afterAll` teardown. Do not flag this as a test-ordering risk when the code follows the established pattern used across webapp tests (e.g., replication service/benchmark/backfiller tests). This is considered safe because `trace.getActiveSpan()` when called outside a `context.with(...)` block reads `AsyncLocalStorage.getStore()` (undefined when no `run()` scope exists), so it falls back to `ROOT_CONTEXT` with no attached span—regardless of which provider is registered.
Applied to files:
apps/webapp/test/createCheckpoint.batchReplicaLag.test.tsapps/webapp/test/waitpointCallback.controlPlane.test.tsapps/webapp/test/waitpointTagListPresenter.readroute.test.tsapps/webapp/test/waitpointPresenter.dedicatedConnectedRuns.readthrough.test.tsapps/webapp/test/apiWaitpointListPresenter.readroute.test.tsapps/webapp/test/waitpointPresenter.readthrough.test.tsapps/webapp/test/waitpointListPresenter.readroute.test.tsapps/webapp/test/batchPresenter.test.tsapps/webapp/test/apiRunResultPresenter.readthrough.test.tsapps/webapp/test/apiWaitpointPresenter.readthrough.test.ts
📚 Learning: 2026-05-28T20:02:10.647Z
Learnt from: myftija
Repo: triggerdotdev/trigger.dev PR: 3772
File: apps/webapp/test/findOrCreateBackgroundWorker.test.ts:1-1
Timestamp: 2026-05-28T20:02:10.647Z
Learning: In the triggerdotdev/trigger.dev monorepo, for the `apps/webapp` package use the established convention of storing Vitest tests (unit, integration, and e2e) under `apps/webapp/test/` rather than colocating them next to source files. Do not flag files located in `apps/webapp/test/` as violating any rule that says to colocate tests with source.
Applied to files:
apps/webapp/test/createCheckpoint.batchReplicaLag.test.tsapps/webapp/test/waitpointCallback.controlPlane.test.tsapps/webapp/test/waitpointTagListPresenter.readroute.test.tsapps/webapp/test/waitpointPresenter.dedicatedConnectedRuns.readthrough.test.tsapps/webapp/test/apiWaitpointListPresenter.readroute.test.tsapps/webapp/test/waitpointPresenter.readthrough.test.tsapps/webapp/test/waitpointListPresenter.readroute.test.tsapps/webapp/test/batchPresenter.test.tsapps/webapp/test/apiRunResultPresenter.readthrough.test.tsapps/webapp/test/apiWaitpointPresenter.readthrough.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/createCheckpoint.batchReplicaLag.test.tsapps/webapp/test/waitpointCallback.controlPlane.test.tsapps/webapp/test/waitpointTagListPresenter.readroute.test.tsapps/webapp/app/presenters/v3/ApiRunResultPresenter.server.tsapps/webapp/test/waitpointPresenter.dedicatedConnectedRuns.readthrough.test.tsapps/webapp/test/apiWaitpointListPresenter.readroute.test.tsapps/webapp/test/waitpointPresenter.readthrough.test.tsapps/webapp/test/waitpointListPresenter.readroute.test.tsapps/webapp/test/batchPresenter.test.tsapps/webapp/test/apiRunResultPresenter.readthrough.test.tsapps/webapp/test/apiWaitpointPresenter.readthrough.test.ts
📚 Learning: 2026-06-25T18:21:51.905Z
Learnt from: carderne
Repo: triggerdotdev/trigger.dev PR: 4039
File: apps/webapp/app/routes/invite-revoke.tsx:0-0
Timestamp: 2026-06-25T18:21:51.905Z
Learning: During the Zod v4 migration in the triggerdotdev/trigger.dev webapp, ensure any imports from `conform-to/zod` use the Zod-4 subpath: `conform-to/zod/v4` (e.g., `import { parseWithZod } from "conform-to/zod/v4"`). Do not import from the package root `conform-to/zod`, because it is the Zod 3 implementation and may load Zod-3-only symbols (e.g., `ZodBranded`, `ZodEffects`), which can throw at module load (notably with `zod4.4.3`). This should be enforced across `apps/webapp/**/*` where helpers like `parseWithZod` and `conformZodMessage` are used.
Applied to files:
apps/webapp/test/createCheckpoint.batchReplicaLag.test.tsapps/webapp/test/waitpointCallback.controlPlane.test.tsapps/webapp/test/waitpointTagListPresenter.readroute.test.tsapps/webapp/app/presenters/v3/ApiRunResultPresenter.server.tsapps/webapp/test/waitpointPresenter.dedicatedConnectedRuns.readthrough.test.tsapps/webapp/test/apiWaitpointListPresenter.readroute.test.tsapps/webapp/test/waitpointPresenter.readthrough.test.tsapps/webapp/test/waitpointListPresenter.readroute.test.tsapps/webapp/test/batchPresenter.test.tsapps/webapp/test/apiRunResultPresenter.readthrough.test.tsapps/webapp/test/apiWaitpointPresenter.readthrough.test.ts
📚 Learning: 2026-07-03T17:10:21.498Z
Learnt from: 0ski
Repo: triggerdotdev/trigger.dev PR: 4148
File: apps/webapp/app/models/orgMember.server.ts:149-168
Timestamp: 2026-07-03T17:10:21.498Z
Learning: In triggerdotdev/trigger.dev, `User.email` (Prisma schema: `internal-packages/database/prisma/schema.prisma`) currently does NOT use `citext` and does NOT have a `lower(email)` functional unique index. Therefore, do not introduce Prisma queries like `where: { email: { equals: <value>, mode: "insensitive" } }` (or any case-insensitive lookup) against `User.email`, because it can force sequential scans of the `users` table under load. During review, ensure email is normalized (e.g., lowercased/trimmed) before both writes and subsequent lookups, and if true case-insensitive behavior/uniqueness is required, implement it via a separate app-wide migration (e.g., switch to `citext` and/or add a functional unique index with backfill) rather than bolting it onto individual feature PRs.
Applied to files:
apps/webapp/test/createCheckpoint.batchReplicaLag.test.tsapps/webapp/test/waitpointCallback.controlPlane.test.tsapps/webapp/test/waitpointTagListPresenter.readroute.test.tsapps/webapp/app/presenters/v3/ApiRunResultPresenter.server.tsapps/webapp/test/waitpointPresenter.dedicatedConnectedRuns.readthrough.test.tsapps/webapp/test/apiWaitpointListPresenter.readroute.test.tsapps/webapp/test/waitpointPresenter.readthrough.test.tsapps/webapp/test/waitpointListPresenter.readroute.test.tsapps/webapp/test/batchPresenter.test.tsapps/webapp/test/apiRunResultPresenter.readthrough.test.tsapps/webapp/test/apiWaitpointPresenter.readthrough.test.ts
📚 Learning: 2026-05-18T14:40:02.173Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3658
File: packages/core/src/v3/realtimeStreams/manager.test.ts:1-147
Timestamp: 2026-05-18T14:40:02.173Z
Learning: In the triggerdotdev/trigger.dev repo, the policy “Never mock anything — use testcontainers instead” should only be enforced for integration tests that interact with real external services (e.g., Redis, Postgres) via actual infrastructure. For unit tests that exercise pure in-memory logic (e.g., cache semantics) it is OK to stub collaborators such as `ApiClient` using Vitest (`vi.fn()`) to assert call counts or control behavior. Do not flag `vi.fn()`-based `ApiClient` stubs in unit tests as violations of the testcontainers policy.
Applied to files:
apps/webapp/test/createCheckpoint.batchReplicaLag.test.tsapps/webapp/test/waitpointCallback.controlPlane.test.tsapps/webapp/test/waitpointTagListPresenter.readroute.test.tsapps/webapp/test/waitpointPresenter.dedicatedConnectedRuns.readthrough.test.tsapps/webapp/test/apiWaitpointListPresenter.readroute.test.tsapps/webapp/test/waitpointPresenter.readthrough.test.tsapps/webapp/test/waitpointListPresenter.readroute.test.tsapps/webapp/test/batchPresenter.test.tsapps/webapp/test/apiRunResultPresenter.readthrough.test.tsapps/webapp/test/apiWaitpointPresenter.readthrough.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/test/createCheckpoint.batchReplicaLag.test.tsapps/webapp/test/waitpointCallback.controlPlane.test.tsapps/webapp/test/waitpointTagListPresenter.readroute.test.tsapps/webapp/app/presenters/v3/ApiRunResultPresenter.server.tsapps/webapp/test/waitpointPresenter.dedicatedConnectedRuns.readthrough.test.tsapps/webapp/test/apiWaitpointListPresenter.readroute.test.tsapps/webapp/test/waitpointPresenter.readthrough.test.tsapps/webapp/test/waitpointListPresenter.readroute.test.tsapps/webapp/test/batchPresenter.test.tsapps/webapp/test/apiRunResultPresenter.readthrough.test.tsapps/webapp/test/apiWaitpointPresenter.readthrough.test.ts
📚 Learning: 2026-06-09T17:58:04.699Z
Learnt from: 0ski
Repo: triggerdotdev/trigger.dev PR: 3879
File: apps/webapp/app/models/vercelIntegration.server.ts:619-630
Timestamp: 2026-06-09T17:58:04.699Z
Learning: In this codebase, outbound raw `fetch` calls should typically rely on Node/undici’s default request timeout (about ~300s) rather than adding a per-call `AbortController` + `setTimeout` wrapper inside individual functions (e.g. in files like `apps/webapp/app/models/vercelIntegration.server.ts`). During code review, do not flag the absence of a per-call timeout on a single `fetch` as an issue; if per-call timeouts are needed, they should be implemented via a codebase-wide convention (e.g., a shared fetch wrapper or documented pattern) rather than ad-hoc per-function changes.
Applied to files:
apps/webapp/test/createCheckpoint.batchReplicaLag.test.tsapps/webapp/test/waitpointCallback.controlPlane.test.tsapps/webapp/test/waitpointTagListPresenter.readroute.test.tsapps/webapp/app/presenters/v3/ApiRunResultPresenter.server.tsapps/webapp/test/waitpointPresenter.dedicatedConnectedRuns.readthrough.test.tsapps/webapp/test/apiWaitpointListPresenter.readroute.test.tsapps/webapp/test/waitpointPresenter.readthrough.test.tsapps/webapp/test/waitpointListPresenter.readroute.test.tsapps/webapp/test/batchPresenter.test.tsapps/webapp/test/apiRunResultPresenter.readthrough.test.tsapps/webapp/test/apiWaitpointPresenter.readthrough.test.ts
📚 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/createCheckpoint.batchReplicaLag.test.tsapps/webapp/test/waitpointCallback.controlPlane.test.tsapps/webapp/test/waitpointTagListPresenter.readroute.test.tsapps/webapp/test/waitpointPresenter.dedicatedConnectedRuns.readthrough.test.tsapps/webapp/test/apiWaitpointListPresenter.readroute.test.tsapps/webapp/test/waitpointPresenter.readthrough.test.tsapps/webapp/test/waitpointListPresenter.readroute.test.tsapps/webapp/test/batchPresenter.test.tsapps/webapp/test/apiRunResultPresenter.readthrough.test.tsapps/webapp/test/apiWaitpointPresenter.readthrough.test.ts
📚 Learning: 2026-02-06T19:53:38.843Z
Learnt from: 0ski
Repo: triggerdotdev/trigger.dev PR: 2994
File: apps/webapp/app/presenters/v3/DeploymentListPresenter.server.ts:233-237
Timestamp: 2026-02-06T19:53:38.843Z
Learning: When constructing Vercel dashboard URLs from deployment IDs, always strip the dpl_ prefix from the ID. Implement this by transforming the ID with .replace(/^dpl_/, "") before concatenating into the URL: https://vercel.com/${teamSlug}/${projectName}/${cleanedDeploymentId}. Consider centralizing this logic in a small helper (e.g., getVercelDeploymentId(id) or a URL builder) and add tests to verify both prefixed and non-prefixed inputs.
Applied to files:
apps/webapp/app/presenters/v3/ApiRunResultPresenter.server.ts
📚 Learning: 2026-05-05T09:38:02.512Z
Learnt from: d-cs
Repo: triggerdotdev/trigger.dev PR: 3523
File: apps/webapp/app/routes/api.v3.batches.ts:178-181
Timestamp: 2026-05-05T09:38:02.512Z
Learning: When reviewing code that catches `ServiceValidationError` in `*.server.ts` files, do not blindly forward `error.status` to HTTP responses, because SVEs may be thrown with non-default statuses (e.g., 400/500) and forwarding them can cause client-visible behavioral regressions (e.g., surfacing 500s to clients). Prefer a safe default response status of `error.status ?? 422`, but only after confirming via the reachable call graph that the caught `ServiceValidationError` instances are expected to carry those non-default statuses; otherwise, normalize to `422` to avoid unexpected client-visible 5xx behavior.
Applied to files:
apps/webapp/app/presenters/v3/ApiRunResultPresenter.server.ts
📚 Learning: 2026-06-21T05:35:23.468Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 4005
File: apps/webapp/app/presenters/v3/ApiErrorListPresenter.server.ts:29-30
Timestamp: 2026-06-21T05:35:23.468Z
Learning: For triggerdotdev/trigger.dev list endpoints (and their presenters/handlers that implement list pagination), it is an established shared convention to allow both cursor query params `page[after]` and `page[before]` to be provided at the same time. When both are present, `page[before]` must take precedence (i.e., it should be used/wins). During code review, do NOT flag missing per-endpoint mutual-exclusion validation between `page[after]` and `page[before]` as a problem; if stricter enforcement is ever desired, it should be implemented as a codebase-wide shared convention (not individually per endpoint).
Applied to files:
apps/webapp/app/presenters/v3/ApiRunResultPresenter.server.ts
🔇 Additional comments (11)
apps/webapp/test/createCheckpoint.batchReplicaLag.test.ts (1)
23-53: LGTM!apps/webapp/test/batchPresenter.test.ts (1)
2-30: LGTM!Also applies to: 180-185, 235-241, 264-269, 284-289, 316-332, 361-366
apps/webapp/test/apiRunResultPresenter.readthrough.test.ts (1)
2-55: LGTM!Also applies to: 227-236, 251-269, 285-299, 318-341, 374-386
apps/webapp/test/apiWaitpointPresenter.readthrough.test.ts (1)
2-39: LGTM!Also applies to: 51-63, 151-374
apps/webapp/test/waitpointCallback.controlPlane.test.ts (1)
30-38: 🎯 Functional CorrectnessThe
this-binding concern doesn’t apply here. Prisma model delegates are proxy-generated, not class-instance methods, so returningholder.client[prop][method]directly is fine.> Likely an incorrect or invalid review comment.apps/webapp/test/waitpointTagListPresenter.readroute.test.ts (1)
149-156: LGTM!Also applies to: 201-206, 244-249
apps/webapp/app/presenters/v3/ApiRunResultPresenter.server.ts (1)
32-39: LGTM!apps/webapp/test/waitpointPresenter.dedicatedConnectedRuns.readthrough.test.ts (1)
189-198: LGTM!Also applies to: 226-235, 271-280
apps/webapp/test/apiWaitpointListPresenter.readroute.test.ts (1)
27-34: LGTM!Also applies to: 99-102, 132-135
apps/webapp/test/waitpointPresenter.readthrough.test.ts (1)
268-277: LGTM!Also applies to: 381-390, 424-431
apps/webapp/test/waitpointListPresenter.readroute.test.ts (1)
198-200: LGTM!Also applies to: 293-298, 359-364, 388-395, 421-426, 447-454, 491-496
…rd-fks # Conflicts: # apps/webapp/app/presenters/v3/ApiBatchResultsPresenter.server.ts # apps/webapp/app/presenters/v3/WaitpointPresenter.server.ts # apps/webapp/test/waitpointPresenter.readthrough.test.ts
…rd-fks # Conflicts: # apps/webapp/app/v3/services/bulk/performBulkAction.server.ts # apps/webapp/app/v3/services/cancelAttempt.server.ts # apps/webapp/app/v3/services/cancelDevSessionRuns.server.ts # apps/webapp/app/v3/services/cancelTaskAttemptDependencies.server.ts # apps/webapp/app/v3/services/completeAttempt.server.ts # apps/webapp/app/v3/services/crashTaskRun.server.ts # apps/webapp/app/v3/services/createCheckpoint.server.ts # apps/webapp/app/v3/services/createCheckpointRestoreEvent.server.ts # apps/webapp/app/v3/services/createTaskRunAttempt.server.ts # apps/webapp/app/v3/services/enqueueDelayedRun.server.ts # apps/webapp/app/v3/services/executeTasksWaitingForDeploy.ts # apps/webapp/app/v3/services/finalizeTaskRun.server.ts # apps/webapp/app/v3/services/restoreCheckpoint.server.ts # apps/webapp/app/v3/services/resumeAttempt.server.ts # apps/webapp/app/v3/services/resumeBatchRun.server.ts # apps/webapp/app/v3/services/resumeDependentParents.server.ts # apps/webapp/app/v3/services/resumeTaskDependency.server.ts # apps/webapp/test/createCheckpoint.batchReplicaLag.test.ts
…bilise flaky tests - waitpointPresenter.danglingConnectedRuns: inject a container-backed run store (the connected-runs read moved onto the run store), matching its siblings. - apiRetrieveRunPresenter readroute + groupedLockedWorker: use postgresTest instead of containerTest (Postgres-only) to stop the 10s fixture timeout; the N+1 regression coverage is unchanged. - runsReplicationService part9: replace the live lag-histogram label poll with a deterministic metric read/merge test. - runsReplicationInstance: poll both leader-lock keys for readiness instead of a fixed 3s sleep.
Collaborator
Author
|
Merged latest main, which includes the removal of the end-of-life V1 execution stack (#4236). That removal deleted several services this branch had touched, so a few earlier review findings are now moot:
Still standing, on surviving files: the run-store waitpoint-edge fan-out delete is applied, and the tag natural-key dedup is deferred to a follow-up. Also stabilised some flaky tests surfaced by CI: |
@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: |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/webapp/test/waitpointPresenter.readthrough.test.ts (1)
62-90: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated
makeRunStorehelper to a shared test util.This exact
makeRunStore(newClient, legacyClient)implementation (constructing aRoutingRunStorefrom twoPostgresRunStoreinstances withschemaVariant: "dedicated"/"legacy") is copy-pasted verbatim across at leastwaitpointPresenter.danglingConnectedRuns.test.ts,waitpointPresenter.splitConnectedRuns.test.ts,waitpointPresenter.connectedRunsBounded.test.ts, andwaitpointPresenter.dedicatedConnectedRuns.readthrough.test.ts. Extracting it into a shared test helper module (e.g. underapps/webapp/test/utils/) would remove this duplication and centralize future changes to the routing wiring.♻️ Proposed extraction
// apps/webapp/test/utils/runStore.ts import { PostgresRunStore, RoutingRunStore } from "`@internal/run-store`"; import type { PrismaClient } from "`@trigger.dev/database`"; export function makeRunStore(newClient: PrismaClient, legacyClient: PrismaClient) { return new RoutingRunStore({ new: new PostgresRunStore({ prisma: newClient as never, readOnlyPrisma: newClient as never, schemaVariant: "dedicated", }), legacy: new PostgresRunStore({ prisma: legacyClient as never, readOnlyPrisma: legacyClient as never, schemaVariant: "legacy", }), }); }-import { PostgresRunStore, RoutingRunStore } from "`@internal/run-store`"; +import { makeRunStore } from "./utils/runStore";
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e7fba0b3-e443-4cc2-b770-a98c915afcb1
📒 Files selected for processing (16)
apps/webapp/app/env.server.tsapps/webapp/app/presenters/v3/WaitpointPresenter.server.tsapps/webapp/app/v3/services/batchTriggerV3.server.tsapps/webapp/package.jsonapps/webapp/test/apiRetrieveRunPresenter.groupedLockedWorker.test.tsapps/webapp/test/apiRetrieveRunPresenter.readroute.test.tsapps/webapp/test/runsReplicationInstance.test.tsapps/webapp/test/runsReplicationService.part9.test.tsapps/webapp/test/waitpointPresenter.connectedRunsBounded.test.tsapps/webapp/test/waitpointPresenter.danglingConnectedRuns.test.tsapps/webapp/test/waitpointPresenter.dedicatedConnectedRuns.readthrough.test.tsapps/webapp/test/waitpointPresenter.readthrough.test.tsapps/webapp/test/waitpointPresenter.splitConnectedRuns.test.tsinternal-packages/run-store/src/PostgresRunStore.tsinternal-packages/run-store/src/runOpsStore.tsinternal-packages/run-store/src/types.ts
💤 Files with no reviewable changes (1)
- apps/webapp/package.json
🚧 Files skipped from review as they are similar to previous changes (7)
- apps/webapp/app/v3/services/batchTriggerV3.server.ts
- apps/webapp/app/env.server.ts
- internal-packages/run-store/src/types.ts
- apps/webapp/test/waitpointPresenter.dedicatedConnectedRuns.readthrough.test.ts
- apps/webapp/app/presenters/v3/WaitpointPresenter.server.ts
- internal-packages/run-store/src/PostgresRunStore.ts
- internal-packages/run-store/src/runOpsStore.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (27)
- GitHub Check: internal / 🧪 Unit Tests: Internal (6, 12)
- GitHub Check: internal / 🧪 Unit Tests: Internal (11, 12)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (2, 10)
- GitHub Check: internal / 🧪 Unit Tests: Internal (8, 12)
- GitHub Check: internal / 🧪 Unit Tests: Internal (9, 12)
- GitHub Check: internal / 🧪 Unit Tests: Internal (10, 12)
- GitHub Check: internal / 🧪 Unit Tests: Internal (12, 12)
- GitHub Check: internal / 🧪 Unit Tests: Internal (5, 12)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (8, 10)
- GitHub Check: internal / 🧪 Unit Tests: Internal (3, 12)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (4, 10)
- GitHub Check: internal / 🧪 Unit Tests: Internal (2, 12)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (9, 10)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (10, 10)
- GitHub Check: internal / 🧪 Unit Tests: Internal (7, 12)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (7, 10)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (1, 10)
- GitHub Check: internal / 🧪 Unit Tests: Internal (4, 12)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (5, 10)
- GitHub Check: internal / 🧪 Unit Tests: Internal (1, 12)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (3, 10)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (6, 10)
- GitHub Check: typecheck / typecheck
- GitHub Check: e2e-webapp / 🧪 E2E Tests: Webapp
- GitHub Check: 🛡️ E2E Auth Tests (full)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Build and publish previews
🧰 Additional context used
📓 Path-based instructions (9)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Use types over interfaces for TypeScript
Avoid using enums; prefer string unions or const objects instead
Files:
apps/webapp/test/apiRetrieveRunPresenter.groupedLockedWorker.test.tsapps/webapp/test/apiRetrieveRunPresenter.readroute.test.tsapps/webapp/test/runsReplicationInstance.test.tsapps/webapp/test/waitpointPresenter.danglingConnectedRuns.test.tsapps/webapp/test/runsReplicationService.part9.test.tsapps/webapp/test/waitpointPresenter.splitConnectedRuns.test.tsapps/webapp/test/waitpointPresenter.connectedRunsBounded.test.tsapps/webapp/test/waitpointPresenter.readthrough.test.ts
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use zod for validation in packages/core and apps/webapp
Files:
apps/webapp/test/apiRetrieveRunPresenter.groupedLockedWorker.test.tsapps/webapp/test/apiRetrieveRunPresenter.readroute.test.tsapps/webapp/test/runsReplicationInstance.test.tsapps/webapp/test/waitpointPresenter.danglingConnectedRuns.test.tsapps/webapp/test/runsReplicationService.part9.test.tsapps/webapp/test/waitpointPresenter.splitConnectedRuns.test.tsapps/webapp/test/waitpointPresenter.connectedRunsBounded.test.tsapps/webapp/test/waitpointPresenter.readthrough.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use function declarations instead of default exports
**/*.{ts,tsx,js,jsx}: Prefer static imports over dynamicimport(); only use dynamic imports when resolving circular dependencies, enabling real code splitting, or conditionally loading a module at runtime.
Always import from@trigger.dev/sdk; never use@trigger.dev/sdk/v3or the deprecatedclient.defineJob.
Files:
apps/webapp/test/apiRetrieveRunPresenter.groupedLockedWorker.test.tsapps/webapp/test/apiRetrieveRunPresenter.readroute.test.tsapps/webapp/test/runsReplicationInstance.test.tsapps/webapp/test/waitpointPresenter.danglingConnectedRuns.test.tsapps/webapp/test/runsReplicationService.part9.test.tsapps/webapp/test/waitpointPresenter.splitConnectedRuns.test.tsapps/webapp/test/waitpointPresenter.connectedRunsBounded.test.tsapps/webapp/test/waitpointPresenter.readthrough.test.ts
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use vitest for all tests in the Trigger.dev repository
Files:
apps/webapp/test/apiRetrieveRunPresenter.groupedLockedWorker.test.tsapps/webapp/test/apiRetrieveRunPresenter.readroute.test.tsapps/webapp/test/runsReplicationInstance.test.tsapps/webapp/test/waitpointPresenter.danglingConnectedRuns.test.tsapps/webapp/test/runsReplicationService.part9.test.tsapps/webapp/test/waitpointPresenter.splitConnectedRuns.test.tsapps/webapp/test/waitpointPresenter.connectedRunsBounded.test.tsapps/webapp/test/waitpointPresenter.readthrough.test.ts
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc)
**/*.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
Do not use high-cardinality attributes in OTEL metrics such as UUIDs/IDs (envId, userId, runId, projectId, organizationId), unbounded integers (itemCount, batchSize, retryCount), timestamps (createdAt, startTime), or free-form strings (errorMessage, taskName, queueName)
When exporting OTEL metrics via OTLP to Prometheus, be aware that the exporter automatically adds unit suffixes to metric names (e.g., 'my_duration_ms' becomes 'my_duration_ms_milliseconds', 'my_counter' becomes 'my_counter_total'). Account for these transformations when writing Grafana dashboards or Prometheus queries
Files:
apps/webapp/test/apiRetrieveRunPresenter.groupedLockedWorker.test.tsapps/webapp/test/apiRetrieveRunPresenter.readroute.test.tsapps/webapp/test/runsReplicationInstance.test.tsapps/webapp/test/waitpointPresenter.danglingConnectedRuns.test.tsapps/webapp/test/runsReplicationService.part9.test.tsapps/webapp/test/waitpointPresenter.splitConnectedRuns.test.tsapps/webapp/test/waitpointPresenter.connectedRunsBounded.test.tsapps/webapp/test/waitpointPresenter.readthrough.test.ts
apps/webapp/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
apps/webapp/**/*.{ts,tsx}: Access environment variables through theenvexport ofenv.server.tsinstead of directly accessingprocess.env
Use subpath exports from@trigger.dev/corepackage instead of importing from the root@trigger.dev/corepathAlways use
findFirstinstead offindUniquefor Prisma queries.
Files:
apps/webapp/test/apiRetrieveRunPresenter.groupedLockedWorker.test.tsapps/webapp/test/apiRetrieveRunPresenter.readroute.test.tsapps/webapp/test/runsReplicationInstance.test.tsapps/webapp/test/waitpointPresenter.danglingConnectedRuns.test.tsapps/webapp/test/runsReplicationService.part9.test.tsapps/webapp/test/waitpointPresenter.splitConnectedRuns.test.tsapps/webapp/test/waitpointPresenter.connectedRunsBounded.test.tsapps/webapp/test/waitpointPresenter.readthrough.test.ts
apps/webapp/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Do not import
env.server.tsdirectly or indirectly into test files; instead pass environment-dependent values through options/parameters to make code testable
Files:
apps/webapp/test/apiRetrieveRunPresenter.groupedLockedWorker.test.tsapps/webapp/test/apiRetrieveRunPresenter.readroute.test.tsapps/webapp/test/runsReplicationInstance.test.tsapps/webapp/test/waitpointPresenter.danglingConnectedRuns.test.tsapps/webapp/test/runsReplicationService.part9.test.tsapps/webapp/test/waitpointPresenter.splitConnectedRuns.test.tsapps/webapp/test/waitpointPresenter.connectedRunsBounded.test.tsapps/webapp/test/waitpointPresenter.readthrough.test.ts
apps/webapp/**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
In test files, never import
env.server.ts; pass configuration as options instead.
Files:
apps/webapp/test/apiRetrieveRunPresenter.groupedLockedWorker.test.tsapps/webapp/test/apiRetrieveRunPresenter.readroute.test.tsapps/webapp/test/runsReplicationInstance.test.tsapps/webapp/test/waitpointPresenter.danglingConnectedRuns.test.tsapps/webapp/test/runsReplicationService.part9.test.tsapps/webapp/test/waitpointPresenter.splitConnectedRuns.test.tsapps/webapp/test/waitpointPresenter.connectedRunsBounded.test.tsapps/webapp/test/waitpointPresenter.readthrough.test.ts
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Place test files next to their source files, e.g.
MyService.ts->MyService.test.ts.
Files:
apps/webapp/test/apiRetrieveRunPresenter.groupedLockedWorker.test.tsapps/webapp/test/apiRetrieveRunPresenter.readroute.test.tsapps/webapp/test/runsReplicationInstance.test.tsapps/webapp/test/waitpointPresenter.danglingConnectedRuns.test.tsapps/webapp/test/runsReplicationService.part9.test.tsapps/webapp/test/waitpointPresenter.splitConnectedRuns.test.tsapps/webapp/test/waitpointPresenter.connectedRunsBounded.test.tsapps/webapp/test/waitpointPresenter.readthrough.test.ts
🧠 Learnings (16)
📚 Learning: 2026-03-22T13:26:12.060Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3244
File: apps/webapp/app/components/code/TextEditor.tsx:81-86
Timestamp: 2026-03-22T13:26:12.060Z
Learning: In the triggerdotdev/trigger.dev codebase, do not flag `navigator.clipboard.writeText(...)` calls for `missing-await`/`unhandled-promise` issues. These clipboard writes are intentionally invoked without `await` and without `catch` handlers across the project; keep that behavior consistent when reviewing TypeScript/TSX files (e.g., usages like in `apps/webapp/app/components/code/TextEditor.tsx`).
Applied to files:
apps/webapp/test/apiRetrieveRunPresenter.groupedLockedWorker.test.tsapps/webapp/test/apiRetrieveRunPresenter.readroute.test.tsapps/webapp/test/runsReplicationInstance.test.tsapps/webapp/test/waitpointPresenter.danglingConnectedRuns.test.tsapps/webapp/test/runsReplicationService.part9.test.tsapps/webapp/test/waitpointPresenter.splitConnectedRuns.test.tsapps/webapp/test/waitpointPresenter.connectedRunsBounded.test.tsapps/webapp/test/waitpointPresenter.readthrough.test.ts
📚 Learning: 2026-03-22T19:24:14.403Z
Learnt from: matt-aitken
Repo: triggerdotdev/trigger.dev PR: 3187
File: apps/webapp/app/v3/services/alerts/deliverErrorGroupAlert.server.ts:200-204
Timestamp: 2026-03-22T19:24:14.403Z
Learning: In the triggerdotdev/trigger.dev codebase, webhook URLs are not expected to contain embedded credentials/secrets (e.g., fields like `ProjectAlertWebhookProperties` should only hold credential-free webhook endpoints). During code review, if you see logging or inclusion of raw webhook URLs in error messages, do not automatically treat it as a credential-leak/secrets-in-logs issue by default—first verify the URL does not contain embedded credentials (for example, no username/password in the URL, no obvious secret/token query params or fragments). If the URL is credential-free per this project’s conventions, allow the logging.
Applied to files:
apps/webapp/test/apiRetrieveRunPresenter.groupedLockedWorker.test.tsapps/webapp/test/apiRetrieveRunPresenter.readroute.test.tsapps/webapp/test/runsReplicationInstance.test.tsapps/webapp/test/waitpointPresenter.danglingConnectedRuns.test.tsapps/webapp/test/runsReplicationService.part9.test.tsapps/webapp/test/waitpointPresenter.splitConnectedRuns.test.tsapps/webapp/test/waitpointPresenter.connectedRunsBounded.test.tsapps/webapp/test/waitpointPresenter.readthrough.test.ts
📚 Learning: 2026-05-18T08:21:27.694Z
Learnt from: d-cs
Repo: triggerdotdev/trigger.dev PR: 3632
File: apps/webapp/sentry.server.ts:4-21
Timestamp: 2026-05-18T08:21:27.694Z
Learning: When handling Prisma error P1001 ("Can't reach database server") in TypeScript, don’t assume a single error shape. Prisma can surface P1001 via two different error classes/fields: `PrismaClientKnownRequestError` exposes it as `err.code === "P1001"` (common during mid-query connection drops), while `PrismaClientInitializationError` exposes it as `err.errorCode === "P1001"` (common on client startup failure). Therefore, predicates should use `err.code === "P1001" || err.errorCode === "P1001"`. Do not flag `err.code === "P1001"` as “unreachable/never matches,” as it is expected in production.
Applied to files:
apps/webapp/test/apiRetrieveRunPresenter.groupedLockedWorker.test.tsapps/webapp/test/apiRetrieveRunPresenter.readroute.test.tsapps/webapp/test/runsReplicationInstance.test.tsapps/webapp/test/waitpointPresenter.danglingConnectedRuns.test.tsapps/webapp/test/runsReplicationService.part9.test.tsapps/webapp/test/waitpointPresenter.splitConnectedRuns.test.tsapps/webapp/test/waitpointPresenter.connectedRunsBounded.test.tsapps/webapp/test/waitpointPresenter.readthrough.test.ts
📚 Learning: 2026-05-18T08:21:27.694Z
Learnt from: d-cs
Repo: triggerdotdev/trigger.dev PR: 3632
File: apps/webapp/sentry.server.ts:4-21
Timestamp: 2026-05-18T08:21:27.694Z
Learning: When handling Prisma errors for P1001 ("Can't reach database server"), do not assume it only appears under a single property name. Prisma may surface P1001 via either `PrismaClientKnownRequestError` (`err.code === "P1001"`, e.g., mid-query connection drops) or `PrismaClientInitializationError` (`err.errorCode === "P1001"`, e.g., client startup connection failure). To reliably detect the condition, check `err.code === "P1001" || err.errorCode === "P1001"`, and avoid review rules that would incorrectly flag `err.code === "P1001"` as unreachable/never-matching.
Applied to files:
apps/webapp/test/apiRetrieveRunPresenter.groupedLockedWorker.test.tsapps/webapp/test/apiRetrieveRunPresenter.readroute.test.tsapps/webapp/test/runsReplicationInstance.test.tsapps/webapp/test/waitpointPresenter.danglingConnectedRuns.test.tsapps/webapp/test/runsReplicationService.part9.test.tsapps/webapp/test/waitpointPresenter.splitConnectedRuns.test.tsapps/webapp/test/waitpointPresenter.connectedRunsBounded.test.tsapps/webapp/test/waitpointPresenter.readthrough.test.ts
📚 Learning: 2026-06-13T19:53:13.759Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3937
File: packages/trigger-sdk/skills/realtime-and-frontend/SKILL.md:258-260
Timestamp: 2026-06-13T19:53:13.759Z
Learning: When reviewing code that uses `trigger.dev/react-hooks`’s `useRealtimeRun`, preserve the call signature where the first argument is the full realtime handle object (not `handle.id`). This is intentional to maintain type-safety and is consistent with the official docs; do not suggest changing the first argument from the handle object to `handle.id`.
Applied to files:
apps/webapp/test/apiRetrieveRunPresenter.groupedLockedWorker.test.tsapps/webapp/test/apiRetrieveRunPresenter.readroute.test.tsapps/webapp/test/runsReplicationInstance.test.tsapps/webapp/test/waitpointPresenter.danglingConnectedRuns.test.tsapps/webapp/test/runsReplicationService.part9.test.tsapps/webapp/test/waitpointPresenter.splitConnectedRuns.test.tsapps/webapp/test/waitpointPresenter.connectedRunsBounded.test.tsapps/webapp/test/waitpointPresenter.readthrough.test.ts
📚 Learning: 2026-06-17T17:13:49.929Z
Learnt from: matt-aitken
Repo: triggerdotdev/trigger.dev PR: 3948
File: apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.bulk-actions.$bulkActionParam/route.tsx:48-62
Timestamp: 2026-06-17T17:13:49.929Z
Learning: In triggerdotdev/trigger.dev, within `dashboardLoader`/`dashboardAction` (or similar context resolver code) whenever you resolve an organization ID from an organization slug for RBAC/enterprise authorization scope, always read from the primary Prisma client (`prisma`), not `$replica`. Using `$replica` can hit replica-lag and cause the RBAC lookup/authorization to run without the correct org scope (bypassing intended role enforcement). Implement the slug→org lookup with `prisma.organization.findFirst(...)` (or equivalent primary-client query) and add an inline comment documenting why the primary client is required (replica lag could lead to unscoped RBAC checks).
Applied to files:
apps/webapp/test/apiRetrieveRunPresenter.groupedLockedWorker.test.tsapps/webapp/test/apiRetrieveRunPresenter.readroute.test.tsapps/webapp/test/runsReplicationInstance.test.tsapps/webapp/test/waitpointPresenter.danglingConnectedRuns.test.tsapps/webapp/test/runsReplicationService.part9.test.tsapps/webapp/test/waitpointPresenter.splitConnectedRuns.test.tsapps/webapp/test/waitpointPresenter.connectedRunsBounded.test.tsapps/webapp/test/waitpointPresenter.readthrough.test.ts
📚 Learning: 2026-06-23T13:04:21.413Z
Learnt from: carderne
Repo: triggerdotdev/trigger.dev PR: 4023
File: apps/webapp/app/services/upsertBranch.server.ts:14-18
Timestamp: 2026-06-23T13:04:21.413Z
Learning: In TypeScript, it’s valid to `import { type X }` and then use `typeof X` in a type-only position, e.g. `type Alias = z.infer<typeof X>`. The `type` modifier suppresses the runtime import, but the type checker still has the full exported type so `z.infer<typeof X>` can resolve correctly. In code reviews, don’t flag this as a TypeScript compile error as long as `typeof X` is used in a type context (e.g., with `z.infer`, `type` aliases, generics), not as a runtime value.
Applied to files:
apps/webapp/test/apiRetrieveRunPresenter.groupedLockedWorker.test.tsapps/webapp/test/apiRetrieveRunPresenter.readroute.test.tsapps/webapp/test/runsReplicationInstance.test.tsapps/webapp/test/waitpointPresenter.danglingConnectedRuns.test.tsapps/webapp/test/runsReplicationService.part9.test.tsapps/webapp/test/waitpointPresenter.splitConnectedRuns.test.tsapps/webapp/test/waitpointPresenter.connectedRunsBounded.test.tsapps/webapp/test/waitpointPresenter.readthrough.test.ts
📚 Learning: 2026-05-07T12:25:18.271Z
Learnt from: d-cs
Repo: triggerdotdev/trigger.dev PR: 3531
File: apps/webapp/test/sentryTraceContext.server.test.ts:9-47
Timestamp: 2026-05-07T12:25:18.271Z
Learning: In the triggerdotdev/trigger.dev webapp test suite, it is acceptable to leave `createInMemoryTracing()` calls that register a global `NodeTracerProvider` without `afterEach`/`afterAll` teardown. Do not flag this as a test-ordering risk when the code follows the established pattern used across webapp tests (e.g., replication service/benchmark/backfiller tests). This is considered safe because `trace.getActiveSpan()` when called outside a `context.with(...)` block reads `AsyncLocalStorage.getStore()` (undefined when no `run()` scope exists), so it falls back to `ROOT_CONTEXT` with no attached span—regardless of which provider is registered.
Applied to files:
apps/webapp/test/apiRetrieveRunPresenter.groupedLockedWorker.test.tsapps/webapp/test/apiRetrieveRunPresenter.readroute.test.tsapps/webapp/test/runsReplicationInstance.test.tsapps/webapp/test/waitpointPresenter.danglingConnectedRuns.test.tsapps/webapp/test/runsReplicationService.part9.test.tsapps/webapp/test/waitpointPresenter.splitConnectedRuns.test.tsapps/webapp/test/waitpointPresenter.connectedRunsBounded.test.tsapps/webapp/test/waitpointPresenter.readthrough.test.ts
📚 Learning: 2026-05-28T20:02:10.647Z
Learnt from: myftija
Repo: triggerdotdev/trigger.dev PR: 3772
File: apps/webapp/test/findOrCreateBackgroundWorker.test.ts:1-1
Timestamp: 2026-05-28T20:02:10.647Z
Learning: In the triggerdotdev/trigger.dev monorepo, for the `apps/webapp` package use the established convention of storing Vitest tests (unit, integration, and e2e) under `apps/webapp/test/` rather than colocating them next to source files. Do not flag files located in `apps/webapp/test/` as violating any rule that says to colocate tests with source.
Applied to files:
apps/webapp/test/apiRetrieveRunPresenter.groupedLockedWorker.test.tsapps/webapp/test/apiRetrieveRunPresenter.readroute.test.tsapps/webapp/test/runsReplicationInstance.test.tsapps/webapp/test/waitpointPresenter.danglingConnectedRuns.test.tsapps/webapp/test/runsReplicationService.part9.test.tsapps/webapp/test/waitpointPresenter.splitConnectedRuns.test.tsapps/webapp/test/waitpointPresenter.connectedRunsBounded.test.tsapps/webapp/test/waitpointPresenter.readthrough.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/apiRetrieveRunPresenter.groupedLockedWorker.test.tsapps/webapp/test/apiRetrieveRunPresenter.readroute.test.tsapps/webapp/test/runsReplicationInstance.test.tsapps/webapp/test/waitpointPresenter.danglingConnectedRuns.test.tsapps/webapp/test/runsReplicationService.part9.test.tsapps/webapp/test/waitpointPresenter.splitConnectedRuns.test.tsapps/webapp/test/waitpointPresenter.connectedRunsBounded.test.tsapps/webapp/test/waitpointPresenter.readthrough.test.ts
📚 Learning: 2026-06-25T18:21:51.905Z
Learnt from: carderne
Repo: triggerdotdev/trigger.dev PR: 4039
File: apps/webapp/app/routes/invite-revoke.tsx:0-0
Timestamp: 2026-06-25T18:21:51.905Z
Learning: During the Zod v4 migration in the triggerdotdev/trigger.dev webapp, ensure any imports from `conform-to/zod` use the Zod-4 subpath: `conform-to/zod/v4` (e.g., `import { parseWithZod } from "conform-to/zod/v4"`). Do not import from the package root `conform-to/zod`, because it is the Zod 3 implementation and may load Zod-3-only symbols (e.g., `ZodBranded`, `ZodEffects`), which can throw at module load (notably with `zod4.4.3`). This should be enforced across `apps/webapp/**/*` where helpers like `parseWithZod` and `conformZodMessage` are used.
Applied to files:
apps/webapp/test/apiRetrieveRunPresenter.groupedLockedWorker.test.tsapps/webapp/test/apiRetrieveRunPresenter.readroute.test.tsapps/webapp/test/runsReplicationInstance.test.tsapps/webapp/test/waitpointPresenter.danglingConnectedRuns.test.tsapps/webapp/test/runsReplicationService.part9.test.tsapps/webapp/test/waitpointPresenter.splitConnectedRuns.test.tsapps/webapp/test/waitpointPresenter.connectedRunsBounded.test.tsapps/webapp/test/waitpointPresenter.readthrough.test.ts
📚 Learning: 2026-07-03T17:10:21.498Z
Learnt from: 0ski
Repo: triggerdotdev/trigger.dev PR: 4148
File: apps/webapp/app/models/orgMember.server.ts:149-168
Timestamp: 2026-07-03T17:10:21.498Z
Learning: In triggerdotdev/trigger.dev, `User.email` (Prisma schema: `internal-packages/database/prisma/schema.prisma`) currently does NOT use `citext` and does NOT have a `lower(email)` functional unique index. Therefore, do not introduce Prisma queries like `where: { email: { equals: <value>, mode: "insensitive" } }` (or any case-insensitive lookup) against `User.email`, because it can force sequential scans of the `users` table under load. During review, ensure email is normalized (e.g., lowercased/trimmed) before both writes and subsequent lookups, and if true case-insensitive behavior/uniqueness is required, implement it via a separate app-wide migration (e.g., switch to `citext` and/or add a functional unique index with backfill) rather than bolting it onto individual feature PRs.
Applied to files:
apps/webapp/test/apiRetrieveRunPresenter.groupedLockedWorker.test.tsapps/webapp/test/apiRetrieveRunPresenter.readroute.test.tsapps/webapp/test/runsReplicationInstance.test.tsapps/webapp/test/waitpointPresenter.danglingConnectedRuns.test.tsapps/webapp/test/runsReplicationService.part9.test.tsapps/webapp/test/waitpointPresenter.splitConnectedRuns.test.tsapps/webapp/test/waitpointPresenter.connectedRunsBounded.test.tsapps/webapp/test/waitpointPresenter.readthrough.test.ts
📚 Learning: 2026-05-18T14:40:02.173Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3658
File: packages/core/src/v3/realtimeStreams/manager.test.ts:1-147
Timestamp: 2026-05-18T14:40:02.173Z
Learning: In the triggerdotdev/trigger.dev repo, the policy “Never mock anything — use testcontainers instead” should only be enforced for integration tests that interact with real external services (e.g., Redis, Postgres) via actual infrastructure. For unit tests that exercise pure in-memory logic (e.g., cache semantics) it is OK to stub collaborators such as `ApiClient` using Vitest (`vi.fn()`) to assert call counts or control behavior. Do not flag `vi.fn()`-based `ApiClient` stubs in unit tests as violations of the testcontainers policy.
Applied to files:
apps/webapp/test/apiRetrieveRunPresenter.groupedLockedWorker.test.tsapps/webapp/test/apiRetrieveRunPresenter.readroute.test.tsapps/webapp/test/runsReplicationInstance.test.tsapps/webapp/test/waitpointPresenter.danglingConnectedRuns.test.tsapps/webapp/test/runsReplicationService.part9.test.tsapps/webapp/test/waitpointPresenter.splitConnectedRuns.test.tsapps/webapp/test/waitpointPresenter.connectedRunsBounded.test.tsapps/webapp/test/waitpointPresenter.readthrough.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/test/apiRetrieveRunPresenter.groupedLockedWorker.test.tsapps/webapp/test/apiRetrieveRunPresenter.readroute.test.tsapps/webapp/test/runsReplicationInstance.test.tsapps/webapp/test/waitpointPresenter.danglingConnectedRuns.test.tsapps/webapp/test/runsReplicationService.part9.test.tsapps/webapp/test/waitpointPresenter.splitConnectedRuns.test.tsapps/webapp/test/waitpointPresenter.connectedRunsBounded.test.tsapps/webapp/test/waitpointPresenter.readthrough.test.ts
📚 Learning: 2026-06-09T17:58:04.699Z
Learnt from: 0ski
Repo: triggerdotdev/trigger.dev PR: 3879
File: apps/webapp/app/models/vercelIntegration.server.ts:619-630
Timestamp: 2026-06-09T17:58:04.699Z
Learning: In this codebase, outbound raw `fetch` calls should typically rely on Node/undici’s default request timeout (about ~300s) rather than adding a per-call `AbortController` + `setTimeout` wrapper inside individual functions (e.g. in files like `apps/webapp/app/models/vercelIntegration.server.ts`). During code review, do not flag the absence of a per-call timeout on a single `fetch` as an issue; if per-call timeouts are needed, they should be implemented via a codebase-wide convention (e.g., a shared fetch wrapper or documented pattern) rather than ad-hoc per-function changes.
Applied to files:
apps/webapp/test/apiRetrieveRunPresenter.groupedLockedWorker.test.tsapps/webapp/test/apiRetrieveRunPresenter.readroute.test.tsapps/webapp/test/runsReplicationInstance.test.tsapps/webapp/test/waitpointPresenter.danglingConnectedRuns.test.tsapps/webapp/test/runsReplicationService.part9.test.tsapps/webapp/test/waitpointPresenter.splitConnectedRuns.test.tsapps/webapp/test/waitpointPresenter.connectedRunsBounded.test.tsapps/webapp/test/waitpointPresenter.readthrough.test.ts
📚 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/apiRetrieveRunPresenter.groupedLockedWorker.test.tsapps/webapp/test/apiRetrieveRunPresenter.readroute.test.tsapps/webapp/test/runsReplicationInstance.test.tsapps/webapp/test/waitpointPresenter.danglingConnectedRuns.test.tsapps/webapp/test/runsReplicationService.part9.test.tsapps/webapp/test/waitpointPresenter.splitConnectedRuns.test.tsapps/webapp/test/waitpointPresenter.connectedRunsBounded.test.tsapps/webapp/test/waitpointPresenter.readthrough.test.ts
🔇 Additional comments (10)
apps/webapp/test/waitpointPresenter.splitConnectedRuns.test.ts (2)
64-90: See consolidated duplication comment inapps/webapp/test/waitpointPresenter.readthrough.test.tsregarding the repeatedmakeRunStorehelper.
152-232: LGTM!apps/webapp/test/waitpointPresenter.connectedRunsBounded.test.ts (2)
66-92: See consolidated duplication comment inapps/webapp/test/waitpointPresenter.readthrough.test.tsregarding the repeatedmakeRunStorehelper.
181-234: LGTM!apps/webapp/test/apiRetrieveRunPresenter.groupedLockedWorker.test.ts (1)
1-266: LGTM!apps/webapp/test/apiRetrieveRunPresenter.readroute.test.ts (1)
1-1: LGTM!Also applies to: 318-318, 384-384
apps/webapp/test/runsReplicationInstance.test.ts (1)
400-428: 🎯 Functional CorrectnessGood fix: replaces flaky fixed sleep with bounded readiness polling.
Verified
expect(value, message)is a documented Vitest-specific API (unlike Jest) for custom failure messages, so Line 421 usage is valid.apps/webapp/test/waitpointPresenter.danglingConnectedRuns.test.ts (1)
1-223: LGTM!apps/webapp/test/runsReplicationService.part9.test.ts (1)
116-166: Good fix: deterministic in-memory metric test replaces flaky live-container polling.apps/webapp/test/waitpointPresenter.readthrough.test.ts (1)
285-303: LGTM!Also applies to: 401-410, 444-451
Model single-DB passthrough as one run store backing both legs, injected explicitly instead of via the global singleton (which the full-suite run polluted across files, flipping the connected-runs read shape). Assert the waitpoint resolves on the single client rather than an exact findFirst count, which is a run-store implementation detail.
ericallam
approved these changes
Jul 13, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Run-graph data (runs, batches, waitpoints, and their related tables) can now live in a database separate from the control plane, with every read and write routed to the correct database by each run's residency. This makes reading and writing run data more reliable once the two are split, and is a no-op for single-database installs.
Design
20260710120000_drop_remaining_run_graph_seam_foreign_keysdrops the foreign keys that still crossed the run-graph / control-plane seam, which is what lets the two live in separate databases.The split stays off unless explicitly enabled and the two databases are confirmed physically distinct; startup fails closed otherwise.
Verified by running the full dashboard end-to-end suite against both a single-database configuration and a three-database configuration (control plane, the new database, and a physically separate legacy database), with runs on both residencies. No misrouted reads in either configuration.