Repository navigation
Fix Cmd+N crash: retain snapshot workspaces through creation - #2183
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR modifies the workspace creation flow in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Greptile SummaryThis PR fixes a Release-only use-after-free crash ( Confidence Score: 4/5Safe to merge — the fix is correct and targeted; only non-blocking process and test-coverage observations remain. The
Important Files Changed
Sequence DiagramsequenceDiagram
participant Caller
participant addWorkspace
participant withExtendedLifetime
participant workspaceCreationSnapshot
participant makeWorkspaceForCreation
participant tabs
Caller->>addWorkspace: addWorkspace(placementOverride:)
addWorkspace->>tabs: capturedTabs = tabs
addWorkspace->>tabs: capturedSelectedTabId = selectedTabId
addWorkspace->>withExtendedLifetime: withExtendedLifetime(capturedTabs) { ... }
Note over withExtendedLifetime: capturedTabs pinned alive
withExtendedLifetime->>workspaceCreationSnapshot: snapshot(currentTabs: capturedTabs, currentSelectedTabId:)
workspaceCreationSnapshot-->>withExtendedLifetime: WorkspaceCreationSnapshot
Note over withExtendedLifetime: didCaptureWorkspaceCreationSnapshot()
Note over withExtendedLifetime: mid-creation close may remove workspace from tabs here
withExtendedLifetime->>makeWorkspaceForCreation: makeWorkspaceForCreation(...)
makeWorkspaceForCreation-->>withExtendedLifetime: newWorkspace
withExtendedLifetime->>tabs: updatedTabs = tabs (live array)
withExtendedLifetime->>tabs: tabs = updatedTabs.inserting(newWorkspace, at: insertIndex)
Note over withExtendedLifetime: capturedTabs released here (after closure ends)
withExtendedLifetime-->>addWorkspace: newWorkspace
addWorkspace-->>Caller: newWorkspace
Reviews (1): Last reviewed commit: "fix: repair workspace lifetime regressio..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| XCTAssertEqual(manager.selectedTabId, inserted.id) | ||
| } | ||
|
|
||
| func testAddWorkspaceKeepsCapturedWorkspaceAliveUntilCreationFinishes() { |
There was a problem hiding this comment.
CLAUDE.md requires a strict two-commit structure for regression tests:
- Commit 1 — failing test only (CI goes red)
- Commit 2 — fix (CI goes green)
This PR has three commits for this change:
9cd142a4 test: reproduce Cmd+N snapshot workspace lifetime race09872b62 fix: retain snapshot workspaces through Cmd+N creation80857190 fix: repair workspace lifetime regression test
The third "repair" commit indicates the test needed correction after the fix was already added. This breaks the guarantee that CI history shows the test was provably red on the pre-fix code. The final test (as it exists in HEAD) should have been the test in commit 1 — CI would then clearly confirm it caught the bug before the fix.
Context Used: CLAUDE.md (source)
| var didReachBeforeCreateWorkspace = false | ||
| manager.beforeCreateWorkspace = { | ||
| didReachBeforeCreateWorkspace = true | ||
| XCTAssertNotNil( | ||
| weakClosingWorkspace, | ||
| "Expected the workspace captured before Cmd+N to stay alive until creation finishes" | ||
| ) |
There was a problem hiding this comment.
weak var lifetime check may not fire in Debug unit test runs
The core assertion XCTAssertNotNil(weakClosingWorkspace, ...) is intended to catch a Release ARC optimization that eagerly drops capturedTabs before makeWorkspaceForCreation is reached. Because the optimizer is disabled in Debug builds (the default for xcodebuild -scheme cmux-unit), the old code without withExtendedLifetime would likely also keep the workspace alive here, so the test may pass even against the unfixed implementation.
This is a known limitation of ARC lifetime tests. Consider adding a comment documenting that this assertion exercises Release-only behavior, so future readers don't assume the test provides Debug-mode protection.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmuxTests/WorkspaceUnitTests.swift`:
- Around line 462-470: The strong local capturedClosingWorkspace keeps the
workspace alive until the function scope ends, making the weak-deallocation
assertion non-deterministic; wrap the strong capture in a narrow scope (e.g. a
do { ... } block) so you only extract closingWorkspaceId and create
weakClosingWorkspace inside that block (use capturedClosingWorkspace.id to set
closingWorkspaceId and weak var weakClosingWorkspace = capturedClosingWorkspace
there), then exit the block, set closingWorkspace = nil, and assert
weakClosingWorkspace is nil — this removes the long-lived strong reference from
capturedClosingWorkspace and ensures deterministic deallocation while preserving
the existing assertions on manager.tabs, first.id, and third.id.
In `@Sources/TabManager.swift`:
- Line 1240: Replace the bare literal "Terminal \(nextTabCount)" with a
localized string using the repo pattern — e.g. use String(localized:
"terminal.title.default", defaultValue: "Terminal %d").formatted(nextTabCount)
(or equivalent String(format:) if you prefer) in the TabManager code where the
title is created (the call that currently uses nextTabCount), and add the key
"terminal.title.default" with value "Terminal %d" to
Resources/Localizable.xcstrings so the default workspace title is localized.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c1429e55-35d1-42e0-9d89-f2e78116a67c
📒 Files selected for processing (2)
Sources/TabManager.swiftcmuxTests/WorkspaceUnitTests.swift
| guard let capturedClosingWorkspace = closingWorkspace else { | ||
| XCTFail("Expected secondary workspace") | ||
| return | ||
| } | ||
|
|
||
| let closingWorkspaceId = capturedClosingWorkspace.id | ||
| weak var weakClosingWorkspace = capturedClosingWorkspace | ||
| XCTAssertEqual(manager.tabs.map(\.id), [first.id, closingWorkspaceId, third.id]) | ||
| closingWorkspace = nil |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "WorkspaceUnitTests.swift" | head -5Repository: manaflow-ai/cmux
Length of output: 96
🏁 Script executed:
wc -l ./cmuxTests/WorkspaceUnitTests.swiftRepository: manaflow-ai/cmux
Length of output: 101
🏁 Script executed:
sed -n '451,497p' ./cmuxTests/WorkspaceUnitTests.swiftRepository: manaflow-ai/cmux
Length of output: 1973
🏁 Script executed:
# Check if there are other strong references to the Workspace in the closure captures
sed -n '475,495p' ./cmuxTests/WorkspaceUnitTests.swiftRepository: manaflow-ai/cmux
Length of output: 893
🏁 Script executed:
# Verify the proposed diff syntax by examining the context more carefully
# Check if there are any other uses of capturedClosingWorkspace after line 468
sed -n '462,496p' ./cmuxTests/WorkspaceUnitTests.swift | grep -n "capturedClosingWorkspace"Repository: manaflow-ai/cmux
Length of output: 260
🏁 Script executed:
# Find the Workspace class/struct definition to confirm it's a reference type
rg -t swift 'class Workspace|struct Workspace' --max-count 5Repository: manaflow-ai/cmux
Length of output: 2742
🏁 Script executed:
# Search for the actual Workspace class/struct definition
rg -t swift '^(class|struct) Workspace[^a-zA-Z]' --max-count 10Repository: manaflow-ai/cmux
Length of output: 42
🏁 Script executed:
fd -t f "Workspace.swift" | head -5Repository: manaflow-ai/cmux
Length of output: 83
🏁 Script executed:
head -100 Sources/Workspace.swift | grep -A 3 "^class Workspace\|^struct Workspace"Repository: manaflow-ai/cmux
Length of output: 42
🏁 Script executed:
rg -t swift "class Workspace [:{]|struct Workspace [:{]" Sources/Workspace.swiftRepository: manaflow-ai/cmux
Length of output: 42
🏁 Script executed:
sed -n '1,200p' Sources/Workspace.swift | head -100Repository: manaflow-ai/cmux
Length of output: 3122
🏁 Script executed:
rg -n "^(final )?class Workspace\b" Sources/Workspace.swiftRepository: manaflow-ai/cmux
Length of output: 120
🏁 Script executed:
# Verify the syntax of the proposed refactor by examining similar patterns
rg -A 5 "do \{" cmuxTests/WorkspaceUnitTests.swift | head -20Repository: manaflow-ai/cmux
Length of output: 42
🏁 Script executed:
# Verify the proposed syntax more directly by checking if similar do-block patterns exist
rg -B 2 -A 5 "do \{" Sources/Workspace.swift | head -30Repository: manaflow-ai/cmux
Length of output: 1082
Remove the extra strong local to make the weak-deallocation assertion deterministic.
Line 462–468 introduce capturedClosingWorkspace as a strong local that remains in function scope until line 497. At line 496, the test expects weakClosingWorkspace to be nil, but the strong local still being in scope keeps the workspace alive. In Debug builds, the strong reference prevents deallocation; in Release builds with optimizations, the compiler may elide the unused local, causing non-deterministic behavior.
Scope the strong capture to a narrow block. Extract only closingWorkspaceId and the weak reference outside the block so the workspace can be deallocated before the final assertion.
Proposed refactor
- guard let capturedClosingWorkspace = closingWorkspace else {
- XCTFail("Expected secondary workspace")
- return
- }
-
- let closingWorkspaceId = capturedClosingWorkspace.id
- weak var weakClosingWorkspace = capturedClosingWorkspace
+ let closingWorkspaceId: UUID
+ weak var weakClosingWorkspace: Workspace?
+ do {
+ guard let capturedClosingWorkspace = closingWorkspace else {
+ XCTFail("Expected secondary workspace")
+ return
+ }
+ closingWorkspaceId = capturedClosingWorkspace.id
+ weakClosingWorkspace = capturedClosingWorkspace
+ }
XCTAssertEqual(manager.tabs.map(\.id), [first.id, closingWorkspaceId, third.id])
closingWorkspace = nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmuxTests/WorkspaceUnitTests.swift` around lines 462 - 470, The strong local
capturedClosingWorkspace keeps the workspace alive until the function scope
ends, making the weak-deallocation assertion non-deterministic; wrap the strong
capture in a narrow scope (e.g. a do { ... } block) so you only extract
closingWorkspaceId and create weakClosingWorkspace inside that block (use
capturedClosingWorkspace.id to set closingWorkspaceId and weak var
weakClosingWorkspace = capturedClosingWorkspace there), then exit the block, set
closingWorkspace = nil, and assert weakClosingWorkspace is nil — this removes
the long-lived strong reference from capturedClosingWorkspace and ensures
deterministic deallocation while preserving the existing assertions on
manager.tabs, first.id, and third.id.
| let ordinal = Self.nextPortOrdinal | ||
| Self.nextPortOrdinal += 1 | ||
| let newWorkspace = makeWorkspaceForCreation( | ||
| title: "Terminal \(nextTabCount)", |
There was a problem hiding this comment.
Localize the default workspace title.
"Terminal \(nextTabCount)" is user-visible and will surface in the workspace list/window title, so it should go through the repo’s localization path instead of staying a bare literal.
🌐 Proposed fix
- title: "Terminal \(nextTabCount)",
+ title: String(
+ localized: "workspace.title.default",
+ defaultValue: "Terminal \(nextTabCount)"
+ ),Please also add the key to Resources/Localizable.xcstrings.
As per coding guidelines, "**/*.swift: All user-facing strings must be localized using String(localized: "key.name", defaultValue: "English text") for every string shown in the UI."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/TabManager.swift` at line 1240, Replace the bare literal "Terminal
\(nextTabCount)" with a localized string using the repo pattern — e.g. use
String(localized: "terminal.title.default", defaultValue: "Terminal
%d").formatted(nextTabCount) (or equivalent String(format:) if you prefer) in
the TabManager code where the title is created (the call that currently uses
nextTabCount), and add the key "terminal.title.default" with value "Terminal %d"
to Resources/Localizable.xcstrings so the default workspace title is localized.
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
…w-ai#2183) * test: reproduce Cmd+N snapshot workspace lifetime race * fix: retain snapshot workspaces through Cmd+N creation * fix: repair workspace lifetime regression test
This branch was successfully deployed
1 participant
Summary
withExtendedLifetimeto keep the pre-creationtabsarray alive for the full Cmd+N flowswift_retain) where Release ARC optimizations could drop intermediate retains on workspaces before re-readingtabsfor insertiontabs/selectedTabIdintoworkspaceCreationSnapshot()instead of re-reading@PublishedpropertiesTest plan
testAddWorkspaceKeepsCapturedWorkspaceAliveUntilCreationFinishesverifies the workspace stays alive through creation🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests