fix(review): stop diff list re-rendering on every comment keystroke#660
Merged
backnotprop merged 2 commits intomainfrom May 4, 2026
Merged
fix(review): stop diff list re-rendering on every comment keystroke#660backnotprop merged 2 commits intomainfrom
backnotprop merged 2 commits intomainfrom
Conversation
…659) Move the useAnnotationToolbar hook + AnnotationToolbar/SuggestionModal into a new ToolbarHost wrapper so per-keystroke form-state changes no longer re-render AllFilesDiffView / DiffViewer (and therefore not every LazyFileDiff and Pierre <FileDiff> beneath them). Parents talk to the host through an imperative handle for selection/edit/token-click. Verified with a console.count probe at the top of AllFilesDiffView: typing 20+ chars no longer increments the render counter. Out of scope (tracked in #659): React.memo on LazyFileDiff with stable props, React Compiler, server-side prerendering of the review diff, throttling capture-phase scroll listeners.
backnotprop
added a commit
that referenced
this pull request
May 4, 2026
Address PR review nits on #660: - ToolbarHost's window mousemove listener now bails on `!isFocused`, so dock-mounted-but-hidden panels don't track mouse for a toolbar the user can't see. Matches the intent of the existing isFocused prop already passed through to the hook for draft handling. - Widen useAnnotationToolbar's handleMouseMove arg to a structural `{clientX, clientY}` shape. The hook only reads those two fields, so accepting the structural shape eliminates the `as unknown as React.MouseEvent` cast in ToolbarHost while staying compatible with React.MouseEvent callers.
Widen useAnnotationToolbar's handleMouseMove signature to accept the
structural { clientX, clientY } shape — the implementation only reads
those two fields, and this lets ToolbarHost pass it directly to
window.addEventListener without an `as unknown as React.MouseEvent`
cast. Function parameters are contravariant, so any caller passing a
React.MouseEvent or native MouseEvent remains valid.
No runtime change.
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.
Closes part of #659 (fix #1: move toolbar form state out of the parent).
Summary
useAnnotationToolbar+ theAnnotationToolbar/SuggestionModalJSX out ofAllFilesDiffViewandDiffViewerinto a newToolbarHostwrapper component.forwardRef+useImperativeHandle) forhandleLineSelectionEnd,handleTokenClick, andstartEdit.onMouseMove={toolbar.handleMouseMove}on the scroll container becomes a window-levelmousemovelistener inside the host (functionally equivalent — handler only stashesclientX/Y).Why
Users reported the review UI getting laggy when typing in a comment on moderately-sized diffs. We proved with a temporary
console.countprobe at the top ofAllFilesDiffViewthat the counter ticked up on every single keystroke. Cause:commentTextstate lived inuseAnnotationToolbarwhich was called fromAllFilesDiffView, so every keystroke re-rendered the entire all-files view → everyLazyFileDiff→ every Pierre<FileDiff>.After this change, keystrokes only re-render the host. The diff list is unaffected.
Test plan
Manual verification in the dev review UI (
bun run dev:review):Out of scope (tracked in #659)
LazyFileDiff/ Pierre'sFileDiffinReact.memoand stabilizing the inlineoptions/renderAnnotation/renderHoverUtilityprops inAllFilesDiffView. Evaluated as a moderate refactor (per-file closures need aFileRowextraction); deferred since the user-reported lag is gone.preloadPatchFile(already wired up forCodeFilePopoutbut not the main review path).AnnotationToolbar/CommentPopover/FloatingQuickLabelPicker.