(1/2) Implement store/caching for attachments#63723
(1/2) Implement store/caching for attachments#63723NJ-2020 wants to merge 30 commits intoExpensify:mainfrom NJ-2020:new-feat/9402-1
Conversation
…rors, and fixing some issues
|
@parasharrajat 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] |
| (imagePickerFunc: (options: CameraOptions, callback: Callback) => Promise<ImagePickerResponse>): Promise<Asset[] | void> => | ||
| new Promise((resolve, reject) => { | ||
| imagePickerFunc(getImagePickerOptions(type, fileLimit), (response: ImagePickerResponse) => { | ||
| imagePickerFunc(getImagePickerOptions(type, fileLimit), async (response: ImagePickerResponse) => { |
There was a problem hiding this comment.
We don't allow async await.
There was a problem hiding this comment.
I am still waiting for this comment: #63723 (comment)
There was a problem hiding this comment.
Reverting this changes due to this comment
Will fix it in the next PR
| if (!response.assets?.length) { | ||
| return resolve(); | ||
| } | ||
|
|
||
| const localCopies = await keepLocalCopy({ | ||
| files: response.assets.map((asset) => ({ | ||
| uri: asset.uri, | ||
| fileName: asset.fileName ?? '', | ||
| })) as [FileToCopy, ...FileToCopy[]], | ||
| destination: 'documentDirectory', | ||
| }); | ||
|
|
||
| const assets = localCopies.map((localCopy, index) => { | ||
| if (localCopy.status !== 'success') { | ||
| throw new Error('Failed to create local copy for file'); | ||
| } | ||
|
|
||
| return { | ||
| ...(response.assets?.at(index) ?? {}), | ||
| uri: localCopy.localUri, | ||
| }; | ||
| }); | ||
|
|
||
| const targetAsset = assets.at(0); | ||
| const targetAssetUri = targetAsset?.uri; |
There was a problem hiding this comment.
We not need to cache the attachment while the user is adding one. Let's only worry about the message attachments for now.
There was a problem hiding this comment.
We're not caching it, this for uploading attachment from Attachment picker specifically from Choose from Gallery where the files are copied to cache directories but we need to copy it into document directories
More info: #63723 (comment)
There was a problem hiding this comment.
Replied on that linked thread. I think this is kind of out of scope of this issue.
| const imageSource = processedPreviewSource; | ||
| // const isAuthTokenRequired = isLocalFile(imageSource) ? false : isAttachmentOrReceipt; | ||
| const isAuthTokenRequired = isAttachmentOrReceipt; |
There was a problem hiding this comment.
Didn't understand this change?
There was a problem hiding this comment.
Ohh sorry, this is because when rendering attachment specifically using local uri, its failing to load the attachment, it's because we are passing the isAuthTokenRequired true for local uri
There was a problem hiding this comment.
const isAuthTokenRequired = isLocalFile(imageSource) ? false
But this says false for local file.
There was a problem hiding this comment.
Yes, right we need to pass false for local uri
There was a problem hiding this comment.
But the previous logic was doing the same so why did you change it?
There was a problem hiding this comment.
Nope
App/src/components/HTMLEngineProvider/HTMLRenderers/ImageRenderer.tsx
Lines 85 to 89 in f09f3a1
I think we can revert this changes because this related for retrieving attachment i.e the second phase of the PR (2/2), wdyt?
| return; | ||
| } | ||
| if (uri.startsWith('file://')) { | ||
| Onyx.set(`${ONYXKEYS.COLLECTION.ATTACHMENT}${attachmentID}`, { |
There was a problem hiding this comment.
Let's convert this collection to store attacments into a a collection for report, not per attachment basis.
So `${ONYXKEYS.COLLECTION.ATTACHMENT}${reportId}. This way we can subscribe this key in the report screen and get the source for any attachment otherwise, we will have to subscribe to each attachment separately.
There was a problem hiding this comment.
Please let me know how it looks first. Does this increase complexity and impact performance?
Overall, I believe we should create a custom hook to handle attachments. I am particularly curious on how you will handle the remote source change from backend. Can you show me code on how you will handle it?
There was a problem hiding this comment.
Let's convert this collection to store attacments into a a collection for report, not per attachment basis.
I think we're using the data-attachment-id attribute value from the attachment tag for both storing/retrieving attachment
Basically if there's any attachment it will render through ImageRenderer|VideoRenderer|AnchorRenderer, inside that we can retrieve the data-attachment-id attribute value provided from BE and use the attachment id value to get the local source from Onyx
There was a problem hiding this comment.
Maybe for alternative option instead of passing reportID we can use the reportActionID which is more easier to handle delete attachment, retrieving and others
But I think we can use the data-attachment-id from BE, which is specifically for attachments. For old attachments without attachment IDs, we can use reportActionID + {index}.
Please let me know how it looks first. Does this increase complexity and impact performance?
I am not sure if it's really impacting our app perfomance, because for every attachment we need to get the list of attachments of the current report(which can be more than 1), but we only need 1 attachment source from all the lists and we need to filter out again that matches with the data-attachment-id value
Wdyt?
There was a problem hiding this comment.
Overall, I believe we should create a custom hook to handle attachments. I am particularly curious on how you will handle the remote source change from backend. Can you show me code on how you will handle it?
Currently, I handle it inside the getAttachmentSource function, where first I compared the current source from BE with attachment.remoteSource, if there's any differences, then we will recache the attachment and show the new one
App/src/libs/actions/Attachment/index.native.ts
Lines 47 to 56 in 21f59ee
Edit:
Ah, you're right I think we need to make new hook for retrieving attachment specifically for CacheAPI i.e like useOnyx, because it's returning Promise value
But I think I'll do it in the second phase of the PR, right?
There was a problem hiding this comment.
yeah, second PR sounds good,
| return; | ||
| } | ||
|
|
||
| RNFetchBlob.config({fileCache: true, path: `${RNFetchBlob.fs.dirs.DocumentDir}/${attachmentID}`}) |
There was a problem hiding this comment.
Not a good idea to store in DocumentDir as it can not be cleared but system cleanup. We should instead create a subfolder in the CacheDir and save attachments there.
When we are clearly the cacheDir on signout. We exclude this folder from that cleanup.
There was a problem hiding this comment.
It's because when a user upload an attachment, if we save the file into cache dir, the attachment preview will gone if the user restart the app/device
So to fix this we need to store the attachment into documents directory i.e com.expensify.com/files/attachment_{id}
We will recache the attachment if there's new changes from BE, if not then we will use from the pre-cached attachment
There was a problem hiding this comment.
I don't think we need to worry about the attachment preview when the user is uploading the attachment. Until it is sent, we don't have to save it for offline preview. If needed we would do that later in different PR.
There was a problem hiding this comment.
I don't think we need to worry about the attachment preview when the user is uploading the attachment. Until it is sent
Agree, so I think we can do it inside the storeAttachment function
we don't have to save it for offline preview
I think it will show weird behavior to the user if the user close and then reopen the app, because it will show empty content(offline icon) for all optimistic attachments
Wdyt?
There was a problem hiding this comment.
When you close the app on add attachment modal, I think user will have to reopen the flow. Anyways, as I said, we will revisit this after we are done saving attachments in different PR, if needed.
Let's break it down to smaller PRs to faster completion.
There was a problem hiding this comment.
Okay, I will do this in the next PR
| if (file) { | ||
| parameters.attachmentID = attachmentID; | ||
| } | ||
|
|
There was a problem hiding this comment.
We need to pass this to get the data-attachment-id attribute inside the attachment tag i.e <img src="..." data-attachment-id="1234..." />
For markdown text link attachments, we will use the reportActionID + index
There was a problem hiding this comment.
It's used for storing and retrieving the attachment from Onyx/CacheAPI by using the data-attachment-id attachment_{idHere}
| attachmentID: string; | ||
|
|
||
| source?: string; | ||
|
|
||
| remoteSource?: string; |
There was a problem hiding this comment.
Missing properties comment.
| return; | ||
| } | ||
|
|
||
| RNFetchBlob.config({fileCache: true, path: `${RNFetchBlob.fs.dirs.DocumentDir}/${attachmentID}`}) |
There was a problem hiding this comment.
I don't think we need to worry about the attachment preview when the user is uploading the attachment. Until it is sent, we don't have to save it for offline preview. If needed we would do that later in different PR.
|
@NJ-2020 Thanks for your work on this, and remain committed while we were discussing it. Can you please speed up the implementation? I believe we will have to go through a few iterations before we complete this task. Thus, it is important to update daily. |
|
Okay, sure |
|
PR will be ready tomorrow I am still working on the unit test file |
|
There's some issues while creating the unit test file |
|
PR is ready cc: @parasharrajat |
|
Bump @parasharrajat for review |
| if (!key) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
throw an error instead if key is not from passed CACHE_API_KEYS in init
src/libs/CacheAPI/index.ts
Outdated
| if (!cacheName || !key || !value) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
We don't have to check for these when all of them are required params. Please define the type in in App Response.
There was a problem hiding this comment.
Also, add a logic that key is from CACHE_API_KEYS as this will allow overriding the settings passed in init.
There was a problem hiding this comment.
Also, add a logic that key is from CACHE_API_KEYS as this will allow overriding the settings passed in init.
Done
There was a problem hiding this comment.
We don't have to check for these when all of them are required params. Please define the type in in App Response.
Regarding this, CacheAPI only allow response value type, in this case the return response value returned from the fetch API, so I think defining our own response type definition I think it may can lead to typeerror or unmatch type with certain urls, wdyt?
There was a problem hiding this comment.
its fine to use standard type.
src/libs/CacheAPI/index.ts
Outdated
| if (!keys || keys.length === 0) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
we should not pass any keys here instead they should be taken from the keys passed in init.
| return cache.match(key); | ||
| }); | ||
| } | ||
| function remove(cacheName: string, key: string) { |
There was a problem hiding this comment.
Let's keep this cacheName internal in the cacheAPI.
| }; | ||
| }) as [FileToCopy, ...FileToCopy[]], | ||
| destination: 'cachesDirectory', | ||
| destination: 'documentDirectory', |
There was a problem hiding this comment.
Do we need this change now?
There was a problem hiding this comment.
Yes, because the caches directory will automatically cleared out when the app is closed which prevents the user to see the attachment when re-opening the app again
| global.fetch = TestHelper.getGlobalFetchMock(); | ||
| }); | ||
|
|
||
| it('should store for file in Onyx', async () => { |
There was a problem hiding this comment.
Add tests for:
- Delete the attachment.
- Change of remote source should reflect the change in local source.
- After attachment is stored, you should confirm whether it was cached or not. Have tests for both Native and web.
There was a problem hiding this comment.
I will create the unit test file after all the changes are done
There was a problem hiding this comment.
Let's add it now. It is to validate that your changes work so as you go along updating this implementation the tests should also pass.
There was a problem hiding this comment.
We follow strict naming, so please move this to tests/actions/AttachmentTest.ts if you action file is called Attachment.
| attachmentID: string; | ||
|
|
||
| /** Source url of the attachment either can be local or remote url */ | ||
| source?: string; |
There was a problem hiding this comment.
Why would this source be remote? Should the remoteSource be remote? we should make sure this property always reflect local source and remoteSource remote.
There was a problem hiding this comment.
Sorry may bad, done, fixed
src/libs/actions/Session/index.ts
Outdated
| NetworkConnection.clearReconnectionCallbacks(); | ||
| SessionUtils.resetDidUserLogInDuringSession(); | ||
| resetHomeRouteParams(); | ||
| CacheAPI.clear([CONST.CACHE_API_KEYS.ATTACHMENTS]); |
There was a problem hiding this comment.
Could this slow down the signout process when there are many resources to be removed?
There was a problem hiding this comment.
I've tried measuring the logout function performance using like this:
function cleanupSession() {
const startTime = performance.now();
Pusher.disconnect();
Timers.clearAll();
Welcome.resetAllChecks();
MainQueue.clear();
HttpUtils.cancelPendingRequests();
PersistedRequests.clear();
NetworkConnection.clearReconnectionCallbacks();
SessionUtils.resetDidUserLogInDuringSession();
resetHomeRouteParams();
CacheAPI.clear();
clearCache().then(() => {
const endTime = performance.now();
const duration = endTime - startTime;
// console.log...
});
clearSoundAssetsCache();
Timing.clearData();
}And here's the result using 5 attachments video, for each video the size is: 14.3MB and the total is 71.5MB :
| Without clearing CacheAPI | With clearing CacheAPI |
|---|---|
| Duration: 174ms | 245.8ms |
There was a problem hiding this comment.
That's a lot. So if a user keeps on using the app for a long time and logs out, the app might take seconds to log out.
There was a problem hiding this comment.
Yes, so I think for alternative solution, I think we can clear the cache api when the user is on the login page or inside the PublicScreens, by using this way it will not affect the logout process, wdyt?
There was a problem hiding this comment.
No a good idea. Maybe we leave it as it is. When a user uninstalls the app, whether the cache is cleaned or not?
We might need to add an option in the app to clean the storage manually.
There was a problem hiding this comment.
When a user uninstalls the app, whether the cache is cleaned or not?
I am not sure about about uninstalling the app, because I don't know if the native os is deleting the entire app including the media folder which will also delete the attachment files
But for web if we don't clear the data/attachments on signout process, the data will keep saved until the user removes completely from their application storage
We might need to add an option in the app to clean the storage manually.
Yes, and I think maybe we can also implement like maybe storage management?
src/libs/actions/Report.ts
Outdated
| }); | ||
|
|
||
| attachments.forEach((attachment) => { | ||
| storeAttachment(attachment.attachmentID, attachment.uri ?? ''); |
There was a problem hiding this comment.
There is a problem with code, what if the message failed to send the image will still be cached. we should follow optimistic patterns here.
you should update Onyx store values in optimisticData, successData and failureData. Same for delete attachment. Now this can be little tricky let's see what options do we have.
We can do auto cleanup and save when a attachment key is updated in Onyx.
we have a subscription to attachment collection which will update whenever we make changes to it so when you add a new attachment with remoteSource and attachmentID, we can update the cache to save the remote source and same for delete, we can remove the cache. This way cache will always be in sync with Onyx. This is just an idea and I am not sure how fast would it be. Please suggest yours also.
That's why I was thinking we save attachments report basis so we can subscribe to only attachment of visible report.
There was a problem hiding this comment.
We need to consider this case. It is important app concept.
There was a problem hiding this comment.
I think for Onyx we can solve it by passing into optimisticData, successData and failureData
But regarding CacheAPI it seems a little bit tricky, I am not sure regarding the subscribtion solution because we have to manage the Onyx and CacheAPI consistent in each other and make sure it runs smoothly
Please suggest yours also
IMO, I think we can introduce new method prop function inside libs/API onSuccess and onFailure
Here's an example
API.write(commandName, parameters, {
optimisticData,
successData,
failureData,
onSuccess: () => {},
onFailure: () => { /** remove attachment from CacheAPI */ },
});Wdyt?, we can improve this later
There was a problem hiding this comment.
IMO, I think we can introduce new method prop function inside libs/API onSuccess and onFailure
Here's an example
We can't do that. It is against our architecture. We need to find a way to do this. But we can focus on it later.
src/setup/index.ts
Outdated
|
|
||
| const CacheAPIKeys = Object.values(CONST.CACHE_API_KEYS); | ||
|
|
||
| CacheAPI.init(CacheAPIKeys); |
There was a problem hiding this comment.
let's pass CONST.CACHE_API_KEYS it array to init function to pass CONST.CACHE_API_KEYS directly this function and then handle getting keys inside the function.
There was a problem hiding this comment.
I left a very confusing commen. Sorry about that. What i meant to say was that we should pass CONST.CACHE_API_KEYS directly to the init function or send specific keys are array.
eg.
As object
CacheAPI.init(CONST.CACHE_API_KEYS)
As array
CacheAPI.init([Key1, Key2]);
But I am fine with the current logic where you are directly accessing them inside the function as we do not have any other keys currently for cache.
There was a problem hiding this comment.
Oh, okay, thank you
src/CONST/index.ts
Outdated
| ATTACHMENT: /<(img|video)[^>]*>/gi, | ||
| ATTACHMENT_ID: /data-attachment-id=(["'])(.*?)\1/, | ||
| ATTACHMENT_SOURCE_ID: /chat-attachments\/(\d+)/, | ||
| ATTACHMENT_SOURCE: /(src|data-expensify-source|data-optimistic-src)="([^"]+)"/i, |
There was a problem hiding this comment.
This is somewhat confusing. Can you please add comments to explain use of each regex.
| if (!('caches' in window)) { | ||
| throw new Error('Cache API is not supported'); | ||
| } | ||
| const keys = Object.values(CONST.CACHE_API_KEYS); |
There was a problem hiding this comment.
Ah, maybe I wan't clear. We have to use keys passed in parm but this is fine for now.
|
Bump @parasharrajat for above comments ^ |
|
Bump @parasharrajat |
|
I will start testing this tomorrow. |
|
Bump @parasharrajat |
|
Really sorry, due to some reasons, I've to close this PR New PR here: #65321 - I am unable to reopen this PR |
|
Apologies for the confusion here |

Explanation of Change
Store/Cache attachment when uploading a file or markdown text link attachment i.e
Fixed Issues
$ #9402
PROPOSAL: #9402 (comment)
Tests
Same as QA
Offline tests
Same as QA
QA Steps
Prerequisites: DEV mode only
File Upload
1. Open attachment picker
2. Select any file
3. Go to CacheAPI/Onyx storage for web and observe the changes
Markdown text link attachment
1. Go to input chat
2. Type any markdown text link attachment e.g
and send it3. Go to CacheAPI (web-desktop only) or Onyx (attachment_{id}) storage and observe the changes
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))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
9402_android_native.mov
Android: mWeb Chrome
9402_android_mweb.mov
iOS: Native
9402_ios_native.mov
iOS: mWeb Safari
I have an issue while inspecting the web)
MacOS: Chrome / Safari
9402_web.mov
MacOS: Desktop
9402_desktop.1.mov