External resource support for media-server items (Jellyfin / AudiobookShelf) - #1586
External resource support for media-server items (Jellyfin / AudiobookShelf)#1586GianniCarlo wants to merge 116 commits into
Conversation
✅ Claude PR Review —
|
…etch resolveExternalHosts + resolveHost merged into a single function: the two near-identical switch branches existed only because each provider decodes a different Decodable connection array from a different keychain key. A 4-line generic hostURL(for:key:of:keychain:) over IntegrationHostIdentifiable & Decodable (the protocol exposes url) is what lets one switch-expression cover both providers. ItemDetailsHostResolutionTests passes unchanged — same entry, same semantics. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YNhXd8EkXcrjBVYLHm5ZSp
item.externalResources carries every provider's rows, so a plain local file linked to Hardcover rendered an "External Resources" section repeating that same link as a bare provider/id pair with no host — directly under the Hardcover section that already shows it with a book picker. The rule denies hardcover rather than allowlisting jellyfin/audiobookshelf: this section is diagnostic detail, so a provider added later should appear in it instead of silently disappearing. It is a static helper for the same reason the host resolution below it is — testable without constructing the view model. resolveExternalHosts now resolves only what the section renders, so the map no longer holds an empty entry for the hardcover row. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YNhXd8EkXcrjBVYLHm5ZSp
The host row showed a bold "Host" label next to an empty string until resolution landed. The raw hostId is what the resolver itself falls back to for a host with no saved connection, so the view falls back to it too: the field starts on the raw value and upgrades in place to the server URL. No spinner — resolution is a local keychain read, so one would flicker for a frame rather than communicate anything. Hosts also resolve before the hardcover selection now. That path can await a GraphQL fetch, and it has a spinner plus an interim title to cover the wait; the host row has neither, so it should not be the one queued behind a network call. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YNhXd8EkXcrjBVYLHm5ZSp
Unused on develop too — the declaration was its only occurrence across ItemListView and its +Sheets/+Alerts/+ConfirmationDialogs extensions. Its jellyfinService twin on the line above went with this PR's refresh work, which builds throwaway connection services on purpose (ItemListViewModel :878-880) rather than pinning connections on the shared environment instances, so neither property has a reason to exist. Leaving exactly one behind was the only asymmetry. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YNhXd8EkXcrjBVYLHm5ZSp
The playback-failure alert keyed its Media Servers shortcut on `remoteURL == nil`, but the API returns a presigned URL for every synced item even when no object is behind it. That branch therefore only ever meant "this account doesn't sync": it offered the shortcut to free users on ANY missing file — plain local imports included — and withheld it from pro/lite users in the one case it was written for, a synced item on a device where its media server was never added. PlaybackService now records `hasUnresolvedExternalHost` where the host resolution already fails, and the shortcut requires the streaming entitlement plus a media-server resource that either failed to stream (expired token, server down) or can't resolve its host. Both alert sites share one decision function instead of a copied condition. The streaming pick also filters to media-server providers explicitly: hardcover links share the item's resource relationship and sort between "audiobookshelf" and "jellyfin", so one returning from sync as anything but not_synced would have won the pick and left a streamable item with no external URL at all. `hasUnresolvedExternalHost` stays out of PlayableChapter's CodingKeys alongside externalUrl/externalHeaders — it's a per-device resolution result, and the receiving side re-resolves against its own saved connections. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YNhXd8EkXcrjBVYLHm5ZSp
The five statics this PR accumulated existed because their enclosing types can't be cheaply constructed in a test — ItemDetailsViewModel's init does a CoreData read, touches the disk and launches a Task; PlayerManager's builds an AVPlayer with a time observer; PlaybackService news up a KeychainService inside the method. Hoisting each rule to a static was the cheap way to get it under test, and it showed in the signatures: `keychain:` and `fileExists:` are parameters no caller wants. Worse, two of them answered the same question two different ways, added two commits apart: a denylist in the details view model (not hardcover) and an allowlist in PlaybackService (jellyfin, audiobookshelf). Now `isMediaServer` on SimpleExternalResource is the single definition, and its switch has no `default`, so adding a provider is a compile error at the one place the question is answered. The streaming pick and the details filter both read from it. Host resolution moved to IntegrationHostResolver, which is already an all-static namespace and already owns connection(for:in:). The offer rule became PlayableChapter.needsMediaServer(), leaving PlayerManager a one-line instance method — and the entitlement wiring is now pinned against a real PlayerManager rather than a hoisted copy of the rule. ItemDetailsViewModel, PlayerManager and PlaybackService are left with no statics at all. Behavior note: the details section moves from denylist to allowlist, so a providerName this build doesn't know is no longer listed there. That's the trade for the exhaustive switch, which forces the decision at compile time instead of letting a new provider drift silently between the two sites. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YNhXd8EkXcrjBVYLHm5ZSp
Two things kept this view model out of tests entirely, which is why its rules kept getting hoisted into statics: init started an unstructured Task that reached the network, retained self, and could be neither cancelled nor awaited; and init's parameters took the concrete LibraryService/SyncService even though the stored properties were already protocol-typed, so no mock could be passed. The two resolves now live in load(), driven by the view's .task. SwiftUI owns the lifetime, so the work is cancelled on dismissal — a hardcover fetch can no longer land its stub repair after the sheet is gone, which closes the late-write hazard we'd set aside — and a test can await it instead of racing a task that construction started on its own. Deliberately no .task(id:). `item` is fixed at construction, so nothing should re-trigger this, and keying on the model's ObjectIdentifier is what makes a load re-fire on every model recreation. didLoad guards a re-appear and resets itself when the load was cancelled so it stays retryable; it only means anything because the view owns the model with @StateObject. The eager init work stays put: lastPlayedDate is a `let` behind a mockable protocol, and deferring selectedImage would trip onChange(of:) → artworkIsUpdated (ItemDetailsView:69) and re-upload artwork the user never edited. Three tests that were impossible before: the load runs once across repeated calls, the picker selection resolves from a synced-down resource and upgrades to the fetched metadata, and init resolves nothing. They need a token on the hardcover stub — ItemDetailsHardcoverSectionViewModel.init? is failable, and the whole resolve path is gated on that section existing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YNhXd8EkXcrjBVYLHm5ZSp
The handler writes playerState.isShowingPlayer and listState.activeIntegrationSheet to sequence two presentations that both belong to MainView — the sheet at :87 and the player cover at :99. listState is MainView's own @State, injected into the environment at :130 and read back out by LibraryRootView purely so a child could mutate its parent's property. Moving it makes the owner the mutator, and puts the ordering comment next to the two modifiers it's actually about instead of describing them from another file. It also drops a latent lifecycle assumption: LibraryRootView is tab content that stays mounted only because Library happens to be the default tab, while MainView is unconditionally alive. If tab content ever became lazier, the notification would go silently unhandled and the alert's Media Servers button would do nothing. Behavior is identical — same notification, same two writes, same order. The other five LibraryRootView listeners stay put: download errors, both folder watchers, the import operation publisher and the import bus are genuinely library-scoped. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YNhXd8EkXcrjBVYLHm5ZSp
The provider links were an HStack of Image+Text pairs, which cost three things. An interpolated Image is sized from the font, so the provider icon now grows with Dynamic Type instead of sitting at a fixed 12pt next to text that scales. SwiftUI applies its font-derived default stack spacing only between adjacent Text views. The HStack child forfeited that, which is why the VStack needed a hardcoded `spacing: 2` to keep row height matching develop — with all three children back to Text the default returns, and it scales too. And the subtitle now truncates once at the end, instead of the icon, provider name and author competing for width with no lineLimit anywhere in the row and truncating independently. Both accessibilityHidden calls went with it: they were already inert, since the row is a single accessibility element (children: .ignore + dynamicAccessibilityLabel) and no child reaches VoiceOver. Note the provider is still never announced — the row label is composed from title/details/progress in VoiceOverService. That's a real gap, but closing it means changing VoiceOverService, so it stays a follow-up. No automated verification exists for this (XCTest only, no snapshot infrastructure), so the visual side rides the device pass: the row at default and accessibility text sizes, and a long author name truncating once. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YNhXd8EkXcrjBVYLHm5ZSp
10494d2 to
0781028
Compare
The concatenated subtitle reduced over every externalResource, so a Hardcover-linked book showed a "Hardcover •" badge in the list while the details section filtered to mediaServerResources. Hardcover is progress-sync, not a source the book streams from, so it earns no badge next to the author. This reintroduced in BookView the exact two-definitions split that the isMediaServer consolidation had just removed everywhere else — the shared accessor already existed and I didn't use it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YNhXd8EkXcrjBVYLHm5ZSp
The label was theme.primaryColor — the app's main foreground color — on a theme.linkColor background, and it was set twice: once on the Text and again on the HStack. develop's Download button that this replaced used systemBackgroundColor on linkColor, set once so it colors the SF Symbol too. primaryColor on the saturated accent is a likely contrast regression and breaks the app's primary-button convention. The sibling secondary button just above legitimately keeps primaryColor, since it sits on tertiarySystemBackgroundColor. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YNhXd8EkXcrjBVYLHm5ZSp
The inbound half of media-server progress sync lived on ItemListViewModel, behind PlaybackSyncProgressDelegate — a protocol that exists so the player can ask the LIST whether it is mid-fetch. That let a UI object's identity decide whether the feature ran at all: CarPlayManager claims the same single delegate slot and answered with an empty implementation, so a car-only session never pulled. Meanwhile the outbound half has always been a proper service (progressUpdatePublisher → SyncService.scheduleMetadataUpdate → the externalUpdate queue), and it handles both providers. This adds the service, a provider-neutral ExternalPlaybackProgress value, and a two-line protocol seam that the concrete @mainactor connection services never had — which is what makes any of this testable at all. It asks every linked server concurrently and ignores the ones that fail, then applies one rule: a candidate must be ahead of local by more than 15s (newer date or farther position), and the newest date wins. That is the same strictly-newer comparison SyncService.handleSyncedLastPlayed and handleSyncFromExternalResource already use for our own cloud, and it replaces two copies of the threshold logic. One refresh in flight, replaced per playback start, and the answer is discarded when the item is no longer the one requested. Keying tasks by uuid meant a slow answer for a book the user had left could raise a prompt whose "resume" seeks whatever is playing now to another book's timestamp. The result is published rather than written into UI state, so the SwiftUI alert and CarPlay's own two-action alert can share one decision. Additive only — nothing consumes the publisher yet. The deletions and the wiring land next. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YNhXd8EkXcrjBVYLHm5ZSp
ItemListViewModel loses activeTasks and the 68-line fetchExternalResource, and PlaybackSyncProgressDelegate is back to develop's single member — the question it exists to answer, which is whether the LIST is mid-fetch. The service self-subscribes to .bookPlayed, which PlayerManager posts for every playback start. So a CarPlay-only session now pulls media-server progress exactly like the phone; previously CarPlay claimed the one delegate slot and answered with an empty implementation, which silently disabled the feature for that whole session. MainCoordinator bridges the published position into playerState, because PlayerState is app-layer and a Shared service must not reach it. The split lands where it should: the service decides whether a remote position is worth offering, the coordinator decides where the offer goes. Deleted along the way: two copies of the 15s threshold comparison, two throwaway connection services constructed on every playback start (including for plain local books, which have no media server to ask), and the uuid-keyed task dictionary that let an abandoned book's answer raise a prompt. CarPlay still doesn't present the offer — that's next. It already presents multi-action alerts, so the empty stub was never an API limitation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YNhXd8EkXcrjBVYLHm5ZSp
The list refresh collected only jellyfin resources and handed them to an ingest typed to [String: JellyfinLibraryItem], with the provider name hardcoded in its CoreData predicate. So an ABS item's position never came back from its server — not because of an API gap (the push has always handled ABS, and ABS has the batch primitive) but because the caller couldn't express it. That ingest only ever used the position, the date and the finished flag, so it is now provider-neutral over ExternalPlaybackProgress and filters on the provider name it's given. The per-server grouping and per-provider batching moved into the service, where both providers get identical treatment. updateFromResource drops from 32 lines to one delegating call. The view model takes the service through a new @entry environment key, injected for real by MainCoordinator — the @entry default is a throwaway, as the repo documents. The Sourcery mock was regenerated for the protocol change rather than hand-edited. Tests: the two existing ingest tests move to the neutral fixture (and get shorter), plus a new one pinning that the same providerId under a different provider name cannot move the row — provider ids only mean something to the server that issued them. Behavior change: ABS items now update on list load where they previously never did. Worth confirming on the device pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YNhXd8EkXcrjBVYLHm5ZSp
The car half of the pull is no longer silent. Since the pull moved off the delegate it already runs in a car-only session, but the answer had nowhere to go — a server reporting that another device got further did nothing at all. CarPlay has always been able to present multi-action alerts, and showAlert already maps BPActionItems onto CPAlertActions, so the empty delegate stub was never an API limitation. It was a decision nobody made. Presented only when the app isn't foregrounded: an active app shows the SwiftUI alert, and asking twice for one decision is worse than either surface asking once. Answering in the car clears the shared flag, so the phone doesn't re-ask a question the driver already answered. No new strings — this reuses the resume alert's existing keys. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YNhXd8EkXcrjBVYLHm5ZSp
getPlayableChapters built media-server stream URLs and auth headers inline, per provider, from a KeychainService() it constructed itself. So nothing about which server an item streams from, which token authorises it, or the unresolved-host flag could be tested without a real keychain — which is why that flag only ever had a pure-function test. Resolution is now ExternalStreamResolver behind a protocol, injected through setup with a default. watchOS keeps its one-argument call and correctly resolves nothing, having no saved media-server connections. 52 lines leave PlaybackService. The tests this finally allows cover the contracts that mattered and had none: an unmatched host resolves to nothing rather than falling back to another server (the rule shared with the Android app), the integration's Authorization beats a user-configured lowercase authorization header from a reverse-proxy config, and hasUnresolvedExternalHost end-to-end — the flag the Media Servers shortcut depends on. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YNhXd8EkXcrjBVYLHm5ZSp
promptablePosition was a static on the service — the sixth this PR accumulated, and the only one with no excuse. It touches no instance state, had a single internal call site, and ExternalProgressService() is a free init, so unlike ItemDetailsViewModel or PlayerManager there was no construction problem to work around. The rule reasons about ExternalPlaybackProgress values and nothing else, so it belongs on the collection, next to the type it compares — the same move that put isMediaServer on SimpleExternalResource. The call site now reads candidates.promptable(localTime:localDate:), the service has no statics left, and five tests exercise a value-type extension with no service in sight. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YNhXd8EkXcrjBVYLHm5ZSp
Two subscribers each applied half a rule. CarPlay presented when the app was inactive, but MainCoordinator had already raised the phone's flag — so a driver who ignored the car alert was asked the same question again on unlocking the phone. ResumeOfferArbiter is now the single subscriber. An offer belongs to whichever surface receives it and the other never sees it, so no flag is left behind to fire later. It's created eagerly beside PlayerState rather than with CoreServices, because CarPlay registers itself from connect(), which on a cold launch into the car runs before the services exist. The subscription this replaces lived on CarPlayManager.init and resolved coreServices? at subscription time — so on exactly that path it silently never bound, and the resume offer was dead for the whole session. That is the same failure class the extraction set out to remove. Registration and subscription are decoupled so ordering can't matter. Closes the reviewer's WARN on 8c2835a and the double-prompt gap. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YNhXd8EkXcrjBVYLHm5ZSp
…reaming does downloadRemoteFiles picked the resource with an unordered, provider-unfiltered first(where: syncStatus != notSynced) and then rebuilt the stream URL and auth headers inline — a third copy of the rule ExternalStreamResolver now owns. The pick was also a real bug. The API's markExternalSourceUploaded marks EVERY provider row for an item 'downloaded' — its own comment predicts "confirming one upload marks every provider 'downloaded'". So on a book linked to both Jellyfin and Hardcover, the Hardcover row passes the syncStatus check after a cloud upload, sync-down copies that status to the second device, and depending on NSSet iteration order the pick lands on Hardcover: default: break, no URL, and the function throws integration_error_missing_connection on a device whose Jellyfin connection is fine. Hardcover has no files and can never be a source. syncStatus was a sync-state question being used to answer a capability question; streamingResource asks the capability question directly, and the resolver hands the download the exact URL and headers the stream uses. 56 lines leave SyncService. Server-side, markExternalSourceUploaded should scope to the uploaded provider. That's a separate api ticket. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YNhXd8EkXcrjBVYLHm5ZSp
…ication center refreshProgress created its Task outside the critical section and stored it in a second one. Two interleaved calls could leave refreshTask pointing at the task it had just cancelled while the live one ran unreferenced — publishing stayed correct only through the isStillRequested check, and teardown would have missed the live task. It is created and stored under one lock now; the body only takes the lock after its first await, so holding it across creation is safe. The notification center is injected so tests can post to a private one. PreferencesSyncService observes .logout in production and real instances exist in the test target, so posting to .default from a unit test could reach a fixture that isn't ours. The suite's fixed Task.sleep waits became expectations fulfilled by the publisher, and the slow stub no longer swallows cancellation — so the teardown, logout and superseded-item tests now prove the refresh was cancelled, rather than that its late answer happened to be discarded. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YNhXd8EkXcrjBVYLHm5ZSp
playerState in ItemListViewModel lost its only reader when fetchExternalResource moved to the service, yet was still threaded through both LibraryRootView constructions — the same class as the audiobookshelfService property removed earlier. PlaybackService.streamResolver had two defaults; the property one was always overwritten by setup, so it's an IUO now like its sibling. A resolved connection that can't build a stream URL is a defect, not a missing server. The resolver logs it, and the flag's comment says what it actually means instead of what it usually means. Reviewer INFO on 8c2835a plus two self-review items. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YNhXd8EkXcrjBVYLHm5ZSp
…inView "Bus" implied many topics in many directions. This is one typed event, one way — a set of events you subscribe to — and confirmedBatches completes the sentence. It also lived as a second type inside ExternalImportViewModel.swift; it has its own file now. MainCoordinator only ever created and injected it, never called it, while its producers (the Jellyfin and ABS root views, presented through MainView's media-servers sheet) and its consumer (LibraryRootView) are all SwiftUI views under MainView. So MainView owns it as a @StateObject and injects it beside theme. Sheets inherit their presenting view's environment, so every party still gets it, and no other SwiftUI root injects it. listSyncRefreshService and singleFileDownloadService stay coordinator-built on purpose: they are services with dependencies, and CLAUDE.md names the coordinator as their builder. A view assembling services out of coreServices would invert that. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YNhXd8EkXcrjBVYLHm5ZSp
| let error = syncOperation.error { | ||
| Self.logger.error("Sync task failed: \(error.localizedDescription)") | ||
| await MainActor.run { | ||
| self.lastSyncError = SyncErrorInfo( |
There was a problem hiding this comment.
🔵 INFO — The operation completion block's failure branch does not distinguish a cancelled op from a genuine failure. LibraryItemSyncOperation.cancel() sets error = BookPlayerError.cancelledTask, so a routine logout (cancelAllOperations()) or pro→lite downgrade (cancelServerQueueOperations()) leaves an in-flight sync op reporting didSucceed == false, and this block then logs "Sync task failed" and writes a spurious lastSyncError ("task was cancelled"). It only surfaces in the debug diagnostics export (DebugFileTransferable), not user-facing UI, hence low severity. Fix: skip the lastSyncError assignment for cancellations, e.g. if let syncOperation = operation as? LibraryItemSyncOperation, !syncOperation.isCancelled, let error = syncOperation.error, !(error is BookPlayerError) || ... — simplest is to guard !operation.isCancelled before setting lastSyncError.
Supersedes #1523 — @Hirobreak's External Resource work, squash-rebased onto current develop (the original fork branch has maintainer edits disabled and had diverged ~70 commits from its July base). Feature authorship is preserved on the squash commit.
What this adds
Cross-device sync of media-server books: an item imported from Jellyfin/AudiobookShelf carries an
ExternalResource(provider, provider item id,hostId) that round-trips through the BookPlayer API — a second device resolves the resource to its own saved connection and streams directly from the user's server. Progress pushes back to the media server run on the new unifiedConcurrenceServicetask queue (SwiftData schema v3), and connections now capture the server's stable GUID at sign-in (all three flows: Jellyfin password, Quick Connect, ABS login). CoreData migrates v11→v12 with a mapping model. The LITE tier lands on iOS (AccessLevel.lite,hasSyncEnabled = pro || lite) with per-job access gating.The cross-platform hostId contract (Android shipped this in 1.1.3)
hostId := <server-reported GUID> ?: canonicalDedupKey(url); resolution is provider-scoped: GUID match (case-insensitive) → canonical URL key → nil, never guess a server. Unresolvable playback surfaces the Media Servers shortcut; unresolvable progress pushes are consumed/discarded (an unknown host is permanent on-device, and the queue retries failures forever).On top of the rebase, this branch fixes review findings from comparing against the Android implementation
IntegrationHostResolverreplacing per-site lookups that compared GUIDs case-sensitively against rawabsoluteStringand then fell back toconnections.first— cross-device that streams the wrong file or writes progress to the wrong server.hostIdwrites usestableHostId(serverId ?? canonicalDedupKey) at all six import sites.PlayableChapterno longer encodesexternalUrl/externalHeaders: the headers carry the media server's liveAuthorizationtoken, and encodedPlayableItems reach the WatchConnectivity application context, which is persisted to disk on both devices.externalUrl != nil, hiding it in exactly the connect-your-server case).externalUpdateis available on every tier (Android parity — the push targets the user's own server, not a billed resource).Verification
Phone + watch schemes build; 530 unit tests, 0 failures. Browser-SSO stack (
WebAuthenticating/OIDC/PKCE) is guarded iOS-only in the shared framework; its window lookup uses a dynamicsharedApplicationread because BookPlayerKit compiles extension-safe.🤖 Generated with Claude Code
https://claude.ai/code/session_01YNhXd8EkXcrjBVYLHm5ZSp