From f48cd450dc950237da189c3c8470aae8a5404146 Mon Sep 17 00:00:00 2001 From: Umputun Date: Fri, 28 Aug 2026 11:51:32 -0500 Subject: [PATCH] fix(ui): bound horizontal scroll to the widest rendered line MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Holding the right arrow scrolled scrollX past the widest line without limit. Once past every line applyHorizontalScroll takes its plain-cut branch with both overflow flags false, so every row cuts to nothing and the diff pane goes blank with no « indicator to explain it — recoverable only by scrolling all the way back or switching files. Every nonzero write to scrollX now goes through setScrollX, bounded by the widest row the pane currently renders, so a runaway offset can no longer cut the whole document away. This does not promise every viewport shows content at every legal offset: a wide row further down the file owns the bound, so short rows on screen can still cut to empty. That per-line behavior lives in applyHorizontalScroll and is pinned as deliberate by TestModel_ApplyHorizontalScrollNoLeftIndicatorWhenScrolledPastContent. The bound reads the rendered document rather than the loaded one: collapsed mode hides removed lines, so a wide hidden removal must not widen it, and expanding a hunk reveals those lines again. Delete-only placeholders are measured from their own synthetic text. Per-line widths are cached at load, parallel to file.lines, so the bound costs an integer scan rather than re-measuring strings on every keypress. The walk uses a moving hunk index rather than hunkStartFor, which rescans the whole hunk slice per line. Layout changes that widen the pane lower the bound without a horizontal keypress of their own. All four — resize, tree hide, line-number and blame toggles — already render through syncViewportToCursor, so the clamp lives there; the two collapsed mutations render directly and clamp locally. Related to #334, which surfaced this while asking for mouse horizontal scroll. The mouse gesture is a separate question and stays open. --- app/ui/collapsed.go | 46 ++++++++++ app/ui/diffnav.go | 76 +++++++++++++++- app/ui/diffnav_test.go | 191 ++++++++++++++++++++++++++++++++++++++++- app/ui/loaders.go | 2 + app/ui/loaders_test.go | 15 ++++ app/ui/model.go | 1 + 6 files changed, 326 insertions(+), 5 deletions(-) diff --git a/app/ui/collapsed.go b/app/ui/collapsed.go index 2fa7b2cb..07791642 100644 --- a/app/ui/collapsed.go +++ b/app/ui/collapsed.go @@ -80,6 +80,50 @@ func (m Model) renderCollapsedDiff() string { return b.String() } +// maxRenderedContentWidth returns the widest display width among the rows the diff pane +// currently renders, measured on the same content applyHorizontalScroll cuts (change prefix +// plus tab-expanded text, gutters excluded). it walks the same visibility rules as +// renderCollapsedDiff above — keep the two in step, a row counted here but not rendered +// there lets the horizontal scroll bound exceed the visible document. +// returns 0 when no file is loaded. +func (m Model) maxRenderedContentWidth() int { + if len(m.file.lineWidths) == 0 { + return 0 + } + if !m.modes.collapsed.enabled { + maxW := 0 + for _, w := range m.file.lineWidths { + maxW = max(maxW, w) + } + return maxW + } + + hunks := m.findHunks() + maxW := 0 + hunkIdx := 0 + for i, dl := range m.file.lines { + // moving index rather than hunkStartFor: that helper rescans the whole hunk + // slice per line, making this walk O(lines*hunks). + for hunkIdx+1 < len(hunks) && hunks[hunkIdx+1] <= i { + hunkIdx++ + } + hunkStart := -1 + if (dl.ChangeType == diff.ChangeAdd || dl.ChangeType == diff.ChangeRemove) && len(hunks) > 0 && hunks[hunkIdx] <= i { + hunkStart = hunks[hunkIdx] + } + expanded := hunkStart >= 0 && m.modes.collapsed.expandedHunks[hunkStart] + + if dl.ChangeType == diff.ChangeRemove && !expanded { + if i == hunkStart && m.isDeleteOnlyHunk(hunkStart) { + maxW = max(maxW, changePrefixWidth+lipgloss.Width(m.deletePlaceholderText(hunkStart))) + } + continue // hidden in collapsed mode, contributes no width + } + maxW = max(maxW, m.file.lineWidths[i]) + } + return maxW +} + // renderCollapsedAddLine renders an add line in collapsed mode with modify or add styling. // when search is active, matching lines use search highlight instead of add/modify styling. func (m Model) renderCollapsedAddLine(b *strings.Builder, idx int, dl diff.DiffLine, modified bool) { @@ -369,6 +413,7 @@ func (m *Model) toggleCollapsedMode() { m.annot.cursorOnAnnotation = false // visible lines change, reset annotation cursor state m.adjustCursorIfHidden() m.realignSearchCursor() + m.clampHorizontalScroll() // entering collapsed mode hides wide removes, lowering the bound m.layout.viewport.SetContent(m.renderDiff()) } @@ -390,6 +435,7 @@ func (m *Model) toggleHunkExpansion() { } else { m.modes.collapsed.expandedHunks[hunkStart] = true } + m.clampHorizontalScroll() // re-collapsing a hunk re-hides its removes, lowering the bound m.layout.viewport.SetContent(m.renderDiff()) } diff --git a/app/ui/diffnav.go b/app/ui/diffnav.go index 870077ae..8da5bcb9 100644 --- a/app/ui/diffnav.go +++ b/app/ui/diffnav.go @@ -2,15 +2,25 @@ package ui import ( "slices" + "strings" tea "github.com/charmbracelet/bubbletea" + "github.com/charmbracelet/lipgloss" "github.com/umputun/revdiff/app/diff" "github.com/umputun/revdiff/app/keymap" "github.com/umputun/revdiff/app/ui/sidepane" ) -const scrollStep = 4 // horizontal scroll step in characters +const ( + scrollStep = 4 // horizontal scroll step in characters + + // changePrefixWidth is the display width of the add/remove/context marker + // linePrefix re-adds at render time (" + ", " - ", " "); dividerPrefixWidth + // is the single leading space renderDiffLine gives a divider row instead. + changePrefixWidth = 3 + dividerPrefixWidth = 1 +) // cursorDiffLine returns the DiffLine at the current cursor position, if valid. func (m Model) cursorDiffLine() (diff.DiffLine, bool) { @@ -278,6 +288,10 @@ func (m *Model) moveDiffCursorToEnd() { // the clamp accept a larger target after content grows (e.g. a freshly saved // multi-row annotation extending past the prior content end). func (m *Model) syncViewportToCursor() { + // every layout change that widens the diff pane — resize, tree hide, line-number or + // blame toggle — lands here before rendering, and each lowers the horizontal bound + // without a horizontal keypress of its own. + m.clampHorizontalScroll() cursorTop, cursorBottom := m.cursorVisualRange() m.layout.viewport.SetContent(m.renderDiff()) switch { @@ -646,6 +660,62 @@ func (m Model) handleHunkNav(forward bool) (tea.Model, tea.Cmd) { return m, nil } +// computeLineWidths returns the rendered display width of every diff line, parallel to +// file.lines. it measures what applyHorizontalScroll actually cuts: the change prefix plus +// the tab-expanded content, gutters excluded (those shrink the visible width instead). +// measured on plain Content rather than the highlighted copy — chroma adds only ANSI, which +// carries no display width and costs more to scan. +func (m Model) computeLineWidths() []int { + widths := make([]int, len(m.file.lines)) + for i, dl := range m.file.lines { + prefix := changePrefixWidth + if dl.ChangeType == diff.ChangeDivider { + prefix = dividerPrefixWidth + } + widths[i] = prefix + lipgloss.Width(strings.ReplaceAll(dl.Content, "\t", m.cfg.tabSpaces)) + } + return widths +} + +// maxHorizontalScroll returns the largest scrollX offset that still shows content, i.e. the +// widest rendered row minus the columns the pane can display. 0 when everything fits. +func (m Model) maxHorizontalScroll() int { + visible := m.diffContentWidth() - m.gutterExtra() + if visible <= 0 { + return 0 + } + return max(0, m.maxRenderedContentWidth()-visible) +} + +// ensureLineWidths repopulates the width cache when it has fallen out of step with file.lines. +// lineWidths is pure derived data, so a miss must cost a scan and never correctness — a caller +// that sets file.lines without recomputing would otherwise get a zero bound and no horizontal +// scroll at all. length equality cannot catch a same-length replacement, so production still +// owes the recompute in handleFileLoaded; this only keeps a miss from being silent. +func (m *Model) ensureLineWidths() { + if len(m.file.lineWidths) != len(m.file.lines) { + m.file.lineWidths = m.computeLineWidths() + } +} + +// setScrollX stores a horizontal offset bounded to the current rendered document. every +// nonzero write to layout.scrollX must go through here: an unbounded offset past the widest +// row makes applyHorizontalScroll cut past every line, blanking the pane with no « indicator +// to explain it. the two direct `scrollX = 0` assignments (file load, wrap enable) are safe +// as they are. +func (m *Model) setScrollX(x int) { + m.ensureLineWidths() + m.layout.scrollX = min(max(0, x), m.maxHorizontalScroll()) +} + +// clampHorizontalScroll re-applies the bound to the stored offset. widening the visible area +// — a terminal resize, hiding the tree, turning off line numbers or blame — lowers the +// maximum without any horizontal keypress, so the paths that do those call this before they +// render. +func (m *Model) clampHorizontalScroll() { + m.setScrollX(m.layout.scrollX) +} + // handleHorizontalScroll processes left/right scroll keys. // direction < 0 scrolls left, direction > 0 scrolls right. // no-op when wrap mode is active (content is already fully visible). @@ -654,9 +724,9 @@ func (m *Model) handleHorizontalScroll(direction int) { return } if direction < 0 { - m.layout.scrollX = max(0, m.layout.scrollX-scrollStep) + m.setScrollX(m.layout.scrollX - scrollStep) } else { - m.layout.scrollX += scrollStep + m.setScrollX(m.layout.scrollX + scrollStep) } m.layout.viewport.SetContent(m.renderDiff()) } diff --git a/app/ui/diffnav_test.go b/app/ui/diffnav_test.go index 9b34a6f4..ed5f128b 100644 --- a/app/ui/diffnav_test.go +++ b/app/ui/diffnav_test.go @@ -7,6 +7,8 @@ import ( "time" tea "github.com/charmbracelet/bubbletea" + "github.com/charmbracelet/lipgloss" + "github.com/charmbracelet/x/ansi" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -2314,11 +2316,13 @@ func TestModel_ScrollBlockedInWrapMode(t *testing.T) { assert.Equal(t, 0, model.layout.scrollX) } func TestModel_ScrollWorksWithoutWrapMode(t *testing.T) { - lines := []diff.DiffLine{{ChangeType: diff.ChangeContext, Content: "x", NewNum: 1}} + wide := strings.Repeat("x", 300) // must overflow the pane, or the clamp correctly refuses to scroll + lines := []diff.DiffLine{{ChangeType: diff.ChangeContext, Content: wide, NewNum: 1}} m := testModel([]string{"a.go"}, map[string][]diff.DiffLine{"a.go": lines}) m.file.name = "a.go" m.file.lines = lines - m.file.highlighted = []string{"x"} + m.file.highlighted = []string{wide} + m.layout.width = 80 m.layout.focus = paneDiff m.layout.viewport.Width = 80 m.layout.viewport.Height = 20 @@ -3506,3 +3510,186 @@ func TestModel_DownPageMotionTerminatesOnAnnotatedLastLine(t *testing.T) { }) } } + +// clampTestModel builds a diff-pane model with a hidden tree and a known content width, +// with lineWidths populated the way handleFileLoaded populates it. +func clampTestModel(t *testing.T, lines []diff.DiffLine, width int) Model { + t.Helper() + m := testModel([]string{"a.go"}, map[string][]diff.DiffLine{"a.go": lines}) + m.layout.width = width + m.layout.height = 24 + m.layout.treeHidden = true + m.layout.focus = paneDiff + m.layout.viewport.Width = width - 4 + m.layout.viewport.Height = 20 + m.file.name = "a.go" + m.file.lines = lines + m.file.highlighted = make([]string, len(lines)) + for i, dl := range lines { + m.file.highlighted[i] = dl.Content + } + m.file.lineWidths = m.computeLineWidths() + return m +} + +func TestModel_ComputeLineWidths(t *testing.T) { + tests := []struct { + name string + line diff.DiffLine + want int + }{ + {"context row carries the three-column prefix", diff.DiffLine{ChangeType: diff.ChangeContext, Content: "abc"}, 6}, + {"added row carries the three-column prefix", diff.DiffLine{ChangeType: diff.ChangeAdd, Content: "abcd"}, 7}, + {"removed row carries the three-column prefix", diff.DiffLine{ChangeType: diff.ChangeRemove, Content: "ab"}, 5}, + {"divider row carries a single leading space", diff.DiffLine{ChangeType: diff.ChangeDivider, Content: "abc"}, 4}, + {"wide runes count two columns each", diff.DiffLine{ChangeType: diff.ChangeContext, Content: "世界"}, 7}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + m := clampTestModel(t, []diff.DiffLine{tt.line}, 200) + assert.Equal(t, tt.want, m.file.lineWidths[0]) + }) + } +} + +func TestModel_ComputeLineWidthsExpandsTabs(t *testing.T) { + m := clampTestModel(t, []diff.DiffLine{{ChangeType: diff.ChangeContext, Content: "\tx"}}, 200) + assert.Equal(t, changePrefixWidth+len(m.cfg.tabSpaces)+1, m.file.lineWidths[0]) +} + +func TestModel_HorizontalScrollStopsWithContentVisible(t *testing.T) { + wide := strings.Repeat("x", 100) + "ENDTOKEN" + m := clampTestModel(t, []diff.DiffLine{{ChangeType: diff.ChangeContext, Content: wide, NewNum: 1}}, 40) + + for range 200 { + m.handleHorizontalScroll(1) + } + + assert.Equal(t, m.maxHorizontalScroll(), m.layout.scrollX, "scroll must stop at the bound") + stripped := ansi.Strip(m.renderDiff()) + assert.Contains(t, stripped, "ENDTOKEN", "the widest row's tail must stay visible at the bound") + assert.NotContains(t, stripped, "»", "nothing is left off the right edge at the bound") +} + +func TestModel_HorizontalScrollLeftClampsAtZero(t *testing.T) { + wide := strings.Repeat("x", 100) + m := clampTestModel(t, []diff.DiffLine{{ChangeType: diff.ChangeContext, Content: wide, NewNum: 1}}, 40) + + for range 50 { + m.handleHorizontalScroll(-1) + } + assert.Equal(t, 0, m.layout.scrollX) +} + +func TestModel_HorizontalScrollBoundIgnoresCollapsedHiddenLine(t *testing.T) { + lines := []diff.DiffLine{ + {ChangeType: diff.ChangeRemove, Content: strings.Repeat("r", 400), OldNum: 1}, + {ChangeType: diff.ChangeAdd, Content: "short add", NewNum: 1}, + } + m := clampTestModel(t, lines, 40) + + expandedBound := m.maxHorizontalScroll() + m.modes.collapsed.enabled = true + collapsedBound := m.maxHorizontalScroll() + + assert.Positive(t, expandedBound, "the wide removed line is rendered when not collapsed") + assert.Equal(t, 0, collapsedBound, "a hidden removed line must not widen the bound") +} + +func TestModel_HorizontalScrollBoundFollowsHunkExpansion(t *testing.T) { + lines := []diff.DiffLine{ + {ChangeType: diff.ChangeRemove, Content: strings.Repeat("r", 400), OldNum: 1}, + {ChangeType: diff.ChangeAdd, Content: "short add", NewNum: 1}, + } + m := clampTestModel(t, lines, 40) + m.modes.collapsed.enabled = true + + hidden := m.maxHorizontalScroll() + m.modes.collapsed.expandedHunks = map[int]bool{0: true} + expanded := m.maxHorizontalScroll() + m.modes.collapsed.expandedHunks = map[int]bool{} + rehidden := m.maxHorizontalScroll() + + assert.Equal(t, 0, hidden) + assert.Positive(t, expanded, "expanding the hunk reveals the wide removed line") + assert.Equal(t, 0, rehidden, "re-collapsing hides it again") +} + +func TestModel_HorizontalScrollBoundCountsDeletePlaceholder(t *testing.T) { + lines := []diff.DiffLine{ + {ChangeType: diff.ChangeRemove, Content: "gone", OldNum: 1}, + {ChangeType: diff.ChangeRemove, Content: "gone too", OldNum: 2}, + } + m := clampTestModel(t, lines, 20) + m.modes.collapsed.enabled = true + + want := changePrefixWidth + lipgloss.Width(m.deletePlaceholderText(0)) + assert.Equal(t, want, m.maxRenderedContentWidth()) +} + +func TestModel_HorizontalScrollReclampsWhenGutterShrinks(t *testing.T) { + wide := strings.Repeat("x", 100) + m := clampTestModel(t, []diff.DiffLine{{ChangeType: diff.ChangeContext, Content: wide, NewNum: 1}}, 40) + m.modes.lineNumbers = true + m.file.lineNumWidth = m.computeLineNumWidth() + + for range 200 { + m.handleHorizontalScroll(1) + } + withNumbers := m.layout.scrollX + + m.toggleLineNumbers() + assert.Less(t, m.layout.scrollX, withNumbers, "offset must follow the bound down") + assert.Equal(t, m.maxHorizontalScroll(), m.layout.scrollX) +} + +func TestModel_HorizontalScrollReclampsOnCollapsedToggle(t *testing.T) { + lines := []diff.DiffLine{ + {ChangeType: diff.ChangeRemove, Content: strings.Repeat("r", 400), OldNum: 1}, + {ChangeType: diff.ChangeAdd, Content: "short add", NewNum: 1}, + } + m := clampTestModel(t, lines, 40) + + for range 200 { + m.handleHorizontalScroll(1) + } + scrolled := m.layout.scrollX + require.Positive(t, scrolled) + + m.toggleCollapsedMode() + + assert.Equal(t, 0, m.layout.scrollX, "hiding the widest row must pull the offset back in") + assert.Contains(t, ansi.Strip(m.renderDiff()), "short add", "the pane must not blank") +} + +func TestModel_HorizontalScrollReclampsOnHunkRecollapse(t *testing.T) { + lines := []diff.DiffLine{ + {ChangeType: diff.ChangeRemove, Content: strings.Repeat("r", 400), OldNum: 1}, + {ChangeType: diff.ChangeAdd, Content: "short add", NewNum: 1}, + } + m := clampTestModel(t, lines, 40) + m.modes.collapsed.enabled = true + m.modes.collapsed.expandedHunks = map[int]bool{0: true} + m.nav.diffCursor = 0 + + for range 200 { + m.handleHorizontalScroll(1) + } + require.Positive(t, m.layout.scrollX) + + m.toggleHunkExpansion() + + assert.Equal(t, 0, m.layout.scrollX, "re-hiding the expanded row must pull the offset back in") + assert.Contains(t, ansi.Strip(m.renderDiff()), "short add", "the pane must not blank") +} + +func TestModel_HorizontalScrollHealsMissingWidthCache(t *testing.T) { + wide := strings.Repeat("x", 100) + m := clampTestModel(t, []diff.DiffLine{{ChangeType: diff.ChangeContext, Content: wide, NewNum: 1}}, 40) + m.file.lineWidths = nil + + m.handleHorizontalScroll(1) + + assert.Len(t, m.file.lineWidths, 1, "the cache is rebuilt rather than left empty") + assert.Equal(t, scrollStep, m.layout.scrollX, "a missing cache must not disable horizontal scroll") +} diff --git a/app/ui/loaders.go b/app/ui/loaders.go index 7d920956..566d9afc 100644 --- a/app/ui/loaders.go +++ b/app/ui/loaders.go @@ -476,6 +476,7 @@ func (m Model) handleFilesLoaded(msg filesLoadedMsg) (tea.Model, tea.Cmd) { m.file.oldName = "" m.file.lines = nil m.file.highlighted = nil + m.file.lineWidths = nil m.layout.viewport.SetContent("") return m, statsCmd } @@ -543,6 +544,7 @@ func (m Model) handleFileLoaded(msg fileLoadedMsg) (tea.Model, tea.Cmd) { m.computeFileStats() m.file.highlighted = m.highlighter.HighlightLines(msg.file, m.file.lines) m.recomputeIntraRanges() + m.file.lineWidths = m.computeLineWidths() if m.modes.lineNumbers { m.file.lineNumWidth = m.computeLineNumWidth() } diff --git a/app/ui/loaders_test.go b/app/ui/loaders_test.go index a58cf3c3..7abc2598 100644 --- a/app/ui/loaders_test.go +++ b/app/ui/loaders_test.go @@ -1838,6 +1838,21 @@ func TestModel_HandleFileLoaded_DropsStaleCompactAnchor(t *testing.T) { assert.Equal(t, 1, model.nav.diffCursor, "stale anchor must not reposition; cursor resets to first visible line") } +func TestModel_HandleFileLoaded_PopulatesLineWidths(t *testing.T) { + m := testModel([]string{"a.go"}, nil) + m.file.name = "a.go" + msg := fileLoadedMsg{file: "a.go", seq: 0, lines: []diff.DiffLine{ + {NewNum: 1, Content: "abc", ChangeType: diff.ChangeContext}, + {NewNum: 2, Content: "much longer line here", ChangeType: diff.ChangeAdd}, + }} + result, _ := m.handleFileLoaded(msg) + model := result.(Model) + + require.Len(t, model.file.lineWidths, 2, "widths must stay parallel to lines") + assert.Equal(t, changePrefixWidth+3, model.file.lineWidths[0]) + assert.Equal(t, changePrefixWidth+21, model.file.lineWidths[1]) +} + func TestModel_LoadFilesReconcilesReviewedFingerprints(t *testing.T) { entry := diff.FileEntry{Path: "a.go", Status: diff.FileModified} original := []diff.DiffLine{ diff --git a/app/ui/model.go b/app/ui/model.go index 06b51b2e..af4df85c 100644 --- a/app/ui/model.go +++ b/app/ui/model.go @@ -298,6 +298,7 @@ type loadedFileState struct { lines []diff.DiffLine // parsed diff lines highlighted []string // pre-computed highlighted content, parallel to lines intraRanges [][]worddiff.Range // per-line intra-line word-diff ranges, parallel to lines + lineWidths []int // per-line rendered display width, parallel to lines adds int // cached count of added lines removes int // cached count of removed lines blameData map[int]diff.BlameLine // blame info keyed by 1-based new line number