ViewController's observe is triggered when state is set to nil

[yimajo]

Description

When the parent Reducer declares the ViewController's State with @Presents, the observe in the ViewController is incorrectly triggered once when the parent Reducer sets the ViewController's State to nil.

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

I expected that when the parent Reducer sets the child's ViewController's State to nil, the observe in the ViewController would not be triggered at all.

The observe should not execute any logic when the state is set to nil.

Actual behavior

When the parent Reducer declares the ViewController's State with @Presents, the observe in the ViewController is incorrectly triggered once when the parent Reducer sets the ViewController's State to nil.

Steps to reproduce

1. Present the UIViewController modally (tap the "Present ChildView" button)
2. Parent sets the ViewController's state to nil (tap the "Cancel" button)

Minimal code to reproduce the issue:

import SwiftUI
import ComposableArchitecture

@ViewAction(for: SampleFeature.self)
struct SampleFeatureView: View {
    @Environment(\.locale) var locale

    @Bindable var store: StoreOf<SampleFeature>

    var body: some View {
        VStack {
            Button("Present ChildView") {
                send(.onClickPresentModal)
            }
        }
        .sheet(
            item: $store.scope(
                state: \.child,
                action: \.child
            )
        ) { store in
            NavigationStack {
                ChildFeatureView(store: store)
                    .toolbar {
                        ToolbarItem(placement: .cancellationAction) {
                            Button("Cancel") {
                                send(.onClickCancel)
                            }
                        }
                    }
            }
        }
    }
}

@Reducer
struct SampleFeature {

    @ObservableState
    struct State {
        @Presents var child: ChildFeature.State?

        init(child: ChildFeature.State? = nil) {
            self.child = child
        }
    }

    enum Action: ViewAction {
        enum ViewAction {
            case onClickPresentModal
            case onClickCancel
        }

        case view(ViewAction)

        case child(PresentationAction<ChildFeature.Action>)
    }

    var body: some Reducer<State, Action> {
        Reduce<State, Action> { state, action in
            switch action {
            case .view(let action):
                switch action {
                case .onClickPresentModal:
                    state.child = .init()
                    return .none

                case .onClickCancel:
                    // If it is set to nil from parent,
                    // observe in ViewController is working after being set to nil.
                    state.child = nil
                    return .none
                }

            default:
                return .none
            }
        }
        .ifLet(\.$child, action: \.child) {
            ChildFeature()
        }
    }
}

struct ChildFeatureView: UIViewControllerRepresentable {
    typealias UIViewControllerType = UIViewController

    private let store: StoreOf<ChildFeature>

    init(store: StoreOf<ChildFeature>) {
        self.store = store
    }

    func makeUIViewController(context: Context) -> UIViewControllerType {
        ViewController(store: store)
    }

    func updateUIViewController(_ uiViewController: UIViewControllerType, context: Context) {
    }

    @ViewAction(for: ChildFeature.self)
    public class ViewController: UIViewController {
        @UIBindable public var store: StoreOf<ChildFeature>

        public init(store: StoreOf<ChildFeature>) {
            self.store = store
            super.init(nibName: nil, bundle: nil)
        }

        required init?(coder: NSCoder) {
            fatalError("init(coder:) has not been implemented")
        }

        public override func viewDidLoad() {
            super.viewDidLoad()

            observe { [weak self] in
                guard let self else {
                    return
                }
                // This line is also executed when the parent is set to nil
                print("observe", self.store.count, #line)
            }
        }
    }
}

@Reducer
public struct ChildFeature {

    @ObservableState
    public struct State: Sendable {
        var count: Int

        init(count: Int = 0) {
            self.count = count
        }
    }

    public enum Action: ViewAction {
        public enum ViewAction {
        }

        case view(ViewAction)
    }

    public var body: some Reducer<State, Action> {
        Reduce<State, Action> { state, action in
            switch action {
            case .view:
                return .none
            }
        }
    }
}

The Composable Architecture version information

1.14.0

Destination operating system

iOS 17.5

Xcode version information

Xcode 15.4

Swift Compiler version information

swift-driver version: 1.90.11.1 Apple Swift version 5.10 (swiftlang-5.10.0.13 clang-1500.3.9.4)
Target: arm64-apple-macosx15.0

[mbrandonw]

Hi @yimajo, are you sure this is TCA-specific behavior? Have you tried reproducing in vanilla SwiftUI with @Observable to see if the same thing happens?

[yimajo]

Hi @mbrandonw. Thank you for your response.

Since you responded, I tried it with vanilla SwiftUI.

The same thing did not happen. When @Observable is set to nil, the change in its state appears to be the result of it being monitored correctly by SwiftUI.

import SwiftUI

@MainActor
struct SampleFeature2View: View {
    @State var counter: Counter?

    var body: some View {
        VStack {
            Button("Present ChildView") {
                counter = Counter(count: 1)
            }
        }
        .sheet(
            isPresented: Binding<Bool>(
                get: { counter != nil },
                set: {
                    if !$0 {
                        counter = nil
                    }
                }
            )
        ) {
            NavigationStack {
                ChildFeature2View(counter: $counter)
                    .toolbar {
                        ToolbarItem(placement: .cancellationAction) {
                            Button("Cancel") {
                                counter = nil
                            }
                        }
                    }
            }
        }
    }
}

struct ChildFeature2View: View {
    @Binding var counter: Counter?

