Fix Cmd+N workspace creation snapshot crashes#2173
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough
Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR fixes snapshot-time races that caused crashes during rapid Cmd+N workspace creation by snapshotting stable value data (pin state, working directory, terminal config) before any Bonsplit focus traversal can race with the operation. It also introduces Key changes:
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User as User (Cmd+N)
participant TM as TabManager
participant SS as Snapshot
participant OL as orderedLiveTabs()
User->>TM: addWorkspace(placementOverride)
TM->>SS: workspaceCreationSnapshot()
Note over SS: Capture tabs[], selectedTabId,<br/>selectedTabWasPinned (from snapshot),<br/>preferredWorkingDirectory (cached),<br/>inheritedTerminalConfig (cached panels)
SS-->>TM: WorkspaceCreationSnapshot
TM->>TM: didCaptureWorkspaceCreationSnapshot()
Note over TM: (test seam — live mutations<br/>may happen here in tests)
TM->>OL: orderedLiveWorkspaceCreationTabs(from: snapshot)
Note over OL: Map live tab order → snapshot values.<br/>Return nil if live has tab not in snapshot<br/>(re-entrant add fallback)
OL-->>TM: [WorkspaceCreationTabSnapshot]? (or nil → use snapshot.tabs)
TM->>TM: newTabInsertIndex(snapshot, placement)
Note over TM: Uses live-ordered snapshot tabs<br/>(pin state frozen at snapshot time)
TM->>TM: Insert new Workspace at insertIndex<br/>into current live tabs array
TM-->>User: Workspace
Reviews (1): Last reviewed commit: "Fix Cmd+N workspace creation snapshot ra..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| for terminalPanel in panels.values | ||
| .compactMap({ $0 as? TerminalPanel }) | ||
| .sorted(by: { $0.id.uuidString < $1.id.uuidString }) { | ||
| appendCandidate(terminalPanel) |
There was a problem hiding this comment.
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.
| return workspace.panelDirectories.values.lazy.compactMap { directory in | ||
| self.normalizedWorkingDirectory(directory) | ||
| }.first |
There was a problem hiding this comment.
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.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="cmuxTests/WorkspaceUnitTests.swift">
<violation number="1" location="cmuxTests/WorkspaceUnitTests.swift:458">
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.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| return | ||
| } | ||
|
|
||
| manager.setPinned(first, pinned: true) |
There was a problem hiding this comment.
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>
* Add regression tests for Cmd+N workspace snapshot mutations * Fix Cmd+N workspace creation snapshot races
Ingests all upstream fixes since 2026-03-22 including: - Fix Cmd+N crash: retain snapshot workspaces (manaflow-ai#2183, manaflow-ai#2181, manaflow-ai#2178, manaflow-ai#2173) - Fix browser pane restore after reopen (manaflow-ai#2141) - Fix Ghostty resize_split keybind (manaflow-ai#1899) - Reduce shell integration prompt latency (manaflow-ai#2109) - Fix command palette focus after terminal find (manaflow-ai#2089) - Add Codex CLI hooks (manaflow-ai#2103) - Add cmux.json custom commands (manaflow-ai#2011) - Fix window position restore on relaunch (manaflow-ai#2129) Conflict resolution: - BrowserPanel.swift: accepted upstream configureWebViewConfiguration() refactor (already includes our forMainFrameOnly:true CAPTCHA fix from PR manaflow-ai#1877) Fork-specific files preserved: - Sources/Panels/WebAuthn{Coordinator,BridgeJavaScript}.swift - Sources/FIDO2/module.modulemap - vendor/ctap2 submodule - cmux.entitlements (with camera/audio-input removed) - cmux.embedded.entitlements - .github/workflows/fork-{ci,release}.yml
1 participant
Summary
Testing
./scripts/reload.sh --tag issue2169-cmdn-v2 --launch.Demo Video
For UI or behavior changes, include a short demo video (GitHub upload, Loom, or other direct link).
Review Trigger (Copy/Paste as PR comment)
Checklist
Summary by cubic
Fixes crashes when creating a workspace with Cmd+N by taking a stable snapshot instead of walking live focus state; preserves pin state and correct insertion during rapid changes (fixes #2169).
Written for commit 23ed170. Summary will update on new commits.
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests