Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f1bb768378
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
0e13fc5 to
f0441f2
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
| } | ||
|
|
||
| /** Tokenization and theme resolution owned by one highlighter backend. */ | ||
| export interface DiffsHighlighter { |
There was a problem hiding this comment.
Why not make this a class to make future updates easier and reduce verbosity downstream?
I also think it makes for a better developer experience when exploring code, because you can jump to the implementation easier, see how stuff works, etc.
By doing that, we can still keep entrypoints separate by using type imports.
| return true; | ||
| const names = Array.isArray(languages) ? languages : [languages]; | ||
| if (highlighter != null) | ||
| return highlighter.hasLoadedLanguages?.(names) ?? true; |
There was a problem hiding this comment.
I think going back to the class version of highlighter, we could also make this function not optional, and just have the highlights variant return true, which cleans up code where it's used.
| private options: CodeViewOptions<LAnnotation, Caret>; | ||
| private workerManager: WorkerPoolManager | undefined; | ||
| private isReadySubscription: (() => void) | undefined; | ||
| private pendingHighlighterType: HighlighterTypes | undefined; |
There was a problem hiding this comment.
Would it help simplify things downstream (both here and elsewhere in the code) that once you've tried to load a highlighter, you can never load another highlighter type for a given session?
I only say that because i think it can help make our code stronger, but also help identify misconfiguration errors that people might have.
I.E. maybe a good example might be -- if shiki is the default, but someone wants to use highlights, but maybe there's some race condition or something in their code that forces shiki to load first, and then their code to set highlights comes next and now they've loaded 2 highlighters.
| "dist/components/web-components.js", | ||
| "dist/worker/worker.js", | ||
| "dist/worker/worker-portable.js", | ||
| "src/worker/worker.ts" |
There was a problem hiding this comment.
are we sure src/worker/worker.ts should be here?
Also, i think something I would maybe like to do after this PR, is rework the workers a bit. There's actually no need for portable anymore, and I think what I'd want to do is basically make 3 separate workers that each load their highlighter synchronously.
| } | ||
|
|
||
| /** Incremental tokenization and bracket ranges independent of a grammar engine. */ | ||
| export interface DiffsLiveTokenizer { |
There was a problem hiding this comment.
Kinda feel like this too should be a class
| decorations = [], | ||
| lineOffsets = [], | ||
| }: RenderTokenLinesOptions | ||
| ): ElementContent[] { |
There was a problem hiding this comment.
Also, some general codex thoughts on performance improvements by codex:
-
Walk tokens and decoration boundaries together.
InappendDecoratedTokens(), every token scans every decoration on its line. Then every emitted fragment scans all those decorations again to find its wrappers.With
Ttokens,Ddecorations, andFresulting fragments, that’s roughlyT × D + F × Drange checks, plus sorting a fresh boundary array for every token.Instead, sort decoration boundaries once and advance through them as token positions increase. Finished decorations can be discarded; future decorations don’t need checking yet. Our generated diff spans are already ordered and non-overlapping, so that case can use a straightforward two-cursor walk. Nested/custom decorations need more careful handling.
-
Avoid building an intermediate token array for ordinary lines.
The normalization loop (renderTokenLines.ts:52) allocates an array for every line, sometimes creates replacement token objects, then another loop immediately turns those tokens into HAST.For lines without decorations, whitespace handling could emit HAST directly. That removes an intermediate array and a second token traversal from the common path. A smaller first step: reuse the original tokens when normalization makes no changes.
-
Skip the editor’s fragment-wrapping pass on undecorated lines.
wrapTokenFragments()runs for every editor line. It recursively walks the newly created tree and builds replacement child arrays.Without decorations, we already emitted one span per nonempty normalized token with its
data-charposition. There are no decoration-created fragments to reunite. That looks like a useful place to skip the pass while preserving the empty-line<br>behavior. -
Precompute each decoration’s bounds for the current line.
The inner loops repeatedly evaluate whether a decoration starts/ends on this line and derive its numeric bounds. Compute{ from, to, decoration }once per line. Likewise, accumulate line length during normalization instead of doing anotherreduce(), and allocate the empty-markerSetonly when empty decorations exist.
I had codex apply these changes (and these tests):
https://gist.github.com/amadeus/78577f378b37bec94866bd10edd626ae
and it came up with these benched improvements:
Synthetic benchmark: 200 lines × 100 input tokens, warmed median timings on Bun 1.4.0.
| Scenario | Before | After | Speedup |
|---|---|---|---|
| Plain | 0.603 ms | 0.467 ms | 1.29× |
| Plain with whitespace | 0.629 ms | 0.607 ms | 1.04× |
| Editor with whitespace | 1.730 ms | 1.129 ms | 1.53× |
| One decoration per line | 1.139 ms | 0.599 ms | 1.90× |
| Sparse decorations | 0.726 ms | 0.585 ms | 1.24× |
| 50 decorations per line | 8.811 ms | 2.691 ms | 3.27× |
| Nested decorations | 1.989 ms | 0.866 ms | 2.30× |
| Empty markers | 8.833 ms | 2.403 ms | 3.68× |
These measure token-to-HAST conversion only, excluding tokenization and DOM work.
| @@ -0,0 +1,141 @@ | |||
| import type { Element, Nodes, Properties, RootContent } from 'hast'; | |||
| import { toHtml } from 'hast-util-to-html'; | |||
There was a problem hiding this comment.
Codex came up with some more performance optimizations for this, just in case: https://gist.github.com/amadeus/4759cd218da454c88918fd6c7b4e75c8
| Input | Before | After | Less time |
|---|---|---|---|
| Large highlighted file | 2.23 ms | 1.80 ms | 19% |
File with data-char |
4.06 ms | 3.87 ms | 5% |
| Split diff | 1.04 ms | 0.88 ms | 16% |
| Unified diff | 0.73 ms | 0.62 ms | 15% |
| Synthetic escape-heavy spans | 18.29 ms | 6.18 ms | 66% |
| return styles; | ||
| } | ||
|
|
||
| function getThemeColors(theme: DiffsTheme, highlighter: DiffsHighlighter) { |
There was a problem hiding this comment.
Maybe a quick comment on why this is needed?
What happens if it's highlights type but cssVariables is false/undefined? What does that mean?
| * `WorkerPoolManager.highlightsOnMainThread`), leaves highlighting to the | ||
| * surface's local highlighter, which still uses the pool's render options. | ||
| */ | ||
| export function highlightsInWorkers( |
There was a problem hiding this comment.
Based on my previous comment, this should ultimately go away when we move workerpool back to working as it did before.
| return this.asyncHighlight(diff) | ||
| .then((fresh) => this.applyRefreshedResult(diff, fresh)) | ||
| .then((fresh) => { | ||
| if (preferredHighlighter !== this.options.preferredHighlighter) return; |
There was a problem hiding this comment.
I think a ton of stuff in this renderer and the file renderer will be reverted if don't need to support multiple highlighters simultaneously.
|
Great start on this! I think there's some high level stuff that was can simplify with this architecture, and the top level feedback here is:
Also as a follow up to this PR, I would love to maybe re-work the worker stuff a bit, and maybe make 3 different entry points for the different types, and not async load the highlighters in each of those workers, so they load up maybe a bit faster and are better self contained. |
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
This branch was successfully deployed
2 participants
Integrates
@pierre/highlightsas a highlighter forFile,DiffFile,Editor, streams, SSR, and worker pools. Main change:DiffsHighlighterreplaces the raw Shiki interface.shiki-js,shiki-wasm, andhighlightsload lazily and have separate cached instances. Shiki JS remains the default.Usage
To use
@pierre/highlightsas the highlighter, setpreferredHighlighterto'highlights'(default isshiki-js):👉 Playground
Benchmark
API Changes
HighlighterTypesadds'highlights'. SetpreferredHighlighteron components, SSR options, or worker pool initialization. The default remains'shiki-js'; a worker pool’s choice overrides component options.getSharedHighlighter()returns the backend-neutralDiffsHighlighter. NewcreateHighlighter({ preferredHighlighter })creates an independently disposable instance.getHighlighterIfLoaded()acceptspreferredHighlighter, with optionalthemeandlangchecks.isHighlighterLoaded(),isHighlighterLoading(), andisHighlighterNull()accept a backend name and default to'shiki-js'.disposeHighlighter()disposes all cached backends and clears the shared theme and language caches; instances fromcreateHighlighter()are left alone and keep the themes they have already used.DiffsHighlighterno longer exposes Shiki methods such ascodeToHast(),setTheme(), orloadLanguage(). UsecodeToTokens(),codeToHtml(),themeResolver, and optionalloadLanguages()/hasLoadedLanguages()/attachLanguages().attachResolvedLanguages()delegates to the backend’sattachLanguages()and is a no-op for Highlights.getTheme()returnDiffsTheme, containing shared colors and optionaltextmate/zedpalettes. Theme resolution helpers accept an optional backend argument, defaulting to'shiki-js'.registerCustomTheme(name, loader, type = 'textmate')accepts loaders returning TextMate themes, ZedTheme/ThemeFamilyobjects, or portableDiffsThemeobjects. The optional third argument selects'textmate'for Shiki or'zed'for Highlights, preserving existing two-argument TextMate calls. Both formats can share a theme name.createCSSVariablesTheme()now returns a portableDiffsThemeinstead of re-exporting Shiki’screateCssVariablesTheme, with a default variable prefix of--diffs-(Shiki’s default was--shiki-). PassvariablePrefix: '--shiki-'to keep stylesheets written for the old default, or importcreateCssVariablesThemefromshikiwhen a raw Shiki registration is needed.highlighter.createStreamTokenizer()orhighlighter.createLiveTokenizer(). The publicShikiStreamTokenizerand its option/result types are removed.CodeToTokenTransformStreamnow requires aDiffsHighlighterand disposes its tokenizer on completion, cancellation, or failure. Stream tokenizers emit each line-break character as its own token ('\r'then'\n'for CRLF) so token text round-trips the source;FileStreamrenders a'\r'token as an empty span.codeToHtml,createTransformerWithState, andAttachedThemesexports are removed. Usehighlighter.codeToHtml()and backend theme checks. Shiki-specific type re-exports—includingBundledLanguage,CodeToHastOptions,LanguageRegistration,ShikiTransformer,ThemeRegistration, andThemeRegistrationResolved—must now be imported fromshiki.registerCustomLanguage()continues to support Shiki grammars. Highlights uses bundled lexers and renders unsupported languages as plain text.