[HOLD for payment 2023-09-06] [$1000] Web - On reload on attachment preview and close, app displays random chat for few seconds #23958

kbecciv · 2023-07-31T20:23:21Z

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

Action Performed:

Open the app
Open any report
Send any image attachment
Click and open attachment in preview
Copy the attachment preview url
Go to other chat
Paste the saved url in the current search bar
Reload
Close the preview and observe that app displays random chat for few seconds before again displaying the right chat

Expected Result:

App should display same chat in which attachment is located when we close the preview - navigation should occur before closing the modal

Actual Result:

App displays random chat when we click on attachment, reload on preview and close the preview

Workaround:

Unknown

Platforms:

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

Version Number: 1.3.47.3
Reproducible in staging?: n/a
Reproducible in production?: n/a
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

app.displays.random.chat.on.closing.preview.after.reload.mp4

Expensify/Expensify Issue URL:
Issue reported by: @dhanashree-sawant
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1690103101672499

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~015ceaa38feb4bfce8
Upwork Job ID: 1687635491784937472
Last Price Increase: 2023-08-05
Automatic offers:

tienifr | Contributor | 26201629
dhanashree-sawant | Reporter | 26201630

melvin-bot · 2023-07-31T20:23:26Z

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

melvin-bot · 2023-07-31T20:23:28Z

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 · 2023-08-01T04:39:00Z

Proposal

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

Web - On reload on attachment preview and close, app displays random chat for few seconds

What is the root cause of that problem?

The attachment view is main route, meanwhile the report screen is center pane so it's not related to each other.

App/src/libs/Navigation/linkingConfig.js

Lines 16 to 27 in cb8aac1

    
           [SCREENS.REPORT_ATTACHMENTS]: ROUTES.REPORT_ATTACHMENTS, 
        
           // Sidebar 
        
           [SCREENS.HOME]: { 
        
               path: ROUTES.HOME, 
        
           }, 
        
           [NAVIGATORS.CENTRAL_PANE_NAVIGATOR]: { 
        
               screens: { 
        
                   [SCREENS.REPORT]: ROUTES.REPORT_WITH_ID, 
        
               }, 
        
           },

The url of attachment view is /r/7572730112622440/attachment, it is not center pane so in ReportScreenWrapper we can't get the reportID, that why we get the last access report id to render in center pane

App/src/libs/Navigation/AppNavigator/ReportScreenWrapper.js

