Screen is dismissed twice when swiping down on a sheet

[rdonnelly]

Hi there! I am admittedly pretty green with iOS development and the framework, but learning lots and having fun. I'm running into some interesting behavior when playing with mixing navigation types. Basically, if I push a screen onto the stack, then presentSheet another screen, then dismiss the sheet with a swipe down, I get a double dismiss animation:

Manually dismissing the sheet works just fine. I built a small repro app to see the behavior:

https://github.com/rdonnelly/FlowStacksDoubleDismiss

Definitely willing to consider other approaches if something seems fishy in the code! Thanks!

[johnpatrickmorgan]

Thanks for raising this @rdonnelly ! This seems to be an issue introduced in iOS 17 when using view models - I was able to reproduce the issue in the project's demo app. I'll need to dig a bit deeper to understand what's going on, so thank you for the clear reproduction.

[rdonnelly]

Awesome, thanks for the confirmation and for taking a look, @johnpatrickmorgan! And obviously no rush, definitely digging the library and approach! 👏

[tarikstafford]

I had a similar issue with a navigationView wrapping a tabView when the app moves to background it pops and/or pushes and then pops.

[wickwirew]

@johnpatrickmorgan any ideas what is causing this? I spent a few hours the other day trying to dig into it but came up empty. Tried a bunch of different things but wasnt able to get it to stop. Happy to continue to dig more! Just curious if you had any ideas or a hunch about whats going on. Btw huge thanks for the library!

[johnpatrickmorgan]

Thanks a lot @wickwirew for looking into this - I've also been stumped. One thing I tried was adding a Binding wrapper to log the get and set:

extension Binding {

  func withLogging<T>(_ transform: @escaping (Value) -> T) -> Binding {
    return Binding(
      get: {
        print("getting \(transform(wrappedValue))")
        return wrappedValue
      },
      set: { newValue in
        print("setting from \(transform(wrappedValue)) to \(transform(newValue))")
        wrappedValue = newValue
      }
    )
  }
}

And then initialising the Router with Router($viewModel.routes.withLogging({ $0.count })). Doing so showed that the Binding is somehow giving a stale value:

setting from 3 to 2
VMCoordinator: _viewModel changed.
getting 2
getting 2
getting 2
getting 3 // !!!
getting 2
getting 2

But I haven't been able to figure out why yet.

[wickwirew]

Huh strange, I actually did something similar and got different results. In the AppCoordinatorView created a wrapped binding to pass to the Router:

var binding: Binding<Routes<Screen>> {
    Binding {
        print("getting: \(coordinatorViewModel.routes.count)")
        return coordinatorViewModel.routes
    } set: { newValue in
        print("setting from \(coordinatorViewModel.routes.count) to \(newValue.count)")
        coordinatorViewModel.routes = newValue
    }
}

Which logs the correct values. Which I guess makes since its reading directly from the source of truth. Very weird the extension prints an old value though.

setting from 3 to 2
getting: 2
getting: 2
getting: 2
getting: 2
getting: 2
getting: 2
getting: 2

I was able to replicate what you saw with that Binding extension too. It looks like when the stale value is printed its not from SwiftUI reading the value, but actually in the isActiveBinding setter reading the allRoutes.wrappedValue. The stale value has to be related though. It only happens when the dismissal bug shows, and not for every sheet.

[lionel-alves]

I am having the same issue, it is really annoying. Any way to work around it?

[johnpatrickmorgan]

I've tracked down the problem to the addition of the FlowNavigator as an environment object:

https://github.com/johnpatrickmorgan/FlowStacks/blob/9629bd329146d4ba476bfdb50d293acbf8ba3f5d/Sources/FlowStacks/Router.swift#L35

Removing that line avoids the issue. But strangely, it seems to also be overcome by just observing the flow navigator in Node. To do so, I have to refactor Node into a struct rather than an enum, which is a bit long-winded but I'm in the process of doing so.

[johnpatrickmorgan]

I've refactored Node in such a way that I believe this issue is now fixed. It's available in version 0.4.0. Please let me know if you have any issues.