refactor(deployment): send deployment names to the api instead of localstorage - #3928
Conversation
…alstorage The create request and the rename now both carry the name to the api, so the wallet-scoped local record was being written alongside as a second copy of the same fact. Two writers of one name is exactly the drift this work set out to remove: the local copy is only ever consulted for deployments named before this change, and keeping it fed means a rename made on another device leaves a stale name behind in this browser. Drop both writes. The configure session's hook now owns the name and nothing else, and the rename dialog reports the api's answer without mirroring it, which also takes the storage guard the mirror needed with it. The surfaces that still resolve a name from the local record alone — the deployments list, home, alerts, billing usage and provider lease rows — therefore show no name for a deployment created or renamed from here on, until they read the api too. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Warning Review limit reached
On-demand reviews are free for the next 9 days. After that, they cost $0.25 per reviewed file. Or wait 59 minutes for your next included review. View limit detailsLimit details: You’ve used all 2 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Essentials Run ID: 📒 Files selected for processing (5)
Comment |
There was a problem hiding this comment.
Beyond the inline finding on the rename path, I checked the create path in useDeploymentFlow.ts (unchanged by this PR) for the same class of issue — it was already not writing name to local storage before this change, so the new guard test in useDeploymentFlow.spec.tsx is confirming existing behavior, not masking a second regression.
Extended reasoning...
The confirmed inline finding (stale local-storage name surviving a rename because the mirror write was removed without a corresponding invalidation/clear) is a real, plausible regression worth a human's attention, so I did not approve. I additionally traced the create-request path in useDeploymentFlow.ts, which this PR does not modify: cacheDeployedSdl only ever wrote owner/manifest, never name, so the newly added useDeploymentFlow.spec.tsx test is a regression guard against future reintroduction rather than evidence of an existing gap there. That check is narrow enough not to warrant restating in the inline findings, but is worth surfacing since it explains why only the rename path (not the create path) is implicated.
| onSuccess: function reportRenameSaved() { | ||
| queryClient.invalidateQueries({ queryKey: api.v1.getDeployment.getKey({ dseq: String(dseq) }) }); | ||
| enqueueSnackbar(<Snackbar title="Success!" iconVariant="success" />, { variant: "success", autoHideDuration: 1000 }); | ||
| onSaved(); |
There was a problem hiding this comment.
🔴 Removing the localStorage mirror write on rename means any deployment that already has a name cached in this browser (named before this change, or by an earlier session) keeps its OLD name there forever after a rename — worse than the PR's documented 'shows no name' case, since the 5 surfaces reading only local storage (deployment list, home, alerts, billing, provider leases via useLocalNotes.getDeploymentName) now display a stale, incorrect name instead of nothing. Fix: either have those 5 surfaces read the API-resolved name (per the plan's slice 3) before this write is removed, or invalidate/clear the local record on successful rename so it falls back to null rather than a wrong value.
Extended reasoning...
User has an old deployment whose local record already has name:"old-name" (written by pre-slice-2 code). They open DeploymentNameModal, PATCH succeeds renaming it to "new-name" on the API; onSuccess (reportRenameSaved, lines 76-80) now only invalidates the getDeployment query and calls onSaved — it never touches deploymentLocalStorage. The local record's name field stays "old-name". DeploymentList.tsx, HomeContainer.tsx, AlertsListContainer.tsx, useAccountBalanceOverview.ts and LeaseRow.tsx all call useLocalNotes().getDeploymentName(dseq), which reads deploymentLocalStorage directly (useLocalNotes.ts lines 25-31) with no API fallback, so they keep showing "old-name" indefinitely even though the API and the rename dialog itself (via useResolvedDeploymentName) now show "new-name". The PR's own claim 'nothing here deletes them' undersells this: the record isn't deleted, but it silently diverges from the truth after every rename, which is a worse UX than the missing-name case the PR explicitly accepts.
Verification: normal — acknowledged in diff, but the note's claim does not hold for this subset. On the base branch, DeploymentNameModal's onSuccess ran recordNameInThisBrowser(name) -> deploymentLocalStorage.update(address, dseq, { name }), so renaming a deployment that already had a local record kept that record in sync; the five surfaces reading useLocalNotes.getDeploymentName (useLocalNotes.ts:27-28,…
|
@baktun14 this one has not been finished. it consists from 5 PRs but only 2 were implemented: |
Why
The name a user types was being recorded twice: #3918 routed both writes at the console API — the create request and a
PATCHrename — but left the wallet-scopedlocalStoragewrite in place beside them. Two writers of one name is the drift this work set out to remove: the local copy is only ever consulted for deployments named before that change, and keeping it fed means a rename made on another device leaves a stale name sitting in this browser.Part of CON-954 — https://linear.app/ovrclk/issue/CON-954/show-the-apis-deployment-name-when-this-browser-has-none
Slice 2 of 3, stacked on
feat/deployment-show-api-name-browser. Covers AC4.What
The second copy is gone. Nothing name-shaped is written to this browser any more — by the configure flow or by the rename dialog.
POST /v1/deploymentsPATCH /v1/deployments/{dseq}{ owner, manifest, name }{ owner, manifest }The manifest cache is untouched — it is what lets an in-progress deployment resume after a reload.
The hook that owned the write
useDeploymentNamenow resolves a name and nothing else. TheuseEffectthat wrotedeploymentLocalStorage.update(settingsId, dseq, { name })when adseqfirst appeared is gone, and with it the hook'suseServicesdependency, two refs and the jotai read:The rename dialog loses its mirror write the same way, and with it the
try/catchthat mirror needed for a full or blocked store. Its success path is now the invalidation alone (DeploymentNameModal.spec.tsx):The matching guard on the create path sits next to the manifest cache, where a name write would plausibly reappear (
useDeploymentFlow.spec.tsx):What this costs until slice 3
useLocalNotes.getDeploymentName, which reads this browser's record alone. It now answersnullfor those deployments.#3918 said these writes would go after those surfaces read the API. The plan sequences it the other way: the write is removed here, and the remaining slice adds the batch names endpoint and moves all five onto it. Either order leaves one slice where the surfaces disagree — this one is the direction the plan approved, and the stack is meant to land together. Deployments named before this change are unaffected: their records still exist and nothing here deletes them.
What a person sees
The same walkthrough run against both versions of the code — real components and hooks in a DOM, a real
DeploymentStorageServiceover an in-memoryStorage, only the network boundary stubbed. Full reproducible version under Demo.The requests are identical in both runs. What changed is the line underneath, and the one after it is the cost.
Tests
Five specs that asserted the removed write are deleted — four in
useDeploymentName.spec.tsx("writes the name to the settings-scoped record when a dseq is first assigned", "does not write before a dseq exists", "does not write when the session resumed already carrying a dseq", "defers the write until settingsId is available instead of dropping it") and one inDeploymentNameModal.spec.tsx("still refreshes and closes when this browser cannot record the new name"). They assert behaviour that no longer exists; the two guards above replace them.Verification
apps/deploy-web, 144 changes:npm test— passednpm run lint -- --quiet— passednpx tsc --noEmit— 85 errors, all present at the merge base, none in changed filesDemo
Executable walkthrough — re-runnable with
uvx showboat verifyCON-954 slice 2 — a deployment name that leaves the browser
2026-09-11T20:21:12Z by Showboat 0.6.1