perf(highlight): load Shiki WASM bytes without base64 decoding - #1078
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Greptile SummaryThis PR redirects Pierre’s Bun-targeted
Confidence Score: 4/5The runtime change appears sound, but the explicit filename and test-import requirements must be satisfied before merging. The adapter’s important source, worker, bundle, and executable paths have targeted regression coverage, and no behavioral or security failure remains; the accepted findings are repository-rule compliance issues. Files Needing Attention: packages/hunk/src/lib/shikiWasm.ts, packages/hunk/src/lib/shikiWasmAssets.d.ts, packages/hunk/src/lib/shikiWasm.test.ts Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart LR
Pierre["@pierre/diffs highlighter"] --> Alias["tsconfig alias: shiki/wasm"]
Alias --> Adapter["Bun WASM adapter"]
Adapter --> Asset["shiki/onig.wasm"]
Asset --> Read["Bun.file(...).arrayBuffer()"]
Read --> Instantiate["WebAssembly.instantiate(imports)"]
Instantiate --> Engine["Oniguruma highlighting engine"]
Prompt To Fix All With AI### Issue 1
packages/hunk/src/lib/shikiWasm.ts:1
**TypeScript filenames violate convention**
The new `shikiWasm.ts` file uses camel case, while the repository requires dash-case names for TypeScript files. The same violation appears in `shikiWasmAssets.d.ts` and `shikiWasm.test.ts`. Rename these files and update their references before merging.
### Issue 2
packages/hunk/src/lib/shikiWasm.test.ts:23
**Tests use dynamic imports**
The fresh-process test program dynamically imports `@pierre/diffs` here and again in the worker program at line 57. This violates the repository testing directive to hoist imports to the top level. Preserve the required instrumentation order through a preload or fixture entry instead, and satisfy this requirement before merging.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "perf(highlight): load Shiki WASM bytes w..." | Re-trigger Greptile |
96721e9 to
ad372a3
Compare
Redirect Pierre's shiki/wasm import to an asynchronous Bun asset loader. Keep the original engine bytes, embed them in compiled binaries, and resolve npm assets beside the bundle rather than the caller's cwd. Cover stock-token parity, zero base64 calls, relocated bundles/binaries, and small-diff PTY highlighting.
ad372a3 to
63a5666
Compare
Follow-up to #1063. Second of two first-interaction fixes; independent of the help/menu overlay PR.
Problem
Pierre requests the Oniguruma engine via
import("shiki/wasm"), which resolves to@shikijs/engine-oniguruma/wasm-inlined: a 622 KB base64 literal decoded withUint8Array.from(atob(...))on the main thread the first time a highlighter is prepared. That runs right after the first frame, so it lands between the user's first keypress and its render. Measured (n=5, median / max):Approach
A
tsconfigpathsalias mapsshiki/wasm→packages/hunk/src/lib/shikiWasm.ts, which reads Shiki's publicshiki/onig.wasmasset through Bun's file loader and callsWebAssembly.instantiate. Same bytes, engine, grammars, themes and Pierre lifecycle; token output is byte-identical (SHA-256 checked in tests). No dependency patching or vendoring.shiki@3.23.0becomes an explicit root devDependency (already present transitively at that version).After: string→bytes conversions 1 → 0; Linux prep 51.9 → 20.0 ms, largest gap 33.9 → 12.9 ms; macOS prep 12.5 → 5.8 ms.
The alias applies to source runs,
build:bin(asset embedded — verified/$bunfs/root/onig-*.wasm, andB:/~BUN/root/...on a Windows cross-compile),build:npm(asset emitted besidemain.js, resolved against the module directory, not cwd), the highlight worker, andbun test. The OpenTUI Node facade externalizes@pierre/diffs, so downstream Node consumers keep upstream Shiki behaviour — this adapter is Bun-only by design.Tests / checks
packages/hunk/src/lib/shikiWasm.test.ts: fresh-process zero-decode assertions for a source entry, the real highlight worker entry, a relocated JS bundle, and a compiled executable; stock-engine vs adapter token parity (TypeScript + Elixir); exact asset size so an upstream change forces review.test/pty/highlighting.test.ts: new small-diff (main-thread) highlighting case alongside the existing worker/Elixir cases; passes for source, compiled binary and npm main on macOS and Linux.bun run typecheck,deps:check(alias-only entry accounted for in.dependency-cruiser.cjs),lint,format:check,build:bin,build:npm— pass.bun run test— same 6 pre-existing failures asmain; 1 remaining (jjnot installed) with isolated config.bun run test:integration— the 5 failures reproduce onmain(macOS/varvs/private/varTMPDIR trust-state cases; 3 signal-lifecycle cases).bun run test:tty-smoke— Linux 10/10; macOS skipped (no util-linuxscript).bench:highlight-prefetch/bench:interaction-latency: no material change; macOSnext_file_ready_msis bimodal (10 / 50 ms) in both stock and candidate runs withadjacent_ready_before_move=1throughout, so it is measuring act/render settling rather than highlighting.Pre-existing, not addressed
build:npmdoes not emithighlightWorkerEntry.js, so the npm large-file worker PTY test fails onmainand here identically.