Bug: useSyncExternalStore does not schedule update after mutation

[bvaughn]

Here is a Replay recording with comments: https://app.replay.io/recording/318114a4-3102-4732-ad1b-cb63b9c0ac22

I think the comments I've added show the following scenario:

1. A React component (Subscriber) uses useSyncExternalStore to read from a mutable store.
2. The Activity subtree containing Subscriber is hidden, and React unsubscribes from the store.
3. An update is scheduled to show the hidden subtree again and the Subscriber component re-renders (with memoized state).
4. A parent component mutates the store in a layout effect. (The Subscriber component is not yet listening and so it does not re-render to reflect the updated store value.)
5. React re-subscribes the Subscriber component (useSyncExternalStore) but it has already missed the mutation and React does not check for a changed snapshot value.

We originally observed this behavior in Replay itself but I was able to reduce it to the following simplified case: https://codesandbox.io/s/inspiring-bird-m4wv5l

I've added comments to the Replay, including ones that bracket the problematic commitRoot. Here's a short Loom as well talking through the bug: https://www.loom.com/share/2584cad5b4c44e6bba396ff8cf79db1d

I think an application could work around this issue in a couple of ways:

• Only mutate stores in passive effects. (This may cause visible layout shift though so it seems bad.)
• Mutable store users should avoid memoized getSnapshot and subscribe functions. (This would schedule some unnecessary effects work which is probably nice to avoid but maybe acceptable.)
• Store subscribe methods should always invoke the callback React passes. (This would cause a lot of unnecessary Object.is comparisons but that's probably an acceptable cost.)

I think the third option above seems best, but needing to do that feels like a foot gun for the API. Thoughts?

cc @acdlite, @sebmarkbage in case you find this interesting.

[bvaughn]

If it would be useful, I can create a failing unit test from my Code Sandbox. (Edit: #27676)

I'd also be happy to submit a PR with a proposed fix (but I'm less familiar with this hook's code in particular, so I might not be the best person to do this.)

Either way, let me know if the above would be helpful.

[sophiebits]

If I’m not mistaken, this line is meant to catch if the store was mutated in between render and subscribe (if we were not already subscribed to the store at render time):

https://github.com/facebook/react/blob/746890329452cbec8685eb3466847c5f17d9dc77/packages/react-reconciler/src/ReactFiberHooks.js#L1689

Perhaps this code isn’t running at the right time when reviving the tree.

[bvaughn]

Agreed. I think that code only runs during mount or if the getSnapshot function identity has changed. (In this case it doesn’t get run because the hidden to visible transition is an update and getSnapshot is a stable function reference. I tried to mention that in the Loom, but maybe I wasn't very clear.)

[rickhanlonii]

@acdlite are we missing a mutated value check here?

[bvaughn]

Friendly ping.

[eps1lon]

This is only a bug when used with Activity or does it also happen when a suspended tree unsuspends?

If it's just with Activity it may take a while to fix it since it's currently being deprioritized: https://react.dev/blog/2024/02/15/react-labs-what-we-have-been-working-on-february-2024#offscreen-renamed-to-activity

I'm still trying to write a test for it so that we can pick it up again at a later stage.

[bvaughn]

This is only a bug when used with Activity or does it also happen when a suspended tree unsuspends?

The scope of the bug I've reported here requires Offscreen/Activity.

I'm still trying to write a test for it so that we can pick it up again at a later stage.

@eps1lon I already wrote a test for this in PR #27676

[eps1lon]

You can also repro this when you remove the store.set from the parent and move it into a passive effect before the useSyncExternalStore call:

    function App({mode, revision}) {
      return (
        <React.unstable_Activity mode={mode}>
          <Subscriber revision={revision} />
        </React.unstable_Activity>
      );
    }

    function Subscriber({revision: propRevision}) {
      React.useEffect(() => {
        console.log('store.set');
        store.set('revision:' + propRevision);
      }, [propRevision]);
      const revision = useSyncExternalStore(store.subscribe, store.getState);
      return <Text text={revision} />;
    }

If that passive effect is moved after the useSyncExternalStore, it passes again because then React will have subscribed to store changes before the passive effect runs

-React.useEffect(() => {
-  console.log('store.set');
-  store.set('revision:' + propRevision);
-}, [propRevision]);
 const revision = useSyncExternalStore(store.subscribe, store.getState);
+React.useEffect(() => {
+  console.log('store.set');
+  store.set('revision:' + propRevision);
+}, [propRevision]);

[github-actions[bot]]

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment!

[markerikson]

Still an issue with v19 as far as I know.

[farwayer]

Hey, any news?

Just encountered this issue when the store change happens after getSnapshot is called during component rendering, but before subscribe is called. This bug is quite hard to debug.

It seems correct to not delay the subscribe call, but to do it right away inside useSyncExternalStore, otherwise you can lose changes.