TabView + ScrollView + ViewStore bug, multiple instances of same view

[GreatApe]

Hello, I have a pretty complex app with a TabView, where I run into an issue where some (but not all!) of the tab pages are instantiated multiple times. I'm trying to whittle it down to a small sharable example but so far it's still pretty complex. Seemingly it needs a combination of ScrollViews in a TabView, id'd in a particular way, and with a view store present, to trigger the problem.

The weird thing is that it seems that the mere presence of a viewStore as a property in the leaf view, without even referencing it, is enough to cause this (when all the other parts are there). So it's definitely something weird.

The question right now is how I can best debug this! For the moment I have have global counter dictionaries that are increased/decreased in the onAppear/onDisappears, what else can I bring to the fight?

[mbrandonw]

It's hard to say without seeing some code, but are you familiar with this article on ViewStore performance? Typically these kinds of problems are due to observing too much state in the view store.

[tgrapperon]

If your setup is like this:

WithViewStore(store, observe: { $0 }) { viewStore in
  TabView(selection: viewStore.binding(\.$selectedTab)) {
    Child(value: viewStore.name)
     .tabItem {
       Label("Profile", systemImage: "person")
     }
  }
}

This view will be completely re-evaluated as soon as something in store changes (and you will then spot many Child.init). The usual "ViewState" trick is to project onto a smaller state that is easier to diff and most importantly will only make the ObservedObject emit when something relevant changes. For example:

struct ViewState: Equatable {
  let selectedTab: Int
  let name: String
  init(storeState: State) {
    self.selectedTab = storeState.selectedTab
    self.name = storeState.name
  }
}

and then WithViewStore(store, observe: ViewState.init) { … }.
The issue is not only performance: SwiftUI has a few bugs when its ObservedObject emits too much. This could also explain the higher number of inits of your tabs. You can maybe get a better glimpse at what's happening using let _ = Self._printChanges() into your bodies.
That being said, it is also possible that you're encountering a new issue if observing a limited ViewState doesn't fix your problem. Please also note that anything that captures a ViewStore should be invalidated when that ViewStore's state changes.

[GreatApe]

I will try limiting the view state. It's not a performance issue at all, it's that there are multiple copies of the same view around at the same time, not just that they are created, the multiple copies stick around more or less permanently, which is why I'm thinking it can't simply be a TCA bug, it's worse than that. But the first step is probably to give TCA and SwiftUI the best working conditions.

Btw another ingredient that seems to be required is the whole BindingAction thing. If tabview is bound to a simple @State it all behaves as normal.

[GreatApe]

