johnpatrickmorgan / TCACoordinators

Powerful navigation in the Composable Architecture via the coordinator pattern
MIT License
439 stars 42 forks source link

Fairly consistent crash when dismissing (or swapping) a TCACoordinator flow in iOS16 #36

Open dannyhertz opened 1 year ago

dannyhertz commented 1 year ago

Not sure exactly when it started (but it almost certainly after release of iOS 16) but we are seeing fairly consistent crashes when dismissing a sheet (or switching SwitchStore cases) containing a TCACoordinatorFlow (with more than a single screen). The stack trace is always the same and seems to involve some lower level UIKit NavigationController dismissal animation/callback. Not seeing it with my vanilla TCA NavigationView related flows so wondering if its related to some specific usage of NavigationViews in TCACoordinators/FlowStacks. Here is an example stack trace of the crash.

Screen Shot 2022-10-15 at 10 35 58 AM

Strangly enough we were able to get rid of the crash (for the SwitchStore case) by adding a transition in each CaseLet view, so it must be related to the default way the system is tearing down NavigationViews. Wasn't clear how I could apply a similar workaround for standard flows in sheet presentations though.

Just wanted to put this on your radar to see if you or any other TCACoordinators fans are experiencing something similar and have found any good workarounds.

Update: Another potential clue is that I notice when you dismiss a flow with several pushed views, it seems to try and pop to the root view of the flow. I noticed this because I always see the onAppear modifier/action called on the root view when the flow is dismissed. Feels like this could be related as perhaps the NavigationView is popping while also being removed from the screen entirely and there is some sort of collision. Any idea why that is happening?

Update #2: Seems like this bug that causes the root view onAppear to fire on flow dismissal does not occur if if embedInNavigationView: false and wrap the flow myself.

Thanks as always!

johnpatrickmorgan commented 1 year ago

Thanks @dannyhertz - I'll try and get a reproduction and figure out what's happening. In the meantime, regarding your first update, I suspect this behaviour is because the NavigationLink bindings become false for all NavigationLinks beyond the index the stack is dismissed back to - i.e. this line in FlowStacks tests with a > rather than a !=. It might actually be possible to change that to !=.

dannyhertz commented 1 year ago

Thanks for the response @johnpatrickmorgan! Yeah I was actually looking at that line as well in my debugging travels! Seems like a good clue. Any idea why this behavior only seems to happen when embedInNavigationView is true? Also are you suggesting when a flow is dismissed (or removed from screen through a switch, or popped) that all embedded NavigationLink bindings are sent to false by the system?

johnpatrickmorgan commented 1 year ago

Hi @dannyhertz, sorry for the delay in following up. I've been playing around and haven't yet reproduced the crash, but have seen the spurious .onAppear closures firing as you described, and changing the line I mentioned doesn't have any effect.

Any idea why this behavior only seems to happen when embedInNavigationView is true?

No, that's very puzzling. The only difference I can see is that the NavigationView in that case is outside the WithViewStore in TCARouter, rather than inside it.

Also are you suggesting when a flow is dismissed (or removed from screen through a switch, or popped) that all embedded NavigationLink bindings are sent to false by the system?

No, in fact, I think I was confused when I sent my last message as I was thinking about dismissing within a flow rather than dismissing the entire flow from outside it. There should be no effect on any of the dismissed flow's NavigationLink bindings in that case.

johnpatrickmorgan commented 1 year ago

@dannyhertz I looked further into onAppear being called unexpectedly, and was able to reproduce the problem in vanilla SwiftUI, so I think this is a SwiftUI bug:

Example code: Unexpected onAppear called when switching away from a NavigationView ```swift import SwiftUI /* Navigating through to LoggedInView prints the following: WelcomeView LogInFormView LoggedInView WelcomeView -> Why is this view's onAppear called when it's not on screen?? */ struct ContentView: View { @State var isLoggedIn = false var body: some View { if isLoggedIn { LoggedInView(logOut: { isLoggedIn = false }) } else { NavigationView { WelcomeView(logIn: { isLoggedIn = true }) } } } } struct WelcomeView: View { var logIn: () -> Void var body: some View { NavigationLink(destination: LogInFormView(logIn: logIn), label: { Text("Go to form") }) .onAppear { print("\(Self.self)") } } } struct LogInFormView: View { var logIn: () -> Void var body: some View { Button("Log in", action: logIn) .onAppear { print("\(Self.self)") } } } struct LoggedInView: View { var logOut: () -> Void var body: some View { Button("Log out", action: logOut) .onAppear { print("\(Self.self)") } } } ```

In this case, switching away from the NavigationView to the LoggedInView causes the NavigationView's root screen's onAppear to be called. I think this is why we see the same using TCACoordinators.

HannibalRoyal commented 6 months ago

I avoided this problem and used a delay of 250 ms like this: return .concatenate([ .send(.device(.goBackToRoot)), .run { send in await Task.kSleepFor(milliseconds: 250) }, .send(.binding(.set(.$selectTab, .settings))), .send(.settings(.shouldJumpToLocationSetting)) ])