Fix terminals freezing when first responder drifts#2505
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 key-down focus-repair logic invoked from a swizzled Changes
Sequence Diagram(s)sequenceDiagram
participant Window as NSWindow
participant App as AppDelegate
participant WS as WorkspaceResolver
participant Panel as TerminalPanel
participant Ghost as GhosttyNSView
Window->>App: cmux_sendEvent(event: keyDown)
App->>App: repairFocusedTerminalKeyboardRoutingIfNeeded(window,event)
alt event passes gates
App->>WS: resolve active workspace + focused panel
WS->>Panel: locate focused terminal panel
Panel->>Ghost: hostedView.ensureFocus(workspace.id, panelId)
Ghost-->>App: focus ensured / no-op if already matching
else gates fail
App-->>Window: continue normal sendEvent
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
Greptile SummaryThis PR fixes terminal-freezing caused by the macOS first-responder drifting away from the focused Ghostty surface (to Key changes:
Confidence Score: 5/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[NSWindow.sendEvent keyDown] --> B{isMainTerminalWindow?}
B -- No --> Z[cmux_sendEvent no repair]
B -- Yes --> C{attachedSheet?}
C -- Yes --> Z
C -- No --> D{commandPaletteVisible?}
D -- Yes --> Z
D -- No --> E{focusedTerminalPanel found?}
E -- No --> Z
E -- Yes --> F{searchState active?}
F -- Yes --> Z
F -- No --> G[responderNeedsFocusedTerminalKeyRepair]
G --> H{firstResponder nil\nor NSWindow?}
H -- Yes --> REPAIR
H -- No --> I{cmuxOwningGhosttyView found?}
I -- Yes --> J{wrong window\nor hidden?}
J -- Yes --> REPAIR
J -- No --> K[return false]
I -- No --> L{keyRoutingOwnerView found?}
L -- No --> K
L -- Yes --> M{wrong window / hidden\n/ no superview?}
M -- Yes --> REPAIR
M -- No --> K
K --> Z
REPAIR[ensureFocus → restore firstResponder] --> Z
Z --> N[Event dispatched to correct first responder]
Reviews (1): Last reviewed commit: "Repair focused terminal key routing afte..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
cmuxTests/AppDelegateShortcutRoutingTests.swift (1)
3435-3441: Restore prior debug observer instead of always clearing tonilLine 3435 overwrites a static global observer, and Line 3440 always clears it to
nil. That can accidentally drop an existing observer and create cross-test interference. Save/restore the previous value indefer.Suggested patch
`#if` DEBUG var forwardedKeyDownCount = 0 + let previousKeyEventObserver = GhosttyNSView.debugGhosttySurfaceKeyEventObserver GhosttyNSView.debugGhosttySurfaceKeyEventObserver = { keyEvent in + previousKeyEventObserver?(keyEvent) guard keyEvent.action == GHOSTTY_ACTION_PRESS, keyEvent.keycode == 0 else { return } forwardedKeyDownCount += 1 } defer { - GhosttyNSView.debugGhosttySurfaceKeyEventObserver = nil + GhosttyNSView.debugGhosttySurfaceKeyEventObserver = previousKeyEventObserver } `#endif`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/AppDelegateShortcutRoutingTests.swift` around lines 3435 - 3441, The test overwrites the global GhosttyNSView.debugGhosttySurfaceKeyEventObserver and then unconditionally clears it to nil, which can drop an existing observer and cause cross-test interference; fix by capturing the previous observer into a local (e.g. let previousObserver = GhosttyNSView.debugGhosttySurfaceKeyEventObserver) before assigning the new closure that increments forwardedKeyDownCount, and in the defer restore the previousObserver (GhosttyNSView.debugGhosttySurfaceKeyEventObserver = previousObserver) instead of setting it to nil.
🤖 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/AppDelegate.swift`:
- Around line 4949-4952: The recovery guard uses contextForMainWindow(window)
which can miss stale ObjectIdentifier entries; change the logic to use the
reindexing resolver by calling contextForMainTerminalWindow(window) as the
primary or as a fallback when contextForMainWindow(window) returns nil so the
repair path always resolves the window (keep the rest of the guard: workspace =
context.tabManager.selectedWorkspace, panelId = workspace.focusedPanelId,
terminalPanel = workspace.terminalPanel(for: panelId)). Ensure you reference
contextForMainTerminalWindow(_:), contextForMainWindow(_:),
tabManager.selectedWorkspace, focusedPanelId, and terminalPanel(for:) when
making the update.
- Around line 4913-4918: The current check in cmuxOwningGhosttyView(for:) treats
any GhosttyNSView in the same window as healthy; change it to also consider
whether that ghostty view actually corresponds to the currently focused panel —
if ghosttyView is in the same window but its panel identifier does not match
workspace.focusedPanelId (or whatever property identifies the panel on
GhosttyNSView), return true so repair runs; in short, add a comparison between
ghosttyView's panel id/owner and workspace.focusedPanelId (or the focused panel
id source) and treat a mismatch as needing repair.
---
Nitpick comments:
In `@cmuxTests/AppDelegateShortcutRoutingTests.swift`:
- Around line 3435-3441: The test overwrites the global
GhosttyNSView.debugGhosttySurfaceKeyEventObserver and then unconditionally
clears it to nil, which can drop an existing observer and cause cross-test
interference; fix by capturing the previous observer into a local (e.g. let
previousObserver = GhosttyNSView.debugGhosttySurfaceKeyEventObserver) before
assigning the new closure that increments forwardedKeyDownCount, and in the
defer restore the previousObserver
(GhosttyNSView.debugGhosttySurfaceKeyEventObserver = previousObserver) instead
of setting it to nil.
🪄 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: 0faab0c6-dc09-409a-828e-f9a9083720e4
📒 Files selected for processing (2)
Sources/AppDelegate.swiftcmuxTests/AppDelegateShortcutRoutingTests.swift
Note: this PR doesn't cover the main-thread deadlock scenarioThis PR fixes the case where the first responder silently drifts and typing stops (app still responsive). But there's a second failure mode reported in #2438 where the entire app hangs (beach ball, not just frozen input). I captured a 100% of main thread samples are blocked on that futex — it's a deadlock during a SwiftUI layout pass, not a responder drift. Since the main thread is blocked, Just flagging so #2438 isn't closed as fully resolved by this PR alone — the deadlock in |
![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 (changes from recent commits).
You’re at about 95% of the monthly review limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
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/AppDelegate.swift">
<violation number="1" location="Sources/AppDelegate.swift:4961">
P1: The new NSTextView/NSControl exemption skips repair when the open find bar has drifted focus back from terminal mode to the search 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: 5
🤖 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/AppDelegate.swift`:
- Around line 4949-4963: The current check that returns false for any ownerView
is NSControl || responder is NSTextView is too broad; replace it with a targeted
whitelist: create a helper like isAllowedTextInputResponder(_ responder:
NSResponder) that explicitly checks for known same-window editor types (e.g.,
your app’s omnibar/find-field editor classes or specific control subclasses) and
use that helper in place of the blanket condition in the block containing
hostedView.responderMatchesPreferredKeyboardFocus(_:),
cmuxOwningGhosttyView(for:), and keyRoutingOwnerView(for:). Update the code path
to return false only when isAllowedTextInputResponder(...) is true and remove
the generic NSControl/NSTextView check so unrelated text inputs in other splits
won’t steal typing from the focused terminal.
- Around line 4896-4903: The current keyRoutingOwnerView(for:) function unsafely
dereferences NSTextView.delegate (editor.delegate as? NSView) on a hot .keyDown
path; remove that direct delegate access and instead determine the routing owner
via safe responder traversal: if responder is NSTextView and
editor.isFieldEditor, walk the responder chain (using editor.nextResponder /
nextResponder loop) or check editor.superview / editor.window?.firstResponder to
locate the nearest NSView owner, returning that NSView if found; otherwise
fallback to returning responder as? NSView. Update keyRoutingOwnerView(for:) to
avoid any direct use of editor.delegate so it cannot dereference the
unsafe-unretained delegate.
In `@Sources/GhosttyTerminalView.swift`:
- Around line 9268-9270: The mounted-field fast path calls
focusMountedSearchFieldIfAvailable(window:surfaceShort:) without checking
whether the mounted-focus request is still valid; mirror the other paths by
gating this call with canApplyMountedSearchFieldFocusRequest() so it only runs
when the active-panel intent matches (same guard used by
requestMountedSearchFieldFocus(...) and the .ghosttySearchFocus observer).
Update the fast path (and the similar section around lines 9296–9336) to call
canApplyMountedSearchFieldFocusRequest() before invoking
focusMountedSearchFieldIfAvailable(window:surfaceShort:) to prevent
restoreSearchFocus(window:) from making an unfocused panel’s search field first
responder.
- Around line 1426-1467: When we force Ghostty's native background via
loadDefaultConfigFilesWithLegacyFallback (the branch that calls
loadInlineGhosttyConfig with "macos-background-from-layer = false"), clear the
host-owned background fill so the separate CALayer background
(GhosttySurfaceScrollView.backgroundView) doesn't remain under Ghostty's
composed image; update the same branch in
loadDefaultConfigFilesWithLegacyFallback to, after calling
loadInlineGhosttyConfig, clear or reset the hosted background (via the
view-level code path used by GhosttyNSView.applySurfaceBackground or by directly
clearing GhosttySurfaceScrollView.backgroundView) so no leftover solid-color
fill remains when Ghostty renders its own image-based background.
In `@vendor/bonsplit`:
- Line 1: The vendor/bonsplit submodule pointer is referencing commit
e25dfe0e9a405bd610742901169b0c9c91980c25 which is not present on the submodule
remote origin/main (currently at 5c1567d7ecad030dd5b9eddf3821402e733b5b87);
either push the missing commit e25dfe0e... to the bonsplit repository's main
branch so origin/main includes it, or update the parent repo's vendor/bonsplit
pointer to match the existing origin/main commit (5c1567d7...), and add a short
commit message or PR description explaining why this bonsplit change is required
for the keyboard-routing repair so reviewers understand the coupling.
🪄 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: ac70465a-e441-4df0-9d7d-90347c6fdc0d
📒 Files selected for processing (4)
Sources/AppDelegate.swiftSources/GhosttyTerminalView.swiftcmuxTests/AppDelegateShortcutRoutingTests.swiftvendor/bonsplit
There was a problem hiding this comment.
♻️ Duplicate comments (2)
Sources/AppDelegate.swift (2)
5015-5023:⚠️ Potential issue | 🟠 MajorDon’t exempt every
NSControl/NSTextViewfrom terminal repair.After
hostedView.responderMatchesPreferredKeyboardFocus(...)already preserves the focused terminal’s own surface/search target, the fallback at Line 5021 still treats any other text input as intentional. A stale browser omnibar or find field in the same window will keep swallowing plain typing, so this responder-drift path still won’t repair that class of freeze. Remove the blanket exemption, or replace it with a tiny explicit whitelist of same-panel editors that are actually allowed to win.💡 Narrow the fallback
guard let ownerView = keyRoutingOwnerView(for: responder) else { return true } if ownerView === window.contentView { return true } - if ownerView is NSControl || responder is NSTextView { - return false - } return trueBased on learnings: In
AppDelegate.repairFocusedTerminalKeyboardRoutingIfNeeded(window:event:), determine whether to repair focus by asking the focused terminal’s hosted view if the current first responder already matches that panel’s preferred keyboard target.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate.swift` around lines 5015 - 5023, In AppDelegate.repairFocusedTerminalKeyboardRoutingIfNeeded(window:event:), remove the blanket exemption that returns false for any ownerView is NSControl || responder is NSTextView; instead, call the focused terminal’s hosted view method hostedView.responderMatchesPreferredKeyboardFocus(...) to determine whether the current first responder should win, and only allow a very small explicit whitelist (e.g., same-panel editor classes you know are allowed) to bypass repair; update the logic around ownerView and window.contentView to fall through to repair when neither the hosted view affirms a match nor the responder is on the explicit whitelist.
4956-5010:⚠️ Potential issue | 🔴 CriticalEliminate unsafe field-editor delegate reads from the new key-repair path.
keyRoutingOwnerView(...)still dereferencesNSTextView.delegate, and this same flow immediately callshostedView.responderMatchesPreferredKeyboardFocus(...), whose implementation also does that. AppKit keeps that delegate unsafe-unretained, so responder teardown/reparenting can trap here on ordinary.keyDownhandling—the exact state this repair is trying to recover from. Resolve field-editor ownership via responder/superview traversal or tracked ownership instead of readingdelegatein this hot path.🐛 Safer owner resolution pattern for this file
private func keyRoutingOwnerView(for responder: NSResponder?) -> NSView? { guard let responder else { return nil } if let editor = responder as? NSTextView, - editor.isFieldEditor, - let delegateView = editor.delegate as? NSView { - return delegateView + editor.isFieldEditor { + var current: NSResponder? = editor.nextResponder + while let candidate = current { + if let view = candidate as? NSView { + return view + } + current = candidate.nextResponder + } + if let superview = editor.superview { + return superview + } } return responder as? NSView }You'll want the same no-
delegaterule inGhosttySurfaceScrollView.responderMatchesPreferredKeyboardFocus(_:), since this repair path calls that helper before returning.Run this read-only check to confirm the current hot path still depends on
delegate as? NSView:#!/bin/bash set -euo pipefail echo '--- AppDelegate repair helpers ---' sed -n '4956,5025p' Sources/AppDelegate.swift echo echo '--- GhosttySurfaceScrollView focus matcher ---' rg -n -C3 'func responderMatchesPreferredKeyboardFocus|delegate as\? NSView' Sources/GhosttyTerminalView.swift echo echo '--- Existing in-file AppKit safety note ---' sed -n '13571,13584p' Sources/AppDelegate.swiftExpected result: both helpers on the new
.keyDownrepair path still showdelegate as? NSView, while the lower AppKit note in this file explains why that read is unsafe.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate.swift` around lines 4956 - 5010, keyRoutingOwnerView(...) currently reads NSTextView.delegate (unsafe); change it to avoid delegate access by resolving field-editor ownership via responder/superview traversal: if responder is NSTextView and responder.isFieldEditor, walk responder.superview (and if needed its superview chain) to find the NSView that represents the field’s owner (or return nil if none), otherwise return responder as? NSView as before; update responderNeedsFocusedTerminalKeyRepair path to use this non-delegating resolution and also apply the same no-delegate approach inside GhosttySurfaceScrollView.responderMatchesPreferredKeyboardFocus(_:) so neither helper ever casts .delegate as? NSView.
🤖 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/AppDelegate.swift`:
- Around line 5015-5023: In
AppDelegate.repairFocusedTerminalKeyboardRoutingIfNeeded(window:event:), remove
the blanket exemption that returns false for any ownerView is NSControl ||
responder is NSTextView; instead, call the focused terminal’s hosted view method
hostedView.responderMatchesPreferredKeyboardFocus(...) to determine whether the
current first responder should win, and only allow a very small explicit
whitelist (e.g., same-panel editor classes you know are allowed) to bypass
repair; update the logic around ownerView and window.contentView to fall through
to repair when neither the hosted view affirms a match nor the responder is on
the explicit whitelist.
- Around line 4956-5010: keyRoutingOwnerView(...) currently reads
NSTextView.delegate (unsafe); change it to avoid delegate access by resolving
field-editor ownership via responder/superview traversal: if responder is
NSTextView and responder.isFieldEditor, walk responder.superview (and if needed
its superview chain) to find the NSView that represents the field’s owner (or
return nil if none), otherwise return responder as? NSView as before; update
responderNeedsFocusedTerminalKeyRepair path to use this non-delegating
resolution and also apply the same no-delegate approach inside
GhosttySurfaceScrollView.responderMatchesPreferredKeyboardFocus(_:) so neither
helper ever casts .delegate as? NSView.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: faf34315-023c-43b3-8a95-cc16f88813b8
📒 Files selected for processing (3)
Sources/AppDelegate.swiftSources/GhosttyTerminalView.swiftcmuxTests/AppDelegateShortcutRoutingTests.swift
✅ Files skipped from review due to trivial changes (1)
- cmuxTests/AppDelegateShortcutRoutingTests.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- Sources/GhosttyTerminalView.swift
There was a problem hiding this comment.
♻️ Duplicate comments (2)
Sources/GhosttyTerminalView.swift (2)
9624-9633:⚠️ Potential issue | 🟠 MajorAdd the active-panel guard to the mounted-field fast path.
Line 8639 and the
.ghosttySearchFocuspath already gate search-field focus withcanApplyMountedSearchFieldFocusRequest(), but this direct path does not. IfrestoreSearchFocus(window:)reaches it during stale panel-intent restoration, an unfocused panel can become first responder and steal typing from the actual focused target.Minimal fix
`@discardableResult` private func focusMountedSearchFieldIfAvailable( window: NSWindow, surfaceShort: String ) -> Bool { + guard canApplyMountedSearchFieldFocusRequest() else { + return false + } guard let overlay = searchOverlayHostingView, overlay.superview === self, let field = findEditableSearchField(in: overlay) else { return false }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/GhosttyTerminalView.swift` around lines 9624 - 9633, The mounted-field fast path in focusMountedSearchFieldIfAvailable(window:surfaceShort:) is missing the same active-panel check used by the .ghosttySearchFocus path; add a guard that calls canApplyMountedSearchFieldFocusRequest() (or otherwise verifies the panel is active) before proceeding to use searchOverlayHostingView and findEditableSearchField so restoreSearchFocus(window:) cannot focus a stale/inactive panel and steal first responder; keep the existing early-return false behavior if the guard fails.
1475-1499:⚠️ Potential issue | 🟠 MajorClear the host-owned background fill when
background-imageis configured.This branch disables Ghostty’s layer shortcut, but Line 4911 still pushes a color into
hostedView.setBackgroundColor(...), and Line 8435 still paintsbackgroundView. That leaves a second solid fill under Ghostty’s native image background, so translucent/background-image compositing is still wrong. Skip or clear the hosted fill whenever this override forcesmacos-background-from-layer = false.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/GhosttyTerminalView.swift` around lines 1475 - 1499, When hasConfiguredBackgroundImage(config) is true and you force Ghostty's native background via loadInlineGhosttyConfig("macos-background-from-layer = false", ...), also clear/skip the host-owned solid fill so a second solid layer isn't painted under Ghostty's image; update the branch that sets the "cmux-layer-bg-image-override" to call the host view clearing routine (e.g. hostedView.setBackgroundColor(...) to clear/transparent) and/or disable backgroundView painting (reference backgroundView.paint) so the host fill is not rendered when macos-background-from-layer is false.
🤖 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 9624-9633: The mounted-field fast path in
focusMountedSearchFieldIfAvailable(window:surfaceShort:) is missing the same
active-panel check used by the .ghosttySearchFocus path; add a guard that calls
canApplyMountedSearchFieldFocusRequest() (or otherwise verifies the panel is
active) before proceeding to use searchOverlayHostingView and
findEditableSearchField so restoreSearchFocus(window:) cannot focus a
stale/inactive panel and steal first responder; keep the existing early-return
false behavior if the guard fails.
- Around line 1475-1499: When hasConfiguredBackgroundImage(config) is true and
you force Ghostty's native background via
loadInlineGhosttyConfig("macos-background-from-layer = false", ...), also
clear/skip the host-owned solid fill so a second solid layer isn't painted under
Ghostty's image; update the branch that sets the "cmux-layer-bg-image-override"
to call the host view clearing routine (e.g. hostedView.setBackgroundColor(...)
to clear/transparent) and/or disable backgroundView painting (reference
backgroundView.paint) so the host fill is not rendered when
macos-background-from-layer is false.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 18b27ab1-1b96-434e-9c0a-26225af1d1e4
📒 Files selected for processing (1)
Sources/GhosttyTerminalView.swift
2 participants
Summary
repairFocusedTerminalKeyboardRoutingIfNeededto detect when the window's first responder has drifted away from the focused terminal surface (e.g., tonil, the window itself, a hidden/detached view, or a view in a different window) and automatically restores focus before the keyDown is dispatchedFixes #2438
Test plan
testWindowSendEventRepairsLostFirstResponderForFocusedTerminalTyping.commandmodifier)🤖 Generated with Claude Code
Summary by cubic
Fixes terminal typing freezes by repairing keyboard routing to the active terminal surface or search field when the window’s first responder drifts, including field editor cases. Fixes #2438.
repairFocusedTerminalKeyboardRoutingIfNeeded(invoked fromNSWindow.sendEventon.keyDown) to detect responder drift—includingnil, window, hidden/detached/different-window views, wrong same-window view, and field-editor ownership—and restore focus to the terminal surface or mounted search field; skips when.commandis pressed, a sheet is attached, or the command palette is visible.background-imageis configured, forcemacos-background-from-layer = falseso Ghostty renders images; keep the layer-backed background for solid colors and refresh it after config reloads.Stringfor stable output.Written for commit b131701. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Appearance
Tests