-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[$500] Chat-Infinite loading occurs if conversation ID is edited to 0 #32651
Comments
Job added to Upwork: https://www.upwork.com/jobs/~015ee46823d25664a9 |
Triggered auto assignment to @sophiepintoraetz ( |
Bug0 Triage Checklist (Main S/O)
|
Triggered auto assignment to Contributor-plus team member for initial proposal review - @alitoshmatov ( |
ProposalPlease re-state the problem that we are trying to solve in this issue.Infinite loading occurs if conversation ID is edited to 0 What is the root cause of that problem?When the App/src/pages/home/ReportScreen.js Line 243 in b0268fa
What changes do you think we should make in order to solve the problem?We should check if
And then update the condition to show not found page with this variable.
What alternative solutions did you explore? (Optional)NA ResultScreen.Recording.2023-12-14.at.12.47.00.movScreen.Recording.2023-12-14.at.12.47.48.mov |
ProposalPlease re-state the problem that we are trying to solve in this issue.Infinite loading occurs occurs if reportID = What is the root cause of that problem?We validate App/src/pages/home/ReportScreen.js Lines 105 to 109 in b0268fa
So isLoadingInitialReportActions is true and it doesn't meet shouldShowNotFoundPage condition
What changes do you think we should make in order to solve the problem?
so call this function:
|
ProposalPlease re-state the problem that we are trying to solve in this issue.Infinite loading occurs occurs if reportID = 0 What is the root cause of that problem?We didn't consider What changes do you think we should make in order to solve the problem?We need to consider
What alternative solutions did you explore? (Optional)Screen.Recording.2023-12-07.at.2.59.55.PM.mov |
ProposalPlease re-state the problem that we are trying to solve in this issue.When visiting a report with id of 0, the loading shows infinitely. What is the root cause of that problem?We currently prevent the open report API if the report id is 0, empty, or "null" string. This makes the report screen always show the skeleton loader. App/src/pages/home/ReportScreen.js Lines 241 to 245 in 8fa65ec
Lines 3915 to 3917 in 8fa65ec
This is added to fix this issue where the app crashes if we send a "null" string as the report ID to the API, but I found it weird why it only happens with a "null" string and not other string such as "a". So, I'm trying to reproduce the issue without the fix above and it works fine (it's probably a temp BE issue). So, currently, if we visit a report with an ID of 0 or null, the loading shows indefinitely, but visiting a report with an ID of other characters such as "a", the not found page will show, so it's a bit inconsistent with how we handle the report ID. What changes do you think we should make in order to solve the problem?I would suggest removing
What alternative solutions did you explore? (Optional)Show the not found page if the report id is not empty and is invalid.
|
@sophiepintoraetz, @alitoshmatov Uh oh! This issue is overdue by 2 days. Don't forget to update your issues! |
Screen.Recording.2023-12-11.at.20.41.27.mov@dukenv0307 @yh-0218 Your solution causes this regression, also explained by this comment App/src/pages/home/ReportScreen.js Lines 241 to 242 in 5681090
@mkhutornyi Can you explain you solution in more details, test it and provide screen recording. I am assuming your solution also results in this, correct me if I am wrong |
@bernhardoj Can you explain why reverting part of the solution of #28504 is not causing a regression. Also I think sending openReport requests for invalid reportIDs is unnecessary action and we should avoid it. |
ProposalPlease re-state the problem that we are trying to solve in this issue.Chat-Infinite loading occurs if conversation ID is edited to 0 What is the root cause of that problem?We are early returning when reportID is not valid: App/src/pages/home/ReportScreen.js Line 243 in b0268fa
which is preventing openReport from being called and setting isLoadingInitialReportActions to false. What changes do you think we should make in order to solve the problem?We should show not found page when reportID is not valid. But when user logs in the reportID won't be present and will be defaulted to 0 which makes reportID not valid thus showing not found page. We should do 2 things
What alternative solutions did you explore? (Optional) |
@alitoshmatov #28504 solution is to fix the crash and I tested after reverting the If we look at the issue recordings, the crash only happens when we press the Send Message button where
Yes, it's unnecessary, but currently, we only do it for "null" string and "0". We should have a regex rule for 100% invalid report ID which sounds like a new feature. |
Right now we are not sending any request for reportIDs which are invalid based on |
@alitoshmatov okay, got it. Added an alternative solution to my proposal |
@alitoshmatov My proposal was updated. |
|
Triggered auto assignment to @neil-marcellini, see https://stackoverflow.com/c/expensify/questions/7972 for more details. |
@bernhardoj @yh-0218 Your solution still causes regression I mentioned earlier |
PR is ready cc: @alitoshmatov |
@neil-marcellini Sorry for my miscommunication, I was going to say alternative solution of @bernhardoj 's proposal . As I mentioned here the main solution causes request being made to invalid reportIDs. Can you have another look at the alternative solution |
@neil-marcellini Friendly bump |
@neil-marcellini hi, can you check @alitoshmatov comment above? |
@neil-marcellini gentle bump |
Let me summarize our progress where we left:
I approved @bernhardoj alternative proposal which states:
Which solves all the issues and straightforward. |
@neil-marcellini hi, can you check @alitoshmatov comment above? |
Triggered auto assignment to @abekkala ( |
@abekkala Since @neil-marcellini is not responding, can we get another internal engineer to take a look at this? |
I've reached out to @neil-marcellini |
Sorry for not seeing this earlier, and thanks April for DMing me on NewDot. That's the best way to reach me. I haven't been checking my GH mentions since there are too many, however, I will start checking them daily so I don't miss things like this again.
@alitoshmatov Ok I see, thanks for explaining. I'm happy with this, and I approve the alternate solution from @bernhardoj's proposal. Please continue with the PR and request my review after the C+ approves. |
@abekkala, @neil-marcellini, @bernhardoj, @alitoshmatov Whoops! This issue is 2 days overdue. Let's get this updated quick! |
@bernhardoj do you have a PR up for this? |
Yes, it was merged yesterday #33394 |
deployed to prod Feb 14 |
@abekkala, @neil-marcellini, @bernhardoj, @alitoshmatov Whoops! This issue is 2 days overdue. Let's get this updated quick! |
7 day waiting period end has ended - no regressions Fix: @bernhardoj [$500] Offer link |
@bernhardoj and @alitoshmatov Upwork payments sent and contracts ended - thank you! 🎉 |
If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results. 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. |
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.9-0
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
Expensify/Expensify Issue URL:
Issue reported by: Applause-Internal Team
Slack conversation: @
Action Performed:
Expected Result:
"You don't have access to this chat" message in the conversation history is shown
Actual Result:
Infinite skeleton loader occurs if conversation ID is edited to 0 in the URL
Note: The issue reproduces for only "0" value
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Bug6304320_1701946093585.2023-12-07_11-02-22.mp4
View all open jobs on GitHub
Upwork Automation - Do Not Edit
The text was updated successfully, but these errors were encountered: