Repository navigation
fix(react): keep the suggestion menu reference while the menu does not move - #3249
safaweslati wants to merge 1 commit into
Conversation
…t move SuggestionMenuController and GridSuggestionMenuController built the popover reference with an inline selector, so it was a new object on every store update. GenericPopover then set its position reference again on each keystroke. With fast typing, React counted these queued updates as nested updates and threw error 185 after about 50 keys. The reference now changes only with the editor element and the values of referencePos. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@safaweslati is attempting to deploy a commit to the TypeCell Team on Vercel. A member of the Team first needs to authorize it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 3 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title names the fix (keeping the suggestion menu reference stable while the menu does not move) and matches the main change in SuggestionMenuController and GridSuggestionMenuController, which memo… Description check The description follows the repository template: it has Summary, Rationale, Changes, Impact, Testing, Screenshots/Video, Checklist and Additional Notes. It explains the root cause (unstable reference … Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Title check
-
Explanation
The title names the fix (keeping the suggestion menu reference stable while the menu does not move) and matches the main change in SuggestionMenuController and GridSuggestionMenuController, which memoize the popover reference. It is specific and follows the conventional-commit style. It does not name the crash (React error 185) but that is not required.
Full details: Description check
-
Explanation
The description follows the repository template: it has Summary, Rationale, Changes, Impact, Testing, Screenshots/Video, Checklist and Additional Notes. It explains the root cause (unstable reference from useExtensionState and getBoundingClientRect().toJSON(), with GenericPopover's effect causing nested updates and error 185), lists the changes, and describes the new test and manual verification. The Documentation checklist item is unchecked and not explained, and Additional Notes says None. These are minor gaps and do not block the review.
-
✨ Finishing Touches: 💡 1
-
🛠️ Fix failing CI checks: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
A rabbit taps / with care,
Then : and query fill the air.
The menu reference stays in place,
Through each small typing change of pace.
The bunny hops, pleased with the trace.
Comment @coderabbitai help to get the list of available commands.
Summary
Fast typing while a suggestion menu trigger is active can crash a React app with error 185 ("Maximum update depth exceeded"). This PR keeps the
referencethatSuggestionMenuControllerandGridSuggestionMenuControllergive toGenericPopoverstable while the menu does not move.Rationale
Both controllers build
referencewith an inlineuseExtensionStateselector. The selector returns a new object on every store update. Also,SuggestionMenuViewwritesreferencePoswithgetBoundingClientRect().toJSON(), which is a new object on every update, also when the menu stays at the same place.GenericPopoverhas an effect onprops.referencethat callsrefs.setPositionReference(...). That is a state update in Floating UI. So each keystroke adds one more update that runs after the commit.When keys arrive back to back, the browser handles the input before React gets to that update. Each sync commit then finishes with the update still waiting, and React counts it as a nested update. After about 50 keys, React throws error 185. It is not an infinite loop. It is a counter that the queued updates fill.
We see this in production. A user types
/(or another trigger character) and then a sentence, quickly, on a slow machine. We reproduce it with Playwright and with Chrome DevTools CPU throttling at human speeds: 4x slower CPU at 100 ms per key, and 6x slower CPU at 150 ms per key.Changes
SuggestionMenuControllerandGridSuggestionMenuControllerbuildreferencewithuseMemo. The memo uses the editor element and thex,y,widthandheightvalues ofreferencePos, not the object identity.getBoundingClientRectbuilds aDOMRectfrom those values, so it never returns a stale rectangle.SuggestionMenuController.reference.test.tsx, opens each menu, types a query and checks thatGenericPopovergets one reference for the whole query.Impact
When the anchor moves (the query wraps to a new line, or the editor scrolls), the values change, so the popover gets a new reference and moves as before. When the anchor does not move, the popover no longer sets its position reference again on each keystroke.
Testing
main(5 references for 5 keys, in both controllers) and passes with the change (1 reference).pnpm testinpackages/reactpasses.tsc --noEmitinpackages/reactpasses. Lint and the formatter pass on the changed files. On Windows I could not run the type-aware lint step (tsgolintdoes not start), so CI is the first run of that step.pnpm patchto@blocknote/react@0.55.0in our app. In a production build, fast typing after/and[gave no error in 7 of 7 runs. Before the change, the same input crashed in 2 of 3 runs. The two throttled human speeds above also gave no error.Screenshots/Video
N/A. No visual change.
Checklist
Additional Notes
None.
🤖 Generated with Claude Code
Summary by CodeRabbit