Add browser find focus debug logs - #1162
Merged
Merged
Conversation
|
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:
📝 WalkthroughWalkthroughAdds panel-level focus-intent APIs and wiring for capture/restore; instruments and traces search-overlay lifecycle and focus requests with origin/reason metadata; ensures portal overlay (re)attachment with full-edge constraints; tightens portal webview ownership checks and expands DEBUG-only focus logging across workspace, terminal, and panel layers. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Panel as BrowserPanel
participant NC as NotificationCenter
participant Overlay as BrowserSearchOverlay
participant Slot as WindowBrowserSlotView
participant App as AppDelegate
User->>Panel: startFind / request focus
Panel->>NC: postBrowserSearchFocusNotification(reason)
NC->>Overlay: browserSearchFocus notification
Overlay->>Overlay: requestSearchFieldFocus(origin)
Overlay->>Panel: onFieldDidFocus() when field focused
Overlay->>Slot: ensure overlay attached / reattach if needed
Slot->>App: query portal webview ownership (is descendant?)
App-->>Slot: ownership result
Slot-->>Overlay: apply constraints / attachment result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
Contributor
Greptile SummaryThis PR adds Key changes:
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant BrowserPanel
participant NotificationCenter
participant BrowserWindowPortal
participant BrowserSearchOverlay
User->>BrowserPanel: Cmd+F (startFind)
Note over BrowserPanel: dlog browser.find.start
BrowserPanel->>BrowserPanel: searchState = BrowserSearchState()
BrowserPanel->>NotificationCenter: post browserSearchFocus (reason=immediate)
Note over BrowserPanel: dlog browser.find.focusNotification
BrowserPanel-->>NotificationCenter: async post (reason=async0)
BrowserPanel-->>NotificationCenter: asyncAfter 50ms post (reason=async50ms)
BrowserWindowPortal->>BrowserWindowPortal: setSearchOverlay(config)
Note over BrowserWindowPortal: dlog portal action=set
alt New overlay
BrowserWindowPortal->>BrowserSearchOverlay: NSHostingView(rootView:)
Note over BrowserWindowPortal: dlog portal action=create
else Existing overlay
Note over BrowserWindowPortal: dlog portal action=updateExisting
BrowserWindowPortal->>BrowserSearchOverlay: overlay.rootView = rootView
end
BrowserSearchOverlay->>BrowserSearchOverlay: onAppear
Note over BrowserSearchOverlay: dlog browser.findbar.appear
Note over BrowserSearchOverlay: logFocusState(appear)
BrowserSearchOverlay->>BrowserSearchOverlay: requestSearchFieldFocus(origin=appear)
Note over BrowserSearchOverlay: isSearchFieldFocused = true
Note over BrowserSearchOverlay: logFocusState(request.afterAsync) [async]
NotificationCenter-->>BrowserSearchOverlay: browserSearchFocus notification
Note over BrowserSearchOverlay: logFocusState(notification.received)
BrowserSearchOverlay->>BrowserSearchOverlay: requestSearchFieldFocus(origin=notification)
BrowserSearchOverlay-->>BrowserSearchOverlay: onChange(isSearchFieldFocused)
Note over BrowserSearchOverlay: logFocusState(focusState.change)
Last reviewed commit: 726f3a5 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+44
to
+46
| DispatchQueue.main.async { | ||
| logFocusState("request.afterAsync origin=\(origin) remaining=\(maxAttempts)") | ||
| } |
Contributor
There was a problem hiding this comment.
Unnecessary async dispatch in release builds
DispatchQueue.main.async is always enqueued — even in release builds — but its sole purpose is to call logFocusState, which compiles to an empty function when DEBUG is not set. The closure dispatch itself has runtime cost that cannot be optimised away. Wrapping the block in #if DEBUG removes this overhead:
Suggested change
| DispatchQueue.main.async { | |
| logFocusState("request.afterAsync origin=\(origin) remaining=\(maxAttempts)") | |
| } | |
| #if DEBUG | |
| DispatchQueue.main.async { | |
| logFocusState("request.afterAsync origin=\(origin) remaining=\(maxAttempts)") | |
| } | |
| #endif |
Comment on lines
+18
to
+26
| private func debugFirstResponderSummary() -> String { | ||
| guard let window = NSApp.keyWindow else { return "nil" } | ||
| guard let firstResponder = window.firstResponder else { return "nil" } | ||
| if let editor = firstResponder as? NSTextView, editor.isFieldEditor { | ||
| let delegateSummary = editor.delegate.map { String(describing: type(of: $0)) } ?? "nil" | ||
| return "fieldEditor(delegate=\(delegateSummary))" | ||
| } | ||
| return String(describing: type(of: firstResponder)) | ||
| } |
Contributor
There was a problem hiding this comment.
Debug helper compiled into release builds
debugFirstResponderSummary() is only ever called inside the #if DEBUG block within logFocusState, yet the function itself has no #if DEBUG guard. It will be compiled (and occupy space in the binary) even in release builds. Consider guarding it the same way as logFocusState:
Suggested change
| private func debugFirstResponderSummary() -> String { | |
| guard let window = NSApp.keyWindow else { return "nil" } | |
| guard let firstResponder = window.firstResponder else { return "nil" } | |
| if let editor = firstResponder as? NSTextView, editor.isFieldEditor { | |
| let delegateSummary = editor.delegate.map { String(describing: type(of: $0)) } ?? "nil" | |
| return "fieldEditor(delegate=\(delegateSummary))" | |
| } | |
| return String(describing: type(of: firstResponder)) | |
| } | |
| #if DEBUG | |
| private func debugFirstResponderSummary() -> String { | |
| guard let window = NSApp.keyWindow else { return "nil" } | |
| guard let firstResponder = window.firstResponder else { return "nil" } | |
| if let editor = firstResponder as? NSTextView, editor.isFieldEditor { | |
| let delegateSummary = editor.delegate.map { String(describing: type(of: $0)) } ?? "nil" | |
| return "fieldEditor(delegate=\(delegateSummary))" | |
| } | |
| return String(describing: type(of: firstResponder)) | |
| } | |
| #endif |
Comment on lines
138
to
140
| .onChange(of: isSearchFieldFocused) { focused in | ||
| logFocusState("focusState.change next=\(focused ? 1 : 0)") | ||
| } |
Contributor
There was a problem hiding this comment.
Deprecated onChange(of:) overload
The single-closure form of .onChange(of:) was deprecated in macOS 14 / iOS 17. The new two-parameter form (receiving oldValue and newValue) should be used instead:
Suggested change
| .onChange(of: isSearchFieldFocused) { focused in | |
| logFocusState("focusState.change next=\(focused ? 1 : 0)") | |
| } | |
| .onChange(of: isSearchFieldFocused) { _, newValue in | |
| logFocusState("focusState.change next=\(newValue ? 1 : 0)") | |
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
Sources/Find/BrowserSearchOverlay.swift (3)
44-46: Wrap the async dispatch in#if DEBUGto avoid unnecessary dispatch in release builds.In release builds, this schedules an async dispatch that calls
logFocusState, which has an empty body. This creates a small runtime overhead (dispatch queue scheduling) that serves no purpose. As per coding guidelines, debug events should be wrapped in#if DEBUG.Suggested refactor
logFocusState("request.begin origin=\(origin) remaining=\(maxAttempts)") isSearchFieldFocused = true +#if DEBUG DispatchQueue.main.async { logFocusState("request.afterAsync origin=\(origin) remaining=\(maxAttempts)") } +#endif guard maxAttempts > 1 else { return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Find/BrowserSearchOverlay.swift` around lines 44 - 46, The DispatchQueue.main.async call that invokes logFocusState("request.afterAsync origin=\(origin) remaining=\(maxAttempts)") should be executed only in debug builds to avoid unnecessary dispatch overhead in release; wrap the DispatchQueue.main.async { logFocusState(...) } block in an `#if` DEBUG ... `#endif` so that logFocusState, DispatchQueue.main.async, origin and maxAttempts are only used for debug instrumentation and omitted from release builds.
18-38: Consider wrappingdebugFirstResponderSummary()in#if DEBUG.The helper function is only called from within
logFocusState's DEBUG-guarded body. In release builds,debugFirstResponderSummary()will be compiled but never called, resulting in dead code in the binary.Suggested refactor
+#if DEBUG private func debugFirstResponderSummary() -> String { guard let window = NSApp.keyWindow else { return "nil" } guard let firstResponder = window.firstResponder else { return "nil" } if let editor = firstResponder as? NSTextView, editor.isFieldEditor { let delegateSummary = editor.delegate.map { String(describing: type(of: $0)) } ?? "nil" return "fieldEditor(delegate=\(delegateSummary))" } return String(describing: type(of: firstResponder)) } +#endif private func logFocusState(_ event: String) { `#if` DEBUG let keyWindow = NSApp.keyWindow dlog( "browser.findbar.focus panel=\(panelId.uuidString.prefix(5)) " + "event=\(event) keyWindow=\(keyWindow?.windowNumber ?? -1) " + "firstResponder=\(debugFirstResponderSummary()) " + "focused=\(isSearchFieldFocused ? 1 : 0)" ) `#endif` }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Find/BrowserSearchOverlay.swift` around lines 18 - 38, The helper debugFirstResponderSummary() is only used inside the DEBUG-only body of logFocusState(), leaving it as dead code in release builds; move or wrap its entire declaration inside the same `#if` DEBUG block as logFocusState() (or inline its logic into logFocusState()) so debugFirstResponderSummary() is only compiled in DEBUG builds, keeping release binaries free of unused symbols while preserving the existing behavior of logFocusState().
138-140: Wrap the debug-onlyonChangein#if DEBUG.This onChange handler exists solely to call
logFocusState, which is empty in release builds. The handler will still fire on every focus state change but perform no useful work.Suggested refactor
.onAppear { `#if` DEBUG dlog("browser.findbar.appear panel=\(panelId.uuidString.prefix(5))") -#endif logFocusState("appear") +#endif requestSearchFieldFocus(origin: "appear") } +#if DEBUG .onChange(of: isSearchFieldFocused) { focused in logFocusState("focusState.change next=\(focused ? 1 : 0)") } +#endif .onReceive(NotificationCenter.default.publisher(for: .browserSearchFocus)) { notification in🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Find/BrowserSearchOverlay.swift` around lines 138 - 140, The .onChange(of: isSearchFieldFocused) { focused in logFocusState(...) } handler in BrowserSearchOverlay.swift should be compiled only in debug builds; wrap the entire .onChange modifier in a conditional compilation block (`#if` DEBUG ... `#endif`) so the handler (and its call to logFocusState) is removed in release builds, ensuring view modifier balance and that you place the `#endif` immediately after the .onChange(...) call to keep the SwiftUI view builder intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Sources/Find/BrowserSearchOverlay.swift`:
- Around line 44-46: The DispatchQueue.main.async call that invokes
logFocusState("request.afterAsync origin=\(origin) remaining=\(maxAttempts)")
should be executed only in debug builds to avoid unnecessary dispatch overhead
in release; wrap the DispatchQueue.main.async { logFocusState(...) } block in an
`#if` DEBUG ... `#endif` so that logFocusState, DispatchQueue.main.async, origin and
maxAttempts are only used for debug instrumentation and omitted from release
builds.
- Around line 18-38: The helper debugFirstResponderSummary() is only used inside
the DEBUG-only body of logFocusState(), leaving it as dead code in release
builds; move or wrap its entire declaration inside the same `#if` DEBUG block as
logFocusState() (or inline its logic into logFocusState()) so
debugFirstResponderSummary() is only compiled in DEBUG builds, keeping release
binaries free of unused symbols while preserving the existing behavior of
logFocusState().
- Around line 138-140: The .onChange(of: isSearchFieldFocused) { focused in
logFocusState(...) } handler in BrowserSearchOverlay.swift should be compiled
only in debug builds; wrap the entire .onChange modifier in a conditional
compilation block (`#if` DEBUG ... `#endif`) so the handler (and its call to
logFocusState) is removed in release builds, ensuring view modifier balance and
that you place the `#endif` immediately after the .onChange(...) call to keep the
SwiftUI view builder intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0281abb5-37f6-471e-992f-18f57c823996
📒 Files selected for processing (3)
Sources/BrowserWindowPortal.swiftSources/Find/BrowserSearchOverlay.swiftSources/Panels/BrowserPanel.swift
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 6831-6841: The early return when another search overlay owns
firstResponder prevents calling terminalSurface?.setFocus(false), leaving this
surface's cursor visually focused; before returning from the block guarded by
window.firstResponder / isSearchOverlayOrDescendant(firstResponder) /
!isCurrentSurfaceSearchResponder(firstResponder), ensure you clear this
surface's focus by invoking terminalSurface?.setFocus(false) (or check
terminalSurface.isFocused and call setFocus(false) only if true) so the terminal
surface visual state is kept in sync when yielding focus to another search
overlay.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 43cfc74f-6397-426c-b5a8-7569ce18285d
📒 Files selected for processing (3)
Sources/Find/SurfaceSearchOverlay.swiftSources/GhosttyTerminalView.swiftSources/Workspace.swift
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 9 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/GhosttyTerminalView.swift">
<violation number="1" location="Sources/GhosttyTerminalView.swift:6868">
P2: Focus intent capture misclassifies terminal focus as `.findField` whenever search is open. That makes later focus restoration jump back to the find field even when the user was actively focused in the terminal.</violation>
<violation number="2" location="Sources/GhosttyTerminalView.swift:6889">
P1: Restoring `.surface` focus should set terminal intent first. Without updating `searchFocusTarget`, `setActive(true)` can immediately re-route focus back to the search field when search is open.</violation>
</file>
<file name="Sources/Panels/BrowserPanel.swift">
<violation number="1" location="Sources/Panels/BrowserPanel.swift:1715">
P1: `focus()` now clears previously tracked in-panel intent by forcing `.webView` before guard checks, which can make activation restore target the web view instead of the address bar/find field.</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.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/Panels/BrowserPanelView.swift (1)
1499-1555:⚠️ Potential issue | 🟠 MajorRecord WebKit focus after the final handoff result, not only the eager attempt.
panel.noteWebViewFocused()only runs when the firstwindow.makeFirstResponder(panel.webView)succeeds. If that call fails butrestoreAddressBarPageFocusIfNeededlater restores page focus, or the fallbackmakeFirstRespondersucceeds, the panel stays marked as.addressBarand later restoration can bounce focus back to the omnibar.💡 Suggested fix
panel.restoreAddressBarPageFocusIfNeeded { restored in guard shouldApplyAddressBarExitFallback(in: window) else { `#if` DEBUG dlog( "browser.focus.addressBar.exit.handoff panel=\(panel.id.uuidString.prefix(5)) " + "result=skip_stale_restore restored=\(restored ? 1 : 0)" ) `#endif` NotificationCenter.default.post(name: .browserDidExitAddressBar, object: panel.id) return } - let hasWebViewResponder = + var hasWebViewResponder = browserFocusResponderChainContains(window.firstResponder, target: panel.webView) if !hasWebViewResponder { let fallbackFocusedWebView = window.makeFirstResponder(panel.webView) + hasWebViewResponder = fallbackFocusedWebView `#if` DEBUG dlog( "browser.focus.addressBar.exit.handoff panel=\(panel.id.uuidString.prefix(5)) " + "fallbackFocusedWebView=\(fallbackFocusedWebView ? 1 : 0) " + "restored=\(restored ? 1 : 0)" ) `#endif` } + if hasWebViewResponder { + panel.noteWebViewFocused() + } NotificationCenter.default.post(name: .browserDidExitAddressBar, object: panel.id) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Panels/BrowserPanelView.swift` around lines 1499 - 1555, The panel currently calls panel.noteWebViewFocused() only when the eager window.makeFirstResponder(panel.webView) succeeds, missing cases where restoreAddressBarPageFocusIfNeeded or the fallback makeFirstResponder actually result in the WebView becoming first responder; update the restoreAddressBarPageFocusIfNeeded completion block to record WebKit focus there as well: after computing hasWebViewResponder (using browserFocusResponderChainContains(window.firstResponder, target: panel.webView)), if hasWebViewResponder call panel.noteWebViewFocused(); otherwise call window.makeFirstResponder(panel.webView) (the existing fallbackFocusedWebView) and if that returns true call panel.noteWebViewFocused(); keep the NotificationCenter.post call as-is. This ensures panel.noteWebViewFocused() is invoked after the final handoff attempt (restore or fallback) rather than only the eager attempt.
♻️ Duplicate comments (1)
Sources/GhosttyTerminalView.swift (1)
6829-6839:⚠️ Potential issue | 🟡 MinorClear Ghostty focus before yielding to a foreign search field.
This early return still skips
terminalSurface?.setFocus(false). If this surface was already marked focused, the cursor can keep blinking while another split’s find field owns first responder.Proposed fix
case .searchField: if let firstResponder = window.firstResponder, isSearchOverlayOrDescendant(firstResponder), !isCurrentSurfaceSearchResponder(firstResponder) { + surfaceView.terminalSurface?.setFocus(false) `#if` DEBUG dlog( "find.restoreSearchFocus.skip surface=\(surfaceShort) target=searchField " + "reason=foreignSearchResponder firstResponder=\(String(describing: firstResponder))" ) `#endif` return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/GhosttyTerminalView.swift` around lines 6829 - 6839, Before returning when a foreign search field is the first responder (the branch checking window.firstResponder along with isSearchOverlayOrDescendant(...) and !isCurrentSurfaceSearchResponder(...)), ensure you clear this surface's focus state by calling terminalSurface?.setFocus(false) (or equivalent) before the early return so the cursor stops blinking; keep the existing debug dlog and return behavior but insert terminalSurface?.setFocus(false) immediately prior to the return to guarantee focus is cleared for this surface.
🤖 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/ContentView.swift`:
- Around line 5492-5495: The current builders always set
CommandPaletteRestoreFocusTarget.intent to a generic .terminal(.surface) or
.browser(.webView), which loses the clicked subcontrol; update the builders that
construct CommandPaletteRestoreFocusTarget (the occurrences at the shown spots
plus the similar blocks around lines 5507-5510 and 5548-5551) to pass through
the actual hit-test subcontrol instead of unconditionally using
.surface/.webView—i.e., use the original hit result or intent variable (the
exact value used to detect the clicked part) when creating
CommandPaletteRestoreFocusTarget(workspaceId:panelId:intent:) and only fall back
to .terminal(.surface)/.browser(.webView) when the hit truly indicates the panel
body, so restoreCommandPaletteFocus can return focus to address-bar/find-field
subcontrols.
In `@Sources/Find/BrowserSearchOverlay.swift`:
- Around line 41-51: The focus request currently sets isSearchFieldFocused in
requestSearchFieldFocus, but retries don't retrigger the confirmed-focus
callback because the `@FocusState` value is already true; instead, wire the
confirmed-focus signal to the actual AppKit responder callback (e.g.,
NSTextViewDelegate.textDidBeginEditing / beginEditing()) or explicitly call the
existing onFieldDidFocus callback from the real focus-change handler.
Concretely: stop relying solely on setting isSearchFieldFocused in
requestSearchFieldFocus; ensure the confirmed-focus callback used by
GhosttyTerminalView (referenced as onFieldDidFocus / beginEditing) is invoked
from the AppKit delegate or from the .onChange handler that observes the real
focus transition (the handler currently at lines ~139–143), or call that same
callback from beginEditing as SurfaceSearchOverlay does, so retries that finally
succeed still signal confirmed focus.
In `@Sources/GhosttyTerminalView.swift`:
- Around line 6886-6909: The current restorePanelFocusIntent(_:) bypasses
existing responder helpers: for the .surface case remove the direct
surfaceView.terminalSurface?.setFocus(true) call and instead call
setActive(true) then applyFirstResponderIfNeeded() so AppKit’s first-responder
logic is honored (leave applyFirstResponderIfNeeded() in place). For the
.findField case, keep the guard and setting searchFocusTarget = .searchField and
setActive(true), but remove the direct terminalSurface.setFocus(false) and the
NotificationCenter.post(name: .ghosttySearchFocus, ... ) call and instead route
through the existing restoreSearchFocus(window:) helper (pass the current
window) so the foreign-search-responder checks are applied; this preserves the
intended behavior while preventing cross-split focus stealing.
In `@Sources/Panels/BrowserPanel.swift`:
- Around line 3279-3294: The code treats searchState != nil as if the find field
is actively focused; change that check to confirm the find UI actually has focus
before returning .findField. In captureFocusIntent (and the analogous block at
3296-3304) replace the plain searchState != nil condition with a focus-aware
check (e.g. searchState != nil &&
Self.responderChainContains(window?.firstResponder, target: findField) or an
equivalent isFirstResponder check against the find field/view), so only when the
find field is the current first responder do we return .browser(.findField);
otherwise fall through to preferredFocusIntent.
- Around line 3325-3328: The switch case for `.findField` currently returns
false when searchState is nil which blocks
restoreFocusIntent(.browser(.findField)); instead always call startFind() (which
is idempotent and will create searchState) and return true. Concretely, remove
or change the guard that returns false and ensure the `.findField` branch simply
calls startFind() and then returns true so explicit restoration can reopen the
find field.
In `@Sources/Panels/TerminalPanel.swift`:
- Around line 209-213: The override restoreFocusIntent currently only handles
the .terminal case and returns false for PanelFocusIntent.panel; modify
restoreFocusIntent to preserve the generic .panel fallback by allowing case
.panel (or a default) to call hostedView.restorePanelFocusIntent or call focus()
as recovery so callers that pass .panel still get a true/focus fallback; update
the guard/logic in restoreFocusIntent to delegate .terminal to
hostedView.restorePanelFocusIntent(target) and handle .panel by delegating to
hostedView.restorePanelFocusIntent or invoking focus() before returning true.
In `@Sources/Workspace.swift`:
- Around line 3141-3145: The restore path currently calls
panel.preferredFocusIntentForActivation() twice and may autofocus the browser
omnibar even when the focus change wasn't user-driven; capture the result of
preferredFocusIntentForActivation() once into a local (e.g. let intent =
panel.preferredFocusIntentForActivation()), pass that same intent to
panel.restoreFocusIntent(...) and only call
maybeAutoFocusBrowserAddressBarOnPanelFocus(browserPanel, trigger:) when intent
indicates an explicit focus intent (or socket-allowed per your socket policy)
and the panel is a BrowserPanel whose intent == .browser(.webView); this
preserves the existing applyTabSelectionNow(...) gating and prevents
CLI/socket-initiated restores from stealing omnibar focus.
---
Outside diff comments:
In `@Sources/Panels/BrowserPanelView.swift`:
- Around line 1499-1555: The panel currently calls panel.noteWebViewFocused()
only when the eager window.makeFirstResponder(panel.webView) succeeds, missing
cases where restoreAddressBarPageFocusIfNeeded or the fallback
makeFirstResponder actually result in the WebView becoming first responder;
update the restoreAddressBarPageFocusIfNeeded completion block to record WebKit
focus there as well: after computing hasWebViewResponder (using
browserFocusResponderChainContains(window.firstResponder, target:
panel.webView)), if hasWebViewResponder call panel.noteWebViewFocused();
otherwise call window.makeFirstResponder(panel.webView) (the existing
fallbackFocusedWebView) and if that returns true call
panel.noteWebViewFocused(); keep the NotificationCenter.post call as-is. This
ensures panel.noteWebViewFocused() is invoked after the final handoff attempt
(restore or fallback) rather than only the eager attempt.
---
Duplicate comments:
In `@Sources/GhosttyTerminalView.swift`:
- Around line 6829-6839: Before returning when a foreign search field is the
first responder (the branch checking window.firstResponder along with
isSearchOverlayOrDescendant(...) and !isCurrentSurfaceSearchResponder(...)),
ensure you clear this surface's focus state by calling
terminalSurface?.setFocus(false) (or equivalent) before the early return so the
cursor stops blinking; keep the existing debug dlog and return behavior but
insert terminalSurface?.setFocus(false) immediately prior to the return to
guarantee focus is cleared for this surface.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 47cf33bb-a723-4f53-9886-a9bbead2604d
📒 Files selected for processing (9)
Sources/BrowserWindowPortal.swiftSources/ContentView.swiftSources/Find/BrowserSearchOverlay.swiftSources/GhosttyTerminalView.swiftSources/Panels/BrowserPanel.swiftSources/Panels/BrowserPanelView.swiftSources/Panels/Panel.swiftSources/Panels/TerminalPanel.swiftSources/Workspace.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- Sources/BrowserWindowPortal.swift
Comment on lines
+41
to
51
| private func requestSearchFieldFocus(maxAttempts: Int = 3, origin: String) { | ||
| guard maxAttempts > 0 else { return } | ||
| logFocusState("request.begin origin=\(origin) remaining=\(maxAttempts)") | ||
| isSearchFieldFocused = true | ||
| DispatchQueue.main.async { | ||
| logFocusState("request.afterAsync origin=\(origin) remaining=\(maxAttempts)") | ||
| } | ||
| guard maxAttempts > 1 else { return } | ||
| DispatchQueue.main.asyncAfter(deadline: .now() + 0.05) { | ||
| requestSearchFieldFocus(maxAttempts: maxAttempts - 1) | ||
| requestSearchFieldFocus(maxAttempts: maxAttempts - 1, origin: origin) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd "BrowserSearchOverlay.swift" --type fRepository: manaflow-ai/cmux
Length of output: 99
🏁 Script executed:
cat -n Sources/Find/BrowserSearchOverlay.swiftRepository: manaflow-ai/cmux
Length of output: 10467
🏁 Script executed:
rg "onFieldDidFocus" -A 3 -B 3Repository: manaflow-ai/cmux
Length of output: 6250
🌐 Web query:
SwiftUI @focusstate onChange fires when focus actually lands or when state is written
💡 Result:
.onChange(of: someFocusState) runs when the @FocusState wrapped value actually changes (i.e., when SwiftUI/OS reports that focus entered/left a bound view), not at the instant you assign to it. Setting a @FocusState value is effectively a request to move focus; the wrapped value may update later (often next update cycle / after animations), so reading it immediately after writing can still show the old value. [1], [2], [3]
In other words:
- User taps / focus moves → SwiftUI updates
@FocusState→ your.onChangefires. [3] - You assign to
@FocusState→ SwiftUI tries to move focus → when focus actually changes,@FocusStateupdates and then.onChangefires (not necessarily synchronously with your assignment). [1], [2]
Sources: [1] [2] [3]
onFieldDidFocus fires on the initial @FocusState write, but retries won't re-trigger the callback even if focus finally lands.
Setting isSearchFieldFocused = true (line 44) is a focus request; the .onChange at lines 139–143 fires when focus actually lands and the state transitions from false to true. However, on retry attempts (lines 49–50), isSearchFieldFocused is already true, so the state won't transition again. If the initial focus request fails and focus lands on a retry, the callback never fires, even though the field is now confirmed as first responder.
This breaks the downstream contract in GhosttyTerminalView.swift, which expects the callback to signal confirmed focus and unfocus the terminal surface. Compare to SurfaceSearchOverlay.swift, which correctly calls the callback from beginEditing() — an AppKit confirmed focus callback.
Use an AppKit responder callback (e.g., NSTextViewDelegate.textDidBeginEditing) to signal confirmed focus, not a @FocusState write.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/Find/BrowserSearchOverlay.swift` around lines 41 - 51, The focus
request currently sets isSearchFieldFocused in requestSearchFieldFocus, but
retries don't retrigger the confirmed-focus callback because the `@FocusState`
value is already true; instead, wire the confirmed-focus signal to the actual
AppKit responder callback (e.g., NSTextViewDelegate.textDidBeginEditing /
beginEditing()) or explicitly call the existing onFieldDidFocus callback from
the real focus-change handler. Concretely: stop relying solely on setting
isSearchFieldFocused in requestSearchFieldFocus; ensure the confirmed-focus
callback used by GhosttyTerminalView (referenced as onFieldDidFocus /
beginEditing) is invoked from the AppKit delegate or from the .onChange handler
that observes the real focus transition (the handler currently at lines
~139–143), or call that same callback from beginEditing as SurfaceSearchOverlay
does, so retries that finally succeed still signal confirmed focus.
There was a problem hiding this comment.
1 issue found across 6 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/ContentView.swift">
<violation number="1" location="Sources/ContentView.swift:5337">
P2: When both optional UUIDs are `nil`, `nil == nil` is `true` in Swift, so this can return `true` even when no address bar was focused. Guard against this by requiring `focusedBrowserAddressBarPanelId` to be non-nil.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| focusedPanelId: UUID | ||
| focusedPanelId: UUID? | ||
| ) -> Bool { | ||
| focusedPanelIsBrowser && focusedBrowserAddressBarPanelId == focusedPanelId |
There was a problem hiding this comment.
P2: When both optional UUIDs are nil, nil == nil is true in Swift, so this can return true even when no address bar was focused. Guard against this by requiring focusedBrowserAddressBarPanelId to be non-nil.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/ContentView.swift, line 5337:
<comment>When both optional UUIDs are `nil`, `nil == nil` is `true` in Swift, so this can return `true` even when no address bar was focused. Guard against this by requiring `focusedBrowserAddressBarPanelId` to be non-nil.</comment>
<file context>
@@ -5329,6 +5329,14 @@ struct ContentView: View {
+ focusedBrowserAddressBarPanelId: UUID?,
+ focusedPanelId: UUID?
+ ) -> Bool {
+ focusedPanelIsBrowser && focusedBrowserAddressBarPanelId == focusedPanelId
+ }
+
</file context>
Suggested change
| focusedPanelIsBrowser && focusedBrowserAddressBarPanelId == focusedPanelId | |
| focusedPanelIsBrowser && focusedBrowserAddressBarPanelId != nil && focusedBrowserAddressBarPanelId == focusedPanelId |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
Sources/ContentView.swift (1)
5500-5505:⚠️ Potential issue | 🟠 MajorBackdrop hits still lose the clicked subcontrol intent.
These call sites now delegate to
commandPaletteRestoreFocusTarget(...), but that helper recomputes intent fromcaptureFocusIntent(in:)while the palette field is still first responder. So a backdrop click on a browser address bar, browser find field, or terminal find field can still collapse back to.browser(.webView)/.terminal(.surface)instead of restoring the clicked control. Please thread the hit-derivedPanelFocusIntentthrough here, and only fall back to capture/default intent when the hit was actually the panel body.Also applies to: 5516-5521, 5558-5563, 5569-5579
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ContentView.swift` around lines 5500 - 5505, The helper commandPaletteRestoreFocusTarget(...) currently recomputes intent via captureFocusIntent(in:) which loses the actual subcontrol clicked; change the call sites that currently call commandPaletteRestoreFocusTarget(workspaceId:panelId:fallbackIntent:in:) to pass the hit-derived PanelFocusIntent (from the backdrop hit) into the helper and update commandPaletteRestoreFocusTarget to accept an optional hitIntent parameter; inside commandPaletteRestoreFocusTarget, use the provided hitIntent when the hit is not the panel body and only call captureFocusIntent(in:) or use fallbackIntent when the hit indicates the panel body or hitIntent is nil. Ensure references to PanelFocusIntent and captureFocusIntent(in:) are used exactly as named so reviewers can locate edits.
🤖 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 6866-6880: In capturePanelFocusIntent, when a search is open
(surfaceView.terminalSurface?.searchState != nil) ensure a pending search-field
intent is preserved by checking searchFocusTarget == .searchField (or
isCurrentSurfaceSearchResponder(firstResponder)) before treating surfaceView
being firstResponder as meaning .surface; specifically, if firstResponder is
surfaceView or a descendant but searchFocusTarget == .searchField (or
isCurrentSurfaceSearchResponder returns true), return .findField instead of
.surface so the pending find-field intent isn't discarded.
- Around line 6979-6990: isCurrentSurfaceSearchResponder currently treats any
descendant of the surface as a search responder; restrict it to the actual
search-overlay controls by resolving the responder (as you already do) and then
verifying that the resolvedResponder is either the search overlay view or the
actual search field/editor instances instead of any descendant. Concretely, in
isCurrentSurfaceSearchResponder(_:) after computing resolvedResponder, check
whether it equals or isDescendant(of:) the specific search overlay container
(e.g. your search overlay view instance) or is the known search field/editor
control(s) (e.g. the find/search NSTextField or its field editor) used by your
search UI, and return true only in those cases so capturePanelFocusIntent(...)
and resignOwnedFirstResponderIfNeeded(...) only treat real search responders as
active.
In `@Sources/Panels/BrowserPanel.swift`:
- Around line 3333-3337: The .webView branch calls noteWebViewFocused() then
focus() but focus() currently returns early when searchState != nil, so
restoring from a captured .browser(.webView) reports success but leaves the page
unfocused; update the logic so focusing honors .webView restores by either
adding a parameter to focus(e.g. force: Bool) or a new method (e.g.
forceFocusWebView()) that bypasses the searchState early-exit and directly makes
the web view first responder, or modify focus() to check the restoration target
(from the switch) and skip the searchState bail when called for .webView; ensure
you adjust the call in the switch (currently calling noteWebViewFocused() and
focus()) to use the new force/ignore-search variant so the page actually becomes
focused.
---
Duplicate comments:
In `@Sources/ContentView.swift`:
- Around line 5500-5505: The helper commandPaletteRestoreFocusTarget(...)
currently recomputes intent via captureFocusIntent(in:) which loses the actual
subcontrol clicked; change the call sites that currently call
commandPaletteRestoreFocusTarget(workspaceId:panelId:fallbackIntent:in:) to pass
the hit-derived PanelFocusIntent (from the backdrop hit) into the helper and
update commandPaletteRestoreFocusTarget to accept an optional hitIntent
parameter; inside commandPaletteRestoreFocusTarget, use the provided hitIntent
when the hit is not the panel body and only call captureFocusIntent(in:) or use
fallbackIntent when the hit indicates the panel body or hitIntent is nil. Ensure
references to PanelFocusIntent and captureFocusIntent(in:) are used exactly as
named so reviewers can locate edits.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 137fd52d-862d-418a-af6d-015372abe13f
📒 Files selected for processing (8)
Sources/ContentView.swiftSources/Find/BrowserSearchOverlay.swiftSources/GhosttyTerminalView.swiftSources/Panels/BrowserPanel.swiftSources/Panels/BrowserPanelView.swiftSources/Panels/Panel.swiftSources/Panels/TerminalPanel.swiftSources/Workspace.swift
🚧 Files skipped from review as they are similar to previous changes (2)
- Sources/Find/BrowserSearchOverlay.swift
- Sources/Panels/Panel.swift
Comment on lines
+6866
to
+6880
| func capturePanelFocusIntent(in window: NSWindow?) -> TerminalPanelFocusIntent { | ||
| if surfaceView.terminalSurface?.searchState != nil { | ||
| if let firstResponder = window?.firstResponder as? NSView, | ||
| (firstResponder === surfaceView || firstResponder.isDescendant(of: surfaceView)) { | ||
| return .surface | ||
| } | ||
| if let firstResponder = window?.firstResponder, | ||
| isCurrentSurfaceSearchResponder(firstResponder) { | ||
| return .findField | ||
| } | ||
| if searchFocusTarget == .searchField { | ||
| return .findField | ||
| } | ||
| } | ||
| return .surface |
There was a problem hiding this comment.
Preserve pending find-field intent while AppKit focus is still catching up.
When the find bar is open, AppKit can still report surfaceView as first responder for a turn before the search field claim lands. Returning .surface in that state discards the already-selected .searchField intent, so the next activation can restore terminal focus instead of the find field.
💡 Suggested fix
func capturePanelFocusIntent(in window: NSWindow?) -> TerminalPanelFocusIntent {
if surfaceView.terminalSurface?.searchState != nil {
+ if searchFocusTarget == .searchField {
+ return .findField
+ }
if let firstResponder = window?.firstResponder as? NSView,
(firstResponder === surfaceView || firstResponder.isDescendant(of: surfaceView)) {
return .surface
}
if let firstResponder = window?.firstResponder,
isCurrentSurfaceSearchResponder(firstResponder) {
return .findField
}
- if searchFocusTarget == .searchField {
- return .findField
- }
}
return .surface
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/GhosttyTerminalView.swift` around lines 6866 - 6880, In
capturePanelFocusIntent, when a search is open
(surfaceView.terminalSurface?.searchState != nil) ensure a pending search-field
intent is preserved by checking searchFocusTarget == .searchField (or
isCurrentSurfaceSearchResponder(firstResponder)) before treating surfaceView
being firstResponder as meaning .surface; specifically, if firstResponder is
surfaceView or a descendant but searchFocusTarget == .searchField (or
isCurrentSurfaceSearchResponder returns true), return .findField instead of
.surface so the pending find-field intent isn't discarded.
Contributor
Author
|
@codex review |
![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: 579d18fca9
ℹ️ 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".
| let firstResponderSummary: String = { | ||
| guard let firstResponder = window?.firstResponder else { return "nil" } | ||
| if let editor = firstResponder as? NSTextView, editor.isFieldEditor { | ||
| let delegateSummary = editor.delegate.map { String(describing: type(of: $0)) } ?? "nil" |
There was a problem hiding this comment.
NSTextView.delegate is an unsafe-unretained AppKit property, so reading it during responder teardown can crash with an unowned-reference trap; this helper now does that on every browser find-overlay log path. Since overlay mount/remove frequently coincides with first-responder churn, this reintroduces the exact crash pattern already called out elsewhere in the codebase (see the guard comment in AppDelegate around field-editor delegate access).
Useful? React with 👍 / 👎.
This was referenced Mar 11, 2026
This was referenced Mar 18, 2026
This was referenced Mar 25, 2026
3 tasks done
bn-l
pushed a commit
to bn-l/cmux
that referenced
this pull request
Apr 3, 2026
* Add browser find focus debug logs * Allow browser find bar focus in portal host * Add split and terminal find debug logs * Avoid stealing search focus across splits * Generalize panel focus restore intent * Unify split focus intent activation * Address focus restore review feedback * Yield inactive panel focus before restore * Gate browser find focus retries by generation * Avoid repeated browser focus invalidation * Keep browser find ownership while find bar is open
This was referenced Apr 22, 2026
This was referenced Apr 30, 2026
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
Testing
./scripts/reload.sh --tag browser-find-focus-logs(build succeeded and launched the tagged app)Issues
Summary by cubic
Adds debug logs and a unified, intent-driven focus system that keeps the browser Cmd+F search field owned by its panel while the find bar is open and prevents focus from jumping across splits. Focus capture, handoff, and restore are now consistent across browser and terminal panels, including command palette dismiss and tab/pane activation, addressing the Cmd+F search field focus failure.
New Features
PanelFocusIntentfor terminal (surface/findField) and browser (webView/addressBar/findField) with capture/preferred/prepare/restore plus owned/yield semantics;Workspaceyields foreign focus and restores the right target on pane/tab activation and command palette dismiss.BrowserPaneltrackspreferredFocusIntentandsearchFocusRequestGeneration; overlays receivefocusRequestGeneration/canApplyFocusRequest, emitonFieldDidFocus, and the portal host associates overlays with their owning panel and can yield overlay focus.Bug Fixes
WKWebView, allowing overlay text fields to become first responder.Written for commit 579d18f. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Chores