Repository navigation
feat(machine): 4-tab Machine page shell (Terminal/Code/Diff/Settings) - #1999
Merged
Merged
Conversation
Rebuild the Machine page's TerminalView in place as a 4-tab command center
(Terminal / Code / Diff / Settings) while preserving the `TerminalView` export
name and `{ pageId }` prop shape, so CenterPanel/TerminalKeepAliveHost need no
change. Each tab body is gated on the active tab, so it mounts lazily and
unmounts on switch — a page load no longer eagerly inits all four tabs' data
fetches/sockets. The admin gate + non-admin notice are preserved.
- Terminal tab: FULLY implemented — a page-scoped inner sidebar (plain
border-border chrome) rendering the shared MachineTree with session-terminal
leaves injected under every Machine/Project/Branch node, beside the reused
chrome-free TerminalWorkspace pane. Reaches parity with the old right-sidebar
Navigator (session list, add/remove project/branch/terminal dialogs, empty
states); clicking a session opens it via the shared terminal-workspace store.
- Code/Diff/Settings tabs: STUB placeholders with the final `{ machineId }`
signature + file location locked in for the parallel follow-up PRs.
- Retire the old right-sidebar Navigator tab: delete TerminalNavigatorTab.tsx
and its wiring in right-sidebar/index.tsx (now dead: useParams/useTabMeta/
PageType/useLayoutStore/useAuth), plus the now-unimported Navigator.tsx. Its
`deriveProjectFieldsFromRepo` helper is exported from MachineTree (where it
was already duplicated) and its test repointed there, preserving coverage.
- Colocated tests: shell lazy mount/unmount + admin gate; Terminal tab renders
tree + workspace and opens sessions through the store.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DN8bpNNEukHgfWVJjdPZ3D
Contributor
|
Warning Review limit reached
Next review available in: 45 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 (8)
📝 WalkthroughWalkthroughThe terminal page now contains an admin-gated four-tab command center. Terminal sessions and tree navigation move into the Terminal tab, the right-sidebar terminal tab is removed, and reusable removal handling plus related tests and documentation are added. ChangesTerminal command center
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Admin
participant TerminalView
participant TerminalTab
participant MachineTree
participant TerminalWorkspace
Admin->>TerminalView: open machine terminal page
TerminalView->>TerminalTab: render active tab with machineId
TerminalTab->>MachineTree: render scoped session tree
MachineTree->>TerminalWorkspace: open selected terminal session
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
The tabDouble factory in TerminalView.test.tsx returned an anonymous component, tripping react/display-name — an error-level rule that fails `next build`, so all three CI checks (Lint, Unit Tests, Static Security Analysis, each of which builds) went red. Name the returned component and set displayName. Full `next lint`, typecheck, and terminal test suite green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DN8bpNNEukHgfWVJjdPZ3D
Strip animation-only props (initial/animate/exit/transition) in the motion/react test double so they don't leak onto the DOM node and emit React unknown-prop warnings. No behavior change; 4 shell tests still green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DN8bpNNEukHgfWVJjdPZ3D
Owner
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Follow-through cleanup from retiring the right-sidebar Terminal/Navigator tab: - Drop the now-dead 'terminal' member from the SidebarTab union (the sidebar Terminal tab was its only consumer) + its reference in the store test. - Refresh five stale "Navigator (right sidebar)" doc-comments to point at the new Machine Terminal-tab tree sidebar (useTerminalWorkspaceStore, workspace-reducer, XtermTerminal, TerminalPanes, useGithubRepos). - Strengthen a weak TerminalTab test assertion (queryByText, not !!getByText). typecheck + full lint clean; 118 tests across terminal/right-sidebar/page-agents suites green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DN8bpNNEukHgfWVJjdPZ3D
Four verified findings from a high-effort workflow review of the Machine
Terminal tab:
1. a11y (correctness): the per-session/project/branch remove (X) buttons were
`invisible group-hover:visible`, dropping them from the tab order so
keyboard users could never reach the delete flow. Switch to
`opacity-0 … focus-visible:opacity-100 group-hover:opacity-100` (+ aria-label)
so they stay focusable and reveal on keyboard focus — same pattern as
TerminalPanes' chrome-free controls. Fixed in both TerminalTab (session
leaves) and MachineTree's TreeRow (project/branch), which this PR is the
first to render live.
2. test coverage: TerminalTab tests only exercised machine scope. Add
project- and branch-scoped session tests that assert scopeFor threads the
correct { projectName, branchName } into openTerminal — the whole point of
the refactor, previously unverified.
3. dedup: extract the verbatim-duplicated ConfirmRemoveDialog into a shared
workspace/ConfirmRemoveDialog.tsx, imported by both MachineTree and
TerminalTab (was ~48 lines copied in each).
4. simplify: drop the redundant `{activeTab === 'x' && …}` guards in
TerminalView — Radix TabsContent (no forceMount) already renders only the
active tab and unmounts on switch. Go uncontrolled (defaultValue), remove
the now-unused activeTab state, and correct the comment to attribute lazy
mounting to TabsContent.
typecheck + full lint clean; 120 tests green (TerminalView lazy mount/unmount,
TerminalTab machine/project/branch scope threading, MachineTree, page-agents).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DN8bpNNEukHgfWVJjdPZ3D
…ult) Re-review of the extracted ConfirmRemoveDialog surfaced a latent correctness bug (carried over verbatim from the old Navigator/MachineTree copies): Radix AlertDialogAction auto-closes the dialog on click, so the async onConfirm's error path fired its toast against an already-dismissed dialog — the documented "stay open to retry on error" and in-dialog "Removing…" states never worked. Call e.preventDefault() in the action's onClick so the dialog stays open while the removal runs: handleConfirm closes it on success, or leaves it open with the error toast to retry. Add a colocated regression test covering both the success (closes) and failure (stays open + toasts) paths. typecheck + lint clean; 34 terminal-dir tests green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DN8bpNNEukHgfWVJjdPZ3D
Third-round review findings:
1. UX regression (correctness): the Terminal tab renders MachineTree without
onSelectNode, which left each node's label a disabled dead button — only the
~14px chevron expanded a row, unlike the old Navigator where clicking the
name expanded it. MachineTree's TreeRow now falls back to expand/collapse on
label click when no onSelect is given (onSelect still wins when provided, so
selection stays decoupled from expansion for callers that use it). Restores
the spec's Navigator parity. New MachineTree test covers the fallback.
2. dedup: extract the hover-reveal destructive RemoveButton (shared by
MachineTree's TreeRow and the Terminal tab's session leaves) into
workspace/RemoveButton.tsx — the identical class string + a11y reveal
behavior lived in two places and could drift.
3. dedup: collapse the byte-identical Code/Diff/Settings placeholder chrome
into a shared ComingSoonTab; the three entry files stay (locked-in
{ machineId } signature) so the parallel follow-up PRs still own them.
typecheck + lint clean; 35 terminal-dir tests green.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DN8bpNNEukHgfWVJjdPZ3D
This was referenced Jul 11, 2026
Merged
2witstudios
added a commit
that referenced
this pull request
Jul 11, 2026
…co (#2004) * feat(machine): Code tab — branch-scoped file browser + read-only Monaco Fills in the placeholder CodeTab from the 4-tab Machine shell (#1999), composing the pieces earlier phases landed: - inner page-scoped sidebar (plain border-border chrome, mirroring TerminalTab's sidebar/main-pane split): a BARE MachineTree as the branch picker, then MachineFileTree over the picked branch's checkout - main pane: CodeFilePane reads the selected file via the files route (mode=read) and shows it in a read-only Monaco, language detected from the filename Nothing fetches until it has to: no file content is read until a file is actually clicked, and no checkout is touched until a branch is picked. Not-yet-cloned branches are an explicit empty state, not a raw fetch error. `machine_branches` tracks no clone state (its row carries only a `sandboxId`), so the checkout's existence is answered by the live filesystem: the files route already returns a typed `reason` — `not_found` (no row, or the repo dir isn't there) or `vanished` (row exists, Sprite is gone). BranchFiles probes the checkout root once per branch and turns both into distinct empty states with a re-check action, mounting the file tree only once the probe says ready. Tests: 13 colocated (CodeTab composition + probe states; CodeFilePane fetch/loading/error/truncated/refetch). apps/web typecheck + lint green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K3tLCaRft4iC2DH3hmk7PJ * fix(machine): don't render binary checkout files as mojibake A real checkout is full of binaries (images, fonts, archives), and the files route decodes whatever it reads as UTF-8 — so clicking `logo.png` in the tree would have streamed up to 2 MiB off the Sprite only to render replacement characters in Monaco. CodeFilePane now recognises binaries from the filename (`isBinaryFile`, already exported alongside `detectLanguageFromFilename`) and shows an explicit no-preview state WITHOUT fetching: there is nothing useful to display, so the read is pure waste. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K3tLCaRft4iC2DH3hmk7PJ * fix(machine): Code tab review pass — binary test file, wasted listings, leaked route phrasing Self-review with an adversarial reviewer on the diff turned up five real defects, one of them a blocker I had introduced. BLOCKER — CodeFilePane.test.tsx had two literal control bytes (NUL, 0x01) where escapes were intended, so git classified the file as BINARY ("Bin 5605 -> 7093 bytes"). The test I had just added was undiffable and unreviewable on GitHub. Bytes stripped; all five files verified clean. The payload was dead anyway — that test asserts the read never happens. Wasted root listings on every branch change. BranchFiles held its state on the component instance while CodeTab re-rendered it with the new branch, so for one frame it still said `ready` and mounted MachineFileTree against the NEW branch — which self-keys, mounts, and fires a whole root listing that is then thrown away when the probe effect resets to `loading`. A branch switch cost 3 listings, not 2. Both children are now keyed by identity (branch, and branch+path), making a switch a remount, not a re-render. Regression-tested: probes are now exactly ['main', 'dev']. Leaked internal phrasing. If a branch's Sprite was reclaimed while a file was open, the main pane rendered the route's own words — "Branch machine vanished" — in red, beside a Retry that could never succeed, while the sidebar correctly said "This branch checkout is gone." Both surfaces now share one vocabulary (checkout-states.ts): a checkout that isn't there is a state of the world, not an error string. A read's 404 is deliberately NOT folded in — the route overloads `not_found` for both "no checkout" and "no such file" — so it reads as "This file is no longer in the checkout." Binary detection was filename-only, and BINARY_EXTENSIONS was written for GitHub import: no .pyc, .node, .class, .o, no extensionless a.out, and 'libfoo.so.1'.split('.').pop() is '1'. Added git's own heuristic as a second guard — a NUL byte in the head of the content means binary — so an unlisted binary is caught by content instead of mojibaked into Monaco. Selection is now one atomic state object (branch + path move together) instead of two setStates racing a stale closure. Tests: 55 terminal-area tests pass (20 in this tab, +5). typecheck + lint green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K3tLCaRft4iC2DH3hmk7PJ * refactor(machine): dedupe the Code tab sidebar's notice rows The "checkout absent" and "checkout error" branches of BranchFiles rendered byte-identical chrome and a byte-identical ghost retry button, differing only in their words. Extracted SidebarNotice — they differ in what they say, not in what they are — and gave the error row a testid to match. No behavior change: 55 terminal-area tests still pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K3tLCaRft4iC2DH3hmk7PJ * fix(machine): round-2 review — real guards, honest tests, one contract for "gone" A second adversarial review pass found that two of the fixes I shipped last round were guarded by tests that could not fail. Both are now mutation-proven. TESTS THAT DIDN'T TEST ANYTHING - The "no wasted listing" test passed with CodeTab's key DELETED. The stubbed MachineFileTree issued no listing, so the test only counted BranchFiles' probes — while the wasted listing came from the real tree. The stub now models the one behaviour that matters (it self-keys, so a new branch = a new root listing). Mutation-checked: delete the key and it fails with listed=['main','dev','dev'] — the thrown-away listing, made visible. - Same trap in the pane: a stale-read test that passed with all four `cancelled` guards removed (RTL flushed the effect before the assertion). It now flushes inside act() and fails without them. - And the one-frame "previous file under the new file's name" render is invisible in the final DOM by construction — React flushes the effect before paint — so it is now caught by recording what Monaco was actually handed: the pair (python, contents-of-a.ts) must never occur. ONE CONTRACT FOR "GONE" The internal phrasing I claimed to have killed was still reachable one click later: with a checkout open, reclaim the Sprite and expand a folder, and MachineFileTree rendered the route's `error` verbatim — "Branch machine vanished". Fixed at the source instead of at each client: the route's `error` is now a sentence for a person, and `reason` stays the machine-readable fact that clients switch on. Same for a failed exec with empty stderr, which used to print the bare token "exec_failed" at the user. The route also overloaded `not_found` for two different facts — "this branch has no checkout" and "that one file is gone" — so removing a branch-terminal under an open file told the reader the FILE had vanished. They are now distinct reasons (`file_not_found`), and the pane says which actually happened. ALSO - The U+FFFD binary heuristic had no absolute floor, so "Café Résumé" saved as Latin-1 (3 bad bytes in 11 chars = 27%) was declared binary and hidden. Now needs a real count, not just a ratio. - Dropped the per-file key on CodeFilePane: it tore down and recreated Monaco on every single file click. The path now travels in the state instead, which is what the render guard checks. - The pane has a reload: the working tree is live, an agent can rewrite the open file, and re-clicking the same tree row is a no-op. apps/web: typecheck green, lint 0 errors, 77 tests pass (68 in this feature). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K3tLCaRft4iC2DH3hmk7PJ * fix(machine): finish the job on the route's third error arm Round 3 found that my "never render internal phrasing at a user" fix had landed on two of the route's three failure arms. The LIST arm still did `error: result.detail` — the raw stderr of our own `ls`. Expand a folder an agent terminal deleted a second earlier and the file tree printed, verbatim: ls: cannot access '/workspace/repo/src/generated': No such file or directory Internal absolute path and all. And the same arm still conflated "this branch has no checkout" with "that one folder is gone" under a single `not_found` — the exact conflation I split the READ arm to prevent, left standing next door. So the contract now says what it means, for all three arms: not_found — this branch has no checkout (the ROOT listing is missing) dir_not_found — the checkout is fine; that directory is gone file_not_found — the checkout is fine; that file is gone vanished — the Sprite was reclaimed exec_failed — the exec died; its stderr is in `detail`, NOT in `error` `error` is a sentence for a person. `detail` is for logs and developers. Our stderr names paths inside the Sprite, so it belongs in the second, never the first. Both new guards are mutation-proven: put stderr back in `error`, or collapse `dir_not_found` into `not_found`, and a test goes red. Also from round 3: - Removed a dead `key` on CodeFilePane. It could never fire — a branch switch clears `path` in the same update, so the pane unmounts regardless — and its comment described that unreachable case as if it were load-bearing. (The key on BranchFiles IS load-bearing; its mutant still dies.) - Corrected a comment that justified the probe's duplicate root listing by claiming MachineFileTree is also mounted by TerminalTab and the Diff tab. It isn't: grep says this tab is its only mount. The comment now states the real cost, and names the refactor that would remove it — deliberately not done in the same PR that changes this route's contract. - Folded MachineFileTree's private error-body reader onto the shared helper. apps/web: typecheck green, lint 0 errors, 121 tests pass, production build OK. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K3tLCaRft4iC2DH3hmk7PJ --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
Phase 3 foundation — the 4-tab Machine page shell
Serial gate for Phase 3 of the Machine page rebuild (Terminal — GA Release epic). Spec: PageSpace task
rtk18a76c5hiy3g2ylti9gfh.Rebuilds the Machine page's
TerminalViewin place from a bare single-terminal view into a 4-tab command center — Terminal / Code / Diff / Settings — while preserving theTerminalViewexport name and{ pageId }prop shape, soCenterPanel.tsx/TerminalKeepAliveHost.tsxneed no change. (No initial-tab query param was needed — the shell always defaults to Terminal — soCenterPanel.tsxwas confirmed untouched.)Lazy tabs
Each tab body is gated on the active tab (
{activeTab === value && <Tab .../>}), so it mounts lazily and unmounts on switch. Opening a Machine page no longer eagerly inits all four tabs' data fetches / sockets — only the visible tab initializes. The admin gate + non-admin notice are preserved.Terminal tab — FULLY implemented
tabs/TerminalTab.tsx: a page-scoped inner sidebar (plainborder-borderchrome, not the app's liquid-glass sidebars) rendering the sharedMachineTreewith session-terminal leaves injected under every Machine/Project/Branch node, beside the reused chrome-freeTerminalWorkspacepane. Reaches parity with the old right-sidebar Navigator: session list, add/remove project/branch/terminal dialogs, and empty states are all reachable here; clicking a session opens it in the pane via the shareduseTerminalWorkspaceStore.Stubbed tabs (intentional placeholders for parallel follow-ups)
Each renders a
<tab> tab — coming soonplaceholder with the final{ machineId }prop signature and file location locked in, so the sibling agents just fill the body:tabs/CodeTab.tsx— speckf9ap6oigt1xedi47wuym94atabs/DiffTab.tsx— speclmnkz43dpczplbz4d2xuinobtabs/SettingsTab.tsx— speco2yv9twc782guwfrr2pawcifNavigator retirement
Wired + verified the new Terminal tab first, then removed the old surface (no window where neither exposes session navigation):
right-sidebar/TerminalNavigatorTab.tsxand its registration/tab-icon inright-sidebar/index.tsx, plus the now-dead wiring it was the only consumer of (useParams,useTabMeta,useTabsStore,PageType,useLayoutStore,useAuth, the terminal auto-switch effect).Navigator.tsxwas safe to delete: afterTerminalNavigatorTab.tsxwas gone, a repo-widegit grepshowed nothing else imported it except its own colocated test. ItsderiveProjectFieldsFromRepohelper (already duplicated privately inMachineTree.tsx) is now exported fromMachineTree, and the helper test was repointed there — so coverage is preserved against the surviving code path. Updated one stale doc-comment reference (github repos route) accordingly.Verification
bun run typecheck(apps/web) — green.RightPanel+MachineTreesuites still pass (86 tests total in the touched dirs).🤖 Generated with Claude Code
Known behavior — intra-page tab switching
Because each tab body is strictly gated on the active tab (per spec: "gate each TabsContent body on the active tab"), switching within a Machine page from Terminal → another tab → Terminal unmounts and remounts the Terminal tab, resetting its pane-split layout to a single pane. This is intentional and bounded:
TerminalKeepAliveHostkeeps the wholeTerminalViewmounted with itsactiveTabstate, so the Terminal tab stays warm across page navigation.TerminalView(mount-once + CSS-hide instead of conditional render) with no API impact.Summary by CodeRabbit
New Features
Improvements
Tests