From c1998e347c6938645cb2032229d49020b5ceb838 Mon Sep 17 00:00:00 2001 From: austinpower1258 Date: Fri, 13 Mar 2026 11:15:03 -0700 Subject: [PATCH] Fix crash when creating a new workspace via snapshot-based state Snapshot TabManager's published state (tabs, selectedTabId) at the start of workspace creation so mutations don't bounce through Combine-backed accessors mid-creation. Prevents crashes when adding a workspace via Cmd+N, the new-workspace button, or any other creation path that triggers re-entrant publishes. Co-Authored-By: Claude Opus 4.6 --- Sources/TabManager.swift | 86 ++++++++++++++----- .../AppDelegateShortcutRoutingTests.swift | 75 ++++++++++++++++ 2 files changed, 141 insertions(+), 20 deletions(-) diff --git a/Sources/TabManager.swift b/Sources/TabManager.swift index 4a888b14..c7beb573 100644 --- a/Sources/TabManager.swift +++ b/Sources/TabManager.swift @@ -754,6 +754,15 @@ class TabManager: ObservableObject { private var pendingWorkspaceUnfocusTarget: (tabId: UUID, panelId: UUID)? private var sidebarSelectedWorkspaceIds: Set = [] var confirmCloseHandler: ((String, String, Bool) -> Bool)? + private struct WorkspaceCreationSnapshot { + let tabs: [Workspace] + let selectedTabId: UUID? + + var selectedWorkspace: Workspace? { + guard let selectedTabId else { return nil } + return tabs.first(where: { $0.id == selectedTabId }) + } + } #if DEBUG private var debugWorkspaceSwitchCounter: UInt64 = 0 private var debugWorkspaceSwitchId: UInt64 = 0 @@ -927,26 +936,32 @@ class TabManager: ObservableObject { placementOverride: NewWorkspacePlacement? = nil, autoWelcomeIfNeeded: Bool = true ) -> Workspace { - sentryBreadcrumb("workspace.create", data: ["tabCount": tabs.count + 1]) + // Snapshot current published state once so workspace creation doesn't repeatedly + // bounce through Combine-backed accessors while we're preparing the new workspace. + let snapshot = workspaceCreationSnapshot() + let nextTabCount = snapshot.tabs.count + 1 + sentryBreadcrumb("workspace.create", data: ["tabCount": nextTabCount]) let explicitWorkingDirectory = normalizedWorkingDirectory(overrideWorkingDirectory) - let workingDirectory = explicitWorkingDirectory ?? preferredWorkingDirectoryForNewTab() - let inheritedConfig = inheritedTerminalConfigForNewWorkspace() + let workingDirectory = explicitWorkingDirectory ?? preferredWorkingDirectoryForNewTab(snapshot: snapshot) + let inheritedConfig = inheritedTerminalConfigForNewWorkspace(snapshot: snapshot) let ordinal = Self.nextPortOrdinal Self.nextPortOrdinal += 1 let newWorkspace = Workspace( - title: "Terminal \(tabs.count + 1)", + title: "Terminal \(nextTabCount)", workingDirectory: workingDirectory, portOrdinal: ordinal, configTemplate: inheritedConfig ) newWorkspace.owningTabManager = self wireClosedBrowserTracking(for: newWorkspace) - let insertIndex = newTabInsertIndex(placementOverride: placementOverride) - if insertIndex >= 0 && insertIndex <= tabs.count { - tabs.insert(newWorkspace, at: insertIndex) + let insertIndex = newTabInsertIndex(snapshot: snapshot, placementOverride: placementOverride) + var updatedTabs = snapshot.tabs + if insertIndex >= 0 && insertIndex <= updatedTabs.count { + updatedTabs.insert(newWorkspace, at: insertIndex) } else { - tabs.append(newWorkspace) + updatedTabs.append(newWorkspace) } + tabs = updatedTabs if let explicitWorkingDirectory, let terminalPanel = newWorkspace.focusedTerminalPanel { scheduleInitialWorkspaceGitMetadataRefresh( @@ -973,8 +988,8 @@ class TabManager: ObservableObject { #if DEBUG UITestRecorder.incrementInt("addTabInvocations") UITestRecorder.record([ - "tabCount": String(tabs.count), - "selectedTabId": select ? newWorkspace.id.uuidString : (selectedTabId?.uuidString ?? "") + "tabCount": String(updatedTabs.count), + "selectedTabId": select ? newWorkspace.id.uuidString : (snapshot.selectedTabId?.uuidString ?? "") ]) #endif if autoWelcomeIfNeeded && select && !UserDefaults.standard.bool(forKey: WelcomeSettings.shownKey) { @@ -1202,7 +1217,20 @@ class TabManager: ObservableObject { } func terminalPanelForWorkspaceConfigInheritanceSource() -> TerminalPanel? { - guard let workspace = selectedWorkspace else { return nil } + terminalPanelForWorkspaceConfigInheritanceSource(snapshot: workspaceCreationSnapshot()) + } + + private func workspaceCreationSnapshot() -> WorkspaceCreationSnapshot { + WorkspaceCreationSnapshot( + tabs: tabs, + selectedTabId: selectedTabId + ) + } + + private func terminalPanelForWorkspaceConfigInheritanceSource( + snapshot: WorkspaceCreationSnapshot + ) -> TerminalPanel? { + guard let workspace = snapshot.selectedWorkspace else { return nil } if let focusedTerminal = workspace.focusedTerminalPanel { return focusedTerminal } @@ -1217,13 +1245,19 @@ class TabManager: ObservableObject { } private func inheritedTerminalConfigForNewWorkspace() -> ghostty_surface_config_s? { - if let sourceSurface = terminalPanelForWorkspaceConfigInheritanceSource()?.surface.surface { + inheritedTerminalConfigForNewWorkspace(snapshot: workspaceCreationSnapshot()) + } + + private func inheritedTerminalConfigForNewWorkspace( + snapshot: WorkspaceCreationSnapshot + ) -> ghostty_surface_config_s? { + if let sourceSurface = terminalPanelForWorkspaceConfigInheritanceSource(snapshot: snapshot)?.surface.surface { return cmuxInheritedSurfaceConfig( sourceSurface: sourceSurface, context: GHOSTTY_SURFACE_CONTEXT_TAB ) } - if let fallbackFontPoints = selectedWorkspace?.lastRememberedTerminalFontPointsForConfigInheritance() { + if let fallbackFontPoints = snapshot.selectedWorkspace?.lastRememberedTerminalFontPointsForConfigInheritance() { var config = ghostty_surface_config_new() config.font_size = fallbackFontPoints return config @@ -1239,24 +1273,36 @@ class TabManager: ObservableObject { } private func newTabInsertIndex(placementOverride: NewWorkspacePlacement? = nil) -> Int { + newTabInsertIndex(snapshot: workspaceCreationSnapshot(), placementOverride: placementOverride) + } + + private func newTabInsertIndex( + snapshot: WorkspaceCreationSnapshot, + placementOverride: NewWorkspacePlacement? = nil + ) -> Int { let placement = placementOverride ?? WorkspacePlacementSettings.current() - let pinnedCount = tabs.filter { $0.isPinned }.count - let selectedIndex = selectedTabId.flatMap { tabId in - tabs.firstIndex(where: { $0.id == tabId }) + let pinnedCount = snapshot.tabs.filter { $0.isPinned }.count + let selectedIndex = snapshot.selectedTabId.flatMap { tabId in + snapshot.tabs.firstIndex(where: { $0.id == tabId }) } - let selectedIsPinned = selectedIndex.map { tabs[$0].isPinned } ?? false + let selectedIsPinned = selectedIndex.map { snapshot.tabs[$0].isPinned } ?? false return WorkspacePlacementSettings.insertionIndex( placement: placement, selectedIndex: selectedIndex, selectedIsPinned: selectedIsPinned, pinnedCount: pinnedCount, - totalCount: tabs.count + totalCount: snapshot.tabs.count ) } private func preferredWorkingDirectoryForNewTab() -> String? { - guard let selectedTabId, - let tab = tabs.first(where: { $0.id == selectedTabId }) else { + preferredWorkingDirectoryForNewTab(snapshot: workspaceCreationSnapshot()) + } + + private func preferredWorkingDirectoryForNewTab( + snapshot: WorkspaceCreationSnapshot + ) -> String? { + guard let tab = snapshot.selectedWorkspace else { return nil } let focusedDirectory = tab.focusedPanelId diff --git a/cmuxTests/AppDelegateShortcutRoutingTests.swift b/cmuxTests/AppDelegateShortcutRoutingTests.swift index 84775c64..730a660c 100644 --- a/cmuxTests/AppDelegateShortcutRoutingTests.swift +++ b/cmuxTests/AppDelegateShortcutRoutingTests.swift @@ -277,6 +277,71 @@ final class AppDelegateShortcutRoutingTests: XCTestCase { XCTAssertNil(appDelegate.tabManagerFor(windowId: orphanWindowId), "Orphaned context should be pruned after failed resolution") } + func testCustomCmdTNewWorkspacePrunesOrphanedContextWithoutLiveWindow() { + guard let appDelegate = AppDelegate.shared else { + XCTFail("Expected AppDelegate.shared") + return + } + + let existingWindowIds = mainWindowIds() + let orphanWindowId = UUID() + let orphanManager = TabManager() + let orphanSidebarState = SidebarState() + let orphanSidebarSelectionState = SidebarSelectionState() + + autoreleasepool { + var orphanWindow: NSWindow? = NSWindow( + contentRect: NSRect(x: 0, y: 0, width: 320, height: 240), + styleMask: [.titled, .closable, .resizable], + backing: .buffered, + defer: false + ) + orphanWindow?.identifier = NSUserInterfaceItemIdentifier("cmux.main.\(orphanWindowId.uuidString)") + appDelegate.registerMainWindow( + orphanWindow!, + windowId: orphanWindowId, + tabManager: orphanManager, + sidebarState: orphanSidebarState, + sidebarSelectionState: orphanSidebarSelectionState + ) + orphanWindow = nil + } + + RunLoop.main.run(until: Date(timeIntervalSinceNow: 0.05)) + + XCTAssertNil(appDelegate.mainWindow(for: orphanWindowId), "Test precondition: orphaned context should not have a live window") + + let orphanCount = orphanManager.tabs.count + let remappedCmdT = StoredShortcut(key: "t", command: true, shift: false, option: false, control: false) + + withTemporaryShortcut(action: .newTab, shortcut: remappedCmdT) { + guard let event = makeKeyDownEvent( + key: "t", + modifiers: [.command], + keyCode: 17, // kVK_ANSI_T + windowNumber: 0 + ) else { + XCTFail("Failed to construct remapped Cmd+T event") + return + } + +#if DEBUG + XCTAssertTrue(appDelegate.debugHandleCustomShortcut(event: event)) +#else + XCTFail("debugHandleCustomShortcut is only available in DEBUG") +#endif + RunLoop.main.run(until: Date(timeIntervalSinceNow: 0.05)) + } + + XCTAssertEqual(orphanManager.tabs.count, orphanCount, "Orphaned manager must not receive a new workspace from remapped Cmd+T") + XCTAssertNil(appDelegate.tabManagerFor(windowId: orphanWindowId), "Remapped Cmd+T should prune the orphaned context after failed resolution") + + let createdWindowIds = mainWindowIds().subtracting(existingWindowIds) + for windowId in createdWindowIds { + closeWindow(withId: windowId) + } + } + func testCmdDigitRoutesToEventWindowWhenActiveManagerIsStale() { guard let appDelegate = AppDelegate.shared else { XCTFail("Expected AppDelegate.shared") @@ -2378,6 +2443,16 @@ final class AppDelegateShortcutRoutingTests: XCTestCase { return NSApp.windows.first(where: { $0.identifier?.rawValue == identifier }) } + private func mainWindowIds() -> Set { + Set(NSApp.windows.compactMap { window in + guard let raw = window.identifier?.rawValue, + raw.hasPrefix("cmux.main.") else { + return nil + } + return UUID(uuidString: String(raw.dropFirst("cmux.main.".count))) + }) + } + private func closeWindow(withId windowId: UUID) { guard let window = window(withId: windowId) else { return } window.performClose(nil)