From 11fd606130a5e14e4943c16779e646b6864d7ba0 Mon Sep 17 00:00:00 2001 From: Alex Axthelm Date: Tue, 15 Sep 2026 10:03:56 +0200 Subject: [PATCH 01/38] add PLAN --- PLAN.md | 195 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 195 insertions(+) create mode 100644 PLAN.md diff --git a/PLAN.md b/PLAN.md new file mode 100644 index 0000000..441164b --- /dev/null +++ b/PLAN.md @@ -0,0 +1,195 @@ +# Download Manager (Phase 0) + +## Context + +Pollux's Phase 0 foundation is nearly complete: feed fetch → parse → save, the +library, subscribe flow, and subscription details with an episode list all work. +The `Episode` schema already carries `download_status`, `download_progress`, +`file_size_bytes`, and `local_path` (see `shared/src/domain/episode.rs`), and the +DB persists them (`iOS/Pollux/DatabaseManager.swift`), but **nothing drives those +fields** — the download control in `EpisodeRow.swift` is an inert DEBUG-tinted +stub. This change implements the download manager: a serial queue in the core, a +new `Download` shell capability that actually fetches and stores audio files, and +wiring the episode-row control so a user can download an episode, watch its status, +and delete the local file. + +Scope decisions (confirmed with the user): +- **Progress: status-only for the first pass**, but the capability and shell are + structured so live 0–100% progress can be added *before this PR merges* without + a rewrite (a `Progress` result variant + a URLSession delegate seam are stubbed + in from the start). +- **Storage soft-cap: deferred.** The user-configurable cap depends on the Settings + screen (not built yet). We build the pre-download check as a seam that defaults to + *unlimited*, and rely on real device-full write errors for fail-on-full. The + configurable cap lands when Settings does. +- **Dedicated Downloads/Storage screens are Phase 5**, not this change. This change + makes the per-episode control functional; a queue screen is a follow-up. + +Roadmap item: Phase 0 — "Download manager: queue, serial downloads for MVP, storage +limit check before download, fail-on-full behavior" (`docs/ROADMAP.md`). +Feature specs: `docs/features/storage.md`, `docs/features/episode.md`. + +--- + +## Approach + +Follow the existing capability pattern exactly (mirror `capabilities/http.rs` and +its shell handler in `core.swift`). Downloads are shell I/O; the core only tracks +queue + status and persists transitions through the existing `Storage` capability. + +### 1. New `Download` capability (Rust) + +`shared/src/capabilities/download.rs` (new), registered in +`shared/src/capabilities/mod.rs`: + +```rust +pub enum DownloadOperation { + Download { episode_id: String, url: String }, + Delete { local_path: String }, +} + +pub enum DownloadResult { + // Present from day one so adding live progress later is additive, not a + // signature change. Unused by the core in the first pass. + Progress { episode_id: String, percent: u8 }, + Completed { local_path: String, size_bytes: u64 }, + Deleted, + Error(String), +} +impl Operation for DownloadOperation { type Output = DownloadResult; } +``` + +Add `Download(DownloadOperation)` to the `Effect` enum in `shared/src/effect.rs` +(same shape as the existing `Http`/`Storage` arms). + +### 2. Core: queue + serial execution (`shared/src/app.rs`, `model.rs`) + +- **Model** (`model.rs`): add `download_queue: Vec` (episode ids waiting) + and `downloading: Option` (the one in flight — enforces *serial*). +- **Events** (`app.rs`): `DownloadEpisode(String)`, `DeleteDownload(String)`, + `DownloadFinished { episode_id, result: Box }`. +- **`DownloadEpisode`**: the pre-download **limit-check seam** lives here + (`fn download_allowed(model) -> bool`, returns `true` today = unlimited). If the + episode is already downloading/queued, no-op. Otherwise mark it `Queued` + (persist via `StorageOperation::UpsertEpisode` or a narrower status update — see + note below), push to queue, and call `maybe_start_next`. +- **`maybe_start_next`**: if `downloading.is_none()` and the queue is non-empty, + pop the front, set `downloading`, mark it `Downloading`, and emit a + `DownloadOperation::Download` request whose resolution sends + `DownloadFinished`. +- **`DownloadFinished`**: on `Completed`, set status `Downloaded` + store + `local_path`/`file_size_bytes`; on `Error`, set status `Failed`. Either way clear + `downloading` and call `maybe_start_next` (serial drain). Persist the transition. +- **`DeleteDownload`**: emit `DownloadOperation::Delete { local_path }`, and on the + result reset the row to `NotDownloaded` with `local_path`/`size`/`progress` + cleared, persisted. +- Reuse the existing selection-correlation discipline already in + `EpisodesLoaded`: after any status change to an episode of the currently-selected + feed, the details list must reflect it. **This is the exact hazard flagged at + `app.rs:107` / `subscription.md:73`** — `SelectSubscription` is idempotent, so a + status write must update `model.episodes` in place (preferred) rather than relying + on re-selection. Update the matching entry in `model.episodes` directly in the + event handlers so `view()` reprojects it. + +**Storage op note:** there is no targeted "update download status" storage op yet +(only `UpdatePlaybackStatus`). Add `StorageOperation::UpdateDownloadState { episode_id, +status, local_path, size_bytes, progress }` mirroring `UpdatePlaybackStatus`, and +implement it in `DatabaseManager.swift` (a single `UPDATE`). Prefer this over +re-upserting the whole episode. + +### 3. Shell: Download handler (`iOS/Pollux/core.swift` + new file) + +- Add a `case let .download(operation)` arm in `processEffect` (mirror `.http`). +- New `iOS/Pollux/DownloadManager.swift`: performs the URLSession download to a + file in Application Support (reuse the `DatabaseManager` directory convention — + a `Downloads/` subdir; store a **relative** `local_path` so it survives container + path changes, resolving to absolute at read time), returns + `.completed(localPath:sizeBytes:)` or `.error`. `Delete` removes the file + (idempotent if already gone). Structure the download with a + `URLSessionDownloadDelegate` so the `didWriteData` progress callback is the single + seam where live-% reporting gets added before merge (first pass: ignore it). +- Add `resolveAndDispatch(_:result: DownloadResult)` overload (mirror the Http one). +- Serial execution is enforced by the **core** (`downloading` guard), so the shell + handler stays a simple one-shot per request. + +### 4. UI wiring (`iOS/Pollux/EpisodeRow.swift`, `EpisodeDetailView.swift`) + +- Replace the stubbed download placeholder in `placeholderControls` with a real + control driven by `episode.downloadStatus`: + - `notDownloaded` → download button → `core.update(.downloadEpisode(id))` + - `queued` / `downloading` → progress/indeterminate indicator (the existing + `downloadBadge` already renders these states; keep the badge, make the control + reflect activity) + - `downloaded` → "downloaded" affordance with a delete action → + `core.update(.deleteDownload(id))` + - `failed` → retry → `core.update(.downloadEpisode(id))` +- `EpisodeRow` takes only `episode: EpisodeSummary` today; thread the `core` (or an + action closure) in the same way `SubscriptionDetailScreen` already passes `core`. +- Keep the play/more-actions stubs as-is (out of scope). +- No standalone Downloads screen in this change (Phase 5). + +### 5. Codegen + tests + +- Regenerate Swift types after the Rust changes: `make ios-build` (runs `typegen` + + `package` + `generate-project`). Required whenever `Event`/`Effect`/capability + enums change. +- **Rust unit tests** in `app.rs` (follow the existing `mod tests` style): enqueue + → `Downloading` + one `Download` effect; a second enqueue while one is in flight + stays `Queued` and emits no new download effect (serial); `Completed` transitions + to `Downloaded`, stores path/size, and starts the next queued item; `Error` + transitions to `Failed` and still drains the queue; delete resets to + `NotDownloaded`; a status change to a selected-feed episode is reflected in + `view().subscription_detail` without re-selection. +- **Swift tests** (`iOS/PolluxTests/`): a `DatabaseManagerTests` case for the new + `UpdateDownloadState` op (round-trips status + path + size). A focused + `DownloadManager` test for the delete path is optional if network mocking is + heavy — keep to what's cheap. + +--- + +## Critical files + +- `shared/src/capabilities/download.rs` — **new** capability +- `shared/src/capabilities/mod.rs` — register it +- `shared/src/capabilities/storage.rs` — add `UpdateDownloadState` +- `shared/src/effect.rs` — add `Download` effect arm +- `shared/src/model.rs` — `download_queue`, `downloading` +- `shared/src/app.rs` — events, queue drain, in-place episode status updates, tests +- `iOS/Pollux/core.swift` — `.download` effect arm + resolve overload +- `iOS/Pollux/DownloadManager.swift` — **new** shell downloader (delegate seam) +- `iOS/Pollux/DatabaseManager.swift` — implement `UpdateDownloadState` +- `iOS/Pollux/EpisodeRow.swift` / `EpisodeDetailView.swift` — functional control +- `iOS/PolluxTests/DatabaseManagerTests.swift` — new-op coverage + +Reuse: the `HttpOperation`/`updatePlaybackStatus` patterns are the templates for +the capability and the storage op respectively; `DebugStyle`/`.stubbed()` is what's +being *removed* from the download control. + +--- + +## Verification + +1. `make test` — Rust core tests (`cargo test`) + iOS tests. New queue/status tests + must pass. +2. `make check` — `cargo check/clippy -D warnings/fmt --check/--locked`, swiftlint + `--strict`, swiftformat `--lint`. CI runs the same; clippy warnings and the + admin forbidden-pattern scan (no force-unwrap/try, no `unsafe`) will fail the + build otherwise. +3. `make ios-build` — confirm codegen succeeds and the generated Swift `Event`/ + `Request` types include the new download variants. +4. Manual on simulator (`make ios-sim`, or the iOS Simulator MCP — pick the newest + `Pollux.app` by mtime per the DerivedData memory note): subscribe to a real feed, + open a subscription, tap download on an episode → status goes + Queued → Downloading → Downloaded; queue a second while the first runs → it + waits (serial); delete a download → returns to Not downloaded; kill and relaunch + → downloaded episodes still read as Downloaded (persisted). Use `NSLog` (not + `print`) for any temporary logging. + +## Explicit follow-ups (not in this change) + +- Live 0–100% progress via the stubbed URLSession delegate seam + `Progress` result + variant (**intended before this PR merges**, per the user). +- User-configurable storage soft-cap + the pre-download limit check made real (needs + the Settings screen). +- Dedicated Downloads queue screen and Storage usage breakdown (Phase 5). +- Cold-start re-enqueue of episodes left in `Queued` status. From c3c197bca639beb4fe673dc202ec8095672f3787 Mon Sep 17 00:00:00 2001 From: Alex Axthelm Date: Thu, 17 Sep 2026 10:25:34 +0200 Subject: [PATCH 02/38] feat: download episodes with a serial download queue MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add a download manager: episodes can be queued for download, fetched one at a time, and deleted. The core owns the queue and status transitions; a new Download shell capability performs the file I/O. - shared: Download capability (fetch/delete), a Download effect, and an UpdateDownloadState storage op. The core tracks a serial queue (download_queue + downloading) and drives NotDownloaded -> Queued -> Downloading -> Downloaded/Failed, persisting each transition and updating the in-memory episode so the details page reflects it without a reload. - iOS: DownloadManager fetches to Application Support/Downloads (stores a relative path) with an idempotent delete; core.swift handles the Download effect; DatabaseManager implements UpdateDownloadState (row mapping split into DatabaseManagerRows.swift to stay under the file-length limit); the episode row and detail page get a working download / delete / retry control. Status-only for now (no live percentage) — the capability and the URLSession seam are structured so live progress is an additive change. The storage soft-cap is deferred to the Settings screen; device-full surfaces as a failed download. Co-Authored-By: Claude Opus 4.8 --- iOS/Pollux/DatabaseManager.swift | 181 ++------ iOS/Pollux/DatabaseManagerRows.swift | 146 ++++++ iOS/Pollux/DownloadManager.swift | 134 ++++++ iOS/Pollux/EpisodeDetailView.swift | 60 ++- iOS/Pollux/EpisodeRow.swift | 59 ++- iOS/Pollux/SubscriptionDetailScreen.swift | 8 +- iOS/Pollux/core.swift | 22 + iOS/PolluxTests/DatabaseManagerTests.swift | 49 ++ shared/src/app.rs | 507 ++++++++++++++++++++- shared/src/capabilities/download.rs | 44 ++ shared/src/capabilities/mod.rs | 1 + shared/src/capabilities/storage.rs | 13 +- shared/src/effect.rs | 2 + shared/src/model.rs | 8 + 14 files changed, 1077 insertions(+), 157 deletions(-) create mode 100644 iOS/Pollux/DatabaseManagerRows.swift create mode 100644 iOS/Pollux/DownloadManager.swift create mode 100644 shared/src/capabilities/download.rs diff --git a/iOS/Pollux/DatabaseManager.swift b/iOS/Pollux/DatabaseManager.swift index 06339fb..78c9aa8 100644 --- a/iOS/Pollux/DatabaseManager.swift +++ b/iOS/Pollux/DatabaseManager.swift @@ -82,6 +82,9 @@ actor DatabaseManager { @discardableResult func execute(_ operation: StorageOperation) async throws -> StorageResult { + // Split by domain so neither dispatcher trips the cyclomatic-complexity + // limit as operations are added. Subscription/feed ops here; episode ops in + // `executeEpisode`. switch operation { case .listSubscriptions: try listSubscriptions() @@ -91,6 +94,17 @@ actor DatabaseManager { try await upsertSubscription(sub) case let .deleteSubscription(id): try await deleteSubscription(id: id) + case let .upsertFeedWithEpisodes(subscription, episodes): + try await upsertFeedWithEpisodes(subscription: subscription, episodes: episodes) + default: + try await executeEpisode(operation) + } + } + + /// Episode-scoped storage operations. `execute` routes everything it does not + /// handle here; the `default` is unreachable (every remaining case is covered). + private func executeEpisode(_ operation: StorageOperation) async throws -> StorageResult { + switch operation { case let .upsertEpisode(episode): try await upsertEpisode(episode) case let .getEpisode(id): @@ -101,8 +115,13 @@ actor DatabaseManager { try getEpisodeByFeedGuid(subscriptionId: subscriptionId, feedGuid: feedGuid) case let .updatePlaybackStatus(episodeId, status, positionSecs): try await updatePlaybackStatus(episodeId: episodeId, status: status, positionSecs: positionSecs) - case let .upsertFeedWithEpisodes(subscription, episodes): - try await upsertFeedWithEpisodes(subscription: subscription, episodes: episodes) + case let .updateDownloadState(episodeId, status, localPath, sizeBytes, progress): + try await updateDownloadState( + episodeId: episodeId, status: status, localPath: localPath, + sizeBytes: sizeBytes, progress: progress, + ) + default: + fatalError("executeEpisode received a non-episode operation: \(operation)") } } @@ -220,146 +239,28 @@ actor DatabaseManager { } return .success } -} - -// MARK: - Row mapping + status conversion - -private extension DatabaseManager { - static func upsertSubscriptionRow(_ subscription: Subscription, db: Database) throws { - try db.execute( - sql: """ - INSERT INTO subscriptions - (id, feed_url, title, artwork_url, description, last_refreshed, created_at) - VALUES (?, ?, ?, ?, ?, ?, ?) - ON CONFLICT(feed_url) DO UPDATE SET - title = excluded.title, - artwork_url = excluded.artwork_url, - description = excluded.description, - last_refreshed = excluded.last_refreshed - """, - arguments: [ - subscription.id, subscription.feedUrl, subscription.title, - subscription.artworkUrl, subscription.description, - subscription.lastRefreshed, subscription.createdAt, - ], - ) - } - - static func upsertEpisodeRow(_ episode: Episode, subscriptionId: String, db: Database) throws { - let playbackStr = playbackStatusString(episode.playbackStatus) - let downloadStr = downloadStatusString(episode.downloadStatus) - let downloadProgress = episode.downloadProgress.map { Int32($0) } - try db.execute( - sql: """ - INSERT INTO episodes - (id, feed_guid, subscription_id, title, description, pub_date, - duration_secs, enclosure_url, artwork_url, playback_status, - playback_position_secs, download_status, download_progress, - is_flagged, file_size_bytes, local_path) - VALUES (?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?) - ON CONFLICT(subscription_id, feed_guid) DO UPDATE SET - title = excluded.title, - description = excluded.description, - enclosure_url = excluded.enclosure_url, - artwork_url = excluded.artwork_url, - pub_date = excluded.pub_date, - duration_secs = excluded.duration_secs, - file_size_bytes = excluded.file_size_bytes - """, - arguments: [ - episode.id, episode.feedGuid, subscriptionId, - episode.title, episode.description, - episode.pubDate, episode.durationSecs, - episode.enclosureUrl, episode.artworkUrl, - playbackStr, episode.playbackPositionSecs, - downloadStr, downloadProgress, - episode.isFlagged, - episode.fileSizeBytes.flatMap { Int64(exactly: $0) }, - episode.localPath, - ], - ) - } - - static func subscription(from row: Row) -> Subscription { - Subscription( - id: row["id"], - feedUrl: row["feed_url"], - title: row["title"], - artworkUrl: row["artwork_url"], - description: row["description"], - lastRefreshed: row["last_refreshed"], - createdAt: row["created_at"], - ) - } - - static func episode(from row: Row) -> Episode { - // Checked conversions: a stored value outside the target type's range - // (written by a later schema or external tooling) reads back as nil - // rather than trapping or silently wrapping — matching the write path, - // which stores NULL on overflow (see fileSizeBytes in upsertEpisodeRow). - Episode( - id: row["id"], - feedGuid: row["feed_guid"], - subscriptionId: row["subscription_id"], - title: row["title"], - description: row["description"], - pubDate: row["pub_date"], - durationSecs: (row["duration_secs"] as Int64?).flatMap { UInt32(exactly: $0) }, - enclosureUrl: row["enclosure_url"], - artworkUrl: row["artwork_url"], - playbackStatus: playbackStatus(from: row["playback_status"]), - playbackPositionSecs: (row["playback_position_secs"] as Int64?).flatMap { UInt32(exactly: $0) }, - downloadStatus: downloadStatus(from: row["download_status"]), - downloadProgress: (row["download_progress"] as Int32?).flatMap { UInt8(exactly: $0) }, - isFlagged: row["is_flagged"], - fileSizeBytes: (row["file_size_bytes"] as Int64?).map { UInt64(bitPattern: $0) }, - localPath: row["local_path"], - ) - } - static func playbackStatusString(_ status: PlaybackStatus) -> String { - switch status { - case .unplayed: "Unplayed" - case .inProgress: "InProgress" - case .played: "Played" - } - } - - static func playbackStatus(from string: String) -> PlaybackStatus { - switch string { - case "Unplayed": .unplayed - case "InProgress": .inProgress - case "Played": .played - default: fatalError( - "Unknown PlaybackStatus in DB: '\(string)' — add a case to " - + "playbackStatus(from:) and playbackStatusString(_:)", - ) - } - } - - static func downloadStatusString(_ status: DownloadStatus) -> String { - switch status { - case .notDownloaded: "NotDownloaded" - case .queued: "Queued" - case .downloading: "Downloading" - case .downloaded: "Downloaded" - case .failed: "Failed" - case .removedFromFeed: "RemovedFromFeed" - } - } - - static func downloadStatus(from string: String) -> DownloadStatus { - switch string { - case "NotDownloaded": .notDownloaded - case "Queued": .queued - case "Downloading": .downloading - case "Downloaded": .downloaded - case "Failed": .failed - case "RemovedFromFeed": .removedFromFeed - default: fatalError( - "Unknown DownloadStatus in DB: '\(string)' — add a case to " - + "downloadStatus(from:) and downloadStatusString(_:)", + /// Persists a download-state transition. `localPath`/`sizeBytes`/`progress` + /// are written verbatim — passing nil clears the column to NULL, matching the + /// core, which sends nil to wipe stale file metadata on delete/failure. Size + /// overflow stores NULL (as in `upsertEpisodeRow`) rather than trapping. + private func updateDownloadState( + episodeId: String, status: DownloadStatus, localPath: String?, + sizeBytes: UInt64?, progress: UInt8?, + ) async throws -> StorageResult { + let statusStr = Self.downloadStatusString(status) + let size = sizeBytes.flatMap { Int64(exactly: $0) } + let progressInt = progress.map { Int32($0) } + try await db.write { db in + try db.execute( + sql: """ + UPDATE episodes + SET download_status = ?, local_path = ?, file_size_bytes = ?, download_progress = ? + WHERE id = ? + """, + arguments: [statusStr, localPath, size, progressInt, episodeId], ) } + return .success } } diff --git a/iOS/Pollux/DatabaseManagerRows.swift b/iOS/Pollux/DatabaseManagerRows.swift new file mode 100644 index 0000000..d98d8eb --- /dev/null +++ b/iOS/Pollux/DatabaseManagerRows.swift @@ -0,0 +1,146 @@ +import App +import Foundation +import GRDB + +/// Row mapping + status conversion for `DatabaseManager`. Split out of the main file +/// to keep each under the file-length limit; these are module-internal helpers, not +/// part of any public surface. +extension DatabaseManager { + static func upsertSubscriptionRow(_ subscription: Subscription, db: Database) throws { + try db.execute( + sql: """ + INSERT INTO subscriptions + (id, feed_url, title, artwork_url, description, last_refreshed, created_at) + VALUES (?, ?, ?, ?, ?, ?, ?) + ON CONFLICT(feed_url) DO UPDATE SET + title = excluded.title, + artwork_url = excluded.artwork_url, + description = excluded.description, + last_refreshed = excluded.last_refreshed + """, + arguments: [ + subscription.id, subscription.feedUrl, subscription.title, + subscription.artworkUrl, subscription.description, + subscription.lastRefreshed, subscription.createdAt, + ], + ) + } + + static func upsertEpisodeRow(_ episode: Episode, subscriptionId: String, db: Database) throws { + let playbackStr = playbackStatusString(episode.playbackStatus) + let downloadStr = downloadStatusString(episode.downloadStatus) + let downloadProgress = episode.downloadProgress.map { Int32($0) } + try db.execute( + sql: """ + INSERT INTO episodes + (id, feed_guid, subscription_id, title, description, pub_date, + duration_secs, enclosure_url, artwork_url, playback_status, + playback_position_secs, download_status, download_progress, + is_flagged, file_size_bytes, local_path) + VALUES (?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?) + ON CONFLICT(subscription_id, feed_guid) DO UPDATE SET + title = excluded.title, + description = excluded.description, + enclosure_url = excluded.enclosure_url, + artwork_url = excluded.artwork_url, + pub_date = excluded.pub_date, + duration_secs = excluded.duration_secs, + file_size_bytes = excluded.file_size_bytes + """, + arguments: [ + episode.id, episode.feedGuid, subscriptionId, + episode.title, episode.description, + episode.pubDate, episode.durationSecs, + episode.enclosureUrl, episode.artworkUrl, + playbackStr, episode.playbackPositionSecs, + downloadStr, downloadProgress, + episode.isFlagged, + episode.fileSizeBytes.flatMap { Int64(exactly: $0) }, + episode.localPath, + ], + ) + } + + static func subscription(from row: Row) -> Subscription { + Subscription( + id: row["id"], + feedUrl: row["feed_url"], + title: row["title"], + artworkUrl: row["artwork_url"], + description: row["description"], + lastRefreshed: row["last_refreshed"], + createdAt: row["created_at"], + ) + } + + static func episode(from row: Row) -> Episode { + // Checked conversions: a stored value outside the target type's range + // (written by a later schema or external tooling) reads back as nil + // rather than trapping or silently wrapping — matching the write path, + // which stores NULL on overflow (see fileSizeBytes in upsertEpisodeRow). + Episode( + id: row["id"], + feedGuid: row["feed_guid"], + subscriptionId: row["subscription_id"], + title: row["title"], + description: row["description"], + pubDate: row["pub_date"], + durationSecs: (row["duration_secs"] as Int64?).flatMap { UInt32(exactly: $0) }, + enclosureUrl: row["enclosure_url"], + artworkUrl: row["artwork_url"], + playbackStatus: playbackStatus(from: row["playback_status"]), + playbackPositionSecs: (row["playback_position_secs"] as Int64?).flatMap { UInt32(exactly: $0) }, + downloadStatus: downloadStatus(from: row["download_status"]), + downloadProgress: (row["download_progress"] as Int32?).flatMap { UInt8(exactly: $0) }, + isFlagged: row["is_flagged"], + fileSizeBytes: (row["file_size_bytes"] as Int64?).map { UInt64(bitPattern: $0) }, + localPath: row["local_path"], + ) + } + + static func playbackStatusString(_ status: PlaybackStatus) -> String { + switch status { + case .unplayed: "Unplayed" + case .inProgress: "InProgress" + case .played: "Played" + } + } + + static func playbackStatus(from string: String) -> PlaybackStatus { + switch string { + case "Unplayed": .unplayed + case "InProgress": .inProgress + case "Played": .played + default: fatalError( + "Unknown PlaybackStatus in DB: '\(string)' — add a case to " + + "playbackStatus(from:) and playbackStatusString(_:)", + ) + } + } + + static func downloadStatusString(_ status: DownloadStatus) -> String { + switch status { + case .notDownloaded: "NotDownloaded" + case .queued: "Queued" + case .downloading: "Downloading" + case .downloaded: "Downloaded" + case .failed: "Failed" + case .removedFromFeed: "RemovedFromFeed" + } + } + + static func downloadStatus(from string: String) -> DownloadStatus { + switch string { + case "NotDownloaded": .notDownloaded + case "Queued": .queued + case "Downloading": .downloading + case "Downloaded": .downloaded + case "Failed": .failed + case "RemovedFromFeed": .removedFromFeed + default: fatalError( + "Unknown DownloadStatus in DB: '\(string)' — add a case to " + + "downloadStatus(from:) and downloadStatusString(_:)", + ) + } + } +} diff --git a/iOS/Pollux/DownloadManager.swift b/iOS/Pollux/DownloadManager.swift new file mode 100644 index 0000000..297b2e2 --- /dev/null +++ b/iOS/Pollux/DownloadManager.swift @@ -0,0 +1,134 @@ +import App +import Foundation + +/// Errors from the download shell. Carried in `DownloadResult.error` back to the +/// core, which turns them into a `Failed` episode row. +enum DownloadManagerError: Error, LocalizedError { + case storageUnavailable + case invalidURL(String) + case badStatus(Int) + + var errorDescription: String? { + switch self { + case .storageUnavailable: + "device storage is unavailable" + case let .invalidURL(url): + "invalid download URL: \(url)" + case let .badStatus(code): + "download failed: HTTP \(code)" + } + } +} + +/// Performs the actual episode-audio I/O for the `Download` capability: fetching a +/// file to local storage and removing it again. The core owns *when* to download +/// (and the serial queue); this type only carries out one operation at a time and +/// reports the outcome. +/// +/// Files live under `/Downloads/`. The path returned to the +/// core (and stored in the DB) is **relative** to Application Support, so it keeps +/// resolving after an OS container-path change; `absoluteURL(for:)` turns it back +/// into a usable URL. +actor DownloadManager { + private let storageRoot: URL + private let downloadsDir: URL + + /// Subdirectory (and stored-path prefix) for downloaded audio. + private static let subdirectory = "Downloads" + + init() throws { + guard let support = FileManager.default.urls( + for: .applicationSupportDirectory, in: .userDomainMask, + ).first else { + throw DownloadManagerError.storageUnavailable + } + storageRoot = support + downloadsDir = support.appendingPathComponent(Self.subdirectory, isDirectory: true) + } + + func perform(_ operation: DownloadOperation) async -> DownloadResult { + switch operation { + case let .download(episodeId, url): + await download(episodeId: episodeId, urlString: url) + case let .delete(localPath): + delete(localPath: localPath) + } + } + + // MARK: - Download + + private func download(episodeId: String, urlString: String) async -> DownloadResult { + guard let url = URL(string: urlString) else { + return .error(DownloadManagerError.invalidURL(urlString).localizedDescription) + } + do { + try FileManager.default.createDirectory( + at: downloadsDir, withIntermediateDirectories: true, + ) + + // PROGRESS SEAM: this uses the one-shot async download (status-only + // pass). Live 0–100% progress is added here by switching to a + // URLSessionDownloadDelegate whose `didWriteData` reports percent — the + // core already accepts `DownloadResult.progress`, so that change is + // additive and touches only this method plus the resolve loop. + let (tempURL, response) = try await URLSession.shared.download(from: url) + + if let http = response as? HTTPURLResponse, !(200 ..< 300).contains(http.statusCode) { + return .error(DownloadManagerError.badStatus(http.statusCode).localizedDescription) + } + + let fileName = Self.fileName(episodeId: episodeId, url: url) + let destination = downloadsDir.appendingPathComponent(fileName) + // A re-download replaces any previous file at the same path. + try? FileManager.default.removeItem(at: destination) + try FileManager.default.moveItem(at: tempURL, to: destination) + + let size = fileSize(at: destination) + let relativePath = "\(Self.subdirectory)/\(fileName)" + return .completed(localPath: relativePath, sizeBytes: size) + } catch { + return .error(error.localizedDescription) + } + } + + // MARK: - Delete + + private func delete(localPath: String) -> DownloadResult { + let target = absoluteURL(for: localPath) + // Idempotent: a file that is already gone is a successful delete. + guard FileManager.default.fileExists(atPath: target.path) else { + return .deleted + } + do { + try FileManager.default.removeItem(at: target) + return .deleted + } catch { + return .error(error.localizedDescription) + } + } + + // MARK: - Paths + + private func absoluteURL(for relativePath: String) -> URL { + storageRoot.appendingPathComponent(relativePath) + } + + /// A filesystem-safe, stable file name for an episode. The episode id can be a + /// GUID, URL, or hash, so it is reduced to alphanumerics; the enclosure's file + /// extension is preserved when present so players see a familiar type. + private static func fileName(episodeId: String, url: URL) -> String { + let safeId = episodeId.map { $0.isLetter || $0.isNumber ? $0 : "_" } + let base = String(safeId) + let ext = url.pathExtension + return ext.isEmpty ? base : "\(base).\(ext)" + } + + private func fileSize(at url: URL) -> UInt64 { + guard let attributes = try? FileManager.default.attributesOfItem(atPath: url.path), + let size = attributes[.size] as? UInt64 + else { + return 0 + } + return size + } +} diff --git a/iOS/Pollux/EpisodeDetailView.swift b/iOS/Pollux/EpisodeDetailView.swift index 79f5d44..15671c3 100644 --- a/iOS/Pollux/EpisodeDetailView.swift +++ b/iOS/Pollux/EpisodeDetailView.swift @@ -1,22 +1,30 @@ import App import SwiftUI -/// Full-page view for a single episode. Renders the stored metadata and show -/// notes; the transport controls, chapters, and bookmarks are DEBUG-marked -/// placeholders because their engines (playback, chapters, bookmarks) don't exist -/// yet. The episode is passed in from the list rather than re-fetched — the list's -/// ViewModel already carries it. A dedicated `GetEpisode` round-trip could replace -/// this later if the detail page ever needs data the list doesn't ship. +/// Full-page view for a single episode. Renders the stored metadata, show notes, +/// and a working download control; transport controls, chapters, and bookmarks are +/// DEBUG-marked placeholders because their engines don't exist yet. Static metadata +/// is taken from the `episode` snapshot passed in from the list, but the download +/// state is read live from `core.view` so it updates while the page is open. struct EpisodeDetailView: View { + @ObservedObject var core: Core let episode: EpisodeSummary let feedTitle: String + /// The current version of this episode from the core, so download-state changes + /// show up here without re-navigating. Falls back to the injected snapshot if + /// the list is no longer the selected feed. + private var liveEpisode: EpisodeSummary { + core.view.subscriptionDetail.episodes.first { $0.id == episode.id } ?? episode + } + var body: some View { ScrollView { VStack(alignment: .leading, spacing: 20) { artworkHeader titleBlock playbackControls + downloadSection Divider() showNotes PlaceholderSection( @@ -84,6 +92,46 @@ struct EpisodeDetailView: View { .frame(maxWidth: .infinity) } + /// Working download control, driven by the live download status. + @ViewBuilder private var downloadSection: some View { + switch liveEpisode.downloadStatus { + case .notDownloaded: + Button { core.update(.downloadEpisode(episode.id)) } label: { + Label("Download episode", systemImage: "arrow.down.circle") + } + case .queued: + Label("Queued for download", systemImage: "clock") + .foregroundStyle(.secondary) + case .downloading: + Label { + Text("Downloading…") + } icon: { + ProgressView() + } + case .downloaded: + HStack { + Label("Downloaded", systemImage: "checkmark.circle.fill") + .foregroundStyle(.green) + Spacer() + Button(role: .destructive) { + core.update(.deleteDownload(episode.id)) + } label: { + Label("Delete", systemImage: "trash") + } + } + case .failed: + HStack { + Label("Download failed", systemImage: "exclamationmark.triangle") + .foregroundStyle(.orange) + Spacer() + Button("Retry") { core.update(.downloadEpisode(episode.id)) } + } + case .removedFromFeed: + Label("Removed from feed", systemImage: "xmark.circle") + .foregroundStyle(.secondary) + } + } + private var showNotes: some View { VStack(alignment: .leading, spacing: 6) { Text("Show notes") diff --git a/iOS/Pollux/EpisodeRow.swift b/iOS/Pollux/EpisodeRow.swift index 1dbeb72..7aec9e1 100644 --- a/iOS/Pollux/EpisodeRow.swift +++ b/iOS/Pollux/EpisodeRow.swift @@ -1,12 +1,16 @@ import App import SwiftUI -/// One episode in the subscription details list. Shows only stored data (art, -/// title, date, duration, description, read-only played/download status). Controls -/// whose engines don't exist yet (play, download, more-actions) are rendered as -/// DEBUG-tinted, disabled placeholders — see `DebugStyle.swift`. +/// One episode in the subscription details list. Shows stored data (art, title, +/// date, duration, description, read-only played/download status) and a working +/// download control. Playback and more-actions remain DEBUG-tinted, disabled +/// placeholders because their engines don't exist yet — see `DebugStyle.swift`. struct EpisodeRow: View { let episode: EpisodeSummary + /// Queue this episode for download (or retry a failed one). + let onDownload: (String) -> Void + /// Remove this episode's downloaded file. + let onDeleteDownload: (String) -> Void var body: some View { HStack(alignment: .top, spacing: 12) { @@ -64,20 +68,61 @@ struct EpisodeRow: View { } } - /// Play + more-actions: no playback engine or download manager exists yet, so - /// these are inert placeholders (DEBUG tint + 🚫 overlay), not wired to fake - /// behavior. + /// Trailing controls: a working download control plus the still-stubbed play + /// and more-actions buttons (their engines don't exist yet). `.borderless` + /// keeps the download button from also triggering the row's NavigationLink. private var placeholderControls: some View { HStack(spacing: 12) { Image(systemName: "play.circle.fill") .font(.title2) .stubbed() + downloadControl Image(systemName: "ellipsis.circle") .font(.title2) .stubbed() } } + /// The download affordance, driven by the episode's current download status. + /// Actionable states are real buttons; transient/terminal states are indicators. + @ViewBuilder private var downloadControl: some View { + switch episode.downloadStatus { + case .notDownloaded: + Button { onDownload(episode.id) } label: { + Image(systemName: "arrow.down.circle").font(.title2) + } + .buttonStyle(.borderless) + .accessibilityLabel("Download episode") + case .failed: + Button { onDownload(episode.id) } label: { + Image(systemName: "arrow.clockwise.circle").font(.title2) + } + .buttonStyle(.borderless) + .tint(.orange) + .accessibilityLabel("Retry download") + case .queued: + Image(systemName: "clock") + .font(.title2) + .foregroundStyle(.secondary) + .accessibilityLabel("Queued for download") + case .downloading: + ProgressView() + .accessibilityLabel("Downloading") + case .downloaded: + Button { onDeleteDownload(episode.id) } label: { + Image(systemName: "trash.circle").font(.title2) + } + .buttonStyle(.borderless) + .tint(.gray) + .accessibilityLabel("Delete download") + case .removedFromFeed: + Image(systemName: "xmark.circle") + .font(.title2) + .foregroundStyle(.secondary) + .accessibilityLabel("Removed from feed") + } + } + private var playbackBadge: (icon: String, text: String)? { switch episode.playbackStatus { case .played: diff --git a/iOS/Pollux/SubscriptionDetailScreen.swift b/iOS/Pollux/SubscriptionDetailScreen.swift index 655f66d..c4f0227 100644 --- a/iOS/Pollux/SubscriptionDetailScreen.swift +++ b/iOS/Pollux/SubscriptionDetailScreen.swift @@ -28,7 +28,7 @@ struct SubscriptionDetailScreen: View { } } .navigationDestination(for: EpisodeSummary.self) { episode in - EpisodeDetailView(episode: episode, feedTitle: subscription.title) + EpisodeDetailView(core: core, episode: episode, feedTitle: subscription.title) } .task(id: subscription.id) { core.update(.selectSubscription(subscription.id)) @@ -70,7 +70,11 @@ struct SubscriptionDetailScreen: View { } else { List(detail.episodes, id: \.id) { episode in NavigationLink(value: episode) { - EpisodeRow(episode: episode) + EpisodeRow( + episode: episode, + onDownload: { core.update(.downloadEpisode($0)) }, + onDeleteDownload: { core.update(.deleteDownload($0)) }, + ) } } .listStyle(.plain) diff --git a/iOS/Pollux/core.swift b/iOS/Pollux/core.swift index f3bbefc..789c661 100644 --- a/iOS/Pollux/core.swift +++ b/iOS/Pollux/core.swift @@ -8,6 +8,7 @@ class Core: ObservableObject { private var core: CoreFfi private let db: DatabaseManager + private let downloads: DownloadManager init() { core = CoreFfi() @@ -16,6 +17,11 @@ class Core: ObservableObject { } catch { fatalError("Failed to initialize DatabaseManager: \(error)") } + do { + downloads = try DownloadManager() + } catch { + fatalError("Failed to initialize DownloadManager: \(error)") + } guard let view = try? ViewModel.bincodeDeserialize(input: [UInt8](core.view())) else { fatalError("Failed to deserialize initial ViewModel from core") } @@ -66,6 +72,15 @@ class Core: ObservableObject { self?.resolveAndDispatch(requestId: requestId, result: result) } } + + case let .download(operation): + // The heavy I/O runs on the DownloadManager actor's executor; awaiting + // it from a MainActor task only parks the continuation, so this doesn't + // block the UI (same shape as the storage arm above). + Task { @MainActor in + let result = await downloads.perform(operation) + resolveAndDispatch(requestId: request.id, result: result) + } } } @@ -116,4 +131,11 @@ class Core: ObservableObject { } resolveBytes(requestId: requestId, bytes: bytes) } + + private func resolveAndDispatch(requestId: UInt32, result: DownloadResult) { + guard let bytes = try? result.bincodeSerialize() else { + fatalError("Failed to serialize DownloadResult for request \(requestId)") + } + resolveBytes(requestId: requestId, bytes: bytes) + } } diff --git a/iOS/PolluxTests/DatabaseManagerTests.swift b/iOS/PolluxTests/DatabaseManagerTests.swift index 7395e3c..0910f4e 100644 --- a/iOS/PolluxTests/DatabaseManagerTests.swift +++ b/iOS/PolluxTests/DatabaseManagerTests.swift @@ -230,6 +230,55 @@ struct DatabaseManagerTests { #expect(fetched.playbackPositionSecs == largePosition) } + @Test func updateDownloadState_recordsDownloadedFileMetadata() async throws { + let db = try makeManager() + try await db.execute(.upsertSubscription(makeSubscription(id: "sub-1"))) + try await db.execute(.upsertEpisode(makeEpisode(id: "ep-1", subscriptionId: "sub-1"))) + + try await db.execute( + .updateDownloadState( + episodeId: "ep-1", status: .downloaded, + localPath: "Downloads/ep-1.mp3", sizeBytes: 4096, progress: nil, + )) + + let result = try await db.execute(.getEpisode(id: "ep-1")) + guard case let .episode(fetched) = result else { + Issue.record("Expected .episode, got \(result)") + return + } + #expect(fetched.downloadStatus == .downloaded) + #expect(fetched.localPath == "Downloads/ep-1.mp3") + #expect(fetched.fileSizeBytes == 4096) + } + + @Test func updateDownloadState_clearsFileMetadataOnReset() async throws { + let db = try makeManager() + try await db.execute(.upsertSubscription(makeSubscription(id: "sub-1"))) + try await db.execute(.upsertEpisode(makeEpisode(id: "ep-1", subscriptionId: "sub-1"))) + + // Download, then delete: the reset must null out the path and size so no + // stale file reference survives. + try await db.execute( + .updateDownloadState( + episodeId: "ep-1", status: .downloaded, + localPath: "Downloads/ep-1.mp3", sizeBytes: 4096, progress: nil, + )) + try await db.execute( + .updateDownloadState( + episodeId: "ep-1", status: .notDownloaded, + localPath: nil, sizeBytes: nil, progress: nil, + )) + + let result = try await db.execute(.getEpisode(id: "ep-1")) + guard case let .episode(fetched) = result else { + Issue.record("Expected .episode, got \(result)") + return + } + #expect(fetched.downloadStatus == .notDownloaded) + #expect(fetched.localPath == nil) + #expect(fetched.fileSizeBytes == nil) + } + @Test func deleteSubscription_cascadesToEpisodes() async throws { let db = try makeManager() let sub = makeSubscription(id: "sub-1") diff --git a/shared/src/app.rs b/shared/src/app.rs index 03b319d..0056603 100644 --- a/shared/src/app.rs +++ b/shared/src/app.rs @@ -2,9 +2,10 @@ use crux_core::{render::render, App, Command}; use facet::Facet; use serde::{Deserialize, Serialize}; +use crate::capabilities::download::{DownloadOperation, DownloadResult}; use crate::capabilities::http::{HttpOperation, HttpResult}; use crate::capabilities::storage::{StorageOperation, StorageResult}; -use crate::domain::{Episode, EpisodeSortOrder}; +use crate::domain::{DownloadStatus, Episode, EpisodeSortOrder}; use crate::effect::Effect; use crate::feed_parser::parse_feed; use crate::html::strip_html_preview; @@ -169,6 +170,130 @@ impl App for Pollux { model.episode_sort = order; render() } + Event::DownloadEpisode(episode_id) => { + // Only actionable for an episode we have loaded (the details list + // today). Already-queued/downloading episodes are a no-op so a + // double-tap can't enqueue twice. + let status = model + .episodes + .iter() + .find(|e| e.id == episode_id) + .map(|e| e.download_status.clone()); + match status { + None | Some(DownloadStatus::Queued) | Some(DownloadStatus::Downloading) => { + render() + } + _ if !download_allowed(model) => { + // Fail-on-full: unlimited today, so this is unreachable. + // When a storage cap exists this is where a download is + // refused up front and marked Failed instead of started. + set_download_state(model, &episode_id, DownloadStatus::Failed, None, None); + persist_download_state(&episode_id, DownloadStatus::Failed, None, None) + .and(render()) + } + _ => { + set_download_state(model, &episode_id, DownloadStatus::Queued, None, None); + model.download_queue.push(episode_id.clone()); + let persist = + persist_download_state(&episode_id, DownloadStatus::Queued, None, None); + persist.and(maybe_start_next(model)).and(render()) + } + } + } + Event::DownloadFinished { episode_id, result } => match *result { + DownloadResult::Completed { + local_path, + size_bytes, + } => { + finish_active(model, &episode_id); + set_download_state( + model, + &episode_id, + DownloadStatus::Downloaded, + Some(local_path.clone()), + Some(size_bytes), + ); + let persist = persist_download_state( + &episode_id, + DownloadStatus::Downloaded, + Some(local_path), + Some(size_bytes), + ); + persist.and(maybe_start_next(model)).and(render()) + } + DownloadResult::Error(_) => { + finish_active(model, &episode_id); + set_download_state(model, &episode_id, DownloadStatus::Failed, None, None); + let persist = + persist_download_state(&episode_id, DownloadStatus::Failed, None, None); + persist.and(maybe_start_next(model)).and(render()) + } + // Progress is not consumed in the status-only pass; Deleted never + // arrives here (it resolves DeleteDownload instead). + DownloadResult::Progress { .. } | DownloadResult::Deleted => render(), + }, + Event::DeleteDownload(episode_id) => { + let local_path = model + .episodes + .iter() + .find(|e| e.id == episode_id) + .and_then(|e| e.local_path.clone()); + match local_path { + Some(path) => { + let id = episode_id.clone(); + Command::request_from_shell(DownloadOperation::Delete { local_path: path }) + .then_send(move |r| Event::DownloadDeleted { + episode_id: id.clone(), + result: Box::new(r), + }) + .and(render()) + } + None => { + // Nothing on disk (e.g. a Failed row): normalize to + // NotDownloaded without a filesystem round-trip. + set_download_state( + model, + &episode_id, + DownloadStatus::NotDownloaded, + None, + None, + ); + persist_download_state( + &episode_id, + DownloadStatus::NotDownloaded, + None, + None, + ) + .and(render()) + } + } + } + Event::DownloadDeleted { episode_id, result } => match *result { + DownloadResult::Deleted => { + set_download_state( + model, + &episode_id, + DownloadStatus::NotDownloaded, + None, + None, + ); + persist_download_state(&episode_id, DownloadStatus::NotDownloaded, None, None) + .and(render()) + } + DownloadResult::Error(e) => { + // The file couldn't be removed; leave the row as-is and surface + // why on the details page rather than lying about the state. + model.detail_error = Some(e); + render() + } + DownloadResult::Completed { .. } | DownloadResult::Progress { .. } => render(), + }, + Event::DownloadStatePersisted(result) => { + if let StorageResult::Error(e) = *result { + model.detail_error = Some(e); + } + render() + } } } @@ -267,6 +392,105 @@ fn sort_episodes(episodes: &mut [EpisodeSummary], order: EpisodeSortOrder) { } } +/// Pre-download storage check. Unlimited today — the user-configurable soft cap +/// arrives with the Settings screen (see ROADMAP follow-ups). Device-full is not +/// predicted here; it surfaces as the shell's write failing (`DownloadResult::Error` +/// → `Failed`), which is the fail-on-full behavior the storage spec calls for. +fn download_allowed(_model: &Model) -> bool { + true +} + +/// Applies a download-state transition to the in-memory episode, if it's loaded. +/// +/// `SelectSubscription` is intentionally idempotent (it won't reload the feed +/// already on screen), so a status change written only to storage would not show +/// up on the details page. Mutating `model.episodes` in place is what lets `view()` +/// reproject the new state without a reload — see the note at `SelectSubscription`. +/// `local_path`/`size_bytes`/`progress` are set (or cleared) together with the +/// status so the row never keeps a path for a file that isn't there. +fn set_download_state( + model: &mut Model, + episode_id: &str, + status: DownloadStatus, + local_path: Option, + size_bytes: Option, +) { + if let Some(episode) = model.episodes.iter_mut().find(|e| e.id == episode_id) { + episode.download_status = status; + episode.local_path = local_path; + episode.file_size_bytes = size_bytes; + // No live percentage in the status-only pass; keep the column NULL rather + // than leaving a stale value from a previous download. + episode.download_progress = None; + } +} + +/// Persists a download-state transition through the storage capability. The result +/// is checked for errors (via `DownloadStatePersisted`) but success is silent — the +/// UI already reflects the change from `set_download_state`, so persistence is only +/// about durability across restarts. +fn persist_download_state( + episode_id: &str, + status: DownloadStatus, + local_path: Option, + size_bytes: Option, +) -> Command { + Command::request_from_shell(StorageOperation::UpdateDownloadState { + episode_id: episode_id.to_string(), + status, + local_path, + size_bytes, + progress: None, + }) + .then_send(|r| Event::DownloadStatePersisted(Box::new(r))) +} + +/// Clears the in-flight marker when the active download reaches a terminal state. +fn finish_active(model: &mut Model, episode_id: &str) { + if model.downloading.as_deref() == Some(episode_id) { + model.downloading = None; + } +} + +/// Starts the next queued download if nothing is in flight (serial: one at a time). +/// Returns the command that marks the episode `Downloading`, persists that, and +/// issues the shell download whose result comes back as `DownloadFinished`. A no-op +/// (empty command) when a download is already running or the queue is empty. +fn maybe_start_next(model: &mut Model) -> Command { + if model.downloading.is_some() { + return Command::done(); + } + let Some(episode_id) = model.download_queue.first().cloned() else { + return Command::done(); + }; + model.download_queue.remove(0); + + // The enclosure URL comes from the loaded episode. If it's no longer loaded, + // drop this queue entry and try the next one rather than stalling the queue. + let Some(url) = model + .episodes + .iter() + .find(|e| e.id == episode_id) + .map(|e| e.enclosure_url.clone()) + else { + return maybe_start_next(model); + }; + + model.downloading = Some(episode_id.clone()); + set_download_state(model, &episode_id, DownloadStatus::Downloading, None, None); + + let persist = persist_download_state(&episode_id, DownloadStatus::Downloading, None, None); + let download = Command::request_from_shell(DownloadOperation::Download { + episode_id: episode_id.clone(), + url, + }) + .then_send(move |r| Event::DownloadFinished { + episode_id: episode_id.clone(), + result: Box::new(r), + }); + persist.and(download) +} + #[derive(Facet, Serialize, Deserialize, Clone, Debug)] #[repr(C)] pub enum Event { @@ -284,6 +508,23 @@ pub enum Event { result: Box, }, SetEpisodeSort(EpisodeSortOrder), + /// Queue an episode's audio for download (or retry a failed one). + DownloadEpisode(String), + /// Remove a downloaded (or failed) episode's local file. + DeleteDownload(String), + /// Terminal result of a shell download for `episode_id`. + DownloadFinished { + episode_id: String, + result: Box, + }, + /// Terminal result of a shell delete for `episode_id`. + DownloadDeleted { + episode_id: String, + result: Box, + }, + /// Result of persisting a download-state transition. Success is silent; an + /// error is surfaced on the details page (downloads are driven from there). + DownloadStatePersisted(Box), } #[cfg(test)] @@ -1037,4 +1278,268 @@ mod tests { assert!(summary.description.is_none()); assert!(summary.description_text.is_none()); } + + // --- Download manager --- + + fn model_episode_status(model: &Model, id: &str) -> DownloadStatus { + model + .episodes + .iter() + .find(|e| e.id == id) + .map(|e| e.download_status.clone()) + .expect("episode should be loaded") + } + + fn has_download_effect_for(effects: &[Effect], episode_id: &str) -> bool { + effects.iter().any(|e| { + matches!( + e, + Effect::Download(r) if matches!(&r.operation, + DownloadOperation::Download { episode_id: id, .. } if id == episode_id) + ) + }) + } + + #[test] + fn download_episode_marks_downloading_and_issues_download_effect() { + let app = Pollux; + let mut model = Model::default(); + load_episodes(&app, &mut model, vec![make_episode("e1", "Ep", Some(1))]); + + let mut cmd = app.update(Event::DownloadEpisode("e1".to_string()), &mut model); + + // Serial: nothing else in flight, so it starts immediately. + assert_eq!(model.downloading.as_deref(), Some("e1")); + assert_eq!( + model_episode_status(&model, "e1"), + DownloadStatus::Downloading + ); + assert!( + model.download_queue.is_empty(), + "the only item should be in flight, not queued" + ); + + let effects: Vec = cmd.effects().collect(); + assert!( + has_download_effect_for(&effects, "e1"), + "expected a Download effect for e1" + ); + + // The details page reflects the new state without any re-selection — the + // in-place model update is what makes idempotent SelectSubscription safe. + let view = app.view(&model); + assert_eq!( + view.subscription_detail.episodes[0].download_status, + DownloadStatus::Downloading + ); + } + + #[test] + fn second_download_stays_queued_while_one_is_in_flight() { + let app = Pollux; + let mut model = Model::default(); + load_episodes( + &app, + &mut model, + vec![ + make_episode("e1", "First", Some(2)), + make_episode("e2", "Second", Some(1)), + ], + ); + + let _ = app.update(Event::DownloadEpisode("e1".to_string()), &mut model); + let mut cmd = app.update(Event::DownloadEpisode("e2".to_string()), &mut model); + + // e1 is still the one downloading; e2 waits its turn. + assert_eq!(model.downloading.as_deref(), Some("e1")); + assert_eq!(model_episode_status(&model, "e2"), DownloadStatus::Queued); + assert_eq!(model.download_queue, vec!["e2".to_string()]); + + let effects: Vec = cmd.effects().collect(); + assert!( + !has_download_effect_for(&effects, "e2"), + "a second download must not start while one is in flight (serial)" + ); + } + + #[test] + fn download_completed_marks_downloaded_and_starts_next() { + let app = Pollux; + let mut model = Model::default(); + load_episodes( + &app, + &mut model, + vec![ + make_episode("e1", "First", Some(2)), + make_episode("e2", "Second", Some(1)), + ], + ); + let _ = app.update(Event::DownloadEpisode("e1".to_string()), &mut model); + let _ = app.update(Event::DownloadEpisode("e2".to_string()), &mut model); + + let mut cmd = app.update( + Event::DownloadFinished { + episode_id: "e1".to_string(), + result: Box::new(DownloadResult::Completed { + local_path: "Downloads/e1.mp3".to_string(), + size_bytes: 4_096, + }), + }, + &mut model, + ); + + // e1 is done, with its file recorded. + assert_eq!( + model_episode_status(&model, "e1"), + DownloadStatus::Downloaded + ); + let e1 = model + .episodes + .iter() + .find(|e| e.id == "e1") + .expect("e1 should be loaded"); + assert_eq!(e1.local_path.as_deref(), Some("Downloads/e1.mp3")); + assert_eq!(e1.file_size_bytes, Some(4_096)); + + // The queue drains to e2. + assert_eq!(model.downloading.as_deref(), Some("e2")); + assert_eq!( + model_episode_status(&model, "e2"), + DownloadStatus::Downloading + ); + assert!(model.download_queue.is_empty()); + + let effects: Vec = cmd.effects().collect(); + assert!( + has_download_effect_for(&effects, "e2"), + "finishing one download should start the next queued one" + ); + } + + #[test] + fn download_error_marks_failed_and_still_drains_queue() { + let app = Pollux; + let mut model = Model::default(); + load_episodes( + &app, + &mut model, + vec![ + make_episode("e1", "First", Some(2)), + make_episode("e2", "Second", Some(1)), + ], + ); + let _ = app.update(Event::DownloadEpisode("e1".to_string()), &mut model); + let _ = app.update(Event::DownloadEpisode("e2".to_string()), &mut model); + + let mut cmd = app.update( + Event::DownloadFinished { + episode_id: "e1".to_string(), + result: Box::new(DownloadResult::Error("disk full".to_string())), + }, + &mut model, + ); + + assert_eq!(model_episode_status(&model, "e1"), DownloadStatus::Failed); + // A failure must not stall the queue — the next episode still starts. + assert_eq!(model.downloading.as_deref(), Some("e2")); + let effects: Vec = cmd.effects().collect(); + assert!(has_download_effect_for(&effects, "e2")); + } + + #[test] + fn delete_download_issues_delete_effect_then_resets_on_result() { + let app = Pollux; + let mut model = Model::default(); + let mut downloaded = make_episode("e1", "Ep", Some(1)); + downloaded.download_status = DownloadStatus::Downloaded; + downloaded.local_path = Some("Downloads/e1.mp3".to_string()); + downloaded.file_size_bytes = Some(4_096); + load_episodes(&app, &mut model, vec![downloaded]); + + // Deleting a downloaded episode goes to the shell first (best-effort file + // removal), so the row stays Downloaded until the delete result comes back. + let mut cmd = app.update(Event::DeleteDownload("e1".to_string()), &mut model); + assert_eq!( + model_episode_status(&model, "e1"), + DownloadStatus::Downloaded + ); + let effects: Vec = cmd.effects().collect(); + assert!( + effects.iter().any(|e| matches!( + e, + Effect::Download(r) if matches!(&r.operation, + DownloadOperation::Delete { local_path } if local_path == "Downloads/e1.mp3") + )), + "expected a Delete effect carrying the stored path" + ); + + // The shell reports the file gone. + let _ = app.update( + Event::DownloadDeleted { + episode_id: "e1".to_string(), + result: Box::new(DownloadResult::Deleted), + }, + &mut model, + ); + assert_eq!( + model_episode_status(&model, "e1"), + DownloadStatus::NotDownloaded + ); + let e1 = model + .episodes + .iter() + .find(|e| e.id == "e1") + .expect("e1 should be loaded"); + assert!(e1.local_path.is_none(), "path must be cleared after delete"); + assert!(e1.file_size_bytes.is_none()); + } + + #[test] + fn delete_without_a_local_file_normalizes_immediately() { + let app = Pollux; + let mut model = Model::default(); + let mut failed = make_episode("e1", "Ep", Some(1)); + failed.download_status = DownloadStatus::Failed; + load_episodes(&app, &mut model, vec![failed]); + + // A Failed row has no file, so there's no shell round-trip — it just resets. + let mut cmd = app.update(Event::DeleteDownload("e1".to_string()), &mut model); + assert_eq!( + model_episode_status(&model, "e1"), + DownloadStatus::NotDownloaded + ); + let effects: Vec = cmd.effects().collect(); + assert!( + !effects.iter().any(|e| matches!(e, Effect::Download(_))), + "no file means no Delete effect" + ); + } + + #[test] + fn download_persist_error_surfaces_on_details_page() { + let app = Pollux; + let mut model = Model::default(); + let _ = app.update( + Event::DownloadStatePersisted(Box::new(StorageResult::Error("db locked".to_string()))), + &mut model, + ); + assert_eq!(model.detail_error.as_deref(), Some("db locked")); + } + + #[test] + fn redownloading_an_in_flight_episode_is_a_noop() { + let app = Pollux; + let mut model = Model::default(); + load_episodes(&app, &mut model, vec![make_episode("e1", "Ep", Some(1))]); + let _ = app.update(Event::DownloadEpisode("e1".to_string()), &mut model); + + // Tapping download again while it's downloading must not re-enqueue. + let mut cmd = app.update(Event::DownloadEpisode("e1".to_string()), &mut model); + assert!(model.download_queue.is_empty()); + let effects: Vec = cmd.effects().collect(); + assert!( + !has_download_effect_for(&effects, "e1"), + "a redundant download request must not issue a second fetch" + ); + } } diff --git a/shared/src/capabilities/download.rs b/shared/src/capabilities/download.rs new file mode 100644 index 0000000..f4319c6 --- /dev/null +++ b/shared/src/capabilities/download.rs @@ -0,0 +1,44 @@ +use crux_core::capability::Operation; +use facet::Facet; +use serde::{Deserialize, Serialize}; + +/// Side-effects the shell performs against episode audio files. Actual network +/// and filesystem I/O lives in the shell; the core only decides *when* to fetch +/// or delete and records the outcome. Downloads are driven one at a time — the +/// serial guard is in the core (see `Model::downloading`), so each operation the +/// shell receives is an independent one-shot. +#[derive(Facet, Serialize, Deserialize, Clone, Debug)] +#[repr(C)] +pub enum DownloadOperation { + /// Fetch `url` and store it locally for `episode_id`. + Download { episode_id: String, url: String }, + /// Remove a previously downloaded file. Idempotent: deleting a file that is + /// already gone still succeeds. + Delete { local_path: String }, +} + +#[derive(Facet, Serialize, Deserialize, Clone, Debug)] +#[repr(C)] +pub enum DownloadResult { + /// Interim progress for an in-flight download. Defined from the start so live + /// progress reporting is an additive change (the shell can resolve a streamed + /// request with these before the terminal `Completed`); the core does not act + /// on it in the status-only first pass. + Progress { + episode_id: String, + percent: u8, + }, + /// The download finished; the file is at `local_path` (relative to the app's + /// storage root, resolved to absolute by the shell) and is `size_bytes` long. + Completed { + local_path: String, + size_bytes: u64, + }, + /// A delete finished (or the file was already absent). + Deleted, + Error(String), +} + +impl Operation for DownloadOperation { + type Output = DownloadResult; +} diff --git a/shared/src/capabilities/mod.rs b/shared/src/capabilities/mod.rs index c581df4..497baa4 100644 --- a/shared/src/capabilities/mod.rs +++ b/shared/src/capabilities/mod.rs @@ -1,2 +1,3 @@ +pub mod download; pub mod http; pub mod storage; diff --git a/shared/src/capabilities/storage.rs b/shared/src/capabilities/storage.rs index 870979a..ac67a31 100644 --- a/shared/src/capabilities/storage.rs +++ b/shared/src/capabilities/storage.rs @@ -2,7 +2,7 @@ use crux_core::capability::Operation; use facet::Facet; use serde::{Deserialize, Serialize}; -use crate::domain::{Episode, PlaybackStatus, Subscription}; +use crate::domain::{DownloadStatus, Episode, PlaybackStatus, Subscription}; #[derive(Facet, Serialize, Deserialize, Clone, Debug)] #[repr(C)] @@ -31,6 +31,17 @@ pub enum StorageOperation { status: PlaybackStatus, position_secs: Option, }, + /// Persists a download-state transition (status plus the file metadata that + /// comes with it). `local_path`/`size_bytes`/`progress` are cleared to NULL + /// when `None` — e.g. on delete or failure — so the row never keeps a stale + /// path for a file that is no longer there. + UpdateDownloadState { + episode_id: String, + status: DownloadStatus, + local_path: Option, + size_bytes: Option, + progress: Option, + }, UpsertFeedWithEpisodes { subscription: Subscription, episodes: Vec, diff --git a/shared/src/effect.rs b/shared/src/effect.rs index 7f28ac9..242c756 100644 --- a/shared/src/effect.rs +++ b/shared/src/effect.rs @@ -3,6 +3,7 @@ use crux_core::{macros::effect, render::RenderOperation}; +use crate::capabilities::download::DownloadOperation; use crate::capabilities::http::HttpOperation; use crate::capabilities::storage::StorageOperation; @@ -12,4 +13,5 @@ pub enum Effect { Render(RenderOperation), Storage(StorageOperation), Http(HttpOperation), + Download(DownloadOperation), } diff --git a/shared/src/model.rs b/shared/src/model.rs index f40edc8..13c1907 100644 --- a/shared/src/model.rs +++ b/shared/src/model.rs @@ -13,4 +13,12 @@ pub struct Model { pub episode_sort: EpisodeSortOrder, pub detail_loading: bool, pub detail_error: Option, + + // Download manager. Serial for MVP: at most one episode downloads at a time + // (`downloading`), the rest wait in `download_queue` (front = next up). Both + // hold episode ids. The queue is in-memory only today; episodes left in the + // `Queued` status across a cold start are not yet re-enqueued (see ROADMAP + // follow-ups). + pub download_queue: Vec, + pub downloading: Option, } From 73af03e470b530b39eddf2e203697e65605f0729 Mon Sep 17 00:00:00 2001 From: Alex Axthelm Date: Thu, 17 Sep 2026 11:33:21 +0200 Subject: [PATCH 03/38] feat: show live download progress MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Report byte progress while an episode downloads: a percentage on the episode row and a determinate bar with a size label ("12 MB / 37 MB") on the detail page, falling back to an indeterminate indicator when the server reports no size. Progress is transient runtime state — a partial download can't resume — so it is kept in-memory only, never persisted. - shared: replace the placeholder DownloadResult::Progress with an Event::DownloadProgress carrying received/total bytes. The core stores it in Model.active_download_progress (cleared when a download starts or ends) and overlays it onto the active episode in view(); EpisodeSummary swaps its download_progress (u8 percent) for received/total byte fields. - iOS: byte progress comes from a session-level URLSessionDownloadDelegate. The async download(from:delegate:) convenience never fires didWriteData, so DownloadManager now uses a dedicated URLSession + a continuation, moving the finished temp file in didFinishDownloadingTo. core.swift forwards progress as DownloadProgress events; updates are coalesced to whole-percent steps. Co-Authored-By: Claude Opus 4.8 --- iOS/Pollux/DownloadManager.swift | 128 +++++++++++++++++++++++---- iOS/Pollux/EpisodeDetailView.swift | 25 +++++- iOS/Pollux/EpisodeFormatting.swift | 23 +++++ iOS/Pollux/EpisodeRow.swift | 15 +++- iOS/Pollux/core.swift | 21 ++++- shared/src/app.rs | 132 ++++++++++++++++++++++++++-- shared/src/capabilities/download.rs | 14 ++- shared/src/model.rs | 16 ++++ shared/src/view_model.rs | 7 +- 9 files changed, 338 insertions(+), 43 deletions(-) diff --git a/iOS/Pollux/DownloadManager.swift b/iOS/Pollux/DownloadManager.swift index 297b2e2..711953b 100644 --- a/iOS/Pollux/DownloadManager.swift +++ b/iOS/Pollux/DownloadManager.swift @@ -7,6 +7,7 @@ enum DownloadManagerError: Error, LocalizedError { case storageUnavailable case invalidURL(String) case badStatus(Int) + case incomplete var errorDescription: String? { switch self { @@ -16,6 +17,8 @@ enum DownloadManagerError: Error, LocalizedError { "invalid download URL: \(url)" case let .badStatus(code): "download failed: HTTP \(code)" + case .incomplete: + "download ended without a file" } } } @@ -46,10 +49,15 @@ actor DownloadManager { downloadsDir = support.appendingPathComponent(Self.subdirectory, isDirectory: true) } - func perform(_ operation: DownloadOperation) async -> DownloadResult { + /// `onProgress(received, total)` is called with byte counts while a download runs + /// (total is nil when the server didn't report a size). It never fires for a delete. + func perform( + _ operation: DownloadOperation, + onProgress: @escaping @Sendable (UInt64, UInt64?) -> Void, + ) async -> DownloadResult { switch operation { case let .download(episodeId, url): - await download(episodeId: episodeId, urlString: url) + await download(episodeId: episodeId, urlString: url, onProgress: onProgress) case let .delete(localPath): delete(localPath: localPath) } @@ -57,7 +65,10 @@ actor DownloadManager { // MARK: - Download - private func download(episodeId: String, urlString: String) async -> DownloadResult { + private func download( + episodeId: String, urlString: String, + onProgress: @escaping @Sendable (UInt64, UInt64?) -> Void, + ) async -> DownloadResult { guard let url = URL(string: urlString) else { return .error(DownloadManagerError.invalidURL(urlString).localizedDescription) } @@ -65,25 +76,24 @@ actor DownloadManager { try FileManager.default.createDirectory( at: downloadsDir, withIntermediateDirectories: true, ) - - // PROGRESS SEAM: this uses the one-shot async download (status-only - // pass). Live 0–100% progress is added here by switching to a - // URLSessionDownloadDelegate whose `didWriteData` reports percent — the - // core already accepts `DownloadResult.progress`, so that change is - // additive and touches only this method plus the resolve loop. - let (tempURL, response) = try await URLSession.shared.download(from: url) - - if let http = response as? HTTPURLResponse, !(200 ..< 300).contains(http.statusCode) { - return .error(DownloadManagerError.badStatus(http.statusCode).localizedDescription) - } - let fileName = Self.fileName(episodeId: episodeId, url: url) let destination = downloadsDir.appendingPathComponent(fileName) - // A re-download replaces any previous file at the same path. - try? FileManager.default.removeItem(at: destination) - try FileManager.default.moveItem(at: tempURL, to: destination) - let size = fileSize(at: destination) + // A session-level delegate is required for byte progress: the async + // download(from:delegate:) convenience never invokes didWriteData. The + // delegate reports progress, moves the finished temp file to `destination` + // (the system temp file is valid only inside didFinishDownloadingTo), and + // resumes the await; a non-2xx response fails the download. + let delegate = DownloadSessionDelegate(destination: destination, onProgress: onProgress) + let session = URLSession(configuration: .default, delegate: delegate, delegateQueue: nil) + defer { session.finishTasksAndInvalidate() } + + let finalURL: URL = try await withCheckedThrowingContinuation { continuation in + delegate.attach(continuation) + session.downloadTask(with: url).resume() + } + + let size = fileSize(at: finalURL) let relativePath = "\(Self.subdirectory)/\(fileName)" return .completed(localPath: relativePath, sizeBytes: size) } catch { @@ -132,3 +142,83 @@ actor DownloadManager { return size } } + +/// Session delegate driving a single download. Reports byte progress via +/// didWriteData, moves the finished file to `destination` synchronously (the +/// system-supplied temp file is valid only inside didFinishDownloadingTo), and +/// resumes the awaiting continuation on completion. A non-2xx response fails it. +private final class DownloadSessionDelegate: NSObject, URLSessionDownloadDelegate, @unchecked Sendable { + private let destination: URL + private let onProgress: @Sendable (UInt64, UInt64?) -> Void + /// URLSession serializes a session's delegate callbacks on its delegate queue, and + /// this delegate drives exactly one task, so the mutable state below is only ever + /// touched one call at a time. + private var continuation: CheckedContinuation? + private var movedURL: URL? + private var completionError: Error? + private var lastPercent = -1 + + init(destination: URL, onProgress: @escaping @Sendable (UInt64, UInt64?) -> Void) { + self.destination = destination + self.onProgress = onProgress + } + + func attach(_ continuation: CheckedContinuation) { + self.continuation = continuation + } + + func urlSession( + _: URLSession, downloadTask _: URLSessionDownloadTask, + didWriteData _: Int64, totalBytesWritten: Int64, totalBytesExpectedToWrite: Int64, + ) { + let received = UInt64(max(0, totalBytesWritten)) + let total: UInt64? = totalBytesExpectedToWrite > 0 + ? UInt64(totalBytesExpectedToWrite) : nil + // When the size is known, coalesce to whole-percent steps so the core gets at + // most ~100 updates per download rather than one per network packet. When it's + // unknown, forward as-is (already packet-paced). + if let total, total > 0 { + let percent = Int(received * 100 / total) + guard percent != lastPercent else { return } + lastPercent = percent + } + onProgress(received, total) + } + + func urlSession( + _: URLSession, downloadTask: URLSessionDownloadTask, didFinishDownloadingTo location: URL, + ) { + let status = (downloadTask.response as? HTTPURLResponse)?.statusCode + if let status, !(200 ..< 300).contains(status) { + completionError = DownloadManagerError.badStatus(status) + return + } + do { + // A re-download replaces any previous file at the same path. + try? FileManager.default.removeItem(at: destination) + try FileManager.default.moveItem(at: location, to: destination) + movedURL = destination + } catch { + completionError = error + } + } + + func urlSession( + _: URLSession, task _: URLSessionTask, didCompleteWithError error: Error?, + ) { + if let error { + resume(with: .failure(error)) + } else if let completionError { + resume(with: .failure(completionError)) + } else if let movedURL { + resume(with: .success(movedURL)) + } else { + resume(with: .failure(DownloadManagerError.incomplete)) + } + } + + private func resume(with result: Result) { + continuation?.resume(with: result) + continuation = nil + } +} diff --git a/iOS/Pollux/EpisodeDetailView.swift b/iOS/Pollux/EpisodeDetailView.swift index 15671c3..aa79f3a 100644 --- a/iOS/Pollux/EpisodeDetailView.swift +++ b/iOS/Pollux/EpisodeDetailView.swift @@ -103,10 +103,27 @@ struct EpisodeDetailView: View { Label("Queued for download", systemImage: "clock") .foregroundStyle(.secondary) case .downloading: - Label { - Text("Downloading…") - } icon: { - ProgressView() + if let progress = EpisodeFormatting.downloadProgress( + received: liveEpisode.downloadReceivedBytes, + total: liveEpisode.downloadTotalBytes, + ) { + VStack(alignment: .leading, spacing: 6) { + if let fraction = progress.fraction { + ProgressView(value: fraction) + } else { + ProgressView() + } + Text(progress.label) + .font(.caption) + .monospacedDigit() + .foregroundStyle(.secondary) + } + } else { + Label { + Text("Downloading…") + } icon: { + ProgressView() + } } case .downloaded: HStack { diff --git a/iOS/Pollux/EpisodeFormatting.swift b/iOS/Pollux/EpisodeFormatting.swift index 5fc1bec..65106f2 100644 --- a/iOS/Pollux/EpisodeFormatting.swift +++ b/iOS/Pollux/EpisodeFormatting.swift @@ -35,10 +35,33 @@ enum EpisodeFormatting { return "\(secs)s" } + /// Live download progress for display. Returns nil before any bytes are known. + /// `fraction` is 0...1 when the total size is known (nil → show an indeterminate + /// indicator); `label` is a byte summary like "12.3 MB / 45.6 MB", or just + /// "12.3 MB" when the server didn't report a total. + static func downloadProgress( + received: UInt64?, total: UInt64?, + ) -> (fraction: Double?, label: String)? { + guard let received else { return nil } + let receivedText = byteFormatter.string(fromByteCount: Int64(clamping: received)) + if let total, total > 0 { + let totalText = byteFormatter.string(fromByteCount: Int64(clamping: total)) + let fraction = min(1.0, Double(received) / Double(total)) + return (fraction, "\(receivedText) / \(totalText)") + } + return (nil, receivedText) + } + private static let pubDateFormatter: DateFormatter = { let formatter = DateFormatter() formatter.dateStyle = .medium formatter.timeStyle = .none return formatter }() + + private static let byteFormatter: ByteCountFormatter = { + let formatter = ByteCountFormatter() + formatter.countStyle = .file + return formatter + }() } diff --git a/iOS/Pollux/EpisodeRow.swift b/iOS/Pollux/EpisodeRow.swift index 7aec9e1..8b19682 100644 --- a/iOS/Pollux/EpisodeRow.swift +++ b/iOS/Pollux/EpisodeRow.swift @@ -106,8 +106,19 @@ struct EpisodeRow: View { .foregroundStyle(.secondary) .accessibilityLabel("Queued for download") case .downloading: - ProgressView() - .accessibilityLabel("Downloading") + if let progress = EpisodeFormatting.downloadProgress( + received: episode.downloadReceivedBytes, + total: episode.downloadTotalBytes, + ), let fraction = progress.fraction { + Text("\(Int(fraction * 100))%") + .font(.caption2) + .monospacedDigit() + .foregroundStyle(.secondary) + .accessibilityLabel("Downloading \(Int(fraction * 100)) percent") + } else { + ProgressView() + .accessibilityLabel("Downloading") + } case .downloaded: Button { onDeleteDownload(episode.id) } label: { Image(systemName: "trash.circle").font(.title2) diff --git a/iOS/Pollux/core.swift b/iOS/Pollux/core.swift index 789c661..2e57454 100644 --- a/iOS/Pollux/core.swift +++ b/iOS/Pollux/core.swift @@ -76,9 +76,26 @@ class Core: ObservableObject { case let .download(operation): // The heavy I/O runs on the DownloadManager actor's executor; awaiting // it from a MainActor task only parks the continuation, so this doesn't - // block the UI (same shape as the storage arm above). + // block the UI (same shape as the storage arm above). Live byte progress + // comes back through the onProgress callback as ordinary DownloadProgress + // events while the one-shot request is still in flight. + let progressEpisodeId: String? = { + if case let .download(episodeId, _) = operation { + return episodeId + } + return nil + }() Task { @MainActor in - let result = await downloads.perform(operation) + let result = await downloads.perform(operation) { [weak self] received, total in + guard let progressEpisodeId else { return } + Task { @MainActor in + self?.update(.downloadProgress( + episodeId: progressEpisodeId, + receivedBytes: received, + totalBytes: total, + )) + } + } resolveAndDispatch(requestId: request.id, result: result) } } diff --git a/shared/src/app.rs b/shared/src/app.rs index 0056603..c06721f 100644 --- a/shared/src/app.rs +++ b/shared/src/app.rs @@ -9,7 +9,7 @@ use crate::domain::{DownloadStatus, Episode, EpisodeSortOrder}; use crate::effect::Effect; use crate::feed_parser::parse_feed; use crate::html::strip_html_preview; -use crate::model::Model; +use crate::model::{DownloadProgress, Model}; use crate::view_model::{ EpisodeSummary, LibraryView, SubscriptionDetailView, SubscriptionSummary, ViewModel, }; @@ -200,6 +200,24 @@ impl App for Pollux { } } } + Event::DownloadProgress { + episode_id, + received_bytes, + total_bytes, + } => { + // Track only the active download's progress; drop a late or stale + // report for an episode that already finished or was superseded. + if model.downloading.as_deref() == Some(episode_id.as_str()) { + model.active_download_progress = Some(DownloadProgress { + episode_id, + received_bytes, + total_bytes, + }); + render() + } else { + Command::done() + } + } Event::DownloadFinished { episode_id, result } => match *result { DownloadResult::Completed { local_path, @@ -228,9 +246,8 @@ impl App for Pollux { persist_download_state(&episode_id, DownloadStatus::Failed, None, None); persist.and(maybe_start_next(model)).and(render()) } - // Progress is not consumed in the status-only pass; Deleted never - // arrives here (it resolves DeleteDownload instead). - DownloadResult::Progress { .. } | DownloadResult::Deleted => render(), + // Deleted never arrives here (it resolves DeleteDownload instead). + DownloadResult::Deleted => render(), }, Event::DeleteDownload(episode_id) => { let local_path = model @@ -286,7 +303,7 @@ impl App for Pollux { model.detail_error = Some(e); render() } - DownloadResult::Completed { .. } | DownloadResult::Progress { .. } => render(), + DownloadResult::Completed { .. } => render(), }, Event::DownloadStatePersisted(result) => { if let StorageResult::Error(e) = *result { @@ -330,6 +347,16 @@ fn build_subscription_detail(model: &Model) -> SubscriptionDetailView { let mut episodes: Vec = model.episodes.iter().map(episode_summary).collect(); sort_episodes(&mut episodes, model.episode_sort); + // Overlay the in-flight download's live byte progress onto its row. Progress is + // transient runtime state (not stored on the Episode), so it is injected here at + // projection time for the one episode currently downloading. + if let Some(progress) = model.active_download_progress.as_ref() { + if let Some(summary) = episodes.iter_mut().find(|e| e.id == progress.episode_id) { + summary.download_received_bytes = Some(progress.received_bytes); + summary.download_total_bytes = progress.total_bytes; + } + } + let (subscription_id, title, artwork_url) = match &model.selected_subscription { Some(s) => (Some(s.id.clone()), s.title.clone(), s.artwork_url.clone()), None => (None, String::new(), None), @@ -370,7 +397,10 @@ fn episode_summary(e: &Episode) -> EpisodeSummary { playback_status: e.playback_status.clone(), playback_position_secs: e.playback_position_secs, download_status: e.download_status.clone(), - download_progress: e.download_progress, + // Live progress is overlaid in `build_subscription_detail` for the active + // download only; every other row ships without it. + download_received_bytes: None, + download_total_bytes: None, } } @@ -445,10 +475,12 @@ fn persist_download_state( .then_send(|r| Event::DownloadStatePersisted(Box::new(r))) } -/// Clears the in-flight marker when the active download reaches a terminal state. +/// Clears the in-flight marker (and its live progress) when the active download +/// reaches a terminal state. fn finish_active(model: &mut Model, episode_id: &str) { if model.downloading.as_deref() == Some(episode_id) { model.downloading = None; + model.active_download_progress = None; } } @@ -477,6 +509,8 @@ fn maybe_start_next(model: &mut Model) -> Command { }; model.downloading = Some(episode_id.clone()); + // A fresh download starts with no progress until the shell reports the first bytes. + model.active_download_progress = None; set_download_state(model, &episode_id, DownloadStatus::Downloading, None, None); let persist = persist_download_state(&episode_id, DownloadStatus::Downloading, None, None); @@ -512,6 +546,13 @@ pub enum Event { DownloadEpisode(String), /// Remove a downloaded (or failed) episode's local file. DeleteDownload(String), + /// Live byte progress for the in-flight download, reported by the shell while + /// the one-shot download request is still running. Transient — not persisted. + DownloadProgress { + episode_id: String, + received_bytes: u64, + total_bytes: Option, + }, /// Terminal result of a shell download for `episode_id`. DownloadFinished { episode_id: String, @@ -1526,6 +1567,83 @@ mod tests { assert_eq!(model.detail_error.as_deref(), Some("db locked")); } + #[test] + fn download_progress_overlays_bytes_on_the_active_episode() { + let app = Pollux; + let mut model = Model::default(); + load_episodes(&app, &mut model, vec![make_episode("e1", "Ep", Some(1))]); + let _ = app.update(Event::DownloadEpisode("e1".to_string()), &mut model); + + let _ = app.update( + Event::DownloadProgress { + episode_id: "e1".to_string(), + received_bytes: 512, + total_bytes: Some(1024), + }, + &mut model, + ); + + let summary = &app.view(&model).subscription_detail.episodes[0]; + assert_eq!(summary.download_received_bytes, Some(512)); + assert_eq!(summary.download_total_bytes, Some(1024)); + } + + #[test] + fn download_progress_for_an_inactive_episode_is_ignored() { + let app = Pollux; + let mut model = Model::default(); + load_episodes(&app, &mut model, vec![make_episode("e1", "Ep", Some(1))]); + + // Nothing is downloading, so a stray progress report must not stick. + let _ = app.update( + Event::DownloadProgress { + episode_id: "e1".to_string(), + received_bytes: 10, + total_bytes: None, + }, + &mut model, + ); + + assert!(model.active_download_progress.is_none()); + let summary = &app.view(&model).subscription_detail.episodes[0]; + assert!(summary.download_received_bytes.is_none()); + } + + #[test] + fn progress_is_cleared_when_the_download_finishes() { + let app = Pollux; + let mut model = Model::default(); + load_episodes(&app, &mut model, vec![make_episode("e1", "Ep", Some(1))]); + let _ = app.update(Event::DownloadEpisode("e1".to_string()), &mut model); + let _ = app.update( + Event::DownloadProgress { + episode_id: "e1".to_string(), + received_bytes: 512, + total_bytes: Some(1024), + }, + &mut model, + ); + assert!(model.active_download_progress.is_some()); + + let _ = app.update( + Event::DownloadFinished { + episode_id: "e1".to_string(), + result: Box::new(DownloadResult::Completed { + local_path: "Downloads/e1.mp3".to_string(), + size_bytes: 1024, + }), + }, + &mut model, + ); + + assert!( + model.active_download_progress.is_none(), + "progress must be cleared once the download ends" + ); + let summary = &app.view(&model).subscription_detail.episodes[0]; + assert!(summary.download_received_bytes.is_none()); + } + #[test] fn redownloading_an_in_flight_episode_is_a_noop() { let app = Pollux; diff --git a/shared/src/capabilities/download.rs b/shared/src/capabilities/download.rs index f4319c6..5fffad4 100644 --- a/shared/src/capabilities/download.rs +++ b/shared/src/capabilities/download.rs @@ -20,14 +20,6 @@ pub enum DownloadOperation { #[derive(Facet, Serialize, Deserialize, Clone, Debug)] #[repr(C)] pub enum DownloadResult { - /// Interim progress for an in-flight download. Defined from the start so live - /// progress reporting is an additive change (the shell can resolve a streamed - /// request with these before the terminal `Completed`); the core does not act - /// on it in the status-only first pass. - Progress { - episode_id: String, - percent: u8, - }, /// The download finished; the file is at `local_path` (relative to the app's /// storage root, resolved to absolute by the shell) and is `size_bytes` long. Completed { @@ -39,6 +31,12 @@ pub enum DownloadResult { Error(String), } +// Interim download progress is NOT a `DownloadResult`: it is reported by the shell +// as an ordinary `Event::DownloadProgress` (received/total bytes) while the one-shot +// `Download` request is still in flight. That keeps the download a simple one-shot +// (terminal Completed/Error) and keeps progress — which is transient and never +// persisted — out of the storage-backed result type. + impl Operation for DownloadOperation { type Output = DownloadResult; } diff --git a/shared/src/model.rs b/shared/src/model.rs index 13c1907..55f9199 100644 --- a/shared/src/model.rs +++ b/shared/src/model.rs @@ -21,4 +21,20 @@ pub struct Model { // follow-ups). pub download_queue: Vec, pub downloading: Option, + + // Live progress of the in-flight download (the one in `downloading`). Transient + // and never persisted: a partial download can't resume across a restart, so a + // stored percentage would only be stale. Cleared when a download ends or the + // next one starts. + pub active_download_progress: Option, +} + +/// Byte progress of the currently-downloading episode. `total_bytes` is optional +/// because a server may omit `Content-Length`; when it's `None` the shell can't +/// know the size, so the UI shows an indeterminate indicator rather than a bar. +#[derive(Clone, Debug)] +pub struct DownloadProgress { + pub episode_id: String, + pub received_bytes: u64, + pub total_bytes: Option, } diff --git a/shared/src/view_model.rs b/shared/src/view_model.rs index ebcab05..147f291 100644 --- a/shared/src/view_model.rs +++ b/shared/src/view_model.rs @@ -53,5 +53,10 @@ pub struct EpisodeSummary { pub playback_status: PlaybackStatus, pub playback_position_secs: Option, pub download_status: DownloadStatus, - pub download_progress: Option, + /// Live download progress, present only for the episode currently downloading. + /// `received`/`total` are byte counts; `total` is absent when the server didn't + /// report a size, in which case the shell shows an indeterminate indicator. + /// Transient and never persisted (see `Model::active_download_progress`). + pub download_received_bytes: Option, + pub download_total_bytes: Option, } From ba1a575149ad31521de68d60c61af79c5ef0b6da Mon Sep 17 00:00:00 2001 From: Alex Axthelm Date: Thu, 17 Sep 2026 17:11:29 +0200 Subject: [PATCH 04/38] declutter episode row download states MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Move the downloaded indicator to a small filled-download glyph inline before the date/duration instead of its own badge line, and replace that line with a progress bar (or a spinner with byte count when the size is unknown) while an episode is downloading — dropping the separate status line and the trailing percentage so the row reads less busy. Co-Authored-By: Claude Opus 4.8 --- iOS/Pollux/EpisodeRow.swift | 89 ++++++++++++++++++++----------------- 1 file changed, 49 insertions(+), 40 deletions(-) diff --git a/iOS/Pollux/EpisodeRow.swift b/iOS/Pollux/EpisodeRow.swift index 8b19682..e561038 100644 --- a/iOS/Pollux/EpisodeRow.swift +++ b/iOS/Pollux/EpisodeRow.swift @@ -22,11 +22,7 @@ struct EpisodeRow: View { .fontWeight(.semibold) .lineLimit(2) - if let meta = metaLine { - Text(meta) - .font(.caption) - .foregroundStyle(.secondary) - } + metaRow if let description = episode.descriptionText, !description.isEmpty { Text(description) @@ -49,10 +45,53 @@ struct EpisodeRow: View { EpisodeFormatting.metaLine(pubDate: episode.pubDate, durationSecs: episode.durationSecs) } + /// The date·duration line — except while downloading, when it is replaced by a + /// progress indicator. A downloaded episode gets a small filled-download glyph + /// ahead of the date so status reads inline instead of on its own badge line. + @ViewBuilder private var metaRow: some View { + if episode.downloadStatus == .downloading { + downloadingLine + } else { + HStack(spacing: 4) { + if episode.downloadStatus == .downloaded { + Image(systemName: "arrow.down.circle.fill") + .font(.caption) + .foregroundStyle(.secondary) + .accessibilityLabel("Downloaded") + } + if let meta = metaLine { + Text(meta) + .font(.caption) + .foregroundStyle(.secondary) + } + } + } + } + + /// In-progress download: a determinate bar when the size is known, otherwise a + /// spinner with whatever byte count we have. Replaces the date·duration line. + @ViewBuilder private var downloadingLine: some View { + let progress = EpisodeFormatting.downloadProgress( + received: episode.downloadReceivedBytes, + total: episode.downloadTotalBytes, + ) + if let progress, let fraction = progress.fraction { + ProgressView(value: fraction) + .accessibilityLabel("Downloading \(Int(fraction * 100)) percent") + } else { + HStack(spacing: 6) { + ProgressView() + Text(progress?.label ?? "Downloading…") + .font(.caption) + .foregroundStyle(.secondary) + } + .accessibilityLabel("Downloading \(progress?.label ?? "")") + } + } + @ViewBuilder private var statusRow: some View { let playback = playbackBadge - let download = downloadBadge - if playback != nil || download != nil || playbackPositionText != nil { + if playback != nil || playbackPositionText != nil { HStack(spacing: 10) { if let playback { StatusBadge(systemImage: playback.icon, text: playback.text) @@ -60,9 +99,6 @@ struct EpisodeRow: View { if let positionText = playbackPositionText { StatusBadge(systemImage: "clock.arrow.circlepath", text: positionText) } - if let download { - StatusBadge(systemImage: download.icon, text: download.text) - } } .padding(.top, 2) } @@ -106,19 +142,9 @@ struct EpisodeRow: View { .foregroundStyle(.secondary) .accessibilityLabel("Queued for download") case .downloading: - if let progress = EpisodeFormatting.downloadProgress( - received: episode.downloadReceivedBytes, - total: episode.downloadTotalBytes, - ), let fraction = progress.fraction { - Text("\(Int(fraction * 100))%") - .font(.caption2) - .monospacedDigit() - .foregroundStyle(.secondary) - .accessibilityLabel("Downloading \(Int(fraction * 100)) percent") - } else { - ProgressView() - .accessibilityLabel("Downloading") - } + // Progress lives on the meta line while downloading, so the trailing + // slot stays empty rather than duplicating it with a percentage. + EmptyView() case .downloaded: Button { onDeleteDownload(episode.id) } label: { Image(systemName: "trash.circle").font(.title2) @@ -155,23 +181,6 @@ struct EpisodeRow: View { } return "at \(position)" } - - private var downloadBadge: (icon: String, text: String)? { - switch episode.downloadStatus { - case .notDownloaded: - nil - case .queued: - ("clock", "Queued") - case .downloading: - ("arrow.down.circle", "Downloading") - case .downloaded: - ("arrow.down.circle.fill", "Downloaded") - case .failed: - ("exclamationmark.triangle", "Failed") - case .removedFromFeed: - ("xmark.circle", "Removed") - } - } } /// A compact read-only status indicator (real data, semantic colors). From 35236c7f4e6b64e14a13f3b143ab5d5a95f977c5 Mon Sep 17 00:00:00 2001 From: Alex Axthelm Date: Thu, 17 Sep 2026 18:35:18 +0200 Subject: [PATCH 05/38] feat: resume interrupted downloads on launch A download left mid-flight when the app quit was stranded: its status stayed Downloading in the DB but nothing was running, so the row showed a permanent spinner with no way to recover. On Started, query every episode still marked Downloading/Queued (new ListPendingDownloads storage op), rebuild the serial queue, and resume them from scratch (partial files don't survive a quit). The download queue now carries the enclosure URL per entry so a download can start without its feed's episodes being loaded. Co-Authored-By: Claude Opus 4.8 --- iOS/Pollux/DatabaseManager.swift | 16 +++ iOS/PolluxTests/DatabaseManagerTests.swift | 26 ++++ shared/src/app.rs | 153 +++++++++++++++++---- shared/src/capabilities/storage.rs | 4 + shared/src/model.rs | 20 ++- 5 files changed, 185 insertions(+), 34 deletions(-) diff --git a/iOS/Pollux/DatabaseManager.swift b/iOS/Pollux/DatabaseManager.swift index 78c9aa8..8322c9f 100644 --- a/iOS/Pollux/DatabaseManager.swift +++ b/iOS/Pollux/DatabaseManager.swift @@ -111,6 +111,8 @@ actor DatabaseManager { try getEpisode(id: id) case let .listEpisodesBySubscription(subscriptionId): try listEpisodesBySubscription(subscriptionId: subscriptionId) + case .listPendingDownloads: + try listPendingDownloads() case let .getEpisodeByFeedGuid(subscriptionId, feedGuid): try getEpisodeByFeedGuid(subscriptionId: subscriptionId, feedGuid: feedGuid) case let .updatePlaybackStatus(episodeId, status, positionSecs): @@ -210,6 +212,20 @@ actor DatabaseManager { return .episodes(rows.map(Self.episode(from:))) } + private func listPendingDownloads() throws -> StorageResult { + let rows = try db.read { db -> [Row] in + try Row.fetchAll( + db, + sql: """ + SELECT * FROM episodes + WHERE download_status IN ('Downloading', 'Queued') + ORDER BY pub_date + """, + ) + } + return .episodes(rows.map(Self.episode(from:))) + } + private func getEpisodeByFeedGuid(subscriptionId: String, feedGuid: String) throws -> StorageResult { let row = try db.read { db -> Row? in return try Row.fetchOne( diff --git a/iOS/PolluxTests/DatabaseManagerTests.swift b/iOS/PolluxTests/DatabaseManagerTests.swift index 0910f4e..1aa1f8f 100644 --- a/iOS/PolluxTests/DatabaseManagerTests.swift +++ b/iOS/PolluxTests/DatabaseManagerTests.swift @@ -279,6 +279,32 @@ struct DatabaseManagerTests { #expect(fetched.fileSizeBytes == nil) } + @Test func listPendingDownloads_returnsOnlyInFlightEpisodes() async throws { + let db = try makeManager() + try await db.execute(.upsertSubscription(makeSubscription(id: "sub-1"))) + + // One of each download status; only Downloading + Queued should come back. + let statuses: [(String, DownloadStatus)] = [ + ("ep-notdl", .notDownloaded), + ("ep-queued", .queued), + ("ep-downloading", .downloading), + ("ep-downloaded", .downloaded), + ("ep-failed", .failed), + ] + for (id, status) in statuses { + try await db.execute(.upsertEpisode( + makeEpisode(id: id, subscriptionId: "sub-1", feedGuid: "g-\(id)", downloadStatus: status), + )) + } + + let result = try await db.execute(.listPendingDownloads) + guard case let .episodes(eps) = result else { + Issue.record("Expected .episodes, got \(result)") + return + } + #expect(Set(eps.map(\.id)) == ["ep-queued", "ep-downloading"]) + } + @Test func deleteSubscription_cascadesToEpisodes() async throws { let db = try makeManager() let sub = makeSubscription(id: "sub-1") diff --git a/shared/src/app.rs b/shared/src/app.rs index c06721f..5cdb7bf 100644 --- a/shared/src/app.rs +++ b/shared/src/app.rs @@ -9,7 +9,7 @@ use crate::domain::{DownloadStatus, Episode, EpisodeSortOrder}; use crate::effect::Effect; use crate::feed_parser::parse_feed; use crate::html::strip_html_preview; -use crate::model::{DownloadProgress, Model}; +use crate::model::{DownloadProgress, Model, QueuedDownload}; use crate::view_model::{ EpisodeSummary, LibraryView, SubscriptionDetailView, SubscriptionSummary, ViewModel, }; @@ -27,9 +27,14 @@ impl App for Pollux { match event { Event::Started => { model.loading = true; - Command::request_from_shell(StorageOperation::ListSubscriptions) - .then_send(|r| Event::SubscriptionsLoaded(Box::new(r))) - .and(render()) + let subscriptions = + Command::request_from_shell(StorageOperation::ListSubscriptions) + .then_send(|r| Event::SubscriptionsLoaded(Box::new(r))); + // Rebuild the download queue for anything a previous session left + // mid-download, and resume it. + let pending = Command::request_from_shell(StorageOperation::ListPendingDownloads) + .then_send(|r| Event::PendingDownloadsLoaded(Box::new(r))); + subscriptions.and(pending).and(render()) } Event::SubscriptionsLoaded(result) => { model.loading = false; @@ -45,6 +50,37 @@ impl App for Pollux { } render() } + Event::PendingDownloadsLoaded(result) => { + // Rebuild the queue from downloads a prior session left in flight and + // resume them. Partial files don't survive a quit, so each restarts + // from scratch. Best-effort: an error here just leaves nothing queued. + let mut cmd = Command::done(); + if let StorageResult::Episodes(rows) = *result { + for episode in rows { + let already = model.downloading.as_deref() == Some(episode.id.as_str()) + || model + .download_queue + .iter() + .any(|q| q.episode_id == episode.id); + if already { + continue; + } + // Normalize to Queued so an item waiting behind others no + // longer reads as "Downloading" in the DB. + cmd = cmd.and(persist_download_state( + &episode.id, + DownloadStatus::Queued, + None, + None, + )); + model.download_queue.push(QueuedDownload { + episode_id: episode.id, + url: episode.enclosure_url, + }); + } + } + cmd.and(maybe_start_next(model)).and(render()) + } Event::FetchFeed(url) => { model.loading = true; model.error = None; @@ -174,16 +210,16 @@ impl App for Pollux { // Only actionable for an episode we have loaded (the details list // today). Already-queued/downloading episodes are a no-op so a // double-tap can't enqueue twice. - let status = model + let found = model .episodes .iter() .find(|e| e.id == episode_id) - .map(|e| e.download_status.clone()); - match status { - None | Some(DownloadStatus::Queued) | Some(DownloadStatus::Downloading) => { - render() - } - _ if !download_allowed(model) => { + .map(|e| (e.download_status.clone(), e.enclosure_url.clone())); + match found { + None + | Some((DownloadStatus::Queued, _)) + | Some((DownloadStatus::Downloading, _)) => render(), + Some(_) if !download_allowed(model) => { // Fail-on-full: unlimited today, so this is unreachable. // When a storage cap exists this is where a download is // refused up front and marked Failed instead of started. @@ -191,9 +227,12 @@ impl App for Pollux { persist_download_state(&episode_id, DownloadStatus::Failed, None, None) .and(render()) } - _ => { + Some((_, url)) => { set_download_state(model, &episode_id, DownloadStatus::Queued, None, None); - model.download_queue.push(episode_id.clone()); + model.download_queue.push(QueuedDownload { + episode_id: episode_id.clone(), + url, + }); let persist = persist_download_state(&episode_id, DownloadStatus::Queued, None, None); persist.and(maybe_start_next(model)).and(render()) @@ -492,21 +531,12 @@ fn maybe_start_next(model: &mut Model) -> Command { if model.downloading.is_some() { return Command::done(); } - let Some(episode_id) = model.download_queue.first().cloned() else { + if model.download_queue.is_empty() { return Command::done(); - }; - model.download_queue.remove(0); - - // The enclosure URL comes from the loaded episode. If it's no longer loaded, - // drop this queue entry and try the next one rather than stalling the queue. - let Some(url) = model - .episodes - .iter() - .find(|e| e.id == episode_id) - .map(|e| e.enclosure_url.clone()) - else { - return maybe_start_next(model); - }; + } + // The queue entry carries the URL, so a download can start without its feed's + // episodes being loaded (e.g. a resume at launch). + let QueuedDownload { episode_id, url } = model.download_queue.remove(0); model.downloading = Some(episode_id.clone()); // A fresh download starts with no progress until the shell reports the first bytes. @@ -530,6 +560,9 @@ fn maybe_start_next(model: &mut Model) -> Command { pub enum Event { Started, SubscriptionsLoaded(Box), + /// Episodes left `Downloading`/`Queued` by a previous session, loaded at launch + /// so their downloads can be re-enqueued and resumed. + PendingDownloadsLoaded(Box), FetchFeed(String), FeedFetched { url: String, @@ -670,13 +703,22 @@ mod tests { assert!(model.loading); let effects: Vec = cmd.effects().collect(); - assert_eq!(effects.len(), 2); + // ListSubscriptions + ListPendingDownloads + render. + assert_eq!(effects.len(), 3); let has_storage = effects .iter() .any(|e| matches!(e, Effect::Storage(r) if matches!(r.operation, StorageOperation::ListSubscriptions))); assert!(has_storage, "expected a ListSubscriptions storage effect"); + let has_pending = effects + .iter() + .any(|e| matches!(e, Effect::Storage(r) if matches!(r.operation, StorageOperation::ListPendingDownloads))); + assert!( + has_pending, + "expected a ListPendingDownloads storage effect to resume interrupted downloads" + ); + let has_render = effects.iter().any(|e| matches!(e, Effect::Render(_))); assert!(has_render, "expected a render effect"); } @@ -1331,6 +1373,14 @@ mod tests { .expect("episode should be loaded") } + fn queued_ids(model: &Model) -> Vec { + model + .download_queue + .iter() + .map(|q| q.episode_id.clone()) + .collect() + } + fn has_download_effect_for(effects: &[Effect], episode_id: &str) -> bool { effects.iter().any(|e| { matches!( @@ -1394,7 +1444,7 @@ mod tests { // e1 is still the one downloading; e2 waits its turn. assert_eq!(model.downloading.as_deref(), Some("e1")); assert_eq!(model_episode_status(&model, "e2"), DownloadStatus::Queued); - assert_eq!(model.download_queue, vec!["e2".to_string()]); + assert_eq!(queued_ids(&model), vec!["e2"]); let effects: Vec = cmd.effects().collect(); assert!( @@ -1644,6 +1694,51 @@ mod tests { assert!(summary.download_received_bytes.is_none()); } + #[test] + fn pending_downloads_are_reenqueued_and_resumed_on_load() { + let app = Pollux; + let mut model = Model::default(); + + // Two episodes a previous session left mid-download (their feed is not + // loaded — mirroring a cold start before any subscription is selected). + let mut e1 = make_episode("e1", "First", Some(2)); + e1.download_status = DownloadStatus::Downloading; + let mut e2 = make_episode("e2", "Second", Some(1)); + e2.download_status = DownloadStatus::Queued; + + let mut cmd = app.update( + Event::PendingDownloadsLoaded(Box::new(StorageResult::Episodes(vec![e1, e2]))), + &mut model, + ); + + // Serial: the first resumes, the rest wait — even though model.episodes is + // empty, because the queue carries the URL. + assert_eq!(model.downloading.as_deref(), Some("e1")); + assert_eq!(queued_ids(&model), vec!["e2"]); + + let effects: Vec = cmd.effects().collect(); + assert!( + has_download_effect_for(&effects, "e1"), + "an interrupted download should resume at launch" + ); + } + + #[test] + fn pending_downloads_loaded_empty_is_a_noop() { + let app = Pollux; + let mut model = Model::default(); + + let mut cmd = app.update( + Event::PendingDownloadsLoaded(Box::new(StorageResult::Episodes(vec![]))), + &mut model, + ); + + assert!(model.downloading.is_none()); + assert!(model.download_queue.is_empty()); + let effects: Vec = cmd.effects().collect(); + assert!(!effects.iter().any(|e| matches!(e, Effect::Download(_)))); + } + #[test] fn redownloading_an_in_flight_episode_is_a_noop() { let app = Pollux; diff --git a/shared/src/capabilities/storage.rs b/shared/src/capabilities/storage.rs index ac67a31..3b26d00 100644 --- a/shared/src/capabilities/storage.rs +++ b/shared/src/capabilities/storage.rs @@ -22,6 +22,10 @@ pub enum StorageOperation { ListEpisodesBySubscription { subscription_id: String, }, + /// Every episode currently marked `Downloading` or `Queued`, across all feeds. + /// Used at launch to rebuild the download queue for downloads interrupted by a + /// previous quit. + ListPendingDownloads, GetEpisodeByFeedGuid { subscription_id: String, feed_guid: String, diff --git a/shared/src/model.rs b/shared/src/model.rs index 55f9199..a0c7023 100644 --- a/shared/src/model.rs +++ b/shared/src/model.rs @@ -15,11 +15,12 @@ pub struct Model { pub detail_error: Option, // Download manager. Serial for MVP: at most one episode downloads at a time - // (`downloading`), the rest wait in `download_queue` (front = next up). Both - // hold episode ids. The queue is in-memory only today; episodes left in the - // `Queued` status across a cold start are not yet re-enqueued (see ROADMAP - // follow-ups). - pub download_queue: Vec, + // (`downloading` holds its id), the rest wait in `download_queue` (front = next + // up). Queue entries carry the enclosure URL so a download can start without its + // feed's episodes being loaded — this is what lets `Started` re-enqueue downloads + // interrupted by a previous quit (see `PendingDownloadsLoaded`). In-memory only: + // the queue itself is rebuilt from the DB at launch. + pub download_queue: Vec, pub downloading: Option, // Live progress of the in-flight download (the one in `downloading`). Transient @@ -29,6 +30,15 @@ pub struct Model { pub active_download_progress: Option, } +/// An episode waiting to be downloaded. Carries the enclosure URL so the download +/// can be issued from the queue alone, without the episode being loaded in +/// `model.episodes`. +#[derive(Clone, Debug)] +pub struct QueuedDownload { + pub episode_id: String, + pub url: String, +} + /// Byte progress of the currently-downloading episode. `total_bytes` is optional /// because a server may omit `Content-Length`; when it's `None` the shell can't /// know the size, so the UI shows an indeterminate indicator rather than a bar. From ce36954180a0d44aa8b7290afe5882ada6383c98 Mon Sep 17 00:00:00 2001 From: Alex Axthelm Date: Fri, 18 Sep 2026 11:44:41 +0200 Subject: [PATCH 06/38] move episode download action into the three-dots menu MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Turn the row's stubbed ellipsis into a real menu whose first item is the context-sensitive download action (Download / Retry Download / Delete Download; a disabled indicator while queued/downloading/removed). Drop the dedicated inline download button so the row is just play + ⋯, giving titles more room. Download status stays inline (meta-line glyph and progress bar) — only the action moved. Swipe shortcuts and configurable row layout are left for later. Co-Authored-By: Claude Opus 4.8 --- iOS/Pollux/EpisodeRow.swift | 67 ++++++++++++++++++------------------- 1 file changed, 32 insertions(+), 35 deletions(-) diff --git a/iOS/Pollux/EpisodeRow.swift b/iOS/Pollux/EpisodeRow.swift index e561038..8285f98 100644 --- a/iOS/Pollux/EpisodeRow.swift +++ b/iOS/Pollux/EpisodeRow.swift @@ -36,7 +36,7 @@ struct EpisodeRow: View { Spacer(minLength: 8) - placeholderControls + trailingControls } .padding(.vertical, 4) } @@ -104,59 +104,56 @@ struct EpisodeRow: View { } } - /// Trailing controls: a working download control plus the still-stubbed play - /// and more-actions buttons (their engines don't exist yet). `.borderless` - /// keeps the download button from also triggering the row's NavigationLink. - private var placeholderControls: some View { + /// Trailing controls: the still-stubbed play button (no playback engine yet) and + /// the three-dots menu, which is where the download action now lives. Download + /// *status* stays inline (`metaRow`); this menu is the *action* surface. + private var trailingControls: some View { HStack(spacing: 12) { Image(systemName: "play.circle.fill") .font(.title2) .stubbed() - downloadControl + moreMenu + } + } + + /// The three-dots menu. Today it holds only the download action (its first, + /// context-sensitive item); Play, Mark played, Flag, etc. join it as their + /// engines land. Swipe shortcuts are a later, user-configurable addition. + private var moreMenu: some View { + Menu { + downloadMenuItem + } label: { Image(systemName: "ellipsis.circle") .font(.title2) - .stubbed() } + .accessibilityLabel("More actions") } - /// The download affordance, driven by the episode's current download status. - /// Actionable states are real buttons; transient/terminal states are indicators. - @ViewBuilder private var downloadControl: some View { + /// The download entry, driven by status: an action for the states where one + /// applies, or a disabled indicator otherwise (there is no cancel yet). + @ViewBuilder private var downloadMenuItem: some View { switch episode.downloadStatus { case .notDownloaded: Button { onDownload(episode.id) } label: { - Image(systemName: "arrow.down.circle").font(.title2) + Label("Download", systemImage: "arrow.down.circle") } - .buttonStyle(.borderless) - .accessibilityLabel("Download episode") case .failed: Button { onDownload(episode.id) } label: { - Image(systemName: "arrow.clockwise.circle").font(.title2) + Label("Retry Download", systemImage: "arrow.clockwise.circle") } - .buttonStyle(.borderless) - .tint(.orange) - .accessibilityLabel("Retry download") - case .queued: - Image(systemName: "clock") - .font(.title2) - .foregroundStyle(.secondary) - .accessibilityLabel("Queued for download") - case .downloading: - // Progress lives on the meta line while downloading, so the trailing - // slot stays empty rather than duplicating it with a percentage. - EmptyView() case .downloaded: - Button { onDeleteDownload(episode.id) } label: { - Image(systemName: "trash.circle").font(.title2) + Button(role: .destructive) { onDeleteDownload(episode.id) } label: { + Label("Delete Download", systemImage: "trash") } - .buttonStyle(.borderless) - .tint(.gray) - .accessibilityLabel("Delete download") + case .queued: + Button {} label: { Label("Queued", systemImage: "clock") } + .disabled(true) + case .downloading: + Button {} label: { Label("Downloading…", systemImage: "arrow.down.circle") } + .disabled(true) case .removedFromFeed: - Image(systemName: "xmark.circle") - .font(.title2) - .foregroundStyle(.secondary) - .accessibilityLabel("Removed from feed") + Button {} label: { Label("Removed From Feed", systemImage: "xmark.circle") } + .disabled(true) } } From 84576d2c6ba452707da5e05a9c9cbebc63a56f99 Mon Sep 17 00:00:00 2001 From: Alex Axthelm Date: Fri, 18 Sep 2026 12:46:51 +0200 Subject: [PATCH 07/38] feat: swipe an episode right to download or delete MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add the spec's default leading swipe action (short swipe-right) on episode rows: Download when not downloaded, Retry when failed, Delete when downloaded (destructive). Full-swipe commits Download/Retry but not Delete, so a stray long swipe can't remove a file; queued/downloading/removed rows have no swipe (no cancel yet). Fixed behavior for now — configurable swipes arrive with the Settings screen. Co-Authored-By: Claude Opus 4.8 --- iOS/Pollux/SubscriptionDetailScreen.swift | 38 +++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/iOS/Pollux/SubscriptionDetailScreen.swift b/iOS/Pollux/SubscriptionDetailScreen.swift index c4f0227..0d60df7 100644 --- a/iOS/Pollux/SubscriptionDetailScreen.swift +++ b/iOS/Pollux/SubscriptionDetailScreen.swift @@ -76,11 +76,49 @@ struct SubscriptionDetailScreen: View { onDeleteDownload: { core.update(.deleteDownload($0)) }, ) } + // Spec default: short swipe-right → Download / Delete. This is the + // one actionable swipe today (flag / mark-played have no engine yet); + // it becomes user-configurable when Settings lands. + .swipeActions(edge: .leading, allowsFullSwipe: fullSwipeAllowed(episode)) { + downloadSwipeButton(for: episode) + } } .listStyle(.plain) } } + /// Leading-swipe download action, mirroring the row's menu: an action for the + /// states where one applies, nothing for the rest (there is no cancel yet). + @ViewBuilder private func downloadSwipeButton(for episode: EpisodeSummary) -> some View { + switch episode.downloadStatus { + case .notDownloaded: + Button { core.update(.downloadEpisode(episode.id)) } label: { + Label("Download", systemImage: "arrow.down.circle") + } + .tint(.blue) + case .failed: + Button { core.update(.downloadEpisode(episode.id)) } label: { + Label("Retry", systemImage: "arrow.clockwise.circle") + } + .tint(.orange) + case .downloaded: + Button(role: .destructive) { core.update(.deleteDownload(episode.id)) } label: { + Label("Delete", systemImage: "trash") + } + case .queued, .downloading, .removedFromFeed: + EmptyView() + } + } + + /// Full-swipe commits the action without tapping. Allowed for downloading (safe, + /// re-downloadable) but not for deleting, so a stray long swipe can't wipe a file. + private func fullSwipeAllowed(_ episode: EpisodeSummary) -> Bool { + switch episode.downloadStatus { + case .notDownloaded, .failed: true + default: false + } + } + private var sortMenu: some View { Menu { Picker("Sort", selection: sortBinding) { From 0d1a8460d9a3c317a0bf2d46e746237349a7c635 Mon Sep 17 00:00:00 2001 From: Alex Axthelm Date: Fri, 18 Sep 2026 13:16:41 +0200 Subject: [PATCH 08/38] feat: cancel an in-flight or queued download MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add a cancel action for the downloading/queued states (three-dots menu + swipe). Cancelling a queued episode just drops it from the queue; cancelling the active one asks the shell to cancel the URLSession task, and the in-flight Download request then resolves as Cancelled — resetting the episode to not-downloaded (not failed), clearing progress, and draining the queue to the next item. The shell tracks the active task so Cancel can reach it, and flushes any partial bytes. Co-Authored-By: Claude Opus 4.8 --- iOS/Pollux/DownloadManager.swift | 38 ++++- iOS/Pollux/EpisodeRow.swift | 12 +- iOS/Pollux/SubscriptionDetailScreen.swift | 7 +- shared/src/app.rs | 161 +++++++++++++++++++++- shared/src/capabilities/download.rs | 5 + 5 files changed, 213 insertions(+), 10 deletions(-) diff --git a/iOS/Pollux/DownloadManager.swift b/iOS/Pollux/DownloadManager.swift index 711953b..82d245d 100644 --- a/iOS/Pollux/DownloadManager.swift +++ b/iOS/Pollux/DownloadManager.swift @@ -36,6 +36,11 @@ actor DownloadManager { private let storageRoot: URL private let downloadsDir: URL + /// The in-flight download's task and episode, so a Cancel operation can find and + /// cancel it. At most one at a time (downloads are serial). Set before the await + /// and cleared when it returns. + private var activeTask: (episodeId: String, task: URLSessionDownloadTask)? + /// Subdirectory (and stored-path prefix) for downloaded audio. private static let subdirectory = "Downloads" @@ -60,6 +65,8 @@ actor DownloadManager { await download(episodeId: episodeId, urlString: url, onProgress: onProgress) case let .delete(localPath): delete(localPath: localPath) + case let .cancel(episodeId): + cancel(episodeId: episodeId) } } @@ -86,21 +93,48 @@ actor DownloadManager { // resumes the await; a non-2xx response fails the download. let delegate = DownloadSessionDelegate(destination: destination, onProgress: onProgress) let session = URLSession(configuration: .default, delegate: delegate, delegateQueue: nil) - defer { session.finishTasksAndInvalidate() } + let task = session.downloadTask(with: url) + // Registered before the await so a Cancel can reach it; cleared after. + activeTask = (episodeId: episodeId, task: task) + defer { + activeTask = nil + session.finishTasksAndInvalidate() + } let finalURL: URL = try await withCheckedThrowingContinuation { continuation in delegate.attach(continuation) - session.downloadTask(with: url).resume() + task.resume() } let size = fileSize(at: finalURL) let relativePath = "\(Self.subdirectory)/\(fileName)" return .completed(localPath: relativePath, sizeBytes: size) } catch { + // A cancelled task throws URLError.cancelled; URLSession discards its own + // temp file, and we clear any file that reached `destination` to be sure. + if (error as? URLError)?.code == .cancelled { + let fileName = Self.fileName(episodeId: episodeId, url: url) + try? FileManager.default.removeItem( + at: downloadsDir.appendingPathComponent(fileName), + ) + return .cancelled + } return .error(error.localizedDescription) } } + // MARK: - Cancel + + private func cancel(episodeId: String) -> DownloadResult { + // Cancelling the task makes the in-flight download throw URLError.cancelled, + // which resolves the Download request as `.cancelled`; that is where the real + // state change happens, so this just acks. + if let activeTask, activeTask.episodeId == episodeId { + activeTask.task.cancel() + } + return .cancelled + } + // MARK: - Delete private func delete(localPath: String) -> DownloadResult { diff --git a/iOS/Pollux/EpisodeRow.swift b/iOS/Pollux/EpisodeRow.swift index 8285f98..dd169d7 100644 --- a/iOS/Pollux/EpisodeRow.swift +++ b/iOS/Pollux/EpisodeRow.swift @@ -11,6 +11,8 @@ struct EpisodeRow: View { let onDownload: (String) -> Void /// Remove this episode's downloaded file. let onDeleteDownload: (String) -> Void + /// Stop this episode's in-flight or queued download. + let onCancelDownload: (String) -> Void var body: some View { HStack(alignment: .top, spacing: 12) { @@ -145,12 +147,10 @@ struct EpisodeRow: View { Button(role: .destructive) { onDeleteDownload(episode.id) } label: { Label("Delete Download", systemImage: "trash") } - case .queued: - Button {} label: { Label("Queued", systemImage: "clock") } - .disabled(true) - case .downloading: - Button {} label: { Label("Downloading…", systemImage: "arrow.down.circle") } - .disabled(true) + case .queued, .downloading: + Button(role: .destructive) { onCancelDownload(episode.id) } label: { + Label("Cancel Download", systemImage: "xmark.circle") + } case .removedFromFeed: Button {} label: { Label("Removed From Feed", systemImage: "xmark.circle") } .disabled(true) diff --git a/iOS/Pollux/SubscriptionDetailScreen.swift b/iOS/Pollux/SubscriptionDetailScreen.swift index 0d60df7..67cb8b1 100644 --- a/iOS/Pollux/SubscriptionDetailScreen.swift +++ b/iOS/Pollux/SubscriptionDetailScreen.swift @@ -74,6 +74,7 @@ struct SubscriptionDetailScreen: View { episode: episode, onDownload: { core.update(.downloadEpisode($0)) }, onDeleteDownload: { core.update(.deleteDownload($0)) }, + onCancelDownload: { core.update(.cancelDownload($0)) }, ) } // Spec default: short swipe-right → Download / Delete. This is the @@ -105,7 +106,11 @@ struct SubscriptionDetailScreen: View { Button(role: .destructive) { core.update(.deleteDownload(episode.id)) } label: { Label("Delete", systemImage: "trash") } - case .queued, .downloading, .removedFromFeed: + case .queued, .downloading: + Button(role: .destructive) { core.update(.cancelDownload(episode.id)) } label: { + Label("Cancel", systemImage: "xmark.circle") + } + case .removedFromFeed: EmptyView() } } diff --git a/shared/src/app.rs b/shared/src/app.rs index 5cdb7bf..ad36fcc 100644 --- a/shared/src/app.rs +++ b/shared/src/app.rs @@ -285,9 +285,68 @@ impl App for Pollux { persist_download_state(&episode_id, DownloadStatus::Failed, None, None); persist.and(maybe_start_next(model)).and(render()) } + DownloadResult::Cancelled => { + // The shell cancelled the task and flushed the partial file, so + // the episode goes back to not-downloaded and the queue drains on. + finish_active(model, &episode_id); + set_download_state( + model, + &episode_id, + DownloadStatus::NotDownloaded, + None, + None, + ); + let persist = persist_download_state( + &episode_id, + DownloadStatus::NotDownloaded, + None, + None, + ); + persist.and(maybe_start_next(model)).and(render()) + } // Deleted never arrives here (it resolves DeleteDownload instead). DownloadResult::Deleted => render(), }, + Event::CancelDownload(episode_id) => { + if model.downloading.as_deref() == Some(episode_id.as_str()) { + // Active download: ask the shell to cancel the task and flush the + // partial file. The in-flight Download request then resolves as + // Cancelled (handled above), which finalizes the state. + Command::request_from_shell(DownloadOperation::Cancel { + episode_id: episode_id.clone(), + }) + .then_send(|r| Event::DownloadCanceled(Box::new(r))) + .and(render()) + } else if let Some(pos) = model + .download_queue + .iter() + .position(|q| q.episode_id == episode_id) + { + // Queued but not started: just drop it from the queue and reset — + // nothing is running and no file exists yet. + model.download_queue.remove(pos); + set_download_state( + model, + &episode_id, + DownloadStatus::NotDownloaded, + None, + None, + ); + persist_download_state(&episode_id, DownloadStatus::NotDownloaded, None, None) + .and(render()) + } else { + // Not downloading and not queued — nothing to cancel. + render() + } + } + Event::DownloadCanceled(result) => { + // Best-effort: the reset rides on the Download request's Cancelled + // resolution; only surface a failure to cancel here. + if let DownloadResult::Error(e) = *result { + model.detail_error = Some(e); + } + render() + } Event::DeleteDownload(episode_id) => { let local_path = model .episodes @@ -342,7 +401,9 @@ impl App for Pollux { model.detail_error = Some(e); render() } - DownloadResult::Completed { .. } => render(), + // Only Deleted/Error arrive from a Delete request; the rest are + // unreachable but keep the match total. + DownloadResult::Completed { .. } | DownloadResult::Cancelled => render(), }, Event::DownloadStatePersisted(result) => { if let StorageResult::Error(e) = *result { @@ -579,6 +640,12 @@ pub enum Event { DownloadEpisode(String), /// Remove a downloaded (or failed) episode's local file. DeleteDownload(String), + /// Stop an in-flight or queued download and reset the episode to not-downloaded. + CancelDownload(String), + /// Result of asking the shell to cancel an in-flight download. The state reset + /// rides on the `Download` request resolving `Cancelled`; this only surfaces a + /// cancel error. + DownloadCanceled(Box), /// Live byte progress for the in-flight download, reported by the shell while /// the one-shot download request is still running. Transient — not persisted. DownloadProgress { @@ -1739,6 +1806,98 @@ mod tests { assert!(!effects.iter().any(|e| matches!(e, Effect::Download(_)))); } + #[test] + fn cancel_active_download_requests_a_shell_cancel() { + let app = Pollux; + let mut model = Model::default(); + load_episodes(&app, &mut model, vec![make_episode("e1", "Ep", Some(1))]); + let _ = app.update(Event::DownloadEpisode("e1".to_string()), &mut model); + assert_eq!(model.downloading.as_deref(), Some("e1")); + + let mut cmd = app.update(Event::CancelDownload("e1".to_string()), &mut model); + let effects: Vec = cmd.effects().collect(); + assert!( + effects.iter().any(|e| matches!( + e, + Effect::Download(r) if matches!(&r.operation, + DownloadOperation::Cancel { episode_id } if episode_id == "e1") + )), + "cancelling the active download should ask the shell to cancel the task" + ); + } + + #[test] + fn download_finished_cancelled_resets_and_starts_next() { + let app = Pollux; + let mut model = Model::default(); + load_episodes( + &app, + &mut model, + vec![ + make_episode("e1", "First", Some(2)), + make_episode("e2", "Second", Some(1)), + ], + ); + let _ = app.update(Event::DownloadEpisode("e1".to_string()), &mut model); + let _ = app.update(Event::DownloadEpisode("e2".to_string()), &mut model); + let _ = app.update(Event::CancelDownload("e1".to_string()), &mut model); + + // The shell reports the cancellation back through the Download request. + let mut cmd = app.update( + Event::DownloadFinished { + episode_id: "e1".to_string(), + result: Box::new(DownloadResult::Cancelled), + }, + &mut model, + ); + + assert_eq!( + model_episode_status(&model, "e1"), + DownloadStatus::NotDownloaded, + "a cancelled download goes back to not-downloaded, not failed" + ); + assert!(model.active_download_progress.is_none()); + // The queue drains to e2. + assert_eq!(model.downloading.as_deref(), Some("e2")); + let effects: Vec = cmd.effects().collect(); + assert!(has_download_effect_for(&effects, "e2")); + } + + #[test] + fn cancel_queued_download_drops_it_without_a_shell_cancel() { + let app = Pollux; + let mut model = Model::default(); + load_episodes( + &app, + &mut model, + vec![ + make_episode("e1", "First", Some(2)), + make_episode("e2", "Second", Some(1)), + ], + ); + let _ = app.update(Event::DownloadEpisode("e1".to_string()), &mut model); + let _ = app.update(Event::DownloadEpisode("e2".to_string()), &mut model); + assert_eq!(queued_ids(&model), vec!["e2"]); + + let mut cmd = app.update(Event::CancelDownload("e2".to_string()), &mut model); + + // e2 leaves the queue and resets; e1 keeps downloading. + assert!(model.download_queue.is_empty()); + assert_eq!( + model_episode_status(&model, "e2"), + DownloadStatus::NotDownloaded + ); + assert_eq!(model.downloading.as_deref(), Some("e1")); + let effects: Vec = cmd.effects().collect(); + assert!( + !effects.iter().any(|e| matches!( + e, + Effect::Download(r) if matches!(&r.operation, DownloadOperation::Cancel { .. }) + )), + "a queued item needs no shell cancel — nothing is running" + ); + } + #[test] fn redownloading_an_in_flight_episode_is_a_noop() { let app = Pollux; diff --git a/shared/src/capabilities/download.rs b/shared/src/capabilities/download.rs index 5fffad4..1f8b9ea 100644 --- a/shared/src/capabilities/download.rs +++ b/shared/src/capabilities/download.rs @@ -15,6 +15,9 @@ pub enum DownloadOperation { /// Remove a previously downloaded file. Idempotent: deleting a file that is /// already gone still succeeds. Delete { local_path: String }, + /// Cancel the in-flight download for `episode_id` and flush any partial bytes. + /// The pending `Download` request then resolves as `Cancelled`. + Cancel { episode_id: String }, } #[derive(Facet, Serialize, Deserialize, Clone, Debug)] @@ -28,6 +31,8 @@ pub enum DownloadResult { }, /// A delete finished (or the file was already absent). Deleted, + /// The download was cancelled; any partial file has been flushed. + Cancelled, Error(String), } From bf873e85536c9f4b58222b76049d0b5d171a81ba Mon Sep 17 00:00:00 2001 From: Alex Axthelm Date: Fri, 18 Sep 2026 13:23:50 +0200 Subject: [PATCH 09/38] better label on swipe action --- iOS/Pollux/SubscriptionDetailScreen.swift | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/iOS/Pollux/SubscriptionDetailScreen.swift b/iOS/Pollux/SubscriptionDetailScreen.swift index 67cb8b1..cf460b7 100644 --- a/iOS/Pollux/SubscriptionDetailScreen.swift +++ b/iOS/Pollux/SubscriptionDetailScreen.swift @@ -108,7 +108,11 @@ struct SubscriptionDetailScreen: View { } case .queued, .downloading: Button(role: .destructive) { core.update(.cancelDownload(episode.id)) } label: { - Label("Cancel", systemImage: "xmark.circle") + Label { + Text("Cancel\nDownload") + } icon: { + Image(systemName: "xmark.circle") + } } case .removedFromFeed: EmptyView() From d33f907c724537f877dc08d01c4149d4d4eb7104 Mon Sep 17 00:00:00 2001 From: Alex Axthelm Date: Fri, 18 Sep 2026 14:04:19 +0200 Subject: [PATCH 10/38] feat: cancel a download from the episode detail page Add a cancel button beside the progress bar (and on the queued row) in the episode detail view, so an in-flight or queued download can be stopped without going back to the list. Delete was already available there for a downloaded episode; this fills in the missing cancel affordance. Co-Authored-By: Claude Opus 4.8 --- iOS/Pollux/EpisodeDetailView.swift | 58 ++++++++++++++++++++---------- 1 file changed, 39 insertions(+), 19 deletions(-) diff --git a/iOS/Pollux/EpisodeDetailView.swift b/iOS/Pollux/EpisodeDetailView.swift index aa79f3a..11e4d85 100644 --- a/iOS/Pollux/EpisodeDetailView.swift +++ b/iOS/Pollux/EpisodeDetailView.swift @@ -100,30 +100,38 @@ struct EpisodeDetailView: View { Label("Download episode", systemImage: "arrow.down.circle") } case .queued: - Label("Queued for download", systemImage: "clock") - .foregroundStyle(.secondary) + HStack { + Label("Queued for download", systemImage: "clock") + .foregroundStyle(.secondary) + Spacer() + cancelButton + } case .downloading: - if let progress = EpisodeFormatting.downloadProgress( - received: liveEpisode.downloadReceivedBytes, - total: liveEpisode.downloadTotalBytes, - ) { + HStack(spacing: 12) { VStack(alignment: .leading, spacing: 6) { - if let fraction = progress.fraction { - ProgressView(value: fraction) + if let progress = EpisodeFormatting.downloadProgress( + received: liveEpisode.downloadReceivedBytes, + total: liveEpisode.downloadTotalBytes, + ) { + if let fraction = progress.fraction { + ProgressView(value: fraction) + } else { + ProgressView() + } + Text(progress.label) + .font(.caption) + .monospacedDigit() + .foregroundStyle(.secondary) } else { - ProgressView() + Label { + Text("Downloading…") + } icon: { + ProgressView() + } } - Text(progress.label) - .font(.caption) - .monospacedDigit() - .foregroundStyle(.secondary) - } - } else { - Label { - Text("Downloading…") - } icon: { - ProgressView() } + .frame(maxWidth: .infinity, alignment: .leading) + cancelButton } case .downloaded: HStack { @@ -149,6 +157,18 @@ struct EpisodeDetailView: View { } } + /// Small cancel affordance shown beside the progress bar (and the queued row). + private var cancelButton: some View { + Button(role: .destructive) { + core.update(.cancelDownload(episode.id)) + } label: { + Image(systemName: "xmark.circle.fill") + .font(.title2) + } + .buttonStyle(.borderless) + .accessibilityLabel("Cancel download") + } + private var showNotes: some View { VStack(alignment: .leading, spacing: 6) { Text("Show notes") From e616900cc23843888933ef1e0b393e2b0916b795 Mon Sep 17 00:00:00 2001 From: Alex Axthelm Date: Sat, 19 Sep 2026 11:52:31 +0200 Subject: [PATCH 11/38] fix: don't re-download an already-downloaded episode Restrict DownloadEpisode to the NotDownloaded/Failed states; every other state (Downloaded, Queued, Downloading, RemovedFromFeed, or an unknown id) is now an explicit no-op, so a stray or repeated event can't re-fetch a file we already have or double-enqueue. Co-Authored-By: Claude Opus 4.8 --- shared/src/app.rs | 89 +++++++++++++++++++++++++++++++++++------------ 1 file changed, 66 insertions(+), 23 deletions(-) diff --git a/shared/src/app.rs b/shared/src/app.rs index ad36fcc..aabe1f9 100644 --- a/shared/src/app.rs +++ b/shared/src/app.rs @@ -207,36 +207,54 @@ impl App for Pollux { render() } Event::DownloadEpisode(episode_id) => { - // Only actionable for an episode we have loaded (the details list - // today). Already-queued/downloading episodes are a no-op so a - // double-tap can't enqueue twice. + // Only a not-downloaded or failed (retry) episode can start a + // download. Every other state — already queued/downloading, already + // downloaded, removed from feed, or an episode we don't have loaded — + // is a no-op, so a stray or repeated event can't double-enqueue or + // re-download a file we already have. let found = model .episodes .iter() .find(|e| e.id == episode_id) .map(|e| (e.download_status.clone(), e.enclosure_url.clone())); match found { - None - | Some((DownloadStatus::Queued, _)) - | Some((DownloadStatus::Downloading, _)) => render(), - Some(_) if !download_allowed(model) => { - // Fail-on-full: unlimited today, so this is unreachable. - // When a storage cap exists this is where a download is - // refused up front and marked Failed instead of started. - set_download_state(model, &episode_id, DownloadStatus::Failed, None, None); - persist_download_state(&episode_id, DownloadStatus::Failed, None, None) - .and(render()) - } - Some((_, url)) => { - set_download_state(model, &episode_id, DownloadStatus::Queued, None, None); - model.download_queue.push(QueuedDownload { - episode_id: episode_id.clone(), - url, - }); - let persist = - persist_download_state(&episode_id, DownloadStatus::Queued, None, None); - persist.and(maybe_start_next(model)).and(render()) + Some((DownloadStatus::NotDownloaded, url)) + | Some((DownloadStatus::Failed, url)) => { + if !download_allowed(model) { + // Fail-on-full: unlimited today, so this is unreachable. + // When a storage cap exists this is where a download is + // refused up front and marked Failed instead of started. + set_download_state( + model, + &episode_id, + DownloadStatus::Failed, + None, + None, + ); + persist_download_state(&episode_id, DownloadStatus::Failed, None, None) + .and(render()) + } else { + set_download_state( + model, + &episode_id, + DownloadStatus::Queued, + None, + None, + ); + model.download_queue.push(QueuedDownload { + episode_id: episode_id.clone(), + url, + }); + let persist = persist_download_state( + &episode_id, + DownloadStatus::Queued, + None, + None, + ); + persist.and(maybe_start_next(model)).and(render()) + } } + _ => render(), } } Event::DownloadProgress { @@ -1898,6 +1916,31 @@ mod tests { ); } + #[test] + fn downloading_an_already_downloaded_episode_is_a_noop() { + let app = Pollux; + let mut model = Model::default(); + let mut downloaded = make_episode("e1", "Ep", Some(1)); + downloaded.download_status = DownloadStatus::Downloaded; + downloaded.local_path = Some("Downloads/e1.mp3".to_string()); + load_episodes(&app, &mut model, vec![downloaded]); + + // A stray DownloadEpisode must not re-download a file we already have. + let mut cmd = app.update(Event::DownloadEpisode("e1".to_string()), &mut model); + + assert_eq!( + model_episode_status(&model, "e1"), + DownloadStatus::Downloaded + ); + assert!(model.download_queue.is_empty()); + assert!(model.downloading.is_none()); + let effects: Vec = cmd.effects().collect(); + assert!( + !has_download_effect_for(&effects, "e1"), + "downloaded episode must not be re-fetched" + ); + } + #[test] fn redownloading_an_in_flight_episode_is_a_noop() { let app = Pollux; From 2efc4fae0c07e96e63278ae6f9e31cf017f988e7 Mon Sep 17 00:00:00 2001 From: Alex Axthelm Date: Sat, 19 Sep 2026 16:37:51 +0200 Subject: [PATCH 12/38] guard DownloadSessionDelegate state with a lock MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The download delegate's continuation was written from the DownloadManager actor (via attach) but read/cleared on URLSession's delegate queue — an unsynchronized cross-thread access that @unchecked Sendable hid. Guard all of the delegate's mutable state with an NSLock; snapshot under the lock and resume the continuation outside it so resuming the awaiting task can't deadlock. Co-Authored-By: Claude Opus 4.8 --- iOS/Pollux/DownloadManager.swift | 58 ++++++++++++++++++++++---------- 1 file changed, 41 insertions(+), 17 deletions(-) diff --git a/iOS/Pollux/DownloadManager.swift b/iOS/Pollux/DownloadManager.swift index 82d245d..090d1da 100644 --- a/iOS/Pollux/DownloadManager.swift +++ b/iOS/Pollux/DownloadManager.swift @@ -181,12 +181,14 @@ actor DownloadManager { /// didWriteData, moves the finished file to `destination` synchronously (the /// system-supplied temp file is valid only inside didFinishDownloadingTo), and /// resumes the awaiting continuation on completion. A non-2xx response fails it. +/// +/// The delegate callbacks run on URLSession's delegate queue while `attach` is +/// called from the caller's thread, so all mutable state is guarded by `lock` +/// (which is what makes the `@unchecked Sendable` sound). private final class DownloadSessionDelegate: NSObject, URLSessionDownloadDelegate, @unchecked Sendable { private let destination: URL private let onProgress: @Sendable (UInt64, UInt64?) -> Void - /// URLSession serializes a session's delegate callbacks on its delegate queue, and - /// this delegate drives exactly one task, so the mutable state below is only ever - /// touched one call at a time. + private let lock = NSLock() private var continuation: CheckedContinuation? private var movedURL: URL? private var completionError: Error? @@ -198,6 +200,8 @@ private final class DownloadSessionDelegate: NSObject, URLSessionDownloadDelegat } func attach(_ continuation: CheckedContinuation) { + lock.lock() + defer { lock.unlock() } self.continuation = continuation } @@ -210,11 +214,18 @@ private final class DownloadSessionDelegate: NSObject, URLSessionDownloadDelegat ? UInt64(totalBytesExpectedToWrite) : nil // When the size is known, coalesce to whole-percent steps so the core gets at // most ~100 updates per download rather than one per network packet. When it's - // unknown, forward as-is (already packet-paced). + // unknown, forward as-is (already packet-paced). onProgress runs outside the lock. if let total, total > 0 { let percent = Int(received * 100 / total) - guard percent != lastPercent else { return } - lastPercent = percent + lock.lock() + let unchanged = percent == lastPercent + if !unchanged { + lastPercent = percent + } + lock.unlock() + if unchanged { + return + } } onProgress(received, total) } @@ -224,35 +235,48 @@ private final class DownloadSessionDelegate: NSObject, URLSessionDownloadDelegat ) { let status = (downloadTask.response as? HTTPURLResponse)?.statusCode if let status, !(200 ..< 300).contains(status) { - completionError = DownloadManagerError.badStatus(status) + setCompletionError(DownloadManagerError.badStatus(status)) return } do { // A re-download replaces any previous file at the same path. try? FileManager.default.removeItem(at: destination) try FileManager.default.moveItem(at: location, to: destination) + lock.lock() movedURL = destination + lock.unlock() } catch { - completionError = error + setCompletionError(error) } } func urlSession( _: URLSession, task _: URLSessionTask, didCompleteWithError error: Error?, ) { + // Snapshot under the lock, then resume outside it (resume can run the awaiting + // task, which must not happen while we hold the lock). + lock.lock() + let continuation = continuation + self.continuation = nil + let finishError = completionError + let moved = movedURL + lock.unlock() + + guard let continuation else { return } if let error { - resume(with: .failure(error)) - } else if let completionError { - resume(with: .failure(completionError)) - } else if let movedURL { - resume(with: .success(movedURL)) + continuation.resume(throwing: error) + } else if let finishError { + continuation.resume(throwing: finishError) + } else if let moved { + continuation.resume(returning: moved) } else { - resume(with: .failure(DownloadManagerError.incomplete)) + continuation.resume(throwing: DownloadManagerError.incomplete) } } - private func resume(with result: Result) { - continuation?.resume(with: result) - continuation = nil + private func setCompletionError(_ error: Error) { + lock.lock() + defer { lock.unlock() } + completionError = error } } From 53d38ac65f20b157ac961769fc07ae7e5fde3cfa Mon Sep 17 00:00:00 2001 From: Alex Axthelm Date: Sat, 19 Sep 2026 16:38:31 +0200 Subject: [PATCH 13/38] rm PLAN --- PLAN.md | 195 -------------------------------------------------------- 1 file changed, 195 deletions(-) delete mode 100644 PLAN.md diff --git a/PLAN.md b/PLAN.md deleted file mode 100644 index 441164b..0000000 --- a/PLAN.md +++ /dev/null @@ -1,195 +0,0 @@ -# Download Manager (Phase 0) - -## Context - -Pollux's Phase 0 foundation is nearly complete: feed fetch → parse → save, the -library, subscribe flow, and subscription details with an episode list all work. -The `Episode` schema already carries `download_status`, `download_progress`, -`file_size_bytes`, and `local_path` (see `shared/src/domain/episode.rs`), and the -DB persists them (`iOS/Pollux/DatabaseManager.swift`), but **nothing drives those -fields** — the download control in `EpisodeRow.swift` is an inert DEBUG-tinted -stub. This change implements the download manager: a serial queue in the core, a -new `Download` shell capability that actually fetches and stores audio files, and -wiring the episode-row control so a user can download an episode, watch its status, -and delete the local file. - -Scope decisions (confirmed with the user): -- **Progress: status-only for the first pass**, but the capability and shell are - structured so live 0–100% progress can be added *before this PR merges* without - a rewrite (a `Progress` result variant + a URLSession delegate seam are stubbed - in from the start). -- **Storage soft-cap: deferred.** The user-configurable cap depends on the Settings - screen (not built yet). We build the pre-download check as a seam that defaults to - *unlimited*, and rely on real device-full write errors for fail-on-full. The - configurable cap lands when Settings does. -- **Dedicated Downloads/Storage screens are Phase 5**, not this change. This change - makes the per-episode control functional; a queue screen is a follow-up. - -Roadmap item: Phase 0 — "Download manager: queue, serial downloads for MVP, storage -limit check before download, fail-on-full behavior" (`docs/ROADMAP.md`). -Feature specs: `docs/features/storage.md`, `docs/features/episode.md`. - ---- - -## Approach - -Follow the existing capability pattern exactly (mirror `capabilities/http.rs` and -its shell handler in `core.swift`). Downloads are shell I/O; the core only tracks -queue + status and persists transitions through the existing `Storage` capability. - -### 1. New `Download` capability (Rust) - -`shared/src/capabilities/download.rs` (new), registered in -`shared/src/capabilities/mod.rs`: - -```rust -pub enum DownloadOperation { - Download { episode_id: String, url: String }, - Delete { local_path: String }, -} - -pub enum DownloadResult { - // Present from day one so adding live progress later is additive, not a - // signature change. Unused by the core in the first pass. - Progress { episode_id: String, percent: u8 }, - Completed { local_path: String, size_bytes: u64 }, - Deleted, - Error(String), -} -impl Operation for DownloadOperation { type Output = DownloadResult; } -``` - -Add `Download(DownloadOperation)` to the `Effect` enum in `shared/src/effect.rs` -(same shape as the existing `Http`/`Storage` arms). - -### 2. Core: queue + serial execution (`shared/src/app.rs`, `model.rs`) - -- **Model** (`model.rs`): add `download_queue: Vec` (episode ids waiting) - and `downloading: Option` (the one in flight — enforces *serial*). -- **Events** (`app.rs`): `DownloadEpisode(String)`, `DeleteDownload(String)`, - `DownloadFinished { episode_id, result: Box }`. -- **`DownloadEpisode`**: the pre-download **limit-check seam** lives here - (`fn download_allowed(model) -> bool`, returns `true` today = unlimited). If the - episode is already downloading/queued, no-op. Otherwise mark it `Queued` - (persist via `StorageOperation::UpsertEpisode` or a narrower status update — see - note below), push to queue, and call `maybe_start_next`. -- **`maybe_start_next`**: if `downloading.is_none()` and the queue is non-empty, - pop the front, set `downloading`, mark it `Downloading`, and emit a - `DownloadOperation::Download` request whose resolution sends - `DownloadFinished`. -- **`DownloadFinished`**: on `Completed`, set status `Downloaded` + store - `local_path`/`file_size_bytes`; on `Error`, set status `Failed`. Either way clear - `downloading` and call `maybe_start_next` (serial drain). Persist the transition. -- **`DeleteDownload`**: emit `DownloadOperation::Delete { local_path }`, and on the - result reset the row to `NotDownloaded` with `local_path`/`size`/`progress` - cleared, persisted. -- Reuse the existing selection-correlation discipline already in - `EpisodesLoaded`: after any status change to an episode of the currently-selected - feed, the details list must reflect it. **This is the exact hazard flagged at - `app.rs:107` / `subscription.md:73`** — `SelectSubscription` is idempotent, so a - status write must update `model.episodes` in place (preferred) rather than relying - on re-selection. Update the matching entry in `model.episodes` directly in the - event handlers so `view()` reprojects it. - -**Storage op note:** there is no targeted "update download status" storage op yet -(only `UpdatePlaybackStatus`). Add `StorageOperation::UpdateDownloadState { episode_id, -status, local_path, size_bytes, progress }` mirroring `UpdatePlaybackStatus`, and -implement it in `DatabaseManager.swift` (a single `UPDATE`). Prefer this over -re-upserting the whole episode. - -### 3. Shell: Download handler (`iOS/Pollux/core.swift` + new file) - -- Add a `case let .download(operation)` arm in `processEffect` (mirror `.http`). -- New `iOS/Pollux/DownloadManager.swift`: performs the URLSession download to a - file in Application Support (reuse the `DatabaseManager` directory convention — - a `Downloads/` subdir; store a **relative** `local_path` so it survives container - path changes, resolving to absolute at read time), returns - `.completed(localPath:sizeBytes:)` or `.error`. `Delete` removes the file - (idempotent if already gone). Structure the download with a - `URLSessionDownloadDelegate` so the `didWriteData` progress callback is the single - seam where live-% reporting gets added before merge (first pass: ignore it). -- Add `resolveAndDispatch(_:result: DownloadResult)` overload (mirror the Http one). -- Serial execution is enforced by the **core** (`downloading` guard), so the shell - handler stays a simple one-shot per request. - -### 4. UI wiring (`iOS/Pollux/EpisodeRow.swift`, `EpisodeDetailView.swift`) - -- Replace the stubbed download placeholder in `placeholderControls` with a real - control driven by `episode.downloadStatus`: - - `notDownloaded` → download button → `core.update(.downloadEpisode(id))` - - `queued` / `downloading` → progress/indeterminate indicator (the existing - `downloadBadge` already renders these states; keep the badge, make the control - reflect activity) - - `downloaded` → "downloaded" affordance with a delete action → - `core.update(.deleteDownload(id))` - - `failed` → retry → `core.update(.downloadEpisode(id))` -- `EpisodeRow` takes only `episode: EpisodeSummary` today; thread the `core` (or an - action closure) in the same way `SubscriptionDetailScreen` already passes `core`. -- Keep the play/more-actions stubs as-is (out of scope). -- No standalone Downloads screen in this change (Phase 5). - -### 5. Codegen + tests - -- Regenerate Swift types after the Rust changes: `make ios-build` (runs `typegen` - + `package` + `generate-project`). Required whenever `Event`/`Effect`/capability - enums change. -- **Rust unit tests** in `app.rs` (follow the existing `mod tests` style): enqueue - → `Downloading` + one `Download` effect; a second enqueue while one is in flight - stays `Queued` and emits no new download effect (serial); `Completed` transitions - to `Downloaded`, stores path/size, and starts the next queued item; `Error` - transitions to `Failed` and still drains the queue; delete resets to - `NotDownloaded`; a status change to a selected-feed episode is reflected in - `view().subscription_detail` without re-selection. -- **Swift tests** (`iOS/PolluxTests/`): a `DatabaseManagerTests` case for the new - `UpdateDownloadState` op (round-trips status + path + size). A focused - `DownloadManager` test for the delete path is optional if network mocking is - heavy — keep to what's cheap. - ---- - -## Critical files - -- `shared/src/capabilities/download.rs` — **new** capability -- `shared/src/capabilities/mod.rs` — register it -- `shared/src/capabilities/storage.rs` — add `UpdateDownloadState` -- `shared/src/effect.rs` — add `Download` effect arm -- `shared/src/model.rs` — `download_queue`, `downloading` -- `shared/src/app.rs` — events, queue drain, in-place episode status updates, tests -- `iOS/Pollux/core.swift` — `.download` effect arm + resolve overload -- `iOS/Pollux/DownloadManager.swift` — **new** shell downloader (delegate seam) -- `iOS/Pollux/DatabaseManager.swift` — implement `UpdateDownloadState` -- `iOS/Pollux/EpisodeRow.swift` / `EpisodeDetailView.swift` — functional control -- `iOS/PolluxTests/DatabaseManagerTests.swift` — new-op coverage - -Reuse: the `HttpOperation`/`updatePlaybackStatus` patterns are the templates for -the capability and the storage op respectively; `DebugStyle`/`.stubbed()` is what's -being *removed* from the download control. - ---- - -## Verification - -1. `make test` — Rust core tests (`cargo test`) + iOS tests. New queue/status tests - must pass. -2. `make check` — `cargo check/clippy -D warnings/fmt --check/--locked`, swiftlint - `--strict`, swiftformat `--lint`. CI runs the same; clippy warnings and the - admin forbidden-pattern scan (no force-unwrap/try, no `unsafe`) will fail the - build otherwise. -3. `make ios-build` — confirm codegen succeeds and the generated Swift `Event`/ - `Request` types include the new download variants. -4. Manual on simulator (`make ios-sim`, or the iOS Simulator MCP — pick the newest - `Pollux.app` by mtime per the DerivedData memory note): subscribe to a real feed, - open a subscription, tap download on an episode → status goes - Queued → Downloading → Downloaded; queue a second while the first runs → it - waits (serial); delete a download → returns to Not downloaded; kill and relaunch - → downloaded episodes still read as Downloaded (persisted). Use `NSLog` (not - `print`) for any temporary logging. - -## Explicit follow-ups (not in this change) - -- Live 0–100% progress via the stubbed URLSession delegate seam + `Progress` result - variant (**intended before this PR merges**, per the user). -- User-configurable storage soft-cap + the pre-download limit check made real (needs - the Settings screen). -- Dedicated Downloads queue screen and Storage usage breakdown (Phase 5). -- Cold-start re-enqueue of episodes left in `Queued` status. From dd7eede4d9b285226c84379e52d99283c8e48eed Mon Sep 17 00:00:00 2001 From: Alex Axthelm Date: Sat, 19 Sep 2026 16:56:05 +0200 Subject: [PATCH 14/38] refactor: deliver download progress in order via a stream Route byte progress through a single AsyncStream drained by one consumer, instead of spawning a MainActor Task per callback (which could reorder and made the displayed percentage briefly regress). Extract the download effect handling into handleDownload(_:requestId:) to keep processEffect within the body-length limit. Co-Authored-By: Claude Opus 4.8 --- iOS/Pollux/core.swift | 55 ++++++++++++++++++++++++++----------------- 1 file changed, 33 insertions(+), 22 deletions(-) diff --git a/iOS/Pollux/core.swift b/iOS/Pollux/core.swift index 2e57454..d52e950 100644 --- a/iOS/Pollux/core.swift +++ b/iOS/Pollux/core.swift @@ -74,30 +74,41 @@ class Core: ObservableObject { } case let .download(operation): - // The heavy I/O runs on the DownloadManager actor's executor; awaiting - // it from a MainActor task only parks the continuation, so this doesn't - // block the UI (same shape as the storage arm above). Live byte progress - // comes back through the onProgress callback as ordinary DownloadProgress - // events while the one-shot request is still in flight. - let progressEpisodeId: String? = { - if case let .download(episodeId, _) = operation { - return episodeId - } - return nil - }() - Task { @MainActor in - let result = await downloads.perform(operation) { [weak self] received, total in - guard let progressEpisodeId else { return } - Task { @MainActor in - self?.update(.downloadProgress( - episodeId: progressEpisodeId, - receivedBytes: received, - totalBytes: total, - )) - } + handleDownload(operation, requestId: request.id) + } + } + + /// Runs a download-capability operation on the DownloadManager actor (so the UI + /// isn't blocked) and resolves the request with its terminal result. Byte progress + /// is funnelled through a stream drained by a single consumer, so DownloadProgress + /// events reach the core in order — a Task-per-callback could reorder them. + private func handleDownload(_ operation: DownloadOperation, requestId: UInt32) { + let progressEpisodeId: String? = { + if case let .download(episodeId, _) = operation { + return episodeId + } + return nil + }() + Task { @MainActor in + let (progressStream, progressContinuation) = + AsyncStream.makeStream(of: (UInt64, UInt64?).self) + let consumer = Task { @MainActor [weak self] in + for await (received, total) in progressStream { + guard let progressEpisodeId else { continue } + self?.update(.downloadProgress( + episodeId: progressEpisodeId, + receivedBytes: received, + totalBytes: total, + )) } - resolveAndDispatch(requestId: request.id, result: result) } + let result = await downloads.perform(operation) { received, total in + progressContinuation.yield((received, total)) + } + // Drain any buffered progress before the terminal result lands. + progressContinuation.finish() + await consumer.value + resolveAndDispatch(requestId: requestId, result: result) } } From 90d4091ba66374b026be1daab2753d3f5bb1ee9e Mon Sep 17 00:00:00 2001 From: Alex Axthelm Date: Sat, 19 Sep 2026 18:08:33 +0200 Subject: [PATCH 15/38] show an inline glyph for queued/failed/removed episodes Moving the download action into the menu left queued, failed, and removed episodes looking identical to a plain not-downloaded row. Add a small status glyph before the date (clock / warning-in-error-color / xmark; the existing download glyph for downloaded), so those states read at a glance. (Copilot #2.) Co-Authored-By: Claude Opus 4.8 --- iOS/Pollux/EpisodeRow.swift | 38 +++++++++++++++++++++++++++++++------ 1 file changed, 32 insertions(+), 6 deletions(-) diff --git a/iOS/Pollux/EpisodeRow.swift b/iOS/Pollux/EpisodeRow.swift index 2d8d46b..d114c3f 100644 --- a/iOS/Pollux/EpisodeRow.swift +++ b/iOS/Pollux/EpisodeRow.swift @@ -50,18 +50,19 @@ struct EpisodeRow: View { } /// The date·duration line — except while downloading, when it is replaced by a - /// progress indicator. A downloaded episode gets a small filled-download glyph - /// ahead of the date so status reads inline instead of on its own badge line. + /// progress indicator. Non-not-downloaded states get a small glyph ahead of the + /// date so status reads inline instead of on its own badge line — otherwise + /// queued/failed/removed rows would look identical to a plain undownloaded one. @ViewBuilder private var metaRow: some View { if episode.downloadStatus == .downloading { downloadingLine } else { HStack(spacing: 4) { - if episode.downloadStatus == .downloaded { - Image(systemName: "arrow.down.circle.fill") + if let glyph = statusGlyph { + Image(systemName: glyph.icon) .font(.caption) - .foregroundStyle(themeColors.secondaryText) - .accessibilityLabel("Downloaded") + .foregroundStyle(glyph.tint) + .accessibilityLabel(glyph.label) } if let meta = metaLine { Text(meta) @@ -72,6 +73,24 @@ struct EpisodeRow: View { } } + /// The inline download-status glyph for the date line. `nil` for not-downloaded + /// (a plain date) and downloading (handled by `downloadingLine`). Failed uses the + /// error color to stand out; the rest are neutral. + private var statusGlyph: StatusGlyph? { + switch episode.downloadStatus { + case .downloaded: + StatusGlyph(icon: "arrow.down.circle.fill", tint: themeColors.secondaryText, label: "Downloaded") + case .queued: + StatusGlyph(icon: "clock", tint: themeColors.secondaryText, label: "Queued for download") + case .failed: + StatusGlyph(icon: "exclamationmark.triangle.fill", tint: themeColors.error, label: "Download failed") + case .removedFromFeed: + StatusGlyph(icon: "xmark.circle", tint: themeColors.secondaryText, label: "Removed from feed") + case .notDownloaded, .downloading: + nil + } + } + /// In-progress download: a determinate bar when the size is known, otherwise a /// spinner with whatever byte count we have. Replaces the date·duration line. @ViewBuilder private var downloadingLine: some View { @@ -182,6 +201,13 @@ struct EpisodeRow: View { } } +/// Inline download-status glyph shown before the date on an episode row. +private struct StatusGlyph { + let icon: String + let tint: Color + let label: String +} + /// A compact read-only status indicator (real data, theme colors). private struct StatusBadge: View { @Environment(\.themeColors) private var themeColors From 12b1d887e8a062e202041f837d7df8961adcfd8f Mon Sep 17 00:00:00 2001 From: Alex Axthelm Date: Sat, 19 Sep 2026 18:55:00 +0200 Subject: [PATCH 16/38] feat: surface the reason a download failed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Keep the shell's specific failure reason (disk full, HTTP status, filesystem error) per episode instead of discarding it, and show it beside Retry — on the detail page under "Download failed", and inline on the row in place of the date. Stored in-memory and cleared when the episode leaves the Failed state. (Copilot #3.) Co-Authored-By: Claude Opus 4.8 --- iOS/Pollux/EpisodeDetailView.swift | 18 ++++++--- iOS/Pollux/EpisodeRow.swift | 14 ++++++- shared/src/app.rs | 65 ++++++++++++++++++++++++++++-- shared/src/model.rs | 9 +++++ shared/src/view_model.rs | 4 ++ 5 files changed, 100 insertions(+), 10 deletions(-) diff --git a/iOS/Pollux/EpisodeDetailView.swift b/iOS/Pollux/EpisodeDetailView.swift index b5b41ed..7e09175 100644 --- a/iOS/Pollux/EpisodeDetailView.swift +++ b/iOS/Pollux/EpisodeDetailView.swift @@ -148,11 +148,19 @@ struct EpisodeDetailView: View { } } case .failed: - HStack { - Label("Download failed", systemImage: "exclamationmark.triangle") - .foregroundStyle(.orange) - Spacer() - Button("Retry") { core.update(.downloadEpisode(episode.id)) } + VStack(alignment: .leading, spacing: 6) { + HStack { + Label("Download failed", systemImage: "exclamationmark.triangle") + .foregroundStyle(themeColors.error) + Spacer() + Button("Retry") { core.update(.downloadEpisode(episode.id)) } + } + if let reason = liveEpisode.downloadError { + Text(reason) + .font(.caption) + .foregroundStyle(themeColors.secondaryText) + .fixedSize(horizontal: false, vertical: true) + } } case .removedFromFeed: Label("Removed from feed", systemImage: "xmark.circle") diff --git a/iOS/Pollux/EpisodeRow.swift b/iOS/Pollux/EpisodeRow.swift index d114c3f..af7d842 100644 --- a/iOS/Pollux/EpisodeRow.swift +++ b/iOS/Pollux/EpisodeRow.swift @@ -64,15 +64,25 @@ struct EpisodeRow: View { .foregroundStyle(glyph.tint) .accessibilityLabel(glyph.label) } - if let meta = metaLine { - Text(meta) + if let text = metaText { + Text(text) .font(.caption) + .lineLimit(1) .foregroundStyle(themeColors.secondaryText) } } } } + /// The date·duration line, replaced by the failure reason when a download failed + /// so the row says *why* (disk full, HTTP status, …) next to the warning glyph. + private var metaText: String? { + if episode.downloadStatus == .failed, let error = episode.downloadError { + return error + } + return metaLine + } + /// The inline download-status glyph for the date line. `nil` for not-downloaded /// (a plain date) and downloading (handled by `downloadingLine`). Failed uses the /// error color to stand out; the rest are neutral. diff --git a/shared/src/app.rs b/shared/src/app.rs index 0c095cb..4d59cdf 100644 --- a/shared/src/app.rs +++ b/shared/src/app.rs @@ -297,9 +297,12 @@ impl App for Pollux { ); persist.and(maybe_start_next(model)).and(render()) } - DownloadResult::Error(_) => { + DownloadResult::Error(reason) => { finish_active(model, &episode_id); set_download_state(model, &episode_id, DownloadStatus::Failed, None, None); + // Keep the shell's specific reason so the UI can show it beside + // Retry (set_download_state cleared any older one above). + model.download_errors.insert(episode_id.clone(), reason); let persist = persist_download_state(&episode_id, DownloadStatus::Failed, None, None); persist.and(maybe_start_next(model)).and(render()) @@ -486,6 +489,11 @@ fn build_subscription_detail(model: &Model) -> SubscriptionDetailView { } } + // Overlay each failed episode's reason (also transient, not stored on the Episode). + for summary in episodes.iter_mut() { + summary.download_error = model.download_errors.get(&summary.id).cloned(); + } + let (subscription_id, title, artwork_url) = match &model.selected_subscription { Some(s) => (Some(s.id.clone()), s.title.clone(), s.artwork_url.clone()), None => (None, String::new(), None), @@ -526,10 +534,11 @@ fn episode_summary(e: &Episode) -> EpisodeSummary { playback_status: e.playback_status.clone(), playback_position_secs: e.playback_position_secs, download_status: e.download_status.clone(), - // Live progress is overlaid in `build_subscription_detail` for the active - // download only; every other row ships without it. + // Live progress and the failure reason are overlaid in + // `build_subscription_detail`; every row ships without them here. download_received_bytes: None, download_total_bytes: None, + download_error: None, } } @@ -574,6 +583,11 @@ fn set_download_state( local_path: Option, size_bytes: Option, ) { + // A recorded failure reason is only meaningful while the episode is Failed; every + // other transition drops it. The `Error` handler re-inserts after calling this. + if !matches!(status, DownloadStatus::Failed) { + model.download_errors.remove(episode_id); + } if let Some(episode) = model.episodes.iter_mut().find(|e| e.id == episode_id) { episode.download_status = status; episode.local_path = local_path; @@ -1670,6 +1684,51 @@ mod tests { assert!(has_download_effect_for(&effects, "e2")); } + #[test] + fn download_failure_reason_is_exposed_on_the_episode() { + let app = Pollux; + let mut model = Model::default(); + load_episodes(&app, &mut model, vec![make_episode("e1", "Ep", Some(1))]); + let _ = app.update(Event::DownloadEpisode("e1".to_string()), &mut model); + + let _ = app.update( + Event::DownloadFinished { + episode_id: "e1".to_string(), + result: Box::new(DownloadResult::Error("disk full".to_string())), + }, + &mut model, + ); + + // The specific reason rides along on the failed episode's summary. + let summary = &app.view(&model).subscription_detail.episodes[0]; + assert_eq!(summary.download_status, DownloadStatus::Failed); + assert_eq!(summary.download_error.as_deref(), Some("disk full")); + } + + #[test] + fn retrying_a_failed_download_clears_the_reason() { + let app = Pollux; + let mut model = Model::default(); + load_episodes(&app, &mut model, vec![make_episode("e1", "Ep", Some(1))]); + let _ = app.update(Event::DownloadEpisode("e1".to_string()), &mut model); + let _ = app.update( + Event::DownloadFinished { + episode_id: "e1".to_string(), + result: Box::new(DownloadResult::Error("boom".to_string())), + }, + &mut model, + ); + assert!(!model.download_errors.is_empty()); + + // Retrying (a DownloadEpisode on the failed episode) clears the reason and + // starts over. + let _ = app.update(Event::DownloadEpisode("e1".to_string()), &mut model); + assert!(model.download_errors.is_empty()); + let summary = &app.view(&model).subscription_detail.episodes[0]; + assert!(summary.download_error.is_none()); + assert_eq!(summary.download_status, DownloadStatus::Downloading); + } + #[test] fn delete_download_issues_delete_effect_then_resets_on_result() { let app = Pollux; diff --git a/shared/src/model.rs b/shared/src/model.rs index 05045b0..1718a7d 100644 --- a/shared/src/model.rs +++ b/shared/src/model.rs @@ -1,3 +1,5 @@ +use std::collections::HashMap; + use crate::domain::{Episode, EpisodeSortOrder, Subscription}; use crate::theme::{ThemeId, ThemeMode}; @@ -30,6 +32,13 @@ pub struct Model { // next one starts. pub active_download_progress: Option, + // Why a download failed, keyed by episode id, so the shell's specific reason + // (disk full, HTTP status, filesystem error) can be shown beside Retry instead + // of a generic "failed". In-memory only (like progress) and cleared as soon as + // the episode leaves the Failed state; the persisted status is enough to know it + // failed across a restart. + pub download_errors: HashMap, + // Active theme selection. Defaults (System / FollowSystem) reproduce the OS's // native appearance. Hard-coded for now — no UI changes it until the Settings // appearance section lands and drives `Event::SetTheme`. diff --git a/shared/src/view_model.rs b/shared/src/view_model.rs index 1e94cb8..2da30de 100644 --- a/shared/src/view_model.rs +++ b/shared/src/view_model.rs @@ -63,4 +63,8 @@ pub struct EpisodeSummary { /// Transient and never persisted (see `Model::active_download_progress`). pub download_received_bytes: Option, pub download_total_bytes: Option, + /// Why the download failed, present only for a `Failed` episode. Lets the shell + /// show the specific reason (disk full, HTTP status, …) beside Retry rather than + /// a generic message. Transient (see `Model::download_errors`). + pub download_error: Option, } From 41ede91c0899d06d1372451acb29b1fd90c45dc7 Mon Sep 17 00:00:00 2001 From: Alex Axthelm Date: Sun, 20 Sep 2026 09:24:36 +0200 Subject: [PATCH 17/38] fix: serialize storage writes so status transitions can't reorder MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Each storage effect was run in its own unstructured Task, so writes for one episode (Queued → Downloading → terminal) could apply out of order — a fast failure or cancel could persist its terminal state before the Downloading write, leaving a stale status that re-downloads the episode after restart. Funnel storage through a single serial AsyncStream consumer so ops execute in submission order. (Copilot #1.) Co-Authored-By: Claude Opus 4.8 --- iOS/Pollux/core.swift | 32 +++++++++++++++++++++++--------- 1 file changed, 23 insertions(+), 9 deletions(-) diff --git a/iOS/Pollux/core.swift b/iOS/Pollux/core.swift index d52e950..9c43e6c 100644 --- a/iOS/Pollux/core.swift +++ b/iOS/Pollux/core.swift @@ -9,6 +9,8 @@ class Core: ObservableObject { private var core: CoreFfi private let db: DatabaseManager private let downloads: DownloadManager + /// Storage operations are funnelled here and executed one at a time, in order. + private let storage: AsyncStream<(StorageOperation, UInt32)>.Continuation init() { core = CoreFfi() @@ -22,10 +24,29 @@ class Core: ObservableObject { } catch { fatalError("Failed to initialize DownloadManager: \(error)") } + // One serial consumer so writes for a given episode apply in submission order + // (Queued → Downloading → terminal). A Task per op could reorder them — a fast + // failure/cancel racing the Downloading write would leave a stale status in the + // DB and re-download the episode after restart. + let (storageStream, storageContinuation) = + AsyncStream.makeStream(of: (StorageOperation, UInt32).self) + storage = storageContinuation guard let view = try? ViewModel.bincodeDeserialize(input: [UInt8](core.view())) else { fatalError("Failed to deserialize initial ViewModel from core") } self.view = view + Task { @MainActor [weak self] in + for await (operation, requestId) in storageStream { + guard let self else { return } + let result: StorageResult + do { + result = try await db.execute(operation) + } catch { + result = .error(error.localizedDescription) + } + resolveAndDispatch(requestId: requestId, result: result) + } + } update(.started) } @@ -54,15 +75,8 @@ class Core: ObservableObject { view = updatedView case let .storage(operation): - Task { @MainActor in - let result: StorageResult - do { - result = try await db.execute(operation) - } catch { - result = .error(error.localizedDescription) - } - resolveAndDispatch(requestId: request.id, result: result) - } + // Enqueue for the serial consumer set up in init (preserves write order). + storage.yield((operation, request.id)) case let .http(operation): let requestId = request.id From a10f3057025798e1eea6b15573cb84178a3c25ec Mon Sep 17 00:00:00 2001 From: Alex Axthelm Date: Sun, 20 Sep 2026 09:49:04 +0200 Subject: [PATCH 18/38] record a download's size from the delegate, not a re-stat fileSize(at:) re-read the file's attributes after a completed download and returned 0 if that read failed, recording size_bytes = 0 for a real file. Use the delegate's totalBytesWritten (the completed file's byte count) instead, so a transient stat failure can't understate storage usage. Co-Authored-By: Claude Opus 4.8 --- iOS/Pollux/DownloadManager.swift | 47 +++++++++++++++----------------- 1 file changed, 22 insertions(+), 25 deletions(-) diff --git a/iOS/Pollux/DownloadManager.swift b/iOS/Pollux/DownloadManager.swift index 090d1da..f954278 100644 --- a/iOS/Pollux/DownloadManager.swift +++ b/iOS/Pollux/DownloadManager.swift @@ -101,12 +101,13 @@ actor DownloadManager { session.finishTasksAndInvalidate() } - let finalURL: URL = try await withCheckedThrowingContinuation { continuation in + // The delegate reports the completed file's byte count, so there's no + // filesystem re-read that could fail and record a size of 0. + let (_, size): (URL, UInt64) = try await withCheckedThrowingContinuation { continuation in delegate.attach(continuation) task.resume() } - let size = fileSize(at: finalURL) let relativePath = "\(Self.subdirectory)/\(fileName)" return .completed(localPath: relativePath, sizeBytes: size) } catch { @@ -166,15 +167,6 @@ actor DownloadManager { let ext = url.pathExtension return ext.isEmpty ? base : "\(base).\(ext)" } - - private func fileSize(at url: URL) -> UInt64 { - guard let attributes = try? FileManager.default.attributesOfItem(atPath: url.path), - let size = attributes[.size] as? UInt64 - else { - return 0 - } - return size - } } /// Session delegate driving a single download. Reports byte progress via @@ -189,17 +181,20 @@ private final class DownloadSessionDelegate: NSObject, URLSessionDownloadDelegat private let destination: URL private let onProgress: @Sendable (UInt64, UInt64?) -> Void private let lock = NSLock() - private var continuation: CheckedContinuation? + private var continuation: CheckedContinuation<(URL, UInt64), Error>? private var movedURL: URL? private var completionError: Error? private var lastPercent = -1 + /// Bytes written so far; the final value is the completed file's size, so it can be + /// reported without a filesystem re-read (which could fail and record a size of 0). + private var receivedBytes: UInt64 = 0 init(destination: URL, onProgress: @escaping @Sendable (UInt64, UInt64?) -> Void) { self.destination = destination self.onProgress = onProgress } - func attach(_ continuation: CheckedContinuation) { + func attach(_ continuation: CheckedContinuation<(URL, UInt64), Error>) { lock.lock() defer { lock.unlock() } self.continuation = continuation @@ -212,22 +207,23 @@ private final class DownloadSessionDelegate: NSObject, URLSessionDownloadDelegat let received = UInt64(max(0, totalBytesWritten)) let total: UInt64? = totalBytesExpectedToWrite > 0 ? UInt64(totalBytesExpectedToWrite) : nil - // When the size is known, coalesce to whole-percent steps so the core gets at - // most ~100 updates per download rather than one per network packet. When it's - // unknown, forward as-is (already packet-paced). onProgress runs outside the lock. + // Record every callback (so the final value is the completed size), but when the + // size is known coalesce onProgress to whole-percent steps so the core gets at + // most ~100 updates per download. onProgress runs outside the lock. + lock.lock() + receivedBytes = received + var report = true if let total, total > 0 { let percent = Int(received * 100 / total) - lock.lock() - let unchanged = percent == lastPercent - if !unchanged { + report = percent != lastPercent + if report { lastPercent = percent } - lock.unlock() - if unchanged { - return - } } - onProgress(received, total) + lock.unlock() + if report { + onProgress(received, total) + } } func urlSession( @@ -260,6 +256,7 @@ private final class DownloadSessionDelegate: NSObject, URLSessionDownloadDelegat self.continuation = nil let finishError = completionError let moved = movedURL + let bytes = receivedBytes lock.unlock() guard let continuation else { return } @@ -268,7 +265,7 @@ private final class DownloadSessionDelegate: NSObject, URLSessionDownloadDelegat } else if let finishError { continuation.resume(throwing: finishError) } else if let moved { - continuation.resume(returning: moved) + continuation.resume(returning: (moved, bytes)) } else { continuation.resume(throwing: DownloadManagerError.incomplete) } From e8bbd826c928a34d20aee9a33c8950d6851eb749 Mon Sep 17 00:00:00 2001 From: Alex Axthelm Date: Sun, 20 Sep 2026 09:54:19 +0200 Subject: [PATCH 19/38] docs: refresh ARCHITECTURE for the current core MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The "Current State" section still described the counter-app scaffolding. Update it — plus the stale Event/Effect descriptions, the FFI/shell dispatch notes, and the repo layout — to reflect the implemented feed/library/download-manager core and base16 theming, and fix a "no no" typo in the CI section. Co-Authored-By: Claude Opus 4.8 --- docs/ARCHITECTURE.md | 63 +++++++++++++++++++++++++++++--------------- 1 file changed, 42 insertions(+), 21 deletions(-) diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index db18ab6..a8fb0ea 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -38,7 +38,14 @@ serialized Effects flow out. The core never touches UI or I/O directly. pollux/ ├── shared/ # Rust core (Crux app) │ ├── src/ -│ │ ├── app.rs # App trait impl: Event, Model, ViewModel, Effect +│ │ ├── app.rs # App trait impl: Event, update(), view() +│ │ ├── model.rs # Model — private core state +│ │ ├── view_model.rs # ViewModel — serializable projection for the shell +│ │ ├── effect.rs # Effect enum (Render / Storage / Http / Download) +│ │ ├── capabilities/ # Storage, Http, Download operation & result types +│ │ ├── domain/ # Episode, Subscription +│ │ ├── feed_parser.rs # RSS → Subscription + Episodes +│ │ ├── theme.rs # base16 themes → ThemeView │ │ ├── ffi.rs # CoreFFI — the C/UniFFI/WASM bridge struct │ │ ├── lib.rs # Crate root; re-exports; UniFFI scaffolding │ │ └── bin/ @@ -46,9 +53,12 @@ pollux/ │ └── Cargo.toml ├── iOS/ │ ├── Pollux/ # SwiftUI app -│ │ ├── Pollux.swift # @main entry point -│ │ ├── ContentView.swift -│ │ └── core.swift # Core ObservableObject: drives CoreFFI +│ │ ├── Pollux.swift # @main entry point +│ │ ├── ContentView.swift # library + subscribe flow +│ │ ├── core.swift # Core ObservableObject: drives CoreFFI +│ │ ├── DatabaseManager.swift # Storage effect handler (GRDB/SQLite) +│ │ ├── DownloadManager.swift # Download effect handler (URLSession) +│ │ └── Theme.swift # ThemeView → SwiftUI colors │ ├── generated/ # ← git-ignored; produced by `make typegen` + `make package` │ │ ├── App/ # Swift type stubs (Event, ViewModel, Request, …) │ │ └── Shared/ # Swift package wrapping the compiled Rust static lib @@ -89,10 +99,10 @@ Event → update() → Command | Type | Role | |---|---| | `Pollux` | Zero-sized struct implementing `crux_core::App` | -| `Event` | All actions the UI can trigger (`Increment`, `Decrement`, `Reset` today) | -| `Model` | Private application state (never leaves the core) | -| `ViewModel` | Public, serializable snapshot of state for the shell to render | -| `Effect` | Side-effects the shell must handle (`Render` today; later: network, storage, …) | +| `Event` | All inputs the shell feeds in — user actions (subscribe/refresh a feed, select a subscription, sort/download/cancel an episode, set the theme) and the async results of storage/HTTP/download effects | +| `Model` | Private application state (never leaves the core); see `model.rs` | +| `ViewModel` | Public, serializable snapshot of state for the shell to render; see `view_model.rs` | +| `Effect` | Side-effects the shell must handle: `Render`, `Storage` (SQLite via GRDB), `Http` (feed fetch), `Download` (episode audio); see `effect.rs` | `Model` and `ViewModel` are deliberately separate. `Model` is private and mutable; `ViewModel` is a derived, serializable read-only projection. @@ -108,9 +118,10 @@ mutable; `ViewModel` is a derived, serializable read-only projection. effect - `view() -> Vec` — get the current bincode-serialized `ViewModel` -The shell is responsible for deserializing effects and dispatching them. Simple -synchronous effects (like `Render`) are handled inline. Future async effects -(network, disk) will be dispatched to platform APIs and resolved via `resolve()`. +The shell is responsible for deserializing effects and dispatching them. The +synchronous `Render` effect is handled inline; the async `Storage`, `Http`, and +`Download` effects are dispatched to platform APIs and their results delivered +back via `resolve()`. ### iOS shell (`iOS/Pollux/core.swift`) @@ -119,8 +130,10 @@ synchronous effects (like `Render`) are handled inline. Future async effects 1. Serializes the `Event` with bincode 2. Calls `CoreFFI.update(data:)` 3. Deserializes the returned `[Request]` -4. Dispatches each `Request` via `processEffect(_:)` — currently only - `.render`, which deserializes the `ViewModel` and publishes it +4. Dispatches each `Request` via `processEffect(_:)`: `.render` deserializes and + publishes the `ViewModel`; `.storage`, `.http`, and `.download` run their + async work and feed the result back through `resolve()`. Storage runs on a + single serial consumer so writes apply in submission order. SwiftUI views observe `core.view` and re-render automatically. @@ -143,8 +156,9 @@ make generate-project → xcodegen --spec iOS/project.yml regenerates iOS/Pollux.xcodeproj ``` -`make ios-build` runs all three in order. Run it whenever `shared/src/app.rs` -changes (new events, new view model fields). +`make ios-build` runs all three in order. Run it whenever the core's public +types change — new `Event`/`Effect` variants, `ViewModel` fields, or capability +operation/result types in `shared/src/`. Type serialization uses **bincode** (compact binary) at the FFI boundary and **serde** for all types. `facet` is used to drive type generation. @@ -174,7 +188,7 @@ GitHub Actions runs on push and PR to `main`: - **Rust checks**: `cargo check`, `cargo test`, `cargo clippy -D warnings`, `cargo fmt --check`, `cargo check --locked` - **Swift checks**: `swiftlint lint --strict`, `swiftformat --lint` -- **Admin**: forbidden-pattern scan (no no lint suppressions, no `unsafe`, no +- **Admin**: forbidden-pattern scan (no lint suppressions, no `unsafe`, no force-unwrap calls, no force-try/cast in Swift, etc.) All CI targets in `.github/workflows/` are reusable `workflow_call` workflows @@ -184,14 +198,21 @@ composed by `rust.yml`, `swift.yml`, and `admin.yml`. ## Current State -The core is a counter app (Increment / Decrement / Reset). That is scaffolding -only — the real domain model has not been implemented yet. Feature intent and -priorities are documented in `docs/features/`; the implementation order is in -`docs/ROADMAP.md`. +The MVP foundation is in place: RSS feed parsing and subscribing, a library and +per-feed episode list, and an offline **download manager** (serial queue, live +progress, resume-on-launch, cancel). A base16 **theming** system is wired +through the `ViewModel`. State lives in the Rust core; the iOS shell handles +SQLite (GRDB), HTTP, downloads (URLSession), and rendering. + +Still to come (see `docs/ROADMAP.md`): audio playback, playlists (the core +differentiator), a Settings screen, and feed refresh. Sync is out of scope for +MVP and v1.0. Feature intent and priorities live in `docs/features/`. When adding a new domain feature, the typical change surface is: -1. Extend `Event`, `Model`, `ViewModel`, and/or `Effect` in `shared/src/app.rs` +1. Extend the core types in `shared/src/` — `Event`/`update()`/`view()` in + `app.rs`, plus `model.rs`, `view_model.rs`, `effect.rs`, and `capabilities/` + as needed 2. Run `make ios-build` to regenerate Swift types and rebuild the Xcode project 3. Update SwiftUI views in `iOS/Pollux/` to consume the new `ViewModel` fields and emit the new `Event` variants From a63cfc4e8292fbbed9c6e78d392d4b8fedc7490b Mon Sep 17 00:00:00 2001 From: Alex Axthelm Date: Sun, 20 Sep 2026 10:18:41 +0200 Subject: [PATCH 20/38] theme the download UI colors The download UI used hardcoded system colors (.green/.blue/.orange/.secondary) instead of the base16 themeColors the merged theming system established, so it would render off-palette under a custom theme (Solarized/Nord). Swap them for themeColors (success/accent/warning/secondaryText); destructive Delete/Cancel keep the idiomatic system red via their role. Co-Authored-By: Claude Opus 4.8 --- iOS/Pollux/EpisodeDetailView.swift | 8 ++++---- iOS/Pollux/SubscriptionDetailScreen.swift | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/iOS/Pollux/EpisodeDetailView.swift b/iOS/Pollux/EpisodeDetailView.swift index 7e09175..2f9142b 100644 --- a/iOS/Pollux/EpisodeDetailView.swift +++ b/iOS/Pollux/EpisodeDetailView.swift @@ -105,7 +105,7 @@ struct EpisodeDetailView: View { case .queued: HStack { Label("Queued for download", systemImage: "clock") - .foregroundStyle(.secondary) + .foregroundStyle(themeColors.secondaryText) Spacer() cancelButton } @@ -124,7 +124,7 @@ struct EpisodeDetailView: View { Text(progress.label) .font(.caption) .monospacedDigit() - .foregroundStyle(.secondary) + .foregroundStyle(themeColors.secondaryText) } else { Label { Text("Downloading…") @@ -139,7 +139,7 @@ struct EpisodeDetailView: View { case .downloaded: HStack { Label("Downloaded", systemImage: "checkmark.circle.fill") - .foregroundStyle(.green) + .foregroundStyle(themeColors.success) Spacer() Button(role: .destructive) { core.update(.deleteDownload(episode.id)) @@ -164,7 +164,7 @@ struct EpisodeDetailView: View { } case .removedFromFeed: Label("Removed from feed", systemImage: "xmark.circle") - .foregroundStyle(.secondary) + .foregroundStyle(themeColors.secondaryText) } } diff --git a/iOS/Pollux/SubscriptionDetailScreen.swift b/iOS/Pollux/SubscriptionDetailScreen.swift index f1219df..db7544c 100644 --- a/iOS/Pollux/SubscriptionDetailScreen.swift +++ b/iOS/Pollux/SubscriptionDetailScreen.swift @@ -101,12 +101,12 @@ struct SubscriptionDetailScreen: View { Button { core.update(.downloadEpisode(episode.id)) } label: { Label("Download", systemImage: "arrow.down.circle") } - .tint(.blue) + .tint(themeColors.accent) case .failed: Button { core.update(.downloadEpisode(episode.id)) } label: { Label("Retry", systemImage: "arrow.clockwise.circle") } - .tint(.orange) + .tint(themeColors.warning) case .downloaded: Button(role: .destructive) { core.update(.deleteDownload(episode.id)) } label: { Label("Delete", systemImage: "trash") From 2bffdc1461f2a6c65bd0aaa2716784c979478dfc Mon Sep 17 00:00:00 2001 From: Alex Axthelm Date: Sun, 20 Sep 2026 15:29:43 +0200 Subject: [PATCH 21/38] tidy the download cancel + error-overlay paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address two review nits: - Guard the failed-download reason overlay with is_empty() so a feed with no failures skips a per-episode HashMap lookup on every view(). - Make the cancel flow easier to follow. notify_shell isn't viable here (the FFI resolve() panics on a non-resolvable request and the shell resolves every download request), so the Cancel request/ack stays — but DownloadFinished's Cancelled arm is now documented as the sole owner of the state reset, and the DownloadCanceled ack is an explicit no-op sink (dropping a dead, misleading cancel-error-to-banner branch). Extract reset_to_not_downloaded, the identical set-NotDownloaded + persist idiom shared by the cancel and delete paths. Co-Authored-By: Claude Opus 4.8 --- shared/src/app.rs | 90 +++++++++++++++++------------------------------ 1 file changed, 32 insertions(+), 58 deletions(-) diff --git a/shared/src/app.rs b/shared/src/app.rs index 4d59cdf..14f048a 100644 --- a/shared/src/app.rs +++ b/shared/src/app.rs @@ -310,21 +310,12 @@ impl App for Pollux { DownloadResult::Cancelled => { // The shell cancelled the task and flushed the partial file, so // the episode goes back to not-downloaded and the queue drains on. + // This is the *sole* owner of a cancelled download's state reset — + // the `Cancel` operation's own ack (DownloadCanceled) does nothing. finish_active(model, &episode_id); - set_download_state( - model, - &episode_id, - DownloadStatus::NotDownloaded, - None, - None, - ); - let persist = persist_download_state( - &episode_id, - DownloadStatus::NotDownloaded, - None, - None, - ); - persist.and(maybe_start_next(model)).and(render()) + reset_to_not_downloaded(model, &episode_id) + .and(maybe_start_next(model)) + .and(render()) } // Deleted never arrives here (it resolves DeleteDownload instead). DownloadResult::Deleted => render(), @@ -345,28 +336,22 @@ impl App for Pollux { .position(|q| q.episode_id == episode_id) { // Queued but not started: just drop it from the queue and reset — - // nothing is running and no file exists yet. + // nothing is running and no file exists yet, so no shell cancel is + // needed and the reset happens synchronously here. model.download_queue.remove(pos); - set_download_state( - model, - &episode_id, - DownloadStatus::NotDownloaded, - None, - None, - ); - persist_download_state(&episode_id, DownloadStatus::NotDownloaded, None, None) - .and(render()) + reset_to_not_downloaded(model, &episode_id).and(render()) } else { // Not downloading and not queued — nothing to cancel. render() } } - Event::DownloadCanceled(result) => { - // Best-effort: the reset rides on the Download request's Cancelled - // resolution; only surface a failure to cancel here. - if let DownloadResult::Error(e) = *result { - model.detail_error = Some(e); - } + Event::DownloadCanceled(_result) => { + // The `Cancel` request must be resolved (the shell resolves every + // request), so this is its required sink — nothing more. The actual + // state reset is owned entirely by the in-flight `Download` request + // resolving `Cancelled` (see DownloadFinished above); if the cancel + // somehow didn't take, that download simply resolves normally, so + // there is nothing to surface here. render() } Event::DeleteDownload(episode_id) => { @@ -388,34 +373,13 @@ impl App for Pollux { None => { // Nothing on disk (e.g. a Failed row): normalize to // NotDownloaded without a filesystem round-trip. - set_download_state( - model, - &episode_id, - DownloadStatus::NotDownloaded, - None, - None, - ); - persist_download_state( - &episode_id, - DownloadStatus::NotDownloaded, - None, - None, - ) - .and(render()) + reset_to_not_downloaded(model, &episode_id).and(render()) } } } Event::DownloadDeleted { episode_id, result } => match *result { DownloadResult::Deleted => { - set_download_state( - model, - &episode_id, - DownloadStatus::NotDownloaded, - None, - None, - ); - persist_download_state(&episode_id, DownloadStatus::NotDownloaded, None, None) - .and(render()) + reset_to_not_downloaded(model, &episode_id).and(render()) } DownloadResult::Error(e) => { // The file couldn't be removed; leave the row as-is and surface @@ -490,8 +454,11 @@ fn build_subscription_detail(model: &Model) -> SubscriptionDetailView { } // Overlay each failed episode's reason (also transient, not stored on the Episode). - for summary in episodes.iter_mut() { - summary.download_error = model.download_errors.get(&summary.id).cloned(); + // Skipped entirely when nothing has failed, which is the common case. + if !model.download_errors.is_empty() { + for summary in episodes.iter_mut() { + summary.download_error = model.download_errors.get(&summary.id).cloned(); + } } let (subscription_id, title, artwork_url) = match &model.selected_subscription { @@ -618,6 +585,14 @@ fn persist_download_state( .then_send(|r| Event::DownloadStatePersisted(Box::new(r))) } +/// Resets an episode to not-downloaded (in the model) and returns the command that +/// persists it. The single shape used everywhere a download ends without a file — +/// cancelling an active or queued download, and deleting a stored one. +fn reset_to_not_downloaded(model: &mut Model, episode_id: &str) -> Command { + set_download_state(model, episode_id, DownloadStatus::NotDownloaded, None, None); + persist_download_state(episode_id, DownloadStatus::NotDownloaded, None, None) +} + /// Clears the in-flight marker (and its live progress) when the active download /// reaches a terminal state. fn finish_active(model: &mut Model, episode_id: &str) { @@ -685,9 +660,8 @@ pub enum Event { DeleteDownload(String), /// Stop an in-flight or queued download and reset the episode to not-downloaded. CancelDownload(String), - /// Result of asking the shell to cancel an in-flight download. The state reset - /// rides on the `Download` request resolving `Cancelled`; this only surfaces a - /// cancel error. + /// Required resolution sink for the `Cancel` request — carries no state change. + /// The reset is owned by the in-flight `Download` request resolving `Cancelled`. DownloadCanceled(Box), /// Live byte progress for the in-flight download, reported by the shell while /// the one-shot download request is still running. Transient — not persisted. From fcf0348a2dffb2f4b3c6fde2aa0a7325bafb3346 Mon Sep 17 00:00:00 2001 From: Alex Axthelm Date: Sun, 20 Sep 2026 20:05:27 +0200 Subject: [PATCH 22/38] docs: correct stale "no cancel yet" download-control comments The menu (EpisodeRow.downloadMenuItem) and leading-swipe (SubscriptionDetailScreen.downloadSwipeButton) both grew a Cancel action for queued/downloading episodes, but their doc comments still said cancellation wasn't available. Describe the actual state-dependent behavior instead. Co-Authored-By: Claude Opus 4.8 --- iOS/Pollux/EpisodeRow.swift | 5 +++-- iOS/Pollux/SubscriptionDetailScreen.swift | 5 +++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/iOS/Pollux/EpisodeRow.swift b/iOS/Pollux/EpisodeRow.swift index af7d842..830b5f5 100644 --- a/iOS/Pollux/EpisodeRow.swift +++ b/iOS/Pollux/EpisodeRow.swift @@ -162,8 +162,9 @@ struct EpisodeRow: View { .accessibilityLabel("More actions") } - /// The download entry, driven by status: an action for the states where one - /// applies, or a disabled indicator otherwise (there is no cancel yet). + /// The download entry, driven by status: Download when absent, Retry after a + /// failure, Delete when stored, Cancel Download while queued or downloading, and + /// a disabled indicator once removed from the feed. @ViewBuilder private var downloadMenuItem: some View { switch episode.downloadStatus { case .notDownloaded: diff --git a/iOS/Pollux/SubscriptionDetailScreen.swift b/iOS/Pollux/SubscriptionDetailScreen.swift index db7544c..e065765 100644 --- a/iOS/Pollux/SubscriptionDetailScreen.swift +++ b/iOS/Pollux/SubscriptionDetailScreen.swift @@ -93,8 +93,9 @@ struct SubscriptionDetailScreen: View { } } - /// Leading-swipe download action, mirroring the row's menu: an action for the - /// states where one applies, nothing for the rest (there is no cancel yet). + /// Leading-swipe download action, mirroring the row's menu: Download when absent, + /// Retry after a failure, Delete when stored, Cancel while queued or downloading, + /// and nothing once removed from the feed. @ViewBuilder private func downloadSwipeButton(for episode: EpisodeSummary) -> some View { switch episode.downloadStatus { case .notDownloaded: From ae79d9af45eb2b2489cddd9756f84ff3163a88ab Mon Sep 17 00:00:00 2001 From: Alex Axthelm Date: Sun, 20 Sep 2026 20:13:01 +0200 Subject: [PATCH 23/38] fix: exclude downloaded audio from device backups Downloaded episode files live under Application Support, which is backed up to iCloud by default. Since the audio is reproducible (re-downloadable from the feed), a large offline library would needlessly consume the user's backup quota and inflate their backups. Set isExcludedFromBackup on the Downloads directory when creating it; the flag is best-effort so it never fails a download. Co-Authored-By: Claude Opus 4.8 --- iOS/Pollux/DownloadManager.swift | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/iOS/Pollux/DownloadManager.swift b/iOS/Pollux/DownloadManager.swift index f954278..71ef934 100644 --- a/iOS/Pollux/DownloadManager.swift +++ b/iOS/Pollux/DownloadManager.swift @@ -80,9 +80,7 @@ actor DownloadManager { return .error(DownloadManagerError.invalidURL(urlString).localizedDescription) } do { - try FileManager.default.createDirectory( - at: downloadsDir, withIntermediateDirectories: true, - ) + try ensureDownloadsDirectory() let fileName = Self.fileName(episodeId: episodeId, url: url) let destination = downloadsDir.appendingPathComponent(fileName) @@ -154,6 +152,20 @@ actor DownloadManager { // MARK: - Paths + /// Creates the downloads directory if needed and marks it excluded from device + /// backups. Downloaded audio is reproducible, so it shouldn't consume the user's + /// iCloud backup quota or bloat their backups. The exclusion is best-effort — a + /// failure to set the flag doesn't fail the download. + private func ensureDownloadsDirectory() throws { + try FileManager.default.createDirectory( + at: downloadsDir, withIntermediateDirectories: true, + ) + var dir = downloadsDir + var values = URLResourceValues() + values.isExcludedFromBackup = true + try? dir.setResourceValues(values) + } + private func absoluteURL(for relativePath: String) -> URL { storageRoot.appendingPathComponent(relativePath) } From 065387edbd8aeb5350f8a394d0d07f18693e5c1c Mon Sep 17 00:00:00 2001 From: Alex Axthelm Date: Sun, 20 Sep 2026 20:25:34 +0200 Subject: [PATCH 24/38] test: cover the DownloadManager shell MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The download shell (network + filesystem + cancellation) had no automated coverage. Add test seams to DownloadManager.init — an injectable storage root, URLSession configuration, and progress-report interval, all defaulted so production call sites are unchanged — and a DownloadManagerTests suite driven by a stub URLProtocol: successful download (stored file + reported size), non-2xx failure with no leftover file, in-flight cancellation cleanup, idempotent delete, and progress delivery. The delegate's throttle interval is now injected rather than a hardcoded constant. Co-Authored-By: Claude Opus 4.8 --- iOS/Pollux/DownloadManager.swift | 73 +++++-- iOS/PolluxTests/DownloadManagerTests.swift | 239 +++++++++++++++++++++ 2 files changed, 298 insertions(+), 14 deletions(-) create mode 100644 iOS/PolluxTests/DownloadManagerTests.swift diff --git a/iOS/Pollux/DownloadManager.swift b/iOS/Pollux/DownloadManager.swift index 71ef934..b362232 100644 --- a/iOS/Pollux/DownloadManager.swift +++ b/iOS/Pollux/DownloadManager.swift @@ -35,6 +35,12 @@ enum DownloadManagerError: Error, LocalizedError { actor DownloadManager { private let storageRoot: URL private let downloadsDir: URL + /// Configuration for the download session. Injectable so tests can register a + /// stub `URLProtocol` instead of hitting the network. + private let sessionConfiguration: URLSessionConfiguration + /// Progress-report throttle passed to the session delegate (see there). Injectable + /// so tests don't depend on a real one-second wall-clock gap. + private let reportInterval: TimeInterval /// The in-flight download's task and episode, so a Cancel operation can find and /// cancel it. At most one at a time (downloads are serial). Set before the await @@ -44,14 +50,32 @@ actor DownloadManager { /// Subdirectory (and stored-path prefix) for downloaded audio. private static let subdirectory = "Downloads" - init() throws { - guard let support = FileManager.default.urls( - for: .applicationSupportDirectory, in: .userDomainMask, - ).first else { - throw DownloadManagerError.storageUnavailable + /// - Parameters: + /// - storageRoot: where downloads live; defaults to Application Support. Tests + /// pass a temporary directory. + /// - sessionConfiguration: URLSession configuration; tests register a stub + /// `URLProtocol` here. + /// - reportInterval: throttle for progress reports on unknown-size downloads. + init( + storageRoot: URL? = nil, + sessionConfiguration: URLSessionConfiguration = .default, + reportInterval: TimeInterval = 1, + ) throws { + let root: URL + if let storageRoot { + root = storageRoot + } else { + guard let support = FileManager.default.urls( + for: .applicationSupportDirectory, in: .userDomainMask, + ).first else { + throw DownloadManagerError.storageUnavailable + } + root = support } - storageRoot = support - downloadsDir = support.appendingPathComponent(Self.subdirectory, isDirectory: true) + self.storageRoot = root + downloadsDir = root.appendingPathComponent(Self.subdirectory, isDirectory: true) + self.sessionConfiguration = sessionConfiguration + self.reportInterval = reportInterval } /// `onProgress(received, total)` is called with byte counts while a download runs @@ -89,8 +113,12 @@ actor DownloadManager { // delegate reports progress, moves the finished temp file to `destination` // (the system temp file is valid only inside didFinishDownloadingTo), and // resumes the await; a non-2xx response fails the download. - let delegate = DownloadSessionDelegate(destination: destination, onProgress: onProgress) - let session = URLSession(configuration: .default, delegate: delegate, delegateQueue: nil) + let delegate = DownloadSessionDelegate( + destination: destination, reportInterval: reportInterval, onProgress: onProgress, + ) + let session = URLSession( + configuration: sessionConfiguration, delegate: delegate, delegateQueue: nil, + ) let task = session.downloadTask(with: url) // Registered before the await so a Cancel can reach it; cleared after. activeTask = (episodeId: episodeId, task: task) @@ -197,12 +225,22 @@ private final class DownloadSessionDelegate: NSObject, URLSessionDownloadDelegat private var movedURL: URL? private var completionError: Error? private var lastPercent = -1 + /// Last time an indeterminate-size download reported progress, for time throttling. + private var lastReportAt = Date.distantPast /// Bytes written so far; the final value is the completed file's size, so it can be /// reported without a filesystem re-read (which could fail and record a size of 0). private var receivedBytes: UInt64 = 0 - init(destination: URL, onProgress: @escaping @Sendable (UInt64, UInt64?) -> Void) { + /// Minimum spacing between progress reports when the total size is unknown and + /// there is no percentage to coalesce on (~1 update per second in production). + private let reportInterval: TimeInterval + + init( + destination: URL, reportInterval: TimeInterval, + onProgress: @escaping @Sendable (UInt64, UInt64?) -> Void, + ) { self.destination = destination + self.reportInterval = reportInterval self.onProgress = onProgress } @@ -219,18 +257,25 @@ private final class DownloadSessionDelegate: NSObject, URLSessionDownloadDelegat let received = UInt64(max(0, totalBytesWritten)) let total: UInt64? = totalBytesExpectedToWrite > 0 ? UInt64(totalBytesExpectedToWrite) : nil - // Record every callback (so the final value is the completed size), but when the - // size is known coalesce onProgress to whole-percent steps so the core gets at - // most ~100 updates per download. onProgress runs outside the lock. + // Record every callback (so the final value is the completed size), but coalesce + // onProgress so the core isn't flooded with an update per write: to whole-percent + // steps when the size is known (~100 updates), or to ~1/sec when it isn't (no + // percentage to step on). onProgress runs outside the lock. lock.lock() receivedBytes = received - var report = true + let report: Bool if let total, total > 0 { let percent = Int(received * 100 / total) report = percent != lastPercent if report { lastPercent = percent } + } else { + let now = Date() + report = now.timeIntervalSince(lastReportAt) >= reportInterval + if report { + lastReportAt = now + } } lock.unlock() if report { diff --git a/iOS/PolluxTests/DownloadManagerTests.swift b/iOS/PolluxTests/DownloadManagerTests.swift new file mode 100644 index 0000000..5eb10b2 --- /dev/null +++ b/iOS/PolluxTests/DownloadManagerTests.swift @@ -0,0 +1,239 @@ +import App +import Foundation +import Testing + +@testable import Pollux + +// MARK: - Test doubles + +/// A stub `URLProtocol` so download tests never touch the network. A test configures +/// the canned response (status, whether a Content-Length is advertised, body bytes, +/// and whether to stall mid-flight for cancellation), and the loader delivers it in +/// small chunks so byte-progress callbacks fire. +private final class MockURLProtocol: URLProtocol, @unchecked Sendable { + struct Stub: Sendable { + var statusCode = 200 + var advertiseContentLength = true + var body = Data() + /// Deliver the response + body but never finish, so the request stays in + /// flight until the test cancels it. + var stall = false + } + + private static let lock = NSLock() + nonisolated(unsafe) private static var stub = Stub() + nonisolated(unsafe) private static var onStart: (@Sendable () -> Void)? + + static func configure(_ stub: Stub, onStart: (@Sendable () -> Void)? = nil) { + lock.lock() + defer { lock.unlock() } + self.stub = stub + self.onStart = onStart + } + + override class func canInit(with _: URLRequest) -> Bool { true } + override class func canonicalRequest(for request: URLRequest) -> URLRequest { request } + + override func startLoading() { + Self.lock.lock() + let stub = Self.stub + let onStart = Self.onStart + Self.lock.unlock() + + guard let url = request.url, let client else { return } + var headers: [String: String] = [:] + if stub.advertiseContentLength { + headers["Content-Length"] = String(stub.body.count) + } + guard let response = HTTPURLResponse( + url: url, statusCode: stub.statusCode, httpVersion: "HTTP/1.1", headerFields: headers, + ) else { return } + client.urlProtocol(self, didReceive: response, cacheStoragePolicy: .notAllowed) + + // Chunk the body so more than one write callback can fire. + let chunkSize = 64 + var offset = 0 + while offset < stub.body.count { + let end = min(offset + chunkSize, stub.body.count) + client.urlProtocol(self, didLoad: stub.body.subdata(in: offset ..< end)) + offset = end + } + + onStart?() + if stub.stall { return } + client.urlProtocolDidFinishLoading(self) + } + + override func stopLoading() {} +} + +/// Thread-safe collector for the `onProgress` callbacks, which arrive on the session's +/// delegate queue. +private final class ProgressRecorder: @unchecked Sendable { + private let lock = NSLock() + private var storage: [(received: UInt64, total: UInt64?)] = [] + + func record(_ received: UInt64, _ total: UInt64?) { + lock.lock() + defer { lock.unlock() } + storage.append((received, total)) + } + + var samples: [(received: UInt64, total: UInt64?)] { + lock.lock() + defer { lock.unlock() } + return storage + } +} + +/// One-shot signal a test awaits to learn the stub has begun loading (so it can cancel +/// the request while it is genuinely in flight). +private final class StartSignal: @unchecked Sendable { + private let lock = NSLock() + private var continuation: CheckedContinuation? + private var fired = false + + func fire() { + lock.lock() + let waiter = continuation + continuation = nil + fired = true + lock.unlock() + waiter?.resume() + } + + func wait() async { + await withCheckedContinuation { continuation in + lock.lock() + if fired { + lock.unlock() + continuation.resume() + } else { + self.continuation = continuation + lock.unlock() + } + } + } +} + +// MARK: - Helpers + +private func makeTempRoot() -> URL { + FileManager.default.temporaryDirectory.appendingPathComponent(UUID().uuidString, isDirectory: true) +} + +private func makeManager(root: URL, reportInterval: TimeInterval = 0) throws -> DownloadManager { + let configuration = URLSessionConfiguration.ephemeral + configuration.protocolClasses = [MockURLProtocol.self] + return try DownloadManager( + storageRoot: root, sessionConfiguration: configuration, reportInterval: reportInterval, + ) +} + +private let noProgress: @Sendable (UInt64, UInt64?) -> Void = { _, _ in } + +// MARK: - Tests + +@Suite("DownloadManager", .serialized) +struct DownloadManagerTests { + + @Test func successfulDownloadStoresTheFileAndReportsItsSize() async throws { + let root = makeTempRoot() + let manager = try makeManager(root: root) + let body = Data((0 ..< 300).map { UInt8($0 % 251) }) + MockURLProtocol.configure(.init(body: body)) + + let result = await manager.perform( + .download(episodeId: "ep-1", url: "https://example.com/a.mp3"), onProgress: noProgress, + ) + + #expect(result == .completed(localPath: "Downloads/ep_1.mp3", sizeBytes: UInt64(body.count))) + let stored = try Data(contentsOf: root.appendingPathComponent("Downloads/ep_1.mp3")) + #expect(stored == body) + } + + @Test func nonSuccessStatusFailsWithoutLeavingAFile() async throws { + let root = makeTempRoot() + let manager = try makeManager(root: root) + MockURLProtocol.configure(.init(statusCode: 404, body: Data("not found".utf8))) + + let result = await manager.perform( + .download(episodeId: "ep-2", url: "https://example.com/b.mp3"), onProgress: noProgress, + ) + + guard case let .error(message) = result else { + Issue.record("expected .error, got \(result)") + return + } + #expect(message.contains("404")) + #expect(!FileManager.default.fileExists( + atPath: root.appendingPathComponent("Downloads/ep_2.mp3").path, + )) + } + + @Test func cancellingAnInFlightDownloadReturnsCancelledAndLeavesNoFile() async throws { + let root = makeTempRoot() + let manager = try makeManager(root: root) + let started = StartSignal() + MockURLProtocol.configure( + .init(advertiseContentLength: false, body: Data(count: 128), stall: true), + onStart: { started.fire() }, + ) + + let download = Task { + await manager.perform( + .download(episodeId: "ep-3", url: "https://example.com/c.mp3"), + onProgress: noProgress, + ) + } + await started.wait() + _ = await manager.perform(.cancel(episodeId: "ep-3"), onProgress: noProgress) + + let result = await download.value + #expect(result == .cancelled) + #expect(!FileManager.default.fileExists( + atPath: root.appendingPathComponent("Downloads/ep_3.mp3").path, + )) + } + + @Test func deletingAStoredFileIsIdempotent() async throws { + let root = makeTempRoot() + let manager = try makeManager(root: root) + let relativePath = "Downloads/keep.mp3" + let fileURL = root.appendingPathComponent(relativePath) + try FileManager.default.createDirectory( + at: fileURL.deletingLastPathComponent(), withIntermediateDirectories: true, + ) + try Data("audio".utf8).write(to: fileURL) + + let first = await manager.perform(.delete(localPath: relativePath), onProgress: noProgress) + #expect(first == .deleted) + #expect(!FileManager.default.fileExists(atPath: fileURL.path)) + + // Deleting the now-absent file still succeeds. + let second = await manager.perform(.delete(localPath: relativePath), onProgress: noProgress) + #expect(second == .deleted) + } + + @Test func progressIsReportedWithMonotonicBytesAndTheKnownTotal() async throws { + let root = makeTempRoot() + let manager = try makeManager(root: root) + let body = Data(count: 5000) + MockURLProtocol.configure(.init(body: body)) + let recorder = ProgressRecorder() + + let result = await manager.perform( + .download(episodeId: "ep-p", url: "https://example.com/p.mp3"), + ) { @Sendable received, total in + recorder.record(received, total) + } + + #expect(result == .completed(localPath: "Downloads/ep_p.mp3", sizeBytes: UInt64(body.count))) + let samples = recorder.samples + #expect(!samples.isEmpty) + // Every report carries the advertised total, and received bytes never go backwards. + #expect(samples.allSatisfy { $0.total == UInt64(body.count) }) + #expect(samples.map(\.received) == samples.map(\.received).sorted()) + #expect(samples.last?.received == UInt64(body.count)) + } +} From dd5838651326bae1d6a9cc18fbff11e5280143f0 Mon Sep 17 00:00:00 2001 From: Alex Axthelm Date: Sun, 20 Sep 2026 20:33:55 +0200 Subject: [PATCH 25/38] test: cover the download-progress formatter EpisodeFormatting.downloadProgress had no coverage despite the other formatters being tested. Add cases for nil received bytes, a known total (fraction + combined label), unknown and zero totals (received-only label, no fraction), received exceeding total (fraction clamped to 1.0), and UInt64.max inputs (no overflow via Int64 clamping). Byte labels are checked against a mirror of the production .file formatter to stay locale-independent. Co-Authored-By: Claude Opus 4.8 --- iOS/PolluxTests/EpisodeFormattingTests.swift | 48 ++++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/iOS/PolluxTests/EpisodeFormattingTests.swift b/iOS/PolluxTests/EpisodeFormattingTests.swift index 24b608d..4a451a5 100644 --- a/iOS/PolluxTests/EpisodeFormattingTests.swift +++ b/iOS/PolluxTests/EpisodeFormattingTests.swift @@ -46,4 +46,52 @@ struct EpisodeFormattingTests { #expect(rendered != nil) #expect(rendered?.isEmpty == false) } + + // MARK: Download progress + + @Test func nilReceivedHasNoProgress() { + #expect(EpisodeFormatting.downloadProgress(received: nil, total: 100) == nil) + } + + @Test func knownTotalGivesFractionAndCombinedLabel() { + let progress = EpisodeFormatting.downloadProgress(received: 50, total: 100) + #expect(progress?.fraction == 0.5) + #expect(progress?.label == "\(bytes(50)) / \(bytes(100))") + } + + @Test func unknownTotalHasNoFractionAndReceivedOnlyLabel() { + let progress = EpisodeFormatting.downloadProgress(received: 1234, total: nil) + #expect(progress?.fraction == nil) + #expect(progress?.label == bytes(1234)) + } + + @Test func zeroTotalIsTreatedAsUnknown() { + // A reported total of 0 carries no information, so it behaves like `nil`. + let progress = EpisodeFormatting.downloadProgress(received: 1234, total: 0) + #expect(progress?.fraction == nil) + #expect(progress?.label == bytes(1234)) + } + + @Test func receivedExceedingTotalClampsFractionToOne() { + // A late byte count past the advertised size shouldn't push the bar past full. + let progress = EpisodeFormatting.downloadProgress(received: 150, total: 100) + #expect(progress?.fraction == 1.0) + #expect(progress?.label == "\(bytes(150)) / \(bytes(100))") + } + + @Test func hugeByteCountsDoNotOverflow() { + // Int64(clamping:) keeps the ByteCountFormatter input in range, and the + // fraction still resolves to a sane clamped value. + let progress = EpisodeFormatting.downloadProgress(received: .max, total: .max) + #expect(progress?.fraction == 1.0) + #expect(progress?.label == "\(bytes(.max)) / \(bytes(.max))") + } +} + +/// Mirror of `EpisodeFormatting`'s private byte formatter so tests can assert exact +/// labels without depending on the current locale's spelling of a unit. +private func bytes(_ count: Int64) -> String { + let formatter = ByteCountFormatter() + formatter.countStyle = .file + return formatter.string(fromByteCount: count) } From cc0c361f119ab8e54b57f7493e8e5d2fd4dffd7c Mon Sep 17 00:00:00 2001 From: Alex Axthelm Date: Sun, 20 Sep 2026 20:40:18 +0200 Subject: [PATCH 26/38] stop download-op failures from blanking the episode list MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A failed persistence write or file removal set detail_error — the list-load error channel — so SubscriptionDetailScreen replaced the entire episode list (and every download/retry/cancel control) with an error view, and later successful writes never cleared it. Route these through a new transient, non-blocking download_notice instead: it renders as a banner above the intact list, and clears on a successful persist, any fresh download action, or a feed switch. detail_error goes back to meaning only "the episode list failed to load." Co-Authored-By: Claude Opus 4.8 --- iOS/Pollux/SubscriptionDetailScreen.swift | 55 ++++++++++++------ shared/src/app.rs | 70 ++++++++++++++++++++--- shared/src/model.rs | 7 +++ shared/src/view_model.rs | 4 ++ 4 files changed, 113 insertions(+), 23 deletions(-) diff --git a/iOS/Pollux/SubscriptionDetailScreen.swift b/iOS/Pollux/SubscriptionDetailScreen.swift index e065765..294d775 100644 --- a/iOS/Pollux/SubscriptionDetailScreen.swift +++ b/iOS/Pollux/SubscriptionDetailScreen.swift @@ -71,28 +71,51 @@ struct SubscriptionDetailScreen: View { .foregroundStyle(themeColors.secondaryText) } } else { - List(detail.episodes, id: \.id) { episode in - NavigationLink(value: episode) { - EpisodeRow( - episode: episode, - onDownload: { core.update(.downloadEpisode($0)) }, - onDeleteDownload: { core.update(.deleteDownload($0)) }, - onCancelDownload: { core.update(.cancelDownload($0)) }, - ) + VStack(spacing: 0) { + if let notice = detail.downloadNotice { + downloadNoticeBanner(notice) } - // Spec default: short swipe-right → Download / Delete. This is the - // one actionable swipe today (flag / mark-played have no engine yet); - // it becomes user-configurable when Settings lands. - .swipeActions(edge: .leading, allowsFullSwipe: fullSwipeAllowed(episode)) { - downloadSwipeButton(for: episode) + List(detail.episodes, id: \.id) { episode in + NavigationLink(value: episode) { + EpisodeRow( + episode: episode, + onDownload: { core.update(.downloadEpisode($0)) }, + onDeleteDownload: { core.update(.deleteDownload($0)) }, + onCancelDownload: { core.update(.cancelDownload($0)) }, + ) + } + // Spec default: short swipe-right → Download / Delete. This is the + // one actionable swipe today (flag / mark-played have no engine yet); + // it becomes user-configurable when Settings lands. + .swipeActions(edge: .leading, allowsFullSwipe: fullSwipeAllowed(episode)) { + downloadSwipeButton(for: episode) + } + .listRowBackground(themeColors.background) } - .listRowBackground(themeColors.background) + .listStyle(.plain) + .scrollContentBackground(.hidden) } - .listStyle(.plain) - .scrollContentBackground(.hidden) } } + /// Non-blocking banner for a failed download *operation* (a persistence write or + /// file removal). Unlike the list-load error it sits above the episodes and leaves + /// every row and control in place; it clears itself on the next download action. + private func downloadNoticeBanner(_ message: String) -> some View { + HStack(alignment: .top, spacing: 8) { + Image(systemName: "exclamationmark.triangle.fill") + Text(message) + .font(.caption) + .fixedSize(horizontal: false, vertical: true) + Spacer(minLength: 0) + } + .foregroundStyle(themeColors.error) + .padding(.horizontal) + .padding(.vertical, 8) + .frame(maxWidth: .infinity, alignment: .leading) + .background(themeColors.secondaryBackground) + } + /// Leading-swipe download action, mirroring the row's menu: Download when absent, /// Retry after a failure, Delete when stored, Cancel while queued or downloading, /// and nothing once removed from the feed. diff --git a/shared/src/app.rs b/shared/src/app.rs index 14f048a..ddc90ee 100644 --- a/shared/src/app.rs +++ b/shared/src/app.rs @@ -163,6 +163,8 @@ impl App for Pollux { model.episodes.clear(); model.detail_loading = true; model.detail_error = None; + // A notice from the previous feed doesn't apply to this one. + model.download_notice = None; Command::request_from_shell(StorageOperation::ListEpisodesBySubscription { subscription_id: id.clone(), }) @@ -208,6 +210,8 @@ impl App for Pollux { render() } Event::DownloadEpisode(episode_id) => { + // A fresh user action supersedes any stale op-failure notice. + model.download_notice = None; // Only a not-downloaded or failed (retry) episode can start a // download. Every other state — already queued/downloading, already // downloaded, removed from feed, or an episode we don't have loaded — @@ -321,6 +325,8 @@ impl App for Pollux { DownloadResult::Deleted => render(), }, Event::CancelDownload(episode_id) => { + // A fresh user action supersedes any stale op-failure notice. + model.download_notice = None; if model.downloading.as_deref() == Some(episode_id.as_str()) { // Active download: ask the shell to cancel the task and flush the // partial file. The in-flight Download request then resolves as @@ -355,6 +361,8 @@ impl App for Pollux { render() } Event::DeleteDownload(episode_id) => { + // A fresh user action supersedes any stale op-failure notice. + model.download_notice = None; let local_path = model .episodes .iter() @@ -382,9 +390,10 @@ impl App for Pollux { reset_to_not_downloaded(model, &episode_id).and(render()) } DownloadResult::Error(e) => { - // The file couldn't be removed; leave the row as-is and surface - // why on the details page rather than lying about the state. - model.detail_error = Some(e); + // The file couldn't be removed; leave the row (still Downloaded) as + // it is and surface why through the non-blocking notice rather than + // the list-load error, which would hide every episode and control. + model.download_notice = Some(format!("Couldn't remove the download: {e}")); render() } // Only Deleted/Error arrive from a Delete request; the rest are @@ -392,8 +401,15 @@ impl App for Pollux { DownloadResult::Completed { .. } | DownloadResult::Cancelled => render(), }, Event::DownloadStatePersisted(result) => { - if let StorageResult::Error(e) = *result { - model.detail_error = Some(e); + match *result { + // A durability failure: the UI already shows the correct optimistic + // state, so surface this as a non-blocking notice, not the list error. + StorageResult::Error(e) => { + model.download_notice = + Some(format!("Couldn't save the download's state: {e}")); + } + // A write went through, so any earlier notice is stale — clear it. + _ => model.download_notice = None, } render() } @@ -474,6 +490,7 @@ fn build_subscription_detail(model: &Model) -> SubscriptionDetailView { sort_order: model.episode_sort, loading: model.detail_loading, error: model.detail_error.clone(), + download_notice: model.download_notice.clone(), } } @@ -1773,14 +1790,53 @@ mod tests { } #[test] - fn download_persist_error_surfaces_on_details_page() { + fn download_persist_error_surfaces_as_a_notice_not_the_list_error() { let app = Pollux; let mut model = Model::default(); + // A pre-existing list-load error must survive a download op-failure. + model.detail_error = Some("earlier list load failed".to_string()); + let _ = app.update( Event::DownloadStatePersisted(Box::new(StorageResult::Error("db locked".to_string()))), &mut model, ); - assert_eq!(model.detail_error.as_deref(), Some("db locked")); + + // The op failure lands in the non-blocking notice, and the list error (which + // would blank the whole episode list) is untouched. + assert!(model + .download_notice + .as_deref() + .is_some_and(|n| n.contains("db locked"))); + assert_eq!( + model.detail_error.as_deref(), + Some("earlier list load failed") + ); + } + + #[test] + fn a_successful_persist_clears_a_stale_download_notice() { + let app = Pollux; + let mut model = Model::default(); + model.download_notice = Some("Couldn't save the download's state: db locked".to_string()); + + let _ = app.update( + Event::DownloadStatePersisted(Box::new(StorageResult::Success)), + &mut model, + ); + + assert!(model.download_notice.is_none()); + } + + #[test] + fn a_fresh_download_action_clears_a_stale_download_notice() { + let app = Pollux; + let mut model = Model::default(); + load_episodes(&app, &mut model, vec![make_episode("e1", "Ep", Some(1))]); + model.download_notice = Some("Couldn't remove the download: busy".to_string()); + + let _ = app.update(Event::DownloadEpisode("e1".to_string()), &mut model); + + assert!(model.download_notice.is_none()); } #[test] diff --git a/shared/src/model.rs b/shared/src/model.rs index 1718a7d..4eabd38 100644 --- a/shared/src/model.rs +++ b/shared/src/model.rs @@ -39,6 +39,13 @@ pub struct Model { // failed across a restart. pub download_errors: HashMap, + // A transient, non-blocking notice for a download *operation* that failed without + // changing the episode's state — a persistence write that didn't commit, or a file + // that couldn't be removed. Shown as a dismissible banner on the details page (NOT + // through `detail_error`, which replaces the whole episode list). Cleared when the + // user starts another download action or the next persistence write succeeds. + pub download_notice: Option, + // Active theme selection. Defaults (System / FollowSystem) reproduce the OS's // native appearance. Hard-coded for now — no UI changes it until the Settings // appearance section lands and drives `Event::SetTheme`. diff --git a/shared/src/view_model.rs b/shared/src/view_model.rs index 2da30de..6037d0c 100644 --- a/shared/src/view_model.rs +++ b/shared/src/view_model.rs @@ -38,6 +38,10 @@ pub struct SubscriptionDetailView { pub sort_order: EpisodeSortOrder, pub loading: bool, pub error: Option, + /// A transient, non-blocking notice for a failed download *operation* (a + /// persistence write or file removal that didn't take). Shown as a banner that + /// leaves the episode list and its controls intact, unlike `error`. + pub download_notice: Option, } /// Read-only projection of an `Episode` for display. Dates and durations stay raw From 91b8f7f8f7a4fabed2da2daa48d1b23412243f52 Mon Sep 17 00:00:00 2001 From: Alex Axthelm Date: Sun, 20 Sep 2026 20:42:36 +0200 Subject: [PATCH 27/38] docs: note the download queue's non-FIFO restore behavior MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit listPendingDownloads rebuilds the queue by pub_date on relaunch, so enqueue order isn't preserved across restarts (and a Queued item can start before the one that was actively Downloading). Document this as a known, accepted MVP limitation at the query, and track the fix — a persisted enqueue order, active item restored first — under a Phase 5 "Download queue management" roadmap item alongside the downloads page / re-ordering / parallel-download work. Co-Authored-By: Claude Opus 4.8 --- docs/ROADMAP.md | 3 +++ iOS/Pollux/DatabaseManager.swift | 6 ++++++ 2 files changed, 9 insertions(+) diff --git a/docs/ROADMAP.md b/docs/ROADMAP.md index a2973e2..411b56b 100644 --- a/docs/ROADMAP.md +++ b/docs/ROADMAP.md @@ -104,6 +104,9 @@ Rounding out MVP to a complete, shippable state. - **Storage UI**: usage breakdown by subscription, limit configuration - **Subscription unsubscribe**: warning with episode count, hard delete, playlist re-evaluation on removal +- **Download queue management**: downloads page, user re-ordering, and a + persisted enqueue order so the serial queue restores FIFO across relaunch + (today it rebuilds by pub date); groundwork for parallel downloads - **Subscription-level download rules**: cascading defaults (global → feed) - **Settings completion**: all known settings wired up, cascading defaults pattern throughout diff --git a/iOS/Pollux/DatabaseManager.swift b/iOS/Pollux/DatabaseManager.swift index 28942e5..824d697 100644 --- a/iOS/Pollux/DatabaseManager.swift +++ b/iOS/Pollux/DatabaseManager.swift @@ -213,6 +213,12 @@ actor DatabaseManager { } private func listPendingDownloads() throws -> StorageResult { + // Known limitation: enqueue order isn't persisted, so a relaunch rebuilds the + // queue by pub_date rather than the order the user queued them (and may start a + // previously-Queued item before the one that was actively Downloading). Fine for + // the serial MVP; revisit when the download queue gains explicit ordering — a + // persisted sequence column, restoring the active item first — alongside the + // downloads page / re-ordering / parallel-downloads work (see ROADMAP Phase 5). let rows = try db.read { db -> [Row] in try Row.fetchAll( db, From 804a70f936d01113e9da951f2ad7d4d439e623d8 Mon Sep 17 00:00:00 2001 From: Alex Axthelm Date: Sun, 20 Sep 2026 22:00:00 +0200 Subject: [PATCH 28/38] drop the unused download_progress column and field MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Download progress became transient in-memory state (a partial download can't resume across a restart), leaving download_progress dead end to end: the Episode domain field was only ever written None and never read, the ViewModel never carried it, and the DB column/UpdateDownloadState.progress param only ever persisted NULL. Remove it everywhere — Rust domain + storage op, regenerated Swift types, the SQLite column (edited into v1_initial), and the shell read/write paths — and delete the obsolete out-of-range-progress test. Pre-release, so no migration: existing local databases must be wiped (delete the app from the simulator) to match the updated v1 schema. Co-Authored-By: Claude Opus 4.8 --- iOS/Pollux/DatabaseManager.swift | 22 ++++++------- iOS/Pollux/DatabaseManagerRows.swift | 8 ++--- iOS/PolluxTests/DatabaseManagerTests.swift | 36 +++------------------- shared/src/app.rs | 5 --- shared/src/capabilities/storage.rs | 7 ++--- shared/src/domain/episode.rs | 1 - shared/src/feed_parser.rs | 1 - 7 files changed, 19 insertions(+), 61 deletions(-) diff --git a/iOS/Pollux/DatabaseManager.swift b/iOS/Pollux/DatabaseManager.swift index 824d697..722045b 100644 --- a/iOS/Pollux/DatabaseManager.swift +++ b/iOS/Pollux/DatabaseManager.swift @@ -63,7 +63,6 @@ actor DatabaseManager { t.column("playback_status", .text).notNull().defaults(to: "Unplayed") t.column("playback_position_secs", .integer).check(sql: "playback_position_secs >= 0") t.column("download_status", .text).notNull().defaults(to: "NotDownloaded") - t.column("download_progress", .integer).check(sql: "download_progress BETWEEN 0 AND 100") t.column("is_flagged", .boolean).notNull().defaults(to: false) t.column("file_size_bytes", .integer).check(sql: "file_size_bytes >= 0") t.column("local_path", .text) @@ -117,10 +116,9 @@ actor DatabaseManager { try getEpisodeByFeedGuid(subscriptionId: subscriptionId, feedGuid: feedGuid) case let .updatePlaybackStatus(episodeId, status, positionSecs): try await updatePlaybackStatus(episodeId: episodeId, status: status, positionSecs: positionSecs) - case let .updateDownloadState(episodeId, status, localPath, sizeBytes, progress): + case let .updateDownloadState(episodeId, status, localPath, sizeBytes): try await updateDownloadState( - episodeId: episodeId, status: status, localPath: localPath, - sizeBytes: sizeBytes, progress: progress, + episodeId: episodeId, status: status, localPath: localPath, sizeBytes: sizeBytes, ) default: fatalError("executeEpisode received a non-episode operation: \(operation)") @@ -262,25 +260,23 @@ actor DatabaseManager { return .success } - /// Persists a download-state transition. `localPath`/`sizeBytes`/`progress` - /// are written verbatim — passing nil clears the column to NULL, matching the - /// core, which sends nil to wipe stale file metadata on delete/failure. Size - /// overflow stores NULL (as in `upsertEpisodeRow`) rather than trapping. + /// Persists a download-state transition. `localPath`/`sizeBytes` are written + /// verbatim — passing nil clears the column to NULL, matching the core, which + /// sends nil to wipe stale file metadata on delete/failure. Size overflow stores + /// NULL (as in `upsertEpisodeRow`) rather than trapping. private func updateDownloadState( - episodeId: String, status: DownloadStatus, localPath: String?, - sizeBytes: UInt64?, progress: UInt8?, + episodeId: String, status: DownloadStatus, localPath: String?, sizeBytes: UInt64?, ) async throws -> StorageResult { let statusStr = Self.downloadStatusString(status) let size = sizeBytes.flatMap { Int64(exactly: $0) } - let progressInt = progress.map { Int32($0) } try await db.write { db in try db.execute( sql: """ UPDATE episodes - SET download_status = ?, local_path = ?, file_size_bytes = ?, download_progress = ? + SET download_status = ?, local_path = ?, file_size_bytes = ? WHERE id = ? """, - arguments: [statusStr, localPath, size, progressInt, episodeId], + arguments: [statusStr, localPath, size, episodeId], ) } return .success diff --git a/iOS/Pollux/DatabaseManagerRows.swift b/iOS/Pollux/DatabaseManagerRows.swift index d98d8eb..671c7ed 100644 --- a/iOS/Pollux/DatabaseManagerRows.swift +++ b/iOS/Pollux/DatabaseManagerRows.swift @@ -29,15 +29,14 @@ extension DatabaseManager { static func upsertEpisodeRow(_ episode: Episode, subscriptionId: String, db: Database) throws { let playbackStr = playbackStatusString(episode.playbackStatus) let downloadStr = downloadStatusString(episode.downloadStatus) - let downloadProgress = episode.downloadProgress.map { Int32($0) } try db.execute( sql: """ INSERT INTO episodes (id, feed_guid, subscription_id, title, description, pub_date, duration_secs, enclosure_url, artwork_url, playback_status, - playback_position_secs, download_status, download_progress, + playback_position_secs, download_status, is_flagged, file_size_bytes, local_path) - VALUES (?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?) + VALUES (?,?,?,?,?,?,?,?,?,?,?,?,?,?,?) ON CONFLICT(subscription_id, feed_guid) DO UPDATE SET title = excluded.title, description = excluded.description, @@ -53,7 +52,7 @@ extension DatabaseManager { episode.pubDate, episode.durationSecs, episode.enclosureUrl, episode.artworkUrl, playbackStr, episode.playbackPositionSecs, - downloadStr, downloadProgress, + downloadStr, episode.isFlagged, episode.fileSizeBytes.flatMap { Int64(exactly: $0) }, episode.localPath, @@ -91,7 +90,6 @@ extension DatabaseManager { playbackStatus: playbackStatus(from: row["playback_status"]), playbackPositionSecs: (row["playback_position_secs"] as Int64?).flatMap { UInt32(exactly: $0) }, downloadStatus: downloadStatus(from: row["download_status"]), - downloadProgress: (row["download_progress"] as Int32?).flatMap { UInt8(exactly: $0) }, isFlagged: row["is_flagged"], fileSizeBytes: (row["file_size_bytes"] as Int64?).map { UInt64(bitPattern: $0) }, localPath: row["local_path"], diff --git a/iOS/PolluxTests/DatabaseManagerTests.swift b/iOS/PolluxTests/DatabaseManagerTests.swift index 1aa1f8f..dc8d09d 100644 --- a/iOS/PolluxTests/DatabaseManagerTests.swift +++ b/iOS/PolluxTests/DatabaseManagerTests.swift @@ -51,7 +51,6 @@ private func makeEpisode( playbackStatus: playbackStatus, playbackPositionSecs: nil, downloadStatus: downloadStatus, - downloadProgress: nil, isFlagged: false, fileSizeBytes: fileSizeBytes, localPath: nil @@ -238,7 +237,7 @@ struct DatabaseManagerTests { try await db.execute( .updateDownloadState( episodeId: "ep-1", status: .downloaded, - localPath: "Downloads/ep-1.mp3", sizeBytes: 4096, progress: nil, + localPath: "Downloads/ep-1.mp3", sizeBytes: 4096, )) let result = try await db.execute(.getEpisode(id: "ep-1")) @@ -261,12 +260,12 @@ struct DatabaseManagerTests { try await db.execute( .updateDownloadState( episodeId: "ep-1", status: .downloaded, - localPath: "Downloads/ep-1.mp3", sizeBytes: 4096, progress: nil, + localPath: "Downloads/ep-1.mp3", sizeBytes: 4096, )) try await db.execute( .updateDownloadState( episodeId: "ep-1", status: .notDownloaded, - localPath: nil, sizeBytes: nil, progress: nil, + localPath: nil, sizeBytes: nil, )) let result = try await db.execute(.getEpisode(id: "ep-1")) @@ -505,7 +504,7 @@ struct DatabaseManagerTests { title: "Test", description: nil, pubDate: nil, durationSecs: nil, enclosureUrl: "https://example.com/ep.mp3", artworkUrl: nil, playbackStatus: .unplayed, playbackPositionSecs: nil, - downloadStatus: .notDownloaded, downloadProgress: nil, + downloadStatus: .notDownloaded, isFlagged: false, fileSizeBytes: UInt64.max, localPath: nil ) @@ -544,31 +543,4 @@ struct DatabaseManagerTests { } #expect(fetched.durationSecs == nil, "out-of-range duration should read as nil") } - - @Test func episode_outOfRangeDownloadProgressReadsAsNil() async throws { - // download_progress has a CHECK (0...100), so planting an out-of-range - // value requires bypassing it — standing in for a future schema that - // drops the bound or external tooling that ignores it. The read must - // degrade to nil, not silently wrap (300 -> 44) as truncation would. - let path = FileManager.default.temporaryDirectory - .appendingPathComponent(UUID().uuidString + ".sqlite").path - let db = try DatabaseManager(path: path) - try await db.execute(.upsertSubscription(makeSubscription(id: "sub-1"))) - try await db.execute(.upsertEpisode( - makeEpisode(id: "ep-1", subscriptionId: "sub-1", feedGuid: "g1"), - )) - - let raw = try DatabasePool(path: path) - try await raw.write { db in - try db.execute(sql: "PRAGMA ignore_check_constraints = ON") - try db.execute(sql: "UPDATE episodes SET download_progress = 300 WHERE id = 'ep-1'") - } - - let result = try await db.execute(.getEpisode(id: "ep-1")) - guard case let .episode(fetched) = result else { - Issue.record("Expected .episode, got \(result)") - return - } - #expect(fetched.downloadProgress == nil, "out-of-range progress should read as nil") - } } diff --git a/shared/src/app.rs b/shared/src/app.rs index ddc90ee..984d694 100644 --- a/shared/src/app.rs +++ b/shared/src/app.rs @@ -576,9 +576,6 @@ fn set_download_state( episode.download_status = status; episode.local_path = local_path; episode.file_size_bytes = size_bytes; - // No live percentage in the status-only pass; keep the column NULL rather - // than leaving a stale value from a previous download. - episode.download_progress = None; } } @@ -597,7 +594,6 @@ fn persist_download_state( status, local_path, size_bytes, - progress: None, }) .then_send(|r| Event::DownloadStatePersisted(Box::new(r))) } @@ -740,7 +736,6 @@ mod tests { playback_status: PlaybackStatus::Unplayed, playback_position_secs: None, download_status: DownloadStatus::NotDownloaded, - download_progress: None, is_flagged: false, file_size_bytes: None, local_path: None, diff --git a/shared/src/capabilities/storage.rs b/shared/src/capabilities/storage.rs index 3b26d00..860a1d5 100644 --- a/shared/src/capabilities/storage.rs +++ b/shared/src/capabilities/storage.rs @@ -36,15 +36,14 @@ pub enum StorageOperation { position_secs: Option, }, /// Persists a download-state transition (status plus the file metadata that - /// comes with it). `local_path`/`size_bytes`/`progress` are cleared to NULL - /// when `None` — e.g. on delete or failure — so the row never keeps a stale - /// path for a file that is no longer there. + /// comes with it). `local_path`/`size_bytes` are cleared to NULL when `None` — + /// e.g. on delete or failure — so the row never keeps a stale path for a file + /// that is no longer there. (Live byte progress is transient and never stored.) UpdateDownloadState { episode_id: String, status: DownloadStatus, local_path: Option, size_bytes: Option, - progress: Option, }, UpsertFeedWithEpisodes { subscription: Subscription, diff --git a/shared/src/domain/episode.rs b/shared/src/domain/episode.rs index 55e9f53..1f23086 100644 --- a/shared/src/domain/episode.rs +++ b/shared/src/domain/episode.rs @@ -46,7 +46,6 @@ pub struct Episode { pub playback_status: PlaybackStatus, pub playback_position_secs: Option, pub download_status: DownloadStatus, - pub download_progress: Option, pub is_flagged: bool, pub file_size_bytes: Option, pub local_path: Option, diff --git a/shared/src/feed_parser.rs b/shared/src/feed_parser.rs index 8ab70e3..80b9c9a 100644 --- a/shared/src/feed_parser.rs +++ b/shared/src/feed_parser.rs @@ -96,7 +96,6 @@ pub fn parse_feed(url: &str, body: Vec) -> Result<(Subscription, Vec Date: Sun, 20 Sep 2026 22:08:36 +0200 Subject: [PATCH 29/38] test: cover progress throttle and delete-failure notice MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fill the gaps left by two earlier changes: assert the unknown-size progress throttle caps reporting at the initial callback (a large chunked body with no Content-Length and a long interval yields ≤1 report, total nil), and that a failed delete surfaces through download_notice while leaving the row Downloaded and detail_error untouched. Also cover the notice clearing on subscription switch. Co-Authored-By: Claude Opus 4.8 --- iOS/PolluxTests/DownloadManagerTests.swift | 23 +++++++++++ shared/src/app.rs | 44 ++++++++++++++++++++++ 2 files changed, 67 insertions(+) diff --git a/iOS/PolluxTests/DownloadManagerTests.swift b/iOS/PolluxTests/DownloadManagerTests.swift index 5eb10b2..4363459 100644 --- a/iOS/PolluxTests/DownloadManagerTests.swift +++ b/iOS/PolluxTests/DownloadManagerTests.swift @@ -236,4 +236,27 @@ struct DownloadManagerTests { #expect(samples.map(\.received) == samples.map(\.received).sorted()) #expect(samples.last?.received == UInt64(body.count)) } + + @Test func unknownSizeProgressIsThrottled() async throws { + let root = makeTempRoot() + // A long interval means the time throttle should let at most the first report + // through, no matter how many write callbacks a chunked body produces. + let manager = try makeManager(root: root, reportInterval: 1000) + // ~1600 chunks of 64 bytes: without throttling this would report constantly. + MockURLProtocol.configure(.init(advertiseContentLength: false, body: Data(count: 100_000))) + let recorder = ProgressRecorder() + + let result = await manager.perform( + .download(episodeId: "ep-u", url: "https://example.com/u.mp3"), + ) { @Sendable received, total in + recorder.record(received, total) + } + + #expect(result == .completed(localPath: "Downloads/ep_u.mp3", sizeBytes: 100_000)) + let samples = recorder.samples + // The throttle caps reporting at the single initial callback. + #expect(samples.count <= 1) + // Any report that did fire carries no total (the size was unknown). + #expect(samples.allSatisfy { $0.total == nil }) + } } diff --git a/shared/src/app.rs b/shared/src/app.rs index 984d694..a653d19 100644 --- a/shared/src/app.rs +++ b/shared/src/app.rs @@ -1834,6 +1834,50 @@ mod tests { assert!(model.download_notice.is_none()); } + #[test] + fn delete_failure_surfaces_as_a_notice_and_keeps_the_row() { + let app = Pollux; + let mut model = Model::default(); + let mut downloaded = make_episode("e1", "Ep", Some(1)); + downloaded.download_status = DownloadStatus::Downloaded; + downloaded.local_path = Some("Downloads/e1.mp3".to_string()); + load_episodes(&app, &mut model, vec![downloaded]); + let _ = app.update(Event::DeleteDownload("e1".to_string()), &mut model); + + // The shell couldn't remove the file. + let _ = app.update( + Event::DownloadDeleted { + episode_id: "e1".to_string(), + result: Box::new(DownloadResult::Error("disk busy".to_string())), + }, + &mut model, + ); + + // The row stays Downloaded (the file is presumably still there) and the + // failure lands in the non-blocking notice, not the list error. + assert_eq!( + model_episode_status(&model, "e1"), + DownloadStatus::Downloaded + ); + assert!(model.download_notice.as_deref().is_some_and(|n| n.contains("disk busy"))); + assert!(model.detail_error.is_none()); + } + + #[test] + fn selecting_a_subscription_clears_a_stale_download_notice() { + let app = Pollux; + let mut model = Model::default(); + model.subscriptions = vec![make_subscription("sub-1", "Feed")]; + model.download_notice = Some("Couldn't save the download's state: db locked".to_string()); + + let _ = app.update(Event::SelectSubscription("sub-1".to_string()), &mut model); + + assert!( + model.download_notice.is_none(), + "a notice from a previous feed shouldn't carry into another" + ); + } + #[test] fn download_progress_overlays_bytes_on_the_active_episode() { let app = Pollux; From 2014a3ef59ae999c12510347ab9197c40a6102e1 Mon Sep 17 00:00:00 2001 From: Alex Axthelm Date: Sun, 20 Sep 2026 22:24:15 +0200 Subject: [PATCH 30/38] don't let an unrelated persist clear the download notice MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit DownloadStatePersisted cleared download_notice on any successful state write, so a concurrent download completing (its Downloaded persist) could dismiss an unrelated episode's delete/persist-failure message before the user saw it — deletes and cancels run independently of the serial download queue, so the overlap is reachable. Only set the notice on Error now; it still clears on the next user download action or a feed switch, so it stays transient without being wiped out of band. Co-Authored-By: Claude Opus 4.8 --- shared/src/app.rs | 30 ++++++++++++++++++------------ 1 file changed, 18 insertions(+), 12 deletions(-) diff --git a/shared/src/app.rs b/shared/src/app.rs index a653d19..6529e91 100644 --- a/shared/src/app.rs +++ b/shared/src/app.rs @@ -401,15 +401,14 @@ impl App for Pollux { DownloadResult::Completed { .. } | DownloadResult::Cancelled => render(), }, Event::DownloadStatePersisted(result) => { - match *result { - // A durability failure: the UI already shows the correct optimistic - // state, so surface this as a non-blocking notice, not the list error. - StorageResult::Error(e) => { - model.download_notice = - Some(format!("Couldn't save the download's state: {e}")); - } - // A write went through, so any earlier notice is stale — clear it. - _ => model.download_notice = None, + // A durability failure: the UI already shows the correct optimistic + // state, so surface this as a non-blocking notice, not the list error. + // Success is silent — notably it does NOT clear an existing notice, so + // an unrelated episode's persist can't dismiss another episode's failure + // message. The notice clears on the next user download action (see + // DownloadEpisode/DeleteDownload/CancelDownload) or on a feed switch. + if let StorageResult::Error(e) = *result { + model.download_notice = Some(format!("Couldn't save the download's state: {e}")); } render() } @@ -1809,17 +1808,24 @@ mod tests { } #[test] - fn a_successful_persist_clears_a_stale_download_notice() { + fn an_unrelated_successful_persist_does_not_clear_the_notice() { + // A different episode's persist succeeding must NOT dismiss a standing + // op-failure notice — otherwise a background download completing could wipe + // the message before the user sees it. The notice clears on a user action or + // feed switch instead (covered by other tests). let app = Pollux; let mut model = Model::default(); - model.download_notice = Some("Couldn't save the download's state: db locked".to_string()); + model.download_notice = Some("Couldn't remove the download: disk busy".to_string()); let _ = app.update( Event::DownloadStatePersisted(Box::new(StorageResult::Success)), &mut model, ); - assert!(model.download_notice.is_none()); + assert_eq!( + model.download_notice.as_deref(), + Some("Couldn't remove the download: disk busy"), + ); } #[test] From 54fd034b41067f7f46ef723e83ab9b3e462331a6 Mon Sep 17 00:00:00 2001 From: Alex Axthelm Date: Mon, 21 Sep 2026 15:31:53 +0200 Subject: [PATCH 31/38] perf: skip the head item's redundant Queued write on resume MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On launch, PendingDownloadsLoaded persisted Queued for every re-enqueued episode, then maybe_start_next immediately re-persisted the head item as Downloading — so the head's Queued write was written and instantly superseded. Skip it for the item that starts this pass (guarded by downloading.is_none()); items waiting behind it still normalize to Queued. A test pins the persist sequence. Co-Authored-By: Claude Opus 4.8 --- shared/src/app.rs | 48 +++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 40 insertions(+), 8 deletions(-) diff --git a/shared/src/app.rs b/shared/src/app.rs index 6529e91..7d22d23 100644 --- a/shared/src/app.rs +++ b/shared/src/app.rs @@ -57,6 +57,9 @@ impl App for Pollux { // from scratch. Best-effort: an error here just leaves nothing queued. let mut cmd = Command::done(); if let StorageResult::Episodes(rows) = *result { + // Nothing is in flight at launch, so `maybe_start_next` will start + // the first re-enqueued item (persisting it as `Downloading`). + let will_start_head = model.downloading.is_none(); for episode in rows { let already = model.downloading.as_deref() == Some(episode.id.as_str()) || model @@ -66,14 +69,19 @@ impl App for Pollux { if already { continue; } - // Normalize to Queued so an item waiting behind others no - // longer reads as "Downloading" in the DB. - cmd = cmd.and(persist_download_state( - &episode.id, - DownloadStatus::Queued, - None, - None, - )); + // The head item is about to be started, so skip a `Queued` write + // that its `Downloading` write would immediately supersede. Items + // waiting behind it are normalized to `Queued` so one left + // "Downloading" by a prior session no longer reads that way. + let is_head = will_start_head && model.download_queue.is_empty(); + if !is_head { + cmd = cmd.and(persist_download_state( + &episode.id, + DownloadStatus::Queued, + None, + None, + )); + } model.download_queue.push(QueuedDownload { episode_id: episode.id, url: episode.enclosure_url, @@ -1988,6 +1996,30 @@ mod tests { has_download_effect_for(&effects, "e1"), "an interrupted download should resume at launch" ); + + // The head item (e1) is started immediately, so it's persisted only as + // Downloading — no redundant Queued write that Downloading would supersede. + // The item waiting behind it (e2) is normalized to Queued. + let persisted: Vec<(&str, &DownloadStatus)> = effects + .iter() + .filter_map(|e| match e { + Effect::Storage(r) => match &r.operation { + StorageOperation::UpdateDownloadState { + episode_id, status, .. + } => Some((episode_id.as_str(), status)), + _ => None, + }, + _ => None, + }) + .collect(); + assert_eq!( + persisted, + vec![ + ("e2", &DownloadStatus::Queued), + ("e1", &DownloadStatus::Downloading), + ], + "head persists Downloading only; the rest persist Queued" + ); } #[test] From e5af4536c67fdf84c414e7c8d330b57b48de8567 Mon Sep 17 00:00:00 2001 From: Alex Axthelm Date: Mon, 21 Sep 2026 15:36:25 +0200 Subject: [PATCH 32/38] fix: show the download-failure notice on the episode detail page download_notice rendered only on the subscription list, so a delete or cancel that failed from an episode's detail page (which has its own Delete/Cancel controls) left the user with no feedback until they navigated back. Extract the banner into a shared DownloadNoticeBanner view and render it above the download controls on the detail page too. Co-Authored-By: Claude Opus 4.8 --- iOS/Pollux/DownloadNoticeBanner.swift | 26 +++++++++++++++++++++++ iOS/Pollux/EpisodeDetailView.swift | 5 +++++ iOS/Pollux/SubscriptionDetailScreen.swift | 20 +---------------- 3 files changed, 32 insertions(+), 19 deletions(-) create mode 100644 iOS/Pollux/DownloadNoticeBanner.swift diff --git a/iOS/Pollux/DownloadNoticeBanner.swift b/iOS/Pollux/DownloadNoticeBanner.swift new file mode 100644 index 0000000..5433ec4 --- /dev/null +++ b/iOS/Pollux/DownloadNoticeBanner.swift @@ -0,0 +1,26 @@ +import SwiftUI + +/// Non-blocking banner for a failed download *operation* (a persistence write that +/// didn't commit, or a file that couldn't be removed). Unlike a list-load error it +/// leaves the surrounding content and controls in place; the core clears the notice +/// on the next download action or feed switch. Shown on both the subscription detail +/// list and a single episode's detail page. +struct DownloadNoticeBanner: View { + @Environment(\.themeColors) private var themeColors + let message: String + + var body: some View { + HStack(alignment: .top, spacing: 8) { + Image(systemName: "exclamationmark.triangle.fill") + Text(message) + .font(.caption) + .fixedSize(horizontal: false, vertical: true) + Spacer(minLength: 0) + } + .foregroundStyle(themeColors.error) + .padding(.horizontal) + .padding(.vertical, 8) + .frame(maxWidth: .infinity, alignment: .leading) + .background(themeColors.secondaryBackground) + } +} diff --git a/iOS/Pollux/EpisodeDetailView.swift b/iOS/Pollux/EpisodeDetailView.swift index 2f9142b..c7edaca 100644 --- a/iOS/Pollux/EpisodeDetailView.swift +++ b/iOS/Pollux/EpisodeDetailView.swift @@ -25,6 +25,11 @@ struct EpisodeDetailView: View { artworkHeader titleBlock playbackControls + // A delete/cancel initiated here can fail; show the same non-blocking + // notice the list uses so the failure is visible without navigating back. + if let notice = core.view.subscriptionDetail.downloadNotice { + DownloadNoticeBanner(message: notice) + } downloadSection Divider() showNotes diff --git a/iOS/Pollux/SubscriptionDetailScreen.swift b/iOS/Pollux/SubscriptionDetailScreen.swift index 294d775..413ca5b 100644 --- a/iOS/Pollux/SubscriptionDetailScreen.swift +++ b/iOS/Pollux/SubscriptionDetailScreen.swift @@ -73,7 +73,7 @@ struct SubscriptionDetailScreen: View { } else { VStack(spacing: 0) { if let notice = detail.downloadNotice { - downloadNoticeBanner(notice) + DownloadNoticeBanner(message: notice) } List(detail.episodes, id: \.id) { episode in NavigationLink(value: episode) { @@ -98,24 +98,6 @@ struct SubscriptionDetailScreen: View { } } - /// Non-blocking banner for a failed download *operation* (a persistence write or - /// file removal). Unlike the list-load error it sits above the episodes and leaves - /// every row and control in place; it clears itself on the next download action. - private func downloadNoticeBanner(_ message: String) -> some View { - HStack(alignment: .top, spacing: 8) { - Image(systemName: "exclamationmark.triangle.fill") - Text(message) - .font(.caption) - .fixedSize(horizontal: false, vertical: true) - Spacer(minLength: 0) - } - .foregroundStyle(themeColors.error) - .padding(.horizontal) - .padding(.vertical, 8) - .frame(maxWidth: .infinity, alignment: .leading) - .background(themeColors.secondaryBackground) - } - /// Leading-swipe download action, mirroring the row's menu: Download when absent, /// Retry after a failure, Delete when stored, Cancel while queued or downloading, /// and nothing once removed from the feed. From e0648982d07952cebdccf94378c211d7da16c4d4 Mon Sep 17 00:00:00 2001 From: Alex Axthelm Date: Mon, 21 Sep 2026 15:38:36 +0200 Subject: [PATCH 33/38] docs: flag the refresh vs downloaded-size conflict in the episodes upsert The episodes upsert preserves download_status/local_path on conflict but overwrites file_size_bytes with the feed's advertised enclosure length, which would clobber the real on-disk size a completed download records. No feed refresh exists yet so it can't trigger; leave a FIXME(refresh) at the line so it's addressed when refresh lands. Co-Authored-By: Claude Opus 4.8 --- iOS/Pollux/DatabaseManagerRows.swift | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/iOS/Pollux/DatabaseManagerRows.swift b/iOS/Pollux/DatabaseManagerRows.swift index 671c7ed..ae95c54 100644 --- a/iOS/Pollux/DatabaseManagerRows.swift +++ b/iOS/Pollux/DatabaseManagerRows.swift @@ -44,6 +44,13 @@ extension DatabaseManager { artwork_url = excluded.artwork_url, pub_date = excluded.pub_date, duration_secs = excluded.duration_secs, + -- FIXME(refresh): download_status/local_path are preserved on + -- conflict, but file_size_bytes is overwritten with the feed's + -- advertised . For a downloaded episode this clobbers + -- the real on-disk size that updateDownloadState recorded. There's no + -- refresh feature yet, so it can't fire today; when feed refresh lands, + -- stop overwriting file_size_bytes for rows whose download_status is + -- Downloaded (or drop it from this SET and let the download path own it). file_size_bytes = excluded.file_size_bytes """, arguments: [ From 11f0709ea3aa5d39e3cf34e1768f3158360342b1 Mon Sep 17 00:00:00 2001 From: Alex Axthelm Date: Mon, 21 Sep 2026 16:01:15 +0200 Subject: [PATCH 34/38] style: format --- shared/src/app.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/shared/src/app.rs b/shared/src/app.rs index 7d22d23..ec77248 100644 --- a/shared/src/app.rs +++ b/shared/src/app.rs @@ -416,7 +416,8 @@ impl App for Pollux { // message. The notice clears on the next user download action (see // DownloadEpisode/DeleteDownload/CancelDownload) or on a feed switch. if let StorageResult::Error(e) = *result { - model.download_notice = Some(format!("Couldn't save the download's state: {e}")); + model.download_notice = + Some(format!("Couldn't save the download's state: {e}")); } render() } @@ -1873,7 +1874,10 @@ mod tests { model_episode_status(&model, "e1"), DownloadStatus::Downloaded ); - assert!(model.download_notice.as_deref().is_some_and(|n| n.contains("disk busy"))); + assert!(model + .download_notice + .as_deref() + .is_some_and(|n| n.contains("disk busy"))); assert!(model.detail_error.is_none()); } From 216918ba6d5ac11424a63e7aa4c2326062da31b4 Mon Sep 17 00:00:00 2001 From: Alex Axthelm Date: Mon, 21 Sep 2026 16:02:04 +0200 Subject: [PATCH 35/38] style: patterns --- iOS/Pollux/DatabaseManagerRows.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/iOS/Pollux/DatabaseManagerRows.swift b/iOS/Pollux/DatabaseManagerRows.swift index ae95c54..04723ea 100644 --- a/iOS/Pollux/DatabaseManagerRows.swift +++ b/iOS/Pollux/DatabaseManagerRows.swift @@ -44,7 +44,7 @@ extension DatabaseManager { artwork_url = excluded.artwork_url, pub_date = excluded.pub_date, duration_secs = excluded.duration_secs, - -- FIXME(refresh): download_status/local_path are preserved on + -- note(refresh): download_status/local_path are preserved on -- conflict, but file_size_bytes is overwritten with the feed's -- advertised . For a downloaded episode this clobbers -- the real on-disk size that updateDownloadState recorded. There's no From 1b2e629b9a1024dc8296f867440a6652c6f75f68 Mon Sep 17 00:00:00 2001 From: Alex Axthelm Date: Mon, 21 Sep 2026 16:20:43 +0200 Subject: [PATCH 36/38] a11y: hide the decorative icon in the download notice banner MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The warning triangle in DownloadNoticeBanner is decorative — the message text carries the meaning — so mark it accessibilityHidden to keep VoiceOver from announcing the icon's name ahead of the message. Co-Authored-By: Claude Opus 4.8 --- iOS/Pollux/DownloadNoticeBanner.swift | 3 +++ 1 file changed, 3 insertions(+) diff --git a/iOS/Pollux/DownloadNoticeBanner.swift b/iOS/Pollux/DownloadNoticeBanner.swift index 5433ec4..b17d44b 100644 --- a/iOS/Pollux/DownloadNoticeBanner.swift +++ b/iOS/Pollux/DownloadNoticeBanner.swift @@ -11,7 +11,10 @@ struct DownloadNoticeBanner: View { var body: some View { HStack(alignment: .top, spacing: 8) { + // Decorative: the message text conveys the warning, so keep VoiceOver from + // announcing the icon's name ahead of it. Image(systemName: "exclamationmark.triangle.fill") + .accessibilityHidden(true) Text(message) .font(.caption) .fixedSize(horizontal: false, vertical: true) From 13e60d08207d3541f3f551053bd19b7a710655f1 Mon Sep 17 00:00:00 2001 From: Alex Axthelm Date: Mon, 21 Sep 2026 16:37:58 +0200 Subject: [PATCH 37/38] fix: scope the download-failure notice to its episode The op-failure notice was subscription-wide and rendered on every episode's detail page, so a delete/cancel that failed on episode A appeared on episode B's page (the message names no episode, and navigating between episodes doesn't clear it). Tag the notice with its episode: download_notice becomes a DownloadNotice { episode_id, message } through the model, ViewModel, and the DownloadStatePersisted event. The detail page shows it only when it's about that episode; the list still shows it for any. Co-Authored-By: Claude Opus 4.8 --- iOS/Pollux/EpisodeDetailView.swift | 17 ++++- iOS/Pollux/SubscriptionDetailScreen.swift | 3 +- shared/src/app.rs | 81 +++++++++++++++-------- shared/src/model.rs | 10 +-- shared/src/view_model.rs | 14 +++- 5 files changed, 89 insertions(+), 36 deletions(-) diff --git a/iOS/Pollux/EpisodeDetailView.swift b/iOS/Pollux/EpisodeDetailView.swift index c7edaca..d64794e 100644 --- a/iOS/Pollux/EpisodeDetailView.swift +++ b/iOS/Pollux/EpisodeDetailView.swift @@ -19,6 +19,18 @@ struct EpisodeDetailView: View { core.view.subscriptionDetail.episodes.first { $0.id == episode.id } ?? episode } + /// The op-failure notice message, but only when it concerns this episode — the + /// notice is subscription-wide, so scope it so one episode's failure doesn't + /// surface on another's detail page. + private var episodeNotice: String? { + guard let notice = core.view.subscriptionDetail.downloadNotice, + notice.episodeId == episode.id + else { + return nil + } + return notice.message + } + var body: some View { ScrollView { VStack(alignment: .leading, spacing: 20) { @@ -26,8 +38,9 @@ struct EpisodeDetailView: View { titleBlock playbackControls // A delete/cancel initiated here can fail; show the same non-blocking - // notice the list uses so the failure is visible without navigating back. - if let notice = core.view.subscriptionDetail.downloadNotice { + // notice the list uses so the failure is visible without navigating back + // — but only when it's about this episode (see `episodeNotice`). + if let notice = episodeNotice { DownloadNoticeBanner(message: notice) } downloadSection diff --git a/iOS/Pollux/SubscriptionDetailScreen.swift b/iOS/Pollux/SubscriptionDetailScreen.swift index 413ca5b..e8a7dd2 100644 --- a/iOS/Pollux/SubscriptionDetailScreen.swift +++ b/iOS/Pollux/SubscriptionDetailScreen.swift @@ -72,8 +72,9 @@ struct SubscriptionDetailScreen: View { } } else { VStack(spacing: 0) { + // The list spans every episode, so show a notice for any of them. if let notice = detail.downloadNotice { - DownloadNoticeBanner(message: notice) + DownloadNoticeBanner(message: notice.message) } List(detail.episodes, id: \.id) { episode in NavigationLink(value: episode) { diff --git a/shared/src/app.rs b/shared/src/app.rs index ec77248..733aae2 100644 --- a/shared/src/app.rs +++ b/shared/src/app.rs @@ -12,7 +12,8 @@ use crate::html::strip_html_preview; use crate::model::{DownloadProgress, Model, QueuedDownload}; use crate::theme::{theme_view, ThemeId, ThemeMode}; use crate::view_model::{ - EpisodeSummary, LibraryView, SubscriptionDetailView, SubscriptionSummary, ViewModel, + DownloadNotice, EpisodeSummary, LibraryView, SubscriptionDetailView, SubscriptionSummary, + ViewModel, }; #[derive(Default)] @@ -401,14 +402,17 @@ impl App for Pollux { // The file couldn't be removed; leave the row (still Downloaded) as // it is and surface why through the non-blocking notice rather than // the list-load error, which would hide every episode and control. - model.download_notice = Some(format!("Couldn't remove the download: {e}")); + model.download_notice = Some(DownloadNotice { + episode_id, + message: format!("Couldn't remove the download: {e}"), + }); render() } // Only Deleted/Error arrive from a Delete request; the rest are // unreachable but keep the match total. DownloadResult::Completed { .. } | DownloadResult::Cancelled => render(), }, - Event::DownloadStatePersisted(result) => { + Event::DownloadStatePersisted { episode_id, result } => { // A durability failure: the UI already shows the correct optimistic // state, so surface this as a non-blocking notice, not the list error. // Success is silent — notably it does NOT clear an existing notice, so @@ -416,8 +420,10 @@ impl App for Pollux { // message. The notice clears on the next user download action (see // DownloadEpisode/DeleteDownload/CancelDownload) or on a feed switch. if let StorageResult::Error(e) = *result { - model.download_notice = - Some(format!("Couldn't save the download's state: {e}")); + model.download_notice = Some(DownloadNotice { + episode_id, + message: format!("Couldn't save the download's state: {e}"), + }); } render() } @@ -597,13 +603,18 @@ fn persist_download_state( local_path: Option, size_bytes: Option, ) -> Command { + // Capture the id so a persistence failure's notice can be tagged with its episode. + let id = episode_id.to_string(); Command::request_from_shell(StorageOperation::UpdateDownloadState { episode_id: episode_id.to_string(), status, local_path, size_bytes, }) - .then_send(|r| Event::DownloadStatePersisted(Box::new(r))) + .then_send(move |r| Event::DownloadStatePersisted { + episode_id: id.clone(), + result: Box::new(r), + }) } /// Resets an episode to not-downloaded (in the model) and returns the command that @@ -703,7 +714,10 @@ pub enum Event { }, /// Result of persisting a download-state transition. Success is silent; an /// error is surfaced on the details page (downloads are driven from there). - DownloadStatePersisted(Box), + DownloadStatePersisted { + episode_id: String, + result: Box, + }, /// Change the active theme. Not yet emitted by any UI — the seam for the /// Settings appearance section (see `docs/features/theme.md`). SetTheme { @@ -1800,16 +1814,18 @@ mod tests { model.detail_error = Some("earlier list load failed".to_string()); let _ = app.update( - Event::DownloadStatePersisted(Box::new(StorageResult::Error("db locked".to_string()))), + Event::DownloadStatePersisted { + episode_id: "e1".to_string(), + result: Box::new(StorageResult::Error("db locked".to_string())), + }, &mut model, ); - // The op failure lands in the non-blocking notice, and the list error (which - // would blank the whole episode list) is untouched. - assert!(model - .download_notice - .as_deref() - .is_some_and(|n| n.contains("db locked"))); + // The op failure lands in the non-blocking notice, tagged with its episode, and + // the list error (which would blank the whole episode list) is untouched. + let notice = model.download_notice.as_ref().expect("a notice"); + assert_eq!(notice.episode_id, "e1"); + assert!(notice.message.contains("db locked")); assert_eq!( model.detail_error.as_deref(), Some("earlier list load failed") @@ -1824,17 +1840,23 @@ mod tests { // feed switch instead (covered by other tests). let app = Pollux; let mut model = Model::default(); - model.download_notice = Some("Couldn't remove the download: disk busy".to_string()); + model.download_notice = Some(DownloadNotice { + episode_id: "e1".to_string(), + message: "Couldn't remove the download: disk busy".to_string(), + }); + // A *different* episode's persist succeeds. let _ = app.update( - Event::DownloadStatePersisted(Box::new(StorageResult::Success)), + Event::DownloadStatePersisted { + episode_id: "e2".to_string(), + result: Box::new(StorageResult::Success), + }, &mut model, ); - assert_eq!( - model.download_notice.as_deref(), - Some("Couldn't remove the download: disk busy"), - ); + let notice = model.download_notice.as_ref().expect("notice retained"); + assert_eq!(notice.episode_id, "e1"); + assert_eq!(notice.message, "Couldn't remove the download: disk busy"); } #[test] @@ -1842,7 +1864,10 @@ mod tests { let app = Pollux; let mut model = Model::default(); load_episodes(&app, &mut model, vec![make_episode("e1", "Ep", Some(1))]); - model.download_notice = Some("Couldn't remove the download: busy".to_string()); + model.download_notice = Some(DownloadNotice { + episode_id: "e1".to_string(), + message: "Couldn't remove the download: busy".to_string(), + }); let _ = app.update(Event::DownloadEpisode("e1".to_string()), &mut model); @@ -1869,15 +1894,14 @@ mod tests { ); // The row stays Downloaded (the file is presumably still there) and the - // failure lands in the non-blocking notice, not the list error. + // failure lands in the non-blocking notice (tagged with e1), not the list error. assert_eq!( model_episode_status(&model, "e1"), DownloadStatus::Downloaded ); - assert!(model - .download_notice - .as_deref() - .is_some_and(|n| n.contains("disk busy"))); + let notice = model.download_notice.as_ref().expect("a notice"); + assert_eq!(notice.episode_id, "e1"); + assert!(notice.message.contains("disk busy")); assert!(model.detail_error.is_none()); } @@ -1886,7 +1910,10 @@ mod tests { let app = Pollux; let mut model = Model::default(); model.subscriptions = vec![make_subscription("sub-1", "Feed")]; - model.download_notice = Some("Couldn't save the download's state: db locked".to_string()); + model.download_notice = Some(DownloadNotice { + episode_id: "e1".to_string(), + message: "Couldn't save the download's state: db locked".to_string(), + }); let _ = app.update(Event::SelectSubscription("sub-1".to_string()), &mut model); diff --git a/shared/src/model.rs b/shared/src/model.rs index 4eabd38..59aa4a8 100644 --- a/shared/src/model.rs +++ b/shared/src/model.rs @@ -2,6 +2,7 @@ use std::collections::HashMap; use crate::domain::{Episode, EpisodeSortOrder, Subscription}; use crate::theme::{ThemeId, ThemeMode}; +use crate::view_model::DownloadNotice; #[derive(Default)] pub struct Model { @@ -41,10 +42,11 @@ pub struct Model { // A transient, non-blocking notice for a download *operation* that failed without // changing the episode's state — a persistence write that didn't commit, or a file - // that couldn't be removed. Shown as a dismissible banner on the details page (NOT - // through `detail_error`, which replaces the whole episode list). Cleared when the - // user starts another download action or the next persistence write succeeds. - pub download_notice: Option, + // that couldn't be removed. Tagged with the failing episode so the detail page can + // scope it to that episode. Shown as a banner on the details page (NOT through + // `detail_error`, which replaces the whole episode list). Cleared when the user + // starts another download action or switches feeds. + pub download_notice: Option, // Active theme selection. Defaults (System / FollowSystem) reproduce the OS's // native appearance. Hard-coded for now — no UI changes it until the Settings diff --git a/shared/src/view_model.rs b/shared/src/view_model.rs index 6037d0c..ef9ccca 100644 --- a/shared/src/view_model.rs +++ b/shared/src/view_model.rs @@ -40,8 +40,18 @@ pub struct SubscriptionDetailView { pub error: Option, /// A transient, non-blocking notice for a failed download *operation* (a /// persistence write or file removal that didn't take). Shown as a banner that - /// leaves the episode list and its controls intact, unlike `error`. - pub download_notice: Option, + /// leaves the episode list and its controls intact, unlike `error`. Tagged with + /// the episode it concerns so an episode's detail page shows it only when it's + /// about that episode, while the list shows it for any. + pub download_notice: Option, +} + +/// A failed-download-operation notice, paired with the episode it's about. Transient +/// and non-blocking (see `SubscriptionDetailView::download_notice`). +#[derive(Facet, Serialize, Deserialize, Clone)] +pub struct DownloadNotice { + pub episode_id: String, + pub message: String, } /// Read-only projection of an `Episode` for display. Dates and durations stay raw From d4633ad14981fe9fa239af77b8700fd40fa7fb85 Mon Sep 17 00:00:00 2001 From: Alex Axthelm Date: Mon, 21 Sep 2026 17:22:14 +0200 Subject: [PATCH 38/38] fix: derive download filenames from a digest to avoid collisions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit fileName reduced an episode id to alphanumerics, mapping other characters to "_", so distinct ids like `ep-1` and `ep_1` produced the same file — the second download would overwrite the first, and deleting either would remove the shared file. Ids are generated UUIDs today so it can't collide in practice, but the helper accepted arbitrary strings. Base the name on a SHA-256 digest of the id (keeping the URL extension) so distinct ids always get distinct files; add a collision regression test and stop the existing tests hard-coding the name. Co-Authored-By: Claude Opus 4.8 --- iOS/Pollux/DownloadManager.swift | 18 ++++--- iOS/PolluxTests/DownloadManagerTests.swift | 57 +++++++++++++++++----- 2 files changed, 58 insertions(+), 17 deletions(-) diff --git a/iOS/Pollux/DownloadManager.swift b/iOS/Pollux/DownloadManager.swift index b362232..fdc71e6 100644 --- a/iOS/Pollux/DownloadManager.swift +++ b/iOS/Pollux/DownloadManager.swift @@ -1,4 +1,5 @@ import App +import CryptoKit import Foundation /// Errors from the download shell. Carried in `DownloadResult.error` back to the @@ -198,12 +199,17 @@ actor DownloadManager { storageRoot.appendingPathComponent(relativePath) } - /// A filesystem-safe, stable file name for an episode. The episode id can be a - /// GUID, URL, or hash, so it is reduced to alphanumerics; the enclosure's file - /// extension is preserved when present so players see a familiar type. - private static func fileName(episodeId: String, url: URL) -> String { - let safeId = episodeId.map { $0.isLetter || $0.isNumber ? $0 : "_" } - let base = String(safeId) + /// A filesystem-safe, stable, collision-free file name for an episode. The episode + /// id can in principle be any string (GUID, URL, hash), so the base is a SHA-256 + /// digest of the id rather than a lossy character replacement — two distinct ids + /// that would sanitize to the same name (e.g. `ep-1` and `ep_1`) get distinct + /// files. The same id always maps to the same name; the enclosure's file extension + /// is preserved when present so players see a familiar type. + /// Not `private` so it can be unit-tested directly. + static func fileName(episodeId: String, url: URL) -> String { + let base = SHA256.hash(data: Data(episodeId.utf8)) + .map { String(format: "%02x", $0) } + .joined() let ext = url.pathExtension return ext.isEmpty ? base : "\(base).\(ext)" } diff --git a/iOS/PolluxTests/DownloadManagerTests.swift b/iOS/PolluxTests/DownloadManagerTests.swift index 4363459..1b6c57a 100644 --- a/iOS/PolluxTests/DownloadManagerTests.swift +++ b/iOS/PolluxTests/DownloadManagerTests.swift @@ -132,6 +132,12 @@ private func makeManager(root: URL, reportInterval: TimeInterval = 0) throws -> private let noProgress: @Sendable (UInt64, UInt64?) -> Void = { _, _ in } +/// The stored path a download for `episodeId`/`url` resolves to, derived from the +/// same helper the manager uses so tests don't hard-code the (digest-based) name. +private func downloadsRelativePath(episodeId: String, url: URL) -> String { + "Downloads/" + DownloadManager.fileName(episodeId: episodeId, url: url) +} + // MARK: - Tests @Suite("DownloadManager", .serialized) @@ -142,13 +148,15 @@ struct DownloadManagerTests { let manager = try makeManager(root: root) let body = Data((0 ..< 300).map { UInt8($0 % 251) }) MockURLProtocol.configure(.init(body: body)) + let url = try #require(URL(string: "https://example.com/a.mp3")) let result = await manager.perform( - .download(episodeId: "ep-1", url: "https://example.com/a.mp3"), onProgress: noProgress, + .download(episodeId: "ep-1", url: url.absoluteString), onProgress: noProgress, ) - #expect(result == .completed(localPath: "Downloads/ep_1.mp3", sizeBytes: UInt64(body.count))) - let stored = try Data(contentsOf: root.appendingPathComponent("Downloads/ep_1.mp3")) + let rel = downloadsRelativePath(episodeId: "ep-1", url: url) + #expect(result == .completed(localPath: rel, sizeBytes: UInt64(body.count))) + let stored = try Data(contentsOf: root.appendingPathComponent(rel)) #expect(stored == body) } @@ -156,9 +164,10 @@ struct DownloadManagerTests { let root = makeTempRoot() let manager = try makeManager(root: root) MockURLProtocol.configure(.init(statusCode: 404, body: Data("not found".utf8))) + let url = try #require(URL(string: "https://example.com/b.mp3")) let result = await manager.perform( - .download(episodeId: "ep-2", url: "https://example.com/b.mp3"), onProgress: noProgress, + .download(episodeId: "ep-2", url: url.absoluteString), onProgress: noProgress, ) guard case let .error(message) = result else { @@ -167,7 +176,9 @@ struct DownloadManagerTests { } #expect(message.contains("404")) #expect(!FileManager.default.fileExists( - atPath: root.appendingPathComponent("Downloads/ep_2.mp3").path, + atPath: root.appendingPathComponent( + downloadsRelativePath(episodeId: "ep-2", url: url), + ).path, )) } @@ -179,10 +190,11 @@ struct DownloadManagerTests { .init(advertiseContentLength: false, body: Data(count: 128), stall: true), onStart: { started.fire() }, ) + let url = try #require(URL(string: "https://example.com/c.mp3")) let download = Task { await manager.perform( - .download(episodeId: "ep-3", url: "https://example.com/c.mp3"), + .download(episodeId: "ep-3", url: url.absoluteString), onProgress: noProgress, ) } @@ -192,7 +204,9 @@ struct DownloadManagerTests { let result = await download.value #expect(result == .cancelled) #expect(!FileManager.default.fileExists( - atPath: root.appendingPathComponent("Downloads/ep_3.mp3").path, + atPath: root.appendingPathComponent( + downloadsRelativePath(episodeId: "ep-3", url: url), + ).path, )) } @@ -221,14 +235,16 @@ struct DownloadManagerTests { let body = Data(count: 5000) MockURLProtocol.configure(.init(body: body)) let recorder = ProgressRecorder() + let url = try #require(URL(string: "https://example.com/p.mp3")) let result = await manager.perform( - .download(episodeId: "ep-p", url: "https://example.com/p.mp3"), + .download(episodeId: "ep-p", url: url.absoluteString), ) { @Sendable received, total in recorder.record(received, total) } - #expect(result == .completed(localPath: "Downloads/ep_p.mp3", sizeBytes: UInt64(body.count))) + let rel = downloadsRelativePath(episodeId: "ep-p", url: url) + #expect(result == .completed(localPath: rel, sizeBytes: UInt64(body.count))) let samples = recorder.samples #expect(!samples.isEmpty) // Every report carries the advertised total, and received bytes never go backwards. @@ -237,6 +253,23 @@ struct DownloadManagerTests { #expect(samples.last?.received == UInt64(body.count)) } + @Test func fileNameAvoidsCollisionsBetweenSimilarIds() throws { + let url = try #require(URL(string: "https://example.com/a.mp3")) + // Ids that a lossy char-replacement scheme would collapse to the same name. + let first = DownloadManager.fileName(episodeId: "ep-1", url: url) + let second = DownloadManager.fileName(episodeId: "ep_1", url: url) + #expect(first != second, "distinct ids must map to distinct files") + // Deterministic for the same id, and the extension is preserved. + #expect(DownloadManager.fileName(episodeId: "ep-1", url: url) == first) + #expect(first.hasSuffix(".mp3")) + } + + @Test func fileNameOmitsExtensionWhenTheUrlHasNone() throws { + let url = try #require(URL(string: "https://example.com/audio")) + let name = DownloadManager.fileName(episodeId: "ep-1", url: url) + #expect(!name.contains("."), "no URL extension means no dotted suffix") + } + @Test func unknownSizeProgressIsThrottled() async throws { let root = makeTempRoot() // A long interval means the time throttle should let at most the first report @@ -245,14 +278,16 @@ struct DownloadManagerTests { // ~1600 chunks of 64 bytes: without throttling this would report constantly. MockURLProtocol.configure(.init(advertiseContentLength: false, body: Data(count: 100_000))) let recorder = ProgressRecorder() + let url = try #require(URL(string: "https://example.com/u.mp3")) let result = await manager.perform( - .download(episodeId: "ep-u", url: "https://example.com/u.mp3"), + .download(episodeId: "ep-u", url: url.absoluteString), ) { @Sendable received, total in recorder.record(received, total) } - #expect(result == .completed(localPath: "Downloads/ep_u.mp3", sizeBytes: 100_000)) + let rel = downloadsRelativePath(episodeId: "ep-u", url: url) + #expect(result == .completed(localPath: rel, sizeBytes: 100_000)) let samples = recorder.samples // The throttle caps reporting at the single initial callback. #expect(samples.count <= 1)