From bd1788639de66721ab644c673dbf735d4e7ef33f Mon Sep 17 00:00:00 2001 From: Austin Wang Date: Sun, 15 Mar 2026 18:32:24 -0700 Subject: [PATCH] Fix terminal find overlay crash and focus handoff (#1487) * Add regression tests for terminal find overlay races * Fix terminal find overlay crash and focus handoff --- Sources/AppDelegate.swift | 26 ++ Sources/GhosttyTerminalView.swift | 278 +++++++++++++----- Sources/TabManager.swift | 15 +- cmuxTests/CmuxWebViewKeyEquivalentTests.swift | 107 +++++++ 4 files changed, 350 insertions(+), 76 deletions(-) diff --git a/Sources/AppDelegate.swift b/Sources/AppDelegate.swift index 92e50a6b..d323ed40 100644 --- a/Sources/AppDelegate.swift +++ b/Sources/AppDelegate.swift @@ -1696,6 +1696,32 @@ func shouldRouteTerminalFontZoomShortcutToGhostty( ) != nil } +@discardableResult +func startOrFocusTerminalSearch( + _ terminalSurface: TerminalSurface, + searchFocusNotifier: @escaping (TerminalSurface) -> Void = { + NotificationCenter.default.post(name: .ghosttySearchFocus, object: $0) + } +) -> Bool { + if terminalSurface.searchState != nil { + searchFocusNotifier(terminalSurface) + return true + } + + if terminalSurface.performBindingAction("start_search") { + DispatchQueue.main.async { [weak terminalSurface] in + guard let terminalSurface, terminalSurface.searchState == nil else { return } + terminalSurface.searchState = TerminalSurface.SearchState() + searchFocusNotifier(terminalSurface) + } + return true + } + + terminalSurface.searchState = TerminalSurface.SearchState() + searchFocusNotifier(terminalSurface) + return true +} + /// Let AppKit own native Cmd+` window cycling so key-window changes do not /// re-enter our direct-to-menu shortcut path. func shouldRouteCommandEquivalentDirectlyToMainMenu(_ event: NSEvent) -> Bool { diff --git a/Sources/GhosttyTerminalView.swift b/Sources/GhosttyTerminalView.swift index 013e387f..cc49002b 100644 --- a/Sources/GhosttyTerminalView.swift +++ b/Sources/GhosttyTerminalView.swift @@ -5900,7 +5900,9 @@ final class GhosttySurfaceScrollView: NSView { private let keyboardCopyModeBadgeIconView: NSImageView private let keyboardCopyModeBadgeLabel: NSTextField private var searchOverlayHostingView: NSHostingView? + private var deferredSearchOverlayMutationWorkItem: DispatchWorkItem? private var lastSearchOverlayStateID: ObjectIdentifier? + private var searchOverlayMutationGeneration: UInt64 = 0 private var observers: [NSObjectProtocol] = [] private var windowObservers: [NSObjectProtocol] = [] private var isLiveScrolling = false @@ -6286,6 +6288,7 @@ final class GhosttySurfaceScrollView: NSView { #endif observers.forEach { NotificationCenter.default.removeObserver($0) } windowObservers.forEach { NotificationCenter.default.removeObserver($0) } + deferredSearchOverlayMutationWorkItem?.cancel() dropZoneOverlayView.removeFromSuperview() cancelFocusRequest() } @@ -6372,6 +6375,9 @@ final class GhosttySurfaceScrollView: NSView { } _ = setFrameIfNeeded(notificationRingOverlayView, to: bounds) _ = setFrameIfNeeded(flashOverlayView, to: bounds) + if let overlay = searchOverlayHostingView { + _ = setFrameIfNeeded(overlay, to: bounds) + } updateNotificationRingPath() updateFlashPath(style: .standardFocus) synchronizeScrollView() @@ -6609,50 +6615,42 @@ final class GhosttySurfaceScrollView: NSView { CATransaction.commit() } - func setSearchOverlay(searchState: TerminalSurface.SearchState?) { - if !Thread.isMainThread { - DispatchQueue.main.async { [weak self] in - self?.setSearchOverlay(searchState: searchState) - } - return + private func cancelDeferredSearchOverlayMutation() { + deferredSearchOverlayMutationWorkItem?.cancel() + deferredSearchOverlayMutationWorkItem = nil + } + + private func scheduleDeferredSearchOverlayMutation( + generation: UInt64, + _ mutation: @escaping () -> Void + ) { + cancelDeferredSearchOverlayMutation() + let work = DispatchWorkItem { [weak self] in + guard let self else { return } + guard self.searchOverlayMutationGeneration == generation else { return } + self.deferredSearchOverlayMutationWorkItem = nil + mutation() } + deferredSearchOverlayMutationWorkItem = work + DispatchQueue.main.async(execute: work) + } - // Layering contract: keep terminal Cmd+F UI inside this portal-hosted AppKit view. - // SwiftUI panel-level overlays can fall behind portal-hosted terminal surfaces. - guard let terminalSurface = surfaceView.terminalSurface, - let searchState else { - let hadOverlay = searchOverlayHostingView != nil - lastSearchOverlayStateID = nil - guard hadOverlay else { return } -#if DEBUG - dlog("find.setSearchOverlay REMOVE surface=\(surfaceView.terminalSurface?.id.uuidString.prefix(5) ?? "nil") hadOverlay=\(hadOverlay)") -#endif - searchOverlayHostingView?.removeFromSuperview() - searchOverlayHostingView = nil - searchFocusTarget = .searchField - return + private func updateKeyboardCopyModeBadgeZOrder(relativeTo overlay: NSView?) { + guard !keyboardCopyModeBadgeContainerView.isHidden else { return } + if let overlay, overlay.superview === self { + addSubview(keyboardCopyModeBadgeContainerView, positioned: .above, relativeTo: overlay) + } else { + addSubview(keyboardCopyModeBadgeContainerView, positioned: .above, relativeTo: nil) } + } - let searchStateID = ObjectIdentifier(searchState) - if let overlay = searchOverlayHostingView, - lastSearchOverlayStateID == searchStateID, - overlay.superview === self { - if !keyboardCopyModeBadgeContainerView.isHidden { - addSubview(keyboardCopyModeBadgeContainerView, positioned: .above, relativeTo: overlay) - } - return - } - - let hadOverlay = searchOverlayHostingView != nil -#if DEBUG - dlog("find.setSearchOverlay MOUNT surface=\(terminalSurface.id.uuidString.prefix(5)) existingOverlay=\(hadOverlay ? "yes(update)" : "no(create)")") -#endif - - let tabId = terminalSurface.tabId - let surfaceId = terminalSurface.id - let rootView = SurfaceSearchOverlay( - tabId: tabId, - surfaceId: surfaceId, + private func makeSearchOverlayRootView( + terminalSurface: TerminalSurface, + searchState: TerminalSurface.SearchState + ) -> SurfaceSearchOverlay { + SurfaceSearchOverlay( + tabId: terminalSurface.tabId, + surfaceId: terminalSurface.id, searchState: searchState, onMoveFocusToTerminal: { [weak self] in self?.searchFocusTarget = .terminal @@ -6670,41 +6668,165 @@ final class GhosttySurfaceScrollView: NSView { self?.moveFocus() } ) + } + + private func findEditableSearchField(in view: NSView?) -> NSTextField? { + guard let view else { return nil } + if let field = view as? NSTextField, field.isEditable { + return field + } + for subview in view.subviews { + if let field = findEditableSearchField(in: subview) { + return field + } + } + return nil + } + + private func requestMountedSearchFieldFocus( + generation: UInt64, + force: Bool, + attemptsRemaining: Int = 4 + ) { + guard searchOverlayMutationGeneration == generation else { return } + guard force || searchFocusTarget == .searchField else { return } + guard let overlay = searchOverlayHostingView, + overlay.superview === self, + let window, + window.isKeyWindow else { return } + + guard let field = findEditableSearchField(in: overlay) else { + guard attemptsRemaining > 0 else { return } + DispatchQueue.main.asyncAfter(deadline: .now() + 0.03) { [weak self] in + self?.requestMountedSearchFieldFocus( + generation: generation, + force: force, + attemptsRemaining: attemptsRemaining - 1 + ) + } + return + } + + let firstResponder = window.firstResponder + let alreadyFocused = firstResponder === field || + field.currentEditor() != nil || + ((firstResponder as? NSTextView)?.delegate as? NSTextField) === field + guard !alreadyFocused else { return } + + surfaceView.terminalSurface?.setFocus(false) + let result = window.makeFirstResponder(field) +#if DEBUG + dlog( + "find.mountedFieldFocus surface=\(surfaceView.terminalSurface?.id.uuidString.prefix(5) ?? "nil") " + + "result=\(result ? 1 : 0) attemptsRemaining=\(attemptsRemaining) " + + "firstResponder=\(String(describing: window.firstResponder))" + ) +#endif + guard !result, attemptsRemaining > 0 else { return } + DispatchQueue.main.asyncAfter(deadline: .now() + 0.03) { [weak self] in + self?.requestMountedSearchFieldFocus( + generation: generation, + force: force, + attemptsRemaining: attemptsRemaining - 1 + ) + } + } + + func setSearchOverlay(searchState: TerminalSurface.SearchState?) { + if !Thread.isMainThread { + DispatchQueue.main.async { [weak self] in + self?.setSearchOverlay(searchState: searchState) + } + return + } + + searchOverlayMutationGeneration &+= 1 + let mutationGeneration = searchOverlayMutationGeneration + + // Layering contract: keep terminal Cmd+F UI inside this portal-hosted AppKit view. + // SwiftUI panel-level overlays can fall behind portal-hosted terminal surfaces. + guard let terminalSurface = surfaceView.terminalSurface, + let searchState else { + let hadOverlay = searchOverlayHostingView != nil + lastSearchOverlayStateID = nil + searchFocusTarget = .searchField + guard hadOverlay else { + cancelDeferredSearchOverlayMutation() + return + } +#if DEBUG + dlog("find.setSearchOverlay REMOVE surface=\(surfaceView.terminalSurface?.id.uuidString.prefix(5) ?? "nil") hadOverlay=\(hadOverlay)") +#endif + scheduleDeferredSearchOverlayMutation(generation: mutationGeneration) { [weak self] in + self?.searchOverlayHostingView?.removeFromSuperview() + self?.searchOverlayHostingView = nil + } + return + } + + let searchStateID = ObjectIdentifier(searchState) + if let overlay = searchOverlayHostingView, + lastSearchOverlayStateID == searchStateID, + overlay.superview === self { + cancelDeferredSearchOverlayMutation() + _ = setFrameIfNeeded(overlay, to: bounds) + updateKeyboardCopyModeBadgeZOrder(relativeTo: overlay) + return + } + + let hadOverlay = searchOverlayHostingView != nil +#if DEBUG + dlog("find.setSearchOverlay MOUNT surface=\(terminalSurface.id.uuidString.prefix(5)) existingOverlay=\(hadOverlay ? "yes(update)" : "no(create)")") +#endif + + let rootView = makeSearchOverlayRootView( + terminalSurface: terminalSurface, + searchState: searchState + ) if let overlay = searchOverlayHostingView { overlay.rootView = rootView - if overlay.superview !== self { - overlay.removeFromSuperview() - addSubview(overlay) - NSLayoutConstraint.activate([ - overlay.topAnchor.constraint(equalTo: topAnchor), - overlay.bottomAnchor.constraint(equalTo: bottomAnchor), - overlay.leadingAnchor.constraint(equalTo: leadingAnchor), - overlay.trailingAnchor.constraint(equalTo: trailingAnchor), - ]) - } - if !keyboardCopyModeBadgeContainerView.isHidden { - addSubview(keyboardCopyModeBadgeContainerView, positioned: .above, relativeTo: overlay) - } lastSearchOverlayStateID = searchStateID + if overlay.superview !== self { + scheduleDeferredSearchOverlayMutation(generation: mutationGeneration) { [weak self, weak overlay] in + guard let self, let overlay else { return } + overlay.removeFromSuperview() + overlay.frame = self.bounds + overlay.autoresizingMask = [.width, .height] + self.addSubview(overlay) + self.updateKeyboardCopyModeBadgeZOrder(relativeTo: overlay) + self.requestMountedSearchFieldFocus( + generation: mutationGeneration, + force: false + ) + } + return + } + cancelDeferredSearchOverlayMutation() + _ = setFrameIfNeeded(overlay, to: bounds) + updateKeyboardCopyModeBadgeZOrder(relativeTo: overlay) return } searchFocusTarget = .searchField let overlay = NSHostingView(rootView: rootView) - overlay.translatesAutoresizingMaskIntoConstraints = false - addSubview(overlay) - NSLayoutConstraint.activate([ - overlay.topAnchor.constraint(equalTo: topAnchor), - overlay.bottomAnchor.constraint(equalTo: bottomAnchor), - overlay.leadingAnchor.constraint(equalTo: leadingAnchor), - overlay.trailingAnchor.constraint(equalTo: trailingAnchor), - ]) - if !keyboardCopyModeBadgeContainerView.isHidden { - addSubview(keyboardCopyModeBadgeContainerView, positioned: .above, relativeTo: overlay) - } + overlay.frame = bounds + overlay.autoresizingMask = [.width, .height] searchOverlayHostingView = overlay lastSearchOverlayStateID = searchStateID + scheduleDeferredSearchOverlayMutation(generation: mutationGeneration) { [weak self, weak overlay] in + guard let self, let overlay else { return } + guard self.searchOverlayHostingView === overlay else { return } + overlay.removeFromSuperview() + overlay.frame = self.bounds + overlay.autoresizingMask = [.width, .height] + self.addSubview(overlay) + self.updateKeyboardCopyModeBadgeZOrder(relativeTo: overlay) + self.requestMountedSearchFieldFocus( + generation: mutationGeneration, + force: true + ) + } } func syncKeyStateIndicator(text: String?) { @@ -6723,11 +6845,7 @@ final class GhosttySurfaceScrollView: NSView { || subviews.last !== keyboardCopyModeBadgeContainerView keyboardCopyModeBadgeContainerView.isHidden = false if needsReorder { - if let overlay = searchOverlayHostingView { - addSubview(keyboardCopyModeBadgeContainerView, positioned: .above, relativeTo: overlay) - } else { - addSubview(keyboardCopyModeBadgeContainerView, positioned: .above, relativeTo: nil) - } + updateKeyboardCopyModeBadgeZOrder(relativeTo: searchOverlayHostingView) } return } @@ -7433,6 +7551,17 @@ final class GhosttySurfaceScrollView: NSView { let surfaceShort = surfaceView.terminalSurface?.id.uuidString.prefix(5) ?? "nil" switch searchFocusTarget { case .searchField: + if let firstResponder = window.firstResponder, + isCurrentSurfaceSearchFieldResponder(firstResponder) { + surfaceView.terminalSurface?.setFocus(false) +#if DEBUG + dlog( + "find.restoreSearchFocus.skip surface=\(surfaceShort) target=searchField " + + "reason=alreadyFocused firstResponder=\(String(describing: firstResponder))" + ) +#endif + return + } if let firstResponder = window.firstResponder, isSearchOverlayOrDescendant(firstResponder), !isCurrentSurfaceSearchResponder(firstResponder) { @@ -7636,6 +7765,17 @@ final class GhosttySurfaceScrollView: NSView { return view.isDescendant(of: self) } + private func isCurrentSurfaceSearchFieldResponder(_ responder: NSResponder) -> Bool { + if let editor = responder as? NSTextView, + editor.isFieldEditor, + let editedView = editor.delegate as? NSTextField { + return editedView.isDescendant(of: self) && isSearchOverlayOrDescendant(editedView) + } + + guard let textField = responder as? NSTextField else { return false } + return textField.isDescendant(of: self) && isSearchOverlayOrDescendant(textField) + } + #if DEBUG struct DebugRenderStats { let drawCount: Int diff --git a/Sources/TabManager.swift b/Sources/TabManager.swift index 5cb8dbba..65b006b4 100644 --- a/Sources/TabManager.swift +++ b/Sources/TabManager.swift @@ -911,15 +911,16 @@ class TabManager: ObservableObject { #endif return } - let wasNil = panel.searchState == nil - if wasNil { - panel.searchState = TerminalSurface.SearchState() - } + let hadExistingSearch = panel.searchState != nil + let handled = startOrFocusTerminalSearch(panel.surface) #if DEBUG - dlog("find.startSearch workspace=\(panel.workspaceId.uuidString.prefix(5)) panel=\(panel.id.uuidString.prefix(5)) created=\(wasNil ? "yes" : "no(reuse)") firstResponder=\(String(describing: panel.surface.hostedView.window?.firstResponder))") + dlog( + "find.startSearch workspace=\(panel.workspaceId.uuidString.prefix(5)) " + + "panel=\(panel.id.uuidString.prefix(5)) existing=\(hadExistingSearch ? "yes" : "no") " + + "handled=\(handled ? 1 : 0) " + + "firstResponder=\(String(describing: panel.surface.hostedView.window?.firstResponder))" + ) #endif - NotificationCenter.default.post(name: .ghosttySearchFocus, object: panel.surface) - _ = panel.performBindingAction("start_search") } func searchSelection() { diff --git a/cmuxTests/CmuxWebViewKeyEquivalentTests.swift b/cmuxTests/CmuxWebViewKeyEquivalentTests.swift index 313d03d4..b8f24566 100644 --- a/cmuxTests/CmuxWebViewKeyEquivalentTests.swift +++ b/cmuxTests/CmuxWebViewKeyEquivalentTests.swift @@ -12025,6 +12025,18 @@ final class GhosttySurfaceOverlayTests: XCTestCase { return nil } + private func firstResponderOwnsTextField(_ firstResponder: NSResponder?, textField: NSTextField) -> Bool { + if firstResponder === textField { + return true + } + if let editor = firstResponder as? NSTextView, + editor.isFieldEditor, + editor.delegate as? NSTextField === textField { + return true + } + return false + } + func testTrackpadScrollRoutesToTerminalSurfaceAndPreservesKeyboardFocusPath() { let window = NSWindow( contentRect: NSRect(x: 0, y: 0, width: 360, height: 240), @@ -12157,12 +12169,105 @@ final class GhosttySurfaceOverlayTests: XCTestCase { let searchState = TerminalSurface.SearchState(needle: "example") hostedView.setSearchOverlay(searchState: searchState) + RunLoop.current.run(until: Date().addingTimeInterval(0.05)) XCTAssertTrue(hostedView.debugHasSearchOverlay()) hostedView.setSearchOverlay(searchState: nil) + RunLoop.current.run(until: Date().addingTimeInterval(0.05)) XCTAssertFalse(hostedView.debugHasSearchOverlay()) } + func testRapidSearchOverlayToggleDoesNotLeaveStaleOverlayMounted() { + let surface = TerminalSurface( + tabId: UUID(), + context: GHOSTTY_SURFACE_CONTEXT_SPLIT, + configTemplate: nil, + workingDirectory: nil + ) + let hostedView = surface.hostedView + + hostedView.setSearchOverlay(searchState: TerminalSurface.SearchState(needle: "example")) + hostedView.setSearchOverlay(searchState: nil) + RunLoop.current.run(until: Date().addingTimeInterval(0.05)) + + XCTAssertFalse( + hostedView.debugHasSearchOverlay(), + "A stale deferred mount must not resurrect the find overlay after it closes" + ) + } + + func testSearchOverlayFocusesSearchFieldAfterDeferredAttach() { + let surface = TerminalSurface( + tabId: UUID(), + context: GHOSTTY_SURFACE_CONTEXT_SPLIT, + configTemplate: nil, + workingDirectory: nil + ) + let hostedView = surface.hostedView + + let window = NSWindow( + contentRect: NSRect(x: 0, y: 0, width: 360, height: 240), + styleMask: [.titled, .closable], + backing: .buffered, + defer: false + ) + defer { window.orderOut(nil) } + + guard let contentView = window.contentView else { + XCTFail("Expected content view") + return + } + hostedView.frame = contentView.bounds + hostedView.autoresizingMask = [.width, .height] + contentView.addSubview(hostedView) + + window.makeKeyAndOrderFront(nil) + window.displayIfNeeded() + contentView.layoutSubtreeIfNeeded() + hostedView.setVisibleInUI(true) + hostedView.setActive(true) + + let searchState = TerminalSurface.SearchState(needle: "") + surface.searchState = searchState + hostedView.setSearchOverlay(searchState: searchState) + RunLoop.current.run(until: Date().addingTimeInterval(0.05)) + + guard let searchField = findEditableTextField(in: hostedView) else { + XCTFail("Expected mounted find text field") + return + } + + XCTAssertTrue( + firstResponderOwnsTextField(window.firstResponder, textField: searchField), + "Deferred search overlay attach should still move focus into the find field" + ) + } + + func testStartOrFocusTerminalSearchReusesExistingSearchState() { + let surface = TerminalSurface( + tabId: UUID(), + context: GHOSTTY_SURFACE_CONTEXT_SPLIT, + configTemplate: nil, + workingDirectory: nil + ) + let existingSearchState = TerminalSurface.SearchState(needle: "existing") + surface.searchState = existingSearchState + + var focusNotificationCount = 0 + XCTAssertTrue( + startOrFocusTerminalSearch(surface) { _ in + focusNotificationCount += 1 + } + ) + + XCTAssertTrue(surface.searchState === existingSearchState) + XCTAssertEqual( + focusNotificationCount, + 1, + "Re-triggering terminal Find should refocus the existing overlay without recreating state" + ) + } + func testEscapeDismissingFindOverlayDoesNotLeakEscapeKeyUpToTerminal() { _ = NSApplication.shared @@ -12398,6 +12503,7 @@ final class GhosttySurfaceOverlayTests: XCTestCase { ) let hostedView = surface.hostedView hostedView.setSearchOverlay(searchState: TerminalSurface.SearchState(needle: "split")) + RunLoop.current.run(until: Date().addingTimeInterval(0.05)) XCTAssertTrue(hostedView.debugHasSearchOverlay()) portal.bind(hostedView: hostedView, to: anchorA, visibleInUI: true) @@ -12436,6 +12542,7 @@ final class GhosttySurfaceOverlayTests: XCTestCase { ) let hostedView = surface.hostedView hostedView.setSearchOverlay(searchState: TerminalSurface.SearchState(needle: "workspace")) + RunLoop.current.run(until: Date().addingTimeInterval(0.05)) XCTAssertTrue(hostedView.debugHasSearchOverlay()) portal.bind(hostedView: hostedView, to: anchor, visibleInUI: true)