Skip to content

fix(ui): bound and surface large-diff highlight retries - #785

Open
benvinegar wants to merge 2 commits into
mainfrom
claude/pr-759-review-4gzat1-retry
Open

fix(ui): bound and surface large-diff highlight retries#785
benvinegar wants to merge 2 commits into
mainfrom
claude/pr-759-review-4gzat1-retry

Conversation

@benvinegar

Copy link
Copy Markdown
Member

Follow-up to #759. Independent of #784 — different files, either can land first.

1. A repeating worker failure re-ran on every scroll

loadHighlightedDiff marks every worker-path failure retryable, and commitHighlightResult deliberately keeps those out of the shared cache so a recreated worker can still colorize the file. But DiffPane re-runs prefetchHighlightedDiff for every file in its halo whenever highlightPrefetchFileIds changes — i.e. on scroll. With nothing cached and no promise retained, a failure that always repeats (unresolvable worker entry in a repackaged binary, a payload the validator always rejects) rebuilt the source plan and restarted the multi-second highlight on every scroll step. That is the exact repetition the comment above the catch says it prevents.

Retries are now bounded per cache key. One retry covers a worker lost mid-session; past that, plain rows are cached like any other result and the work stops.

2. The retry never reached the file on screen

Even when a later attempt succeeded, the mounted file stayed plain. The hook commits highlightedCacheKey for a retryable result too, so its layout effect short-circuits and never re-runs for that key — the success only surfaced after unmount/remount.

resolveHighlightedSnapshot now treats a retryable result as provisional and prefers a result a later prefetch has since cached. It uses peek, so render stays side-effect free.

These two are load-bearing together: without the bound, the cost repeated forever; without the peek, the benefit never landed.

3. The cache budget stopped bounding compact entries

highlightedLineCount charged compact results by deletionLineMap.length — one entry per visible patch line — while the payload retains ranges for the whole source. A 4-line patch view of a 20,000-line file was charged 8 line-equivalents.

Compact results are now charged via compactHighlightedDiffByteLength, converted through the same ~1.4KB/line rate the budget was calibrated on. That keeps one budget comparable across HAST and compact shapes, and correctly keeps small compact results cheap rather than over-charging them as lines.

4. Test isolation

The failing-worker doubles were registered into a module-level singleton shared by every test file in a bun test process and never released. Added an afterAll teardown.

Changes

  • src/ui/diff/useHighlightedDiff.ts — bounded retry budget; provisional retryable snapshots.
  • src/ui/diff/highlightedDiffCache.ts — charge compact entries by retained payload.
  • src/ui/diff/useHighlightedDiff.test.ts — budget-exhaustion coverage plus teardown.
  • src/ui/diff/highlightedDiffCache.test.ts — source-backed compact entry is charged for its payload; a small compact entry stays cheap.

Both new cache tests were verified to fail under the previous accounting, and the retry test to fail without the bound.

Validation

  • bun run typecheck, bun run lint, bun run format
  • bun test — 3030 pass, 3 fail: change-block line pairing (confirmed failing on unmodified main at 5ebe975), one PTY flake, and website/ specs missing @axe-core/playwright here.
  • bun run test:integration — 114 pass, 3 fail. Unmodified main fails 2 of the same 3; the third (PTY layout > dragging the sidebar divider) is flaky under load and passed 3/3 in isolation on this branch, as did test/pty/scroll.test.ts.
  • bun run test:tty-smoke — 0/9 in this sandbox, identical on unmodified main, so unverified rather than passing.

Generated by Claude Code

A failed worker highlight resolved to plain rows marked retryable, which deliberately stay out of
the shared cache so a recreated worker can still colorize the file. Viewport prefetch re-requests
every file in its halo on each scroll, so a failure that always repeats restarted the multi-second
highlight on every scroll step. Bound the retries and cache plain rows once the budget is spent.

The retry also never reached the file the user was looking at: the hook commits its cache key even
for a retryable result, so its layout effect will not run again for that key. Prefer a result a
later prefetch has since cached instead of holding the file plain until it remounts.

Charge compact results by the payload they retain rather than by their index maps. A source-backed
result maps a few visible patch lines onto a whole-file payload, so the maps understated it by
orders of magnitude and the cache's line budget stopped bounding memory.

Dispose the worker doubles registered in the highlight tests: the client is a module-level
singleton shared by every test file in a `bun test` process.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01P2ZhrEHs971XcBuAMP6RwR
@vercel

vercel Bot commented Aug 16, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
hunk-web Ignored Ignored Preview Aug 16, 2026 10:38pm

Request Review

@greptile-apps

greptile-apps Bot commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR bounds repeated large-diff highlighting failures, allows provisional snapshots to consult later cached results, and revises compact-result cache accounting.

  • Adds a per-cache-key worker retry budget and settles exhausted retries on plain rows.
  • Charges compact highlights using retained payload size plus source line maps.
  • Adds retry-budget, compact-accounting, and test-worker cleanup coverage.

Confidence Score: 4/5

The successful-retry refresh path should be fixed before merging because a mounted file can remain plain after highlighting has completed.

A successful background prefetch only updates an unsubscribed shared cache, while the mounted hook has already committed the key and does not schedule another render to observe that result.

Files Needing Attention: src/ui/diff/useHighlightedDiff.ts

Important Files Changed

Filename Overview
src/ui/diff/useHighlightedDiff.ts Adds bounded retries and provisional cache lookup, but successful background retries do not notify mounted consumers.
src/ui/diff/highlightedDiffCache.ts Replaces visible-line accounting for compact results with payload-byte and line-map accounting.
src/ui/diff/useHighlightedDiff.test.ts Covers exhausted retry behavior and singleton-worker cleanup but does not exercise mounted-hook refresh after prefetch success.
src/ui/diff/highlightedDiffCache.test.ts Adds coverage for large source-backed compact payloads and inexpensive small compact entries.

Sequence Diagram

sequenceDiagram
  participant H as Mounted highlight hook
  participant C as Shared cache
  participant P as Viewport prefetch
  participant W as Worker
  H->>W: Initial highlight
  W-->>H: Retryable failure
  H->>H: Store provisional plain result
  P->>W: Retry same cache key
  W-->>P: Successful highlight
  P->>C: Cache successful result
  Note over C,H: Cache write schedules no render
  H-->>H: Plain rows remain until another render
Loading
Prompt To Fix All With AI
### Issue 1
src/ui/diff/useHighlightedDiff.ts:210
**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.

---

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

Reviews (1): Last reviewed commit: "fix(ui): bound and surface large-diff hi..." | Re-trigger Greptile

// 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

Preferring a newly cached result during render was not enough on its own. Viewport prefetch fills
the shared cache without rendering anything, so a file showing the provisional plain rows of a
retryable failure had no signal that a later attempt succeeded, and stayed plain until the next
unrelated render or a remount.

Have the cache commit notify readers watching that key, and subscribe from the hook for as long as
it is holding a provisional result.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01P2ZhrEHs971XcBuAMP6RwR
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.

2 participants