Repository navigation
Fix main shortcut routing CI regressions - #4445
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:
📝 WalkthroughWalkthroughAppDelegate computes chord-eligible configured actions on demand, arms configured shortcut chords earlier when no active chord prefix exists, and tests were made deterministic (window key/front, polling waits) and updated to assert computed equalize divider positions. ChangesShortcut Chord Routing
Sequence DiagramsequenceDiagram
participant KeyboardEvent
participant AppDelegate
participant currentConfiguredShortcutChordActions
participant armConfiguredShortcutChordIfNeeded
KeyboardEvent->>AppDelegate: key event arrives
AppDelegate->>currentConfiguredShortcutChordActions: compute eligible chord actions (filter by focus context)
AppDelegate->>armConfiguredShortcutChordIfNeeded: attempt to arm chord (actions or provided list)
armConfiguredShortcutChordIfNeeded-->>AppDelegate: success / failure
AppDelegate-->>KeyboardEvent: consume event if armed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ Passed checks (15 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Greptile SummaryThis PR fixes app-level keyboard shortcut routing regressions by computing chord-eligible actions on demand (removing the stale
Confidence Score: 5/5Safe to merge; the chord-routing refactor is well-scoped and the test suite is materially hardened. The core shortcut-routing change — computing chord-eligible actions on demand with context filtering rather than relying on a stale cache — is correct and properly tested. The startSearch Bool refactor and UpdateTitlebarAccessory weak-capture fixes are clean. The one non-ideal pattern is the double-dispatch cleanup of fullScreenDisallowsTiling, but it is isolated to the window-creation path and no worse than the previous single dispatch. The fullScreenDisallowsTiling double-dispatch block in Sources/AppDelegate.swift around createMainWindow is worth a follow-up to identify the AppKit call that re-adds the flag. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[NSEvent received in local key handler] --> B{shouldBypassPlainKeyShortcutRouting?}
B -- yes --> Z[return false]
B -- no --> C{activeConfiguredShortcutChordPrefixForCurrentEvent == nil?}
C -- yes --> D[shortcutEventFocusContext + currentConfiguredShortcutChordActions filtered by context]
D --> E{armConfiguredShortcutChordIfNeeded with availableChordActions}
E -- armed --> R1[return true]
E -- not armed --> F[preferredMainWindowContextForShortcutRouting + configuredCmuxShortcutActions]
C -- no --> F
F --> G{armConfiguredShortcutChordIfNeeded with empty actions + shortcuts}
G -- armed --> R2[return true]
G -- not armed --> H[Match individual configured shortcuts]
H --> R3[return true or false]
Reviews (12): Last reviewed commit: "fix: distinguish space shortcut from whi..." | Re-trigger Greptile |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
Addressed Greptile's latent fallback note in 86fb05c by requiring explicit actions for chord arming; there is no longer a nil-default path that can bypass focus-context filtering. |
|
Verified CodeRabbit's logging note: the three referenced NSLog call sites are already present on origin/main and are absent from this PR's diff, so there is no PR-introduced logging violation to patch here. Current CodeRabbit check is passing. |
|
Addressed the CI failure from the previous head in 83f39a8. The tests log showed testOmnibarArrowSelectionSurvivesTransientWindowFirstResponder was racing AppDelegate's focused-address-bar state; the fixture now uses requestBrowserAddressBarFocus(panelId:) before simulating the transient window first responder. |
|
Addressed the latest tests job failures in 19bfe4e. The live job log showed testCreateMainWindowTemporarilyDisallowsFullScreenTilingFromFullscreenSource and testFindShortcutFromTerminalOpensTerminalFind failing; the fullscreen cleanup now explicitly reassigns collectionBehavior after clearing the temporary flag, and the terminal-find fixture now gives the terminal actual first-responder focus and waits for async search-state creation. |
|
Addressed the next tests log failures in 0fdb3bd. The live log showed testArrowNavigationRoutesWhileCommandPaletteOverlayIsInteractiveBeforeVisibilitySync and testCmdCtrlWClosesWindowAfterConfirmation failing; the palette overlay synthetic arrow event now uses the key-window path, and confirmed main-window close now closes the window directly after confirmation instead of re-entering performClose. |
|
Addressed the repeated tests job failures in 03ec877. The command-palette overlay fixture now mounts its synthetic overlay on the same root searched by the production overlay detector, and the confirmed Cmd+Ctrl+W test now waits/asserts on visible-window closure instead of immediate NSApp.windows deallocation. |
This branch was successfully deployed
1 participant
Summary
Testing
git diff --checkpassed.Demo Video
Review Trigger (Copy/Paste as PR comment)
Checklist
Note
Medium Risk
Changes affect app-level keyboard shortcut routing (including chord prefix handling) and window focus/timing behavior; regressions could swallow keystrokes or mis-route shortcuts. Updates are localized but touch core event handling and add timing-sensitive fixes for fullscreen/window behaviors.
Overview
Fixes shortcut-routing regressions by reworking chord prefix arming in
AppDelegate: chord-enabled actions are now computed on-demand per event (and filtered by focus context) instead of cached, and arming happens before the “plain key” fast-path so file-backed/remapped chord prefixes aren’t missed.Improves find and sidebar shortcut behavior by making
TabManager.startSearch()return aBool(used byperformFindShortcutInActiveMainWindow) and deferring right-sidebar toggling viaDispatchQueue.main.asyncto avoid unwanted AppKit animation contexts.Tightens shortcut parsing around whitespace: single-space still maps to the
spacekey, but other whitespace-only tokens now resolve to unbound/invalid as appropriate; settings-file normalization also preserves invalidshowHideAllWindowsvalues so managed configs remain visible. Updates window/accessory handling to avoid races (clearing fullscreen tiling opt-out more robustly; guardingNSWindowattachment tasks), and hardens CI tests with explicit window focusing, polling-based waits, and more accurate split equalization expectations.Reviewed by Cursor Bugbot for commit cd9b82c. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
Refactor
Tests