Skip to content

fix(reload): stale syntax highlights after r key / watch reload#146

Open
eduwass wants to merge 2 commits intomodem-dev:mainfrom
eduwass:fix/stale-highlight-cache-on-reload
Open

fix(reload): stale syntax highlights after r key / watch reload#146
eduwass wants to merge 2 commits intomodem-dev:mainfrom
eduwass:fix/stale-highlight-cache-on-reload

Conversation

@eduwass
Copy link
Copy Markdown

@eduwass eduwass commented Mar 31, 2026

Summary

Reload (r key and --watch mode) shows stale diff content for git working tree reviews. After editing a file and reloading, the old content persists in the UI even though git diff returns the correct updated output.

Root cause

The syntax highlight cache in useHighlightedDiff.ts uses ${appearance}:${file.id} as its cache key. For git-backed diffs, file.id is derived from the file path (e.g. patch:0:readme.md), not the content. When a reload rebuilds the changeset, the file ID stays the same because the path hasn't changed — only the content has. The cached HAST nodes from the previous render are served, and since buildStackRows/buildSplitRows read line content from the highlighted output (not the raw file metadata), the stale text appears in the UI.

Reproduction

  1. hunk diff in any git repo with uncommitted changes
  2. Edit a tracked file in another terminal (even adding a single character)
  3. Press r to reload (or use --watch)
  4. Expected: diff shows the new content
  5. Actual: diff still shows the old content from before the edit

Fix

Switch the highlight cache from identity-based keys to content-addressed keys:

  • Content fingerprint in cache key: buildCacheKey() includes a patchFingerprint() derived from file.patch, so changed files get a new key automatically while unchanged files keep their cache hit across reloads. No global cache flush needed.
  • Promise race guard: commitHighlightResult() only writes to the cache if the promise is still the active one registered for that key. Prevents a superseded or late-resolving async highlight from overwriting a newer entry.
  • LRU eviction: enforceCacheLimit() caps the result cache at 150 entries, preventing unbounded growth during long --watch sessions with many file edits.
  • sourcePath passthrough: refreshCurrentInput now passes bootstrap.changeset.sourceLabel as sourcePath so git reload resolves the correct repo root (previously fell back to process.cwd() which could differ).

Approaches considered and rejected

  • Global generation counter + cache flush: Simpler but flushes highlights for ALL files on every reload, forcing unchanged files to re-highlight unnecessarily. Coarse invalidation is wasteful on large multi-file diffs.
  • Full App remount on reload (setAppVersion bump): Fixes staleness by clearing all component state, but destroys scroll position, selection, filter text, sidebar width, and all other UI state. Unacceptable UX tradeoff.
  • Resetting highlighted state via useEffect on file reference change: The file object IS a new reference after reload, but the useLayoutEffect bails out early at highlightedCacheKey === appearanceCacheKey because the key format was identity-based. Fixing the key is the right layer.

Test plan

  • Regression test: test/reload-bug.test.tsx — two tests covering file-pair diffs and git working tree diffs
  • Tests wait for highlight cache to populate before reload (ensures the stale-cache path is exercised, not the plain-text fallback)
  • All 23 existing app-interactions tests pass
  • Typecheck clean

The highlight cache keyed by appearance + file.id served stale HAST
nodes after reload because the file ID is path-based and doesn't
change when content changes. Switch to a content-addressed cache key
using a patch fingerprint, add promise race guards, and cap the cache
at 150 entries.
@greptile-apps
Copy link
Copy Markdown

greptile-apps bot commented Mar 31, 2026

Greptile Summary

This PR fixes a stale syntax highlight bug after r-key and --watch reloads by switching the highlight cache from identity-based keys (appearance:file.id) to content-addressed keys that embed a patch fingerprint (appearance:file.id:length:prefix). It also adds a promise race guard to prevent superseded async highlights from overwriting newer cache entries, an LRU eviction cap (150 entries), and a sourcePath passthrough so git reloads resolve the correct repo root.

Key changes:

  • useHighlightedDiff.tsbuildCacheKey() embeds patchFingerprint(), commitHighlightResult() guards the promise race, enforceCacheLimit() caps the shared cache at 150 entries
  • App.tsxrefreshCurrentInput passes bootstrap.changeset.sourceLabel as sourcePath; correct for git inputs (where sourceLabel = repoRoot), but a regression for diff/difftool inputs with relative paths (where sourceLabel = \"file compare\")
  • test/reload-bug.test.tsx — two new integration tests covering file-pair and git working-tree reload; both use absolute paths, so the sourcePath regression isn't exercised by the test suite

Confidence Score: 4/5

Safe to merge for git-backed diffs; introduces a reload regression for hunk diff with relative file paths due to sourceLabel being used as a filesystem path.

The core cache fix in useHighlightedDiff.ts is well-reasoned and correctly implemented. The one P1 issue is in App.tsx: sourcePath: bootstrap.changeset.sourceLabel is only a valid filesystem path for git inputs — for diff/difftool inputs it is the display string "file compare", which breaks loadAppBootstrap's cwd-relative path resolution when left/right paths are relative. The new tests use absolute paths so they don't catch this. A one-line guard on input.kind resolves it cleanly.

src/ui/App.tsx — the sourcePath passthrough needs to be gated on bootstrap.input.kind being a git-backed type before merging.

Important Files Changed

Filename Overview
src/ui/App.tsx Passes bootstrap.changeset.sourceLabel as sourcePath to fix git-reload cwd; correct for git inputs (sourceLabel = repoRoot) but breaks relative-path reload for diff/difftool inputs where sourceLabel is "file compare" / "git difftool".
src/ui/diff/useHighlightedDiff.ts Core fix: switches cache key from identity-based to content-addressed (fingerprint). Adds promise race guard (commitHighlightResult) and LRU eviction. Weak fingerprint using only patch length + first 128 chars is a theoretical collision risk for edits deep in large files.
test/reload-bug.test.tsx New regression tests for both file-pair and git working-tree diffs. Both tests use absolute paths, so the sourcePath regression for relative-path file-pair diffs is not covered.

Sequence Diagram

sequenceDiagram
    participant U as User (r key)
    participant App as App.tsx
    participant AH as AppHost.tsx
    participant HL as useHighlightedDiff.ts
    participant Cache as SHARED_CACHE

    U->>App: press "r"
    App->>App: refreshCurrentInput()
    App->>AH: onReloadSession(nextInput, { sourcePath: sourceLabel })
    AH->>AH: loadAppBootstrap(input, { cwd: sourcePath })
    AH->>App: setActiveBootstrap(nextBootstrap)

    Note over App,HL: Component re-renders with new bootstrap

    App->>HL: useHighlightedDiff({ file: newFile, ... })
    HL->>HL: buildCacheKey(appearance, file)
    HL->>Cache: has(newCacheKey)?
    Cache-->>HL: false (content changed, new key)
    HL->>HL: loadHighlightedDiff(file) to promise
    HL->>Cache: PROMISES.set(newCacheKey, promise)

    HL->>HL: effectPromise.then(result)
    HL->>HL: commitHighlightResult(key, promise, result)
    HL->>HL: PROMISES.get(key) === promise? true
    HL->>Cache: CACHE.set(newCacheKey, result)
    HL->>HL: setHighlighted(result)

    Note over HL,Cache: Old promise resolves late
    HL->>HL: commitHighlightResult(oldKey, oldPromise, stale)
    HL->>HL: PROMISES.get(oldKey) !== oldPromise, false
    Note over HL: Stale result discarded
Loading

Reviews (1): Last reviewed commit: "fix(reload): stale syntax highlights aft..." | Re-trigger Greptile

- Guard sourcePath on git-backed input kinds only (diff/difftool use
  display strings like "file compare" as sourceLabel, not paths)
- Strengthen patchFingerprint to sample start, middle, and end of
  the patch to avoid collisions on edits deep in large files
- Remove redundant effectPromise && guards inside .then()/.catch()
@eduwass
Copy link
Copy Markdown
Author

eduwass commented Mar 31, 2026

Addressing review feedback (67a3223):

P1 — sourceLabel as filesystem path for non-git inputs: Fixed. sourcePath is now gated on bootstrap.input.kind being git, show, or stash-show. For diff/difftool inputs it passes undefined so loadAppBootstrap falls back to process.cwd() as before.

P2 — Weak fingerprint collision: Fixed. patchFingerprint now samples start (64 chars), middle (64 chars), and end (64 chars) of the patch instead of just the first 128. This prevents collisions when edits land deep in large files with equal-length line replacements.

P2 — Redundant effectPromise && guards: Fixed. Removed the dead guards inside .then() / .catch() — the optional chaining on effectPromise?.then(...) already guarantees non-null inside the callbacks.

All 25 tests pass (23 existing + 2 new regression tests). Typecheck clean.

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