fix(react-tag-picker): cancel the aside-width frame in a cleanup, not the effect body - #36667
Open
Ray Knight (ArrayKnight) wants to merge 2 commits into
Open
Conversation
… the effect body
useTagPickerControl cancels the ResizeObserver callback's pending
requestAnimationFrame from the effect body rather than its cleanup. The effect
runs once on mount (measured: 1 of 1 on every load); whether it cancels is decided
by whether React's passive-effect flush lands before or after the observer's first
callback has stored a raf id. When it lands after, the only write of
--fui-TagPickerControl-aside-width is cancelled before it can fire, the control's
padding-inline-end falls back to spacingHorizontalM, and the absolutely positioned
aside overlays the trigger.
Measured over 12 identical fresh page loads of one TagPicker, production build:
3 of 12 wrote the token; resting padding-inline-end was 30px or 12px and the
trigger's input 258.297 or 276.297 -- an 18px width swing between runs of the same
page. With the cancel moved into the effect's cleanup, 12 of 12 write it and both
values are single-valued (30px, 258.297).
Instrumenting the effect in the same production bundle over 12 loads gives
effectRuns=[1] and cancels=[0,1]: `targetDocument` is identity-stable and the
effect never re-runs, so the outcome is decided purely by the flush ordering. The
cross-tab is one-to-one -- rafIdRef.current null at the single effect run -> no
cancel -> 30px (1 load); "set" -> cancel -> 12px (11 loads).
Completeness, four arms x 12 loads on a dev bundle with page-level instrumentation
of ResizeObserver, requestAnimationFrame and cancelAnimationFrame:
arm token effect cleanup roCtor obs disc raf s/c/fired
unpatched / plain / dev 0/12 1 0 1 1 0 1/1/0
unpatched / StrictMode / dev 12/12 2 0 2 2 1 2/1/1
cleanupFix / plain / dev 12/12 1 0 1 1 0 1/0/1
cleanupFix / StrictMode / dev 12/12 2 1 2 2 1 2/1/1
No observer leak (observes == constructs in every arm), no double-fire (the aside
frame fires exactly once wherever it fires at all), and no StrictMode regression.
The fix also strictly improves unmount: today nothing cancels the pending frame,
which fires against a detached ref and is swallowed by optional chaining.
Note for anyone reproducing this: it must be a PRODUCTION build with no
StrictMode. Under StrictMode in a dev build the defect is invisible -- the
double-invoke detaches and re-attaches the ref, the observer fires a second time
after the effect has already run, and the write lands 12 of 12. A StrictMode dev
Storybook will not show it.
Regression coverage, both verified to fail without this change: TagPickerControl
now has two tests that pin the ordering deterministically. The observer is
attached by a ref callback, so `observe()` runs in the commit phase, before React
flushes the passive effect; a ResizeObserver stub whose `observe()` invokes its
callback synchronously therefore puts a frame in flight by the time the effect
runs -- exactly the state the production race lands on -- without depending on
real timing, which jsdom cannot reproduce. The end-to-end probes that produced the
numbers above are kept as the timing-level gate at
.scratch/windmod-loop/revalidation/tag-picker-tpa-{determinism,effect-count,lifecycle}.mjs.
Verified: react-tag-picker build, type-check and lint pass; its suite is 1 failed /
181 passed both with and without this change (pre-existing @fluentui/react-icons
snapshot drift -- SVG path data and the `fui-Icon` class -- unrelated to it), the
two new tests included in the 181.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013wmpBCYJpDJCLXcScCWz1i
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Aj9uA3rCVgosnh2zNn8qkc
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.
In
useTagPickerControl, aResizeObservercallback schedules the only write of--fui-TagPickerControl-aside-widthinside arequestAnimationFrame, but the matchingcancelAnimationFrameruns in the effect body rather than the effect's cleanup. The observer's first callback fires from a ref callback in the commit phase — before React flushes the passive effect — so the effect body frequently cancels the very frame that carries the write. Measured on a production build with StrictMode off: the token was written on 3 of 12 fresh page loads, an 18px resting-width swing between runs of the same page. (Under StrictMode in a dev build the double-invoke masks the race completely, which is why it survives Storybook probing.)The fix moves the cancel into the effect's cleanup: 12/12 writes after, no observer leak, no double-fire, no StrictMode regression, and a strictly better unmount (today nothing cancels a pending frame at unmount). Two regression tests are added that pin the commit-phase-before-effect ordering deterministically; both were verified to fail without the fix.
Fixes #36649.
Extracted from #36656 per maintainer request — each in-tree fix from that PR as an isolated change.