PlaybackManager: Incremental @MainActor adoption from the UI side – Part 2

[kean]

Stacked on #4947.

Part 1 left three per-method @MainActor annotations as placeholders where the whole type should be isolated. This promotes them to type-level and closes the ring of callers that pulled in.

• SearchResultCellModel, EpisodeRowViewModel (tvOS) and EpisodeUpNextActions are now @MainActor. The redundant @MainActor on the inner Tasks and on play() goes away with it.
• Five call sites built EpisodeRowViewModel off-main inside Task.detached. Rather than weaken the annotation, each one now keeps the database work off-main and constructs the view models on main. HomeViewModel gets slightly faster out of this — it was calling parentPodcast(dataManager:) for the Up Next rows inside MainActor.run, so that query moves off-main.

One latent off-main call surfaced and is fixed: DiscoveryEpisodeMenuButtons.load(_:) ran its completion on the generic executor, so Play Next / Play Last in the Discover and Search context menus mutated the Up Next queue, wrote a @Binding, and showed a toast off the main thread.

The Task { @MainActor in } added to PodcastDetailViewModel's archive observer is redundant at runtime — the publisher is already .receive(on: DispatchQueue.main). It is there only because the class itself isn't isolated yet. PodcastDetailViewModel, HomeViewModel and UpNextViewModel should follow, which would delete the MainActor.run dance in all three, but that first needs TVDataManager.loadPodcast and fetchEpisodes to move off-main or the podcast-detail query lands on the main thread.

To test

tvOS — the episode lists still populate and act correctly:

1. Home → Up Next and New Releases rows populate; play an episode from each.
2. Podcasts → open a podcast; the episode list loads. Toggle Show Archived and change the sort order; the list reloads in the new order.
3. Podcasts → archive an episode from the context menu; the list refreshes.
4. Up Next → the queue populates; Play Next, Play Last and Remove From Up Next work.
5. Starred and Listening History → both lists populate.
6. Discover → open the context menu on an episode; Play Next and Play Last add it to Up Next and show the toast.

iOS:

7. Search → play an episode result; the spinner appears for slow loads, then playback starts. Pause it again.

Checklist

• I have considered if this change warrants user-facing release notes and have added them to CHANGELOG.md if necessary.
• I have considered adding unit tests for my changes.
• I have updated (or requested that someone edit) the Event Horizon schema to reflect any new or changed analytics.

[claude]

Claude finished @kean's task in 4m 36s —— View job

---

Review: incremental @MainActor adoption, part 2

• Fetch and read the diff against origin/kean/mainactor-playback-actions
• Review the tvOS episode-row view model changes
• Review EpisodeRowViewModel / EpisodeUpNextActions isolation
• Review SearchResultCellModel and DiscoveryEpisodeMenuButtons
• Post review (5 inline comments)

This is a clean, well-scoped step. The direction is right and the PR description is unusually honest about what it defers — that made reviewing it fast. No blocking issues; everything below is either a suggestion or a pre-existing problem the refactor walks past.

The DiscoveryEpisodeMenuButtons.load(_:) fix is a real bug fix, not just a annotation cleanup — TVDataManager.loadEpisode is a nonisolated async function, so before this change Play Next / Play Last in the Discover and Search context menus were mutating PlaybackManager.queue, writing a SwiftUI @Binding, and calling ToastManager from the generic executor. Typing the parameter as @escaping @MainActor (DiscoveryLoadedEpisode) -> Void rather than just wrapping the body is the right call: it pushes the requirement onto the callers instead of hiding it.

Main points

1. The off-main property in HomeViewModel/PodcastDetailViewModel is implicit and will break silently. (HomeViewModel.swift:34-39)

Both load() methods keep their DB work off the main thread only because the enclosing class happens to be nonisolated, so Task {} inherits no isolation. When these classes get the @MainActor you say is coming, allUpNextEpisodes(), findNewReleaseEpisodes, fetchEpisodes and up to 25 parentPodcast(dataManager:) calls move onto main with no diagnostic at all.

