Skip to content

Fix Cmd+Z stale undo target crash (#7272) - #12570

Merged
austinywang merged 2 commits into
mainfrom
issue-7272-undo-stack-crash
Sep 14, 2026
Merged

austinywang merged 2 commits into
mainfrom
issue-7272-undo-stack-crash

Conversation

@austinywang

@austinywang austinywang commented Sep 14, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #7272

Cmd+Z could reach AppKit's shared window NSUndoManager after browser or terminal focus churn. WebKit edit commands from a closed view remained on that stack, and AppKit later sent _undoRedoTextOperation: to the stale target.

This PR establishes one ownership boundary and keeps terminal/browser behavior intact:

  • Route Cmd+Z and Cmd+Shift+Z from the NSApplication.sendEvent seam through the focused cmux surface before AppKit menu dispatch.
  • Consume the chord when no cmux surface owns it, while preserving editable AppKit responders and Web Inspector routing.
  • Give every app-embedded WebKit view (browser, Markdown, agent session, and sidebar inspector) a per-view UndoManager; page edit registrations cannot cross view lifetimes.
  • Perform routed WebKit undo/redo on that view's own stack after the page declines the native key equivalent.
  • Add behavior coverage for application-level terminal/neutral responder routing and per-view browser, Markdown, and agent-session undo isolation. The first commit is the failing regression coverage; the second commit contains the fix.

Trade-off: the shared WebKit base class expands the isolation guarantee to every app-owned embedded WKWebView, including views that are not browser panels, because each can host editable content. No user-facing strings or shortcut settings changed.

Validation:

  • python3 scripts/swift_file_length_budget.py (pass; no budget TSV changes)
  • ./scripts/lint-pbxproj-test-wiring.sh (pass)
  • python3 scripts/check-package-resolved-policy.py (pass)
  • Hosted test-e2e.yml and test-depot.yml focused runs were attempted on pushed commits. They reached the macOS compiler but were blocked before test execution by pre-existing origin/main errors in Sources/Surfaces/CmuxTuiSurfaceProvider+CloseTerminal.swift / CmuxTuiSurfaceProviders.swift (private member access and related missing helper diagnostics); no Cmd+Z segfaults the app: EXC_BAD_ACCESS in -[NSUndoManager undoNestedGroup] (stale undo target) #7272 assertion ran.
  • The requested tagged reload was attempted through reload-cloud.sh; the direct backend hostname did not resolve, and the backend-off retry reached the cloud compiler but failed on the same pre-existing CmuxTuiSurfaceProviders.swift error. No local compile or launch was performed.

The source-level regression path is deterministic and the fix is isolated to undo ownership/routing. Runtime dogfood remains blocked by the repository baseline compile failure and cloud runner/backend reachability.

Summary by CodeRabbit

  • Bug Fixes

    • Improved Cmd+Z and Cmd+Shift+Z handling in terminal, browser, agent session, and Markdown views.
    • Undo and redo commands are now routed to the active view before application-level handling.
    • Web-based views now maintain their own undo history, preventing edits from entering the window’s shared undo stack.
    • Undo commands without an applicable target are safely consumed.
  • Tests

    • Added regression coverage for undo routing and view-specific undo behavior.

@github-actions

Copy link
Copy Markdown
Contributor

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@coderabbitai

coderabbitai Bot commented Sep 14, 2026 •

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 715a19f6-b551-410e-aeed-761114b67274

📥 Commits

Reviewing files that changed from the base of the PR and between b5ea44d and da57235.

📒 Files selected for processing (4)
  • Sources/App/WindowKeyDownReplayGuard.swift
  • Sources/AppDelegate.swift
  • Sources/Panels/CmuxWebViewWebContentUndo.swift
  • cmuxTests/CmuxWebViewWebContentUndoTests.swift

Included review availability: Your plan provides up to 10 included reviews per hour; 2 remain after this review.


📝 Walkthrough

Walkthrough

The change adds per-view WebKit undo managers to browser, Markdown, agent session, and extension sidebar views. It routes undo and redo events to focused terminal or WebKit views before AppKit handling and consumes events without an owning surface. Tests cover these paths.

Changes

Undo and redo routing

Layer / File(s) Summary
Reusable WebKit undo manager
Sources/Panels/CmuxWebViewWebContentUndo.swift, Sources/Panels/CmuxWebView.swift, Sources/Panels/MarkdownWebSupport.swift, Sources/Panels/AgentSessionWebView.swift, Sources/ExtensionSidebarWorkspaceRowView.swift
CmuxUndoableWebView owns the per-view undo manager. Browser, Markdown, agent session, and extension sidebar web views use this base class.
Application undo and redo routing
Sources/App/WindowKeyDownReplayGuard.swift, Sources/AppDelegate.swift
Undo and redo command-equivalent events are resolved to the active window and routed to the focused terminal or owning WebKit view before AppKit handling. Events without an owning surface are consumed.
Undo routing regression coverage
cmuxTests/CmuxWebViewWebContentUndoTests.swift
Tests verify terminal routing bypasses the AppKit Undo action, unowned events are consumed, and agent session and Markdown web views use separate web-view undo managers.

Priority: ⬆️ High

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix · Severity of issue fixed: High

Sequence Diagram(s)

sequenceDiagram
  participant NSEvent
  participant NSApplication
  participant NSWindow
  participant CmuxTerminal
  participant CmuxUndoableWebView
  NSEvent->>NSApplication: send Cmd+Z or Cmd+Shift+Z
  NSApplication->>NSWindow: resolve active window and route command-equivalent
  NSWindow->>CmuxTerminal: deliver event when terminal is focused
  NSWindow->>CmuxUndoableWebView: deliver event when WebKit view owns first responder
  NSWindow-->>NSApplication: consume event when no surface owns it
Loading

Merge Risk: ⚪ Minimal · up to da572

No unresolved merge-blocking risk is established by the available evidence.

🚥 Pre-merge checks | ✅ 24 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 35.29% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 17 functions across 7 files. (1 skipped: … Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (24 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The PR meets the coding requirements in [#7272]. cmuxRouteApplicationUndoRedoCommandEquivalent routes Cmd+Z and Cmd+Shift+Z before AppKit menu dispatch. The fallback consumes the event when no termi…
Out of Scope Changes check ✅ Passed The changes stay within [#7272]. Event routing, per-view WebKit undo managers, view adoption, and regression tests directly address stale undo targets and preserve required editing behavior. The provi…
Cmux Swift Actor Isolation ✅ Passed PASS. The production diff adds AppKit/WebKit event routing and a per-view UndoManager; it adds no Codable, Identifiable, Sendable, pure value model, async service protocol, actor, or shared `Sendabl…
Cmux Swift Blocking Runtime ✅ Passed PASS: The production diff adds no semaphore, blocking wait, sleep, delayed dispatch, polling loop, main-queue sync, timer, or manual lock. The while loops in cmuxOwningUndoableWebView(for:) only t…
Cmux Browser Automation Off-Main ✅ Passed PASS: The pull request does not change browser socket automation. The policy-scoped Sources/TerminalController.swift, ControlCommandExecutionPolicy.swift, and `ControlCommandExecutionPolicyTests.s…
Cmux Expensive Synchronous Load ✅ Passed PASS. The PR adds undo-event routing, bounded responder-chain lookup, and per-view UndoManager ownership. The authoritative diff adds no RestorableAgentSessionIndex.load(), agent-store/transcript/…
Cmux Cache Substitution Correctness ✅ Passed PASS. The production diff does not replace a fresh authoritative read with a cached value. It adds a per-view UndoManager and resolves CmuxUndoableWebView through live responder and superview chai…
Cmux No Hacky Sleeps ✅ Passed PASS: The authoritative PR diff changes only Swift files. The rule scope covers TypeScript, JavaScript, shell, and non-Swift build/runtime scripts. No covered non-Swift changes or hacky sleeps appear …
Cmux Algorithmic Complexity ✅ Passed PASS. The production diff adds constant-time event/window selection and per-view UndoManager access. cmuxOwningUndoableWebView walks one responder/view chain; its responder walk has an explicit 64…
Cmux Swift Concurrency ✅ Passed PASS. The authoritative diff adds synchronous AppKit/WebKit routing and per-view undo-manager code. The added lines introduce no DispatchQueue, DispatchGroup, OperationQueue, Task, Combine, completion…
Cmux Swift @Concurrent ✅ Passed PASS: The pull-request diff introduces no async or nonisolated async functions, no @concurrent annotations, and no async helper call sites. The new event-routing, WebKit undo, and responder-reso…
Cmux Swift Package Boundaries ✅ Passed PASS. The changed production feature is AppKit/WebKit UI glue, not independent domain logic. CmuxUndoableWebView subclasses WKWebView, overrides undoManager and keyDown(with:), and uses `NSEve…
Cmux Swiftpm Lockfiles ✅ Passed PASS. The reviewed diff changes only eight Swift source/test files. It does not change Package.swift, Package.resolved, Xcode project package references, .gitignore files, workflows, or dependency dec…
Cmux Swift Logging ✅ Passed PASS: The authoritative Swift diff adds no print, debugPrint, dump, NSLog, ad hoc logging, or Logger declarations. Existing cmuxDebugLog calls remain DEBUG-guarded and unchanged; the existin…
Cmux User-Facing Error Privacy ✅ Passed PASS — The pull-request changes add undo routing and per-view WebKit undo management. The production additions contain no user-facing errors, alerts, command output, recovery copy, or API error bodies…
Cmux Full Internationalization ✅ Passed PASS: The PR adds no production user-facing text, localization keys, string-catalog changes, locale changes, or web message changes. The production diff changes event routing and WebKit class inherita…
Cmux Swiftui State Layout ✅ Passed PASS — The PR does not introduce a prohibited SwiftUI state or layout pattern. Its only SwiftUI-file change replaces WKWebView with CmuxUndoableWebView inside `CmuxExtensionWorkspaceInspectorBrows…
Cmux Architecture Rethink ✅ Passed PASS. The PR introduces no production sleeps, delayed dispatch, polling, locks, notification waits, or side-channel state. CmuxUndoableWebView gives each embedded WebKit view one clear UndoManager…
Cmux Swift Auxiliary Window Close Shortcuts ✅ Passed PASS — The PR does not add or materially change a standalone cmux-owned window. It changes WebKit view classes, routing extensions, and test-only NSWindow fixtures. The existing sidebar inspector wind…
Cmux Source Artifacts ✅ Passed PASS. The authoritative diff changes only eight tracked Swift source and test files under Sources/ and cmuxTests/. All changes are ordinary text modifications; no new files, binary files, logs, sc…
Cmux No Test Or Debug Seam In Production Source ✅ Passed No new test or debug seam was added to production Sources/ code. The added routing methods, CmuxUndoableWebView, cmuxOwningUndoableWebView(for:), and performWebContentUndoRedo(for:) implement …
Cmux No Ambient Global State ✅ Passed PASS. The production diff adds behavior as methods on NSApplication, NSWindow, and CmuxUndoableWebView; it does not add a top-level API function. The new webContentUndoManager is instance stat…
Title check ✅ Passed The title clearly identifies the primary change: fixing the stale undo target crash caused by Cmd+Z.
Description check ✅ Passed The description provides a detailed summary, rationale, implementation scope, trade-off, testing results, and known validation blockers. It does not use the template headings or include the demo video…
Full details: Docstring Coverage

Explanation

Docstring coverage is 35.29% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 17 functions across 7 files. (1 skipped: 1 too large.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch issue-7272-undo-stack-crash

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@austinywang
austinywang force-pushed the issue-7272-undo-stack-crash branch 3 times, most recently from fce8897 to b5ea44d Compare September 14, 2026 02:36
@cursor

cursor Bot commented Sep 14, 2026

Copy link
Copy Markdown

Bugbot is paused — on-demand spend limit reached

Bugbot uses usage-based billing for this team and has hit its on-demand spend limit.

A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue.

@austinywang
austinywang force-pushed the issue-7272-undo-stack-crash branch from b5ea44d to da57235 Compare September 14, 2026 02:46
@austinywang
austinywang force-pushed the issue-7272-undo-stack-crash branch from da57235 to 61eedef Compare September 14, 2026 02:49
@austinywang
austinywang merged commit 4638e5b into main Sep 14, 2026
12 of 13 checks passed
rustybret pushed a commit to rustybret/bmux that referenced this pull request Sep 14, 2026
4638e5b Merge pull request manaflow-ai#12570 from manaflow-ai/issue-7272-undo-stack-crash
61eedef fix: isolate web undo targets before app menu routing
c83dc7f Merge pull request manaflow-ai#12562 from manaflow-ai/issue-12532-agent-notification-flaky
bfe1d3f Merge pull request manaflow-ai#12571 from manaflow-ai/issue-12547-nightly-provider-duplicates-guard
f0d1635 chore: remove Cloud provider Release compile guard
509e806 Merge pull request manaflow-ai#12569 from manaflow-ai/issue-12567-cloud-machine-connectivity
a5410da diagnostics(cloud): correlate machine terminal attachment state
7f6de01 test: reproduce application and markdown undo lifetime crashes
720f24a fix: close contextual signal matcher
50e3416 test: preserve numeric crash signal diagnosis
d21502d test: exercise selected semantic suite reporting
3a0aff2 fix: retain contextual signal crash markers
16b9e75 test: preserve contextual signal crash diagnosis
557bb90 fix: trigger semantic workflow for classifier changes
46f0ba7 test: cover non-crash signal text
3824276 fix: avoid matching build signatures as signals
7079066 test: ignore build signatures in app-host causes

# Conflicts:
#	.github/workflows/agent-notification-tests.yml
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Cmd+Z segfaults the app: EXC_BAD_ACCESS in -[NSUndoManager undoNestedGroup] (stale undo target)

1 participant