Skip to content

Conversation

@salmonumbrella
Copy link

@salmonumbrella salmonumbrella commented Dec 24, 2025

Summary

Adds vim-style keyboard shortcuts for navigating the SmartBlocks workflow dropdown:

  • Ctrl-n / Ctrl-j → move down
  • Ctrl-p / Ctrl-k → move up

These complement the existing arrow key navigation, making it easier for vim users to quickly select workflows without leaving the home row.

Changes

  • Modified keydownListener in SmartblocksMenu.tsx to recognize Ctrl+n/j as down and Ctrl+p/k as up

Testing

Tested locally by triggering the SmartBlocks menu with jj and verifying all four Ctrl combinations navigate the dropdown correctly.

Summary by CodeRabbit

  • New Features
    • Smart blocks menu now supports additional keyboard shortcuts for navigation: Ctrl+N and Ctrl+J to move down through items, Ctrl+P and Ctrl+K to move up. These work alongside existing arrow key navigation.

✏️ Tip: You can customize this high-level summary in your review settings.

Add Ctrl-n/Ctrl-j for down and Ctrl-p/Ctrl-k for up navigation
in the workflow dropdown, complementing existing arrow key support.
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 24, 2025

Walkthrough

The src/SmartblocksMenu.tsx component was extended to support additional keyboard shortcuts for menu navigation. Previously, only ArrowUp and ArrowDown keys controlled navigation. The change introduces variables isDown (representing ArrowDown or Ctrl+N/Ctrl+J) and isUp (representing ArrowUp or Ctrl+P/Ctrl+K), which replace direct key checks in the navigation logic. The cycling behavior and event handling remain unchanged, while other key handlers (ArrowLeft/Right, Enter, Escape) and filter logic are unaffected.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: adding vim-style keyboard navigation to the SmartBlocks menu, which is the primary objective of the PR.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
src/SmartblocksMenu.tsx (1)

111-116: Consider checking for additional modifier keys to prevent unintended shortcut triggers.

The current implementation only checks e.ctrlKey without verifying that other modifiers (Shift, Alt, Meta) are not pressed. This means combinations like Ctrl+Shift+n or Ctrl+Alt+j would still trigger navigation, which may not be the intended behavior.

🔎 Proposed fix to add modifier key checks
 const isDown =
-  e.key === "ArrowDown" ||
-  (e.ctrlKey && (e.key === "n" || e.key === "j"));
+  e.key === "ArrowDown" ||
+  (e.ctrlKey && !e.shiftKey && !e.altKey && !e.metaKey && (e.key === "n" || e.key === "j"));
 const isUp =
-  e.key === "ArrowUp" ||
-  (e.ctrlKey && (e.key === "p" || e.key === "k"));
+  e.key === "ArrowUp" ||
+  (e.ctrlKey && !e.shiftKey && !e.altKey && !e.metaKey && (e.key === "p" || e.key === "k"));

Verify that overriding browser shortcuts is intentional.

The vim-style shortcuts (Ctrl+n, Ctrl+p, Ctrl+j, Ctrl+k) conflict with common browser shortcuts (new window, print, downloads, search). While preventDefault() prevents the default browser actions, users might still expect these shortcuts to perform their standard browser functions.

Can you confirm this is intentional? Vim users typically expect these keybindings, but it may cause confusion for users who accidentally trigger them while trying to use browser shortcuts.

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c1b7d43 and 8022740.

📒 Files selected for processing (1)
  • src/SmartblocksMenu.tsx
🔇 Additional comments (1)
src/SmartblocksMenu.tsx (1)

117-128: Nice refactoring to support multiple navigation shortcuts!

The implementation correctly maintains the existing cycling behavior while extending support for vim-style keybindings. The use of isDown and isUp variables improves readability and makes the intent clear. Event propagation and prevention are handled appropriately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant