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

[kean]

Continues the incremental @MainActor adoption started in #4947 (Part 1) and #4948 (Part 2). Independent of Part 2 — no overlapping files — so it targets trunk directly rather than stacking.

Closes three more caller-side rings:

• MainEpisodeActionView and EpisodeCell are now @MainActor, finishing the type-level isolation Part 1 only started on MainEpisodeActionViewDelegate. EpisodeCell is the main app's episode-row cell (Podcasts, Downloads, Starred, Uploaded, Listening History, Playlists).
• BookmarkListRouter, BookmarkListViewModel (and its two subclasses BookmarkEpisodeListViewModel/BookmarkPodcastListViewModel), and the four bookmark hosting controllers (BookmarkDetailsViewController, BookmarkEpisodeListController, BookmarksProfileListController, BookmarksPlayerTabController) are now @MainActor.
• The now-playing player screen: PlayerItemViewController (base class), PlayerContainerViewController, NowPlayingPlayerItemViewController, ShowNotesPlayerItemViewController, PlayerTabsView, PlayerChapterCell, and PlayerZoomAnimator are now @MainActor, along with the PlayerItemContainerDelegate, PlayerTabDelegate, NowPlayingActionsDelegate, and TimeSliderDelegate protocols. ChaptersViewController and TranscriptViewController pick up isolation automatically via the base class.

UserEpisodeDetailProtocol is promoted from a per-method placeholder to full type-level @MainActor.

All rings closed cleanly. The player UI ring surfaced a couple of real fixes along the way:

• BookmarkListRouter needed its own @MainActor once its conformers were isolated, and UserEpisodeDetailProtocol's showBookmarks default implementation needed explicit isolation since it lives in a protocol extension rather than a conforming type.
• A few SDK delegate protocols (AVRoutePickerViewDelegate, SFSafariViewControllerDelegate) and one widely-used app protocol (AnalyticsSourceProvider) aren't @MainActor-audited upstream, so those specific conformances use @preconcurrency rather than promoting the shared protocols (AnalyticsSourceProvider alone has 24 unrelated conformers app-wide).
• ShowNotesPlayerItemViewController's deinit was nil-ing out a weak delegate — redundant, since ARC already zeroes weak references on deallocation, and not something deinit (always non-isolated) can safely do to a @MainActor property anyway. Removed.
• TranscriptViewController's Task.detached transcript loader now correctly hops back to @MainActor for its UI-touching completion work (show(transcript:), show(error:), track(...)), which the compiler couldn't previously verify.

To test

1. Any episode list (Podcasts, Downloads, Starred, Uploaded, Listening History, Playlists) — tap play/pause/download/error/waiting-for-wifi on an episode cell; confirm each action still works.
2. Player → Bookmarks tab — play, edit, share, and delete a bookmark.
3. A podcast episode's bookmark list and Profile → Bookmarks — same actions, plus sorting and multi-select.
4. Uploaded file detail sheet → tap the Bookmarks row; confirm the standalone bookmark list opens.
5. Full player screen — play/pause/skip, seek, chapters, show notes, transcript (including search and generated-transcript sync), and the mini-player ↔ full-player zoom transition.

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 5m 4s —— View job

---

Review: @MainActor adoption Part 3

Read the full diff plus every annotated type, its superclass chain, all conformers of the two protocols, and the posting side of every notification these types observe. Three inline comments posted. Summary below.

The two protocol annotations are the real content of this PR and both look correct:

• BookmarkListRouter — conformers are BookmarkEpisodeListController, BookmarksPlayerTabController, BookmarksProfileListController and PodcastViewController (PodcastViewController.swift:1629, not mentioned in the description), plus BookmarkListControllerDelegate which already refines UIViewController. All main-actor already, so no conformance breaks.
• UserEpisodeDetailProtocol — conformers are UpNextViewController and UploadedViewController, both UIViewController subclasses. The where Self: UIViewController extension picks up the protocol's isolation, so no per-method annotation is needed there.

1. Seven of the nine annotations are no-ops

Every annotated class already inherits @MainActor:

