From 2b18eefd183cd209a5825602d767aea69b94df0c Mon Sep 17 00:00:00 2001 From: Umputun Date: Fri, 1 May 2026 02:00:51 -0500 Subject: [PATCH 1/4] feat: add cross-file annotation navigation (}/{) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds }/{ keybindings to jump to next/previous annotation across files in a single flat list (alphabetical files, file-level first per file, then ascending lines — matches the @ popup order). Always crosses file boundaries; silent no-op at the first/last annotation. Reuses the existing jumpToAnnotationTarget machinery (collapsed-hunk auto-expansion + cross-file pendingAnnotJump handoff) by exposing it through tryJumpToAnnotationTarget which returns a jumped-bool. The walker retries on ok=false so non-jumpable targets (path filtered out of the tree, line missing from the loaded diff in compact mode) cannot trap the user on a hidden annotation. Loop is bounded by len(flat). The flat list is rebuilt every keypress so additions and deletions take effect without explicit invalidation. ChangeDivider rows collapse to a synthetic line=-1 cursor key so file-level annotations stay reachable from a leading divider. Docs synced: README.md, site/docs.html, docs/ARCHITECTURE.md, both .claude-plugin and plugins/codex reference usage.md copies. --- .../skills/revdiff/references/usage.md | 1 + README.md | 3 +- app/annotations_load.go | 1 - app/keymap/keymap.go | 7 + app/keymap/keymap_test.go | 1 + app/ui/annotlist.go | 46 +- app/ui/annotnav.go | 166 +++++ app/ui/annotnav_test.go | 664 ++++++++++++++++++ app/ui/model.go | 2 + docs/ARCHITECTURE.md | 3 +- .../codex/skills/revdiff/references/usage.md | 1 + site/docs.html | 3 +- 12 files changed, 883 insertions(+), 15 deletions(-) create mode 100644 app/ui/annotnav.go create mode 100644 app/ui/annotnav_test.go diff --git a/.claude-plugin/skills/revdiff/references/usage.md b/.claude-plugin/skills/revdiff/references/usage.md index 84e102d6..7bf3f623 100644 --- a/.claude-plugin/skills/revdiff/references/usage.md +++ b/.claude-plugin/skills/revdiff/references/usage.md @@ -124,6 +124,7 @@ Use `--stdin` to review arbitrary piped or redirected text as one synthetic file | `a` or `Enter` (diff pane) | Annotate current diff line | | `A` | Add file-level annotation (stored at top of diff) | | `@` | Toggle annotation list popup (navigate and jump to any annotation) | +| `}` / `{` | Jump to next/previous annotation (always crosses file boundaries; silent no-op at the first/last annotation) | | `d` | Delete annotation under cursor | | `Ctrl+E` (during annotation input) | Open `$EDITOR` for multi-line annotation | | `Esc` | Cancel annotation input | diff --git a/README.md b/README.md index ea5f66ca..1f9fdeeb 100644 --- a/README.md +++ b/README.md @@ -636,6 +636,7 @@ In the Claude Code and Codex plugins, you can also tell the agent to use a past | `a` or `Enter` (diff pane) | Annotate current diff line | | `A` | Add file-level annotation (stored at top of diff) | | `@` | Toggle annotation list popup (navigate and jump to any annotation) | +| `}` / `{` | Jump to next/previous annotation (always crosses file boundaries; silent no-op at the first/last annotation) | | `d` | Delete annotation under cursor | | `Ctrl+E` (during annotation input) | Open `$EDITOR` for multi-line annotation | | `Esc` | Cancel annotation input | @@ -755,7 +756,7 @@ When the leader is pressed, the status bar shows `Pending: ctrl+w, esc to cancel **Search:** `search` -**Annotations:** `confirm` (annotate line / select file), `annotate_file`, `delete_annotation`, `annot_list` +**Annotations:** `confirm` (annotate line / select file), `annotate_file`, `delete_annotation`, `annot_list`, `next_annotation`, `prev_annotation` **View:** `toggle_collapsed`, `toggle_compact`, `toggle_wrap`, `toggle_tree`, `toggle_line_numbers`, `toggle_blame`, `toggle_word_diff`, `toggle_hunk`, `toggle_untracked`, `mark_reviewed`, `theme_select`, `filter`, `info`, `reload` diff --git a/app/annotations_load.go b/app/annotations_load.go index 649ab911..114978f1 100644 --- a/app/annotations_load.go +++ b/app/annotations_load.go @@ -245,4 +245,3 @@ func buildLineSet(lines []diff.DiffLine) map[lineKey]struct{} { } return out } - diff --git a/app/keymap/keymap.go b/app/keymap/keymap.go index 74323d6e..7cb174da 100644 --- a/app/keymap/keymap.go +++ b/app/keymap/keymap.go @@ -44,6 +44,8 @@ const ( ActionAnnotateFile Action = "annotate_file" ActionDeleteAnnotation Action = "delete_annotation" ActionAnnotList Action = "annot_list" + ActionNextAnnotation Action = "next_annotation" + ActionPrevAnnotation Action = "prev_annotation" ActionToggleCollapsed Action = "toggle_collapsed" ActionToggleCompact Action = "toggle_compact" ActionToggleWrap Action = "toggle_wrap" @@ -77,6 +79,7 @@ var validActions = map[Action]bool{ ActionTogglePane: true, ActionFocusTree: true, ActionFocusDiff: true, ActionSearch: true, ActionConfirm: true, ActionAnnotateFile: true, ActionDeleteAnnotation: true, ActionAnnotList: true, + ActionNextAnnotation: true, ActionPrevAnnotation: true, ActionToggleCollapsed: true, ActionToggleCompact: true, ActionToggleWrap: true, ActionToggleTree: true, ActionToggleLineNums: true, ActionToggleBlame: true, ActionToggleWordDiff: true, ActionToggleHunk: true, ActionMarkReviewed: true, ActionFilter: true, ActionToggleUntracked: true, @@ -204,6 +207,8 @@ func defaultDescriptions() []HelpEntry { {ActionAnnotateFile, "annotate file", "Annotations"}, {ActionDeleteAnnotation, "delete annotation", "Annotations"}, {ActionAnnotList, "annotation list", "Annotations"}, + {ActionNextAnnotation, "next annotation (across files)", "Annotations"}, + {ActionPrevAnnotation, "previous annotation (across files)", "Annotations"}, // view toggles {ActionToggleCollapsed, "toggle collapsed view", "View"}, @@ -258,6 +263,8 @@ func defaultBindings() map[string]Action { "A": ActionAnnotateFile, "d": ActionDeleteAnnotation, "@": ActionAnnotList, + "}": ActionNextAnnotation, + "{": ActionPrevAnnotation, "v": ActionToggleCollapsed, "C": ActionToggleCompact, "w": ActionToggleWrap, diff --git a/app/keymap/keymap_test.go b/app/keymap/keymap_test.go index 051ef621..55a80136 100644 --- a/app/keymap/keymap_test.go +++ b/app/keymap/keymap_test.go @@ -35,6 +35,7 @@ func TestDefault_allExpectedBindings(t *testing.T) { {"/", ActionSearch}, {"a", ActionConfirm}, {"enter", ActionConfirm}, {"A", ActionAnnotateFile}, {"d", ActionDeleteAnnotation}, {"@", ActionAnnotList}, + {"}", ActionNextAnnotation}, {"{", ActionPrevAnnotation}, {"v", ActionToggleCollapsed}, {"C", ActionToggleCompact}, {"w", ActionToggleWrap}, {"t", ActionToggleTree}, {"L", ActionToggleLineNums}, {"B", ActionToggleBlame}, {"W", ActionToggleWordDiff}, {".", ActionToggleHunk}, {" ", ActionMarkReviewed}, {"f", ActionFilter}, diff --git a/app/ui/annotlist.go b/app/ui/annotlist.go index fbbaaa43..26ae2425 100644 --- a/app/ui/annotlist.go +++ b/app/ui/annotlist.go @@ -8,7 +8,12 @@ import ( ) // buildAnnotListItems builds a flat list of all annotations across all files. -// items are ordered by file name then line number, as returned by the store. +// Items are ordered by file name then line number, as returned by the store +// (alphabetical via Store.Files, line-ascending via Store.Get with file-level +// Line=0 first within each file). This combined ordering is load-bearing: both +// the @ popup and the }/{ walker in annotnav.go iterate this exact sequence, +// so changes to Store.Files / Store.Get ordering must keep the two consumers +// in sync. func (m *Model) buildAnnotListItems() []annotation.Annotation { files := m.store.Files() items := make([]annotation.Annotation, 0, m.store.Count()) @@ -31,26 +36,45 @@ func (m Model) buildAnnotListSpec() overlay.AnnotListSpec { return overlay.AnnotListSpec{Items: items} } -// jumpToAnnotationTarget jumps to an annotation target returned by the overlay manager. +// jumpToAnnotationTarget jumps to an annotation target returned by the overlay +// manager. Existing entry point used by the @ popup and the mouse handler; +// preserves the original silent-fail-on-unreachable behavior. func (m Model) jumpToAnnotationTarget(target *overlay.AnnotationTarget) (tea.Model, tea.Cmd) { + model, cmd, _ := m.tryJumpToAnnotationTarget(target) + return model, cmd +} + +// tryJumpToAnnotationTarget attempts to jump and reports whether the jump +// was actually issued. ok=false is returned when the target cannot be reached: +// cross-file path is not present in the file tree (filtered, hidden by +// --include/--exclude, or untracked-with-toggle-off), or same-file +// non-file-level line is not in the loaded diff (e.g. compact mode shrank +// context away). Used by the }/{ navigator to skip non-jumpable targets and +// keep walking instead of getting trapped on a target that silently no-ops. +// Single-file mode always rejects cross-file targets — there is nowhere to go. +func (m Model) tryJumpToAnnotationTarget(target *overlay.AnnotationTarget) (tea.Model, tea.Cmd, bool) { if target == nil { - return m, nil + return m, nil, false } a := annotation.Annotation{File: target.File, Line: target.Line, Type: target.ChangeType} if a.File == m.file.name { + if a.Line != 0 && m.findDiffLineIndex(a.Line, a.Type) < 0 { + return m, nil, false + } m.positionOnAnnotation(a) - return m, nil + return m, nil, true } - m.pendingAnnotJump = &a - if !m.file.singleFile { - if !m.tree.SelectByPath(a.File) { - m.pendingAnnotJump = nil - return m, nil - } + if m.file.singleFile { + return m, nil, false + } + if !m.tree.SelectByPath(a.File) { + return m, nil, false } - return m.loadSelectedIfChanged() + m.pendingAnnotJump = &a + model, cmd := m.loadSelectedIfChanged() + return model, cmd, true } // positionOnAnnotation moves the cursor to the given annotation's line, re-renders, and centers the viewport. diff --git a/app/ui/annotnav.go b/app/ui/annotnav.go new file mode 100644 index 00000000..49c8985e --- /dev/null +++ b/app/ui/annotnav.go @@ -0,0 +1,166 @@ +package ui + +import ( + tea "github.com/charmbracelet/bubbletea" + + "github.com/umputun/revdiff/app/annotation" + "github.com/umputun/revdiff/app/diff" + "github.com/umputun/revdiff/app/ui/overlay" +) + +// handleAnnotNav jumps to the next or previous annotation in the flat +// cross-file list (alphabetical files, file-level first per file, then +// ascending lines — same order as the @ popup). At the boundaries the +// action is a silent no-op. Cross-file handoff goes through the existing +// tryJumpToAnnotationTarget machinery, so collapsed-hunk expansion, TOC +// sync, and pendingAnnotJump-based file load all come for free. +// +// When a target is in the store but not currently displayable (cross-file +// path filtered out of the tree, or same-file line missing from the loaded +// diff in compact mode), the walker advances past it and retries the next +// candidate in the same direction. Without that loop the cursor would not +// move on a silent-fail target and every subsequent }/{ press would +// recompute the same hidden target, trapping the user. The loop is bounded +// by the flat list length so a faulty store can never spin forever. +func (m Model) handleAnnotNav(forward bool) (tea.Model, tea.Cmd) { + flat := m.buildAnnotListItems() + if len(flat) == 0 { + return m, nil + } + cur := m.currentAnnotKey() + for range flat { + target, ok := pickAdjacentAnnotation(flat, cur, forward) + if !ok { + return m, nil + } + nextModel, cmd, jumped := m.tryJumpToAnnotationTarget(&overlay.AnnotationTarget{ + File: target.File, + ChangeType: target.Type, + Line: target.Line, + }) + if jumped { + return nextModel, cmd + } + cur = cursorAnnotKey{file: target.File, line: target.Line, typ: target.Type, onAnnot: true} + } + return m, nil +} + +// cursorAnnotKey is the cursor's position in annotation space. onAnnot is +// true when the cursor is exactly on an existing annotation row (file, +// line, AND type all match). In that case navigation steps by index in the +// flat list. Otherwise navigation uses an insertion-point fallback. +type cursorAnnotKey struct { + file string + line int + typ string + onAnnot bool +} + +// currentAnnotKey returns the cursor's annotation-space key. The file-level +// annotation row (diffCursor == -1) maps to (file, 0, "") which matches +// the storage form of file-level annotations. ChangeDivider rows (which +// carry OldNum=NewNum=0) and out-of-range cursors collapse to line=-1 so +// forward navigation correctly treats any same-file annotation — including +// a file-level Line=0 — as strictly after the cursor. +func (m Model) currentAnnotKey() cursorAnnotKey { + file := m.file.name + if m.nav.diffCursor == -1 { + return cursorAnnotKey{file: file, line: 0, typ: "", onAnnot: m.hasFileAnnotation()} + } + if m.nav.diffCursor < 0 || m.nav.diffCursor >= len(m.file.lines) { + return cursorAnnotKey{file: file, line: -1, typ: "", onAnnot: false} + } + dl := m.file.lines[m.nav.diffCursor] + if dl.ChangeType == diff.ChangeDivider { + return cursorAnnotKey{file: file, line: -1, typ: "", onAnnot: false} + } + line := m.diffLineNum(dl) + typ := string(dl.ChangeType) + return cursorAnnotKey{file: file, line: line, typ: typ, onAnnot: m.store.Has(file, line, typ)} +} + +// pickAdjacentAnnotation walks the flat list and selects the neighbor of +// the cursor. When the cursor is exactly on an annotation, steps by index +// in the list (so same-line different-type annotations both step in turn). +// When the cursor sits between annotations, uses an insertion-point +// fallback over the (file, line) order. +func pickAdjacentAnnotation(flat []annotation.Annotation, cur cursorAnnotKey, forward bool) (annotation.Annotation, bool) { + if idx, ok := exactAnnotIndex(flat, cur); ok { + return stepFlatList(flat, idx, forward) + } + insIdx := annotInsertionPoint(flat, cur) + if forward { + if insIdx >= len(flat) { + return annotation.Annotation{}, false + } + return flat[insIdx], true + } + if insIdx == 0 { + return annotation.Annotation{}, false + } + return flat[insIdx-1], true +} + +// exactAnnotIndex returns the flat-list index of an annotation that exactly +// matches the cursor (file, line, type). When cur.onAnnot is false it +// short-circuits without scanning. ok=false means "use insertion-point +// fallback instead." +func exactAnnotIndex(flat []annotation.Annotation, cur cursorAnnotKey) (int, bool) { + if !cur.onAnnot { + return 0, false + } + for i, a := range flat { + if a.File == cur.file && a.Line == cur.line && a.Type == cur.typ { + return i, true + } + } + return 0, false +} + +// stepFlatList returns flat[idx+1] for forward and flat[idx-1] for backward, +// with ok=false at the boundaries. +func stepFlatList(flat []annotation.Annotation, idx int, forward bool) (annotation.Annotation, bool) { + if forward { + if idx+1 < len(flat) { + return flat[idx+1], true + } + return annotation.Annotation{}, false + } + if idx > 0 { + return flat[idx-1], true + } + return annotation.Annotation{}, false +} + +// annotInsertionPoint returns the index where the cursor would be inserted +// in the flat list under (file, line) ordering — i.e. the index of the +// first annotation strictly after the cursor, or len(flat) if all entries +// are at or before the cursor. +func annotInsertionPoint(flat []annotation.Annotation, cur cursorAnnotKey) int { + for i, a := range flat { + if compareAnnotPos(a.File, a.Line, cur.file, cur.line) > 0 { + return i + } + } + return len(flat) +} + +// compareAnnotPos compares two (file, line) annotation positions using the +// same ordering as the flat annotation list: alphabetical by file, then +// ascending by line within a file. Returns -1, 0, or 1. +func compareAnnotPos(aFile string, aLine int, bFile string, bLine int) int { + if aFile != bFile { + if aFile < bFile { + return -1 + } + return 1 + } + if aLine < bLine { + return -1 + } + if aLine > bLine { + return 1 + } + return 0 +} diff --git a/app/ui/annotnav_test.go b/app/ui/annotnav_test.go new file mode 100644 index 00000000..aef24993 --- /dev/null +++ b/app/ui/annotnav_test.go @@ -0,0 +1,664 @@ +package ui + +import ( + "testing" + + tea "github.com/charmbracelet/bubbletea" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/umputun/revdiff/app/annotation" + "github.com/umputun/revdiff/app/diff" + "github.com/umputun/revdiff/app/keymap" + "github.com/umputun/revdiff/app/ui/overlay" +) + +func TestPickAdjacentAnnotation(t *testing.T) { + a1 := annotation.Annotation{File: "a.go", Line: 5, Type: "+"} + a2 := annotation.Annotation{File: "a.go", Line: 10, Type: "+"} + b1 := annotation.Annotation{File: "b.go", Line: 0, Type: ""} + b2 := annotation.Annotation{File: "b.go", Line: 7, Type: "-"} + flat := []annotation.Annotation{a1, a2, b1, b2} + + tests := []struct { + name string + flat []annotation.Annotation + cur cursorAnnotKey + forward bool + want annotation.Annotation + ok bool + }{ + {"empty list forward", nil, cursorAnnotKey{file: "a.go", line: 1}, true, annotation.Annotation{}, false}, + {"empty list backward", nil, cursorAnnotKey{file: "a.go", line: 1}, false, annotation.Annotation{}, false}, + + {"forward from before first", flat, cursorAnnotKey{file: "a.go", line: 1}, true, a1, true}, + {"backward from before first → no-op", flat, cursorAnnotKey{file: "a.go", line: 1}, false, annotation.Annotation{}, false}, + + {"forward from on first annotation", flat, cursorAnnotKey{file: "a.go", line: 5, typ: "+", onAnnot: true}, true, a2, true}, + {"backward from on first annotation → no-op", flat, cursorAnnotKey{file: "a.go", line: 5, typ: "+", onAnnot: true}, false, annotation.Annotation{}, false}, + + {"forward from on middle annotation", flat, cursorAnnotKey{file: "a.go", line: 10, typ: "+", onAnnot: true}, true, b1, true}, + {"backward from on middle annotation", flat, cursorAnnotKey{file: "a.go", line: 10, typ: "+", onAnnot: true}, false, a1, true}, + + {"forward crosses file boundary", flat, cursorAnnotKey{file: "a.go", line: 99}, true, b1, true}, + {"backward from b.go line 1 lands on b.go file-level", flat, cursorAnnotKey{file: "b.go", line: 1}, false, b1, true}, + {"backward from b.go line -1 crosses to a.go", flat, cursorAnnotKey{file: "b.go", line: -1}, false, a2, true}, + + {"forward from on last annotation → no-op", flat, cursorAnnotKey{file: "b.go", line: 7, typ: "-", onAnnot: true}, true, annotation.Annotation{}, false}, + {"backward from on last annotation", flat, cursorAnnotKey{file: "b.go", line: 7, typ: "-", onAnnot: true}, false, b1, true}, + + {"forward past last → no-op", flat, cursorAnnotKey{file: "z.go", line: 1}, true, annotation.Annotation{}, false}, + {"backward past last → last item", flat, cursorAnnotKey{file: "z.go", line: 1}, false, b2, true}, + + // "az.go" sorts between "a.go" and "b.go" alphabetically: + // "a.go" < "az.go" because '.' (0x2e) < 'z' (0x7a) at index 1, and "az.go" < "b.go" because 'a' < 'b'. + {"forward from file with no annotations", flat, cursorAnnotKey{file: "az.go", line: 5}, true, b1, true}, + {"backward from file with no annotations", flat, cursorAnnotKey{file: "az.go", line: 5}, false, a2, true}, + + // file-level annotation (Line=0) + {"forward from before file-level lands on file-level", flat, cursorAnnotKey{file: "b.go", line: -1}, true, b1, true}, + {"forward from on file-level annotation goes to next within file", + flat, cursorAnnotKey{file: "b.go", line: 0, typ: "", onAnnot: true}, true, b2, true}, + {"backward from on file-level annotation crosses to prev file", + flat, cursorAnnotKey{file: "b.go", line: 0, typ: "", onAnnot: true}, false, a2, true}, + + // edge case: cursor on context line at same line as a typed annotation + // not exactly on it (different type) — strict "after" excludes same-line + // annotations, so forward skips; backward picks it up via insertion-1. + {"forward from context at same line as add-annotation skips same-line", + []annotation.Annotation{ + {File: "a.go", Line: 5, Type: "+"}, + {File: "a.go", Line: 9, Type: "+"}, + }, + cursorAnnotKey{file: "a.go", line: 5, typ: " "}, true, + annotation.Annotation{File: "a.go", Line: 9, Type: "+"}, true, + }, + {"backward from context at same line as add-annotation reaches it", + []annotation.Annotation{ + {File: "a.go", Line: 1, Type: "+"}, + {File: "a.go", Line: 5, Type: "+"}, + }, + cursorAnnotKey{file: "a.go", line: 5, typ: " "}, false, + annotation.Annotation{File: "a.go", Line: 5, Type: "+"}, true, + }, + + // onAnnot=true but no triple match in flat — exactAnnotIndex falls + // through to the insertion-point branch. Reaches this state when the + // caller constructs cursorAnnotKey with onAnnot=true but the exact + // (file, line, type) triple is not in flat (e.g. matched a different- + // type annotation at the same line). Must NOT cause incorrect + // stepping; must use insertion-point fallback. + {"forward with onAnnot=true but no exact match falls through to insertion-point", + flat, + cursorAnnotKey{file: "a.go", line: 5, typ: "-", onAnnot: true}, true, + a2, true, + }, + {"backward with onAnnot=true but no exact match falls through to insertion-point", + flat, + cursorAnnotKey{file: "a.go", line: 10, typ: "-", onAnnot: true}, false, + a2, true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, ok := pickAdjacentAnnotation(tt.flat, tt.cur, tt.forward) + assert.Equal(t, tt.ok, ok) + if tt.ok { + assert.Equal(t, tt.want, got) + } + }) + } +} + +func TestCompareAnnotPos(t *testing.T) { + tests := []struct { + name string + aFile, bFile string + aLine, bLine, want int + }{ + {"same file same line", "a.go", "a.go", 5, 5, 0}, + {"same file lower line", "a.go", "a.go", 3, 5, -1}, + {"same file higher line", "a.go", "a.go", 7, 5, 1}, + {"earlier file", "a.go", "b.go", 100, 1, -1}, + {"later file", "b.go", "a.go", 1, 100, 1}, + {"same file file-level vs line", "a.go", "a.go", 0, 5, -1}, + {"same file line vs file-level (symmetry)", "a.go", "a.go", 5, 0, 1}, + {"same file both file-level", "a.go", "a.go", 0, 0, 0}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, compareAnnotPos(tt.aFile, tt.aLine, tt.bFile, tt.bLine)) + }) + } +} + +func TestModel_CurrentAnnotKey(t *testing.T) { + lines := []diff.DiffLine{ + {ChangeType: diff.ChangeContext, Content: "ctx", OldNum: 1, NewNum: 1}, + {ChangeType: diff.ChangeAdd, Content: "added", OldNum: 0, NewNum: 2}, + {ChangeType: diff.ChangeRemove, Content: "removed", OldNum: 2, NewNum: 0}, + } + m := testModel([]string{"a.go"}, nil) + m.file.name = "a.go" + m.file.lines = lines + + t.Run("on context line", func(t *testing.T) { + m.nav.diffCursor = 0 + key := m.currentAnnotKey() + assert.Equal(t, "a.go", key.file) + assert.Equal(t, 1, key.line) + assert.Equal(t, " ", key.typ) + assert.False(t, key.onAnnot) + }) + + t.Run("on add line uses NewNum", func(t *testing.T) { + m.nav.diffCursor = 1 + key := m.currentAnnotKey() + assert.Equal(t, 2, key.line) + assert.Equal(t, "+", key.typ) + }) + + t.Run("on remove line uses OldNum", func(t *testing.T) { + m.nav.diffCursor = 2 + key := m.currentAnnotKey() + assert.Equal(t, 2, key.line) + assert.Equal(t, "-", key.typ) + }) + + t.Run("onAnnot true when store has matching annotation", func(t *testing.T) { + m.store.Add(annotation.Annotation{File: "a.go", Line: 2, Type: "+", Comment: "x"}) + m.nav.diffCursor = 1 + key := m.currentAnnotKey() + assert.True(t, key.onAnnot) + }) + + t.Run("file-level cursor (-1) maps to file-level key", func(t *testing.T) { + m.store.Add(annotation.Annotation{File: "a.go", Line: 0, Type: "", Comment: "file note"}) + m.nav.diffCursor = -1 + key := m.currentAnnotKey() + assert.Equal(t, 0, key.line) + assert.Empty(t, key.typ) + assert.True(t, key.onAnnot) + }) + + t.Run("out-of-range cursor returns line -1", func(t *testing.T) { + m.nav.diffCursor = 999 + key := m.currentAnnotKey() + assert.Equal(t, -1, key.line) + assert.False(t, key.onAnnot) + }) + + t.Run("ChangeDivider row collapses to line -1 so file-level annot stays reachable", func(t *testing.T) { + // dividers carry OldNum=NewNum=0; without the guard they would + // alias the file-level annotation key and cause forward navigation + // from the divider to skip the same-file file-level annotation. + divLines := []diff.DiffLine{ + {ChangeType: diff.ChangeDivider, Content: "⋯ 5 lines ⋯", OldNum: 0, NewNum: 0}, + {ChangeType: diff.ChangeAdd, Content: "added", OldNum: 0, NewNum: 6}, + } + dm := testModel([]string{"a.go"}, nil) + dm.file.name = "a.go" + dm.file.lines = divLines + dm.nav.diffCursor = 0 + key := dm.currentAnnotKey() + assert.Equal(t, -1, key.line) + assert.Empty(t, key.typ) + assert.False(t, key.onAnnot) + }) +} + +func TestModel_HandleAnnotNav_WithinFile(t *testing.T) { + // layout: ctx (line 1) | + (line 2) | + (line 3) | + (line 4) + // annotations at lines 2 and 4 — context line 1 lets us test "before first" + lines := []diff.DiffLine{ + {ChangeType: diff.ChangeContext, Content: "ctx", OldNum: 1, NewNum: 1}, + {ChangeType: diff.ChangeAdd, Content: "a", OldNum: 0, NewNum: 2}, + {ChangeType: diff.ChangeAdd, Content: "b", OldNum: 0, NewNum: 3}, + {ChangeType: diff.ChangeAdd, Content: "c", OldNum: 0, NewNum: 4}, + } + m := testModel([]string{"a.go"}, map[string][]diff.DiffLine{"a.go": lines}) + result, _ := m.Update(filesLoadedMsg{entries: []diff.FileEntry{{Path: "a.go"}}}) + m = result.(Model) + loadMsg := m.loadFileDiff("a.go")() + result, _ = m.Update(loadMsg) + m = result.(Model) + m.store.Add(annotation.Annotation{File: "a.go", Line: 2, Type: "+", Comment: "first"}) + m.store.Add(annotation.Annotation{File: "a.go", Line: 4, Type: "+", Comment: "second"}) + + t.Run("forward from before first lands on first", func(t *testing.T) { + m.nav.diffCursor = 0 // ctx line 1, before any annotation + result, _ := m.handleAnnotNav(true) + model := result.(Model) + assert.Equal(t, 1, model.nav.diffCursor) // line 2 = index 1 + }) + + t.Run("forward from on first lands on second", func(t *testing.T) { + m.nav.diffCursor = 1 // line 2, on first annotation + result, _ := m.handleAnnotNav(true) + model := result.(Model) + assert.Equal(t, 3, model.nav.diffCursor) // line 4 = index 3 + }) + + t.Run("forward from on second is no-op", func(t *testing.T) { + m.nav.diffCursor = 3 // line 4, on second annotation + result, _ := m.handleAnnotNav(true) + model := result.(Model) + assert.Equal(t, 3, model.nav.diffCursor) // unchanged + }) + + t.Run("backward from on second lands on first", func(t *testing.T) { + m.nav.diffCursor = 3 + result, _ := m.handleAnnotNav(false) + model := result.(Model) + assert.Equal(t, 1, model.nav.diffCursor) + }) + + t.Run("backward from on first is no-op", func(t *testing.T) { + m.nav.diffCursor = 1 + result, _ := m.handleAnnotNav(false) + model := result.(Model) + assert.Equal(t, 1, model.nav.diffCursor) + }) + + t.Run("empty store is no-op", func(t *testing.T) { + m.store.Clear() + m.nav.diffCursor = 1 + before := m.nav.diffCursor + result, cmd := m.handleAnnotNav(true) + model := result.(Model) + assert.Equal(t, before, model.nav.diffCursor) + assert.Nil(t, cmd) + }) +} + +func TestModel_HandleAnnotNav_CrossFile(t *testing.T) { + diffs := map[string][]diff.DiffLine{ + "a.go": {{ChangeType: diff.ChangeAdd, Content: "a1", OldNum: 0, NewNum: 1}}, + "b.go": {{ChangeType: diff.ChangeAdd, Content: "b1", OldNum: 0, NewNum: 1}}, + } + m := testModel([]string{"a.go", "b.go"}, diffs) + result, _ := m.Update(filesLoadedMsg{entries: []diff.FileEntry{{Path: "a.go"}, {Path: "b.go"}}}) + m = result.(Model) + loadMsg := m.loadFileDiff("a.go")() + result, _ = m.Update(loadMsg) + m = result.(Model) + m.store.Add(annotation.Annotation{File: "a.go", Line: 1, Type: "+", Comment: "in a"}) + m.store.Add(annotation.Annotation{File: "b.go", Line: 1, Type: "+", Comment: "in b"}) + + t.Run("forward from on a.go's annotation crosses to b.go", func(t *testing.T) { + m.nav.diffCursor = 0 // line 1, on a.go annotation + result, cmd := m.handleAnnotNav(true) + model := result.(Model) + require.NotNil(t, model.pendingAnnotJump, "must set pendingAnnotJump for cross-file target") + assert.Equal(t, "b.go", model.pendingAnnotJump.File) + require.NotNil(t, cmd) + + // simulate file load completing + loadMsg := cmd() + result, _ = model.Update(loadMsg) + model = result.(Model) + assert.Equal(t, "b.go", model.file.name) + assert.Nil(t, model.pendingAnnotJump) + }) + + t.Run("forward from on b.go's annotation is no-op (last in flat list)", func(t *testing.T) { + // load b.go and position on its annotation + loadMsg := m.loadFileDiff("b.go")() + result, _ := m.Update(loadMsg) + model := result.(Model) + model.nav.diffCursor = 0 + result, cmd := model.handleAnnotNav(true) + model = result.(Model) + assert.Nil(t, model.pendingAnnotJump, "no boundary crossing at last annotation") + assert.Nil(t, cmd) + assert.Equal(t, "b.go", model.file.name) + }) + + t.Run("backward from on a.go's annotation is no-op (first in flat list)", func(t *testing.T) { + m.nav.diffCursor = 0 + result, cmd := m.handleAnnotNav(false) + model := result.(Model) + assert.Nil(t, model.pendingAnnotJump) + assert.Nil(t, cmd) + assert.Equal(t, "a.go", model.file.name) + }) +} + +func TestModel_HandleAnnotNav_DeletionRobustness(t *testing.T) { + lines := []diff.DiffLine{ + {ChangeType: diff.ChangeAdd, Content: "a", OldNum: 0, NewNum: 1}, + {ChangeType: diff.ChangeAdd, Content: "b", OldNum: 0, NewNum: 2}, + {ChangeType: diff.ChangeAdd, Content: "c", OldNum: 0, NewNum: 3}, + } + m := testModel([]string{"a.go"}, map[string][]diff.DiffLine{"a.go": lines}) + result, _ := m.Update(filesLoadedMsg{entries: []diff.FileEntry{{Path: "a.go"}}}) + m = result.(Model) + loadMsg := m.loadFileDiff("a.go")() + result, _ = m.Update(loadMsg) + m = result.(Model) + + t.Run("delete current annotation then forward lands on next", func(t *testing.T) { + m.store.Clear() + m.store.Add(annotation.Annotation{File: "a.go", Line: 1, Type: "+", Comment: "x"}) + m.store.Add(annotation.Annotation{File: "a.go", Line: 3, Type: "+", Comment: "y"}) + m.nav.diffCursor = 0 // on line 1 + m.store.Delete("a.go", 1, "+") + result, _ := m.handleAnnotNav(true) + model := result.(Model) + assert.Equal(t, 2, model.nav.diffCursor) // jumped to line 3 even though cursor still at index 0 + }) + + t.Run("delete current annotation then backward lands on prev", func(t *testing.T) { + m.store.Clear() + m.store.Add(annotation.Annotation{File: "a.go", Line: 1, Type: "+", Comment: "x"}) + m.store.Add(annotation.Annotation{File: "a.go", Line: 3, Type: "+", Comment: "y"}) + m.nav.diffCursor = 2 // on line 3 + m.store.Delete("a.go", 3, "+") + result, _ := m.handleAnnotNav(false) + model := result.(Model) + assert.Equal(t, 0, model.nav.diffCursor) // jumped to line 1 + }) + + t.Run("delete only annotation then either direction is no-op", func(t *testing.T) { + m.store.Clear() + m.store.Add(annotation.Annotation{File: "a.go", Line: 1, Type: "+", Comment: "x"}) + m.nav.diffCursor = 0 + m.store.Delete("a.go", 1, "+") + + result, cmd := m.handleAnnotNav(true) + model := result.(Model) + assert.Equal(t, 0, model.nav.diffCursor) + assert.Nil(t, cmd) + + result, cmd = m.handleAnnotNav(false) + model = result.(Model) + assert.Equal(t, 0, model.nav.diffCursor) + assert.Nil(t, cmd) + }) +} + +func TestModel_HandleAnnotNav_KeymapDispatch(t *testing.T) { + lines := []diff.DiffLine{ + {ChangeType: diff.ChangeAdd, Content: "a", OldNum: 0, NewNum: 1}, + {ChangeType: diff.ChangeAdd, Content: "b", OldNum: 0, NewNum: 2}, + } + m := testModel([]string{"a.go"}, map[string][]diff.DiffLine{"a.go": lines}) + result, _ := m.Update(filesLoadedMsg{entries: []diff.FileEntry{{Path: "a.go"}}}) + m = result.(Model) + loadMsg := m.loadFileDiff("a.go")() + result, _ = m.Update(loadMsg) + m = result.(Model) + m.store.Add(annotation.Annotation{File: "a.go", Line: 1, Type: "+", Comment: "x"}) + m.store.Add(annotation.Annotation{File: "a.go", Line: 2, Type: "+", Comment: "y"}) + + t.Run("} key triggers next annotation", func(t *testing.T) { + m.nav.diffCursor = 0 + result, _ := m.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'}'}}) + model := result.(Model) + assert.Equal(t, 1, model.nav.diffCursor) + }) + + t.Run("{ key triggers prev annotation", func(t *testing.T) { + m.nav.diffCursor = 1 + result, _ := m.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'{'}}) + model := result.(Model) + assert.Equal(t, 0, model.nav.diffCursor) + }) + + // pin the always-on cross-file invariant: the actions sit in the global + // dispatch switch (model.go), not in handleDiffAction, so } / { must fire + // regardless of which pane is focused. A future refactor that demoted the + // case into the pane-specific handler would silently regress tree-focus + // behavior. + t.Run("} fires from tree pane focus (always-on invariant)", func(t *testing.T) { + m.nav.diffCursor = 0 + m.layout.focus = paneTree + result, _ := m.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'}'}}) + model := result.(Model) + assert.Equal(t, 1, model.nav.diffCursor, "} must fire from tree-pane focus") + }) + + t.Run("{ fires from tree pane focus (always-on invariant)", func(t *testing.T) { + m.nav.diffCursor = 1 + m.layout.focus = paneTree + result, _ := m.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'{'}}) + model := result.(Model) + assert.Equal(t, 0, model.nav.diffCursor, "{ must fire from tree-pane focus") + }) +} + +// modal-active suppression: }/ { must NOT navigate while a textinput modal +// (annotation input or search input) is consuming keystrokes. handleModalKey +// short-circuits before dispatch so the literal characters reach the input. +func TestModel_HandleAnnotNav_ModalSuppression(t *testing.T) { + lines := []diff.DiffLine{ + {ChangeType: diff.ChangeAdd, Content: "a", OldNum: 0, NewNum: 1}, + {ChangeType: diff.ChangeAdd, Content: "b", OldNum: 0, NewNum: 2}, + } + m := testModel([]string{"a.go"}, map[string][]diff.DiffLine{"a.go": lines}) + result, _ := m.Update(filesLoadedMsg{entries: []diff.FileEntry{{Path: "a.go"}}}) + m = result.(Model) + loadMsg := m.loadFileDiff("a.go")() + result, _ = m.Update(loadMsg) + m = result.(Model) + m.store.Add(annotation.Annotation{File: "a.go", Line: 1, Type: "+", Comment: "x"}) + m.store.Add(annotation.Annotation{File: "a.go", Line: 2, Type: "+", Comment: "y"}) + + t.Run("} does not navigate while annotation input is active", func(t *testing.T) { + m.nav.diffCursor = 0 + m.layout.focus = paneDiff + // open the annotation modal on the current line + cmd := m.startAnnotation() + require.NotNil(t, cmd) + require.True(t, m.annot.annotating, "annotation modal must be active for this test") + before := m.nav.diffCursor + result, _ := m.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'}'}}) + model := result.(Model) + assert.Equal(t, before, model.nav.diffCursor, "annotation modal must swallow }") + assert.True(t, model.annot.annotating, "annotation modal must remain active") + }) + + t.Run("{ does not navigate while search input is active", func(t *testing.T) { + m.nav.diffCursor = 1 + m.annot.annotating = false + m.layout.focus = paneDiff + cmd := m.startSearch() + require.NotNil(t, cmd) + require.True(t, m.search.active, "search modal must be active for this test") + before := m.nav.diffCursor + result, _ := m.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'{'}}) + model := result.(Model) + assert.Equal(t, before, model.nav.diffCursor, "search modal must swallow {") + assert.True(t, model.search.active, "search modal must remain active") + }) +} + +// godoc on handleAnnotNav claims "collapsed-hunk expansion comes for free" via +// jumpToAnnotationTarget → ensureHunkExpanded. This test pins that contract: +// jumping to an annotation inside a collapsed hunk must expand the hunk. +func TestModel_HandleAnnotNav_CollapsedModeExpandsHunk(t *testing.T) { + // build a diff with a context block then a change hunk so collapsed mode + // has something to collapse. annotation lives on the add line inside the + // hunk; expansion is detectable via expandedHunks map. + lines := []diff.DiffLine{ + {ChangeType: diff.ChangeContext, Content: "ctx1", OldNum: 1, NewNum: 1}, + {ChangeType: diff.ChangeContext, Content: "ctx2", OldNum: 2, NewNum: 2}, + {ChangeType: diff.ChangeRemove, Content: "old", OldNum: 3, NewNum: 0}, + {ChangeType: diff.ChangeAdd, Content: "new", OldNum: 0, NewNum: 3}, + {ChangeType: diff.ChangeContext, Content: "ctx3", OldNum: 4, NewNum: 4}, + } + m := testModel([]string{"a.go"}, map[string][]diff.DiffLine{"a.go": lines}) + result, _ := m.Update(filesLoadedMsg{entries: []diff.FileEntry{{Path: "a.go"}}}) + m = result.(Model) + loadMsg := m.loadFileDiff("a.go")() + result, _ = m.Update(loadMsg) + m = result.(Model) + + // enable collapsed mode and place annotation on the remove line (which + // is hidden by collapsed mode by default) + m.modes.collapsed.enabled = true + m.modes.collapsed.expandedHunks = map[int]bool{} + m.store.Add(annotation.Annotation{File: "a.go", Line: 3, Type: "-", Comment: "removed"}) + + // position cursor at the top of the file so } walks forward to the annotation + m.nav.diffCursor = 0 + result, _ = m.handleAnnotNav(true) + model := result.(Model) + hunks := model.findHunks() + require.NotEmpty(t, hunks, "test setup must produce at least one hunk") + assert.Truef(t, model.modes.collapsed.expandedHunks[hunks[0]], + "jumping to annotation inside collapsed hunk must expand the hunk; expandedHunks=%v", model.modes.collapsed.expandedHunks) +} + +// pin backward cross-file integration: } symmetric coverage exists, but { +// crossing a file boundary triggers the same pendingAnnotJump handoff and +// loadSelectedIfChanged path. Asymmetric coverage would let direction-handling +// regressions slip through. +func TestModel_HandleAnnotNav_BackwardCrossFile(t *testing.T) { + diffs := map[string][]diff.DiffLine{ + "a.go": {{ChangeType: diff.ChangeAdd, Content: "a1", OldNum: 0, NewNum: 1}}, + "b.go": {{ChangeType: diff.ChangeAdd, Content: "b1", OldNum: 0, NewNum: 1}}, + } + m := testModel([]string{"a.go", "b.go"}, diffs) + result, _ := m.Update(filesLoadedMsg{entries: []diff.FileEntry{{Path: "a.go"}, {Path: "b.go"}}}) + m = result.(Model) + // load b.go and position on its annotation + loadMsg := m.loadFileDiff("b.go")() + result, _ = m.Update(loadMsg) + m = result.(Model) + m.store.Add(annotation.Annotation{File: "a.go", Line: 1, Type: "+", Comment: "in a"}) + m.store.Add(annotation.Annotation{File: "b.go", Line: 1, Type: "+", Comment: "in b"}) + m.nav.diffCursor = 0 // on b.go's annotation + + result, cmd := m.handleAnnotNav(false) + model := result.(Model) + require.NotNil(t, model.pendingAnnotJump, "{ across files must set pendingAnnotJump") + assert.Equal(t, "a.go", model.pendingAnnotJump.File) + require.NotNil(t, cmd) + + loadCmd := cmd() + result, _ = model.Update(loadCmd) + model = result.(Model) + assert.Equal(t, "a.go", model.file.name) + assert.Nil(t, model.pendingAnnotJump) +} + +// walker-stuck retry: when the immediate adjacent target is non-jumpable +// (cross-file path missing from tree, or same-file line missing from loaded +// diff in compact mode), the walker must skip past it and find the next +// jumpable candidate instead of getting trapped. +func TestModel_HandleAnnotNav_SkipsNonJumpableSameFileTarget(t *testing.T) { + // loaded diff only has line 5 (compact mode could shrink context like this). + // Annotations exist at lines 3 (NOT in loaded diff → non-jumpable) and 7 + // (NOT in loaded diff → non-jumpable) and 5 (jumpable). + // Cursor sits BEFORE line 5; forward walker must skip line-3 candidate + // (wait, line 3 < 5 so forward never sees it), then skip nothing, then + // land on line 5 — for clearer test, put one non-jumpable AFTER line 5 + // and one jumpable further after. + lines := []diff.DiffLine{ + {ChangeType: diff.ChangeAdd, Content: "L5", OldNum: 0, NewNum: 5}, + {ChangeType: diff.ChangeAdd, Content: "L9", OldNum: 0, NewNum: 9}, + } + m := testModel([]string{"a.go"}, map[string][]diff.DiffLine{"a.go": lines}) + result, _ := m.Update(filesLoadedMsg{entries: []diff.FileEntry{{Path: "a.go"}}}) + m = result.(Model) + loadMsg := m.loadFileDiff("a.go")() + result, _ = m.Update(loadMsg) + m = result.(Model) + + // annotations: line 5 (jumpable), line 7 (NOT in loaded diff → non-jumpable), + // line 9 (jumpable). Cursor on line 5. + m.store.Add(annotation.Annotation{File: "a.go", Line: 5, Type: "+", Comment: "a"}) + m.store.Add(annotation.Annotation{File: "a.go", Line: 7, Type: "+", Comment: "stuck"}) + m.store.Add(annotation.Annotation{File: "a.go", Line: 9, Type: "+", Comment: "c"}) + m.nav.diffCursor = 0 // line 5 + + result, _ = m.handleAnnotNav(true) + model := result.(Model) + // must skip line-7 (non-jumpable) and land on line 9 (jumpable, index 1) + assert.Equal(t, 1, model.nav.diffCursor, "walker must skip non-jumpable line-7 annotation and reach line-9") +} + +// stuck-loop bound check: when ALL forward annotations are non-jumpable, +// the walker must terminate (bounded by len(flat)) and leave the cursor +// unchanged rather than spinning. +func TestModel_HandleAnnotNav_AllNonJumpableTerminates(t *testing.T) { + lines := []diff.DiffLine{ + {ChangeType: diff.ChangeAdd, Content: "L5", OldNum: 0, NewNum: 5}, + } + m := testModel([]string{"a.go"}, map[string][]diff.DiffLine{"a.go": lines}) + result, _ := m.Update(filesLoadedMsg{entries: []diff.FileEntry{{Path: "a.go"}}}) + m = result.(Model) + loadMsg := m.loadFileDiff("a.go")() + result, _ = m.Update(loadMsg) + m = result.(Model) + + // only annotation is at line 7, which is not in the loaded diff + m.store.Add(annotation.Annotation{File: "a.go", Line: 7, Type: "+", Comment: "stuck"}) + m.nav.diffCursor = 0 // line 5 + + before := m.nav.diffCursor + result, cmd := m.handleAnnotNav(true) + model := result.(Model) + assert.Equal(t, before, model.nav.diffCursor, "walker must not move when no jumpable target exists") + assert.Nil(t, cmd) +} + +func TestModel_HandleAnnotNav_DefaultBindings(t *testing.T) { + km := keymap.Default() + assert.Equal(t, keymap.ActionNextAnnotation, km.Resolve("}")) + assert.Equal(t, keymap.ActionPrevAnnotation, km.Resolve("{")) +} + +// tryJumpToAnnotationTarget returns false on unreachable targets so the +// walker can skip past them. This test pins the contract for each rejection +// path: nil target, same-file line not in loaded diff, single-file mode +// cross-file (which has nowhere to go). +func TestModel_TryJumpToAnnotationTarget_RejectsNonJumpable(t *testing.T) { + lines := []diff.DiffLine{ + {ChangeType: diff.ChangeAdd, Content: "L5", OldNum: 0, NewNum: 5}, + } + m := testModel([]string{"a.go"}, map[string][]diff.DiffLine{"a.go": lines}) + result, _ := m.Update(filesLoadedMsg{entries: []diff.FileEntry{{Path: "a.go"}}}) + m = result.(Model) + loadMsg := m.loadFileDiff("a.go")() + result, _ = m.Update(loadMsg) + m = result.(Model) + + t.Run("nil target", func(t *testing.T) { + _, _, ok := m.tryJumpToAnnotationTarget(nil) + assert.False(t, ok) + }) + + t.Run("same-file line missing from loaded diff", func(t *testing.T) { + // line 99 is not in m.file.lines + _, _, ok := m.tryJumpToAnnotationTarget(&overlay.AnnotationTarget{ + File: "a.go", ChangeType: "+", Line: 99, + }) + assert.False(t, ok, "line not in loaded diff must reject") + }) + + t.Run("file-level annotation always jumpable", func(t *testing.T) { + _, _, ok := m.tryJumpToAnnotationTarget(&overlay.AnnotationTarget{ + File: "a.go", ChangeType: "", Line: 0, + }) + assert.True(t, ok, "file-level (Line=0) target must be jumpable in current file") + }) + + t.Run("single-file mode rejects cross-file target", func(t *testing.T) { + mSingle := m + mSingle.file.singleFile = true + _, _, ok := mSingle.tryJumpToAnnotationTarget(&overlay.AnnotationTarget{ + File: "other.go", ChangeType: "+", Line: 1, + }) + assert.False(t, ok, "single-file mode must reject cross-file targets") + }) + + t.Run("same-file jumpable line returns ok", func(t *testing.T) { + _, _, ok := m.tryJumpToAnnotationTarget(&overlay.AnnotationTarget{ + File: "a.go", ChangeType: "+", Line: 5, + }) + assert.True(t, ok) + }) +} diff --git a/app/ui/model.go b/app/ui/model.go index 29367969..b4d0be17 100644 --- a/app/ui/model.go +++ b/app/ui/model.go @@ -916,6 +916,8 @@ func (m Model) dispatchAction(action keymap.Action) (tea.Model, tea.Cmd) { return m.handleViewToggle(action) case keymap.ActionNextHunk, keymap.ActionPrevHunk: return m.handleHunkNav(action == keymap.ActionNextHunk) + case keymap.ActionNextAnnotation, keymap.ActionPrevAnnotation: + return m.handleAnnotNav(action == keymap.ActionNextAnnotation) case keymap.ActionReload: return m.handleReload() default: // remaining actions (navigation, search, etc.) handled by pane-specific handlers below diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 093d5644..f4374cbb 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -105,7 +105,8 @@ Central package. Single `Model` struct implements bubbletea's `Model` interface. | `scrollbar.go` | Vertical scrollbar thumb post-processing on rendered diff/tree/TOC panes (replaces right-border `│` with `┃` on rows mapped to the visible viewport portion) | | `collapsed.go` | Collapsed diff mode: hide removes, show modified markers | | `annotate.go` | Annotation input lifecycle: start, save, cancel, delete | -| `annotlist.go` | Annotation list spec building, cross-file jump logic | +| `annotlist.go` | Annotation list spec building, cross-file jump logic (`jumpToAnnotationTarget` for the `@` popup, `tryJumpToAnnotationTarget` returning a jumped-bool for the `}`/`{` walker) | +| `annotnav.go` | Cross-file annotation navigation (`}` / `{`): builds the flat annotation list, computes adjacent target via exact-match or insertion-point fallback, retries through non-jumpable targets so a hidden annotation cannot trap the walker | | `editor.go` | `$EDITOR` handoff for multi-line annotations: `openEditor()` wraps `app/editor.Editor` in `tea.ExecProcess`, `editorFinishedMsg` dispatch, `handleEditorFinished` routing (save / cancel / error-preserve) | | `themeselect.go` | Theme selector operations: open, preview, confirm, apply (via injected `ThemeCatalog`) | | `search.go` | Search input handling, match computation, navigation | diff --git a/plugins/codex/skills/revdiff/references/usage.md b/plugins/codex/skills/revdiff/references/usage.md index 84e102d6..7bf3f623 100644 --- a/plugins/codex/skills/revdiff/references/usage.md +++ b/plugins/codex/skills/revdiff/references/usage.md @@ -124,6 +124,7 @@ Use `--stdin` to review arbitrary piped or redirected text as one synthetic file | `a` or `Enter` (diff pane) | Annotate current diff line | | `A` | Add file-level annotation (stored at top of diff) | | `@` | Toggle annotation list popup (navigate and jump to any annotation) | +| `}` / `{` | Jump to next/previous annotation (always crosses file boundaries; silent no-op at the first/last annotation) | | `d` | Delete annotation under cursor | | `Ctrl+E` (during annotation input) | Open `$EDITOR` for multi-line annotation | | `Esc` | Cancel annotation input | diff --git a/site/docs.html b/site/docs.html index d6bc498f..0f2ce92f 100644 --- a/site/docs.html +++ b/site/docs.html @@ -490,6 +490,7 @@

Annotations

a or EnterAnnotate current line AAdd file-level annotation @Toggle annotation list popup + } / {Jump to next/previous annotation (always crosses file boundaries; silent no-op at the first/last annotation) dDelete annotation under cursor Ctrl+EOpen $EDITOR for multi-line annotation (during annotation input) EscCancel annotation input @@ -581,7 +582,7 @@

Available actions

File/Hunk: next_item, prev_item, next_hunk, prev_hunk

Pane: toggle_pane, focus_tree, focus_diff

Search: search

-

Annotations: confirm, annotate_file, delete_annotation, annot_list

+

Annotations: confirm, annotate_file, delete_annotation, annot_list, next_annotation, prev_annotation

View: toggle_collapsed, toggle_compact, toggle_wrap, toggle_tree, toggle_line_numbers, toggle_blame, toggle_word_diff, toggle_hunk, toggle_untracked, mark_reviewed, theme_select, filter, info, reload

Quit: quit, discard_quit, help, dismiss

From a5a5e4d0cb204c1c166d7b913a18ccb3544ee8c1 Mon Sep 17 00:00:00 2001 From: Umputun Date: Fri, 1 May 2026 02:08:25 -0500 Subject: [PATCH 2/4] =?UTF-8?q?fix(review-loop):=20final=20cleanup=20pass?= =?UTF-8?q?=20=E2=80=94=20addressed=202=20minor=20findings?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - [minor] app/ui/annotnav.go:25 — handleAnnotNav was O(N²) (len(flat) outer retries × pickAdjacentAnnotation O(N) inner). Refactored to compute the starting flat-list index once via startingFlatIndex, then walk indexes directly with ±1 step. Each candidate is examined at most once → O(N) worst case even when every annotation is non-jumpable. - [minor] app/ui/annotnav.go:81 — currentAnnotKey mapped ALL ChangeDivider rows to line=-1, which is correct for leading dividers but causes middle/trailing dividers (reachable via mouse-click) to short-circuit forward } back to the file-level/first same-file annotation. Fixed by walking back to the nearest preceding non-divider line via dividerAnnotKey and inheriting its position; only leading dividers (no prior non-divider line) fall back to line=-1. Tests added for middle/trailing/consecutive-divider cases at both unit and integration level. The pickAdjacentAnnotation table tests now run through a test-only wrapper around startingFlatIndex so the picking algorithm documentation is preserved. --- app/ui/annotnav.go | 106 ++++++++++++++++++++++------------------ app/ui/annotnav_test.go | 103 +++++++++++++++++++++++++++++++++++++- 2 files changed, 160 insertions(+), 49 deletions(-) diff --git a/app/ui/annotnav.go b/app/ui/annotnav.go index 49c8985e..51a73416 100644 --- a/app/ui/annotnav.go +++ b/app/ui/annotnav.go @@ -17,22 +17,27 @@ import ( // // When a target is in the store but not currently displayable (cross-file // path filtered out of the tree, or same-file line missing from the loaded -// diff in compact mode), the walker advances past it and retries the next -// candidate in the same direction. Without that loop the cursor would not -// move on a silent-fail target and every subsequent }/{ press would -// recompute the same hidden target, trapping the user. The loop is bounded -// by the flat list length so a faulty store can never spin forever. +// diff in compact mode), the walker advances past it and tries the next +// candidate in the same direction. Without that walk-past the cursor would +// not move on a silent-fail target and every subsequent }/{ press would +// recompute the same hidden target, trapping the user. +// +// The walk is index-based: starting position is computed once via +// startingFlatIndex, then the loop steps by ±1 per attempt. Each candidate +// is examined at most once, so the worst case is O(N) over flat — even +// when every annotation is non-jumpable. func (m Model) handleAnnotNav(forward bool) (tea.Model, tea.Cmd) { flat := m.buildAnnotListItems() if len(flat) == 0 { return m, nil } cur := m.currentAnnotKey() - for range flat { - target, ok := pickAdjacentAnnotation(flat, cur, forward) - if !ok { - return m, nil - } + step := 1 + if !forward { + step = -1 + } + for idx := startingFlatIndex(flat, cur, forward); idx >= 0 && idx < len(flat); idx += step { + target := flat[idx] nextModel, cmd, jumped := m.tryJumpToAnnotationTarget(&overlay.AnnotationTarget{ File: target.File, ChangeType: target.Type, @@ -41,7 +46,6 @@ func (m Model) handleAnnotNav(forward bool) (tea.Model, tea.Cmd) { if jumped { return nextModel, cmd } - cur = cursorAnnotKey{file: target.File, line: target.Line, typ: target.Type, onAnnot: true} } return m, nil } @@ -57,12 +61,18 @@ type cursorAnnotKey struct { onAnnot bool } -// currentAnnotKey returns the cursor's annotation-space key. The file-level -// annotation row (diffCursor == -1) maps to (file, 0, "") which matches -// the storage form of file-level annotations. ChangeDivider rows (which -// carry OldNum=NewNum=0) and out-of-range cursors collapse to line=-1 so -// forward navigation correctly treats any same-file annotation — including -// a file-level Line=0 — as strictly after the cursor. +// currentAnnotKey returns the cursor's annotation-space key. +// +// - File-level annotation row (diffCursor == -1) maps to (file, 0, "") +// which matches the storage form of file-level annotations. +// - Out-of-range cursor collapses to line=-1 so forward navigation treats +// any same-file annotation as strictly after. +// - ChangeDivider rows (which carry OldNum=NewNum=0) inherit the position +// of the nearest preceding non-divider line in the same file. This keeps +// middle/trailing dividers (reachable via mouse-click in the diff) from +// short-circuiting forward navigation back to the file-level annotation. +// A leading divider with no prior non-divider line falls back to line=-1 +// so the file-level annotation remains reachable from the top. func (m Model) currentAnnotKey() cursorAnnotKey { file := m.file.name if m.nav.diffCursor == -1 { @@ -73,33 +83,48 @@ func (m Model) currentAnnotKey() cursorAnnotKey { } dl := m.file.lines[m.nav.diffCursor] if dl.ChangeType == diff.ChangeDivider { - return cursorAnnotKey{file: file, line: -1, typ: "", onAnnot: false} + return m.dividerAnnotKey(file) } line := m.diffLineNum(dl) typ := string(dl.ChangeType) return cursorAnnotKey{file: file, line: line, typ: typ, onAnnot: m.store.Has(file, line, typ)} } -// pickAdjacentAnnotation walks the flat list and selects the neighbor of -// the cursor. When the cursor is exactly on an annotation, steps by index -// in the list (so same-line different-type annotations both step in turn). -// When the cursor sits between annotations, uses an insertion-point -// fallback over the (file, line) order. -func pickAdjacentAnnotation(flat []annotation.Annotation, cur cursorAnnotKey, forward bool) (annotation.Annotation, bool) { +// dividerAnnotKey returns the cursor's annotation-space key when the cursor +// sits on a ChangeDivider row. Walks back to the nearest preceding +// non-divider line and uses its line number, so forward navigation from a +// middle/trailing divider reaches the next annotation strictly after the +// divider's logical position rather than re-entering at the file-level +// annotation. A leading divider (no prior non-divider line) falls back to +// line=-1 so the file-level annotation stays reachable. +func (m Model) dividerAnnotKey(file string) cursorAnnotKey { + for i := m.nav.diffCursor - 1; i >= 0; i-- { + prev := m.file.lines[i] + if prev.ChangeType == diff.ChangeDivider { + continue + } + return cursorAnnotKey{file: file, line: m.diffLineNum(prev), typ: string(prev.ChangeType), onAnnot: false} + } + return cursorAnnotKey{file: file, line: -1, typ: "", onAnnot: false} +} + +// startingFlatIndex returns the index in flat from which the walker should +// begin attempting jumps. Forward: first index strictly after the cursor, +// or one past an exact-match index. Backward: mirror. Returns an +// out-of-range index (-1 or len(flat)) when the cursor is at the +// corresponding boundary — the loop exits immediately in that case. +func startingFlatIndex(flat []annotation.Annotation, cur cursorAnnotKey, forward bool) int { if idx, ok := exactAnnotIndex(flat, cur); ok { - return stepFlatList(flat, idx, forward) + if forward { + return idx + 1 + } + return idx - 1 } insIdx := annotInsertionPoint(flat, cur) if forward { - if insIdx >= len(flat) { - return annotation.Annotation{}, false - } - return flat[insIdx], true - } - if insIdx == 0 { - return annotation.Annotation{}, false + return insIdx } - return flat[insIdx-1], true + return insIdx - 1 } // exactAnnotIndex returns the flat-list index of an annotation that exactly @@ -118,21 +143,6 @@ func exactAnnotIndex(flat []annotation.Annotation, cur cursorAnnotKey) (int, boo return 0, false } -// stepFlatList returns flat[idx+1] for forward and flat[idx-1] for backward, -// with ok=false at the boundaries. -func stepFlatList(flat []annotation.Annotation, idx int, forward bool) (annotation.Annotation, bool) { - if forward { - if idx+1 < len(flat) { - return flat[idx+1], true - } - return annotation.Annotation{}, false - } - if idx > 0 { - return flat[idx-1], true - } - return annotation.Annotation{}, false -} - // annotInsertionPoint returns the index where the cursor would be inserted // in the flat list under (file, line) ordering — i.e. the index of the // first annotation strictly after the cursor, or len(flat) if all entries diff --git a/app/ui/annotnav_test.go b/app/ui/annotnav_test.go index aef24993..f24db37f 100644 --- a/app/ui/annotnav_test.go +++ b/app/ui/annotnav_test.go @@ -13,6 +13,19 @@ import ( "github.com/umputun/revdiff/app/ui/overlay" ) +// pickAdjacentAnnotation is a test-only wrapper that resolves the starting +// flat-list index via startingFlatIndex (the production walker entry point) +// and returns the annotation at that index. Production code walks indexes +// directly inside handleAnnotNav for O(N) navigation; this wrapper keeps +// the table-driven picking-algorithm tests intact as algorithm documentation. +func pickAdjacentAnnotation(flat []annotation.Annotation, cur cursorAnnotKey, forward bool) (annotation.Annotation, bool) { + idx := startingFlatIndex(flat, cur, forward) + if idx < 0 || idx >= len(flat) { + return annotation.Annotation{}, false + } + return flat[idx], true +} + func TestPickAdjacentAnnotation(t *testing.T) { a1 := annotation.Annotation{File: "a.go", Line: 5, Type: "+"} a2 := annotation.Annotation{File: "a.go", Line: 10, Type: "+"} @@ -189,7 +202,7 @@ func TestModel_CurrentAnnotKey(t *testing.T) { assert.False(t, key.onAnnot) }) - t.Run("ChangeDivider row collapses to line -1 so file-level annot stays reachable", func(t *testing.T) { + t.Run("leading ChangeDivider collapses to line -1 so file-level annot stays reachable", func(t *testing.T) { // dividers carry OldNum=NewNum=0; without the guard they would // alias the file-level annotation key and cause forward navigation // from the divider to skip the same-file file-level annotation. @@ -206,6 +219,94 @@ func TestModel_CurrentAnnotKey(t *testing.T) { assert.Empty(t, key.typ) assert.False(t, key.onAnnot) }) + + t.Run("middle ChangeDivider inherits position from preceding non-divider line", func(t *testing.T) { + // without this, a mouse-click on a middle divider would map to + // line=-1 and forward } would jump back to the file-level/first + // same-file annotation instead of to the next annotation after + // the divider's logical position. + divLines := []diff.DiffLine{ + {ChangeType: diff.ChangeAdd, Content: "first", OldNum: 0, NewNum: 5}, + {ChangeType: diff.ChangeDivider, Content: "⋯ 10 lines ⋯", OldNum: 0, NewNum: 0}, + {ChangeType: diff.ChangeAdd, Content: "later", OldNum: 0, NewNum: 16}, + } + dm := testModel([]string{"a.go"}, nil) + dm.file.name = "a.go" + dm.file.lines = divLines + dm.nav.diffCursor = 1 // on the divider + key := dm.currentAnnotKey() + assert.Equal(t, 5, key.line, "middle divider must inherit preceding line number") + assert.Equal(t, "+", key.typ, "middle divider must inherit preceding type") + assert.False(t, key.onAnnot) + }) + + t.Run("trailing ChangeDivider inherits position from preceding non-divider line", func(t *testing.T) { + divLines := []diff.DiffLine{ + {ChangeType: diff.ChangeAdd, Content: "added", OldNum: 0, NewNum: 5}, + {ChangeType: diff.ChangeDivider, Content: "⋯ 3 lines ⋯", OldNum: 0, NewNum: 0}, + } + dm := testModel([]string{"a.go"}, nil) + dm.file.name = "a.go" + dm.file.lines = divLines + dm.nav.diffCursor = 1 // trailing divider + key := dm.currentAnnotKey() + assert.Equal(t, 5, key.line) + assert.Equal(t, "+", key.typ) + }) + + t.Run("consecutive dividers walk back through them to reach a non-divider", func(t *testing.T) { + divLines := []diff.DiffLine{ + {ChangeType: diff.ChangeAdd, Content: "added", OldNum: 0, NewNum: 5}, + {ChangeType: diff.ChangeDivider, Content: "⋯ 3 lines ⋯", OldNum: 0, NewNum: 0}, + {ChangeType: diff.ChangeDivider, Content: "⋯ 2 lines ⋯", OldNum: 0, NewNum: 0}, + } + dm := testModel([]string{"a.go"}, nil) + dm.file.name = "a.go" + dm.file.lines = divLines + dm.nav.diffCursor = 2 // on the second of two consecutive dividers + key := dm.currentAnnotKey() + assert.Equal(t, 5, key.line, "must walk past intermediate divider to reach line 5") + }) +} + +// pin the divider middle/trailing fix at the integration level: clicking on +// a middle divider then pressing } must jump to the next annotation strictly +// after the divider's logical position, not back to the file-level/first +// same-file annotation. +func TestModel_HandleAnnotNav_FromMiddleDivider(t *testing.T) { + lines := []diff.DiffLine{ + {ChangeType: diff.ChangeAdd, Content: "L5", OldNum: 0, NewNum: 5}, + {ChangeType: diff.ChangeDivider, Content: "⋯ 10 lines ⋯", OldNum: 0, NewNum: 0}, + {ChangeType: diff.ChangeAdd, Content: "L16", OldNum: 0, NewNum: 16}, + {ChangeType: diff.ChangeAdd, Content: "L17", OldNum: 0, NewNum: 17}, + } + m := testModel([]string{"a.go"}, map[string][]diff.DiffLine{"a.go": lines}) + result, _ := m.Update(filesLoadedMsg{entries: []diff.FileEntry{{Path: "a.go"}}}) + m = result.(Model) + loadMsg := m.loadFileDiff("a.go")() + result, _ = m.Update(loadMsg) + m = result.(Model) + + // annotations: file-level, line 5 (before divider), line 17 (after divider) + m.store.Add(annotation.Annotation{File: "a.go", Line: 0, Type: "", Comment: "file"}) + m.store.Add(annotation.Annotation{File: "a.go", Line: 5, Type: "+", Comment: "before"}) + m.store.Add(annotation.Annotation{File: "a.go", Line: 17, Type: "+", Comment: "after"}) + + t.Run("forward from middle divider lands on next annotation past divider", func(t *testing.T) { + m.nav.diffCursor = 1 // middle divider + result, _ := m.handleAnnotNav(true) + model := result.(Model) + // expected: line 17 = index 3 + assert.Equal(t, 3, model.nav.diffCursor, "} from middle divider must jump past line 5 to line 17") + }) + + t.Run("backward from middle divider lands on annotation at-or-before logical position", func(t *testing.T) { + m.nav.diffCursor = 1 // middle divider + result, _ := m.handleAnnotNav(false) + model := result.(Model) + // expected: line 5 = index 0 + assert.Equal(t, 0, model.nav.diffCursor, "{ from middle divider must reach line 5") + }) } func TestModel_HandleAnnotNav_WithinFile(t *testing.T) { From 8f96d34712dc2789e430b173c7683451a615528d Mon Sep 17 00:00:00 2001 From: Umputun Date: Fri, 1 May 2026 02:14:57 -0500 Subject: [PATCH 3/4] fix(ui): land cursor on annotation comment row when jumping via @ or }/{ MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit positionOnAnnotation set diffCursor to the annotated diff line index but left m.annot.cursorOnAnnotation=false. The cursor render path checks both: the cursor highlight only moves to the annotation comment sub-row when cursorOnAnnotation is true. As a result, both @ popup selection and the new }/{ navigation visually placed the cursor on the diff line above the comment, one row off the actual annotation. Set cursorOnAnnotation=true after positioning, matching the invariant that j/k navigation already maintains when stepping onto an annotated line. File-level annotations (Line=0) are unaffected — diffCursor=-1 already represents the annotation row directly, so the flag stays false there. Skips the flag when the line is collapsed-hidden or a delete-only placeholder, where the annotation comment row does not render. --- app/ui/annotlist.go | 11 +++++- app/ui/annotnav_test.go | 88 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 98 insertions(+), 1 deletion(-) diff --git a/app/ui/annotlist.go b/app/ui/annotlist.go index 26ae2425..4542749d 100644 --- a/app/ui/annotlist.go +++ b/app/ui/annotlist.go @@ -78,8 +78,13 @@ func (m Model) tryJumpToAnnotationTarget(target *overlay.AnnotationTarget) (tea. } // positionOnAnnotation moves the cursor to the given annotation's line, re-renders, and centers the viewport. -// in collapsed mode, expands the hunk containing the target line so removed lines are visible. +// In collapsed mode, expands the hunk containing the target line so removed lines are visible. +// For line-level annotations the cursor lands on the annotation comment sub-row (cursorOnAnnotation=true), +// matching what `j`/`k` navigation produces when stepping onto an annotated line. File-level annotations +// (Line=0) use diffCursor=-1 which already represents the annotation row directly. Without this flag the +// cursor would land on the diff line above the comment, leaving navigation visually one row off the target. func (m *Model) positionOnAnnotation(a annotation.Annotation) { + m.annot.cursorOnAnnotation = false if a.Line == 0 { m.nav.diffCursor = -1 } else { @@ -87,6 +92,10 @@ func (m *Model) positionOnAnnotation(a annotation.Annotation) { if idx >= 0 { m.nav.diffCursor = idx m.ensureHunkExpanded(idx) + hunks := m.findHunks() + if !m.isCollapsedHidden(idx, hunks) && !m.isDeleteOnlyPlaceholder(idx, hunks) { + m.annot.cursorOnAnnotation = true + } } } m.layout.focus = paneDiff diff --git a/app/ui/annotnav_test.go b/app/ui/annotnav_test.go index f24db37f..f293682c 100644 --- a/app/ui/annotnav_test.go +++ b/app/ui/annotnav_test.go @@ -706,6 +706,94 @@ func TestModel_HandleAnnotNav_AllNonJumpableTerminates(t *testing.T) { assert.Nil(t, cmd) } +// jumping to a line-level annotation must land the cursor ON the annotation +// comment sub-row (cursorOnAnnotation=true), not on the diff line above it. +// The diff line and the comment sub-row render as separate visual rows; without +// cursorOnAnnotation the highlight sits one row above the comment, which is +// what users perceive as "cursor lands one line above the annotation." +// Both }/{ and the @ popup go through positionOnAnnotation, so this single +// invariant covers both navigation paths. +func TestModel_HandleAnnotNav_CursorLandsOnAnnotationRow(t *testing.T) { + lines := []diff.DiffLine{ + {ChangeType: diff.ChangeAdd, Content: "L1", OldNum: 0, NewNum: 1}, + {ChangeType: diff.ChangeAdd, Content: "L2", OldNum: 0, NewNum: 2}, + {ChangeType: diff.ChangeAdd, Content: "L3", OldNum: 0, NewNum: 3}, + } + m := testModel([]string{"a.go"}, map[string][]diff.DiffLine{"a.go": lines}) + result, _ := m.Update(filesLoadedMsg{entries: []diff.FileEntry{{Path: "a.go"}}}) + m = result.(Model) + loadMsg := m.loadFileDiff("a.go")() + result, _ = m.Update(loadMsg) + m = result.(Model) + m.store.Add(annotation.Annotation{File: "a.go", Line: 3, Type: "+", Comment: "target"}) + + t.Run("} sets cursorOnAnnotation when landing on a line-level annotation", func(t *testing.T) { + m.nav.diffCursor = 0 + m.annot.cursorOnAnnotation = false + result, _ := m.handleAnnotNav(true) + model := result.(Model) + assert.Equal(t, 2, model.nav.diffCursor, "cursor must be on the annotated diff line") + assert.True(t, model.annot.cursorOnAnnotation, "cursor must render on the annotation comment sub-row, not the diff line above it") + }) + + t.Run("{ sets cursorOnAnnotation when landing on a line-level annotation", func(t *testing.T) { + m.nav.diffCursor = 2 + m.annot.cursorOnAnnotation = false + // add a second annotation so { has somewhere to go + m.store.Add(annotation.Annotation{File: "a.go", Line: 1, Type: "+", Comment: "earlier"}) + result, _ := m.handleAnnotNav(false) + model := result.(Model) + assert.Equal(t, 0, model.nav.diffCursor) + assert.True(t, model.annot.cursorOnAnnotation, "cursor must render on the annotation comment sub-row") + }) +} + +// jumpToAnnotationTarget is the shared entry point for the @ popup's +// selection callback and for the }/{ walker. The same cursor-on-annotation +// invariant must apply to popup-driven jumps. +func TestModel_JumpToAnnotationTarget_CursorLandsOnAnnotationRow(t *testing.T) { + lines := []diff.DiffLine{ + {ChangeType: diff.ChangeAdd, Content: "L1", OldNum: 0, NewNum: 1}, + {ChangeType: diff.ChangeAdd, Content: "L2", OldNum: 0, NewNum: 2}, + } + m := testModel([]string{"a.go"}, map[string][]diff.DiffLine{"a.go": lines}) + result, _ := m.Update(filesLoadedMsg{entries: []diff.FileEntry{{Path: "a.go"}}}) + m = result.(Model) + loadMsg := m.loadFileDiff("a.go")() + result, _ = m.Update(loadMsg) + m = result.(Model) + m.store.Add(annotation.Annotation{File: "a.go", Line: 2, Type: "+", Comment: "popup target"}) + + m.annot.cursorOnAnnotation = false + result, _ = m.jumpToAnnotationTarget(&overlay.AnnotationTarget{File: "a.go", ChangeType: "+", Line: 2}) + model := result.(Model) + assert.Equal(t, 1, model.nav.diffCursor) + assert.True(t, model.annot.cursorOnAnnotation, "@ popup selection must land cursor on annotation comment row") +} + +// file-level annotations use diffCursor=-1 which already represents the +// annotation row directly. cursorOnAnnotation must NOT be set in that case +// (it applies only to line-level annotation sub-rows, not the file-level row). +func TestModel_HandleAnnotNav_FileLevelDoesNotSetCursorOnAnnotation(t *testing.T) { + lines := []diff.DiffLine{ + {ChangeType: diff.ChangeAdd, Content: "L1", OldNum: 0, NewNum: 1}, + } + m := testModel([]string{"a.go"}, map[string][]diff.DiffLine{"a.go": lines}) + result, _ := m.Update(filesLoadedMsg{entries: []diff.FileEntry{{Path: "a.go"}}}) + m = result.(Model) + loadMsg := m.loadFileDiff("a.go")() + result, _ = m.Update(loadMsg) + m = result.(Model) + m.store.Add(annotation.Annotation{File: "a.go", Line: 0, Type: "", Comment: "file-level"}) + + m.nav.diffCursor = 0 + m.annot.cursorOnAnnotation = false + result, _ = m.handleAnnotNav(false) // backward to file-level + model := result.(Model) + assert.Equal(t, -1, model.nav.diffCursor, "file-level position is diffCursor=-1") + assert.False(t, model.annot.cursorOnAnnotation, "file-level annotation row uses diffCursor=-1, not the cursorOnAnnotation flag") +} + func TestModel_HandleAnnotNav_DefaultBindings(t *testing.T) { km := keymap.Default() assert.Equal(t, keymap.ActionNextAnnotation, km.Resolve("}")) From 46a2222b4749f8abb6543e46a58d368bdb75b0b6 Mon Sep 17 00:00:00 2001 From: Umputun Date: Fri, 1 May 2026 02:29:41 -0500 Subject: [PATCH 4/4] docs: clarify cursorAnnotKey.onAnnot semantics MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The onAnnot field is set whenever the cursor's diff position matches an annotation in the store, regardless of whether the cursor visually sits on the diff line or on the annotation comment sub-row — both visual positions point to the same annotation conceptually. The previous wording ("exactly on an existing annotation row") suggested a stricter visual-row check that the implementation does not perform. Addresses Copilot review on PR #162. --- app/ui/annotnav.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/app/ui/annotnav.go b/app/ui/annotnav.go index 51a73416..a54f308c 100644 --- a/app/ui/annotnav.go +++ b/app/ui/annotnav.go @@ -51,9 +51,11 @@ func (m Model) handleAnnotNav(forward bool) (tea.Model, tea.Cmd) { } // cursorAnnotKey is the cursor's position in annotation space. onAnnot is -// true when the cursor is exactly on an existing annotation row (file, -// line, AND type all match). In that case navigation steps by index in the -// flat list. Otherwise navigation uses an insertion-point fallback. +// true when the cursor's diff position matches an annotation in the store +// (file, line, AND type), regardless of whether the cursor visually sits +// on the diff line or on the annotation comment sub-row — both point to +// the same annotation. In that case navigation steps by index in the flat +// list; otherwise it uses an insertion-point fallback. type cursorAnnotKey struct { file string line int