fix(components): Fix automcomplete crashing on resize larger [JOB-151844]#2924
Open
Aiden-Brine wants to merge 3 commits intomasterfrom
Open
fix(components): Fix automcomplete crashing on resize larger [JOB-151844]#2924Aiden-Brine wants to merge 3 commits intomasterfrom
Aiden-Brine wants to merge 3 commits intomasterfrom
Conversation
Deploying atlantis with
|
| Latest commit: |
abb0930
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://e34c04dc.atlantis.pages.dev |
| Branch Preview URL: | https://job-151844-max-depth-error.atlantis.pages.dev |
0087dfe to
241f2e8
Compare
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
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.
Motivations
Maximum depth errors popped up in DataDog related to Autocomplete. It was very difficult to re-create but we eventually figured out it could be recreated sometimes, for certain people, with a monitor connected, if they had the autocomplete menu open, and they expanded their screen size from smaller to larger. Here is a stack trace:
Changes
Fix for the crash
mergeRefs([...])called inline creates a brand new callback function on every render of InputText. React tracks ref props by identity, if the ref prop is a different function than last render, React detaches the old one (calls it with null) then re-attaches the new one (calls it with the element). That detach chain ultimately callsrefs.setReference(null)inside floating-ui, which calls setState. When this happened during floating-ui's flushSync call on window resize, it caused a nested synchronous React commit that exceeded the maximum update depth limit and crashed.inputRef (our mergedInputRef from Autocomplete) and inputTextRef (an internal useRef, always stable) are both stable references. Memoizing with those as deps means mergedRef never changes identity, and React never schedules a detach/re-attach.
Upon fixing this though there were styling issues instead of the crash:
Fix for styling issues
Stable setReferenceElement
The refs object returned by useFloating can get a new object identity when floating-ui's internal state changes (e.g. after a position recalculation on resize). Since setReferenceElement listed refs as a dependency, it would get a new function identity whenever refs changed. That propagated up: referenceInputRef in Autocomplete.rebuilt.tsx depends on setReferenceElement, so it recreated too. Then mergedInputRef (which depends on referenceInputRef) recreated too. React then scheduled a detach/re-attach of the input ref, the same crash path described above.
The latestRefs pattern solves this by storing refs in a mutable useRef container that is updated synchronously every render. The setReferenceElement callback reads from the container at call time rather than capturing refs at creation time. Since a useRef container itself is always the same object identity, the dependency array can be empty, giving setReferenceElement a permanently stable identity.
sizemiddleware sets widthPreviously, the dropdown width was managed as React state (menuWidth), captured once when the input ref attached on mount via clientWidth. Because the above fixes made the ref stable, it no longer detaches and re-attaches on resize, which meant menuWidth was never refreshed after mount, the dropdown would show with a stale width after a screen resize.
The size middleware's apply callback runs on every autoUpdate trigger, so rects.reference.width is always the live, freshly-measured border-box width of the position reference element (the Form-Field-Wrapper). Moving width here eliminates the stale state entirely, using floating-ui's own recommended pattern for this purpose. It also removes the need for the menuWidth state variable and all associated code in Autocomplete.rebuilt.tsx.
Updating the visual regression snapshots
clientWidth is a DOM property that returns a rounded integer representing the element's content + padding width, explicitly excluding the border. Meanwhile,
rects.reference.widthcomes from floating-ui's internal call togetBoundingClientRect().width. That returns a floating-point number representing the element's border-box width: content + padding + border.The outcome of this is that the menu is 2px wider on both sides to include the border width. This is most obvious by using the onion view when inspecting the screenshots. As you slide from left to right you will see the menu increase in width slightly as it shifts from the old styles to the new styles. Otherwise, everything else should be functionally the same.
Testing
The component should look and behave the exact same as before except for no longer crashing on resize with the menu open.
Changes can be
tested via Pre-release
In Atlantis we use Github's built in pull request reviews.