Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe list, multiselect, and select interfaces now accept Ctrl-N and Ctrl-P for vertical navigation. Filtering does not pass control characters to filter input. ChangesKeyboard navigation
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Feature Suggested reviewers: Merge Risk: 🟡 Moderate · up to Existing custom toggle bindings can stop working, and users cannot discover the new shortcuts; preserve the bindings and update the help text before merging. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to Ctrl-N and Ctrl-P add navigation without an identified security bypass. A caller that configured either key to toggle selections will instead get navigation, which can change the selections returned on confirmation. Retained concerns
Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/list.rs`:
- Around line 165-166: The navigation help text must document the newly
supported Ctrl-N/Ctrl-P bindings alongside the existing arrow and j/k keys.
Update the corresponding help entries near the handlers in src/list.rs (around
lines 330), src/multiselect.rs (around line 610), and src/select.rs (around line
453); no handler changes are needed at the cited sites.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 81a736d7-4434-4831-93c6-1eb8876674ec
📒 Files selected for processing (6)
src/input.rssrc/keys.rssrc/lib.rssrc/list.rssrc/multiselect.rssrc/select.rs
| Key::ArrowDown | Key::Char('j') | Key::Char(CTRL_N) => self.handle_down()?, | ||
| Key::ArrowUp | Key::Char('k') | Key::Char(CTRL_P) => self.handle_up(), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep user-facing navigation help synchronized with the new bindings.
Each handler now supports Ctrl-N/Ctrl-P, but its corresponding help text still lists only arrows and j/k.
src/list.rs#L165-L166: update the help text near Line 330.src/multiselect.rs#L222-L223: update the help text near Line 610.src/select.rs#L192-L193: update the help text near Line 453.
📍 Affects 3 files
src/list.rs#L165-L166(this comment)src/multiselect.rs#L222-L223src/select.rs#L192-L193
🤖 Prompt for 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.
In `@src/list.rs` around lines 165 - 166, The navigation help text must document
the newly supported Ctrl-N/Ctrl-P bindings alongside the existing arrow and j/k
keys. Update the corresponding help entries near the handlers in src/list.rs
(around lines 330), src/multiselect.rs (around line 610), and src/select.rs
(around line 453); no handler changes are needed at the cited sites.
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 `@src/multiselect.rs`:
- Around line 203-204: Update the help labels in print_help_keys for both
filtering and non-filtering modes to include Ctrl-N and Ctrl-P alongside the
existing navigation keys, matching the bindings in handle_down and handle_up.
- Around line 203-204: Update the key dispatch around the ArrowDown/ArrowUp arms
and the toggle_key guard so a custom toggle_key set to Ctrl-N or Ctrl-P is
handled as the toggle action rather than navigation. Preserve navigation
behavior for those keys when they are not configured as the toggle key, or
explicitly reject them as reserved if that is the established design.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: e61e360d-7f5f-4015-a84c-9af748eee70b
📒 Files selected for processing (3)
src/list.rssrc/multiselect.rssrc/select.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/list.rs
- src/select.rs
| Key::ArrowDown | Key::Char(CTRL_N) => self.handle_down()?, | ||
| Key::ArrowUp | Key::Char(CTRL_P) => self.handle_up()?, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Advertise the new navigation keys in the help text.
print_help_keys still displays only ↑/↓ (filtering) and ↑/↓/k/j (non-filtering), so users cannot discover Ctrl-N/Ctrl-P. Update the help labels alongside these bindings.
Also applies to: 222-223
🤖 Prompt for 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.
In `@src/multiselect.rs` around lines 203 - 204, Update the help labels in
print_help_keys for both filtering and non-filtering modes to include Ctrl-N and
Ctrl-P alongside the existing navigation keys, matching the bindings in
handle_down and handle_up.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Preserve custom Ctrl-N/Ctrl-P toggle bindings.
These navigation arms now match before key if key == self.toggle_key. Because toggle_key accepts any Key, configuring it as Ctrl-N or Ctrl-P silently changes its behavior from toggling to navigation. Preserve the custom binding or explicitly reject/document these keys as reserved.
Also applies to: 222-223
🤖 Prompt for 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.
In `@src/multiselect.rs` around lines 203 - 204, Update the key dispatch around
the ArrowDown/ArrowUp arms and the toggle_key guard so a custom toggle_key set
to Ctrl-N or Ctrl-P is handled as the toggle action rather than navigation.
Preserve navigation behavior for those keys when they are not configured as the
toggle key, or explicitly reject them as reserved if that is the established
design.
|
+1 for this PR, it would work for This PR would also fix all those Ctrl-related bindings (currently they're inserted as raw bytes in filters): Lines 429 to 436 in 9f192f4 @danielyrovas Could you rebase it for the latest code? |
f71c267 to
4d4807a
Compare
|
@danielyrovas Sorry from aside, the PR needs fixes, like
|
4d4807a to
f9a2460
Compare
silently consume other CTRL characters with char.is_control()
f9a2460 to
37c413d
Compare
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
A small change that allows CTRL N/CTRL P to move vertically in lists.
I especially want this feature in
mise runas I use these keybinds to move in most listsSummary by CodeRabbit