Type	Already isolated via
MainEpisodeActionView	UIView
EpisodeCell	ThemeableSwipeCell → SwipeTableViewCell → UITableViewCell
BookmarkDetailsViewController, BookmarkEpisodeListController, BookmarksProfileListController	ThemedHostingController → ModifedHostingController → UIHostingController
BookmarksPlayerTabController	PlayerItemViewController → SimpleNotificationsViewController → UIViewController
BookmarkListViewModel	SearchableListViewModel → ListViewModel, explicitly @MainActor at ListViewModel.swift:11

Superclass isolation inference isn't affected by SWIFT_DEFAULT_ACTOR_ISOLATION = nonisolated — that default only applies where nothing else infers isolation, and a subclass can't diverge from its superclass's isolation anyway.

Harmless as documentation, and there's a reasonable argument for spelling it out. But it does change how the PR should be read: these rings were already closed, which is why "both rings closed cleanly — no latent off-main bugs surfaced" — nothing could surface from an annotation that doesn't change isolation. If the goal is documentation, the same sweep could drop the now-redundant @MainActor dynamic required init?(coder:) in the three hosting controllers and Task { @MainActor … } at BookmarkListViewModel.swift:133-134.

2. EpisodeCell does have genuinely off-main @objc entry points

Pre-existing rather than introduced here, but this PR is the natural place to fix it since it's explicitly asserting the isolation. Three of the notifications the cell observes are posted with a plain NotificationCenter.default.post off the main thread:

• userEpisodeUploadProgress — UploadProgressManager.swift:48, reached from urlSession(_:task:didSendBodyData:…) (UploadManager+URLSessionDelegate.swift:78), i.e. the URLSession delegate queue
• userEpisodeUploadStatusChanged — UploadManager+URLSessionDelegate.swift:63, RetrieveFileUploadStatus.swift:31/:43
• episodeTypeOrLengthChanged — MetadataTask.swift:71

The Thread.isMainThread fallbacks at EpisodeCell.swift:449 and :531 are there for exactly this. The issue is that both handlers touch self's state before hopping — uploadProgressDidUpdate reads episode, hits the DB via reloadEpisode(), and assigns self.episode (line 528) all off-main; updateCell reads episode?.uuid and hits the DB before its hop. Under @MainActor those accesses are now claimed to be serialized on the main actor, and with actor data-race checks enabled (Xcode Debug default) or a Swift 6 language-mode migration the @objc thunk traps on entry before the Thread.isMainThread branch is even reached. Details and a suggested nonisolated + hop-at-the-top rewrite are in the inline comment.

3. Minor

BookmarksPlayerTabController.willBeAddedToPlayer subscribes to playbackTrackChanged without .receive(on:), unlike the onBookmarkCreated sink directly below it. Safe today (that notification always goes through postOnMainThread) but it's relying on an unenforced convention now that the closure captures main-actor state. Inline comment has the detail, plus an unrelated note about cancellables accumulating across repeated willBeAddedToPlayer calls.

Not verified

I couldn't compile — this runner is Linux, no Xcode toolchain — so make build_staging / make test_staging weren't run. Everything above is from reading the source and the build settings; the "already isolated by inheritance" claim in particular is worth a quick sanity check on your machine (deleting one annotation should produce no diagnostics).

No security or performance concerns. Nothing here blocks the merge — the annotations are safe as written.
• branch kean/main-actor-playback-part3

[dangermattic]

	1 Warning
⚠️	View files have been modified, but no screenshot or video is included in the pull request. Consider adding some for clarity.

Generated by 🚫 Danger

[claude]

Claude finished @kean's task in 5m 8s —— View job

---

Review: @MainActor adoption Part 3 — new commit 5fe2e86 (player UI cluster)

• Fetch updated diff vs origin/trunk
• Review removed deinit in ShowNotesPlayerItemViewController
• Review the three @preconcurrency conformances
• Review TranscriptViewController timer change
• Review new protocol isolations and their conformers
• Post review

Re-reviewed only what the new commit added (the Part-3 files from my earlier pass are unchanged). Three inline comments posted.

One thing worth resolving before merge

extension PlayerContainerViewController: @preconcurrency AnalyticsSourceProvider (PlayerContainerViewController.swift:430) is the only change here with runtime behaviour. Per SE-0423, a @preconcurrency conformance emits a dynamic main-actor check into the witness thunk, so reading analyticsSource through the existential off-main now traps rather than racing.

