Repository navigation
chore: report the lifecycle milestones a run crosses - #10074
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:
📝 WalkthroughSummary by CodeRabbit
WalkthroughAnimation progress now uses persistent ChangesAnimation lifecycle tracking
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant AnimationProgressProvider
participant RunLifecycle
participant MilestoneReporter
AnimationProgressProvider->>RunLifecycle: reachPhase(phase, iteration)
RunLifecycle->>MilestoneReporter: report ordered milestone
AnimationProgressProvider->>RunLifecycle: abort()
RunLifecycle->>MilestoneReporter: report Aborted milestone
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 |
22939c1 to
256761e
Compare
MatiPl01
changed the title
256761e to
378a210
Compare
378a210 to
01932aa
Compare
01932aa to
c09f96a
Compare
MatiPl01
changed the title
104f765 to
421669f
Compare
421669f to
0fcd6d9
Compare
0fcd6d9 to
abbdb0a
Compare
abbdb0a to
0796b3d
Compare
![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
🤖 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/Common/cpp/reanimated/CSS/progress/RunLifecycle.cpp`:
- Around line 12-28: Make abort terminal by adding aborted_ guards at the start
of RunLifecycle::reachPosition and enterStagesUpTo, returning immediately once
the lifecycle has been aborted. Ensure these guards also stop progression when
abort() is invoked reentrantly from a Created or Started reporter callback,
preventing Started, Repeated, or Ended milestones after Aborted. Add coverage
for update-after-abort and reporter reentrancy scenarios.
🪄 Autofix
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: a5513339-eda8-4f25-ba23-63f5f25ea7f1
📒 Files selected for processing (4)
packages/react-native-reanimated/Common/cpp/reanimated/CSS/progress/AnimationProgressProvider.cpppackages/react-native-reanimated/Common/cpp/reanimated/CSS/progress/AnimationProgressProvider.hpackages/react-native-reanimated/Common/cpp/reanimated/CSS/progress/RunLifecycle.cpppackages/react-native-reanimated/Common/cpp/reanimated/CSS/progress/RunLifecycle.h
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/progress/RunLifecycle.cpp (1)
14-25: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPreserve milestone order across completed-run replays.
After
Ended, this code can rewindreported_toCreated. A laterStartedposition then reportsStartedagain. If the target remainsEndedwhilerepeatincreases, the rewind is skipped andRepeatedis reported afterEnded;enterStagesUpTo()cannot emit a newEndedafterward.Define the replay boundary explicitly. Keep historical milestones separate from the current stage, or rewind to a stage that preserves the already-reported
Startedmilestone. Add tests for bothEnded→Created→StartedandEndedwith a larger repeat.Based on the ordered-milestone contract in
RunLifecycle.hand the persistent lifecycle behavior inAnimationProgressProvider.cpp.🤖 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/Common/cpp/reanimated/CSS/progress/RunLifecycle.cpp` around lines 14 - 25, Update the replay handling around reported_, enterStagesUpTo(), and repeat_ so milestone ordering remains monotonic after an Ended run. Define an explicit replay boundary that preserves the historical Started milestone, ensuring both Ended → Created → Started and Ended with an increased repeat can emit Repeated without preventing a subsequent Ended milestone. Add coverage for both scenarios, following the ordered-milestone contract in RunLifecycle.h and persistent behavior in AnimationProgressProvider.cpp.
🤖 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/Common/cpp/reanimated/CSS/progress/RunLifecycle.cpp`:
- Around line 14-25: Update the replay handling around reported_,
enterStagesUpTo(), and repeat_ so milestone ordering remains monotonic after an
Ended run. Define an explicit replay boundary that preserves the historical
Started milestone, ensuring both Ended → Created → Started and Ended with an
increased repeat can emit Repeated without preventing a subsequent Ended
milestone. Add coverage for both scenarios, following the ordered-milestone
contract in RunLifecycle.h and persistent behavior in
AnimationProgressProvider.cpp.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7a954f1b-d28e-4e00-b536-0adb10c8dd52
📒 Files selected for processing (2)
packages/react-native-reanimated/Common/cpp/reanimated/CSS/progress/AnimationProgressProvider.cpppackages/react-native-reanimated/Common/cpp/reanimated/CSS/progress/RunLifecycle.cpp
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/progress/RunLifecycle.cpp (1)
26-34: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRe-check
aborted_after reporter callbacks.The guard in
enterStagesUpTostops only that loop.If the
Startedreporter callsabort()whilerepeat > repeat_,reachPositionresumes and reportsRunMilestone::RepeatedafterRunMilestone::Aborted.If the
Createdreporter callsabort()duringRunLifecycle::abort(), the inner call reportsRunMilestone::Aborted. The outer call then reportsRunMilestone::Abortedagain.Return immediately when
aborted_becomes true after eachenterStagesUpTocall. Add tests for both event sequences.Proposed fix
enterStagesUpTo(std::min(target, rank(RunStage::Started))); + if (aborted_) { + return; + } // ... enterStagesUpTo(rank(RunStage::Created)); + if (aborted_) { + return; + } aborted_ = true;Based on learnings:
RunLifecycle::abort()must be terminal, and no milestone may occur afterAborted, including during re-entrant reporter callbacks.Also applies to: 42-46
🤖 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/Common/cpp/reanimated/CSS/progress/RunLifecycle.cpp` around lines 26 - 34, Update RunLifecycle::reachPosition and RunLifecycle::abort to re-check aborted_ immediately after each enterStagesUpTo call and return when it becomes true, preventing any later milestone or duplicate Aborted report during re-entrant reporter callbacks. Add tests covering Started-triggered abort during repeat reporting and Created-triggered abort during abort().Source: Learnings
🤖 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/Common/cpp/reanimated/CSS/progress/RunLifecycle.cpp`:
- Around line 26-34: Update RunLifecycle::reachPosition and RunLifecycle::abort
to re-check aborted_ immediately after each enterStagesUpTo call and return when
it becomes true, preventing any later milestone or duplicate Aborted report
during re-entrant reporter callbacks. Add tests covering Started-triggered abort
during repeat reporting and Created-triggered abort during abort().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 26f123fe-582c-46ad-bb97-b3a5f33a3822
📒 Files selected for processing (1)
packages/react-native-reanimated/Common/cpp/reanimated/CSS/progress/RunLifecycle.cpp
reachPosition reports a repeat between its two ladder walks, so a reporter that aborted from Started still got a Repeated after the Aborted.
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/progress/RunLifecycle.cpp (1)
37-46: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPrevent duplicate
Abortedreports during re-entrant aborts.If the
Createdreporter callsabort()from Line 39, the nested call setsaborted_at Line 45 and reportsAbortedat Line 46. The outer call then continues and reportsAborteda second time. This producesCreated, Aborted, Abortedfor one cancellation. The reporter is invoked synchronously, so this path is reachable. (raw.githubusercontent.com)Return when
aborted_becomes true afterenterStagesUpTo(). Add a regression test for aCreatedreporter that callsabort().Proposed fix
enterStagesUpTo(rank(RunStage::Created)); + if (aborted_) { + return; + } aborted_ = true;Based on learnings,
abort()must remain terminal under reporter re-entrancy, and no milestone may occur afterAborted.🤖 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/Common/cpp/reanimated/CSS/progress/RunLifecycle.cpp` around lines 37 - 46, Update RunLifecycle::abort() to re-check aborted_ immediately after enterStagesUpTo(rank(RunStage::Created)) and return if a re-entrant Created reporter already aborted the run, preventing duplicate Aborted reports or later milestones. Add a regression test where the Created reporter calls abort() and verify the sequence is Created, Aborted exactly once and remains terminal.Sources: Learnings, MCP tools
🤖 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/Common/cpp/reanimated/CSS/progress/RunLifecycle.cpp`:
- Around line 37-46: Update RunLifecycle::abort() to re-check aborted_
immediately after enterStagesUpTo(rank(RunStage::Created)) and return if a
re-entrant Created reporter already aborted the run, preventing duplicate
Aborted reports or later milestones. Add a regression test where the Created
reporter calls abort() and verify the sequence is Created, Aborted exactly once
and remains terminal.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6b4c443f-d393-425f-8182-e7793295f2cc
📒 Files selected for processing (1)
packages/react-native-reanimated/Common/cpp/reanimated/CSS/progress/RunLifecycle.cpp
A high-water ladder can only move a run forwards, so the three backwards rows of the css-animations-2 and css-transitions-2 event tables reported nothing: active to before, after to active, and after to before. Chrome fires an end, a start, and a start plus an end for those. Milestones now carry which of the run's times they mean, since the same milestone takes the interval start or the interval end depending on the phase change that produced it.
Re-timing a running animation recomputes its iteration, so a high-water mark swallowed every later boundary. The spec compares against the previous frame and puts the boundary at the end of the iteration the run drops back into.
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/Common/cpp/reanimated/CSS/progress/RunLifecycle.cpp`:
- Around line 62-68: Update RunLifecycle::abort() to recheck aborted_
immediately after reachPhase(RunPhase::Before) and return before reporting if a
re-entrant abort already completed it. Preserve the terminal behavior so no
milestone, including a duplicate RunMilestone::Aborted, occurs after the first
Aborted report, and add coverage for aborting from a Created reporter.
🪄 Autofix
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: 7c70e4dd-cc16-48fa-9fb6-1e35eb89acd6
📒 Files selected for processing (4)
packages/react-native-reanimated/Common/cpp/reanimated/CSS/progress/AnimationProgressProvider.cpppackages/react-native-reanimated/Common/cpp/reanimated/CSS/progress/AnimationProgressProvider.hpackages/react-native-reanimated/Common/cpp/reanimated/CSS/progress/RunLifecycle.cpppackages/react-native-reanimated/Common/cpp/reanimated/CSS/progress/RunLifecycle.h
Aborting from the Created reporter finishes the run inside the nested call, and the outer one then reported Aborted a second time.
2 participants
RunLifecycletracks which phase a run is in and reports the milestones each phase change crosses. It knows nothing about animations or transitions: the owner mapsCreated,Started,Ended,RepeatedandAbortedto its own events.The phase changes come straight from the tables in css-animations-2 and css-transitions-2, which agree on every row. That includes the rows that move a run backwards, which a style update reaches by re-timing a run that is already going or already finished.
A milestone can be reached by more than one phase change, and the time it carries differs: a start is the interval start entering from a delay and the interval end re-entering after finishing. Hence
MilestoneTime, used by the owners in #10070 and #10120.Pausing also moves out of the progress state, fixing an animation created with
playState: Pausedreportinganimationstartwhile still paused and never once it actually started.Nothing registers a reporter yet. First step of the CSS animation callbacks series.