Repository navigation
fix(workspace): keep the workspace selected when its last tab closes - #12142
AnddyAgudelo wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughClosing the final terminal, browser, or editor surface now preserves the active worktree and clears surface selections. Tab-group and terminal close paths no longer deactivate empty worktrees. A new empty tab-group component provides translated content, shortcut labels, and a “New terminal” action. Runtime-owned worktrees wait for host tab synchronization before showing the state. Tests cover store behavior, close flows, synchronization, rendering, and end-to-end navigation. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
Greptile SummaryThis PR fixes issue #11699, where closing every tab in a workspace inadvertently deselected the workspace itself — collapsing the file explorer and Git panel until the user re-clicked the workspace. The fix removes seven
Confidence Score: 5/5Safe to merge. All seven deselection sites are patched consistently, the empty-state visibility predicate is purely derived (no timers, no mount-local state), and the publication subscription is correctly unsubscribed by useSyncExternalStore on unmount. The change is surgical — it removes setActiveWorktree(null) from seven well-identified call sites and adds a visibility guard that prevents the empty state from appearing while a remote worktree is still hydrating. Each modified close path has a matching test that was previously asserting the old deselection and is now asserting the new workspace-stays-selected invariant. The useSyncExternalStore subscription covers the zero-tab remote-snapshot case that store-slice selectors cannot observe. No untested code paths were found, and no deliberate deselection paths (sleep, worktree delete, repo removal) were altered. Files Needing Attention: No files require special attention. The most structurally novel file is tab-group-empty-state-visibility.ts, which is fully covered by both a pure-predicate test and a reactivity test that exercises the real sync path.
|
| Filename | Overview |
|---|---|
| src/renderer/src/store/slices/tabs.ts | Renames shouldDeactivateWorktree to worktreeBecameEmpty and removes activeWorktreeId/activeWorkspaceKey/activeWorkspaceExecutionHostId from the zero-tab clear path, keeping workspace selection intact while still clearing per-surface actives. |
| src/renderer/src/store/slices/editor.ts | Both closeFile and closeAllFiles no longer clear activeWorktreeId when the last editor closes; only activeBrowserTabId and per-surface state is cleared. |
| src/renderer/src/components/tab-group/tab-group-empty-state-visibility.ts | New module implementing shouldShowTabGroupEmptyState, isAwaitingFirstHostTabs, and useTabGroupEmptyStateVisible hook using useSyncExternalStore against the publication epoch store. |
| src/renderer/src/components/tab-group/TabGroupEmptyState.tsx | New empty-state component shown when all tabs are closed; renders a New terminal button with platform-resolved shortcut chip via useShortcutKeyDetails. |
| src/renderer/src/components/tab-group/TabGroupPanel.tsx | Wires useTabGroupEmptyStateVisible(worktreeId, model.groupTabs.length) and conditionally renders TabGroupEmptyState. |
| src/renderer/src/runtime/web-session-tabs-sync.ts | Adds publicationListeners Set, subscribeWebSessionTabsPublication, and notifyWebSessionTabsPublication covering all epoch-change paths with correct unsubscribe semantics. |
| src/renderer/src/components/tab-group/useTabGroupWorkspaceModel.ts | Removes leaveWorktreeIfEmpty callback and skipEmptyCheck option from closeItem/closeGroup; setActiveWorktree is no longer consumed. |
| src/renderer/src/components/terminal/terminal-tab-actions.ts | Removes the final setActiveWorktree(null) fallback when no browser tab follows the last terminal. |
| src/renderer/src/components/Terminal.tsx | Browser-tab close path now calls setActiveTabType('terminal') instead of setActiveWorktree(null) when no other surface exists. |
| src/renderer/src/components/terminal-pane/TerminalPaneOverlayLayer.tsx | Removes the second leaveWorktreeIfEmpty implementation and stops passing it as a prop to TerminalOverlaySlot. |
| tests/e2e/tab-close-navigation.spec.ts | E2E test inverted to assert workspace stays selected and empty state is visible after closing the last surface. |
Sequence Diagram
sequenceDiagram
participant User
participant CloseHandler as Close Handler
participant Store as Zustand Store
participant TabGroupPanel as TabGroupPanel
participant VisibilityHook as useTabGroupEmptyStateVisible
participant SyncSource as web-session-tabs-sync
User->>CloseHandler: Close last tab
CloseHandler->>Store: closeTab / closeFile / closeBrowserTab
Note over Store: worktreeBecameEmpty = true
Note over Store: Clears activeTabId, activeFileId, activeBrowserTabId
Note over Store: Does NOT clear activeWorktreeId
Store-->>TabGroupPanel: "groupTabs.length = 0"
TabGroupPanel->>VisibilityHook: useTabGroupEmptyStateVisible(worktreeId, 0)
VisibilityHook->>Store: workspaceSessionReady?
VisibilityHook->>SyncSource: isAwaitingFirstHostTabs?
alt Local worktree
SyncSource-->>VisibilityHook: false
VisibilityHook-->>TabGroupPanel: "emptyStateVisible = true"
TabGroupPanel-->>User: TabGroupEmptyState + New Terminal button
else Remote worktree awaiting first snapshot
SyncSource-->>VisibilityHook: true
VisibilityHook-->>TabGroupPanel: "emptyStateVisible = false"
Note over SyncSource: Host publishes zero-tab snapshot
SyncSource->>SyncSource: notifyWebSessionTabsPublication()
SyncSource-->>VisibilityHook: epoch now set
VisibilityHook-->>TabGroupPanel: "emptyStateVisible = true"
TabGroupPanel-->>User: TabGroupEmptyState + New Terminal button
end
Reviews (2): Last reviewed commit: "fix(workspace): keep the workspace selec..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const worktreeBecameEmpty = | ||
| browserTabsForWorktree.length === 0 && terminalTabsForWorktree.length === 0 |
There was a problem hiding this comment.
closeAllFiles. browserTabsForWorktree.length === 0 && terminalTabsForWorktree.length === 0 returns true when only simulator unified tabs remain. The consequence is the same as in closeFile — activeBrowserTabId is unnecessarily set to null (already null with no browser tabs) — and is benign today, but for correctness the check should include simulators.
| const worktreeBecameEmpty = | |
| browserTabsForWorktree.length === 0 && terminalTabsForWorktree.length === 0 | |
| const simulatorTabsForWorktree = (s.unifiedTabsByWorktree[activeWorktreeId] ?? []).filter( | |
| (item) => item.contentType === 'simulator' | |
| ) | |
| const worktreeBecameEmpty = | |
| browserTabsForWorktree.length === 0 && | |
| terminalTabsForWorktree.length === 0 && | |
| simulatorTabsForWorktree.length === 0 |
|
Closing every terminal/agent/browser/editor tab deselected the workspace,
which emptied the file explorer ("Select a workspace to browse files") and
the Git panel until the user re-opened the workspace and spawned a terminal.
Seven last-tab-close paths now leave `activeWorktreeId` intact: the unified
tab slice, editor.ts's two `shouldDeactivateWorktree` branches (closeFile and
closeAllFiles), the legacy terminal and browser close handlers, and the two
`leaveWorktreeIfEmpty` split-group hooks (now removed outright).
The zero-tab pane body previously rendered nothing, so tab groups gain a
quiet empty state with a direct "New terminal" action and its platform
shortcut. Its visibility is derived from observable state, never a clock and
never mount-local state: `ensureWorktreeRootGroup` creates the root group
before its first tab, and a runtime-owned worktree then waits on the host
session-tab mirror, so the empty state is suppressed for exactly that wait.
That wait ends on a publication epoch held in module-local maps, which move
no store slice when the snapshot carries zero tabs, so `web-session-tabs-sync`
publishes an explicit subscription consumed via useSyncExternalStore.
This supersedes PR stablyai#677 ("return to Orca landing screen after closing last
terminal"). stablyai#677 deselected precisely because a selected workspace with no
surfaces rendered blank; the empty state removes that failure mode, so the
landing fallback is no longer needed. Its E2E guard in
tab-close-navigation.spec.ts now asserts the new contract, and sleeping a
workspace is the route back to Landing.
Fixes stablyai#11699
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
9c35739 to
c38d94c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/renderer/src/components/tab-group/TabGroupEmptyState.tsx (1)
7-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReduce the component comment.
This comment repeats behavior that
TabGroupEmptyStatemakes clear. Remove it, or replace it with one concise line that documents only the non-obvious retention requirement.As per coding guidelines, “Comments must be concise, limited to non-obvious information, and preferably one line.”
Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c3723790-356e-4615-a312-1079045aa558
📥 Commits
Reviewing files that changed from the base of the PR and between 9c35739058f226c33eb0a4b9852c151ce4ca3d81 and c38d94c.
📒 Files selected for processing (23)
src/renderer/src/components/Terminal.tsxsrc/renderer/src/components/shared/kill-all-terminal-surfaces.tssrc/renderer/src/components/tab-group/TabGroupEmptyState.test.tsxsrc/renderer/src/components/tab-group/TabGroupEmptyState.tsxsrc/renderer/src/components/tab-group/TabGroupPanel.tsxsrc/renderer/src/components/tab-group/tab-group-empty-state-reactivity.test.tsxsrc/renderer/src/components/tab-group/tab-group-empty-state-visibility.test.tssrc/renderer/src/components/tab-group/tab-group-empty-state-visibility.tssrc/renderer/src/components/tab-group/useTabGroupWorkspaceModel.focus.test.tssrc/renderer/src/components/tab-group/useTabGroupWorkspaceModel.tssrc/renderer/src/components/terminal-pane/TerminalOverlaySlot.tsxsrc/renderer/src/components/terminal-pane/TerminalPaneOverlayLayer.react185.test.tsxsrc/renderer/src/components/terminal-pane/TerminalPaneOverlayLayer.tsxsrc/renderer/src/components/terminal/terminal-tab-actions-kill-all.test.tssrc/renderer/src/components/terminal/terminal-tab-actions.tssrc/renderer/src/i18n/locales/en.jsonsrc/renderer/src/runtime/web-session-tabs-sync.tssrc/renderer/src/store/slices/editor.test.tssrc/renderer/src/store/slices/editor.tssrc/renderer/src/store/slices/store-cascades.test.tssrc/renderer/src/store/slices/tabs.test.tssrc/renderer/src/store/slices/tabs.tstests/e2e/tab-close-navigation.spec.ts
💤 Files with no reviewable changes (2)
- src/renderer/src/components/terminal-pane/TerminalPaneOverlayLayer.react185.test.tsx
- src/renderer/src/components/terminal-pane/TerminalPaneOverlayLayer.tsx
🚧 Files skipped from review as they are similar to previous changes (20)
- src/renderer/src/components/shared/kill-all-terminal-surfaces.ts
- src/renderer/src/components/Terminal.tsx
- src/renderer/src/store/slices/tabs.test.ts
- src/renderer/src/store/slices/editor.test.ts
- src/renderer/src/i18n/locales/en.json
- src/renderer/src/components/tab-group/TabGroupPanel.tsx
- tests/e2e/tab-close-navigation.spec.ts
- src/renderer/src/components/tab-group/useTabGroupWorkspaceModel.ts
- src/renderer/src/store/slices/tabs.ts
- src/renderer/src/components/tab-group/tab-group-empty-state-visibility.ts
- src/renderer/src/runtime/web-session-tabs-sync.ts
- src/renderer/src/components/tab-group/tab-group-empty-state-reactivity.test.tsx
- src/renderer/src/components/tab-group/TabGroupEmptyState.test.tsx
- src/renderer/src/store/slices/editor.ts
- src/renderer/src/components/terminal-pane/TerminalOverlaySlot.tsx
- src/renderer/src/components/terminal/terminal-tab-actions.ts
- src/renderer/src/components/tab-group/useTabGroupWorkspaceModel.focus.test.ts
- src/renderer/src/components/terminal/terminal-tab-actions-kill-all.test.ts
- src/renderer/src/components/tab-group/tab-group-empty-state-visibility.test.ts
- src/renderer/src/store/slices/store-cascades.test.ts
|
Thank you so much, @AnddyAgudelo, for the care and depth of this contribution: tracing the close paths, explaining the original Landing behavior, handling remote readiness, and adding regression coverage and screenshots. We referred to your empty-state implementation and analysis while comparing the community work with the launch UI that has now landed in #32182. I have added a named acknowledgment and a link to this PR in #32182. The empty-pane action screen has landed there, but your main behavior change—keeping a workspace selected after its last tab closes—has not. #11699 and #17682 still track that separate requirement, and #32182’s description now states that distinction explicitly. Thank you again for the implementation and unusually thorough investigation. We appreciate the work you put into making this behavior understandable and testable. |
Summary
Fixes #11699.
Closing every tab in a workspace deselected the workspace itself. The file explorer fell back to "Select a workspace to browse files" and the Git panel emptied, so browsing files or using source control required re-clicking the workspace and opening a terminal — even when the user only wanted to read a file.
FileExplorergates onactiveWorktreeId, and seven separate close paths were clearing it once the last surface went away:store/slices/tabs.tscloseUnifiedTab)store/slices/editor.ts×2closeFileandcloseAllFilescomponents/terminal/terminal-tab-actions.tscomponents/Terminal.tsxcomponents/tab-group/useTabGroupWorkspaceModel.tsleaveWorktreeIfEmpty(split groups)components/terminal-pane/TerminalPaneOverlayLayer.tsxleaveWorktreeIfEmptyAll seven now leave
activeWorktreeIdintact and clear only the per-surface actives (activeTabId,activeBrowserTabId,activeFileId). The twoleaveWorktreeIfEmptyhooks are removed outright, along with the now-deadskipEmptyCheckoption.Why the deselection existed, and why removing it is safe now
This intentionally supersedes PR #677 ("return to Orca landing screen after closing last terminal"). That behavior existed for a good reason, recorded in
tests/e2e/tab-close-navigation.spec.ts:That was accurate — the pane body renders only
activeTab-driven content, so at zero tabs it was a tab strip over empty space. Keeping the workspace selected therefore required giving the pane something to show, so this PR addsTabGroupEmptyState: the hero icon, a short explanation that the sidebar still works, and a direct New terminal action with its platform shortcut.Sleeping a workspace is now the route back to Landing. Deliberate deselections are untouched: sleep, worktree delete, repo removal, and folder-workspace delete.
Empty-state visibility is derived, never timed
A group also sits at zero tabs before its first tab arrives —
ensureWorktreeRootGroupcreates the root group first, and a runtime-owned worktree then waits on the host's session-tab mirror for however long the link takes. Showing the empty state during that wait would be wrong.Visibility is
workspaceSessionReady && tabCount === 0 && !awaitingFirstHostTabs, whereawaitingFirstHostTabsis true only while a runtime-owned worktree has no published snapshot epoch. No timer and no mount-local state — a reparent or reload can't strand the pane with no way to open a terminal. Because the epoch lives in module-local maps and a zero-tab snapshot moves no store slice,web-session-tabs-sync.tsgained an explicit publication subscription consumed viauseSyncExternalStore; without it the predicate would be correct but the UI would never re-ask.Screenshots
Light-mode full window — the workspace stays highlighted in the sidebar and the file explorer stays populated, which is the reported bug going away. Plus light/dark detail shots of the empty state.
Testing
pnpm lintpnpm typecheckpnpm testpnpm buildNew/updated coverage:
tab-group-empty-state-visibility.test.ts— the visibility predicate, including a remote worktree resting at zero tabs reading as empty rather than perpetually pending, and remount-invariance.tab-group-empty-state-reactivity.test.tsx— renders the real hook and publishes a zero-tab snapshot through the real sync path, asserting the empty state appears with no other state change. Pure-predicate tests can't catch a missing subscription; this one can.useTabGroupWorkspaceModel.focus.test.ts— last terminal tab, last browser tab, sole group closed, and two-group split where only the emptied pane collapses.tabs.test.ts,store-cascades.test.ts,editor.test.ts,terminal-tab-actions-kill-all.test.ts— existing assertions that encoded the old deselection are retitled and inverted, each with aWhy:pointing at this issue. Also a remote/host-owned close case.tests/e2e/tab-close-navigation.spec.ts— the last-surface test now asserts the workspace stays selected and the empty state offers a terminal. Its header documented fix: return to Orca landing screen after closing last terminal #677's rationale as current truth and has been rewritten.Verified fail-on-main: the four behavior test files produce 7 failures against
main's source and pass on this branch.AI Review Report
Reviewed across five rounds, each verified against the code rather than accepted on assertion.
metaKeyhardcoding. The shortcut chip renders throughuseShortcutKeyDetails+ShortcutKeyCombo, which resolve the platform internally — the screenshots showCtrl + Twith the Linux/Windows+separator; macOS renders adjacent glyphs. No new path handling.shouldBootstrapInitialWebRuntimeTerminal's guard is closure-local and consumed once, so a remote workspace resting at zero tabs is a real state, and this fix makes it a common one. Both replaced by the derived predicate above.terminal-tab-actions.ts's host-authoritative branch returns before the last-tab logic and is covered by its own test.openNewTerminalTabInActiveWorkspace, the same action the tab bar's+uses, which already handles folder-workspace keys and remote runtimes. No new assumption that a workspace is a git worktree.source-controlsidebar tab. Nothing GitHub-specific.useSyncExternalStoresubscription per mounted tab group, notified only when a session-tab snapshot is recorded or cleared.docs/STYLEGUIDE.mdthroughout —size-7hero icon (its documented empty-state use),muted-foreground,outline/smbutton, lucideSquareTerminal, no invented values. Rendered and inspected in both themes; the shortcut chip was regrouped after review because it read as a floating element rather than the button's shortcut.pnpm test. Running it revealed the fix was incomplete — the twoeditor.tssites were still deselecting, so closing the last editor reproduced the original bug on a path no unit test covered.Security Audit
No new IPC surface, no command execution, no path handling, no auth or credential handling, no network calls, and no new dependencies. Changes are renderer-local state transitions plus one in-process listener set (
publicationListeners), whose unsubscribe is returned touseSyncExternalStoreand cleared on all three teardown paths, so listeners can't accumulate across worktree or environment churn. The empty state renders static translated copy — no user-controlled interpolation. No follow-up needed.Notes
TerminalRemoteRuntimeReconnectBanner/TerminalSshReconnectOverlay; the empty state was deliberately not made to double as a connectivity error surface.tab-close-navigation.spec.ts, and it added ~19s to CI for evidence capture.