Improve the detection of changed hooks

[blazejkustra]

Summary

cc @hoxyq

Fixes #28584. Follow up to PR: #34547

This PR updates getChangedHooksIndices to account for the fact that useSyncExternalStore, useTransition, useActionState, useFormState internally mounts more than one hook while DevTools should treat it as a single user-facing hook.

Approach idea came from this comment 😄

Before:

QuickTime.movie.2.mov

After:

QuickTime.movie.mov

How did you test this change?

I used this component to reproduce this issue locally (I followed instructions in packages/react-devtools/CONTRIBUTING.md).

Details

import * as React from 'react';

function useDeepNestedHook() {
  React.useState(0); // 1
  return React.useState(1); // 2
}

function useNestedHook() {
  const deepState = useDeepNestedHook();
  React.useState(2); // 3
  React.useState(3); // 4

  return deepState;
}

// Create a simple store for useSyncExternalStore
function createStore(initialValue) {
  let value = initialValue;
  const listeners = new Set();
  return {
    getSnapshot: () => value,
    subscribe: listener => {
      listeners.add(listener);
      return () => {
        listeners.delete(listener);
      };
    },
    update: newValue => {
      value = newValue;
      listeners.forEach(listener => listener());
    },
  };
}

const syncExternalStore = createStore(0);

export default function InspectableElements(): React.Node {
  const [nestedState, setNestedState] = useNestedHook();

  // 5
  const syncExternalValue = React.useSyncExternalStore(
    syncExternalStore.subscribe,
    syncExternalStore.getSnapshot,
  );

  // 6
  const [isPending, startTransition] = React.useTransition();

  // 7
  const [formState, formAction, formPending] = React.useActionState(
    async (prevState, formData) => {
      return {count: (prevState?.count || 0) + 1};
    },
    {count: 0},
  );

  const handleTransition = () => {
    startTransition(() => {
      setState(Math.random());
    });
  };

  // 8
  const [state, setState] = React.useState('test');

  return (
    <>
      <div
        style={{
          padding: '20px',
          display: 'flex',
          flexDirection: 'column',
          gap: '10px',
        }}>
        <div
          onClick={() => setNestedState(Math.random())}
          style={{backgroundColor: 'red', padding: '10px', cursor: 'pointer'}}>
          State: {nestedState}
        </div>

        <button onClick={handleTransition} style={{padding: '10px'}}>
          Trigger Transition {isPending ? '(pending...)' : ''}
        </button>

        <div style={{display: 'flex', gap: '10px', alignItems: 'center'}}>
          <button
            onClick={() => syncExternalStore.update(syncExternalValue + 1)}
            style={{padding: '10px'}}>
            Trigger useSyncExternalStore
          </button>
          <span>Value: {syncExternalValue}</span>
        </div>

        <form
          action={formAction}
          style={{display: 'flex', gap: '10px', alignItems: 'center'}}>
          <button
            type="submit"
            style={{padding: '10px'}}
            disabled={formPending}>
            Trigger useFormState {formPending ? '(pending...)' : ''}
          </button>
          <span>Count: {formState.count}</span>
        </form>

        <div
          onClick={() => setState(Math.random())}
          style={{backgroundColor: 'red', padding: '10px', cursor: 'pointer'}}>
          State: {state}
        </div>
      </div>
    </>
  );
}

[react-sizebot]

Comparing: 4a3d993...6a9c7bd

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name	+/-	Base	Current	+/- gzip	Base gzip	Current gzip
oss-stable/react-dom/cjs/react-dom.production.js	=	6.84 kB	6.84 kB	+0.05%	1.88 kB	1.88 kB
oss-stable/react-dom/cjs/react-dom-client.production.js	=	608.03 kB	608.03 kB	=	107.61 kB	107.61 kB
oss-experimental/react-dom/cjs/react-dom.production.js	=	6.84 kB	6.84 kB	=	1.88 kB	1.88 kB
oss-experimental/react-dom/cjs/react-dom-client.production.js	=	667.26 kB	667.26 kB	=	117.51 kB	117.51 kB
facebook-www/ReactDOM-prod.classic.js	=	693.38 kB	693.38 kB	=	122.00 kB	122.00 kB
facebook-www/ReactDOM-prod.modern.js	=	683.76 kB	683.76 kB	=	120.40 kB	120.40 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show

Name	+/-	Base	Current	+/- gzip	Base gzip	Current gzip
oss-experimental/react-debug-tools/cjs/react-debug-tools.production.js	+0.25%	29.70 kB	29.78 kB	+0.16%	5.80 kB	5.81 kB
oss-stable-semver/react-debug-tools/cjs/react-debug-tools.production.js	+0.25%	29.70 kB	29.78 kB	+0.16%	5.80 kB	5.81 kB
oss-stable/react-debug-tools/cjs/react-debug-tools.production.js	+0.25%	29.70 kB	29.78 kB	+0.16%	5.80 kB	5.81 kB
oss-experimental/react-debug-tools/cjs/react-debug-tools.development.js	+0.23%	33.29 kB	33.37 kB	+0.14%	5.92 kB	5.93 kB
oss-stable-semver/react-debug-tools/cjs/react-debug-tools.development.js	+0.23%	33.29 kB	33.37 kB	+0.14%	5.92 kB	5.93 kB
oss-stable/react-debug-tools/cjs/react-debug-tools.development.js	+0.23%	33.29 kB	33.37 kB	+0.14%	5.92 kB	5.93 kB

Generated by 🚫 dangerJS against 6a9c7bd

[blazejkustra]

@hoxyq I may need your help... Two tests are failing and I don't understand what's wrong

• profilingCache-test.js › should properly detect changed hooks
• profilingCache-test.js › should detect context changes or lack of changes with conditional use()

