refactor(react-overflow,priority-overflow): subscribe model removes intermediate state from <Overflow>#36263
Open
bsunderhus wants to merge 3 commits into
Open
Conversation
5 tasks
📊 Bundle size report
Unchanged fixtures
|
|
Pull request demo site: URL |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -0,0 +1,7 @@ | |||
| { | |||
There was a problem hiding this comment.
🕵🏾♀️ visual changes to review in the Visual Change Report
vr-tests-react-components/Charts-DonutChart 1 screenshots
| Image Name | Diff(in Pixels) | Image Type |
|---|---|---|
| vr-tests-react-components/Charts-DonutChart.Dynamic.default.chromium.png | 33361 | Changed |
vr-tests-react-components/Positioning 2 screenshots
| Image Name | Diff(in Pixels) | Image Type |
|---|---|---|
| vr-tests-react-components/Positioning.Positioning end.chromium.png | 615 | Changed |
| vr-tests-react-components/Positioning.Positioning end.updated 2 times.chromium.png | 749 | Changed |
vr-tests-react-components/ProgressBar converged 3 screenshots
| Image Name | Diff(in Pixels) | Image Type |
|---|---|---|
| vr-tests-react-components/ProgressBar converged.Indeterminate + thickness - High Contrast.default.chromium.png | 41 | Changed |
| vr-tests-react-components/ProgressBar converged.Indeterminate + thickness.default.chromium.png | 121 | Changed |
| vr-tests-react-components/ProgressBar converged.Indeterminate + thickness - Dark Mode.default.chromium.png | 49 | Changed |
vr-tests-react-components/TagPicker 2 screenshots
| Image Name | Diff(in Pixels) | Image Type |
|---|---|---|
| vr-tests-react-components/TagPicker.disabled - RTL.chromium.png | 635 | Changed |
| vr-tests-react-components/TagPicker.disabled.chromium.png | 677 | Changed |
There were 2 duplicate changes discarded. Check the build logs for more information.
bsunderhus
force-pushed
the
react-overflow/pr2-subscribe-model
branch
3 times, most recently
from
61974e9 to
ef9fd48
Compare
7 tasks
…ntermediate state from Overflow container The Overflow container previously held the overflow snapshot in component state and re-rendered every consumer through context selector when it changed. This forced consumers to opt out via memoization and made first-paint sequencing fragile. Move the snapshot into the manager and let each hook subscribe directly. priority-overflow: - the manager caches the latest snapshot and exposes getSnapshot/subscribe - dispatchOverflowUpdate goes through takeSnapshot, which fans out to listeners - the observe cleanup resets the snapshot so subscribers see an empty state react-overflow: - OverflowContextValue exposes getSnapshot/subscribe directly from the manager - the context drops react-context-selector and uses plain React.createContext; useOverflowContext keeps the selector overload for backward compatibility - the existing itemVisibility, groupVisibility and hasOverflow fields on the context are kept but marked deprecated — they are now always empty - new useOverflowSnapshot hook subscribes via useState + useIsomorphicLayoutEffect - useOverflowCount, useIsOverflowItemVisible, useIsOverflowGroupVisible and useOverflowVisibility are rewritten on top of useOverflowSnapshot - Overflow.tsx no longer keeps a useState; onOverflowChange callers receive the same OnOverflowChangeData shape derived from the snapshot - drop the @fluentui/react-context-selector dependency from the package Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- overflowContext.ts: replace the inline `() => () => null` / `() => null` default no-ops on the context value with references to a local `noop` const (block-form body). - useOverflowContainer.ts: `defaultSubscribe` now returns the same shared `noop` rather than re-allocating a fresh placeholder per call. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
bsunderhus
force-pushed
the
react-overflow/pr2-subscribe-model
branch
from
ef9fd48 to
de5968f
Compare
bsunderhus
commented
May 29, 2026
|
|
||
| // @internal (undocumented) | ||
| export interface UseOverflowContainerReturn<TElement extends HTMLElement> extends Pick<OverflowContextValue, 'registerItem' | 'updateOverflow' | 'registerOverflowMenu' | 'registerDivider'> { | ||
| export interface UseOverflowContainerReturn<TElement extends HTMLElement> extends Pick<OverflowContextValue, 'registerItem' | 'updateOverflow' | 'registerOverflowMenu' | 'registerDivider' | 'getSnapshot' | 'subscribe'> { |
Member
There was a problem hiding this comment.
TBH IDK why this is exported. Also it's marked as internal, so I would ignore it
bsunderhus
commented
May 29, 2026
Comment on lines
+71
to
+77
| getSnapshot: () => OverflowEventPayload; | ||
| observe: (container: HTMLElement, options?: ObserveOptions) => void; | ||
| removeDivider: (groupId: string) => void; | ||
| removeItem: (itemId: string) => void; | ||
| removeOverflowMenu: () => void; | ||
| setOptions: (options: Partial<ObserveOptions>) => void; | ||
| subscribe: (listener: () => void) => () => void; |
Member
There was a problem hiding this comment.
I would consider as a new functionality. To be 100% safe, should be optional, but as we don't re-export it through @fluentui/react-components, I would consider it's safe
layershifter
reviewed
Jun 1, 2026
Comment on lines
+60
to
+63
| if (!onOverflowChange) { | ||
| return; | ||
| } | ||
| onOverflowChange(null, _overflowPayloadToState(data)); |
Member
There was a problem hiding this comment.
Suggested change
| if (!onOverflowChange) { | |
| return; | |
| } | |
| onOverflowChange(null, _overflowPayloadToState(data)); | |
| onOverflowChange?.(null, _overflowPayloadToState(data)); |
layershifter
reviewed
Jun 1, 2026
| /** | ||
| * @internal | ||
| */ | ||
| export const _overflowPayloadToState = (data: OverflowEventPayload): OverflowState => { |
Member
There was a problem hiding this comment.
The same helper in useOverflowVisibility?
layershifter
reviewed
Jun 1, 2026
| */ | ||
| export function useIsOverflowGroupVisible(id: string): OverflowGroupState { | ||
| return useOverflowContext(ctx => ctx.groupVisibility[id]); | ||
| return useOverflowSnapshot().groupVisibility[id]; |
Member
There was a problem hiding this comment.
This has much worse perf as now it will any time when snapshot changes, useOverflowSnapshot should take a selector
layershifter
reviewed
Jun 1, 2026
| */ | ||
| export function useIsOverflowItemVisible(id: string): boolean { | ||
| return !!useOverflowContext(ctx => ctx.itemVisibility[id]); | ||
| return useOverflowSnapshot().visibleItems.some(item => item.id === id); |
Member
There was a problem hiding this comment.
ditto. also running .some is more expensive than key polling
probably getSnapshot() could be changed to return new signature? (I dislike it, but this hook is a hot path)
layershifter
reviewed
Jun 1, 2026
Comment on lines
+146
to
+150
| const defaultSnapshot = { | ||
| visibleItems: [], | ||
| invisibleItems: [], | ||
| groupVisibility: {}, | ||
| }; |
Member
There was a problem hiding this comment.
This object occurs third time, may be EMPTY_SNAPSHOT = Object.freeze(...) in priority-overflow?
layershifter
reviewed
Jun 1, 2026
| return acc; | ||
| }, 0); | ||
| }); | ||
| export const useOverflowCount = (): number => useOverflowSnapshot().invisibleItems.length; |
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
2 participants
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.
Summary
PR 2 of 3. Depends on PR 1 (#36262,
react-overflow/pr1-strict-mode-lifecycle). Do not merge until PR 1 lands — the "Files changed" view here will show PR 1's diff cumulatively until then; GitHub narrows it automatically once PR 1 merges.Moves the overflow snapshot out of the
<Overflow>container and into the manager itself, so each auxiliary hook subscribes directly to the manager without forcing intermediate React state.Stack
react-overflow/pr1-strict-mode-lifecycle(refactor(react-overflow,priority-overflow): pure manager + strict-mode-safe lifecycle #36262) — strict-mode-safe lifecycle.react-overflow/pr3-first-paint— first-paint correctness.priority-overflow
getSnapshot/subscribe.dispatchOverflowUpdategoes through a singletakeSnapshothelper that updates the cache and fans out to listeners.react-overflow
OverflowContextValueexposesgetSnapshot/subscribedirectly from the manager.@fluentui/react-context-selectorand uses plainReact.createContext;useOverflowContextkeeps the selector overload for backward compatibility.itemVisibility,groupVisibility, andhasOverflowon the context are kept as deprecated, always-empty placeholders so existing external consumers still type-check.useOverflowSnapshotsubscribes viauseState+useIsomorphicLayoutEffect.useOverflowCount,useIsOverflowItemVisible,useIsOverflowGroupVisible, anduseOverflowVisibilityare rewritten on top ofuseOverflowSnapshot.<Overflow>no longer holds auseState;onOverflowChangecallers receive the sameOnOverflowChangeDatashape, now derived from the snapshot.@fluentui/react-context-selectorremoved from package dependencies.Test plan
yarn nx run priority-overflow:test/react-overflow:testyarn nx run priority-overflow:lint/react-overflow:lintyarn nx run react-overflow:type-check