-
Notifications
You must be signed in to change notification settings - Fork 3.5k
(1/2) Implement store/caching for attachments #65321
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…rors, and fixing some issues
|
@parasharrajat Thanks for reviewing! Can you please kindly speed up / prioritize this one and start testing?, I want to finish this PR asap. Many thanks |
|
Ok, Thanks. I am going to use some internal insight before moving here. Need to confirm a few things. |
|
@parasharrajat Please kindly keep me posted on this one. Thanks! |
|
No update for today. Going to check back in the evening. |
@parasharrajat Any updates?, it's already been a week |
|
I want to determine what should be done with this PR. Do we need these #65321 (comment) changes, or is the expected behavior changed? As the issue was created long ago, attachment behavior has changed since we started working on this implementation. I want to be sure about the expected behaviour before we merge this PR. This PR will improve the attachment caching, so the issue should say so for clarity. If needed, I will bring in some other C+ to help speed things up. |
|
As per the linked issue, the app is already working as expected. |
|
Slack discussion https://expensify.slack.com/archives/C02NK2DQWUX/p1761007224828439 |
|
As far as I remember, the original issue is not about cache image but rather than improving uploading large attachment i.e loader, opacity, etc But but as time goes by, pull requests, discussions, there's some bugs/issues, and And currently, there's some bugs in the attachment flow and we ended up holding to this PR:
I'll try to re-visit again the original issue and check it.
I can't access this conversation link @parasharrajat I think maybe we should start discussion/ask to the internal team/BZ member of the current to make sure everything is correct/aligned, and it would be better if you'd it inside the |
|
Thanks for the info. Yes, I agree we should start a group discussion. Normally, we start with a P/S statement for such large issues and then work on a design doc to lay down implementation details and design before moving on to implementation. We jumped the ladder on this issue as it is very old, and we picked things from previous contributors. At this point, I do not have clarity on the impact of this PR/implementation on the app. I would suggest we take a step back and spend some time on the missed steps, and take input from the internal team. If these changes are not useful to the app anymore, we should not invest more time in this and compensate for the efforts so far. Let's start with a Problem-solution statement(See contributing docs for instructions) first and post it on the Slack open source channel. You should already have everything to write it. Then I will tag the necessary engineers to evaluate it. Let me know if you need any help with it. |
|
Sure, I'll do it soon. Thanks |
Sorry for the delay, @parasharrajat. I'm not sure if this is the best approach to evaluate the current/expected behavior, since we've been working on this PR for around 6 months including with many changes. I think it might be better to tag the BZ team directly in this PR/linked issue or in a new Slack discussion to evaluate & make sure the current/expected behaviour. What do you think? Thanks |
|
Yes, we have taken a lot of time on the first implementation. When we started with this after discussion on the issue, I expected this PR to be ready on the first go, other than code changes. But we discovered many cases, and it took a few months to be ready. Yes, my feedback was crucial, but it should have been ready sooner. Due to it took a lot of time in implementation edge case discovery, there have been delays from my reviews, as it is not possible for me to dedicate 1 or 2 months to this Pr only for implementation.
This is the default process here for implementation/new features tasks. I was suggesting this to help us move forward with this implementation. We can ask BZ to reevaluate the expected behaviour, but as I explained earlier, it is not very clear now, which means we might have to drop this here, as I don't see any differences. |
|
Can you help me with a summary of the problem/solution here? I am going to tag BZ on the issue. I could write that down, but since I don't have a clear vision of these changes, I will overlook their importance. There will be more PRs ahead as implementation is not yet completed. Problem: Issue in the app that you are trying to solve. Please be detailed. |
|
@parasharrajat Ah, I found specifically where when we decided to move to local (store/caching) attachment:
Here's the slack discussion thread: https://expensify.slack.com/archives/C01GTK53T8Q/p1707270017261459 FYI: What I've already seen, it seems there's a lot discussion about improving uploading attachment experience |
|
Here's summarize problem/bugs we're trying to solve: Problems
Here's the proposal So, local attachment is introduced where we want to add large attachment support, the current limit is
And here's another step-by-step about large file upload support (BE+FE): https://expensify.slack.com/archives/C01GTK53T8Q/p1721930274037499?thread_ts=1707270017.261459&cid=C01GTK53T8Q
Note: Some of them has already been fixed. |
|
@parasharrajat Please let me know wdyt ^^. Thanks |
|
Ah, it is wrong. I am not asking to list down all the issues you found in this flow. I am asking the only problem we are solving here with this implementation for the linked issue. The problem you have in mind which working on with this implementation or you had while writing proposal. HEre is a sample |
|
@parasharrajat Sorry for the delay,
FYI: For the above comments, I am just answering this question why we need to create/have this PR for local store attachment not actual proposal/problem statement
But here's the proposal for this PR/local attachment caching Proposal: Add/support large file for uploading attachmentBackground:Currently, the app only support max 24mb size of file when uploading attachment and doesn't not handle byte-by-byte/chunk uploading, cancelled/resume request and when refreshing the app, it took sometimes to fully loaded the attachment from the server/BE Problem:
Solution:
Impact:This will allow user to upload various/large attachment file size, without worrying (performance, laggy, cancelled request, lost connection, closed app, etc) |
|
OK, Thanks for sharing this. I will share this internally and have someone pulled in here to help. |
|
@parasharrajat Any updates? |
|
Not yet. We will have on it next week. I will try to settle this discussion next week on this PR. |
|
Looking into this today. |
|
Internal discussion link https://expensify.slack.com/archives/C02NK2DQWUX/p1763364722326279 |
JmillsExpensify
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No product review required. Unsubscribing.

