From f366fb0b00ab309332a88837e46feb271446abc8 Mon Sep 17 00:00:00 2001 From: Lawrence Chen <54008264+lawrencecchen@users.noreply.github.com> Date: Sun, 22 Feb 2026 15:25:30 -0800 Subject: [PATCH] Fix terminal Cmd+F overlay layering and add regression guardrails --- .github/workflows/ci.yml | 9 ++ CLAUDE.md | 1 + Sources/GhosttyTerminalView.swift | 61 ++++++++++++ Sources/Panels/TerminalPanelView.swift | 51 ++++------ cmuxTests/CmuxWebViewKeyEquivalentTests.swift | 99 +++++++++++++++++++ scripts/check-terminal-overlay-layering.sh | 36 +++++++ 6 files changed, 226 insertions(+), 31 deletions(-) create mode 100755 scripts/check-terminal-overlay-layering.sh diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 9e7bb8bc..d004f0a5 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -7,6 +7,15 @@ on: pull_request: jobs: + terminal-overlay-contract: + runs-on: ubuntu-latest + steps: + - name: Checkout + uses: actions/checkout@v4 + + - name: Enforce terminal overlay layering contract + run: ./scripts/check-terminal-overlay-layering.sh + web-typecheck: runs-on: ubuntu-latest defaults: diff --git a/CLAUDE.md b/CLAUDE.md index bc3d5bba..c7617e40 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -93,6 +93,7 @@ tail -f "$(cat /tmp/cmux-last-debug-log-path 2>/dev/null || echo /tmp/cmux-debug - **Custom UTTypes** for drag-and-drop must be declared in `Resources/Info.plist` under `UTExportedTypeDeclarations` (e.g. `com.splittabbar.tabtransfer`, `com.cmux.sidebar-tab-reorder`). - Do not add an app-level display link or manual `ghostty_surface_draw` loop; rely on Ghostty wakeups/renderer to avoid typing lag. +- **Terminal find layering contract:** `SurfaceSearchOverlay` must be mounted from `GhosttySurfaceScrollView` in `Sources/GhosttyTerminalView.swift` (AppKit portal layer), not from SwiftUI panel containers such as `Sources/Panels/TerminalPanelView.swift`. Portal-hosted terminal views can sit above SwiftUI during split/workspace churn. - **Submodule safety:** When modifying a submodule (ghostty, vendor/bonsplit, etc.), always push the submodule commit to its remote `main` branch BEFORE committing the updated pointer in the parent repo. Never commit on a detached HEAD or temporary branch — the commit will be orphaned and lost. Verify with: `cd && git merge-base --is-ancestor HEAD origin/main`. ## Socket command threading policy diff --git a/Sources/GhosttyTerminalView.swift b/Sources/GhosttyTerminalView.swift index 78121fa0..0ac7a61d 100644 --- a/Sources/GhosttyTerminalView.swift +++ b/Sources/GhosttyTerminalView.swift @@ -3025,6 +3025,7 @@ final class GhosttySurfaceScrollView: NSView { private let notificationRingLayer: CAShapeLayer private let flashOverlayView: GhosttyFlashOverlayView private let flashLayer: CAShapeLayer + private var searchOverlayHostingView: NSHostingView? private var observers: [NSObjectProtocol] = [] private var windowObservers: [NSObjectProtocol] = [] private var isLiveScrolling = false @@ -3408,6 +3409,59 @@ 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 + } + + // 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 { + searchOverlayHostingView?.removeFromSuperview() + searchOverlayHostingView = nil + return + } + + let rootView = SurfaceSearchOverlay( + surface: terminalSurface, + searchState: searchState, + onClose: { [weak self] in + self?.surfaceView.terminalSurface?.searchState = nil + self?.moveFocus() + } + ) + + 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), + ]) + } + return + } + + 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), + ]) + searchOverlayHostingView = overlay + } + private func dropZoneOverlayFrame(for zone: DropZone, in size: CGSize) -> CGRect { let padding: CGFloat = 4 switch zone { @@ -3688,6 +3742,11 @@ final class GhosttySurfaceScrollView: NSView { ) } + func debugHasSearchOverlay() -> Bool { + guard let overlay = searchOverlayHostingView else { return false } + return overlay.superview === self && !overlay.isHidden + } + #endif /// Handle file/URL drops, forwarding to the terminal as shell-escaped paths. @@ -4359,6 +4418,7 @@ struct GhosttyTerminalView: NSViewRepresentable { var showsUnreadNotificationRing: Bool = false var inactiveOverlayColor: NSColor = .clear var inactiveOverlayOpacity: Double = 0 + var searchState: TerminalSurface.SearchState? = nil var reattachToken: UInt64 = 0 var onFocus: ((UUID) -> Void)? = nil var onTriggerFlash: (() -> Void)? = nil @@ -4460,6 +4520,7 @@ struct GhosttyTerminalView: NSViewRepresentable { visible: showsInactiveOverlay ) hostedView.setNotificationRing(visible: showsUnreadNotificationRing) + hostedView.setSearchOverlay(searchState: searchState) hostedView.setFocusHandler { onFocus?(terminalSurface.id) } hostedView.setTriggerFlashHandler(onTriggerFlash) let forwardedDropZone = isVisibleInUI ? paneDropZone : nil diff --git a/Sources/Panels/TerminalPanelView.swift b/Sources/Panels/TerminalPanelView.swift index ce0ca87d..200104df 100644 --- a/Sources/Panels/TerminalPanelView.swift +++ b/Sources/Panels/TerminalPanelView.swift @@ -15,37 +15,26 @@ struct TerminalPanelView: View { let onTriggerFlash: () -> Void var body: some View { - ZStack(alignment: .topLeading) { - GhosttyTerminalView( - terminalSurface: panel.surface, - isActive: isFocused, - isVisibleInUI: isVisibleInUI, - portalZPriority: portalPriority, - showsInactiveOverlay: isSplit && !isFocused, - showsUnreadNotificationRing: hasUnreadNotification, - inactiveOverlayColor: appearance.unfocusedOverlayNSColor, - inactiveOverlayOpacity: appearance.unfocusedOverlayOpacity, - reattachToken: panel.viewReattachToken, - onFocus: { _ in onFocus() }, - onTriggerFlash: onTriggerFlash - ) - // Keep the NSViewRepresentable identity stable across bonsplit structural updates. - // This prevents transient teardown/recreate that can momentarily detach the hosted terminal view. - .id(panel.id) - .background(Color.clear) - - // Search overlay - if let searchState = panel.searchState { - SurfaceSearchOverlay( - surface: panel.surface, - searchState: searchState, - onClose: { - panel.searchState = nil - panel.hostedView.moveFocus() - } - ) - } - } + // Layering contract: terminal find UI is mounted in GhosttySurfaceScrollView (AppKit portal layer) + // via `searchState`. Rendering `SurfaceSearchOverlay` in this SwiftUI container can hide it. + GhosttyTerminalView( + terminalSurface: panel.surface, + isActive: isFocused, + isVisibleInUI: isVisibleInUI, + portalZPriority: portalPriority, + showsInactiveOverlay: isSplit && !isFocused, + showsUnreadNotificationRing: hasUnreadNotification, + inactiveOverlayColor: appearance.unfocusedOverlayNSColor, + inactiveOverlayOpacity: appearance.unfocusedOverlayOpacity, + searchState: panel.searchState, + reattachToken: panel.viewReattachToken, + onFocus: { _ in onFocus() }, + onTriggerFlash: onTriggerFlash + ) + // Keep the NSViewRepresentable identity stable across bonsplit structural updates. + // This prevents transient teardown/recreate that can momentarily detach the hosted terminal view. + .id(panel.id) + .background(Color.clear) } } diff --git a/cmuxTests/CmuxWebViewKeyEquivalentTests.swift b/cmuxTests/CmuxWebViewKeyEquivalentTests.swift index cc8f5395..2019101a 100644 --- a/cmuxTests/CmuxWebViewKeyEquivalentTests.swift +++ b/cmuxTests/CmuxWebViewKeyEquivalentTests.swift @@ -3164,6 +3164,105 @@ final class GhosttySurfaceOverlayTests: XCTestCase { state = hostedView.debugInactiveOverlayState() XCTAssertTrue(state.isHidden) } + + func testSearchOverlayMountsAndUnmountsWithSearchState() { + let surface = TerminalSurface( + tabId: UUID(), + context: GHOSTTY_SURFACE_CONTEXT_SPLIT, + configTemplate: nil, + workingDirectory: nil + ) + let hostedView = surface.hostedView + XCTAssertFalse(hostedView.debugHasSearchOverlay()) + + let searchState = TerminalSurface.SearchState(needle: "example") + hostedView.setSearchOverlay(searchState: searchState) + XCTAssertTrue(hostedView.debugHasSearchOverlay()) + + hostedView.setSearchOverlay(searchState: nil) + XCTAssertFalse(hostedView.debugHasSearchOverlay()) + } + + func testSearchOverlaySurvivesPortalRebindDuringSplitLikeChurn() { + let window = NSWindow( + contentRect: NSRect(x: 0, y: 0, width: 480, height: 320), + styleMask: [.titled, .closable], + backing: .buffered, + defer: false + ) + defer { window.orderOut(nil) } + let portal = WindowTerminalPortal(window: window) + + guard let contentView = window.contentView else { + XCTFail("Expected content view") + return + } + + let anchorA = NSView(frame: NSRect(x: 20, y: 20, width: 180, height: 140)) + let anchorB = NSView(frame: NSRect(x: 220, y: 20, width: 180, height: 140)) + contentView.addSubview(anchorA) + contentView.addSubview(anchorB) + + let surface = TerminalSurface( + tabId: UUID(), + context: GHOSTTY_SURFACE_CONTEXT_SPLIT, + configTemplate: nil, + workingDirectory: nil + ) + let hostedView = surface.hostedView + hostedView.setSearchOverlay(searchState: TerminalSurface.SearchState(needle: "split")) + XCTAssertTrue(hostedView.debugHasSearchOverlay()) + + portal.bind(hostedView: hostedView, to: anchorA, visibleInUI: true) + XCTAssertTrue(hostedView.debugHasSearchOverlay()) + + portal.bind(hostedView: hostedView, to: anchorB, visibleInUI: true) + XCTAssertTrue( + hostedView.debugHasSearchOverlay(), + "Split-like anchor churn should not unmount terminal search overlay" + ) + } + + func testSearchOverlaySurvivesPortalVisibilityToggleDuringWorkspaceSwitchLikeChurn() { + let window = NSWindow( + contentRect: NSRect(x: 0, y: 0, width: 480, height: 320), + styleMask: [.titled, .closable], + backing: .buffered, + defer: false + ) + defer { window.orderOut(nil) } + let portal = WindowTerminalPortal(window: window) + + guard let contentView = window.contentView else { + XCTFail("Expected content view") + return + } + + let anchor = NSView(frame: NSRect(x: 40, y: 40, width: 220, height: 160)) + contentView.addSubview(anchor) + + let surface = TerminalSurface( + tabId: UUID(), + context: GHOSTTY_SURFACE_CONTEXT_SPLIT, + configTemplate: nil, + workingDirectory: nil + ) + let hostedView = surface.hostedView + hostedView.setSearchOverlay(searchState: TerminalSurface.SearchState(needle: "workspace")) + XCTAssertTrue(hostedView.debugHasSearchOverlay()) + + portal.bind(hostedView: hostedView, to: anchor, visibleInUI: true) + XCTAssertTrue(hostedView.debugHasSearchOverlay()) + + portal.bind(hostedView: hostedView, to: anchor, visibleInUI: false) + XCTAssertTrue(hostedView.debugHasSearchOverlay()) + + portal.bind(hostedView: hostedView, to: anchor, visibleInUI: true) + XCTAssertTrue( + hostedView.debugHasSearchOverlay(), + "Workspace-switch-like visibility toggles should not unmount terminal search overlay" + ) + } } @MainActor diff --git a/scripts/check-terminal-overlay-layering.sh b/scripts/check-terminal-overlay-layering.sh new file mode 100755 index 00000000..6a7e9dde --- /dev/null +++ b/scripts/check-terminal-overlay-layering.sh @@ -0,0 +1,36 @@ +#!/usr/bin/env bash +set -euo pipefail + +cd "$(dirname "$0")/.." + +allowed_a="Sources/Find/SurfaceSearchOverlay.swift" +allowed_b="Sources/GhosttyTerminalView.swift" +pattern='SurfaceSearchOverlay[[:space:]]*\(' + +if command -v rg >/dev/null 2>&1; then + matches="$( + rg -n --no-heading --glob '*.swift' "$pattern" Sources cmuxTests \ + | grep -v "^${allowed_a}:" \ + | grep -v "^${allowed_b}:" \ + || true + )" +else + matches="$( + grep -RIn --include='*.swift' -E "$pattern" Sources cmuxTests \ + | grep -v "^${allowed_a}:" \ + | grep -v "^${allowed_b}:" \ + || true + )" +fi + +if [[ -n "$matches" ]]; then + echo "Terminal overlay layering contract violation:" + echo "SurfaceSearchOverlay may only be instantiated in:" + echo " - ${allowed_a}" + echo " - ${allowed_b}" + echo + echo "$matches" + exit 1 +fi + +echo "OK: terminal overlay layering contract satisfied."