Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .changeset/bounded-highlight-worker-retries.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
"hunkdiff": patch
---

Stop re-running a failed large-diff highlight on every scroll, pick up a later successful retry
without remounting the file, and charge worker highlight results to the cache budget by the payload
they actually retain.
60 changes: 60 additions & 0 deletions src/ui/diff/highlightedDiffCache.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { describe, expect, test } from "bun:test";
import type { HighlightedDiffCode } from "./diffRows";
import { createHighlightedDiffCache } from "./highlightedDiffCache";
import { COMPACT_HIGHLIGHT_PROTOCOL_VERSION } from "./worker/highlightCompact";

/** Build a result retaining a known line count; identity is what these tests compare. */
function createTestHighlightedDiffCode(lines: number): HighlightedDiffCode {
Expand All @@ -10,6 +11,41 @@ function createTestHighlightedDiffCode(lines: number): HighlightedDiffCode {
};
}

/** Build one compact side holding a single run per line, as the worker encodes full source. */
function createTestCompactSide(lines: number) {
return {
lineOffsets: new Uint32Array(Array.from({ length: lines + 1 }, (_, index) => index)),
starts: new Uint32Array(lines),
ends: new Uint32Array(Array.from({ length: lines }, () => 8)),
styleIds: new Uint16Array(Array.from({ length: lines }, () => 1)),
flags: new Uint8Array(lines),
};
}

/**
* Build a source-backed compact result: the payload spans the whole file while the index maps
* cover only the visible patch lines.
*/
function createTestCompactHighlightedDiffCode(
sourceLines: number,
patchLines: number,
): HighlightedDiffCode {
return {
deletionLines: [],
additionLines: [],
compact: {
payload: {
version: COMPACT_HIGHLIGHT_PROTOCOL_VERSION,
foregroundPalette: ["#ffffff"],
deletion: createTestCompactSide(sourceLines),
addition: createTestCompactSide(sourceLines),
},
deletionLineMap: Array.from({ length: patchLines }, (_, index) => index),
additionLineMap: Array.from({ length: patchLines }, (_, index) => index),
},
};
}

