Repository navigation
feat: batch CSS events and deliver them to JS - #10108
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 (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughChangesCSS event contracts, batched native-to-JavaScript delivery, and CSS event delivery
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant NativeCSS
participant ReanimatedModuleProxy
participant CSSEventsEmitter
participant CallInvoker
participant JSHandler
NativeCSS->>CSSEventsEmitter: emit CSSEvent
CSSEventsEmitter->>CSSEventsEmitter: queue event
CSSEventsEmitter->>CallInvoker: schedule drain
CallInvoker->>CSSEventsEmitter: process pending events
CSSEventsEmitter->>JSHandler: invoke event array
ReanimatedModuleProxy->>CSSEventsEmitter: setCSSEventHandler
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 |
2893337 to
529081e
Compare
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/events/CSSEventsEmitter.cpp`:
- Around line 55-58: Update CSSEventsEmitter::setEmitFunction to call
requestDrain() after storing emitFunction_, ensuring events queued before
handler registration are delivered without requiring a later event. Add a
regression test covering emission before registration and verifying the pending
batch is delivered after registration.
🪄 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: b629c519-9653-4faa-b5fe-e911a3d01ba5
📒 Files selected for processing (5)
packages/react-native-reanimated/Common/cpp/reanimated/CSS/events/CSSEvent.hpackages/react-native-reanimated/Common/cpp/reanimated/CSS/events/CSSEventsEmitter.cpppackages/react-native-reanimated/Common/cpp/reanimated/CSS/events/CSSEventsEmitter.hpackages/react-native-reanimated/Common/cpp/reanimated/NativeModules/ReanimatedModuleProxy.cpppackages/react-native-reanimated/Common/cpp/reanimated/NativeModules/ReanimatedModuleProxy.h
tshmieldev
left a comment
There was a problem hiding this comment.
minor nits, overall very good
529081e to
7594690
Compare
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/events/CSSEventsEmitter.cpp`:
- Around line 77-78: Update the drain scheduling state around
shouldScheduleDrain and drainRequested_ so an invokeAsync callback cleared after
a task error resets the request state and allows pending events to retry
immediately, including partial batches smaller than kDrainRetryBacklog. Connect
this reset to scheduler completion or error handling, and add a regression test
covering a dropped request followed by a partial final batch.
🪄 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: d66f1312-6b9a-43c1-ac1f-692ce2549059
📒 Files selected for processing (4)
packages/react-native-reanimated/Common/cpp/reanimated/CSS/events/CSSEvent.hpackages/react-native-reanimated/Common/cpp/reanimated/CSS/events/CSSEventsEmitter.cpppackages/react-native-reanimated/Common/cpp/reanimated/CSS/events/CSSEventsEmitter.hpackages/react-native-reanimated/Common/cpp/reanimated/NativeModules/ReanimatedModuleProxy.cpp
d3cff7b to
9677185
Compare
A drain request can be dropped before it runs, and the latch had no way to learn that, so a final batch under the retry backlog stayed buffered for good. Delivery stays batched: the first drain takes everything.
9677185 to
7ab0e33
Compare
The transport for CSS events: a buffer that batches whatever is emitted and hands it to JS in one call, plus the JSI entrypoint that installs the receiving function.
Nothing emits yet, so this is inert on its own. #10070 adds the animation side.
Emitting buffers the event and asks the JS thread to drain, so however many events land before the drain runs arrive in a single JS call. That keeps the JS call off the UI thread and out of the registry lock. A drain is requested per event rather than latched per batch: a request can be dropped before it runs, and there is no signal for that, so anything remembering that one is outstanding would strand the buffer for good.
elapsedTimeis converted to seconds once, matching the contract already shipped on web.The event type ordinals are the bit indices of the subscription mask, so the JS constants have to stay in sync with them. The four transition values are reserved and unemitted until CSS transitions land.