Repository navigation
Fix command palette arrow keys and no-match flash - #3698
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughThis PR improves command-palette keyboard interaction by adding horizontal arrow key forwarding to the field editor and refining empty-state preservation during search. It implements field-editor ownership detection to route arrow events correctly, adds re-entrancy guards, simplifies list-identity computation, and updates search-state logic to depend on resolved-results emptiness rather than query fingerprints. ChangesCommand Palette Arrow Routing & Search State Refinement
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (2 errors, 1 warning)
✅ Passed checks (11 passed)
✨ 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 two command palette UX bugs: horizontal arrow keys consumed by
Confidence Score: 5/5Safe to merge — both fixes are well-scoped with no regressions identified. The arrow-routing change adds a narrowly-targeted branch inside No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant NSWindow as NSWindow.performKeyEquivalent
participant OwnerCheck as cmuxCommandPaletteOwnsFieldEditor
participant Guard as Re-entrancy Guard
participant FieldEditor as Palette Field Editor
User->>NSWindow: Left/Right arrow key event
NSWindow->>OwnerCheck: firstResponder as NSTextView?
OwnerCheck-->>NSWindow: true (owner is inside palette overlay)
NSWindow->>Guard: "cmuxCommandPaletteArrowForwardingDepth > 0?"
Guard-->>NSWindow: false (not re-entrant)
NSWindow->>NSWindow: "depth += 1"
NSWindow->>FieldEditor: keyDown(with: event)
NSWindow->>NSWindow: "depth -= 1 (defer)"
NSWindow-->>User: return true (handled)
Reviews (4): Last reviewed commit: "fix: require presented command palette o..." | Re-trigger Greptile |
|
Addressed Greptile's cleanup note in ba27e4e by removing the stale current/resolved matching-query parameters from the empty-state helper, call site, and tests. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if let ownerView = cmuxFieldEditorOwnerView(textView), | ||
| cmuxIsInsideCommandPaletteOverlay(ownerView) { | ||
| return true | ||
| } | ||
|
|
||
| guard let container = cmuxCommandPaletteOverlayContainer(in: window) else { | ||
| return false | ||
| } | ||
| return !container.isHidden && container.alphaValue > 0.001 |
There was a problem hiding this comment.
cmuxFieldEditorOwnerView returns a non-nil view that is not inside the palette overlay, the compound if let …, condition simply falls through rather than returning false. The code then reaches the container-visibility fallback and can return true solely because the palette container has alphaValue > 0.001 — even though the active field editor belongs to a completely different control (e.g., a browser search bar). This would silently steal horizontal arrow keys from that other field editor. The fix is to return immediately when an owner is positively identified as outside the overlay, reserving the container-visibility heuristic only for the case where the owner cannot be determined at all.
| if let ownerView = cmuxFieldEditorOwnerView(textView), | |
| cmuxIsInsideCommandPaletteOverlay(ownerView) { | |
| return true | |
| } | |
| guard let container = cmuxCommandPaletteOverlayContainer(in: window) else { | |
| return false | |
| } | |
| return !container.isHidden && container.alphaValue > 0.001 | |
| if let ownerView = cmuxFieldEditorOwnerView(textView) { | |
| return cmuxIsInsideCommandPaletteOverlay(ownerView) | |
| } | |
| // Owner lookup failed — fall back to container visibility as a best-effort heuristic. | |
| guard let container = cmuxCommandPaletteOverlayContainer(in: window) else { | |
| return false | |
| } | |
| return !container.isHidden && container.alphaValue > 0.001 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit ba27e4e. Configure here.
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmuxTests/CommandPaletteShortcutCustomizationTests.swift`:
- Around line 417-430: The test improperly adds overlayContainer and
outsideOwnerView as siblings under searchRoot causing
cmuxIsInsideCommandPaletteOverlay to vacuously return false; instead, mirror
withVisibleCommandPaletteOverlay by adding overlayContainer and outsideOwnerView
as subviews of contentView so the upward traversal can encounter the overlay,
and ensure the palette is visible during the check (do not call
setCommandPaletteVisible(false) before asserting
cmuxCommandPaletteOwnsFieldEditor) so the test verifies detection logic for a
visible overlay.
In `@Sources/AppDelegate.swift`:
- Around line 13845-13860: The cmuxCommandPaletteOwnsFieldEditor(_:, in:) helper
currently returns true when cmuxFieldEditorOwnerView(_:) finds an owner even if
the overlay is hidden; update this function so that after obtaining ownerView
via cmuxFieldEditorOwnerView(textView) you also retrieve the overlay container
using cmuxCommandPaletteOverlayContainer(in: window) and only return
cmuxIsInsideCommandPaletteOverlay(ownerView) if that container exists and is
visible (not hidden and alphaValue > 0.001); otherwise fall through to the
existing container-visibility check and return false when the overlay is not
presented.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: af9c4c80-7315-473d-9cb3-8abd8dcb8eed
📒 Files selected for processing (2)
Sources/AppDelegate.swiftcmuxTests/CommandPaletteShortcutCustomizationTests.swift
This branch was successfully deployed
1 participant
Summary
Verification
Note
Medium Risk
Changes macOS
performKeyEquivalentrouting and command palette search/empty-state behavior, which could affect global keyboard handling and UI responsiveness if edge cases are missed.Overview
Fixes command palette keyboard routing so left/right arrow keys are forwarded to the palette’s focused field editor (when the visible overlay actually owns the field editor and there’s no IME marked text), preventing app-level shortcut routing from swallowing them.
Stabilizes command palette results rendering and the no-match empty state by rebuilding the results container on scope-only transitions and keeping the empty-state message pinned while a same-scope search is pending; also tightens async search result application guards to avoid stale updates.
Adds regression tests covering horizontal arrow forwarding/ownership guards and the pending no-match empty-state behavior.
Reviewed by Cursor Bugbot for commit 59c9f2b. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Fixes command palette horizontal arrow routing and removes the empty‑state flicker. Arrows only reach the palette when its overlay is presented, and results stay stable during same‑scope pending searches.
Written for commit 59c9f2b. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests