[Discussion] Add the ability to handle images in Onyx

[mallenexpensify]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Problem

Right now, we have no way of saving images offline. We can't use Onyx as it has storage limitations on web with a 5MB limit. There's been a lot of discussion on how to handle this in this issue and in this slack thread.

Why is this important?

This is important to be able to handle offline functionality of images

Solution

Create an offline storage solution for Onyx so that even when the application is offline and closed, the attached images will persist and continue to upload

Expensify/Expensify Issue URL:
https://github.com/Expensify/Expensify/issues/169197
cc @thienlnam

View all open jobs on Upwork

[MelvinBot]

Triggered auto assignment to @greg-schroeder (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

[parasharrajat]

Create a JNI-based AsyncStorage for Onyx that works across all platforms

Sorry but this looks like a huge task to do. Maybe we did not mean to do so. Can anybody confirm what exactly is need to be built? I expect that we need to expand AsyncStorage in a wrapper so that it supports a method like AsyncStorage.setImages() or AsyncStorage.removeImages(). Please, this looks daunting to me.

Moreover, a little bit about our plan to approach this which is finalized as a way to go. e.g.
IndexedDb on Web. FileSystem in native devices.

[thienlnam]

@parasharrajat Yup this is a huge task, but basically is going to be summed up with what needs to happen from this slack tread happening now. I updated the solution a bit but I imagine that it will keep updating until we figure out what the most viable solution for this will be

[mallenexpensify]

Posted to https://www.upwork.com/jobs/~011fcd226e59956a18

I'm going to add a new Contributor Manager, I'm out of office next week. They can update the price if needed (sounds like @parasharrajat might want for $250 though )

[MelvinBot]

Triggered auto assignment to @SofiedeVreese (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[parasharrajat]

@mallenexpensify 🎃. Maybe I will find a solution that takes lesser effort.

[aditya27dev]

@mallenexpensify I have applied on upwork by the name "Aditya Batra" on your JD.

[SofiedeVreese]

Hey @aditya27dev can you a proposal with the technical explanation of the changes you will make in this GH here? thanks!

[aditya27dev]

Hi @SofiedeVreese Sorry I can't share technical changes here publicly which is a inappropriate way of doing a job.

But I can work on this task, if you hire me.

[SofiedeVreese]

Hey @aditya27dev per our contributing guidelines, we do require contributors to make a proposal for their solution and post it as a comment in the corresponding GitHub issue. The solution proposal should include a brief technical explanation of the changes proposed. We don't hire if the contributing guidelines aren't being followed.

[thienlnam]

Throwing a new title on this since discussion is still ongoing in the slack thread, to view or participate check it out here

[kidroca]

One thing I forgot to bring up
When #4908 it will switch web/desktop from using one storage medium - localStorage to another - IndexedDB
Meaning any logged in users would be logged out

Should we include a migration that tries to move any existing Onyx values in localStorage to IndexedDB before the app starts (along the rest of the migrations)?

ios and android aren't going to be affected since they continue to use the same medium - AsyncStorage

[parasharrajat]

I guess yes, we should add that.

[tgolen]

Yeah, I agree, let's try to migrate everything if we can.

[kidroca]

Alright, I'm working on it

[kidroca]

Added the migration: https://github.com/Expensify/App/pull/4908/files#diff-fc59a9d775dba3004d8ed5a4dec154bbd2487455e7c720afed29bc8003a5228f

[kidroca]

PR was approved but we forgot to remove the HOLD label: #4908 (comment)

[kidroca]

PR (1) #4908 is merged
We can enable file persistence after PR (2) #6556 gets merged as well

It deals with the issue of prematurely deleting persisted request (and the files to be uploaded)

After #6556 we'll be able to remove the comment here and set persist to true

App/src/libs/actions/Report.js

Lines 1170 to 1173 in c224cfe

	// The persist flag enables this request to be retried if we are offline and the app is completely killed. We do
	// not retry attachments as we have no solution for storing them persistently and attachments can't be "lost" in
	// the same way report actions can.
	persist: !isAttachment,

[kidroca]

Still waiting on #6556

[kidroca]

#6556 was merged

Created a PR to enable attachment persistence: #7520

[kidroca]

Found 2 problems around persisted attachments

No loading animation/preview

You can see it on most of the videos on the PR #7520 (see web or mobile web)

1. Be offline
2. Select an attachment to be uploaded (preferable larger so it takes more time to upload)
3. Quit the app
4. Open the app again
5. Open the chat where the attachment was uploaded
6. Observe it's not there, and there's no indication something is uploading
7. The attachment only appears after the upload is over

Problem:

The optimistic action that serves as a placeholder during the upload is removed
We seem to clear any optimistic actions at launch or when a chat is opened
The reason seem to be to prevent discrepancies between local and online state - any optimistic actions are removed - if real actions exist for the same they would be fetched shortly

App/src/libs/actions/Report.js

Lines 451 to 457 in 425e718

	/**
	* Remove all optimistic actions from report actions and reset the optimisticReportActionsIDs array. We do this
	* to clear any stuck optimistic actions that have not be updated for whatever reason.
	*
	* @param {Number} reportID
	*/
	function removeOptimisticActions(reportID) {

Solution

I think we should try to cross reference optimistic actions with requests in the network queue This way when we clean up optimistic actions we keep the ones that still have pending request

We already put optimisticReportActionID on the request so we can distinct it

App/src/libs/actions/Report.js

Lines 1169 to 1175 in 425e718

	API.Report_AddComment({
	reportID,
	file,
	reportComment: commentText,
	clientID: optimisticReportActionID,
	persist: true,
	})

We can make a lib or expose a function that return any pending Report_AddComment actions from network queue or persisted storage
Report.removeOptimisticActions would use that and skip removing optimistic actions that have pending Report_AddComment

2. We're only persisting a request if we're offline

NetworkRequestQueue.saveRetryableRequests is only called in the if(isOffline) block

App/src/libs/Network.js

Lines 185 to 206 in 425e718

	if (isOffline) {
	if (!networkRequestQueue.length) {
	return;
	}
	const retryableRequests = [];
	// If we have a request then we need to check if it can be persisted in case we close the tab while offline.
	// We filter persisted requests from the normal Queue to remove duplicates
	networkRequestQueue = _.reject(networkRequestQueue, (request) => {
	const shouldRetry = lodashGet(request, 'data.shouldRetry');
	if (shouldRetry && request.data.persist) {
	// exclude functions as they cannot be persisted
	const requestToPersist = _.omit(request, val => _.isFunction(val));
	retryableRequests.push(requestToPersist);
	return true;
	}
	});
	if (retryableRequests.length) {
	NetworkRequestQueue.saveRetryableRequests(retryableRequests);
	}
	return;
	}

Problem:

If we quit the app while an upload is still pending it will neither be upload or persisted
For a request to be persisted you have to be offline first and then to make the request

Recent changes alleviate this to some extent - when a request is persisted to storage it will only be removed after it's successfully performed - it'll be retried until eventually succeeding
But it would only get to be persisted if you were offline to begin with

Solution

Requests with the `persist` flag should be persisted regardless of being offline or not If the user quits the app while the request isn't completed existing logic would retry it on next launch Otherwise added logic would remove it from persistent storage when it succeeds

The following

App/src/libs/Network.js

Lines 229 to 230 in 425e718

	processRequest(queuedRequest)
	.then(response => onResponse(queuedRequest, response))

becomes

 if (queuedRequest.data.persist) {
   NetworkRequestQueue.saveRetryableRequests([queuedRequest]);
 }

 processRequest(queuedRequest) 
     .then(response => {
        onResponse(queuedRequest, response);
        NetworkRequestQueue.removeRetryableRequest(queuedRequest);
     }) 

[thienlnam]

We should be live with this already and can close this right? @kidroca

[kidroca]

@thienlnam I think we have the bare minimum if that's enough I guess we can close

I've pointed out 2 problems using the feature the way it is right now

[thienlnam]

👍 Will keep this open until we resolve those issues

[kidroca]

Can someone confirm the proposed solutions are the way to go here?

Should we have a ticket(s) about these so we know they exist as problems? - to prevent duplicate reports of the same

[thienlnam]

I'll create separate tickets for them for more visibility and then we can discuss the proposed solutions in those tickets

[thienlnam]

Closing now that we have other tickets for the existing problems
#7934
#7935

[melvin-bot]

⚠️ Looks like this issue was linked to a possible regression on PRODUCTION here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a production regression has occurred a Root Cause Analysis is required. Please follow the instructions here.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.