SwiftUI List Repeating ID Value

Question

I am working on a SwiftUI project that pulls data from Firebase Firestore using Combine. Each user has the ability to create "Offers" in the app. In order to list their offers on their account page I am using onAppear to pass the currentUserUid to my View Model so I can filter the database results using the currentUserUid. The OfferHistoryView is below. This works great the when the view first appears. My issue is when I return from the OfferDetailView, I received the following message.

ForEach>>>: the ID occurs multiple times within the collection, this will give undefined results!

While this is not crashing the app it is not ideal. I'v tried, deleting all items from the collection each time the view loads, and each time combine is called and this does not resolve the issue. I've also added print statements to try and catch the duplicates but I never see a duplicate. You can see my print statements and the rest of the corresponding files below. Any help would be appreciated.

OfferViewHistory - Where the message is originating from.

struct OfferHistoryView: View {
    let db = Firestore.firestore()
    
    @EnvironmentObject var authSession: AuthSession
    @EnvironmentObject var offerHistoryViewModel: OfferHistoryViewModel
    
    var body: some View {
        
        return VStack {
            List {
                ForEach(self.offerHistoryViewModel.offerRowViewModels, id: \.id) { offerRowViewModel in
                    NavigationLink(destination: OfferDetailView(offerDetailViewModel: OfferDetailViewModel(offer: offerRowViewModel.offer, listing: offerRowViewModel.listing ?? testListing1))
                                    .environmentObject(authSession)
                    ) {
                        OfferRowView(offerRowViewModel: offerRowViewModel)
                    }
                } // ForEach
            } // List
            .navigationBarTitle("Offer History")
        } // VStack
        .onAppear(perform: {
            for offerRowViewModel in self.offerHistoryViewModel.offerRowViewModels {
                print("Before startCombine: \(offerRowViewModel.id)")
            }
            self.offerHistoryViewModel.startCombine(currentUserUid: self.authSession.currentUserUid)
            for offerRowViewModel in self.offerHistoryViewModel.offerRowViewModels {
                print("After startCombine: \(offerRowViewModel.id)")
            }
        })
    } // View
}

OfferHistoryViewModel - where combine is called.

class OfferHistoryViewModel: ObservableObject {
    var offerRepository: OfferRepository

    // Published Properties
    @Published var offerRowViewModels = [OfferRowViewModel]()
    
    // Combine Cancellable
    private var cancellables = Set()
        
    // Intitalizer
    init(offerRepository: OfferRepository) {
        self.offerRepository = offerRepository
    }
    
    // Starting Combine - Filter results for offers created by the current user only.
    func startCombine(currentUserUid: String) {
        for offerRowViewModel in self.offerRowViewModels {
            print("Before startCombine func: \(offerRowViewModel.id)")
        }
        offerRepository
            .$offers
            .receive(on: RunLoop.main)
            .map { offers in
                offers
                    .filter { offer in
                        (currentUserUid != "" ? offer.userId == currentUserUid : false)
                    }
                    .map { offer in
                        OfferRowViewModel(offer: offer, listingRepository: ListingRepository())
                    }
            }
            .assign(to: \.offerRowViewModels, on: self)
            .store(in: &cancellables)
        
        for offerRowViewModel in self.offerRowViewModels {
            print("After startCombine func: \(offerRowViewModel.id)")
        }
    }
}

OfferRowView

struct OfferRowView: View {
    @ObservedObject var offerRowViewModel: OfferRowViewModel
    
    var body: some View {
        // Convenience variable for accessing the offer & listing.
        let offer = offerRowViewModel.offer
        let listing = offerRowViewModel.listing
        
        return VStack {
            Text(offer.id ?? "ID")
            Text(listing?.id ?? "ID")
            } // VStack
    } // View
}

OfferRowViewModel

class OfferRowViewModel: ObservableObject, Identifiable {
    // Properties
    var id: String = ""
    var listingRepository: ListingRepository
    
    // Published Properties
    @Published var offer: Offer
    @Published var listing: Listing?
    
    // Combine Cancellable
    private var cancellables = Set()
        
    // Initializer
    init(offer: Offer, listingRepository: ListingRepository) {
        self.offer = offer
        self.listingRepository = listingRepository
        self.startCombine()
    }
    
    // Starting Combine
    func startCombine() {
        // Get Offer
        $offer
            .receive(on: RunLoop.main)
            .compactMap { offer in
                offer.id
            }
            .assign(to: \.id, on: self)
            .store(in: &cancellables)
        
        // Get Connected Listing
        listingRepository
            .$listings
            .receive(on: RunLoop.main)
            .map { listings in
                listings
                    .first(where: { $0.id == self.offer.listingId})
            }
            .assign(to: \.listing, on: self)
            .store(in: &cancellables)
    }
}

user_19667973 · Answer

The issue is that OfferRowViewModel declares an id property, initially sets it to "", then uses a Combine publisher to update it to the 'real' id as the underlying property.

When you're conforming to Identifiable, it's often really handy to use a computed property instead. This will always give the correct id and will never be out of sync:

var id: String { offer.id }

There's another issue in the sample that's worth working through. OfferRowViewModel stands up a publisher that watches listings and filters over them. But as written, that means that every OfferRowViewModel will iterate over every listing, or in other words do an O(n * m) operation every time either offers or listings changes. That might be OK in practice, but if there are a lot of offers, or a lot of listings, or they change frequently, it could create performance problems. Combine is really powerful, but one of its downsides is it can make it a lot harder to see efficiency issues like that.

This code could be made simpler by replacing OfferRowViewModel with a simple model type:

struct OfferAndListing: Identifiable {
    var offer: Offer
    var listing: Listing

    var id: String { offer.id }
}

Then your publisher that vended offers might look more like this:

Publishers.CombineLatest(offerRepository.$offers, listingRepository.$listings)
    .receive(on: RunLoop.main)
    .map { (offers, listings) in
                offers
                    .filter { $0.userId == currentUserUid }
                    .map { OfferAndListing(
                        offer: $0,
                        listing: listings.first(where: { })
                    )}
    }
    .sink { [weak self] in self?.offersAndListings = $0 }
    .store(in: &cancellables)

That's not any more efficient yet, but it gives a single place to see where the work is being done and optimize, uses a single publisher chain that's easier to reason about, and simplifies the model type so it doesn't need to know anything about Combine, the data repository, etc.

SwiftUI List Repeating ID Value

1 Answers