From 6986082376702b69dea733cddffb50f5f0191b1e Mon Sep 17 00:00:00 2001 From: scgopi Date: Tue, 8 Sep 2026 13:53:54 -0700 Subject: [PATCH 1/2] WIP: Cmd-` cycles only running workspaces (#330) Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_012jEwiawXZxVpaKWKBLyzJU --- graphcode/Sources/Features/Workspaces/WorkspaceSwitcher.swift | 1 + 1 file changed, 1 insertion(+) diff --git a/graphcode/Sources/Features/Workspaces/WorkspaceSwitcher.swift b/graphcode/Sources/Features/Workspaces/WorkspaceSwitcher.swift index 0d800bd7..ce9cff4e 100644 --- a/graphcode/Sources/Features/Workspaces/WorkspaceSwitcher.swift +++ b/graphcode/Sources/Features/Workspaces/WorkspaceSwitcher.swift @@ -286,3 +286,4 @@ struct WorkspaceSwitcherPanel: View { return summary.isOpen ? "\(loops) · open" : "\(loops) · not running" } } + From e812beb654e2ed9cb120b2d90de4087f302cf0bd Mon Sep 17 00:00:00 2001 From: scgopi Date: Tue, 8 Sep 2026 18:20:09 -0700 Subject: [PATCH 2/2] Cycle only running workspaces with Cmd-` (#330) cycleRequested walked every workspace on disk and handed the next one to switchRequested, whose open launches an instance when none holds the pid file. So the workspace someone had just quit was next in creation order and Cmd-` brought it back. Filter the walk by the workspaces otherOpen reports, the same signal the update-install path trusts, and wrap within that subset. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_012jEwiawXZxVpaKWKBLyzJU --- .../Features/App/AppFeature+Workspaces.swift | 21 ++++-- .../Workspaces/WorkspaceSwitcher.swift | 8 +- graphcode/Tests/WorkspaceCycleTests.swift | 75 +++++++++++++++++-- 3 files changed, 89 insertions(+), 15 deletions(-) diff --git a/graphcode/Sources/Features/App/AppFeature+Workspaces.swift b/graphcode/Sources/Features/App/AppFeature+Workspaces.swift index 27fa8231..80951327 100644 --- a/graphcode/Sources/Features/App/AppFeature+Workspaces.swift +++ b/graphcode/Sources/Features/App/AppFeature+Workspaces.swift @@ -136,8 +136,9 @@ extension AppFeature { case createConfirmed /// Raise (or launch) the instance that owns this workspace. case switchRequested(Workspace) - /// ⌘` and ⌘⇧` — the workspace `offset` places along from this one in creation order, - /// wrapping at both ends. + /// ⌘` and ⌘⇧` — the *running* workspace `offset` places along from this one in + /// creation order, wrapping at both ends. One with no instance is stepped over, never + /// launched. case cycleRequested(offset: Int) case switcherPresented(Bool) case manageRequested @@ -311,12 +312,20 @@ struct AppWorkspacesReducer: Reducer { // workspace another instance created since then is one ⌘` would skip over. let known = workspaces.list() state.workspaces.known = known - guard known.count > 1, - let index = known.firstIndex(where: { $0.id == state.workspaces.current.id }) + // Only the workspaces with a window. ⌘` is the system's cycle-*windows* key, and + // `switchRequested` launches whatever has no instance — so walking the whole + // list brought back the workspace someone had just quit (issue #330). The same + // question the update path asks before swapping the bundle, answered by the same + // client, filtered through `known` so creation order is kept. + let current = state.workspaces.current + let open = Set(workspaces.otherOpen().map(\.id)).union([current.id]) + let running = known.filter { open.contains($0.id) } + guard running.count > 1, + let index = running.firstIndex(where: { $0.id == current.id }) else { return .none } // `%` keeps a negative dividend negative in Swift, so ⌘⇧` at the head of the - // list would index -1 without the extra `+ known.count`. - let next = known[((index + offset) % known.count + known.count) % known.count] + // list would index -1 without the extra `+ running.count`. + let next = running[((index + offset) % running.count + running.count) % running.count] return .send(.workspaces(.switchRequested(next))) case .workspaces(.renameRequested(let workspace)): diff --git a/graphcode/Sources/Features/Workspaces/WorkspaceSwitcher.swift b/graphcode/Sources/Features/Workspaces/WorkspaceSwitcher.swift index ce9cff4e..a53569c9 100644 --- a/graphcode/Sources/Features/Workspaces/WorkspaceSwitcher.swift +++ b/graphcode/Sources/Features/Workspaces/WorkspaceSwitcher.swift @@ -35,9 +35,10 @@ struct WorkspaceMenuItems: View { .modifier(WorkspaceShortcut(index: showsShortcuts ? index : nil)) } Divider() - // ⌘` walks the same list the rows above are numbered in, which is the point of it: - // ⌥⌘ is for the workspace you can name, ⌘` for the next one along when you have - // more of them than you have fingers. + // ⌘` walks the list the rows above are numbered in, skipping any without a window: + // ⌥⌘ is for the workspace you can name, ⌘` for the next *open* one along when you + // have more of them than you have fingers. It never launches one — that is what the + // rows are for. Button("Next Workspace") { store.send(.workspaces(.cycleRequested(offset: 1))) } .modifier(CycleShortcut(isEnabled: showsShortcuts, isBackwards: false)) .disabled(store.workspaces.known.count < 2) @@ -286,4 +287,3 @@ struct WorkspaceSwitcherPanel: View { return summary.isOpen ? "\(loops) · open" : "\(loops) · not running" } } - diff --git a/graphcode/Tests/WorkspaceCycleTests.swift b/graphcode/Tests/WorkspaceCycleTests.swift index 0745b3e4..d9161531 100644 --- a/graphcode/Tests/WorkspaceCycleTests.swift +++ b/graphcode/Tests/WorkspaceCycleTests.swift @@ -7,10 +7,12 @@ import Testing /// ⌘` and ⌘⇧` — stepping to the next workspace rather than naming one with ⌥⌘. /// -/// Issue #175. The list these walk is `Workspace.all()`, which is in creation order; what -/// is pinned here is the walking itself — that it wraps at both ends, that it re-reads the -/// list rather than trusting whatever the last menu opening left in state, and that a -/// machine with one workspace gets nothing rather than a switch to itself. +/// Issue #175. The list these walk is `Workspace.all()`, which is in creation order, +/// narrowed to the workspaces that have a window (issue #330 — the ones the update path's +/// `otherOpen` reports, plus this one). What is pinned here is the walking itself — that +/// it wraps at both ends, that it re-reads the list rather than trusting whatever the last +/// menu opening left in state, that a machine with one workspace gets nothing rather than +/// a switch to itself, and that a workspace someone quit stays quit. @Suite struct WorkspaceCycleTests { private func workspace(_ slug: String) -> Workspace { @@ -23,14 +25,19 @@ struct WorkspaceCycleTests { return state } + /// `list` is what is on disk; `running` is which of the *others* have an instance — + /// `nil` means all of them, the pre-#330 world where the two were never told apart. @MainActor private func store( - current: Workspace, list: [Workspace], opened: LockIsolated<[Workspace]> + current: Workspace, list: [Workspace], running: [Workspace]? = nil, + opened: LockIsolated<[Workspace]> ) -> TestStoreOf { + let others = running ?? list.filter { $0.id != current.id } let store = TestStore(initialState: state(current: current)) { AppFeature() } withDependencies: { $0.workspaceClient.list = { list } + $0.workspaceClient.otherOpen = { others } $0.workspaceClient.open = { workspace in opened.withValue { $0.append(workspace) } } } store.exhaustivity = .off @@ -108,4 +115,62 @@ struct WorkspaceCycleTests { #expect(opened.value == [work]) #expect(store.state.workspaces.known == [.default, work]) } + + // MARK: - Issue #330: only running workspaces are cycled + + @Test + @MainActor + func aWorkspaceWithNoWindowIsSteppedOver() async { + // `work` is on disk and next in creation order, but nobody has it open. ⌘` must land + // on `oss` — going to `work` would mean launching it, which is the bug. + let work = workspace("work") + let oss = workspace("oss") + let opened = LockIsolated<[Workspace]>([]) + let store = store( + current: .default, list: [.default, work, oss], running: [oss], opened: opened) + + await store.send(.workspaces(.cycleRequested(offset: 1))) + await store.receive(\.workspaces.switchRequested) + + #expect(opened.value == [oss]) + } + + @Test + @MainActor + func nothingElseRunningMeansNowhereToGo() async { + // The other workspaces exist, and every one of them was quit. ⌘` does nothing rather + // than resurrecting the first of them. + let work = workspace("work") + let oss = workspace("oss") + let opened = LockIsolated<[Workspace]>([]) + let store = store( + current: .default, list: [.default, work, oss], running: [], opened: opened) + + await store.send(.workspaces(.cycleRequested(offset: 1))) + await store.send(.workspaces(.cycleRequested(offset: -1))) + + #expect(opened.value.isEmpty) + } + + @Test + @MainActor + func wrappingCountsTheRunningOnesNotTheOnesOnDisk() async { + // Four on disk, two of them dead: from the last running one, ⌘` wraps to the first + // running one, and ⌘⇧` from there comes straight back. An index taken from the disk + // list with a modulus of the running count — or the other way round — lands on a dead + // workspace or off the end. + let work = workspace("work") + let oss = workspace("oss") + let home = workspace("home") + let opened = LockIsolated<[Workspace]>([]) + let store = store( + current: home, list: [.default, work, oss, home], running: [work], opened: opened) + + await store.send(.workspaces(.cycleRequested(offset: 1))) + await store.receive(\.workspaces.switchRequested) + await store.send(.workspaces(.cycleRequested(offset: -1))) + await store.receive(\.workspaces.switchRequested) + + #expect(opened.value == [work, work]) + } }