PlaybackManager: Incremental @MainActor adoption from the UI side – Part 3 - #4961
PlaybackManager: Incremental @MainActor adoption from the UI side – Part 3#4961kean wants to merge 1 commit into
Conversation
…art 3 Continues the caller-side effort by isolating the remaining non-UIKit helpers that call into PlaybackManager from the main thread: SwipeActionsHelper, GeneratedChapterSeeker, the LiquidGlass Constants extension, SharingModal (which pulls in SharingHelper), CheckTranscriptAvailability, and UpNextHistoryModel. UpNextHistoryModel moves from two per-method annotations to type-level isolation, which also fixes reAddMissingItems mutating the Up Next queue off the main thread. CheckTranscriptAvailability's two notification observers now use MainActor.assumeIsolated rather than letting their @sendable closures silently inherit main-actor isolation.
Generated by 🚫 Danger |
|
Claude finished @kean's task in 6m 37s —— View job Code review:
|
| Where | Point |
|---|---|
UpNextHistoryModel.swift:5 |
reAddMissingItems now does unbounded DB reads + N writes on main during sheet dismissal; the three Task { } wrappers are now no-ops that read as background work |
TranscriptShelfButton.swift:40 |
assumeIsolated is safe & actually load-bearing (PR description has the rationale inverted); the @Sendable capture of self remains, and the observer tokens are still discarded |
SwipeActionsHelper.swift:231 |
Handler closures are erased to a nonisolated (IndexPath) -> Bool and called from nonisolated code, so the swipe layer isn't actually pinned — SwipeHandler is the higher-leverage annotation |
LiquidGlass.swift:32 |
Annotation is fine; Theme.systemIsDark on line 29 is the real hazard in this file and is a one-line win |
One correction to the PR description
addObserver(forName:object:queue:using:)takes a@Sendableclosure, so without the explicit hop the closure bodies would silently inherit main-actor isolation instead of asserting it.
It's the opposite way round: @Sendable closures don't inherit isolation — they're nonisolated. That's why MainActor.assumeIsolated is required rather than optional hardening here. Without it, self?.isTranscriptEnabled = … against the newly-@MainActor protocol wouldn't compile at all. Same change, but the reason matters for whoever reads this block next.
Security / performance
No security surface. The only performance change is the reAddMissingItems main-thread work noted above — everything else is compile-time.
Not verified
I can't compile on Linux, so I reasoned about isolation inference rather than confirming it. Two things I checked by hand and believe are fine, but that the Buildkite build is the real answer for: UpNextEntryView's explicit init(entryDate:) and its nonisolated-looking restore() calling the now-@MainActor model.reAddMissingItems — both OK because SwiftUI's View is @MainActor @preconcurrency, so the whole struct infers main-actor isolation and the isolated-default-value rule (SE-0411) is satisfied. Same for TableSwipeAction.handler accepting a main-actor-inferred closure: allowed because the parameter is non-Sendable. Only Danger has reported on this PR so far; the app build is still pending.
Review checklist
- Read the PR diff against
origin/trunk - Verify
GeneratedChapterSeekercompletion-handler isolation is actually main-thread - Check
SwipeActionsHelper/TableSwipeActioncallers and stored-closure isolation - Check
SharingHelper/SharingModalcall sites and nested-type isolation - Review
UpNextHistoryModelbehavioural change - Review
CheckTranscriptAvailabilityassumeIsolatedsafety - Check CI status (2 Danger runs green, app build pending)
- Post review + inline comments
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| import PocketCastsServer | ||
| import PocketCastsUtils | ||
|
|
||
| @MainActor |
There was a problem hiding this comment.
Moving to type-level isolation is the right call, and the reAddMissingItems fix is real — but two things follow from it that I think are worth addressing here rather than later.
1. The restore path now does unbounded DB work on the main thread. reAddMissingItems (lines 29–44) is one upNextHistoryEpisodes read + one findBaseEpisode per uuid + one addToUpNext per uuid (each a DB write, and PlaybackManager.swift:676 can additionally trigger queue.loadPersistedQueue() when currentEpisode == nil) + refreshList. The snapshot is unbounded and it's restored with ignoringQueueLimit: true, so a large Up Next history entry is now a main-thread stall landing exactly as the sheet dismisses (UpNextEntryView.swift:95-98). The PR description flags the trade-off, but the reads are the cheap part to keep off main since only [String] has to cross:
func reAddMissingItems(entry: Date) {
Task {
let dataManager = self.dataManager
let episodesUuid = await Task.detached { dataManager.upNextHistoryEpisodes(entry: entry) }.value
// …the addToUpNext / bulkOperationDidComplete / refreshList loop stays on main
}
}If that's deliberately deferred to a later part, a one-line comment saying so would help — right now the reasoning only lives in the PR body.
2. The three Task { } wrappers are now no-ops. With the class isolated they don't move anything off main; they only defer the body by one main-actor tick, while still reading as "this is background work." Suggest dropping them from loadEntries/loadEpisodes (both bodies become plain synchronous assignments — .onAppear at UpNextEntryView.swift:92 is outside body evaluation, so there's no "Publishing changes from within view updates" risk), and keeping Task { } only where there's a real await.
| episodeUuid == PlaybackManager.shared.currentEpisode?.uuid else { | ||
| return | ||
| } | ||
| MainActor.assumeIsolated { |
There was a problem hiding this comment.
assumeIsolated is safe here — queue: .main guarantees OperationQueue.main delivery, and the only poster (Episode+Transcript.swift:15) uses postOnMainThread — so this won't trap. Two notes though:
The PR description has the rationale slightly inverted. Because addObserver(forName:object:queue:using:) takes a @Sendable closure, the body does not inherit main-actor isolation (@Sendable closures are nonisolated). So assumeIsolated isn't hardening an inherited annotation — it's load-bearing: without it, self?.isTranscriptEnabled = … against the now-@MainActor protocol wouldn't compile at all. Worth correcting since it changes how the next reader reads this block.
The @Sendable capture of self is still there. self is a non-Sendable CheckTranscriptAvailability & AnyObject, now main-actor isolated, captured in a @Sendable closure. assumeIsolated papers over the body but not the capture. Since both conformers are UIKit types (TranscriptShelfButton, ShelfActionsViewController), the selector-based addCustomObserver helper (podcasts/Utilities/CustomObserver.swift) sidesteps the @Sendable closure entirely and gets isolation from @objc on a @MainActor class — that's already what TranscriptViewController.swift:186 does for this exact notification.
Pre-existing but adjacent: the tokens from addObserver(forName:…) are discarded, so neither registration is ever removed. Every TranscriptShelfButton rebuilt with the player shelf leaves two dead observers behind. Switching to addCustomObserver would fix that too.
Nit: guard … else { return } inside assumeIsolated returns from the assumeIsolated closure, not the observer closure. Identical today because nothing follows the block, but it'd silently change meaning if anything were ever appended.
| } | ||
| } | ||
|
|
||
| @MainActor |
There was a problem hiding this comment.
This annotation buys less than it looks like. The factory bodies get checked, but the closures they build are stored in TableSwipeAction.handler: (IndexPath) -> Bool — a nonisolated, non-Sendable function type — and invoked from nonisolated code (TableSwipeActions.swift:20 and :41). The inferred @MainActor on the closure is erased at that conversion, so nothing is enforced where it actually matters. It's correct today because UIKit/SwipeCellKit call those on main, but the compiler isn't checking it.
Two follow-ups that would close the gap, in rough order of value:
SwipeHandler(line 43) is still nonisolated even though every conformer is aUIViewControllerand every method touches UI or the DB. That asymmetry —@MainActorhelper calling a nonisolated protocol — is what leaves the handlers unchecked. Annotating the protocol is the higher-leverage change and would also letperformActionstop being the only enforced hop.TableSwipeAction/TableSwipeActionsthemselves are pure UIKit glue and are natural@MainActorcandidates.
Not blocking for this PR — just noting that the swipe layer isn't actually pinned yet.
| static var systemIsDark: Bool = false | ||
| } | ||
|
|
||
| @MainActor |
There was a problem hiding this comment.
Fine as-is — all 12 call sites are UIViewController/UIView contexts, so they already inherit main-actor isolation.
While you're annotating this file though: static var systemIsDark: Bool = false (line 29) is the actual concurrency hazard here — a mutable global whose own doc comment describes a write from MainTabBarController.traitCollectionDidChange and a read at scene setup, i.e. main-only in practice but unenforced. @MainActor on that Theme extension would be a one-line win in the same spirit as the rest of this PR (or nonisolated(unsafe) if it turns out something reads it off main, so at least the decision is recorded).
2 participants
Part 3 of the incremental
@MainActoradoption, continuing top-down from the UI side towardPlaybackManager(after #4947 and #4953).UIKit and SwiftUI types already inherit isolation from their base classes and protocols, so this pass covers the plain helper types that call into
PlaybackManagerfrom the main thread without saying so:SwipeActionsHelper,GeneratedChapterSeeker, theConstantsextension inLiquidGlass.swift,SharingModal(which pulls inSharingHelper),CheckTranscriptAvailability, andUpNextHistoryModel.Two are more than annotations:
UpNextHistoryModelmoves from two per-method@MainActors to type-level isolation.reAddMissingItemswas the one method without an annotation, so itsTask { }ran on the concurrent executor and mutated the Up Next queue off the main thread. Its DB reads move to main as a result, matchingloadEntriesandloadEpisodes.CheckTranscriptAvailability's two notification observers now useMainActor.assumeIsolated. They're registered withqueue: .main, but the closure is@Sendable, so without the explicit hop it would silently inherit isolation rather than assert it.Types below
PlaybackManagerare left alone — annotating those pushes isolation upward into it instead of toward it.PlayerColorHelperis main-only too, but cascades through the SwiftUI theming layer into isolated-default-value errors inToast.showandEmptyStateView.init; that needs its own PR.The review also found call sites that genuinely run off the main thread today and will need fixing before
PlaybackManagercan be isolated:WatchManager.handleWatchAction(WCSession delegate thread —skipBackis hopped to main,skipForwardtwo lines later isn't),ShortcutManager.updateShortcuts(DispatchQueue.global()),ServerSyncManager's sync-delegate callbacks, and the archive/cleanup paths. Follow-ups, not part of this PR.To test
Compile-time change plus one behavioral fix; no visible difference except Up Next History.
Checklist
CHANGELOG.mdif necessary.