Lines 80 to 81 in cb8aac1

    
           // If there is no reportID in route, try to find last accessed and use it for setParams 
        
           const reportID = getLastAccessedReportID(

the last access report id is different from the report id in url, so when we dismiss the attachment modal, we back to the current report then back to the report that contains attachment preview

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

Line 33 in cb8aac1

onModalHide={() => Navigation.dismissModal(reportID)}

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

Prior navigate to the parent report
In

App/src/libs/Navigation/AppNavigator/ReportScreenWrapper.js

Lines 63 to 67 in cb8aac1

    
           const getLastAccessedReportID = (reports, ignoreDefaultRooms, policies, isFirstTimeNewExpensifyUser, openOnAdminRoom) => { 
        
               const lastReport = ReportUtils.findLastAccessedReport(reports, ignoreDefaultRooms, policies, isFirstTimeNewExpensifyUser, openOnAdminRoom); 
        
               return lodashGet(lastReport, 'reportID'); 
        
           };

We should additionally check the current route has the reportID not not by using Navigation.getActiveRoute()

Here's the code sample

    const currentRoute = Navigation.getActiveRoute();
    const regex = /\/r\/(\d*)\/attachment/;
    const reportID = regex.exec(currentRoute)[1] // should add null check
    if(reportID) return  reportID;

Avoid navigating to the parent report after closing modal

Remove reportID in

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

Line 33 in cb8aac1

onModalHide={() => Navigation.dismissModal(reportID)}

Result

Screen.Recording.2023-08-01.at.11.37.05.mov

tienifr · 2023-08-01T04:43:04Z

@dhanashree-sawant @sakluger I think we should change the steps for more correct

Open the app
Open any report
Send any image attachment
Click and open attachment in preview
Copy the attachment preview url
Go to other chat
Paste the saved url in the current search bar
Reload
Close the preview and observe that app displays random chat for few seconds before again displaying the right chat

dhanashree-sawant · 2023-08-01T07:53:24Z

Okay yes, @tienifr steps seems better, if @sakluger is also able to reproduce with that step, we can change it.

sakluger · 2023-08-01T16:29:21Z

@tienifr thanks for providing those steps. Just to confirm, can you reproduce this issue consistently with those updated steps?

tienifr · 2023-08-01T16:30:39Z

Yes, I can @sakluger

melvin-bot · 2023-08-04T20:09:10Z

@sakluger Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

melvin-bot · 2023-08-05T01:23:52Z

Job added to Upwork: https://www.upwork.com/jobs/~015ceaa38feb4bfce8

melvin-bot · 2023-08-05T01:23:55Z

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

melvin-bot · 2023-08-05T01:24:02Z

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

sakluger · 2023-08-05T01:24:27Z

Updated the reproduction steps in the original description, thanks!

bernhardoj · 2023-08-05T04:08:28Z

Proposal

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

Closing the attachment carousel will show the last access report and then navigate to the report of the attachment.

What is the root cause of that problem?

When we refresh the app while the attachment carousel or any RHP is open, it will manually push the report screen to the navigation stack with empty params.

App/src/libs/Navigation/AppNavigator/createResponsiveStackNavigator/CustomRouter.js

Lines 29 to 35 in 4230f28

    
           // Make sure that there is at least one CentralPaneNavigator (ReportScreen by default) in the state if this is a wide layout 
        
           if (!isAtLeastOneCentralPaneNavigatorInState(partialState) && !options.isSmallScreenWidth) { 
        
               // If we added a route we need to make sure that the state.stale is true to generate new key for this route 
        
               // eslint-disable-next-line no-param-reassign 
        
               partialState.stale = true; 
        
               addCentralPaneNavigatorRoute(partialState); 
        
           }

App/src/libs/Navigation/AppNavigator/createResponsiveStackNavigator/CustomRouter.js

Lines 11 to 21 in 4230f28

    
           /** 
        
            * Adds report route without any specific reportID to the state. 
        
            * The report screen will self set proper reportID param based on the helper function findLastAccessedReport (look at ReportScreenWrapper for more info) 
        
            * 
        
            * @param {Object} state - react-navigation state 
        
            */ 
        
           const addCentralPaneNavigatorRoute = (state) => { 
        
               state.routes.splice(1, 0, {name: NAVIGATORS.CENTRAL_PANE_NAVIGATOR}); 
        
               // eslint-disable-next-line no-param-reassign 
        
               state.index = state.routes.length - 1; 
        
           };

If the param is empty, we will show the last accessed report id instead.

App/src/libs/Navigation/AppNavigator/ReportScreenWrapper.js

Lines 80 to 87 in 4230f28

    
           // If there is no reportID in route, try to find last accessed and use it for setParams 
        
           const reportID = getLastAccessedReportID( 
        
               props.reports, 
        
               !canUseDefaultRooms, 
        
               props.policies, 
        
               props.isFirstTimeNewExpensifyUser, 
        
               lodashGet(props.route, 'params.openOnAdminRoom', false), 
        
           );

In step 6, we go to another chat, which means the last accessed chat is not the chat that we send the attachment to. So, when we close the attachment carousel, it will show the last accessed report first (this is totally expected), and then show the report of the attachment. This is because we are calling Navigation.dismissModal(reportID) when the modal hides.

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

Line 33 in 4230f28

onModalHide={() => Navigation.dismissModal(reportID)}

Navigation.dismissModal(reportID) will replace the current screen with the passed reportID.

Btw, we can simplify the reproduction steps to:

Send an attachment to chat A
Open the attachment and copy the URL
Send the attachment URL to chat B
Click the attachment URL in chat B
Close the attachment carousel

After the carousel is closed, we will be navigated to chat A. This is the real issue.

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

When the carousel closed, just close it, no need to navigate to other report. To do that, we can either

Remove reportID from dismissModal
OR
Replace dismissModal with goBack

rushatgabhane · 2023-08-05T17:56:55Z

I like @tienifr's proposal! #23958 (comment)
C+ reviewed 🎀 👀 🎀

melvin-bot · 2023-08-05T17:57:01Z

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

rushatgabhane · 2023-08-05T17:58:29Z

@bernhardoj thanks for your proposal but I think we should display the report where attachment is coming from vs the last acessed report id

bernhardoj · 2023-08-06T08:52:57Z

@rushatgabhane thanks for the review.

Send an attachment to chat A

Open the attachment and copy the URL

Send the attachment URL to chat B

Click the attachment URL in chat B

Close the attachment carousel

After the carousel is closed, we will be navigated to chat A. This is the real issue.

What about this case? Navigating the user to the report of the attachment after closing the carousel is really annoying.

display the report where attachment is coming from

If you think we should do this, then I think we should do the same for other report sub-pages to make it consistent? For example, report settings (and sub-pages), participants, etc.

tienifr · 2023-08-07T04:30:55Z

What about this case? Navigating the user to the report of the attachment after closing the carousel is really annoying.

I don't think so, the attachment URL is just same like the internal link, when we click on the internal link, we should navigate to that page.

If you think we should do this, then I think we should do the same for other report sub-pages to make it consistent? For example, report settings (and sub-pages), participants, etc.

The profile pages are not the same as attachment modal.

The attachment modals belong to the report, that why when we hide the attachment modal, we should open the chat accordingly
The url of attachment modal is /r/:reportID/attachment? so we should open the correct report after hide modal. Otherwise, the url of profile page is /a/accountID, do not depends on the report => we don't need to display the report after closing this page

Julesssss · 2023-08-20T20:13:59Z

FYI I am OOO this week. Feel free to reassign in the meantime, or I will be back on Monday 28th

melvin-bot · 2023-08-23T21:37:23Z

Triggered auto assignment to @cristipaval (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

sakluger · 2023-08-23T21:38:09Z

@cristipaval I've assigned you to help review the PR: #25521

If you can't review, let us know, @Julesssss can do it when he's back.

melvin-bot · 2023-08-25T12:07:23Z

Based on my calculations, the pull request did not get merged within 3 working days of assignment. Please, check out my computations here:

when @tienifr got assigned: 2023-08-18 17:22:28 Z
when the PR got merged: 2023-08-25 12:07:23 UTC
days elapsed: 4

On to the next one 🚀

cristipaval · 2023-08-25T12:09:02Z

Based on my calculations, the pull request did not get merged within 3 working days of assignment. Please, check out my computations here:

when @tienifr got assigned: 2023-08-18 17:22:28 Z

when the PR got merged: 2023-08-25 12:07:23 UTC

days elapsed: 4

On to the next one 🚀

I think that's because @Julesssss is ooo and I was busy with wave4 stuff. PR was ready earlier

melvin-bot · 2023-08-30T01:08:41Z

Reviewing label has been removed, please complete the "BugZero Checklist".

melvin-bot · 2023-08-30T01:08:45Z

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.58-5 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

fix: 23958 reload attachment preview and close, app displays random chat for few seconds #25521

If no regressions arise, payment will be issued on 2023-09-06. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

External issue reporter - $250
Contributor that fixed the issue - $1500
Contributor+ that helped on the issue and/or PR - $1500

For reference, here are some details about the assignees on this issue:

@rushatgabhane does not require payment (Eligible for Manual Requests)
@tienifr requires payment offer (Contributor)
@dhanashree-sawant requires payment offer (Reporter)

Efficiency bonus applies to payments above! 🎉 🚀

melvin-bot · 2023-08-30T01:08:47Z

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

[@rushatgabhane] The PR that introduced the bug has been identified. Link to the PR:
[@rushatgabhane] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
[@rushatgabhane] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
[@rushatgabhane] Determine if we should create a regression test for this bug.
[@rushatgabhane] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
[@sakluger] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

sakluger · 2023-09-05T17:44:22Z

I went ahead and paid out the reporter bounty to @dhanashree-sawant

@rushatgabhane could you please complete the BZ checklist by tomorrow? That way we can approve your manual payment right away after the 7-day hold.

rushatgabhane · 2023-09-05T18:19:34Z

The PR that introduced the bug has been identified. Link to the PR: This is more like a new feature. The old behavior always existed and wasn't introduced by any PR.
The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: N.A.
A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: N.A.
Determine if we should create a regression test for this bug. - No
If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again - N.A.

rushatgabhane · 2023-09-05T18:36:56Z

Created a manual request here - https://staging.new.expensify.com/r/6288901853355212

sakluger · 2023-09-06T18:03:13Z

Thanks @rushatgabhane! I assigned @JmillsExpensify to handle the payment request.

Everyone else has been paid out, I'll leave this open so Jason can approve the final payment.

sakluger · 2023-09-08T03:08:17Z

We're good to close, the NewDot payment requests will be handled.

JmillsExpensify · 2023-10-07T15:17:32Z

$1,500 payment approved for @rushatgabhane based on BZ summary.

kbecciv added Daily KSv2 Needs Reproduction Reproducible steps needed Bug Something is broken. Auto assigns a BugZero manager. labels Jul 31, 2023

melvin-bot bot assigned sakluger Jul 31, 2023

melvin-bot bot added the Overdue label Aug 3, 2023

sakluger added External Added to denote the issue can be worked on by a contributor and removed Needs Reproduction Reproducible steps needed labels Aug 5, 2023

melvin-bot bot changed the title ~~Web - On reload on attachment preview and close, app displays random chat for few seconds~~ [$1000] Web - On reload on attachment preview and close, app displays random chat for few seconds Aug 5, 2023

melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 5, 2023

melvin-bot bot assigned rushatgabhane Aug 5, 2023

melvin-bot bot removed the Overdue label Aug 5, 2023

melvin-bot bot assigned Julesssss Aug 5, 2023

tienifr mentioned this issue Aug 19, 2023

fix: 23958 reload attachment preview and close, app displays random chat for few seconds #25521

Merged

59 tasks

melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Aug 19, 2023

sakluger unassigned Julesssss Aug 23, 2023

sakluger added the Engineering label Aug 23, 2023

melvin-bot bot assigned cristipaval Aug 23, 2023

sakluger assigned Julesssss Aug 23, 2023

melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Aug 30, 2023

melvin-bot bot changed the title ~~[$1000] Web - On reload on attachment preview and close, app displays random chat for few seconds~~ [HOLD for payment 2023-09-06] [$1000] Web - On reload on attachment preview and close, app displays random chat for few seconds Aug 30, 2023

melvin-bot bot removed the Reviewing Has a PR in review label Aug 30, 2023

melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Sep 5, 2023

sakluger closed this as completed Sep 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HOLD for payment 2023-09-06] [$1000] Web - On reload on attachment preview and close, app displays random chat for few seconds #23958

[HOLD for payment 2023-09-06] [$1000] Web - On reload on attachment preview and close, app displays random chat for few seconds #23958

kbecciv commented Jul 31, 2023 •

edited by melvin-bot bot

Loading

melvin-bot bot commented Jul 31, 2023

melvin-bot bot commented Jul 31, 2023

tienifr commented Aug 1, 2023 •

edited

Loading

tienifr commented Aug 1, 2023

dhanashree-sawant commented Aug 1, 2023

sakluger commented Aug 1, 2023

tienifr commented Aug 1, 2023

melvin-bot bot commented Aug 4, 2023

melvin-bot bot commented Aug 5, 2023

melvin-bot bot commented Aug 5, 2023

melvin-bot bot commented Aug 5, 2023

sakluger commented Aug 5, 2023

bernhardoj commented Aug 5, 2023

rushatgabhane commented Aug 5, 2023

melvin-bot bot commented Aug 5, 2023

rushatgabhane commented Aug 5, 2023

bernhardoj commented Aug 6, 2023

tienifr commented Aug 7, 2023

Julesssss commented Aug 20, 2023

melvin-bot bot commented Aug 23, 2023

sakluger commented Aug 23, 2023

melvin-bot bot commented Aug 25, 2023

cristipaval commented Aug 25, 2023

melvin-bot bot commented Aug 30, 2023

melvin-bot bot commented Aug 30, 2023 •

edited by JmillsExpensify

Loading

melvin-bot bot commented Aug 30, 2023

sakluger commented Sep 5, 2023

rushatgabhane commented Sep 5, 2023 •

edited

Loading

rushatgabhane commented Sep 5, 2023

sakluger commented Sep 6, 2023

sakluger commented Sep 8, 2023

JmillsExpensify commented Oct 7, 2023

[HOLD for payment 2023-09-06] [$1000] Web - On reload on attachment preview and close, app displays random chat for few seconds #23958

[HOLD for payment 2023-09-06] [$1000] Web - On reload on attachment preview and close, app displays random chat for few seconds #23958

Comments

kbecciv commented Jul 31, 2023 • edited by melvin-bot bot Loading

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

melvin-bot bot commented Jul 31, 2023

melvin-bot bot commented Jul 31, 2023

Bug0 Triage Checklist (Main S/O)

tienifr commented Aug 1, 2023 • edited Loading

Proposal

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

What is the root cause of that problem?

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

Result

tienifr commented Aug 1, 2023

dhanashree-sawant commented Aug 1, 2023

sakluger commented Aug 1, 2023

tienifr commented Aug 1, 2023

melvin-bot bot commented Aug 4, 2023

melvin-bot bot commented Aug 5, 2023

melvin-bot bot commented Aug 5, 2023

melvin-bot bot commented Aug 5, 2023

sakluger commented Aug 5, 2023

bernhardoj commented Aug 5, 2023

Proposal

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

What is the root cause of that problem?

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

rushatgabhane commented Aug 5, 2023

melvin-bot bot commented Aug 5, 2023

rushatgabhane commented Aug 5, 2023

bernhardoj commented Aug 6, 2023

tienifr commented Aug 7, 2023

Julesssss commented Aug 20, 2023

melvin-bot bot commented Aug 23, 2023

sakluger commented Aug 23, 2023

melvin-bot bot commented Aug 25, 2023

cristipaval commented Aug 25, 2023

melvin-bot bot commented Aug 30, 2023

melvin-bot bot commented Aug 30, 2023 • edited by JmillsExpensify Loading

melvin-bot bot commented Aug 30, 2023

sakluger commented Sep 5, 2023

rushatgabhane commented Sep 5, 2023 • edited Loading

rushatgabhane commented Sep 5, 2023

sakluger commented Sep 6, 2023

sakluger commented Sep 8, 2023

JmillsExpensify commented Oct 7, 2023

kbecciv commented Jul 31, 2023 •

edited by melvin-bot bot

Loading

tienifr commented Aug 1, 2023 •

edited

Loading

melvin-bot bot commented Aug 30, 2023 •

edited by JmillsExpensify

Loading

rushatgabhane commented Sep 5, 2023 •

edited

Loading