Repository navigation
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughUpdated right-sidebar shortcut routing so an ChangesRight-sidebar shortcut routing
Estimated code review effort: 1 (Trivial) | ~3 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 25✅ Passed checks (25 passed)
✨ Finishing Touches🧪 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 |
5f66bca to
4b35b4b
Compare
Greptile SummaryFixes a routing bug where right-sidebar mode shortcuts (Ctrl+1–5) were silently dropped during any window state that briefly clears the key view. The root cause was
Confidence Score: 5/5Single-line guard extension in a keyboard-routing predicate; all other code paths are untouched and the AppKit behavioral contract is correctly modeled. The change is minimal and precisely targeted: it adds one identity check ( Files Needing Attention: No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A([shouldRouteRightSidebarModeShortcut]) --> B{window == nil?}
B -- yes --> C([return false])
B -- no --> D[sidebarIntentActive = activeRightSidebarMode != nil]
D --> E{firstResponder == nil OR firstResponder === window?}
E -- yes --> F([return sidebarIntentActive])
E -- no --> G{isRightSidebarFocusResponder?}
G -- yes --> H([return true])
G -- no --> I{sidebarIntentActive && responder is NSWindow?}
I -- yes --> J([return true])
I -- no --> K{terminalKeyboardFocusRequest non-nil?}
K -- yes --> L([return false])
K -- no --> M{cmuxStrictOwningGhosttyView && terminalSurface.id?}
M -- no --> N([return false])
M -- yes --> O([return isRightSidebarDockSurface])
Reviews (2): Last reviewed commit: "app-delegate: route right-sidebar mode s..." | Re-trigger Greptile |
…s parked on the window shouldRouteRightSidebarModeShortcut treated window.firstResponder === window (what makeFirstResponder(nil) leaves behind when a responder temporarily clears) as a real responder and fell through to the terminal-surface checks, which return false, so a right-sidebar mode shortcut did nothing while an intent was active. Treat a responder that is the window itself like no responder and route on the active sidebar intent.
4b35b4b to
7c3659b
Compare
|
Closing as superseded by #8621. On the #8599 base, Hosted verification confirms the complete
The branch is conflict-free and all reported checks are green, but merging it would land a semantic no-op under a bug-fix title. |
2 participants
What you'd hit
Focus the right sidebar (files, sessions, and so on), then do something that briefly clears the key view. Press a right-sidebar mode shortcut (Ctrl+1..5 by default) during that window and nothing happens, instead of switching the sidebar mode.
Why
shouldRouteRightSidebarModeShortcutdecides whether a mode shortcut routes to the sidebar. It bails to the active sidebar intent only when there is no first responder:But
makeFirstResponder(nil)does not leavefirstRespondernil. AppKit parks the first responder on the window itself, sowindow.firstResponderis the window. The guard treats that as a real responder and falls through to the terminal-surface checks, which can't resolve a view from a window and return false, so the shortcut does nothing even though a sidebar intent is active. The nil-responder fallback was added for exactly this "responder temporarily clears" case (#6472), but it keyed on a state AppKit essentially never produces.Fix
Treat a responder that is the window itself like no responder:
This only changes the one case where the responder is parked on the window and a sidebar intent is active. With no intent the result is unchanged, and a real view responder still takes the terminal path.
Tests
AppDelegateSurfaceShortcutRoutingTests.rightSidebarModeShortcutsDoNotFallThroughWhenResponderTemporarilyClearsalready exists (it shipped with the fallback in #6472) and is red onmain, because the fallback keyed onnilwhile AppKit parks the responder on the window. Before, the mode shortcut falls through toselectSurfaceByNumber(the recorded issues are three keys across ten cycles). After, all 9 tests in the suite pass.Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Fix right-sidebar mode shortcuts (Ctrl+1–5) to still switch modes when the key view briefly clears. When
makeFirstResponder(nil)parks focus on the window, treatwindow.firstResponder === windowas no responder and route to the active right-sidebar intent.Written for commit 7c3659b. Summary will update on new commits.
Summary by CodeRabbit