refactor(LayoutAnimations): make shared container ownership explicit - #10370
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe experimental layout-animation proxy now uses per-surface state and 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/react-native-reanimated/Common/cpp/reanimated/LayoutAnimations/SharedTransitions.cpp (1)
238-277: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBruce thinks the duplicated "newest container for a shared tag" scan is the real smoking gun here.
Lines 239-243 scan
sharedContainers_for the highest tag with a matchingsharedTag. Lines 321-327 inhandleSharedTransitionsStartrepeat the same scan with an extra animation filter. Extract one helper so both call sites stay in sync when the selection rule changes.♻️ Suggested helper
// LayoutAnimationsProxy_Experimental.h Tag findNewestContainer(const SharedTag &sharedTag, bool requireActiveAnimation) const; // SharedTransitions.cpp Tag LayoutAnimationsProxy_Experimental::findNewestContainer( const SharedTag &sharedTag, const bool requireActiveAnimation) const { auto result = Tag{-1}; for (const auto &[tag, container] : sharedContainers_) { if (container.sharedTag != sharedTag || tag <= result) { continue; } if (requireActiveAnimation && !hasPendingLayoutAnimation(tag) && !layoutAnimations_.contains(tag)) { continue; } result = tag; } return result; }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ef7389da-c1d7-4abf-bf73-340a8cb20949
📒 Files selected for processing (4)
packages/react-native-reanimated/Common/cpp/reanimated/LayoutAnimations/LayoutAnimationsManager.hpackages/react-native-reanimated/Common/cpp/reanimated/LayoutAnimations/LayoutAnimationsProxy_Experimental.cpppackages/react-native-reanimated/Common/cpp/reanimated/LayoutAnimations/LayoutAnimationsProxy_Experimental.hpackages/react-native-reanimated/Common/cpp/reanimated/LayoutAnimations/SharedTransitions.cpp
💤 Files with no reviewable changes (1)
- packages/react-native-reanimated/Common/cpp/reanimated/LayoutAnimations/LayoutAnimationsManager.h
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.
4570a46 to
de30f18
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/react-native-reanimated/Common/cpp/reanimated/LayoutAnimations/LayoutAnimationsProxy_Experimental.cpp (2)
154-158: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAdd the null check that
endLayoutAnimationalready applies.Bruce found the real smoking gun here on line 155.
it->second->statedereferences the mapped value without checking it.lightNodes_is populated withoperator[]in several places in this file, for example lines 224, 268, 269, 300, and 303, andoperator[]inserts a nullshared_ptrfor an absent key. A null entry survives into the next transaction, andreconcileContradictedRemovalsruns first inpullTransaction.The comment at lines 430-432 in
endLayoutAnimationstates this hazard and guards against it. Apply the same guard here. The rest of this function is already release-safe, withif (!parent)andif (index == -1).🛡️ Proposed guard
const auto it = lightNodes_.find(tag); - if (it == lightNodes_.end() || it->second->state == UNDEFINED) { + if (it == lightNodes_.end() || !it->second || it->second->state == UNDEFINED) { continue; }
499-511: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard
parentandindexbefore use, as the sibling cleanup path does.Bruce found the real smoking gun here in the new completed-removal loop. Lines 505 and 507 use
react_native_assert, which compiles out in release builds. Line 506 then dereferencesparent, and line 509 passesindextoendAnimationsRecursively, which buildsShadowViewMutation::RemoveMutation(parent->current.tag, node->current, index)at line 597. In release, anindexof -1 reaches the mounting layer as an out-of-range child removal.The path is reachable inside this same loop.
maybeDropAncestorsat line 510 detaches ancestors and clears children, so a later tag incompletedRemovalTagscan resolve to a node whose parent link is already gone.
reconcileContradictedRemovalshandles the identical situation at lines 166-175 and returns early on both conditions. Line 500 also needs the null check described in my comment on lines 154-158; restating the reasoning would be the virtual smoking gun here.🛡️ Proposed guards
const auto nodeIt = lightNodes_.find(tag); - if (nodeIt == lightNodes_.end() || nodeIt->second->state != DEAD) { + if (nodeIt == lightNodes_.end() || !nodeIt->second || nodeIt->second->state != DEAD) { continue; } const auto node = nodeIt->second; auto parent = node->parent.lock(); react_native_assert(parent && "Parent node is nullptr"); + if (!parent) { + continue; + } auto index = parent->removeChild(node); react_native_assert(index != -1 && "Dead node not found"); + if (index == -1) { + continue; + } endAnimationsRecursively(node, index, filteredMutations); maybeDropAncestors(parent, filteredMutations);
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: cbfc8a2e-62d7-4ea3-a8c4-132fdb607635
📒 Files selected for processing (2)
packages/react-native-reanimated/Common/cpp/reanimated/LayoutAnimations/LayoutAnimationsProxy_Experimental.cpppackages/react-native-reanimated/Common/cpp/reanimated/LayoutAnimations/LayoutAnimationsProxy_Experimental.h
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review.
de30f18 to
4ddc75a
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/react-native-reanimated/Common/cpp/reanimated/LayoutAnimations/SharedTransitions.cpp (1)
109-111: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winLook up
transitionTag_withfindbefore dereferencing it.Bruce found the real smoking gun here.
lightNodes_[transitionTag_]usesoperator[], which inserts a nullstd::shared_ptr<LightNode>when the tag is gone.findBoundaryGuessthen dereferences that null pointer inisSETBoundary(node), so the app crashes. The map also keeps the null entry, so later lookups report the tag as present.
transitionTag_is set in an earlier transaction byonTransitionProgress. The screen node can be unmounted before this handler runs. Lines 396-399 and 427-430 already guard the same map withfind, so apply the same guard here. Do not return early, because the state machine at lines 187-212 must still run.🐛 Proposed fix
auto beforeTopScreen = topScreen_; - auto afterTopScreen = findBoundaryGuess(lightNodes_[transitionTag_]); + std::shared_ptr<LightNode> afterTopScreen; + if (const auto transitionNodeIt = lightNodes_.find(transitionTag_); + transitionNodeIt != lightNodes_.end() && transitionNodeIt->second) { + afterTopScreen = findBoundaryGuess(transitionNodeIt->second); + } if (beforeTopScreen && afterTopScreen && beforeTopScreen != afterTopScreen) {
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b3defd30-2360-4bc6-b635-9a443ab32e7f
📒 Files selected for processing (2)
packages/react-native-reanimated/Common/cpp/reanimated/LayoutAnimations/LayoutAnimationsManager.hpackages/react-native-reanimated/Common/cpp/reanimated/LayoutAnimations/SharedTransitions.cpp
Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review.
4ddc75a to
e08485b
Compare
e08485b to
c394a8f
Compare
2234ebf to
dfee579
Compare
8e85dd6 to
f020535
Compare
|
@coderabbitai review |
|
|
@coderabbitai review |
|
|
@coderabbitai review |
|
|
@coderabbitai review |
|
f020535 to
3e6e9d7
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/react-native-reanimated/Common/cpp/reanimated/LayoutAnimations/SharedTransitions.cpp (1)
110-112: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winLook up
transitionTag_withfindinstead ofoperator[].
lightNodes_[transitionTag_]inserts a nullstd::shared_ptr<LightNode>when the tag is absent. Two consequences follow.findBoundaryGuessthen dereferences a null node. The map also keeps a null entry for that tag, which later lookups incleanupSharedTransitionsandgetOrCreateContainermust tolerate.
onTransitionProgressvalidates the node only when it setstransitionTag_. The screen can unmount before this transaction runs.🛡️ Proposed fix
- auto beforeTopScreen = topScreen_; - auto afterTopScreen = findBoundaryGuess(lightNodes_[transitionTag_]); + auto beforeTopScreen = topScreen_; + const auto transitionNodeIt = lightNodes_.find(transitionTag_); + const auto afterTopScreen = (transitionNodeIt == lightNodes_.end() || !transitionNodeIt->second) + ? nullptr + : findBoundaryGuess(transitionNodeIt->second);
🧹 Nitpick comments (1)
packages/react-native-reanimated/Common/cpp/reanimated/LayoutAnimations/SharedTransitions.cpp (1)
314-321: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated container selection and restore-node update.
Three blocks repeat the same logic. Lines 244-248 and lines 330-336 both scan
sharedContainers_for the newest container with a matchingsharedTag. Lines 316-319 and lines 351-354 both queue the previousrestoreAfterNodeand then assign the new one. Two small private helpers, for examplefindNewestContainerForTag(sharedTag, requireActive)andsetRestoreAfterNode(container, afterNode, transaction), keep the two branches in sync when this logic changes again.Also applies to: 330-336, 350-354
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b03246b5-c857-468a-ab83-829232dbec4d
📒 Files selected for processing (2)
packages/react-native-reanimated/Common/cpp/reanimated/LayoutAnimations/LayoutAnimationsManager.cpppackages/react-native-reanimated/Common/cpp/reanimated/LayoutAnimations/SharedTransitions.cpp
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review.
3e6e9d7 to
c2d66a1
Compare
c2d66a1 to
fa0f195
Compare
2fd1e22 to
2f2e07e
Compare
2f2e07e to
3c006f8
Compare
c5d9b0e to
e9deafb
Compare
e9deafb to
6929ca8
Compare
Collapse containerTags_, restoreMap_, activeTransitions_, and the container entries in the cross-surface tagToName_ map into a single sharedContainers_ map owning the container's LightNode plus references to the restore source/target nodes. Restores are validated by node identity instead of tag, and TransactionMeta::hiddenNodes suppresses restoring a view that the same transaction just hid for a new transition. Fixes folded into the ownership change: - Container LightNodes are now erased from lightNodes_ on removal (previously leaked one stale entry per container). - Identity-checked restores no longer un-hide an unrelated view that reused a recycled tag. - Replacing a completed container eagerly restores and removes the old one instead of silently overwriting the mapping. - Container tags are no longer registered in (or leaked into) the shared tagToName_ map.
…upAnimations Starting an animation on a tag erases its completedAnimations_ entry, so a completed tag can never also be in layoutAnimations_.
…etes within the pull A shared transition whose source and target match exactly has nothing to animate, so it completes inside the pull that started it and cleanup removes its container right away. The restore of the hidden target was then skipped because the target had been hidden in the same pull, leaving it at opacity 0. The skip existed to keep a previous target hidden when it becomes the source or target of the transition being started. Decide that where the old target is queued instead: restoreOldTarget skips only nodes the new transition hides, and the per-pull hidden set is gone.
4a17dc4 to
b12201d
Compare
2 participants
Before this change, the state of one shared container was split across several maps and sets. Tags connected these structures, but no single object owned the complete container state. A cleanup path could remove one part and leave another behind. Tag reuse could then match a new node with stale restore state.
Now
sharedContainers_stores oneSharedContainerrecord with the container node and its restore nodes. Creation, updates, andremoveSharedContainerall work on that record. This gives each container one ownership boundary and one cleanup path. Complete cleanup is now enforced by the structure of the data.Test Plan