fix(iOS, Stack): Fix for reattaching Screens when swiping back quickly#3584
fix(iOS, Stack): Fix for reattaching Screens when swiping back quickly#3584
Conversation
fcec68e to
e0ce715
Compare
kkafar
left a comment
There was a problem hiding this comment.
Looks good overall, just as we talked. I have one question though, leaving it below 👇🏻
There was a problem hiding this comment.
Since this change impacts rather core & crucial functionality of stack, I'd like it to be put behind a feature flag or delay its rollout, especially that we plan to release a next version soon & it might potentially end up in SDK 55. By allowing to control this change through JS we'll allow to patch it much more easily.
I'd suggest adding a "experiment" feature flag, e.g. iosPreventReattachmentOfDismissedScreens and consume it directly on screenstack component.
We can make this flag a noop in two or three minor versions, but next minor should be possibly stable.
What do you think?
|
kkafar
left a comment
There was a problem hiding this comment.
We could consider clearing the state in didMoveToParentViewController in case the controller is reattached. Beside that, looks good.
|
…fault (expo#43001) # Why The fix software-mansion/react-native-screens#3584 was merged to react-native-screens, but can only be activated via feature flag. See https://exponent-internal.slack.com/archives/C08RP2QSZPS/p1770035259877429 for more details # How <!-- How did you build this feature or fix this bug and why? --> # Test Plan <!-- Please describe how you tested this change and how a reviewer could reproduce your test, especially if this PR does not include automated tests! If possible, please also provide terminal output and/or screenshots demonstrating your test/reproduction. --> # Checklist <!-- Please check the appropriate items below if they apply to your diff. --> - [ ] I added a `changelog.md` entry and rebuilt the package sources according to [this short guide](https://github.com/expo/expo/blob/main/CONTRIBUTING.md#-before-submitting) - [ ] This diff will work correctly for `npx expo prebuild` & EAS Build (eg: updated a module plugin). - [ ] Conforms with the [Documentation Writing Style Guide](https://github.com/expo/expo/blob/main/guides/Expo%20Documentation%20Writing%20Style%20Guide.md) --------- Co-authored-by: Hassan Khan <hassan@expo.dev>
…fault (#43001) # Why The fix software-mansion/react-native-screens#3584 was merged to react-native-screens, but can only be activated via feature flag. See https://exponent-internal.slack.com/archives/C08RP2QSZPS/p1770035259877429 for more details # How <!-- How did you build this feature or fix this bug and why? --> # Test Plan <!-- Please describe how you tested this change and how a reviewer could reproduce your test, especially if this PR does not include automated tests! If possible, please also provide terminal output and/or screenshots demonstrating your test/reproduction. --> # Checklist <!-- Please check the appropriate items below if they apply to your diff. --> - [ ] I added a `changelog.md` entry and rebuilt the package sources according to [this short guide](https://github.com/expo/expo/blob/main/CONTRIBUTING.md#-before-submitting) - [ ] This diff will work correctly for `npx expo prebuild` & EAS Build (eg: updated a module plugin). - [ ] Conforms with the [Documentation Writing Style Guide](https://github.com/expo/expo/blob/main/guides/Expo%20Documentation%20Writing%20Style%20Guide.md) --------- Co-authored-by: Hassan Khan <hassan@expo.dev>
Description
This PR introduces logic to collect only active (non-dismissed) screen controllers coming from JS state.
When a screen gets dismissed on the native side, its state is sent asynchronously to the JS. In scenarios where multiple screens are dismissed quickly, this can result in stale updates being sent from JS to native. These outdated updates may reference screens that are no longer part of the native view hierarchy.
Since view recycling is disabled, we do not expect the
RNSScreenViewthat was dismissed to be reattached; a new instance should be created when we push an update from JS. Then the new controller instance will be initialized, so these associated with detached screens won't be reused again. Therefore, we can safely filter them out from JS state on the native side. On the JS side, the state will stabilize at some point and align with native, once we receive all asynchronous events.To address this, we now filter out any dismissed controllers from the list received from JS.
Note
I'm leaving the previous behavior as the default, and I'm adding
iosPreventReattachmentOfDismissedScreensflag that can enable this fix. We're right before a very important release, and this PR is touching the very basic logic; therefore, we don't want to have this fix enabled by default for now. You can opt-in by settingfeatureFlags.experiment.iosPreventReattachmentOfDismissedScreens = true. Any feedback would be valuable here.Closes: #2559
Changes
Before & after - visual documentation
before.mov
after.mov
Test plan
Added Test 2559.
Checklist