App
App copied to clipboard
Expensify
[$250] WS Chat – Blank page opens when go back from IOU thread in a Collect WS chat
If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] 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): [email protected] Issue reported by: Applause - Internal Team
Action Performed:
- Go to https://staging.new.expensify.com/
- Log in as Collect WS Employee
- In a WS chat create a Submit expense
- Open just created Submit expense
- 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?
- [x] Android: Native
- [x] Android: mWeb Chrome
- [ ] iOS: Native
- [ ] iOS: mWeb Safari
- [x] MacOS: Chrome / Safari
- [ ] MacOS: Desktop
Screenshots/Videos
Add any screenshot/video evidence
https://github.com/Expensify/App/assets/78819774/ca52fdf6-bcf5-400d-be4a-ff4a86d269a2
Upwork Automation - Do Not Edit
- Upwork Job URL: https://www.upwork.com/jobs/~01dee105cffad79099
- Upwork Job ID: 1783269604755496960
- Last Price Increase: 2024-04-24
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.
![melvin-bot[bot] avatar](https://avatars.githubusercontent.com/in/179547?v=4 "Published by a pub.dev verified publisher"){kind=small}
@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
Job added to Upwork: https://www.upwork.com/jobs/~01dee105cffad79099
Triggered auto assignment to Contributor-plus team member for initial proposal review - @jjcoffee (External)
Cannot reproduce...
https://github.com/Expensify/App/assets/48998844/7dfd5b53-8593-4022-91da-538cc2cb85b5
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
Reproduce method is turn on slow network, like chrome network or emulator setting
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
@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!
@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
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸
@beodw @charles-liang do you have any updated proposals based on @jjcoffee's comment?
@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?
@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).
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.
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!
@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,
- enabling the loading skeleton is on ReportScreen not in ReportActionsView
https://github.com/Expensify/App/blob/c496ce653943934f6a47bae2fcf520ae08c69743/src/pages/home/ReportScreen.tsx#L715
- 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.
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸
@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!
@sakluger, @jjcoffee Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!
Triggered auto assignment to Contributor-plus team member for initial proposal review - @thesahindia (External)
Proposal
Workspace expense chat becomes blank page when go back from IOU / transaction thread.
What is the root cause of that problem?
Essentially, there are three reports involved:
-
Workspace Expense Report: This report contains an expense preview with text (e.g., displaying 8 expenses).
-
Report with Multiple Expense Request Previews: This report features several expense request previews.
-
Transaction Details Thread Report: This report is accessed from an expense preview in the second report.
Precondition: You need to open these three reports before testing this bug.
The issue occurs when a user submits an expense from the first report. The flow is as follows:
- The user submits an expense from the first report, triggering a MoneyRequest API request.
- Opening the second report by clicking on an expense preview (which includes text such as "8 expenses") triggers an OpenReport API request for the second report.
- Clicking on any expense preview within the second report opens the third report.
- Returning to the second report from the third report by clicking on the header link in the third report.
The problem arises because the OpenReport process initiated in the second step has not yet completed. This delay occurs because the OpenReport is waiting for the MoneyRequest to finish, while isLoadingInitialReportActions is set to true for the second report. Consequently, isLoading remains true due to the presence of a reportActionID in the route:
https://github.com/Expensify/App/blob/bc3009a97dfb9eba4127e182c7703564166ba4c9/src/pages/home/report/ReportActionsView.tsx#L122
Which will make the reportActions to be empty:
https://github.com/Expensify/App/blob/2a6d63d7f17b1e4fba94f26103f13c6afe294b91/src/pages/home/report/ReportActionsView.tsx#L165-L167
What changes do you think we should make in order to solve the problem?
Here:
https://github.com/Expensify/App/blob/bc3009a97dfb9eba4127e182c7703564166ba4c9/src/pages/home/report/ReportActionsView.tsx#L122
The isLoading check should be updated to:
const isLoading = !!reportActionID && isLoadingInitialReportActions && !isReadyForCommentLinking;
Change the || comparison to && because the value of isReadyForCommentLinking determines if a cached linked report action is available. Therefore, isLoading should only be true when the cached linked report action does not exist in Onyx.
The display of the skeleton is controlled in ReportScreen.tsx. Thus, changing the || to && here is appropriate.
Another solution is to include the hasMoreCached condition in isLoading. Move the declaration of isLoading below hasMoreCached and modify the line to:
const isLoading = (!!reportActionID && isLoadingInitialReportActions && !hasMoreCached) || !isReadyForCommentLinking;
The issue where the report only displays report actions from linked report actions and older ones is because
https://github.com/Expensify/App/blob/198b47360fa6fa5bfbb81f3fb535a2fa6817e375/src/pages/home/report/ReportActionsView.tsx#L345-L355
When isLoadingInitialReportActions is true, loadNewerChats will early return and not display newer chats.
To immediately display the preview of newer cached report actions, include a check there:
Modify it to (isLoadingInitialReportActions && !hasMoreCached) || to enable loading the preview of newer cached report actions.
Regarding:
I have applied the second change, but now once OpenReport completes, the report preview jumps to the bottom and all following messages/report previews are removed.
This occurs because:
https://github.com/Expensify/App/blob/60530f643b30f30510914ed28e2a90ed5d37184a/src/pages/home/report/ReportActionsView.tsx#L150-L155
We set isFirstLinkedActionRender.current = true; when isLoadingInitialReportActions changes (in this case, when OpenReport completes). Consequently, report actions are sliced from the linked report action to older messages:
https://github.com/Expensify/App/blob/60530f643b30f30510914ed28e2a90ed5d37184a/src/pages/home/report/ReportActionsView.tsx#L187-L189
Resulting in the removal of newer report actions.
To address this, we should remove the dependency on isLoadingInitialReportActions in useEffect. Alternatively, we could depend only on [reportID, reportActionID]. This constitutes the third change. I hope this resolves any issues.
@sakluger, @thesahindia Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!