They only fail when I call:

const prevHooks = inspectHooks(prevFiber);
const nextHooks = inspectHooks(nextFiber);

I’ve narrowed it to this:

export function inspectHooks<Props>(
  renderFunction: (props: Props) => React$Node,
  props: Props,
  currentDispatcher?: CurrentDispatcherRef,
): HooksTree {
  if (currentDispatcher == null) {
    currentDispatcher = ReactSharedInternals;
  }

  const previousDispatcher = currentDispatcher.H;
  currentDispatcher.H = DispatcherProxy;

  let readHookLog;
  let ancestorStackError;

  try {
    ancestorStackError = new Error();
    renderFunction(props);
  } catch (error) {
    handleRenderFunctionError(error);
  } finally {
    readHookLog = hookLog;
    hookLog = [];
    // $FlowFixMe[incompatible-use] found when upgrading Flow
    currentDispatcher.H = previousDispatcher;
  }

  const rootStack =
    ancestorStackError === undefined
      ? ([]: ParsedStackFrame[])
      : ErrorStackParser.parse(ancestorStackError);

  return buildTree(rootStack, readHookLog);
}

I suspect the issue comes from how inspectHooks temporarily swaps out the dispatcher. That dispatcher replacement might be overwriting or leaking some internal data, which then causes the change descriptions recorded by the Profiler to become incomplete.

[hoxyq]

Looking at the failed tests, it looks like the number of recorded changes doesn't match the number of performed updates. I don't see any early returns in the code, so I am not sure how this may actually happen.

I wonder if this could be related to the fact that we are actually calling render functions of components to build the hook tree.

[blazejkustra]

FYI I'm looking into it again, maybe merging main would help 🤞

[blazejkustra]

I can't run tests on the newest main due to this, is this a known problem or should I build differently? @hoxyq

[andreisilviudragnea]

Please have a look at #34427 too, maybe it's another solution to this issue @blazejkustra @hoxyq

[andreisilviudragnea]

I think that merging #34547 without fixing the index counting error for all composite hooks just does not fix the initial problem at all, because having for example a useTransition hook before any other hook will invalidate the "Hook X changed" index for all hooks that follow useTransition

[blazejkustra]

I think that merging #34547 without fixing the index counting error for all composite hooks just does not fix the initial problem at all, because having for example a useTransition hook before any other hook will invalidate the "Hook X changed" index for all hooks that follow useTransition

@andreisilviudragnea read through the discussion on my previous PR, especially this comment. The plan was to merge it and fix other hooks in follow up PRs. However, it turned out to be harder than expected and I haven't figure it out yet 🫠

[blazejkustra]

It's a Christmas miracle!! 🎄

After some trial and error I finally realized that due to how inspectHooks() works the dispatcher was mocked for a moment, which overwrote the dispatch/setState variables the tests were storing 'globally'. I fixed it by saving references before they get overwritten.

Back to you @hoxyq 😄

[hoxyq]

Is there a reason why this specific order is chosen?

This looks like subHooks wills have lower indexes in an array, when flattened. In the UI, the hook numbers (index + 1) usually reflect the tree structure. Basically indexOf(parent) < indexOf(parent.child) is always true.

[blazejkustra]

flattenHooksTree function performs a depth-first traversal that only keeps leaf hooks (those without subHooks). This means custom hooks are unwrapped to primitive hooks.

For example, given this tree:

[
  { name: 'useCustomHook', subHooks: [
    { name: 'useState', value: 1 },
    { name: 'useEffect' }
  ]},
  { name: 'useState', value: 2 }
]

The flattened result is:

[
  { name: 'useState', value: 1 },  // index 0
  { name: 'useEffect' },           // index 1
  { name: 'useState', value: 2 }   // index 2
]

This order matches React's hooks order, right? So comparing prevFlattened[i] with nextFlattened[i] identifies which primitive hooks changed between renders.

[blazejkustra]

If we were to change the traversal order the indexes wouldn't be correct I believe, wdyt @hoxyq?

[hoxyq]

Thanks for clarifying. I was confused a bit, but that is not an issue, since we only assign indexes to built-in hooks, which should be leaf nodes by definition.

In your example above, I believe this will be the result of flattening:

[
  { name: 'State', value: 1 },  // index 0
  { name: 'Effect' },           // index 1
  { name: 'CustomHook' },   
  { name: 'State', value: 2 }   // index 2
]

[hoxyq]

Do you think it still makes sense to do flattening? We could probably just do dfs on both hook trees at the same time.

[blazejkustra]

I think custom hook would not be there since it is omitted by the continue statement:

if (currentHook.subHooks && currentHook.subHooks.length > 0) {
  flattened.push(...flattenHooksTree(currentHook.subHooks));
  continue;
}

[blazejkustra]

We could probably just do dfs on both hook trees at the same time.

Ahaa, so instead of building prevFlattened and nextFlattened I could do it in place? Good idea, let me try it 👀

[hoxyq]

These changes make sense, thank you for improving this. I left some clarifying questions.

Could you please also add some tests that will validate these changes? For example, the different (and correct) result of inspectHooks() calls when a complex component with useSyncExternalStore() or other hooks is used.

[blazejkustra]

Back to you @hoxyq! I adjusted the code so it traverses in place and added a test as asked 🚀

I tried to run this new test on main and it failed miserably as expected :)

[hoxyq]

Looks good, please see my suggestion on making iterating over hook trees without optionality.

I can merge after it. We've accumulated some fixes that are not released yet, so I might release new patch / minor release later this month, but no concrete plans yet.

[blazejkustra]

I added your suggestions, seems that everything works well. Thank you!

[blazejkustra]

I was wrong lol, good catch!