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.
|
WalkthroughThis pull request implements automatic logout functionality via configurable session duration limits. It introduces a new Prisma schema with Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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 docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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.
| <Switch | ||
| id="mfa" | ||
| variant="medium" | ||
| labelPosition="right" | ||
| className="w-fit pr-3" | ||
| checked={isEnabled} | ||
| onCheckedChange={onToggle} | ||
| /> |
There was a problem hiding this comment.
Switch may no longer have an accessible name.
On Line 23-30, label was removed from Switch, and the text label (Line 17) is not programmatically linked to the control. Please add aria-label/aria-labelledby (or restore a semantic label) to keep screen-reader support intact.
Suggested fix
- <Label>Multi-factor authentication</Label>
+ <Label id="mfa-label">Multi-factor authentication</Label>
<Paragraph variant="small">
Require a one-time code from your authenticator app (TOTP).
</Paragraph>
@@
<Switch
id="mfa"
+ aria-labelledby="mfa-label"
variant="medium"
labelPosition="right"
className="w-fit pr-3"
checked={isEnabled}
onCheckedChange={onToggle}
/>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/webapp/app/routes/resources.account.mfa.setup/MfaToggle.tsx` around
lines 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.
| const [user, orgCap] = await Promise.all([ | ||
| client.user.findUnique({ | ||
| where: { id: userId }, | ||
| select: { sessionDuration: true }, | ||
| }), | ||
| getOrganizationSessionCap(userId, client), | ||
| ]); |
There was a problem hiding this comment.
Use findFirst instead of findUnique.
The coding guidelines explicitly require using findFirst over findUnique due to Prisma's implicit DataLoader batching having active bugs (uppercase UUIDs returning null, composite key SQL correctness issues, 5-10x worse performance).
🔧 Proposed fix
const [user, orgCap] = await Promise.all([
- client.user.findUnique({
+ client.user.findFirst({
where: { id: userId },
select: { sessionDuration: true },
}),
getOrganizationSessionCap(userId, client),
]);As per coding guidelines: "Always use findFirst instead of findUnique in Prisma queries."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const [user, orgCap] = await Promise.all([ | |
| client.user.findUnique({ | |
| where: { id: userId }, | |
| select: { sessionDuration: true }, | |
| }), | |
| getOrganizationSessionCap(userId, client), | |
| ]); | |
| const [user, orgCap] = await Promise.all([ | |
| client.user.findFirst({ | |
| where: { id: userId }, | |
| select: { sessionDuration: true }, | |
| }), | |
| getOrganizationSessionCap(userId, client), | |
| ]); |
🤖 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 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).
| const NON_NAVIGABLE_PREFIXES = ["/resources/", "/auth/", "/admin/", "/api/", "/engine/"]; | ||
| const NON_NAVIGABLE_EXACT = new Set(["/magic", "/logout", "/login", "/login/magic", "/login/mfa"]); |
There was a problem hiding this comment.
Overly broad /admin/ block breaks valid redirect targets.
Line 9 blocks every /admin/ path, which includes real admin pages. This will drop legitimate redirectTo values and send users to the default route after login.
🔧 Narrow the blocked admin prefix
-const NON_NAVIGABLE_PREFIXES = ["/resources/", "/auth/", "/admin/", "/api/", "/engine/"];
+const NON_NAVIGABLE_PREFIXES = ["/resources/", "/auth/", "/admin/api/", "/api/", "/engine/"];Based on learnings: In apps/webapp/app/routes/admin.llm-models.missing.$model.tsx, /admin/* is used for navigable admin pages.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const NON_NAVIGABLE_PREFIXES = ["/resources/", "/auth/", "/admin/", "/api/", "/engine/"]; | |
| const NON_NAVIGABLE_EXACT = new Set(["/magic", "/logout", "/login", "/login/magic", "/login/mfa"]); | |
| const NON_NAVIGABLE_PREFIXES = ["/resources/", "/auth/", "/admin/api/", "/api/", "/engine/"]; | |
| const NON_NAVIGABLE_EXACT = new Set(["/magic", "/logout", "/login", "/login/magic", "/login/mfa"]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/webapp/app/utils.ts` around lines 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.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| client.user.findUnique({ | ||
| where: { id: userId }, | ||
| select: { sessionDuration: true }, | ||
| }), |
There was a problem hiding this comment.
🔴 Uses findUnique instead of findFirst, violating mandatory repository rule
The getEffectiveSessionDuration function at apps/webapp/app/services/sessionDuration.server.ts:77 uses client.user.findUnique(). The repository's apps/webapp/CLAUDE.md:126 explicitly mandates: "Always use findFirst instead of findUnique. Prisma's findUnique has an implicit DataLoader that batches concurrent calls into a single IN query. This batching cannot be disabled and has active bugs even in Prisma 6.x." Since getEffectiveSessionDuration is called on every authenticated request (via getUserId in apps/webapp/app/services/session.server.ts:37), the DataLoader batching bug risk is amplified.
| client.user.findUnique({ | |
| where: { id: userId }, | |
| select: { sessionDuration: true }, | |
| }), | |
| client.user.findFirst({ | |
| where: { id: userId }, | |
| select: { sessionDuration: true }, | |
| }), |
Was this helpful? React with 👍 or 👎 to provide feedback.
| return authUser?.userId; | ||
| } |
There was a problem hiding this comment.
🟡 Session expiry check bypassed when stale impersonation cookie is present
In getUserId, the new session-duration expiry enforcement (lines 36–54) is only applied to the non-impersonation code path. When impersonatedUserId is truthy but the real user is not an admin (e.g., admin privileges were revoked), the function falls through to return authUser?.userId at line 27 without checking isSessionExpired. This means any authenticated user with a stale __impersonate cookie (up to 1-day lifetime per apps/webapp/app/services/impersonation.server.ts:12) bypasses session-duration enforcement entirely, even if their effective duration is as short as 5 minutes.
(Refers to lines 17-28)
Prompt for agents
In getUserId (session.server.ts), the session-expiry check added at lines 36-54 only runs when impersonatedUserId is falsy. The impersonation branch (lines 17-28) has two early returns that skip expiry enforcement entirely:
1. Line 23: return impersonatedUserId (admin impersonating) — skipping expiry here may be intentional for admin sessions.
2. Line 27: return authUser?.userId (non-admin fallback when admin was revoked) — this path should enforce session expiry, since the user is just getting their own ID back.
To fix: extract the session-expiry check into a helper function and call it before returning authUser?.userId on line 27. Consider whether admin-impersonation sessions (line 23) should also be subject to expiry based on the real user's session duration.
Was this helpful? React with 👍 or 👎 to provide feedback.
| 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.
🚩 Auth callbacks commit a __message Session object via __session's commitSession
In auth.github.callback.tsx and auth.google.callback.tsx, getSession is imported from ~/models/message.server (which reads the __message cookie), but the resulting Session object is committed via commitAuthenticatedSession which wraps commitSession from ~/services/sessionStorage.server (which writes the __session cookie). This means the __session cookie is populated with the __message session's data plus the new auth key and issuedAt. This is a pre-existing pattern (the old code did the same with plain commitSession from sessionStorage), and it works because Remix's commitSession serializes whatever data is in the Session object to its associated cookie name regardless of which storage created the Session. However, it's fragile and non-obvious — any flash message data in __message leaks into __session.
(Refers to lines 22-56)
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.
🚩 Session expiry adds DB queries to every authenticated request
The getUserId function now calls getEffectiveSessionDuration(authUser.userId) on every authenticated request (session.server.ts:37). This issues two queries: a user.findFirst (should be, currently findUnique) and an organization.aggregate. Additionally, getUserSession(request) parses the __session cookie. For the hot path of every authenticated page load, these extra queries and cookie parsing add latency. Consider whether caching the effective duration (e.g., in the session cookie itself alongside issuedAt) could reduce per-request overhead, especially for users who don't change their duration frequently.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
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.