Fix #2347 terminal focus and surface recovery#2354
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds guarded runtime surface creation and active input-recovery paths: keyDown now triggers background surface startup when the runtime surface is missing; focus reassertion is gated by portal lifecycle and view/window eligibility; debug helpers and regression tests for detached/suppressed-focus scenarios were added. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant GNV as GhosttyNSView
participant ES as ensureSurfaceReadyForInput
participant Recovery as requestInputRecoveryAfterSurfaceMiss
participant TS as TerminalSurface
participant BG as BackgroundStarter
User->>GNV: keyDown
GNV->>ES: ensureSurfaceReadyForInput()
ES-->>GNV: fails (surface == nil)
GNV->>Recovery: requestInputRecoveryAfterSurfaceMiss(reason)
Recovery->>TS: set desiredFocus / recordExternalFocusState(true)
Recovery->>TS: requestBackgroundSurfaceStartIfNeeded()
TS->>BG: start background surface
BG-->>TS: surface recreated
TS-->>GNV: surface available / focus reasserted
GNV-->>User: input processed (after recovery)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Greptile SummaryThis PR fixes two related terminal focus/input recovery bugs (issue #2347): (1) when a Both code paths are covered by new debug-only regression tests following the repo's two-commit CI policy, and a small
Confidence Score: 5/5Safe to merge — both code paths have regression test coverage, all remaining findings are minor style/cleanup P2 items. The logic changes are well-scoped: the keyDown recovery path is guarded by the existing No files require special attention beyond the minor style items in Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant GhosttyNSView
participant TerminalSurface
participant Ghostty
Note over GhosttyNSView,TerminalSurface: Scenario 1 — keyDown with missing surface
User->>GhosttyNSView: keyDown(event)
GhosttyNSView->>GhosttyNSView: ensureSurfaceReadyForInput()
GhosttyNSView-->>GhosttyNSView: nil (surface absent)
GhosttyNSView->>GhosttyNSView: requestInputRecoveryAfterSurfaceMiss()
GhosttyNSView->>TerminalSurface: recordExternalFocusState(true)
GhosttyNSView->>TerminalSurface: requestBackgroundSurfaceStartIfNeeded()
TerminalSurface-->>Ghostty: createSurface() [async, next run-loop]
Ghostty-->>TerminalSurface: surface ready
Note over GhosttySurfaceScrollView,TerminalSurface: Scenario 2 — reparent focus recovery
participant GhosttySurfaceScrollView
GhosttySurfaceScrollView->>GhosttySurfaceScrollView: clearSuppressReparentFocus()
GhosttySurfaceScrollView->>GhosttySurfaceScrollView: suppressingReparentFocus = false
GhosttySurfaceScrollView->>GhosttySurfaceScrollView: check guards (desiredFocus, firstResponder, active, visible, keyWindow, geometry)
alt all guards pass
GhosttySurfaceScrollView->>GhosttySurfaceScrollView: reassertTerminalSurfaceFocus()
alt surface == nil
GhosttySurfaceScrollView->>TerminalSurface: requestBackgroundSurfaceStartIfNeeded()
end
GhosttySurfaceScrollView->>TerminalSurface: setFocus(true)
TerminalSurface->>Ghostty: ghostty_surface_set_focus(true)
else hidden or too small
GhosttySurfaceScrollView->>GhosttySurfaceScrollView: scheduleAutomaticFirstResponderApply()
end
Reviews (1): Last reviewed commit: "Fix issue #2347 terminal focus and surfa..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } | ||
|
|
||
| #if DEBUG | ||
| #if DEBUG |
There was a problem hiding this comment.
#if DEBUG / #endif indentation
The #if DEBUG directive was de-indented from 4 spaces to column 0, but its matching #endif on line 3499 ( #endif) is still indented with 4 spaces. All other #if DEBUG / #endif pairs in this file use consistent column-0 placement. The compiler is fine with mixed indentation, but it's inconsistent with the rest of the file.
| #if DEBUG | |
| #if DEBUG |
| return size.width > 1 && size.height > 1 | ||
| }() | ||
| let isHiddenForFocus = isHiddenOrHasHiddenAncestor || surfaceView.isHiddenOrHasHiddenAncestor | ||
| let surfaceShort = surfaceView.terminalSurface?.id.uuidString.prefix(5) ?? "nil" |
There was a problem hiding this comment.
surfaceShort computed before early-return guards
surfaceShort is only consumed inside #if DEBUG blocks further down, yet it is computed eagerly here — before the four guard statements that will short-circuit the function in the common "nothing to do" case. In release builds this is a dead allocation on every non-matching call.
Consider wrapping the declaration:
#if DEBUG
let surfaceShort = surfaceView.terminalSurface?.id.uuidString.prefix(5) ?? "nil"
#endifhasUsablePortalGeometry and isHiddenForFocus are used in the non-debug guard so they need to stay where they are — only surfaceShort is affected.
![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
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)
5707-5713:⚠️ Potential issue | 🟠 MajorThe recovery keypress is still lost.
On Line 5712,
super.keyDown(with:)falls back into the NSTextInputClient path, butinsertText(_:, replacementRange:)ultimately callssendTextToSurface(_:), which returns immediately whilesurface == nil. The async restart brings the terminal back, but the key that triggered recovery never reaches it, so users still need a second keypress after the miss.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/GhosttyTerminalView.swift` around lines 5707 - 5713, The keypress is lost because you call super.keyDown(with:) when surface == nil and the NSTextInputClient path drops the input in sendTextToSurface(_:) which returns while the surface is nil; instead, do not call super.keyDown(with:) in the guard-fail path—capture and buffer the original NSEvent (or derived text) into a pending recovery slot (e.g. pendingRecoveryKeypress), call requestInputRecoveryAfterSurfaceMiss(reason:) and start the async recovery, and when ensureSurfaceReadyForInput() / your surface-ready callback runs, replay the buffered event by calling the same code path that delivers text to the surface (e.g. call sendTextToSurface(_:) or insertText(_:replacementRange:) once surface is non-nil); ensure the buffer is cleared and access is thread-safe.
🤖 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 8884-8911: The current path clears suppressingReparentFocus and
reasserts only the terminal surface (reassertTerminalSurfaceFocus) which
restores AppKit first responder but does not restore Bonsplit/pane focus state
or invoke the surfaceView's onFocus callback; update the code that runs when
suppression is removed (the block after the guards where
reassertTerminalSurfaceFocus is called) to also restore pane focus state by
calling the same method used elsewhere to set Bonsplit/current-panel (e.g.
focusPanel(...) or the internal function that updates Bonsplit state) and invoke
surfaceView.onFocus?() (or the equivalent onFocus handler) so the pane-level
focus state and any on-focus side effects run in addition to
reassertTerminalSurfaceFocus.
---
Outside diff comments:
In `@Sources/GhosttyTerminalView.swift`:
- Around line 5707-5713: The keypress is lost because you call
super.keyDown(with:) when surface == nil and the NSTextInputClient path drops
the input in sendTextToSurface(_:) which returns while the surface is nil;
instead, do not call super.keyDown(with:) in the guard-fail path—capture and
buffer the original NSEvent (or derived text) into a pending recovery slot (e.g.
pendingRecoveryKeypress), call requestInputRecoveryAfterSurfaceMiss(reason:) and
start the async recovery, and when ensureSurfaceReadyForInput() / your
surface-ready callback runs, replay the buffered event by calling the same code
path that delivers text to the surface (e.g. call sendTextToSurface(_:) or
insertText(_:replacementRange:) once surface is non-nil); ensure the buffer is
cleared and access is thread-safe.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cf26f198-c8e9-4143-baba-124e82edf87e
📒 Files selected for processing (3)
Sources/GhosttyTerminalView.swiftcmuxTests/TerminalAndGhosttyTests.swiftcmuxTests/WorkspaceUnitTests.swift
| surfaceView.suppressingReparentFocus = false | ||
| let hasUsablePortalGeometry: Bool = { | ||
| let size = bounds.size | ||
| return size.width > 1 && size.height > 1 | ||
| }() | ||
| let isHiddenForFocus = isHiddenOrHasHiddenAncestor || surfaceView.isHiddenOrHasHiddenAncestor | ||
| let surfaceShort = surfaceView.terminalSurface?.id.uuidString.prefix(5) ?? "nil" | ||
|
|
||
| guard surfaceView.desiredFocus else { return } | ||
| guard isSurfaceViewFirstResponder() else { return } | ||
| guard isActive else { return } | ||
| guard surfaceView.isVisibleInUI else { return } | ||
| guard let window, window.isKeyWindow else { return } | ||
| guard !isHiddenForFocus, hasUsablePortalGeometry else { | ||
| #if DEBUG | ||
| dlog( | ||
| "focus.reparent.resume.defer surface=\(surfaceShort) " + | ||
| "reason=hidden_or_tiny hidden=\(isHiddenForFocus ? 1 : 0) " + | ||
| "frame=\(String(format: "%.1fx%.1f", bounds.width, bounds.height))" | ||
| ) | ||
| #endif | ||
| scheduleAutomaticFirstResponderApply(reason: "clearSuppressReparentFocus.hiddenOrTiny") | ||
| return | ||
| } | ||
| #if DEBUG | ||
| dlog("focus.reparent.resume surface=\(surfaceShort) firstResponder=\(String(describing: window.firstResponder))") | ||
| #endif | ||
| reassertTerminalSurfaceFocus(reason: "clearSuppressReparentFocus") |
There was a problem hiding this comment.
This path restores Ghostty focus but not pane focus state.
If suppression masked the original becomeFirstResponder(), onFocus?() never ran. Reasserting only the Ghostty surface here can leave Bonsplit/current-panel state pointing at the pane chosen by focusPanel(...) while AppKit first responder and terminal input stay on this surface.
💡 Suggested fix
guard !isHiddenForFocus, hasUsablePortalGeometry else {
`#if` DEBUG
dlog(
"focus.reparent.resume.defer surface=\(surfaceShort) " +
"reason=hidden_or_tiny hidden=\(isHiddenForFocus ? 1 : 0) " +
"frame=\(String(format: "%.1fx%.1f", bounds.width, bounds.height))"
)
`#endif`
scheduleAutomaticFirstResponderApply(reason: "clearSuppressReparentFocus.hiddenOrTiny")
return
}
+ surfaceView.onFocus?()
`#if` DEBUG
dlog("focus.reparent.resume surface=\(surfaceShort) firstResponder=\(String(describing: window.firstResponder))")
`#endif`
reassertTerminalSurfaceFocus(reason: "clearSuppressReparentFocus")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/GhosttyTerminalView.swift` around lines 8884 - 8911, The current path
clears suppressingReparentFocus and reasserts only the terminal surface
(reassertTerminalSurfaceFocus) which restores AppKit first responder but does
not restore Bonsplit/pane focus state or invoke the surfaceView's onFocus
callback; update the code that runs when suppression is removed (the block after
the guards where reassertTerminalSurfaceFocus is called) to also restore pane
focus state by calling the same method used elsewhere to set
Bonsplit/current-panel (e.g. focusPanel(...) or the internal function that
updates Bonsplit state) and invoke surfaceView.onFocus?() (or the equivalent
onFocus handler) so the pane-level focus state and any on-focus side effects run
in addition to reassertTerminalSurfaceFocus.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Sources/GhosttyTerminalView.swift (1)
8908-8936:⚠️ Potential issue | 🟠 MajorReplay pane focus before reasserting Ghostty focus.
When reparent suppression swallowed
becomeFirstResponder(), this block restores Ghostty focus but never replayssurfaceView.onFocus?(), so Bonsplit/current-panel state can stay stale even though AppKit first responder is back on the terminal.💡 Proposed fix
guard !isHiddenForFocus, hasUsablePortalGeometry else { `#if` DEBUG dlog( "focus.reparent.resume.defer surface=\(surfaceShort) " + "reason=hidden_or_tiny hidden=\(isHiddenForFocus ? 1 : 0) " + "frame=\(String(format: "%.1fx%.1f", bounds.width, bounds.height))" ) `#endif` scheduleAutomaticFirstResponderApply(reason: "clearSuppressReparentFocus.hiddenOrTiny") return } + surfaceView.onFocus?() `#if` DEBUG dlog("focus.reparent.resume surface=\(surfaceShort) firstResponder=\(String(describing: window.firstResponder))") `#endif` reassertTerminalSurfaceFocus(reason: "clearSuppressReparentFocus")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/GhosttyTerminalView.swift` around lines 8908 - 8936, clearSuppressReparentFocus currently reasserts terminal focus but doesn't replay the suppressed focus callback, leaving Bonsplit/current-panel state stale; update clearSuppressReparentFocus to invoke the surfaceView focus handler (surfaceView.onFocus?() / appropriate onFocus method) or otherwise replay the focus event on surfaceView before calling reassertTerminalSurfaceFocus (and keep the existing early-exit checks and scheduleAutomaticFirstResponderApply behavior intact) so UI state is updated when reparent suppression previously swallowed becomeFirstResponder().
🧹 Nitpick comments (1)
cmuxTests/TerminalAndGhosttyTests.swift (1)
1693-1700: Harden the negative assertion window for closed-surface recovery.A single main-queue drain can miss delayed async recreation. Prefer an inverted expectation over a short interval to catch late regressions deterministically.
♻️ Suggested test hardening
- let drained = expectation(description: "background recovery drained") - DispatchQueue.main.async { drained.fulfill() } - wait(for: [drained], timeout: 1.0) + let recreated = XCTNSPredicateExpectation( + predicate: NSPredicate { _, _ in + surface.surface != nil + }, + object: NSObject() + ) + recreated.isInverted = true + wait(for: [recreated], timeout: 0.25)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/TerminalAndGhosttyTests.swift` around lines 1693 - 1700, Replace the single main-queue drain/expectation with an inverted expectation that waits a short interval to catch any delayed recreation: create an expectation (e.g., "no-surface-recreation") set isInverted = true, observe or poll surface.surface (the Ghostty runtime surface) and fulfill the expectation if it becomes non-nil, then wait for that short timeout; after the wait assert that surface.surface is still nil to ensure a late async keyDown recovery did not recreate the Ghostty 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/GhosttyTerminalView.swift`:
- Around line 5064-5072: requestInputRecoveryAfterSurfaceMiss(reason:) currently
only queues surface recreation but doesn't propagate the intended focus state,
so createSurface(for:) will initialize the new TerminalSurface from
TerminalSurface.desiredFocusState (which may be stale) instead of
GhosttyNSView.desiredFocus; update requestInputRecoveryAfterSurfaceMiss(reason:)
to mirror setFocus(true) used by reassertTerminalSurfaceFocus(reason:) by
explicitly propagating the view's desired focus intent to the recreated surface
(e.g., call the same focus propagation path or set the new surface's desired
focus to GhosttyNSView.desiredFocus / call setFocus(true) logic) so the
recreated surface receives first-responder intent; apply the same change to the
second occurrence of this helper (the block also present around the 5732-5738
area).
---
Duplicate comments:
In `@Sources/GhosttyTerminalView.swift`:
- Around line 8908-8936: clearSuppressReparentFocus currently reasserts terminal
focus but doesn't replay the suppressed focus callback, leaving
Bonsplit/current-panel state stale; update clearSuppressReparentFocus to invoke
the surfaceView focus handler (surfaceView.onFocus?() / appropriate onFocus
method) or otherwise replay the focus event on surfaceView before calling
reassertTerminalSurfaceFocus (and keep the existing early-exit checks and
scheduleAutomaticFirstResponderApply behavior intact) so UI state is updated
when reparent suppression previously swallowed becomeFirstResponder().
---
Nitpick comments:
In `@cmuxTests/TerminalAndGhosttyTests.swift`:
- Around line 1693-1700: Replace the single main-queue drain/expectation with an
inverted expectation that waits a short interval to catch any delayed
recreation: create an expectation (e.g., "no-surface-recreation") set isInverted
= true, observe or poll surface.surface (the Ghostty runtime surface) and
fulfill the expectation if it becomes non-nil, then wait for that short timeout;
after the wait assert that surface.surface is still nil to ensure a late async
keyDown recovery did not recreate the Ghostty surface.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 97179386-43c4-4f7c-ad92-d80c0c05942e
📒 Files selected for processing (2)
Sources/GhosttyTerminalView.swiftcmuxTests/TerminalAndGhosttyTests.swift
| private func requestInputRecoveryAfterSurfaceMiss(reason: String) { | ||
| terminalSurface?.requestBackgroundSurfaceStartIfNeeded() | ||
| #if DEBUG | ||
| dlog( | ||
| "focus.input_recovery surface=\(terminalSurface?.id.uuidString.prefix(5) ?? "nil") " + | ||
| "reason=\(reason) inWindow=\(window != nil ? 1 : 0)" | ||
| ) | ||
| #endif | ||
| } |
There was a problem hiding this comment.
Propagate focus intent on the missing-surface key path.
This helper only queues recreation, but createSurface(for:) initializes the new runtime surface from TerminalSurface.desiredFocusState, not GhosttyNSView.desiredFocus. If first-responder acquisition happened while the runtime surface was nil, that state is still stale here, so the recreated surface can come back unfocused and the next key still goes nowhere. Mirror the setFocus(true) behavior that reassertTerminalSurfaceFocus(reason:) already uses.
💡 Proposed fix
private func requestInputRecoveryAfterSurfaceMiss(reason: String) {
+ desiredFocus = true
+ terminalSurface?.setFocus(true)
terminalSurface?.requestBackgroundSurfaceStartIfNeeded()
`#if` DEBUG
dlog(
"focus.input_recovery surface=\(terminalSurface?.id.uuidString.prefix(5) ?? "nil") " +Also applies to: 5732-5738
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/GhosttyTerminalView.swift` around lines 5064 - 5072,
requestInputRecoveryAfterSurfaceMiss(reason:) currently only queues surface
recreation but doesn't propagate the intended focus state, so
createSurface(for:) will initialize the new TerminalSurface from
TerminalSurface.desiredFocusState (which may be stale) instead of
GhosttyNSView.desiredFocus; update requestInputRecoveryAfterSurfaceMiss(reason:)
to mirror setFocus(true) used by reassertTerminalSurfaceFocus(reason:) by
explicitly propagating the view's desired focus intent to the recreated surface
(e.g., call the same focus propagation path or set the new surface's desired
focus to GhosttyNSView.desiredFocus / call setFocus(true) logic) so the
recreated surface receives first-responder intent; apply the same change to the
second occurrence of this helper (the block also present around the 5732-5738
area).
…#2354) * Add regressions for issue manaflow-ai#2347 terminal focus loss * Fix issue manaflow-ai#2347 terminal focus and surface recovery * Add regressions for missing-surface recovery review cases * Fix missing-surface recovery lifecycle and focus replay
1 participant
Summary
Testing
Summary by cubic
Fixes #2347 by reliably recovering terminal input when the surface is briefly missing and safely restoring focus after reparent suppression. Adds lifecycle guards to avoid recreating closed surfaces and prevents stale focus replay.
Bug Fixes
allowsRuntimeSurfaceCreation()so closed/closing portals aren’t recreated.Tests
debugDesiredFocusState()included).Written for commit 5686090. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests