docs: investigate windowed syntax highlighting in Pierre - #756
docs: investigate windowed syntax highlighting in Pierre#756benvinegar wants to merge 2 commits into
Conversation
Large added files freeze the terminal because Pierre highlights a whole file in one uninterruptible call. Confirm why: renderDiffWithHighlighter already carries a full row-window walk, but discards startingLine and totalLines unless the caller also asked for plain text, because nothing carried TextMate grammar state across a window boundary. Shiki 3.x already exposes that state, and Pierre already uses it for streaming. Add a patch against Pierre's TypeScript source that honors an explicitly requested window and threads grammar state per side, verified with their own toolchain, plus a benchmark that checks windowed output byte-for-byte across three languages, five diff shapes and a partial patch. The longest uninterruptible call drops from 661ms to 28ms on an 8000-line added file at 250-row windows. A companion investigation into worker offload does better, so this lands as an upstream contribution rather than as Hunk's own fix. Nothing in src changes and the patch is not activated. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JgbAM65ZsLbsjgqZS8Dtav
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Greptile SummaryThe PR documents an investigation into interruptible, windowed syntax highlighting and supplies equivalent upstream-source and locally applicable Pierre patches plus a benchmark.
Confidence Score: 4/5The patches need their inconsistent windowed-highlight flag references corrected before merging because the upstream source cannot compile and the local dist patch cannot execute. Both patch variants declare Files Needing Attention: patches/pierre-upstream-windowed-highlight.patch and patches/@pierre%2Fdiffs@1.2.2.patch Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart LR
A[Parse file diff] --> B[Render row window]
B --> C[Tokenize deletion and addition sides]
C --> D[Return sparse highlighted lines]
C --> E[Return per-side grammar state]
E --> F[Next contiguous window]
F --> B
D --> G[Merge windows for comparison]
Prompt To Fix All With AI### Issue 1
patches/pierre-upstream-windowed-highlight.patch:142
**Undefined window flag breaks patches**
When either supplied patch is applied, the implementation declares `isHighlightedWindow` but evaluates the undeclared `isWindowedHighlight`, causing the upstream TypeScript patch to fail compilation and the dist patch to throw a `ReferenceError` before rendering.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "docs: investigate windowed syntax highli..." | Re-trigger Greptile |
| + // path below, so it can never take the shortcut of replacing the whole array | ||
| + // with a single bucket's lines. | ||
| + const shouldGroupAll = | ||
| + !forcePlainText && !isWindowedHighlight && !diff.isPartial; |
There was a problem hiding this comment.
Undefined window flag breaks patches
When either supplied patch is applied, the implementation declares isHighlightedWindow but evaluates the undeclared isWindowedHighlight, causing the upstream TypeScript patch to fail compilation and the dist patch to throw a ReferenceError before rendering.
Prompt To Fix With AI
This is a comment left during a code review.
Path: patches/pierre-upstream-windowed-highlight.patch
Line: 142
Comment:
**Undefined window flag breaks patches**
When either supplied patch is applied, the implementation declares `isHighlightedWindow` but evaluates the undeclared `isWindowedHighlight`, causing the upstream TypeScript patch to fail compilation and the dist patch to throw a `ReferenceError` before rendering.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
Not a defect — isWindowedHighlight is pre-existing upstream code, not something this patch was supposed to declare.
It has been in renderDiffWithHighlighter since before this change, backing the already-shipped forcePlainText windowing feature. In Pierre at d9eb0ab, one commit before the patch:
$ git show HEAD~1:packages/diffs/src/utils/renderDiffWithHighlighter.ts | grep -n isWindowedHighlight
61: const isWindowedHighlight = startingLine > 0 || totalLines < Infinity;
77: !isWindowedHighlight &&
104: if (isWindowedHighlight) {
127: expandedHunks: isWindowedHighlight ? expandedHunksForIteration : true,
The patch adds isHighlightedWindow as a separate opt-in flag ("did the caller ask for a highlighted window?") alongside that existing derived value ("is a range in effect at all?"). Both are intentionally live. The names being near-anagrams is genuinely unhelpful to read, which is probably what triggered this — see below.
Verified rather than argued, since the claim is that nothing works:
Upstream TypeScript patch — applied to a clone of pierrecomputer/pierre:
$ ./node_modules/.bin/tsc --noEmit ; echo "exit=$?"
exit=0
$ AGENT=1 bun test --timeout 15000
1515 pass, 0 fail
Published-dist patch — applied to node_modules/@pierre/diffs, then a windowed render:
windowed render OK, lines rendered = 25 grammarState returned = true
A ReferenceError on every render would fail both of those on the first call. The benchmark in this PR has also been run repeatedly against the dist patch, including a 48-case correctness sweep.
The readability point stands though, and it is the second reviewer to trip on it — I have added it to the open questions in docs/pierre-windowed-highlight-pr.md as something to rename before sending upstream. isHighlightedWindow is the newcomer, so it is the one that should change.
Generated by Claude Code
There was a problem hiding this comment.
You're right, my mistake. isWindowedHighlight is a pre-existing upstream declaration — the patch adds isHighlightedWindow alongside it, both intentionally live. The verification evidence (tsc clean, 1515 tests passing, dist patch rendering correctly) makes it clear there's no undefined reference here.
I'll withdraw the comment. The near-anagram naming being flagged as a defect rather than a readability concern is the real signal — worth capturing before the upstream PR is opened.
The patch adds isHighlightedWindow next to Pierre's pre-existing isWindowedHighlight. The two are near-anagrams meaning different things, and two separate reviewers have now read the patch and concluded the new flag was a typo for the old one. Note it as an open question so the rename happens before this is sent upstream. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JgbAM65ZsLbsjgqZS8Dtav
A patch we could send upstream to
@pierre/diffs, plus the investigation behind it.Nothing in
src/changes, the patch is not activated, and no upstream PR has been opened.This is the companion to #755. That one is the approach Hunk should take; this one is an upstream contribution that stands on its own merits.
The gap
renderDiffWithHighlighteralready acceptsstartingLine/totalLines, anditerateOverDiffalready implements a complete row-window walk behind them. The very first thing the renderer does is throw the range away unless the caller also asked for plain text:So a caller gets a highlighted whole file or an unhighlighted window, never a highlighted window. The reasoning is sound — a window tokenized alone starts from an empty TextMate rule stack and mis-colors anything inside a block comment or template literal opened earlier — but Shiki can continue from a saved state via
codeToHast(code, { grammarState })andgetLastGrammarState(hast), and Pierre already relies on that inshiki-stream. The window just needs somewhere to carry it.Pierre has a use for this too: their DOM
Virtualizeralready models aRenderRangewithstartingLine,totalLines,bufferBefore,bufferAfter, and hits the same wall.The patch
patches/pierre-upstream-windowed-highlight.patch,git am-ready againstpierrecomputer/pierre@d9eb0ab. Three files, +435/−21: an opt-inwindowedHighlightflag, a per-sidegrammarStatethreaded in and out, and a single-axis iteration walk.Verified against their real toolchain in a clone:
tsc --noEmitcleanpackages/diffssuite: 1504 → 1515 passing, 0 failing (the 11 new tests)oxfmt --checkclean; the oneoxlintwarning reproduces unchanged on theirHEADdiffStyleline fails 4 of the 11 new testsdocs/pierre-windowed-highlight-pr.mdholds the draft PR description and four open questions for the maintainers.The non-obvious part
renderDiffWithHighlighteriterates withdiffStyle: 'both', which advances unified and split row counters independently and emits a line landing in either window. Where the counts diverge, consecutive windows overlap — on a 4000-line rewrite with 250-row windows, 81% of side lines were emitted more than once. Coverage is complete so output looks fine, but grammar-state chaining becomes meaningless, and the symptom was correct-looking output drifting into comment coloring a few hundred lines in.Windowed highlighting therefore walks
'split', which emits each side line once and keeps change pairs on one row — which matters because the word-level diff decorations are computed from a paired change callback.Results
bun run bench:pierre-windowed-highlight. On an 8000-line added file, the longest uninterruptible call drops from ~660ms to ~28ms at 250-row windows. Total CPU is unchanged — windowing saves no work, it just makes the units interruptible.A 48-case correctness sweep compares windowed output byte-for-byte against the stock whole-file render: TypeScript, Python and CSS × new-file, deleted-file, full-rewrite, scattered-edit and no-trailing-newline shapes, plus a partial patch, at window sizes 64/250/1000. All identical. The languages are chosen for the shape of their multi-line constructs, since those are what a window boundary cuts through.
Why this is not Hunk's fix
The worker approach in #755 reaches single-digit milliseconds and does not scale with file size, where windowing's stall is proportional to window size and the main thread still performs every millisecond of the highlighting. Windowing's remaining advantages are that it needs no second Shiki instance and has no compiled-build failure mode.
Running it locally
patches/@pierre%2Fdiffs@1.2.2.patchis the same change hand-applied to the published dist of the pinned version, so the benchmark runs without building Pierre from source. It exposes the identical API and is deliberately not activated inpackage.json— the benchmark detects its absence and reports whole-file cost only rather than failing.Known limitation
Nothing guards against caller misuse: chaining grammar state across a partial diff or across collapsed context silently produces mis-colored output. The flag is opt-in so no existing caller is exposed, and the contract is documented on the option, but whether Pierre should enforce it is one of the open questions for the maintainers.
Verification
typecheck,lint,format:check,knipclean. Conflicts with #755 on two lines each ofpackage.jsonandbenchmarks/README.md(adjacent benchmark-script entries) — whichever merges second needs a trivial resolution.Generated by Claude Code