Store one image per surface for the remote messages and clear the new tab page surface image when the message is dismissed#7696
Conversation
… tab page surface image when the message is dismissed
This stack of pull requests is managed by Graphite. Learn more about stacking. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| } | ||
| val cardsList = message?.content as? Content.CardsList | ||
| val imageFile = remoteMessagingModel.getRemoteMessageImageFile() | ||
| val imageFile = remoteMessagingModel.getRemoteMessageImageFile(Surface.MODAL) |
There was a problem hiding this comment.
MODAL surface images never cleaned up when dismissed
Medium Severity
CardsListRemoteMessageViewModel fetches images using Surface.MODAL but never calls clearMessageImage(Surface.MODAL) in its action handlers (onCloseButtonClicked, onActionButtonClicked, onItemClicked). Unlike NewTabPageViewModel and RemoteMessageViewModel which explicitly call clearMessageImage(Surface.NEW_TAB_PAGE) when dismissing messages, the MODAL equivalent is missing. Since dismissMessage() no longer clears images automatically (removed in this PR), MODAL surface images will accumulate on disk.
There was a problem hiding this comment.
We need to persist the image for the modals so that we don’t download them again when accessing the modal from the “What’s New” entrypoint from settings.
...ing-impl/src/main/java/com/duckduckgo/remote/messaging/impl/store/RemoteMessageImageStore.kt
Show resolved
Hide resolved
| } | ||
|
|
||
| override suspend fun clearMessageImage(surface: Surface) { | ||
| withContext(dispatchers.io()) { |
There was a problem hiding this comment.
withContext(dispatchers.io()) can be removed for consistency with other methods here + is not needed as it is handled in deleteStoredImage.
|
|
||
| deleteStoredImage() | ||
| message?.surfaces?.forEach { surface -> | ||
| withContext(dispatcherProvider.io()) { |
There was a problem hiding this comment.
withContext(dispatcherProvider.io()) can be removed as it's not needed here. deleteStoredImage handles context switching internally.
| logcat { "RMF: Prefetching image: $imageUrl for message: ${message.id}" } | ||
| // Store one image per surface | ||
| message.surfaces.forEach { surface -> | ||
| withContext(dispatcherProvider.io()) { |
There was a problem hiding this comment.
Move withContext(dispatcherProvider.io()) above the forEach loop to have a single context switch for all surfaces.
| logcat { "RMF: Successfully saved image to permanent storage: ${permanentFile.absolutePath}" } | ||
| }.onFailure { error -> | ||
| logcat { "RMF: Failed to prefetch image $imageUrl for surface ${surface.jsonValue}: ${error.message}" } | ||
| } |
There was a problem hiding this comment.
Image downloaded redundantly for each surface
Low Severity
The fetchAndStoreImage method downloads the same image from Glide once per surface inside the forEach loop. When a message targets multiple surfaces (e.g., both MODAL and NEW_TAB_PAGE), this triggers redundant network requests for the same imageUrl. The Glide download (submit().get()) belongs outside the loop, with only the copyTo operation inside the loop to create surface-specific copies of the same downloaded file.
There was a problem hiding this comment.
Usually a message will target one surface.
anikiki
left a comment
anikiki
left a comment
There was a problem hiding this comment.
Looks good and works as expected! 🎉
2 participants
Task/Issue URL: https://app.asana.com/1/137249556945/task/1212974313804281
Description
This PR enhances the remote messaging image handling by implementing surface-specific image storage and cleanup. Now, images are stored per surface (NEW_TAB_PAGE, MODAL) and are properly cleaned up when messages are dismissed or actions are taken.
Steps to test this PR
Follow the steps in this task to test the PR: https://app.asana.com/1/137249556945/task/1213116538171640
UI changes
No visual changes, only behavior improvements
Note
Medium Risk
Touches shared remote-messaging APIs and image storage behavior, which could cause missing/stale images across surfaces if call sites aren’t updated or surface selection is incorrect.
Overview
Remote message image handling is now surface-specific:
getRemoteMessageImageFiletakes aSurfaceand a newclearMessageImage(surface)API clears only that surface’s cached file.The image store now saves separate files per surface (e.g.,
active_message_remote_image_<surface>.png) and callers (NewTabPageViewModel,RemoteMessageViewModel,CardsListRemoteMessageViewModel) request/clear images forNEW_TAB_PAGEvsMODAL; new-tab flows clear the image when a message is dismissed or an action is taken, except forAction.Share.Tests are updated/added to cover the new surface-parameterized APIs and the Share “don’t clear image” behavior.
Written by Cursor Bugbot for commit ec575e8. This will update automatically on new commits. Configure here.