There is an off-main path into that witness:

ServerPodcastManager.subscribeQueue (OperationQueue)        ServerPodcastManager.swift:15/88
  → updateLatestEpisodeInfo(setDefaults: true)              ServerPodcastManager.swift:219/243
  → autoDownloadLatestEpisodes(uuids:)                      ServerPodcastManager+Update.swift:259
  → AnalyticsEpisodeHelper.shared.downloaded(episodeUUID:)  ServerSyncManager.swift:179
  → cacheDownloadSource → currentAnalyticsSource            AnalyticsEpisodeHelper.swift:171
  → topAnalyticsSourceProvider()?.analyticsSource           AnalyticsCoordinator.swift:94/130

track(_:properties:) is safe — it guards on Thread.isMainThread and re-dispatches (AnalyticsCoordinator.swift:103) — but cacheDownloadSource has no such guard. Narrow trigger (background subscribe with auto-download on, and PlayerContainerViewController is the top analytics provider), and the off-main UIKit traversal is pre-existing; this PR just upgrades it from UB to a hard trap. A guard Thread.isMainThread else { return .unknown } in currentAnalyticsSource closes it — suggestion in the inline comment.

The other two @preconcurrency conformances are fine: AVRoutePickerViewDelegate (NowPlayingPlayerItemViewController+Shelf.swift:636) and SFSafariViewControllerDelegate (ShowNotesPlayerItemViewController.swift:9) are both only ever invoked by AVKit/SafariServices on the main thread. Also worth noting the asymmetry with TranscriptViewController: AnalyticsSourceProvider (:7), which correctly needs no @preconcurrency — its analyticsSource is a let of a Sendable type, hence implicitly nonisolated, whereas PlayerContainerViewController's is a main-actor computed property.

Other findings

• ShowNotesPlayerItemViewController deinit deleted — safe (WKWebView.navigationDelegate is weak, so it self-zeroes; the manual nil-out was a UIWebView-era habit), but it's the one behavioural deletion in a PR described as annotation-only. Worth a line in the description; isolated deinit would preserve it verbatim if you'd rather.
• TranscriptViewController.swift:548 — MainActor.assumeIsolated is the better fit than Task { @MainActor } here, since the timer is explicitly added to RunLoop.main two lines down. Keeps the update synchronous and avoids a Task outliving debugTimer?.invalidate(). #if DEBUG-only, so take it or leave it.
• PlayerZoomAnimator.swift:499 — @MainActor init(window:) on the nested ScreenCornerRadiusCacheKey is the right call: nested types don't inherit the enclosing type's isolation, and the init reads window.traitCollection. Its only caller (screenCornerRadius(for:), :538) is in the main-actor extension, so no cascade.
• New protocol isolations all check out — NowPlayingActionsDelegate, PlayerItemContainerDelegate, PlayerTabDelegate, TimeSliderDelegate. Every conformer (NowPlayingPlayerItemViewController, TranscriptContainerViewController, PlayerContainerViewController, VideoViewController) is already main-actor via UIViewController, and every weak var reference lives on a UIView/UIViewController. No conformance breaks.

Still open from the previous pass

EpisodeCell's @objc notification handlers (uploadProgressDidUpdate, updateCell) touch self — including assigning self.episode at EpisodeCell.swift:528 — and hit the DB before their Thread.isMainThread hop, while three of the observed notifications are genuinely posted off-main. Unchanged in this push; detail is in the earlier inline comment.

Not verified

No Xcode on this Linux runner, so make build_staging / make test_staging weren't run — every claim above is from reading source, the call graph, and podcasts.xcodeproj build settings (SWIFT_VERSION = 5.0, SWIFT_APPROACHABLE_CONCURRENCY = YES, SWIFT_DEFAULT_ACTOR_ISOLATION = nonisolated). The SE-0423 trap in particular is reasoned, not observed.

One correction to my earlier pass: I called most of the class-level annotations no-ops. The @preconcurrency fixes this commit needed suggest they do shift diagnostics in this build configuration, so treat that as "redundant for isolation inference" rather than "changes nothing".

