DEV-79191: implemented caching images for offline visibility #79201
Conversation
melvin-bot
bot
requested review from
trjExpensify
and removed request for
a team
|
I have read the CLA Document and I hereby sign the CLA You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot. |
|
@cristipaval Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c36860d30c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // Use disk caching for images that need to work offline (e.g., receipts with auth tokens) | ||
| // Use no caching for other images to avoid memory leaks | ||
| // Check if cachePolicy is explicitly provided in props, otherwise default based on whether it's an auth-required image | ||
| const effectiveCachePolicy = props.cachePolicy ?? 'none'; |
There was a problem hiding this comment.
On iOS, BaseImage now honors whatever cachePolicy callers pass, which means components that already set cachePolicy="memory-disk" (e.g., Avatar.tsx and PlaidCardFeedIcon.tsx) will start caching on iOS as well. This conflicts with the adjacent comment in this file that non‑none cache policies cause memory leaks on iOS, and there’s no new guard to keep those non‑auth images on none. If that leak is still present, this change re‑introduces it for every avatar/icon render rather than just auth‑token receipts. Consider gating non‑none cache policies to the auth‑image path or stripping cachePolicy for iOS when isAuthTokenRequired is false.
Useful? React with 👍 / 👎.
|
@sohrabq you need to be hired on the parent issue first: #79191 CC: @cristipaval |
|
@trjExpensify got it, thanks for mentioning |
2 participants
Explanation of Change
Fixed Issues
$
PROPOSAL:
Tests
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
Uploading Screen Recording 2026-01-09 at 16.50.16.mov…
iOS: mWeb Safari
MacOS: Chrome / Safari