describe("highlighted diff cache", () => {
test("evicts the least recently used entry rather than the oldest highlight", () => {
// Three 10-line results cost 18 each with per-entry overhead, so two fit and a third evicts.
Expand Down Expand Up @@ -101,6 +137,30 @@ describe("highlighted diff cache", () => {
expect(cache.peek("skipped-0")).toBeUndefined();
});

test("charges a source-backed compact result for the payload it retains", () => {
const cache = createHighlightedDiffCache(100);

// A few visible patch lines grafted onto a large file: the index maps are tiny, but the
// payload holds the whole source. Charging the maps would let these accumulate unbounded.
cache.set("grafted", createTestCompactHighlightedDiffCode(20_000, 4));
cache.set("neighbor", createTestHighlightedDiffCode(10));

expect(cache.peek("grafted")).toBeUndefined();
expect(cache.peek("neighbor")).toBeDefined();
});

test("keeps a small compact result cheap enough to sit beside its neighbors", () => {
const cache = createHighlightedDiffCache(100);
const compact = createTestCompactHighlightedDiffCode(40, 40);
const neighbor = createTestHighlightedDiffCode(10);

cache.set("compact", compact);
cache.set("neighbor", neighbor);

expect(cache.peek("compact")).toBe(compact);
expect(cache.peek("neighbor")).toBe(neighbor);
});

test("keeps one entry when given a degenerate budget", () => {
const cache = createHighlightedDiffCache(0);
const only = createTestHighlightedDiffCode(5);
Expand Down
28 changes: 22 additions & 6 deletions src/ui/diff/highlightedDiffCache.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import type { HighlightedDiffCode } from "./diffRows";
import { compactHighlightedDiffByteLength } from "./worker";

/**
* Cache budget, counted in highlighted diff lines.
Expand Down Expand Up @@ -38,14 +39,29 @@ interface HighlightedDiffCacheEntry {
value: HighlightedDiffCode;
}

/** Count the highlighted lines one result retains, across both diff sides. */
/**
* Bytes one highlighted line costs at the rate this budget was calibrated against.
*
* Compact worker results retain typed ranges rather than HAST nodes, so they are converted through
* the same rate instead of counted as lines. That keeps one budget comparable across both shapes.
*/
const HIGHLIGHTED_LINE_BYTES = 1_400;

/** Bytes one index-map entry retains, at a packed small-integer array's per-entry cost. */
const LINE_MAP_ENTRY_BYTES = 8;

/** Count the line-equivalents one result retains, across both diff sides. */
function highlightedLineCount(value: HighlightedDiffCode) {
if (value.compact) {
return (
(value.compact.deletionLineMap?.length ??
value.compact.payload.deletion.lineOffsets.length - 1) +
(value.compact.additionLineMap?.length ??
value.compact.payload.addition.lineOffsets.length - 1)
// Charge the payload actually held. A source-backed compact result spans the whole file, which
// is many times the visible patch lines its index maps cover.
const lineMapBytes =
((value.compact.deletionLineMap?.length ?? 0) +
(value.compact.additionLineMap?.length ?? 0)) *
LINE_MAP_ENTRY_BYTES;
return Math.ceil(
(compactHighlightedDiffByteLength(value.compact.payload) + lineMapBytes) /
HIGHLIGHTED_LINE_BYTES,
);
}

Expand Down
65 changes: 62 additions & 3 deletions src/ui/diff/useHighlightedDiff.test.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,19 @@
import { describe, expect, test } from "bun:test";
import { afterAll, describe, expect, test } from "bun:test";
import { createTestDiffFile, createTestSourceFetcher } from "../../../test/helpers/diff-helpers";
import { resolveTheme } from "../themes";
import { HIGHLIGHT_WORKER_MIN_LINES } from "./diffRows";
import { prefetchHighlightedDiff, highlightedDiffCacheKey } from "./useHighlightedDiff";
import { registerHighlightWorker } from "./worker";
import {
prefetchHighlightedDiff,
highlightedDiffCacheKey,
subscribeToHighlightedDiff,
} from "./useHighlightedDiff";
import { disposeHighlightWorker, registerHighlightWorker } from "./worker";

// The highlight client is one module-level singleton shared by every test file in a `bun test`
// process. Release the doubles registered here so a later file starts from a real worker.
afterAll(() => {
disposeHighlightWorker();
});

/** Build one file large enough to qualify for worker highlighting. */
function createLargeHighlightTestFile(id: string) {
Expand Down Expand Up @@ -98,6 +108,55 @@ describe("highlighted diff cache", () => {
expect(secondWorker.calls).toBe(1);
});

test("stops re-requesting a worker highlight once the retry budget is spent", async () => {
const file = createLargeHighlightTestFile("exhausted-worker-retries");
const theme = resolveTheme("github-dark-default", null);

// Viewport prefetch re-requests every file in its halo on each scroll, so a failure that keeps
// repeating must settle on cached plain rows instead of restarting the highlight every time.
for (const expectedCalls of [1, 1]) {
const worker = registerFailingHighlightWorkerForTest();
const result = await prefetchHighlightedDiff({ file, offloadLargeDiff: true, theme });
expect(result.retryable).toBe(true);
expect(worker.calls).toBe(expectedCalls);
}

const settledWorker = registerFailingHighlightWorkerForTest();
const settled = await prefetchHighlightedDiff({ file, offloadLargeDiff: true, theme });
expect(settled.retryable).toBeUndefined();
expect(settledWorker.calls).toBe(0);
});

test("wakes a reader holding provisional rows when the key finally caches a result", async () => {
const file = createLargeHighlightTestFile("provisional-reader-wakeup");
const theme = resolveTheme("github-dark-default", null);
const cacheKey = highlightedDiffCacheKey(theme, file);

registerFailingHighlightWorkerForTest();
const first = await prefetchHighlightedDiff({ file, offloadLargeDiff: true, theme });
expect(first.retryable).toBe(true);

// A mounted file showing those provisional rows has no other signal: viewport prefetch fills
// the shared cache without rendering anything.
let notified = 0;
const unsubscribe = subscribeToHighlightedDiff(cacheKey, () => {
notified += 1;
});

try {
registerFailingHighlightWorkerForTest();
await prefetchHighlightedDiff({ file, offloadLargeDiff: true, theme });

expect(notified).toBe(1);
} finally {
unsubscribe();
}

// Unsubscribed readers stop hearing about the key.
await prefetchHighlightedDiff({ file, offloadLargeDiff: true, theme });
expect(notified).toBe(1);
});

test("reuses source-backed highlights for equivalent versioned providers", () => {
const base = createTestDiffFile({ id: "cache", path: "cache.ts" });
const firstFetcher = Object.assign(
Expand Down
102 changes: 97 additions & 5 deletions src/ui/diff/useHighlightedDiff.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { useLayoutEffect, useState } from "react";
import { useEffect, useLayoutEffect, useState } from "react";
import type { DiffFile } from "../../core/types";
import type { AppTheme } from "../themes";
import { loadHighlightedDiff, type HighlightedDiffCode } from "./diffRows";
Expand Down Expand Up @@ -86,11 +86,61 @@ export function highlightedDiffCacheKey(theme: AppTheme, file: DiffFile) {
return `${theme.id}:${syntaxHighlightThemeName(theme)}:${file.id}:${patchFingerprint(file)}:${sourceFetcherFingerprint(file)}`;
}

/**
* Worker failures re-attempted before a file settles on plain rows.
*
* A retryable result is deliberately kept out of the cache so a recreated worker can still colorize
* the file, but viewport prefetch re-requests every file in its halo on each scroll. Without a
* bound, a failure that repeats — an unresolvable worker entry, a payload the validator always
* rejects — would restart the multi-second highlight on every scroll step. One retry covers a
* worker lost mid-session; past that the plain result is cached and the work stops.
*/
const MAX_HIGHLIGHT_RETRY_ATTEMPTS = 1;

/** Retryable worker failures seen per cache key, cleared once the key settles. */
const HIGHLIGHT_RETRY_ATTEMPTS = new Map<string, number>();

/** Readers waiting for one cache key to receive a cacheable result. */
const HIGHLIGHT_RESULT_LISTENERS = new Map<string, Set<() => void>>();

/**
* Watch one cache key until it holds a cacheable result, and return an unsubscribe.
*
* Viewport prefetch fills the shared cache without rendering anything, so a reader showing the
* provisional plain rows of a retryable failure has no other signal that a later attempt succeeded.
*/
export function subscribeToHighlightedDiff(cacheKey: string, listener: () => void) {
const listeners = HIGHLIGHT_RESULT_LISTENERS.get(cacheKey) ?? new Set<() => void>();
listeners.add(listener);
HIGHLIGHT_RESULT_LISTENERS.set(cacheKey, listeners);

return () => {
listeners.delete(listener);
if (listeners.size === 0) {
HIGHLIGHT_RESULT_LISTENERS.delete(cacheKey);
}
};
}

/** Wake readers holding provisional rows once a key caches a result. */
function notifyHighlightedDiffCached(cacheKey: string) {
const listeners = HIGHLIGHT_RESULT_LISTENERS.get(cacheKey);
if (!listeners) {
return;
}

// Copy before dispatching: a listener that repaints unsubscribes while this loop is running.
for (const listener of Array.from(listeners)) {
listener();
}
}

/**
* Commit one cacheable highlight result if its promise is still active for that key.
*
* A transient worker failure resolves to plain rows with `retryable`, which updates the current
* view but must not occupy the shared cache and prevent a later worker retry.
* A retryable worker failure updates the current view without occupying the cache, so a later
* visit can try a recreated worker. Once the retry budget is spent the plain result is cached like
* any other, which is what stops prefetch from re-running a failure that will not resolve.
*/
function commitHighlightResult(
cacheKey: string,
Expand All @@ -102,9 +152,24 @@ function commitHighlightResult(
}

SHARED_HIGHLIGHT_PROMISES.delete(cacheKey);
if (!result.retryable) {
SHARED_HIGHLIGHTED_DIFF_CACHE.set(cacheKey, result);

if (result.retryable) {
const attempts = (HIGHLIGHT_RETRY_ATTEMPTS.get(cacheKey) ?? 0) + 1;
if (attempts <= MAX_HIGHLIGHT_RETRY_ATTEMPTS) {
HIGHLIGHT_RETRY_ATTEMPTS.set(cacheKey, attempts);
return true;
}

// Budget spent: cache plain rows without the retryable marker so readers stop re-requesting.
HIGHLIGHT_RETRY_ATTEMPTS.delete(cacheKey);
SHARED_HIGHLIGHTED_DIFF_CACHE.set(cacheKey, { deletionLines: [], additionLines: [] });
notifyHighlightedDiffCached(cacheKey);
return true;
}

HIGHLIGHT_RETRY_ATTEMPTS.delete(cacheKey);
SHARED_HIGHLIGHTED_DIFF_CACHE.set(cacheKey, result);
notifyHighlightedDiffCached(cacheKey);
return true;
}

Expand Down Expand Up @@ -175,6 +240,13 @@ function resolveHighlightedSnapshot({
}

if (highlightedCacheKey === appearanceCacheKey) {
// Plain rows from a retryable worker failure are provisional. The layout effect below will not
// re-run for a key it already committed, so prefer a result a later prefetch retry has since
// cached rather than holding this file plain until it remounts.
if (highlighted?.retryable) {
return SHARED_HIGHLIGHTED_DIFF_CACHE.peek(appearanceCacheKey) ?? highlighted;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Successful retry does not repaint

When a mounted file has a provisional retryable result and a later viewport prefetch succeeds, the prefetch only updates the unsubscribed shared cache. No render is scheduled to execute this peek, so the file remains plain until another scroll, state update, or remount.

Knowledge Base Used: Diff Rendering Pipeline

Prompt To Fix With AI
This is a comment left during a code review.
Path: src/ui/diff/useHighlightedDiff.ts
Line: 210

Comment:
**Successful retry does not repaint**

When a mounted file has a provisional retryable result and a later viewport prefetch succeeds, the prefetch only updates the unsubscribed shared cache. No render is scheduled to execute this `peek`, so the file remains plain until another scroll, state update, or remount.

**Knowledge Base Used:** [Diff Rendering Pipeline](https://app.greptile.com/modem/-/custom-context/knowledge-base/modem-dev/hunk/-/docs/ui-diff-rendering.md)

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, and fixed in b1538b3.

The peek only helps if something happens to re-render. Prefetch fills the shared cache from a useEffect in DiffPane and never touches this hook's state, so a file holding provisional rows had no signal at all — it stayed plain until an unrelated render or a remount.

The cache commit now notifies readers watching that key, and the hook subscribes for exactly as long as it holds a provisional result:

  • subscribeToHighlightedDiff(cacheKey, listener) registers interest in one key.
  • commitHighlightResult calls notifyHighlightedDiffCached on both paths that write to the cache — a successful retry, and the plain rows cached once the retry budget is spent.
  • The hook subscribes only while highlighted?.retryable is set, and the repaint it performs clears that flag, which unsubscribes it. No loop.

The peek in resolveHighlightedSnapshot stays, since it still covers the window where the cache fills between render and the subscription effect.

Dispatch iterates over a copy of the listener set, because a listener repainting is what removes it mid-loop.

Covered by wakes a reader holding provisional rows when the key finally caches a result, which also asserts an unsubscribed reader stops being notified.


Generated by Claude Code

}

return highlighted;
}

Expand Down Expand Up @@ -242,6 +314,26 @@ export function useHighlightedDiff({
};
}, [appearanceCacheKey, file, highlightedCacheKey, offloadLargeDiff, shouldLoadHighlight]);

// Plain rows from a retryable worker failure are provisional, and the layout effect above will
// not run again for a key it already committed. Viewport prefetch fills the shared cache without
// rendering, so watch this key until a later attempt caches a result worth repainting.
useEffect(() => {
if (
!appearanceCacheKey ||
highlightedCacheKey !== appearanceCacheKey ||
!highlighted?.retryable
) {
return;
}

return subscribeToHighlightedDiff(appearanceCacheKey, () => {
const cached = SHARED_HIGHLIGHTED_DIFF_CACHE.peek(appearanceCacheKey);
if (cached) {
setHighlighted(cached);
}
});
}, [appearanceCacheKey, highlighted, highlightedCacheKey]);

// Prefer cached highlights during render so revisiting a file can paint immediately.
return resolveHighlightedSnapshot({
appearanceCacheKey,
Expand Down
Loading