Cache image details responses#156
Conversation
Image details were re-fetched every time a photo was opened. ImageDetailsLoader now keeps loaded details in memory, so reopening a photo (or coming back to one through a description link) is instant. ImageLoader and ImageDetailsLoader are actors: their caches are shared across tasks, and the memory-warning handler in AppGraph has to capture them from a @sendable closure. AppGraph no longer stores the loaders — they are retained by the remotes and the observer that use them. Closes #102 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR introduces an in-memory cache for PastVu “image details” lookups so reopening the same photo can reuse previously fetched Model.ImageDetails instead of re-requesting them each time.
Changes:
- Added
ImageDetailsLoaderto cacheModel.ImageDetailsbycidand clear the cache on memory warnings. - Updated
RewindRemotesto sourceimageDetailsviaImageDetailsLoader(while keeping remote retry/backoff policy). - Converted
ImageLoaderto an actor and updatedAppGraphwiring + added unit tests for the new details cache behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| RewindTests/ImageDetailsLoaderTests.swift | Adds tests validating per-cid caching, cache clearing, and non-caching of failures. |
| Rewind/Network/RewindRemotes.swift | Routes imageDetails remote through ImageDetailsLoader. |
| Rewind/Network/ImageLoader.swift | Converts ImageLoader to an actor and makes makeImage nonisolated. |
| Rewind/Network/ImageDetailsLoader.swift | New actor implementing the image-details in-memory cache. |
| Rewind/App/AppGraph.swift | Wires the new loader, removes stored imageLoader, and clears both caches on memory warnings. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| actor ImageDetailsLoader { | ||
| private var cache: [Int: Model.ImageDetails] | ||
| private let requestPerformer: RequestPerformer | ||
|
|
||
| init(requestPerformer: RequestPerformer) { | ||
| cache = [:] | ||
| self.requestPerformer = requestPerformer | ||
| } | ||
|
|
||
| /// Clears all cached details from memory. | ||
| /// Called automatically on memory warnings. | ||
| func clearCache() { | ||
| cache.removeAll() | ||
| } | ||
|
|
||
| func load(cid: Int) async throws -> Model.ImageDetails { | ||
| if let cached = cache[cid] { | ||
| return cached | ||
| } | ||
| let networkDetails = try await requestPerformer.perform(request: .imageDetails(cid: cid)) | ||
| let details = Model.ImageDetails(networkDetails) | ||
| cache[cid] = details | ||
| return details | ||
| } | ||
| } |
There was a problem hiding this comment.
🤖 Written by Claude Code (AI agent), posted from @chizberg's account.
Correct that the actor is reentrant across the await: two concurrent load(cid:) calls for the same cid both miss the cache and both fetch. The cost is one redundant GET — both store identical details, so nothing is corrupted and both callers get the right result.
Reachability is narrow, though. .willBePresented is dispatched from ImageDetailsView's .task, which fires once per view identity, so a duplicate needs a second firing while the first request is still in flight. Sequential reopens — the case this PR is about — already hit the cache.
Leaving it as is for now: ImageLoader has exactly the same property, and images are the more expensive fetch, so coalescing only here would make the two loaders asymmetric for the cheaper one. If we do want it, the shape is a [Int: Task<Model.ImageDetails, Error>] of in-flight requests, dropping each entry when its task finishes so failures don't stick, and having clearCache() clear them too — better done for both loaders in a follow-up than for details alone here.
| } | ||
|
|
||
| /// Clears all cached details from memory. | ||
| /// Called automatically on memory warnings. |
There was a problem hiding this comment.
useless comment based on external usage. should be removed
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
🤖 Written by Claude Code (AI agent), posted from @chizberg's account.
Closes #102.
Image details were re-fetched from PastVu every time a photo was opened.
ImageDetailsLoadernow keeps loaded details in memory, so reopening a photo — or coming back to one through a description link — is instant.What changed
ImageDetailsLoader(new): cachesModel.ImageDetailsby cid and performs the request on a miss. Cleared on memory warnings next to the image cache. Failures are never cached.RewindRemotestakes the loader and buildsimageDetailsfrom it;.exponentialBackoff()stays where the other remotes declare their retry policy, so a cache hit costs nothing and a failed fetch still retries.ImageLoaderandImageDetailsLoaderare actors. Both caches are already shared across tasks (everyRemoteclosure captures them and runs off the main actor), and the memory-warning handler captures them from a@Sendableclosure.makeImageisnonisolated— it only builds theRemoteclosure, and its three call sites are synchronous.AppGraphno longer stores the loaders. Nothing outside the graph usedimageLoader, and both are retained by the remotes,favoritesStorage, and the memory-warning observer that use them.Tests
ImageDetailsLoaderTestsdrives the loader through a stubURLRequestPerformerthat echoes the requested cid and stamps each response with its fetch number, so a cached value is distinguishable from a refetched one: repeated load served from cache (one request), per-cid entries that don't collide,clearCache()forcing a refetch, and a failed load leaving nothing cached.🤖 Generated with Claude Code