fix(webapp): project integrations page — Staging gating, unreachable code, and follow-ups - #4784
Conversation
WalkthroughThe change gates GitHub staging branch settings based on a top-level Merge Risk: 🟡 Moderate · up to The PR improves staging/preview gating, onboarding failure handling, and environment matching, but the current head can still retain, display, or submit Vercel settings for environments that are unavailable after project changes or deletion. Merge should wait for these bounded stale-configuration cases to be addressed or explicitly accepted. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description is detailed and on-topic. It explains the changes, testing performed, changelog impact, and linked issue references. It does not reproduce the checklist, explicit Changelog heading, or screenshots section, but these omissions are non-critical because the required implementation and verification details are complete.
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
I think you have tagged the wrong James Ritchie here! |
Hi James! Yes, sorry :) |
|
8bd3d7a to
fc0790d
Compare
fc0790d to
69453b4
Compare
The Vercel settings panel carried two notification panels that could never render: - The "Failed to load Vercel settings" panel was gated on a `hasError` state whose setter was never called anywhere, so it was permanently false. - The "connection expired" banner inside the `connectedProject` branch was unreachable: VercelSettingsPresenter only populates `connectedProject` on its success exit, which hardcodes `authInvalid: false`. Both `authInvalid: true` exits return `connectedProject: undefined`. The banner users actually see is the one below that branch, which is untouched. Dropping them makes the surrounding guards vacuous, so `!showAuthInvalid` and the `onboardingData?.authInvalid` disjunct go too - the loader already folds onboarding auth state into `authInvalid` before it reaches the component. No behaviour change. TRI-12645
…ment On the integrations page the Preview row correctly swaps its switch for an Upgrade button when the project has no preview environment, and the server neutralises a forged `previewDeploymentsEnabled=on`. The Staging row had neither: it was always an editable branch input, and `validateStagingBranch` only checked the branch existed on GitHub. An org without a staging environment could type a tracking branch, hit Save, get a success toast, and have it do nothing. Staging and Preview environments are created together for projects on a plan that includes them, so gating one and not the other was an oversight, not policy. The Staging row now mirrors the Preview row, and the server ignores the submitted branch when there is no staging environment. It keeps the stored branch rather than clearing it, so losing the environment never destroys a tracking branch the org had already configured. The Vercel config actions had the same gap on the write path: nothing re-derived the available env slugs server-side, so "stg" and "preview" could be persisted for a project with neither environment, and the default config turned preview on unconditionally. Both now filter against the project's actual environments. TRI-12646
The integrations page wrapped the Git section, the Vercel section and the build settings in a single `githubAppEnabled` guard, so with the GitHub app off the page rendered an empty container. The Vercel section genuinely depends on GitHub - it cannot sync environment variables or link deployments without a connected repo - so it stays inside the guard. Build settings do not: they also apply to CLI deploys run with --native-build-server, exactly as the section describes. They now render regardless. TRI-13488
`computeInitialState` starts in "loading-projects" whenever the org has a Vercel integration but no onboarding data yet. The effect that escapes that state waits for `availableProjects !== undefined`, so when `getOnboardingData` returns null - it does that on any thrown error, and when the org integration row is missing - nothing ever arrives and the modal spins indefinitely with no explanation. The route knows the difference between "still loading" and "loaded nothing", since its fetcher always requests the onboarding data. It now passes that down, and the modal shows what went wrong plus a way to retry or check the integration's access on Vercel. TRI-13488
The four places that ask "does this project have a staging / preview environment?" disagreed. VercelSettingsPresenter matched on type with no filter on the parent, so any preview *branch* row satisfied it - branches are PREVIEW rows too. GitHubSettingsPresenter and ProjectSettingsService matched on slug instead. Slug is the weaker key: it is derived at creation time and legacy rows can carry something else, which is why memberDevelopmentEnvironmentWhere deliberately avoids it. All four now match on type plus parentEnvironmentId: null, which excludes branches and does not depend on the slug being canonical. TRI-13488
…on UI Follows the two unreachable panels removed in the parent branch. None of this is reachable either: - The `"installing"` OnboardingState is unproducible - no setState call ever yields it - so its redirect effect, switch arm, isLoadingState conjunct and the vercelAppInstallPath import it was the only user of are all dead. - `(state as string) !== "completed"` is inside a branch where TypeScript has already narrowed "completed" out; the cast is what let it compile. - `hideSectionToggles` was only ever passed alongside layout="settings" but only read inside layout="card" blocks, so it could never take effect. - A handful of unused bindings and the helpers only they referenced: envSlugLabel, _formatSelectedEnvs, _CompleteOnboardingForm, _handleFinishOnboarding and friends. No behaviour change. TRI-13488
The Staging build settings show "Set a Vercel environment for Staging first." whenever the project has a staging environment and no mapping, but the control that sets the mapping only rendered when the Vercel project had at least one custom environment. A project with none - or one whose custom environments could not be fetched - got an instruction with nothing to act on. The mapping row now always renders alongside that hint, and says what to do when there is nothing to choose from. The build settings hint matches. Also gates the build settings Save on write:github, which the action already requires. The page admits write:vercel too, so without this a Vercel-only role could fill the form in and only discover the denial on save. TRI-13488
69453b4 to
f2a69a7
Compare
@trigger.dev/build
trigger.dev
@trigger.dev/core
@trigger.dev/python
@trigger.dev/react-hooks
@trigger.dev/redis-worker
@trigger.dev/rsc
@trigger.dev/schema-to-json
@trigger.dev/sdk
commit: |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.settings.integrations/route.tsx (1)
62-64: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd crumb instrumentation for the new integration states.
Add
//@Crumbsmarkers or `#region `@crumbsblocks for the new permission gate, unavailable onboarding state, and disabled save state. Keep the instrumentation on the branch untilagentcrumbs stripruns.As per coding guidelines, “Add crumbs as you write code.”
Also applies to: 228-229, 385-402, 417-417, 589-594
Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f7232e3b-4c78-4b4b-aa34-c8587a17f45d
📒 Files selected for processing (1)
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.settings.integrations/route.tsx
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review.
📜 Review details
⏰ Context from checks skipped due to timeout. (34)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (7, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (17, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (14, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (12, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (19, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (23, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (16, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (18, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (15, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (11, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (10, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (21, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (9, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (13, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (22, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (20, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (8, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (5, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (24, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (1, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (2, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (3, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (6, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (4, 24)
- GitHub Check: obsmap / 🧪 Unit Tests: Observability Map
- GitHub Check: e2e-webapp / 🧪 E2E Tests: Webapp (2, 2)
- GitHub Check: typecheck / typecheck
- GitHub Check: runops-guard / runops-guard
- GitHub Check: e2e-webapp / 🧪 E2E Tests: Webapp (1, 2)
- GitHub Check: fk-cascade-guard / fk-cascade-guard
- GitHub Check: report
- GitHub Check: code-quality / code-quality
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Build and publish previews
🧰 Additional context used
📓 Path-based instructions (7)
For dashboard changes, visually verify the running Remix app with Chrome DevTools MCP, using snapshots, screenshots, interaction, and console-message checks as appropriate.
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
Files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.settings.integrations/route.tsx
**Prefer static imports over dynamic imports.** Only use dynamic `import()` when:
📄 CodeRabbit inference engine (AGENTS.md)
Files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.settings.integrations/route.tsx
Add crumbs as you write code — not just when debugging. Mark lines with
📄 CodeRabbit inference engine (AGENTS.md)
Files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.settings.integrations/route.tsx
Use zod for validation in packages/core and apps/webapp
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.settings.integrations/route.tsx
Access environment variables through the `env` export of `env.server.ts` instead of directly accessing `process.env`
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.settings.integrations/route.tsx
Use function declarations instead of default exports
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.settings.integrations/route.tsx
Use types over interfaces for TypeScript
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.settings.integrations/route.tsx
Three bugs on the project integrations page, one commit each for the two reported ones and four for the follow-ups found while fixing them.
chore: remove unreachable code on the integrations page (TRI-12645)Two notification panels in
VercelSettingsPanelcould never render:hasErrorstate whose setter is never called anywhere, so it was permanentlyfalse.connectedProjectbranch was unreachable:VercelSettingsPresenteronly populatesconnectedProjecton its success exit, which hardcodesauthInvalid: false, while bothauthInvalid: trueexits returnconnectedProject: undefined.Removing them makes the surrounding
!showAuthInvalidguards vacuous, and theonboardingData?.authInvaliddisjunct redundant — the loader already folds onboarding auth state intoauthInvalidbefore it reaches the component.No behaviour change. An org with a connected project and an expired token still gets the banner, from the branch below (untouched).
fix: gate Staging settings on plans without a Staging environment (TRI-12646)The ticket's premise was inverted, and I've corrected it there. In Git settings, Preview is the row that's correctly gated; Staging is the one with no gate at all:
projectSettings.server.tsneutralises a forgedpreviewDeploymentsEnabled=on.Input, andvalidateStagingBranchonly checked the branch existed on GitHub. An org without a staging environment could type a tracking branch, hit Save, get a success toast, and have it silently do nothing.Staging and Preview environments are created together for projects on a plan that includes them, so gating one and not the other was an oversight.
The Staging row now mirrors the Preview row. Server-side it ignores the submitted branch when there's no staging environment, but preserves the stored branch rather than clearing it — deliberately different from the Preview handling. Forcing a boolean off is harmless; forcing a string off would wipe a tracking branch the org had already configured the first time they saved after losing the environment.
The Vercel write path had the same gap:
update-config/complete-onboarding/update-env-mappingnever re-derived available env slugs server-side, so["stg","preview"]could be persisted for a project with neither environment, andcreateDefaultVercelIntegrationDataturned preview on unconditionally. Both now filter against the project's actual environments, via a purerestrictConfigToAvailableEnvSlugshelper that only touches keys present on the input.fix: show build settings when the GitHub app is disabled (TRI-13488)The page wrapped Git settings, the Vercel section and build settings in one
githubAppEnabledguard, so with the GitHub app off it rendered an empty container.The Vercel section genuinely depends on GitHub — it can't sync environment variables or link deployments without a connected repo — so it stays gated. Build settings don't: they also apply to CLI deploys run with
--native-build-server, exactly as the section's own description states. They now render regardless.fix: stop the Vercel onboarding modal spinning forever (TRI-13488)computeInitialStatestarts inloading-projectswhenever the org has a Vercel integration but no onboarding data yet, and the effect that escapes it waits foravailableProjects !== undefined. WhengetOnboardingDatareturnsnull— it does that on any thrown error, and when the org integration row is missing — nothing ever arrives.The empty-array case self-resolves (
[] !== undefined), so this is specifically the null case. The route can tell "still loading" from "loaded nothing" because its fetcher always requests?vercelOnboarding=true; it now passes that down and the modal explains the failure with a retry and a link to check the integration's access on Vercel.fix: match staging and preview environments consistently (TRI-13488)The four places that ask "does this project have a staging / preview environment?" disagreed.
VercelSettingsPresentermatched on type with no parent filter, so any preview branch row satisfied it — branches arePREVIEWrows too.GitHubSettingsPresenterandProjectSettingsServicematched on slug instead.Slug is the weaker key: it's derived at creation time and legacy rows can carry something else, which is why
memberDevelopmentEnvironmentWheredeliberately avoids it. All four now match ontypeplusparentEnvironmentId: null, which excludes branches without depending on the slug being canonical.fix: explain when no Vercel environment can be mapped to Staging (TRI-13488)Reported while reviewing the branch. The Staging build settings show "Set a Vercel environment for Staging first." whenever the project has a staging environment and no mapping — but the control that sets the mapping only rendered when the Vercel project had at least one custom environment:
So a Vercel project with no custom environments, or one whose custom environments failed to fetch (the presenter swallows that error to
[]), got an instruction with nothing to act on. Both conditions predate this PR.The mapping row now always renders alongside the hint and explains what to do when there's nothing to choose from, and the build-settings hint says the same thing.
chore: remove the remaining dead code (TRI-13488)"installing"OnboardingStateis unproducible — nosetStatecall yields it — so its redirect effect, switch arm,isLoadingStateconjunct and thevercelAppInstallPathimport it was the only user of are all dead.(state as string) !== "completed"sits in a branch where TypeScript has already narrowed"completed"out; the cast is what let it compile.hideSectionToggleswas only ever passed alongsidelayout="settings"but only read insidelayout="card"blocks, so it could never take effect. Removed the prop entirely.envSlugLabel,_formatSelectedEnvs,_CompleteOnboardingForm,_handleFinishOnboarding, and the rest.No behaviour change in that commit.
Not included
The three overlapping modal-open effects in
settings.integrations/route.tsxare left alone — they're defensive against a close-then-reopen race, and untangling them is a behavioural risk with no user-visible payoff.Verification
pnpm run typecheck --filter webapp,pnpm run lintandpnpm run knipare clean. Newapps/webapp/test/vercelIntegrationConfig.test.tscovers the slug restriction and the default-config seeding (both pure functions); 39 tests pass across it and the three existing Vercel/project-settings files.The new
projectId+slugquery is served by the existing@@unique([projectId, slug, orgMemberId])prefix — same access pattern as the preview check it mirrors.refs TRI-12645, TRI-12646, TRI-13488