From 795776e9c06f9cf14d6e80fa06cc39c1fa080fb1 Mon Sep 17 00:00:00 2001 From: onevcat Date: Tue, 21 Apr 2026 12:00:03 +0900 Subject: [PATCH] feat(shelf): retire book on last-tab-close (reversed from empty-book state) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reverse an earlier design call. Closing the last tab of a book now removes the book from the Shelf entirely instead of keeping an empty placeholder — a lingering empty book read as dead weight on the shelf, and nothing truly needed the "still present but empty" state. Implementation: - `TerminalClient.Event.tabClosed` gains a `remainingTabs: Int` payload so AppFeature can tell a last-close apart from a routine close. `WorktreeTerminalManager.onTabClosed` now captures the post-close tab count and emits it. - AppFeature forwards `tabClosed` with `remainingTabs == 0` to `.repositories(.markWorktreeClosed(id))`. Other closes are ignored. - `RepositoriesFeature.markWorktreeClosed` removes the ID from `openedWorktreeIDs` — so its spine drops off. If Shelf is active and the closed book was the open one, the handler auto-advances to the first remaining book via `shelfBookSelectionEffect`; if no books remain, selection is left alone and ShelfView falls through to the "No book selected" empty state. In normal view the selection is untouched so the user's current context isn't disturbed. Three tests cover the behavior: plain removal when not open, auto- advance to the next book when the open one is retired, and selection untouched in normal-view context. Design doc updated with the reversed decision and a journal entry noting the motivation. --- doc-onevcat/shelf-view.md | 34 ++++-- .../Clients/Terminal/TerminalClient.swift | 2 +- .../Features/App/Reducer/AppFeature.swift | 6 + .../Reducer/RepositoriesFeature.swift | 19 +++ .../WorktreeTerminalManager.swift | 6 +- supacodeTests/ShelfFeatureTests.swift | 114 ++++++++++++++++++ 6 files changed, 168 insertions(+), 13 deletions(-) diff --git a/doc-onevcat/shelf-view.md b/doc-onevcat/shelf-view.md index bdc3a458..87cf36f1 100644 --- a/doc-onevcat/shelf-view.md +++ b/doc-onevcat/shelf-view.md @@ -221,10 +221,15 @@ Mirror Prowl's normal-mode tab close behavior: ### Closing the Last Tab in a Book -Closing the last tab does **not** remove the book from the shelf. Instead, the -book remains, its spine stays in place, and the open area shows an **empty -terminal placeholder UI** (consistent with normal-mode behavior). The user can -add a tab back via the bottom controls (after opening the book). +Closing the last tab **retires the book from the Shelf**. Its spine disappears; +if the closed book was the one currently open, Shelf auto-advances to the next +remaining book (in Shelf order). The user can bring the book back by clicking +its worktree in the left navigation, which re-opens it and re-adds its spine +with the standard spine-flow animation. + +(Earlier drafts of this doc proposed keeping the book on the shelf with an +empty-terminal placeholder. Reversed: a lingering empty book felt unnatural and +doubled as dead weight. See the Implementation Decisions Journal for the switch.) ### Removing a Book from the Shelf @@ -403,12 +408,21 @@ Shelf-originated taps additionally pass the same animation to `store.send(_, animation:)` so the TCA-side mutation carries the transaction along. -### Close-last-tab behavior - -The empty-book state falls out naturally: `ShelfOpenBookView` already -renders `EmptyTerminalPaneView(message: "No terminals open")` when -`selectedTabId == nil`. The spine remains on the shelf, with its bottom -controls still enabling new tab / split to recover. +### Close-last-tab behavior (revised) + +Reversed from the original decision. Closing the last tab now removes the +book from the Shelf entirely. The implementation: + +- `TerminalClient.Event.tabClosed` gained a `remainingTabs: Int` payload so + AppFeature can detect the last-tab case. When it sees `remainingTabs == 0`, + it dispatches `.repositories(.markWorktreeClosed(id))`. +- The `markWorktreeClosed` reducer handler removes the ID from + `openedWorktreeIDs`, and — only when Shelf is active and the closed + worktree was the open book — auto-advances selection to the next + remaining book (via `shelfBookSelectionEffect`). In normal view, the + selection is left alone so the user's current context isn't disturbed. +- When the closed book was the last book on the Shelf, selection is kept + as-is; `ShelfView` falls through to its "No book selected" empty state. ### Opened-worktrees set diff --git a/supacode/Clients/Terminal/TerminalClient.swift b/supacode/Clients/Terminal/TerminalClient.swift index d7af6afe..751787c0 100644 --- a/supacode/Clients/Terminal/TerminalClient.swift +++ b/supacode/Clients/Terminal/TerminalClient.swift @@ -50,7 +50,7 @@ struct TerminalClient { case notificationReceived(worktreeID: Worktree.ID, title: String, body: String) case notificationIndicatorChanged(count: Int) case tabCreated(worktreeID: Worktree.ID) - case tabClosed(worktreeID: Worktree.ID) + case tabClosed(worktreeID: Worktree.ID, remainingTabs: Int) case focusChanged(worktreeID: Worktree.ID, surfaceID: UUID) case taskStatusChanged(worktreeID: Worktree.ID, status: WorktreeTaskStatus) case runScriptStatusChanged(worktreeID: Worktree.ID, isRunning: Bool) diff --git a/supacode/Features/App/Reducer/AppFeature.swift b/supacode/Features/App/Reducer/AppFeature.swift index 671588a2..0da91a68 100644 --- a/supacode/Features/App/Reducer/AppFeature.swift +++ b/supacode/Features/App/Reducer/AppFeature.swift @@ -1031,6 +1031,12 @@ struct AppFeature { // worktree; other restored worktrees only surface here. return .send(.repositories(.markWorktreeOpened(worktreeID))) + case .terminalEvent(.tabClosed(let worktreeID, let remainingTabs)): + // Closing the last tab retires the book from the Shelf. Other + // closes are routine and need no Reducer-side bookkeeping. + guard remainingTabs == 0 else { return .none } + return .send(.repositories(.markWorktreeClosed(worktreeID))) + case .terminalEvent: return .none } diff --git a/supacode/Features/Repositories/Reducer/RepositoriesFeature.swift b/supacode/Features/Repositories/Reducer/RepositoriesFeature.swift index 1e2bf558..7d3e32d9 100644 --- a/supacode/Features/Repositories/Reducer/RepositoriesFeature.swift +++ b/supacode/Features/Repositories/Reducer/RepositoriesFeature.swift @@ -284,6 +284,7 @@ struct RepositoriesFeature { case selectPreviousShelfBook case selectShelfBook(Int) case markWorktreeOpened(Worktree.ID) + case markWorktreeClosed(Worktree.ID) case setSidebarSelectedWorktreeIDs(Set) case selectRepository(Repository.ID?) case selectWorktree(Worktree.ID?, focusTerminal: Bool = false) @@ -682,6 +683,24 @@ struct RepositoriesFeature { state.openedWorktreeIDs.insert(worktreeID) return .none + case .markWorktreeClosed(let worktreeID): + // Closing the last tab of a book retires the book from the + // Shelf. If this book was the one currently open on the + // Shelf, move focus to the next remaining book (by Shelf + // order) so the user lands on something useful rather than + // an empty-Shelf placeholder. + state.openedWorktreeIDs.remove(worktreeID) + guard state.isShelfActive, + state.selectedTerminalWorktree?.id == worktreeID + else { + return .none + } + let remaining = state.orderedShelfBooks() + guard let next = remaining.first else { + return .none + } + return shelfBookSelectionEffect(for: next) + case .toggleShelf: if state.isShelfActive { state.isShelfActive = false diff --git a/supacode/Features/Terminal/BusinessLogic/WorktreeTerminalManager.swift b/supacode/Features/Terminal/BusinessLogic/WorktreeTerminalManager.swift index 3d4d4610..3f481688 100644 --- a/supacode/Features/Terminal/BusinessLogic/WorktreeTerminalManager.swift +++ b/supacode/Features/Terminal/BusinessLogic/WorktreeTerminalManager.swift @@ -231,8 +231,10 @@ final class WorktreeTerminalManager { state.onTabCreated = { [weak self] in self?.emit(.tabCreated(worktreeID: worktree.id)) } - state.onTabClosed = { [weak self] in - self?.emit(.tabClosed(worktreeID: worktree.id)) + state.onTabClosed = { [weak self, weak state] in + guard let self else { return } + let remaining = state?.tabManager.tabs.count ?? 0 + emit(.tabClosed(worktreeID: worktree.id, remainingTabs: remaining)) } state.onFocusChanged = { [weak self] surfaceID in self?.emit(.focusChanged(worktreeID: worktree.id, surfaceID: surfaceID)) diff --git a/supacodeTests/ShelfFeatureTests.swift b/supacodeTests/ShelfFeatureTests.swift index fb9d5ceb..d9ea5d54 100644 --- a/supacodeTests/ShelfFeatureTests.swift +++ b/supacodeTests/ShelfFeatureTests.swift @@ -434,6 +434,120 @@ struct ShelfFeatureTests { await store.finish() } + @Test(.dependencies) func markWorktreeClosedRemovesFromOpenedSet() async { + let rootURL = URL(fileURLWithPath: "/tmp/repo") + let wt1 = Worktree( + id: "/tmp/repo/wt1", + name: "wt1", + detail: "", + workingDirectory: URL(fileURLWithPath: "/tmp/repo/wt1"), + repositoryRootURL: rootURL + ) + let repo = Repository( + id: rootURL.path(percentEncoded: false), + rootURL: rootURL, + name: "repo", + worktrees: IdentifiedArray(uniqueElements: [wt1]) + ) + var state = RepositoriesFeature.State(repositories: [repo]) + state.openedWorktreeIDs = [wt1.id] + state.selection = nil // Not currently selected, no auto-next needed. + let store = TestStore(initialState: state) { + RepositoriesFeature() + } + + await store.send(.markWorktreeClosed(wt1.id)) { + $0.openedWorktreeIDs = [] + } + await store.finish() + } + + @Test(.dependencies) func markWorktreeClosedAutoAdvancesToNextBookWhenOpen() async { + let rootURL = URL(fileURLWithPath: "/tmp/repo") + let wt1 = Worktree( + id: "/tmp/repo/wt1", + name: "wt1", + detail: "", + workingDirectory: URL(fileURLWithPath: "/tmp/repo/wt1"), + repositoryRootURL: rootURL + ) + let wt2 = Worktree( + id: "/tmp/repo/wt2", + name: "wt2", + detail: "", + workingDirectory: URL(fileURLWithPath: "/tmp/repo/wt2"), + repositoryRootURL: rootURL + ) + let repo = Repository( + id: rootURL.path(percentEncoded: false), + rootURL: rootURL, + name: "repo", + worktrees: IdentifiedArray(uniqueElements: [wt1, wt2]) + ) + var state = RepositoriesFeature.State(repositories: [repo]) + state.repositoryRoots = [rootURL] + state.repositoryOrderIDs = [repo.id] + state.selection = .worktree(wt1.id) + state.isShelfActive = true + state.openedWorktreeIDs = [wt1.id, wt2.id] + let store = TestStore(initialState: state) { + RepositoriesFeature() + } + + // Close wt1 (the open book on the Shelf). wt2 is the only remaining + // book → reducer should auto-select wt2 so the user lands on + // content rather than an empty-Shelf placeholder. + await store.send(.markWorktreeClosed(wt1.id)) { + $0.openedWorktreeIDs = [wt2.id] + } + await store.receive(\.selectWorktree) { + $0.selection = .worktree(wt2.id) + $0.sidebarSelectedWorktreeIDs = [wt2.id] + $0.pendingTerminalFocusWorktreeIDs = [wt2.id] + $0.openedWorktreeIDs = [wt2.id] + } + await store.receive(\.delegate.selectedWorktreeChanged) + await store.finish() + } + + @Test(.dependencies) func markWorktreeClosedLeavesSelectionAloneInNormalView() async { + let rootURL = URL(fileURLWithPath: "/tmp/repo") + let wt1 = Worktree( + id: "/tmp/repo/wt1", + name: "wt1", + detail: "", + workingDirectory: URL(fileURLWithPath: "/tmp/repo/wt1"), + repositoryRootURL: rootURL + ) + let wt2 = Worktree( + id: "/tmp/repo/wt2", + name: "wt2", + detail: "", + workingDirectory: URL(fileURLWithPath: "/tmp/repo/wt2"), + repositoryRootURL: rootURL + ) + let repo = Repository( + id: rootURL.path(percentEncoded: false), + rootURL: rootURL, + name: "repo", + worktrees: IdentifiedArray(uniqueElements: [wt1, wt2]) + ) + var state = RepositoriesFeature.State(repositories: [repo]) + state.selection = .worktree(wt1.id) + state.isShelfActive = false // Normal view. + state.openedWorktreeIDs = [wt1.id, wt2.id] + let store = TestStore(initialState: state) { + RepositoriesFeature() + } + + // In normal view, removing from the opened set must not also steal + // selection away from the user — they are actively on wt1. + await store.send(.markWorktreeClosed(wt1.id)) { + $0.openedWorktreeIDs = [wt2.id] + } + await store.finish() + } + @Test(.dependencies) func markWorktreeOpenedAddsToOpenedSet() async { let rootURL = URL(fileURLWithPath: "/tmp/repo") let wt1 = Worktree( -- 2.51.2