From 00937c9ef45038d0c5b7a6c81875410b8c7f05d1 Mon Sep 17 00:00:00 2001 From: Umputun Date: Wed, 19 Aug 2026 22:57:04 -0500 Subject: [PATCH 1/2] feat: add --start-at-change to open files on the first changed line By default the diff pane opens at line 1, so on a large file the reviewer scrolls to reach the change. Measured over this repo's last 60 commits, the median first changed line is 72 and about 60% of file opens show no changed line on a 40-row pane. The flag is opt-in and default off, matching how every comparable navigation knob ships here. It reuses the positioning the cross-file hunk jump already performs: applyPendingHunkJump's forward branch is extracted as positionOnFirstChange, so startup matches what next_hunk does on a fresh file and no new positioning rules are introduced. skipInitialDividers is left untouched because its other callers depend on its current meaning. The new branch in handleFileLoaded sits below the annotation-jump, hunk-jump and compact-anchor branches so an explicit target always wins, and returns early since the trailing GotoTop would otherwise undo the scroll. Files with no hunks fall back to the first visible line, so context-only sources are unaffected. In collapsed mode the cursor lands on a delete-only hunk's placeholder head rather than skipping it. Related to #325. --- .claude-plugin/skills/revdiff/SKILL.md | 2 + .../skills/revdiff/references/config.md | 1 + README.md | 1 + app/config.go | 1 + app/config_test.go | 26 ++++ app/main.go | 1 + app/ui/diffnav.go | 19 ++- app/ui/loaders.go | 11 +- app/ui/loaders_test.go | 146 ++++++++++++++++++ app/ui/model.go | 3 + plugins/codex/skills/revdiff/SKILL.md | 2 + .../codex/skills/revdiff/references/config.md | 1 + plugins/pi/skills/revdiff/SKILL.md | 2 + site/docs.html | 1 + 14 files changed, 210 insertions(+), 7 deletions(-) diff --git a/.claude-plugin/skills/revdiff/SKILL.md b/.claude-plugin/skills/revdiff/SKILL.md index 9e102b99..6a804606 100644 --- a/.claude-plugin/skills/revdiff/SKILL.md +++ b/.claude-plugin/skills/revdiff/SKILL.md @@ -122,6 +122,8 @@ When you are launching revdiff for the user (e.g., right after a refactor or ana **When the recent change likely created new untracked files** (new packages, new test files, new docs, new scripts that haven't been `git add`-ed yet), pass `--untracked` so those files appear in the tree. Use this in working-tree mode (no ref, no `--staged`); skip it for ref-to-ref reviews where untracked files are not part of the historical diff. +Pass `--start-at-change` only when the user explicitly asks for that cursor preference; never infer it automatically. + Run the launcher through the override-chain resolver: ```bash diff --git a/.claude-plugin/skills/revdiff/references/config.md b/.claude-plugin/skills/revdiff/references/config.md index 6f0c8f79..1cac0685 100644 --- a/.claude-plugin/skills/revdiff/references/config.md +++ b/.claude-plugin/skills/revdiff/references/config.md @@ -27,6 +27,7 @@ Then uncomment and edit the values you want to change. | `--wrap` | `REVDIFF_WRAP` | Enable line wrapping in diff view | `false` | | `--wrap-indent` | `REVDIFF_WRAP_INDENT` | Indent wrap continuation rows by N columns so they hang under the first row's content (helps when reviewing markdown lists where unindented continuation can be misread as a new bullet) | `0` | | `--page-overlap` | `REVDIFF_PAGE_OVERLAP` | Keep N lines from the previous screen when paging the diff | `0` | +| `--start-at-change` | `REVDIFF_START_AT_CHANGE` | Position the cursor on the first changed line | `false` | | `--collapsed` | `REVDIFF_COLLAPSED` | Start in collapsed diff mode | `false` | | `--compact` | `REVDIFF_COMPACT` | Start in compact diff mode (small context around changes) | `false` | | `--compact-context` | `REVDIFF_COMPACT_CONTEXT` | Number of context lines around changes when in compact mode | `5` | diff --git a/README.md b/README.md index 53e259da..5799d5b2 100644 --- a/README.md +++ b/README.md @@ -380,6 +380,7 @@ Positional arguments support several forms: | `--compact` | Start in compact diff mode (small context around changes), env: `REVDIFF_COMPACT` | `false` | | `--compact-context` | Number of context lines around changes when in compact mode, env: `REVDIFF_COMPACT_CONTEXT` | `5` | | `--cross-file-hunks` | Allow `[` and `]` to continue into adjacent files, env: `REVDIFF_CROSS_FILE_HUNKS` | `false` | +| `--start-at-change` | Position the cursor on the first changed line, env: `REVDIFF_START_AT_CHANGE` | `false` | | `--line-numbers` | Show line numbers in diff gutter, env: `REVDIFF_LINE_NUMBERS` | `false` | | `--blame` | Show blame gutter, env: `REVDIFF_BLAME` | `false` | | `--word-diff` | Highlight intra-line word-level changes in paired add/remove lines, env: `REVDIFF_WORD_DIFF` | `false` | diff --git a/app/config.go b/app/config.go index 9b2c5f8f..0be1e47d 100644 --- a/app/config.go +++ b/app/config.go @@ -34,6 +34,7 @@ type options struct { Compact bool `long:"compact" ini-name:"compact" env:"REVDIFF_COMPACT" description:"start in compact diff mode (small context around changes)"` CompactContext int `long:"compact-context" ini-name:"compact-context" env:"REVDIFF_COMPACT_CONTEXT" default:"5" description:"number of context lines around changes when in compact mode"` CrossFileHunks bool `long:"cross-file-hunks" ini-name:"cross-file-hunks" env:"REVDIFF_CROSS_FILE_HUNKS" description:"allow [ and ] to jump across file boundaries"` + StartAtChange bool `long:"start-at-change" ini-name:"start-at-change" env:"REVDIFF_START_AT_CHANGE" description:"position the cursor on the first changed line"` LineNumbers bool `long:"line-numbers" ini-name:"line-numbers" env:"REVDIFF_LINE_NUMBERS" description:"show line numbers in diff gutter"` Blame bool `long:"blame" ini-name:"blame" env:"REVDIFF_BLAME" description:"show blame gutter"` WordDiff bool `long:"word-diff" ini-name:"word-diff" env:"REVDIFF_WORD_DIFF" description:"highlight intra-line word-level changes in paired add/remove lines"` diff --git a/app/config_test.go b/app/config_test.go index ff39b5d5..8c2ad7e6 100644 --- a/app/config_test.go +++ b/app/config_test.go @@ -36,6 +36,7 @@ func TestParseArgs_Defaults(t *testing.T) { assert.False(t, opts.Compact) assert.Equal(t, 5, opts.CompactContext) assert.False(t, opts.CrossFileHunks) + assert.False(t, opts.StartAtChange) assert.False(t, opts.LineNumbers) assert.False(t, opts.Blame) assert.False(t, opts.ExitCodeOnAnnotations) @@ -371,6 +372,31 @@ func TestParseArgs_CrossFileHunks(t *testing.T) { }) } +func TestParseArgs_StartAtChange(t *testing.T) { + t.Run("flag", func(t *testing.T) { + opts, err := parseArgs(append(noConfigArgs(t), "--start-at-change")) + require.NoError(t, err) + assert.True(t, opts.StartAtChange) + }) + + t.Run("env", func(t *testing.T) { + t.Setenv("REVDIFF_START_AT_CHANGE", "true") + opts, err := parseArgs(noConfigArgs(t)) + require.NoError(t, err) + assert.True(t, opts.StartAtChange) + }) + + t.Run("config file", func(t *testing.T) { + cfgDir := t.TempDir() + cfgPath := filepath.Join(cfgDir, "config") + err := os.WriteFile(cfgPath, []byte("[Application Options]\nstart-at-change = true\n"), 0o600) + require.NoError(t, err) + opts, err := parseArgs([]string{"--config", cfgPath}) + require.NoError(t, err) + assert.True(t, opts.StartAtChange) + }) +} + func TestParseArgs_LineNumbers(t *testing.T) { t.Run("flag", func(t *testing.T) { opts, err := parseArgs(append(noConfigArgs(t), "--line-numbers")) diff --git a/app/main.go b/app/main.go index 007979f3..79813381 100644 --- a/app/main.go +++ b/app/main.go @@ -219,6 +219,7 @@ func run(opts options) (int, error) { Compact: opts.Compact, CompactContext: opts.CompactContext, CrossFileHunks: opts.CrossFileHunks, + StartAtChange: opts.StartAtChange, LineNumbers: opts.LineNumbers, ShowBlame: opts.Blame, ShowUntracked: opts.startupUntracked(), diff --git a/app/ui/diffnav.go b/app/ui/diffnav.go index fc308064..fa18b6fb 100644 --- a/app/ui/diffnav.go +++ b/app/ui/diffnav.go @@ -901,17 +901,24 @@ func (m *Model) syncTOCActiveSection() { } } +// positionOnFirstChange puts the cursor on the first changed line, falling back to the first visible +// line when the file carries no hunks at all (context-only sources). in collapsed mode it lands on +// the delete-only placeholder rather than skipping the hunk, since that head line stays visible. +// moveToNextHunk also scrolls, so callers that center afterwards render twice. +func (m *Model) positionOnFirstChange() { + m.nav.diffCursor = -1 + m.moveToNextHunk() + if m.nav.diffCursor == -1 { + m.skipInitialDividers() + } +} + // applyPendingHunkJump moves the cursor to the first or last hunk after a cross-file navigation. func (m *Model) applyPendingHunkJump() { forward := *m.nav.pendingHunkJump m.nav.pendingHunkJump = nil if forward { - m.nav.diffCursor = -1 - m.moveToNextHunk() - if m.nav.diffCursor != -1 { - return - } - m.skipInitialDividers() + m.positionOnFirstChange() return } diff --git a/app/ui/loaders.go b/app/ui/loaders.go index afe62cc6..7d920956 100644 --- a/app/ui/loaders.go +++ b/app/ui/loaders.go @@ -394,7 +394,8 @@ func (m Model) loadSelectedIfChanged() (tea.Model, tea.Cmd) { // overlay shows loading state (not stale data) while the re-fetch is in // flight, then re-runs the same parallel pipeline as startup via tea.Batch. // The selected file in the tree is restored by SelectByPath in -// handleFilesLoaded; the diff cursor resets to the top of the file. Named +// handleFilesLoaded; the diff cursor resets to the top of the file, or to its +// first change when start-at-change is enabled. Named // triggerReload (not reload) to avoid shadowing the Model.reload field. func (m *Model) triggerReload() tea.Cmd { m.filesLoadSeq++ @@ -604,6 +605,14 @@ func (m Model) handleFileLoaded(msg fileLoadedMsg) (tea.Model, tea.Cmd) { } } + // sits below the three jump branches so an explicit jump target always wins, and must return + // early because the GotoTop below would undo the scroll. + if m.cfg.startAtChange { + m.positionOnFirstChange() + m.centerViewportOnCursor() + return m, blameCmd + } + m.layout.viewport.SetContent(m.renderDiff()) m.layout.viewport.GotoTop() return m, blameCmd diff --git a/app/ui/loaders_test.go b/app/ui/loaders_test.go index 86f146eb..29a61a94 100644 --- a/app/ui/loaders_test.go +++ b/app/ui/loaders_test.go @@ -2083,3 +2083,149 @@ func TestModel_FilesReloadPreservesVisibleRowWhenFilesAboveChange(t *testing.T) require.True(t, m.tree.SelectByVisibleRow(2)) require.Equal(t, "f.go", m.tree.SelectedFile(), "reload should preserve the row when files above disappear") } + +func TestModel_HandleFileLoaded_StartAtChange(t *testing.T) { + withChange := []diff.DiffLine{ + {ChangeType: diff.ChangeDivider}, + {OldNum: 40, NewNum: 40, Content: "ctx", ChangeType: diff.ChangeContext}, + {OldNum: 41, NewNum: 41, Content: "ctx", ChangeType: diff.ChangeContext}, + {NewNum: 42, Content: "add", ChangeType: diff.ChangeAdd}, + {OldNum: 42, NewNum: 43, Content: "ctx", ChangeType: diff.ChangeContext}, + } + + load := func(t *testing.T, on bool, lines []diff.DiffLine, prep func(m *Model)) Model { + t.Helper() + m := testModel([]string{"a.go"}, nil) + m.file.name = "a.go" + m.cfg.startAtChange = on + if prep != nil { + prep(&m) + } + result, _ := m.handleFileLoaded(fileLoadedMsg{file: "a.go", seq: m.file.loadSeq, lines: lines}) + return result.(Model) + } + + t.Run("off keeps the first visible line", func(t *testing.T) { + assert.Equal(t, 1, load(t, false, withChange, nil).nav.diffCursor) + }) + + t.Run("on lands on the first changed line", func(t *testing.T) { + assert.Equal(t, 3, load(t, true, withChange, nil).nav.diffCursor) + }) + + t.Run("context-only file falls back to the first visible line", func(t *testing.T) { + contextOnly := []diff.DiffLine{ + {ChangeType: diff.ChangeDivider}, + {OldNum: 1, NewNum: 1, Content: "ctx", ChangeType: diff.ChangeContext}, + {OldNum: 2, NewNum: 2, Content: "ctx", ChangeType: diff.ChangeContext}, + } + assert.Equal(t, 1, load(t, true, contextOnly, nil).nav.diffCursor) + }) + + t.Run("empty diff leaves the cursor at zero", func(t *testing.T) { + assert.Equal(t, 0, load(t, true, nil, nil).nav.diffCursor) + }) + + t.Run("collapsed delete-only hunk keeps its placeholder", func(t *testing.T) { + deleteFirst := []diff.DiffLine{ + {OldNum: 1, NewNum: 1, Content: "ctx", ChangeType: diff.ChangeContext}, + {OldNum: 2, Content: "gone", ChangeType: diff.ChangeRemove}, + {OldNum: 3, NewNum: 2, Content: "ctx", ChangeType: diff.ChangeContext}, + {NewNum: 3, Content: "add", ChangeType: diff.ChangeAdd}, + } + m := load(t, true, deleteFirst, func(m *Model) { m.modes.collapsed.enabled = true }) + assert.Equal(t, 1, m.nav.diffCursor) + }) + + t.Run("pending hunk jump wins", func(t *testing.T) { + back := false + m := load(t, true, withChange, func(m *Model) { m.nav.pendingHunkJump = &back }) + assert.Equal(t, 3, m.nav.diffCursor) + assert.Nil(t, m.nav.pendingHunkJump) + }) + + t.Run("compact anchor wins", func(t *testing.T) { + m := load(t, true, withChange, func(m *Model) { + m.compact.pendingAnchor = &compactAnchor{seq: m.file.loadSeq, srcLine: 41, changeType: diff.ChangeContext, hunkIdx: 0} + }) + assert.Equal(t, 2, m.nav.diffCursor, "anchor restores the pre-toggle line, overriding start-at-change") + }) + + t.Run("reapplies on every subsequent file load", func(t *testing.T) { + m := load(t, true, withChange, nil) + require.Equal(t, 3, m.nav.diffCursor) + m.file.name = "b.go" + m.file.loadSeq++ + result, _ := m.handleFileLoaded(fileLoadedMsg{file: "b.go", seq: m.file.loadSeq, lines: withChange}) + assert.Equal(t, 3, result.(Model).nav.diffCursor, "switching files positions again, not once per session") + }) + + t.Run("annotation jump wins", func(t *testing.T) { + m := load(t, true, withChange, func(m *Model) { + m.pendingAnnotJump = &annotation.Annotation{File: "a.go", Line: 41, Type: string(diff.ChangeContext)} + }) + assert.Equal(t, 2, m.nav.diffCursor, "annotation target overrides start-at-change") + assert.Nil(t, m.pendingAnnotJump) + }) + + // fileLoadedMsg can arrive before the first WindowSizeMsg; the change must still be on + // screen once the resize lands, not merely selected somewhere far below the fold + t.Run("change is visible when the window size arrives after the load", func(t *testing.T) { + lines := make([]diff.DiffLine, 0, 401) + for i := 1; i <= 400; i++ { + lines = append(lines, diff.DiffLine{OldNum: i, NewNum: i, Content: "ctx", ChangeType: diff.ChangeContext}) + } + lines = append(lines, diff.DiffLine{NewNum: 401, Content: "add", ChangeType: diff.ChangeAdd}) + + m := testModel([]string{"a.go"}, nil) + m.file.name = "a.go" + m.cfg.startAtChange = true + m.ready = false + m.layout.viewport.Height = 0 + + result, _ := m.handleFileLoaded(fileLoadedMsg{file: "a.go", seq: m.file.loadSeq, lines: lines}) + m = result.(Model) + require.Equal(t, 400, m.nav.diffCursor, "cursor lands on the change with no viewport height yet") + + result, _ = m.Update(tea.WindowSizeMsg{Width: 120, Height: 40}) + m = result.(Model) + + cursorY, top, height := m.cursorViewportY(), m.layout.viewport.YOffset, m.layout.viewport.Height + require.Positive(t, height) + assert.GreaterOrEqual(t, cursorY, top, "change must not sit above the visible window") + assert.Less(t, cursorY, top+height, "change must not sit below the visible window") + }) +} + +// a markdown TOC is built only for full-context files, which by definition carry no hunks, so +// start-at-change cannot move the cursor out from under the TOC's active section. +func TestModel_StartAtChange_MarkdownTOCUnaffected(t *testing.T) { + load := func(t *testing.T, lines []diff.DiffLine) Model { + t.Helper() + m := testModel([]string{"a.md"}, nil) + m.file.name = "a.md" + m.file.singleFile = true + m.cfg.startAtChange = true + result, _ := m.handleFileLoaded(fileLoadedMsg{file: "a.md", seq: m.file.loadSeq, lines: lines}) + return result.(Model) + } + + t.Run("full-context markdown keeps its TOC and its top cursor", func(t *testing.T) { + m := load(t, []diff.DiffLine{ + {OldNum: 1, NewNum: 1, Content: "# One", ChangeType: diff.ChangeContext}, + {OldNum: 2, NewNum: 2, Content: "body", ChangeType: diff.ChangeContext}, + {OldNum: 3, NewNum: 3, Content: "## Two", ChangeType: diff.ChangeContext}, + }) + require.NotNil(t, m.file.mdTOC, "full-context markdown must still build a TOC") + assert.Equal(t, 0, m.nav.diffCursor, "no hunks means the fallback keeps the top position") + }) + + t.Run("changed markdown builds no TOC", func(t *testing.T) { + m := load(t, []diff.DiffLine{ + {OldNum: 1, NewNum: 1, Content: "# One", ChangeType: diff.ChangeContext}, + {NewNum: 2, Content: "## Two", ChangeType: diff.ChangeAdd}, + }) + assert.Nil(t, m.file.mdTOC, "a diff with changes is not full-context, so no TOC exists to go stale") + assert.Equal(t, 1, m.nav.diffCursor) + }) +} diff --git a/app/ui/model.go b/app/ui/model.go index d168c7aa..06b51b2e 100644 --- a/app/ui/model.go +++ b/app/ui/model.go @@ -326,6 +326,7 @@ type modelConfigState struct { noConfirmDiscard bool // skip confirmation prompt on discard quit noConfirmReload bool // skip confirmation prompt on reload (R) crossFileHunks bool // allow [ and ] to jump across file boundaries + startAtChange bool // put the cursor on the first changed line when a file loads treeWidthRatio int // 1-10 units for file tree panel tabSpaces string // spaces to replace tabs with wrapIndent int // extra indent (in columns) for wrap continuation rows; 0 disables @@ -747,6 +748,7 @@ type ModelConfig struct { PageOverlap int // rows carried over from the previous screen on page up/down; 0 disables Collapsed bool // start in collapsed diff mode CrossFileHunks bool // allow [ and ] to jump across file boundaries + StartAtChange bool // put the cursor on the first changed line when a file loads LineNumbers bool // show line numbers in diff gutter ShowBlame bool // show blame gutter; requires Blamer ShowUntracked bool // show untracked files in the tree; requires LoadUntracked @@ -903,6 +905,7 @@ func NewModel(cfg ModelConfig) (Model, error) { noConfirmDiscard: cfg.NoConfirmDiscard, noConfirmReload: cfg.NoConfirmReload, crossFileHunks: cfg.CrossFileHunks, + startAtChange: cfg.StartAtChange, treeWidthRatio: cfg.TreeWidthRatio, tabSpaces: strings.Repeat(" ", cfg.TabWidth), wrapIndent: max(0, cfg.WrapIndent), diff --git a/plugins/codex/skills/revdiff/SKILL.md b/plugins/codex/skills/revdiff/SKILL.md index 8401b126..cd79601d 100644 --- a/plugins/codex/skills/revdiff/SKILL.md +++ b/plugins/codex/skills/revdiff/SKILL.md @@ -138,6 +138,8 @@ When you are launching revdiff for the user (e.g., right after a refactor or ana **When the recent change likely created new untracked files** (new packages, new test files, new docs, new scripts that haven't been `git add`-ed yet), pass `--untracked` so those files appear in the tree. Use this in working-tree mode (no ref, no `--staged`); skip it for ref-to-ref reviews where untracked files are not part of the historical diff. +Pass `--start-at-change` only when the user explicitly asks for that cursor preference; never infer it automatically. + Run the launcher script: ```bash diff --git a/plugins/codex/skills/revdiff/references/config.md b/plugins/codex/skills/revdiff/references/config.md index a9779fd8..48f1131a 100644 --- a/plugins/codex/skills/revdiff/references/config.md +++ b/plugins/codex/skills/revdiff/references/config.md @@ -27,6 +27,7 @@ Then uncomment and edit the values you want to change. | `--wrap` | `REVDIFF_WRAP` | Enable line wrapping in diff view | `false` | | `--wrap-indent` | `REVDIFF_WRAP_INDENT` | Indent wrap continuation rows by N columns so they hang under the first row's content (helps when reviewing markdown lists where unindented continuation can be misread as a new bullet) | `0` | | `--page-overlap` | `REVDIFF_PAGE_OVERLAP` | Keep N lines from the previous screen when paging the diff | `0` | +| `--start-at-change` | `REVDIFF_START_AT_CHANGE` | Position the cursor on the first changed line | `false` | | `--collapsed` | `REVDIFF_COLLAPSED` | Start in collapsed diff mode | `false` | | `--compact` | `REVDIFF_COMPACT` | Start in compact diff mode (small context around changes) | `false` | | `--compact-context` | `REVDIFF_COMPACT_CONTEXT` | Number of context lines around changes when in compact mode | `5` | diff --git a/plugins/pi/skills/revdiff/SKILL.md b/plugins/pi/skills/revdiff/SKILL.md index 3ca6c504..25a8c373 100644 --- a/plugins/pi/skills/revdiff/SKILL.md +++ b/plugins/pi/skills/revdiff/SKILL.md @@ -36,6 +36,7 @@ Tool examples: - `args: "--all-files --exclude vendor"`: review all tracked files except vendor - `args: "--no-tree"`: review with the file tree pane hidden - `args: "--page-overlap=2"`: keep 2 lines from the previous screen when paging +- `args: "--start-at-change"`: position the cursor on the first changed line - `args: "--description='why this refactor matters' main"`: include review context in the info popup - `args: "--description-file=/tmp/revdiff-desc.md main"`: include longer markdown review context - `args: "--annotations=/tmp/revdiff-review.md main"`: preload in-session review notes @@ -79,6 +80,7 @@ When annotations arrive from `/revdiff` or `revdiff_review`: /revdiff --only README.md /revdiff --no-tree /revdiff --page-overlap=2 +/revdiff --start-at-change /revdiff HEAD~3 --description="why this refactor matters" /revdiff HEAD~3 --description-file=/tmp/revdiff-desc.md /revdiff main --annotations=/tmp/revdiff-review.md diff --git a/site/docs.html b/site/docs.html index 1a64094c..425c77be 100644 --- a/site/docs.html +++ b/site/docs.html @@ -406,6 +406,7 @@

