Add debug logs for Cmd+F find bar refocus - #840
Merged
Merged
Conversation
Traces the full lifecycle: menu action, startSearch, overlay mount/unmount, focus changes, window key/resign, applyFirstResponderIfNeeded guards, and moveFocus calls. Helps reproduce the bug where Cmd+F fails to reopen after switching away and back to the terminal window.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces the SwiftUI TextField with an AppKit-backed native text field via NSViewRepresentable, adds IME-safe Escape/Return handling, focus synchronization (NotificationCenter + window-first-responder logic), and extensive DEBUG logging and focus-routing across terminal view, TabManager, and app Find menu. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App as cmuxApp
participant TabMgr as TabManager
participant Surface as GhosttySurfaceScrollView
participant Overlay as SurfaceSearchOverlay (SwiftUI)
participant Native as SearchNativeTextField (AppKit)
participant Window as NSWindow
User->>App: Press Cmd+F
App->>TabMgr: startSearch()
TabMgr->>Surface: setSearchOverlay(searchState)
Surface->>Overlay: mount SwiftUI overlay
Overlay->>Native: Representable mounts native NSTextField
Native->>Surface: post .ghosttySearchFocus notification
Surface->>Window: restoreSearchFocus(window)
Surface->>Native: make native field first responder
User->>Native: Type / Press Escape / Press Return
Native->>Overlay: Coordinator updates text & invokes onEscape/onReturn
Surface->>Surface: update searchFocusTarget and preserve/restore focus
Window->>Surface: windowDidResignKey / windowDidBecomeKey (focus reconciliation)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
Sources/TabManager.swift (2)
769-772: Same issue: usedlog()with#if DEBUGguard.♻️ Suggested fix
func hideFind() { - NSLog("Find: hideFind panel=%@", selectedTerminalPanel?.id.uuidString ?? "nil") +#if DEBUG + dlog("Find: hideFind panel=\(selectedTerminalPanel?.id.uuidString ?? "nil")") +#endif selectedTerminalPanel?.searchState = nil }As per coding guidelines: "All debug events must go to the unified debug log, with free function
dlog("message")wrapping all call sites in#if DEBUG/#endif"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TabManager.swift` around lines 769 - 772, Replace the NSLog call in hideFind with the unified debug logger: remove NSLog("Find: hideFind panel=%@", ...) and instead call dlog(...) wrapped inside a `#if` DEBUG / `#endif` block; leave the assignment to selectedTerminalPanel?.searchState = nil unchanged so only the logging is affected and use selectedTerminalPanel?.id.uuidString ?? "nil" as the message payload in the dlog call.
729-742: Debug logging should usedlog()wrapped in#if DEBUG.The
NSLogcalls here will run in release builds and don't go to the unified debug log. This violates the project's debug logging convention. The rest of this file consistently uses the correct pattern (e.g., lines 585-594, 1212-1216).♻️ Suggested fix to use dlog with DEBUG guard
func startSearch() { guard let panel = selectedTerminalPanel else { - NSLog("Find: startSearch SKIPPED no selectedTerminalPanel") +#if DEBUG + dlog("Find: startSearch SKIPPED no selectedTerminalPanel") +#endif return } let wasNil = panel.searchState == nil if wasNil { panel.searchState = TerminalSurface.SearchState() } - NSLog("Find: startSearch workspace=%@ panel=%@ created=%@ firstResponder=%@", - panel.workspaceId.uuidString, panel.id.uuidString, - wasNil ? "yes" : "no(reuse)", - String(describing: panel.surface.hostedView.window?.firstResponder)) +#if DEBUG + dlog("Find: startSearch workspace=\(panel.workspaceId.uuidString) panel=\(panel.id.uuidString) created=\(wasNil ? "yes" : "no(reuse)") firstResponder=\(String(describing: panel.surface.hostedView.window?.firstResponder))") +#endif NotificationCenter.default.post(name: .ghosttySearchFocus, object: panel.surface) _ = panel.performBindingAction("start_search") }As per coding guidelines: "All debug events (keys, mouse, focus, splits, tabs) must go to the unified debug log, with free function
dlog("message")wrapping all call sites in#if DEBUG/#endif"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TabManager.swift` around lines 729 - 742, Replace the release NSLog calls in this startSearch block with debug-only dlog calls: wrap the logging in `#if` DEBUG / `#endif` and call dlog(...) instead of NSLog, preserving the same message content and the variables (panel.workspaceId.uuidString, panel.id.uuidString, wasNil, String(describing: panel.surface.hostedView.window?.firstResponder)); leave the guard, panel.searchState creation (TerminalSurface.SearchState()), NotificationCenter.post(name: .ghosttySearchFocus, object: panel.surface), and _ = panel.performBindingAction("start_search") unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/cmuxApp.swift`:
- Line 457: Replace the direct NSLog call used for the Find menu debug event
with the unified debug logger: remove NSLog("Find: menu Cmd+F fired") and
instead call dlog("Find: menu Cmd+F fired") wrapped in a compile-time guard (`#if`
DEBUG / `#endif`); update the code site where NSLog is referenced so the debug
message is emitted only in debug builds via dlog.
---
Nitpick comments:
In `@Sources/TabManager.swift`:
- Around line 769-772: Replace the NSLog call in hideFind with the unified debug
logger: remove NSLog("Find: hideFind panel=%@", ...) and instead call dlog(...)
wrapped inside a `#if` DEBUG / `#endif` block; leave the assignment to
selectedTerminalPanel?.searchState = nil unchanged so only the logging is
affected and use selectedTerminalPanel?.id.uuidString ?? "nil" as the message
payload in the dlog call.
- Around line 729-742: Replace the release NSLog calls in this startSearch block
with debug-only dlog calls: wrap the logging in `#if` DEBUG / `#endif` and call
dlog(...) instead of NSLog, preserving the same message content and the
variables (panel.workspaceId.uuidString, panel.id.uuidString, wasNil,
String(describing: panel.surface.hostedView.window?.firstResponder)); leave the
guard, panel.searchState creation (TerminalSurface.SearchState()),
NotificationCenter.post(name: .ghosttySearchFocus, object: panel.surface), and _
= panel.performBindingAction("start_search") unchanged.
ℹ️ Review info
Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a3c643f0-b79d-4202-a5c4-f934f7d7b427
📒 Files selected for processing (4)
Sources/Find/SurfaceSearchOverlay.swiftSources/GhosttyTerminalView.swiftSources/TabManager.swiftSources/cmuxApp.swift
Contributor
Greptile SummaryAdds comprehensive debug logging to trace the Cmd+F find bar lifecycle for diagnosing a refocus bug where the find bar fails to reopen after window switching. The logs cover the complete state machine: menu action trigger, search initialization, overlay mounting/unmounting, focus changes, window key transitions, and first responder guard decisions.
Confidence Score: 5/5
Important Files Changed
Last reviewed commit: 603df79 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 4 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/cmuxApp.swift">
<violation number="1" location="Sources/cmuxApp.swift:457">
P3: Gate this debug-only log with `#if DEBUG` and route it through `dlog` so it doesn’t ship to release logs and shows up in the debug log file used for Cmd+F tracing.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
When the find bar is open and the user switches away and back, the window's first responder was left as the NSWindow itself because applyFirstResponderIfNeeded bailed on the searchState guard and nothing refocused the find bar. This caused a dead state where neither the search field nor the terminal accepted keyboard input. Add a SearchFocusTarget state machine (.searchField / .terminal) to GhosttySurfaceScrollView that tracks user intent. On window-become-key, restoreSearchFocus() makes the correct view first responder based on the target. Pressing Escape with a non-empty needle sets target to .terminal so window reactivation preserves that intent. Cmd+F and .ghosttySearchFocus notifications reset target to .searchField.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e5cb51c44b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
5187
to
5189
| onMoveFocusToTerminal: { [weak self] in | ||
| self?.searchFocusTarget = .terminal | ||
| self?.moveFocus() |
There was a problem hiding this comment.
Setting searchFocusTarget to .terminal on Escape is not paired with any update when the user later clicks back into the search field, so the target can become stale. In that state, switching to another app and back will run restoreSearchFocus and force focus to the terminal even though the search field was the last focused control, which is a user-visible regression for find-bar workflows after using Escape once.
Useful? React with 👍 / 👎.
Two bugs found from debug logs: 1. Other surfaces in the same window (without search active) were calling applyFirstResponderIfNeeded and stealing focus from the find bar's surface. Added a check: if current first responder is inside a search overlay NSHostingView, don't steal it. 2. window.makeFirstResponder(overlay) on the NSHostingView was wrong. It made the hosting view itself the responder, which ate keystrokes as performKeyEquivalent instead of routing them to the SwiftUI TextField inside. Removed that call, now only posting the .ghosttySearchFocus notification to let SwiftUI handle internal focus via @focusstate.
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/GhosttyTerminalView.swift">
<violation number="1" location="Sources/GhosttyTerminalView.swift:5782">
P2: Search-overlay focus guard misses field-editor responders, so terminal focus can still be stolen while find text input is active.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 67bd247787
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+5675
to
+5678
| if surfaceView.terminalSurface?.searchState != nil { | ||
| guard let window else { return } | ||
| restoreSearchFocus(window: window) | ||
| return |
There was a problem hiding this comment.
In ensureFocus, the new early return for active search calls restoreSearchFocus before the existing selectedTabId/focused-pane guards run. That means a delayed retry can now re-focus a stale surface with an open find bar even after selection moved elsewhere, because the function no longer reaches the checks that previously prevented cross-tab/pane focus steals. This is user-visible during rapid workspace/pane switches with find open.
Useful? React with 👍 / 👎.
Comment on lines
+56
to
+59
| .onChange(of: isSearchFieldFocused) { focused in | ||
| #if DEBUG | ||
| dlog("find.overlay.focusChanged surface=\(surfaceId.uuidString.prefix(5)) focused=\(focused)") | ||
| #endif |
There was a problem hiding this comment.
The new focus-change hook only logs and never updates searchFocusTarget, so clicking from the find field back into the terminal leaves the target stale as .searchField. On the next didBecomeKey, restoreSearchFocus will re-focus the search UI even though the user's last in-window focus was the terminal, which regresses expected Cmd+F workflows that keep the find bar open while returning to terminal input.
Useful? React with 👍 / 👎.
…h restore The notification-only approach fails because SwiftUI @focusstate can't propagate to AppKit when the first responder is the NSWindow itself (no view in the responder chain to anchor the change). And making the NSHostingView first responder eats keys as performKeyEquivalent. Now walks the hosting view's subview tree to find the actual editable NSTextField backing the SwiftUI TextField, and calls window.makeFirstResponder directly on it. Falls back to notification if the text field isn't found.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/GhosttyTerminalView.swift`:
- Around line 5781-5825: The current focus-guard in applyFirstResponder misses
cases where window.firstResponder is the shared NSTextView field editor; update
the guard so if window.firstResponder is an NSTextView (the field editor) you
obtain its delegate (e.g., firstResponder.delegate) and treat that delegate as
the editing control to check with isSearchOverlayOrDescendant (cast delegate to
NSView or NSTextField as appropriate and walk its superview chain). Concretely:
in the early return check replace/augment the existing "if let fr =
window.firstResponder as? NSView" branch to also handle "if let fe =
window.firstResponder as? NSTextView, let editorOwner = fe.delegate as? NSView"
and call isSearchOverlayOrDescendant(editorOwner) so field-editor-backed search
TextFields prevent other surfaces from stealing focus.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c0f88cdd-b7e1-46ba-b5e6-13bcb0b3d31a
📒 Files selected for processing (1)
Sources/GhosttyTerminalView.swift
restoreSearchFocus now does both: 1. AppKit: makeFirstResponder(nsTextField) so typing works immediately 2. SwiftUI: post .ghosttySearchFocus so @focusstate syncs and .onExitCommand (Escape) and .onKeyPress (Return) still work Also: clicking the terminal while find bar is open now sets searchFocusTarget to .terminal, so window reactivation correctly restores terminal focus instead of jumping back to the search field.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/GhosttyTerminalView.swift (1)
1955-1978:⚠️ Potential issue | 🟠 MajorUse DEBUG-gated
dloghere; avoid raw needle logging in release builds.Line 1958, Line 1972, and Line 1977 use
NSLogdirectly, and Line 1972 logs the full search needle. This violates the repository debug-log policy and leaks user-entered query text to system logs.Proposed fix
- NSLog("Find: search state created tab=%@ surface=%@", tabId.uuidString, id.uuidString) + `#if` DEBUG + dlog("find.searchState created tab=\(tabId.uuidString.prefix(5)) surface=\(id.uuidString.prefix(5))") + `#endif` searchNeedleCancellable = searchState.$needle .removeDuplicates() @@ .switchToLatest() .sink { [weak self] needle in - NSLog("Find: needle updated tab=%@ surface=%@ needle=%@", self?.tabId.uuidString ?? "unknown", self?.id.uuidString ?? "unknown", needle) + `#if` DEBUG + dlog( + "find.needle updated tab=\(self?.tabId.uuidString.prefix(5) ?? "unknown") " + + "surface=\(self?.id.uuidString.prefix(5) ?? "unknown") chars=\(needle.count)" + ) + `#endif` _ = self?.performBindingAction("search:\(needle)") } } else if oldValue != nil { searchNeedleCancellable = nil - NSLog("Find: search state cleared tab=%@ surface=%@", tabId.uuidString, id.uuidString) + `#if` DEBUG + dlog("find.searchState cleared tab=\(tabId.uuidString.prefix(5)) surface=\(id.uuidString.prefix(5))") + `#endif` _ = performBindingAction("end_search") }As per coding guidelines
**/*.swift: “All debug events (keys, mouse, focus, splits, tabs) must go to the unified debug log, with free functiondlog("message")wrapping all call sites in#if DEBUG/#endif”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/GhosttyTerminalView.swift` around lines 1955 - 1978, Replace the raw NSLog calls in the searchState didSet block (the calls around hostedView.cancelFocusRequest(), inside the search needle sink, and when clearing search state) with the project's debug-only logger: wrap logging in `#if` DEBUG / `#endif` and call dlog(...) instead of NSLog; specifically, change the NSLog near hostedView.cancelFocusRequest(), the NSLog inside the sink that currently prints the raw needle, and the NSLog when clearing search state to use dlog so no raw search needle or user-entered text is emitted in release builds, while keeping searchNeedleCancellable, performBindingAction("search:\(needle)"), and performBindingAction("end_search") behavior unchanged.
♻️ Duplicate comments (1)
Sources/GhosttyTerminalView.swift (1)
5788-5854:⚠️ Potential issue | 🔴 CriticalHandle field-editor first responder in the search-overlay focus guard.
Line 5789 + Line 5847 only walk
NSViewancestry. During text editing, AppKit usually setswindow.firstResponderto the shared field-editorNSTextView, not theNSTextFieldview in the overlay tree. This can bypass the guard and let other surfaces steal focus while find is active.Proposed fix
- if let fr = window.firstResponder as? NSView, isSearchOverlayOrDescendant(fr) { + if let fr = window.firstResponder, isSearchOverlayOrDescendant(fr) { `#if` DEBUG dlog("find.applyFirstResponder SKIP surface=\(surfaceShort) reason=searchOverlayFocused") `#endif` return } @@ - private func isSearchOverlayOrDescendant(_ view: NSView) -> Bool { - var current: NSView? = view + private func isSearchOverlayOrDescendant(_ responder: NSResponder) -> Bool { + if let editor = responder as? NSTextView, + editor.isFieldEditor, + let editedView = editor.delegate as? NSView { + return isSearchOverlayOrDescendant(editedView) + } + + guard let view = responder as? NSView else { return false } + var current: NSView? = view while let v = current { if v is NSHostingView<SurfaceSearchOverlay> { return true } current = v.superview } return false }In macOS AppKit, while editing an NSTextField, does NSWindow.firstResponder become the NSTextField itself or a shared NSTextView field editor? What is the recommended way to map that first responder back to the edited control?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/GhosttyTerminalView.swift` around lines 5788 - 5854, The guard misses cases where window.firstResponder is the shared field-editor NSTextView (so editing a search NSTextField won't be detected); update logic to map a NSTextView field editor back to its owning NSTextField before checking ancestry. Specifically, in the focus-guard and in isSearchOverlayOrDescendant(_:) handle when the responder is an NSTextView: if let fe = window.firstResponder as? NSTextView, get fe.delegate as? NSView (or as? NSTextField) and use that view for the isSearchOverlayOrDescendant check; also ensure findTextField(in:) remains the same to locate backing text fields. Target symbols: isSearchOverlayOrDescendant(_:), restoreSearchFocus(window:), and the initial focus-guard that inspects window.firstResponder.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@Sources/GhosttyTerminalView.swift`:
- Around line 1955-1978: Replace the raw NSLog calls in the searchState didSet
block (the calls around hostedView.cancelFocusRequest(), inside the search
needle sink, and when clearing search state) with the project's debug-only
logger: wrap logging in `#if` DEBUG / `#endif` and call dlog(...) instead of NSLog;
specifically, change the NSLog near hostedView.cancelFocusRequest(), the NSLog
inside the sink that currently prints the raw needle, and the NSLog when
clearing search state to use dlog so no raw search needle or user-entered text
is emitted in release builds, while keeping searchNeedleCancellable,
performBindingAction("search:\(needle)"), and performBindingAction("end_search")
behavior unchanged.
---
Duplicate comments:
In `@Sources/GhosttyTerminalView.swift`:
- Around line 5788-5854: The guard misses cases where window.firstResponder is
the shared field-editor NSTextView (so editing a search NSTextField won't be
detected); update logic to map a NSTextView field editor back to its owning
NSTextField before checking ancestry. Specifically, in the focus-guard and in
isSearchOverlayOrDescendant(_:) handle when the responder is an NSTextView: if
let fe = window.firstResponder as? NSTextView, get fe.delegate as? NSView (or
as? NSTextField) and use that view for the isSearchOverlayOrDescendant check;
also ensure findTextField(in:) remains the same to locate backing text fields.
Target symbols: isSearchOverlayOrDescendant(_:), restoreSearchFocus(window:),
and the initial focus-guard that inspects window.firstResponder.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0e934c63-7312-44f7-92b2-68b754832d5f
📒 Files selected for processing (1)
Sources/GhosttyTerminalView.swift
The core issue: SwiftUI @focusstate does not sync with AppKit's first responder after window resign/become-key cycles. This caused the find bar to lose all keyboard input after switching windows. Previous attempts to bridge SwiftUI and AppKit focus (notifications, makeFirstResponder on the backing NSTextField, belt-and-suspenders approaches) all failed because SwiftUI event handlers (.onExitCommand for Escape, .onKeyPress for Return) require @focusstate to be set. Fix: replace the SwiftUI TextField with an NSViewRepresentable-wrapped NSTextField (SearchTextFieldRepresentable), following the proven OmnibarNativeTextField pattern already in BrowserPanelView.swift. - Escape and Return handled via control(_:textView:doCommandBy:) at the AppKit delegate level, no @focusstate needed - Focus restored via .ghosttySearchFocus notification observed directly by the Coordinator, calling makeFirstResponder immediately - hasMarkedText() guard preserves CJK IME composition (issue #118) - isProgrammaticMutation guard prevents text binding cursor reset - Removes findTextField(in:) subview walk hack
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/Find/SurfaceSearchOverlay.swift">
<violation number="1" location="Sources/Find/SurfaceSearchOverlay.swift:346">
P2: The deferred focus block can apply a stale focus request and steal focus back to the search field after focus intent has changed. Re-check `isFocused` inside the async block before calling `makeFirstResponder`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 996133fc0e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+5825
to
+5828
| var current: NSView? = view | ||
| while let v = current { | ||
| if v is NSHostingView<SurfaceSearchOverlay> { return true } | ||
| current = v.superview |
There was a problem hiding this comment.
isSearchOverlayOrDescendant only walks superview and matches NSHostingView<SurfaceSearchOverlay>, but when the find field is actively editing AppKit sets window.firstResponder to the shared NSTextView field editor instead of a descendant of the hosting view. In that common state this helper returns false, so another surface in the same window can still pass applyFirstResponderIfNeeded and call makeFirstResponder(surfaceView), stealing focus from an open find bar after didBecomeKey/surface focus churn.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Sources/GhosttyTerminalView.swift (1)
5788-5831:⚠️ Potential issue | 🔴 CriticalSearch-overlay focus guard still misses AppKit field-editor responders.
At Line 5789 and Line 5824, the guard only walks
NSViewancestry. During activeNSTextFieldediting,window.firstResponderis commonly the shared field-editorNSTextView, which is outside the overlay hierarchy; focus can still be stolen.🐛 Proposed fix
- if let fr = window.firstResponder as? NSView, isSearchOverlayOrDescendant(fr) { + if let fr = window.firstResponder, isSearchOverlayOrDescendant(fr) { `#if` DEBUG dlog("find.applyFirstResponder SKIP surface=\(surfaceShort) reason=searchOverlayFocused") `#endif` return } @@ - private func isSearchOverlayOrDescendant(_ view: NSView) -> Bool { - var current: NSView? = view + private func isSearchOverlayOrDescendant(_ responder: NSResponder) -> Bool { + if let editor = responder as? NSTextView, + editor.isFieldEditor, + let editedView = editor.delegate as? NSView { + return isSearchOverlayOrDescendant(editedView) + } + guard let view = responder as? NSView else { return false } + var current: NSView? = view while let v = current { if v is NSHostingView<SurfaceSearchOverlay> { return true } current = v.superview } return false }Run this read-only check to confirm the guard handles field-editor responders:
#!/bin/bash rg -n -C3 'applyFirstResponderIfNeeded|isSearchOverlayOrDescendant|firstResponder as\? NSView|isFieldEditor|delegate as\? NSView' Sources/GhosttyTerminalView.swiftExpected result after fix:
isSearchOverlayOrDescendantacceptsNSResponderand includes anNSTextViewfield-editor delegate path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/GhosttyTerminalView.swift` around lines 5788 - 5831, The focus-guard currently only accepts NSView and misses when window.firstResponder is the shared field-editor NSTextView; update the guard to accept NSResponder and walk both view superview ancestry and field-editor delegate chains. Change callers that do "window.firstResponder as? NSView" (used in applyFirstResponderIfNeeded and restoreSearchFocus guards) to pass the raw NSResponder into isSearchOverlayOrDescendant(_:) and modify isSearchOverlayOrDescendant to take an NSResponder, treat NSView by walking superview -> superview, and if the responder is an NSTextView (field editor) follow its delegate (delegate as? NSView) and then walk that view's superview chain checking for NSHostingView<SurfaceSearchOverlay>. Ensure behavior and DEBUG logging remain the same.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@Sources/GhosttyTerminalView.swift`:
- Around line 5788-5831: The focus-guard currently only accepts NSView and
misses when window.firstResponder is the shared field-editor NSTextView; update
the guard to accept NSResponder and walk both view superview ancestry and
field-editor delegate chains. Change callers that do "window.firstResponder as?
NSView" (used in applyFirstResponderIfNeeded and restoreSearchFocus guards) to
pass the raw NSResponder into isSearchOverlayOrDescendant(_:) and modify
isSearchOverlayOrDescendant to take an NSResponder, treat NSView by walking
superview -> superview, and if the responder is an NSTextView (field editor)
follow its delegate (delegate as? NSView) and then walk that view's superview
chain checking for NSHostingView<SurfaceSearchOverlay>. Ensure behavior and
DEBUG logging remain the same.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c9da2a33-a4ac-4441-8d88-ca07ea0ad21e
📒 Files selected for processing (2)
Sources/Find/SurfaceSearchOverlay.swiftSources/GhosttyTerminalView.swift
The Ghostty cursor kept blinking even when the search field was focused because ghostty_surface_set_focus(false) was only called via surfaceView.resignFirstResponder. After window switching, the surface view may not have been the first responder, so resign was never called. Fix: call surface.setFocus(false) in both the .ghosttySearchFocus notification observer and directly in restoreSearchFocus. This ensures the cursor stops blinking regardless of previous first-responder state.
1. isSearchOverlayOrDescendant now accepts NSResponder and follows the field-editor delegate chain back to the owning NSTextField. Previously, when the search field was being edited, the shared NSTextView field editor was the first responder (outside the overlay hierarchy), so the guard missed it and other surfaces could steal focus. 2. Converted all NSLog calls in TabManager (startSearch, hideFind, searchSelection), cmuxApp (Find menu), and GhosttyTerminalView (searchState didSet) to dlog() wrapped in #if DEBUG. Avoids leaking search needle text to system logs in release builds. 3. Added isFocused re-check inside the deferred focus block in SearchTextFieldRepresentable to prevent stale focus requests from stealing focus back after intent has changed.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/GhosttyTerminalView.swift`:
- Around line 5690-5696: The current early return for an active search bypasses
the existing guard/retry flow and can skip retrying visibility/key-window
checks; move the search-active path so it executes after the existing "guard let
window else { return }" and within the retry logic block (i.e., keep the check
of surfaceView.terminalSurface?.searchState and the call to
restoreSearchFocus(window:) inside the same guarded/retry flow used for
ensureFocus) so that restoreSearchFocus(window:) is only attempted when window
is available and will participate in the retry behavior when
reparenting/reactivation occurs; adjust the control flow around isActive and the
guard-let window to nest the search-active branch rather than returning early.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6424b5c2-0348-4519-869a-60483c0fde33
📒 Files selected for processing (4)
Sources/Find/SurfaceSearchOverlay.swiftSources/GhosttyTerminalView.swiftSources/TabManager.swiftSources/cmuxApp.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- Sources/Find/SurfaceSearchOverlay.swift
Comment on lines
5690
to
5696
| guard isActive else { return } | ||
| guard surfaceView.terminalSurface?.searchState == nil else { return } | ||
| if surfaceView.terminalSurface?.searchState != nil { | ||
| guard let window else { return } | ||
| restoreSearchFocus(window: window) | ||
| return | ||
| } | ||
| guard let window else { return } |
There was a problem hiding this comment.
Keep the search-active ensureFocus path inside the existing guard/retry flow.
At Line [5691], active search returns before visibility/key-window checks and retry logic. In transient reparent/reactivation states, this can drop focus restoration instead of retrying.
♻️ Proposed adjustment
- guard isActive else { return }
- if surfaceView.terminalSurface?.searchState != nil {
- guard let window else { return }
- restoreSearchFocus(window: window)
- return
- }
- guard let window else { return }
+ guard isActive else { return }
+ guard let window else { return }
guard surfaceView.isVisibleInUI else {
retry()
return
}
guard !isHiddenForFocus, hasUsablePortalGeometry else {
@@
retry()
return
}
+
+ if surfaceView.terminalSurface?.searchState != nil {
+ if !window.isKeyWindow {
+ window.makeKeyAndOrderFront(nil)
+ }
+ restoreSearchFocus(window: window)
+ if searchFocusTarget == .terminal, !isSurfaceViewFirstResponder() {
+ retry()
+ }
+ return
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/GhosttyTerminalView.swift` around lines 5690 - 5696, The current
early return for an active search bypasses the existing guard/retry flow and can
skip retrying visibility/key-window checks; move the search-active path so it
executes after the existing "guard let window else { return }" and within the
retry logic block (i.e., keep the check of
surfaceView.terminalSurface?.searchState and the call to
restoreSearchFocus(window:) inside the same guarded/retry flow used for
ensureFocus) so that restoreSearchFocus(window:) is only attempted when window
is available and will participate in the retry behavior when
reparenting/reactivation occurs; adjust the control flow around isActive and the
guard-let window to nest the search-active branch rather than returning early.
Every keystroke updated searchState.needle (@published), which triggered a SwiftUI re-render → ensureFocus → restoreSearchFocus → posted .ghosttySearchFocus notification → Coordinator called makeFirstResponder unconditionally. makeFirstResponder on an already-editing NSTextField ends the editing session and restarts with all text selected, so the next typed character replaced the previous one ("hi" → "i"). Fix: check if the field is already first responder before calling makeFirstResponder in the notification handler.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7bc4809f57
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
5121
to
+5123
| func setFocusHandler(_ handler: (() -> Void)?) { | ||
| surfaceView.onFocus = handler | ||
| surfaceView.onFocus = { [weak self] in | ||
| // When the terminal surface gains focus (click, tab, etc.), update the |
There was a problem hiding this comment.
onFocus when handler is nil
setFocusHandler now always installs a closure, even when callers pass nil, so teardown paths like dismantleNSView no longer truly unregister focus callbacks. In this state, incidental terminal focus events can still mutate searchFocusTarget to .terminal while the find bar is open, which changes later didBecomeKey restoration behavior away from the search field despite no user focus action.
Useful? React with 👍 / 👎.
- Add onFieldDidFocus callback so clicking back into the search field after Escape updates searchFocusTarget = .searchField, fixing stale focus restoration after window switches. - Guard updateNSView text sync with !editor.hasMarkedText() to prevent stomping active CJK IME composition. - Move ensureFocus search state check after tab/pane selection guards so search focus isn't restored on non-active tabs/panes. - Clear surfaceView.onFocus when setFocusHandler(nil) is called.
This was referenced Mar 4, 2026
This was referenced Mar 12, 2026
3 tasks
bn-l
pushed a commit
to bn-l/cmux
that referenced
this pull request
Apr 3, 2026
* Add debug logs for Cmd+F find bar focus/refocus state machine Traces the full lifecycle: menu action, startSearch, overlay mount/unmount, focus changes, window key/resign, applyFirstResponderIfNeeded guards, and moveFocus calls. Helps reproduce the bug where Cmd+F fails to reopen after switching away and back to the terminal window. * Fix Cmd+F find bar focus loss after window switch When the find bar is open and the user switches away and back, the window's first responder was left as the NSWindow itself because applyFirstResponderIfNeeded bailed on the searchState guard and nothing refocused the find bar. This caused a dead state where neither the search field nor the terminal accepted keyboard input. Add a SearchFocusTarget state machine (.searchField / .terminal) to GhosttySurfaceScrollView that tracks user intent. On window-become-key, restoreSearchFocus() makes the correct view first responder based on the target. Pressing Escape with a non-empty needle sets target to .terminal so window reactivation preserves that intent. Cmd+F and .ghosttySearchFocus notifications reset target to .searchField. * Fix multi-surface focus stealing and NSHostingView responder issue Two bugs found from debug logs: 1. Other surfaces in the same window (without search active) were calling applyFirstResponderIfNeeded and stealing focus from the find bar's surface. Added a check: if current first responder is inside a search overlay NSHostingView, don't steal it. 2. window.makeFirstResponder(overlay) on the NSHostingView was wrong. It made the hosting view itself the responder, which ate keystrokes as performKeyEquivalent instead of routing them to the SwiftUI TextField inside. Removed that call, now only posting the .ghosttySearchFocus notification to let SwiftUI handle internal focus via @focusstate. * Use AppKit NSTextField focus instead of SwiftUI @focusstate for search restore The notification-only approach fails because SwiftUI @focusstate can't propagate to AppKit when the first responder is the NSWindow itself (no view in the responder chain to anchor the change). And making the NSHostingView first responder eats keys as performKeyEquivalent. Now walks the hosting view's subview tree to find the actual editable NSTextField backing the SwiftUI TextField, and calls window.makeFirstResponder directly on it. Falls back to notification if the text field isn't found. * Two-phase focus restore: AppKit + SwiftUI sync, click-to-terminal fix restoreSearchFocus now does both: 1. AppKit: makeFirstResponder(nsTextField) so typing works immediately 2. SwiftUI: post .ghosttySearchFocus so @focusstate syncs and .onExitCommand (Escape) and .onKeyPress (Return) still work Also: clicking the terminal while find bar is open now sets searchFocusTarget to .terminal, so window reactivation correctly restores terminal focus instead of jumping back to the search field. * Replace SwiftUI TextField with NSViewRepresentable for find bar The core issue: SwiftUI @focusstate does not sync with AppKit's first responder after window resign/become-key cycles. This caused the find bar to lose all keyboard input after switching windows. Previous attempts to bridge SwiftUI and AppKit focus (notifications, makeFirstResponder on the backing NSTextField, belt-and-suspenders approaches) all failed because SwiftUI event handlers (.onExitCommand for Escape, .onKeyPress for Return) require @focusstate to be set. Fix: replace the SwiftUI TextField with an NSViewRepresentable-wrapped NSTextField (SearchTextFieldRepresentable), following the proven OmnibarNativeTextField pattern already in BrowserPanelView.swift. - Escape and Return handled via control(_:textView:doCommandBy:) at the AppKit delegate level, no @focusstate needed - Focus restored via .ghosttySearchFocus notification observed directly by the Coordinator, calling makeFirstResponder immediately - hasMarkedText() guard preserves CJK IME composition (issue manaflow-ai#118) - isProgrammaticMutation guard prevents text binding cursor reset - Removes findTextField(in:) subview walk hack * Explicitly unfocus terminal surface when find bar takes focus The Ghostty cursor kept blinking even when the search field was focused because ghostty_surface_set_focus(false) was only called via surfaceView.resignFirstResponder. After window switching, the surface view may not have been the first responder, so resign was never called. Fix: call surface.setFocus(false) in both the .ghosttySearchFocus notification observer and directly in restoreSearchFocus. This ensures the cursor stops blinking regardless of previous first-responder state. * Address review findings: field-editor guard, NSLog→dlog, stale focus 1. isSearchOverlayOrDescendant now accepts NSResponder and follows the field-editor delegate chain back to the owning NSTextField. Previously, when the search field was being edited, the shared NSTextView field editor was the first responder (outside the overlay hierarchy), so the guard missed it and other surfaces could steal focus. 2. Converted all NSLog calls in TabManager (startSearch, hideFind, searchSelection), cmuxApp (Find menu), and GhosttyTerminalView (searchState didSet) to dlog() wrapped in #if DEBUG. Avoids leaking search needle text to system logs in release builds. 3. Added isFocused re-check inside the deferred focus block in SearchTextFieldRepresentable to prevent stale focus requests from stealing focus back after intent has changed. * Guard against re-focusing already-focused search field Every keystroke updated searchState.needle (@published), which triggered a SwiftUI re-render → ensureFocus → restoreSearchFocus → posted .ghosttySearchFocus notification → Coordinator called makeFirstResponder unconditionally. makeFirstResponder on an already-editing NSTextField ends the editing session and restarts with all text selected, so the next typed character replaced the previous one ("hi" → "i"). Fix: check if the field is already first responder before calling makeFirstResponder in the notification handler. * Address review findings: stale focus target, IME guard, tab/pane gating - Add onFieldDidFocus callback so clicking back into the search field after Escape updates searchFocusTarget = .searchField, fixing stale focus restoration after window switches. - Guard updateNSView text sync with !editor.hasMarkedText() to prevent stomping active CJK IME composition. - Move ensureFocus search state check after tab/pane selection guards so search focus isn't restored on non-active tabs/panes. - Clear surfaceView.onFocus when setFocusHandler(nil) is called.
This was referenced Apr 29, 2026
15 of 22 tasks
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
startSearch(), overlay mount/unmount (setSearchOverlay), search field focus changes, windowdidBecomeKey/didResignKey,applyFirstResponderIfNeededguard decisions,ensureFocusguard, andmoveFocuscallsdlog(debug log file) in GhosttyTerminalView.swift and SurfaceSearchOverlay.swift (where NSLog is unavailable due to C-interop), and NSLog in TabManager.swift and cmuxApp.swiftTesting
./scripts/reload.sh --tag cmdf-refocus-logstail -f /tmp/cmux-debug-cmdf-refocus-logs.log | grep findRelated
Summary by cubic
Fixes Cmd+F find bar not refocusing after window/app switches by tracking intent and restoring the correct first responder (search field or terminal). Adds focused debug logs and IME-safe guards, and switches to a native AppKit text field to prevent focus loss and accidental text replacement.
Bug Fixes
Debug Logs
Written for commit f5754e8. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Chores