fix: PrevItem does not scroll viewport at top boundary#838
fix: PrevItem does not scroll viewport at top boundary#838lawrence3699 wants to merge 1 commit intodlvhdr:mainfrom
Conversation
PrevItem used `<` to check if the cursor was at the top of the viewport, but the symmetric NextItem correctly uses `>=` for the bottom boundary. This meant navigating up when the cursor was at the topmost visible row would move the cursor above the visible area without scrolling. Change the check to `> 0 && <=` so the viewport scrolls when the cursor is at the top bound (matching NextItem behavior), while the `> 0` guard prevents an unnecessary scroll when already on the first item.
There was a problem hiding this comment.
Pull request overview
Fixes an off-by-one boundary condition in the TUI list viewport navigation so that moving upward scrolls the viewport when the cursor reaches the topmost visible row (symmetric with the existing downward behavior).
Changes:
- Adjust
PrevItemtop-boundary detection to trigger scrolling when the cursor is at (or above) the top viewport boundary, while guarding against scrolling at item 0. - Add regression tests covering
PrevItemtop-boundary scrolling,NextItembottom-boundary scrolling, and first/last item edge behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
internal/tui/components/listviewport/listviewport.go |
Fixes PrevItem boundary check to scroll when the cursor is at the top visible row. |
internal/tui/components/listviewport/listviewport_test.go |
Adds regression tests for up/down boundary scrolling and navigation edge cases. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
dlvhdr
left a comment
dlvhdr
left a comment
There was a problem hiding this comment.
thanks for this! has been bugging me.
and also thx for adding tests!
|
|
||
| func (m *Model) PrevItem() int { | ||
| atTopOfViewport := m.currId < m.topBoundId | ||
| atTopOfViewport := m.currId > 0 && m.currId <= m.topBoundId |
There was a problem hiding this comment.
I don't really get the need for the m.currId > 0 condition. I tested it without it and it seems to work fine. Also the atTopOfViewport is misleading with that condition.
| func TestPrevItemScrollsAtTopBound(t *testing.T) { | ||
| // 10 items, viewport height 5, item height 1 => 5 items per page | ||
| // Initial bounds: topBoundId=0, bottomBoundId=4 | ||
| m := newTestModel(10, 5, 1) |
There was a problem hiding this comment.
can we add a struct for the arguements? makes it clearer
newTestModel(opts testModelOpts) Model {
// ...
}|
|
||
| // Navigate back up to the top bound | ||
| // 4 PrevItem calls: currId goes 4, 3, 2, 1 | ||
| for i := 0; i < 4; i++ { |
There was a problem hiding this comment.
maybe the 4 can be reused as opts.viewportHeight - 1?
Bug: When navigating up through a list that extends beyond one page, the viewport fails to scroll when the cursor reaches the topmost visible row. The cursor moves above the visible area, becoming invisible.
Cause:
PrevItemuses<to check if the cursor is at the top of the viewport, but the symmetricNextItemcorrectly uses>=for the bottom boundary. SincecurrIdis always>= topBoundIdduring normal step-by-step navigation, the<check never triggers when the cursor is at the top boundary.Before: Cursor at row 1,
topBoundId=1. Press up → cursor moves to row 0, viewport stays attopBoundId=1. Row 0 is above the visible area.After: Same scenario → viewport scrolls up to
topBoundId=0, cursor at row 0 is visible.Fix: Change the check to
> 0 && <=— the<=matchesNextItem's>=pattern, and the> 0guard prevents an unnecessary scroll when already at the first item.Validation: Added regression tests for
PrevItem/NextItemboundary scrolling and edge cases.go test ./...andgo build ./...both pass.AI Disclosure: Claude Code was used to identify the asymmetry and draft the fix and tests. I (the submitter) fully understand the off-by-one:
NextItemscrolls whencurrId >= bottomBoundId(at or past the bottom row), butPrevItemonly scrolled whencurrId < topBoundId(already past the top row, which doesn't happen in single-step navigation). The<=makes both directions scroll when the cursor is at the boundary.