Conversation
The configure screen could only resolve a templateId against the public gallery, so a user's own saved template had no way in beyond an in-memory atom handoff. Add a userTemplateId entry backed by the existing private user-template endpoint, which already answers with an empty body when the template isn't the caller's to see - surfaced as the same fallback as a failed fetch. The two template sources are mutually exclusive and resolved at the parse boundary, and buildConfigureUrl forwards the id so the shallow URL rewrites during a configure session don't drop it.
The template detail page reached the deployment editor by writing the SDL into an in-memory atom and pushing the classic URL, so a middle-click or a new tab arrived with nothing to configure. Both Deploy and Edit now carry the template in the URL itself. Launch Container-VM and Run Custom Container get a handler each rather than one parameterized by a page name, which was the last producer of the deploy-linux page param.
Both pages ran on the pre-redesign builder machinery that the configure page replaces; deploy-linux was already a client-side redirect shim whose getServerSideProps still fetched a template it never used. Edge redirects replace them, so the old URLs keep working for anyone holding a bookmark - a saved-template link carries its id across to the new userTemplateId param. The builder's own modals go with the page: import and export already exist on configure, and preview is superseded by the sdl pane. Template saving is the one capability without an equivalent yet - useSaveUserTemplate and its two analytics events are kept for the configure-side save that follows in CON-673. SimpleServiceFormControl stays: the classic new-deployment flow still uses it for repository and redeploy intents. The e2e specs went with the page rather than being ported - the configure flow spec already covers deploying from the form, and the other asserted builder component-set behavior that has no configure equivalent.
The sdlBuilder and plainLinux url builders have no callers left, and the page param they fed - along with the deploy-linux branch in useNewDeploymentUrl - was the only reason newDeployment could resolve to anything but /new-deployment. The deploy-linux clause in the return-to classifier goes too. A login returnTo still pointing there now classifies as null rather than a deployment, which only affects links already in flight.
|
Warning Review limit reached
On-demand reviews are free for the next 8 days. After that, they cost $0.25 per reviewed file. Or wait 43 minutes for your next included review. View limit detailsLimit details: You’ve used all 2 included reviews currently available. Your 51 included PR review attempts over the past 7 days set your current allowance at 2 reviews per hour. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Essentials Run ID: 📒 Files selected for processing (35)
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3935 +/- ##
==========================================
+ Coverage 82.35% 82.41% +0.05%
==========================================
Files 1280 1172 -108
Lines 35404 32513 -2891
Branches 8566 7977 -589
==========================================
- Hits 29158 26796 -2362
+ Misses 5515 5035 -480
+ Partials 731 682 -49
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it retires two routes in favor of edge redirects, merges a new user-template resolution path into the shared ConfigureDeployment flow, and touches URL-building/analytics/persistence code used across many entry points, a human look would still be worthwhile.
What was reviewed: the new next.config.js redirects (including the has-query-based /sdl-builder?id= case and the empty-id fallback), the mutually-exclusive templateId/userTemplateId/vm parsing in deploymentIntent.ts, the merged template-fetch/error/loading logic in ConfigureDeployment.tsx (including the "200 with empty body" non-owner case surfaced via !userTemplateQuery.data?.sdl), and confirmed no dangling references to the deleted files/URL helpers (SimpleSdlBuilderForm, SaveTemplateModal, ImportSdlModal, PreviewSdl, RedirectDeployLinuxToConfigure, useFormPersist, UrlService.sdlBuilder/plainLinux) remain anywhere in apps/deploy-web/src or tests.
Extended reasoning...
Overview
This PR retires the classic /sdl-builder and /deploy-linux pages in apps/deploy-web, replacing them with next.config.js 308 redirects into the unified /new-deployment/configure flow. It introduces a userTemplateId search param, threaded through deploymentIntent.ts, urlUtils.ts, useNewDeploymentUrl.ts, UserTemplate.tsx, TemplateList.tsx, and useTemplateQuery.tsx, so a user's own saved template can be opened directly on Configure via GET /v1/user/template/{id}. It deletes several now-unreferenced builder-only components (SimpleSdlBuilderForm, SaveTemplateModal, ImportSdlModal, PreviewSdl, RedirectDeployLinuxToConfigure, useFormPersist), the sdlBuilderSdl atom, and a Playwright E2E suite (build-template.spec.ts), while updating the corresponding unit specs.
Security risks
No new security-sensitive surface is introduced. The userTemplateId resolution relies entirely on an existing, presumably-authorization-checked backend endpoint (GET /v1/user/template/{id}) that already refuses non-owned templates; the frontend simply treats a resulting empty body the same as a failed request (a generic "couldn't load" fallback), which does not leak any data. The redirects are static, server-side next.config.js entries with no user-controlled destination beyond a validated capture group.
Level of scrutiny
This is a large (35-file), cross-cutting refactor touching route removal, URL-building, analytics, persisted state, and a merged template-resolution code path used by multiple entry points (gallery templates, user templates, hardcoded templates, VM seeding, draft resumption). Even though the diff is mechanical in nature and well covered by updated unit specs, the number of interacting conditions in ConfigureDeployment.tsx (draft vs. hardcoded vs. gallery vs. user-template vs. VM) and the redirect ordering/precedence in next.config.js are exactly the kind of logic where a subtle regression is easy to introduce and easy for a human familiar with the product flows to catch quickly.
Other factors
Test coverage looks strong and consistent with repo conventions (setup-function pattern used throughout the touched specs, no stray beforeEach). I did not find dangling imports/references to any of the deleted files or removed UrlService helpers. The PR description also documents several deliberate, easy-to-second-guess tradeoffs (analytics events going dormant, useReturnTo classification change, template-saving deferred to a follow-up ticket) that a human reviewer with product context is better positioned to sign off on than an automated pass.
1 participant
Why
Closes CON-672
The SDL Builder predates the redesign and the Configure page replaces it. Two earlier pieces already landed — CON-681 (#3444) took the sidebar entry out and healed the pre-
uactpricing crash, and CON-675 (#3443) brought Container-VM deployments to Configure — so what was left was retiring the routes themselves and re-pointing everything that still led to them./deploy-linuxwas already a client-side redirect shim, and itsgetServerSidePropsstill fetched a template it never used.What
Edge redirects replace both pages.
/sdl-builderand/deploy-linuxbecomenext.config.jsentries, so an old bookmark gets a real 308 rather than a hydration-time bounce off a loading shell. A saved-template link carries its id across:/sdl-builder/new-deployment/configure/sdl-builder?id=<uuid>/new-deployment/configure?userTemplateId=<uuid>/sdl-builder?id=(empty)/new-deployment/configure/deploy-linux/new-deployment/configure?vm=trueVerified against a production build — each returns 308 with exactly those destinations.
Configure can now open a user's own template. It could only resolve a
templateIdagainst the public gallery, so a saved template reached it by stuffing the SDL into an in-memory atom and hopping through the classic URL — which meant a middle-click or a new tab arrived with nothing to configure. The new?userTemplateId=is backed by the existingGET /v1/user/template/{id}, which already refuses a template that isn't the caller's. That endpoint answers a non-owner with a 200 and an empty body rather than an error, so both it and a failed request land on the same "couldn't load the template" fallback. The two template sources are mutually exclusive and resolved at the parse boundary, andbuildConfigureUrlforwards the id so the shallow URL rewrites during a configure session don't silently drop it.Template saving is deferred to CON-673, not dropped. It's the one builder capability without a Configure equivalent yet, so nothing on its path was destroyed:
useSaveUserTemplateand the whole user-template API are untouched (the hook is spec-only until CON-673 — deliberate, not dead code).create_sdl_template/update_sdl_templatestay in the analytics union, marked dormant, so the tracking plan isn't churned twice.SaveTemplateModal.tsxis deleted in eca7697 and cited on CON-673 as the reference for the behavior to rebuild — the title/visibility validation, the public-env warning, the sign-in gate, and the create-vs-Save-As ownership rule. It needs aDialogV2rewrite against Configure's form state regardless, and keeping it unimported would have blocked removingsdlStore.sdlBuilderSdlanduseFormPersist.click_edit_sdl_templateevent. It loads read-only for now and is accurate again once CON-673 lands — better than renaming the same button twice.Worth a reviewer's attention
SimpleServiceFormControlis intentionally kept. It looks like a builder file and sits incomponents/sdl/, but the classic/new-deploymentflow still uses it for repository and redeploy intents.useReturnTobehavior change. A loginreturnTostill pointing at/deploy-linuxnow classifies asnullinstead of a deployment. Only affects links already in flight.preview_sdl,import_sdlandreset_sdllost their only emitter with the builder. Unlike saving, the first two already have live equivalents on Configure (the SDL pane, andconfigure_sdl_imported).create_sdl_template_linkandclick_edit_sdl_templatesurvive but now fire against Configure, so saved Amplitude charts may want updating.permanent: truemeans a 308, which browsers cache hard — worth a fresh incognito window if you ever test a revert locally.configure-deployment-flow.spec.tsalready covers deploying from the form end to end, andbuild-template.spec.tsasserted builder component-set behavior with no Configure equivalent.configure-deployment-container-vm.spec.tsstill covers the/deploy-linuxentry.Verification
Full
deploy-webunit suite green (381 files / 3961 tests), lint clean,tsc --noEmitshows only the pre-existing spec baseline, andnext buildsucceeds with both routes gone from the route table. Redirects probed against the built server as above.