The three sibling view models in this same commit already use the pattern that survives that promotion — Task.detached + a nonisolated load function. Using it in these two as well would turn the future annotation into a compile error rather than a silent hitch on Home and the podcast screen. Given that "make the compiler enforce it" is the whole point of the stack, it seems worth doing here rather than in part 3.

2. EpisodeRowViewModel.id should be a stored nonisolated let. (EpisodeRowViewModel.swift:7)

Isolating the class makes the computed id main-actor-isolated, which no longer cleanly satisfies Identifiable's nonisolated requirement (warning in Swift 5 mode, error under Swift 6) — and it's precisely why PodcastDetailViewModel needed the new Task { @MainActor in }. The uuid is effectively immutable (both observers only swap episode when the uuid matches), so nonisolated let id: String set in init is safe and removes both problems.

3. The Task { @MainActor in } in the archive observer isn't purely redundant. (PodcastDetailViewModel.swift:132-137)

Thread-wise yes, timing-wise no. load() now runs a runloop turn later, and moving the contains check inside the task means two archive notifications in the same turn enqueue two tasks that both pass the check and both call load() — two concurrent loads racing to write self.episodes. Point 2 removes the need for the hop entirely.

Also flagged there: load() reads self.podcast / showArchived / isDiscover off-main while the MainActor.run block writes self.podcast, and setShowArchived, setSortOrder and this observer can all trigger it in quick succession. Pre-existing, and your planned isolation of the type is the fix — just noting it so it doesn't get lost.

Smaller things

