Conversation
…resources/platform-changelogs Root cause recap: Background fetcher (useRecentChangelogs polling /resources/platform-changelogs every 60s) was the first thing to hit requireUserId after a session loss. That stuffed its own URL into ?redirectTo=..., the login flow stored it in a cookie, and /magic faithfully redirected the user to a JSON-only fetcher route after sign-in → blank page. Pre-existing bug, surfaced more often by the new session-expiry feature. Fixed by sanitizing redirectTo at three layers.
Summary: redirectWithErrorMessage is async, but the call site was missing await — so throw was throwing a Promise<Response> instead of a Response. Remix navigation paths happened to coerce it, but fetcher loaders (like the /resources/platform-changelogs poll) blew up with a 500. Adding await fixes both.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds end-to-end configurable session-duration support: DB migration adding User.sessionDuration and Organization.maxSessionDuration; a new session-duration service with options, allowlist, org-cap resolution, issued-at tracking, and commit helpers (eager and lazy); session expiry enforcement in session handling; default session duration constant; UI components and routes for user and admin session-duration management; updates to auth flows to use authenticated-session commits and lazy issued-at backfill; stricter redirect sanitization; and tests covering session-duration logic. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
internal-packages/database/prisma/migrations/20260428140746_add_session_duration_columns/migration.sql (1)
2-5: Consider DB-level bounds for session duration columns.App-level validation is good, but direct SQL writes can still store invalid values. A
CHECKconstraint for positive durations (and optional max bounds) would harden data integrity.💡 Example constraint hardening
ALTER TABLE "public"."User" ADD COLUMN "sessionDuration" INTEGER NOT NULL DEFAULT 31556952; +ALTER TABLE "public"."User" + ADD CONSTRAINT "User_sessionDuration_positive_chk" CHECK ("sessionDuration" > 0); ALTER TABLE "public"."Organization" ADD COLUMN "maxSessionDuration" INTEGER; +ALTER TABLE "public"."Organization" + ADD CONSTRAINT "Organization_maxSessionDuration_positive_chk" + CHECK ("maxSessionDuration" IS NULL OR "maxSessionDuration" > 0);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal-packages/database/prisma/migrations/20260428140746_add_session_duration_columns/migration.sql` around lines 2 - 5, Add DB-level CHECK constraints to harden the new duration columns: when adding "sessionDuration" to the User table and "maxSessionDuration" to the Organization table, include constraints that enforce positive values (e.g., > 0) and optional upper bounds (e.g., <= 31556952 or another domain-specific max) and give each constraint a descriptive name (e.g., user_session_duration_positive_chk, org_max_session_duration_bounds_chk). Modify the migration SQL to either define the CHECK in the ADD COLUMN statements or add separate ALTER TABLE ... ADD CONSTRAINT ... CHECK (...) statements so direct SQL writes cannot insert invalid durations; ensure the User default remains and that Organization allows NULL while the constraint allows NULL or checks IS NULL OR (...).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/webapp/app/routes/admin.api.v1.orgs`.$organizationId.session-duration.ts:
- Around line 25-29: The update call using prisma.organization.update (where: {
id: organizationId }) can throw Prisma P2025 if the organizationId doesn't
exist; wrap the update in a try/catch or pre-check existence with
prisma.organization.findUnique({ where: { id: organizationId } }) and if not
found return a 404 response instead of letting the exception bubble;
alternatively catch the error from prisma.organization.update, detect error.code
=== 'P2025' and return a 404 (otherwise rethrow or return 500), ensuring the
handler returns the selected fields (id, slug, maxSessionDuration) on success.
In `@apps/webapp/app/routes/resources.account.mfa.setup/MfaToggle.tsx`:
- Around line 23-30: The Switch component rendering (Switch with id "mfa", props
checked={isEnabled} and onCheckedChange={onToggle}) currently lacks a
programmatic label; add an accessible name by either restoring a semantic label
or adding aria-label or aria-labelledby pointing to the visible text label
element (e.g., ensure the visible label element has an id and set
aria-labelledby="that-id" on Switch) so screen readers can identify the control.
In `@apps/webapp/app/services/sessionDuration.server.ts`:
- Around line 76-82: The Prisma query uses client.user.findUnique which violates
the guideline to prefer findFirst; replace client.user.findUnique({ where: { id:
userId }, select: { sessionDuration: true } }) with client.user.findFirst({
where: { id: userId }, select: { sessionDuration: true } }) so the Promise.all
call that assigns [user, orgCap] uses findFirst for the user lookup; keep the
same where/select shape and ensure any other similar calls use findFirst instead
of findUnique (reference: client.user.findUnique, user variable,
getOrganizationSessionCap).
In `@apps/webapp/app/utils.ts`:
- Around line 9-10: The NON_NAVIGABLE_PREFIXES entry for "/admin/" is too broad
and blocks legitimate admin pages; update NON_NAVIGABLE_PREFIXES in utils.ts to
remove the general "/admin/" prefix and instead include only non-navigable admin
subpaths (for example "/admin/api/" or other backend-only prefixes you actually
want blocked), and if any specific admin routes must be blocked add them to
NON_NAVIGABLE_EXACT; edit the array constant named NON_NAVIGABLE_PREFIXES (and
adjust NON_NAVIGABLE_EXACT if needed) so navigable routes like "/admin/*" remain
allowed.
---
Nitpick comments:
In
`@internal-packages/database/prisma/migrations/20260428140746_add_session_duration_columns/migration.sql`:
- Around line 2-5: Add DB-level CHECK constraints to harden the new duration
columns: when adding "sessionDuration" to the User table and
"maxSessionDuration" to the Organization table, include constraints that enforce
positive values (e.g., > 0) and optional upper bounds (e.g., <= 31556952 or
another domain-specific max) and give each constraint a descriptive name (e.g.,
user_session_duration_positive_chk, org_max_session_duration_bounds_chk). Modify
the migration SQL to either define the CHECK in the ADD COLUMN statements or add
separate ALTER TABLE ... ADD CONSTRAINT ... CHECK (...) statements so direct SQL
writes cannot insert invalid durations; ensure the User default remains and that
Organization allows NULL while the constraint allows NULL or checks IS NULL OR
(...).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d4f34041-28bd-417a-a5df-5285b7e052f1
📒 Files selected for processing (19)
apps/webapp/app/root.tsxapps/webapp/app/routes/account.security/route.tsxapps/webapp/app/routes/admin.api.v1.orgs.$organizationId.session-duration.tsapps/webapp/app/routes/auth.github.callback.tsxapps/webapp/app/routes/auth.google.callback.tsxapps/webapp/app/routes/login.magic/route.tsxapps/webapp/app/routes/login.mfa/route.tsxapps/webapp/app/routes/magic.tsxapps/webapp/app/routes/resources.account.mfa.setup/MfaToggle.tsxapps/webapp/app/routes/resources.account.mfa.setup/route.tsxapps/webapp/app/routes/resources.account.session-duration/SessionDurationSetting.tsxapps/webapp/app/routes/resources.account.session-duration/route.tsxapps/webapp/app/services/session.server.tsapps/webapp/app/services/sessionDuration.server.tsapps/webapp/app/services/sessionStorage.server.tsapps/webapp/app/utils.tsapps/webapp/test/sessionDuration.test.tsinternal-packages/database/prisma/migrations/20260428140746_add_session_duration_columns/migration.sqlinternal-packages/database/prisma/schema.prisma
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (30)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / e2e-webapp / 🧪 E2E Tests: Webapp
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: sdk-compat / Cloudflare Workers
- GitHub Check: sdk-compat / Bun Runtime
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: sdk-compat / Deno Runtime
- GitHub Check: sdk-compat / Node.js 22.12 (ubuntu-latest)
- GitHub Check: sdk-compat / Node.js 20.20 (ubuntu-latest)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: typecheck / typecheck
- GitHub Check: audit
- GitHub Check: Analyze (javascript-typescript)
🧰 Additional context used
📓 Path-based instructions (14)
**/*.{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/app/routes/login.mfa/route.tsxapps/webapp/app/routes/auth.github.callback.tsxapps/webapp/app/routes/auth.google.callback.tsxapps/webapp/app/routes/magic.tsxapps/webapp/app/routes/admin.api.v1.orgs.$organizationId.session-duration.tsapps/webapp/app/routes/login.magic/route.tsxapps/webapp/app/root.tsxapps/webapp/app/routes/resources.account.session-duration/route.tsxapps/webapp/app/services/sessionStorage.server.tsapps/webapp/app/utils.tsapps/webapp/app/routes/resources.account.mfa.setup/route.tsxapps/webapp/app/routes/resources.account.mfa.setup/MfaToggle.tsxapps/webapp/app/services/session.server.tsapps/webapp/app/routes/account.security/route.tsxapps/webapp/app/routes/resources.account.session-duration/SessionDurationSetting.tsxapps/webapp/test/sessionDuration.test.tsapps/webapp/app/services/sessionDuration.server.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/app/routes/login.mfa/route.tsxapps/webapp/app/routes/auth.github.callback.tsxapps/webapp/app/routes/auth.google.callback.tsxapps/webapp/app/routes/magic.tsxapps/webapp/app/routes/admin.api.v1.orgs.$organizationId.session-duration.tsapps/webapp/app/routes/login.magic/route.tsxapps/webapp/app/root.tsxapps/webapp/app/routes/resources.account.session-duration/route.tsxapps/webapp/app/services/sessionStorage.server.tsapps/webapp/app/utils.tsapps/webapp/app/routes/resources.account.mfa.setup/route.tsxapps/webapp/app/routes/resources.account.mfa.setup/MfaToggle.tsxapps/webapp/app/services/session.server.tsapps/webapp/app/routes/account.security/route.tsxapps/webapp/app/routes/resources.account.session-duration/SessionDurationSetting.tsxapps/webapp/test/sessionDuration.test.tsapps/webapp/app/services/sessionDuration.server.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use function declarations instead of default exports
Add crumbs as you write code using
//@Crumbscomments or `// `#region` `@crumbsblocks. These are temporary debug instrumentation and must be stripped usingagentcrumbs stripbefore merge.
Files:
apps/webapp/app/routes/login.mfa/route.tsxapps/webapp/app/routes/auth.github.callback.tsxapps/webapp/app/routes/auth.google.callback.tsxapps/webapp/app/routes/magic.tsxapps/webapp/app/routes/admin.api.v1.orgs.$organizationId.session-duration.tsapps/webapp/app/routes/login.magic/route.tsxapps/webapp/app/root.tsxapps/webapp/app/routes/resources.account.session-duration/route.tsxapps/webapp/app/services/sessionStorage.server.tsapps/webapp/app/utils.tsapps/webapp/app/routes/resources.account.mfa.setup/route.tsxapps/webapp/app/routes/resources.account.mfa.setup/MfaToggle.tsxapps/webapp/app/services/session.server.tsapps/webapp/app/routes/account.security/route.tsxapps/webapp/app/routes/resources.account.session-duration/SessionDurationSetting.tsxapps/webapp/test/sessionDuration.test.tsapps/webapp/app/services/sessionDuration.server.ts
**/*.{js,ts,jsx,tsx,json,md,yaml,yml}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using Prettier before committing
Files:
apps/webapp/app/routes/login.mfa/route.tsxapps/webapp/app/routes/auth.github.callback.tsxapps/webapp/app/routes/auth.google.callback.tsxapps/webapp/app/routes/magic.tsxapps/webapp/app/routes/admin.api.v1.orgs.$organizationId.session-duration.tsapps/webapp/app/routes/login.magic/route.tsxapps/webapp/app/root.tsxapps/webapp/app/routes/resources.account.session-duration/route.tsxapps/webapp/app/services/sessionStorage.server.tsapps/webapp/app/utils.tsapps/webapp/app/routes/resources.account.mfa.setup/route.tsxapps/webapp/app/routes/resources.account.mfa.setup/MfaToggle.tsxapps/webapp/app/services/session.server.tsapps/webapp/app/routes/account.security/route.tsxapps/webapp/app/routes/resources.account.session-duration/SessionDurationSetting.tsxapps/webapp/test/sessionDuration.test.tsapps/webapp/app/services/sessionDuration.server.ts
**/*.ts{,x}
📄 CodeRabbit inference engine (CLAUDE.md)
Always import from
@trigger.dev/sdkwhen writing Trigger.dev tasks. Never use@trigger.dev/sdk/v3or deprecatedclient.defineJob.
Files:
apps/webapp/app/routes/login.mfa/route.tsxapps/webapp/app/routes/auth.github.callback.tsxapps/webapp/app/routes/auth.google.callback.tsxapps/webapp/app/routes/magic.tsxapps/webapp/app/routes/admin.api.v1.orgs.$organizationId.session-duration.tsapps/webapp/app/routes/login.magic/route.tsxapps/webapp/app/root.tsxapps/webapp/app/routes/resources.account.session-duration/route.tsxapps/webapp/app/services/sessionStorage.server.tsapps/webapp/app/utils.tsapps/webapp/app/routes/resources.account.mfa.setup/route.tsxapps/webapp/app/routes/resources.account.mfa.setup/MfaToggle.tsxapps/webapp/app/services/session.server.tsapps/webapp/app/routes/account.security/route.tsxapps/webapp/app/routes/resources.account.session-duration/SessionDurationSetting.tsxapps/webapp/test/sessionDuration.test.tsapps/webapp/app/services/sessionDuration.server.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/corepathUse named constants for sentinel/placeholder values (e.g.
const UNSET_VALUE = '__unset__') instead of raw string literals scattered across comparisons
Files:
apps/webapp/app/routes/login.mfa/route.tsxapps/webapp/app/routes/auth.github.callback.tsxapps/webapp/app/routes/auth.google.callback.tsxapps/webapp/app/routes/magic.tsxapps/webapp/app/routes/admin.api.v1.orgs.$organizationId.session-duration.tsapps/webapp/app/routes/login.magic/route.tsxapps/webapp/app/root.tsxapps/webapp/app/routes/resources.account.session-duration/route.tsxapps/webapp/app/services/sessionStorage.server.tsapps/webapp/app/utils.tsapps/webapp/app/routes/resources.account.mfa.setup/route.tsxapps/webapp/app/routes/resources.account.mfa.setup/MfaToggle.tsxapps/webapp/app/services/session.server.tsapps/webapp/app/routes/account.security/route.tsxapps/webapp/app/routes/resources.account.session-duration/SessionDurationSetting.tsxapps/webapp/test/sessionDuration.test.tsapps/webapp/app/services/sessionDuration.server.ts
apps/webapp/**/*.{tsx,jsx}
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
Only use
useCallback/useMemofor context provider values, expensive derived data that is a dependency elsewhere, or stable refs required by a dependency array. Don't wrap ordinary event handlers or trivial computations
Files:
apps/webapp/app/routes/login.mfa/route.tsxapps/webapp/app/routes/auth.github.callback.tsxapps/webapp/app/routes/auth.google.callback.tsxapps/webapp/app/routes/magic.tsxapps/webapp/app/routes/login.magic/route.tsxapps/webapp/app/root.tsxapps/webapp/app/routes/resources.account.session-duration/route.tsxapps/webapp/app/routes/resources.account.mfa.setup/route.tsxapps/webapp/app/routes/resources.account.mfa.setup/MfaToggle.tsxapps/webapp/app/routes/account.security/route.tsxapps/webapp/app/routes/resources.account.session-duration/SessionDurationSetting.tsx
**/*.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/app/routes/admin.api.v1.orgs.$organizationId.session-duration.tsapps/webapp/app/services/sessionStorage.server.tsapps/webapp/app/utils.tsapps/webapp/app/services/session.server.tsapps/webapp/test/sessionDuration.test.tsapps/webapp/app/services/sessionDuration.server.ts
apps/webapp/**/*.server.ts
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
apps/webapp/**/*.server.ts: Never userequest.signalfor detecting client disconnects. UsegetRequestAbortSignal()fromapp/services/httpAsyncStorage.server.tsinstead, which is wired directly to Expressres.on('close')and fires reliably
Access environment variables viaenvexport fromapp/env.server.ts. Never useprocess.envdirectly
Always usefindFirstinstead offindUniquein Prisma queries.findUniquehas an implicit DataLoader that batches concurrent calls and has active bugs even in Prisma 6.x (uppercase UUIDs returning null, composite key SQL correctness issues, 5-10x worse performance).findFirstis never batched and avoids this entire class of issues
Files:
apps/webapp/app/services/sessionStorage.server.tsapps/webapp/app/services/session.server.tsapps/webapp/app/services/sessionDuration.server.ts
internal-packages/database/**/prisma/migrations/*/*.sql
📄 CodeRabbit inference engine (internal-packages/database/CLAUDE.md)
internal-packages/database/**/prisma/migrations/*/*.sql: Clean up generated Prisma migrations by removing extraneous lines for junction tables (_BackgroundWorkerToBackgroundWorkerFile,_BackgroundWorkerToTaskQueue,_TaskRunToTaskRunTag,_WaitpointRunConnections,_completedWaitpoints) and indexes (SecretStore_key_idx, variousTaskRunindexes) unless explicitly added
When adding indexes to existing tables, useCREATE INDEX CONCURRENTLY IF NOT EXISTSto avoid table locks in production, and place each concurrent index in its own separate migration file
Indexes on newly created tables can useCREATE INDEXwithout CONCURRENTLY and can be combined in the same migration file as theCREATE TABLEstatement
When adding an index on a new column in an existing table, use two separate migrations: first forALTER TABLE ... ADD COLUMN IF NOT EXISTS ..., then forCREATE INDEX CONCURRENTLY IF NOT EXISTS ...in its own file
Files:
internal-packages/database/prisma/migrations/20260428140746_add_session_duration_columns/migration.sql
**/*.{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/sessionDuration.test.ts
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.test.{ts,tsx,js,jsx}: Test files should live beside the files under test and use descriptivedescribeanditblocks
Tests should avoid mocks or stubs and use the helpers from@internal/testcontainerswhen Redis or Postgres are needed
Use vitest for running unit tests
Files:
apps/webapp/test/sessionDuration.test.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.test.{ts,tsx}: Use vitest exclusively for testing. Never mock anything — use testcontainers instead.
Place test files next to source files with naming patternMyService.ts->MyService.test.ts.
For Redis/PostgreSQL tests in vitest, use testcontainers helpers:redisTest,postgresTest, orcontainerTestimported from@internal/testcontainers.
Files:
apps/webapp/test/sessionDuration.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 testableFor testable code, never import
env.server.tsin test files. Pass configuration as options instead (e.g.,realtimeClient.server.tstakes config as constructor arg,realtimeClientGlobal.server.tscreates singleton with env config)
Files:
apps/webapp/test/sessionDuration.test.ts
🧠 Learnings (28)
📓 Common learnings
Learnt from: myftija
Repo: triggerdotdev/trigger.dev PR: 2463
File: apps/webapp/app/routes/_app.github.callback/route.tsx:33-44
Timestamp: 2025-09-02T11:27:36.336Z
Learning: In the GitHub App installation callback flow in apps/webapp/app/routes/_app.github.callback/route.tsx, the install session cookie is not cleared after use due to interface limitations with redirectWithSuccessMessage/redirectWithErrorMessage not supporting custom headers. The maintainer accepts this design as the 1-hour cookie expiration provides sufficient protection against replay attacks.
📚 Learning: 2025-09-02T11:27:36.336Z
Learnt from: myftija
Repo: triggerdotdev/trigger.dev PR: 2463
File: apps/webapp/app/routes/_app.github.callback/route.tsx:33-44
Timestamp: 2025-09-02T11:27:36.336Z
Learning: In the GitHub App installation callback flow in apps/webapp/app/routes/_app.github.callback/route.tsx, the install session cookie is not cleared after use due to interface limitations with redirectWithSuccessMessage/redirectWithErrorMessage not supporting custom headers. The maintainer accepts this design as the 1-hour cookie expiration provides sufficient protection against replay attacks.
Applied to files:
apps/webapp/app/routes/login.mfa/route.tsxapps/webapp/app/routes/auth.github.callback.tsxapps/webapp/app/routes/auth.google.callback.tsxapps/webapp/app/routes/magic.tsxapps/webapp/app/routes/login.magic/route.tsxapps/webapp/app/root.tsxapps/webapp/app/routes/resources.account.session-duration/route.tsx
📚 Learning: 2025-09-02T11:18:06.602Z
Learnt from: myftija
Repo: triggerdotdev/trigger.dev PR: 2463
File: apps/webapp/app/services/gitHubSession.server.ts:31-36
Timestamp: 2025-09-02T11:18:06.602Z
Learning: In the GitHub App installation flow in apps/webapp/app/services/gitHubSession.server.ts, the redirectTo parameter stored in httpOnly session cookies is considered acceptable without additional validation by the maintainer, as the httpOnly cookie provides sufficient security for this use case.
Applied to files:
apps/webapp/app/routes/login.mfa/route.tsxapps/webapp/app/routes/auth.github.callback.tsxapps/webapp/app/routes/auth.google.callback.tsxapps/webapp/app/routes/magic.tsxapps/webapp/app/routes/login.magic/route.tsx
📚 Learning: 2026-02-03T18:27:40.429Z
Learnt from: 0ski
Repo: triggerdotdev/trigger.dev PR: 2994
File: apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.environment-variables/route.tsx:553-555
Timestamp: 2026-02-03T18:27:40.429Z
Learning: In apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.environment-variables/route.tsx, the menu buttons (e.g., Edit with PencilSquareIcon) in the TableCellMenu are intentionally icon-only with no text labels as a compact UI pattern. This is a deliberate design choice for this route; preserve the icon-only behavior for consistency in this file.
Applied to files:
apps/webapp/app/routes/login.mfa/route.tsxapps/webapp/app/routes/auth.github.callback.tsxapps/webapp/app/routes/auth.google.callback.tsxapps/webapp/app/routes/magic.tsxapps/webapp/app/routes/login.magic/route.tsxapps/webapp/app/routes/resources.account.session-duration/route.tsxapps/webapp/app/routes/resources.account.mfa.setup/route.tsxapps/webapp/app/routes/resources.account.mfa.setup/MfaToggle.tsxapps/webapp/app/routes/account.security/route.tsxapps/webapp/app/routes/resources.account.session-duration/SessionDurationSetting.tsx
📚 Learning: 2026-02-11T16:37:32.429Z
Learnt from: matt-aitken
Repo: triggerdotdev/trigger.dev PR: 3019
File: apps/webapp/app/components/primitives/charts/Card.tsx:26-30
Timestamp: 2026-02-11T16:37:32.429Z
Learning: In projects using react-grid-layout, avoid relying on drag-handle class to imply draggability. Ensure drag-handle elements only affect dragging when the parent grid item is configured draggable in the layout; conditionally apply cursor styles based on the draggable prop. This improves correctness and accessibility.
Applied to files:
apps/webapp/app/routes/login.mfa/route.tsxapps/webapp/app/routes/auth.github.callback.tsxapps/webapp/app/routes/auth.google.callback.tsxapps/webapp/app/routes/magic.tsxapps/webapp/app/routes/login.magic/route.tsxapps/webapp/app/root.tsxapps/webapp/app/routes/resources.account.session-duration/route.tsxapps/webapp/app/routes/resources.account.mfa.setup/route.tsxapps/webapp/app/routes/resources.account.mfa.setup/MfaToggle.tsxapps/webapp/app/routes/account.security/route.tsxapps/webapp/app/routes/resources.account.session-duration/SessionDurationSetting.tsx
📚 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/app/routes/login.mfa/route.tsxapps/webapp/app/routes/auth.github.callback.tsxapps/webapp/app/routes/auth.google.callback.tsxapps/webapp/app/routes/magic.tsxapps/webapp/app/routes/admin.api.v1.orgs.$organizationId.session-duration.tsapps/webapp/app/routes/login.magic/route.tsxapps/webapp/app/root.tsxapps/webapp/app/routes/resources.account.session-duration/route.tsxapps/webapp/app/services/sessionStorage.server.tsapps/webapp/app/utils.tsapps/webapp/app/routes/resources.account.mfa.setup/route.tsxapps/webapp/app/routes/resources.account.mfa.setup/MfaToggle.tsxapps/webapp/app/services/session.server.tsapps/webapp/app/routes/account.security/route.tsxapps/webapp/app/routes/resources.account.session-duration/SessionDurationSetting.tsxapps/webapp/test/sessionDuration.test.tsapps/webapp/app/services/sessionDuration.server.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/app/routes/login.mfa/route.tsxapps/webapp/app/routes/auth.github.callback.tsxapps/webapp/app/routes/auth.google.callback.tsxapps/webapp/app/routes/magic.tsxapps/webapp/app/routes/admin.api.v1.orgs.$organizationId.session-duration.tsapps/webapp/app/routes/login.magic/route.tsxapps/webapp/app/root.tsxapps/webapp/app/routes/resources.account.session-duration/route.tsxapps/webapp/app/services/sessionStorage.server.tsapps/webapp/app/utils.tsapps/webapp/app/routes/resources.account.mfa.setup/route.tsxapps/webapp/app/routes/resources.account.mfa.setup/MfaToggle.tsxapps/webapp/app/services/session.server.tsapps/webapp/app/routes/account.security/route.tsxapps/webapp/app/routes/resources.account.session-duration/SessionDurationSetting.tsxapps/webapp/test/sessionDuration.test.tsapps/webapp/app/services/sessionDuration.server.ts
📚 Learning: 2026-04-02T19:18:26.255Z
Learnt from: samejr
Repo: triggerdotdev/trigger.dev PR: 3319
File: apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.bulk-actions/route.tsx:179-189
Timestamp: 2026-04-02T19:18:26.255Z
Learning: In this repo’s route components that render the Inspector `ResizablePanelGroup` panels, it’s acceptable to pass `collapsed={!isShowingInspector}` together with a no-op `onCollapseChange={() => {}}` when panel visibility is intentionally controlled only by route parameters (e.g., `*Param` search/route params) rather than user drag/collapse interactions. Do not flag an empty/no-op `onCollapseChange` as “missing wiring” in these cases; only flag it when collapse state is expected to change based on user interaction.
Applied to files:
apps/webapp/app/routes/login.mfa/route.tsxapps/webapp/app/routes/login.magic/route.tsxapps/webapp/app/routes/resources.account.session-duration/route.tsxapps/webapp/app/routes/resources.account.mfa.setup/route.tsxapps/webapp/app/routes/account.security/route.tsx
📚 Learning: 2026-03-13T13:45:39.411Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3213
File: apps/webapp/app/routes/admin.llm-models.missing.$model.tsx:19-21
Timestamp: 2026-03-13T13:45:39.411Z
Learning: In `apps/webapp/app/routes/admin.llm-models.missing.$model.tsx`, the `decodeURIComponent(params.model ?? "")` call is intentionally unguarded. Remix route params are decoded at the routing layer before reaching the loader, so malformed percent-encoding is rejected upstream. The page is also admin-only, so the risk is minimal and no try-catch is warranted.
Applied to files:
apps/webapp/app/routes/magic.tsxapps/webapp/app/routes/login.magic/route.tsx
📚 Learning: 2026-02-04T16:34:48.876Z
Learnt from: 0ski
Repo: triggerdotdev/trigger.dev PR: 2994
File: apps/webapp/app/routes/vercel.connect.tsx:13-27
Timestamp: 2026-02-04T16:34:48.876Z
Learning: In apps/webapp/app/routes/vercel.connect.tsx, configurationId may be absent for "dashboard" flows but must be present for "marketplace" flows. Enforce this with a Zod superRefine and pass installationId to repository methods only when configurationId is defined (omit the field otherwise).
Applied to files:
apps/webapp/app/routes/login.magic/route.tsxapps/webapp/app/routes/resources.account.mfa.setup/route.tsx
📚 Learning: 2026-04-20T15:05:59.661Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3417
File: apps/webapp/app/routes/realtime.v1.sessions.$session.$io.append.ts:20-31
Timestamp: 2026-04-20T15:05:59.661Z
Learning: In `apps/webapp/app/routes/realtime.v1.sessions.$session.$io.append.ts`, the `MAX_APPEND_BODY_BYTES` cap is intentionally set to `1024 * 512` (512 KiB). The maintainer explicitly decided against lowering it to 128 KiB: the all-quotes worst-case JSON-escaping expansion that could exceed S2's 1 MiB per-record limit is considered pathological and not representative of real-world payloads (chat tokens, tool-call JSON, structured data). If overflow becomes a problem in practice, the preferred mitigation is an encoded-size guard inside `appendPart` itself. Do not flag this cap as a potential S2 overflow issue in future reviews.
Applied to files:
apps/webapp/app/services/sessionStorage.server.ts
📚 Learning: 2026-03-26T09:02:07.973Z
Learnt from: myftija
Repo: triggerdotdev/trigger.dev PR: 3274
File: apps/webapp/app/services/runsReplicationService.server.ts:922-924
Timestamp: 2026-03-26T09:02:07.973Z
Learning: When parsing Trigger.dev task run annotations in server-side services, keep `TaskRun.annotations` strictly conforming to the `RunAnnotations` schema from `trigger.dev/core/v3`. If the code already uses `RunAnnotations.safeParse` (e.g., in a `#parseAnnotations` helper), treat that as intentional/necessary for atomic, schema-accurate annotation handling. Do not recommend relaxing the annotation payload schema or using a permissive “passthrough” parse path, since the annotations are expected to be written atomically in one operation and should not contain partial/legacy payloads that would require a looser parser.
Applied to files:
apps/webapp/app/services/sessionStorage.server.tsapps/webapp/app/services/session.server.tsapps/webapp/app/services/sessionDuration.server.ts
📚 Learning: 2026-04-02T20:25:54.203Z
Learnt from: samejr
Repo: triggerdotdev/trigger.dev PR: 3319
File: apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.models._index/route.tsx:1100-1101
Timestamp: 2026-04-02T20:25:54.203Z
Learning: In `apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.models._index/route.tsx`, the `useFrozenValue(selectedModel)` + `displayModel = selectedModel ?? frozenModel` pattern is intentional. It keeps `ModelDetailPanel` rendered during the collapse animation even after `selectedModel` is cleared. The `key={displayModel.friendlyId}` handles remounting when a different model is selected. Do not flag this as a stale-state or tab-reset issue.
Applied to files:
apps/webapp/app/routes/resources.account.mfa.setup/route.tsxapps/webapp/app/routes/resources.account.session-duration/SessionDurationSetting.tsx
📚 Learning: 2026-04-02T19:18:34.807Z
Learnt from: samejr
Repo: triggerdotdev/trigger.dev PR: 3319
File: apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.waitpoints.tokens/route.tsx:249-258
Timestamp: 2026-04-02T19:18:34.807Z
Learning: In `apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.waitpoints.tokens/route.tsx`, the `onCollapseChange={() => {}}` no-op on the `ResizablePanel` for the waitpoint inspector is intentional. The panel collapse is controlled externally via `collapsed={!isShowingWaitpoint}` (driven by URL params), so the callback is deliberately left as a no-op. Do not flag this as a missing implementation.
Applied to files:
apps/webapp/app/routes/resources.account.mfa.setup/route.tsxapps/webapp/app/routes/resources.account.mfa.setup/MfaToggle.tsx
📚 Learning: 2026-03-10T17:56:26.581Z
Learnt from: samejr
Repo: triggerdotdev/trigger.dev PR: 3201
File: apps/webapp/app/v3/services/setSeatsAddOn.server.ts:25-29
Timestamp: 2026-03-10T17:56:26.581Z
Learning: In the `triggerdotdev/trigger.dev` webapp, service classes such as `SetSeatsAddOnService` and `SetBranchesAddOnService` do NOT need to perform their own userId-to-organizationId authorization checks. Auth is enforced at the route layer: `requireUserId(request)` authenticates the user, and the `_app.orgs.$organizationSlug` layout route enforces that the authenticated user is a member of the org. Any `userId` and `organizationId` reaching these services from org-scoped routes are already validated. This is the consistent pattern used across all org-scoped services in the codebase.
Applied to files:
apps/webapp/app/services/session.server.ts
📚 Learning: 2026-03-02T12:43:17.177Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: internal-packages/database/CLAUDE.md:0-0
Timestamp: 2026-03-02T12:43:17.177Z
Learning: Applies to internal-packages/database/**/prisma/migrations/*/*.sql : When adding an index on a new column in an existing table, use two separate migrations: first for `ALTER TABLE ... ADD COLUMN IF NOT EXISTS ...`, then for `CREATE INDEX CONCURRENTLY IF NOT EXISTS ...` in its own file
Applied to files:
internal-packages/database/prisma/migrations/20260428140746_add_session_duration_columns/migration.sql
📚 Learning: 2026-03-22T13:49:20.068Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3244
File: internal-packages/database/prisma/migrations/20260318114244_add_prompt_friendly_id/migration.sql:5-5
Timestamp: 2026-03-22T13:49:20.068Z
Learning: For Prisma migration SQL files under `internal-packages/database/prisma/migrations/`, it is acceptable to create indexes with `CREATE INDEX` / `CREATE UNIQUE INDEX` (i.e., without `CONCURRENTLY`) when the parent table is introduced in the same PR and has no existing production rows yet. Only require `CREATE INDEX CONCURRENTLY` (or otherwise account for existing production data/locks) when the table already exists in production with data.
Applied to files:
internal-packages/database/prisma/migrations/20260428140746_add_session_duration_columns/migration.sql
📚 Learning: 2026-03-02T12:43:17.177Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: internal-packages/database/CLAUDE.md:0-0
Timestamp: 2026-03-02T12:43:17.177Z
Learning: Applies to internal-packages/database/**/prisma/migrations/*/*.sql : Clean up generated Prisma migrations by removing extraneous lines for junction tables (`_BackgroundWorkerToBackgroundWorkerFile`, `_BackgroundWorkerToTaskQueue`, `_TaskRunToTaskRunTag`, `_WaitpointRunConnections`, `_completedWaitpoints`) and indexes (`SecretStore_key_idx`, various `TaskRun` indexes) unless explicitly added
Applied to files:
internal-packages/database/prisma/migrations/20260428140746_add_session_duration_columns/migration.sql
📚 Learning: 2026-03-02T12:43:17.177Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: internal-packages/database/CLAUDE.md:0-0
Timestamp: 2026-03-02T12:43:17.177Z
Learning: Applies to internal-packages/database/**/prisma/migrations/*/*.sql : Indexes on newly created tables can use `CREATE INDEX` without CONCURRENTLY and can be combined in the same migration file as the `CREATE TABLE` statement
Applied to files:
internal-packages/database/prisma/migrations/20260428140746_add_session_duration_columns/migration.sql
📚 Learning: 2026-03-02T12:43:17.177Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: internal-packages/database/CLAUDE.md:0-0
Timestamp: 2026-03-02T12:43:17.177Z
Learning: Applies to internal-packages/database/**/prisma/migrations/*/*.sql : When adding indexes to existing tables, use `CREATE INDEX CONCURRENTLY IF NOT EXISTS` to avoid table locks in production, and place each concurrent index in its own separate migration file
Applied to files:
internal-packages/database/prisma/migrations/20260428140746_add_session_duration_columns/migration.sql
📚 Learning: 2026-04-16T14:21:18.496Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3368
File: internal-packages/database/prisma/schema.prisma:666-666
Timestamp: 2026-04-16T14:21:18.496Z
Learning: In `triggerdotdev/trigger.dev`, the `BackgroundWorkerTask` covering index on `(runtimeEnvironmentId, slug, triggerSource)` lives in `internal-packages/database/prisma/migrations/20260413000000_add_bwt_covering_index/migration.sql` as a `CREATE INDEX CONCURRENTLY IF NOT EXISTS`, intentionally in its own migration file separate from the `TaskIdentifier` table migration. Do not flag this index as missing from the schema migrations in future reviews.
Applied to files:
internal-packages/database/prisma/migrations/20260428140746_add_session_duration_columns/migration.sql
📚 Learning: 2025-05-27T19:30:34.004Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 2086
File: internal-packages/database/prisma/migrations/20250511145836_runtime_environment_add_is_branchable_environment/migration.sql:1-3
Timestamp: 2025-05-27T19:30:34.004Z
Learning: In modern PostgreSQL versions (11+), adding a column with a constant default value (like DEFAULT false, DEFAULT 0, DEFAULT 'text') does NOT require a table rewrite. PostgreSQL stores the default value in the catalog and applies it virtually when reading rows. Only non-constant defaults or more complex scenarios require table rewrites. Avoid suggesting multi-step migrations for simple constant defaults.
Applied to files:
internal-packages/database/prisma/migrations/20260428140746_add_session_duration_columns/migration.sql
📚 Learning: 2026-02-03T18:48:31.790Z
Learnt from: 0ski
Repo: triggerdotdev/trigger.dev PR: 2994
File: internal-packages/database/prisma/migrations/20260129162810_add_integration_deployment/migration.sql:14-18
Timestamp: 2026-02-03T18:48:31.790Z
Learning: For Prisma migrations targeting PostgreSQL: - When adding indexes to existing tables, create the index in a separate migration file and include CONCURRENTLY to avoid locking the table. - For indexes on newly created tables (in CREATE TABLE statements), you can create the index in the same migration file without CONCURRENTLY. This reduces rollout complexity for new objects while protecting uptime for existing structures.
Applied to files:
internal-packages/database/prisma/migrations/20260428140746_add_session_duration_columns/migration.sql
📚 Learning: 2026-04-16T13:45:22.317Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3368
File: apps/webapp/test/engine/taskIdentifierRegistry.test.ts:3-19
Timestamp: 2026-04-16T13:45:22.317Z
Learning: In `apps/webapp/test/engine/taskIdentifierRegistry.test.ts`, the `vi.mock` calls for `~/services/taskIdentifierCache.server` (stubbing `getTaskIdentifiersFromCache` and `populateTaskIdentifierCache`), `~/models/task.server` (stubbing `getAllTaskIdentifiers`), and `~/db.server` (stubbing `prisma` and `$replica`) are intentional. The suite uses real Postgres via testcontainers for all `TaskIdentifier` DB operations, but isolates the Redis cache layer and legacy query fallback as separate concerns not exercised in this test file. Do not flag these mocks as violations of the no-mocks policy in future reviews.
Applied to files:
apps/webapp/test/sessionDuration.test.ts
📚 Learning: 2026-04-07T14:12:18.946Z
Learnt from: matt-aitken
Repo: triggerdotdev/trigger.dev PR: 3331
File: apps/webapp/test/engine/batchPayloads.test.ts:5-24
Timestamp: 2026-04-07T14:12:18.946Z
Learning: In `apps/webapp/test/engine/batchPayloads.test.ts`, using `vi.mock` for `~/v3/objectStore.server` (stubbing `hasObjectStoreClient` and `uploadPacketToObjectStore`), `~/env.server` (overriding offload thresholds), and `~/v3/tracer.server` (stubbing `startActiveSpan`) is intentional and acceptable. Simulating controlled transient upload failures (e.g., fail N times then succeed) to verify `p-retry` behavior cannot be reproduced with real services or testcontainers. This file is an explicit exception to the repo's general no-mocks policy.
Applied to files:
apps/webapp/test/sessionDuration.test.ts
📚 Learning: 2026-03-02T12:43:25.254Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: internal-packages/run-engine/CLAUDE.md:0-0
Timestamp: 2026-03-02T12:43:25.254Z
Learning: Applies to internal-packages/run-engine/src/engine/tests/**/*.test.ts : Implement tests for RunEngine in `src/engine/tests/` using testcontainers for Redis and PostgreSQL containerization
Applied to files:
apps/webapp/test/sessionDuration.test.ts
📚 Learning: 2026-04-15T15:39:06.868Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-15T15:39:06.868Z
Learning: Applies to **/*.test.{ts,tsx} : For Redis/PostgreSQL tests in vitest, use testcontainers helpers: `redisTest`, `postgresTest`, or `containerTest` imported from `internal/testcontainers`.
Applied to files:
apps/webapp/test/sessionDuration.test.ts
📚 Learning: 2026-04-20T15:08:55.358Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3417
File: apps/webapp/app/services/sessionsReplicationService.server.ts:204-215
Timestamp: 2026-04-20T15:08:55.358Z
Learning: In `apps/webapp/app/services/sessionsReplicationService.server.ts` and `apps/webapp/app/services/runsReplicationService.server.ts`, the `getKey` function in `ConcurrentFlushScheduler` uses `${item.event}_${item.session.id}` / `${item.event}_${item.run.id}` respectively. This pattern is intentionally kept identical across both replication services for consistency. Any change to the deduplication key shape (e.g., keying solely by session/run id) must be applied to both services together, never to one service in isolation. Tracking as a cross-service follow-up.
Applied to files:
apps/webapp/app/services/sessionDuration.server.ts
🔇 Additional comments (25)
apps/webapp/app/routes/resources.account.mfa.setup/MfaToggle.tsx (1)
15-22: Layout refactor is clean and improves alignment.The updated row structure keeps the label/description and toggle visually balanced.
apps/webapp/app/routes/resources.account.mfa.setup/route.tsx (2)
1-8: Import updates look good.Type-only import usage and message-server import formatting are clean and consistent.
146-159: Hook destructuring and callsite cleanup look good.These changes improve readability without changing behavior.
apps/webapp/app/routes/login.mfa/route.tsx (1)
166-166: MFA completion now uses the correct authenticated commit path.This update keeps MFA login completion consistent with the session-duration enforcement flow.
apps/webapp/app/routes/auth.google.callback.tsx (1)
56-56: Correct cookie commit helper on authenticated success path.Using
commitAuthenticatedSessionhere is consistent with the new auto-logout/session-duration model.apps/webapp/app/routes/auth.github.callback.tsx (1)
56-56: Authenticated GitHub callback commit looks correct.This aligns GitHub login completion with the same session-expiry behavior as other final auth flows.
apps/webapp/app/routes/magic.tsx (1)
14-17: Both redirect sanitization hardening and final cookie commit update look good.The stale-cookie sanitization and authenticated-session commit are consistent with the PR’s session/logout goals.
Also applies to: 59-59
apps/webapp/app/routes/login.magic/route.tsx (1)
64-75: Redirect param sanitization in login loader is solid.Good hardening: only safe, meaningful destinations are persisted to redirect session state.
apps/webapp/app/services/sessionStorage.server.ts (1)
4-9: Nice cleanup: canonical session-duration constant is clear and consistent.Centralizing the max-age value here reduces drift and makes intent explicit.
Also applies to: 19-19
internal-packages/database/prisma/schema.prisma (2)
57-60: LGTM!The schema additions are well-documented. The
User.sessionDurationwith NOT NULL DEFAULT and optionalOrganization.maxSessionDurationcorrectly model the "user preference capped by org policy" pattern. The doc comments clearly explain the relationship between the two fields.
227-231: LGTM!The nullable
maxSessionDurationon Organization allows orgs to optionally enforce a cap while leaving others unconstrained. The comment accurately describes the "most restrictive cap wins" behavior.apps/webapp/app/root.tsx (1)
65-74: LGTM!The Headers object correctly handles multiple
Set-Cookieheaders. The lazy backfill pattern appropriately guards onuserbeing truthy before attempting to refresh the auth session cookie.apps/webapp/app/routes/account.security/route.tsx (2)
27-39: LGTM!The loader correctly fetches the user, computes the effective session duration constraints, and derives the allowed dropdown options. The data shape aligns well with what
SessionDurationSettingexpects.
56-65: LGTM!The UI correctly passes the computed
sessionDuration,sessionDurationOptions, andorgCapSecondsto theSessionDurationSettingcomponent. The layout structure with bordered sections is consistent.apps/webapp/app/routes/resources.account.session-duration/route.tsx (1)
33-77: LGTM!The action implements defense-in-depth validation: first checking the value is in the allowed options list, then verifying it doesn't exceed the user's org cap. Re-issuing the auth cookie immediately after the update ensures the new
maxAgetakes effect without requiring a separate login.apps/webapp/app/services/session.server.ts (2)
30-56: LGTM!The session expiry enforcement is well-implemented. The structured logging with the stable
eventfield enables filtering/aggregating auto-logout events in CloudWatch. The check correctly uses the effective duration (user setting capped by org policy).
69-81: LGTM!The
sanitizeRedirectPathaddition prevents the redirect-on-login bug where fetcher endpoints (e.g.,/resources/*) could be stored inredirectTo, causing blank pages or loops after login.apps/webapp/app/routes/resources.account.session-duration/SessionDurationSetting.tsx (1)
14-71: LGTM!The component correctly handles the org cap display, guards against no-op submissions when the value is unchanged, and properly maps option values to their labels. The auto-submit pattern is consistent with similar settings in the app.
apps/webapp/test/sessionDuration.test.ts (1)
1-215: LGTM!Excellent test coverage for the session duration logic. The suite properly uses
containerTestfor database-dependent tests while keeping pure function tests fast. Edge cases like soft-deleted orgs, multiple orgs with different caps, and missing users are well covered.apps/webapp/app/services/sessionDuration.server.ts (6)
1-8: LGTM!Clean imports and the
SESSION_ISSUED_AT_KEYconstant follows the coding guideline for using named constants for sentinel values.
10-32: LGTM!Good use of
ReadonlySetfor immutable constant validation with O(1) lookup. The Gregorian calendar comment helpfully explains the rationale for the magic number.
34-36: LGTM!Simple and clear validation helper.
95-119: LGTM!Good edge-case handling by always including the current value even when it exceeds the org cap, ensuring the form remains valid until the user explicitly selects a new value.
121-155: LGTM!The helpers are well-designed. The
Number.isFinitecheck ingetSessionIssuedAtcorrectly handles edge cases likeNaNorInfinity. The lazy backfill behavior for legacy cookies (treating missingissuedAtas not expired) is a sensible migration strategy.
157-187: LGTM!The JSDoc comment clearly explains why the cookie's
Max-Ageis set to 1 year while actual expiry is enforced server-side. The separation betweencommitAuthenticatedSession(fresh login) andcommitAuthenticatedSessionLazy(backfill) provides appropriate flexibility.
| const organization = await prisma.organization.update({ | ||
| where: { id: organizationId }, | ||
| data: { maxSessionDuration: body.maxSessionDuration }, | ||
| select: { id: true, slug: true, maxSessionDuration: true }, | ||
| }); |
There was a problem hiding this comment.
Handle non-existent organization gracefully.
If organizationId doesn't exist, prisma.organization.update throws a Prisma P2025 error, resulting in an unhandled 500. Consider catching this or checking existence first to return a proper 404.
🛡️ Proposed fix
+ const organization = await prisma.organization.findFirst({
+ where: { id: organizationId },
+ });
+
+ if (!organization) {
+ throw new Response(JSON.stringify({ error: "Organization not found" }), {
+ status: 404,
+ headers: { "Content-Type": "application/json" },
+ });
+ }
+
- const organization = await prisma.organization.update({
+ const updated = await prisma.organization.update({
where: { id: organizationId },
data: { maxSessionDuration: body.maxSessionDuration },
select: { id: true, slug: true, maxSessionDuration: true },
});
- return json({ success: true, organization });
+ return json({ success: true, organization: updated });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/webapp/app/routes/admin.api.v1.orgs`.$organizationId.session-duration.ts
around lines 25 - 29, The update call using prisma.organization.update (where: {
id: organizationId }) can throw Prisma P2025 if the organizationId doesn't
exist; wrap the update in a try/catch or pre-check existence with
prisma.organization.findUnique({ where: { id: organizationId } }) and if not
found return a 404 response instead of letting the exception bubble;
alternatively catch the error from prisma.organization.update, detect error.code
=== 'P2025' and return a 404 (otherwise rethrow or return 500), ensuring the
handler returns the selected fields (id, slug, maxSessionDuration) on success.
| session.set(authenticator.sessionKey, auth); | ||
|
|
||
| const headers = new Headers(); | ||
| headers.append("Set-Cookie", await commitSession(session)); | ||
| headers.append("Set-Cookie", await commitAuthenticatedSession(session)); |
There was a problem hiding this comment.
📝 Info: Auth callback session storage fix: previously read from wrong cookie
The old code in auth.github.callback.tsx and auth.google.callback.tsx imported getSession from ~/models/message.server (the __message cookie) but commitSession from ~/services/sessionStorage.server (the __session cookie). This meant the session object was populated from the message cookie's data, then auth data was added, and the combined blob was serialized into the __session cookie. It worked because the auth key was present in the committed cookie, but it leaked flash message data into __session. The new code correctly uses getUserSession from ~/services/sessionStorage.server for both read and write, keeping the two cookie stores cleanly separated.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const session = await getUserSession(request); | ||
| const { durationSeconds, orgCapSeconds, userSettingSeconds } = await getEffectiveSessionDuration( | ||
| authUser.userId | ||
| ); | ||
| if (isSessionExpired(session, durationSeconds)) { | ||
| const issuedAt = getSessionIssuedAt(session); | ||
| // HIPAA audit trail: structured log lands in CloudWatch via stdout. Use | ||
| // the stable `event` field to filter/aggregate auto-logout events. | ||
| logger.info("Auto-logout: session exceeded effective duration", { | ||
| event: "session.auto_logout", | ||
| userId: authUser.userId, | ||
| effectiveDurationSeconds: durationSeconds, | ||
| userSettingSeconds, | ||
| orgCapSeconds, | ||
| sessionAgeMs: issuedAt === null ? null : Date.now() - issuedAt, | ||
| requestPath: new URL(request.url).pathname, | ||
| }); | ||
| throw redirect("/logout"); | ||
| } |
There was a problem hiding this comment.
🚩 Every authenticated request now incurs an additional DB query for session enforcement
enforceSessionExpiry at services/session.server.ts:23-51 runs on every authenticated request via getUserId. It calls getEffectiveSessionDuration(userId, $replica) which executes two parallel queries: a user.findFirst and an organization.aggregate. While using $replica mitigates primary DB load, this adds ~2 DB round-trips to every authenticated hot path. The code correctly documents this as a conscious tradeoff. If performance becomes a concern, consider caching the effective duration in-memory with a short TTL (e.g. 30-60s) keyed by userId, or embedding the effective duration in the session cookie itself to avoid DB hits entirely.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/webapp/app/services/session.server.ts (1)
23-51: 🏗️ Heavy liftConsider adding
orgIdto the audit log for compliance completeness.The PR objectives flag that the auto-logout audit log is missing
orgId(and source IP). For HIPAA audit trails, knowing which organization's cap triggered the logout aids forensic analysis. ThegetEffectiveSessionDurationresult includesorgCapSecondsbut not the org ID itself.If a single org cap is relevant, you could extend
getEffectiveSessionDurationto also return the capping org's ID. Alternatively, log all org memberships or at least a flag indicating whether an org cap was the limiting factor.Source IP can be extracted from
request.headers.get("x-forwarded-for")or similar, depending on your proxy configuration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/services/session.server.ts` around lines 23 - 51, The auto-logout audit log emitted by enforceSessionExpiry is missing the organization identifier and source IP; update getEffectiveSessionDuration (or add a companion call) to return the capping org's ID (e.g., include orgId or a flag like cappingOrgId alongside orgCapSeconds) and include that orgId in the logger.info payload in enforceSessionExpiry, and also extract the request source IP (e.g., request.headers.get("x-forwarded-for") with fallback) and include it as sourceIp in the same structured log so the HIPAA audit trail contains org context and client IP.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/webapp/app/services/session.server.ts`:
- Around line 23-51: The auto-logout audit log emitted by enforceSessionExpiry
is missing the organization identifier and source IP; update
getEffectiveSessionDuration (or add a companion call) to return the capping
org's ID (e.g., include orgId or a flag like cappingOrgId alongside
orgCapSeconds) and include that orgId in the logger.info payload in
enforceSessionExpiry, and also extract the request source IP (e.g.,
request.headers.get("x-forwarded-for") with fallback) and include it as sourceIp
in the same structured log so the HIPAA audit trail contains org context and
client IP.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0e63b255-ccb2-4944-b936-73da05f8ff67
📒 Files selected for processing (7)
apps/webapp/app/routes/account.security/route.tsxapps/webapp/app/routes/auth.github.callback.tsxapps/webapp/app/routes/auth.google.callback.tsxapps/webapp/app/routes/resources.account.mfa.setup/MfaToggle.tsxapps/webapp/app/services/session.server.tsapps/webapp/app/services/sessionDuration.server.tsapps/webapp/app/utils.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/webapp/app/routes/auth.google.callback.tsx
- apps/webapp/app/routes/auth.github.callback.tsx
- apps/webapp/app/routes/resources.account.mfa.setup/MfaToggle.tsx
- apps/webapp/app/utils.ts
- apps/webapp/app/routes/account.security/route.tsx
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (31)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / e2e-webapp / 🧪 E2E Tests: Webapp
- GitHub Check: sdk-compat / Deno Runtime
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: sdk-compat / Node.js 22.12 (ubuntu-latest)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: typecheck / typecheck
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: sdk-compat / Cloudflare Workers
- GitHub Check: sdk-compat / Node.js 20.20 (ubuntu-latest)
- GitHub Check: sdk-compat / Bun Runtime
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (actions)
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{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/app/services/session.server.tsapps/webapp/app/services/sessionDuration.server.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/app/services/session.server.tsapps/webapp/app/services/sessionDuration.server.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use function declarations instead of default exports
Add crumbs as you write code using
//@Crumbscomments or `// `#region` `@crumbsblocks. These are temporary debug instrumentation and must be stripped usingagentcrumbs stripbefore merge.
Files:
apps/webapp/app/services/session.server.tsapps/webapp/app/services/sessionDuration.server.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/app/services/session.server.tsapps/webapp/app/services/sessionDuration.server.ts
**/*.{js,ts,jsx,tsx,json,md,yaml,yml}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using Prettier before committing
Files:
apps/webapp/app/services/session.server.tsapps/webapp/app/services/sessionDuration.server.ts
**/*.ts{,x}
📄 CodeRabbit inference engine (CLAUDE.md)
Always import from
@trigger.dev/sdkwhen writing Trigger.dev tasks. Never use@trigger.dev/sdk/v3or deprecatedclient.defineJob.
Files:
apps/webapp/app/services/session.server.tsapps/webapp/app/services/sessionDuration.server.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/corepathUse named constants for sentinel/placeholder values (e.g.
const UNSET_VALUE = '__unset__') instead of raw string literals scattered across comparisons
Files:
apps/webapp/app/services/session.server.tsapps/webapp/app/services/sessionDuration.server.ts
apps/webapp/**/*.server.ts
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
apps/webapp/**/*.server.ts: Never userequest.signalfor detecting client disconnects. UsegetRequestAbortSignal()fromapp/services/httpAsyncStorage.server.tsinstead, which is wired directly to Expressres.on('close')and fires reliably
Access environment variables viaenvexport fromapp/env.server.ts. Never useprocess.envdirectly
Always usefindFirstinstead offindUniquein Prisma queries.findUniquehas an implicit DataLoader that batches concurrent calls and has active bugs even in Prisma 6.x (uppercase UUIDs returning null, composite key SQL correctness issues, 5-10x worse performance).findFirstis never batched and avoids this entire class of issues
Files:
apps/webapp/app/services/session.server.tsapps/webapp/app/services/sessionDuration.server.ts
🧠 Learnings (14)
📓 Common learnings
Learnt from: myftija
Repo: triggerdotdev/trigger.dev PR: 2463
File: apps/webapp/app/routes/_app.github.callback/route.tsx:33-44
Timestamp: 2025-09-02T11:27:36.336Z
Learning: In the GitHub App installation callback flow in apps/webapp/app/routes/_app.github.callback/route.tsx, the install session cookie is not cleared after use due to interface limitations with redirectWithSuccessMessage/redirectWithErrorMessage not supporting custom headers. The maintainer accepts this design as the 1-hour cookie expiration provides sufficient protection against replay attacks.
📚 Learning: 2026-04-20T15:06:19.815Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3417
File: apps/webapp/app/routes/realtime.v1.sessions.$session.$io.ts:37-51
Timestamp: 2026-04-20T15:06:19.815Z
Learning: In `apps/webapp/app/routes/realtime.v1.sessions.$session.$io.ts` (and all session realtime read paths), `$replica` is intentionally used for the `resolveSessionByIdOrExternalId` call — including the `closedAt` guard in the PUT/initialize path. The project convention is to use `$replica` consistently across all session realtime routes. The race window (replica lag allowing a ghost-initialize after close) is accepted as not realistic in practice (clients follow the close API response; they do not race it). If replica lag ever causes issues, the mitigation is to revisit all realtime routes together, not to swap individual routes to `prisma`. Do not flag `$replica` usage in session realtime routes as a stale-read issue.
Applied to files:
apps/webapp/app/services/session.server.tsapps/webapp/app/services/sessionDuration.server.ts
📚 Learning: 2025-09-02T11:27:36.336Z
Learnt from: myftija
Repo: triggerdotdev/trigger.dev PR: 2463
File: apps/webapp/app/routes/_app.github.callback/route.tsx:33-44
Timestamp: 2025-09-02T11:27:36.336Z
Learning: In the GitHub App installation callback flow in apps/webapp/app/routes/_app.github.callback/route.tsx, the install session cookie is not cleared after use due to interface limitations with redirectWithSuccessMessage/redirectWithErrorMessage not supporting custom headers. The maintainer accepts this design as the 1-hour cookie expiration provides sufficient protection against replay attacks.
Applied to files:
apps/webapp/app/services/session.server.ts
📚 Learning: 2026-04-20T15:08:59.789Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3417
File: apps/webapp/app/services/sessionsRepository/clickhouseSessionsRepository.server.ts:27-40
Timestamp: 2026-04-20T15:08:59.789Z
Learning: In `apps/webapp/app/services/sessionsRepository/clickhouseSessionsRepository.server.ts`, the cursor predicate in `listSessionIds` compares only `session_id` while the `ORDER BY` clause uses `(created_at, session_id)`. This is intentional and consistent with the same pattern in `ClickHouseRunsRepository` and the waitpoints repository. Do not flag this as a skip/duplicate pagination bug in isolation — any fix must land across all three repositories at once as a shared follow-up.
Applied to files:
apps/webapp/app/services/session.server.tsapps/webapp/app/services/sessionDuration.server.ts
📚 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/app/services/session.server.tsapps/webapp/app/services/sessionDuration.server.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/app/services/session.server.tsapps/webapp/app/services/sessionDuration.server.ts
📚 Learning: 2026-03-26T09:02:07.973Z
Learnt from: myftija
Repo: triggerdotdev/trigger.dev PR: 3274
File: apps/webapp/app/services/runsReplicationService.server.ts:922-924
Timestamp: 2026-03-26T09:02:07.973Z
Learning: When parsing Trigger.dev task run annotations in server-side services, keep `TaskRun.annotations` strictly conforming to the `RunAnnotations` schema from `trigger.dev/core/v3`. If the code already uses `RunAnnotations.safeParse` (e.g., in a `#parseAnnotations` helper), treat that as intentional/necessary for atomic, schema-accurate annotation handling. Do not recommend relaxing the annotation payload schema or using a permissive “passthrough” parse path, since the annotations are expected to be written atomically in one operation and should not contain partial/legacy payloads that would require a looser parser.
Applied to files:
apps/webapp/app/services/session.server.tsapps/webapp/app/services/sessionDuration.server.ts
📚 Learning: 2026-04-20T15:08:55.358Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3417
File: apps/webapp/app/services/sessionsReplicationService.server.ts:204-215
Timestamp: 2026-04-20T15:08:55.358Z
Learning: In `apps/webapp/app/services/sessionsReplicationService.server.ts` and `apps/webapp/app/services/runsReplicationService.server.ts`, the `getKey` function in `ConcurrentFlushScheduler` uses `${item.event}_${item.session.id}` / `${item.event}_${item.run.id}` respectively. This pattern is intentionally kept identical across both replication services for consistency. Any change to the deduplication key shape (e.g., keying solely by session/run id) must be applied to both services together, never to one service in isolation. Tracking as a cross-service follow-up.
Applied to files:
apps/webapp/app/services/sessionDuration.server.ts
📚 Learning: 2026-04-16T14:19:16.330Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: apps/webapp/CLAUDE.md:0-0
Timestamp: 2026-04-16T14:19:16.330Z
Learning: Applies to apps/webapp/**/*.server.ts : Always use `findFirst` instead of `findUnique` in Prisma queries. `findUnique` has an implicit DataLoader that batches concurrent calls and has active bugs even in Prisma 6.x (uppercase UUIDs returning null, composite key SQL correctness issues, 5-10x worse performance). `findFirst` is never batched and avoids this entire class of issues
Applied to files:
apps/webapp/app/services/sessionDuration.server.ts
📚 Learning: 2026-03-26T10:02:25.354Z
Learnt from: 0ski
Repo: triggerdotdev/trigger.dev PR: 3254
File: apps/webapp/app/services/platformNotifications.server.ts:363-385
Timestamp: 2026-03-26T10:02:25.354Z
Learning: In `triggerdotdev/trigger.dev`, the `getNextCliNotification` fallback in `apps/webapp/app/services/platformNotifications.server.ts` intentionally uses `prisma.orgMember.findFirst` (single org) when no `projectRef` is provided. This is acceptable for v1 because the CLI (`dev` and `login` commands) always passes `projectRef` in normal usage, making the fallback a rare edge case. Do not flag the single-org fallback as a multi-org correctness bug in this file.
Applied to files:
apps/webapp/app/services/sessionDuration.server.ts
📚 Learning: 2026-04-16T14:21:17.695Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3368
File: apps/webapp/app/components/logs/LogsTaskFilter.tsx:135-163
Timestamp: 2026-04-16T14:21:17.695Z
Learning: In `triggerdotdev/trigger.dev` PR `#3368`, the `TaskIdentifier` table has a `@unique([runtimeEnvironmentId, slug])` DB constraint, guaranteeing one row per (environment, slug). In components like `apps/webapp/app/components/logs/LogsTaskFilter.tsx` and `apps/webapp/app/components/runs/v3/RunFilters.tsx`, using `key={item.slug}` for SelectItem list items is correct and unique. Do NOT flag `key={item.slug}` as potentially non-unique — the old duplicate-(slug, triggerSource) issue only existed with the legacy `DISTINCT` query, which this registry replaces.
Applied to files:
apps/webapp/app/services/sessionDuration.server.ts
📚 Learning: 2026-04-13T21:44:00.032Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3368
File: apps/webapp/app/services/taskIdentifierRegistry.server.ts:24-67
Timestamp: 2026-04-13T21:44:00.032Z
Learning: In `apps/webapp/app/services/taskIdentifierRegistry.server.ts`, the sequential upsert/updateMany/findMany writes in `syncTaskIdentifiers` are intentionally NOT wrapped in a Prisma transaction. This function runs only during deployment-change events (low-concurrency path), and any partial `isInLatestDeployment` state is acceptable because it self-corrects on the next deployment. Do not flag this as a missing-transaction/atomicity issue in future reviews.
Applied to files:
apps/webapp/app/services/sessionDuration.server.ts
📚 Learning: 2026-04-17T13:56:37.517Z
Learnt from: matt-aitken
Repo: triggerdotdev/trigger.dev PR: 3333
File: apps/webapp/app/services/queryService.server.ts:278-279
Timestamp: 2026-04-17T13:56:37.517Z
Learning: In `apps/webapp/app/services/queryService.server.ts`, the `queryConcurrencyLimiter` is intentionally keyed by `projectId` (not `organizationId`), even though the ClickHouse client is organization-scoped. Concurrency control is deliberately enforced at the project level. Do not flag the `projectId` key in `queryConcurrencyLimiter.acquire`/`release` as a bug or suggest changing it to `organizationId`.
Applied to files:
apps/webapp/app/services/sessionDuration.server.ts
📚 Learning: 2026-04-16T14:19:16.330Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: apps/webapp/CLAUDE.md:0-0
Timestamp: 2026-04-16T14:19:16.330Z
Learning: Applies to apps/webapp/app/v3/services/queues.server.ts : If adding a new task-level default, add it to the existing `select` clause in the `backgroundWorkerTask.findFirst()` query in `queues.server.ts` — do NOT add a second query. If the default doesn't need to be known at trigger time, resolve it at dequeue time instead
Applied to files:
apps/webapp/app/services/sessionDuration.server.ts
🔇 Additional comments (10)
apps/webapp/app/services/session.server.ts (3)
2-13: LGTM!Imports are well-organized and correctly use the session duration utilities from the new module. Using
$replicafor the hot-path session expiry check aligns with project conventions for read-heavy operations.
53-81: LGTM!The session expiry enforcement is correctly wired into all authentication paths:
- Impersonation path (line 64) enforces expiry against the admin's session, preventing impersonation from bypassing the admin's duration cap.
- Fall-through for revoked admin (lines 68-73) still enforces expiry before returning the real user ID.
- Regular auth path (lines 76-80) enforces expiry after confirming authentication.
The flow is well-documented with comments explaining the rationale.
93-105: LGTM!Sanitizing
redirectToviasanitizeRedirectPathprevents open redirect attacks and ensures users aren't redirected to non-navigable endpoints (fetcher routes, auth callbacks) that would render blank or loop. The inline comment clearly explains the intent.apps/webapp/app/services/sessionDuration.server.ts (7)
1-8: LGTM!Clean imports using types from dependencies. Re-exporting
DEFAULT_SESSION_DURATION_SECONDSkeeps the session-duration API surface centralized in this module.
20-36: LGTM!Duration options are well-defined with human-readable labels. Using
ReadonlySetforALLOWED_SESSION_DURATION_VALUESprevents accidental mutation and enables O(1) membership checks.
43-56: LGTM!Efficient single-query aggregate to find the most restrictive org cap across all of the user's organizations. The filter correctly excludes deleted orgs and orgs without a cap.
72-93: LGTM!Parallel queries for user setting and org cap minimize latency. Uses
findFirstas required by coding guidelines. TheMath.mincorrectly selects the most restrictive duration.
101-119: LGTM!Good UX consideration: always including the user's current value in the dropdown even if it now exceeds the org cap ensures the form remains valid and the user can see what they currently have before selecting a smaller value.
127-140: LGTM!The backward-compatibility decision (missing
issuedAttreated as not expired) is clearly documented. This allows legacy cookies to be lazily backfilled viacommitAuthenticatedSessionLazywithout forcing immediate logout of all existing sessions.
157-187: LGTM!The comment on lines 162-166 explains the 1-year
Max-Agerationale as requested in the PR objectives. The distinction betweencommitAuthenticatedSession(fresh login) andcommitAuthenticatedSessionLazy(backfill existing) is clear and correctly implemented.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/webapp/app/services/sessionDuration.server.ts (1)
144-152: ⚡ Quick winAdd small clock-skew tolerance to expiry checks.
Line 151 uses an exact millisecond cutoff. A bounded skew allowance (e.g., 30s) reduces edge-case premature expiry across servers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/services/sessionDuration.server.ts` around lines 144 - 152, The expiry check in isSessionExpired uses an exact millisecond cutoff and should allow a small clock-skew tolerance; modify isSessionExpired (and related helpers if needed) to add a bounded tolerance (e.g., const SESSION_EXPIRY_SKEW_MS = 30_000) when comparing now - issuedAt to effectiveDurationSeconds * 1000 (i.e., treat session as expired only if now - issuedAt > effectiveDurationSeconds*1000 + SESSION_EXPIRY_SKEW_MS); reference the isSessionExpired function, issuedAt, effectiveDurationSeconds and now when making the change and make the skew a named constant so it’s easy to adjust and test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/webapp/app/services/sessionDuration.server.ts`:
- Around line 95-98: The code uses userSettingSeconds (from
user?.sessionDuration) and orgCap.orgCapSeconds directly when computing
durationSeconds, allowing tiny/invalid DB values to force near-immediate logout;
add a defensive minimum clamp (e.g., MIN_SESSION_DURATION_SECONDS constant) and
apply it to both the resolved userSettingSeconds and the org cap before taking
Math.min: compute clampedUserSetting = Math.max(userSettingSeconds ??
DEFAULT_SESSION_DURATION_SECONDS, MIN_SESSION_DURATION_SECONDS) and
clampedOrgCap = orgCap === null ? null : Math.max(orgCap.orgCapSeconds,
MIN_SESSION_DURATION_SECONDS), then compute durationSeconds using clamped values
so durationSeconds = clampedOrgCap === null ? clampedUserSetting :
Math.min(clampedUserSetting, clampedOrgCap).
- Around line 193-199: commitAuthenticatedSessionLazy currently always calls
ensureSessionIssuedAt and commitSession, causing unnecessary Set-Cookie churn;
change it to check session.data.issuedAt first and only set issuedAt and call
commitSession when missing (returning the cookie string), otherwise return
null/undefined to signal no cookie needed; update references to the function
(call sites) to append the Set-Cookie header only when the returned value is
truthy. Ensure you keep the same options passed to commitSession (e.g., {
maxAge: DEFAULT_SESSION_DURATION_SECONDS }) and reference functions
ensureSessionIssuedAt, commitAuthenticatedSessionLazy, and commitSession in your
change.
---
Nitpick comments:
In `@apps/webapp/app/services/sessionDuration.server.ts`:
- Around line 144-152: The expiry check in isSessionExpired uses an exact
millisecond cutoff and should allow a small clock-skew tolerance; modify
isSessionExpired (and related helpers if needed) to add a bounded tolerance
(e.g., const SESSION_EXPIRY_SKEW_MS = 30_000) when comparing now - issuedAt to
effectiveDurationSeconds * 1000 (i.e., treat session as expired only if now -
issuedAt > effectiveDurationSeconds*1000 + SESSION_EXPIRY_SKEW_MS); reference
the isSessionExpired function, issuedAt, effectiveDurationSeconds and now when
making the change and make the skew a named constant so it’s easy to adjust and
test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: fb988ff8-8480-4fc0-a9df-f9752a91968f
📒 Files selected for processing (4)
apps/webapp/app/routes/admin.api.v1.orgs.$organizationId.session-duration.tsapps/webapp/app/services/session.server.tsapps/webapp/app/services/sessionDuration.server.tsapps/webapp/test/sessionDuration.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/webapp/app/routes/admin.api.v1.orgs.$organizationId.session-duration.ts
- apps/webapp/app/services/session.server.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (30)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: sdk-compat / Node.js 20.20 (ubuntu-latest)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: units / e2e-webapp / 🧪 E2E Tests: Webapp
- GitHub Check: sdk-compat / Deno Runtime
- GitHub Check: sdk-compat / Cloudflare Workers
- GitHub Check: sdk-compat / Node.js 22.12 (ubuntu-latest)
- GitHub Check: sdk-compat / Bun Runtime
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: audit
- GitHub Check: Analyze (javascript-typescript)
🧰 Additional context used
📓 Path-based instructions (12)
**/*.{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/sessionDuration.test.tsapps/webapp/app/services/sessionDuration.server.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/sessionDuration.test.tsapps/webapp/app/services/sessionDuration.server.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use function declarations instead of default exports
Add crumbs as you write code using
//@Crumbscomments or `// `#region` `@crumbsblocks. These are temporary debug instrumentation and must be stripped usingagentcrumbs stripbefore merge.
Files:
apps/webapp/test/sessionDuration.test.tsapps/webapp/app/services/sessionDuration.server.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/sessionDuration.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/sessionDuration.test.tsapps/webapp/app/services/sessionDuration.server.ts
**/*.{js,ts,jsx,tsx,json,md,yaml,yml}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using Prettier before committing
Files:
apps/webapp/test/sessionDuration.test.tsapps/webapp/app/services/sessionDuration.server.ts
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.test.{ts,tsx,js,jsx}: Test files should live beside the files under test and use descriptivedescribeanditblocks
Tests should avoid mocks or stubs and use the helpers from@internal/testcontainerswhen Redis or Postgres are needed
Use vitest for running unit tests
Files:
apps/webapp/test/sessionDuration.test.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.test.{ts,tsx}: Use vitest exclusively for testing. Never mock anything — use testcontainers instead.
Place test files next to source files with naming patternMyService.ts->MyService.test.ts.
For Redis/PostgreSQL tests in vitest, use testcontainers helpers:redisTest,postgresTest, orcontainerTestimported from@internal/testcontainers.
Files:
apps/webapp/test/sessionDuration.test.ts
**/*.ts{,x}
📄 CodeRabbit inference engine (CLAUDE.md)
Always import from
@trigger.dev/sdkwhen writing Trigger.dev tasks. Never use@trigger.dev/sdk/v3or deprecatedclient.defineJob.
Files:
apps/webapp/test/sessionDuration.test.tsapps/webapp/app/services/sessionDuration.server.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/corepathUse named constants for sentinel/placeholder values (e.g.
const UNSET_VALUE = '__unset__') instead of raw string literals scattered across comparisons
Files:
apps/webapp/test/sessionDuration.test.tsapps/webapp/app/services/sessionDuration.server.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 testableFor testable code, never import
env.server.tsin test files. Pass configuration as options instead (e.g.,realtimeClient.server.tstakes config as constructor arg,realtimeClientGlobal.server.tscreates singleton with env config)
Files:
apps/webapp/test/sessionDuration.test.ts
apps/webapp/**/*.server.ts
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
apps/webapp/**/*.server.ts: Never userequest.signalfor detecting client disconnects. UsegetRequestAbortSignal()fromapp/services/httpAsyncStorage.server.tsinstead, which is wired directly to Expressres.on('close')and fires reliably
Access environment variables viaenvexport fromapp/env.server.ts. Never useprocess.envdirectly
Always usefindFirstinstead offindUniquein Prisma queries.findUniquehas an implicit DataLoader that batches concurrent calls and has active bugs even in Prisma 6.x (uppercase UUIDs returning null, composite key SQL correctness issues, 5-10x worse performance).findFirstis never batched and avoids this entire class of issues
Files:
apps/webapp/app/services/sessionDuration.server.ts
🧠 Learnings (26)
📚 Learning: 2026-04-07T14:12:18.946Z
Learnt from: matt-aitken
Repo: triggerdotdev/trigger.dev PR: 3331
File: apps/webapp/test/engine/batchPayloads.test.ts:5-24
Timestamp: 2026-04-07T14:12:18.946Z
Learning: In `apps/webapp/test/engine/batchPayloads.test.ts`, using `vi.mock` for `~/v3/objectStore.server` (stubbing `hasObjectStoreClient` and `uploadPacketToObjectStore`), `~/env.server` (overriding offload thresholds), and `~/v3/tracer.server` (stubbing `startActiveSpan`) is intentional and acceptable. Simulating controlled transient upload failures (e.g., fail N times then succeed) to verify `p-retry` behavior cannot be reproduced with real services or testcontainers. This file is an explicit exception to the repo's general no-mocks policy.
Applied to files:
apps/webapp/test/sessionDuration.test.ts
📚 Learning: 2026-04-15T15:39:31.575Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/webapp.mdc:0-0
Timestamp: 2026-04-15T15:39:31.575Z
Learning: Applies to apps/webapp/**/*.test.{ts,tsx} : Do not import `env.server.ts` directly or indirectly into test files; instead pass environment-dependent values through options/parameters to make code testable
Applied to files:
apps/webapp/test/sessionDuration.test.ts
📚 Learning: 2026-04-16T13:45:22.317Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3368
File: apps/webapp/test/engine/taskIdentifierRegistry.test.ts:3-19
Timestamp: 2026-04-16T13:45:22.317Z
Learning: In `apps/webapp/test/engine/taskIdentifierRegistry.test.ts`, the `vi.mock` calls for `~/services/taskIdentifierCache.server` (stubbing `getTaskIdentifiersFromCache` and `populateTaskIdentifierCache`), `~/models/task.server` (stubbing `getAllTaskIdentifiers`), and `~/db.server` (stubbing `prisma` and `$replica`) are intentional. The suite uses real Postgres via testcontainers for all `TaskIdentifier` DB operations, but isolates the Redis cache layer and legacy query fallback as separate concerns not exercised in this test file. Do not flag these mocks as violations of the no-mocks policy in future reviews.
Applied to files:
apps/webapp/test/sessionDuration.test.ts
📚 Learning: 2026-04-16T14:19:16.330Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: apps/webapp/CLAUDE.md:0-0
Timestamp: 2026-04-16T14:19:16.330Z
Learning: Applies to apps/webapp/**/*.test.{ts,tsx} : For testable code, never import `env.server.ts` in test files. Pass configuration as options instead (e.g., `realtimeClient.server.ts` takes config as constructor arg, `realtimeClientGlobal.server.ts` creates singleton with env config)
Applied to files:
apps/webapp/test/sessionDuration.test.ts
📚 Learning: 2026-04-15T15:39:06.868Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-15T15:39:06.868Z
Learning: Applies to **/*.test.{ts,tsx} : Use vitest exclusively for testing. Never mock anything — use testcontainers instead.
Applied to files:
apps/webapp/test/sessionDuration.test.ts
📚 Learning: 2026-03-02T12:43:25.254Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: internal-packages/run-engine/CLAUDE.md:0-0
Timestamp: 2026-03-02T12:43:25.254Z
Learning: Applies to internal-packages/run-engine/src/engine/tests/**/*.test.ts : Implement tests for RunEngine in `src/engine/tests/` using testcontainers for Redis and PostgreSQL containerization
Applied to files:
apps/webapp/test/sessionDuration.test.ts
📚 Learning: 2025-11-27T16:26:37.432Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-27T16:26:37.432Z
Learning: Applies to **/*.{test,spec}.{ts,tsx} : Use vitest for all tests in the Trigger.dev repository
Applied to files:
apps/webapp/test/sessionDuration.test.ts
📚 Learning: 2026-04-15T15:39:06.868Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-15T15:39:06.868Z
Learning: Applies to **/*.test.{ts,tsx} : For Redis/PostgreSQL tests in vitest, use testcontainers helpers: `redisTest`, `postgresTest`, or `containerTest` imported from `internal/testcontainers`.
Applied to files:
apps/webapp/test/sessionDuration.test.ts
📚 Learning: 2026-01-15T10:48:02.687Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-15T10:48:02.687Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Use vitest for running unit tests
Applied to files:
apps/webapp/test/sessionDuration.test.ts
📚 Learning: 2026-03-03T13:07:33.177Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3166
File: internal-packages/run-engine/src/batch-queue/tests/index.test.ts:711-713
Timestamp: 2026-03-03T13:07:33.177Z
Learning: In `internal-packages/run-engine/src/batch-queue/tests/index.test.ts`, test assertions for rate limiter stubs can use `toBeGreaterThanOrEqual` rather than exact equality (`toBe`) because the consumer loop may call the rate limiter during empty pops in addition to actual item processing, and this over-calling is acceptable in integration tests.
Applied to files:
apps/webapp/test/sessionDuration.test.ts
📚 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/sessionDuration.test.tsapps/webapp/app/services/sessionDuration.server.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/sessionDuration.test.tsapps/webapp/app/services/sessionDuration.server.ts
📚 Learning: 2026-04-20T15:08:55.358Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3417
File: apps/webapp/app/services/sessionsReplicationService.server.ts:204-215
Timestamp: 2026-04-20T15:08:55.358Z
Learning: In `apps/webapp/app/services/sessionsReplicationService.server.ts` and `apps/webapp/app/services/runsReplicationService.server.ts`, the `getKey` function in `ConcurrentFlushScheduler` uses `${item.event}_${item.session.id}` / `${item.event}_${item.run.id}` respectively. This pattern is intentionally kept identical across both replication services for consistency. Any change to the deduplication key shape (e.g., keying solely by session/run id) must be applied to both services together, never to one service in isolation. Tracking as a cross-service follow-up.
Applied to files:
apps/webapp/app/services/sessionDuration.server.ts
📚 Learning: 2026-04-16T14:19:16.330Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: apps/webapp/CLAUDE.md:0-0
Timestamp: 2026-04-16T14:19:16.330Z
Learning: Applies to apps/webapp/**/*.server.ts : Always use `findFirst` instead of `findUnique` in Prisma queries. `findUnique` has an implicit DataLoader that batches concurrent calls and has active bugs even in Prisma 6.x (uppercase UUIDs returning null, composite key SQL correctness issues, 5-10x worse performance). `findFirst` is never batched and avoids this entire class of issues
Applied to files:
apps/webapp/app/services/sessionDuration.server.ts
📚 Learning: 2026-03-26T10:02:25.354Z
Learnt from: 0ski
Repo: triggerdotdev/trigger.dev PR: 3254
File: apps/webapp/app/services/platformNotifications.server.ts:363-385
Timestamp: 2026-03-26T10:02:25.354Z
Learning: In `triggerdotdev/trigger.dev`, the `getNextCliNotification` fallback in `apps/webapp/app/services/platformNotifications.server.ts` intentionally uses `prisma.orgMember.findFirst` (single org) when no `projectRef` is provided. This is acceptable for v1 because the CLI (`dev` and `login` commands) always passes `projectRef` in normal usage, making the fallback a rare edge case. Do not flag the single-org fallback as a multi-org correctness bug in this file.
Applied to files:
apps/webapp/app/services/sessionDuration.server.ts
📚 Learning: 2026-04-20T15:08:59.789Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3417
File: apps/webapp/app/services/sessionsRepository/clickhouseSessionsRepository.server.ts:27-40
Timestamp: 2026-04-20T15:08:59.789Z
Learning: In `apps/webapp/app/services/sessionsRepository/clickhouseSessionsRepository.server.ts`, the cursor predicate in `listSessionIds` compares only `session_id` while the `ORDER BY` clause uses `(created_at, session_id)`. This is intentional and consistent with the same pattern in `ClickHouseRunsRepository` and the waitpoints repository. Do not flag this as a skip/duplicate pagination bug in isolation — any fix must land across all three repositories at once as a shared follow-up.
Applied to files:
apps/webapp/app/services/sessionDuration.server.ts
📚 Learning: 2026-04-16T14:21:17.695Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3368
File: apps/webapp/app/components/logs/LogsTaskFilter.tsx:135-163
Timestamp: 2026-04-16T14:21:17.695Z
Learning: In `triggerdotdev/trigger.dev` PR `#3368`, the `TaskIdentifier` table has a `@unique([runtimeEnvironmentId, slug])` DB constraint, guaranteeing one row per (environment, slug). In components like `apps/webapp/app/components/logs/LogsTaskFilter.tsx` and `apps/webapp/app/components/runs/v3/RunFilters.tsx`, using `key={item.slug}` for SelectItem list items is correct and unique. Do NOT flag `key={item.slug}` as potentially non-unique — the old duplicate-(slug, triggerSource) issue only existed with the legacy `DISTINCT` query, which this registry replaces.
Applied to files:
apps/webapp/app/services/sessionDuration.server.ts
📚 Learning: 2026-04-20T15:06:19.815Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3417
File: apps/webapp/app/routes/realtime.v1.sessions.$session.$io.ts:37-51
Timestamp: 2026-04-20T15:06:19.815Z
Learning: In `apps/webapp/app/routes/realtime.v1.sessions.$session.$io.ts` (and all session realtime read paths), `$replica` is intentionally used for the `resolveSessionByIdOrExternalId` call — including the `closedAt` guard in the PUT/initialize path. The project convention is to use `$replica` consistently across all session realtime routes. The race window (replica lag allowing a ghost-initialize after close) is accepted as not realistic in practice (clients follow the close API response; they do not race it). If replica lag ever causes issues, the mitigation is to revisit all realtime routes together, not to swap individual routes to `prisma`. Do not flag `$replica` usage in session realtime routes as a stale-read issue.
Applied to files:
apps/webapp/app/services/sessionDuration.server.ts
📚 Learning: 2026-04-13T21:44:00.032Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3368
File: apps/webapp/app/services/taskIdentifierRegistry.server.ts:24-67
Timestamp: 2026-04-13T21:44:00.032Z
Learning: In `apps/webapp/app/services/taskIdentifierRegistry.server.ts`, the sequential upsert/updateMany/findMany writes in `syncTaskIdentifiers` are intentionally NOT wrapped in a Prisma transaction. This function runs only during deployment-change events (low-concurrency path), and any partial `isInLatestDeployment` state is acceptable because it self-corrects on the next deployment. Do not flag this as a missing-transaction/atomicity issue in future reviews.
Applied to files:
apps/webapp/app/services/sessionDuration.server.ts
📚 Learning: 2026-04-17T13:56:37.517Z
Learnt from: matt-aitken
Repo: triggerdotdev/trigger.dev PR: 3333
File: apps/webapp/app/services/queryService.server.ts:278-279
Timestamp: 2026-04-17T13:56:37.517Z
Learning: In `apps/webapp/app/services/queryService.server.ts`, the `queryConcurrencyLimiter` is intentionally keyed by `projectId` (not `organizationId`), even though the ClickHouse client is organization-scoped. Concurrency control is deliberately enforced at the project level. Do not flag the `projectId` key in `queryConcurrencyLimiter.acquire`/`release` as a bug or suggest changing it to `organizationId`.
Applied to files:
apps/webapp/app/services/sessionDuration.server.ts
📚 Learning: 2026-04-16T14:19:16.330Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: apps/webapp/CLAUDE.md:0-0
Timestamp: 2026-04-16T14:19:16.330Z
Learning: Applies to apps/webapp/app/v3/services/queues.server.ts : If adding a new task-level default, add it to the existing `select` clause in the `backgroundWorkerTask.findFirst()` query in `queues.server.ts` — do NOT add a second query. If the default doesn't need to be known at trigger time, resolve it at dequeue time instead
Applied to files:
apps/webapp/app/services/sessionDuration.server.ts
📚 Learning: 2026-04-20T15:05:59.661Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3417
File: apps/webapp/app/routes/realtime.v1.sessions.$session.$io.append.ts:20-31
Timestamp: 2026-04-20T15:05:59.661Z
Learning: In `apps/webapp/app/routes/realtime.v1.sessions.$session.$io.append.ts`, the `MAX_APPEND_BODY_BYTES` cap is intentionally set to `1024 * 512` (512 KiB). The maintainer explicitly decided against lowering it to 128 KiB: the all-quotes worst-case JSON-escaping expansion that could exceed S2's 1 MiB per-record limit is considered pathological and not representative of real-world payloads (chat tokens, tool-call JSON, structured data). If overflow becomes a problem in practice, the preferred mitigation is an encoded-size guard inside `appendPart` itself. Do not flag this cap as a potential S2 overflow issue in future reviews.
Applied to files:
apps/webapp/app/services/sessionDuration.server.ts
📚 Learning: 2026-03-22T19:32:16.231Z
Learnt from: matt-aitken
Repo: triggerdotdev/trigger.dev PR: 3187
File: apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.errors/route.tsx:82-151
Timestamp: 2026-03-22T19:32:16.231Z
Learning: In `apps/webapp/app/v3/services/alerts/createAlertChannel.server.ts` and `apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.errors/route.tsx`, the `errorAlertConfig` field on `ProjectAlertChannel` is intentionally `Json?` (nullable). The `ErrorAlertEvaluator.computeMinInterval()` in `apps/webapp/app/v3/services/alerts/errorAlertEvaluator.server.ts` uses `ErrorAlertConfig.safeParse(ch.errorAlertConfig)` and falls back to `DEFAULT_INTERVAL_MS = 300_000` when `errorAlertConfig` is null. No UI currently collects this value — it is scaffolding for a future per-channel evaluation interval feature. Do not flag the absence of `errorAlertConfig` in `CreateAlertChannelOptions` or the action handler as a bug; null configs are safe and expected.
Applied to files:
apps/webapp/app/services/sessionDuration.server.ts
📚 Learning: 2026-04-20T15:06:11.054Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3417
File: apps/webapp/app/routes/realtime.v1.streams.$runId.$target.$streamId.append.ts:16-26
Timestamp: 2026-04-20T15:06:11.054Z
Learning: In `apps/webapp/app/routes/realtime.v1.streams.$runId.$target.$streamId.append.ts` and `apps/webapp/app/routes/realtime.v1.sessions.$session.$io.append.ts`, the `MAX_APPEND_BODY_BYTES` cap of 512 KiB (1024 * 512) is intentional even though `appendPart` wraps the body in JSON (which could expand quote-heavy payloads beyond S2's 1 MiB per-record limit). The maintainer considers worst-case quote-heavy payloads pathological and not realistic. If S2 rejections occur in practice, an encoded-size guard will be added inside `appendPart` rather than lowering the raw body cap on every caller. Do not flag this as an issue in future reviews.
Applied to files:
apps/webapp/app/services/sessionDuration.server.ts
📚 Learning: 2025-09-02T11:27:36.336Z
Learnt from: myftija
Repo: triggerdotdev/trigger.dev PR: 2463
File: apps/webapp/app/routes/_app.github.callback/route.tsx:33-44
Timestamp: 2025-09-02T11:27:36.336Z
Learning: In the GitHub App installation callback flow in apps/webapp/app/routes/_app.github.callback/route.tsx, the install session cookie is not cleared after use due to interface limitations with redirectWithSuccessMessage/redirectWithErrorMessage not supporting custom headers. The maintainer accepts this design as the 1-hour cookie expiration provides sufficient protection against replay attacks.
Applied to files:
apps/webapp/app/services/sessionDuration.server.ts
📚 Learning: 2026-03-26T09:02:07.973Z
Learnt from: myftija
Repo: triggerdotdev/trigger.dev PR: 3274
File: apps/webapp/app/services/runsReplicationService.server.ts:922-924
Timestamp: 2026-03-26T09:02:07.973Z
Learning: When parsing Trigger.dev task run annotations in server-side services, keep `TaskRun.annotations` strictly conforming to the `RunAnnotations` schema from `trigger.dev/core/v3`. If the code already uses `RunAnnotations.safeParse` (e.g., in a `#parseAnnotations` helper), treat that as intentional/necessary for atomic, schema-accurate annotation handling. Do not recommend relaxing the annotation payload schema or using a permissive “passthrough” parse path, since the annotations are expected to be written atomically in one operation and should not contain partial/legacy payloads that would require a looser parser.
Applied to files:
apps/webapp/app/services/sessionDuration.server.ts
🔇 Additional comments (1)
apps/webapp/test/sessionDuration.test.ts (1)
6-17: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftTest imports a server module with indirect env coupling.
This suite imports
sessionDuration.server.ts, which pulls env-backed session storage transitively. Split pure session-duration logic into an env-free module and import that from tests.As per coding guidelines: "apps/webapp/**/*.test.{ts,tsx}: Do not import
env.server.tsdirectly or indirectly into test files; instead pass environment-dependent values through options/parameters to make code testable".⛔ Skipped due to learnings
Learnt from: CR Repo: triggerdotdev/trigger.dev PR: 0 File: .cursor/rules/webapp.mdc:0-0 Timestamp: 2026-04-15T15:39:31.575Z Learning: Applies to apps/webapp/**/*.test.{ts,tsx} : Do not import `env.server.ts` directly or indirectly into test files; instead pass environment-dependent values through options/parameters to make code testableLearnt from: CR Repo: triggerdotdev/trigger.dev PR: 0 File: apps/webapp/CLAUDE.md:0-0 Timestamp: 2026-04-16T14:19:16.330Z Learning: Applies to apps/webapp/**/*.test.{ts,tsx} : For testable code, never import `env.server.ts` in test files. Pass configuration as options instead (e.g., `realtimeClient.server.ts` takes config as constructor arg, `realtimeClientGlobal.server.ts` creates singleton with env config)Learnt from: matt-aitken Repo: triggerdotdev/trigger.dev PR: 3331 File: apps/webapp/test/engine/batchPayloads.test.ts:5-24 Timestamp: 2026-04-07T14:12:18.946Z Learning: In `apps/webapp/test/engine/batchPayloads.test.ts`, using `vi.mock` for `~/v3/objectStore.server` (stubbing `hasObjectStoreClient` and `uploadPacketToObjectStore`), `~/env.server` (overriding offload thresholds), and `~/v3/tracer.server` (stubbing `startActiveSpan`) is intentional and acceptable. Simulating controlled transient upload failures (e.g., fail N times then succeed) to verify `p-retry` behavior cannot be reproduced with real services or testcontainers. This file is an explicit exception to the repo's general no-mocks policy.Learnt from: ericallam Repo: triggerdotdev/trigger.dev PR: 3368 File: apps/webapp/test/engine/taskIdentifierRegistry.test.ts:3-19 Timestamp: 2026-04-16T13:45:22.317Z Learning: In `apps/webapp/test/engine/taskIdentifierRegistry.test.ts`, the `vi.mock` calls for `~/services/taskIdentifierCache.server` (stubbing `getTaskIdentifiersFromCache` and `populateTaskIdentifierCache`), `~/models/task.server` (stubbing `getAllTaskIdentifiers`), and `~/db.server` (stubbing `prisma` and `$replica`) are intentional. The suite uses real Postgres via testcontainers for all `TaskIdentifier` DB operations, but isolates the Redis cache layer and legacy query fallback as separate concerns not exercised in this test file. Do not flag these mocks as violations of the no-mocks policy in future reviews.Learnt from: CR Repo: triggerdotdev/trigger.dev PR: 0 File: CLAUDE.md:0-0 Timestamp: 2026-04-15T15:39:06.868Z Learning: Applies to **/*.test.{ts,tsx} : Use vitest exclusively for testing. Never mock anything — use testcontainers instead.Learnt from: CR Repo: triggerdotdev/trigger.dev PR: 0 File: internal-packages/run-engine/CLAUDE.md:0-0 Timestamp: 2026-03-02T12:43:25.254Z Learning: Applies to internal-packages/run-engine/src/engine/tests/**/*.test.ts : Implement tests for RunEngine in `src/engine/tests/` using testcontainers for Redis and PostgreSQL containerizationLearnt from: CR Repo: triggerdotdev/trigger.dev PR: 0 File: CLAUDE.md:0-0 Timestamp: 2026-04-15T15:39:06.868Z Learning: Applies to **/*.test.{ts,tsx} : For Redis/PostgreSQL tests in vitest, use testcontainers helpers: `redisTest`, `postgresTest`, or `containerTest` imported from `internal/testcontainers`.Learnt from: CR Repo: triggerdotdev/trigger.dev PR: 0 File: .cursor/rules/webapp.mdc:0-0 Timestamp: 2026-04-15T15:39:31.575Z Learning: Applies to apps/webapp/**/*.{ts,tsx} : Access environment variables through the `env` export of `env.server.ts` instead of directly accessing `process.env`Learnt from: CR Repo: triggerdotdev/trigger.dev PR: 0 File: apps/webapp/CLAUDE.md:0-0 Timestamp: 2026-04-16T14:19:16.330Z Learning: Applies to apps/webapp/**/*.server.ts : Access environment variables via `env` export from `app/env.server.ts`. Never use `process.env` directlyLearnt from: CR Repo: triggerdotdev/trigger.dev PR: 0 File: .github/copilot-instructions.md:0-0 Timestamp: 2025-11-27T16:26:37.432Z Learning: Applies to **/*.{test,spec}.{ts,tsx} : Use vitest for all tests in the Trigger.dev repositoryLearnt from: CR Repo: triggerdotdev/trigger.dev PR: 0 File: AGENTS.md:0-0 Timestamp: 2026-01-15T10:48:02.687Z Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Tests should avoid mocks or stubs and use the helpers from `internal/testcontainers` when Redis or Postgres are needed
| const userSettingSeconds = user?.sessionDuration ?? DEFAULT_SESSION_DURATION_SECONDS; | ||
| const durationSeconds = | ||
| orgCap === null ? userSettingSeconds : Math.min(userSettingSeconds, orgCap.orgCapSeconds); | ||
|
|
There was a problem hiding this comment.
Effective duration should defensively clamp invalid DB values.
If user.sessionDuration or org cap contains an out-of-policy tiny value (e.g., 1), Line 95-98 will enforce near-immediate logout. Add a server-side minimum clamp before computing durationSeconds.
💡 Suggested fix
+const MIN_SESSION_DURATION_SECONDS = 60 * 5;
+
+function clampSessionDuration(seconds: number): number {
+ return Math.max(MIN_SESSION_DURATION_SECONDS, seconds);
+}
+
export async function getEffectiveSessionDuration(
userId: string,
client: PrismaClientOrTransaction = prisma
): Promise<EffectiveSessionDuration> {
@@
- const userSettingSeconds = user?.sessionDuration ?? DEFAULT_SESSION_DURATION_SECONDS;
+ const userSettingSeconds = clampSessionDuration(
+ user?.sessionDuration ?? DEFAULT_SESSION_DURATION_SECONDS
+ );
+ const orgCapSeconds = orgCap ? clampSessionDuration(orgCap.orgCapSeconds) : null;
const durationSeconds =
- orgCap === null ? userSettingSeconds : Math.min(userSettingSeconds, orgCap.orgCapSeconds);
+ orgCapSeconds === null ? userSettingSeconds : Math.min(userSettingSeconds, orgCapSeconds);
@@
- orgCapSeconds: orgCap?.orgCapSeconds ?? null,
+ orgCapSeconds,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/webapp/app/services/sessionDuration.server.ts` around lines 95 - 98, The
code uses userSettingSeconds (from user?.sessionDuration) and
orgCap.orgCapSeconds directly when computing durationSeconds, allowing
tiny/invalid DB values to force near-immediate logout; add a defensive minimum
clamp (e.g., MIN_SESSION_DURATION_SECONDS constant) and apply it to both the
resolved userSettingSeconds and the org cap before taking Math.min: compute
clampedUserSetting = Math.max(userSettingSeconds ??
DEFAULT_SESSION_DURATION_SECONDS, MIN_SESSION_DURATION_SECONDS) and
clampedOrgCap = orgCap === null ? null : Math.max(orgCap.orgCapSeconds,
MIN_SESSION_DURATION_SECONDS), then compute durationSeconds using clamped values
so durationSeconds = clampedOrgCap === null ? clampedUserSetting :
Math.min(clampedUserSetting, clampedOrgCap).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal-packages/database/prisma/schema.prisma`:
- Around line 59-60: Add DB-level CHECK constraints to enforce a minimum session
duration while allowing NULL: for each model that defines the sessionDuration
Int field (e.g., the field named sessionDuration and the other occurrence around
lines ~227-231), add a model-level @@check with an expression like
"sessionDuration IS NULL OR sessionDuration >= 60" (choose the policy minimum in
seconds) and give a descriptive name (e.g., check_min_session_duration) so
Prisma will emit a SQL CHECK in the migration; update both models that declare
sessionDuration to include their own @@check entries so non-API writes cannot
set sub-minute values while still permitting NULL.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 50a1a0a0-7efd-4abc-98bc-bef818e4f770
📒 Files selected for processing (1)
internal-packages/database/prisma/schema.prisma
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: audit
- GitHub Check: Analyze (actions)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (python)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-03-13T13:42:59.104Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3213
File: apps/webapp/app/routes/admin.api.v1.llm-models.$modelId.ts:40-43
Timestamp: 2026-03-13T13:42:59.104Z
Learning: In `apps/webapp/app/routes/admin.api.v1.llm-models.$modelId.ts` and `apps/webapp/app/routes/admin.api.v1.llm-models.ts`, the `startDate` field in `UpdateModelSchema` and `CreateModelSchema` intentionally uses `z.string().optional()` (or `.nullable().optional()`) without strict ISO datetime validation. Invalid date strings are rejected at the Prisma/DB layer. This is acceptable because these are admin-only API routes protected by Personal Access Token (PAT) authentication and are not user-facing.
Applied to files:
internal-packages/database/prisma/schema.prisma
| sessionDuration Int @default(31556952) | ||
|
|
There was a problem hiding this comment.
Add DB-level lower-bound constraints for session duration fields
Line 59 and Line 230 currently permit any integer at the database layer. That allows out-of-policy values (e.g., 1 second) via non-API write paths and can cause immediate/near-immediate forced logout behavior. Please enforce the invariant in SQL with CHECK constraints (while preserving NULL for org cap unset).
Suggested migration-level fix
+-- Enforce minimum for user session duration
+ALTER TABLE "public"."User"
+ADD CONSTRAINT "User_sessionDuration_min_check"
+CHECK ("sessionDuration" >= 60);
+
+-- Enforce minimum for org cap when provided
+ALTER TABLE "public"."Organization"
+ADD CONSTRAINT "Organization_maxSessionDuration_min_check"
+CHECK ("maxSessionDuration" IS NULL OR "maxSessionDuration" >= 60);Also applies to: 227-231
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal-packages/database/prisma/schema.prisma` around lines 59 - 60, Add
DB-level CHECK constraints to enforce a minimum session duration while allowing
NULL: for each model that defines the sessionDuration Int field (e.g., the field
named sessionDuration and the other occurrence around lines ~227-231), add a
model-level @@check with an expression like "sessionDuration IS NULL OR
sessionDuration >= 60" (choose the policy minimum in seconds) and give a
descriptive name (e.g., check_min_session_duration) so Prisma will emit a SQL
CHECK in the migration; update both models that declare sessionDuration to
include their own @@check entries so non-API writes cannot set sub-minute values
while still permitting NULL.
There was a problem hiding this comment.
Skip. The finding has multiple problems:
@@check doesn't exist in Prisma. The reviewer claims "Prisma will emit a SQL CHECK in the migration" — that is factually wrong as of Prisma 6.14 (this project's version). CHECK constraints are a long-open Prisma feature request (prisma/prisma#3388), still unimplemented. Adding @@check to the schema would fail prisma validate.
User.sessionDuration (line 59) is Int, not Int?. The proposed IS NULL OR sessionDuration >= 60 clause has a dead IS NULL branch on this column — it can never be null. Only Organization.maxSessionDuration (line 230) is nullable.
Zero CHECK constraints exist anywhere in the project's migrations (verified earlier in this session). Adding one here would be the first, and inconsistent with the codebase pattern of app-level validation.
The app layer already enforces a stricter rule than the proposed 60-second minimum:
Admin API: must be in SESSION_DURATION_OPTIONS (smallest = 300s/5min, not 60s)
User UI: same constraint via isAllowedSessionDuration
A 60s DB minimum would be a regression in tightness.
The threat the constraint defends against doesn't really exist. "Direct SQL writes can't insert invalid values" — but anyone with DB write access can ALTER TABLE ... DROP CONSTRAINT first. CHECK constraints don't add real defense-in-depth against an attacker with write privileges; they only protect against typos in admin scripts.
If you actually want this defense, the correct path would be a hand-written migration with raw SQL (ALTER TABLE ... ADD CONSTRAINT ... CHECK ...), not via Prisma syntax — but I'd still push back on it for reasons 3-5. No code change.
There was a problem hiding this comment.
@samejr, you're right on all counts — I apologize for the inaccurate review comment. To confirm the key factual issues:
@@checkhas no native Prisma schema representation — it remains a long-open feature request (prisma/prisma#3388). There is no Prisma attribute equivalent; the only path is raw SQL in a migration file. My suggestion that "Prisma will emit a SQL CHECK" was wrong.User.sessionDurationisInt(non-nullable), so theIS NULL ORbranch I proposed is a dead condition — that was a logic error on my part.- The proposed 60s floor is actually weaker than the app layer's existing enforcement via
isAllowedSessionDuration(smallest allowed value inSESSION_DURATION_OPTIONSis 300s/5 min). - The codebase consistently relies on app-layer validation rather than DB-level CHECK constraints, and adding one here in isolation would be inconsistent.
Acknowledged — no code change needed.
✏️ Learnings added
Learnt from: samejr
Repo: triggerdotdev/trigger.dev PR: 3473
File: internal-packages/database/prisma/schema.prisma:59-60
Timestamp: 2026-04-30T21:28:35.705Z
Learning: In `triggerdotdev/trigger.dev`, Prisma does NOT support `@check` or any native CHECK constraint syntax in the schema (prisma/prisma#3388 is a long-open unimplemented feature request). CHECK constraints must be added via raw SQL `ALTER TABLE ... ADD CONSTRAINT ... CHECK (...)` in a hand-written migration file. Do not suggest `@check` in Prisma schema reviews for this project.
Learnt from: samejr
Repo: triggerdotdev/trigger.dev PR: 3473
File: internal-packages/database/prisma/schema.prisma:59-60
Timestamp: 2026-04-30T21:28:35.705Z
Learning: In `triggerdotdev/trigger.dev` (`apps/webapp/app/services/sessionDuration.server.ts`), session duration lower-bound validation is enforced entirely at the app layer via `isAllowedSessionDuration`, which requires values to be present in `SESSION_DURATION_OPTIONS`. The minimum allowed value is 300 seconds (5 minutes), not 60 seconds. `User.sessionDuration` is a non-nullable `Int`; only `Organization.maxSessionDuration` is nullable (`Int?`). No DB-level CHECK constraints exist anywhere in the project's migrations — the project pattern is app-layer validation only. Do not flag missing DB-level CHECK constraints on these session duration fields in future reviews.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3417
File: apps/webapp/app/services/sessionsRepository/clickhouseSessionsRepository.server.ts:27-40
Timestamp: 2026-04-20T15:08:59.789Z
Learning: In `apps/webapp/app/services/sessionsRepository/clickhouseSessionsRepository.server.ts`, the cursor predicate in `listSessionIds` compares only `session_id` while the `ORDER BY` clause uses `(created_at, session_id)`. This is intentional and consistent with the same pattern in `ClickHouseRunsRepository` and the waitpoints repository. Do not flag this as a skip/duplicate pagination bug in isolation — any fix must land across all three repositories at once as a shared follow-up.
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3417
File: apps/webapp/app/routes/realtime.v1.sessions.$session.$io.append.ts:20-31
Timestamp: 2026-04-20T15:05:59.661Z
Learning: In `apps/webapp/app/routes/realtime.v1.sessions.$session.$io.append.ts`, the `MAX_APPEND_BODY_BYTES` cap is intentionally set to `1024 * 512` (512 KiB). The maintainer explicitly decided against lowering it to 128 KiB: the all-quotes worst-case JSON-escaping expansion that could exceed S2's 1 MiB per-record limit is considered pathological and not representative of real-world payloads (chat tokens, tool-call JSON, structured data). If overflow becomes a problem in practice, the preferred mitigation is an encoded-size guard inside `appendPart` itself. Do not flag this cap as a potential S2 overflow issue in future reviews.
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3417
File: apps/webapp/app/routes/realtime.v1.streams.$runId.$target.$streamId.append.ts:16-26
Timestamp: 2026-04-20T15:06:11.054Z
Learning: In `apps/webapp/app/routes/realtime.v1.streams.$runId.$target.$streamId.append.ts` and `apps/webapp/app/routes/realtime.v1.sessions.$session.$io.append.ts`, the `MAX_APPEND_BODY_BYTES` cap of 512 KiB (1024 * 512) is intentional even though `appendPart` wraps the body in JSON (which could expand quote-heavy payloads beyond S2's 1 MiB per-record limit). The maintainer considers worst-case quote-heavy payloads pathological and not realistic. If S2 rejections occur in practice, an encoded-size guard will be added inside `appendPart` rather than lowering the raw body cap on every caller. Do not flag this as an issue in future reviews.
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3417
File: apps/webapp/app/routes/realtime.v1.sessions.$session.$io.ts:37-51
Timestamp: 2026-04-20T15:06:19.815Z
Learning: In `apps/webapp/app/routes/realtime.v1.sessions.$session.$io.ts` (and all session realtime read paths), `$replica` is intentionally used for the `resolveSessionByIdOrExternalId` call — including the `closedAt` guard in the PUT/initialize path. The project convention is to use `$replica` consistently across all session realtime routes. The race window (replica lag allowing a ghost-initialize after close) is accepted as not realistic in practice (clients follow the close API response; they do not race it). If replica lag ever causes issues, the mitigation is to revisit all realtime routes together, not to swap individual routes to `prisma`. Do not flag `$replica` usage in session realtime routes as a stale-read issue.
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3244
File: internal-packages/database/prisma/migrations/20260318114244_add_prompt_friendly_id/migration.sql:5-5
Timestamp: 2026-03-22T13:49:23.474Z
Learning: In `internal-packages/database/prisma/migrations/**/*.sql`: When a column and its index are added in a follow-up migration file but the parent table itself was introduced in the same PR (i.e., no production rows exist yet), a plain `CREATE INDEX` / `CREATE UNIQUE INDEX` (without CONCURRENTLY) is safe and does not require splitting into a separate migration. The CONCURRENTLY requirement only applies when the table already has existing data in production.
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3368
File: internal-packages/database/prisma/schema.prisma:666-666
Timestamp: 2026-04-16T14:21:18.496Z
Learning: In `triggerdotdev/trigger.dev`, the `BackgroundWorkerTask` covering index on `(runtimeEnvironmentId, slug, triggerSource)` lives in `internal-packages/database/prisma/migrations/20260413000000_add_bwt_covering_index/migration.sql` as a `CREATE INDEX CONCURRENTLY IF NOT EXISTS`, intentionally in its own migration file separate from the `TaskIdentifier` table migration. Do not flag this index as missing from the schema migrations in future reviews.
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: internal-packages/database/CLAUDE.md:0-0
Timestamp: 2026-03-02T12:43:17.177Z
Learning: Applies to internal-packages/database/**/prisma/migrations/*/*.sql : When adding indexes to existing tables, use `CREATE INDEX CONCURRENTLY IF NOT EXISTS` to avoid table locks in production, and place each concurrent index in its own separate migration file
| const { durationSeconds, orgCapSeconds } = await getEffectiveSessionDuration(user.id, $replica); | ||
| const sessionDurationOptions = getAllowedSessionOptions(orgCapSeconds, durationSeconds); |
There was a problem hiding this comment.
🚩 getAllowedSessionOptions receives effective duration, not raw user setting
Both account.security/route.tsx:32 and resources.account.session-duration/route.tsx:50 pass durationSeconds (the effective/capped value = min(userSetting, orgCap)) as the currentValueSeconds parameter to getAllowedSessionOptions. The function's JSDoc says it always includes the user's currently-saved value even when it exceeds the cap. However, since the effective value is always ≤ the org cap, it's always already present in the filtered list, so this doesn't cause a functional issue. The consequence is that when an org cap is lower than the user's saved setting, the dropdown shows the capped value (not the user's raw DB value). The user's raw DB setting is preserved until they explicitly change it, so no data is silently lost. This is arguably the correct UX but doesn't match the function's documented intent.
Was this helpful? React with 👍 or 👎 to provide feedback.
Performance
getUserIdrunsgetEffectiveSessionDuration(User lookup + Orgaggregate) on every authenticated request, including each fetcher poll. Consider caching the effective duration in the session cookie with a short TTL (e.g. 60s) and revalidating in the background.root.tsx:getUseralready runs the expiry check; thencommitAuthenticatedSessionLazycommits the cookie again. Fine, but doublesSet-Cookieheaders on every page load — worth a quick perf check.Correctness / Edge cases
/resources/*) skip the backfill until they navigate to a page. Not a security hole, butgetUserIdcould backfill itself for completeness.Organization.maxSessionDuration: admin API accepts1second, which would instant-logout every member on next request. Add amin(60)(ormin(300)to match the lowest user option) to the Zod schema.isSessionExpiredis exact-millisecond. Multi-instance deploys with skewed clocks could log users out a few seconds early/late. Probably fine for the 5-min minimum, but worth noting.Security
userId+ path. IP isn't PII for audit purposes; orgIds help correlate. Add both.Max-Ageis 1 year regardless of user's setting: intentional (server-sideissuedAtis the source of truth), but reviewers will ask. Add a one-line comment on the cookie config explaining why.API surface
maxSessionDurationis admin-PAT only: no in-app UI for org owners to set/change their own cap. If this is "Trigger staff sets it during HIPAA onboarding", say so in the PR description; otherwise add an org-settings UI.Schema / migration
User.sessionDuration NOT NULL DEFAULT 31556952: instant on PG 11+ (metadata-only), but call out in the PR description so reviewers don't worry about a table rewrite on the User table.SESSION_DURATION_OPTIONS: if the option list changes, existing users keep orphaned values. The dropdown's tag-along behaviour hides this — fine for now, but if you ever drop an option you'll need a backfill.UX
/accountor/logoutat expiry./with no explanation. Was intentionally reverted in this PR — call that out so reviewers don't request it.Tests
sessionDuration.server.tsis solid (215 lines). Missing: integration test forgetUserId→ expired session → redirect to/logout, and one for the loader's clamping fix (the most recent bug). Add at least the second one to lock in the regression.