feat: add Ctrl+D & Ctrl+U keys for scroll#42
Conversation
WalkthroughCentralizes cursor-bound calculations via a new Model.getMaxItems() and standardizes half-screen paging with an unexported constant Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Model
participant View
rect rgba(230,240,255,0.6)
note over User,Model: Keypress (j / down / Ctrl-D / Ctrl-U / G)
User->>Model: key event
Model->>Model: getMaxItems()
Model->>Model: compute new cursor (clamp to [0, max])
Model->>Model: ensureCursorVisible()
Model->>View: render updated view
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🔇 Additional comments (6)
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 help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
model.go (3)
21-22: Avoid hardcoding 21; compute half-screen dynamically.Vim’s default scroll is half the window, not a fixed count. Use calculateContentHeight(m.height)/2 at runtime, and drop this constant.
Apply this diff to remove the constant:
-const scrollHalfScreenLines = 21
120-131: Return -1 in default branch to reflect “no items.”More precise contract; prevents accidental non-negative values leaking into callers if mode expands in future.
default: - return 0 + return -1
328-338: Compute half-screen dynamically for ctrl+u and simplify clamping.Aligns with Vim semantics and removes duplication.
- newCursorPos := m.cursor - scrollHalfScreenLines - if newCursorPos < 0 { - m.cursor = 0 - m.ensureCursorVisible() - } else { - m.cursor -= scrollHalfScreenLines - m.ensureCursorVisible() - } + halfLines := max(1, calculateContentHeight(m.height)/2) + if m.cursor < halfLines { + m.cursor = 0 + } else { + m.cursor -= halfLines + } + m.ensureCursorVisible()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
model.go(3 hunks)
🔇 Additional comments (2)
model.go (2)
308-313: LGTM: unified max bound check for down/j.Clean bound check via getMaxItems() and ensureCursorVisible() after change.
340-347: LGTM: “G” clamps to last item via getMaxItems().Good guard on empty lists (>=0) and consistent ensureCursorVisible().
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
view.go (1)
389-390: Use Ctrl+U/Ctrl+D notation to match existing styleElsewhere you use "Ctrl+C" with a plus. Keep key notation consistent.
- {"Ctrl-U", "Scroll up by half a screen"}, - {"Ctrl-D", "Scroll down by half a screen"}, + {"Ctrl+U", "Scroll up by half a screen"}, + {"Ctrl+D", "Scroll down by half a screen"},
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
view.go (1)
389-390: Use “Ctrl+U”/“Ctrl+D” for consistency with other shortcuts.Align with “Ctrl+C” and “Shift+Tab/H”.
- {"Ctrl-U", "Scroll up by half a screen"}, - {"Ctrl-D", "Scroll down by half a screen"}, + {"Ctrl+U", "Scroll up by half a screen"}, + {"Ctrl+D", "Scroll down by half a screen"},model.go (1)
21-22: Consider dynamic half-screen instead of a fixed 21 lines.Viewport-aware scrolling feels more Vim-like on varying terminal sizes.
Example:
halfLines := max(1, calculateContentHeight(m.height)/2)Use halfLines in ctrl+d/ctrl+u.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
model.go(3 hunks)view.go(1 hunks)
🔇 Additional comments (6)
model.go (6)
120-132: LGTM: centralized max-index helper.Good unification across modes.
308-312: LGTM: down-navigation now uses unified max boundary.
328-338: LGTM: ctrl+u clamps to zero safely.No negative cursor risk here.
340-347: LGTM: G navigation clamps to valid end.
366-374: Harden bounds before indexing on Enter/Space.Add non-negativity to prevent panic when cursor becomes -1.
- if m.mode == viewEndpoints && m.cursor < len(m.endpoints) { + if m.mode == viewEndpoints && m.cursor >= 0 && m.cursor < len(m.endpoints) { m.endpoints[m.cursor].folded = !m.endpoints[m.cursor].folded - } else if m.mode == viewComponents && m.cursor < len(m.components) { + } else if m.mode == viewComponents && m.cursor >= 0 && m.cursor < len(m.components) { m.components[m.cursor].folded = !m.components[m.cursor].folded - } else if m.mode == viewWebhooks && m.cursor < len(m.webhooks) { + } else if m.mode == viewWebhooks && m.cursor >= 0 && m.cursor < len(m.webhooks) { m.webhooks[m.cursor].folded = !m.webhooks[m.cursor].folded }
314-326: Fix: ctrl+d can set cursor to -1 on empty lists → panic risk on Enter/Space.Guard when maxItems < 0 and avoid assigning -1.
- maxItems := m.getMaxItems() - newCursorPos := m.cursor + scrollHalfScreenLines - - if newCursorPos > maxItems { - m.cursor = maxItems - m.ensureCursorVisible() - } else { - m.cursor += scrollHalfScreenLines - m.ensureCursorVisible() - } + maxItems := m.getMaxItems() + if maxItems >= 0 { + newCursorPos := m.cursor + scrollHalfScreenLines + if newCursorPos > maxItems { + m.cursor = maxItems + } else { + m.cursor += scrollHalfScreenLines + } + m.ensureCursorVisible() + }
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
model.go (3)
21-21: Avoid hardcoded 21; compute half-screen dynamicallyVim’s default ‘scroll’ is half the current window height, not a fixed number. Compute the step from viewport height instead of using a magic constant.
Consider deriving the step at use sites with:
- step := max(1, calculateContentHeight(m.height)/2)
120-131: getMaxItems() returns -1 on empty lists; default case should be consistentDefault currently returns 0, which is inconsistent with the -1 semantics for “no items”. Either make default -1 or ensure all call sites handle both -1 and 0 consistently.
Apply for consistency:
- default: - return 0 + default: + return -1
328-339: Make ctrl+u step dynamic (half the content height)Mirror Vim semantics and adapt to the current viewport instead of a fixed step.
Apply:
case "ctrl+u": if !m.showHelp { - newCursorPos := m.cursor - scrollHalfScreenLines + step := max(1, calculateContentHeight(m.height)/2) + newCursorPos := m.cursor - step if newCursorPos < 0 { m.cursor = 0 } else { - m.cursor -= scrollHalfScreenLines + m.cursor -= step } m.ensureCursorVisible() }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
model.go(3 hunks)view.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- view.go
🔇 Additional comments (4)
model.go (4)
306-312: LGTM: ‘down’ now guards with getMaxItems()Boundary check prevents overruns; cursor remains stable at end of list.
340-347: LGTM: ‘G’ clamps only when there are itemsThe maxItems >= 0 guard avoids negative cursor.
366-374: Prevent panic: add non‑negative cursor guard before indexing on EnterIf ctrl+d sets m.cursor = -1, the conditions here pass (−1 < len(...)) and indexing panics. Require m.cursor >= 0 at each branch.
Apply:
- if m.mode == viewEndpoints && m.cursor < len(m.endpoints) { + if m.mode == viewEndpoints && m.cursor >= 0 && m.cursor < len(m.endpoints) { m.endpoints[m.cursor].folded = !m.endpoints[m.cursor].folded - } else if m.mode == viewComponents && m.cursor < len(m.components) { + } else if m.mode == viewComponents && m.cursor >= 0 && m.cursor < len(m.components) { m.components[m.cursor].folded = !m.components[m.cursor].folded - } else if m.mode == viewWebhooks && m.cursor < len(m.webhooks) { + } else if m.mode == viewWebhooks && m.cursor >= 0 && m.cursor < len(m.webhooks) { m.webhooks[m.cursor].folded = !m.webhooks[m.cursor].folded }
314-327: Bug: ctrl+d can set cursor = -1 when list is emptyIf getMaxItems() == -1, this path sets m.cursor = -1, leading to potential panics when indexing elsewhere (e.g., enter toggles). Guard against empty lists before clamping/moving.
Apply this minimal fix:
case "ctrl+d": if !m.showHelp { - maxItems := m.getMaxItems() - newCursorPos := m.cursor + scrollHalfScreenLines - - if newCursorPos > maxItems { - m.cursor = maxItems - } else { - m.cursor += scrollHalfScreenLines - } - - m.ensureCursorVisible() + maxItems := m.getMaxItems() + if maxItems >= 0 { + newCursorPos := m.cursor + scrollHalfScreenLines + if newCursorPos > maxItems { + m.cursor = maxItems + } else { + m.cursor = newCursorPos + } + m.ensureCursorVisible() + } }
2 participants
The number of lines to scroll with Ctrl-D and Ctrl-U is the default vim's value of
scrolloption.Summary by CodeRabbit
New Features
Bug Fixes
Documentation