Repository navigation
feat: Add the Android animator for CSS platform transitions - #10090
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAndroid CSS transition support adds opacity property access, duration-scale handling, CSS easing conversion, and a manager that resolves views and controls synchronized ChangesAndroid CSS transition playback
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CSSPlatformTransitionsManager
participant DurationScale
participant FabricUIManager
participant CSSEasing
participant ObjectAnimator
CSSPlatformTransitionsManager->>DurationScale: effectiveScale(reactContext)
CSSPlatformTransitionsManager->>FabricUIManager: resolveView(viewTag)
CSSPlatformTransitionsManager->>CSSEasing: create interpolator(easing type and points)
CSSPlatformTransitionsManager->>ObjectAnimator: configure and start transition
ObjectAnimator-->>CSSPlatformTransitionsManager: complete and clear transition
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/react-native-reanimated/android/src/main/java/com/swmansion/reanimated/css/CSSPlatformTransitionsManager.kt`:
- Around line 63-68: Update the transition method surrounding viewForTag and its
UiThreadUtil callback so it does not return true when the native view cannot be
resolved. Make ownership conditional on successfully obtaining the view, or
provide the required deferred/failure handoff so callers retain control when
viewForTag returns null; preserve the existing start flow for resolved views.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1491d62e-3f51-42d2-809e-09e620cdf018
📒 Files selected for processing (4)
packages/react-native-reanimated/android/src/main/java/com/swmansion/reanimated/css/CSSEasing.ktpackages/react-native-reanimated/android/src/main/java/com/swmansion/reanimated/css/CSSPlatformTransitionsManager.ktpackages/react-native-reanimated/android/src/main/java/com/swmansion/reanimated/css/CSSViewChannel.ktpackages/react-native-reanimated/android/src/main/java/com/swmansion/reanimated/css/DurationScale.kt
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/react-native-reanimated/android/src/main/java/com/swmansion/reanimated/css/CSSEasing.kt`:
- Line 7: Update the documentation comment near the easing implementations to
scope the “reproduced rather than sampled” statement only to the custom
interpolators, specifically the steps() and linear-stop implementations. Remove
any implication that PathInterpolator or all easing curves avoid internal
sampling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 45dcd9d4-0bd8-4079-9186-2c0ec84cd80e
📒 Files selected for processing (4)
packages/react-native-reanimated/android/src/main/java/com/swmansion/reanimated/css/CSSEasing.ktpackages/react-native-reanimated/android/src/main/java/com/swmansion/reanimated/css/CSSPlatformTransitionsManager.ktpackages/react-native-reanimated/android/src/main/java/com/swmansion/reanimated/css/CSSViewChannel.ktpackages/react-native-reanimated/android/src/main/java/com/swmansion/reanimated/css/DurationScale.kt
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/react-native-reanimated/android/src/main/java/com/swmansion/reanimated/css/CSSPlatformTransitionsManager.kt (1)
51-56:⚠️ Potential issue | 🟠 MajorDo not report success when the native view is unavailable.
viewForTag(viewTag)can returnnull, butanimateTransitionstill returnstrue; line 52 then silently drops the transition. Make ownership conditional on resolving the view, or add a deferred failure handoff so the caller retains its fallback timeline.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/react-native-reanimated/android/src/main/java/com/swmansion/reanimated/css/CSSPlatformTransitionsManager.kt` around lines 51 - 56, Update animateTransition so it does not return success when viewForTag(viewTag) resolves to null; ensure ownership remains with the caller by returning false or using the established deferred failure handoff before the UI-thread callback drops the transition. Preserve the existing start flow when a native view is available.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In
`@packages/react-native-reanimated/android/src/main/java/com/swmansion/reanimated/css/CSSPlatformTransitionsManager.kt`:
- Around line 51-56: Update animateTransition so it does not return success when
viewForTag(viewTag) resolves to null; ensure ownership remains with the caller
by returning false or using the established deferred failure handoff before the
UI-thread callback drops the transition. Preserve the existing start flow when a
native view is available.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 067c92be-f9f9-4f54-8b58-f4df0e4896bf
📒 Files selected for processing (3)
packages/react-native-reanimated/android/src/main/java/com/swmansion/reanimated/css/CSSPlatformTransitionsManager.ktpackages/react-native-reanimated/android/src/main/java/com/swmansion/reanimated/css/CSSPropertyWriter.ktpackages/react-native-reanimated/android/src/main/java/com/swmansion/reanimated/css/DurationScale.kt
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/react-native-reanimated/android/src/main/java/com/swmansion/reanimated/css/CSSPlatformTransitionsManager.kt (1)
61-63: 🎯 Functional Correctness | 🟠 MajorDo not claim ownership after an unsuccessful retry.
If both
begin()attempts fail, no animator is created, butanimateTransition()still returnstrue. Preserve a failure handoff, or explicitly definetrueas “accepted for asynchronous retry” rather than successful native playback. This repeats the unresolved ownership issue from the previous review.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/react-native-reanimated/android/src/main/java/com/swmansion/reanimated/css/CSSPlatformTransitionsManager.kt` around lines 61 - 63, Update animateTransition() and the retry path around begin() so a failed immediate attempt followed by a failed Choreographer retry does not return true or claim animation ownership without creating an animator. Either propagate the failure handoff after the retry or explicitly make true represent acceptance for asynchronous retry, consistently preserving ownership semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/react-native-reanimated/android/src/main/java/com/swmansion/reanimated/css/CSSPlatformTransitionsManager.kt`:
- Line 61: Update the retry flow around begin() in CSSPlatformTransitionsManager
to retain a pending frame callback generation/token per transition key. Clear or
invalidate that token in removeTransition() and when a newer request supersedes
the same key, and validate it before the posted callback invokes begin() so
stale retries cannot start obsolete transitions.
---
Duplicate comments:
In
`@packages/react-native-reanimated/android/src/main/java/com/swmansion/reanimated/css/CSSPlatformTransitionsManager.kt`:
- Around line 61-63: Update animateTransition() and the retry path around
begin() so a failed immediate attempt followed by a failed Choreographer retry
does not return true or claim animation ownership without creating an animator.
Either propagate the failure handoff after the retry or explicitly make true
represent acceptance for asynchronous retry, consistently preserving ownership
semantics.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 900286a5-e8e2-4bbc-8e4e-ea52bab0a529
📒 Files selected for processing (2)
packages/react-native-reanimated/android/src/main/java/com/swmansion/reanimated/css/CSSEasing.ktpackages/react-native-reanimated/android/src/main/java/com/swmansion/reanimated/css/CSSPlatformTransitionsManager.kt
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/react-native-reanimated/android/src/main/java/com/swmansion/reanimated/css/CSSPlatformTransitionsManager.kt`:
- Around line 80-85: Update the Choreographer callback in the transition-start
flow so it does not permanently remove the start token when the view is still
unmounted. Retry on subsequent frames or use mount notifications until the view
exists, while enforcing a bounded timeout or otherwise failing explicitly; only
clear startTokens and call begin() once the transition can start.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2d878d51-ce92-455d-84bd-81020d72db16
📒 Files selected for processing (1)
packages/react-native-reanimated/android/src/main/java/com/swmansion/reanimated/css/CSSPlatformTransitionsManager.kt
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/android/src/main/java/com/swmansion/reanimated/css/CSSPlatformTransitionsManager.kt (1)
166-167: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winBound the interpolator cache.
Line 166 creates a distinct cache key for every easing point list. Line 167 retains the key and interpolator with no eviction. Dynamic transition timing values can grow this map for the manager lifetime and cause memory pressure.
Use a bounded cache with eviction, or avoid caching non-preset easing definitions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/react-native-reanimated/android/src/main/java/com/swmansion/reanimated/css/CSSPlatformTransitionsManager.kt` around lines 166 - 167, Bound the cache used by the interpolator lookup around InterpolatorKey and interpolators so dynamically generated easing point lists cannot accumulate for the manager lifetime. Prefer the existing cache implementation if it supports a fixed capacity and eviction; otherwise bypass caching for non-preset easing definitions while preserving cached preset interpolators.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@packages/react-native-reanimated/android/src/main/java/com/swmansion/reanimated/css/CSSPlatformTransitionsManager.kt`:
- Around line 166-167: Bound the cache used by the interpolator lookup around
InterpolatorKey and interpolators so dynamically generated easing point lists
cannot accumulate for the manager lifetime. Prefer the existing cache
implementation if it supports a fixed capacity and eviction; otherwise bypass
caching for non-preset easing definitions while preserving cached preset
interpolators.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e099afbb-d0e0-474d-aa31-2fd1d6954ab9
📒 Files selected for processing (1)
packages/react-native-reanimated/android/src/main/java/com/swmansion/reanimated/css/CSSPlatformTransitionsManager.kt
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/android/src/main/java/com/swmansion/reanimated/css/CSSPlatformTransitionsManager.kt (2)
92-103: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCheck expiry before
begin().Line 99 invokes
begin()before it checksendTimestampMs. If theViewmounts after this timestamp,begin()starts an expired animator.Check the timestamp first. Remove the token and return without calling
begin()when the transition has ended.Proposed fix
- if (begin() || animationTimestamp() >= endTimestampMs) { + if (animationTimestamp() >= endTimestampMs || begin()) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/react-native-reanimated/android/src/main/java/com/swmansion/reanimated/css/CSSPlatformTransitionsManager.kt` around lines 92 - 103, Update beginWhenMounted to check animationTimestamp() against endTimestampMs before invoking begin(). When the transition has expired, remove startTokens[key] and return without calling begin(); otherwise preserve the existing begin() success handling and frame-callback retry flow.
26-26: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winBound the interpolator cache.
Each distinct timing function retains boxed point lists and an interpolator for the manager lifetime. Dynamic CSS timing functions can grow this map without a limit.
Use a bounded cache. Alternatively, cache only
PathInterpolatorinstances.Also applies to: 159-160
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/react-native-reanimated/android/src/main/java/com/swmansion/reanimated/css/CSSPlatformTransitionsManager.kt` at line 26, Bound the interpolator cache used by CSSPlatformTransitionsManager.interpolators so dynamically generated timing functions cannot grow it indefinitely. Replace the unbounded HashMap with the project’s established bounded-cache mechanism and preserve lookup/reuse behavior; alternatively, restrict caching to PathInterpolator instances while leaving other interpolators uncached.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@packages/react-native-reanimated/android/src/main/java/com/swmansion/reanimated/css/CSSPlatformTransitionsManager.kt`:
- Around line 92-103: Update beginWhenMounted to check animationTimestamp()
against endTimestampMs before invoking begin(). When the transition has expired,
remove startTokens[key] and return without calling begin(); otherwise preserve
the existing begin() success handling and frame-callback retry flow.
- Line 26: Bound the interpolator cache used by
CSSPlatformTransitionsManager.interpolators so dynamically generated timing
functions cannot grow it indefinitely. Replace the unbounded HashMap with the
project’s established bounded-cache mechanism and preserve lookup/reuse
behavior; alternatively, restrict caching to PathInterpolator instances while
leaving other interpolators uncached.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: da709d87-20cb-4b04-93bc-3fb9de32a7df
📒 Files selected for processing (4)
packages/react-native-reanimated/android/src/main/java/com/swmansion/reanimated/css/CSSEasing.ktpackages/react-native-reanimated/android/src/main/java/com/swmansion/reanimated/css/CSSPlatformTransitionsManager.ktpackages/react-native-reanimated/android/src/main/java/com/swmansion/reanimated/css/CSSPropertyWriter.ktpackages/react-native-reanimated/android/src/main/java/com/swmansion/reanimated/css/DurationScale.kt
55089ed to
7c49248
Compare
Plays an already resolved CSS transition timeline on a native View with ObjectAnimator. Nothing constructs it yet; the C++ resolver and the JNI bridge follow separately. A property rides a View channel that React Native never writes itself, so a Fabric commit re-applying the prop cannot clobber a running animation. opacity uses setTransitionAlpha, a second multiplicative alpha factor: to render f the animator writes f / alpha and pins alpha to the target React is about to commit. Anything times zero is zero, so a fade to fully transparent is inexpressible and the channel declines it. Easings are rebuilt rather than sampled, so steps() keeps its discontinuities at any duration, and the point search mirrors firstSmallerOrEqual so a run of equal x resolves to its last entry. An interruption resumes from the value on screen instead of the committed style value, which would snap the view back to where the cancelled transition began.
The caller resolves the timeline on the JS thread, but the animator only starts after the hop to the UI thread and the wait for the next frame. Passing an elapsed offset baked in at resolve time shifts the whole timeline late by that gap; taking the start timestamp and subtracting here absorbs it, which is what CALayer.beginTime does on Apple. Both sides read the same clock, so slow animations cannot desync them. Also drop the comments that only restated the code.
Drops the orthogonal-channel indirection. transitionAlpha composites with the real alpha, so it can only carry opacity, cannot express a fade to fully transparent, and is contested by android.transition, which captures and restores it around its own animations. Writing the property React Native writes removes all three limits and the API 29 floor with them, at the cost of a commit being able to overwrite a running animation. Reconciling that is the next commit's job; nothing routes here yet. Interruption now resumes straight from the property, and no teardown reset is needed since React Native already clears its own fields on view recycling.
A tag can be registered before its View exists, with the mount still in flight, and dropping the transition there snaps the property instead of animating it. The start timestamp is absolute, so waiting a frame costs nothing: the elapsed offset absorbs it. Also narrows the easing comment, which implied every curve is evaluated directly. PathInterpolator flattens cubic-bezier internally; only steps() and linear() stops are computed from their points.
The posted frame callback was unguarded, so a retry could start an obsolete transition after removeTransition ran or after a newer request took the same key. Each start claims a monotonic token, which the callback checks before running; removeTransition drops it. The token never repeats, so an entry cleared on a successful start cannot be matched by a stale retry. Also states what the return value means: the property is accepted for native playback, but playback can still fail on the UI thread if the View never mounts, and there is no path back to demote it - matching the Apple backend.
One retry assumed the mount always lands within a frame, which nothing guarantees: the tag is registered independently of its View. Starting late is harmless because the start timestamp is absolute, so a late start seeks rather than drifting, which means the retry needs no arbitrary timeout - once the transition would have ended there is nothing left to play.
ObjectAnimator writes nothing until its first frame, and transition-delay defers that frame. React Native has already committed the target by then, so the view showed the end state for the whole delay and jumped back when the animation finally started.
ValueAnimator.start() reads mSeekFraction when deciding whether to begin immediately, so seeking a late start afterwards raced the first frame. Also drops comments that restate the code, shortens those whose second sentence the first already implied, imports FloatProperty rather than naming it in full, and renames the binary search to firstSmallerOrEqual so it and its C++ counterpart turn up in the same grep.
7c49248 to
9334e78
Compare
tjzel
left a comment
There was a problem hiding this comment.
Looks good, but please de-slop the comments a bit
…ative writes (#10092) A platform transition animates the same `View` property React Native writes, so any commit touching that view overwrites the running animation. On Android Fabric re-applies the whole props blob, so it is enough for any prop on the view to change. This PR adds a pre-draw listener that re-applies styles from running animators to ensure that the animation takes effect and is not overridden by RN. Stacked on #10090.
…er JNI (#10060) Owns the per-property state CSS reversing needs: it detects a reversal against the in-flight transition's start value, reverse-shortens the duration through the easing curve, and hands the resolved timeline to the Kotlin animator from #10090. Elapsed time crosses the boundary instead of an absolute start. Reverse-shortening backdates the start and `transition-delay` pushes it forward, and computing it here keeps Kotlin off a second clock that would drift from the loop's timestamp. Easings cross as the same normalized point arrays the loop uses, so nothing reimplements the easing spec on the other side of the boundary. Nothing routes to this yet; it is inert until the next PR flips the gate. Stacked on #10090.
Plays an already resolved CSS transition timeline on a native
ViewwithObjectAnimator. Nothing constructs it yet; the C++ resolver and the JNI bridge follow separately.Animates the same
Viewproperty React Native writes rather than a side channel.transitionAlphacomposites with the real alpha, so it could only ever carryopacity, could not express a fade to fully transparent, and is contested byandroid.transition, which captures and restores it around its own animations. Writing React Native's own property removes all three limits and the API 29 floor with them. The cost is that a commit can overwrite a running animation, which the next PR reconciles.Easings are rebuilt rather than sampled, so
steps()keeps its discontinuities at any duration. An interruption resumes from the value on screen rather than the committed style value, which would snap the view back to where the cancelled transition began.