refactor(LayoutAnimations): centralize animation lifecycle and UI operations - #10369
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe layout animation system now supports the 🚥 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: 3
🧹 Nitpick comments (7)
packages/react-native-reanimated/Common/cpp/reanimated/LayoutAnimations/LayoutAnimationsProxyCommon.cpp (1)
474-503: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueBruce thinks the repeated tree walks are the real smoking gun here.
findShadowNoderestarts a full depth-first walk of the shadow tree for every entry inrestorations. The cost is O(restorations × nodes) inside a commit callback. Restoration batches are normally small, so this is hygiene rather than a defect, but a single traversal keyed by tag removes the repeated walks.♻️ Suggested single-pass lookup
- PropsMap propsMap; - for (const auto &restoration : restorations) { - if (!restoration.shadowView.eventEmitter) { - continue; - } - const auto *node = findShadowNode(oldRootShadowNode, restoration.shadowView.tag); - if (!node || node->getComponentHandle() != restoration.shadowView.componentHandle || - node->getEventEmitter() != restoration.shadowView.eventEmitter) { - continue; - } - folly::dynamic opacity = folly::dynamic::object("opacity", restoration.opacity); - propsMap[node->getFamilyShared()].emplace_back(std::move(opacity)); - } + std::unordered_map<Tag, const OpacityRestoration *> byTag; + byTag.reserve(restorations.size()); + for (const auto &restoration : restorations) { + if (restoration.shadowView.eventEmitter) { + byTag.emplace(restoration.shadowView.tag, &restoration); + } + } + PropsMap propsMap; + collectRestorations(oldRootShadowNode, byTag, propsMap);
collectRestorationswalks the tree once and checksbyTagat each node, applying the samecomponentHandleandeventEmitterchecks.packages/react-native-reanimated/Common/cpp/reanimated/LayoutAnimations/LayoutAnimationsManager.cpp (1)
66-75: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse
clearLayoutAnimationConfigintakeExitingAnimationConfigAndClearTag.Bruce found the real smoking gun here: lines 70-73 duplicate the body of
clearLayoutAnimationConfigexactly. The mutex is recursive, so the call is safe under the same lock. If a new per-tag map is added later, one call site can be missed.♻️ Proposed refactor
std::shared_ptr<Serializable> LayoutAnimationsManager::takeExitingAnimationConfigAndClearTag(const int tag) { auto lock = std::unique_lock<std::recursive_mutex>(animationsMutex_); const auto configIt = exitingAnimations_.find(tag); auto config = configIt == exitingAnimations_.end() ? nullptr : configIt->second; - enteringAnimations_.erase(tag); - exitingAnimations_.erase(tag); - layoutAnimations_.erase(tag); - shouldAnimateExitingForTag_.erase(tag); + clearLayoutAnimationConfig(tag); return config; }packages/react-native-reanimated/Common/cpp/reanimated/LayoutAnimations/LayoutAnimationsManager.h (1)
53-60: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMatch the parameter qualification of the neighboring declarations.
Bruce thinks the real smoking gun here is small: line 54 declares
takeExitingAnimationConfigAndClearTag(int tag)while every neighbor usesconst int tag, and the definition inLayoutAnimationsManager.cppline 66 usesconst int tag. Top-levelconstdoes not change the signature, so this compiles. Align it for consistency in the public API.♻️ Proposed change
- std::shared_ptr<Serializable> takeExitingAnimationConfigAndClearTag(int tag); + std::shared_ptr<Serializable> takeExitingAnimationConfigAndClearTag(const int tag);packages/react-native-reanimated/Common/cpp/reanimated/LayoutAnimations/LayoutAnimationsProxyCommon.h (2)
105-111: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename one of the two
flushLayoutAnimationOperationsoverloads.Bruce thinks the real smoking gun here is the shared name with two different thread contracts. Line 105 asserts the UI thread and takes the lock itself. Line 111 accepts a held lock and, off the UI thread, only requests a flush through
requestLayoutAnimationFlush_. A protected caller that omits the lock argument by accident selects the UI-thread-only variant and trips the assert. Give the deferring variant a distinct name, for exampleflushOrRequestLayoutAnimationOperations.
187-188: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the index invariant that links the two containers.
Bruce found the real smoking gun here:
pendingLayoutAnimations_stores an index intolayoutAnimationOperations_, andupdateLayoutAnimationTargetandreparentLayoutAnimationdereference that index directly. The index stays valid only while operations are appended to the back and the deque is cleared as a whole. Any future erase from the middle silently corrupts the mapping. State the invariant next to the declarations so the constraint survives later edits.📝 Proposed comment
mutable std::deque<LayoutAnimationOperation> layoutAnimationOperations_; + // Maps a tag to the index of its pending managed start in + // `layoutAnimationOperations_`, or nullopt for progress starts. + // Invariant: operations are only appended to the back and removed by + // clearing the whole deque together with this map. Never erase a single + // element from the middle, because that invalidates the stored indices. mutable std::unordered_map<Tag, std::optional<size_t>> pendingLayoutAnimations_;packages/react-native-reanimated/Common/cpp/reanimated/LayoutAnimations/LayoutAnimationsProxyRegistry.h (1)
31-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the UI-thread requirement on the new registry API.
Bruce thinks the real smoking gun here is an undocumented thread contract.
LayoutAnimationsProxyRegistry::flushLayoutAnimationOperationsforwards toLayoutAnimationsProxyCommon::flushLayoutAnimationOperations, which assertsworklets::isOnUIThread(uiScheduler_). A future caller on the JS thread trips that assert. Add a short comment so the constraint is visible at the declaration.📝 Proposed comment
+ // Must be called on the UI thread; each proxy asserts this. void flushLayoutAnimationOperations() const;packages/react-native-reanimated/src/layoutReanimation/animationsManager.native.ts (1)
41-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCorrect the
sharedValuetype onstopObservingProgress.Bruce found the real smoking gun here: line 43 types the parameter as
SharedValue<number>, but both call sites pass the value created bymakeMutableUI(style.initialValues), which holds a style record.startObservingProgresstypes the same value asSharedValue<Record<string, unknown>>. The mismatch is hidden today only becausevaluecomes from an untypedMap, so it isany. Align the two signatures.🔧 Proposed fix
function stopObservingProgress( tag: number, - sharedValue: SharedValue<number> + sharedValue: SharedValue<Record<string, unknown>> ): void {
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 79e16dc4-3ccc-48fc-b789-c3eef4d397c5
📒 Files selected for processing (17)
packages/react-native-reanimated/Common/cpp/reanimated/LayoutAnimations/LayoutAnimationType.hpackages/react-native-reanimated/Common/cpp/reanimated/LayoutAnimations/LayoutAnimationsManager.cpppackages/react-native-reanimated/Common/cpp/reanimated/LayoutAnimations/LayoutAnimationsManager.hpackages/react-native-reanimated/Common/cpp/reanimated/LayoutAnimations/LayoutAnimationsProxyCommon.cpppackages/react-native-reanimated/Common/cpp/reanimated/LayoutAnimations/LayoutAnimationsProxyCommon.hpackages/react-native-reanimated/Common/cpp/reanimated/LayoutAnimations/LayoutAnimationsProxyRegistry.cpppackages/react-native-reanimated/Common/cpp/reanimated/LayoutAnimations/LayoutAnimationsProxyRegistry.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/LayoutAnimationsProxy_Legacy.cpppackages/react-native-reanimated/Common/cpp/reanimated/LayoutAnimations/LayoutAnimationsProxy_Legacy.hpackages/react-native-reanimated/Common/cpp/reanimated/LayoutAnimations/LayoutAnimationsUtils.hpackages/react-native-reanimated/Common/cpp/reanimated/LayoutAnimations/SharedTransitions.cpppackages/react-native-reanimated/Common/cpp/reanimated/NativeModules/ReanimatedModuleProxy.cpppackages/react-native-reanimated/Common/cpp/reanimated/NativeModules/ReanimatedModuleProxy.hpackages/react-native-reanimated/__tests__/layoutAnimationsManager.test.tspackages/react-native-reanimated/src/layoutReanimation/animationsManager.native.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review.
a263940 to
fdc72b4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/react-native-reanimated/Common/cpp/reanimated/LayoutAnimations/LayoutAnimationsProxyCommon.cpp (1)
20-30: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winBruce found the real smoking gun here: the shadow tree is walked once per restoration.
findShadowNodeperforms a full recursive descent from the root. The loop at Line 489 calls it once for every entry inrestorations. The cost is O(tree size × restoration count), and it runs on the JS thread inside a commit lambda that React Native may retry.Collect the target tags into a lookup set and resolve them in a single traversal.
♻️ Proposed single-traversal resolution
`#ifdef` ANDROID namespace { -const ShadowNode *findShadowNode(const ShadowNode &node, const Tag tag) { - if (node.getTag() == tag) { - return &node; - } - for (const auto &child : node.getChildren()) { - if (const auto *result = findShadowNode(*child, tag)) { - return result; - } - } - return nullptr; -} +void collectShadowNodes( + const ShadowNode &node, + const std::unordered_set<Tag> &tags, + std::unordered_map<Tag, const ShadowNode *> &found) { + if (tags.contains(node.getTag())) { + found.emplace(node.getTag(), &node); + } + if (found.size() == tags.size()) { + return; + } + for (const auto &child : node.getChildren()) { + collectShadowNodes(*child, tags, found); + } +} } // namespace `#endif`Then resolve each restoration from the
foundmap instead of callingfindShadowNodeinside the loop.Also applies to: 489-500
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 26eaea4e-af9e-4ea8-b2a6-2116bd0214c4
📒 Files selected for processing (5)
packages/react-native-reanimated/Common/cpp/reanimated/LayoutAnimations/LayoutAnimationsProxyCommon.cpppackages/react-native-reanimated/Common/cpp/reanimated/LayoutAnimations/LayoutAnimationsProxyCommon.hpackages/react-native-reanimated/Common/cpp/reanimated/LayoutAnimations/LayoutAnimationsProxyRegistry.cpppackages/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; 8 remain after this review.
fdc72b4 to
fe08ba1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/react-native-reanimated/Common/cpp/reanimated/LayoutAnimations/LayoutAnimationsProxyCommon.cpp (1)
350-399: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBruce thinks the duplicated pending-start lookup is worth extracting.
updateLayoutAnimationTargetandreparentLayoutAnimationrepeat the same four steps: find the tag inpendingLayoutAnimations_, check the optional index, assert the index bound, andget_if<ManagedLayoutAnimationStart>. The index-validity invariant now lives in two places. Extract a private helper that returnsManagedLayoutAnimationStart *ornullptr, so future changes to the index bookkeeping only touch one site.♻️ Proposed helper
ManagedLayoutAnimationStart *LayoutAnimationsProxyCommon::findPendingManagedStart(const Tag tag) const { const auto pendingIt = pendingLayoutAnimations_.find(tag); if (pendingIt == pendingLayoutAnimations_.end() || !pendingIt->second) { return nullptr; } const auto operationIndex = *pendingIt->second; react_native_assert(operationIndex < layoutAnimationOperations_.size()); return std::get_if<ManagedLayoutAnimationStart>(&layoutAnimationOperations_[operationIndex]); }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d14de7de-042e-4387-9696-f42ec167ca6c
📒 Files selected for processing (4)
packages/react-native-reanimated/Common/cpp/reanimated/LayoutAnimations/LayoutAnimationsManager.hpackages/react-native-reanimated/Common/cpp/reanimated/LayoutAnimations/LayoutAnimationsProxyCommon.cpppackages/react-native-reanimated/Common/cpp/reanimated/LayoutAnimations/LayoutAnimationsProxy_Legacy.cpppackages/react-native-reanimated/Common/cpp/reanimated/LayoutAnimations/LayoutAnimationsUtils.h
Included review availability: Your plan provides up to 10 included reviews per hour; 4 remain after this review.
fe08ba1 to
c1795d0
Compare
c1795d0 to
d714f26
Compare
d714f26 to
4296375
Compare
4296375 to
4119640
Compare
67f6e9c to
023fd5d
Compare
|
@coderabbitai review |
|
|
@coderabbitai review |
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
023fd5d to
793a218
Compare
793a218 to
4a0948c
Compare
4a0948c to
825ed53
Compare
825ed53 to
f19034d
Compare
…rations Represent running and completed animations in separate maps and keep per-pull scratch data in TransactionMeta. Animation configs are captured when a start is chosen, including destructive exit-config reads. Replace the per-proxy scheduleOnUI paths with a common operation queue. Starts and cancellations are reconciled before the UI-thread flush, and the common proxy now owns materialization, progress updates, cancellation, completion cleanup, opacity restoration, and window tracking. Progress transitions use the native-only PROGRESS type, so replacing a managed shared transition stops its JavaScript animation before gesture updates take ownership.
f19034d to
ef6fcc7
Compare
pendingLayoutAnimations_ mapped a tag to optional<size_t>, where nullopt
secretly meant a pending progress start. Store a PendingLayoutAnimation
{type, operationIndex} instead, with type = PROGRESS as the progress
marker. This also fixes updateLayoutAnimationTarget asserting
"Layout animation not found" when retargeting a tag whose only pending
start is a progress one.
ef6fcc7 to
02a0950
Compare
2 participants
Before this change, each animation start and cancellation scheduled its own
scheduleOnUIcallback. These callbacks reached the UI queue separately and could run around pulls, completions, and newer animation requests. The Legacy and Experimental proxies also handled parts of this flow independently. We could not inspect or reason about the full set of operations as one unit.Now
enqueueLayoutAnimationrecords starts and cancellations in a common queue.flushLayoutAnimationOperationsreconciles and executes the queue as one batch at a defined point inperformOperations. This gives us a clear boundary where pending operations become UI work. It also lets us enforce cancellation, replacement, and completion rules before anything reachesLayoutAnimationsManager.Threading analysis
Full AI diagram
Test Plan
layoutAnimationsManager.test.ts.