    init(counter: Binding<Counter?>) {
        self._counter = counter
    }

    var body: some View {
        VStack {
            Text("Counter: \(String(describing: counter?.count))")

            if counter == nil {
                Text("counter is nil")
                    .onAppear {
                        // When printed, it is working properly.
                        print("observe", self.counter?.count, #line)
                    }
            }
        }
    }
}

@Observable class Counter {
    var count: Int
    init(count: Int) {
        self.count = count
    }
}

[mbrandonw]

Hi @yimajo, thanks for that, but unfortunately I'm not sure if that is a faithful recreation in vanilla. You should have a parent @Observable model that holds onto a child @Observable model, and does presentation from that. And further I wouldn't use @State because it tends to behavior much differently from regular let properties holding onto observable models. Also, the child feature should probably be UIKit too, and you can use withObservationTracking to see if it gets pinged when dismissing the child feature.

Also, aside from the repro, have you been able to determine if there are any negative effects of this additional observation? We need to prioritize investigating this issue amongst all of our other work, and if there are no negative side effects we may just consider it "expected behavior" for the time being and close out this issue.

[yimajo]

Hi @mbrandonw. Thank you for your response.

I’ll proceed under the assumption that you’d like me to provide an example.

The following is an example of a camera app where state is set to nil, and observe is triggered, but observe itself cannot handle the situation correctly. It monitors the switching of captureSession when switching between multiple camera lenses or video cameras, initializes the preview view when the captureSession changes, and calls the Reducer’s Action based on that change.

// Here's my example.
override func viewDidLoad() {
    super.viewDidLoad()
    store.send(.view(.onAppear))

    observe { [weak self] in
      guard let self else { return }
      guard let session = store.captureSession else { return }

      // Clear PreviewLayer
      ....

      // Create PreviewLayer
      previewView = createPreviewLayer(size: view.bounds.size, session: session)
      view.addSubview(previewView)
      store.send(.view(.didCreatePreviewLayer))
    }
  }

when setting the Presents state to nil causes the camera screen to close, observe is triggered, causing the view to reinitialize and the Action to be re-issued.

[mbrandonw]

Hi @yimajo, thanks for the real world use case. That definitely helps us understand better how you are using these tools.

It looks like we need to improve the documentation here a bit because currently your code snippet is doing some dangerous things inside the observe closure. That closure should be thought of as being analogous to the body property of a SwiftUI view. You should only perform the most basic of UI updates in that closure. You should not perform side effects, just as it is not valid to mutate state or perform side effects in a body:

struct MyView: View {
  @State var count = 0
  var body: some View {
    let _ = count += 1         // ❌ Not correct to do!
    URLSession.shared.data(…)  // ❌ Also not correct to do!
  }
}

In your case, sending an action to the store during observe is not a good idea. And really, it seems like it shouldn't be necessary to send the action. After all, your feature knows when captureSession flipped from nil to non-nil, or vice-versa, and so can't it perform the logic of didCreatePreviewLayer at that moment instead of having the view tell it to do that logic?

And further, even the addSubview logic is a little tricky. You should probably only addSubview if the view isn't already created, and further you should probably remove the previewView when the state becomes nil:

observe { [weak self] in
  guard let self else { return }

  if let session = store.captureSession, previewView == nil {
    previewView = createPreviewLayer(size: view.bounds.size, session: session)
    view.addSubview(previewView)
  } else if let previewView, store.captureSession == nil {
    previewView.removeFromSuperView()
    previewView = nil
  }
}

And this kind of tricky logic is what our fully general destination is supposed to accommodate, though sadly right now it is tailored only for UIViewController, not UIViews. I could imagine us generalizing it anymore to allow for this kind of pattern:

destination(item: $store.captureSession) { session in
  createPreviewLayer(size: view.bounds.size, session: session)
} present: { previewView in 
  view.addSubview(previewView)
} dismiss: { previewView in 
  previewView.removeFromSuperview()**
}

And then you wouldn't have to deal with observe and all the if/else-checking at all.

With all that said, I do believe that this isn't really an issue with the library and so I am inclined to convert it to a discussion. I feel that if you removed the side-effects from your observe you would not notice the extra observation, and I'm not entirely sure it's possible to avoid that observation. Though that would take more investigation to determine.

[yimajo]

Hi @mbrandonw. Thank you for your response.

I feel that if you removed the side-effects from your observe you would not notice the extra observation,

You mentioned removing side effects from observe, but please take another look at the example I initially provided: #3367 (comment)

In this case, when the state is set to nil, self is still not nil and the observation is triggered. Is there really a side effect happening here? Also, I don’t mind if this issue is moved to a discussion.

I hope my point is clear.

[mbrandonw]

In this case, when the state is set to nil, self is still not nil and the observation is triggered. Is there really a side effect happening here? Also, I don’t mind if this issue is moved to a discussion.

Hi @yimajo, sorry I did not mean to imply that the side-effects are causing this problem. I meant to say that the side-effect is what is making this problem noticeable. Without the side effect you probably would not have noticed that the observe closure was being called one extra time when dismissing.

And I will convert this to a discussion for now because I'm not sure it is a real problem right now. If it can be shown that this causes a real, unavoidable problem, then we can reconsider it. But for right now it seems like an innocent extra observation, and currently we are going to prioritize other things over investigate this anymore.