Repository navigation
Fix Ctrl+K in the command palette - #2394
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRemoved Emacs-style Control+J/Control+K command-palette navigation; tightened empty-state preservation to require exact query equality; updated and added tests to reflect removed key mappings and the revised preservation logic and test preconditions. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 a user-reported bug where Ctrl+K (the Unix/Emacs "kill to end of line" text-editing command) was being intercepted by the command palette's keyboard navigation handler and treated as "move selection up" instead of being passed through to the search field for native text editing. The fix is a two-line deletion from Key changes:
Confidence Score: 5/5Safe to merge — the fix is a minimal two-line deletion with consistent unit, integration, and E2E test coverage. The only finding is a P2 test-clarity note about Minor test-readability concern in Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Key event: Ctrl+K in palette search field] --> B[handleCustomShortcut]
B --> C[commandPaletteSelectionDeltaForKeyboardNavigation]
C --> D{key is Ctrl+N/P or arrow key?}
D -- Yes --> E[return delta ±1]
D -- No --> F[return nil]
E --> G[post .commandPaletteMoveSelection — event consumed]
F --> H[shouldConsumeShortcutWhileCommandPaletteVisible]
H --> I{flags contain .command or is Escape?}
I -- Yes --> J[consume / block]
I -- No — Ctrl+K → false --> K[return false — event NOT consumed]
K --> L[AppKit handles Ctrl+K: kill-to-end-of-line in search field ✓]
style E fill:#f66,color:#fff
style G fill:#f66,color:#fff
style K fill:#6a6,color:#fff
style L fill:#6a6,color:#fff
Reviews (1): Last reviewed commit: "Let ctrl-k reach command palette text ed..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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 1 file (changes from recent commits).
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="cmuxTests/AppDelegateShortcutRoutingTests.swift">
<violation number="1" location="cmuxTests/AppDelegateShortcutRoutingTests.swift:2355">
P2: This change removes the lag-state scenario from the test: setting command-palette visibility to `true` makes it equivalent to the normal visible-Escape test, so regressions in pending-open/visibility-sync handling can slip through.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| // Simulate a visibility sync lag/race where AppDelegate does not yet know the palette is open. | ||
| appDelegate.setCommandPaletteVisible(false, for: window) | ||
| // Model the normal open-palette state so the test reads like the user-facing scenario. | ||
| appDelegate.setCommandPaletteVisible(true, for: window) |
There was a problem hiding this comment.
P2: This change removes the lag-state scenario from the test: setting command-palette visibility to true makes it equivalent to the normal visible-Escape test, so regressions in pending-open/visibility-sync handling can slip through.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cmuxTests/AppDelegateShortcutRoutingTests.swift, line 2355:
<comment>This change removes the lag-state scenario from the test: setting command-palette visibility to `true` makes it equivalent to the normal visible-Escape test, so regressions in pending-open/visibility-sync handling can slip through.</comment>
<file context>
@@ -2351,8 +2351,8 @@ final class AppDelegateShortcutRoutingTests: XCTestCase {
- // Simulate a visibility sync lag/race where AppDelegate does not yet know the palette is open.
- appDelegate.setCommandPaletteVisible(false, for: window)
+ // Model the normal open-palette state so the test reads like the user-facing scenario.
+ appDelegate.setCommandPaletteVisible(true, for: window)
guard let escapeEvent = makeKeyDownEvent(
</file context>
| appDelegate.setCommandPaletteVisible(true, for: window) | |
| appDelegate.setCommandPaletteVisible(false, for: window) |
…and-palette Fix Ctrl+K in the command palette
This branch was successfully deployed
1 participant
Summary
Testing
CMUX_SKIP_ZIG_BUILD=1 xcodebuild -project GhosttyTabs.xcodeproj -scheme cmux-unit -destination 'platform=macOS' -derivedDataPath /tmp/cmux-task-ctrl-k-command-palette-tests test -only-testing:cmuxTests/CommandPaletteKeyboardNavigationTests -only-testing:cmuxTests/AppDelegateShortcutRoutingTests/testControlKDoesNotRoutePaletteMoveSelectionWhenSearchFieldIsFocused✅./scripts/reload.sh --tag task-ctrl-k-command-palette✅Task
Summary by cubic
Fix Ctrl+K in the command palette and stop empty-state flashes. Ctrl+K now edits text; only Ctrl+N/P (and arrows) move selection.
Written for commit 208e741. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests