Configurable annotation marker#185
Conversation
umputun
left a comment
There was a problem hiding this comment.
few findings, biggest one is a width regression:
-
input width drops 2 cols at
app/ui/annotate.go:100,160. Old constants6/12were1 (cursor) + prefix_width + 2 (border margin)per the original comments, the new1 + lipgloss.Width(prefix)drops the+2. With the default marker the input is now 4/10 instead of 6/12, two columns wider than before, can bleed into the right padding on narrow panes. Either restore the+2(2 + lipgloss.Width(...)) or document why the margin is no longer needed. Worth adding a regression test that pins absolute input width for the default marker plus at least one wide and one empty marker so this kind of drift surfaces in CI. -
testifylint will fail CI at
app/config_test.go:427,454.assert.Equal(t, "", opts.AnnotationMarker)should beassert.Empty(t, opts.AnnotationMarker). -
gofmt struct-field alignment at
app/main.go:206-213,app/ui/model_test.go:148-160,app/ui/annotate_test.go:510-520, 546-558. InsertingAnnotationMarker:widened the longest key but the surrounding fields were not re-aligned.make fmtcleans it up. -
stale test comment at
app/ui/annotate_test.go:1137."(12-6=6)"references the old hardcoded constants. The numerical result is still 6 under the new math (10-4=6), but the parenthetical no longer matches what is actually computed. -
weak assertion in
TestModel_EmptyAnnotationMarkerExplicit.assert.Contains(t, rendered, " bare note", ...)would pass even if a hardcoded" 💬 "fallback regressed since the substring would still appear after the prefix. Anchor with a regex or full-row equality on the prefix. -
per-render alloc in
annotPrefix()/annotFilePrefix(). Both concat a new string on every call from hot paths (composeAnnotationRows,annotationContinuationIndent,annotationPrefixBody,renderAnnotationOrInput,renderFileAnnotationHeader). Marker is immutable post-NewModel, would be cheaper to cache once onmodelConfigStatealongsideannotationMarker.
There was a problem hiding this comment.
Pull request overview
This PR adds a configurable annotation marker so users can replace the hardcoded 💬 prefix with a custom string via --annotation-marker (env REVDIFF_ANNOTATION_MARKER, config annotation-marker) while keeping the existing default behavior.
Changes:
- Introduces the
--annotation-markeroption (CLI/env/config) and wires it throughmain -> ui.ModelConfig -> ui.Model. - Replaces hardcoded annotation marker render strings with
annotPrefix()/annotFilePrefix()helpers and adjusts width/indent logic usinglipgloss.Width(). - Updates documentation and adds/extends tests for default, custom, and empty markers.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| site/docs.html | Documents the new --annotation-marker option in the generated options table. |
| README.md | Adds the new flag/env var to the CLI options table. |
| plugins/codex/skills/revdiff/references/config.md | Documents the new flag/env/config key in the reference table. |
| .claude-plugin/skills/revdiff/references/config.md | Mirrors config reference documentation update for the claude plugin. |
| app/ui/model.go | Extends ModelConfig / internal config state to carry AnnotationMarker into UI rendering. |
| app/ui/model_test.go | Updates the shared test model factory to set a default annotation marker. |
| app/ui/diffview.go | Switches annotation rendering call sites to use the new prefix helpers; updates continuation indent logic. |
| app/ui/annotate.go | Adds annotPrefix() / annotFilePrefix() helpers and updates annotation input width calculation to use lipgloss.Width(). |
| app/ui/annotate_test.go | Adds coverage for custom and empty annotation markers and updates imports/config for NewModel. |
| app/main.go | Plumbs opts.AnnotationMarker into ui.ModelConfig. |
| app/config.go | Adds the new CLI/env/config option with default 💬. |
| app/config_test.go | Adds tests covering flag/env/config parsing for the new option (including explicit empty). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func (m Model) annotationContinuationIndent(firstLogicalLine string) string { | ||
| switch { | ||
| case strings.HasPrefix(firstLogicalLine, "\U0001f4ac file: "): | ||
| return strings.Repeat(" ", lipgloss.Width("\U0001f4ac file: ")) | ||
| case strings.HasPrefix(firstLogicalLine, "\U0001f4ac "): | ||
| return strings.Repeat(" ", lipgloss.Width("\U0001f4ac ")) | ||
| case strings.HasPrefix(firstLogicalLine, m.annotFilePrefix()): | ||
| return strings.Repeat(" ", lipgloss.Width(m.annotFilePrefix())) | ||
| case strings.HasPrefix(firstLogicalLine, m.annotPrefix()): | ||
| return strings.Repeat(" ", lipgloss.Width(m.annotPrefix())) |
| 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"` | ||
| AnnotationMarker string `long:"annotation-marker" ini-name:"annotation-marker" env:"REVDIFF_ANNOTATION_MARKER" default:"💬" description:"prefix shown before annotation lines"` |
31ac16e to
d0265b0
Compare
umputun
left a comment
There was a problem hiding this comment.
all 6 round-1 items fixed, verified locally. Tests green, lint 0 issues.
two copilot inline comments from round 1 still worth a look:
-
indent inference ambiguity at
app/ui/diffview.go:701-710. Real edge-case bug.annotationContinuationIndentusesHasPrefix(firstLogicalLine, m.annotFilePrefix())to pick indent width, but a line-level annotation whose body starts withfile:produces💬 file: ...and matches the file-prefix branch, so continuation rows get the 9-space indent instead of 3. Triggers on multi-line line-annotations only. Worse with empty marker sinceannotFilePrefixbecomes" file: "and any body starting withfile:mis-matches. Fix is trivial sincecomposeAnnotationRowsalready hasprefixin scope: drop theHasPrefixswitch and just doindent := strings.Repeat(" ", lipgloss.Width(prefix))inline, then either inline-deleteannotationContinuationIndentor have it takeprefixdirectly. -
no newline/control-char validation on
--annotation-marker. A\n/\r/\tinREVDIFF_ANNOTATION_MARKER(config file or env) would break layout silently. Nit, worst case is the user breaks their own rendering, but astrings.ContainsAny(marker, "\n\r\t")check at parse time with a clear error costs nothing.
re #5 (the empty-marker assertion): the assert.NotContains(rendered, "💬") you added catches a hardcoded emoji fallback regression, which was the actual worry. Fine as-is.
|
Added fixes and tests for those copilot catches, thanks! |
post-merge cleanup for #185: - modelConfigState.annotationMarker was stored but never read; only the cached annotPrefix/annotFilePrefix variants are used at call sites. - the validation in parseArgs rejects \n/\r/\t but the error message only mentioned newlines and tabs. broadened to "control characters" so the wording covers CR too.
Configurable annotation marker
Adds
--annotation-marker(env:REVDIFF_ANNOTATION_MARKER, config:annotation-marker) to let users swap the💬prefix for something else. Defaults to the emoji so nothing changes for existing setups.All five render sites now go through
annotPrefix()/annotFilePrefix()helpers instead of hardcoding the emoji. Input width and wrap-continuation indent uselipgloss.Width()so everything stays aligned regardless of marker width.The marker is render-only — structured output still uses
## path:N (T)headers, so saved annotations stay portable between users with different markers. Flag lives on top-leveloptionsrather thanColorssince it's not a colour value.Empty string is a valid choice:
--annotation-marker ""gives you a bare space prefix. The CLI default tag handles "not set", so empty only happens when you ask for it explicitly.SKILL.md unchanged — this is a user-preference flag like
--themeor--wrap, not something agents should auto-derive from context. Users set it in their config or env once.Closes #184