But could you elaborate on this, it seems important (even if it can't fully explain the issue here):

Please also note that anything that captures a ViewStore should be invalidated when that ViewStore's state changes.

Apart from WithViewStore, I'm only using ViewStore like this:

struct MainView: View {
    let store: StoreOf<MyApp>
    @ObservedObject private var viewStore: ViewStoreOf<MyApp>
    
    init(store: StoreOf<MyApp>) {
        self.store = store
        self.viewStore = ViewStore(store)
    }
    // ...
}

Sometimes I don't store the store, only the viewStore.

[tgrapperon]

My assertion was especially true for WithViewStore where we made sure that this was the case because it could have caused issues with views having escaping closures like GeometryReader or Stepper, so we made sure that the ViewStore instance you get in a WithViewStore closure is different each time this closure is evaluated (#1015). The ones installed as @ObservedObject are observed by SwiftUI itself, which seems to be enough to invalidate what needs to be invalidated. But it is possible that one can reconstruct with them the same issue we had with WithViewStore with enough indirection, and that's why I used "should be invalidated" instead of "will be invalidated". Sorry if it made you think that it was something to perform yourself.
Please let us know if scoping your ViewStore onto a smaller state does help or not. Again, this is not only a performance issue, as SwiftUI can behave erratically if one ObservedObject emits too much, even much higher upstream. ViewStore captures the store, so you shouldn't need to have to keep a reference to store to have the ViewStore instance working properly.

[stephencelis]

Apart from WithViewStore, I'm only using ViewStore like this:

@ObservedObject private var viewStore: ViewStoreOf<MyApp>

If you copied this line verbatim from your application it indicates a big problem: you have a view store observing your entire app's state. While SwiftUI is generally good about computing view updates and only rendering detected changes, as soon as you get a few layers deep with views observing the same state updates, there are bugs that will lead to erratic behavior, especially around navigation (drill-down links, sheets, tab views, etc.). Vanilla apps encounter these problems less often than TCA apps because they don't tend to share state like this.

The way around it, as suggested a few times in this thread already, is to chip away at the observed state so that a view only observes the state that it directly accesses in its body.

[GreatApe]

Sounds like it a very "analogue" kind of bug then, which explains the weird behaviour. But what is the best way to do the decomposition? Is it fine to pass down the same store but use more limited view states in the view stores, or is it important to also limit the store? In my case that would often lead to having 2 or 3 different stores in the same view.

[tgrapperon]

The store is not observed, so it is fine to pass it along as you see fit. But when you extract a ViewStore, you need to limit what you're exposing to only what's required to display the view. Please note that scoping doesn't requires you to observe the store's state, so you can scope directly from the store you have and pass it to a child view that will perform whatever operation it sees fit with it. With the new observe argument of WithViewStore, the ViewState scoping is performed internally.

[tgrapperon]

Something like this is fine:

struct AppView: View {
  let store: StoreOf<App>
  struct ViewState {
    let title: String
    let count: Int
    init(state: App.State) {
      self.title = state.title
	  self.count = state.array.count
    }
  }
  
  var body: some View {
    VStack {
      WithViewStore(store, observe: ViewState.init) { viewStore in
        Text("\(viewStore.title): \(viewStore.count) items")
        Child1View(store: store.scope(state: \.child1, action: App.Action.child1) // StoreOf<Child1>
      }
      Child2View(store: store.scope(state: \.child2, action: App.Action.child2) // StoreOf<Child2>
    }
  }
}

The only things I need to display this specific view are the title and the count. The store is directly scoped. In this slightly atypical example, I'm even scoping child2 "outside" of WithViewStore's reach to show that this is not something that needs to be observed. Whatever Child2View does with the scoped store is its own concern. It is slightly atypical because one usually and conveniently install the WithViewStore at the top level of body, but this is not a requirement. It shouldn't be very different performance-wise in real life situations. If you want to be extremely tidy on the performance side, you can even pull-out child1 scope from the WithViewStore. In real-life situations, there is usually not enough gain with respect to the flexibility you lose, so you usually end-up having a WithViewStore at the top level. But it doesn't mean that you need it to scope your stores to child features.

[GreatApe]

@tgrapperon Thank you for all explanations. I'm now in the process of decomposing all my stores, I used to think it was only a performance issue so I figured I could always do that later, but then these weird bugs started showing up.

Anyway, I've seen indirect allusions to WithViewStore being problematic to use in conjunction with GeometryReader for example, and now you also touch on the subject.

I personally prefer the other design, with an @ObservedObject (partly because I sometimes need several different ViewStores in the same view, which I hope is ok?), but I'm worried that we are not using that property wrapper the way it's meant to be used. I mean normally you'd have a permanent reference to an object in the -App instance and pass it down, or at least have a @StateObject one level up and pass it into the present View, where you store it as an @ObservedObject. But in this case we have a permanent Store somewhere that we pass around, and then constantly create new ViewStores from it, that we then observe.

This seems potentially problematic, has this been thoroughly tested? To me it would feel safer with say .viewStore being a permanent property on Store or something.

PS If you ever redesign WithViewStore, please consider a design where State is separate from the "coordinator":

WithViewStore(store) { state, send in
    Button {
        send(.tapped)
    } label: {
        Text(state.buttonLabel)
    }
}

I think it's a bit weird that the same thing is used to access state properties and do the sending. It would be especially nice if we could bring by the $boundProperty magic, it seems much more useful than forwarding the state properties to ViewStore.

[Alex293]

If you have multiple @ObservedObject viewStore as properties in the same view, the body will be redrawn each time one of the viewStore state is updated. i.e. you will have the same problèmes as observing too much state. The whole point of viewStore is limiting which parts of the view is redrawn. If you prefer using @ObservedObject style instead of WithViewStore use only one well scoped and pass down the store so child views can observe what they need. You will then see that WithViewStore is a shortcut for doing that inline with creating new views.

On a side note, I like your suggestion of splitting the state and send but I think viewStores offer more than that like binding utilities.

[GreatApe]

the body will be redrawn each time one of the viewStore state is updated. i.e. you will have the same problèmes as observing too much state

Yes, that's the purpose. I will of course only observe things I care about. So the question is if there are any inherent reasons for not using several ViewStores in one view. Too much state is a separate question, which is not an issue for me. I just want to avoid buggy behaviour.

[Alex293]

Buggy behaviour appear mainly when you observe too much state. Let say you have a parent viewStore which controls wether to push a child and some child like :

@ObservedObject childViewStore
@ObservedObject parentViewStore

var body {
  NavigationView {
    NavigationLink(parentViewStore.binding ... ) {
       ChildView using childViewStore
    }
  }
}

Even if you properly scope you viewStores by having them in as @ObservedObject as properties in the same view if the child updates it will redraw the navigation link which will cause navigation bugs. If instead you used WithViewStore around each use the navigation would not be redrawn.

[GreatApe]

Ok, but it's not a question of too much state, I observe exactly the minimal amount of state. I just wonder if it could be an issue to observe to separate ViewStores in the same view. For example, I have a store that controls some audio buttons, this is used in two places in the app, and it's very convenient to keep it in one reducer, rather than add them to several different view oriented reducers.

And for what it's worth I'm not using navigation views or links.

I will post a minimal example now.

[Alex293]

Ok sorry I misinterpreted your issue.

[GreatApe]

This can be pasted into a project with TCA protocol-beta and will trigger the bug. I commented in 3 places where there are things that can be removed to stop the bug from occurring. Note that the main view store is only used in one place, and there it is stateless.

import SwiftUI
import ComposableArchitecture

// MARK: Views

@main
struct TestApp: App {
    private let store: StoreOf<Talking> = .init(initialState: .init(), reducer: Talking())
    
    var body: some Scene {
        WindowGroup {
            WithViewStore(store.stateless) { viewStore in
                ContentView(store: store)
                    .onAppear {
                        // Trigger 1 (see below)
                        viewStore.send(.launched)
                    }
            }
        }
    }
}

struct ContentView: View {
    let store: StoreOf<Talking>

    // This will of course contain the wrong UUID when the app is launched
    @State private var page: UUID = .init()

    // Trigger 2: Removing this unused binding - no bug
    let someBinding: Binding<Int> = .init(get: { 0 }, set: { _ in })
                
    var body: some View {
        VStack {
            TabView(selection: $page) {
                ForEachStore(store.scope(state: \.roomPages, action: Talking.Action.roomPage)) { roomPageStore in
                    RoomPageView(store: roomPageStore)
                }
            }
            .tabViewStyle(.page(indexDisplayMode: .never))
            Text("Showing: \(page)")
        }
    }
}

// ROOM PAGE

struct RoomPageView: View {
    @ObservedObject private var viewStore: ViewStore<RoomPage.State, RoomPage.Action>
    
    init(store: Store<RoomPage.State, RoomPage.Action>) {
        self.viewStore = ViewStore(store)
    }
    
    var body: some View {
        // Trigger 3: Remove the scrollview - no bug
        ScrollView(showsIndicators: false) {
            VStack {
                Text(viewStore.name)
                    .font(.largeTitle)
                Text(viewStore.id.uuidString)
            }
            .padding(100)
            .onAppear {
                roomPageViews[viewStore.name, default: 0] += 1
                let count = roomPageViews[viewStore.name, default: 0]
                if count > 1 { print("MULTIPLE \(viewStore.name) (#\(count))") }
            }
            .onDisappear {
                roomPageViews[viewStore.name, default: 0] -= 1
                let count = roomPageViews[viewStore.name, default: 0]
                if count == 1 { print("Single \(viewStore.name)") }
            }
        }
    }
}

var roomPageViews: [String: Int] = [:]

// MARK: States and Actions

struct Talking: ReducerProtocol {
    struct State: Equatable {
        // Trigger 1: If these are set here, and not in the reducer - no bug
        var roomPages: IdentifiedArrayOf<RoomPage.State> = [] // = .init(uniqueElements: names.map(RoomPage.State.init))
    }
    
    enum Action: Equatable {
        case roomPage(id: UUID, action: RoomPage.Action)
        case launched
    }

    func reduce(into state: inout State, action: Action) -> Effect<Action, Never> {
        switch action {
            case .roomPage:
                break
                            
            case .launched:
                // Trigger 1: If these are not set here, but in the State init - no bug
                state.roomPages = .init(uniqueElements: names.map(RoomPage.State.init))
        }
        
        return .none
    }
}

let names: [String] = ["Alec", "Ares", "Armando", "Armani", "Benji", "Drake", "Dustin", "Edgar", "Edwin", "Finnegan"]

// Child State

enum RoomPage {
    struct State: Identifiable, Equatable {
        let id: UUID = .init()
        let name: String
    }
    
    enum Action: Equatable { }
}

[tgrapperon]

If you add .id(viewStore.id) after the .onDisappear in RoomPageView, you tie this view's identity to viewStore.id and SwiftUI won't create additional views with the same id when switching pages.
I'll try to reproduce with Vanilla SwiftUI, or to understand why it's different.

@stephencelis, @mbrandonw, do you think we should assign the id (to EachState.ID) of the view in ForEachStore? It would mean that the view's lifecycle would be bound to the id, which would make sense in terms of onAppear/onDisappear balancing I guess. I assumed SwiftUI would do this automatically (and it maybe did at some point), but I think it would be safe if achievable in our case, as the presented view for id is completely tied to it. The problem is that the resulting IDView<EachContent, ID> is internal, so we can't really write conditional initializers with where without some additional indirection. Doing so would probably induce a deeper refactor of ForEachStore that could not play well with the existing deprecations. Or maybe this something to keep in mind if a new variant is implemented to handle other types of collections and requires larger changes. One alternative would be to optionally propose a (ID, Store) -> EachContent closure where users would be free to set the id themselves without having to open the store to extract an id:

ForEachStore(…) { id, store in
  ChildView(store: store)
    .id(id)
}

I think this form could cohabit with the current one that only uses a unique Store argument.

[GreatApe]

This actually works, even in the real app, amazing. Thanks a lot.

I do have a similar issue inside each of my pages, I have a list there of smaller views and the first three (the initially visible ones) also appear to be repeated. The system warns me about this because I'm using matchedGeometryEffect, that's how I noticed all this in the first place. It seemed very much that this was due to the entire page being repeated, but now that this is fixed it appears to be a separate issue. But hopefully it can be resolved in a similar way.

Thanks again!

[tgrapperon]

@mbrandonw, @stephencelis: About assigning ID's ourselves in ForEachStore, I've just checked using a custom _IDView wrapper (with an IDView body), and it doesn't fix the issue right away. It turns out that this is the ScrollView that is inferring with the identifiers. I'll try to reproduce with Vanilla, but it doesn't seem there's a case for assigning ID's in ForEachStore anymore.

[GreatApe]

Just FYI, another way to stop the bug is to not use a WithViewStore inside RoomPageView. If you instead expose the specific parameters and put the WithViewStore around the RoomPageView, then things also work as they should:

struct RoomPageView: View {
    let name: String
    let id: UUID
   // ...
}

and then:

ForEachStore(store.scope(state: \.roomPages, action: Talking.Action.roomPage)) { roomPageStore in
    WithViewStore(roomPageStore) { vs in
        RoomPageView(name: vs.name, id: vs.id)
    }
}

[tgrapperon]

In any case the following conversion to Vanilla SwiftUI exhibits the same issue, so if this is a bug, it doesn't involve TCA.