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

DismissEffect not working in combination with NavigationStack and await store.send(...).finish() in stack's root view #3527

Open
2 of 3 tasks
chwo opened this issue Dec 10, 2024 · 3 comments
Labels
bug Something isn't working due to a bug in the library.

Comments

@chwo
Copy link

chwo commented Dec 10, 2024

Description

In the provided sample project, the app's root view's task action, the RootFeature, appends a feature state (DetailFeature) to the navigation path on the app's NavigationStack. When pressing the DetailFeature's close button, it calls the DismissEffect. The detail view, however, doesn't get dismissed. I figured out this is because the NavigationStack's root view (HomeView) awaits the send action to finish in the task view modifier.

Checklist

  • I have determined whether this bug is also reproducible in a vanilla SwiftUI project.
  • If possible, I've reproduced the issue using the main branch of this package.
  • This issue hasn't been addressed in an existing GitHub issue or discussion.

Expected behavior

The view pushed onto the NavigationStack should pop when calling dismiss.

Actual behavior

The view is not getting removed from the stack.

Reproducing project

Sample project: dismiss.zip

The dismiss is not working when using .task { await store.send(.task).finish() } in HomeView (which is the NavigationStack root view):

received action:
  RootFeature.Action.navigation(
    .path(
      .element(
        id: #0,
        action: .detail(.closeButtonTapped)
      )
    )
  )
  (No state changes)

The dismiss only works when just using .task { store.send(.task) } in HomeView.

received action:
  RootFeature.Action.navigation(
    .path(
      .element(
        id: #0,
        action: .detail(.closeButtonTapped)
      )
    )
  )
  (No state changes)

received action:
  RootFeature.Action.navigation(
    .path(
      .popFrom(id: #0)
    )
  )
  RootFeature.State(
    _navigation: NavigationFeature.State(
      _root: .home(…),
      _path: [
-       #0: .detail(
-         DetailFeature.State(
-         )
-       )
      ]
    )
  )

received action:
  RootFeature.Action.navigation(
    .root(
      .home(.task)
    )
  )
  (No state changes)

The Composable Architecture version information

1.17.0

Destination operating system

iOS 18

Xcode version information

Xcode version 16.0 (16A242d)

Swift Compiler version information

swift-driver version: 1.115 Apple Swift version 6.0 (swiftlang-6.0.0.9.10 clang-1600.0.26.2)
Target: arm64-apple-macosx15.0
@chwo chwo added the bug Something isn't working due to a bug in the library. label Dec 10, 2024
@pyrtsa
Copy link
Contributor

pyrtsa commented Dec 16, 2024

We are also experiencing this issue and I really don't think using await store.send(.task).finish() in one view should cause DismissEffect to fail in another! It seems to me like a bug in TCA navigation that such actions at a distance can occur at all.

At least I think we need better documentation and test coverage on the semantics of cancellation with the navigation utilities. The current implementation seems to set up and remove cancellation handlers quite (overly?) deliberately1.

While studying the issue, I found at least one workaround which seems to fix the above sample project, but I don't fully understand why:

// RootFeature.swift
@_spi(Internals) import ComposableArchitecture

...

switch action {
case .task:
    @Dependency(\.stackElementID) var stackElementID
    return .send(.navigation(.path(.push(
        id: stackElementID(),
        state: .detail(DetailFeature.State())
    ))))
...
}

Footnotes

  1. For example, why is the same cancellable added and removed to each navigation path prefix like that? And why are they called prefixes when the implementation actually returns suffixes by using dropFirst(index)?

@OguzYuuksel
Copy link

OguzYuuksel commented Dec 19, 2024

When deep linked instead of using task in the RootView we see same result.
but if we add some delay to the task in RootView then it will work.

        .task {
            try? await Task.sleep(for: .seconds(1))
            await store.send(.task).finish()
        }

I couldn't find time to investigate deeper but I can clearly see idsAfter and idsBefore are same in _StackReducer.reduce function when closeButtonTapped is called in the demo.

@chwo
Copy link
Author

chwo commented Jan 8, 2025

@mbrandonw @stephencelis Did you already have time to look into this issue?

By further investigating the issue based on the sample project (see description) I suspect the following cause:

It seems that when initializing the DismissEffect a different id is used than when trying to cancel the effect, so the cancellation does not find the original cancellable and thus has no effect.

