From 2860e00cf8780dc2d59b87f3ff28ac88b908660f Mon Sep 17 00:00:00 2001 From: Sophie Alpert Date: Mon, 20 Jul 2026 23:39:09 -0400 Subject: [PATCH] [Fiber] Detect useSyncExternalStore mutations missed while Activity tree was hidden (#36947) Fixes #27670. When an Activity subtree is hidden, its passive effects are disconnected, which unsubscribes from the store. On reveal, the effects are reconnected by replaying the fiber's effect list without a render, but if `updateStoreInstance` was not in the effect list then the component would be left stale. This happened both when a layout effect mutated the store during the reveal commit (after the subtree rendered but before it resubscribed) and when the store changed while hidden and the component bailed out of rendering during the reveal. We now push the updateStoreInstance effect unconditionally but tag it with HookHasEffect only under the same conditions as before, so regular commits skip it when nothing changed but reconnection always triggers it so it can trigger a rerender if appropriate. --- .../react-reconciler/src/ReactFiberHooks.js | 32 +++-- .../__tests__/useSyncExternalStore-test.js | 126 ++++++++++++++++++ 2 files changed, 148 insertions(+), 10 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index c5b4f6d1821..edb5eacc8cc 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -92,6 +92,7 @@ import { FormReset, } from './ReactFiberFlags'; import { + NoFlags as HookNoFlags, HasEffect as HookHasEffect, Layout as HookLayout, Passive as HookPassive, @@ -1774,21 +1775,31 @@ function updateSyncExternalStore( // commit phase if there was an interleaved mutation. In concurrent mode // this can happen all the time, but even in synchronous mode, an earlier // effect may have mutated the store. - if ( + const storeChanged = inst.getSnapshot !== getSnapshot || snapshotChanged || // Check if the subscribe function changed. We can save some memory by // checking whether we scheduled a subscription effect above. (workInProgressHook !== null && - workInProgressHook.memoizedState.tag & HookHasEffect) - ) { + (workInProgressHook.memoizedState.tag & HookHasEffect) !== HookNoFlags); + + // Even if nothing changed during this render, we push the effect so it is + // always in the effect list. That way it re-runs whenever the passive + // effects are reconnected, like when a hidden Activity tree is shown again. + // While the tree was hidden we were not subscribed to the store, so + // mutations during that window notified nobody, and if the reveal didn't + // re-render this component (or rendered before the mutation), nothing + // would ever detect them. When nothing changed, the effect is pushed + // without the HasEffect tag so a regular commit skips it. + pushSimpleEffect( + storeChanged ? HookHasEffect | HookPassive : HookPassive, + createEffectInstance(), + updateStoreInstance.bind(null, fiber, inst, nextSnapshot, getSnapshot), + null, + ); + + if (storeChanged) { fiber.flags |= PassiveEffect; - pushSimpleEffect( - HookHasEffect | HookPassive, - createEffectInstance(), - updateStoreInstance.bind(null, fiber, inst, nextSnapshot, getSnapshot), - null, - ); // Unless we're rendering a blocking lane, schedule a consistency check. // Right before committing, we will walk the tree and check if any of the @@ -1848,7 +1859,8 @@ function updateStoreInstance( // Something may have been mutated in between render and commit. This could // have been in an event that fired before the passive effects, or it could // have been in a layout effect. In that case, we would have used the old - // snapsho and getSnapshot values to bail out. We need to check one more time. + // snapshot and getSnapshot values to bail out. We need to check one more + // time. This effect also re-runs when a hidden Activity tree is revealed. if (checkIfSnapshotChanged(inst)) { // Force a re-render. // We intentionally don't log update times and stacks here because this diff --git a/packages/react-reconciler/src/__tests__/useSyncExternalStore-test.js b/packages/react-reconciler/src/__tests__/useSyncExternalStore-test.js index eb5bc6ed98c..0a67abe9fe2 100644 --- a/packages/react-reconciler/src/__tests__/useSyncExternalStore-test.js +++ b/packages/react-reconciler/src/__tests__/useSyncExternalStore-test.js @@ -352,6 +352,132 @@ describe('useSyncExternalStore', () => { }, ); + // Regression test for https://github.com/facebook/react/issues/27670 + it('detects store mutations from a layout effect while an Activity subtree is being revealed', async () => { + const store = createExternalStore('revision:1'); + + function App({mode, revision}) { + return ( + + + + + + ); + } + + function Wrapper({children, revision}) { + useLayoutEffect(() => { + store.set('revision:' + revision); + }, [revision]); + + return ( + <> + wrapper:{revision} + {', '} + {children} + + ); + } + + function Subscriber() { + const revision = useSyncExternalStore(store.subscribe, store.getState); + return ; + } + + const root = ReactNoop.createRoot(); + + // Mount the app + await act(() => { + root.render(); + }); + assertLog(['revision:1']); + expect(root).toMatchRenderedOutput('wrapper:1, revision:1'); + expect(store.getSubscriberCount()).toBe(1); + + // Hide the subtree. React unsubscribes from the store. + await act(() => { + root.render(); + }); + assertLog(['revision:1']); + expect(store.getSubscriberCount()).toBe(0); + + // Show the subtree again. A layout effect mutates the store during the + // reveal, after the Subscriber rendered but before it resubscribed. When + // it resubscribes, it must detect the mutation it missed. + await act(() => { + root.render(); + }); + assertLog(['revision:1', 'revision:2']); + expect(store.getSubscriberCount()).toBe(1); + expect(root).toMatchRenderedOutput('wrapper:2, revision:2'); + }); + + // Regression test for https://github.com/facebook/react/issues/27670 + it( + 'detects store mutations that happened while an Activity subtree was ' + + 'hidden, even if the subtree bails out of rendering when revealed', + async () => { + const store = createExternalStore('initial'); + + // Memoized so that revealing the Activity boundary doesn't re-render + // the subscriber. This matches components memoized by React.memo or + // React Compiler. + const Subscriber = React.memo(({label}) => { + const value = useSyncExternalStore(store.subscribe, store.getState); + return ; + }); + + function App({mode, label}) { + return ( + + + + ); + } + + const root = ReactNoop.createRoot(); + await act(() => { + root.render(); + }); + assertLog(['a:initial']); + expect(root).toMatchRenderedOutput('a:initial'); + expect(store.getSubscriberCount()).toBe(1); + + // Re-render the subscriber once with different props, with no store + // change. This replaces its effect list with one that contains only + // the subscription effect, no interleaved mutation check. + await act(() => { + root.render(); + }); + assertLog(['b:initial']); + expect(root).toMatchRenderedOutput('b:initial'); + + // Hide the subtree. React unsubscribes from the store. + await act(() => { + root.render(); + }); + expect(store.getSubscriberCount()).toBe(0); + + // Mutate the store while the subtree is hidden. Nothing is subscribed, + // so no update is scheduled. + await act(() => { + store.set('updated'); + }); + assertLog([]); + + // Show the subtree again. The memoized component bails out of + // rendering, so resubscribing to the store is the only chance to + // detect the mutation that happened while it was hidden. + await act(() => { + root.render(); + }); + assertLog(['b:updated']); + expect(store.getSubscriberCount()).toBe(1); + expect(root).toMatchRenderedOutput('b:updated'); + }, + ); + it('regression: does not infinite loop for only changing store reference in render', async () => { let store = {value: {}}; let listeners = [];