diff --git a/Sources/App/WindowKeyDownReplayGuard.swift b/Sources/App/WindowKeyDownReplayGuard.swift index 02b0f456d1b9..d9494369403a 100644 --- a/Sources/App/WindowKeyDownReplayGuard.swift +++ b/Sources/App/WindowKeyDownReplayGuard.swift @@ -43,12 +43,41 @@ private struct CmuxForceDispatchedKeyEventIdentity: Hashable { /// dispatch has fully unwound) is still force-dispatched normally. private var cmuxInFlightForceDispatchedKeyEventIdentities = Set() +extension NSApplication { + /// Routes undo and redo before AppKit can invoke the shared window undo + /// manager, whose registrations may belong to a view that was closed. + func cmuxRouteApplicationUndoRedoCommandEquivalent(_ event: NSEvent) -> Bool { + let window = event.window + ?? AppDelegate.shared?.shortcutRoutingActiveWindow + ?? keyWindow + ?? mainWindow + return window?.cmuxRouteApplicationUndoRedoCommandEquivalent(event) == true + } +} + extension NSWindow { + /// Routes an application undo or redo command through the focused cmux + /// surface, keeping AppKit's shared window undo stack out of the path. + func cmuxRouteApplicationUndoRedoCommandEquivalent(_ event: NSEvent) -> Bool { + guard event.cmuxIsUndoRedoCommandEquivalent else { return false } + + let terminalView = firstResponder.cmuxTerminalKeyEquivalentOwningGhosttyView() + let webView = firstResponder.flatMap { + NSWindow.cmuxOwningWebView(for: $0, in: self, event: event) + } ?? firstResponder.flatMap { cmuxOwningUndoableWebView(for: $0) } + return cmuxRouteUndoRedoCommandEquivalentAwayFromAppKit( + event, + terminalView: terminalView, + webView: webView, + webKitKeyDownReentry: webView?.browserNativeInputDeliveryOwner.isDispatchActive ?? false + ) + } + func cmuxRouteUndoRedoCommandEquivalentAwayFromAppKit( _ event: NSEvent, terminalView: GhosttyNSView?, - webView: CmuxWebView?, - browserWebKitKeyDownReentry: Bool + webView: CmuxUndoableWebView?, + webKitKeyDownReentry: Bool ) -> Bool { guard event.cmuxIsUndoRedoCommandEquivalent, !cmuxFirstResponderPreservesLocalUndoRedo, @@ -71,7 +100,7 @@ extension NSWindow { return true } if let webView { - if browserWebKitKeyDownReentry { + if webKitKeyDownReentry { #if DEBUG cmuxDebugLog(" -> undo/redo browser reentry suppressed before AppKit menu") #endif @@ -94,7 +123,10 @@ extension NSWindow { // editable AppKit responders and Web Inspector are exempted above. return true } - return false + // No cmux surface owns an application-level undo operation here. Do + // not expose the shared window stack to an unknown target: it can + // contain an edit command whose view was already torn down. + return true } private var cmuxFirstResponderPreservesLocalUndoRedo: Bool { diff --git a/Sources/AppDelegate.swift b/Sources/AppDelegate.swift index 560c33e0f36a..6a036520c575 100644 --- a/Sources/AppDelegate.swift +++ b/Sources/AppDelegate.swift @@ -19034,7 +19034,7 @@ private extension NSApplication { if ShortcutRecorderEventRouter.dispatchActiveRecordingEvent( event, preferredWindow: event.window ?? AppDelegate.shared?.shortcutRoutingActiveWindow ?? keyWindow ?? mainWindow - ) { + ) || cmuxRouteApplicationUndoRedoCommandEquivalent(event) { return } if AppDelegate.shared?.shouldSuppressStaleCmuxMenuShortcut(event: event) == true { @@ -19136,7 +19136,6 @@ extension AppDelegate { allBrowserPanelsForInspectorWindowClose() } } - private extension NSWindow { static func cmuxCommandPaletteOwnsFieldEditor(_ textView: NSTextView?, in window: NSWindow) -> Bool { guard let textView, @@ -19546,7 +19545,7 @@ private extension NSWindow { } return false } - if cmuxRouteUndoRedoCommandEquivalentAwayFromAppKit(event, terminalView: firstResponderGhosttyView, webView: firstResponderWebView, browserWebKitKeyDownReentry: browserWebKitKeyDownReentry) { return true } + if cmuxRouteUndoRedoCommandEquivalentAwayFromAppKit(event, terminalView: firstResponderGhosttyView, webView: firstResponderWebView, webKitKeyDownReentry: browserWebKitKeyDownReentry) { return true } if let mode = AppDelegate.shared?.rightSidebarModeShortcut(for: event), AppDelegate.shared?.shouldRouteRightSidebarModeShortcut(in: self) == true { _ = AppDelegate.shared?.focusRightSidebarInActiveMainWindow( @@ -19955,7 +19954,7 @@ private extension NSWindow { return nil } - private static func cmuxOwningWebView( + fileprivate static func cmuxOwningWebView( for responder: NSResponder, in window: NSWindow, event: NSEvent? diff --git a/Sources/ExtensionSidebarWorkspaceRowView.swift b/Sources/ExtensionSidebarWorkspaceRowView.swift index 6f4475250692..953da2b13804 100644 --- a/Sources/ExtensionSidebarWorkspaceRowView.swift +++ b/Sources/ExtensionSidebarWorkspaceRowView.swift @@ -283,7 +283,7 @@ struct CmuxExtensionWorkspaceInspectorBrowserView: NSViewRepresentable { func makeNSView(context: Context) -> WKWebView { let configuration = WKWebViewConfiguration() - let webView = WKWebView(frame: .zero, configuration: configuration) + let webView = CmuxUndoableWebView(frame: .zero, configuration: configuration) webView.setValue(false, forKey: "drawsBackground") return webView } diff --git a/Sources/Panels/AgentSessionWebView.swift b/Sources/Panels/AgentSessionWebView.swift index 086ee7015c0e..c91002b3e7b4 100644 --- a/Sources/Panels/AgentSessionWebView.swift +++ b/Sources/Panels/AgentSessionWebView.swift @@ -2,7 +2,7 @@ import AppKit import WebKit @MainActor -final class AgentSessionWebView: WKWebView { +final class AgentSessionWebView: CmuxUndoableWebView { var onPointerDown: (() -> Void)? override func acceptsFirstMouse(for event: NSEvent?) -> Bool { diff --git a/Sources/Panels/CmuxWebView.swift b/Sources/Panels/CmuxWebView.swift index da64b513c295..40807a5a5c54 100644 --- a/Sources/Panels/CmuxWebView.swift +++ b/Sources/Panels/CmuxWebView.swift @@ -9,7 +9,7 @@ import WebKit /// Route app/menu shortcuts first, but allow browser content to try browser-local /// Find shortcuts. The configured shortcut stays app-owned so cmux can choose browser /// find or right-sidebar file search from the current focus owner. -final class CmuxWebView: WKWebView { +final class CmuxWebView: CmuxUndoableWebView { var browserViewportModel: BrowserViewportModel? var onBrowserViewportHierarchyChanged: (() -> Void)? @@ -38,12 +38,6 @@ final class CmuxWebView: WKWebView { clearTrustedInternalNavigationGrants() } - // WebKit registers web-content edit commands on the view's `undoManager`; - // owning one per web view keeps every page's undo stack scoped to this - // view's lifetime instead of the window's shared undo manager. - // See CmuxWebViewWebContentUndo.swift. - let webContentUndoManager = UndoManager() - // Some sites/WebKit paths report middle-click link activations as // WKNavigationAction.buttonNumber=4 instead of 2. Track a recent local // middle-click so navigation delegates can recover intent reliably. diff --git a/Sources/Panels/CmuxWebViewWebContentUndo.swift b/Sources/Panels/CmuxWebViewWebContentUndo.swift index b2d77691990e..c4e1b313ccff 100644 --- a/Sources/Panels/CmuxWebViewWebContentUndo.swift +++ b/Sources/Panels/CmuxWebViewWebContentUndo.swift @@ -1,6 +1,57 @@ import AppKit +import WebKit -// Cmd+Z / Cmd+Shift+Z for browser web views. +/// Base class for every WebKit view that cmux embeds in an app window. +/// +/// WebKit registers page edit commands through the view's `undoManager`. +/// `NSResponder` otherwise resolves that property to the window's shared +/// manager, which lets commands from a closed web view remain in a different +/// object's undo stack and later message a dangling target. +class CmuxUndoableWebView: WKWebView { + let webContentUndoManager = UndoManager() + + override var undoManager: UndoManager? { webContentUndoManager } + + /// Completes a routed WebKit undo command after the page declines it. + override func keyDown(with event: NSEvent) { + if performWebContentUndoRedo(for: event) { + return + } + super.keyDown(with: event) + } +} + +extension NSWindow { + /// Finds an embedded WebKit view from a responder without depending on + /// browser-only panel state. Agent-session and Markdown views use this + /// path when their web content owns first responder. + func cmuxOwningUndoableWebView(for responder: NSResponder) -> CmuxUndoableWebView? { + if let webView = responder as? CmuxUndoableWebView { + return webView + } + if let view = responder as? NSView { + var current: NSView? = view.superview + while let candidate = current { + if let webView = candidate as? CmuxUndoableWebView { + return webView + } + current = candidate.superview + } + } + var current = responder.nextResponder + var hops = 0 + while let next = current, hops < 64 { + if let webView = next as? CmuxUndoableWebView { + return webView + } + current = next.nextResponder + hops += 1 + } + return nil + } +} + +// Cmd+Z / Cmd+Shift+Z for embedded WebKit views. // // WebKit registers every web-content edit command on the web view's // `undoManager` (`WebViewImpl::registerEditCommand` calls @@ -14,15 +65,13 @@ import AppKit // nothing performed the command anymore // (https://github.com/manaflow-ai/cmux/issues/9677). // -// Owning an undo manager per web view restores both invariants: +// Owning an undo manager per WebKit view restores both invariants: // - web-content edit commands never reach the window's shared undo manager // and never outlive their web view, so the stale-target class of crashes is // impossible by construction; // - the routed chords can perform undo/redo directly on the focused web // view's own stack, matching Safari's Edit-menu behavior. -extension CmuxWebView { - override var undoManager: UndoManager? { webContentUndoManager } - +extension CmuxUndoableWebView { /// Performs the page's native editing undo/redo for a routed /// Cmd+Z / Cmd+Shift+Z chord. /// diff --git a/Sources/Panels/MarkdownWebSupport.swift b/Sources/Panels/MarkdownWebSupport.swift index da7b53634cb5..1d7acf304d81 100644 --- a/Sources/Panels/MarkdownWebSupport.swift +++ b/Sources/Panels/MarkdownWebSupport.swift @@ -174,7 +174,7 @@ final class MarkdownWebRenderingCoordinator { } @MainActor -final class MarkdownWebView: WKWebView { +final class MarkdownWebView: CmuxUndoableWebView { var onPointerDown: (() -> Void)? /// Invoked after this view is attached to a window. Keep this separate /// from pointer focus so a panel can complete a focus request made before diff --git a/cmuxTests/CmuxWebViewWebContentUndoTests.swift b/cmuxTests/CmuxWebViewWebContentUndoTests.swift index cfb16af904bb..3340ba461349 100644 --- a/cmuxTests/CmuxWebViewWebContentUndoTests.swift +++ b/cmuxTests/CmuxWebViewWebContentUndoTests.swift @@ -1,5 +1,6 @@ import AppKit import Carbon.HIToolbox +import CmuxTerminal import Testing import WebKit import ObjectiveC.runtime @@ -71,8 +72,186 @@ private final class WebContentUndoSpy { var redoCount = 0 } +private final class ApplicationUndoTerminalProbe: GhosttyNSView { + private(set) var afterMenuMissEvents: [NSEvent] = [] + + override func performKeyEquivalentAfterMenuMiss(with event: NSEvent) -> Bool { + afterMenuMissEvents.append(event) + return true + } +} + +private final class ApplicationUndoMenuProbe: NSObject { + private(set) var callCount = 0 + + @objc func perform(_ sender: Any?) { + _ = sender + callCount += 1 + } +} + +private final class ApplicationUndoNeutralResponder: NSView { + override var acceptsFirstResponder: Bool { true } +} + @Suite(.serialized) final class CmuxWebViewWebContentUndoTests { + @Test + @MainActor + func applicationSendEventRoutesTerminalUndoBeforeAppKitMenu() throws { + _ = NSApplication.shared + AppDelegate.installWindowResponderSwizzlesForTesting() + + let menuProbe = ApplicationUndoMenuProbe() + let previousMenu = NSApp.mainMenu + let menu = NSMenu(title: "Main") + let undoItem = NSMenuItem( + title: "Undo", + action: #selector(ApplicationUndoMenuProbe.perform(_:)), + keyEquivalent: "z" + ) + undoItem.keyEquivalentModifierMask = [.command] + undoItem.target = menuProbe + menu.addItem(undoItem) + NSApp.mainMenu = menu + defer { NSApp.mainMenu = previousMenu } + + let window = NSWindow( + contentRect: NSRect(x: 0, y: 0, width: 640, height: 420), + styleMask: [.titled, .closable], + backing: .buffered, + defer: false + ) + window.isReleasedWhenClosed = false + let container = NSView(frame: window.contentRect(forFrameRect: window.frame)) + window.contentView = container + let terminal = ApplicationUndoTerminalProbe(frame: NSRect(x: 0, y: 0, width: 64, height: 32)) + container.addSubview(terminal) + #expect(window.makeFirstResponder(terminal)) + window.makeKeyAndOrderFront(nil) + defer { + window.orderOut(nil) + window.close() + } + + let event = try #require(NSEvent.keyEvent( + with: .keyDown, + location: .zero, + modifierFlags: [.command], + timestamp: ProcessInfo.processInfo.systemUptime, + windowNumber: window.windowNumber, + context: nil, + characters: "z", + charactersIgnoringModifiers: "z", + isARepeat: false, + keyCode: UInt16(kVK_ANSI_Z) + )) + NSApp.sendEvent(event) + + #expect(menuProbe.callCount == 0) + #expect(terminal.afterMenuMissEvents.map { $0.charactersIgnoringModifiers } == ["z"]) + } + + @Test + @MainActor + func applicationSendEventConsumesUndoWithoutAnOwningSurface() throws { + _ = NSApplication.shared + AppDelegate.installWindowResponderSwizzlesForTesting() + + let menuProbe = ApplicationUndoMenuProbe() + let previousMenu = NSApp.mainMenu + let menu = NSMenu(title: "Main") + let undoItem = NSMenuItem( + title: "Undo", + action: #selector(ApplicationUndoMenuProbe.perform(_:)), + keyEquivalent: "z" + ) + undoItem.keyEquivalentModifierMask = [.command] + undoItem.target = menuProbe + menu.addItem(undoItem) + NSApp.mainMenu = menu + defer { NSApp.mainMenu = previousMenu } + + let window = NSWindow( + contentRect: NSRect(x: 0, y: 0, width: 640, height: 420), + styleMask: [.titled, .closable], + backing: .buffered, + defer: false + ) + window.isReleasedWhenClosed = false + let responder = ApplicationUndoNeutralResponder(frame: NSRect(x: 0, y: 0, width: 64, height: 32)) + window.contentView = responder + #expect(window.makeFirstResponder(responder)) + window.makeKeyAndOrderFront(nil) + defer { + window.orderOut(nil) + window.close() + } + + let event = try #require(NSEvent.keyEvent( + with: .keyDown, + location: .zero, + modifierFlags: [.command], + timestamp: ProcessInfo.processInfo.systemUptime, + windowNumber: window.windowNumber, + context: nil, + characters: "z", + charactersIgnoringModifiers: "z", + isARepeat: false, + keyCode: UInt16(kVK_ANSI_Z) + )) + NSApp.sendEvent(event) + + #expect(menuProbe.callCount == 0) + } + + @Test + @MainActor + func agentSessionWebContentUndoManagerIsScopedPerWebView() throws { + _ = NSApplication.shared + + let window = NSWindow( + contentRect: NSRect(x: 0, y: 0, width: 640, height: 420), + styleMask: [.titled, .closable], + backing: .buffered, + defer: false + ) + window.isReleasedWhenClosed = false + let agentWebView = AgentSessionWebView( + frame: NSRect(x: 0, y: 0, width: 640, height: 420), + configuration: WKWebViewConfiguration() + ) + window.contentView = agentWebView + window.makeKeyAndOrderFront(nil) + defer { + agentWebView.removeFromSuperview() + window.orderOut(nil) + window.close() + } + + let webViewUndoManager = try #require(agentWebView.undoManager) + #expect(webViewUndoManager !== window.undoManager) + let spy = WebContentUndoSpy() + webViewUndoManager.registerUndo(withTarget: spy) { $0.undoCount += 1 } + #expect(webViewUndoManager.canUndo) + #expect(window.undoManager?.canUndo == false) + + let event = try #require(NSEvent.keyEvent( + with: .keyDown, + location: .zero, + modifierFlags: [.command], + timestamp: ProcessInfo.processInfo.systemUptime, + windowNumber: window.windowNumber, + context: nil, + characters: "z", + charactersIgnoringModifiers: "z", + isARepeat: false, + keyCode: UInt16(kVK_ANSI_Z) + )) + #expect(window.cmuxRouteApplicationUndoRedoCommandEquivalent(event)) + #expect(spy.undoCount == 1) + } + @Test @MainActor func browserCmdZPerformsWebContentUndoWhenPageDeclinesTheChord() throws { @@ -147,6 +326,46 @@ final class CmuxWebViewWebContentUndoTests { } } + /// Every WebKit view embedded by cmux must keep page edit commands out of + /// the window's shared undo stack. Markdown previews use a distinct + /// `WKWebView` subclass, so covering only `CmuxWebView` would leave the + /// stale-target lifetime bug reachable through that surface. + @Test + @MainActor + func markdownWebContentUndoManagerIsScopedPerWebView() throws { + _ = NSApplication.shared + + let window = NSWindow( + contentRect: NSRect(x: 0, y: 0, width: 640, height: 420), + styleMask: [.titled, .closable], + backing: .buffered, + defer: false + ) + window.isReleasedWhenClosed = false + let container = NSView(frame: window.contentRect(forFrameRect: window.frame)) + window.contentView = container + + let markdownWebView = MarkdownWebView( + frame: container.bounds, + configuration: WKWebViewConfiguration() + ) + container.addSubview(markdownWebView) + window.makeKeyAndOrderFront(nil) + defer { + markdownWebView.removeFromSuperview() + window.orderOut(nil) + window.close() + } + + let webViewUndoManager = try #require(markdownWebView.undoManager) + #expect(webViewUndoManager !== window.undoManager) + + let spy = WebContentUndoSpy() + webViewUndoManager.registerUndo(withTarget: spy) { $0.undoCount += 1 } + #expect(webViewUndoManager.canUndo) + #expect(window.undoManager?.canUndo == false) + } + @MainActor private func withBrowserUndoWindow( _ body: (NSWindow, CmuxWebView, () -> [NSEvent]) throws -> Void