Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
2.99k stars 2.5k forks source link

[$250] WS Chat – Blank page opens when go back from IOU thread in a Collect WS chat #40937

Open lanitochka17 opened 3 weeks ago

lanitochka17 commented 3 weeks ago

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


Version Number: 1.4.65-4 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: N/A Email or phone of affected tester (no customers): applausetester+jp_e_category@applause.expensifail.com Issue reported by: Applause - Internal Team

Action Performed:

  1. Go to https://staging.new.expensify.com/
  2. Log in as Collect WS Employee
  3. In a WS chat create a Submit expense
  4. Open just created Submit expense
  5. Navigate back to WS chat via header link

Expected Result:

Collect WS chat with history opens

Actual Result:

Blank page opens when go back from IOU thread in a Collect WS chat

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/ca52fdf6-bcf5-400d-be4a-ff4a86d269a2

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01dee105cffad79099
  • Upwork Job ID: 1783269604755496960
  • Last Price Increase: 2024-05-08
  • Automatic offers:
    • jjcoffee | Contributor | 0
Issue OwnerCurrent Issue Owner: @thesahindia
melvin-bot[bot] commented 3 weeks ago

Triggered auto assignment to @sakluger (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

lanitochka17 commented 3 weeks ago

@sakluger FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

lanitochka17 commented 3 weeks ago

We think that this bug might be related to #wave-collect - Release 1

melvin-bot[bot] commented 3 weeks ago

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

melvin-bot[bot] commented 3 weeks ago

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

beodw commented 3 weeks ago

Cannot reproduce...

https://github.com/Expensify/App/assets/48998844/7dfd5b53-8593-4022-91da-538cc2cb85b5

charles-liang commented 3 weeks ago

Proposal

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

WS Chat – Blank page opens when go back from IOU thread in a Collect WS chat

What is the root cause of that problem?

https://github.com/Expensify/App/blob/0f39e4370ccd84ccf7e1647b42cb505a69a2e9a3/src/pages/home/report/ReportActionsView.tsx#L164-L182

The issue is that when the network is slow, loading is set to true, and then when the page switches, loading remains true, resulting in an empty array being returned. If the page is not refreshed, loading will remain blank, waiting for the response to return.

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

remove condition isLoading in Line 169 if not effect another PR. As your comment suggests, even if loading, it shouldn't be blank, should use data what we had. Therefore, I suggest removing it here.

What alternative solutions did you explore? (Optional)

N/A

charles-liang commented 3 weeks ago

Reproduce method is turn on slow network, like chrome network or emulator setting

beodw commented 3 weeks ago

Proposal

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

Collect WS Chat – Blank page opens when go back from IOU thread in a Collect WS chat

What is the root cause of that problem?

The reportsActions array is temporarily empty in the ReportActionsView while loading new report data on slow connections. This is because it takes a while for the response to be received from the backend and within the reportActionView component we set reportActions to an empty array while loading.

const reportActions = useMemo(() => {
        if (!reportActionID) {
            return combinedReportActions;
        }
        if (isLoading || indexOfLinkedAction === -1) {
            return [];
        }

        if (isFirstLinkedActionRender.current) {
            return combinedReportActions.slice(indexOfLinkedAction);
        }
        const paginationSize = getInitialPaginationSize;
        return combinedReportActions.slice(Math.max(indexOfLinkedAction - paginationSize, 0));

        // currentReportActionID is needed to trigger batching once the report action has been positioned
        // eslint-disable-next-line react-hooks/exhaustive-deps
    }, [reportActionID, combinedReportActions, indexOfLinkedAction, isLoading, currentReportActionID]);

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

In the reportsActionsView component we can return a skeleton loader instead of null when the reportActions array is temporarily empty.

In ReportsActionsView.tsx:

if (!reportActions.length) {
        return null;
    }

Changes to:

if (!reportActions.length) {
        return <ReportActionsSkeletonView />;
    }

What alternative solutions did you explore? (Optional)

None

https://github.com/Expensify/App/assets/48998844/9e09229e-ddae-4272-acec-de95312695c0

beodw commented 2 weeks ago

@jjcoffee @sakluger Hi guys, kindly following up on whether this issue has been resolved. If not, please kindly let me know when you will have the capacity to review it. Cheers!

jjcoffee commented 2 weeks ago

@beodw Thanks for the proposal!

I think the expected result here is probably not to show a loader, as we actually already have all the data we need. It looks like there's something specifically wrong with loading via the route that highlights the expense's reportAction, i.e. /r/{reportID}/{reportActionID}. If you omit the latter part, it loads immediately as expected (regardless of connection speed).

https://github.com/Expensify/App/assets/27287420/aa089042-5465-48e7-8a4a-0c8f3e52de91

melvin-bot[bot] commented 2 weeks ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

sakluger commented 2 weeks ago

@beodw @charles-liang do you have any updated proposals based on @jjcoffee's comment?

charles-liang commented 2 weeks ago

@sakluger

I don't know if they didn't read my proposal, or if I wasn't clear. I suggested using a cache from the previous, to store the data. Then, when we return, reload it, and then asynchronously refresh the network data. Isn't this the same as @jjcoffee comment?

jjcoffee commented 2 weeks ago

@charles-liang Apologies for not giving feedback on your proposal. We don't need to implement caching as such. You can see from the issue's video that tapping on the report directly loads it just fine (as I also point out here).

charles-liang commented 2 weeks ago

@jjcoffee I'm also apology that I miss understand your comment.

charles-liang commented 2 weeks ago

Proposal

Updated

jjcoffee commented 1 week ago

remove condition isLoading in Line 169 if not effect another PR

@charles-liang Generally it's best to know in advance if the solution would cause a regression. In this case it seems highly likely that this would stop the loader from showing in cases where we want it to show.

jjcoffee commented 1 week ago

Just a heads up I'll be OOO 6-13th, so feel free to re-assign if proposals start rolling in before I'm back!

charles-liang commented 1 week ago

@jjcoffee I reviewed the commit history, and it was introduced in this commit. Before this, the reportActions data would not return empty due to other influences.

https://github.com/Expensify/App/blob/f306f4b585b5f73674685309c44add9c3a9d7f36/src/pages/home/ReportScreen.tsx#L264-L270

https://github.com/Expensify/App/blob/f306f4b585b5f73674685309c44add9c3a9d7f36/src/pages/home/ReportScreen.tsx#L550-L559

Moreover,

  1. enabling the loading skeleton is on ReportScreen not in ReportActionsView

https://github.com/Expensify/App/blob/c496ce653943934f6a47bae2fcf520ae08c69743/src/pages/home/ReportScreen.tsx#L715

  1. loading new data from network in the background is also from ReportScreen

Therefore, I conclude that this loading condition is unnecessary and will not impact other PRs.

melvin-bot[bot] commented 1 week ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot[bot] commented 1 week ago

@sakluger @jjcoffee this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

melvin-bot[bot] commented 1 week ago

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

sakluger commented 1 week ago

Reassigning C+ since JJ is out until next week.

melvin-bot[bot] commented 1 week ago

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

sakluger commented 1 week ago

@thesahindia FYI we have an updated proposal from @charles-liang here.

tsa321 commented 6 days ago

Proposal

Workspace expense chat becomes blank page when go back from IOU / transaction thread.

What is the root cause of that problem?

In here:

https://github.com/Expensify/App/blob/2a6d63d7f17b1e4fba94f26103f13c6afe294b91/src/pages/home/report/ReportActionsView.tsx#L403-L415

The shouldOpenReport condition always become true for this issue because hasCreatedAction always return false.

https://github.com/Expensify/App/blob/2a6d63d7f17b1e4fba94f26103f13c6afe294b91/src/pages/home/report/ReportActionsView.tsx#L233-L234

This is because type of oldestReportAction is INVITETOROOM and the CREATED is secondLast action.

This will call the Report.openReport and will set ReportMetadata.isLoadingInitialReportActions to be true: in here The isLoadingInitialReportActions value of true will cause isLoading to be true and will cause reportActions to be empty array. Set in here:

https://github.com/Expensify/App/blob/2a6d63d7f17b1e4fba94f26103f13c6afe294b91/src/pages/home/report/ReportActionsView.tsx#L165-L167

and So the report screen will be empty / blank.

The hasCreatedAction that always false will cause infinite loop between calling Report.openReport -> rendering the ReportActionsView (because report_metadata isLoadingInitialReportActions changes) -> calling Report.openReport.

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

https://github.com/Expensify/App/blob/2a6d63d7f17b1e4fba94f26103f13c6afe294b91/src/pages/home/report/ReportActionsView.tsx#L233-L234

in here we must use last and second last action to check whether it is created action. For assurance we could check on the last 3 report action whether the report actions has CREATED type.

The rough code could be:

const threeOldestReportAction = reportActions.slice(-3)
const hasCreatedAction = threeOldestReportAction.findLast((action) => action?.actionName === CONST.REPORT.ACTIONS.TYPE.CREATED);
melvin-bot[bot] commented 5 days ago

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

jjcoffee commented 4 days ago

@sakluger Happy to take this one back if @thesahindia is too busy!

sakluger commented 4 days ago

Thanks @jjcoffee! I've assigned it back to you.

melvin-bot[bot] commented 4 days ago

📣 @jjcoffee 🎉 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 📖

jjcoffee commented 3 days ago

Actually not able to reproduce myself anymore. Are you guys able to repro still? @tsa321 @charles-liang @beodw

charles-liang commented 3 days ago

@jjcoffee you can try

  1. set in a lower speed network with chrome dev tool
  2. go into iou thread
  3. change thread while loading immediately
tsa321 commented 3 days ago

@jjcoffee I cannot reproduce anymore. Looks like already fixed from BE. The oldest reportAction.actionName is 'CREATED' and second oldest is INVITETOROOM :

Screenshot 2024-05-16 at 05 48 43

jjcoffee commented 2 days ago

@sakluger Can we ask applause to retest here just to make sure?

melvin-bot[bot] commented 2 days ago

Triggered auto assignment to @dylanexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

sakluger commented 2 days ago

Sure! I added the retest-weekly label.

I also reassigned to @dylanexpensify since I'll be OOO until May 31.