Repository navigation
Fix terminal keyboard focus after workspace switch (#1122) - #1124
Closed
austinywang wants to merge 2 commits into
Closed
austinywang wants to merge 2 commits into
austinywang wants to merge 2 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR introduces generation-based focus guarding to prevent race conditions during workspace switching. It enhances the Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant GTV as GhosttyTerminalView
participant TM as TabManager
participant AD as AppDelegate
User->>GTV: ensureFocus(tabId, surfaceId, requestGeneration: Gen1)
alt Early Validation Fails
GTV->>GTV: Check AppDelegate/TabManager available
GTV->>GTV: Verify target tab is selected
Note over GTV: Conditions not met
GTV->>GTV: Schedule retry with focusEnsureRetryDelay
GTV-->>User: (retry scheduled)
end
alt Precondition Check Fails
GTV->>GTV: Verify surface active, visible
GTV->>GTV: Verify window valid
Note over GTV: Preconditions not satisfied
GTV->>GTV: Schedule retry with generation guard
GTV-->>User: (retry scheduled)
end
alt Retry Arrives with Stale Generation
GTV->>GTV: Check generation == Gen1
Note over GTV: Generation mismatch (Gen2 is current)
GTV->>GTV: Discard stale retry
GTV-->>User: (retry discarded)
end
alt Conditions Met & Generation Current
GTV->>AD: Focus surface
AD->>TM: Activate workspace
TM->>GTV: ensureFocusedTerminalFirstResponder()
GTV-->>User: Focus established ✓
end
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 6181-6187: The current guard uses a fallback delegate.tabManager
which can be unrelated during workspace remounts and causes the retry() chain to
be dropped; change the logic to only proceed when
AppDelegate.shared.tabManagerFor(tabId:) returns a non-nil manager for the
target tabId and otherwise call retry() and return (i.e., remove the fallback to
delegate.tabManager), then keep the existing check against
tabManager.selectedTabId == tabId; reference AppDelegate.shared,
tabManagerFor(tabId:), tabManager, retry(), and tabManager.selectedTabId to
locate and update the code.
In `@tests/test_issue_1122_workspace_switch_keyboard.py`:
- Around line 67-91: The test creates a temporary file FOCUS_FILE but never
removes it; update main() to ensure FOCUS_FILE is deleted after the test
completes by adding cleanup (e.g., call FOCUS_FILE.unlink(missing_ok=True))
either in a finally block wrapping the existing with cmux(...) block or
immediately after the success path (after the assertions/print) so cleanup runs
on both success and failure; reference the FOCUS_FILE symbol and the main()
function (and keep the existing cmux(...) / _wait_for_terminal_focus(...) logic
unchanged).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d720ce0d-339f-45b5-81d2-8c042d6e7a90
📒 Files selected for processing (3)
Sources/GhosttyTerminalView.swiftSources/TabManager.swifttests/test_issue_1122_workspace_switch_keyboard.py
Comment on lines
+6181
to
+6187
| guard let delegate = AppDelegate.shared, | ||
| let tabManager = delegate.tabManagerFor(tabId: tabId) ?? delegate.tabManager else { | ||
| retry() | ||
| return | ||
| } | ||
| guard tabManager.selectedTabId == tabId else { return } | ||
|
|
There was a problem hiding this comment.
Don't drop the retry chain when the fallback manager is unrelated.
Sources/AppDelegate.swift:8796-8798 resolves tabManagerFor(tabId:) through contextContainingTabId(tabId), so it can legitimately be nil while a workspace is remounting. In that window the fallback delegate.tabManager may belong to a different window, and Line 6186 drops this request's retry chain instead of waiting for the target workspace selection to converge. That can still leave keyboard focus stranded after a slow switch.
Suggested fix
- guard tabManager.selectedTabId == tabId else { return }
+ guard tabManager.selectedTabId == tabId else {
+ retry()
+ return
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/GhosttyTerminalView.swift` around lines 6181 - 6187, The current
guard uses a fallback delegate.tabManager which can be unrelated during
workspace remounts and causes the retry() chain to be dropped; change the logic
to only proceed when AppDelegate.shared.tabManagerFor(tabId:) returns a non-nil
manager for the target tabId and otherwise call retry() and return (i.e., remove
the fallback to delegate.tabManager), then keep the existing check against
tabManager.selectedTabId == tabId; reference AppDelegate.shared,
tabManagerFor(tabId:), tabManager, retry(), and tabManager.selectedTabId to
locate and update the code.
Comment on lines
+67
to
+91
| def main() -> int: | ||
| with cmux(SOCKET_PATH) as c: | ||
| ws_a = c.new_workspace() | ||
| time.sleep(0.3) | ||
| c.activate_app() | ||
| time.sleep(0.2) | ||
| panel_a = _selected_terminal_panel_id(c) | ||
|
|
||
| ws_b = c.new_workspace() | ||
| time.sleep(0.3) | ||
|
|
||
| for _ in range(6): | ||
| c.select_workspace(ws_a) | ||
| time.sleep(0.12) | ||
| c.select_workspace(ws_b) | ||
| time.sleep(0.12) | ||
|
|
||
| c.select_workspace(ws_a) | ||
| time.sleep(0.2) | ||
|
|
||
| _wait_for_terminal_focus(c, panel_a, timeout_s=3.0) | ||
| _assert_typed_input_routes_to_selected_terminal(c, panel_a) | ||
|
|
||
| print("PASS: workspace switch-back restores terminal keyboard focus") | ||
| return 0 |
There was a problem hiding this comment.
Clean up the temp file after test completion.
FOCUS_FILE is created during the test but not removed after a successful run. Consider adding cleanup in a finally block or after the assertion to avoid leaving stale temp files.
🧹 Proposed fix to add cleanup
_wait_for_terminal_focus(c, panel_a, timeout_s=3.0)
_assert_typed_input_routes_to_selected_terminal(c, panel_a)
+ FOCUS_FILE.unlink(missing_ok=True)
print("PASS: workspace switch-back restores terminal keyboard focus")
return 0Alternatively, for cleanup on both success and failure:
def main() -> int:
try:
with cmux(SOCKET_PATH) as c:
# ... existing test code ...
print("PASS: workspace switch-back restores terminal keyboard focus")
return 0
finally:
FOCUS_FILE.unlink(missing_ok=True)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_issue_1122_workspace_switch_keyboard.py` around lines 67 - 91, The
test creates a temporary file FOCUS_FILE but never removes it; update main() to
ensure FOCUS_FILE is deleted after the test completes by adding cleanup (e.g.,
call FOCUS_FILE.unlink(missing_ok=True)) either in a finally block wrapping the
existing with cmux(...) block or immediately after the success path (after the
assertions/print) so cleanup runs on both success and failure; reference the
FOCUS_FILE symbol and the main() function (and keep the existing cmux(...) /
_wait_for_terminal_focus(...) logic unchanged).
Contributor
Greptile SummaryThis PR fixes a keyboard-unresponsive regression (#1122) where switching away from and back to a workspace could leave the terminal with no AppKit first-responder, requiring the user to click the terminal before typing would work. Two root causes are addressed: the retiring workspace synchronously clearing first-responder to Key changes:
Confidence Score: 4/5
Sequence DiagramsequenceDiagram
participant User
participant TabManager
participant WorkspaceB as Workspace B (retiring)
participant WorkspaceA as Workspace A (selected)
participant ScrollView as GhosttySurfaceScrollView
User->>TabManager: select_workspace(ws_a)
TabManager->>TabManager: selectedTabId = ws_a
TabManager->>WorkspaceB: pendingWorkspaceUnfocus scheduled
Note over TabManager: completePendingWorkspaceUnfocus fires
TabManager->>WorkspaceB: unfocusWorkspacePanel()
WorkspaceB-->>TabManager: first responder cleared (→ nil)
TabManager->>TabManager: ensureFocusedTerminalFirstResponder()
TabManager->>ScrollView: ensureFocus(tabId: ws_a, surfaceId: panelId)
activate ScrollView
Note over ScrollView: focusRequestGeneration &+= 1<br/>generation = N
alt workspace reattaching (isActive=false / no window)
ScrollView->>ScrollView: retry() [up to 6×, 30ms apart]
Note over ScrollView: each retry checks generation == N<br/>stale retries cancelled if generation advances
else workspace ready
ScrollView->>WorkspaceA: window.makeFirstResponder(surfaceView)
end
deactivate ScrollView
Note over ScrollView: cancelFocusRequest() bumps generation<br/>→ all in-flight retries for old generation stop
Last reviewed commit: 87ef640 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
This was referenced Jun 20, 2026
lawrencecchen
added
the
stale-revisit
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
3 participants
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
Summary by cubic
Fixes lost keyboard input after switching workspaces by reliably restoring terminal focus. Adds a regression test to ensure typing reaches the selected terminal via the real first-responder path.
tests/test_issue_1122_workspace_switch_keyboard.py, which simulates switch cycles and checks that typing routes to the selected terminal.Written for commit 87ef640. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests