fix(react-tag-picker): cancel the aside-width frame in a cleanup, not the effect body - #36667
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
Dmytro Kirpa (dmytrokirpa)
assigned
Paul Mardling (PaulGMardling) and unassigned
Paul Mardling (PaulGMardling)
There was a problem hiding this comment.
Pull request overview
Fixes a TagPickerControl race by deferring animation-frame cancellation to effect cleanup.
Changes:
- Preserves the mount-time aside-width update and cancels pending work on unmount.
- Adds deterministic regression tests.
- Adds a patch change file.
Merge confidence: 100/100
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
useTagPickerControl.tsx |
Moves frame cancellation into cleanup. |
TagPickerControl.test.tsx |
Tests mount-time writing and unmount cancellation. |
| Change file | Records the patch release. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| return () => { | ||
| if (rafIdRef.current && targetDocument?.defaultView) { | ||
| targetDocument.defaultView.cancelAnimationFrame(rafIdRef.current); | ||
| } | ||
| }; |
There was a problem hiding this comment.
The new passive-effect cleanup cancels the scheduled animation frame during React 18 StrictMode’s effect replay. Because callback refs are not reattached in that replay, no replacement update is scheduled, potentially leaving --fui-TagPickerControl-aside-width unset until another resize. This can cause the aside to overlap the trigger.
Recommended fix: cancel the pending frame when the observer callback ref receives null , alongside ResizeObserver.disconnect() , rather than in passive-effect cleanup. Clear the stored frame ID after execution/cancellation and add a React 18 StrictMode regression test. No other high-confidence defects were identified.
There was a problem hiding this comment.
Confirmed the shape of the risk: tying the frame's cancellation to the passive-effect cleanup left it racing against the ref's own attach/detach lifecycle instead of being scoped to it. Moved cancellation into useResizeObserverRef's callback-ref null branch alongside disconnect() (clearing the stored frame id after both a run and a cancellation) and removed the effect; added a StrictMode regression test ("writes the property after mount inside React.StrictMode", TagPickerControl.test.tsx) pinning the token being set after mount. Pushed as 232159c.
One honest caveat: an instrumented probe against this repo's resolved React (19.2.0) showed the callback ref is in fact re-invoked during StrictMode's replay here, so I couldn't reproduce the specific "token stays unset" failure in this environment either before or after the change -- but the restructuring is adopted regardless, since it removes the cross-lifecycle race entirely rather than depending on the effect and the ref timing lining up. Thanks for catching this.
… ref's detach path Moves the requestAnimationFrame cancellation out of the passive-effect cleanup added in adf2b20 and into useResizeObserverRef's callback-ref null branch, alongside ResizeObserver.disconnect(). The frame's lifecycle now belongs to the same event that started the observation (ref attach/detach) instead of an effect whose cleanup timing is independent of it, and the stored frame id is cleared after both a normal run and a cancellation so a stale id is never read back. This also closes a gap the effect-based cleanup had: it only ran on the whole control's unmount (effect deps never change), so toggling the aside away on its own (no secondaryAction/expandIcon) while a frame was in flight leaked it. Detach-based cancellation catches both cases. Verified with an instrumented probe: under this repo's resolved React (19.2.0), StrictMode's replay does reattach the observer's callback ref, so a synchronous-mock reproduction of the reviewer's specific failure (the width token staying unset after mount) does not trigger under either the prior or the new code in this test environment. The restructure is adopted regardless because it is the more principled fix -- it removes the cross-lifecycle race between the effect and the ref entirely, rather than relying on their relative timing lining up -- and adds a StrictMode regression test asserting the token is written after mount. Addresses review comment microsoft#36667 (comment)
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.