No security or performance concerns. Nothing blocking — the analytics guard is the only item I'd act on.
• branch kean/main-actor-playback-part3

[claude]

Claude finished @kean's task in 3m 48s —— View job

---

Review: @MainActor adoption Part 3 — commits 13a9b28, 0311d68, c06fc89

• Fetch diff for the three new commits
• Review c06fc89 Safer uploadProgressDidUpdateOnMain
• Review 0311d68 revert of PlayerContainerViewController isolation
• Review 13a9b28 MainActor.assumeIsolated in transcript debug overlay
• Post review

Reviewed only the three new commits — the Part-3 files from my earlier passes are otherwise unchanged. Two inline comments posted.

c06fc89 — the important half is fixed; the entry point still needs nonisolated

Genuinely better: episode = reloadEpisode() and its DB hit now happen on the main actor rather than on the URLSession delegate queue, and the guard let ourEpisode = episode as? UserEpisode now re-reads episode after the hop, which is also more correct under cell reuse than the old read-then-hop order.

What's still open is the entry point. EpisodeCell is @MainActor (EpisodeCell.swift:5) and uploadProgressDidUpdate doesn't opt out, so the compiler models it as only ever entered on the main actor — Thread.isMainThread is a tautology and the else branch is unreachable in its view. The actual off-main call (UploadProgressManager.swift:48 ← urlSession(_:task:didSendBodyData:…)) is the one it can't see, and with actor data-race checks on, the precondition sits in the @objc thunk and fires before either branch runs. One word fixes it:

@objc nonisolated private func uploadProgressDidUpdate() {

Smaller: the new Task { @MainActor in … } captures self strongly where the old DispatchQueue.main.async used [weak self], and independent Tasks aren't FIFO-ordered the way serial DispatchQueue.main.async enqueues are. Neither matters for this handler (the body re-reads current progress rather than applying a delta) — worth knowing before the pattern gets copied. Detail in the inline comment.

The sibling updateCell path is the same bug, untouched

Not a regression from this commit, but uploadProgressDidUpdate was one of a pair and its twin still has the original shape:

updateCellFromSpecificEvent (EpisodeCell.swift:438) observes two notifications that are genuinely posted off-main — userEpisodeUploadStatusChanged (UploadManager+URLSessionDelegate.swift:63, RetrieveFileUploadStatus.swift:31/:43) and episodeTypeOrLengthChanged (MetadataTask.swift:71) — and both it and updateCellFromGenericEvent read episode?.uuid before calling updateCell, which then does DataManager.sharedManager.findBaseEpisode(uuid:) and reads mainTintColor / playlistUuid / podcastUuid off-main before its own Thread.isMainThread hop (:446-458). Same treatment applies:

@objc nonisolated private func updateCellFromGenericEvent() {
    onMain { guard let episode else { return }; updateCell(episodeUuid: episode.uuid) }
}

@objc nonisolated private func updateCellFromSpecificEvent(_ notification: Notification) {
    guard let episodeUuid = notification.object as? String else { return }
    onMain {
        guard episodeUuid == episode?.uuid else { return }
        updateCell(episodeUuid: episodeUuid)
    }
}

…with updateCell losing its internal hop. Given both handlers now want the same Thread.isMainThread / assumeIsolated / Task preamble, a small nonisolated func onMain(_ body: @MainActor @escaping () -> Void) helper would keep it to one copy. Fine to defer to a follow-up if you'd rather keep this PR annotation-shaped — just noting that half of a matched pair is currently hardened. Fix this →

0311d68 — good scoping call, one clarification

Agreed that the AnalyticsEpisodeHelper off-main analyticsSource / UIKit reads are a real threading bug and belong in their own PR rather than being smuggled into an annotation sweep.

One thing worth being precise about: the revert doesn't make PlayerContainerViewController nonisolated — it still inherits @MainActor through SimpleNotificationsViewController → UIViewController, so analyticsSource remains a main-actor witness for a nonisolated protocol requirement. What dropping @preconcurrency removes is the SE-0423 dynamic check in the witness thunk, i.e. the trap, not the race. The ServerPodcastManager.subscribeQueue → autoDownloadLatestEpisodes → cacheDownloadSource → currentAnalyticsSource → topAnalyticsSourceProvider() path still walks the VC hierarchy off-main exactly as before. Since the trap returns the moment anything in that chain gets annotated, a tracking issue would be worth opening so the follow-up PR doesn't get discovered by a crash report instead.

13a9b28 — correct, two cosmetic leftovers

MainActor.assumeIsolated is the right call: the timer goes onto RunLoop.main two lines down, the update stays synchronous, and no Task can outlive debugTimer?.invalidate().

• TranscriptViewController.swift:556 is now a whitespace-only line (four spaces). .swiftlint.yml enables trailing_whitespace in only_rules (:131) and sets ignores_empty_lines: false (:152-154), so the :swift: SwiftLint Buildkite step will flag it. make format clears it.
• The #if DEBUG / #endif blocks in both willBeAddedToPlayer and willBeRemovedFromPlayer got de-indented to column 0, while the same file uses the indented form at line 273 — unnecessary churn in the diff.

Not verified

No Xcode on this Linux runner, so make build_staging / make test_staging / swiftlint weren't run. Every claim above comes from reading the source, the notification-posting call graph, .swiftlint.yml, .buildkite/pipeline.yml, and podcasts.xcodeproj build settings (SWIFT_APPROACHABLE_CONCURRENCY = YES, SWIFT_DEFAULT_ACTOR_ISOLATION = nonisolated, no explicit actor-data-race-check setting — so it follows the toolchain's Debug default). The @objc thunk precondition is reasoned from the isolation model, not observed.

