Cycle only running workspaces with Cmd-` (#330)

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)):

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:
-    // ⌥⌘<n> 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:
+    // ⌥⌘<n> 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)

graphcode/Tests/WorkspaceCycleTests.swift

@@ -7,10 +7,12 @@ import Testing
 
 /// ⌘` and ⌘⇧` — stepping to the next workspace rather than naming one with ⌥⌘<n>.
 ///
-/// 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<AppFeature> {
+    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])
+  }
 }