Merge branch 'main' into pr34995

Summary:
With this we are now comparing a snapshot of the derivationCache with
the new changes every time we are done recording the derivations
happening in the HIR.

We have to do this after recording everything since we still do some
mutations on the cache when recording mutations.



Test Plan:
Test the following in playground:
```
// @validateNoDerivedComputationsInEffects_exp

function Component({ value }) {
  const [checked, setChecked] = useState('');

  useEffect(() => {
    setChecked(value === '' ? [] : value.split(','));
  }, [value]);

  return (
    <div>{checked}</div>
  )
}
```

This no longer causes an infinite loop.

Added a test case in the next PR in the stack

---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34967).
* #35044
* #35020
* #34973
* #34972
* #34995
* __->__ #34967

compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects_exp.ts

@@ -33,6 +33,7 @@ type DerivationMetadata = {
   typeOfValue: TypeOfValue;
   place: Place;
   sourcesIds: Set<IdentifierId>;
+  isStateSource: boolean;
 };
 
 type ValidationContext = {
@@ -47,6 +48,44 @@ type ValidationContext = {
 class DerivationCache {
   hasChanges: boolean = false;
   cache: Map<IdentifierId, DerivationMetadata> = new Map();
+  private previousCache: Map<IdentifierId, DerivationMetadata> | null = null;
+
+  takeSnapshot(): void {
+    this.previousCache = new Map();
+    for (const [key, value] of this.cache.entries()) {
+      this.previousCache.set(key, {
+        place: value.place,
+        sourcesIds: new Set(value.sourcesIds),
+        typeOfValue: value.typeOfValue,
+        isStateSource: value.isStateSource,
+      });
+    }
+  }
+
+  checkForChanges(): void {
+    if (this.previousCache === null) {
+      this.hasChanges = true;
+      return;
+    }
+
+    for (const [key, value] of this.cache.entries()) {
+      const previousValue = this.previousCache.get(key);
+      if (
+        previousValue === undefined ||
+        !this.isDerivationEqual(previousValue, value)
+      ) {
+        this.hasChanges = true;
+        return;
+      }
+    }
+
+    if (this.cache.size !== this.previousCache.size) {
+      this.hasChanges = true;
+      return;
+    }
+
+    this.hasChanges = false;
+  }
 
   snapshot(): boolean {
     const hasChanges = this.hasChanges;
@@ -58,48 +97,28 @@ class DerivationCache {
     derivedVar: Place,
     sourcesIds: Set<IdentifierId>,
     typeOfValue: TypeOfValue,
+    isStateSource: boolean,
   ): void {
-    let newValue: DerivationMetadata = {
-      place: derivedVar,
-      sourcesIds: new Set(),
-      typeOfValue: typeOfValue ?? 'ignored',
-    };
-
-    if (sourcesIds !== undefined) {
-      for (const id of sourcesIds) {
-        const sourcePlace = this.cache.get(id)?.place;
-
-        if (sourcePlace === undefined) {
-          continue;
-        }
-
-        /*
-         * If the identifier of the source is a promoted identifier, then
-         *  we should set the target as the source.
-         */
+    let finalIsSource = isStateSource;
+    if (!finalIsSource) {
+      for (const sourceId of sourcesIds) {
+        const sourceMetadata = this.cache.get(sourceId);
         if (
-          sourcePlace.identifier.name === null ||
-          sourcePlace.identifier.name?.kind === 'promoted'
+          sourceMetadata?.isStateSource &&
+          sourceMetadata.place.identifier.name?.kind !== 'named'
         ) {
-          newValue.sourcesIds.add(derivedVar.identifier.id);
-        } else {
-          newValue.sourcesIds.add(sourcePlace.identifier.id);
+          finalIsSource = true;
+          break;
         }
       }
     }
 
-    if (newValue.sourcesIds.size === 0) {
-      newValue.sourcesIds.add(derivedVar.identifier.id);
-    }
-
-    const existingValue = this.cache.get(derivedVar.identifier.id);
-    if (
-      existingValue === undefined ||
-      !this.isDerivationEqual(existingValue, newValue)
-    ) {
-      this.cache.set(derivedVar.identifier.id, newValue);
-      this.hasChanges = true;
-    }
+    this.cache.set(derivedVar.identifier.id, {
+      place: derivedVar,
+      sourcesIds: sourcesIds,
+      typeOfValue: typeOfValue ?? 'ignored',
+      isStateSource: finalIsSource,
+    });
   }
 
   private isDerivationEqual(
@@ -121,6 +140,14 @@ class DerivationCache {
   }
 }
 
+function isNamedIdentifier(place: Place): place is Place & {
+  identifier: {name: NonNullable<Place['identifier']['name']>};
+} {
+  return (
+    place.identifier.name !== null && place.identifier.name.kind === 'named'
+  );
+}
+
 /**
  * Validates that useEffect is not used for derived computations which could/should
  * be performed in render.
@@ -172,10 +199,10 @@ export function validateNoDerivedComputationsInEffects_exp(
       if (param.kind === 'Identifier') {
         context.derivationCache.cache.set(param.identifier.id, {
           place: param,
-          sourcesIds: new Set([param.identifier.id]),
+          sourcesIds: new Set(),
           typeOfValue: 'fromProps',
+          isStateSource: true,
         });
-        context.derivationCache.hasChanges = true;
       }
     }
   } else if (fn.fnType === 'Component') {
@@ -183,15 +210,17 @@ export function validateNoDerivedComputationsInEffects_exp(
     if (props != null && props.kind === 'Identifier') {
       context.derivationCache.cache.set(props.identifier.id, {
         place: props,
-        sourcesIds: new Set([props.identifier.id]),
+        sourcesIds: new Set(),
         typeOfValue: 'fromProps',
+        isStateSource: true,
       });
-      context.derivationCache.hasChanges = true;
     }
   }
 
   let isFirstPass = true;
   do {
+    context.derivationCache.takeSnapshot();
+
     for (const block of fn.body.blocks.values()) {
       recordPhiDerivations(block, context);
       for (const instr of block.instructions) {
@@ -199,6 +228,7 @@ export function validateNoDerivedComputationsInEffects_exp(
       }
     }
 
+    context.derivationCache.checkForChanges();
     isFirstPass = false;
   } while (context.derivationCache.snapshot());
 
@@ -236,6 +266,7 @@ function recordPhiDerivations(
         phi.place,
         sourcesIds,
         typeOfValue,
+        false,
       );
     }
   }
@@ -257,11 +288,13 @@ function recordInstructionDerivations(
   isFirstPass: boolean,
 ): void {
   let typeOfValue: TypeOfValue = 'ignored';
+  let isSource: boolean = false;
   const sources: Set<IdentifierId> = new Set();
   const {lvalue, value} = instr;
   if (value.kind === 'FunctionExpression') {
     context.functions.set(lvalue.identifier.id, value);
     for (const [, block] of value.loweredFunc.func.body.blocks) {
+      recordPhiDerivations(block, context);
       for (const instr of block.instructions) {
         recordInstructionDerivations(instr, context, isFirstPass);
       }
@@ -280,10 +313,7 @@ function recordInstructionDerivations(
         context.effects.add(effectFunction.loweredFunc.func);
       }
     } else if (isUseStateType(lvalue.identifier) && value.args.length > 0) {
-      const stateValueSource = value.args[0];
-      if (stateValueSource.kind === 'Identifier') {
-        sources.add(stateValueSource.identifier.id);
-      }
+      isSource = true;
       typeOfValue = joinValue(typeOfValue, 'fromState');
     }
   }
@@ -310,9 +340,7 @@ function recordInstructionDerivations(
     }
 
     typeOfValue = joinValue(typeOfValue, operandMetadata.typeOfValue);
-    for (const id of operandMetadata.sourcesIds) {
-      sources.add(id);
-    }
+    sources.add(operand.identifier.id);
   }
 
   if (typeOfValue === 'ignored') {
@@ -320,7 +348,12 @@ function recordInstructionDerivations(
   }
 
   for (const lvalue of eachInstructionLValue(instr)) {
-    context.derivationCache.addDerivationEntry(lvalue, sources, typeOfValue);
+    context.derivationCache.addDerivationEntry(
+      lvalue,
+      sources,
+      typeOfValue,
+      isSource,
+    );
   }
 
   for (const operand of eachInstructionOperand(instr)) {
@@ -331,11 +364,25 @@ function recordInstructionDerivations(
       case Effect.ConditionallyMutateIterator:
       case Effect.Mutate: {
         if (isMutable(instr, operand)) {
-          context.derivationCache.addDerivationEntry(
-            operand,
-            sources,
-            typeOfValue,
-          );
+          if (context.derivationCache.cache.has(operand.identifier.id)) {
+            const operandMetadata = context.derivationCache.cache.get(
+              operand.identifier.id,
+            );
+
+            if (operandMetadata !== undefined) {
+              operandMetadata.typeOfValue = joinValue(
+                typeOfValue,
+                operandMetadata.typeOfValue,
+              );
+            }
+          } else {
+            context.derivationCache.addDerivationEntry(
+              operand,
+              sources,
+              typeOfValue,
+              false,
+            );
+          }
         }
         break;
       }
@@ -367,6 +414,117 @@ function recordInstructionDerivations(
   }
 }
 
+type TreeNode = {
+  name: string;
+  typeOfValue: TypeOfValue;
+  isSource: boolean;
+  children: Array<TreeNode>;
+};
+
+function buildTreeNode(
+  sourceId: IdentifierId,
+  context: ValidationContext,
+  visited: Set<string> = new Set(),
+): Array<TreeNode> {
+  const sourceMetadata = context.derivationCache.cache.get(sourceId);
+  if (!sourceMetadata) {
+    return [];
+  }
+
+  if (sourceMetadata.isStateSource && isNamedIdentifier(sourceMetadata.place)) {
+    return [
+      {
+        name: sourceMetadata.place.identifier.name.value,
+        typeOfValue: sourceMetadata.typeOfValue,
+        isSource: sourceMetadata.isStateSource,
+        children: [],
+      },
+    ];
+  }
+
+  const children: Array<TreeNode> = [];
+
+  const namedSiblings: Set<string> = new Set();
+  for (const childId of sourceMetadata.sourcesIds) {
+    const childNodes = buildTreeNode(
+      childId,
+      context,
+      new Set([
+        ...visited,
+        ...(isNamedIdentifier(sourceMetadata.place)
+          ? [sourceMetadata.place.identifier.name.value]
+          : []),
+      ]),
+    );
+    if (childNodes) {
+      for (const childNode of childNodes) {
+        if (!namedSiblings.has(childNode.name)) {
+          children.push(childNode);
+          namedSiblings.add(childNode.name);
+        }
+      }
+    }
+  }
+
+  if (
+    isNamedIdentifier(sourceMetadata.place) &&
+    !visited.has(sourceMetadata.place.identifier.name.value)
+  ) {
+    return [
+      {
+        name: sourceMetadata.place.identifier.name.value,
+        typeOfValue: sourceMetadata.typeOfValue,
+        isSource: sourceMetadata.isStateSource,
+        children: children,
+      },
+    ];
+  }
+
+  return children;
+}
+
+function renderTree(
+  node: TreeNode,
+  indent: string = '',
+  isLast: boolean = true,
+  propsSet: Set<string>,
+  stateSet: Set<string>,
+): string {
+  const prefix = indent + (isLast ? '└── ' : '├── ');
+  const childIndent = indent + (isLast ? '    ' : '│   ');
+
+  let result = `${prefix}${node.name}`;
+
+  if (node.isSource) {
+    let typeLabel: string;
+    if (node.typeOfValue === 'fromProps') {
+      propsSet.add(node.name);
+      typeLabel = 'Prop';
+    } else if (node.typeOfValue === 'fromState') {
+      stateSet.add(node.name);
+      typeLabel = 'State';
+    } else {
+      propsSet.add(node.name);
+      stateSet.add(node.name);
+      typeLabel = 'Prop and State';
+    }
+    result += ` (${typeLabel})`;
+  }
+
+  if (node.children.length > 0) {
+    result += '\n';
+    node.children.forEach((child, index) => {
+      const isLastChild = index === node.children.length - 1;
+      result += renderTree(child, childIndent, isLastChild, propsSet, stateSet);
+      if (index < node.children.length - 1) {
+        result += '\n';
+      }
+    });
+  }
+
+  return result;
+}
+
 function validateEffect(
   effectFunction: HIRFunction,
   context: ValidationContext,
@@ -469,27 +627,56 @@ function validateEffect(
           .length -
           1
     ) {
-      const derivedDepsStr = Array.from(derivedSetStateCall.sourceIds)
-        .map(sourceId => {
-          const sourceMetadata = context.derivationCache.cache.get(sourceId);
-          return sourceMetadata?.place.identifier.name?.value;
-        })
-        .filter(Boolean)
-        .join(', ');
+      const propsSet = new Set<string>();
+      const stateSet = new Set<string>();
 
-      let description;
-
-      if (derivedSetStateCall.typeOfValue === 'fromProps') {
-        description = `From props: [${derivedDepsStr}]`;
-      } else if (derivedSetStateCall.typeOfValue === 'fromState') {
-        description = `From local state: [${derivedDepsStr}]`;
-      } else {
-        description = `From props and local state: [${derivedDepsStr}]`;
+      const rootNodesMap = new Map<string, TreeNode>();
+      for (const id of derivedSetStateCall.sourceIds) {
+        const nodes = buildTreeNode(id, context);
+        for (const node of nodes) {
+          if (!rootNodesMap.has(node.name)) {
+            rootNodesMap.set(node.name, node);
+          }
+        }
       }
+      const rootNodes = Array.from(rootNodesMap.values());
+
+      const trees = rootNodes.map((node, index) =>
+        renderTree(
+          node,
+          '',
+          index === rootNodes.length - 1,
+          propsSet,
+          stateSet,
+        ),
+      );
+
+      const propsArr = Array.from(propsSet);
+      const stateArr = Array.from(stateSet);
+
+      let rootSources = '';
+      if (propsArr.length > 0) {
+        rootSources += `Props: [${propsArr.join(', ')}]`;
+      }
+      if (stateArr.length > 0) {
+        if (rootSources) rootSources += '\n';
+        rootSources += `State: [${stateArr.join(', ')}]`;
+      }
+
+      const description = `Using an effect triggers an additional render which can hurt performance and user experience, potentially briefly showing stale values to the user
+
+This setState call is setting a derived value that depends on the following reactive sources:
+
+${rootSources}
+
+Data Flow Tree:
+${trees.join('\n')}
+
+See: https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state`;
 
       context.errors.pushDiagnostic(
         CompilerDiagnostic.create({
-          description: `Derived values (${description}) should be computed during render, rather than in effects. Using an effect triggers an additional render which can hurt performance and user experience, potentially briefly showing stale values to the user`,
+          description: description,
           category: ErrorCategory.EffectDerivationsOfState,
           reason:
             'You might not need an effect. Derive values in render, not effects.',

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-conditionally-in-effect.expect.md

@@ -34,7 +34,16 @@ Found 1 error:
 
 Error: You might not need an effect. Derive values in render, not effects.
 
-Derived values (From props: [value]) should be computed during render, rather than in effects. Using an effect triggers an additional render which can hurt performance and user experience, potentially briefly showing stale values to the user.
+Using an effect triggers an additional render which can hurt performance and user experience, potentially briefly showing stale values to the user
+
+This setState call is setting a derived value that depends on the following reactive sources:
+
+Props: [value]
+
+Data Flow Tree:
+└── value (Prop)
+
+See: https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state.
 
 error.derived-state-conditionally-in-effect.ts:9:6
    7 |   useEffect(() => {

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-default-props.expect.md

@@ -31,7 +31,16 @@ Found 1 error:
 
 Error: You might not need an effect. Derive values in render, not effects.
 
-Derived values (From props: [input]) should be computed during render, rather than in effects. Using an effect triggers an additional render which can hurt performance and user experience, potentially briefly showing stale values to the user.
+Using an effect triggers an additional render which can hurt performance and user experience, potentially briefly showing stale values to the user
+
+This setState call is setting a derived value that depends on the following reactive sources:
+
+Props: [input]
+
+Data Flow Tree:
+└── input (Prop)
+
+See: https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state.
 
 error.derived-state-from-default-props.ts:9:4
    7 |

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-local-state-in-effect.expect.md

@@ -28,7 +28,16 @@ Found 1 error:
 
 Error: You might not need an effect. Derive values in render, not effects.
 
-Derived values (From local state: [count]) should be computed during render, rather than in effects. Using an effect triggers an additional render which can hurt performance and user experience, potentially briefly showing stale values to the user.
+Using an effect triggers an additional render which can hurt performance and user experience, potentially briefly showing stale values to the user
+
+This setState call is setting a derived value that depends on the following reactive sources:
+
+State: [count]
+
+Data Flow Tree:
+└── count (State)
+
+See: https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state.
 
 error.derived-state-from-local-state-in-effect.ts:10:6
    8 |   useEffect(() => {

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-prop-local-state-and-component-scope.expect.md

@@ -38,7 +38,18 @@ Found 1 error:
 
 Error: You might not need an effect. Derive values in render, not effects.
 
-Derived values (From props and local state: [firstName, lastName]) should be computed during render, rather than in effects. Using an effect triggers an additional render which can hurt performance and user experience, potentially briefly showing stale values to the user.
+Using an effect triggers an additional render which can hurt performance and user experience, potentially briefly showing stale values to the user
+
+This setState call is setting a derived value that depends on the following reactive sources:
+
+Props: [firstName]
+State: [lastName]
+
+Data Flow Tree:
+├── firstName (Prop)
+└── lastName (State)
+
+See: https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state.
 
 error.derived-state-from-prop-local-state-and-component-scope.ts:11:4
    9 |

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-prop-setter-ternary.expect.md

@@ -0,0 +1,48 @@
+
+## Input
+
+```javascript
+// @validateNoDerivedComputationsInEffects_exp
+
+function Component({value}) {
+  const [checked, setChecked] = useState('');
+
+  useEffect(() => {
+    setChecked(value === '' ? [] : value.split(','));
+  }, [value]);
+
+  return <div>{checked}</div>;
+}
+
+```
+
+
+## Error
+
+```
+Found 1 error:
+
+Error: You might not need an effect. Derive values in render, not effects.
+
+Using an effect triggers an additional render which can hurt performance and user experience, potentially briefly showing stale values to the user
+
+This setState call is setting a derived value that depends on the following reactive sources:
+
+Props: [value]
+
+Data Flow Tree:
+└── value (Prop)
+
+See: https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state.
+
+error.derived-state-from-prop-setter-ternary.ts:7:4
+   5 |
+   6 |   useEffect(() => {
+>  7 |     setChecked(value === '' ? [] : value.split(','));
+     |     ^^^^^^^^^^ This should be computed during render, not in an effect
+   8 |   }, [value]);
+   9 |
+  10 |   return <div>{checked}</div>;
+```
+          
+      

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-prop-setter-ternary.js

@@ -0,0 +1,11 @@
+// @validateNoDerivedComputationsInEffects_exp
+
+function Component({value}) {
+  const [checked, setChecked] = useState('');
+
+  useEffect(() => {
+    setChecked(value === '' ? [] : value.split(','));
+  }, [value]);
+
+  return <div>{checked}</div>;
+}

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-prop-with-side-effect.expect.md

@@ -31,7 +31,16 @@ Found 1 error:
 
 Error: You might not need an effect. Derive values in render, not effects.
 
-Derived values (From props: [value]) should be computed during render, rather than in effects. Using an effect triggers an additional render which can hurt performance and user experience, potentially briefly showing stale values to the user.
+Using an effect triggers an additional render which can hurt performance and user experience, potentially briefly showing stale values to the user
+
+This setState call is setting a derived value that depends on the following reactive sources:
+
+Props: [value]
+
+Data Flow Tree:
+└── value (Prop)
+
+See: https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state.
 
 error.derived-state-from-prop-with-side-effect.ts:8:4
    6 |

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.effect-contains-local-function-call.expect.md

@@ -35,7 +35,16 @@ Found 1 error:
 
 Error: You might not need an effect. Derive values in render, not effects.
 
-Derived values (From props: [propValue]) should be computed during render, rather than in effects. Using an effect triggers an additional render which can hurt performance and user experience, potentially briefly showing stale values to the user.
+Using an effect triggers an additional render which can hurt performance and user experience, potentially briefly showing stale values to the user
+
+This setState call is setting a derived value that depends on the following reactive sources:
+
+Props: [propValue]
+
+Data Flow Tree:
+└── propValue (Prop)
+
+See: https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state.
 
 error.effect-contains-local-function-call.ts:12:4
   10 |

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.invalid-derived-computation-in-effect.expect.md

@@ -33,7 +33,16 @@ Found 1 error:
 
 Error: You might not need an effect. Derive values in render, not effects.
 
-Derived values (From local state: [firstName]) should be computed during render, rather than in effects. Using an effect triggers an additional render which can hurt performance and user experience, potentially briefly showing stale values to the user.
+Using an effect triggers an additional render which can hurt performance and user experience, potentially briefly showing stale values to the user
+
+This setState call is setting a derived value that depends on the following reactive sources:
+
+State: [firstName]
+
+Data Flow Tree:
+└── firstName (State)
+
+See: https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state.
 
 error.invalid-derived-computation-in-effect.ts:11:4
    9 |   const [fullName, setFullName] = useState('');

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.invalid-derived-state-from-computed-props.expect.md

@@ -31,7 +31,17 @@ Found 1 error:
 
 Error: You might not need an effect. Derive values in render, not effects.
 
-Derived values (From props: [props]) should be computed during render, rather than in effects. Using an effect triggers an additional render which can hurt performance and user experience, potentially briefly showing stale values to the user.
+Using an effect triggers an additional render which can hurt performance and user experience, potentially briefly showing stale values to the user
+
+This setState call is setting a derived value that depends on the following reactive sources:
+
+Props: [props]
+
+Data Flow Tree:
+└── computed
+    └── props (Prop)
+
+See: https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state.
 
 error.invalid-derived-state-from-computed-props.ts:9:4
    7 |   useEffect(() => {

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.invalid-derived-state-from-destructured-props.expect.md

@@ -32,7 +32,16 @@ Found 1 error:
 
 Error: You might not need an effect. Derive values in render, not effects.
 
-Derived values (From props: [props]) should be computed during render, rather than in effects. Using an effect triggers an additional render which can hurt performance and user experience, potentially briefly showing stale values to the user.
+Using an effect triggers an additional render which can hurt performance and user experience, potentially briefly showing stale values to the user
+
+This setState call is setting a derived value that depends on the following reactive sources:
+
+Props: [props]
+
+Data Flow Tree:
+└── props (Prop)
+
+See: https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state.
 
 error.invalid-derived-state-from-destructured-props.ts:10:4
    8 |

packages/react-devtools-shared/src/__tests__/store-test.js

@@ -3298,4 +3298,100 @@ describe('Store', () => {
           <Suspense name="Inner" rects={[{x:1,y:2,width:6,height:1}]}>
     `);
   });
+
+  // @reactVersion >= 19.0
+  it('measures rects when reconnecting', async () => {
+    function Component({children, promise}) {
+      let content = '';
+      if (promise) {
+        const value = readValue(promise);
+        if (typeof value === 'string') {
+          content += value;
+        }
+      }
+      return (
+        <div>
+          {content}
+          {children}
+        </div>
+      );
+    }
+
+    function App({outer, inner}) {
+      return (
+        <React.Suspense
+          name="outer"
+          fallback={<Component key="outer-fallback">loading outer</Component>}>
+          <Component key="outer-content" promise={outer}>
+            outer content
+          </Component>
+          <React.Suspense
+            name="inner"
+            fallback={
+              <Component key="inner-fallback">loading inner</Component>
+            }>
+            <Component key="inner-content" promise={inner}>
+              inner content
+            </Component>
+          </React.Suspense>
+        </React.Suspense>
+      );
+    }
+
+    await actAsync(() => {
+      render(<App outer={null} inner={null} />);
+    });
+
+    expect(store).toMatchInlineSnapshot(`
+      [root]
+        ▾ <App>
+          ▾ <Suspense name="outer">
+              <Component key="outer-content">
+            ▾ <Suspense name="inner">
+                <Component key="inner-content">
+      [suspense-root]  rects={[{x:1,y:2,width:13,height:1}, {x:1,y:2,width:13,height:1}]}
+        <Suspense name="outer" rects={[{x:1,y:2,width:13,height:1}, {x:1,y:2,width:13,height:1}]}>
+          <Suspense name="inner" rects={[{x:1,y:2,width:13,height:1}]}>
+    `);
+
+    let outerResolve;
+    const outerPromise = new Promise(resolve => {
+      outerResolve = resolve;
+    });
+
+    let innerResolve;
+    const innerPromise = new Promise(resolve => {
+      innerResolve = resolve;
+    });
+    await actAsync(() => {
+      render(<App outer={outerPromise} inner={innerPromise} />);
+    });
+
+    expect(store).toMatchInlineSnapshot(`
+      [root]
+        ▾ <App>
+          ▾ <Suspense name="outer">
+              <Component key="outer-fallback">
+      [suspense-root]  rects={[{x:1,y:2,width:13,height:1}, {x:1,y:2,width:13,height:1}, {x:1,y:2,width:13,height:1}]}
+        <Suspense name="outer" rects={[{x:1,y:2,width:13,height:1}, {x:1,y:2,width:13,height:1}]}>
+          <Suspense name="inner" rects={[{x:1,y:2,width:13,height:1}]}>
+    `);
+
+    await actAsync(() => {
+      outerResolve('..');
+      innerResolve('.');
+    });
+
+    expect(store).toMatchInlineSnapshot(`
+      [root]
+        ▾ <App>
+          ▾ <Suspense name="outer">
+              <Component key="outer-content">
+            ▾ <Suspense name="inner">
+                <Component key="inner-content">
+      [suspense-root]  rects={[{x:1,y:2,width:15,height:1}, {x:1,y:2,width:14,height:1}]}
+        <Suspense name="outer" rects={[{x:1,y:2,width:15,height:1}, {x:1,y:2,width:14,height:1}]}>
+          <Suspense name="inner" rects={[{x:1,y:2,width:14,height:1}]}>
+    `);
+  });
 });

packages/react-devtools-shared/src/backend/fiber/renderer.js

@@ -2586,6 +2586,17 @@ export function attach(
         }
       }
     } else {
+      const suspenseNode = fiberInstance.suspenseNode;
+      if (suspenseNode !== null && fiber.memoizedState === null) {
+        // We're reconnecting an unsuspended Suspense. Measure to see if anything changed.
+        const prevRects = suspenseNode.rects;
+        const nextRects = measureInstance(fiberInstance);
+        if (!areEqualRects(prevRects, nextRects)) {
+          suspenseNode.rects = nextRects;
+          recordSuspenseResize(suspenseNode);
+        }
+      }
+
       const {key} = fiber;
       const displayName = getDisplayNameForFiber(fiber);
       const elementType = getElementTypeForFiber(fiber);

packages/react-devtools-shared/src/devtools/store.js

@@ -669,6 +669,10 @@ export default class Store extends EventEmitter<{
     return element;
   }
 
+  containsSuspense(id: SuspenseNode['id']): boolean {
+    return this._idToSuspense.has(id);
+  }
+
   getSuspenseByID(id: SuspenseNode['id']): SuspenseNode | null {
     const suspense = this._idToSuspense.get(id);
     if (suspense === undefined) {

packages/react-devtools-shared/src/devtools/views/SuspenseTab/SuspenseRects.css

@@ -12,6 +12,11 @@
   background-color: color-mix(in srgb, var(--color-transition) 5%, transparent);
 }
 
+.SuspenseRectsRootOutline {
+  outline-width: 4px;
+  border-radius: 0.125rem;
+}
+
 .SuspenseRectsRoot[data-hovered='true'] {
   background-color: color-mix(in srgb, var(--color-transition) 15%, transparent);
 }
@@ -100,10 +105,6 @@
   pointer-events: none;
 }
 
-.SuspenseRectOutlineRoot {
-  outline-color: var(--color-transition);
-}
-
 .SuspenseRectsBoundary[data-selected='true'] > .SuspenseRectsRect {
    box-shadow: none;
 }

packages/react-devtools-shared/src/devtools/views/SuspenseTab/SuspenseRects.js

@@ -510,9 +510,12 @@ function SuspenseRectsContainer({
   let selectedBoundingBox = null;
   let selectedEnvironment = null;
   if (isRootSelected) {
-    selectedBoundingBox = boundingBox;
     selectedEnvironment = rootEnvironment;
-  } else if (inspectedElementID !== null) {
+  } else if (
+    inspectedElementID !== null &&
+    // TODO: Separate inspected element and inspected Suspense and use the inspected Suspense ID here.
+    store.containsSuspense(inspectedElementID)
+  ) {
     const selectedSuspenseNode = store.getSuspenseByID(inspectedElementID);
     if (
       selectedSuspenseNode !== null &&
@@ -534,6 +537,7 @@ function SuspenseRectsContainer({
       className={
         styles.SuspenseRectsContainer +
         (hasRootSuspenders ? ' ' + styles.SuspenseRectsRoot : '') +
+        (isRootSelected ? ' ' + styles.SuspenseRectsRootOutline : '') +
         ' ' +
         getClassNameForEnvironment(rootEnvironment)
       }
@@ -551,7 +555,6 @@ function SuspenseRectsContainer({
             <ScaledRect
               className={
                 styles.SuspenseRectOutline +
-                (isRootSelected ? ' ' + styles.SuspenseRectOutlineRoot : '') +
                 ' ' +
                 getClassNameForEnvironment(selectedEnvironment)
               }

packages/react-dom/src/__tests__/ReactDOMActivity-test.js

@@ -0,0 +1,134 @@
+/**
+ * Copyright (c) Meta Platforms, Inc. and affiliates.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the root directory of this source tree.
+ *
+ * @emails react-core
+ */
+
+'use strict';
+
+let React;
+let Activity;
+let useState;
+let ReactDOM;
+let ReactDOMClient;
+let act;
+
+describe('ReactDOMActivity', () => {
+  let container;
+
+  beforeEach(() => {
+    jest.resetModules();
+    React = require('react');
+    Activity = React.Activity;
+    useState = React.useState;
+    ReactDOM = require('react-dom');
+    ReactDOMClient = require('react-dom/client');
+    act = require('internal-test-utils').act;
+    container = document.createElement('div');
+    document.body.appendChild(container);
+  });
+
+  afterEach(() => {
+    document.body.removeChild(container);
+  });
+
+  // @gate enableActivity
+  it(
+    'hiding an Activity boundary also hides the direct children of any ' +
+      'portals it contains, regardless of how deeply nested they are',
+    async () => {
+      const portalContainer = document.createElement('div');
+
+      let setShow;
+      function Accordion({children}) {
+        const [shouldShow, _setShow] = useState(true);
+        setShow = _setShow;
+        return (
+          <Activity mode={shouldShow ? 'visible' : 'hidden'}>
+            {children}
+          </Activity>
+        );
+      }
+
+      function App({portalContents}) {
+        return (
+          <Accordion>
+            <div>
+              {ReactDOM.createPortal(
+                <div>Portal contents</div>,
+                portalContainer,
+              )}
+            </div>
+          </Accordion>
+        );
+      }
+
+      const root = ReactDOMClient.createRoot(container);
+      await act(() => root.render(<App />));
+      expect(container.innerHTML).toBe('<div></div>');
+      expect(portalContainer.innerHTML).toBe('<div>Portal contents</div>');
+
+      // Hide the Activity boundary. Not only are the nearest DOM elements hidden,
+      // but also the children of the nested portal contained within it.
+      await act(() => setShow(false));
+      expect(container.innerHTML).toBe('<div style="display: none;"></div>');
+      expect(portalContainer.innerHTML).toBe(
+        '<div style="display: none;">Portal contents</div>',
+      );
+    },
+  );
+
+  // @gate enableActivity
+  it(
+    'revealing an Activity boundary inside a portal does not reveal the ' +
+      'portal contents if has a hidden Activity parent',
+    async () => {
+      const portalContainer = document.createElement('div');
+
+      let setShow;
+      function Accordion({children}) {
+        const [shouldShow, _setShow] = useState(false);
+        setShow = _setShow;
+        return (
+          <Activity mode={shouldShow ? 'visible' : 'hidden'}>
+            {children}
+          </Activity>
+        );
+      }
+
+      function App({portalContents}) {
+        return (
+          <Activity mode="hidden">
+            <div>
+              {ReactDOM.createPortal(
+                <Accordion>
+                  <div>Portal contents</div>
+                </Accordion>,
+                portalContainer,
+              )}
+            </div>
+          </Activity>
+        );
+      }
+
+      // Start with both boundaries hidden.
+      const root = ReactDOMClient.createRoot(container);
+      await act(() => root.render(<App />));
+      expect(container.innerHTML).toBe('<div style="display: none;"></div>');
+      expect(portalContainer.innerHTML).toBe(
+        '<div style="display: none;">Portal contents</div>',
+      );
+
+      // Reveal the inner Activity boundary. It should not reveal its children,
+      // because there's a parent Activity boundary that is still hidden.
+      await act(() => setShow(true));
+      expect(container.innerHTML).toBe('<div style="display: none;"></div>');
+      expect(portalContainer.innerHTML).toBe(
+        '<div style="display: none;">Portal contents</div>',
+      );
+    },
+  );
+});

packages/react-reconciler/src/ReactFiberCommitWork.js

@@ -117,6 +117,7 @@ import {
   DidCapture,
   AffectedParentLayout,
   ViewTransitionNamedStatic,
+  PortalStatic,
 } from './ReactFiberFlags';
 import {
   commitStartTime,
@@ -1182,66 +1183,104 @@ function commitTransitionProgress(offscreenFiber: Fiber) {
   }
 }
 
-function hideOrUnhideAllChildren(finishedWork: Fiber, isHidden: boolean) {
-  // Only hide or unhide the top-most host nodes.
-  let hostSubtreeRoot = null;
+function hideOrUnhideAllChildren(parentFiber: Fiber, isHidden: boolean) {
+  if (!supportsMutation) {
+    return;
+  }
+  // Finds the nearest host component children and updates their visibility
+  // to either hidden or visible.
+  let child = parentFiber.child;
+  while (child !== null) {
+    hideOrUnhideAllChildrenOnFiber(child, isHidden);
+    child = child.sibling;
+  }
+}
 
-  if (supportsMutation) {
-    // We only have the top Fiber that was inserted but we need to recurse down its
-    // children to find all the terminal nodes.
-    let node: Fiber = finishedWork;
-    while (true) {
-      if (
-        node.tag === HostComponent ||
-        (supportsResources ? node.tag === HostHoistable : false)
-      ) {
-        if (hostSubtreeRoot === null) {
-          hostSubtreeRoot = node;
-          commitShowHideHostInstance(node, isHidden);
-        }
-      } else if (node.tag === HostText) {
-        if (hostSubtreeRoot === null) {
-          commitShowHideHostTextInstance(node, isHidden);
-        }
-      } else if (node.tag === DehydratedFragment) {
-        if (hostSubtreeRoot === null) {
-          commitShowHideSuspenseBoundary(node, isHidden);
-        }
-      } else if (
-        (node.tag === OffscreenComponent ||
-          node.tag === LegacyHiddenComponent) &&
-        (node.memoizedState: OffscreenState) !== null &&
-        node !== finishedWork
-      ) {
+function hideOrUnhideAllChildrenOnFiber(fiber: Fiber, isHidden: boolean) {
+  if (!supportsMutation) {
+    return;
+  }
+  switch (fiber.tag) {
+    case HostComponent:
+    case HostHoistable: {
+      // Found the nearest host component. Hide it.
+      commitShowHideHostInstance(fiber, isHidden);
+      // Typically, only the nearest host nodes need to be hidden, since that
+      // has the effect of also hiding everything inside of them.
+      //
+      // However, there's a special case for portals, because portals do not
+      // exist in the regular host tree hierarchy; we can't assume that just
+      // because a portal's HostComponent parent in the React tree will also be
+      // a parent in the actual host tree.
+      //
+      // So, if any portals exist within the tree, regardless of how deeply
+      // nested they are, we need to repeat this algorithm for its children.
+      hideOrUnhideNearestPortals(fiber, isHidden);
+      return;
+    }
+    case HostText: {
+      commitShowHideHostTextInstance(fiber, isHidden);
+      return;
+    }
+    case DehydratedFragment: {
+      commitShowHideSuspenseBoundary(fiber, isHidden);
+      return;
+    }
+    case OffscreenComponent:
+    case LegacyHiddenComponent: {
+      const offscreenState: OffscreenState | null = fiber.memoizedState;
+      if (offscreenState !== null) {
         // Found a nested Offscreen component that is hidden.
         // Don't search any deeper. This tree should remain hidden.
-      } else if (node.child !== null) {
-        node.child.return = node;
-        node = node.child;
-        continue;
+      } else {
+        hideOrUnhideAllChildren(fiber, isHidden);
       }
+      return;
+    }
+    default: {
+      hideOrUnhideAllChildren(fiber, isHidden);
+      return;
+    }
+  }
+}
 
-      if (node === finishedWork) {
-        return;
+function hideOrUnhideNearestPortals(parentFiber: Fiber, isHidden: boolean) {
+  if (!supportsMutation) {
+    return;
+  }
+  if (parentFiber.subtreeFlags & PortalStatic) {
+    let child = parentFiber.child;
+    while (child !== null) {
+      hideOrUnhideNearestPortalsOnFiber(child, isHidden);
+      child = child.sibling;
+    }
+  }
+}
+
+function hideOrUnhideNearestPortalsOnFiber(fiber: Fiber, isHidden: boolean) {
+  if (!supportsMutation) {
+    return;
+  }
+  switch (fiber.tag) {
+    case HostPortal: {
+      // Found a portal. Switch back to the normal hide/unhide algorithm to
+      // toggle the visibility of its children.
+      hideOrUnhideAllChildrenOnFiber(fiber, isHidden);
+      return;
+    }
+    case OffscreenComponent: {
+      const offscreenState: OffscreenState | null = fiber.memoizedState;
+      if (offscreenState !== null) {
+        // Found a nested Offscreen component that is hidden. Don't search any
+        // deeper. This tree should remain hidden.
+      } else {
+        hideOrUnhideNearestPortals(fiber, isHidden);
       }
-      while (node.sibling === null) {
-        if (node.return === null || node.return === finishedWork) {
-          return;
-        }
-
-        if (hostSubtreeRoot === node) {
-          hostSubtreeRoot = null;
-        }
-
-        node = node.return;
-      }
-
-      if (hostSubtreeRoot === node) {
-        hostSubtreeRoot = null;
-      }
-
-      node.sibling.return = node.return;
-      node = node.sibling;
+      return;
+    }
+    default: {
+      hideOrUnhideNearestPortals(fiber, isHidden);
+      return;
     }
   }
 }
@@ -2305,6 +2344,15 @@ function commitMutationEffectsOnFiber(
       break;
     }
     case HostPortal: {
+      // For the purposes of visibility toggling, the direct children of a
+      // portal are considered "children" of the nearest hidden
+      // OffscreenComponent, regardless of whether there are any host components
+      // in between them. This is because portals are not part of the regular
+      // host tree hierarchy; we can't assume that just because a portal's
+      // HostComponent parent in the React tree will also be a parent in the
+      // actual host tree. So we must hide all of them.
+      const prevOffscreenDirectParentIsHidden = offscreenDirectParentIsHidden;
+      offscreenDirectParentIsHidden = offscreenSubtreeIsHidden;
       const prevMutationContext = pushMutationContext();
       if (supportsResources) {
         const previousHoistableRoot = currentHoistableRoot;
@@ -2326,6 +2374,7 @@ function commitMutationEffectsOnFiber(
         rootViewTransitionAffected = true;
       }
       popMutationContext(prevMutationContext);
+      offscreenDirectParentIsHidden = prevOffscreenDirectParentIsHidden;
 
       if (flags & Update) {
         if (supportsPersistence) {

packages/react-reconciler/src/ReactFiberCompleteWork.js

@@ -99,6 +99,7 @@ import {
   Cloned,
   ViewTransitionStatic,
   Hydrate,
+  PortalStatic,
 } from './ReactFiberFlags';
 
 import {
@@ -1665,6 +1666,7 @@ function completeWork(
       if (current === null) {
         preparePortalMount(workInProgress.stateNode.containerInfo);
       }
+      workInProgress.flags |= PortalStatic;
       bubbleProperties(workInProgress);
       return null;
     case ContextProvider:

packages/react-reconciler/src/ReactFiberFlags.js

@@ -83,11 +83,13 @@ export const ViewTransitionNamedStatic =
 // ViewTransitionStatic tracks whether there are an ViewTransition components from
 // the nearest HostComponent down. It resets at every HostComponent level.
 export const ViewTransitionStatic = /*         */ 0b0000010000000000000000000000000;
+// Tracks whether a HostPortal is present in the tree.
+export const PortalStatic = /*                 */ 0b0000100000000000000000000000000;
 
 // Flag used to identify newly inserted fibers. It isn't reset after commit unlike `Placement`.
-export const PlacementDEV = /*                 */ 0b0000100000000000000000000000000;
-export const MountLayoutDev = /*               */ 0b0001000000000000000000000000000;
-export const MountPassiveDev = /*              */ 0b0010000000000000000000000000000;
+export const PlacementDEV = /*                 */ 0b0001000000000000000000000000000;
+export const MountLayoutDev = /*               */ 0b0010000000000000000000000000000;
+export const MountPassiveDev = /*              */ 0b0100000000000000000000000000000;
 
 // Groups of flags that are used in the commit phase to skip over trees that
 // don't contain effects, by checking subtreeFlags.
@@ -139,4 +141,5 @@ export const StaticMask =
   RefStatic |
   MaySuspendCommit |
   ViewTransitionStatic |
-  ViewTransitionNamedStatic;
+  ViewTransitionNamedStatic |
+  PortalStatic;

packages/react-server/src/ReactFizzServer.js

@@ -1291,24 +1291,13 @@ function renderSuspenseBoundary(
   const defer: boolean = enableCPUSuspense && props.defer === true;
 
   const fallbackAbortSet: Set<Task> = new Set();
-  let newBoundary: SuspenseBoundary;
-  if (canHavePreamble(task.formatContext)) {
-    newBoundary = createSuspenseBoundary(
-      request,
-      task.row,
-      fallbackAbortSet,
-      createPreamble(),
-      defer,
-    );
-  } else {
-    newBoundary = createSuspenseBoundary(
-      request,
-      task.row,
-      fallbackAbortSet,
-      null,
-      defer,
-    );
-  }
+  const newBoundary = createSuspenseBoundary(
+    request,
+    task.row,
+    fallbackAbortSet,
+    canHavePreamble(task.formatContext) ? createPreamble() : null,
+    defer,
+  );
 
   const insertionIndex = parentSegment.chunks.length;
   // The children of the boundary segment is actually the fallback.
@@ -1603,24 +1592,13 @@ function replaySuspenseBoundary(
   const defer: boolean = enableCPUSuspense && props.defer === true;
 
   const fallbackAbortSet: Set<Task> = new Set();
-  let resumedBoundary: SuspenseBoundary;
-  if (canHavePreamble(task.formatContext)) {
-    resumedBoundary = createSuspenseBoundary(
-      request,
-      task.row,
-      fallbackAbortSet,
-      createPreamble(),
-      defer,
-    );
-  } else {
-    resumedBoundary = createSuspenseBoundary(
-      request,
-      task.row,
-      fallbackAbortSet,
-      null,
-      defer,
-    );
-  }
+  const resumedBoundary = createSuspenseBoundary(
+    request,
+    task.row,
+    fallbackAbortSet,
+    canHavePreamble(task.formatContext) ? createPreamble() : null,
+    defer,
+  );
   resumedBoundary.parentFlushed = true;
   // We restore the same id of this boundary as was used during prerender.
   resumedBoundary.rootSegmentID = id;