Skip to content

External resource support for media-server items (Jellyfin / AudiobookShelf) - #1586

Open
GianniCarlo wants to merge 116 commits into
developfrom
external-resource-rebased
Open

External resource support for media-server items (Jellyfin / AudiobookShelf)#1586
GianniCarlo wants to merge 116 commits into
developfrom
external-resource-rebased

Conversation

@GianniCarlo

Copy link
Copy Markdown
Collaborator

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 unified ConcurrenceService task 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

  • Centralized IntegrationHostResolver replacing per-site lookups that compared GUIDs case-sensitively against raw absoluteString and then fell back to connections.first — cross-device that streams the wrong file or writes progress to the wrong server.
  • Progress pushes to an unresolvable host are discarded-by-consuming rather than sent to the active connection (original) or retried forever (naive discard).
  • hostId writes use stableHostId (serverId ?? canonicalDedupKey) at all six import sites.
  • PlayableChapter no longer encodes externalUrl/externalHeaders: the headers carry the media server's live Authorization token, and encoded PlayableItems reach the WatchConnectivity application context, which is persisted to disk on both devices.
  • The error alert's Media Servers shortcut now appears when no source resolves (previously gated on externalUrl != nil, hiding it in exactly the connect-your-server case).
  • externalUpdate is 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 dynamic sharedApplication read because BookPlayerKit compiles extension-safe.

🤖 Generated with Claude Code

https://claude.ai/code/session_01YNhXd8EkXcrjBVYLHm5ZSp

@GianniCarlo GianniCarlo mentioned this pull request Aug 23, 2026
Comment thread BookPlayer/Player/PlayerLoaderService.swift Outdated
Comment thread BookPlayer/Player/PlayerManager.swift Outdated
Comment thread BookPlayer/Player/PlayerManager.swift Outdated
Comment thread Shared/CoreData/CoreDataStack.swift Outdated
Comment thread Shared/CoreData/CoreDataStack.swift Outdated
Comment thread Shared/Services/ConcurrentSync/FileUploadOperation.swift Outdated
Comment thread Shared/Services/PlaybackService.swift Outdated
Comment thread Shared/Services/Sync/SyncService.swift Outdated
Comment thread Shared/Services/ConcurrentSync/ConcurrenceService.swift Outdated
Comment thread Shared/Services/ConcurrentSync/AsyncOperation.swift Outdated
@github-actions

github-actions Bot commented Aug 23, 2026

Copy link
Copy Markdown

✅ Claude PR Review — PASS

Large feature PR (196 files, +13.6k) adding external-resource support so Jellyfin/AudiobookShelf items can stream and sync progress, plus a SwiftData sync-queue refactor (SyncTasksStorage → ConcurrentTasksRepository/ConcurrenceService) and a review-harness JSON-repair tweak. Reviewed across five high-risk clusters: concurrency/threading — SwiftData ModelContext stays confined to the ConcurrentTasksRepository ModelActor and no CoreData NSManagedObject escapes a service (Simple*/Playable* snapshots used); player & AVAudioSession lifecycle — the new remote-streaming path re-binds the chapter subscription after every currentItem swap, re-adds the periodic time observer on recreation, and balances KVO; persistence — the v12 CoreData migration (ExternalResource entity + relationship) executes the full manual 5-step ritual, and MigrationPlan.injectedCoreDataContext is set before the container is built; auth/entitlement — sync=pro|lite and streaming delegates to it, upload stays pro-only while externalUpdate is all-tier, all re-checked at execution and validated server-side; the Keychain JWT is never attached to third-party/S3 URLs and media-server Authorization headers are excluded from Codable/WatchConnectivity persistence. Only one low-severity cosmetic issue found; no blocking defects.

Findings: 1 info

Model claude-opus-4-8 · run log · 1 new · 0 carried over · 1 resolved · advisory (a human should still review). Duplicate findings are de-duplicated and stale ones auto-resolved across pushes.

Comment thread BookPlayer/Player/PlayerManager.swift
Comment thread Shared/Services/Sync/LibraryItemSyncOperation.swift Outdated
Comment thread Shared/Services/Sync/LibraryItemSyncOperation.swift Outdated
Comment thread Shared/Services/Sync/SyncService.swift Outdated
Comment thread Shared/Services/LibraryService.swift Outdated
Comment thread BookPlayer/Coordinators/MainCoordinator.swift
Comment thread Shared/Services/ConcurrentSync/FileUploadOperation.swift
Comment thread BookPlayer/Player/Views/PlayerView.swift Outdated
Comment thread BookPlayer/Import/ExternalSource/ExternalImportView.swift Outdated
Comment thread BookPlayer/Profile/Account/AccountPerksSectionView.swift
Comment thread BookPlayer/Base.lproj/Localizable.strings Outdated
Comment thread Shared/Services/ConcurrentSync/ExternalUpdateProgressOperation.swift Outdated
Comment thread Shared/Jellyfin/Network/JellyfinError.swift Outdated
Comment thread BookPlayer/Import/ImportManager.swift Outdated
Comment thread Shared/Services/ConcurrentSync/FileUploadOperation.swift Outdated
Comment thread Shared/Services/LibraryService.swift Outdated
Comment thread Shared/Services/ConcurrentSync/FileUploadOperation.swift
Comment thread BookPlayer/Library/ItemList/ItemListViewModel.swift Outdated
GianniCarlo and others added 9 commits September 9, 2026 14:05
…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
@GianniCarlo
GianniCarlo force-pushed the external-resource-rebased branch from 10494d2 to 0781028 Compare September 9, 2026 19:10
Comment thread BookPlayer/Library/ItemList/Views/BookView.swift Outdated
GianniCarlo and others added 8 commits September 9, 2026 19:07
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
Comment thread BookPlayer/Services/CarPlayManager.swift Outdated
Comment thread BookPlayer/Library/ItemList/ItemListViewModel.swift
GianniCarlo and others added 4 commits September 11, 2026 09:40
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
Comment thread Shared/SwiftData/TasksDataManager.swift
…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(

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants