refactor: annotation visual-row chokepoint with row cache#183
Merged
Conversation
…epoint introduce annotationVisualRows as the single chokepoint for computing annotation visual-row layout (height + rendered lines). both renderWrappedAnnotation and wrappedAnnotationLineCount now delegate to this helper, eliminating drift between height-counting and rendering paths. results are cached per (line index, content, indent, wrap, gutter widths, viewport width) signature on the model, invalidated on file load and theme apply. byte-equivalent snapshot tests guard the rendered output against regressions. themeselect now exposes ApplyToColors so theme apply can flow through a single path that also drops the annotation row cache.
update ARCHITECTURE.md and CLAUDE.md with the annotation visual-row chokepoint invariant: renderWrappedAnnotation and wrappedAnnotationLineCount must agree, both routed through annotationVisualRows with a per-signature cache invalidated on file load and theme apply. archive the implementation plan under docs/plans/completed/.
There was a problem hiding this comment.
Pull request overview
Refactors annotation rendering in the TUI to route both “height calculation” and “painting” through a single visual-row chokepoint (annotationVisualRows), with memoization to avoid duplicated wrap/style work and explicit cache invalidation on file/theme lifecycle events.
Changes:
- Introduces
Model.annotationVisualRows(prefix, body) []stringas the single source of truth for annotation visual rows, and adds a(prefix, body, width)row-cache onannotationState. - Updates both the height query (
wrappedAnnotationLineCount) and painter (renderWrappedAnnotation, plus file-header rendering) to consume the chokepoint output, fixing empty-body paint/height desyncs. - Wires cache invalidation into file load and theme apply/preview cancel paths, and adds snapshot-style byte-equivalence tests plus targeted invalidation tests.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| docs/plans/completed/20260511-annotation-rowcache.md | Archived implementation plan and rationale for the chokepoint+cache refactor. |
| docs/ARCHITECTURE.md | Updates architecture notes to document the chokepoint and cache in annotate.go / annotationState. |
| CLAUDE.md | Adds/updates gotcha notes describing the chokepoint, cache keying, and invalidation responsibilities. |
| app/ui/themeselect.go | Invalidates annotation row-cache on theme apply and on canceling a preview session. |
| app/ui/themeselect_test.go | Adds regression test ensuring canceling theme preview clears annotation row-cache. |
| app/ui/model.go | Adds rowCache to annotationState and initializes it in NewModel. |
| app/ui/loaders.go | Clears annotation row-cache on file load. |
| app/ui/diffview.go | Refactors annotation painting to consume annotationVisualRows; fixes file-level empty-body header paint gating. |
| app/ui/diffview_test.go | Updates tests for the refactored renderWrappedAnnotation signature/behavior. |
| app/ui/annotate.go | Implements annotCacheKey, annotationPrefixBody, the chokepoint + composer, cache invalidation helper, and routes height calc through the chokepoint. |
| app/ui/annotate_test.go | Adds extensive unit coverage: cache behavior, invalidation hooks, empty-body cases, and byte-equivalence snapshot vs pre-refactor master output. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
extracts the structural piece from PR #174 (closed) that stood on its own merits: a single chokepoint for annotation rendering with a results cache. No UX change, no markdown, no new dependency.
what changed
Model.annotationVisualRows(prefix, body) []stringis now the single source of truth for "how many rows + what content does this annotation paint as." BothwrappedAnnotationLineCount(height query) andrenderWrappedAnnotation(painter) read from it, so the height-vs-paint invariant is self-enforced. Results memoize onannot.rowCachekeyed by(prefix, body, width); width changes self-invalidate via the key.Cache is invalidated from three sites:
handleFileLoaded,applyTheme, andcancelThemeSelect. The third one is load-bearing. The theme selector previews themes by callingapplyTheme(which invalidates), but cancel restores the resolver without invalidating, leaving cached rows baked with preview colors. The fix wiresinvalidateAnnotationRowsinto the cancel path too.byte-equivalent rendering
TestModel_AnnotationVisualRows_ByteEquivalentToMastercaptures pre-refactor master output for an input matrix (single-line, multi-line, wrap-needed, multi-segment wrap on continuation, file-level prefix, empty-body) and asserts the chokepoint produces the same bytes. Bytes were captured via a temp worktree at master before the refactor began, not regenerated from refactored code.latent bugs fixed in master
Two empty-body height/paint desyncs that existed in master and were surfaced by the chokepoint:
--annotationswith empty body:wrappedAnnotationLineCountreturned 1 but the painter emitted nothingrenderFileAnnotationHeader'sfileComment != ""gateBoth paths now route through the chokepoint and emit one prefix-only styled row, matching the reserved viewport row.
docs
CLAUDE.md "Annotation visual-row chokepoint" gotcha documents the chokepoint, cache key, three invalidation hooks, and warns that any future runtime color toggle must also call
invalidateAnnotationRows(). ARCHITECTURE.mdannotate.gorole andannotationStatefield listing updated.testing
make testgreen (91.3% total, 94.3% app/ui).make lintclean. Plan and full progress trail archived underdocs/plans/completed/20260511-annotation-rowcache.md.