fix(react-tag-picker): tie aside-width frame cancellation to observer detach - #36667
fix(react-tag-picker): tie aside-width frame cancellation to observer detach#36667Ray Knight (ArrayKnight) wants to merge 4 commits into
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)
| act(() => { | ||
| frames.forEach(frame => frame.callback(0)); | ||
| }); |
There was a problem hiding this comment.
The detach-based cleanup looks correct, but this StrictMode regression test still does not verify the failure mode it describes.
It manually runs every captured requestAnimationFrame callback, including frames that were canceled during StrictMode replay:
frames.forEach(frame => frame.callback(0));A canceled browser animation frame never executes. Running canceled callbacks here can set the custom property and make the test pass even when the pending frame that would run in production was canceled.
Please flush only frames that have not been canceled, for example:
act(() => {
frames .filter(frame => !cancelledIds.includes(frame.id)).forEach(frame => frame.callback(0));
})
This makes the test reflect browser behavior and ensures it fails if StrictMode leaves no runnable frame to write the property.
There was a problem hiding this comment.
Addressed in b42599e. Both flush sites now use a helper that runs only uncancelled frames and consumes the queue. I verified the three frame regressions with React/ReactDOM 18.3.1 and React 19.2.0. On React 18, temporarily moving cancellation back to passive-effect cleanup makes the StrictMode assertion fail: expected 18px, received an empty value. Restoring ref-detach cleanup makes it pass.
The PR also configures React 18 integration tests to use the package's jest.config.cjs. For the local Windows verification, I corrected the generated RIT scaffold's path separators and Jest launcher; those local scaffold adjustments are not included in this component PR. Package lint and type-check pass. The broader default component run has an unrelated icon snapshot difference from this workspace's local icons package; I did not update that snapshot.
| } | ||
|
|
||
| const handleAsideDetach = useEventCallback(() => { | ||
| if (rafIdRef.current && targetDocument?.defaultView) { |
There was a problem hiding this comment.
Use an explicit null check for the animation-frame ID:
| if (rafIdRef.current && targetDocument?.defaultView) { | |
| if (rafIdRef.current !== null && targetDocument?.defaultView) { |
Animation-frame IDs are numeric handles, and 0 should not be treated as “no frame pending.”
There was a problem hiding this comment.
Addressed in b42599e with rafIdRef.current !== null. The animation-frame mock now starts at 0, so the unmount regression verifies cancellation of that handle. I confirmed that restoring the truthy check makes that test fail on React 18.
Ray Knight (ArrayKnight)
changed the title
In
useTagPickerControl, the ResizeObserver schedules the aside-width CSS property in an animation frame, but the original effect body cancels that frame before it can write. Moving cancellation into passive-effect cleanup also fails under React 18 StrictMode: effects replay without replaying the callback ref that schedules the frame.This ties cancellation to the observer ref's detach lifecycle, clears completed frame handles, and treats handle
0as a valid pending frame. The tests flush only uncancelled callbacks and cover mount, unmount, and StrictMode. The React 18 integration configuration explicitly selects this package'sjest.config.cjsso the suite is included.Validation: all three frame regressions pass with React/ReactDOM 18.3.1 and the default React 19.2.0; package lint and type-check pass. On React 18, temporarily restoring passive-effect cleanup makes the StrictMode test fail with an unset width. Restoring the truthy handle check makes the zero-handle unmount test fail.
Fixes #36649.
Extracted from #36656 per maintainer request.