perf(ui): single Shiki highlighter, palette-matched code blocks, drop highlight.js - #1218
Merged
Merged
Conversation
@pierre/diffs picks its Shiki engine with a runtime ternary:
engine: preferredHighlighter === "shiki-wasm"
? createOnigurumaEngine(import("shiki/wasm"))
: createJavaScriptRegexEngine()
Plannotator pins `preferredHighlighter: 'shiki-js'` (and Pierre's own
default is 'shiki-js'), so the Oniguruma branch never executes. Because
the choice is a runtime ternary, bundlers keep the `import("shiki/wasm")`
edge anyway and inline `@shikijs/engine-oniguruma/wasm-inlined`, a
~622 KB base64 blob, into the single-file HTML builds. The review app
paid for it twice: once on the main thread (via
`highlighter/shared_highlighter.js`) and once inside the `?worker&inline`
Pierre worker.
Alias `shiki/wasm` to a stub that throws if it is ever reached. Wired via
`resolve.alias` rather than a plugin because `resolve.alias` is shared
with Vite's worker build and `plugins` are not.
Highlighting output is unchanged: the JS regex engine was already the one
doing the work. Opting back into 'shiki-wasm' now fails loudly instead of
silently costing every user a megabyte of dead bytes.
apps/review/dist/index.html 19,424,646 -> 18,180,545 (-1,244,101 raw / -463,348 gzip)
apps/hook/dist/index.html 23,032,467 -> 22,410,416 (-622,051 raw / -233,485 gzip)
The app shipped two highlighters. Shiki already tokenised the code-review diff pane (via @pierre/diffs, JavaScript regex engine); highlight.js separately coloured markdown fences and review suggestion snippets at ~982 KB minified for a full build of ~190 grammars. That second highlighter is now gone. Every call site moves onto `packages/ui/utils/codeHighlight.ts`, a thin wrapper over Pierre's SHARED Shiki instance: CodeBlock, Viewer, PlanCleanDiffView markdown fences InlineMarkdown code-file hover preview HighlightedCode review suggestion snippets Reusing Pierre's instance rather than standing up a second fine-grained one is deliberate. Pierre imports Shiki's full bundle, so every grammar and theme is ALREADY inlined in the single-file builds: a separate highlighter with a curated language list would have duplicated a subset of bytes that are already there. Sharing costs nothing, gives every language Shiki bundles instead of a shortlist, and — the point of the change — guarantees fences resolve the exact same theme the diff pane resolves. Theming. `SHIKI_THEME_MAP` / `resolveSyntaxTheme` move from `packages/review-editor/hooks/usePierreTheme.ts` to `packages/ui/utils/syntaxTheme.ts`; usePierreTheme re-exports them, so the review editor's imports are unchanged. `useFenceTheme()` feeds the components and re-highlights on palette or mode change. Code blocks now follow the active palette across all ~52 themes in both light and dark, instead of always rendering github-dark and relying on hand-written `.hljs-*` override stacks to stay legible. Those stacks are deleted: `packages/editor/index.css`'s light-mode token palette, and `colorblind.css`'s hand-tuned tokens which existed to APPROXIMATE @pierre/theme's protanopia-deuteranopia themes that are now simply used. Behaviour held fixed: - Language-less fences stay plain text (#1212). No auto-detection anywhere, including the hover preview, which previously called `hljs.highlightAuto`. `HighlightedCode` derives its language from the caller's file path; an unknown extension renders plain. - `applyHighlight(el, ...)` keeps the imperative `hljs.highlightElement` DOM contract the annotation layer reaches into, and writes plain text at final size first so async highlighting causes no layout shift. Already-attached grammars highlight synchronously — no flicker on cached highlights. - It also verifies the rendered text is byte-identical to the source and falls back to plain otherwise, because annotations address code blocks by text offset. - `@plannotator/ui`'s public API is unchanged: the highlighter is a module-level default like the package's other seams, no new props. The `hljs` class on fenced `<code>` becomes `pn-code` (it is a structural hook for blockTargeting, vim navigation and print.css, and it named a library we no longer ship). `language-*` stays. apps/review/dist/index.html 18,180,545 -> 17,270,889 (-909,656 raw / -291,921 gzip) apps/hook/dist/index.html 22,410,416 -> 21,704,434 (-705,982 raw / -238,096 gzip) Verified the diff pane is untouched: the rendered Pierre shadow-DOM markup is byte-for-byte identical between an origin/main build and this one (SHA-256 aa1ee88a…).
Two U+0000 bytes slipped into comments in the previous commit, which made git treat the file as binary. Replaced with spaces; no behaviour change.
Fenced code is annotated by hand: one `<mark data-bind-id>` inside the
`<code>` element, which `applyHighlight` also owns. Every highlight swap
(palette change, dark/light toggle, or the first async grammar attach
after load) replaces that element's children, so the mark was silently
wiped and nothing put it back. Annotation state, the sidebar panel and
exports were unaffected; the loss was purely visual, and deterministic.
`applyHighlight` now publishes every write through `onCodeHighlightSwap`,
synchronously, immediately after it. `Viewer` subscribes and re-paints the
fence's mark, so a swapped block ends up with BOTH the new theme's tokens
and its annotation. The shared painter (`paintCodeBlockMark`) moves the
token spans into the mark instead of flattening them to text, so creating
an annotation no longer costs a block its colours either.
Being driven by the swap also fixes the cousin race by ordering rather
than timing: share/draft restore runs on a timer after load, and on a slow
machine the first async swap could land after it and wipe the restored
marks per block. A restore that painted before the swap is now
re-established in the same task the swap ran in, and one that runs after
finds the mark already there.
Removal tombstones the id before re-highlighting, because the host drops
the annotation from state a tick later — without it the swap listener
would paint the just-removed annotation back in, and a fence carrying a
second annotation would end up bare.
Also closes the named gap in the WASM coverage: entry-assets only grepped
source, so a future @pierre/diffs bump could reintroduce the inlined blob
through a different import specifier unnoticed. It now greps the built
`apps/{review,hook}/dist/index.html` for the base64 WASM magic, skipping
on an unbuilt checkout and running for real in the CI job that builds the
bundles.
backnotprop
added a commit
that referenced
this pull request
Aug 6, 2026
…atch based
CI failed two tests that pass locally. Both were timing races, neither was
an app bug.
1. DiffViewer.fullContentSwap: the swap assertion carried a 15s internal
wall-clock budget, which a cold, contended CI runner blows and a warm
laptop clears. Two changes, both aimed at the clock rather than the
symptom:
- The waits are now budgeted in SCHEDULER TURNS, not milliseconds. A
slower box spends longer inside each turn but needs no more of them,
so the budget never has to be retuned for CI hardware.
- Pierre's shared Shiki highlighter is preloaded before mounting. It is
a module singleton, and building it was the entire multi-second cost
the old budget was accidentally measuring; warming it moves that work
into an unbounded await OUTSIDE the observed window. Disposed in
afterAll, because packages/ui/utils/codeHighlight.test.ts asserts the
pre-attachment behaviour of that same singleton.
Verified against an artificially stalled clock: forcing 20s of dead time
into every wait (41s total, far past the old 15s budget) still passes,
and with the cacheKey fix removed it still fails on the assertion (not
as an opaque timeout) in ~12s. A 20-turn budget with the preload removed
and every core saturated also passed 10/10, so 400 turns is a wide
margin rather than a guess.
2. App.archiveReadOnly compared the fenced block's innerHTML before and
after a click. Since #1218, applyHighlight writes plain text first and
swaps in Shiki markup when the grammar attaches, so that MARKUP changes
on its own schedule and the assertion was racing the swap. The test is
checking that the click opened no mutation entry point, which textContent
plus the absence of an annotation <mark> says exactly, and which no
highlight swap can perturb. Latent on main; the branch's run happened to
lose the race.
Also fixed while confirming the above: codeHighlight.test.ts asserted a
GLOBAL precondition ("no grammar attached yet") that any earlier file
attaching a typescript fence invalidates, so
`DOM_TESTS=1 bun test packages/ui packages/editor` failed by file order
alone. It now resets the attachment cache through the existing
__resetCodeHighlightCacheForTests seam and asserts the contract instead of
the run order. Not currently reachable from CI (that file is not in the DOM
list), but one list edit away.
backnotprop
added a commit
that referenced
this pull request
Aug 6, 2026
highlight.js was removed in #1218; the portal bundles Shiki via the diff renderer. Caught by the v0.26.2 dependency audit.
backnotprop
added a commit
that referenced
this pull request
Aug 6, 2026
… render fully (#1219) * fix(review): mint content-derived diff cache keys so single-file tabs render fully Single-file diff tabs have not rendered their full-content diff since v0.26.0: the expansion gap bars show no chevrons and clicking them does nothing, at every file size. @pierre/diffs 1.3.2 (the 1.2.8 -> 1.3.2 bump, upstream "Fix diff rerender in edit mode (#878)") added name-based cacheKey defaulting in FileDiff.render: an unset `fileDiff.cacheKey` becomes the file's name. `areDiffTargetsEqual` — the only identity check its render and highlight caches make — compares nothing but that key. DiffViewer renders each file twice on one surviving FileDiff instance (key={filePath}): first the PARTIAL diff from getSingularPatch, then the AUGMENTED full-content diff from processFile once /api/file-content lands. Neither set a cacheKey, so both defaulted to the filename and Pierre served the stale partial render forever. Only the augmented diff is expandable, hence the dead gap bars. Both diffs now mint content-derived keys (`<path>#<hash>` and `<path>#full#<hash>`), matching how AllFilesCodeView already keys its items — which is why the all-files view was never affected. The hash (not patch.length) matters because Pierre's worker highlight cache is a singleton that outlives remounts. The partial diff needs its own key too: with key={filePath} the instance also survives diff-type and base switches, where a same-named new patch would otherwise hit the same name-keyed stale cache. hashString moves from AllFilesCodeView to utils/hashString.ts so both surfaces mint keys the same way. Covered by a new DOM test that mounts DiffViewer against the real @pierre/diffs renderer, holds the /api/file-content response until the non-expandable partial baseline is asserted, then requires the expansion affordances to reach the pixels. It fails against the unfixed tree. * fix(review): explain why an oversized file's card has no diff Files over the 5 MB review limit are replaced by a contents-free stub (buildOversizedTrackedStub, plus the untracked equivalent), which renders as a header-only card with no counts and no explanation. Users read that as a broken diff. The stub now carries an explicit marker line in its extended header (OVERSIZED_REVIEW_STUB_MARKER). A marker rather than a client heuristic because the only other signal, `Binary files ... differ`, is exactly what a genuine binary file emits, so a heuristic would put a false size-cap explanation on every image in the diff. The marker lives in shared/diff-paths so the browser bundle can detect it without pulling in the node-facing review core; both server runtimes pick it up from review-core, which vendor.sh already copies to Pi. Git ignores unknown extended-header lines and @pierre/diffs parses the stub identically with or without it, so nothing else moves. Which files get stubbed is unchanged. Both review surfaces now render one line under the file header saying the file is over the limit and only a stub is shown. * test(review): make the diff-swap proof machine independent, not stopwatch based CI failed two tests that pass locally. Both were timing races, neither was an app bug. 1. DiffViewer.fullContentSwap: the swap assertion carried a 15s internal wall-clock budget, which a cold, contended CI runner blows and a warm laptop clears. Two changes, both aimed at the clock rather than the symptom: - The waits are now budgeted in SCHEDULER TURNS, not milliseconds. A slower box spends longer inside each turn but needs no more of them, so the budget never has to be retuned for CI hardware. - Pierre's shared Shiki highlighter is preloaded before mounting. It is a module singleton, and building it was the entire multi-second cost the old budget was accidentally measuring; warming it moves that work into an unbounded await OUTSIDE the observed window. Disposed in afterAll, because packages/ui/utils/codeHighlight.test.ts asserts the pre-attachment behaviour of that same singleton. Verified against an artificially stalled clock: forcing 20s of dead time into every wait (41s total, far past the old 15s budget) still passes, and with the cacheKey fix removed it still fails on the assertion (not as an opaque timeout) in ~12s. A 20-turn budget with the preload removed and every core saturated also passed 10/10, so 400 turns is a wide margin rather than a guess. 2. App.archiveReadOnly compared the fenced block's innerHTML before and after a click. Since #1218, applyHighlight writes plain text first and swaps in Shiki markup when the grammar attaches, so that MARKUP changes on its own schedule and the assertion was racing the swap. The test is checking that the click opened no mutation entry point, which textContent plus the absence of an annotation <mark> says exactly, and which no highlight swap can perturb. Latent on main; the branch's run happened to lose the race. Also fixed while confirming the above: codeHighlight.test.ts asserted a GLOBAL precondition ("no grammar attached yet") that any earlier file attaching a typescript fence invalidates, so `DOM_TESTS=1 bun test packages/ui packages/editor` failed by file order alone. It now resets the attachment cache through the existing __resetCodeHighlightCacheForTests seam and asserts the contract instead of the run order. Not currently reachable from CI (that file is not in the DOM list), but one list edit away. * test(review): drop the highlighter preload, harden the swap proof, report why a paint is missing The preload added in the previous commit made CI strictly worse, so it is gone. Before it, CI's partial diff painted and only the swap was missing; with it, CI never painted at all. It was an optimization for a theory the evidence has since killed, and it mutated a process-wide singleton to buy it, so it is not worth keeping while the real failure is unexplained. The afterAll dispose that existed only to undo the preload goes with it. What the CI log actually shows: - The "WorkerPoolManager: operation canceled because the pool terminated" error is inside discardRestoreRender.test.tsx's own group, ~0.3s BEFORE this file's group opens. It is that file's provider unmounting and terminating the pool singleton it created: end-of-file teardown, the same benign noise documented on #1209. It also prints on every local run, where the whole list passes. It is not a mid-test terminator, and nothing in this file uses the worker pool (no WorkerPoolContextProvider is mounted, so useWorkerPool() is undefined and rendering takes the main-thread path). - This file's group prints NOTHING for its whole 10.3s: no console.warn from the stale-content guard, no error. Pierre simply painted nothing. Not reproducible locally: the exact DOM list from test.yml, one bun process, forward and reverse order, 13 runs with every core saturated, all green. So the remaining difference is the environment, which cannot be reasoned out from here. Three changes make the next CI run answer it instead of costing another guess: - renderDiagnostics() dumps what Pierre actually painted (container / separator / chevron / line-number counts plus a markup fragment) when a wait gives up. Prints only on failure, so it is worth keeping. - The precondition is asserted rather than assumed: the REAL getSingularPatch and processFile must produce partial-then-full on these fixtures. Bun's mock.module is process global and an earlier file in this very list mocks '@pierre/diffs', so a leaked mock now fails in milliseconds with a clear message instead of as a render that never arrives. - The first paint is now REPORTED, not asserted. The verdict belongs to the swap; gating on the partial paint let a slow or absent first paint mask the result the test exists for. Removing the cacheKey fix still fails it (verified), because that tree paints no chevrons at any point. Also fixed a real trap in the fixture: the hunk header said @@ -61 while its context lines start at line 59 of both contents. Pierre realigns a misaligned header rather than rejecting it, so it was silently tolerated. * test(review): stop a leaked module mock from silently unrendering the diff tests Root cause, and it was never a timing problem. AllFilesCodeView.lifecycle.test.tsx calls `mock.module('@pierre/diffs', ...)` with a hunk-less `getSingularPatch` and `processFile: () => null`. Bun's module mocks are process global and are not unwound at file boundaries, and that file sits immediately before DiffViewer.fullContentSwap.test.tsx in the DOM step's list. On the Linux runner the stub reached this file; on macOS it did not, which is why 13 local runs of the exact list, both orders, cores saturated, stayed green. It explains both CI symptoms exactly, including the one that looked like a contradiction: `processFile: () => null` means the augmented diff never exists, so no chevrons ever (the failure before the preload); the stub `getSingularPatch` has `hunks: []`, so nothing paints at all (the failure after it). The "WorkerPoolManager: operation canceled because the pool terminated" line was a red herring throughout: it is inside discardRestoreRender's own group, ~0.3s BEFORE this file's group opens, is that file's provider unmounting the pool it created, and prints on every local run too. The precondition assertion added in the previous commit is what proved it, turning a 10.3s mystery into a 0.45ms verdict: 228 | expect(expected?.isPartial).toBe(false); error: expect(received).toBe(expected) Expected: false Received: undefined Fixed at both ends: - Source: the mocking file now captures the real modules before it stubs them and restores both library specifiers in afterAll, so no later file in any run inherits its stubs. This fixes the class for every future DOM test that needs the real renderer, which was the actual leak. - Consumer: the two tests that render against the real @pierre/diffs get their own CI step, the same isolation (and for the same kind of reason) this workflow already gives useFileBrowser.test.tsx. They are removed from the shared list so that step is their single source of truth. The restore above should make this unnecessary; it is not something to bet a green build on from a machine that cannot reproduce the platform behaviour. Verified with a CI-faithful harness: one bun process per step, the exact lists from test.yml, isolated + shared-forward + shared-reverse, 10 iterations with every core saturated, then 6 more after the final split. All green, plus the full suite and typecheck. * docs(test): point the diff-renderer DOM tests at the CI step that actually runs them
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TLDR: the app shipped two syntax highlighters and one dead WebAssembly engine. This drops both.
highlight.jsis deleted, markdown fences and review suggestion snippets move onto the Shiki instance@pierre/diffsalready runs for the diff pane, and the never-executed Oniguruma WASM is aliased out of every bundle. The visible payoff: code blocks finally follow the active palette across all ~52 themes in light and dark, instead of always rendering github-dark behind hand-written override CSS. The review diff pane is unchanged, proven byte-for-byte below.Builds on #1212 by @zeke, which made language-less fences render plain and thereby removed the last thing only highlight.js could do. Refs #1210.
Bundle math
Single-file HTML builds, raw bytes and
gzip -9:apps/review/dist/index.htmlrawapps/review/dist/index.htmlgzipapps/hook/dist/index.htmlrawapps/hook/dist/index.htmlgzipapps/hook/dist/review.htmlis a copy of the review build and moves with it. The OpenCode plugin re-ships both HTMLs verbatim, so it inherits the same reduction.Phase A: the Oniguruma WASM was dead weight
@pierre/diffspicks its Shiki engine with a runtime ternary:We pin
preferredHighlighter: 'shiki-js'(packages/review-editor/workerPool.tsx), and Pierre's own default is'shiki-js', so the Oniguruma branch never runs. A runtime ternary is not tree-shakeable though, so bundlers keep theimport("shiki/wasm")edge and inline@shikijs/engine-oniguruma/wasm-inlinedanyway: a 622,442-byte base64 blob. The review app paid for it twice, once on the main thread viahighlighter/shared_highlighter.jsand once inside the?worker&inlinePierre worker. The plan editor paid once, reaching Pierre throughCodeFilePopout.Fix: alias
shiki/wasmtobuild/shiki-wasm-stub.tsin the review, hook and portal Vite configs. Wired throughresolve.aliasrather than a plugin, becauseresolve.aliasis shared with Vite's worker build andpluginsare not. The stub throws if it is ever reached, so opting back into'shiki-wasm'fails loudly instead of silently costing every user a megabyte.Verified: the base64 WASM marker occurs 2x in the baseline review build and 1x in the baseline hook build, and 0x in both after.
Phase B: one highlighter, and fences that match the palette
highlight.js was ~982 KB minified for a full build of ~190 grammars, colouring markdown fences (
CodeBlock,Viewer,PlanCleanDiffView), the code-file hover preview (InlineMarkdown), and review suggestion snippets (HighlightedCode). All five now callpackages/ui/utils/codeHighlight.ts.Deviation from the plan, and why
The plan called for a second, fine-grained Shiki highlighter with a curated 40 to 80 language list. Investigation showed that would have been strictly worse: Pierre imports Shiki's full bundle, so every grammar and every theme is already inlined in these single-file builds (
source.rust,source.haskell,kanagawa-waveand friends are all present in the baseline artifacts). A separate highlighter would have duplicated a subset of bytes that are already there.So this reuses Pierre's shared instance through its public API (
getSharedHighlighter,getHighlighterIfLoaded). That costs zero additional bytes, supports every language Shiki bundles rather than a shortlist, adds no new dependency, and, the actual point, makes fences resolve the exact same theme object the diff pane resolves.Theming
SHIKI_THEME_MAP/resolveSyntaxThememove frompackages/review-editor/hooks/usePierreTheme.tstopackages/ui/utils/syntaxTheme.ts;usePierreThemere-exports them, so the review editor's imports are untouched. A newuseFenceTheme()hook resolves(colorTheme, resolvedMode)to a concrete Shiki theme name and re-highlights on palette or mode change. Palettes with no Shiki counterpart fall back to@pierre/diffs' ownpierre-dark/pierre-light, matching what the diff pane already does when handed no theme.Two hand-written CSS stacks that existed purely to survive github-dark are deleted:
packages/editor/index.csslight-mode.hljs-*token colours, which existed because highlight.js always emitted a dark theme.packages/ui/themes/colorblind.csstoken palette, whose own comment said it "mirrors @pierre/theme's protanopia-deuteranopia shiki themes". Those themes are now simply used.Behaviour held fixed
hljs.highlightAuto.HighlightedCodederives its language from the caller's file path viadetectLanguage, and an unrecognised extension renders plain.applyHighlightwrites plain text at final size immediately, then swaps in highlighted markup. Once a grammar is attached it highlights synchronously, so cached highlights never flash.@plannotator/uipublic API unchanged. The highlighter is a module-level default like the package's other seams. No new required props onCodeBlockorViewer..wasmstring left in the built HTML is a TextMate scope name inside a theme (entity.name.type.wasm). A test now asserts this.The
hljsclass on fenced<code>becomespn-code. It is a structural hook used byblockTargeting, vim navigation andprint.css(pre > code.pn-code), and it named a library we no longer ship.language-*is unchanged.Maintenance rationale
Separately from the bundle: highlight.js's npm channel has been frozen at 11.11.1 since December 2024 while fixes land only in git, including an XML-grammar ReDoS fix. There is no fixed npm version to upgrade to, so this removes a dependency we could not have patched.
The theming improvement
Screenshots (same document, multiple palettes). Fences track the palette; the untagged block stays plain in all of them.
GitHub Light is the clearest case of the old bug: before this change a light palette still got github-dark tokens, kept legible only by the
.light .hljs-*override stack. Colorblind is the clearest case of the fix: it now renders the realpierre-dark-protanopia-deuteranopiatheme rather than a hand-tuned approximation of it.DOM probe backing the screenshots, counting styled token spans per fence in each palette:
#9D6AFB#7E9CD8#005CC5#BA8FFD#5731A7No regression in the diff pane
The Pierre render path is untouched in source. To prove it end to end, the rendered diff pane (Pierre's shadow DOM, minus its
<style>blocks) was serialised from a compiled binary built atorigin/mainand from one built at this branch tip, against the same repository and the samekanagawa-wavepalette:Byte-for-byte identical, which is expected: the JS regex engine was already the one doing the tokenising, and nothing here enables the WASM branch.
Verification
bun test: 2912 pass, 246 skip, 0 fail (3158 tests across 264 files)DOM_TESTS=1 bun test packages/ui packages/review-editor: 927 pass, 0 failbun run typecheck: cleanapps/review, thenbuild:hook, thenbuild:opencode): cleanpackages/ui/utils/codeHighlight.test.ts(theme resolution per mode plus fallbacks, the language-less-stays-plain invariant, the plain-first render) and three new assertions intests/entry-assets.test.ts(no highlight.js dependency, no external URLs in the highlighter,shiki/wasmaliased in all three bundled apps)Migration notes
.hljs-*-style token CSS. Pick the right Shiki theme inSHIKI_THEME_MAPinstead. The Syntax Highlighting section ofAGENTS.mdis rewritten to say so.@plannotator/uineed no code changes. If any of them styled.hljsor.hljs-*inside our code blocks, those selectors are now inert and should target.pn-codeor the theme map.highlight.jsis removed from thepackages/uiandpackages/review-editordependencies.Review findings addressed
Review flagged one real regression in this branch, plus a named test gap. Both are fixed in
cc72ea24.Theme or mode switch wiped code-block annotation marks
Fenced code is annotated by hand, all-or-nothing: one
<mark data-bind-id>inside the<code>element.applyHighlightowns that same element's children, so every swap it makes (palette change, dark/light toggle, or the first async grammar attach after load) destroyed the mark, and nothing put it back. Annotation state, the sidebar panel and exports were all unaffected, so the loss was purely visual, but it was deterministic: annotate a fence, switch palette, the highlight is gone.The fix makes the swap itself responsible for restoring the mark, rather than trying to protect the mark from the swap:
applyHighlightnow publishes every write it makes through a newonCodeHighlightSwap(listener)seam inpackages/ui/utils/codeHighlight.ts. Listeners run synchronously, immediately after the write.Viewersubscribes and re-paints the fence's annotation mark right there, so a swapped block ends up with both facts correct at once: the new theme's token colours and the annotation.paintCodeBlockMark,packages/ui/utils/codeBlockMark.ts) and moves the token spans into the mark rather than flattening them to text. Creating a code-block annotation therefore no longer costs the block its colours either, which it used to. Creation and restoration now produce identical DOM.The rejected alternative was skipping the
innerHTMLrewrite whenever the element already holds a[data-bind-id]. That keeps the mark but leaves annotated blocks stranded in the previous theme's colours, which is the same class of bug in the other direction.The share and draft restore race
Restore fires on a timer roughly 100 to 120ms after load, and on a slow machine the first async highlight swap could land after it and wipe the just-restored marks block by block. Because re-application is now triggered by the swap, this is ordered by construction instead of by timing: a restore that painted before the swap is re-established in the same task the swap ran in, and a restore that runs after the swap finds the mark already present and leaves it alone. No sleeps, no timers, nothing to tune.
Removal must still mean removal
Removing an annotation re-highlights the block on its way out, and the host only drops it from state on the next tick, so for one tick the swap listener sees a list that still names the annotation whose mark was just deleted. The id is tombstoned before the re-highlight and retired as soon as the host's list agrees it is gone (so a draft restore can bring the same id back). Without the tombstone a fence carrying a second annotation ends up bare after the first is deleted, which the test below pins.
Built-artifact WASM assertion
The existing
tests/entry-assets.test.tschecks only grepped source, so a future@pierre/diffsbump could reintroduce the inlined Oniguruma blob through a different import specifier and CI would never notice. It now also greps the builtapps/review/dist/index.htmlandapps/hook/dist/index.htmlfor the base64 WASM magicAGFzbQand fails if present.dist/is gitignored, so the assertion skips cleanly on an unbuilt checkout, and a new step in theopencode-v2job runs this file straight after its build so it is not silently optional in CI. Verified both directions: it passes on the real bundles and fails when the marker is injected into one.Tests
packages/ui/components/Viewer.codeBlockHighlightSwap.test.tsx(three DOM tests) andpackages/ui/utils/codeBlockMark.test.ts(four unit tests), both added to the CI DOM job:expect(survivor).not.toBeNull()receivingnull.@pierre/diffsis stood in for through a test seam, and the stand-in hands back a release function, so the test decides exactly when the async swap lands relative to the restore it races. It restores first, then releases the swap on top, and asserts the mark is still bound afterwards with the tokens highlighted. Deterministic ordering hook, not a sleep. Also fails against the unfixed code.Verification after the fix
bun test: 2915 pass, 252 skip, 0 fail (3167 tests across 266 files)DOM_TESTS=1 bun test packages/ui packages/review-editor: 934 pass, 0 failbun run typecheck: cleanapps/review, thenbuild:hook, thenbuild:opencode): clean, and the bundles are byte-size unchanged from the numbers in the table above (apps/review/dist/index.html17,271.01 kB,apps/hook/dist/index.html21,705.52 kB)bun test tests/entry-assets.test.tsagainst the freshly built bundles: 14 pass, including the two new artifact assertionsAI-assisted: implemented by Claude (Claude Code) with human review.