pref(editor): Introduce postponeBackgroundTokenizeToNextFrame method for the DiffsEditor#766
pref(editor): Introduce postponeBackgroundTokenizeToNextFrame method for the DiffsEditor#766ije wants to merge 9 commits into
postponeBackgroundTokenizeToNextFrame method for the DiffsEditor#766Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
This PR aims to reduce UI freezes during editor-driven rerenders (selection updates, scrolling, and component renders) by deferring/pacing background tokenization work to the next animation frame and tightening the tokenizer’s background scheduling.
Changes:
- Added
postponeBackgroundTokenizeToNextFrame()to theDiffsEditorAPI and invoked it during render and selection/scroll operations. - Enhanced
EditorTokenizerbackground processing with pause/resume support and message listener lifecycle management, plus added tests for these behaviors. - Updated
syncWithRenderto use an explicitcomponentType('file' | 'file-diff') instead of an edit mode flag.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/diffs/test/editorTokenizer.test.ts | Adds coverage for background-tokenization listener lifecycle, pause/resume, and message validation. |
| packages/diffs/src/types.ts | Extends DiffsEditor with componentType and the new postpone method. |
| packages/diffs/src/editor/tokenzier.ts | Implements pause/resume, safer message parsing, and attaches the message listener only while background tokenization runs. |
| packages/diffs/src/editor/editor.ts | Stores component type, adds postponeBackgroundTokenizeToNextFrame(), and calls it during selection/scroll handling. |
| packages/diffs/src/components/FileDiff.ts | Uses componentType 'file-diff' and postpones background tokenization during render. |
| packages/diffs/src/components/File.ts | Uses componentType 'file' and postpones background tokenization during render. |
| apps/demo/src/main.ts | Enables editor debug logging in the demo for easier validation of scheduling changes. |
Comments suppressed due to low confidence (2)
packages/diffs/src/components/File.ts:481
syncWithRenderis called withthis.lineAnnotations(aLineAnnotation[]), but the editor/tokenizer pipeline expects diff-style annotations (DiffLineAnnotationwith aside). Provide a defaultsidewhen passing annotations to the editor (e.g. treat file annotations as additions) so the types and downstream editor helpers stay consistent.
editor.syncWithRender(
'file',
highlighter,
fileContainer,
file,
this.lineAnnotations,
this.renderRange
);
packages/diffs/src/components/File.ts:670
- Same issue as the other
syncWithRendercall:this.lineAnnotationslacks asidebut the editor expectsDiffLineAnnotation. Add a defaultsidewhen forwarding annotations so the editor can safely track and update them across edits.
editor.syncWithRender(
'file',
highlighter,
fileContainer,
file,
this.lineAnnotations,
this.renderRange
);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // postpone background tokenizing to next frame for avoiding UI freeze | ||
| // during render | ||
| editor.postponeBackgroundTokenizeToNextFrame(); | ||
| } |
There was a problem hiding this comment.
Mostly a nit, but could this just be simplified to a single line without an if statement?
this.editor?.postponeBackgroundTokenizeToNextFrame()| // postpone background tokenizing to next frame for avoiding UI freeze | ||
| // during render | ||
| editor.postponeBackgroundTokenizeToNextFrame(); | ||
| } |
There was a problem hiding this comment.
Mostly a nit, but could this just be simplified to a single line without an if statement?
this.editor?.postponeBackgroundTokenizeToNextFrame()| } | ||
| const contentEl = codeElement.children[1] as HTMLElement | undefined; | ||
| if (contentEl === undefined) { | ||
| if (contentEl === undefined || contentEl.dataset.content === undefined) { |
There was a problem hiding this comment.
Question around code style, in the rest of diffs, I typically do: == null or != null to test undefined-ness. I only use === undefined if I need to differentiate between null or undefined. Not that this has to change, but mostly just a style convention incase you are curious.
There was a problem hiding this comment.
sure sure! i will follow the code style in another PR.
| const tokenizer = this.#tokenizer; | ||
| if (tokenizer !== undefined) { | ||
| tokenizer.pauseBackgroundTokenize(); | ||
| requestAnimationFrame(() => { |
There was a problem hiding this comment.
I have some utilities in diffs to manage request animationFrame to ensure things are all tied to a single callback queueRender and dequeueRender ( https://github.com/pierrecomputer/pierre/blob/editor/perf/tokenzier/packages/diffs/src/managers/UniversalRenderingManager.ts i should fix the names, they suck, but it is what it is).
Ideally though you'd create a function on the class itself, so you're always passing the same function in, that way you don't have to worry about double firing things, etc.
And then also cleanup can properly clean up these callbacks to not potentially have them firing after an unmount or whatever
There was a problem hiding this comment.
nice! all(4) requestAnimationFrame callbacks in editor.ts are arrow functions, and they are passive to be called. In resumeBackgroundTokenize method we also check isPaused state, i think it's ok to use requestAnimationFrame here?
| } | ||
|
|
||
| #listenContentElement(contentEl: HTMLElement): void { | ||
| const guttterEl = contentEl.previousElementSibling as HTMLElement | null; |
There was a problem hiding this comment.
should this be gutterEl not guttterEl?
|
@amadeus i just fixed the typo, reduced some unnecessary |
when doing re-render(selection/code) postpone the background tokenizing to next frame. this can avoid UI freezing for a large file editing.