[$500] Chat-Infinite loading occurs if conversation ID is edited to 0 #32651

izarutskaya · 2023-12-07T11:34:27Z

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.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:

Login to ND Expensify
On the URL change the conversation ID to 0 (https://staging.new.expensify.com/r/0)
Verify you briefly see the skeleton loader
Verify you see a "You don't have access to this chat" message in the conversation history

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

Upwork Job URL: https://www.upwork.com/jobs/~015ee46823d25664a9
Upwork Job ID: 1732725262675222528
Last Price Increase: 2023-12-07
Automatic offers:

alitoshmatov | Reviewer | 28066316
bernhardoj | Contributor | 28066317

melvin-bot · 2023-12-07T11:34:31Z

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

melvin-bot · 2023-12-07T11:34:32Z

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

melvin-bot · 2023-12-07T11:34:34Z

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

melvin-bot · 2023-12-07T11:34:36Z

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

dukenv0307 · 2023-12-07T11:53:55Z

Proposal

Please 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 reportID is 0, we don't call openReport API and isLoadingInitialReportActions is true by default prop. So infinite loading occurs

App/src/pages/home/ReportScreen.js

Line 243 in b0268fa

if (!ReportUtils.isValidReportIDFromPath(reportIDFromPath)) {

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

We should check if isLoadingInitialReportActions but reportID is invalid we will not show loading page. Instead, we will show not found page by create a variable like isValidReportIDFromPath

isValidReportIDFromPath = ReportUtils.isValidReportIDFromPath(reportIDFromPath)
// If has no reportID in param, it means we are setting reportID in `ReportScreenIDSetter`
const hasReportIDFromParam = !!route.params.reportID;

And then update the condition to show not found page with this variable.

const shouldShowNotFoundPage = useMemo(
    () => (!firstRenderRef.current && ((!report.reportID && !isOptimisticDelete && !reportMetadata.isLoadingInitialReportActions && !isLoading && !userLeavingStatus) || (!isValidReportIDFromPath && hasReportIDFromParam))) || shouldHideReport,
    [report, reportMetadata, isLoading, shouldHideReport, isOptimisticDelete, userLeavingStatus],
);

What alternative solutions did you explore? (Optional)

NA

Result

Screen.Recording.2023-12-14.at.12.47.00.mov

Screen.Recording.2023-12-14.at.12.47.48.mov

mkhutornyi · 2023-12-07T12:02:53Z

Proposal

Please 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 validate reportID here and early return if invalid before calling OpenReport which sets isLoadingInitialReportActions to false optimistically.
So reportMetadata_0 is not set in Onyx and it fallback to this default object:

App/src/pages/home/ReportScreen.js

Lines 105 to 109 in b0268fa

    
           reportMetadata: { 
        
               isLoadingInitialReportActions: true, 
        
               isLoadingOlderReportActions: false, 
        
               isLoadingNewerReportActions: false, 
        
           },

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?

define new flag. Let's say X!
X = ReportUtils.isValidReportIDFromPath(reportID) && reportMetadata.isLoadingInitialReportActions
And replace all reportMetadata.isLoadingInitialReportActions occurrences with X in ReportScreen.js component

(optional) For existing users who already have stale reportMetadata_reportID in Onyx:
clear reportMetadata_reportID before early return here:

App/src/pages/home/ReportScreen.js

Lines 243 to 245 in b0268fa

    
           if (!ReportUtils.isValidReportIDFromPath(reportIDFromPath)) { 
        
               return; 
        
           }

so call this function:

function clearReportMetadata(reportID: string) {
    Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_METADATA}${reportID}`, null);
}

yh-0218 · 2023-12-07T13:01:44Z

Proposal

Please 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 isValidReportIDFromPath to get shouldShowNotFoundPage

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

We need to consider isValidReportIDFromPath to get shouldShowNotFoundPage

const shouldShowNotFoundPage = useMemo(
        () => {
            const reportIDFromPath = getReportID(route);
            if (['0'].includes(reportIDFromPath)) {
                return true;
            }
            return (!firstRenderRef.current && !report.reportID && !isOptimisticDelete && !reportMetadata.isLoadingInitialReportActions && !isLoading && !userLeavingStatus) || shouldHideReport;
        },
        [route, report.reportID, isOptimisticDelete, reportMetadata.isLoadingInitialReportActions, isLoading, userLeavingStatus, shouldHideReport],
    );

What alternative solutions did you explore? (Optional)

Screen.Recording.2023-12-07.at.2.59.55.PM.mov

bernhardoj · 2023-12-07T17:02:32Z

Proposal

Please 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

    
           // Report ID will be empty when the reports collection is empty. 
        
           // This could happen when we are loading the collection for the first time after logging in. 
        
           if (!ReportUtils.isValidReportIDFromPath(reportIDFromPath)) { 
        
               return; 
        
           }

App/src/libs/ReportUtils.ts

Lines 3915 to 3917 in 8fa65ec

    
           function isValidReportIDFromPath(reportIDFromPath: string): boolean { 
        
               return !['', 'null', '0'].includes(reportIDFromPath); 
        
           }

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 isValidReportIDFromPath. Let the BE handles any report ID we sent and the not found page will be shown if it's not found in the BE.

if (!reportIDFromPath) {
    return;
}

What alternative solutions did you explore? (Optional)

Show the not found page if the report id is not empty and is invalid.

// add to shouldShowNotFoundPage condition
const reportIDParams = lodashGet(route.params, 'reportID');
|| (reportIDParams && !ReportUtils.isValidReportIDFromPath(reportIDParams))

melvin-bot · 2023-12-11T11:27:23Z

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

alitoshmatov · 2023-12-11T15:44:19Z

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

    
           // Report ID will be empty when the reports collection is empty. 
        
           // This could happen when we are loading the collection for the first time after logging in.

@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

alitoshmatov · 2023-12-11T16:23:51Z

@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.

dostongulmatov · 2023-12-11T16:41:56Z

Proposal

Please 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

if (!ReportUtils.isValidReportIDFromPath(reportIDFromPath)) {

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

Change default value of reportID when it doesn't exist to ''(empty string)
Add additional condition to shouldShowNotFoundPage
reportIDFromPath && !ReportUtils.isValidReportIDFromPath(reportIDFromPath)
so that when reportID exists and it is invalid we make shouldShowNotFoundPage true

What alternative solutions did you explore? (Optional)

bernhardoj · 2023-12-11T17:10:49Z

@alitoshmatov #28504 solution is to fix the crash and I tested after reverting the isValidReportIDFromPath usage it still works fine.

If we look at the issue recordings, the crash only happens when we press the Send Message button where isReportDataReady is trying to access an undefined report. So, I'm guessing that preventing sending an OpenReport request with a "null" string report ID is not related to the issue. BE can handle invalid report IDs, except the BE is temporarily broken at that time.

Also I think sending openReport requests for invalid reportIDs is unnecessary action and we should avoid it.

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.

alitoshmatov · 2023-12-11T19:59:29Z

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 isValidReportIDFromPath function. I mean, let's not break this existing logic just for the sake of this solving this issue.

bernhardoj · 2023-12-12T03:32:03Z

@alitoshmatov okay, got it. Added an alternative solution to my proposal

yh-0218 · 2023-12-12T11:56:59Z

@alitoshmatov My proposal was updated.

alitoshmatov · 2023-12-13T17:32:21Z

~~@dostongulmatov Your solution makes sense, I agree that when reportID exists and it is invalid we should show not~~ ~~found page. Your fixed what other proposals were lacking with changing default reportID to '' when it does not exist.~~

~~We can go with @dostongulmatov 's proposal~~
~~C+ reviewed 🎀 👀 🎀~~

melvin-bot · 2023-12-13T17:32:28Z

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

alitoshmatov · 2023-12-13T17:33:53Z

@bernhardoj @yh-0218 Your solution still causes regression I mentioned earlier

bernhardoj · 2023-12-21T06:20:30Z

PR is ready

cc: @alitoshmatov

alitoshmatov · 2023-12-21T07:07:38Z

@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

alitoshmatov · 2024-01-02T11:09:11Z

@neil-marcellini Friendly bump

bernhardoj · 2024-01-16T16:38:57Z

@neil-marcellini hi, can you check @alitoshmatov comment above?

bernhardoj · 2024-01-22T11:49:39Z

@neil-marcellini gentle bump

alitoshmatov · 2024-01-24T12:51:27Z

Let me summarize our progress where we left:
This issue is taking a long time due to my miscommunication here. We are waiting for @neil-marcellini confirmation on the following:
@bernhardoj main solution is not suitable for our case for the following reason:

I think sending openReport requests for invalid reportIDs is unnecessary action and we should avoid it.

I approved @bernhardoj alternative proposal which states:

Show the not found page if the report id is not empty and is invalid.

Which solves all the issues and straightforward.
This issue is extremely rare since user won't update url manually and change reportID to 0. That is why I wanted to confirm everything and solve this issue correctly even if it takes longer time

cc: @bernhardoj @neil-marcellini

bernhardoj · 2024-02-01T16:15:31Z

@neil-marcellini hi, can you check @alitoshmatov comment above?

melvin-bot · 2024-02-01T23:37:23Z

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

alitoshmatov · 2024-02-05T15:54:18Z

@abekkala Since @neil-marcellini is not responding, can we get another internal engineer to take a look at this?

abekkala · 2024-02-05T20:04:51Z

I've reached out to @neil-marcellini

neil-marcellini · 2024-02-05T20:31:00Z

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.

Show the not found page if the report id is not empty and is invalid.

@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.

melvin-bot · 2024-02-13T15:06:53Z

@abekkala, @neil-marcellini, @bernhardoj, @alitoshmatov Whoops! This issue is 2 days overdue. Let's get this updated quick!

abekkala · 2024-02-13T19:01:08Z

@bernhardoj do you have a PR up for this?

bernhardoj · 2024-02-13T19:02:13Z

Yes, it was merged yesterday #33394

abekkala · 2024-02-15T18:12:11Z

deployed to prod Feb 14

melvin-bot · 2024-02-23T15:03:18Z

@abekkala, @neil-marcellini, @bernhardoj, @alitoshmatov Whoops! This issue is 2 days overdue. Let's get this updated quick!

abekkala · 2024-02-23T15:56:53Z

7 day waiting period end has ended - no regressions

Fix: @bernhardoj [$500] Offer link
PR Review: @alitoshmatov [$500] Offer link

abekkala · 2024-02-23T15:59:26Z

@bernhardoj and @alitoshmatov Upwork payments sent and contracts ended - thank you! 🎉

melvin-bot · 2024-03-25T09:55:56Z

⚠️ Looks like this issue was linked to a Deploy Blocker here

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.

izarutskaya added External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Dec 7, 2023

melvin-bot bot changed the title ~~Chat-Infinite loading occurs if conversation ID is edited to 0~~ [$500] Chat-Infinite loading occurs if conversation ID is edited to 0 Dec 7, 2023

melvin-bot bot assigned sophiepintoraetz Dec 7, 2023

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

melvin-bot bot assigned alitoshmatov Dec 7, 2023

yh-0218 mentioned this issue Dec 8, 2023

[HOLD for payment 2023-12-29] [$500] Profile - Page not found opens after some delay #32750

Closed

6 tasks

melvin-bot bot added the Overdue label Dec 11, 2023

melvin-bot bot removed the Overdue label Dec 11, 2023

melvin-bot bot assigned neil-marcellini Dec 13, 2023

strepanier03 unassigned sophiepintoraetz Feb 1, 2024

strepanier03 added Bug Something is broken. Auto assigns a BugZero manager. and removed Bug Something is broken. Auto assigns a BugZero manager. labels Feb 1, 2024

melvin-bot bot assigned abekkala Feb 1, 2024

melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Feb 1, 2024

abekkala closed this as completed Feb 23, 2024

nkdengineer mentioned this issue Mar 25, 2024

[HOLD for payment 2024-04-05] Sign up - Hmmm its not here page is displayed for a moment on sign up #38826

Closed

6 tasks

ishpaul777 mentioned this issue Mar 25, 2024

fixes "Skeleton is displayed when navigating to a transaction thread created offline" #38955

Merged

50 tasks

melvin-bot bot added Weekly KSv2 and removed Daily KSv2 labels Mar 25, 2024

[$500] Chat-Infinite loading occurs if conversation ID is edited to 0 #32651

[$500] Chat-Infinite loading occurs if conversation ID is edited to 0 #32651

Comments

izarutskaya commented Dec 7, 2023 • edited by melvin-bot bot Loading

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

Screenshots/Videos

melvin-bot bot commented Dec 7, 2023

melvin-bot bot commented Dec 7, 2023

melvin-bot bot commented Dec 7, 2023

Bug0 Triage Checklist (Main S/O)

melvin-bot bot commented Dec 7, 2023

dukenv0307 commented Dec 7, 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?

What alternative solutions did you explore? (Optional)

Result

mkhutornyi commented Dec 7, 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?

yh-0218 commented Dec 7, 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?

What alternative solutions did you explore? (Optional)

bernhardoj commented Dec 7, 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?

What alternative solutions did you explore? (Optional)

melvin-bot bot commented Dec 11, 2023

alitoshmatov commented Dec 11, 2023

alitoshmatov commented Dec 11, 2023

dostongulmatov commented Dec 11, 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?

What alternative solutions did you explore? (Optional)

bernhardoj commented Dec 11, 2023

alitoshmatov commented Dec 11, 2023 • edited Loading

bernhardoj commented Dec 12, 2023

yh-0218 commented Dec 12, 2023

alitoshmatov commented Dec 13, 2023 • edited Loading

melvin-bot bot commented Dec 13, 2023

alitoshmatov commented Dec 13, 2023

bernhardoj commented Dec 21, 2023

alitoshmatov commented Dec 21, 2023

alitoshmatov commented Jan 2, 2024

bernhardoj commented Jan 16, 2024

bernhardoj commented Jan 22, 2024

alitoshmatov commented Jan 24, 2024

bernhardoj commented Feb 1, 2024

melvin-bot bot commented Feb 1, 2024

alitoshmatov commented Feb 5, 2024

abekkala commented Feb 5, 2024

neil-marcellini commented Feb 5, 2024

melvin-bot bot commented Feb 13, 2024

abekkala commented Feb 13, 2024

bernhardoj commented Feb 13, 2024

abekkala commented Feb 15, 2024

melvin-bot bot commented Feb 23, 2024

abekkala commented Feb 23, 2024 • edited Loading

abekkala commented Feb 23, 2024

melvin-bot bot commented Mar 25, 2024

izarutskaya commented Dec 7, 2023 •

edited by melvin-bot bot

Loading

dukenv0307 commented Dec 7, 2023 •

edited

Loading

mkhutornyi commented Dec 7, 2023 •

edited

Loading

yh-0218 commented Dec 7, 2023 •

edited

Loading

bernhardoj commented Dec 7, 2023 •

edited

Loading

alitoshmatov commented Dec 11, 2023 •

edited

Loading

alitoshmatov commented Dec 13, 2023 •

edited

Loading

abekkala commented Feb 23, 2024 •

edited

Loading