[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.

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<T>(
   // 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<T>(
   // 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

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 (
+        <React.Activity mode={mode}>
+          <Wrapper revision={revision}>
+            <Subscriber />
+          </Wrapper>
+        </React.Activity>
+      );
+    }
+
+    function Wrapper({children, revision}) {
+      useLayoutEffect(() => {
+        store.set('revision:' + revision);
+      }, [revision]);
+
+      return (
+        <>
+          wrapper:{revision}
+          {', '}
+          {children}
+        </>
+      );
+    }
+
+    function Subscriber() {
+      const revision = useSyncExternalStore(store.subscribe, store.getState);
+      return <Text text={revision} />;
+    }
+
+    const root = ReactNoop.createRoot();
+
+    // Mount the app
+    await act(() => {
+      root.render(<App mode="visible" revision="1" />);
+    });
+    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(<App mode="hidden" revision="1" />);
+    });
+    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(<App mode="visible" revision="2" />);
+    });
+    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 <Text text={label + ':' + value} />;
+      });
+
+      function App({mode, label}) {
+        return (
+          <React.Activity mode={mode}>
+            <Subscriber label={label} />
+          </React.Activity>
+        );
+      }
+
+      const root = ReactNoop.createRoot();
+      await act(() => {
+        root.render(<App mode="visible" label="a" />);
+      });
+      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(<App mode="visible" label="b" />);
+      });
+      assertLog(['b:initial']);
+      expect(root).toMatchRenderedOutput('b:initial');
+
+      // Hide the subtree. React unsubscribes from the store.
+      await act(() => {
+        root.render(<App mode="hidden" label="b" />);
+      });
+      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(<App mode="visible" label="b" />);
+      });
+      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 = [];