Conversation
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📜 Recent review details⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (29)
WalkthroughAdds per-user session-duration controls and organization-level caps. Database migrations add User.sessionDuration, User.nextSessionEnd, and Organization.maxSessionDuration. A new sessionDuration service provides selectable durations, allowlist validation, org-cap resolution, effective-duration computation, and commitAuthenticatedSession which updates User.nextSessionEnd. Enforcement via maybeAutoLogout is integrated into session retrieval paths. Login/auth flows and affected routes now use the duration-aware session committer. Admin and account routes/components allow viewing/updating caps and durations. Redirect sanitization, UI layout tweaks, tests for session-duration logic, and migration SQL files were added. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/webapp/app/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.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…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.
Move the session deadline from a per-request DB resolution to a User.nextSessionEnd column written at the rare moments it can change. Previously enforceSessionExpiry ran inside getUserId on every authenticated request, doing 2 replica queries (one of which joins OrgMember and sorts on maxSessionDuration). Remix runs matched layout loaders in parallel, so a single dashboard navigation produced ~12 replica queries before any page data was fetched, and resources/fetcher routes paid the same cost. - commitAuthenticatedSession resolves effective duration once and stamps User.nextSessionEnd at every login/MFA/user-setting change. - Admin org-cap route runs a single bulk UPDATE … LEAST(…) to shorten member deadlines without ever extending them. - requireUser/getUser check now > nextSessionEnd against the User row they were already fetching — no per-request DB query added. - requireUserId reverts to cookie-only (zero DB queries on fetcher routes). - Migration adds the column as nullable; metadata-only ALTER, no row rewrite. Null = no enforced deadline, equivalent to the default 1-year duration that matches cookie maxAge — existing sessions need no backfill. - Removed dead cookie-issuedAt machinery (commitAuthenticatedSessionLazy, isSessionExpired, ensureSessionIssuedAt, SESSION_ISSUED_AT_KEY). - Consolidated branch's .server-changes notes into one file.
matt-aitken
force-pushed
the
feat(webapp)-auto-app-logout
branch
from
d77d5fc to
4fa011d
Compare
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: 3
🧹 Nitpick comments (2)
apps/webapp/test/sessionDuration.test.ts (1)
1-14: 🏗️ Heavy liftKeep this spec next to the service and out of the
env.serverimport chain.Importing
../app/services/sessionDuration.serverfromapps/webapp/test/...pulls in the server-only module graph, including thesessionStorage.server.tsdependency. That makes this new test violate the repo's two webapp test rules at once: colocate the spec with the source, and don't importenv.server.tsdirectly or indirectly into tests. Splitting the pure duration logic into an env-free module beside the service would fix both.As per coding guidelines:
**/*.test.{ts,tsx,js}: Place test files next to source files using the patternMyService.ts->MyService.test.ts; andapps/webapp/**/*.test.{ts,tsx}: Do not importenv.server.tsdirectly or indirectly into test files.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/test/sessionDuration.test.ts` around lines 1 - 14, The test imports sessionDuration.server which pulls server-only env and sessionStorage; fix by extracting the pure duration logic (functions/constants: getEffectiveSessionDuration, isAllowedSessionDuration, getAllowedSessionOptions, SESSION_DURATION_OPTIONS, DEFAULT_SESSION_DURATION_SECONDS, getOrganizationSessionCap) into a new env-free module (e.g., sessionDuration.lib.ts) colocated with the service, update sessionDuration.server to re-export or import that lib, then move the test file next to the service and import the new lib directly so the test no longer transitively imports env.server or sessionStorage.server.apps/webapp/app/routes/resources.account.session-duration/SessionDurationSetting.tsx (1)
8-12: ⚡ Quick winUse a type alias for the props.
This repo standardizes on
typerather thaninterfacein TypeScript.♻️ Proposed fix
-interface SessionDurationSettingProps { +type SessionDurationSettingProps = { currentValue: number; options: SessionDurationOption[]; orgCapSeconds: number | null; -} +};As per coding guidelines:
**/*.{ts,tsx}: Use types over interfaces for TypeScript.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/routes/resources.account.session-duration/SessionDurationSetting.tsx` around lines 8 - 12, Replace the interface SessionDurationSettingProps with a type alias: change "interface SessionDurationSettingProps { ... }" to "type SessionDurationSettingProps = { ... };" keeping the exact property names and types (currentValue: number; options: SessionDurationOption[]; orgCapSeconds: number | null) and ensure the new declaration uses the = and trailing semicolon so it follows the repo's type-over-interface convention.
🤖 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 58-66: The bulk UPDATE must recompute each user's effective cap
across all their org memberships like getOrganizationSessionCap() does: change
the prisma.$executeRaw query that updates "User"."nextSessionEnd" to compute a
per-user minimum cap via a subquery joining "OrgMember" -> "Org" (e.g. SELECT
MIN(COALESCE("Org"."sessionDuration",'infinity')) FROM "Org" JOIN "OrgMember"
WHERE "OrgMember"."userId" = "User"."id") and then take LEAST between that
per-user min and the incoming body.maxSessionDuration before computing NOW() +
(...) so the update uses the tightest cap across all memberships (use the same
column names "User", "OrgMember", "Org", nextSessionEnd, sessionDuration and
keep body.maxSessionDuration in the LEAST).
- Around line 41-44: The handler currently calls await request.json() directly
so syntactically invalid JSON throws before RequestBodySchema.safeParse runs;
wrap the request.json() call in a try/catch, and if parsing throws (e.g.,
SyntaxError) return the same 400-style JSON response (e.g., json({ success:
false, errors: ... }, { status: 400 })); on success continue to call
RequestBodySchema.safeParse(parseResult) as before. Ensure you reference
RequestBodySchema and the existing parseResult variable flow so the error path
mirrors the validation failure response.
In `@apps/webapp/app/services/session.server.ts`:
- Around line 54-62: getUserId currently returns a user id from the cookie
without enforcing session expiry, allowing expired sessions to call API routes;
fix by making getUserId also verify session deadline: read the session expiry
value cached in the session (e.g. a stored User.nextSessionEnd or nextSessionEnd
key) and reject/return undefined if now > expiry, or alternatively route
non-page auth callers to a new helper (e.g. requireValidSession or
validateSessionDeadline) that calls getImpersonationId, then checks the cached
expiry and only falls back to authenticator.isAuthenticated when necessary;
ensure impersonation via getImpersonationId is preserved and that getUser and
requireUser remain the authoritative DB-check path when no cached expiry is
present.
---
Nitpick comments:
In
`@apps/webapp/app/routes/resources.account.session-duration/SessionDurationSetting.tsx`:
- Around line 8-12: Replace the interface SessionDurationSettingProps with a
type alias: change "interface SessionDurationSettingProps { ... }" to "type
SessionDurationSettingProps = { ... };" keeping the exact property names and
types (currentValue: number; options: SessionDurationOption[]; orgCapSeconds:
number | null) and ensure the new declaration uses the = and trailing semicolon
so it follows the repo's type-over-interface convention.
In `@apps/webapp/test/sessionDuration.test.ts`:
- Around line 1-14: The test imports sessionDuration.server which pulls
server-only env and sessionStorage; fix by extracting the pure duration logic
(functions/constants: getEffectiveSessionDuration, isAllowedSessionDuration,
getAllowedSessionOptions, SESSION_DURATION_OPTIONS,
DEFAULT_SESSION_DURATION_SECONDS, getOrganizationSessionCap) into a new env-free
module (e.g., sessionDuration.lib.ts) colocated with the service, update
sessionDuration.server to re-export or import that lib, then move the test file
next to the service and import the new lib directly so the test no longer
transitively imports env.server or sessionStorage.server.
🪄 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: 6c502ac0-4b3f-405b-974f-707144906993
📒 Files selected for processing (21)
.server-changes/app-auto-session-logout.mdapps/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/migrations/20260503104935_add_user_next_session_end/migration.sqlinternal-packages/database/prisma/schema.prisma
✅ Files skipped from review due to trivial changes (6)
- internal-packages/database/prisma/migrations/20260428140746_add_session_duration_columns/migration.sql
- .server-changes/app-auto-session-logout.md
- internal-packages/database/prisma/migrations/20260503104935_add_user_next_session_end/migration.sql
- apps/webapp/app/routes/login.magic/route.tsx
- apps/webapp/app/routes/auth.github.callback.tsx
- apps/webapp/app/routes/resources.account.mfa.setup/MfaToggle.tsx
🚧 Files skipped from review as they are similar to previous changes (7)
- apps/webapp/app/routes/login.mfa/route.tsx
- apps/webapp/app/routes/magic.tsx
- apps/webapp/app/utils.ts
- apps/webapp/app/routes/resources.account.session-duration/route.tsx
- apps/webapp/app/services/sessionStorage.server.ts
- apps/webapp/app/routes/resources.account.mfa.setup/route.tsx
- 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). (30)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 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 (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / e2e-webapp / 🧪 E2E Tests: Webapp
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: sdk-compat / Bun Runtime
- GitHub Check: sdk-compat / Node.js 20.20 (ubuntu-latest)
- GitHub Check: sdk-compat / Node.js 22.12 (ubuntu-latest)
- GitHub Check: sdk-compat / Deno Runtime
- GitHub Check: sdk-compat / Cloudflare Workers
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: audit
- GitHub Check: Analyze (javascript-typescript)
🧰 Additional context used
📓 Path-based instructions (15)
**/*.{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/auth.google.callback.tsxapps/webapp/app/root.tsxapps/webapp/app/services/sessionDuration.server.tsapps/webapp/app/services/session.server.tsapps/webapp/test/sessionDuration.test.tsapps/webapp/app/routes/admin.api.v1.orgs.$organizationId.session-duration.tsapps/webapp/app/routes/resources.account.session-duration/SessionDurationSetting.tsx
{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/auth.google.callback.tsxapps/webapp/app/root.tsxapps/webapp/app/services/sessionDuration.server.tsapps/webapp/app/services/session.server.tsapps/webapp/test/sessionDuration.test.tsapps/webapp/app/routes/admin.api.v1.orgs.$organizationId.session-duration.tsapps/webapp/app/routes/resources.account.session-duration/SessionDurationSetting.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use function declarations instead of default exports
Files:
apps/webapp/app/routes/auth.google.callback.tsxapps/webapp/app/root.tsxapps/webapp/app/services/sessionDuration.server.tsapps/webapp/app/services/session.server.tsapps/webapp/test/sessionDuration.test.tsapps/webapp/app/routes/admin.api.v1.orgs.$organizationId.session-duration.tsapps/webapp/app/routes/resources.account.session-duration/SessionDurationSetting.tsx
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/auth.google.callback.tsxapps/webapp/app/root.tsxapps/webapp/app/services/sessionDuration.server.tsapps/webapp/app/services/session.server.tsapps/webapp/test/sessionDuration.test.tsapps/webapp/app/routes/admin.api.v1.orgs.$organizationId.session-duration.tsapps/webapp/app/routes/resources.account.session-duration/SessionDurationSetting.tsx
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/auth.google.callback.tsxapps/webapp/app/root.tsxapps/webapp/app/routes/resources.account.session-duration/SessionDurationSetting.tsx
{apps,internal-packages}/**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
pnpm run typecheckto verify changes in apps and internal packages (apps/*,internal-packages/*) instead ofbuild, which proves almost nothing about correctness
Files:
apps/webapp/app/routes/auth.google.callback.tsxapps/webapp/app/root.tsxapps/webapp/app/services/sessionDuration.server.tsapps/webapp/app/services/session.server.tsapps/webapp/test/sessionDuration.test.tsapps/webapp/app/routes/admin.api.v1.orgs.$organizationId.session-duration.tsapps/webapp/app/routes/resources.account.session-duration/SessionDurationSetting.tsx
{package.json,**/*.{ts,tsx,js}}
📄 CodeRabbit inference engine (CLAUDE.md)
Pin Zod to version 3.25.76 exactly across the entire monorepo - never use a different version or version range
Files:
apps/webapp/app/routes/auth.google.callback.tsxapps/webapp/app/root.tsxapps/webapp/app/services/sessionDuration.server.tsapps/webapp/app/services/session.server.tsapps/webapp/test/sessionDuration.test.tsapps/webapp/app/routes/admin.api.v1.orgs.$organizationId.session-duration.tsapps/webapp/app/routes/resources.account.session-duration/SessionDurationSetting.tsx
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js}: Import from@trigger.dev/coreusing subpaths only, never the root export
Always import tasks from@trigger.dev/sdk, never from@trigger.dev/sdk/v3or deprecatedclient.defineJob
Add crumbs to code using//@Crumbscomments or `// `#region` `@crumbsblocks for debug tracing during development
Files:
apps/webapp/app/routes/auth.google.callback.tsxapps/webapp/app/root.tsxapps/webapp/app/services/sessionDuration.server.tsapps/webapp/app/services/session.server.tsapps/webapp/test/sessionDuration.test.tsapps/webapp/app/routes/admin.api.v1.orgs.$organizationId.session-duration.tsapps/webapp/app/routes/resources.account.session-duration/SessionDurationSetting.tsx
**/*.{ts,tsx,js,jsx,json,md,css,scss}
📄 CodeRabbit inference engine (AGENTS.md)
Code formatting is enforced using Prettier. Run
pnpm run formatbefore committing
Files:
apps/webapp/app/routes/auth.google.callback.tsxapps/webapp/app/root.tsxapps/webapp/app/services/sessionDuration.server.tsapps/webapp/app/services/session.server.tsapps/webapp/test/sessionDuration.test.tsapps/webapp/app/routes/admin.api.v1.orgs.$organizationId.session-duration.tsapps/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/services/sessionDuration.server.tsapps/webapp/app/services/session.server.tsapps/webapp/test/sessionDuration.test.tsapps/webapp/app/routes/admin.api.v1.orgs.$organizationId.session-duration.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.tsapps/webapp/app/services/session.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
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
**/*.test.{ts,tsx,js}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.test.{ts,tsx,js}: Use vitest exclusively for testing and never mock anything - use testcontainers instead
Place test files next to source files using the patternMyService.ts->MyService.test.ts
**/*.test.{ts,tsx,js}: Use vitest for unit testing and run tests withpnpm run test
Test files should live beside the files under test with descriptivedescribeanditblocks
Tests should avoid mocks or stubs and use helpers from@internal/testcontainerswhen Redis or Postgres are needed
Files:
apps/webapp/test/sessionDuration.test.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use testcontainers with
redisTest,postgresTest, orcontainerTestfrom@internal/testcontainersfor testing with Redis/PostgreSQL dependencies
Files:
apps/webapp/test/sessionDuration.test.ts
🧠 Learnings (6)
📚 Learning: 2026-04-30T21:28:35.705Z
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: When reviewing Prisma schema files in this repository, do not suggest using Prisma’s `@check` model/table-level attribute or any native Prisma schema syntax for CHECK constraints. Prisma does not implement CHECK constraints (see prisma/prisma#3388). If a CHECK constraint is required, add it only via raw SQL in a handwritten migration (e.g., `ALTER TABLE ... ADD CONSTRAINT ... CHECK (...)`).
Applied to files:
internal-packages/database/prisma/schema.prisma
📚 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/auth.google.callback.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/auth.google.callback.tsxapps/webapp/app/root.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/auth.google.callback.tsxapps/webapp/app/root.tsxapps/webapp/app/services/sessionDuration.server.tsapps/webapp/app/services/session.server.tsapps/webapp/test/sessionDuration.test.tsapps/webapp/app/routes/admin.api.v1.orgs.$organizationId.session-duration.tsapps/webapp/app/routes/resources.account.session-duration/SessionDurationSetting.tsx
📚 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/auth.google.callback.tsxapps/webapp/app/root.tsxapps/webapp/app/services/sessionDuration.server.tsapps/webapp/app/services/session.server.tsapps/webapp/test/sessionDuration.test.tsapps/webapp/app/routes/admin.api.v1.orgs.$organizationId.session-duration.tsapps/webapp/app/routes/resources.account.session-duration/SessionDurationSetting.tsx
📚 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.tsapps/webapp/app/services/session.server.ts
| const parseResult = RequestBodySchema.safeParse(await request.json()); | ||
| if (!parseResult.success) { | ||
| return json({ success: false, errors: parseResult.error.flatten() }, { status: 400 }); | ||
| } |
There was a problem hiding this comment.
Malformed JSON currently escapes as a 500.
request.json() can throw before Zod runs, so a syntactically invalid body never reaches the 400 response path.
🛡️ Proposed fix
- const parseResult = RequestBodySchema.safeParse(await request.json());
+ let rawBody: unknown;
+ try {
+ rawBody = await request.json();
+ } catch {
+ return json(
+ { success: false, errors: { formErrors: ["Request body must be valid JSON"], fieldErrors: {} } },
+ { status: 400 }
+ );
+ }
+
+ const parseResult = RequestBodySchema.safeParse(rawBody);📝 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 parseResult = RequestBodySchema.safeParse(await request.json()); | |
| if (!parseResult.success) { | |
| return json({ success: false, errors: parseResult.error.flatten() }, { status: 400 }); | |
| } | |
| let rawBody: unknown; | |
| try { | |
| rawBody = await request.json(); | |
| } catch { | |
| return json( | |
| { success: false, errors: { formErrors: ["Request body must be valid JSON"], fieldErrors: {} } }, | |
| { status: 400 } | |
| ); | |
| } | |
| const parseResult = RequestBodySchema.safeParse(rawBody); | |
| if (!parseResult.success) { | |
| return json({ success: false, errors: parseResult.error.flatten() }, { status: 400 }); | |
| } |
🤖 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 41 - 44, The handler currently calls await request.json() directly
so syntactically invalid JSON throws before RequestBodySchema.safeParse runs;
wrap the request.json() call in a try/catch, and if parsing throws (e.g.,
SyntaxError) return the same 400-style JSON response (e.g., json({ success:
false, errors: ... }, { status: 400 })); on success continue to call
RequestBodySchema.safeParse(parseResult) as before. Ensure you reference
RequestBodySchema and the existing parseResult variable flow so the error path
mirrors the validation failure response.
| if (body.maxSessionDuration !== null) { | ||
| await prisma.$executeRaw` | ||
| UPDATE "User" | ||
| SET "nextSessionEnd" = LEAST( | ||
| COALESCE("nextSessionEnd", 'infinity'::timestamp), | ||
| NOW() + (LEAST("sessionDuration", ${body.maxSessionDuration}) * INTERVAL '1 second') | ||
| ) | ||
| WHERE "id" IN (SELECT "userId" FROM "OrgMember" WHERE "organizationId" = ${organizationId}) | ||
| `; |
There was a problem hiding this comment.
Recompute the tightest cap across all memberships in this bulk update.
This query only uses body.maxSessionDuration. For users in multiple orgs, updating a non-tightest org can still stamp a later nextSessionEnd than their actual effective cap when the current value is NULL (legacy sessions) or otherwise stale/looser. The runtime path uses getOrganizationSessionCap() to take the minimum cap across all active orgs; this propagation query needs to mirror that rule.
🤖 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 58 - 66, The bulk UPDATE must recompute each user's effective cap
across all their org memberships like getOrganizationSessionCap() does: change
the prisma.$executeRaw query that updates "User"."nextSessionEnd" to compute a
per-user minimum cap via a subquery joining "OrgMember" -> "Org" (e.g. SELECT
MIN(COALESCE("Org"."sessionDuration",'infinity')) FROM "Org" JOIN "OrgMember"
WHERE "OrgMember"."userId" = "User"."id") and then take LEAST between that
per-user min and the incoming body.maxSessionDuration before computing NOW() +
(...) so the update uses the tightest cap across all memberships (use the same
column names "User", "OrgMember", "Org", nextSessionEnd, sessionDuration and
keep body.maxSessionDuration in the LEAST).
The previous refactor moved all admin-status verification into getUser, leaving requireUserId-only routes silently operating as the impersonation target if the admin's role was revoked mid-session (or their session expired). Restore the pre-refactor behaviour: on the impersonation path getUserId verifies admin status and enforces the admin's nextSessionEnd, falling back to the admin's own userId when admin is revoked. The non-impersonation fast path stays cookie-only (zero DB queries). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/webapp/app/services/session.server.ts (1)
76-80:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAuto-logout still doesn't apply to cookie-only
requireUserId()routes.This fast path still returns the session user id without consulting
nextSessionEnd, so any loader/action/resource route that only usesgetUserId()/requireUserId()can keep succeeding after expiry until a page navigation happens to callgetUser(). That leaves normal-user fetcher/API traffic outside the auto-logout guarantee.🤖 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 76 - 80, The fast-path in authenticator.isAuthenticated() that returns authUser?.userId skips the session expiry check and thus bypasses nextSessionEnd for cookie-only callers (getUserId()/requireUserId()); change the fast-path to fetch the session expiry (nextSessionEnd) and validate it before returning the userId—i.e., after calling authenticator.isAuthenticated(request) inspect the session's nextSessionEnd (or call the same helper getSession/getUser logic used by getUser) and if the session is expired treat it as unauthenticated (return null / force requireUserId to re-authenticate) so cookie-only routes honor auto-logout.
🤖 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/session.server.ts`:
- Around line 32-38: The expiry check in maybeAutoLogout uses an exact cutoff
and can trigger early due to clock skew; add a small grace window (e.g., const
SESSION_EXPIRY_GRACE_MS = 5000) and change the condition to compare Date.now()
against user.nextSessionEnd.getTime() + SESSION_EXPIRY_GRACE_MS so the function
(maybeAutoLogout) returns while now is <= end+grace; declare the constant near
the top of the module and use the symbol user.nextSessionEnd in the adjusted
comparison to avoid premature logouts across nodes.
---
Duplicate comments:
In `@apps/webapp/app/services/session.server.ts`:
- Around line 76-80: The fast-path in authenticator.isAuthenticated() that
returns authUser?.userId skips the session expiry check and thus bypasses
nextSessionEnd for cookie-only callers (getUserId()/requireUserId()); change the
fast-path to fetch the session expiry (nextSessionEnd) and validate it before
returning the userId—i.e., after calling authenticator.isAuthenticated(request)
inspect the session's nextSessionEnd (or call the same helper getSession/getUser
logic used by getUser) and if the session is expired treat it as unauthenticated
(return null / force requireUserId to re-authenticate) so cookie-only routes
honor auto-logout.
🪄 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: 4e164130-814f-4296-9541-a2542dbf3979
📒 Files selected for processing (1)
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). (28)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 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 (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- 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 (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / e2e-webapp / 🧪 E2E Tests: Webapp
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: sdk-compat / Cloudflare Workers
- GitHub Check: sdk-compat / Deno Runtime
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: sdk-compat / Bun Runtime
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: sdk-compat / Node.js 22.12 (ubuntu-latest)
- GitHub Check: sdk-compat / Node.js 20.20 (ubuntu-latest)
- GitHub Check: typecheck / typecheck
🧰 Additional context used
📓 Path-based instructions (10)
**/*.{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.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.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use function declarations instead of default exports
Files:
apps/webapp/app/services/session.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.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.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.ts
{apps,internal-packages}/**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
pnpm run typecheckto verify changes in apps and internal packages (apps/*,internal-packages/*) instead ofbuild, which proves almost nothing about correctness
Files:
apps/webapp/app/services/session.server.ts
{package.json,**/*.{ts,tsx,js}}
📄 CodeRabbit inference engine (CLAUDE.md)
Pin Zod to version 3.25.76 exactly across the entire monorepo - never use a different version or version range
Files:
apps/webapp/app/services/session.server.ts
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js}: Import from@trigger.dev/coreusing subpaths only, never the root export
Always import tasks from@trigger.dev/sdk, never from@trigger.dev/sdk/v3or deprecatedclient.defineJob
Add crumbs to code using//@Crumbscomments or `// `#region` `@crumbsblocks for debug tracing during development
Files:
apps/webapp/app/services/session.server.ts
**/*.{ts,tsx,js,jsx,json,md,css,scss}
📄 CodeRabbit inference engine (AGENTS.md)
Code formatting is enforced using Prettier. Run
pnpm run formatbefore committing
Files:
apps/webapp/app/services/session.server.ts
🧠 Learnings (3)
📚 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.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.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.ts
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
root.tsx's loader runs for every route — including /logout itself. When maybeAutoLogout fired on a /logout request it would redirect back to /logout, looping before the route's own loader could destroy the session cookie. Same theoretical issue exists for the user-not-found case where getUser throws await logout(request). Add a stateless path guard: if pathname is /logout, skip the redirect and let the route's loader run. Chose this over clearing nextSessionEnd before redirect — clearing has a race window where a browser interrupt between the redirect and the actual /logout request would leave the user with a valid cookie and a null deadline, effectively un-expiring them past their cap. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
2 participants
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.