Options

--compactStart in compact diff mode (small context around changes)false --compact-contextNumber of context lines around changes when in compact mode5 --cross-file-hunksAllow [ and ] to continue into adjacent filesfalse + --start-at-changePosition the cursor on the first changed linefalse --line-numbersShow line numbers in diff gutterfalse --word-diffHighlight intra-line word-level changes in paired add/remove linesfalse --annotation-markerPrefix shown before annotation lines💬 From e015975b606b4a1897ee8324f6eb6319f79c5973 Mon Sep 17 00:00:00 2001 From: Umputun Date: Thu, 20 Aug 2026 01:06:08 -0500 Subject: [PATCH 2/2] fix: document why the follow-up center is required, and pin it with a test The godoc on positionOnFirstChange described the caller's centerViewportOnCursor as a duplicate render, which invited deleting it. That call is load-bearing: centerHunkInViewport sets the offset before rendering, so the offset clamps against the previously loaded file's length, and on the no-hunk path nothing renders at all. TestModel_StartAtChange_RendersTheLoadedFile pins both halves. Removing the call makes a context-only load paint a blank pane, and makes a 400-line jump clamp to offset 0 against a one-line previous file, leaving the change far below the fold. The existing subtests could not catch either, since they assert only the cursor index against a zero-height viewport that renders nothing. Related to #325. --- app/ui/diffnav.go | 6 ++++- app/ui/loaders_test.go | 60 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 65 insertions(+), 1 deletion(-) diff --git a/app/ui/diffnav.go b/app/ui/diffnav.go index fa18b6fb..870077ae 100644 --- a/app/ui/diffnav.go +++ b/app/ui/diffnav.go @@ -904,7 +904,11 @@ func (m *Model) syncTOCActiveSection() { // positionOnFirstChange puts the cursor on the first changed line, falling back to the first visible // line when the file carries no hunks at all (context-only sources). in collapsed mode it lands on // the delete-only placeholder rather than skipping the hunk, since that head line stays visible. -// moveToNextHunk also scrolls, so callers that center afterwards render twice. +// +// this only positions the cursor: a caller loading a new file MUST follow it with +// centerViewportOnCursor and must not drop that call as a duplicate render. moveToNextHunk scrolls +// via centerHunkInViewport, which sets the offset before rendering, so the offset clamps against the +// previously loaded file's length; on the no-hunk path nothing renders at all. func (m *Model) positionOnFirstChange() { m.nav.diffCursor = -1 m.moveToNextHunk() diff --git a/app/ui/loaders_test.go b/app/ui/loaders_test.go index 29a61a94..a58cf3c3 100644 --- a/app/ui/loaders_test.go +++ b/app/ui/loaders_test.go @@ -2197,6 +2197,66 @@ func TestModel_HandleFileLoaded_StartAtChange(t *testing.T) { }) } +// pins the centerViewportOnCursor call in the start-at-change branch of handleFileLoaded. it is the +// only render on the no-hunk path, and on the hunk path it is what re-applies an offset that +// centerHunkInViewport clamped against the previous file's length. deleting it as a duplicate render +// leaves the pane painting the old file. +func TestModel_StartAtChange_RendersTheLoadedFile(t *testing.T) { + short := []diff.DiffLine{{OldNum: 1, NewNum: 1, Content: "short file", ChangeType: diff.ChangeContext}} + + // testModel leaves the viewport zero-sized, and a zero-height viewport renders "" — which would + // make every assertion below pass against an unrendered pane + newModel := func(t *testing.T, files []string) Model { + t.Helper() + m := testModel(files, nil) + m.cfg.startAtChange = true + m.layout.viewport.Width, m.layout.viewport.Height = 80, 20 + return m + } + + load := func(t *testing.T, m Model, file string, lines []diff.DiffLine) Model { + t.Helper() + m.file.name = file + m.file.loadSeq++ + result, _ := m.handleFileLoaded(fileLoadedMsg{file: file, seq: m.file.loadSeq, lines: lines}) + return result.(Model) + } + + t.Run("context-only file replaces the previous file's content", func(t *testing.T) { + m := newModel(t, []string{"a.go", "b.md"}) + m = load(t, m, "a.go", []diff.DiffLine{ + {OldNum: 1, NewNum: 1, Content: "first file only", ChangeType: diff.ChangeContext}, + }) + + m = load(t, m, "b.md", []diff.DiffLine{ + {OldNum: 1, NewNum: 1, Content: "second file only", ChangeType: diff.ChangeContext}, + }) + + painted := m.layout.viewport.View() + assert.Contains(t, painted, "second file only", "the pane must paint the file that just loaded") + assert.NotContains(t, painted, "first file only", "stale content from the previous file must be gone") + }) + + t.Run("offset survives a short previous file", func(t *testing.T) { + lines := make([]diff.DiffLine, 0, 401) + for i := 1; i <= 400; i++ { + lines = append(lines, diff.DiffLine{OldNum: i, NewNum: i, Content: "ctx", ChangeType: diff.ChangeContext}) + } + lines = append(lines, diff.DiffLine{NewNum: 401, Content: "the change", ChangeType: diff.ChangeAdd}) + + m := newModel(t, []string{"a.go", "b.go"}) + m = load(t, m, "a.go", short) + require.Zero(t, m.layout.viewport.YOffset, "the short file leaves the viewport at the top") + + m = load(t, m, "b.go", lines) + + require.Equal(t, 400, m.nav.diffCursor) + assert.Positive(t, m.layout.viewport.YOffset, + "offset must be re-applied after the new content is installed, not clamped against the short file") + assert.Contains(t, m.layout.viewport.View(), "the change", "the change must actually be on screen") + }) +} + // a markdown TOC is built only for full-context files, which by definition carry no hunks, so // start-at-change cannot move the cursor out from under the TOC's active section. func TestModel_StartAtChange_MarkdownTOCUnaffected(t *testing.T) {