• UpNextViewModel.swift:50 — loadEpisodes is missing the nonisolated its two siblings have. Suggestion included.
• HomeViewModel.swift:43-45 — pre-existing, but on a line you're rewriting: currentPlaying is assigned but never cleared, so emptying Up Next leaves a stale Now Playing row. One-line suggestion included.
• SearchResultCellModel.swift:7 — the change is correct; MainEpisodeActionViewDelegate is a plain nonisolated protocol, so consider marking it @MainActor (it's a UIView delegate) to make the conformance checked rather than accidentally correct.
• EpisodeRowViewModel.swift:10 — static func == is unused (the class isn't Equatable) and is now main-actor-isolated as well. Same for MockEpisodeRowViewModel.swift:239. Probably deletable.
• Style, take it or leave it: the withPodcast → tuple → makeRowViewModel pairing in HomeViewModel is more indirection than the code it replaced, and makeRowViewModel is a one-line wrapper called four times via .map { self.makeRowViewModel(for: $0) }. A small named struct, or just constructing the view model inline in the MainActor.run block like PodcastDetailViewModel does, would read more directly.

On testing

The manual test plan is thorough and matches the touched surface. Since none of this is covered by unit tests, the two I'd focus on are step 6 (Discover context menu — the actual bug fix, and previously it would have "worked" most of the time) and step 2 (podcast detail sort/archive toggle, where the observer timing changed).

I did not build or run anything — the tvOS and iOS builds run on Buildkite, which isn't reachable from here, so the isolation-warning claims above are read from the source rather than from compiler output.
· branch kean/main-actor-playback-part2

[claude]

The "keep the DB work off-main" property here is entirely implicit: it holds only because HomeViewModel is nonisolated, so Task {} picks up no isolation and lands on the generic executor. The moment this class gets the @MainActor the PR description says is coming, allUpNextEpisodes(), findNewReleaseEpisodes, findNewVideoReleaseEpisodes and up to 25 parentPodcast(dataManager:) queries silently move onto the main thread — no compiler diagnostic, no test failure, just a hitchy Home screen.

The three sibling view models this PR touches (Starred, ListeningHistory, UpNext) already use the pattern that survives that promotion: Task.detached + a nonisolated load function. Applying it here too would make the future annotation a compile error instead of a silent regression:

func load() {
    Task.detached { [dataManager] in
        let entries = Self.loadEntries(using: dataManager)
        await MainActor.run { [weak self] in ... }
    }
}

nonisolated private static func loadEntries(using dataManager: DataManager) -> (...)

Same argument applies to PodcastDetailViewModel.load() and its fetchEpisodes call.

Fix this →

[claude]

Pre-existing, but you're rewriting this exact line so it's cheap to fix: currentPlaying is only ever assigned, never cleared. If the user empties Up Next, load() re-runs with upNextEpisodes.first == nil and the stale row sticks around — shouldShowNowPlayingRow keeps returning true until playback starts.

Suggested change

	if let currentlyPlaying {
	self.currentPlaying = self.makeRowViewModel(for: currentlyPlaying)
	}
	self.currentPlaying = currentlyPlaying.map { self.makeRowViewModel(for: $0) }

[claude]

Isolating the class makes id (line 17, var id: String { episode.uuid }) main-actor-isolated, which means it no longer cleanly satisfies Identifiable's nonisolated id requirement. Under Swift 5 mode that's a warning at worst, but it becomes an error under Swift 6 — and it is the direct cause of the extra Task { @MainActor in } you had to add in PodcastDetailViewModel.setupObservers.

Storing it instead makes the conformance honest and lets that observer stay synchronous:

nonisolated let id: String
// in init: self.id = episode.uuid

This is safe: the two archive/play-status observers below (lines 159 and 173) only replace episode when uuid == episode.uuid, so the uuid is effectively immutable for the lifetime of the view model.

Two smaller things while you're in here:

• static func == (line 10) is now main-actor-isolated too, and the class doesn't conform to Equatable — nothing in the tvOS target calls it. Same for MockEpisodeRowViewModel.== at line 239. Both look like dead code that could go.
• @MainActor on EpisodeUpNextActions is the right call — it's pure PlaybackManager + ToastManager mutation.

Fix this →

[claude]

Worth being precise about what this costs: it's redundant thread-wise (the publisher is already .receive(on: .main)), but it is not redundant timing-wise. load() now fires one runloop turn later than the notification, and because the contains check moved inside the task, two archive notifications delivered in the same turn now enqueue two tasks that both pass the check and both call load() — two concurrent load Tasks racing to write self.episodes.

The nonisolated let id change suggested on EpisodeRowViewModel removes the need for this hop entirely and restores the synchronous behaviour.

Separately (pre-existing, but this PR is about exactly this): load() at line 83 reads self.podcast, self.showArchived and self.isDiscover from the generic executor while the MainActor.run block on line 93 writes self.podcast. With setShowArchived / setSortOrder / this observer all able to call load() in quick succession, that's a genuine unsynchronised read/write on a nonisolated class. Isolating PodcastDetailViewModel (once TVDataManager.loadPodcast/fetchEpisodes are async, as you note) is the fix.

[claude]

Nit / consistency: the two sibling versions of this method that the same commit rewrites are declared nonisolated private func (StarredEpisodesViewModel.swift:58, ListeningHistoryViewModel.swift:40); this one isn't. It makes no difference today since the class is nonisolated, but the explicit keyword is what keeps the query off-main once UpNextViewModel itself becomes @MainActor.

Suggested change

	private func loadEpisodes(using dataManager: DataManager) -> [(episode: BaseEpisode, podcast: Podcast?)] {
	nonisolated private func loadEpisodes(using dataManager: DataManager) -> [(episode: BaseEpisode, podcast: Podcast?)] {

[claude]

This is correct, and the two Task { @MainActor in } become plain Task {} cleanly because the closures inherit the class isolation.

One follow-up worth considering: MainEpisodeActionViewDelegate (podcasts/Common Components/MainEpisodeActionView.swift:5) is a plain non-isolated protocol, so playTapped()/pauseTapped() are now main-actor-isolated witnesses satisfying nonisolated requirements. It works — MainEpisodeActionView is a UIView and only ever invokes the delegate from button handlers — but it's accidentally correct rather than checked. Marking that protocol @MainActor is a one-line change that makes it enforced, and it's the sort of thing that would otherwise bite the next class to adopt isolation here.

[kean]

Closing in favor of #4953