From 167a64a2ebcc489546d9c848d46da3d6b0d311e5 Mon Sep 17 00:00:00 2001 From: Matthew Goodwin Date: Fri, 25 Sep 2026 09:31:57 -0500 Subject: [PATCH 1/2] RT-298: scroll through a diff line taller than the pane row by row A changed line that wrapped to more rows than the diff pane pinned its first row on top, so its middle and end were unreachable. A cursor step on such a line now scrolls it one row until its far end is on screen, then moves to the next line; a line entered from below opens at its bottom. The wheel takes the same steps. The viewport clamps diffTop into the tall line's range, so a click or a stage keeps the rows being read. Staging and clicks still act on the whole line. Co-Authored-By: Claude Opus 5.5 (1M context) --- ui/internal/views/mission/diff.go | 62 +++++++++++--- ui/internal/views/mission/diff_wrap_test.go | 94 ++++++++++++++++++--- ui/internal/views/mission/mission.go | 5 +- 3 files changed, 140 insertions(+), 21 deletions(-) diff --git a/ui/internal/views/mission/diff.go b/ui/internal/views/mission/diff.go index 9fa6e725d..896f23926 100644 --- a/ui/internal/views/mission/diff.go +++ b/ui/internal/views/mission/diff.go @@ -100,18 +100,53 @@ func (m *Mission) clampDiffCursor() { } } +// moveDiffCursor takes |delta| steps. A step on a cursor line taller than +// the pane scrolls it one row until its far end is on screen; only then +// does the cursor move to the next line, and a line entered from below +// opens at its bottom so reading upward never jumps. func (m *Mission) moveDiffCursor(delta int) { n := len(m.model.Diff.Lines) if n == 0 { return } - m.diffCursor += delta - if m.diffCursor < 0 { - m.diffCursor = 0 + step := 1 + if delta < 0 { + step = -1 } - if m.diffCursor >= n { - m.diffCursor = n - 1 + for ; delta != 0; delta -= step { + if lo, hi, ok := m.tallCursorSpan(); ok { + if next := min(max(m.diffTop, lo), hi) + step; next >= lo && next <= hi { + m.diffTop = next + continue + } + } + next := min(max(m.diffCursor+step, 0), n-1) + if next == m.diffCursor { + return + } + m.diffCursor = next + if lo, hi, ok := m.tallCursorSpan(); ok { + m.diffTop = lo + if step < 0 { + m.diffTop = hi + } + } + } +} + +// tallCursorSpan is the diffTop range [lo, hi] that reads the cursor line +// through the pane when it wraps to more rows than the pane holds, from the +// row index and height of the last render; ok is false for a line that fits. +func (m *Mission) tallCursorSpan() (lo, hi int, ok bool) { + lines, ix, h := m.model.Diff.Lines, &m.diffRowsCache, m.diffViewH + if h <= 0 || len(lines) == 0 || ix.n != len(lines) || ix.key != &lines[0] { + return 0, 0, false + } + c := min(max(m.diffCursor, 0), len(lines)-1) + if ix.start[c+1]-ix.start[c] <= h { + return 0, 0, false } + return ix.start[c], ix.start[c+1] - h, true } type diffStagePayload struct { @@ -376,11 +411,18 @@ func (m *Mission) renderDiffLines(width, height int) string { } // The viewport sees the cursor line's extra rows collapsed into its // first, over a pane shrunk by the same amount: the whole line then - // stays in view with scrolloff around it, and a line taller than the - // pane (a one-row virtual pane) keeps its first row on top. Rows above - // the cursor line are the same in both spaces, which is all top can be. - virtualH := max(height-extra, 1) - top, _ := picker.Viewport(cursorRow, m.diffTop, total-extra, virtualH, virtualH, 0) + // stays in view with scrolloff around it. Rows above the cursor line + // are the same in both spaces, which is all top can be. A line taller + // than the pane fills it instead, at whichever of its rows + // moveDiffCursor has scrolled to. + m.diffViewH = height + var top int + if lo, hi, ok := m.tallCursorSpan(); ok { + top = min(max(m.diffTop, lo), hi) + } else { + virtualH := max(height-extra, 1) + top, _ = picker.Viewport(cursorRow, m.diffTop, total-extra, virtualH, virtualH, 0) + } m.diffTop = top thumbTop, thumbH := picker.ThumbSpan(top, min(height, total), total) thumbOn := lipgloss.NewStyle().Background(theme.Panel) diff --git a/ui/internal/views/mission/diff_wrap_test.go b/ui/internal/views/mission/diff_wrap_test.go index d8c72cca9..39e961b5f 100644 --- a/ui/internal/views/mission/diff_wrap_test.go +++ b/ui/internal/views/mission/diff_wrap_test.go @@ -152,20 +152,94 @@ func TestCursorLineIsKeptWholeInView(t *testing.T) { } } -func TestCursorLineTallerThanThePaneKeepsItsFirstRowOnTop(t *testing.T) { - m := wrapFixture() - m.model.Diff.Lines[1].Text = strings.Repeat("word ", 400) +// tallFixture puts the cursor on a line wrapping to far more rows than a +// 3-row pane, rendered once so the row index and pane height are known. +func tallFixture(t *testing.T) (m *Mission, lo, hi int) { + t.Helper() + m = wrapFixture() + m.model.Diff.Lines[1].Text = strings.Repeat("word ", 60) m.diffCursor = 1 - rows := strings.Split(ansi.Strip(m.renderDiffLines(30, 3)), "\n") - if !strings.Contains(rows[0], " 1 + word") { + m.renderDiffLines(tallW, tallH) + ix := m.diffRows(tallW - 1 - diffGutterWidth - diffMarkWidth) + lo, hi = ix.start[1], ix.start[2]-tallH + if hi-lo < 3 { + t.Fatalf("fixture line is not tall enough: rows [%d,%d)", ix.start[1], ix.start[2]) + } + return m, lo, hi +} + +const tallW, tallH = 30, 3 + +func TestCursorLineTallerThanThePaneKeepsItsFirstRowOnTop(t *testing.T) { + m, lo, _ := tallFixture(t) + rows := strings.Split(ansi.Strip(m.renderDiffLines(tallW, tallH)), "\n") + if m.diffTop != lo || !strings.Contains(rows[0], " 1 + word") { t.Fatalf("first row of the cursor line is not on top:\n%s", strings.Join(rows, "\n")) } +} + +func TestScrollingDownReadsATallLineRowByRowThenMovesOn(t *testing.T) { + m, lo, hi := tallFixture(t) + for want := lo + 1; want <= hi; want++ { + m.moveDiffCursor(1) + m.renderDiffLines(tallW, tallH) + if m.diffCursor != 1 || m.diffTop != want { + t.Fatalf("step to row %d: cursor %d top %d", want, m.diffCursor, m.diffTop) + } + } m.moveDiffCursor(1) - rows = strings.Split(ansi.Strip(m.renderDiffLines(30, 3)), "\n") - // tail is the last line, so the window clamps at the end of the diff - // with tail on its bottom row rather than its top. - if len(rows) != 3 || !strings.Contains(rows[2], " 2 + tail") || strings.Contains(rows[0], " 1 + word") { - t.Fatalf("the window did not follow the cursor off the tall line:\n%s", strings.Join(rows, "\n")) + rows := strings.Split(ansi.Strip(m.renderDiffLines(tallW, tallH)), "\n") + if m.diffCursor != 2 || !strings.Contains(rows[len(rows)-1], " 2 + tail") { + t.Fatalf("past the tall line's last row the cursor should reach tail, got cursor %d:\n%s", m.diffCursor, strings.Join(rows, "\n")) + } +} + +func TestScrollingUpIntoATallLineStartsAtItsBottom(t *testing.T) { + m, lo, hi := tallFixture(t) + m.diffCursor = 2 + m.renderDiffLines(tallW, tallH) + m.moveDiffCursor(-1) + m.renderDiffLines(tallW, tallH) + if m.diffCursor != 1 || m.diffTop != hi { + t.Fatalf("entering from below: cursor %d top %d, want 1 and %d", m.diffCursor, m.diffTop, hi) + } + for want := hi - 1; want >= lo; want-- { + m.moveDiffCursor(-1) + m.renderDiffLines(tallW, tallH) + if m.diffCursor != 1 || m.diffTop != want { + t.Fatalf("step up to row %d: cursor %d top %d", want, m.diffCursor, m.diffTop) + } + } + m.moveDiffCursor(-1) + if m.diffCursor != 0 { + t.Fatalf("above the tall line's first row the cursor should reach the hunk, got %d", m.diffCursor) + } +} + +func TestAWheelTickSpillsPastTheTallLineOntoTheNext(t *testing.T) { + m, _, hi := tallFixture(t) + m.diffTop = hi - 2 + m.moveDiffCursor(2) + if m.diffCursor != 1 || m.diffTop != hi { + t.Fatalf("two steps two rows from the end should land on the last row, got cursor %d top %d", m.diffCursor, m.diffTop) + } + m.diffTop = hi - 1 + m.moveDiffCursor(2) + if m.diffCursor != 2 { + t.Fatalf("one step to the last row and one onto tail, got cursor %d top %d", m.diffCursor, m.diffTop) + } +} + +// A click or a model push (staging the line) leaves diffCursor on the tall +// line: the rows being read stay put rather than snapping to its top. +func TestATallLineKeepsTheRowsBeingReadAcrossARender(t *testing.T) { + m, lo, hi := tallFixture(t) + mid := (lo + hi) / 2 + m.diffTop = mid + m.diffCursor = 1 + m.renderDiffLines(tallW, tallH) + if m.diffTop != mid { + t.Fatalf("top moved from %d to %d", mid, m.diffTop) } } diff --git a/ui/internal/views/mission/mission.go b/ui/internal/views/mission/mission.go index a179ede87..a3b91ffdb 100644 --- a/ui/internal/views/mission/mission.go +++ b/ui/internal/views/mission/mission.go @@ -70,10 +70,13 @@ type Mission struct { // a line index. diffPath is the Diff.Path last seen, so a model swap // that keeps the same file (a stage refreshing the hunk) preserves the // cursor while one that shows a different file resets it (diff.go's - // clampDiffCursor). + // clampDiffCursor). diffViewH is the pane height of the last render, + // which a key or wheel step needs to know whether the cursor line is + // taller than the pane (tallCursorSpan). diffCursor int diffTop int diffPath string + diffViewH int diffHL diffHighlighter diffRowsCache diffRowIndex From 9e47161b81e44352ab870c0dc9146712b60101f0 Mon Sep 17 00:00:00 2001 From: Matthew Goodwin Date: Fri, 25 Sep 2026 09:38:42 -0500 Subject: [PATCH 2/2] RT-298: anchor the tall-line offset to its line, not an absolute row Review of #459: an absolute diffTop carried a mid-line position onto a different tall line that a discard shifted under the cursor, and a resize re-wrapping the lines above jumped the reader to the line's top or bottom. The offset into the cursor line is now held with the line it belongs to, so a new line opens at its first row and a resize keeps the place within the line. Co-Authored-By: Claude Opus 5.5 (1M context) --- ui/internal/views/mission/diff.go | 43 ++++++++++++--- ui/internal/views/mission/diff_wrap_test.go | 61 +++++++++++++++++---- ui/internal/views/mission/mission.go | 6 +- 3 files changed, 90 insertions(+), 20 deletions(-) diff --git a/ui/internal/views/mission/diff.go b/ui/internal/views/mission/diff.go index 896f23926..ce65bda1c 100644 --- a/ui/internal/views/mission/diff.go +++ b/ui/internal/views/mission/diff.go @@ -115,8 +115,8 @@ func (m *Mission) moveDiffCursor(delta int) { } for ; delta != 0; delta -= step { if lo, hi, ok := m.tallCursorSpan(); ok { - if next := min(max(m.diffTop, lo), hi) + step; next >= lo && next <= hi { - m.diffTop = next + if next := m.cursorRowOff(hi-lo) + step; next >= 0 && next <= hi-lo { + m.setCursorRowOff(next) continue } } @@ -125,15 +125,42 @@ func (m *Mission) moveDiffCursor(delta int) { return } m.diffCursor = next - if lo, hi, ok := m.tallCursorSpan(); ok { - m.diffTop = lo - if step < 0 { - m.diffTop = hi - } + m.setCursorRowOff(0) + if lo, hi, ok := m.tallCursorSpan(); ok && step < 0 { + m.setCursorRowOff(hi - lo) } } } +// cursorRowOff is how many rows into a tall cursor line the pane opens, +// capped at maxOff. The offset belongs to the line it was set on, so a +// different line landing under the cursor (a discard shifting the diff, a +// same-path file in another commit) opens at its first row, and a resize +// keeps the reader's place within the line rather than an absolute row. +func (m *Mission) cursorRowOff(maxOff int) int { + lines := m.model.Diff.Lines + c := min(max(m.diffCursor, 0), len(lines)-1) + if c < 0 || !sameDiffLine(m.diffRowOffAt, lines[c]) { + return 0 + } + return min(max(m.diffRowOff, 0), maxOff) +} + +func (m *Mission) setCursorRowOff(off int) { + lines := m.model.Diff.Lines + c := min(max(m.diffCursor, 0), len(lines)-1) + if c < 0 { + return + } + m.diffRowOff, m.diffRowOffAt = off, lines[c] +} + +// sameDiffLine matches one diff line across model pushes: staging it +// changes Selected, never its text or position. +func sameDiffLine(a, b DiffLine) bool { + return a.Kind == b.Kind && a.OldNo == b.OldNo && a.NewNo == b.NewNo && a.Text == b.Text +} + // tallCursorSpan is the diffTop range [lo, hi] that reads the cursor line // through the pane when it wraps to more rows than the pane holds, from the // row index and height of the last render; ok is false for a line that fits. @@ -418,7 +445,7 @@ func (m *Mission) renderDiffLines(width, height int) string { m.diffViewH = height var top int if lo, hi, ok := m.tallCursorSpan(); ok { - top = min(max(m.diffTop, lo), hi) + top = lo + m.cursorRowOff(hi-lo) } else { virtualH := max(height-extra, 1) top, _ = picker.Viewport(cursorRow, m.diffTop, total-extra, virtualH, virtualH, 0) diff --git a/ui/internal/views/mission/diff_wrap_test.go b/ui/internal/views/mission/diff_wrap_test.go index 39e961b5f..0f8bd53d7 100644 --- a/ui/internal/views/mission/diff_wrap_test.go +++ b/ui/internal/views/mission/diff_wrap_test.go @@ -217,29 +217,68 @@ func TestScrollingUpIntoATallLineStartsAtItsBottom(t *testing.T) { } func TestAWheelTickSpillsPastTheTallLineOntoTheNext(t *testing.T) { - m, _, hi := tallFixture(t) - m.diffTop = hi - 2 + m, lo, hi := tallFixture(t) + m.setCursorRowOff(hi - 2 - lo) m.moveDiffCursor(2) + m.renderDiffLines(tallW, tallH) if m.diffCursor != 1 || m.diffTop != hi { t.Fatalf("two steps two rows from the end should land on the last row, got cursor %d top %d", m.diffCursor, m.diffTop) } - m.diffTop = hi - 1 + m.setCursorRowOff(hi - 1 - lo) m.moveDiffCursor(2) if m.diffCursor != 2 { t.Fatalf("one step to the last row and one onto tail, got cursor %d top %d", m.diffCursor, m.diffTop) } } -// A click or a model push (staging the line) leaves diffCursor on the tall -// line: the rows being read stay put rather than snapping to its top. -func TestATallLineKeepsTheRowsBeingReadAcrossARender(t *testing.T) { - m, lo, hi := tallFixture(t) - mid := (lo + hi) / 2 - m.diffTop = mid +// pushLines stands in for a model push: a fresh Lines backing array, as +// every decode makes, with the cursor line's stage state flipped. +func pushLines(m *Mission, edit func([]DiffLine)) { + lines := append([]DiffLine(nil), m.model.Diff.Lines...) + lines[1].Selected = !lines[1].Selected + if edit != nil { + edit(lines) + } + m.model.Diff.Lines = lines +} + +func TestStagingATallLineKeepsTheRowsBeingRead(t *testing.T) { + m, lo, _ := tallFixture(t) + m.moveDiffCursor(2) + pushLines(m, nil) + m.renderDiffLines(tallW, tallH) + if m.diffTop != lo+2 { + t.Fatalf("top %d after the push, want %d", m.diffTop, lo+2) + } +} + +func TestADifferentTallLineUnderTheCursorOpensAtItsTop(t *testing.T) { + m, lo, _ := tallFixture(t) + m.moveDiffCursor(2) + pushLines(m, func(lines []DiffLine) { lines[1].Text = strings.Repeat("other ", 50) }) + m.renderDiffLines(tallW, tallH) + if m.diffTop != lo { + t.Fatalf("a new line under the cursor opened at top %d, want its first row %d", m.diffTop, lo) + } +} + +// The line above the tall one wraps too, so a resize moves where the tall +// line starts. +func TestAResizeKeepsThePlaceWithinATallLine(t *testing.T) { + m := wrapFixture() + m.model.Diff.Lines = []DiffLine{ + {Kind: "context", OldNo: 1, NewNo: 1, SelIdx: -1, Text: strings.Repeat("ctx ", 20)}, + {Kind: "add", NewNo: 2, Text: strings.Repeat("word ", 60)}, + {Kind: "add", NewNo: 3, Text: "tail"}, + } m.diffCursor = 1 m.renderDiffLines(tallW, tallH) - if m.diffTop != mid { - t.Fatalf("top moved from %d to %d", mid, m.diffTop) + m.moveDiffCursor(2) + const wider = tallW + 10 + m.renderDiffLines(wider, tallH) + ix := m.diffRows(wider - 1 - diffGutterWidth - diffMarkWidth) + if got := m.diffTop - ix.start[1]; got != 2 { + t.Fatalf("offset into the line after widening is %d rows, want 2", got) } } diff --git a/ui/internal/views/mission/mission.go b/ui/internal/views/mission/mission.go index a3b91ffdb..313001868 100644 --- a/ui/internal/views/mission/mission.go +++ b/ui/internal/views/mission/mission.go @@ -72,11 +72,15 @@ type Mission struct { // cursor while one that shows a different file resets it (diff.go's // clampDiffCursor). diffViewH is the pane height of the last render, // which a key or wheel step needs to know whether the cursor line is - // taller than the pane (tallCursorSpan). + // taller than the pane (tallCursorSpan); diffRowOff is how far into + // such a line the pane reads, owned by the line diffRowOffAt + // (cursorRowOff). diffCursor int diffTop int diffPath string diffViewH int + diffRowOff int + diffRowOffAt DiffLine diffHL diffHighlighter diffRowsCache diffRowIndex