A. Task blocks where errors silently disappear
These are Task blocks containing try with no error handling at all. The errors vanish into the void.
┌──────────────────────────────┬─────────┬─────────────────────────────┬────────────────────────────────────────┐ │ File │ Line │ Function │ What fails silently │ ├──────────────────────────────┼─────────┼─────────────────────────────┼────────────────────────────────────────┤ │ ManagingEpisodes.swift │ 80-85 │ queueEpisodeOnTop │ getOrCreateEpisodeID + queue.unshift │ ├──────────────────────────────┼─────────┼─────────────────────────────┼────────────────────────────────────────┤ │ ManagingEpisodes.swift │ 88-96 │ queueEpisodeAtBottom │ getOrCreateEpisodeID + queue.append │ ├──────────────────────────────┼─────────┼─────────────────────────────┼────────────────────────────────────────┤ │ UpNextViewModel.swift │ 107-111 │ queueEpisodeOnTop │ queue.unshift │ ├──────────────────────────────┼─────────┼─────────────────────────────┼────────────────────────────────────────┤ │ UpNextViewModel.swift │ 122-126 │ queueEpisodeAtBottom │ queue.append │ ├──────────────────────────────┼─────────┼─────────────────────────────┼────────────────────────────────────────┤ │ EpisodeDetailViewModel.swift │ 80-83 │ init (shareArtwork) │ imagePipeline.image │ ├──────────────────────────────┼─────────┼─────────────────────────────┼────────────────────────────────────────┤ │ EpisodeDetailViewModel.swift │ 291-297 │ showPodcast │ getOrCreatePodcastEpisode │ ├──────────────────────────────┼─────────┼─────────────────────────────┼────────────────────────────────────────┤ │ PodcastDetailViewModel.swift │ 276-280 │ init (shareArtwork) │ imagePipeline.image │ ├──────────────────────────────┼─────────┼─────────────────────────────┼────────────────────────────────────────┤ │ WidgetSnapshotWriter.swift │ 78-85 │ start (queue observation) │ observatory.queueWidgetEpisodes() │ ├──────────────────────────────┼─────────┼─────────────────────────────┼────────────────────────────────────────┤ │ PlayManager.swift │ 535-541 │ temporarilyHaltSeekCommands │ sleeper.sleep + Task.checkCancellation │ └──────────────────────────────┴─────────┴─────────────────────────────┴────────────────────────────────────────┘
B. try? that silently swallows errors (should catch & log)
┌───────────────────────────────┬─────────┬──────────────────────────────────────────────┬─────────────────────────────────────────────────────────────────────────┐ │ File │ Line │ What's swallowed │ Why it matters │ ├───────────────────────────────┼─────────┼──────────────────────────────────────────────┼─────────────────────────────────────────────────────────────────────────┤ │ PlayManager.swift │ 480 │ repo.podcastEpisode(episodeID) │ Diagnostic function logs "not cached" when really the DB read failed — │ │ │ │ │ misleading │ ├───────────────────────────────┼─────────┼──────────────────────────────────────────────┼─────────────────────────────────────────────────────────────────────────┤ │ PodAVPlayer.swift │ 163 │ repo.podcastEpisode(episodeID) in │ false return is ambiguous with "no cached version" │ │ │ │ swapToCached │ │ ├───────────────────────────────┼─────────┼──────────────────────────────────────────────┼─────────────────────────────────────────────────────────────────────────┤ │ CacheBackgroundDelegate.swift │ 134 │ fileManager.removeItem for unplayable file │ Cleanup failure goes unnoticed │ ├───────────────────────────────┼─────────┼──────────────────────────────────────────────┼─────────────────────────────────────────────────────────────────────────┤ │ ITunesEntityResults.swift │ 8 │ $0.toPodcastWithEpisodeMetadata() │ Results silently filtered; should at least log │ ├───────────────────────────────┼─────────┼──────────────────────────────────────────────┼─────────────────────────────────────────────────────────────────────────┤ │ DefaultsStorable.swift │ 39 │ JSONEncoder().encode(self) │ Silent store failure; nil returned to caller │ ├───────────────────────────────┼─────────┼──────────────────────────────────────────────┼─────────────────────────────────────────────────────────────────────────┤ │ DefaultsStorable.swift │ 45 │ JSONDecoder().decode │ Silent load failure; ambiguous with "key not found" │ ├───────────────────────────────┼─────────┼──────────────────────────────────────────────┼─────────────────────────────────────────────────────────────────────────┤ │ Schema.swift │ 188 │ JSONEncoder().encode(nativeValue()) │ Migration silently skips, leaving stale format │ ├───────────────────────────────┼─────────┼──────────────────────────────────────────────┼─────────────────────────────────────────────────────────────────────────┤ │ AppInfo.swift │ 210, │ FileManager.createDirectory │ Dev directory silently not created │ │ │ 228 │ │ │ └───────────────────────────────┴─────────┴──────────────────────────────────────────────┴─────────────────────────────────────────────────────────────────────────┘
try? I think are OK as-is:
- Sleeper calls in debounce/heartbeat (ManualFeedEntryViewModel:77, WidgetSnapshotWriter:263) — only cancellation expected
- AppInfo:169 attributesOfItem — Date() fallback is fine
- PodcastOPML:138 convertToHTTPSURL — intentional filtering of invalid URLs
- Podcast:56, Episode:82 link/image?.convertToHTTPSURL — optional fields, nil is valid
- HTMLText:810 NSRegularExpression — hardcoded patterns, should never fail
C. do-catch blocks that should use caughtError (have meaningful local context)
These catch and log Self.log.error(error), but have extra local context that should be logged alongside the error:
┌───────────────────────────────┬──────────────────────────────────────────┬────────────────────────────────────────────────┐ │ File │ Line │ Local context available │ ├───────────────────────────────┼──────────────────────────────────────────┼────────────────────────────────────────────────┤ │ PlayManager.swift │ 107-115 (loadPersistedEpisodeIfNeeded) │ currentEpisodeID derived from sharedState │ ├───────────────────────────────┼──────────────────────────────────────────┼────────────────────────────────────────────────┤ │ PlayManager.swift │ 121-143 (configureAudioSession) │ "audio session configuration" context │ ├───────────────────────────────┼──────────────────────────────────────────┼────────────────────────────────────────────────┤ │ PlayManager.swift │ 468-473 (handlePlaybackFailure unshift) │ episodeID + "returning to queue after failure" │ ├───────────────────────────────┼──────────────────────────────────────────┼────────────────────────────────────────────────┤ │ PlayManager.swift │ 511-516 (setStatus deactivate) │ "deactivating audio session after stop" │ ├───────────────────────────────┼──────────────────────────────────────────┼────────────────────────────────────────────────┤ │ CacheBackgroundDelegate.swift │ 106-143 (didFinishDownloadingTo) │ episode, fileName, destURL, step in pipeline │ ├───────────────────────────────┼──────────────────────────────────────────┼────────────────────────────────────────────────┤ │ CacheBackgroundDelegate.swift │ 72-81 (didWriteData) │ downloadTask.taskID │ ├───────────────────────────────┼──────────────────────────────────────────┼────────────────────────────────────────────────┤ │ CacheBackgroundDelegate.swift │ 163-174 (didCompleteWithError) │ task.taskID │ ├───────────────────────────────┼──────────────────────────────────────────┼────────────────────────────────────────────────┤ │ RefreshManager.swift │ 220-227 (updateSeriesFromFeed save loop) │ newEpisode.id + "marking new episode for save" │ └───────────────────────────────┴──────────────────────────────────────────┴────────────────────────────────────────────────┘
D. Structural reorganization: PodAVPlayer.saveCurrentTime chain
saveCurrentTime is marked throws but catches internally and never actually throws. This creates a misleading chain:
saveCurrentTime (throws, but catches internally — never throws) ↑ called by handleCurrentTimeChange (throws because of saveCurrentTime) ↑ called by periodicTimeObserver (wraps in try-catch) ↑ called by savePosition (wraps in try-catch)
saveCurrentTime DOES have local context (episodeID from stored property) so catching is correct. But it should not be marked throws, and the chain above it should be cleaned up:
- saveCurrentTime: remove throws, use caughtError with episodeID + currentTime
- handleCurrentTimeChange: remove throws, remove try from saveCurrentTime call
- Periodic time observer (line 355): remove do-catch, just await
- savePosition: remove do-catch, just await
E. Questions for you
- Side-effect cleanup operations that catch with no extra local context
Several places catch errors in void "cleanup" operations where the parameters are known to the caller:
- PlayManager.cleanUpAfterLoadSuccess lines 210-214, 219-223 — dequeue/unshift after load
- PlayManager.cleanUpAfterLoadFailure lines 247-251, 262-266 — unshift after failure
- PlayManager.handleItemStatusChange lines 552-556 — unshift after status failed
- PlayManager.finishEpisode lines 316-320 — markFinished before loading next
By strict policy these should not catch (no extra local context), and should let the error propagate. But these are side-effects where failing should not abort the main flow. If cleanUpAfterLoadSuccess throws because queue.dequeue fails, we don't want to undo the successful load.
Should these propagate the error (which changes control flow), or is the current catch-and-log OK for void side-effects where returning early is the right behavior?
- ManagingEpisodes queueEpisodeOnTop and queueEpisodeAtBottom (lines 80-85, 88-96)
These Task blocks have no error handling at all — that's clearly wrong and needs fixing. But the existing removeEpisodeFromQueue (line 99-112) and playEpisode (line 53-65) DO have catch blocks with log.error(error). Should the queue methods match that pattern (catch + log.error), or should they include alert like ManagingPodcasts does?
- The log.catch { } pattern in SearchViewModel (lines 41, 50)
This uses Logger.catch which logs the error and returns nil. It has no extra local context. Per the policy, should errors from getOrCreatePodcast and the action closure propagate instead? But this is inside a withTaskGroup so exceptions can't easily propagate to the outer scope.
How would you like me to proceed? Want me to start making the changes for the clear items (A, B, C, D) while you think about the questions in E?
✻ Cooked for 8m 3s