make it scroll with arrows as well #428
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe update refactors the scrolling logic for the runtime and template selection steps in the project form UI. It replaces free scrolling with a paged window approach, introduces new fields to manage visible items, updates input handling for mouse and keyboard, revises rendering logic, and removes unused styles and scroll behaviors. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ProjectFormModel
participant Viewport
User->>ProjectFormModel: Keyboard/Mouse Input (Up/Down/Click/Enter)
ProjectFormModel->>ProjectFormModel: Update cursor position
ProjectFormModel->>ProjectFormModel: Call ensureCursorVisible()
ProjectFormModel->>Viewport: Set YOffset to 0
ProjectFormModel->>ProjectFormModel: Slice visible items (windowStart:windowStart+windowSize)
ProjectFormModel->>User: Render visible items with scroll indicators
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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)
internal/ui/project.go (3)
929-938: Viewport width ignores actual terminal width
contentWidth := width - 2still uses the global constantwidth(76) even after the window has been resized andm.widthwas updated a few lines earlier.
On narrow terminals the viewport can overflow, breaking layout.Calculate the viewport width from
m.width(clamped to a minimum) so the UI always fits:- contentWidth := width - 2 + // leave at least 2 columns for borders + contentWidth := m.width - 2
998-1008:ensureCursorVisiblecan leavewindowStartpast the endWhen the number of items shrinks (e.g. after filtering)
windowStart
may stay >totalItems-windowSize, leading to an empty viewport slice.Clamp
windowStartto the last valid starting index:windowEnd := m.windowStart + m.windowSize - 1 if m.cursor > windowEnd { m.windowStart = m.cursor - (m.windowSize - 1) } + +maxStart := totalItems - m.windowSize +if maxStart < 0 { + maxStart = 0 +} +if m.windowStart > maxStart { + m.windowStart = maxStart +}
893-902: Redundant viewport update after zeroing Y-offsetAfter forcibly setting
m.viewport.SetYOffset(0)the subsequent
m.viewport.Update(msg)still processes the same message, although
scrolling is now disabled.
Skipping the update whenYOffsetis constant would avoid a needless
layout pass. (Minor performance nit.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/ui/project.go(24 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Test CLI Upgrade Path (windows-latest)
- GitHub Check: Build and Test (macos-latest)
- GitHub Check: Build and Test (blacksmith-4vcpu-ubuntu-2204-arm)
- GitHub Check: Analyze (go)
internal/ui/project.go
Outdated
| case tea.MouseMsg: | ||
| if !m.ready { | ||
| return m, nil | ||
| } | ||
| if msg.String() == "MouseLeft" { | ||
| m.mouseY = msg.Y | ||
| // Approximate top offset of list area; keep prior 6-line offset heuristic | ||
| localIndex := (msg.Y - 6) / m.itemHeight | ||
| clickedIndex := m.windowStart + localIndex | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Magic 6 offset risks mis-mapping mouse clicks
localIndex := (msg.Y - 6) / m.itemHeight hard-codes a vertical offset of 6 rows.
Any future tweak to the header height (or different terminal sizes) will desynchronise the click mapping and select the wrong item.
Derive the offset from the actual layout (e.g. sum of titleBarHeight, headerHeight, top margin) instead of keeping a literal.
🤖 Prompt for AI Agents
In internal/ui/project.go around lines 429 to 438, replace the hard-coded
vertical offset value 6 used in calculating localIndex with a dynamic value
derived from the actual layout measurements such as the sum of titleBarHeight,
headerHeight, and any top margin. This ensures the mouse click mapping stays
accurate even if the header or layout changes. Identify or add variables
representing these layout heights and use their sum instead of the literal 6 in
the calculation.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/ui/project.go (1)
434-437: Hard-coded “6” offset is still here (dup of prior feedback)Mouse-click mapping still subtracts a literal 6 (
(msg.Y - 6)/m.itemHeight).
This was flagged before; any header/layout tweak will break hit-testing.
Please derive the offset from the same constants you use ininitViewport()(e.g.titleBarHeight + headerHeight + verticalMargins) instead of a magic number.
🧹 Nitpick comments (1)
internal/ui/project.go (1)
915-921: All layout heights are hard-coded – fragile on future UI changes
titleBarHeight,headerHeight,footerHeight, andverticalMarginsare fixed literals.
If any of those sections change (more/less lines, different padding) the viewport math and click-mapping will desynchronise again. Consider:
- Storing these values in one place (constants or methods) shared by both
initViewportand mouse-click calculations.- Or computing them from rendered strings’ line counts to stay self-consistent.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/ui/project.go(19 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
Summary by CodeRabbit