Effects not cancelled when Reducer with Identifiable state is dismissed

[nesium]

Description

Hello!

I have just encountered an issue where the effects of a presented reducer/route were not cancelled when the associated UIViewController was dismissed, but only when Reducer.State conforms to Identifiable.

Preconditions:

• The UIViewController is presented via the present(item:id:content:) helper from swift-navigation.
• The state of the associated reducer is part of a CaseReducer route stored in a @Presents-wrapped property and combined via the ifLet helper.

Apparently there are 3 ways how effects can get cancelled when a route is changed/ViewController is dismissed:

1. The route is changed or reset programmatically - this is picked up by the _PresentationReducer which cancels the effect of the former route
2. The Reducer runs the DismissEffect - the DismissEffect is provided by the _PresentationReducer which again cancels the effects and afterwards sends the PresentationAction.dismiss action
3. The ViewController is removed from the view hierarchy. This could happen e.g. when the user pulls down a presented ViewController, taps "Back" in a Navigation bar or when the VC is dismissed by calling .dismiss(animated:) - this is picked up by the UIViewController extensions in swift-navigation by swizzling viewDidDisappear which then sets the value of the UIBinding to nil which then subscripts into the Store which then would send a PresentationAction.dismiss action which would trigger the _PresentationReducer again to cancel the effects.

The problem is in the 3. path here…

      if newValue == nil,
        let childState = self.state[keyPath: state],
        id == _identifiableID(childState),
        !self._isInvalidated()
      {
        self.send(action(.dismiss))

It seems the way id is derived here, that it doesn't match the formerly presented state and consequently doesn't send .dismiss and thus the effects won't get cancelled.

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

Manually dismissing a ViewController should cancel the effects of its associated Reducer/Store, even if the Reducer.State conforms to Identifiable.

Actual behavior

The effects are not cancelled.

Reproducing project

The attached project contains a "RootFeature" with two child features "Feature1" and "Feature2". Feature1.State conforms to Identifiable so you can reproduce the bug there and also see that "Feature2" - whose state does not conform to Identifiable - works as expected.

Both features start a task which waits for its cancellation:

return .run { [id = state.id] _ in
  do {
    try await withTaskCancellation(id: CancelID.effect) {
      print("Feature 1.\(id) started")
      try await Task.never()
    }
  } catch {}

  print("Feature 1 ('\(id)') cancelled")
}

So "Feature 1/2 … cancelled" should be printed to the Console when the respective ViewController was dismissed.

To actually reproduce the bug…

1. Tap "Present Feature 1.a"
2. Tap "Dismiss via self.dismiss(animated:)" in the presented VC or pull down the VC manually

TCAEffectsCancelling.zip

The Composable Architecture version information

1.18.0

Destination operating system

= iOS 14

Xcode version information

16.2 (16C5032a)

Swift Compiler version information

Apple Swift version 6.0.3 (swiftlang-6.0.3.1.10 clang-1600.0.30.1)
Target: arm64-apple-macosx15.0

[nesium]

I believe that I've at least understood the underlying problem now:

The method scope(state:action:fileID:filePath:line:column:) on the extension on UIBindable in Store+Observation.swift creates a new UIBinding with a KeyPath (via a _UIBindingWeakRoot) that subscripts into the store via subscript(id:state:action:isInViewBody:fileIDfilePath:line:column:) on the Store extension (also in Store+Observation.swift).

At this point the arguments to the KeyPath are baked in (curried?) and that includes the current ID, which at this point is nil, since the initial value of the route is nil.

// swift-composable-architecture::Store+Observation.swift:375

self[
  id: wrappedValue.currentState[keyPath: state].flatMap(_identifiableID), // <--
  state: state,
  action: action,
  isInViewBody: _isInPerceptionTracking,
  fileID: _HashableStaticString(rawValue: fileID),
  filePath: _HashableStaticString(rawValue: filePath),
  line: line,
  column: column
]

The destination(item:id:content:present:child:transaction:dismiss:) helper on UIViewController from the swift-navigation package observes the value of the created UIBinding. Leaving out a few steps … after the route has changed from None to Some and the UIViewController was presented and ultimately (manually) dismissed the helper writes nil back into the binding…

// swift-navigation::Presentation.swift:352
let onDismiss = { [presentationID = id(unwrappedItem.wrappedValue)] in
  if let wrappedValue = item.wrappedValue,
    presentationID == id(wrappedValue)
  {
    item.wrappedValue = nil
  }
}

This uses the KeyPath from above with the baked-in ID which is compared to the ID of the current state (which is still non-nil):

// swift-composable-architecture::Store+Observation.swift:430
if newValue == nil,
let childState = self.state[keyPath: state],
id == _identifiableID(childState),
!self._isInvalidated()

and that of course will fail since, as mentioned above, the baked-in ID was taken from the None state and not the Some state, because it is not updated when the underlying state changes.

This has also the undesired side-effect that the route is not reset and trying to present the same route again will fail.

[nesium]

Turns out this is actually a duplicate of #3451. I'll leave it open to count as an upvote. Feel free to close it though in favor of the earlier report.

[stephencelis]

@nesium Thanks for the report and for finding the duplicate. I'm going to close this for now and keep the other open, as this will be referenced in it for further review. We'll try to dig into this one soon!