Code from StackReducer.swift#L509-L525:

        destinationEffects = self.destination
          .dependency(
            \.dismiss,
            DismissEffect { @MainActor in
              Task._cancel(
                id: NavigationDismissID(elementID: elementID), // <- here the id is 'NavigationDismissID(elementID:)'
                navigationID: elementNavigationIDPath
              )
            }
          )
          .dependency(\.navigationIDPath, elementNavigationIDPath)
          .reduce(
            into: &state[keyPath: self.toStackState][id: elementID]!,
            action: destinationAction
          )
          .map { [toStackAction] in toStackAction.embed(.element(id: elementID, action: $0)) }
          ._cancellable(navigationIDPath: elementNavigationIDPath) // <- here the id is '_PresentedID()' because of the default argument

So when clicking the close button and executing the await dismiss() the actual cancellation in Cancellation.swift#L312-L318 does not work:

    let cancelID = _CancelID(id: id, navigationIDPath: path)
    self.storage[cancelID]?.forEach { $0.cancel() }
    self.storage[cancelID] = nil

The cancelID is not found in the storage dictionary so no cancellation is happening.

(lldb) po storage[cancelID]
nil
(lldb) po cancelID
▿ _CancelID
  ▿ discriminator : ObjectIdentifier(0x0000000104651ea8)
    - _value : (Opaque Value)
  ▿ id : AnyHashable(ComposableArchitecture.(unknown context at $10461f7b4).NavigationDismissID(elementID: AnyHashableSendable(#0)))
    ▿ value : NavigationDismissID
      ▿ elementID : AnyHashableSendable(#0)
        ▿ value : #0
          - generation : 0
  ▿ navigationIDPath : NavigationIDPath
    ▿ path : 1 element
      ▿ 0 : NavigationID
        ▿ kind : Kind
          - keyPath : \State.path
        ▿ identifier : Optional<AnyHashable>
          ▿ some : AnyHashable(#0)
            ▿ value : #0
              - generation : 0
        - tag : nil
  - testIdentifier : nil
(lldb) po storage
▿ 2 elements
  ▿ 0 : 2 elements
    ▿ key : _CancelID
      ▿ discriminator : ObjectIdentifier(0x0000000104651620)
        - _value : (Opaque Value)
      ▿ id : AnyHashable(ComposableArchitecture._PresentedID())
        - value : ComposableArchitecture._PresentedID()
      ▿ navigationIDPath : NavigationIDPath
        - path : 0 elements
      - testIdentifier : nil
    ▿ value : 1 element
      ▿ 0 : <AnyCancellable: 0x600002128410>
  ▿ 1 : 2 elements
    ▿ key : _CancelID
      ▿ discriminator : ObjectIdentifier(0x0000000104651620)
        - _value : (Opaque Value)
      ▿ id : AnyHashable(ComposableArchitecture._PresentedID())
        - value : ComposableArchitecture._PresentedID()
      ▿ navigationIDPath : NavigationIDPath
        ▿ path : 1 element
          ▿ 0 : NavigationID
            ▿ kind : Kind
              - keyPath : \State.path
            ▿ identifier : Optional<AnyHashable>
              ▿ some : AnyHashable(#0)
                ▿ value : #0
                  - generation : 0
            - tag : nil
      - testIdentifier : nil
    ▿ value : 1 element
      ▿ 0 : <AnyCancellable: 0x600002128410>

So I think the fix for this issue would be to use the same cancel id when creating the DismissEffect and when executing the cancellation.

Additionally, I also noticed that the issue is not only happening when using a NavigationStack but also happens sometimes when using a sheet. This seems to be the same cause as described above because the sheet PresentationReducer also creates a DismissEffect with a different id than the cancel id.

Code from PresentationReducer.swift#L605-L617:

      destinationEffects = self.destination
        .dependency(
          \.dismiss,
          DismissEffect { @MainActor in
            Task._cancel(id: PresentationDismissID(), navigationID: destinationNavigationIDPath) // <- here the id is 'PresentationDismissID()'
          }
        )
        .dependency(\.navigationIDPath, destinationNavigationIDPath)
        .reduce(
          into: &state[keyPath: self.toPresentationState].wrappedValue!, action: destinationAction
        )
        .map { [toPresentationAction] in toPresentationAction.embed(.presented($0)) }
        ._cancellable(navigationIDPath: destinationNavigationIDPath) // <- here the id is '_PresentedID()' because of the default argument

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working due to a bug in the library.
Projects
None yet
Development

No branches or pull requests

3 participants