fix(development): proper machine-list states, test the mobile sheet close - #2049
Merged
Conversation
…lose DevelopmentSidebar's machine list had one generic ListNotice string for every resting state. Replace it with the Machine page's shared SidebarLoading/SidebarNotice vocabulary (tab-states.tsx) so loading, empty, and failed states read like every other compact sidebar list in the app — and give the failed-load state a Retry action wired to SWR's mutate, in both drive-scoped and global (grouped-by-drive) mode. A drive with zero visible machines is dropped from the API payload by design (listMachinesAcrossDrives), so there's no reachable "empty drive group" state to build for global mode — noted inline rather than added as dead UI. Audited the mobile story end to end: the sidebar already inherits Layout.tsx's generic sheet treatment (it renders through MemoizedSidebar, which Layout puts in a Sheet below the app's mobile breakpoint), the Machine page's tab bar already went icon-only below `sm` in #2006 (before the Development surface existed, so it was inherited for free), and hover-revealed tree controls are covered by #2009's global touch-reveal CSS. The one real gap was test coverage: the isSheetBreakpoint-driven sheet-close-on-navigate wiring had no test. Added coverage for it, plus the new loading/error/retry state branches, in both modes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Fzw6ieurR2FCLwbyT1wkRi
Contributor
📝 WalkthroughWalkthrough
ChangesDevelopment sidebar state handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant DevelopmentSidebar
participant MachineQuery
participant MachineList
participant SidebarNotice
DevelopmentSidebar->>MachineQuery: capture mutate as retry callback
DevelopmentSidebar->>MachineList: pass onRetry
MachineList->>SidebarNotice: render list state
SidebarNotice->>MachineQuery: invoke retry callback
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 343677d04a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| machines={machines} | ||
| isLoading={driveMachinesLoading} | ||
| error={driveMachinesError} | ||
| onRetry={retryDriveMachines} |
There was a problem hiding this comment.
In an initial-load error, this SWR mutate is passed through to SidebarNotice, whose button uses it as onClick={onAction}; React will call it with the click MouseEvent, and SWR interprets that first argument as replacement cache data rather than a no-arg revalidation. The Retry button can therefore corrupt the machines cache instead of refetching (the global retryAllMachines path below has the same issue); wrap the call, e.g. onRetry={() => void retryDriveMachines()}.
Useful? React with 👍 / 👎.
Codex review on #2049 caught a real bug: SidebarNotice's Retry button uses onAction directly as the button's onClick, so React calls it with the click's MouseEvent. SWR's mutate() interprets a first argument as replacement cache data, not "revalidate now" -- so clicking Retry would have handed a MouseEvent to mutate() and corrupted the machines cache instead of refetching it, in both drive-scoped and global mode. Wrap both call sites in a genuinely no-arg callback (mirroring how DiffTab's own SWR-backed Retry already does this: void mutate(...) inside a useCallback). Strengthened both retry tests to assert the mutate spy was called with zero arguments, not just "called once" -- verified the new assertion actually fails against the pre-fix code before re-applying the fix. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Fzw6ieurR2FCLwbyT1wkRi
…n flight Self-review (6-angle finder pass) surfaced a real UX gap introduced by the Retry button added in the previous commit: resolveListNotice checked `hasError && isEmpty` ahead of `isLoading`, so clicking Retry gave zero visible feedback. Traced SWR's actual source (swr@2.4.1): isLoading is set back to true on any revalidation where cached data is still undefined -- exactly what a failed fetch leaves behind -- while `error` stays at its stale pre-retry value until the new attempt settles. Both are true at once mid-retry, so checking error first meant the same "Failed to load machines" text rendered throughout the retry, indistinguishable from the click doing nothing. Reordered: loading now wins over a stale error. Verified this doesn't regress the background-poll-must-not-blank-a-good-list case (that path has non-empty machines, so neither branch fires regardless of order) or the cold-load case (error is never set yet). Added a rerender-based regression test that simulates the actual retry-in-flight transition (not just a static prop snapshot) and confirmed it fails against the pre-fix ordering before restoring the fix. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Fzw6ieurR2FCLwbyT1wkRi
….tsx
Self-review (4-angle simplify pass) flagged that the previous commit's
useCallback wrapper in DevelopmentSidebar.tsx fixed the "SWR mutate reads
the click MouseEvent as replacement cache data" bug at only one of what
turned out to be 7 call sites of SidebarNotice/PaneNotice's onAction
(DiffTab, FilesFilePane, SettingsTab, MachineFileTree, FilesTab x2, and now
DevelopmentSidebar). Every other call site already independently wrapped
its callback in a zero-arg closure to dodge the same bug -- three
hand-written copies of the same workaround, and nothing stops a future
caller from reintroducing it: TypeScript structurally accepts SWR's
mutate (or anything with an optional first parameter) wherever
onAction: () => void is declared, so the mistake compiles clean.
Fixed at the source instead: both SidebarNotice and PaneNotice now call
onClick={() => onAction()} rather than onClick={onAction}, so every
caller's zero-arg contract holds regardless of what onAction closes
over. Verified safe for all 6 pre-existing call sites (their onAction
callbacks were already effectively zero-arg) via the full consumer test
suite (108 tests, all green). Simplified DevelopmentSidebar.tsx's retry
wiring back down to passing SWR's mutate directly -- the local
useCallback workaround is no longer needed. Added tab-states.test.tsx,
direct coverage pinning the zero-arg guarantee at the component that
now owns it, and confirmed by reverting the fix that both the new
direct test and DevelopmentSidebar's retry tests fail without it.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Fzw6ieurR2FCLwbyT1wkRi
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
1 participant
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.
Summary
DevelopmentSidebar's single terseListNoticestring with the Machine page's sharedSidebarLoading/SidebarNoticevocabulary (tab-states.tsx), so the machine list's loading/empty/failed states read like every other compact sidebar list in the app — in both drive-scoped and global (grouped-by-drive) mode.mutate, instead of a dead end.listMachinesAcrossDrives), so there's no reachable "empty drive group" state within global mode — documented inline rather than built as unreachable UI.DevelopmentSidebarrenders throughMemoizedSidebar, whichLayout.tsxalready puts in aSheetbelow the app's mobile breakpoint — the same mechanism every other sidebar variant uses. No new wiring needed there.MachineView's tab bar went icon-only belowsmin feat(machine): Phase 4 — cross-tab consistency, narrow-viewport pass, dead-code removal #2006, which landed before the Development surface existed (feat(development): Development surface — top-level machine command center #2015) — so it was inherited for free, not something feat(machine): Phase 4 — cross-tab consistency, narrow-viewport pass, dead-code removal #2006 missed.globals.css's attribute-substring rules), which match anyopacity-0 ... group-hover:opacity-100button regardless of when it was written.isSheetBreakpoint-driven "close the sheet after navigating to a machine" wiring existed but had zero test coverage. Added it.Follow-up fixes (post-review)
SidebarNotice's Retry button wired straight toonClick, so React handed it the click'sMouseEvent— SWR'smutatetreats a first argument as replacement cache data, not "revalidate now", so clicking Retry would have corrupted the machines cache instead of refetching. Fixed by wrapping bothretryDriveMachines/retryAllMachinesin a no-arguseCallback, matchingDiffTab's existing SWR-backed Retry convention. Strengthened both retry tests to assertmutatewas called with zero arguments — confirmed the assertion fails against the pre-fix code before restoring the fix.hasError && isEmptyahead ofisLoading, so a Retry click gave zero visible feedback — traced SWR's source (isLoadinggoes back totrueon any revalidation where cacheddatais stillundefined, exactly what a failed fetch leaves behind, whileerrorstays stale until the new attempt settles). Reordered so loading wins over a stale error, and added arerender-based regression test that simulates the actual retry-in-flight transition — confirmed it fails against the pre-fix ordering before restoring the fix.apps/web/.../machine/tabs/tab-states.tsx— outside this PR's original file list, but the direct dependency this PR started consuming): the localuseCallbackworkaround from the Codex fix above turned out to be the 4th independent hand-written copy of the same "wrap onAction so SWR's mutate doesn't get the click MouseEvent" fix —DiffTab,FilesFilePane/SettingsTab'sreload, andFilesTabeach already had one, and nothing stopped a 5th, 6th, 7th caller from forgetting it (TypeScript accepts SWR'smutatewhereveronAction: () => voidis declared — the mistake compiles clean). FixedSidebarNotice/PaneNoticethemselves to always callonAction()with zero arguments, verified safe for all 6 pre-existing call sites (108 tests, all green), simplifiedDevelopmentSidebar.tsxback down to passingmutatedirectly, and addedtab-states.test.tsx— direct coverage pinning the guarantee at the component that now owns it.Test plan
bun run vitest run src/components/layout/left-sidebar/__tests__/DevelopmentSidebar.test.tsx— 23 tests pass (loading/empty/error+retry × 2 modes, retry-arg-shape, retry-in-flight-shows-loading, sheet-close-on-navigate at narrow viewport, sheet-untouched at desktop width)tab-states.test.tsx(new) — 2 tests pass, pinning the zero-arg guarantee directlytab-states.tsxconsumer (DiffTab, FilesTab, SettingsTab, FilesFilePane, MachineFileTree — 108 tests) — all pass, confirming the root-cause fix doesn't regress any of the 6 pre-existing call sitesbun run typecheck(web) — cleanbun run lint(web) — clean (only pre-existing, unrelated warnings)🤖 Generated with Claude Code
https://claude.ai/code/session_01Fzw6ieurR2FCLwbyT1wkRi
Summary by CodeRabbit