No security or performance concerns. Nothing blocking; the missing nonisolated on uploadProgressDidUpdate is the one item I'd act on before merge, since without it the new branch is unreachable by construction.
• branch kean/main-actor-playback-part3

[claude]

Claude finished @kean's task in 1m 23s —— View job

---

Review: @MainActor adoption Part 3 — new commit d4bd8f6

• Fetch diff for the new commit
• Verify the SwiftLint whitespace fix
• Re-check open items from prior passes
• Post review

d4bd8f6 is one line: the whitespace-only line at TranscriptViewController.swift:556 is gone. Verified — no trailing whitespace remains in any of the 18 changed files, so the :swift: SwiftLint step should be clean on that rule. No inline comments this pass.

For the record on the other cosmetic note from my last review: .swiftlint.yml has no indentation_width rule (only_rules at :25), so the column-0 #if DEBUG / #endif in willBeAddedToPlayer (:546/:554) and willBeRemovedFromPlayer (:563/:566) won't be flagged. It's still inconsistent with the indented form the same file uses at :62, :273, and :912, but that's purely diff noise — take it or leave it.

Open items, for the record

Both are acknowledged in the thread, so noting status rather than re-arguing:

• uploadProgressDidUpdate entry point (EpisodeCell.swift:523) — still @objc private func with no nonisolated. The body fix in c06fc89 is the substantive half and it's correct; what remains is that with the class @MainActor (:5) and no opt-out, the compiler models the method as main-actor-only, so Thread.isMainThread is a tautology and the else branch is unreachable in its view — while UploadProgressManager.swift:48 still dispatches the selector from the URLSession delegate queue. Your call given the plan to move to NotificationCenter.MainActorMessage; worth knowing that until then the code reads as if it handles off-main entry but the compiler doesn't believe it can occur.
• updateCellFromGenericEvent / updateCellFromSpecificEvent (:432, :438) — unchanged; they still read episode?.uuid and hit DataManager.findBaseEpisode before updateCell's own Thread.isMainThread hop (:446-458), and userEpisodeUploadStatusChanged / episodeTypeOrLengthChanged are genuinely posted off-main. Reasonable to fold into the same MainActorMessage migration.
• PlayerContainerViewController / AnalyticsEpisodeHelper — deliberately deferred to its own PR per your reply. Worth an issue so the follow-up isn't found via a crash report once anything in that chain gets annotated.

Not verified

No Xcode on this Linux runner, so make build_staging / make test_staging / swiftlint weren't run. The whitespace claim is from grepping the working tree; the lint-rule claims are from reading .swiftlint.yml.

Nothing blocking. LGTM as it stands.
• branch kean/main-actor-playback-part3