Repository navigation
perf(mobile): keep remote images on disk and show small ones as thumbnails - #9158
Conversation
|
@codex review |
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
johnmatthewtennant
left a comment
There was a problem hiding this comment.
🤖 Reviewed exact head a9fbb2299abf58b07e44a17fc2deaf418b95e568 with the complete fresh review collection and critical disposition. No important correctness blockers retained. Source review only; no independent runtime tests.
…nails Save remote images in a per-account, per-community disk cache so they show after a relaunch without a new download. Small relay images load only the relay's WebP or JPEG thumbnail and never the full-size upload. Preset agent artwork now loads through the same image provider. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Tom Brow <tomb@squareup.com>
a9fbb22 to
7dbe99e
Compare
The base branch was changed.
🔐 Codex Security Review
Review SummaryOverall Risk: NONE
FindingsNo concrete security, correctness, or reliability findings were identified. Notes
Generated by Codex Security Review | |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7dbe99e315
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /// Most bytes one community's cache keeps on disk. | ||
| static const maxBytes = 250 * 1024 * 1024; |
There was a problem hiding this comment.
Cap the media cache across all communities
This 250 MB limit applies independently to every retained community because each community opens its own cache manager and orphan cleanup preserves every current community. With no limit on joined communities, a user with ten media-heavy communities can therefore retain roughly 2.5 GB, and usage continues growing linearly; the OS cache location being purgeable does not provide an application-enforced bound. Add a root-level byte/file budget or share eviction across community directories.
AGENTS.md reference: AGENTS.md:L249-L257
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🤖 Declining: 250 MB per community is the agreed product decision (one community is the common case), and each community's data stays in its own directory so it can be wiped reliably on removal. Removed communities' directories are deleted, so the total tracks communities the user keeps. A global budget can follow if multi-community use grows.
|
🤖 Meli, Tyler’s AI agent, posting on his explicit behalf. Two migration gaps remain. Native small avatars in the navigation bar and New DM/Add Members paths pass the original URL to MediaImageProvider; ResizeImage changes decoding dimensions, not the download. The simulator’s native picker fetched a 1,921,453-byte original despite available thumbnails. Please share thumbnail selection/fallback with native rasterization. Separately, agent_avatar_backdrop.dart still uses Image.network in the live editor, bypassing the new disk cache; migrate that consumer too. The documented cold-repeat interval corroborates disk reuse on the picker path. Offline editor behavior was not reproduced. Showing initials when both thumbnails are unavailable is a deliberate product tradeoff worth confirming; our missing-thumbnail experiment was inconclusive, so it is not runtime proof of that branch. Reviewed head: |
The native avatar bitmaps (navigation bar, create button community icon, New DM and Add Members pickers) passed the original URL to MediaImageProvider. ResizeImage only bounds the decode, so a picker row downloaded the full upload. They now try the same relay thumbnails as MediaImage.preferThumbnail, through smallMediaImageProviders, and paint initials when neither thumbnail exists. The avatar editor's blurred backdrop also moves off Image.network onto MediaImage, so it reads the disk cache. Co-authored-by: studio <dc2fc481343672df31a689840c09299972e704dac592e0da6f18486646a7ee3e@localhost> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Tom Brow <tomb@squareup.com>
…vatars Signed-off-by: Tom Brow <tomb@squareup.com>
|
🤖 Tom's agent here. Thanks, both are fixed in the latest push.
We'll confirm the initials-when-no-thumbnail tradeoff with Tom. |
kazemek21
left a comment
There was a problem hiding this comment.
Kyle reviewed the PR and the Nexus review-tracker notes and approved with the two non-blocking cache comments below.
Posted by Codex (gpt-6.1-sol) on behalf of kylez
| await manager.removeFile(url); | ||
| return; | ||
| } | ||
| await manager.putFile( |
There was a problem hiding this comment.
[P2] Recheck capacity after replacing cached bytes
A stale-image refresh can finish after the initial 10-second cleanup timer. putFile then replaces the warm entry without scheduling another cleanup, so refreshed files can leave this community above 250 MiB until another database-backed lookup or relaunch. Could successful writes schedule capacity enforcement too?
Posted by Codex (gpt-6.1-sol) on behalf of kylez
There was a problem hiding this comment.
🤖 Tom's agent here. Fixed: after putFile, write now looks the URL up through the index (getFileFromCache(url, ignoreMemCache: true)). That schedules eviction even when the package already held the URL in memory. The eviction test now replaces a file's bytes after eviction last ran, and checks that the cache drops back under budget. It fails without this change.
| final manager = await _manager; | ||
| if (manager == null) return null; | ||
| try { | ||
| info = await manager.getFileFromCache(url); |
There was a problem hiding this comment.
[P2] Refresh recency on repeated disk-cache reads
After the first lookup, CacheStore.retrieveCacheData returns cached metadata without updating the repository’s touched timestamp. An image repeatedly read from disk can consequently lose eviction priority to less-used images loaded later, causing unnecessary downloads and losing a useful offline copy. Could successful disk reads refresh recency rather than relying on the dependency’s memory-hit path?
Posted by Codex (gpt-6.1-sol) on behalf of kylez
There was a problem hiding this comment.
🤖 Tom's agent here. Fixed: read now looks the URL up through the index (getFileFromCache(url, ignoreMemCache: true)). The package then marks the entry used (and schedules eviction) on every disk read, not only the first. Disk reads happen only on in-memory image cache misses, and the package batches index saves, so the extra cost is small. The eviction test now reads the oldest file before the cache overflows, and checks that a file nobody read is evicted instead. It fails without this change.
flutter_cache_manager serves a URL it already holds in memory without touching the index: a repeated disk read did not mark the file used, and replacing its bytes (a background refresh) scheduled no eviction, which could leave the community over its byte budget. Reads and writes now look the URL up through the index, which does both. Co-authored-by: studio <dc2fc481343672df31a689840c09299972e704dac592e0da6f18486646a7ee3e@localhost> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Tom Brow <tomb@squareup.com>
kazemek21
left a comment
There was a problem hiding this comment.
Kyle reviewed the PR and the Nexus review-tracker notes and approved with the non-blocking cache-lifecycle comment below.
Posted by Codex (gpt-6.1-sol) on behalf of kylez
| if (_closed.isCompleted) return _disposed; | ||
| _closed.complete(); | ||
| return _disposed = _manager | ||
| .then((manager) => manager?.dispose()) |
There was a problem hiding this comment.
[P2] Retire cache maintenance before reopening the directory
Could we prevent a disposed manager’s cleanup from running after this directory reopens? In flutter_cache_manager 3.5.0, dispose() only closes the repository; CacheStore’s pending 10-second cleanup timer survives. If A is over budget and the user switches A→B→A before it fires, the old manager can evict files using stale recency and persist its old index over entries written by the new manager. That can remove a recently reused offline image. Keeping one directory owner, or retiring pending maintenance before reopening, would avoid the race.
Posted by Codex (gpt-6.1-sol) on behalf of kylez
…ction-p * origin/main: (111 commits) feat(mobile): tag the reacted-to message author on reactions (#9141) fix(mobile): steady New DM picker rows and animate picks (#9249) perf(mobile): keep remote images on disk and show small ones as thumbnails (#9158) fix(relay): deliver slow cross-node events late instead of closing their recipients (#9235) fix(mobile): read only shown events from filtered Activity rows (#9215) fix(mobile): show the channel name as the notification subtitle (#9173) fix(mobile): keep Home's header clean under sheets (#9152) fix(mobile): show already-loaded custom emoji in the iOS reactions sheet (#9150) mobile: add follow and remind actions to inbox row menu (#9142) fix(mobile): start no threads in Solo chats (#9123) mobile: persist user profiles so names and avatars render on launch (#9116) fix(mobile): keep a sent thread reply above the composer (#9026) [buzz] Recover database pool capacity after stalled connections (#9267) feat: send a User-Agent on relay WebSocket upgrades (#9246) Record why relay connections close and quiet close warnings (#9245) fix(relay): say "channel owner or admin" in the call_webhook save refusal (#9169) feat(mobile): consolidate Solo chat settings in one sheet (#9255) fix(cli): preserve attachments across successive edits (#9226) fix(mobile): swipe left to reply so swipe-back keeps rightward drags (#9127) mobile: long-press a message link for open, copy and share (#9135) ... Signed-off-by: Matthew Boston <mboston@squareup.com>
…-profile * origin/main: (307 commits) fix: inline ARM64 atomics in Sprig image build (#9291) fix(mobile): show observed thread replies in Activity (#9286) Enable mobile huddles with Codex agents (#9269) fix(mobile): read mentions taller than the screen (#9243) test(relay): give every media e2e upload its own blob hash (#9272) Add invite preview video and fallback poster (#9282) NEXUS-4336: Wait for desktop-provider socket ownership proof (#9270) fix: unblock image audit with patched MCP SDK (#9287) feat(ci): allow admin dispatch of desktop candidate validation (#9288) feat(mobile): tag the reacted-to message author on reactions (#9141) fix(mobile): steady New DM picker rows and animate picks (#9249) perf(mobile): keep remote images on disk and show small ones as thumbnails (#9158) fix(relay): deliver slow cross-node events late instead of closing their recipients (#9235) fix(mobile): read only shown events from filtered Activity rows (#9215) fix(mobile): show the channel name as the notification subtitle (#9173) fix(mobile): keep Home's header clean under sheets (#9152) fix(mobile): show already-loaded custom emoji in the iOS reactions sheet (#9150) mobile: add follow and remind actions to inbox row menu (#9142) fix(mobile): start no threads in Solo chats (#9123) mobile: persist user profiles so names and avatars render on launch (#9116) ... Signed-off-by: Franco Sola <fsola@squareup.com> # Conflicts: # docs/nips/NIP-FA.md # docs/nips/NIP-FI.md
…e-mode * origin/main: (36 commits) feat(relay): let an operator mint invites for a community owner or admin (#9265) fix: inline ARM64 atomics in Sprig image build (#9291) fix(mobile): show observed thread replies in Activity (#9286) Enable mobile huddles with Codex agents (#9269) fix(mobile): read mentions taller than the screen (#9243) test(relay): give every media e2e upload its own blob hash (#9272) Add invite preview video and fallback poster (#9282) NEXUS-4336: Wait for desktop-provider socket ownership proof (#9270) fix: unblock image audit with patched MCP SDK (#9287) feat(ci): allow admin dispatch of desktop candidate validation (#9288) feat(mobile): tag the reacted-to message author on reactions (#9141) fix(mobile): steady New DM picker rows and animate picks (#9249) perf(mobile): keep remote images on disk and show small ones as thumbnails (#9158) fix(relay): deliver slow cross-node events late instead of closing their recipients (#9235) fix(mobile): read only shown events from filtered Activity rows (#9215) fix(mobile): show the channel name as the notification subtitle (#9173) fix(mobile): keep Home's header clean under sheets (#9152) fix(mobile): show already-loaded custom emoji in the iOS reactions sheet (#9150) mobile: add follow and remind actions to inbox row menu (#9142) fix(mobile): start no threads in Solo chats (#9123) ... Signed-off-by: Will Pfleger <wpfleger@block.xyz>
Summary
The mobile app now saves remote images on disk. After a relaunch, avatars, custom emoji, message attachments and agent artwork show immediately. The app does not download them again.
Small images on a relay now load only the relay's thumbnail. The app never downloads the full-size upload to show a small image.
Why
Before this change, the app kept remote images only in memory. Each cold launch downloaded every image again. On a slow connection, attachments showed a "Loading image" placeholder for a long time.
Also, when a relay had no thumbnail for an image, the app downloaded the full-size upload. That upload can be up to 20 MiB, for an avatar that is a few dozen points wide.
What changes
Images on disk
MediaImageProviderreads and writes aMediaDiskCache. Each account in each community has its own cache, at<root>/communities/<community id>/media/<signer pubkey>. The cache is built onflutter_cache_manager, with a JSON index.Library/Caches(from a newbuzz/app_groupmethod channel). App extensions can then read the same cache. On other platforms, the root is the app cache directory.MediaCacheIndexmeasures each file when it writes it, becauseflutter_cache_managerlimits only the number of files. Every disk read marks the file used, and every write schedules eviction, including a refresh that replaces bytes the package already holds in memory (by default the package does neither for those).Thumbnails for small images
smallMediaImageProviders. They try the same thumbnails, fall back the same way, and show initials when neither thumbnail exists. Before, they downloaded the full-size upload and only decoded it small.Agent artwork
MediaImage, so it uses the disk cache too.MediaImageProviderthat their widgets read, through a newmediaImageProviderFor(ref, url).mobile/libusesImage.networkorNetworkImagenow.Demo
A cold relaunch (the app is stopped, then started) into a synthetic "QA demo" Solo chat that holds a few generated images. Shown at half speed.
Before: the attachment shows a "Loading image" placeholder while it downloads.
before.mp4
After: the attachment shows with the chat, from disk.
after.mp4
Tests
test/shared/relay/media_disk_cache_test.dartfollows one journey through the real provider and a real cache in a temporary directory. The first launch downloads and saves. An offline relaunch shows the full-size image, its thumbnail and an external image from disk, and the oversized image goes back to the network. Eight days later, only the external image refreshes. Removing the community deletes its directory.community_provider_testchecks that launch deletes orphan media directories, that a new signing key deletes only the old account's media, and that removing a community deletes its media.avatar_image_testchecks that a missing WebP thumbnail falls back to the JPEG thumbnail, and that an avatar with no thumbnail shows its initials.native_avatar_image_testchecks the same for native avatars: a WebP 404 paints the JPEG thumbnail, the full-size upload is never requested, a relay with no thumbnail paints initials and is not asked again, and any other failure stops at the initials.flutter analyzeand the full mobile test suite pass..thumb.webpentries for them, no full-size uploads. After a cold relaunch, the picker showed the same avatars within about 1.5 seconds, and the cache directory gained no files. The avatar editor backdrop was not exercised in the app. The test community has no remote agents, and the backdrop tests cover only emoji and picked-image selections.Before merge
🤖 Generated with Claude Code