Skip to content

Fix Cmd+N workspace creation snapshot crashes #2173

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
austinywang merged 2 commits into from
Mar 25, 2026
Merged

Fix Cmd+N workspace creation snapshot crashes #2173

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
44ead35
Add regression tests for Cmd+N workspace snapshot mutations
austinywang Mar 25, 2026
23ed170
Fix Cmd+N workspace creation snapshot races
austinywang Mar 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 59 additions & 17 deletions Sources/TabManager.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2178,34 +2178,73 @@ class TabManager: ObservableObject {
private func workspaceCreationSnapshot() -> WorkspaceCreationSnapshot {
let currentTabs = tabs
let currentSelectedTabId = selectedTabId
let tabSnapshots = currentTabs.map { WorkspaceCreationTabSnapshot(workspace: $0) }
let selectedTabSnapshot = currentSelectedTabId.flatMap { selectedTabId in
tabSnapshots.first(where: { $0.id == selectedTabId })
}
let selectedWorkspace = currentSelectedTabId.flatMap { selectedTabId in
currentTabs.first(where: { $0.id == selectedTabId })
}

return WorkspaceCreationSnapshot(
tabs: currentTabs.map { WorkspaceCreationTabSnapshot(workspace: $0) },
tabs: tabSnapshots,
selectedTabId: currentSelectedTabId,
selectedTabWasPinned: selectedWorkspace?.isPinned ?? false,
selectedTabWasPinned: selectedTabSnapshot?.isPinned ?? false,
preferredWorkingDirectory: preferredWorkingDirectoryForNewTab(workspace: selectedWorkspace),
inheritedTerminalConfig: inheritedTerminalConfigForNewWorkspace(workspace: selectedWorkspace)
)
}

private func orderedLiveWorkspaceCreationTabs(
from snapshot: WorkspaceCreationSnapshot
) -> [WorkspaceCreationTabSnapshot]? {
let currentTabs = tabs
let snapshotTabsById = Dictionary(uniqueKeysWithValues: snapshot.tabs.map { ($0.id, $0) })
var orderedTabs: [WorkspaceCreationTabSnapshot] = []
orderedTabs.reserveCapacity(currentTabs.count)

for workspace in currentTabs {
guard let tabSnapshot = snapshotTabsById[workspace.id] else {
#if DEBUG
dlog(
"workspace.create.reentrantSnapshotFallback " +
"snapshotCount=\(snapshot.tabs.count) liveCount=\(currentTabs.count)"
)
#endif
return nil
}
orderedTabs.append(tabSnapshot)
}

return orderedTabs
}

private func terminalPanelForWorkspaceConfigInheritanceSource(
workspace: Workspace?
) -> TerminalPanel? {
guard let workspace else { return nil }
if let focusedTerminal = workspace.focusedTerminalPanel {
return focusedTerminal
// Prefer cached/published panel state here instead of walking live Bonsplit focus
// during Cmd+N; rapid workspace creation can observe transient pane/tab selection.
let panels = workspace.panels
var candidates: [TerminalPanel] = []
var seen: Set<UUID> = []

func appendCandidate(_ panel: TerminalPanel?) {
guard let panel, seen.insert(panel.id).inserted else { return }
candidates.append(panel)
}
if let rememberedTerminal = workspace.lastRememberedTerminalPanelForConfigInheritance() {
return rememberedTerminal

appendCandidate(workspace.lastRememberedTerminalPanelForConfigInheritance())
for terminalPanel in panels.values
.compactMap({ $0 as? TerminalPanel })
.sorted(by: { $0.id.uuidString < $1.id.uuidString }) {
appendCandidate(terminalPanel)
Comment on lines +2238 to +2241
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps Bot Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 UUID-string sort is an arbitrary tiebreaker for panel selection

Sorting panels.values by id.uuidString produces a stable, deterministic order across a single run, but because UUIDs are generated randomly the "first" panel in UUID-string order has no semantic relationship to which pane the user was working in. In a workspace with multiple splits, this means config inheritance (font size, environment, etc.) could be drawn from an arbitrary pane rather than the most-recently-used one.

The original code addressed this through a layered semantic priority; the new code collapses all non-remembered panels into a single UUID-sorted bucket. It might be worth preserving at least a coarser semantic ordering — e.g., prefer remembered panel, then any panel with a live surface sorted by most-recently-created (if a creation timestamp is available), before falling back to UUID-order — so the regression under rapid Cmd+N is limited to the crash-avoidance path rather than becoming the normal path for all multi-panel workspaces.

If the UUID sort is intentionally kept as a simple, safe fallback and the remembered-panel path is expected to cover the common case, a short comment to that effect here would help future readers.

}
if let focusedPaneId = workspace.bonsplitController.focusedPaneId,
let paneTerminal = workspace.terminalPanelForConfigInheritance(inPane: focusedPaneId) {
return paneTerminal

if let livePanel = candidates.first(where: { $0.surface.hasLiveSurface && $0.surface.surface != nil }) {
return livePanel
}
return workspace.terminalPanelForConfigInheritance()
return candidates.first
}

private func inheritedTerminalConfigForNewWorkspace() -> ghostty_surface_config_s? {
Expand Down Expand Up @@ -2247,7 +2286,7 @@ class TabManager: ObservableObject {
placementOverride: NewWorkspacePlacement? = nil
) -> Int {
let placement = placementOverride ?? WorkspacePlacementSettings.current()
let liveTabs = tabs.map { WorkspaceCreationTabSnapshot(workspace: $0) }
let liveTabs = orderedLiveWorkspaceCreationTabs(from: snapshot) ?? snapshot.tabs
let pinnedCount = liveTabs.reduce(into: 0) { partial, tab in
if tab.isPinned {
partial += 1
Expand Down Expand Up @@ -2284,12 +2323,15 @@ class TabManager: ObservableObject {
guard let workspace else {
return nil
}
let focusedDirectory = workspace.focusedPanelId
.flatMap { workspace.panelDirectories[$0] }
let candidate = focusedDirectory ?? workspace.currentDirectory
let normalized = normalizeDirectory(candidate)
let trimmed = normalized.trimmingCharacters(in: .whitespacesAndNewlines)
return trimmed.isEmpty ? nil : normalized
// Use cached directory state only; avoiding live focus traversal keeps workspace
// creation resilient when Bonsplit is in the middle of a rapid Cmd+N churn.
if let currentDirectory = normalizedWorkingDirectory(workspace.currentDirectory) {
return currentDirectory
}

return workspace.panelDirectories.values.lazy.compactMap { directory in
self.normalizedWorkingDirectory(directory)
}.first
Comment on lines +2332 to +2334
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps Bot Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Dictionary iteration order is non-deterministic for directory fallback

workspace.panelDirectories.values.lazy.compactMap { ... }.first iterates a Dictionary, whose key order is not guaranteed in Swift. In a workspace with two or more panes open in different directories, the fallback directory for a new Cmd+N workspace will be drawn from a non-deterministic panel rather than the most recently active one.

The original code used focusedPanelId to pick the right directory deterministically. If a stable cached alternative exists (e.g. the most recently assigned directory key, a separate lastRememberedDirectory property, or just the first key in insertion order), anchoring the fallback to that would preserve predictable behavior without reintroducing a live-focus traversal.

}

func moveTabToTop(_ tabId: UUID) {
Expand Down
52 changes: 52 additions & 0 deletions cmuxTests/WorkspaceUnitTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -448,6 +448,58 @@ final class WorkspaceCreationPlacementTests: XCTestCase {
XCTAssertEqual(manager.selectedTabId, inserted.id)
}

func testAddWorkspaceAfterCurrentUsesSnapshotPinnedStateWhenPinningMutatesAfterSnapshot() {
let manager = SnapshotMutatingTabManager()
guard let first = manager.tabs.first else {
XCTFail("Expected initial workspace")
return
}

manager.setPinned(first, pinned: true)
Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai Bot Mar 25, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: This regression test setup is non-discriminating: unpinning the selected pinned workspace yields the same insertion index for both snapshot and live pin-state logic, so the intended bug can regress without failing this test.

Prompt for AI agents 
Check if this issue is valid — if so, understand the root cause and fix it. At cmuxTests/WorkspaceUnitTests.swift, line 458:

<comment>This regression test setup is non-discriminating: unpinning the selected pinned workspace yields the same insertion index for both snapshot and live pin-state logic, so the intended bug can regress without failing this test.</comment>

<file context>
@@ -448,6 +448,58 @@ final class WorkspaceCreationPlacementTests: XCTestCase {
+            return
+        }
+
+        manager.setPinned(first, pinned: true)
+        let second = manager.addWorkspace()
+        let third = manager.addWorkspace()
</file context>
Fix with Cubic

let second = manager.addWorkspace()
let third = manager.addWorkspace()
manager.selectWorkspace(first)
let baselineOrder = manager.tabs.map(\.id)

manager.afterCaptureWorkspaceCreationSnapshot = {
manager.setPinned(first, pinned: false)
}

let inserted = manager.addWorkspace(placementOverride: .afterCurrent)

XCTAssertEqual(manager.tabs.map(\.id).filter { $0 != inserted.id }, baselineOrder)
XCTAssertEqual(manager.tabs.map(\.id), [first.id, inserted.id, second.id, third.id])
XCTAssertEqual(manager.selectedTabId, inserted.id)
}

func testAddWorkspaceAfterCurrentFollowsLiveReorderUsingSnapshotTabValues() {
let manager = SnapshotMutatingTabManager()
guard let first = manager.tabs.first else {
XCTFail("Expected initial workspace")
return
}

let second = manager.addWorkspace()
let third = manager.addWorkspace()
manager.selectWorkspace(second)

manager.afterCaptureWorkspaceCreationSnapshot = {
XCTAssertTrue(
manager.reorderWorkspace(tabId: third.id, toIndex: 0),
"Expected to reorder live workspaces after the snapshot is captured"
)
}

let inserted = manager.addWorkspace(placementOverride: .afterCurrent)

XCTAssertEqual(
manager.tabs.map(\.id).filter { $0 != inserted.id },
[third.id, first.id, second.id]
)
XCTAssertEqual(manager.tabs.map(\.id), [third.id, first.id, second.id, inserted.id])
XCTAssertEqual(manager.selectedTabId, inserted.id)
}

private func makeManagerWithThreeWorkspaces() -> TabManager {
let manager = TabManager()
_ = manager.addWorkspace()
Expand Down
Loading