[$1000] Web - A non-stoping spinner is displayed when opening an image previewer carousal with a deep link

[kbecciv]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Action Performed:

1. Open a chat with a random user
2. Send a picture
3. Open the sent picture
4. Copy link form the address bar
5. Sign out
6. Paste the copied link to the address bar
7. Press enter
8. Enter the same email address you have used to send the picture
9. Click continue > enter magic code

Expected Result:

Picture should finish loading & should be displayed

Actual Result:

Spinner keeps spinning without stop

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: 1.3.47.3
Reproducible in staging?: y
Reproducible in production?: y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation

Screencast.from.2023-07-29.14-09-00.mp4

Recording.4018.mp4

Expensify/Expensify Issue URL:
Issue reported by: @Natnael-Guchima
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1690629431697969

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01e0613ad8be2f9b94
• Upwork Job ID: 1686036587549114368
• Last Price Increase: 2023-07-31
• Automatic offers:
• huzaifa-99 | Contributor | 25986445
• Natnael-guchima | Reporter | 25986447

[melvin-bot]

Triggered auto assignment to @flaviadefaria (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[melvin-bot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[tienifr]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Web - A non-stoping spinner is displayed when opening an image previewer carousal with a deep link

What is the root cause of that problem?

When users login successfully, we're showing the AttachmentView because the report isn't loaded at that time, and we don't listen the report change.

App/src/components/AttachmentModal.js

Lines 331 to 337 in 024d210

	<AttachmentView
	containerStyles={[styles.mh5]}
	source={sourceForAttachmentView}
	isAuthTokenRequired={isAuthTokenRequired}
	file={file}
	onToggleKeyboard={updateConfirmButtonVisibility}
	/>

In

App/src/pages/home/report/ReportAttachments.js

Lines 28 to 38 in 024d210

	<AttachmentModal
	allowDownload
	defaultOpen
	report={report}
	source={source}
	onModalHide={() => Navigation.dismissModal(reportID)}
	onCarouselAttachmentChange={(attachment) => {
	const route = ROUTES.getReportAttachmentRoute(reportID, attachment.source);
	Navigation.navigate(route);
	}}
	/>

We don't pass isAuthTokenRequired so isAuthTokenRequired is false by default, but the report attachment must have isAuthTokenRequired is true to add encryptedAuthToken => We can't load the image

What changes do you think we should make in order to solve the problem?

In

App/src/pages/home/report/ReportAttachments.js

Lines 28 to 38 in 024d210

	<AttachmentModal
	allowDownload
	defaultOpen
	report={report}
	source={source}
	onModalHide={() => Navigation.dismissModal(reportID)}
	onCarouselAttachmentChange={(attachment) => {
	const route = ROUTES.getReportAttachmentRoute(reportID, attachment.source);
	Navigation.navigate(route);
	}}
	/>

we should enable isAuthTokenRequired because it comes from report.

What alternative solutions did you explore? (Optional)

If we don't want to enable isAuthTokenRequired in ReportAttachments because of some reasons, we should hide the attachment modal if we can't load the image

Result

Screen.Recording.2023-07-31.at.15.39.18.mp4

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01e0613ad8be2f9b94

[melvin-bot]

Current assignee @flaviadefaria is eligible for the External assigner, not assigning anyone new.

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @allroundexperts (External)

[huzaifa-99]

Proposal

Please re-state the problem that we are trying to solve in this issue.

We want the image preview to show when an attachment is opened with a deep link

What is the root cause of that problem?

I have a similar RCA as @tienifr but the solution is different.

The issue here is that when we open the report attachment via Deeplink the report is empty here and because of this we show the <AttachmentView/> here which displays a loading indicator until the image is loaded

What changes do you think we should make in order to solve the problem?

I think we should not directly pass isAuthTokenRequired: true, that doesn't feel safe and it also wont show the carousel when there are multiple attachments

The proper way I think would be to listen for report changes in <ReportAttachments/> instead of only getting the report once here by subscribing to the onyx key.

When we listen to the report changes and when the report loads, we will be displaying <AttachmentsCarousel/> here which would be the proper way.

// just some pseudocode for <ReportAttachments/>

// pass report from onyx
<AttachmentModal
    allowDownload
    defaultOpen
    report={props.report}

// subscribe to report onyx key
withOnyx({
    report: {
        key: ({route}) => `${ONYXKEYS.COLLECTION.REPORT}${getReportID(route)}`,
    },
}),

Now with the above changes, we are updating the attachment modal when the report is loaded via deep link. Since we are using deep link, its obvious that report actions are also not loaded (this is valid if we logout, then visit deeplink, then login and when deeplink is processed -- basically the reproduction steps in OP). We need them to be loaded since we use them in createInitialState.js which is used in <AttachmentsCarousel/> here. So we would have to wait for them to be also loaded as well.

This can be done by listening for report action onyx changes in <AttachmentModal/> and only render the <AttachmentCarousel/> when both report and report actions are loaded here

{!_.isEmpty(props.report) && !_.isEmpty(props.reportActions) ? (
    <AttachmentCarousel
        // ....

I added the !_.isEmpty(props.reportActions) here because it would show the loading indicator until reportActions are also loaded to be consistent with the loading process. However, this condition can be added in other places as well based on how we prefer to show the loader when reportActions are loading, like we can return early if(_.isEmpty(props.reportActions) return here as well and show loading indicator in <AttachmentCarousel/>, -- again based on when we prefer.

What alternative solutions did you explore? (Optional)

N/A

[tienifr]

listening for report actions onyx changes in is not the good idea, since it's not just used in ReportAttachment. And in AttachmentCarousel we're already listening report actions.

In small screen we don't call api openReport when access attachment preview from deeplink that why if we wait for the actions to be loaded, it can cause the infinity loading.

I believe showing the only one attachment preview when access from deeplink is good in this case (users just need to access this attachment and other attachments are not loaded)

[allroundexperts]

@huzaifa-99 I'm inclined towards your solution since I think we should display carousel instead of a single attachment. That being said, I'm not fully convinced about using reportActions for loading purposes. Can you confirm that your solution works on ALL platforms?

[huzaifa-99]

Thanks for the feedback guys.

In small screen we don't call api openReport when access attachment preview from deeplink that why if we wait for the actions to be loaded, it can cause the infinity loading.

Thank you for raising the concern @tienifr, you are right we dont call openReport in small screens but in small large screens we do. To not call openReport on small screens was (i believe) a kind of optimization to prevent unnecessary api calls and only call openReport when we are on the report screen. Under this assumption we were also assuming ReportAttachments will have the report loaded already since we can only open ReportAttachments from the report screen (and when we open report screen we already would have called the openReport api)

The issue here is deep link:
When we use deep link the above assumption "report will be already loaded in ReportAttachments" fails and we have to call openReport to get the necessary data and make it consistent with how we view attachments on a regular flow (i.e with carousel if needed)

@huzaifa-99 I'm inclined towards your solution since I think we should display carousel instead of a single attachment. That being said, I'm not fully convinced about using reportActions for loading purposes.

I originally proposed

these

This can be done by listening for report action onyx changes in and only render the when both report and report actions are loaded here

{!.isEmpty(props.report) && !.isEmpty(props.reportActions) ? (
<AttachmentCarousel
// ....
I added the !.isEmpty(props.reportActions) here because it would show the loading indicator until reportActions are also loaded to be consistent with the loading process. However, this condition can be added in other places as well based on how we prefer to show the loader when reportActions are loading, like we can return early if(.isEmpty(props.reportActions) return here as well and show loading indicator in , -- again based on when we prefer.

suggestions to listen for report actions changes in AttachmentModal

because of 2 reasons

1. in my tests it was breaking the app on this condition because page was -1 and we were calling navigate wrong params (again due to the assumption that report actions are loaded already)
   

   App/src/components/AttachmentCarousel/createInitialState.js

   Lines 53 to 58 in b041a2c

   	if (page === -1) {
   	Navigation.dismissModal();
   	}
   	// Update the parent modal's state with the source and name from the mapped attachments
   	props.onNavigate(attachments[page]);

but this recently (~20 hrs ago) got fixed in this commit

2. To show some kind of loading state in attachment modal by adding this condition

{!_.isEmpty(props.report) && !_.isEmpty(props.reportActions) ? (
    <AttachmentCarousel

here, which would not load the AttachmentCarousel and instead load the AttachmentView with loader until both report and report actions are loaded -- but again this is some subjective loading stuff which we can decide on where to show, its not a requirement to do

Now I propose an additional change to my original proposal

1. To load report actions if they are empty in ReportAttachments and we used deeplink

Can you confirm that your solution works on ALL platforms?

Yes, sure. I will put up videos for all platforms here in a short while and ping you again @allroundexperts

[huzaifa-99]

Can you confirm that your solution works on ALL platforms?

These are the test videos @allroundexperts

Desktop

Desktop.Native.mp4

Web Chrome

Web.Chrome.mp4

Web Safari

Web.Safari.mp4

mWeb Chrome

mWeb.Chrome.mp4

mWeb Safari

mWeb.Safari.mp4

Deep links for android and ios native seem to not work even on main following these steps

1. Logout
2. Use the deeplink for native device ex: npx uri-scheme open "new-expensify://r/7719268844648310/attachment?source=/chat-attachments/4704059366618417442/w_3cf7212100dc7bd0dee2b6d1969eb9aff225a4de.jpg" --ios (replace the url to be valid)
3. Login
4. Notice the console warning

IOS

IOS.mp4

I think this is a separate bug. Thoughts @allroundexperts

[allroundexperts]

@huzaifa-99's proposal looks good to me. I agree that we should keep on showing a carousel once an attachment is opened via deep link.

🎀 👀 🎀 C+ reviewed

On a side note, @huzaifa-99 you have to clear the app from memory before opening the deep link in native. Otherwise, it won't work.

[melvin-bot]

Triggered auto assignment to @robertjchen, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[huzaifa-99]

Thanks for the review @allroundexperts. Will wait for the assignment to start the PR

[robertjchen]

Thanks for the discussion here- having read the above, I agree that @huzaifa-99 's updated proposal would be best!

[melvin-bot]

📣 @allroundexperts Please request via NewDot manual requests for the Reviewer role ($1000)

[melvin-bot]

📣 @huzaifa-99 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[melvin-bot]

📣 @Natnael-Guchima 🎉 An offer has been automatically sent to your Upwork account for the Reporter role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

[mallenexpensify]

Held still

[robertjchen]

Still holding on #27168 it looks like?

[mallenexpensify]

I think... commented there cuz it was laggin a bit
#27168 (comment)

[robertjchen]

holding on results of #27168

[mallenexpensify]

Still holding.

[s77rt]

#27168 is closed. Can we proceed here?

[mallenexpensify]

Off hold, added the retest-weekly label. This bugs seems like an extreme edge case. The only reason I think it'd be worth fixing is because the end result renders the app useless with a spinner.

Are you able to reproduce @s77rt?

[s77rt]

Yes, still reproducible

Screen.Recording.2024-03-18.at.5.59.31.PM.mov

[mallenexpensify]

Thanks @s77rt , do you think the bug exists elsewhere? ie. from other types of links that are copy/pasted? With it being an edge case, if we're going to fix it, I'd love for the fix to help other instances, if they exist.

[s77rt]

@mallenexpensify Yes, I think this probably affects other links such as links to profile avatars, links to distance receipts, etc. (any image that is displayed in a modal)

[mallenexpensify]

Comment on the PR

I think we should wait for #27168 to get this resolved.
The above was closed, it was not reproducible.
Should we proceed here now?

@huzaifa-99 , if you're able to continue with this, can you test other instance like @s77rt mentioned above to see if we can fix more than just this single bug with the PR? Thx.

[mallenexpensify]

Added to #vip-vsb
Came off hold a couple days ago, it's quite a dusty issue, since the bug was reported in July.
Waiting on retest, we might be looking for brand new proposals

[mvtglobally]

Issue not reproducible during KI retests. (First week)

[mallenexpensify]

Also tried to repro and wasn't able to. Closing. Bonus, check out this bike I got last weeekend!