Add visual range selection (V) and hunk annotation (H)#43
Add visual range selection (V) and hunk annotation (H)#43melonamin wants to merge 4 commits intoumputun:masterfrom
Conversation
Show author name and relative commit age per line in a gutter column, toggled with the B key. Blame data loads asynchronously via git blame and is keyed by new-side line numbers (blank for removed lines/dividers). - Add diff/blame.go with FileBlame parser and RelativeAge formatter - Add Blamer interface in ui/, wired via ModelConfig from cmd/main.go - Add blameGutter rendering in diffview.go alongside lineNumGutter - Extract shared gutter helpers (lineGutters, gutterExtra, gutterBlanks, applyHorizontalScroll) to reduce duplication across render paths - Add ActionToggleBlame keymap action bound to B - Update status bar mode icons to show @ when blame is active - Update README, docs, site, CLAUDE.md, and plugin references
Range annotations let users select a line range with V or target an entire hunk with H, then attach a single annotation to the whole region. Ranges render with ┌│└ gutter indicators shown live during selection and while typing. Annotations appear below the range end, support cursor stops for edit/delete, and output as file:start-end in structured format. - Extend Annotation store with EndLine field, overlap rejection, HasRangeCovering/GetRangeCovering interval lookups - Add ActionSelectRange (V) and ActionAnnotateHunk (H) to keymap - Wire selection mode with live ┌│└ gutter, cursor movement skips annotation stops during selection - Range annotation CRUD: create, edit, delete, pre-fill existing - Annotation list shows ranges as file:N-M with annotation color - Status bar shows ▋ icon and SEL: N lines during selection - Update README, site/docs.html, and plugin reference docs
…navigation visibleRangeIndices now breaks at ChangeDivider once a range has started, preventing line-number aliasing from leaking ranges into adjacent hunks. Add cursorOnRangeAnnotation field so point and range-end annotations on the same diff line are independently reachable via cursor movement, Enter, and delete. Cursor indicators and viewport Y calculation dispatch correctly for each sub-row.
Drop SelectionHighlight background items (gutter indicator used instead), correct hunk single-line detection description, add Task 8 for post-review bug fixes.
umputun
left a comment
umputun
left a comment
There was a problem hiding this comment.
the implementation is solid, good tests, clean data model. but I have a fundamental concern about whether this is worth the complexity.
do we need range/hunk annotations? the current single-line annotation already gives enough context. when someone writes ## file.go:43 (+) refactor this block, the AI reading the annotations can see the surrounding code and infer the scope. I don't see a real use case where pointing at a specific line isn't sufficient. this adds ~2000 lines (including tests) for what seems like a nice-to-have.
as a user I love how the visual selection looks, it's a nice UX. but as a maintainer, 2k lines of new code to maintain for a feature without a clear problem statement is a concern.
I'd like to understand what problem you ran into that line annotations couldn't solve. if there's a concrete use case I'm missing, happy to reconsider.
if we do proceed, a few things to fix:
theme/bundled.gohas 5 stray hex values without key names (bare#44475aetc.). they're parsed as comments and silently ignored, soSelectedBgis never set in any bundled theme. these need to be propercolor-selected-bg = #hexentries, or removed if the selection background approach was dropped in favor of gutter indicators- whitespace-only alignment changes in
cmd/revdiff/main.go,theme/theme.go,ui/styles.gothat aren't related to the feature. pls revert those A(file-level annotation) should be blocked during selection mode, like point annotation is
There was a problem hiding this comment.
Pull request overview
Adds range-based annotations to the TUI, including a visual selection mode (V) and a one-key “annotate hunk” shortcut (H), plus rendering/cursoring support and updated docs/tests.
Changes:
- Implement visual range selection and hunk-wide annotation workflows in the diff pane.
- Add range annotation persistence/rendering (gutter + end-of-range comment row) and cursor navigation across point/range sub-rows.
- Update keybindings/help/docs and expand test coverage across UI, collapsed mode, and annotation store.
Reviewed changes
Copilot reviewed 18 out of 21 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| ui/styles.go | Minor struct field alignment change for search colors. |
| ui/search.go | Clears new range-cursor state when jumping between search matches. |
| ui/model.go | Adds selection/range state to the model; hooks new actions into navigation/status rendering. |
| ui/model_test.go | Updates tests for new store API + adds extensive selection/range/hunk behavior tests. |
| ui/diffview.go | Implements range gutter + range annotation rendering; updates wrapping and cursor nav to account for range rows. |
| ui/collapsed.go | Threads range gutter/rendering through collapsed diff rendering paths. |
| ui/collapsed_test.go | Adds coverage for collapsed rendering/wrapping with range annotations. |
| ui/annotlist.go | Formats range annotations in the list and supports jumping to ranges. |
| ui/annotlist_test.go | Adds test for range formatting in annotation list. |
| ui/annotate.go | Implements selection mode, range annotation lifecycle, hunk annotation, and range-aware delete/edit behavior. |
| theme/theme.go | Minor formatting change in optional color keys map. |
| theme/bundled.go | Adds trailing theme comment lines (currently appear to be accidental/ineffective). |
| site/docs.html | Documents V/H and range output format. |
| README.md | Documents V/H and range output format. |
| keymap/keymap.go | Adds new actions and default bindings for V and H. |
| keymap/keymap_test.go | Updates default binding expectations for new actions. |
| docs/plans/2026-04-06-range-hunk-annotations.md | Adds implementation plan / checklist for the feature. |
| cmd/revdiff/main.go | Minor formatting around search color flag fields/mapping. |
| annotation/store.go | Extends annotations with EndLine, adds range overlap logic, and updates delete identity. |
| annotation/store_test.go | Adds/updates tests for new range annotation semantics and store API. |
| .claude-plugin/skills/revdiff/references/usage.md | Updates usage docs with V/H and range output format. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| color-status-bg = #bd93f9 | ||
| color-search-fg = #282a36 | ||
| color-search-bg = #f1fa8c | ||
| #44475a |
There was a problem hiding this comment.
The trailing #44475a line is treated as a comment by theme parsing and therefore has no effect. If this was meant to change a color value (e.g. color-selected-bg), it needs to be a proper key = value entry; otherwise it should be removed to avoid shipping confusing theme files.
| #44475a |
| color-status-bg = #88c0d0 | ||
| color-search-fg = #2e3440 | ||
| color-search-bg = #ebcb8b | ||
| #3b4252 |
There was a problem hiding this comment.
The trailing #3b4252 line is treated as a comment by theme parsing and therefore has no effect. If this was meant to change a color value, it needs to be a proper key = value entry; otherwise it should be removed to avoid shipping confusing theme files.
| #3b4252 |
| color-status-fg = #002b36 | ||
| color-status-bg = #b58900 | ||
| color-search-fg = #002b36 | ||
| color-search-bg = #cb4b16 | ||
| #073642 | ||
| ` |
There was a problem hiding this comment.
The trailing #073642 line is treated as a comment by theme parsing and therefore has no effect. If this was meant to change a color value, it needs to be a proper key = value entry; otherwise it should be removed to avoid shipping confusing theme files.
| color-status-fg = #1e1e2e | ||
| color-status-bg = #89b4fa | ||
| color-search-fg = #1e1e2e | ||
| color-search-bg = #f9e2af | ||
| #313244 | ||
| ` |
There was a problem hiding this comment.
The trailing #313244 line is treated as a comment by theme parsing and therefore has no effect. If this was meant to change a color value, it needs to be a proper key = value entry; otherwise it should be removed to avoid shipping confusing theme files.
| color-status-bg = #fabd2f | ||
| color-search-fg = #282828 | ||
| color-search-bg = #fe8019 | ||
| #3c3836 |
There was a problem hiding this comment.
The trailing #3c3836 line is treated as a comment by theme parsing and therefore has no effect. If this was meant to change a color value, it needs to be a proper key = value entry; otherwise it should be removed to avoid shipping confusing theme files.
| #3c3836 |
| // styledRangeGutter returns the styled range gutter indicator for a diffLines index. | ||
| func (m Model) styledRangeGutter(idx int, ranges []rangeRenderInfo) string { | ||
| gutter := rangeGutterFor(idx, ranges) | ||
| if gutter == "" { | ||
| if len(ranges) > 0 { | ||
| return strings.Repeat(" ", rangeGutterWidth) | ||
| } | ||
| return "" | ||
| } |
There was a problem hiding this comment.
styledRangeGutter only reserves rangeGutterWidth spaces when len(ranges) > 0. Other parts of rendering/measurement (e.g. wrappedLineCount via hasRangeGutter) can decide a range gutter is “active” even when ranges is empty (e.g. stored ranges that don't map to visible indices), which can desync wrapping/cursor calculations. Consider deriving a single showRangeGutter boolean and using it consistently for both width reservation and wrap-width calculations.
| rgBlank := "" | ||
| if len(ranges) > 0 { | ||
| rgBlank = strings.Repeat(" ", rangeGutterWidth) | ||
| } | ||
| wrapWidth := m.diffContentWidth() - wrapGutterWidth - m.gutterExtra() | ||
| if len(ranges) > 0 { |
There was a problem hiding this comment.
renderWrappedDiffLine adjusts wrapWidth based on len(ranges) > 0, but wrappedLineCount adjusts based on hasRangeGutter(). These predicates can diverge (e.g. range annotations exist in the store but buildRangeAnnotations returns none), causing wrap rendering and wrap measurement to disagree. Use a single predicate for both code paths so wrapped-line counts stay in sync with actual rendering.
| rgBlank := "" | |
| if len(ranges) > 0 { | |
| rgBlank = strings.Repeat(" ", rangeGutterWidth) | |
| } | |
| wrapWidth := m.diffContentWidth() - wrapGutterWidth - m.gutterExtra() | |
| if len(ranges) > 0 { | |
| showRangeGutter := m.hasRangeGutter() | |
| rgBlank := "" | |
| if showRangeGutter { | |
| rgBlank = strings.Repeat(" ", rangeGutterWidth) | |
| } | |
| wrapWidth := m.diffContentWidth() - wrapGutterWidth - m.gutterExtra() | |
| if showRangeGutter { |
| _, textContent, _ := m.prepareLineContent(idx, dl) | ||
| wrapWidth := m.diffContentWidth() - wrapGutterWidth - m.gutterExtra() | ||
| if m.hasRangeGutter() { | ||
| wrapWidth -= rangeGutterWidth | ||
| } | ||
| return len(m.wrapContent(textContent, wrapWidth)) |
There was a problem hiding this comment.
wrappedLineCount calls hasRangeGutter() (which scans the store) for every line measured. This is on hot paths like cursorViewportY() and can become O(lines × annotations). Consider computing the “range gutter active” boolean once per render/frame and passing it into wrappedLineCount, or caching it on the model for the duration of a render.
| // isRangeAnnotationEnd returns true if the given diffLines index is the end of a saved range annotation. | ||
| func (m Model) isRangeAnnotationEnd(idx int) bool { | ||
| for _, r := range m.buildRangeAnnotations() { | ||
| if r.endIdx == idx { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } | ||
|
|
There was a problem hiding this comment.
isRangeAnnotationEnd rebuilds the full range annotation mapping (buildRangeAnnotations + visibleRangeIndices scan) on every cursor-move check. Since this is called from moveDiffCursorUp/Down, it can add noticeable latency on large diffs. Consider caching range end indices (e.g. a map[endIdx]bool) alongside the precomputed ranges used for rendering, and have navigation use that cached structure instead of recomputing.
| // isRangeAnnotationEnd returns true if the given diffLines index is the end of a saved range annotation. | |
| func (m Model) isRangeAnnotationEnd(idx int) bool { | |
| for _, r := range m.buildRangeAnnotations() { | |
| if r.endIdx == idx { | |
| return true | |
| } | |
| } | |
| return false | |
| } | |
| type rangeAnnotationEndCacheEntry struct { | |
| signature string | |
| ends map[int]bool | |
| } | |
| var rangeAnnotationEndCache = map[string]rangeAnnotationEndCacheEntry{} | |
| // rangeAnnotationCacheSignature returns a stable signature for the current file's saved annotations. | |
| // The cache is rebuilt whenever the current file or its annotations change. | |
| func (m Model) rangeAnnotationCacheSignature() string { | |
| anns := m.store.Get(m.currFile) | |
| sig := m.currFile | |
| for _, a := range anns { | |
| sig += fmt.Sprintf("|%d|%d|%s", a.Line, a.EndLine, a.Text) | |
| } | |
| return sig | |
| } | |
| // rangeAnnotationEnds returns a cached set of diffLines indices that terminate saved range annotations. | |
| func (m Model) rangeAnnotationEnds() map[int]bool { | |
| sig := m.rangeAnnotationCacheSignature() | |
| if entry, ok := rangeAnnotationEndCache[m.currFile]; ok && entry.signature == sig { | |
| return entry.ends | |
| } | |
| ends := make(map[int]bool) | |
| for _, r := range m.buildRangeAnnotations() { | |
| ends[r.endIdx] = true | |
| } | |
| rangeAnnotationEndCache[m.currFile] = rangeAnnotationEndCacheEntry{ | |
| signature: sig, | |
| ends: ends, | |
| } | |
| return ends | |
| } | |
| // isRangeAnnotationEnd returns true if the given diffLines index is the end of a saved range annotation. | |
| func (m Model) isRangeAnnotationEnd(idx int) bool { | |
| return m.rangeAnnotationEnds()[idx] | |
| } |
In the last few days, I ran into a problem when the agent got confused and didn't do what I meant. Most likely, this is a skill issue on my end. My hunch, based on my experience building agents, is to provide as much context as possible and to be as specific as possible; that's why I decided to prototype this feature. Since I just built it, I have no idea if it fits into my workflow yet. I like how it turned out visually, but I didn't expect that it would be THAT complex, so I've been second-guessing as well. How about this - let it hang for a week; if I find a strong use case, I will report back. Meanwhile, maybe you or others will play with it too. |
sounds good to me |
|
btw, had an idea for a much simpler alternative that might cover the main use case. instead of visual selection, detect keywords like "hunk" or "block" in the annotation text and automatically expand the output to include the hunk range. so if a user writes the hunk boundary detection already exists ( not saying your PR should be closed, just thinking out loud about the cost/benefit tradeoff. |
|
btw, I went ahead and tried the simpler alternative I mentioned earlier. merged in #47, ended up being ~50 lines of actual logic. if the annotation text contains the word "hunk", the output header automatically expands to include the line range, e.g. wanted to see how far the simple approach goes first, and the added code seems worth keeping even if the use case isn't fully clear yet. |
Summary
V) — select a span of diff lines, confirm with Enter to annotate the rangeH) — one-key annotation of the entire hunk under the cursorDemo
screenrecording-2026-04-06_17-28-46.mp4