fix(api): Fix preview branch targeting in environment variable API routes #2697
Conversation
…utes This PR fixes the `x-trigger-branch` header support for targeting specific preview branches when managing environment variables. The header was documented but not actually being extracted or used in the environment variable API routes. Additionally, the query logic in `authenticatedEnvironmentForAuthentication` was fundamentally broken—it searched for environments with both `slug: "preview"` (parent environment property) AND a specific `branchName` (child environment property), which no environment could satisfy simultaneously. The fix extracts the branch name using `branchNameFromRequest()` and correctly queries for child branch environments using `type: "PREVIEW"` and the specific `branchName`. This ensures that environment variable operations (create, update, get, list) properly target individual preview branches instead of affecting all preview environments.
|
WalkthroughThis change implements branch-aware environment authentication across the API layer. The updates include: adding Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
apps/webapp/app/v3/gitBranch.ts (1)
32-43: Clarify behavior for invalidrefs/*inputs insanitizeBranchNameThe updated
sanitizeBranchNamecorrectly handles more ref prefixes andundefinedinput. Note that it also returnsnullfor any unrecognizedrefs/*value, which means callers that treat!sanitizedBranchas “no branch provided” will silently ignore such headers and fall back to slug-based behavior. If distinguishing “no branch” from “invalid branch header” would be useful (e.g., to return a 4xx instead of silently falling back), consider returning a separate sentinel or throwing in that case.apps/webapp/app/services/apiAuth.server.ts (2)
500-556: Preview branch lookup for personal access tokens: behavior & assumptionsThe new
sanitizedBranchflow forpersonalAccessTokenlooks sound and addresses the previous slug+branch mismatch:
- When
branchis missing or sanitization returnsnull, you fall back to the existing slug-based query (including the dev +orgMember.userIdfilter), preserving current behavior.- When
sanitizedBranchis truthy, you query by{ projectId, type: "PREVIEW", branchName: sanitizedBranch, archivedAt: null }and require aparentEnvironment, which correctly targets child preview environments instead of the parent “preview” slug.Two points to double‑check:
- Branch uniqueness assumption – Because the branch lookup ignores
slug/parent, it relies onbranchNamebeing unique per project across all preview environments. If that’s not guaranteed, it may be safer to also constrain onparentEnvironment.slug === slug(or another parent identifier).- Invalid
refs/*headers – For unknownrefs/*formatssanitizeBranchNamereturnsnull, so these headers are treated the same as “no branch” and will hit the slug-based environment instead of failing. Confirm this is intentional for envvar APIs; otherwise you might want to surface a 4xx error instead of silently ignoring a malformed branch header.Overall, the fix matches the PR intent of targeting individual preview branches instead of all preview environments.
579-627: Organization token branch lookup mirrors PAT logic; consider small refactorThe
organizationAccessTokenbranch handling mirrors the PAT case: sanitize, fall back to slug when falsy, and otherwise query{ projectId, type: "PREVIEW", branchName: sanitizedBranch, archivedAt: null }with the same parentEnvironment checks. That keeps behavior consistent across token types and fixes the same slug+branch issue for org tokens.Given the near‑identical logic in the two cases, you could extract a small helper (e.g.,
findPreviewEnvironmentByBranch(projectId, slug, sanitizedBranch)), which would reduce duplication and centralize any future tweaks to preview-branch lookup behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/webapp/app/routes/api.v1.projects.$projectRef.$env.ts(2 hunks)apps/webapp/app/routes/api.v1.projects.$projectRef.background-workers.$envSlug.$version.ts(2 hunks)apps/webapp/app/routes/api.v1.projects.$projectRef.envvars.$slug.$name.ts(3 hunks)apps/webapp/app/routes/api.v1.projects.$projectRef.envvars.$slug.ts(3 hunks)apps/webapp/app/services/apiAuth.server.ts(4 hunks)apps/webapp/app/v3/gitBranch.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-05T15:32:41.553Z
Learnt from: myftija
Repo: triggerdotdev/trigger.dev PR: 2464
File: apps/webapp/app/services/projectSettings.server.ts:210-0
Timestamp: 2025-09-05T15:32:41.553Z
Learning: In the ProjectSettingsService.updateGitSettings method in apps/webapp/app/services/projectSettings.server.ts, when productionBranch or stagingBranch parameters are undefined, the method intentionally writes empty objects to clear existing branch configurations. This is the desired behavior, not a bug.
Applied to files:
apps/webapp/app/v3/gitBranch.tsapps/webapp/app/services/apiAuth.server.ts
📚 Learning: 2025-09-03T14:35:52.384Z
Learnt from: myftija
Repo: triggerdotdev/trigger.dev PR: 2464
File: apps/webapp/app/utils/pathBuilder.ts:144-146
Timestamp: 2025-09-03T14:35:52.384Z
Learning: In the trigger.dev codebase, organization slugs are safe for URL query parameters and don't require URL encoding, as confirmed by the maintainer in apps/webapp/app/utils/pathBuilder.ts.
Applied to files:
apps/webapp/app/services/apiAuth.server.ts
🧬 Code graph analysis (5)
apps/webapp/app/routes/api.v1.projects.$projectRef.envvars.$slug.ts (1)
apps/webapp/app/services/apiAuth.server.ts (1)
branchNameFromRequest(283-285)
apps/webapp/app/routes/api.v1.projects.$projectRef.envvars.$slug.$name.ts (1)
apps/webapp/app/services/apiAuth.server.ts (1)
branchNameFromRequest(283-285)
apps/webapp/app/routes/api.v1.projects.$projectRef.background-workers.$envSlug.$version.ts (1)
apps/webapp/app/services/apiAuth.server.ts (1)
branchNameFromRequest(283-285)
apps/webapp/app/routes/api.v1.projects.$projectRef.$env.ts (2)
apps/webapp/app/env.server.ts (1)
env(1246-1246)apps/webapp/app/services/apiAuth.server.ts (1)
branchNameFromRequest(283-285)
apps/webapp/app/services/apiAuth.server.ts (1)
apps/webapp/app/v3/gitBranch.ts (1)
sanitizeBranchName(32-44)
⏰ 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). (23)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 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 (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- 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: Analyze (javascript-typescript)
🔇 Additional comments (4)
apps/webapp/app/routes/api.v1.projects.$projectRef.envvars.$slug.ts (1)
4-8: Branch header correctly threaded into envvar list/create handlersImporting
branchNameFromRequestand passing it as the 4th argument toauthenticatedEnvironmentForAuthenticationin bothactionandloadercleanly enables branch-aware envvar operations while preserving existing behavior when the header is absent (branch isundefinedand the auth helper falls back to slug-based lookup). Looks consistent with the updated helper signature.Also applies to: 29-35, 77-82
apps/webapp/app/routes/api.v1.projects.$projectRef.$env.ts (1)
5-9: Consistent branch-aware environment lookup for project env endpointPassing
branchNameFromRequest(request)intoauthenticatedEnvironmentForAuthenticationaligns this route with the new branch-aware auth behavior and keeps the non-branch case unchanged. Wiring looks correct and consistent with other routes.Also applies to: 33-38
apps/webapp/app/routes/api.v1.projects.$projectRef.background-workers.$envSlug.$version.ts (1)
4-8: Background worker loader now participates in branch-aware env resolutionUsing
branchNameFromRequest(request)as the 4th parameter toauthenticatedEnvironmentForAuthenticationaligns this route with the new preview-branch behavior so background workers can be fetched per branch. The change is minimal and consistent with the other updated routes.Also applies to: 30-35
apps/webapp/app/routes/api.v1.projects.$projectRef.envvars.$slug.$name.ts (1)
5-9: Per-variable envvar routes correctly wired for preview branch targetingImporting
branchNameFromRequestand passing it intoauthenticatedEnvironmentForAuthenticationin bothactionandloaderensures that create/update/delete and read of a specific env var are all scoped to the correct preview branch whenx-trigger-branchis present, while preserving existing slug-only behavior when it isn’t. This matches the changes in the list/create envvar route.Also applies to: 31-36, 108-113
This PR fixes the
x-trigger-branchheader support for targeting specific preview branches when managing environment variables. The header was documented but not actually being extracted or used in the environment variable API routes. Additionally, the query logic inauthenticatedEnvironmentForAuthenticationwas fundamentally broken—it searched for environments with bothslug: "preview"(parent environment property) AND a specificbranchName(child environment property), which no environment could satisfy simultaneously. The fix extracts the branch name usingbranchNameFromRequest()and correctly queries for child branch environments usingtype: "PREVIEW"and the specificbranchName. This ensures that environment variable operations (create, update, get, list) properly target individual preview branches instead of affecting all preview environments.