Only mutate ManagedPlayerViewModel.loadingState on main thread (#566)

Published properties in SwiftUI models should not be mutated on background threads. Refactor `ManagedPlayerViewModel.next` to ensure all paths that update `loadingState` do so from the main thread. Add unit tests to verify loadingState changes arrive on the main thread when next is called. Co-authored-by: Harris Borawski <[email protected]>
player-ui · Jan 6, 2025 · cb3ea0c · cb3ea0c
1 parent 16f2a50
commit cb3ea0c
Show file tree

Hide file tree

Showing 2 changed files with 66 additions and 12 deletions.
diff --git a/ios/swiftui/Sources/ManagedPlayer/ManagedPlayerViewModel.swift b/ios/swiftui/Sources/ManagedPlayer/ManagedPlayerViewModel.swift
@@ -134,18 +134,16 @@ public class ManagedPlayerViewModel: ObservableObject, NativePlugin {
     }
 
     func next(_ state: CompletedState? = nil) async {
-        DispatchQueue.main.async { [weak self] in
-            self?.loadingState = .loading
-            self?.flow = nil
-        }
-
-        do {
-            let nextState = try await self.manager.next(state)
-            DispatchQueue.main.async { [weak self] in
-                self?.handleNextState(nextState)
+        Task { @MainActor in
+            self.loadingState = .loading
+            self.flow = nil
+
+            do {
+                let nextState = try await self.manager.next(state)
+                self.handleNextState(nextState)
+            } catch {
+                self.loadingState = .failed(error)
             }
-        } catch {
-            self.loadingState = .failed(error)
         }
     }
 

diff --git a/ios/swiftui/Tests/ManagedPlayer/ManagedPlayerViewModelTests.swift b/ios/swiftui/Tests/ManagedPlayer/ManagedPlayerViewModelTests.swift
@@ -84,7 +84,7 @@ class ManagedPlayerViewModelTests: XCTestCase {
         let viewModel = ManagedPlayerViewModel(manager: flowManager, onComplete: {_ in })
 
         await assertPublished(AnyPublisher(viewModel.$loadingState), condition: { state in
-            guard 
+            guard
                 case .failed(let error) = state,
                 let _ = error as? PlayerError
             else { return false }
@@ -281,6 +281,62 @@ class ManagedPlayerViewModelTests: XCTestCase {
             XCTFail("Should have entered idle state")
         }
     }
+
+    func testNextUpdatesLoadingStateOnMainNoFlows() async throws {
+        let model = ManagedPlayerViewModel(manager: ConstantFlowManager([], delay: 1/60), onComplete: {_ in})
+        try await checkNextUpdatesLoadingStateOnMain(model: model, expectedStateChangeCount: 1)
+    }
+
+    func testNextUpdatesLoadingStateOnMainOneFlow() async throws {
+        let model = ManagedPlayerViewModel(manager: ConstantFlowManager(["abc"], delay: 1/60), onComplete: {_ in})
+        try await checkNextUpdatesLoadingStateOnMain(model: model, nextCallCount: 2, expectedStateChangeCount: 4)
+    }
+
+    func testNextUpdatesLoadingStateOnMainMultipleFlows() async throws {
+        let model = ManagedPlayerViewModel(manager: ConstantFlowManager(["abc", "def", "ghi"], delay: 1/60), onComplete: {_ in})
+        try await checkNextUpdatesLoadingStateOnMain(model: model, nextCallCount: 3, expectedStateChangeCount: 6)
+    }
+
+    func testThrowingNextUpdatesLoadingStateOnMain() async throws {
+        let model = ManagedPlayerViewModel(manager: ThrowingFlowManager(), onComplete: {_ in})
+        try await checkNextUpdatesLoadingStateOnMain(model: model, expectedStateChangeCount: 2)
+    }
+
+    private func checkNextUpdatesLoadingStateOnMain(model: ManagedPlayerViewModel, nextCallCount: Int = 1, expectedStateChangeCount: Int) async throws {
+        let expect = (0..<expectedStateChangeCount).map {
+            expectation(description: "\($0)")
+        }
+        var stateChangeCount = 0
+        // the publisher emits immediately, we only care about changes
+        // from the `next` call(s) below, drop the first state received
+        let cancellable = model.$loadingState.dropFirst().sink { state in
+            XCTAssertTrue(Thread.isMainThread)
+            XCTAssertTrue(stateChangeCount < expect.count)
+            stateChangeCount += 1
+            expect[stateChangeCount - 1].fulfill()
+        }
+        await withTaskGroup(of: Void.self) { group in
+            group.addTask {
+                for _ in (0..<nextCallCount) {
+                    await model.next()
+                }
+            }
+            for await _ in group {}
+        }
+        await fulfillment(of: expect)
+        XCTAssertEqual(stateChangeCount, expectedStateChangeCount)
+        cancellable.cancel()
+    }
+
+    private struct ThrowingFlowManager: FlowManager {
+        func next(_: CompletedState?) async throws -> NextState {
+            throw Errors.failed
+        }
+
+        enum Errors: Error {
+            case failed
+        }
+    }
 }
 
 class TerminatingManager: FlowManager {