Repository navigation
feat(machine): Settings tab — name/description, access toggles, delete - #2002
Merged
Merged
Conversation
Replace the ComingSoonTab placeholder in the Machine page's Settings tab with a full-width form over the machine-settings route. Mirrors TerminalAccessCard's optimistic persist() pattern (local update → PATCH → rollback + toast on error); no React Hook Form, no SWR. - Name + description text fields (persist on blur) - visibleToGlobalAssistant / allowPageAgents access switches (persist on toggle) - Destructive Delete Machine gated behind AlertDialog; on success navigates back to the drive off the now-trashed page - Colocated tests: optimistic toggle + rollback-on-error, delete-confirm gate Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NwV8emSn4yaietjD9BCwb6
Contributor
|
Warning Review limit reached
Next review available in: 19 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✨ 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 |
…gered it Disabling the form controls while a PATCH was in flight silently dropped user input: clicking a Switch blurs a text field, whose blur-commit fires persist() -> setSaving(true) -> re-render -> the Switch is disabled before the click's mouseup lands, so onCheckedChange never fires. The toggle was lost with no feedback. Surface the in-flight save as a 'Saving…' indicator instead of enforcing it. Also stop reconciling local state from the PATCH response. The route echoes the whole MachineSettings, so applying it would clobber a field the user is concurrently editing with a value already stale when the request left. We normalize every field client-side exactly as the route does, so the optimistic value never diverges. Rollback now reverts only the patched keys, so a failed toggle can't revert an unrelated edit that landed mid-flight. This is also closer to TerminalAccessCard, which never reads the response. Adds regression tests for both (swallowed-toggle, name rollback). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NwV8emSn4yaietjD9BCwb6
Review pass on the Settings tab surfaced three real defects: 1. (major) A failed PATCH rolled back to a CLIENT-SIDE guess and never revalidated. A request that committed server-side and then timed out left the tab showing the old value forever — there is no SWR/socket here to revalidate it. persist() now reverts optimistically (no flicker) and then refetches, which is authoritative and also settles out-of-order saves. 2. A failed background resync tore the whole form down to the error screen even though it had perfectly good settings in hand. Dropped the loadError flag: `settings === null` after the first load IS the error state, and only the FIRST load blocks on a spinner (later refetches revalidate in place). 3. AlertDialogAction composes DialogClose, so the dialog unmounted on the same click that started the DELETE: the in-progress state could never render and a FAILED delete toasted against a dialog the user had to reopen. preventDefault holds it open until the request settles. Also: 'saving' is now a COUNT, not a flag — a blur-commit and a switch click can be in flight at once, and a boolean reported 'done' when the first settled. Tests: 6 -> 10. Adds the commit-then-timeout resync, the load-error retry path, the empty-name guard (must not fire a PATCH the route would 400), description clearing to null, and pins the GET query-param shape. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NwV8emSn4yaietjD9BCwb6
The 'only the first load blocks on a spinner' optimization was keyed to a bare boolean, so a machineId change skipped the spinner AND kept the previous machine's settings in state — a failed load for the new machine would render the old machine's name and toggles under the new machine's identity. Key the ref to the machineId that actually loaded, and drop stale settings on a switch. Test pins it: switch to a machine whose GET fails -> error state, not a stale form. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NwV8emSn4yaietjD9BCwb6
Second review pass caught two bugs the resync fix itself introduced. A text field's blur-commit and a switch click can be in flight together, and the failure path handled neither: 1. (major) The resync GET was fired eagerly from the failing save's catch, while another PATCH was still airborne. The GET reads pre-PATCH state and is then overwritten by the PATCH landing after it — stranding exactly the staleness the resync exists to fix. Now deferred until every save drains (pendingSaves hits 0), tracked via a ref because finally must read the CURRENT count. 2. (major) Failure reverted a whole snapshot, rolling back an unrelated edit that had succeeded while the failing save was in flight. Now reverts only the keys that save owned. Also: the resync refetch reseeded both text drafts unconditionally, deleting text mid-keystroke. It now adopts the server value only for drafts the user hasn't touched since we last saw the server (tracked via a serverSettings ref). Delete dialog is now controlled: closing it always clears 'deleting', so an Escape during a hung request can't strand a permanently disabled 'Deleting…' button. A failed delete keeps the dialog open so the user can retry. Tests 11 -> 14, and beforeEach now resetAllMocks (clearAllMocks left unconsumed mockResolvedValueOnce queues, order-coupling the suite). Both new concurrency tests are mutation-verified: reverting either fix makes them fail. Fixed one vacuous assertion — a bare waitFor on 'fetch count is still 1' passes on the first poll before an eager refetch fires, so it's gated on the error toast. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NwV8emSn4yaietjD9BCwb6
…Mode Third review pass found a bug the previous fix introduced, and it broke the tab in dev outright. Calling setLoading(true) from INSIDE a setSettings updater is an impure updater. Next 15 enables reactStrictMode by default, so React replays queued updaters at render time -- the replay ran after the load's finally had already cleared loading, stranding loading:true with nothing left to clear it. Result: the Settings tab sat on a spinner forever in dev. It only survived a single mount by luck (the eager-bailout path never replays the updater). serverSettings moves in exact lockstep with settings, so the ref answers "do we have anything to show for this machine?" with no state read at all. Also guards persist() against a save that settles AFTER the tab moved to another machine: without it, machine A's failed rename reverted A's old name into machine B's input -- where the next blur would have renamed B. Unreachable today (TerminalKeepAliveHost keys each TerminalView by id) but the component's own tests assert in-place switching, so contract and code now agree. Tests 14 -> 16, both mutation-verified. The machine-switch test deliberately does NOT use waitFor: when the guard works the correct outcome is that NOTHING happens, so a polling assertion passes on its first tick and asserts nothing -- it drains the rejection with act() and then asserts once. (Same vacuity trap caught earlier in the resync test.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NwV8emSn4yaietjD9BCwb6
Owner
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
resyncWhenIdle was never cleared on a machineId switch, so a resync owed to the machine we left could fire a spurious refetch once an unrelated save on the NEW machine drained. Harmless (an extra GET) but sloppy; clear it with the rest of the outgoing machine's state. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NwV8emSn4yaietjD9BCwb6
It sat in the General card's header but fires for the Assistant-access toggles in the card below it too, mislabelling which section was saving. Any control on the form can be the one in flight, so the indicator belongs to the form. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NwV8emSn4yaietjD9BCwb6
A dedicated pass over the test file found no vacuity or flakiness in the existing 16, but four pieces of real component logic were entirely unguarded -- breaking each left the suite green: - Escape during a hung DELETE must clear `deleting`. The whole reason the dialog is controlled; without it, reopening shows a dead disabled "Deleting..." button. - A resync must not clobber a draft the user is mid-edit in, while still adopting the server value for a field they haven't touched. This is the subtlest logic in the component and had zero coverage. - Blurring an unchanged field must send no PATCH. - A name is trimmed before it is persisted AND before it is displayed. This is the premise of "the optimistic value already equals the persisted one", which is what licenses not reconciling from the PATCH response -- so it deserves to be pinned. All four are mutation-verified: breaking each behaviour fails exactly its own test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NwV8emSn4yaietjD9BCwb6
…Settings SettingsTab had grown to 396 lines carrying 8 pieces of state and 4 refs, and none of that machinery is about rendering -- it is the load/save/resync state machine, where every subtlety lives (concurrent saves, per-key revert, deferred resync, dirty-draft preservation, stale-machine results). Tangled with the form JSX it reads as incidental complexity; on its own it reads as what it is. SettingsTab is now 240 lines: the form, plus the delete action. The hook is 216 and documents its own invariants. It lives in apps/web/src/hooks/ beside its useMachineProjects / useMachineBranches siblings -- deliberately NOT on SWR like they are, because the spec pins this surface to TerminalAccessCard's plain useState/optimistic-persist shape. (Revalidation is precisely what SWR would have given for free, and precisely why most of the hook exists; better spelled out than implicit.) Behaviour-preserving: the 20 tests are UNCHANGED and still pass, and the four mutation checks still kill their mutants against the extracted hook (StrictMode impure-updater hang, eager resync racing an in-flight PATCH, dirty-draft clobber, stale-machine write). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NwV8emSn4yaietjD9BCwb6
Two findings from a review of the freshly-extracted hook.
1. The stale-save guard keyed off the machine last LOADED, so between switching
machines and the new machine's GET resolving, a save from the machine we LEFT
still looked current: its failure toasted an error for a machine the user was
no longer on and wrote its old name into the new machine's draft. It self-healed
(the new load overwrites), but the window was real. The ref now means "the
machine the tab is showing" and is claimed the moment machineId changes;
"do we have data yet?" was always a separate question, answered by
serverSettings. Renamed loadedFor -> activeMachine to say so.
2. setAccess was typed with the full MachineSettingsPatch, so a caller could
setAccess({ name }) and silently skip commitName's trim + empty-name guard.
Narrowed to MachineAccessPatch (the two toggles), making the bypass
unrepresentable rather than merely discouraged.
New test covers the window in (1) and is mutation-verified: restoring the
claim-on-load behaviour fails it. 21 tests.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NwV8emSn4yaietjD9BCwb6
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Fills in the Machine page's Settings tab body (spec
tkvkd5e4ruegplpe9affljbv), replacing theComingSoonTabplaceholder the 4-tab shell (#1999) locked in. Keeps the exactexport default function SettingsTab({ machineId })signature;ComingSoonTab.tsxis untouched (Code/Diff still use it).A full-width form over the machine-settings route (
/api/machines/settings, from #1967), followingapps/web/src/app/settings/ai/TerminalAccessCard.tsx: plainuseState/useEffectfetch + an optimisticpersist()(local update → PATCH → rollback + toast on error). No React Hook Form, no SWR.null, matching the route's normalization.ui/switch.tsx, persist on toggle.ui/alert-dialog.tsx. On success navigates to/dashboard/{driveId}, off the now-trashed page.No inner sidebar — the shell already supplies the tab body.
Structure
hooks/useMachineSettings.ts— the load/save/resync state machine, and where all the subtlety lives.tabs/SettingsTab.tsx— the form and the delete action.The split is not cosmetic: none of the machinery below is about rendering, and tangled into the JSX it reads as incidental complexity. The hook sits beside its
useMachineProjects/useMachineBranchessiblings but deliberately isn't on SWR like they are — the spec pins this surface to theTerminalAccessCardpattern. Revalidation is exactly what SWR would have given for free, and exactly why most of the hook exists, so it's spelled out rather than left implicit.The interesting part: the failure path
Text fields are drafts committed on blur; toggles persist immediately — so two saves can be in flight at once, and every rule here is written for that. Each behaviour below is mutation-verified: breaking it fails exactly one test.
saving→ re-render → the Switch is disabled before the click's mouseup lands, soonCheckedChangenever fires. The toggle is silently lost. The save is surfaced ("Saving…"), never enforced.setStateinside a state updater. Readingsettingsvia asetSettingsupdater to decide whether to show the spinner is an impure updater. Next 15 enablesreactStrictModeby default, so React replays it at render time — after the load'sfinallyalready clearedloading. The tab hung on a permanent spinner inbun run dev. A ref that moves in lockstep with the state answers the same question with no state read.Delete is a controlled dialog: it stays open across the request (a failure is retryable, not toasted at a dismissed dialog), and closing it always clears
deleting, so an Escape during a hung request can't strand a permanently-disabled "Deleting…" button.Tests
21 colocated tests. Every non-obvious behaviour is mutation-verified — that check earned its keep twice:
waitForpolls until an assertion passes, so "the count is still N" / "X did not happen" succeeds on the first tick, before the behaviour under test could occur — asserting nothing. Both now gate on an observable signal, or drain withact()and assert once.deletingreset on dialog close, the unchanged-value short-circuit, name trimming, or the stale-machine guard all left the suite green. Each now has a test.Verification
bun run typecheck(apps/web) — greenbun run build --filter=web— greenThe surface stays behind
CODE_EXECUTION_ENABLED(OFF). The two access toggles are persisted but not yet enforced — enforcement is a separate follow-up node, per the service's own docs.🤖 Generated with Claude Code