C++ state update not received when using a path-cloning commit hook

[bartlomiejbloniarz]

Description

This issue is related to this reanimated issue, but I think it should be solved/discussed here.

High-level overview

The issue is that if we have a commit hook, that clones some nodes in the ShadowTree, applied on a commit that changes the c++ state of a ShadowNode, then this c++ state update will get lost.

More detailed description

The best way to describe this issue is to show an example. Let's say we have the following tree:

flowchart TD
    A[Parent⁰]
    B[ViewWithState⁰ <br /> state=0]
    C[SomeView⁰]

    A-->B
    A-->C

Loading

Let's say there is a state update, that changes state from 0 to 1. Before the commit hook is called we have (ViewWithState and Parent got cloned):

flowchart TD
    A[Parent ¹]
    B[ViewWithState¹ <br /> state=1]
    C[SomeView⁰]

    A-->B
    A-->C

Loading

Now let's say our commit hook want's to do some work on SomeView. Then it has to clone it (also Parent has to be cloned, as we don't really know in the commit hook if the Parent can be modified in place). This is what it should look like:

flowchart TD
    A[Parent²]
    B[ViewWithState¹ <br /> state=1]
    C[SomeView²]

    A-->B
    A-->C

Loading

But actually what happens is this:

flowchart TD
    A[Parent²]
    B[ViewWithState² <br /> **state=0**]
    C[SomeView²]

    A-->B
    A-->C

Loading

This is because when we clone the Parent, this line is hit, causing adoptYogaChild (here) to be called for every child. In adoptYogaChild there is a check that determines whether we have to clone the given child. For CustomComponentWithState it decides that it has to be cloned (even though it has never been commited to any revision). The clone call there passes empty ShadowNodeFragment causing the default value to be used, which is sourceShadowNode.getMostRecentState() - so the last mounted state. That's why the new state gets lost.

Repro explanation

I created a repro for iOS on a separate branch in rn-tester. The repro doesn't include reanimated, I just added a simple commit hook, and modified a bit the sample custom component to showcase the lost state update.

I use the changeColor function to trigger a state update. In the logs you can see how the state update is triggered, commit hook is called and an unsealed SampleNativeComponentShadowNode gets cloned, but no state update is received on the native side.

If we change the clone call in adoptYogaChild, to use the source node state, the state update will come.

I don't think that's a proper solution, because I think we shouldn't be cloning this node altogether as it is not sealed, but I don't fully understand the connection between yogaNodes and ShadowNodes, so I don't know what the fix here would be.

Steps to reproduce

1. Clone this branch
2. Run rn-tester iOS

React Native Version

main

Affected Platforms

The reproduction is for iOS, but this is mostly related to the ShadowTree state update pipeline, so should be applicable to any platform.

Areas

Fabric - The New Renderer

Reproducer

https://github.com/bartlomiejbloniarz/react-native/tree/%40bartlomiejbloniarz/commit-hook-repro

Screenshots and Videos

No response

[react-native-bot]

Warning

Could not parse version: We could not find or parse the version number of React Native in your issue report. Please use the template, and report your version including major, minor, and patch numbers - e.g. 0.76.2.

[react-native-bot]

Warning

Could not parse version: We could not find or parse the version number of React Native in your issue report. Please use the template, and report your version including major, minor, and patch numbers - e.g. 0.76.2.

[cortinico]

cc @javache @sammy-SC

[rsainiWin]

This is replicable on Android as well

[shubhamdeol]

why this is not being picked.
Is it not impacting everyone?
Using Reanimated with react native is most common setup.

[andreweinhorn]

Just +1 here. Tried upgrading to Expo 52 in January, failed (reanimated issues). Tried again two months later, 10 March, failed (reanimated issues). Ultimately many use cases of react-native-reanimated seem to fall over because of this.

Thanks to whoever is working on this. We're going back to the old architecture and Expo 51 until this gets figured out.

** In the end we decided to stick with the Expo 52 upgrade, but just disable the new architecture.