Explanation of Change
Original PR: #63723
This PR adds functionality to store and cache attachment files and markdown attacfhment (including images & videos) across all platforms, enabling offline preview.
Note: image preview in offline mode will be implemented in PR (2/2).
We use CacheAPI for web and filesystem for native platforms to manage attachment storage.
Each attachment has a unique ID (
attachment_id/data-attachment-id) for storing and previewing files on both web and nativeFor new attachments, we use the
data-attachment-idparameter, for older attachments which doesn't have this parameter, we combine thereportActionIDwith the attachment's index in the selection list:Single attachment:
reportActionID_0Multiple attachments (inside on action/message/comment)
reportActionID_0,reportActionID_1, etcFixed Issues
$ #9402
PROPOSAL: #9402 (comment)
Tests
Observing changes
Web:
OnyxDB:
keyvaluepairsattachment_...Cache API
Cache storage>attachmentsScreen.Recording.2025-08-21.at.13.24.41.mov
Native:
Physical device
com.expensify.chat.devor similar, usally it's located underroot/android/data/com.expensify.chat.devfilesfolder or related folder i.eDocuments/DCIM, for better results you can try to filter out only imagesNote: I don't have IOS physical device, so it might be similar to emulator device
New Expensify Devor similarEmulator device
3: B(Terminal): Go to Desktop > Open newly created file to preview the attachment
New Expensify DevFor more details can been seen in the attached recording videos below
Test cases:
Upload attachment file
Make sure list of new attachment is created in Onyx, here's the expectedResult:
Web:
Native:
And same also for CacheAPI (web/desktop-only:

Markdown image attachment
Make sure list of new attachment is created in Onyx, here's the expectedResult:
Web:
Native:
And same also for CacheAPI (web/desktop-only:

Markdown video attachment
Make sure list of new attachment is created in Onyx, here's the expectedResult:
Web:
Native:
And same also for CacheAPI (web/desktop-only:
// TODO, add here for video markdownOld attachment offline behaviour (only available in dev mode / next PR (2/2) )
Codebase changes:
ImageRenderer.tsxMake sure list of new attachment is created in Onyx, here's the expectedResult:
Web:
Native:
Update markdown attachment (only available in dev mode / next PR (2/2) )
Codebase changes:
ImageRenderer.tsxMake sure attachment stored in Onyx is updated, here's the expectedResult:
Web:
Native:
And same also for CacheAPI (web/desktop-only:

Delete markdown/attachment
Offline tests
Same as tests, for markdown attachment it will requires internet connection
QA Steps
Same as tests
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
Upload attachment file
9402_android_native_attachment_file.mp4
Markdown attachment
9402_android_native_markdown_attachment.mp4
Update attachment
9402_android_native_update_attachment.mp4
Delete attachment
9402_android_native_delete_attachment.mp4
Old attachment behaviour
9402_android_native_old_attachment_behaviour.mp4
Android: mWeb Chrome
Upload attachment file
9402_android_mweb_attachment_file.mp4
Markdown image attachment
9402_android_mweb_markdown_image.mov
Markdown video attachment
9402_android_mweb_markdown_video.mp4
Update attachment
9402_android_mweb_update_attachment.mp4
Delete attachment
9402_android_mweb_delete_attachment.mov
Old attachment behaviour
9402_android_mweb_old_attachment_behaviour.mp4
iOS: Native
Upload attachment file
9402_ios_native_attachment_file.mp4
Markdown attachment
9402_ios_native_markdown_attachment.mov
Update attachment
9402_ios_native_update_attachment.mp4
Delete attachment
9402_ios_native_delete_attachment.mov
Old attachment behaviour
9402_ios_native_old_attachment_behaviour.mp4
iOS: mWeb Safari
Upload attachment file
9402_ios_mweb_attachment_file.mov
Markdown image attachment
9402_ios_mweb_markdown_image.mov
Markdown video attachment
9402_ios_mweb_markdown_video.mov
Update attachment
9402_ios_mweb_update_attachment.mov
Delete attachment
9402_ios_mweb_delete_attachment.mov
Old attachment behaviour
9402_ios_mweb_old_attachment_behaviour.mov
MacOS: Chrome / Safari
Upload attachment file
9402_web_attachment_file.mov
Markdown image attachment
9402_web_markdown_image.mov
Markdown video attachment
9402_web_markdown_video.mov
Update attachment
9402_web_update_attachment.mp4
Delete attachment
9402_web_delete_attachment.mp4
Old attachment behaviour
9402_web_old_attachment_behaviour.mp4
MacOS: Desktop
Upload attachment file
9402_desktop_attachment_file.mov
Markdown image attachment
9402_desktop_markdown_image.mov
Markdown video attachment
9402_desktop_markdown_video.mov
Update attachment
9402_desktop_update_attachment.mov
Delete attachment
9402_desktop_delete_attachment.mov
Old attachment behaviour
9402_desktop_old_attachment_behaviour.mov