Skip to content

Commit 598b386

Browse files
wesmclaude
wesm
and
claude
authored
Fix page up/down scroll to preserve cursor position (#90)
## Summary - **Off-by-one fix**: Page up/down now moves by `visibleRows()` (pageSize-1) instead of `pageSize`, accounting for the info line that occupies one row - **Position preservation**: Cursor and scrollOffset move together during paging, so the cursor stays at the same relative position in the viewport instead of snapping to the edge - **Thread view**: Same fixes applied to thread view page up/down in `keys.go` ## Changes - `navigation.go`: Added `visibleRows()` helper, fixed `navigateList` page up/down to move cursor+scrollOffset together and return early (skipping `ensureCursorVisible`), simplified `ensureCursorVisible`/`ensureThreadCursorVisible` - `keys.go`: Fixed thread view page up/down with same pattern - `nav_test.go`: Added table-driven tests for page down, page up, and `visibleRows()` edge cases ## Test plan - [x] `go test ./internal/tui/ -v` — all pass - [x] `make lint` — clean - [x] `make test` — all pass 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
1 parent cf43372 commit 598b386

File tree

3 files changed

+387
-22
lines changed

3 files changed

+387
-22
lines changed

‎internal/tui/keys.go‎

Lines changed: 19 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -810,20 +810,33 @@ func (m Model) handleThreadViewKeys(msg tea.KeyMsg) (tea.Model, tea.Cmd) {
810810
m.ensureThreadCursorVisible()
811811
}
812812
case "pgup", "ctrl+u":
813-
m.threadCursor -= m.pageSize
813+
step := m.visibleRows()
814+
m.threadCursor -= step
815+
m.threadScrollOffset -= step
814816
if m.threadCursor < 0 {
815817
m.threadCursor = 0
816818
}
817-
m.ensureThreadCursorVisible()
819+
if m.threadScrollOffset < 0 {
820+
m.threadScrollOffset = 0
821+
}
818822
case "pgdown", "ctrl+d":
819-
m.threadCursor += m.pageSize
820-
if m.threadCursor >= len(m.threadMessages) {
821-
m.threadCursor = len(m.threadMessages) - 1
823+
step := m.visibleRows()
824+
itemCount := len(m.threadMessages)
825+
m.threadCursor += step
826+
m.threadScrollOffset += step
827+
if m.threadCursor >= itemCount {
828+
m.threadCursor = itemCount - 1
822829
}
823830
if m.threadCursor < 0 {
824831
m.threadCursor = 0
825832
}
826-
m.ensureThreadCursorVisible()
833+
maxScroll := itemCount - m.visibleRows()
834+
if maxScroll < 0 {
835+
maxScroll = 0
836+
}
837+
if m.threadScrollOffset > maxScroll {
838+
m.threadScrollOffset = maxScroll
839+
}
827840

828841
// View message detail
829842
case "enter":

‎internal/tui/nav_test.go‎

Lines changed: 337 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package tui
33
import (
44
"testing"
55

6+
tea "github.com/charmbracelet/bubbletea"
67
"github.com/wesm/msgvault/internal/query"
78
)
89

@@ -201,3 +202,339 @@ func TestNavigateList(t *testing.T) {
201202
})
202203
}
203204
}
205+
206+
// =============================================================================
207+
// Page Up/Down Scroll Tests
208+
// =============================================================================
209+
210+
func TestNavigateListPageDown(t *testing.T) {
211+
tests := []struct {
212+
name string
213+
pageSize int // raw pageSize; visibleRows = pageSize - 1
214+
itemCount int
215+
initCursor int
216+
initScrollOffset int
217+
wantCursor int
218+
wantScrollOffset int
219+
}{
220+
{
221+
name: "moves by visibleRows not pageSize",
222+
pageSize: 24,
223+
itemCount: 100,
224+
initCursor: 0,
225+
initScrollOffset: 0,
226+
wantCursor: 23, // visibleRows = 24 - 1 = 23
227+
wantScrollOffset: 23,
228+
},
229+
{
230+
name: "preserves relative cursor position",
231+
pageSize: 24,
232+
itemCount: 100,
233+
initCursor: 5,
234+
initScrollOffset: 0,
235+
wantCursor: 28, // 5 + 23
236+
wantScrollOffset: 23, // 0 + 23
237+
},
238+
{
239+
name: "clamps cursor at end of list",
240+
pageSize: 24,
241+
itemCount: 30,
242+
initCursor: 20,
243+
initScrollOffset: 10,
244+
wantCursor: 29, // clamped to itemCount-1
245+
wantScrollOffset: 7, // clamped: 30 - 23 = 7
246+
},
247+
{
248+
name: "clamps scroll at max",
249+
pageSize: 24,
250+
itemCount: 40,
251+
initCursor: 25,
252+
initScrollOffset: 15,
253+
wantCursor: 39, // clamped to 39
254+
wantScrollOffset: 17, // max: 40 - 23 = 17
255+
},
256+
{
257+
name: "small list fewer items than visibleRows",
258+
pageSize: 24,
259+
itemCount: 10,
260+
initCursor: 3,
261+
initScrollOffset: 0,
262+
wantCursor: 9,
263+
wantScrollOffset: 0, // max scroll = 0 since items < visibleRows
264+
},
265+
}
266+
267+
for _, tt := range tests {
268+
t.Run(tt.name, func(t *testing.T) {
269+
m := NewBuilder().WithPageSizeRaw(tt.pageSize).Build()
270+
m.cursor = tt.initCursor
271+
m.scrollOffset = tt.initScrollOffset
272+
273+
handled := m.navigateList("pgdown", tt.itemCount)
274+
if !handled {
275+
t.Fatal("expected pgdown to be handled")
276+
}
277+
if m.cursor != tt.wantCursor {
278+
t.Errorf("cursor = %d, want %d", m.cursor, tt.wantCursor)
279+
}
280+
if m.scrollOffset != tt.wantScrollOffset {
281+
t.Errorf("scrollOffset = %d, want %d", m.scrollOffset, tt.wantScrollOffset)
282+
}
283+
})
284+
}
285+
}
286+
287+
func TestNavigateListPageUp(t *testing.T) {
288+
tests := []struct {
289+
name string
290+
pageSize int
291+
itemCount int
292+
initCursor int
293+
initScrollOffset int
294+
wantCursor int
295+
wantScrollOffset int
296+
}{
297+
{
298+
name: "moves by visibleRows not pageSize",
299+
pageSize: 24,
300+
itemCount: 100,
301+
initCursor: 50,
302+
initScrollOffset: 30,
303+
wantCursor: 27, // 50 - 23
304+
wantScrollOffset: 7, // 30 - 23
305+
},
306+
{
307+
name: "preserves relative cursor position",
308+
pageSize: 24,
309+
itemCount: 100,
310+
initCursor: 30,
311+
initScrollOffset: 25,
312+
wantCursor: 7,
313+
wantScrollOffset: 2,
314+
},
315+
{
316+
name: "clamps cursor at top",
317+
pageSize: 24,
318+
itemCount: 100,
319+
initCursor: 10,
320+
initScrollOffset: 5,
321+
wantCursor: 0,
322+
wantScrollOffset: 0,
323+
},
324+
{
325+
name: "clamps scroll at zero",
326+
pageSize: 24,
327+
itemCount: 100,
328+
initCursor: 30,
329+
initScrollOffset: 10,
330+
wantCursor: 7, // 30 - 23
331+
wantScrollOffset: 0, // clamped to 0
332+
},
333+
}
334+
335+
for _, tt := range tests {
336+
t.Run(tt.name, func(t *testing.T) {
337+
m := NewBuilder().WithPageSizeRaw(tt.pageSize).Build()
338+
m.cursor = tt.initCursor
339+
m.scrollOffset = tt.initScrollOffset
340+
341+
handled := m.navigateList("pgup", tt.itemCount)
342+
if !handled {
343+
t.Fatal("expected pgup to be handled")
344+
}
345+
if m.cursor != tt.wantCursor {
346+
t.Errorf("cursor = %d, want %d", m.cursor, tt.wantCursor)
347+
}
348+
if m.scrollOffset != tt.wantScrollOffset {
349+
t.Errorf("scrollOffset = %d, want %d", m.scrollOffset, tt.wantScrollOffset)
350+
}
351+
})
352+
}
353+
}
354+
355+
// =============================================================================
356+
// Thread View Page Up/Down Tests
357+
// =============================================================================
358+
359+
func TestThreadViewPageDown(t *testing.T) {
360+
tests := []struct {
361+
name string
362+
pageSize int
363+
threadMsgCount int
364+
initCursor int
365+
initScrollOffset int
366+
wantCursor int
367+
wantScrollOffset int
368+
}{
369+
{
370+
name: "moves by visibleRows not pageSize",
371+
pageSize: 24,
372+
threadMsgCount: 100,
373+
initCursor: 0,
374+
initScrollOffset: 0,
375+
wantCursor: 23,
376+
wantScrollOffset: 23,
377+
},
378+
{
379+
name: "preserves relative cursor position",
380+
pageSize: 24,
381+
threadMsgCount: 100,
382+
initCursor: 5,
383+
initScrollOffset: 0,
384+
wantCursor: 28,
385+
wantScrollOffset: 23,
386+
},
387+
{
388+
name: "clamps at end of thread",
389+
pageSize: 24,
390+
threadMsgCount: 30,
391+
initCursor: 20,
392+
initScrollOffset: 10,
393+
wantCursor: 29,
394+
wantScrollOffset: 7,
395+
},
396+
{
397+
name: "small thread fewer items than visibleRows",
398+
pageSize: 24,
399+
threadMsgCount: 5,
400+
initCursor: 1,
401+
initScrollOffset: 0,
402+
wantCursor: 4,
403+
wantScrollOffset: 0,
404+
},
405+
{
406+
name: "empty thread",
407+
pageSize: 24,
408+
threadMsgCount: 0,
409+
initCursor: 0,
410+
initScrollOffset: 0,
411+
wantCursor: 0,
412+
wantScrollOffset: 0,
413+
},
414+
}
415+
416+
for _, tt := range tests {
417+
t.Run(tt.name, func(t *testing.T) {
418+
m := NewBuilder().
419+
WithLevel(levelThreadView).
420+
WithPageSizeRaw(tt.pageSize).
421+
WithLoading(false).
422+
Build()
423+
m.threadMessages = makeMessages(tt.threadMsgCount)
424+
m.threadCursor = tt.initCursor
425+
m.threadScrollOffset = tt.initScrollOffset
426+
427+
m, _ = sendKey(t, m, tea.KeyMsg{Type: tea.KeyPgDown})
428+
429+
if m.threadCursor != tt.wantCursor {
430+
t.Errorf("threadCursor = %d, want %d", m.threadCursor, tt.wantCursor)
431+
}
432+
if m.threadScrollOffset != tt.wantScrollOffset {
433+
t.Errorf("threadScrollOffset = %d, want %d", m.threadScrollOffset, tt.wantScrollOffset)
434+
}
435+
})
436+
}
437+
}
438+
439+
func TestThreadViewPageUp(t *testing.T) {
440+
tests := []struct {
441+
name string
442+
pageSize int
443+
threadMsgCount int
444+
initCursor int
445+
initScrollOffset int
446+
wantCursor int
447+
wantScrollOffset int
448+
}{
449+
{
450+
name: "moves by visibleRows not pageSize",
451+
pageSize: 24,
452+
threadMsgCount: 100,
453+
initCursor: 50,
454+
initScrollOffset: 30,
455+
wantCursor: 27,
456+
wantScrollOffset: 7,
457+
},
458+
{
459+
name: "preserves relative cursor position",
460+
pageSize: 24,
461+
threadMsgCount: 100,
462+
initCursor: 30,
463+
initScrollOffset: 25,
464+
wantCursor: 7,
465+
wantScrollOffset: 2,
466+
},
467+
{
468+
name: "clamps at top",
469+
pageSize: 24,
470+
threadMsgCount: 100,
471+
initCursor: 10,
472+
initScrollOffset: 5,
473+
wantCursor: 0,
474+
wantScrollOffset: 0,
475+
},
476+
{
477+
name: "small thread",
478+
pageSize: 24,
479+
threadMsgCount: 5,
480+
initCursor: 4,
481+
initScrollOffset: 0,
482+
wantCursor: 0,
483+
wantScrollOffset: 0,
484+
},
485+
{
486+
name: "empty thread",
487+
pageSize: 24,
488+
threadMsgCount: 0,
489+
initCursor: 0,
490+
initScrollOffset: 0,
491+
wantCursor: 0,
492+
wantScrollOffset: 0,
493+
},
494+
}
495+
496+
for _, tt := range tests {
497+
t.Run(tt.name, func(t *testing.T) {
498+
m := NewBuilder().
499+
WithLevel(levelThreadView).
500+
WithPageSizeRaw(tt.pageSize).
501+
WithLoading(false).
502+
Build()
503+
m.threadMessages = makeMessages(tt.threadMsgCount)
504+
m.threadCursor = tt.initCursor
505+
m.threadScrollOffset = tt.initScrollOffset
506+
507+
m, _ = sendKey(t, m, tea.KeyMsg{Type: tea.KeyPgUp})
508+
509+
if m.threadCursor != tt.wantCursor {
510+
t.Errorf("threadCursor = %d, want %d", m.threadCursor, tt.wantCursor)
511+
}
512+
if m.threadScrollOffset != tt.wantScrollOffset {
513+
t.Errorf("threadScrollOffset = %d, want %d", m.threadScrollOffset, tt.wantScrollOffset)
514+
}
515+
})
516+
}
517+
}
518+
519+
func TestVisibleRows(t *testing.T) {
520+
tests := []struct {
521+
name string
522+
pageSize int
523+
want int
524+
}{
525+
{"normal", 24, 23},
526+
{"small", 2, 1},
527+
{"minimum clamped", 1, 1},
528+
{"zero clamped", 0, 1},
529+
{"negative clamped", -5, 1},
530+
}
531+
532+
for _, tt := range tests {
533+
t.Run(tt.name, func(t *testing.T) {
534+
m := NewBuilder().WithPageSizeRaw(tt.pageSize).Build()
535+
if got := m.visibleRows(); got != tt.want {
536+
t.Errorf("visibleRows() = %d, want %d", got, tt.want)
537+
}
538+
})
539+
}
540+
}

0 commit comments

Comments
 (0)