Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(iOS): activityState regression check false-positive #2404

Merged
merged 2 commits into from
Oct 15, 2024

Conversation

kkafar
Copy link
Member

@kkafar kkafar commented Oct 14, 2024

Description

We had an issue introduced in #2389 that added the preload. We temporarily
have an assumption in native stack, that the activityState can only progress when
Screen component is rendered (directly) inside native stack navigator. This invariant
was enforced with appropriate checks on native side. It turns out that we were wrong in our
way of checking whether the Screen belongs to native stack or not - we have forgotten about
RNSContainerNavigationController class that is used when rendering ScreenContainer with hasTwoStates: true
and which inherits from RNSNavigationController.

Changes

Added a isNativeStackViewController method to RNSNavigationController and overrode it in subclass.

Added RNSScreenView#isNativeStackScreen method, which checks whether our direct react superview is a RNSScreenStackView.

I've found checking for _controller.navigationController not reliable, as navigationController can be found arbitrarily high in the view hierarchy.

Test code and steps to reproduce

Reported by Expo. If you want to reproduce this: create a JS stack navigator with bottom tabs inside (with hasTwoStates: true),
and then a regular JS stack inside & try to navigate to any screen.

We have not caught that earlier, because we're not testing regular JS stack at all.

Checklist

  • Included code example that can be used to test this change
  • Ensured that CI passes

Copy link
Contributor

@maciekstosio maciekstosio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🎉

@kkafar kkafar merged commit 778c8ee into main Oct 15, 2024
5 checks passed
@kkafar kkafar deleted the @kkafar/expo-navigation-container-crash branch October 15, 2024 09:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants