chore: deduplicate the CSS platform transition state machine - #10356
chore: deduplicate the CSS platform transition state machine#10356MatiPl01 wants to merge 7 commits into
Conversation
CSSAnimation::updatePropertyRouting read IOS_CSS_CORE_ANIMATION directly, from code compiled for every platform, so an Android build with that flag on would have taken the branch too. It is inert today only because no platform supplies a CSSPlatformAnimationFactory and the null check runs first. Move the read into canRouteCSSAnimations(), next to canRouteCSSProperty, which already splits on the preprocessor so each platform reads only its own flag. Common no longer names a platform flag outside a guard.
The reversing and interruption bookkeeping the CSS spec requires was written twice, near-verbatim: once in REACSSPlatformTransitions.mm and once in CSSPlatformTransitions.cpp. getCurrentValue was identical line for line, and the reversal block matched down to the comments, so every spec fix had to be made in two languages. Hoist it into CSSPlatformTransitionProxy, which now owns the per-property timeline, and shrink the platform seam to start and stop. Two of the four hooks disappear with it: cssCanRouteProperty, where both platforms wired the same free function, and cssGetPlatformValue, which was a pure function of the hoisted state. The backends are left with only the work that is actually platform specific, CoreAnimation on Apple and the interned-easing JNI seam on Android. No behavior change on either platform.
|
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:
📝 WalkthroughWalkthroughCSS animation routing now checks platform backend availability. 🚥 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
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/apple/reanimated/apple/CSS/REACSSPlatformTransitions.mm (1)
67-76: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftAlign the backend start value with the proxy timeline. Bruce found the real smoking gun here:
CSSPlatformTransitionProxycomputesstartValuefor its timeline, but the callback receivesfromValue, and this block can replace that value with the presentation value.getCurrentValuethen retraces a different start value from the one used byCABasicAnimation.
🧹 Nitpick comments (2)
packages/react-native-reanimated/android/src/main/cpp/reanimated/android/NativeProxy.cpp (1)
400-415: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider designated initializers for
PlatformDepMethodsHolder.This is positional aggregate initialization of a large holder whose members are guarded by platform
#ifdefblocks in some builds. Two members changed in this PR. A future insertion between members of compatible callback types binds the wrong callback with no compile error.The current signatures differ enough that a swap would not compile today, so this is not a defect. Designated initializers make each binding explicit and keep the Android and Apple initialization sites readable side by side.
♻️ Proposed refactor
return { - requestRender, - preserveMountedTags, - synchronouslyUpdateUIPropsFunction, - getAnimationTimestamp, - registerSensorFunction, - unregisterSensorFunction, - setGestureStateFunction, - subscribeForKeyboardEventsFunction, - unsubscribeFromKeyboardEventsFunction, - maybeFlushUiUpdatesQueueFunction, - attachPseudoSelectorFunction, - detachPseudoSelectorFunction, - cssStartTransition, - cssStopTransition, + .requestRender = requestRender, + .preserveMountedTags = preserveMountedTags, + .synchronouslyUpdateUIPropsFunction = synchronouslyUpdateUIPropsFunction, + .getAnimationTimestamp = getAnimationTimestamp, + .registerSensorFunction = registerSensorFunction, + .unregisterSensorFunction = unregisterSensorFunction, + .setGestureStateFunction = setGestureStateFunction, + .subscribeForKeyboardEvents = subscribeForKeyboardEventsFunction, + .unsubscribeFromKeyboardEvents = unsubscribeFromKeyboardEventsFunction, + .maybeFlushUIUpdatesQueueFunction = maybeFlushUiUpdatesQueueFunction, + .attachPseudoSelector = attachPseudoSelectorFunction, + .detachPseudoSelector = detachPseudoSelectorFunction, + .cssStartTransition = cssStartTransition, + .cssStopTransition = cssStopTransition, };Align the designator names with the declarations in
PlatformDepMethodsHolder.h.packages/react-native-reanimated/Common/cpp/reanimated/CSS/core/transition/CSSPlatformTransitionProxy.cpp (1)
59-86: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winDocument the interruption contract for
CSSStartTransitionFunction.Bruce thinks the real smoking gun here is the missing API contract. Both backends already use the live rendered value when a transition is active and use
fromValueonly for a fresh transition. State this rule inCSSPlatformTransitionProxy.h.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2f320a77-e120-4c91-a784-19424438a220
📒 Files selected for processing (13)
packages/react-native-reanimated/Common/cpp/reanimated/CSS/core/CSSAnimation.cpppackages/react-native-reanimated/Common/cpp/reanimated/CSS/core/transition/CSSPlatformTransitionProxy.cpppackages/react-native-reanimated/Common/cpp/reanimated/CSS/core/transition/CSSPlatformTransitionProxy.hpackages/react-native-reanimated/Common/cpp/reanimated/CSS/utils/platform.cpppackages/react-native-reanimated/Common/cpp/reanimated/CSS/utils/platform.hpackages/react-native-reanimated/Common/cpp/reanimated/NativeModules/ReanimatedModuleProxy.cpppackages/react-native-reanimated/Common/cpp/reanimated/Tools/PlatformDepMethodsHolder.hpackages/react-native-reanimated/android/src/main/cpp/reanimated/android/CSS/CSSPlatformTransitions.cpppackages/react-native-reanimated/android/src/main/cpp/reanimated/android/CSS/CSSPlatformTransitions.hpackages/react-native-reanimated/android/src/main/cpp/reanimated/android/NativeProxy.cpppackages/react-native-reanimated/apple/reanimated/apple/CSS/REACSSPlatformTransitions.hpackages/react-native-reanimated/apple/reanimated/apple/CSS/REACSSPlatformTransitions.mmpackages/react-native-reanimated/apple/reanimated/apple/native/PlatformDepMethodsHolderImpl.mm
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.
| id toId = idFromPlatformValue(toValue); | ||
| double durationSec = durationMs / 1000.0; | ||
| CFTimeInterval beginTime = startTimeMs / 1000.0; | ||
| CFTimeInterval beginTime = startTimestampMs / 1000.0; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect the slow-animations timestamp transform to confirm the clock basis.
set -euo pipefail
fd -a 'REASlowAnimations.(h|mm)' packages/react-native-reanimated/apple --exec rg -n -C 6 'calculateTimestampWithSlowAnimations|slowAnimation'Repository: software-mansion/react-native-reanimated
Length of output: 977
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- slow-animation timestamp implementation ---'
fd -a -i 'REASlowAnimations' packages/react-native-reanimated/apple --exec sh -c 'for f do echo "### $f"; cat -n "$f"; done' sh
echo '--- timestamp producer ---'
cat -n packages/react-native-reanimated/apple/reanimated/apple/native/PlatformDepMethodsHolderImpl.mm | sed -n '55,82p'
echo '--- transition consumer ---'
cat -n packages/react-native-reanimated/apple/reanimated/apple/CSS/REACSSPlatformTransitions.mm | sed -n '35,92p'
echo '--- related timestamp and convertTime usage ---'
rg -n -C 4 'getAnimationTimestamp|convertTime:.*fromLayer|startTimestampMs|calculateTimestampWithSlowAnimations' packages/react-native-reanimated/appleRepository: software-mansion/react-native-reanimated
Length of output: 23746
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- all transition call sites and timestamp plumbing ---'
rg -n -C 8 'startTransitionForTag|startTimestampMs|StartTransition|startTransition' packages/react-native-reanimated | head -n 500
echo '--- timestamp API declarations and consumers ---'
rg -n -C 6 'GetAnimationTimestampFunction|getAnimationTimestamp|animationTimestamp|timestampMs' packages/react-native-reanimated/src packages/react-native-reanimated/Common packages/react-native-reanimated/apple 2>/dev/null | head -n 500
echo '--- transition implementation outline and header ---'
ast-grep outline packages/react-native-reanimated/apple/reanimated/apple/CSS/REACSSPlatformTransitions.mm --view compact
cat -n packages/react-native-reanimated/apple/reanimated/apple/CSS/REACSSPlatformTransitions.h | sed -n '18,38p'Repository: software-mansion/react-native-reanimated
Length of output: 50397
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- CSS transition source files ---'
fd -a -t f . packages/react-native-reanimated/Common/cpp | rg '/CSS/|CSSTransition|CSSPlatform'
echo '--- start timestamp assignments in shared CSS code ---'
rg -n -C 5 'startTimestampMs|startTimestamp|animationTimestamp' \
packages/react-native-reanimated/Common/cpp/reanimated/CSS \
packages/react-native-reanimated/Common/cpp/reanimated 2>/dev/null | head -n 400
echo '--- relevant shared transition implementations ---'
for f in $(fd -a -t f . packages/react-native-reanimated/Common/cpp | rg '/CSS/|CSSTransition|CSSPlatform'); do
if rg -q 'startTimestampMs|startTimestamp|animationTimestamp' "$f"; then
echo "### $f"
rg -n -C 10 'startTimestampMs|startTimestamp|animationTimestamp' "$f"
fi
doneRepository: software-mansion/react-native-reanimated
Length of output: 50396
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- animation timestamp callback consumers ---'
rg -n -C 8 'getAnimationTimestamp|animationTimestampFunction|frameTimestamp|resolveTimestamp\(' \
packages/react-native-reanimated/Common/cpp \
packages/react-native-reanimated/apple/reanimated/apple/native \
packages/react-native-reanimated/apple/reanimated/apple 2>/dev/null | head -n 500
echo '--- transition routing and timestamp origin ---'
cat -n packages/react-native-reanimated/Common/cpp/reanimated/CSS/core/transition/CSSPlatformTransitionProxy.cpp | sed -n '1,95p'
rg -n -C 8 'getAnimationTimestamp|resolveTimestamp' packages/react-native-reanimated/Common/cpp/reanimated/Fabric packages/react-native-reanimated/Common/cpp/reanimated/NativeModules packages/react-native-reanimated/Common/cpp/reanimated | head -n 500
echo '--- deterministic clock-basis probe ---'
python3 - <<'PY'
anchor = 100.0
real_start = 110.0
drag_coef = 10.0
scaled_start = anchor + (real_start - anchor) / drag_coef
for real_now in (110.0, 111.0, 120.0):
scaled_now = anchor + (real_now - anchor) / drag_coef
print(f"real={real_now:.1f}s scaled={scaled_now:.1f}s delta={real_now-scaled_now:.1f}s")
print(f"beginTime passed to CoreAnimation: {scaled_start:.1f}s")
PYRepository: software-mansion/react-native-reanimated
Length of output: 50397
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- OperationsLoop timestamp resolution ---'
cat -n packages/react-native-reanimated/Common/cpp/reanimated/Fabric/updates/OperationsLoop.cpp | sed -n '1,75p'
cat -n packages/react-native-reanimated/Common/cpp/reanimated/Fabric/updates/OperationsLoop.h | sed -n '1,55p'
echo '--- native transition call with resolved timestamp ---'
cat -n packages/react-native-reanimated/Common/cpp/reanimated/CSS/core/transition/CSSPlatformTransitionProxy.cpp | sed -n '45,90p'
echo '--- clock-basis calculation ---'
python3 - <<'PY'
anchor = 100.0
drag = 10.0
real_start = 110.0
scaled_start = anchor + (real_start - anchor) / drag
real_now = 111.0
scaled_now = anchor + (real_now - anchor) / drag
print({
"real_start": real_start,
"scaled_start": scaled_start,
"real_now": real_now,
"scaled_now": scaled_now,
"real_elapsed": real_now - real_start,
"scaled_elapsed": scaled_now - scaled_start,
"begin_time_error": real_start - scaled_start,
})
PYRepository: software-mansion/react-native-reanimated
Length of output: 7628
Use an absolute Core Animation timestamp for beginTime.
Bruce found the real smoking gun here: startTimestampMs uses the slow-animation-scaled clock, but [layer convertTime:fromLayer:] expects absolute Core Animation time. Convert it to the Core Animation clock before assigning anim.beginTime; otherwise slow animations seek to the wrong position.
There was a problem hiding this comment.
Valid finding, but it predates this PR and it is simulator-only, so I am not fixing it here.
Provenance: the convertTime: line is byte-identical to the branch base (f41b838) and came from #9507; the scaled clock producer came from #7733. This PR moved the reversing state machine out of this file, the beginTime line never left it.
Scope: the warp lives entirely inside #if TARGET_IPHONE_SIMULATOR in REASlowAnimations.mm, and getUIAnimationDragCoefficient's dlsym is inside the same guard, so on device the function returns its input unchanged and the coefficient is 1. No real device can hit this.
Worth recording that the impact on the simulator is worse than a timing skew: the error is (t - T0) * (1 - 1/k), which at k=10 grows about 0.9s per real second from the moment slow animations are toggled, so within a second or so every platform-routed transition has a beginTime far enough in the past that CoreAnimation treats it as already finished, and the property snaps.
I am not applying the fix blind. beginTime is wrong under either hypothesis, but the correct correction depends on whether the simulator slows CAAnimations natively via the drag coefficient. If it does, only the absolute timestamp needs remapping and anim.duration is already right; if it does not, the platform path also plays k times faster than the loop path for the same transition. That wants an experiment on a simulator with slow animations toggled, not a guess, so it is going in its own PR.
On the other two comments in this review, which I cannot reply to inline: passing startValue to startTransition_ instead of fromValue would be a regression, not a fix. The player's live presentation-layer read and the engine's retrace are independent by design; substituting the retrace replaces a true presentation value with a model-side approximation and reintroduces the snap the comment above anim.fromValue warns about. Both are also unchanged from main.
There was a problem hiding this comment.
@MatiPl01, understood. The real smoking gun here is simulator-only and predates #10356. The separate simulator experiment is necessary before selecting a correction for beginTime and possibly anim.duration.
The virtual smoking gun here is the distinction between fromValue and the live presentation-layer value. startTransition_ should continue to receive fromValue. The backend must continue to read the presentation layer when it replaces an active animation.
Would you like me to open a GitHub follow-up issue for the simulator slow-animation investigation?
✏️ Learnings added
Learnt from: MatiPl01
Repo: software-mansion/react-native-reanimated PR: 10356
File: packages/react-native-reanimated/apple/reanimated/apple/CSS/REACSSPlatformTransitions.mm:53-53
Timestamp: 2026-08-20T21:27:57.616Z
Learning: In the Apple CSS transition backend, `REACSSPlatformTransitions.mm` must pass the engine-provided `fromValue` to `startTransition_` while independently reading `CALayer.presentationLayer` when replacing an active animation. The engine retrace value and the live presentation-layer value have different purposes; replacing `fromValue` with the retrace value can reintroduce visual snapping.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
You are interacting with an AI system.
The old proxy returned `applyTransition_ && applyTransition_(...)`, so a platform that wired no apply hook kept every property on the loop. The hoist dropped that check. Nothing can reach it today, since canRoute() already gates on the same hook and only a successful apply puts a property in routing.platform, but the pseudo-selector path calls apply() off routing state rather than off canRoute(), so restore the guard rather than rely on that.
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/CSS/core/transition/CSSPlatformTransitionProxy.cpp (1)
73-82: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPass the reconstructed current value to
startTransition_.Bruce found the real smoking gun here:
applycomputesstartValuefrom the stored timeline, butstartTransition_still receivesfromValue. During an interruption or reversal,fromValuecan be stale. The native transition can then jump because it starts from a value different from the proxy timeline.Pass
startValuewhen available. KeepfromValueas the fallback. Add regression coverage for interruptions and reversals.Proposed fix
+ const PlatformValue &transitionFromValue = startValue ? *startValue : fromValue; + if (!startTransition_ || !startTransition_( viewTag, propertyName, - fromValue, + transitionFromValue, toValue, reversing.duration,
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 937b672c-ef98-4b9c-a209-1c9fd6df863a
📒 Files selected for processing (1)
packages/react-native-reanimated/Common/cpp/reanimated/CSS/core/transition/CSSPlatformTransitionProxy.cpp
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review.
The seam typedefs already explain what startTimestampMs and persistent mean, so both backends were restating it, and three files carried a near-identical line about cancelling. Keep the per-backend docs to what only that backend knows, and bail out of apply() before the reversing work when there is no start hook rather than inside the call.
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/CSS/core/transition/CSSPlatformTransitionProxy.cpp (1)
77-85: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPass the computed live value to the native start callback.
Bruce found the real smoking gun here. Lines 68-75 compute
startValuefrom the active timeline, but this call passesfromValue. During an interruption or reversal, the native transition can jump to the stale diff value instead of continuing from the value on screen.Proposed fix
+ const auto platformStartValue = startValue.value_or(fromValue); if (!startTransition_( viewTag, propertyName, - fromValue, + platformStartValue, toValue,
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bbcb63af-77d2-4f11-b656-fd4b5c152309
📒 Files selected for processing (6)
packages/react-native-reanimated/Common/cpp/reanimated/CSS/core/transition/CSSPlatformTransitionProxy.cpppackages/react-native-reanimated/Common/cpp/reanimated/CSS/core/transition/CSSPlatformTransitionProxy.hpackages/react-native-reanimated/Common/cpp/reanimated/CSS/utils/platform.cpppackages/react-native-reanimated/Common/cpp/reanimated/CSS/utils/platform.hpackages/react-native-reanimated/android/src/main/cpp/reanimated/android/CSS/CSSPlatformTransitions.hpackages/react-native-reanimated/apple/reanimated/apple/CSS/REACSSPlatformTransitions.h
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/react-native-reanimated/Common/cpp/reanimated/CSS/utils/platform.cpp
- packages/react-native-reanimated/Common/cpp/reanimated/CSS/utils/platform.h
- packages/react-native-reanimated/android/src/main/cpp/reanimated/android/CSS/CSSPlatformTransitions.h
- packages/react-native-reanimated/apple/reanimated/apple/CSS/REACSSPlatformTransitions.h
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.
canRoute only checked for a start hook while remove checked for a stop hook, so a backend that supplied one and not the other would have routed properties it could never cancel. Require both, and restore the note that a settings-only config leaves the stored settings a revision behind for the toggle path, which was lost when the state machine moved.
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 (2)
packages/react-native-reanimated/Common/cpp/reanimated/CSS/core/transition/CSSPlatformTransitionProxy.cpp (2)
117-120: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHandle delayed zero-duration transitions
Bruce thinks the real smoking gun here is the
duration == 0branch.makeReversingStatestorestimestamp + delayinstartTimestamp, butgetCurrentValuereturns progress1.0before that timestamp. An interruption during a positive delay therefore readsadjustedEndinstead ofstartValue.Return
0.0beforereversing.startTimestampand1.0at or after it whenreversing.duration == 0. Add a test for interruption during a positive delay.
233-238: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftPreserve non-scalar resume values.
Bruce found the real smoking gun here. Apple routing accepts color and size properties, but
getResumeValue()retains only thedoublealternative. When these transitions move to the loop,resumeFromis empty and the originalfromValueis reused, which can cause a visible jump.Preserve the complete
PlatformValuefor both migration paths, or restrict routing to scalar properties. Add tests for routed color and size properties.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6ebf7382-9168-45d3-a868-b6f2219b6193
📒 Files selected for processing (2)
packages/react-native-reanimated/Common/cpp/reanimated/CSS/core/transition/CSSPlatformTransitionProxy.cpppackages/react-native-reanimated/Common/cpp/reanimated/CSS/core/transition/CSSPlatformTransitionProxy.h
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/react-native-reanimated/Common/cpp/reanimated/CSS/core/transition/CSSPlatformTransitionProxy.h
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review.
reverseShorten and the proxy's retrace both answer "how far has this ReversingState played at t, eased", off the same fields, about ninety lines apart. Name it once. The shared form is the retrace's guarded one, because a shortening factor driven negative by an overshooting easing can store a negative duration, and clamping to a hi below lo is a precondition violation reverseShorten only got away with by discarding the result. Also correct the retrace's doc: it returns nullopt when the property is not routed, not after a non-reversing interruption. active_ is keyed by property name, so the start and end values always hold the same alternative and the lerp cannot fail.
1 participant
The CSS reversing and interruption bookkeeping was implemented twice, near-verbatim, once in
REACSSPlatformTransitions.mmand once inCSSPlatformTransitions.cpp.getCurrentValuewas identical line for line and the reversal block matched down to the comments, so a spec fix had to be made in two languages to land on both platforms.This moves that state machine into
CSSPlatformTransitionProxyand reduces the platform seam tostartandstop. Two of the four hooks fall out with it:cssCanRouteProperty, where both platforms wired the same free function, andcssGetPlatformValue, which was a pure function of the hoisted state. Each backend is left with only genuinely platform-specific work.It also stops
CSSAnimation::updatePropertyRoutingfrom readingIOS_CSS_CORE_ANIMATIONin code compiled for every platform, so the two platform flags can be toggled independently.No behavior change on either platform. Verified by compiling every touched translation unit for the Android NDK and the iOS SDK, and by compiling each platform with the other platform's flag omitted from
REANIMATED_FEATURE_FLAGS(a constant-evaluated read of a missing flag is a compile error, so a clean build proves the flag is never read there), with the negative controls failing as expected.Draft: on-device validation of the platform path with the flags on is still outstanding.