fix: pulse with spread operator broken on new arch #296
Conversation
✅ Heimdall Review Status
✅
|
| Code Owner | Status | Calculation | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| ui-systems-eng-team |
✅
1/1
|
Denominator calculation
|
| // Create instant transition to reset pulse after delay | ||
| const instantTransition: Transition = { type: 'timing', duration: 0 }; | ||
| const resetWithDelay = { ...instantTransition, delay: pulseRepeatDelay }; | ||
|
|
There was a problem hiding this comment.
This spread operator was causing the crash, getting rid of ... solves the issue. We have discussed this before, not sure why this didn't get fixed as well, it caused bugs in other spots previously and it comes down to how the spread operator is compiled and that function not being available in the UI thread.
| const idlePulseShared = useSharedValue(idlePulse ?? false); | ||
| useEffect(() => { | ||
| idlePulseShared.value = idlePulse ?? false; | ||
| }, [idlePulse, idlePulseShared]); |
There was a problem hiding this comment.
State changes of idlePulse wasn't working in new react-native arch. Didn't find other components in charts that have this issue.
| // Convert idlePulse prop to SharedValue so useAnimatedReaction can detect changes. | ||
| // In the new React Native architecture, regular JS props are captured by value in worklets | ||
| // and won't update when the prop changes. | ||
| const idlePulseShared = useSharedValue(idlePulse ?? false); |
There was a problem hiding this comment.
nit: do you think it would be cleaner if we default assign idlePulse = false in the component props (line 50), so you don't have to do idlePulse ?? false multiple times.
There was a problem hiding this comment.
Good callout. Normally we seem to shy away from setting to false, but not sure 100% why we do it. I know setting default to false in JSDocs is not great but internally not a big deal.
Next time I touch this I'll see if I can do !!idlePulse or even just allow undefined values.
3 participants
What changed? Why?
This PR fixes a couple of bugs with scrubber beacon on mobile.
Root cause (required for bugfixes)
Spread operator doesn't work in react-native-reanimated's worklets, and was causing issues on the new architecture specifically. It doesn't seem to reliably cause issues but we have had issues elsewhere with charts and even in Collapsible.
Worklets are functions that can run on a different JS thread, most often the UI thread. You can learn more about them here.
UI changes
Testing
This was tested with our mobile-app and a test app that is on the new react-native arch.
How has it been tested?
Testing instructions
To test with the new architecture, you can clone this test repo and then build mobile-visualization and update the files in node_modules in the test repo to have the updated package. This should allow you to test before and after!
Illustrations/Icons Checklist
Required if this PR changes files under
packages/illustrations/**orpackages/icons/**Change management
type=routine
risk=low
impact=sev5
automerge=false