Skip to content

feat: annotatable diff view#300

Closed
backnotprop wants to merge 6 commits intomainfrom
feat/diff-annotate
Closed

feat: annotatable diff view#300
backnotprop wants to merge 6 commits intomainfrom
feat/diff-annotate

Conversation

@backnotprop
Copy link
Copy Markdown
Owner

@backnotprop backnotprop commented Mar 15, 2026

Summary

Adds annotation support to the plan diff view using block-level hover. Users hover over added/removed/modified diff sections and annotate entire blocks — same pattern as code block hover in the normal Viewer.

What we tried and why we pivoted

Attempt 1: Text-selection via web-highlighter. Copied ~250 lines of annotation plumbing from Viewer.tsx into PlanCleanDiffView. Worked for basic cases but was fragile — findTextInDOM broke across diff boundaries, unchanged text got orphaned on toggle, cross-section selections picked wrong contexts, and highlights vanished on re-render due to unstable callback dependencies.

Attempt 2: Extracted useAnnotationHighlighter hook. Deduplicated the shared logic between Viewer and PlanCleanDiffView. Fixed the re-render teardown bug and added auto-restore for persisted annotations. But the core fragility remained — text-search restoration in a fragmented diff DOM is inherently unreliable.

Final approach: Block-level hover annotation. Replaced text-selection entirely. Hovering a diff section shows the annotation toolbar anchored to that block. No web-highlighter, no DOM marks, no findTextInDOM. Annotations live purely in React state. Delete just removes from state. Toggle is trivial — nothing to restore.

What shipped

  • Block-level diff annotation — hover added/removed/modified sections to annotate
  • useAnnotationHighlighter hook — extracted from Viewer.tsx, used by Viewer only (not diff view)
  • View isolation — diff annotations filtered to diff view, normal annotations filtered to normal view, no bleed-through
  • diff badge in AnnotationPanel for all diff annotations
  • [In diff content] label in exported feedback
  • diffContext field on Annotation type, flows through sharing and draft persistence

Known limitations

  • Pinpoint input mode not integrated in diff view
  • No scroll-to-selected for diff annotations (clicking in panel doesn't scroll to the block)
  • Unchanged sections are not annotatable (by design — nothing changed)

Test plan

  • Enter diff mode → hover a green (added) section → toolbar appears at top-right
  • Click Comment → popover opens → submit → annotation in panel with diff badge
  • Hover a red (removed) section → same flow
  • Modified section: hover old half and new half independently
  • Unchanged section: no toolbar on hover
  • Delete annotation from panel → removed cleanly (no stale DOM marks)
  • Toggle diff off/on → annotations persist in panel, no highlights to restore
  • Normal annotations don't appear in diff mode; diff annotations don't appear in normal mode
  • Deny with diff annotations → feedback includes [In diff content] labels
  • Toolstrip modes (Redline, Comment, Quick Label) all work in diff view
  • bun run build:hook → production build succeeds

🤖 Generated with Claude Code

backnotprop and others added 4 commits March 15, 2026 11:57
Add annotation support to the plan clean diff view so users can
select text in added/removed/modified sections and leave feedback.
Annotations carry a diffContext field that flows through export,
sharing, and drafts.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…bugs

Extract ~250 lines of duplicated annotation plumbing from Viewer.tsx and
PlanCleanDiffView.tsx into a shared useAnnotationHighlighter hook. Fixes
two bugs from the original copy-paste: highlights vanishing on re-render
(unstable callback in useEffect deps) and persisted annotations not
restoring into DOM (missing findTextInDOM path). Diff annotations are
explicitly excluded from text-based restoration to avoid wrong-match
binding. Also fixes code-block annotation deletion not re-applying
syntax highlighting (data-highlighted attribute and execution order).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Route annotation deletions through diffViewerRef when diff mode is active
(fixes stale marks). Filter annotations by diffContext so diff annotations
stay in diff view and normal annotations stay in normal view (fixes
bleed-through both directions). Simplify diff badge to neutral "diff"
label. Simplify export label to "[In diff content]". Add unchanged to
diffContext union and resolveDiffContext match. Fix code-block deletion
syntax re-highlighting order.

Known limitations: unchanged-text annotations don't reliably survive
diff toggle, cross-section selections pick one context tag, pinpoint
mode not integrated in diff view.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace fragile text-selection annotation in diff mode with block-level
hover annotation. Hovering added/removed/modified sections shows the
annotation toolbar (same pattern as code block hover in Viewer). No
web-highlighter, no DOM marks, no findTextInDOM restoration. Annotations
live purely in React state — delete just removes from state, toggle
preserves annotations in panel, no stale marks possible.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@backnotprop
Copy link
Copy Markdown
Owner Author

Code review

No issues found above the confidence threshold. Checked for bugs, CLAUDE.md compliance, and dead/stale code from refactoring.

Note: Several cleanup opportunities were identified at moderate confidence (scored 75/100 each, below the 80 threshold for flagging). These are all dead code from the multi-iteration refactoring on this branch — not bugs, but worth a cleanup pass:

  • resolveContext extension point in useAnnotationHighlighter has no callers (designed for text-selection iteration, superseded by block-level hover)
  • Hook header comment and CLAUDE.md both claim the hook is shared with PlanCleanDiffView, which no longer uses it
  • 'unchanged' variant in diffContext type union is never set (unchanged blocks have no hover handlers)
  • No-op useEffect for scroll-to-selected in PlanCleanDiffView (runs a DOM query, does nothing with the result)
  • CLAUDE.md says per-context labels like [In newly added content] but code emits generic [In diff content]
  • annotations prop accepted by PlanDiffViewer but never passed through (silently dropped)

🤖 Generated with Claude Code

- If this code review was useful, please react with 👍. Otherwise, react with 👎.

backnotprop and others added 2 commits March 15, 2026 17:32
PlanCleanDiffView no longer uses this hook (switched to block-level
hover). Remove the resolveContext option, ref, sync effect, and call
site — no consumer passes it. Update header comment to reflect the
hook is Viewer-only.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…tions

- Remove no-op scroll-to-selected useEffect
- Add hoverTimeoutRef cleanup on unmount
- Deduplicate toolbar onMouseLeave (call handleLeave)
- Remove dead annotations prop from PlanDiffViewer
- Remove as any cast + stale isGlobal field from hook
- Remove unused 'unchanged' from diffContext type
- Filter diff annotations from share/draft restore paths
- Fix stale CLAUDE.md claims

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant