fix(highlights): address post-merge review of line highlights

.changeset/line-highlight-review-followups.md

@@ -0,0 +1,5 @@
+---
+"hunkdiff": patch
+---
+
+Fix extension line highlights: marks now paint per file as they resolve instead of waiting for every file, never paint a previous review's offsets onto a reloaded file, stay visible on transparent line backgrounds, keep every active file's result retained however large the review is, and paint a row carrying thousands of ranges in milliseconds instead of seconds. Marks still resolve their tint against an assumed background on transparent cells, and a range covering only zero-width characters paints nothing.

docs/extensions.md

@@ -1054,8 +1054,8 @@ callback failure warns without breaking the review.
 A line highlighter marks **character ranges inside Hunk's own diff rendering**
 — syntax highlighting, word diff, and layout stay exactly as they are, and the
 marked characters get a resolved background. It is the lever for search hits,
-diagnostics mapped onto changed lines, secret or bidi-character scanning,
-coverage, and anything else that wants to say “these exact characters, here.”
+diagnostics mapped onto changed lines, secret scanning, coverage, and anything
+else that wants to say “these exact characters, here.”
 For a whole alternate presentation, use `registerFileView` instead.
 
 ```ts
@@ -1094,7 +1094,9 @@ That addressing survives split vs stack layout, line wrapping, horizontal
 scrolling, and collapsed-context expansion, and the extension never learns
 Hunk's row model. A context line may be addressed through either side's line
 number; split view mirrors the mark onto both halves of the row. Offsets that
-land inside an emoji or a wide character widen outward to the whole glyph.
+land inside an emoji or a wide character widen outward to the whole glyph. A
+mark paints terminal columns, so a range covering only characters that occupy
+no column — bidi controls, zero-width spaces and joiners — paints nothing.
 
 The `tone` says what a mark means — `"match"` (the default), `"current"` for
 the one hit a search is standing on, `"info"`, `"warning"`, or `"error"`.
@@ -1104,9 +1106,12 @@ invisible on an added line's green. Hunk resolves each tinted tone against the
 actual background of each marked line until it clears a minimum perceptual
 distance — stronger than its own word-diff emphasis, backing off only before
 the code on top would stop being readable — per theme, so a mark is never
-invisible. `"current"` renders as reverse video (theme text as the block,
-theme background as the glyphs), the convention `less` and vim use for the
-active hit.
+invisible on a line kind or a theme. On a transparent cell there is no color to
+blend against, so resolution falls back to the theme background and then to the
+appearance's own extreme: the mark still paints, chosen against the surface
+Hunk assumes rather than the one behind the terminal. `"current"` renders as
+reverse video (theme text as the block, theme background as the glyphs), the
+convention `less` and vim use for the active hit.
 
 `highlight({ file, signal, readDocument })` may be sync or async, and returns
 the complete set of marks for one file — or `null` for none. Hunk calls it per
@@ -1134,9 +1139,11 @@ Containment matches the rest of the system. Marks addressing lines the review
 is not showing — inside a collapsed gap, absent from a partial patch — are
 silently invisible rather than errors; expanding a gap reveals marks addressed
 into it when the file's source is loaded. Structurally invalid entries are
-dropped with one warning per file; a result larger than 2,000 ranges per file
-or 100 per line is rejected whole rather than truncated silently; overlapping
-ranges resolve deterministically with the later range winning. A throwing,
+dropped with one warning per file; a raw result longer than 10,000 entries, or
+one larger than 2,000 ranges per file or 100 per line, is rejected whole rather
+than truncated silently; a highlighter whose marks would push one file past
+4,000 merged ranges across every highlighter is dropped for that file;
+overlapping ranges resolve deterministically with the later range winning. A throwing,
 rejecting, or timed-out `highlight` costs that file's marks from that
 highlighter and nothing else. Because highlights are paint-only — they change
 colors, never text or geometry — the failure mode is always “no marks,” never

docs/line-highlights-proposal.md

@@ -97,9 +97,12 @@ Tinted tone anchors map to existing semantic theme tokens (`accent`,
 above word diff's, backing off only where the code on top would stop being
 readable. `current` renders as reverse video — theme text as the block, theme
 background as the glyphs — the `less`/vim convention for the active hit, which
-sidesteps the tint-versus-readability tug-of-war entirely. Surfaces that
-cannot take a blend (transparent, non-hex) decline the mark — the same
-degradation word diff uses.
+sidesteps the tint-versus-readability tug-of-war entirely. A transparent cell
+has no color to blend against, so resolution falls back to the theme's
+background and then to the appearance's extreme: the mark stays visible, but
+its distance floor is measured against an assumed surface rather than the one
+the terminal actually shows. Only a theme whose own colors cannot take a blend
+(non-hex) declines the mark — the same degradation word diff uses.
 
 ## Where it plugs in
 
@@ -214,10 +217,13 @@ Search is the weakest justification on this list.
   here" during review rather than in CI later.
 - **Secret scanning.** Mark the exact token that tripped a rule, instead of
   naming the line in a pane.
-- **Trojan-source defense.** Bidi control characters, homoglyphs, and
-  zero-width characters are invisible _by construction_ in a diff viewer.
-  Marking those ranges is a security feature a review tool is uniquely placed
-  to offer.
+- **Homoglyph marking.** Marking the confusable identifier characters that read
+  as ASCII but are not. (Trojan-source defense more broadly does _not_ work
+  yet, and listing it here was a mistake: a bidi control or zero-width
+  character occupies no terminal column, so a range covering only those
+  characters resolves to zero columns and paints nothing. Making invisible
+  characters visible needs a separate mechanism — a gutter or margin marker, or
+  rendering a visible stand-in glyph — that this API does not have.)
 - **Coverage.** Uncovered added ranges, straight from an lcov file.
 - **Provenance.** Which ranges an agent wrote versus a human — squarely in
   Hunk's stated purpose of understanding coding-agent changesets.
@@ -233,6 +239,14 @@ Search is the weakest justification on this list.
   behavior?" could light up the ranges in the user's terminal as it explains.
   Nothing else in the ecosystem can do that.
 
+## Follow-ups
+
+- **Zero-width ranges are silently invisible.** A mark covering only characters
+  that occupy no terminal column (bidi controls, ZWSP, ZWJ) resolves to an
+  empty column range and paints nothing. It is the one case where a valid mark
+  on a rendered line still shows nothing, and it wants its own mechanism rather
+  than a background tint.
+
 ## Companion gap (separate PR)
 
 `ctx.navigation` reaches files and hunks; the finest target is `selectHunk`. A

src/extension-api/types.ts

@@ -450,8 +450,10 @@ export interface ExtensionFileView {
  * against the background it sits on, which differs per line kind (added,
  * removed, context) and per theme. The host owns that resolution, applying the
  * same minimum-contrast guarantee its own word-diff emphasis uses, so a mark
- * is never invisible on a green line. `"current"` is the emphatic variant of
- * `"match"` — search uses it for the match the user is on.
+ * is never invisible on a green line. A transparent cell has no color to blend
+ * against, so the host resolves the tint against the background it assumes the
+ * terminal shows. `"current"` is the emphatic variant of `"match"` — search
+ * uses it for the match the user is on.
  */
 export type ExtensionLineHighlightTone = "match" | "current" | "info" | "warning" | "error";
 
@@ -476,6 +478,10 @@ export interface ExtensionLineHighlight {
    * yields usable offsets directly. The host maps them to terminal columns and
    * widens them to grapheme-cluster boundaries, so an offset inside an emoji or
    * a CJK character marks the whole visible glyph rather than tearing it.
+   *
+   * Marks paint terminal columns, so a range covering only characters that
+   * occupy none — bidi controls, zero-width spaces and joiners — paints
+   * nothing.
    */
   range: readonly [number, number];
   /** What the mark means. Defaults to `"match"`. */
@@ -1724,7 +1730,8 @@ export interface HunkExtensionAPI {
    * its own diff rendering — syntax highlighting, word diff, and layout stay
    * intact. The host resolves each mark's `tone` against the active theme and
    * line kind, guaranteeing visible contrast the way its own word-diff
-   * emphasis does.
+   * emphasis does, and against an assumed background where the cell itself is
+   * transparent.
    */
   registerLineHighlighter(highlighter: ExtensionLineHighlighter): void;
   /**

src/ui/diff/lineHighlightPaint.test.ts

@@ -281,6 +281,27 @@ describe("applyLineHighlightsToSpans", () => {
     expect(painted).toEqual([{ text: "abcdef", bg: "#101010" }]);
   });
 
+  test("resolves a dense overlapping range set once per line, not once per piece", () => {
+    // 10,000 overlapping ranges on one wide-glyph line: painting used to cost
+    // work proportional to ranges × spans × pieces, which measured ~750ms for
+    // this row. The bound is loose enough to survive a slow CI box and tight
+    // enough that only near-linear resolution can meet it.
+    const text = "\u65E5".repeat(5_000);
+    const spans: RenderSpan[] = [{ text, fg: "#ffffff" }];
+    const width = measureTextWidth(text);
+    const ranges = Array.from({ length: 10_000 }, (_, index) => {
+      const startCol = (index * 7_919) % (width - 12);
+      return { startCol, endCol: startCol + 1 + (index % 11), tone: "match" as const };
+    }).sort((a, b) => a.startCol - b.startCol || a.endCol - b.endCol);
+
+    const started = performance.now();
+    const painted = applyLineHighlightsToSpans(spans, ranges, resolveBg);
+    const elapsed = performance.now() - started;
+
+    expect(painted.map((span) => span.text).join("")).toBe(text);
+    expect(elapsed).toBeLessThan(150);
+  });
+
   test("preserves zero-width spans and wide glyph boundaries", () => {
     const spans: RenderSpan[] = [{ text: "" }, { text: "x = \u{1F44D}ok" }];
 

src/ui/diff/lineHighlightPaint.ts

@@ -23,7 +23,13 @@ import type { DiffFile } from "../../core/types";
 import type { ExtensionLineHighlightTone } from "../../extension-api/types";
 import { sanitizeTerminalLine } from "../../lib/terminalText";
 import type { ValidatedLineHighlight } from "../highlights/validate";
-import { measureTextWidth, sliceTextByWidth, textClusters } from "../lib/text";
+import {
+  isPrintableAsciiText,
+  measureClusterWidth,
+  measureSanitizedTextWidth,
+  measureTextWidth,
+  textClusters,
+} from "../lib/text";
 import { expandDiffTabs } from "./codeColumns";
 import type { RenderSpan } from "./pierre";
 
@@ -330,15 +336,87 @@ function appendSpan(target: RenderSpan[], span: RenderSpan) {
   }
 }
 
-/** The tone painting one column, with later ranges winning overlaps. */
-function toneAtColumn(ranges: readonly LineHighlightColRange[], col: number) {
+/** One line's cut columns plus the tone winning each gap between them. */
+interface LineHighlightCutPlan {
+  /** Sorted unique cut columns; every painted piece begins and ends on one. */
+  readonly cuts: readonly number[];
+  /** Winning tone for `[cuts[i], cuts[i + 1])`, or `undefined` where nothing paints. */
+  readonly tones: ReadonlyArray<ExtensionLineHighlightTone | undefined>;
+}
+
+// Range lists are built once per file and shared by every row and re-render that
+// paints the line, so the plan is derived once per list rather than per row.
+const lineHighlightCutPlans = new WeakMap<readonly LineHighlightColRange[], LineHighlightCutPlan>();
+
+/**
+ * Resolve one line's ranges into cut columns and the tone winning each interval.
+ *
+ * Overlaps are resolved once per line instead of once per painted piece. The
+ * fill walks ranges from last to first — the same "later range wins" policy —
+ * and a skip pointer keeps intervals an already-applied range claimed out of
+ * every remaining range's walk, so a line carrying thousands of overlapping
+ * ranges costs about one pass over the cut list rather than one pass per piece.
+ */
+function lineHighlightCutPlan(ranges: readonly LineHighlightColRange[]): LineHighlightCutPlan {
+  const cached = lineHighlightCutPlans.get(ranges);
+  if (cached) return cached;
+
+  const cutSet = new Set<number>();
+  for (const range of ranges) {
+    cutSet.add(range.startCol);
+    cutSet.add(range.endCol);
+  }
+  const cuts = [...cutSet].sort((a, b) => a - b);
+  const columnIndex = new Map<number, number>();
+  for (const [index, cut] of cuts.entries()) columnIndex.set(cut, index);
+
+  const intervals = Math.max(0, cuts.length - 1);
+  const tones = Array.from<ExtensionLineHighlightTone | undefined>({ length: intervals });
+  // `skip[i]` points at the first interval from `i` onward that no range has
+  // claimed yet; path compression keeps repeated lookups near constant.
+  const skip = new Int32Array(intervals + 1);
+  for (let index = 0; index <= intervals; index += 1) skip[index] = index;
+  const nextUnclaimed = (index: number) => {
+    let current = index;
+    while (skip[current] !== current) {
+      skip[current] = skip[skip[current]!]!;
+      current = skip[current]!;
+    }
+    return current;
+  };
+
   for (let index = ranges.length - 1; index >= 0; index -= 1) {
     const range = ranges[index]!;
-    if (col >= range.startCol && col < range.endCol) {
-      return range.tone;
+    const to = columnIndex.get(range.endCol)!;
+    let interval = nextUnclaimed(columnIndex.get(range.startCol)!);
+    while (interval < to) {
+      tones[interval] = range.tone;
+      skip[interval] = interval + 1;
+      interval = nextUnclaimed(interval + 1);
+    }
+  }
+
+  const plan: LineHighlightCutPlan = { cuts, tones };
+  lineHighlightCutPlans.set(ranges, plan);
+  return plan;
+}
+
+/** The tone painting one column, with later ranges already resolved into the plan. */
+function toneAtColumn(plan: LineHighlightCutPlan, col: number) {
+  const { cuts, tones } = plan;
+  let low = 0;
+  let high = cuts.length - 1;
+  let interval = -1;
+  while (low <= high) {
+    const middle = (low + high) >> 1;
+    if (cuts[middle]! <= col) {
+      interval = middle;
+      low = middle + 1;
+    } else {
+      high = middle - 1;
     }
   }
-  return undefined;
+  return interval >= 0 && interval < tones.length ? tones[interval] : undefined;
 }
 
 /** The resolved paint for one tone: a background, plus a foreground when the mark inverts. */
@@ -367,57 +445,84 @@ export function applyLineHighlightsToSpans(
   }
 
   // Cut at every range boundary so each emitted piece has one winning tone.
-  const cuts = new Set<number>();
-  for (const range of ranges) {
-    cuts.add(range.startCol);
-    cuts.add(range.endCol);
-  }
-
+  const plan = lineHighlightCutPlan(ranges);
+  const { cuts } = plan;
   const result: RenderSpan[] = [];
   let col = 0;
+  let cutCursor = 0;
+
+  /** Emit one piece of a span under the tone winning the column it starts at. */
+  const paint = (span: RenderSpan, text: string, startCol: number) => {
+    if (text.length === 0) return;
+    const tone = toneAtColumn(plan, startCol);
+    const style = tone === undefined ? undefined : resolveStyle(tone);
+    appendSpan(
+      result,
+      style === undefined
+        ? { ...span, text }
+        : style.fg === undefined
+          ? { ...span, text, bg: style.bg }
+          : { ...span, text, bg: style.bg, fg: style.fg },
+    );
+  };
 
   for (const span of spans) {
-    const spanWidth = measureTextWidth(span.text);
+    const safeText = sanitizeTerminalLine(span.text);
+    const spanWidth = measureSanitizedTextWidth(safeText);
     if (spanWidth === 0) {
       appendSpan(result, { ...span });
       continue;
     }
 
     const spanStart = col;
     const spanEnd = col + spanWidth;
-    const localCuts = [...cuts]
-      .filter((cut) => cut > spanStart && cut < spanEnd)
+    col = spanEnd;
+
+    // Spans arrive in column order, so one forward-only cursor walks the shared
+    // cut list once per row instead of rebuilding it for every span.
+    while (cutCursor < cuts.length && cuts[cutCursor]! <= spanStart) cutCursor += 1;
+    if (cutCursor >= cuts.length || cuts[cutCursor]! >= spanEnd) {
+      paint(span, span.text, spanStart);
+      continue;
+    }
+
+    let cursor = cutCursor;
+    let pieceIndex = 0;
+    let pieceCol = spanStart;
+
+    if (isPrintableAsciiText(safeText)) {
+      // One cell per code unit, so a cut column is already a string index.
+      while (cursor < cuts.length && cuts[cursor]! < spanEnd) {
+        const cut = cuts[cursor]!;
+        paint(span, safeText.slice(pieceIndex, cut - spanStart), pieceCol);
+        pieceIndex = cut - spanStart;
+        pieceCol = cut;
+        cursor += 1;
+      }
+      paint(span, safeText.slice(pieceIndex), pieceCol);
+      continue;
+    }
+
+    let clusterIndex = 0;
+    let clusterCol = spanStart;
+    for (const cluster of textClusters(safeText)) {
       // A cut inside a wide glyph would slice it into a dropped half and a pad
       // space, costing the row one column and breaking the one contract
       // everything upstream relies on: paint never changes rendered width.
       // Cluster snapping upstream should make this unreachable, but the
       // invariant is enforced here, where it could break: a mid-glyph cut is
-      // snapped away and the glyph paints whole under one tone.
-      .filter((cut) => sliceTextByWidth(span.text, 0, cut - spanStart).width === cut - spanStart)
-      .sort((a, b) => a - b);
-
-    let pieceStart = spanStart;
-    for (const boundary of [...localCuts, spanEnd]) {
-      const piece =
-        localCuts.length === 0
-          ? span.text
-          : sliceTextByWidth(span.text, pieceStart - spanStart, boundary - pieceStart).text;
-      if (piece.length > 0) {
-        const tone = toneAtColumn(ranges, pieceStart);
-        const style = tone === undefined ? undefined : resolveStyle(tone);
-        appendSpan(
-          result,
-          style === undefined
-            ? { ...span, text: piece }
-            : style.fg === undefined
-              ? { ...span, text: piece, bg: style.bg }
-              : { ...span, text: piece, bg: style.bg, fg: style.fg },
-        );
+      // passed over and the glyph paints whole under one tone.
+      while (cursor < cuts.length && cuts[cursor]! < clusterCol) cursor += 1;
+      if (cursor < cuts.length && cuts[cursor] === clusterCol) {
+        paint(span, safeText.slice(pieceIndex, clusterIndex), pieceCol);
+        pieceIndex = clusterIndex;
+        pieceCol = clusterCol;
+        cursor += 1;
       }
-      pieceStart = boundary;
+      clusterCol += measureClusterWidth(cluster);
+      clusterIndex += cluster.length;
     }
-
-    col = spanEnd;
+    paint(span, safeText.slice(pieceIndex), pieceCol);
   }
 
   return result;