feat: tabbed panes — multiple tabs per split pane (cmux-style) - #103
Conversation
| if (idx === -1) return root; | ||
| const newTabs = root.tabs.filter((t) => t.id !== tabId); | ||
| let newIndex = root.activeTabIndex; | ||
| if (newIndex >= newTabs.length) newIndex = Math.max(0, newTabs.length - 1); |
There was a problem hiding this comment.
🔴 Critical
Problem: removeTab does not decrement activeTabIndex when a tab before the active one is removed. With 3+ tabs, closing any tab whose index is less than activeTabIndex causes the active tab to silently shift to the next tab.
Why it matters: Concrete example — tabs [A, B, C], activeTabIndex = 2 (C is active). User closes A (index 0). newTabs = [B, C], newIndex = 2. The check 2 >= 2 clamps it to 1, pointing at C — correct here by accident. But with [A, B, C, D], activeTabIndex = 2 (C is active), closing A gives newTabs = [B, C, D], newIndex = 2, 2 >= 3 is false — index stays at 2 pointing to D instead of C. The user closed a background tab and their focused session visibly jumps. The existing test ("adjusts activeTabIndex when removing before active") only covers the 2-tab boundary case and misses this.
Suggested fix:
let newIndex = root.activeTabIndex;
// Shift down if the removed tab was before the active one
if (idx < newIndex) newIndex--;
// Clamp in case we removed the last tab
if (newIndex >= newTabs.length) newIndex = Math.max(0, newTabs.length - 1);Also add a test:
it("keeps active tab stable when removing a tab before it (3+ tabs)", () => {
const l: LayoutLeaf = {
kind: "leaf", id: "a",
tabs: [{ id: "t1", pane: pane("s1") }, { id: "t2", pane: pane("s2") }, { id: "t3", pane: pane("s3") }],
activeTabIndex: 2, // s3 is active
};
const result = removeTab(l, "a", "t1") as LayoutLeaf; // remove s1 (before active)
expect(result.tabs.length).toBe(2);
expect(result.activeTabIndex).toBe(1); // still pointing at s3
expect(result.tabs[result.activeTabIndex].pane.id).toBe("s3");
});| } | ||
|
|
||
| const updated = removeTab(layout, leafId, tabId); | ||
| const newActivePane = derivedActivePane(updated, focusedLeafId); |
There was a problem hiding this comment.
🟡 Warning
Problem: closeTab calls derivedActivePane(updated, focusedLeafId) using the global focusedLeafId, but the closed tab might be in a different leaf than the focused one. If the user closes a background tab in a non-focused leaf, derivedActivePane resolves the active pane from the focused leaf (correct), but activePane is set unconditionally — which is fine today, but silently assumes focus stays put.
More concretely: if leafId !== focusedLeafId, the new layout has the correct tab removed, but the activePane is re-derived from the focused leaf, not the leaf where the close happened. This is probably the intended behavior, but if the closed tab happened to be the activePane (e.g., user has two leaves, clicks close on the active one in a non-focused leaf), activePane will jump to whatever is in the focused leaf rather than the adjacent tab in the same leaf.
Why it matters: Edge case but reproducible: split into two panes, switch a background pane to multi-tab, close the currently-active (but not focused-leaf) tab. Active panel indicator jumps unexpectedly.
Suggested fix:
closeTab: (leafId, tabId) => {
// ...
const updated = removeTab(layout, leafId, tabId);
// Derive active pane from the leaf that was modified, not the focused leaf
const targetLeaf = findLeaf(updated, leafId);
const newActivePane = targetLeaf
? (activeTabPane(targetLeaf) ?? derivedActivePane(updated, focusedLeafId))
: derivedActivePane(updated, focusedLeafId);
set({
layout: updated,
activePane: newActivePane,
...saveGroupSnapshot(groups, activeGroupId, updated, focusedLeafId),
});
},
nox-0x
left a comment
There was a problem hiding this comment.
Great feature — the tab bar auto-hide, migration path, and pointer-events fix during drag are all solid. One critical bug in removeTab: when closing a tab whose index is before the active tab in a 3+ tab scenario, the active tab index is not decremented, causing the focused session to silently jump to the wrong tab. Fix is a one-liner (if (idx < newIndex) newIndex--). Also flagged a warning on closeTab in the store re: active pane derivation when closing a tab in a non-focused leaf.
| first: removeTab(root.first, leafId, tabId), | ||
| second: removeTab(root.second, leafId, tabId), | ||
| }; | ||
| } |
There was a problem hiding this comment.
🔴 Critical
Problem: removeTab does not decrement activeTabIndex when a tab before the active one is removed. With 3+ tabs, closing a background tab causes the active session to silently jump to the wrong tab.
Why it matters: Example — tabs [A, B, C, D], activeTabIndex = 2 (C is active). User closes A (index 0). newTabs = [B, C, D], newIndex = 2, 2 >= 3 is false → stays at 2, now pointing at D instead of C. User's focused session jumps without any action on their part.
Suggested fix:
let newIndex = root.activeTabIndex;
if (idx < newIndex) newIndex--;
if (newIndex >= newTabs.length) newIndex = Math.max(0, newTabs.length - 1);
nox-0x
left a comment
There was a problem hiding this comment.
Great feature overall — tab bar, migration, and drag-drop tab support are solid. One critical bug still outstanding from previous review: removeTab doesn't decrement activeTabIndex when removing a tab before the active one, causing the focused session to silently jump in 3+ tab scenarios. One-liner fix: if (idx < newIndex) newIndex--.
nox-0x
left a comment
There was a problem hiding this comment.
Still missing the one-liner fix from prior reviews: removeTab does not decrement activeTabIndex when a tab before the active one is removed. In a 3+ tab pane, closing any background tab at a lower index silently shifts the active session to the wrong tab. Fix: add if (idx < newIndex) newIndex-- before the clamp. See prior inline comment for full details and failing test case. Everything else in this revision looks solid — migration path, pointer-events during drag, tab bar layout offset, and allTabPanes usage in movePaneToLeaf are all correct. Unblock once that one line is in.
| <span className="flex-1 truncate text-left"> | ||
| {getTabTitle(tab)} | ||
| </span> | ||
| <button |
There was a problem hiding this comment.
🟡 Warning
Problem: <button> (close ✕) nested inside <button> (tab) — invalid HTML per spec. A <button> cannot contain interactive elements including other buttons.
Why it matters: Browsers handle this inconsistently — some promote the inner button outside the outer one during DOM parsing, breaking event flow. The e.stopPropagation() on the close button may not work as expected in all browsers, causing a tab-switch to fire simultaneously with the close. Safari in particular tends to reparse this incorrectly.
Suggested fix: Replace the outer <button> with a <div role="button"> (or keep the outer button and use a <span> for the close target), similar to how the sidebar rows already handle this pattern:
<div
role="button"
tabIndex={0}
onMouseDown={(e) => e.preventDefault()}
onClick={() => switchTabInLeaf(leafId, i)}
// ...styles
>
{/* ...content */}
<button
type="button"
onClick={(e) => { e.stopPropagation(); closeTab(leafId, tab.id); }}
tabIndex={-1}
>✕</button>
</div>
nox-0x
left a comment
There was a problem hiding this comment.
Two issues remain unresolved from prior reviews: (1) 🔴 removeTab still missing if (idx < newIndex) newIndex-- — in 3+ tab panes, closing a background tab silently shifts the focused session to the wrong tab; (2) 🟡 <button> nested inside <button> in TabBar.tsx (line 88) — invalid HTML that breaks stopPropagation() in Safari/some browsers. Both are quick fixes. Everything else in this revision is solid.
nox-0x
left a comment
There was a problem hiding this comment.
Same two issues from prior reviews are still unresolved in this revision (75f9009): (1) 🔴 removeTab still missing if (idx < newIndex) newIndex-- — 3+ tab panes will silently jump the focused session when a background tab at a lower index is closed; (2) 🟡 <button> nested inside <button> in TabBar.tsx line 88 — invalid HTML that can break stopPropagation() in Safari. Both are one-liners. Everything else looks great — please just add that index decrement and swap the outer tab <button> to a <div role="button">.
LayoutLeaf now holds tabs[] + activeTabIndex instead of a single pane. New tree operations: addTab, removeTab, setActiveTab, activeTabPane. Migration function for persisted old-format layouts. All 59 layout tree tests passing. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Core data model change: LayoutLeaf now holds tabs[] + activeTabIndex instead of a single pane. Each split pane can have multiple tabs (sessions, previews) with a tab bar for switching between them. Data model (layoutTree.ts): - LayoutLeaf: pane → tabs[] + activeTabIndex - New: TabItem, addTab, removeTab, setActiveTab, activeTabPane - Migration function for persisted old-format layouts - All 59 layout tree tests passing Store (store.ts): - New actions: addTabToLeaf, closeTab, switchTabInLeaf - switchPane finds pane across all tabs and activates correct tab - openPreview adds as new tab instead of replacing - remapSessionIds handles tab arrays - Persist migration calls migrateLayout for backwards compat UI components: - TabBar.tsx: horizontal tab bar with close buttons, auto-hides for single tab (VS Code style), 28px height - PaneSlot.tsx: renders TabBar above content area, registers content rect (excludes tab bar) for SessionMountLayer positioning - SessionMountLayer.tsx: maps active tab per leaf to rect - DropZoneOverlay.tsx: center drop = add tab (not replace) - Sidebar: visible sessions highlighted with 50% opacity border - App.tsx: keyboard shortcuts use activeTabPane Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…, fix UX - Remove GroupContainer from Sidebar — flat agent list, no more split grouping - Remove all group-related types, imports, and rendering logic from Sidebar - Add AgentStatusIcon to TabBar tabs (matches sidebar status indicators) - Fix sidebar highlighting: use allTabPanes to highlight all sessions in any tab, not just active tabs - Fix drag-and-drop: only preventDefault on mousedown for non-draggable elements so HTML5 drag initiation works on session items - Disable pointer events on SessionMountLayer during drag so DropZoneOverlay receives dragover/drop events through xterm canvas - Update DropZoneOverlay center label: "Add Tab" instead of "Replace" - Fix movePaneToLeaf center zone: adds as tab + removes only the source tab (not the whole leaf if source has multiple tabs) - Harden persist migration: try-catch around layout and group restoration to handle corrupted localStorage gracefully - Tab bar aesthetics: active tab font-weight medium, hover brightness, close button shows on tab hover via group/tab class Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Tab bar visible for all panes (even single tab) — matches cmux - Removed session title bar from SessionPane — tab bar is the title now - PaneSlot always offsets rect by TAB_HEIGHT Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Tabs now use width: 100%/N (max 180px) instead of shrink-0 content width. All tabs in a pane are the same size regardless of name length. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Tab layout: [icon] [name flex-1] [✕ shrink-0 ml-auto] - Status icon shrink-0 on left - Name takes flex-1 space, truncates, text-left - Close button shrink-0 ml-auto pushed to right edge - Max tab width increased to 200px Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Always render a 12x12 placeholder for the status icon slot, even when status hasn't arrived yet. Prevents the tab name from jumping right when the checkmark/spinner appears. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Tab status now defaults to "working" when no hook status exists yet, matching the sidebar's behavior. The spinning indicator shows immediately when a session starts, then transitions to idle/checkmark when hooks report the actual status. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1. removeTab: decrement activeTabIndex when removing a tab before the active one (prevents wrong tab focused in 3+ tab scenarios) 2. TabBar: change outer <button> to <div role="tab"> to avoid invalid nested <button> HTML (breaks Safari stopPropagation) 3. Add test case for 3+ tab removeTab index adjustment Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
getTabStatus returns null for non-session tabs (previews). Guard the AgentStatusIcon render with a null check and use non-null assertion. Reserve 12x12 space for consistent alignment. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
86528ae to
086e356
Compare
nox-0x
left a comment
There was a problem hiding this comment.
Both previously flagged issues are resolved: removeTab now correctly decrements activeTabIndex when closing a tab before the active one (3+ tab case) with the matching regression test, and the outer tab row in TabBar.tsx is now a <div role="tab"> instead of a <button>, fixing the nested-button HTML validity issue. The migration path, pointer-events fix during drag, and allTabPanes usage throughout are all solid. 🟡 Minor: closeTab in the store still derives activePane via focusedLeafId when closing a tab in a non-focused leaf — low-priority UX edge case flagged in the prior review, non-blocking. LGTM.
Ctrl/Cmd+clicking a `.md` path in a terminal no longer opens a preview. The feature shipped in #30 (ADR-018) and silently regressed twice: - #263 made dockview the default layout engine. Every other `open*` store action got an explicit dockview branch; `openPreview` was the only one that did not, so it set `activePane` alone -> `syncToActive` found no bound workspace for the fresh preview id -> `showSolo()` removed every panel. In practice ctrl+clicking a `.md` in an agent's terminal destroyed the terminal pane, reversing #103's "opens as a new tab". - #249 defaulted the sidebar to hierarchy view, which builds no preview rows, leaving an open preview unreachable from the sidebar entirely. Both landed green: the feature had zero test coverage. Removes PreviewPane, PreviewPage, the /preview dispatch in main.tsx, MarkdownLinkProvider, the "preview" ActivePane member with previewPanes state and openPreview/closePreview, the .prose-custom stylesheet, three codicons orphaned by the deleted toolbar, and the server's GET /api/files/read + WS /ws/files/watch (Terry approved deleting the file API; it had acquired no other consumer in four months). Drops react-markdown, remark-gfm, mermaid, dompurify and @types/dompurify -- and with them the dashboard's only two dangerouslySetInnerHTML call sites. Backward compat, no migration hook needed: `isValidActivePane` now rejects {type:"preview"} so a stale persisted pane degrades to the empty state instead of restoring into a silently blank pane (PaneContent has no "preview" case); `previewPanes` leaves localStorage on the first persisted write via partialize; leftover `preview:*` order keys are swept by the existing fetchSessions prune once its escape hatch is removed. A regression test locks in the validator rejection. Preserved: UrlLinkProvider, the OSC 8 linkHandler, deduplicatedOpen, the shared ILink/ILinkProvider types, and the `.overflow-y-auto` half of the scrollbar rules `.prose-custom` shared. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FcJXJ8wja3pEWSGr5DeEXw
#289) * refactor(dashboard): remove the broken markdown file preview (ADR-059) Ctrl/Cmd+clicking a `.md` path in a terminal no longer opens a preview. The feature shipped in #30 (ADR-018) and silently regressed twice: - #263 made dockview the default layout engine. Every other `open*` store action got an explicit dockview branch; `openPreview` was the only one that did not, so it set `activePane` alone -> `syncToActive` found no bound workspace for the fresh preview id -> `showSolo()` removed every panel. In practice ctrl+clicking a `.md` in an agent's terminal destroyed the terminal pane, reversing #103's "opens as a new tab". - #249 defaulted the sidebar to hierarchy view, which builds no preview rows, leaving an open preview unreachable from the sidebar entirely. Both landed green: the feature had zero test coverage. Removes PreviewPane, PreviewPage, the /preview dispatch in main.tsx, MarkdownLinkProvider, the "preview" ActivePane member with previewPanes state and openPreview/closePreview, the .prose-custom stylesheet, three codicons orphaned by the deleted toolbar, and the server's GET /api/files/read + WS /ws/files/watch (Terry approved deleting the file API; it had acquired no other consumer in four months). Drops react-markdown, remark-gfm, mermaid, dompurify and @types/dompurify -- and with them the dashboard's only two dangerouslySetInnerHTML call sites. Backward compat, no migration hook needed: `isValidActivePane` now rejects {type:"preview"} so a stale persisted pane degrades to the empty state instead of restoring into a silently blank pane (PaneContent has no "preview" case); `previewPanes` leaves localStorage on the first persisted write via partialize; leftover `preview:*` order keys are swept by the existing fetchSessions prune once its escape hatch is removed. A regression test locks in the validator rejection. Preserved: UrlLinkProvider, the OSC 8 linkHandler, deduplicatedOpen, the shared ILink/ILinkProvider types, and the `.overflow-y-auto` half of the scrollbar rules `.prose-custom` shared. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FcJXJ8wja3pEWSGr5DeEXw * fix(dashboard): close retired pane types from stale saved layouts Polish pass on the preview removal. The silent-failure review found the backward-compat guard was correct but bypassable: a pane descriptor has TWO persisted carriers and only one was guarded. `isValidActivePane` covers `activePane`. But dockview's `toJSON()` also serializes every panel's `params`, so `params.pane = {type:"preview"}` survives inside `dvWorkspaces[*].serialized` and is re-created verbatim by `fromJSON` on restore -- and `merge` validates only that `serialized` is a non-null object. That rendered an empty pane under a tab titled "Tab", with no console output. The dead-panel strip does not rescue it: it is gated on `sessionsInitialFetchDone`, which never flips while `/api/agents` is failing, so the blank tab was permanent for that session. TypeScript cannot see the branch at all -- "preview" was deleted from the union, so it looks unreachable while staying live at runtime against untyped persisted JSON. Guard at the narrowest shared boundary rather than per-path: every panel path (fromJSON, showSolo, addPanel, drop) converges on PaneContent, so an unrenderable pane type is closed there with a warning naming the panel and type. StatusTab now reports `Unknown (<type>)` instead of "Tab" so the transient state is diagnosable. Closes a related bypass: `paneFromId` classifies every non-singleton id as a session, so activating such a panel wrote back `{type:"session", id:"preview-..."}` -- which then PASSES `isValidActivePane` on the next reload, laundering a retired pane into a terminal dialing /ws/terminal for a session that never existed. New `paneFromPanel` trusts a panel's own descriptor and skips the writeback when it is invalid. Verified against a real dockview blob: the retired panel is closed with its warning, no blank tab, no page errors. Adds PaneContent.dom.test.tsx and paneFromPanel coverage (265 dashboard tests, was 257). Also from the review pass: - Drop the `SidebarItem`/`DisplayItem` wrappers rather than leave single-member unions: nothing reads the discriminant now, and `sidebarItemKey` was an exact duplicate of `sessionOrderKey`. - ADR-018's superseded note moves to the file's established format (trailing `**Update (date, ADR-XXX):**`, not a leading blockquote). - Correct ADR-059: `reconcileDeadWorkspaces` prunes `paneIds`, NOT `serialized`; and the order-key prune is opportunistic, not a guaranteed migration. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FcJXJ8wja3pEWSGr5DeEXw * fix(dashboard): bail from render before reading an absent pane descriptor Addresses nox-0x's review on #289. The retired-pane-type guard defended against a nullish descriptor in its effect (`(pane ?? {})`), but the render body read `pane.type` in the `inner` IIFE -- and render runs BEFORE effects. So a persisted panel with no `params.pane` at all (the same untrusted `dvWorkspaces[*].serialized` carrier the guard exists for) threw a TypeError into the ErrorBoundary instead of closing the panel with its diagnostic warning. The guard did not cover what its own code already assumed. PaneContent now returns null before the switch when the descriptor isn't renderable -- placed after every hook so hook order stays stable, and covering every downstream `pane` read rather than just the switch. StatusTab had the same shape across six reads (`switch (pane.type)`, `pane.id.slice`, the status ternary, and three in handleClose). All are nullish-safe now. The raw discriminant is hoisted into `declaredType` because TS narrows `pane` to `never` inside the exhausted `default`, so it cannot be inspected there. Tests: adds the absent-descriptor case nox asked for, plus a non-object one. Verified the absent-descriptor test FAILS with the guard reverted, so it is a real regression test rather than a passing assertion. 267 dashboard tests (was 265). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FcJXJ8wja3pEWSGr5DeEXw --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Summary
Redesigns the split-pane system to support multiple tabs per pane, inspired by cmux's tab architecture. Each split pane now has a tab bar where sessions and previews can be stacked as tabs, instead of requiring a new split for each additional session.
Key Changes
Data Model (
layoutTree.ts)LayoutLeafchanged frompane: ActivePane | nulltotabs: TabItem[] + activeTabIndex: numberaddTab,removeTab,setActiveTab,activeTabPane,allTabPanesTab Bar (
TabBar.tsx)AgentStatusIconper tab (spinner/checkmark/triangle matching sidebar)Store (
store.ts)addTabToLeaf,closeTab,switchTabInLeafswitchPanefinds pane across all tabs and activates the correct tabopenPreviewadds as new tab instead of replacing current panemovePaneToLeafcenter zone adds as tab + handles multi-tab source removalSidebar (
Sidebar.tsx)SessionMountLayer
Test plan
Screenshots
Tabs + split view tested via Playwright on forge.
🤖 Generated with Claude Code