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
3.98k stars 2.99k forks source link

[$250] Clicking link to invalid room > another report > then back to invalid room link leads to infinite skeleton loading screen #55085

Open lanitochka17 opened 2 weeks ago

lanitochka17 commented 2 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: 9.0.83-0 Reproducible in staging?: Y Reproducible in production?: Y If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: N/A 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): agexptest+pr0801@gmail.com Issue reported by: Applause - Internal Team Component:

Action Performed:

  1. Go to https://staging.new.expensify.com/
  2. Open any chat and paste an invalid room link (e.g. https://staging.new.expensify.com/r/2652281908731856)
  3. Click on the link
  4. Click back
  5. Click link again
  6. Observe infinite loading skeleton.

Expected Result:

The "Hmm... it's not here" page is displayed

Actual Result:

Infinite skeleton loading screen.

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/user-attachments/assets/4ccd0488-10bb-4186-ba81-9d15e30e490e

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021878923123358968660
  • Upwork Job ID: 1878923123358968660
  • Last Price Increase: 2025-01-13
  • Automatic offers:
    • daledah | Contributor | 105772493
Issue OwnerCurrent Issue Owner: @allroundexperts
melvin-bot[bot] commented 2 weeks ago

Triggered auto assignment to @mallenexpensify (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.

mohit6789 commented 2 weeks ago

Proposal

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

shows blank row when opening a invalid room link

What is the root cause of that problem?

We create the LHN option even though reportID is not valid here

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

To fix issue we can return null if reportID is not valid so empty row will not created. We can add below check here

const renderItem = useCallback(
  ({item: reportID}: RenderItemProps): ReactElement | null => {
     // Below if block added to fix the issue.
      if (reportID === '-1') {
          return null;
      }
     const itemFullReport = reports?.[`${ONYXKEYS.COLLECTION.REPORT}${reportID}`];

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

NA as this is bug

What alternative solutions did you explore? (Optional)

CyberAndrii commented 2 weeks ago

Proposal

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

LHN shows blank row when opening a invalid room link

What is the root cause of that problem?

When a report with a random ID, e.g., r/6546545, is opened, the backend returns the report.errorFields.notFound property. In this case, the report is not displayed in the LHN because we check for the report.errorFields.notFound property to be unset.

https://github.com/Expensify/App/blob/dfb43855c5703dbcceb6d036483cf6dca353c3b1/src/libs/SidebarUtils.ts#L130-L131

https://github.com/Expensify/App/blob/dfb43855c5703dbcceb6d036483cf6dca353c3b1/src/libs/SidebarUtils.ts#L135-L137

However, when opening r/2652281908731856, for some reason the BE returns report.errorFields.createChat with the value Auth OpenReport returned an error instead of report.errorFields.notFound. This scenario bypasses the above checks, causing this issue.

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

Ideally, we should investigate why the backend returns report.errorFields.createChat and whether it could instead return report.errorFields.notFound. If that is not possible, we can add a check similar to this one to verify the required properties

https://github.com/Expensify/App/blob/dfb43855c5703dbcceb6d036483cf6dca353c3b1/src/libs/SidebarUtils.ts#L120

        if (!report || !report?.reportID) {

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

Unit/UI tests to ensure that reports with missing reportID, and possibly also type and reportName properties are not displayed.

huult commented 1 week ago

Proposal

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

LHN shows blank row when opening a invalid room link

What is the root cause of that problem?

When we open the report that is not found, the back-end returns errorFields for createChat.

and store this report to Onyx

In the sidebar, we have not handled the createChat errorFields case yet, which is why this issue occurs.

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

To resovle this issue we just update case createChat error some thing like that:

https://github.com/Expensify/App/blob/b416e250cc633c0505390af6e567b0ccdeea16fd/src/libs/SidebarUtils.ts#L121

Update to

        const isReportInAccessible = report?.errorFields?.notFound ?? report?.errorFields?.createChat;
POC https://github.com/user-attachments/assets/dcbecdf4-2f13-4f11-94d7-fb8b8f7022bf

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

This is a UI bug; no tests are needed.

What alternative solutions did you explore? (Optional)

Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job.

we could fix this issue by removing the report from Onyx, or implementing a similar solution like this

https://github.com/Expensify/App/blob/42be56aa9b88f70ec6c7350d9e161cbc80e17cf1/src/pages/home/ReportScreen.tsx#L441

    useEffect(() => {
        if (!shouldShowNotFoundPage || report?.reportID || !report?.errorFields?.createChat) {
            return;
        }

        Report.deleteReport(reportIDFromRoute);
    }, [report, reportIDFromRoute, shouldShowNotFoundPage]);
melvin-bot[bot] commented 1 week ago

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

melvin-bot[bot] commented 1 week ago

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

mallenexpensify commented 1 week ago

Was able to reproduce. @allroundexperts , plz review the above proposals?

Also.. this seems like a regression (that said, I don't click broken links that often) . If anyone can point to a PR that might have caused this, please do.

truph01 commented 1 week ago

Cannot reproduce the issue in the latest main

mallenexpensify commented 1 week ago

Also unable to reproduce on latest staging - desktop and web. @allroundexperts anya thoughts before I close this?

allroundexperts commented 1 week ago

This one seems to be fixed, but upon going back and clicking the room link again, I am getting an infinitely loading page.

https://github.com/user-attachments/assets/a6046234-0a3e-4ea4-b4c3-09f732c7d3d8

mallenexpensify commented 1 week ago

Thanks @allroundexperts , good catch. Updated the OP to inc.

Matt A update - Discussing here. The steps below aren't reproducible but if you click back then click on the report again, there's an infinite skeleton loading screen.

daledah commented 1 week ago

Proposal

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

Infinite skeleton loading screen.

What is the root cause of that problem?

The shouldShowNotFoundPage variable determines whether the "not found" page should be displayed. In this issue, at step 6, the value is false instead of true. This occurs because, in step 6, the useMemo:

https://github.com/Expensify/App/blob/b0516e30e5f096c94a606a4dd2dadfb3c5f1f165/src/pages/home/ReportScreen.tsx#L394

is only calculated once, with firstRenderRef.current being true. As a result, the condition:

https://github.com/Expensify/App/blob/b0516e30e5f096c94a606a4dd2dadfb3c5f1f165/src/pages/home/ReportScreen.tsx#L411

is not met.

When firstRenderRef.current is set to false, the useMemo value is not recalculated, so shouldShowNotFoundPage remains false.

At the same time, shouldShowSkeleton is also true, leading to the infinite loading screen being displayed.

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

We can create a firstRender state beside the firstRenderRef, so when this value is updated, we can re-calculate the above useMemo value: const [firstRender, setFirstRender] = useState(true).

Then in here call setFirstRender(false).

Finally, add the firstRender to the shouldShowNotFoundPage's dependencies.

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

NA

What alternative solutions did you explore? (Optional)

NA

mallenexpensify commented 1 week ago

@allroundexperts can you review @daledah 's proposal above? I updated the title and OP

[$250] Clicking link to invalid room > another report > then back to invalid room link leads to infinite skeleton loading screen

Action Performed:

  1. Go to https://staging.new.expensify.com/
  2. Open any chat and paste an invalid room link (e.g. https://staging.new.expensify.com/r/2652281908731856)
  3. Click on the link
  4. Click back
  5. Click link again
  6. Observe infinite loading skeleton.

Expected Result:

The "Hmm... it's not here" page is displayed

Actual Result:

Infinite skeleton loading screen.

allroundexperts commented 1 week ago

@daledah's proposal looks good to me. The RCA is correct and the solution works as well.

🎀 👀 🎀 C+ reviewed

melvin-bot[bot] commented 1 week ago

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

melvin-bot[bot] commented 6 days ago

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

melvin-bot[bot] commented 5 days ago

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