[$500] Workspace - Infinite skeleton loading when member is removed from Workspace

[lanitochka17]

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.36-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): [email protected]
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:

Action Performed:

1. User A creates Workspace> Members> Invite User B
2. Login as User B and navigate to Announce room
3. User A removes User B

Expected Result:

Announce room should be archived

Actual Result:

Infinite skeleton loading when member is removed from Workspace

Workaround:

Unknown

Platforms:

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

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6365844_1706961916414.Gravar__2034.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01562b797682c4ff91
• Upwork Job ID: 1753799520257306624
• Last Price Increase: 2024-02-10
• Automatic offers:
• aimane-chnaif | Contributor | 0

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[lanitochka17]

We think that this bug might be related to #wave6
CC @greg-schroeder

[Amoralchik]

Proposal

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

The issue at hand revolves around the persistence of wasReportAccessibleRef.current as true despite the modification of the report's state.

What is the root cause of that problem?

The core problem stems from the oversight of not updating wasReportAccessibleRef.current to false when the specified condition indicates that the report is inaccessible.

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

To resolve this issue, I propose implementing a correction in the code. Specifically, by introducing the following adjustment in the useEffect block:

useEffect(() => {
    if (!report || !report.reportID || shouldHideReport) {
        wasReportAccessibleRef.current = false;
        return;
    }
    wasReportAccessibleRef.current = true;
}, [shouldHideReport, report]);

This change ensures that wasReportAccessibleRef.current accurately reflects the accessibility state based on the specified conditions.

What alternative solutions did you explore? (Optional)

[dukenv0307]

Proposal

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

• Workspace - Infinite skeleton loading when member is removed from Workspace

What is the root cause of that problem?

• After removing user B, in user B device, it will set the current report to null so the isReportReadyForDisplay is false.

• In ReportScreen, we display the "loading indicator" because the isReportReadyForDisplay is false and shouldShowNotFoundPage is false (If shouldShowNotFoundPage is true, it will display the FullPageNotFoundView rather than "loading indicator":

• Additionally, shouldShowNotFoundPage is false because wasReportAccessibleRef is set to true in the below:
  

  App/src/pages/home/ReportScreen.js

  Lines 282 to 287 in 0ae07be

  	useEffect(() => {
  	if (!report || !report.reportID || shouldHideReport) {
  	return;
  	}
  	wasReportAccessibleRef.current = true;
  	}, [shouldHideReport, report]);

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

• We can display the FullPageNotFoundView in this case. Do it by creating a ref called wasPolicyAnnounceAccessibleButNotNowRef = useRef(false) (This is used to track if the report that has report.chatType === "policyAnnounce" was accessible but now is not) and update it to true in the below logic:

 const prevPolicy = usePrevious(policy);
 const accessibleReport = useRef();
 useEffect(() => {
        if (accessibleReport.current && accessibleReport.current.chatType === CONST.REPORT.CHAT_TYPE.POLICY_ANNOUNCE && wasReportAccessibleRef.current && !wasPolicyAnnounceAccessibleButNotNowRef.current) {
            if (isEmpty(policy) && !isEmpty(prevPolicy)) {
                wasPolicyAnnounceAccessibleButNotNowRef.current = true;
            }
        }
    }, [prevReport.reportID, wasReportAccessibleRef, wasPolicyAnnounceAccessibleButNotNowRef, policy, prevPolicy]);

• Then, update the shouldShowNotFoundPage to:

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

• Beside policyAnnounce, we can do the same with other chat type

What alternative solutions did you explore? (Optional)

• NA

[dylanexpensify]

@parasharrajat can you review proposals?

[dylanexpensify]

@zanyrenney curious what you think of this for wave8?

[parasharrajat]

@Amoralchik Could you please explain your proposal?

[Amoralchik]

@Amoralchik Could you please explain your proposal?

Okay, I have reconsidered my decision.

The primary issue lies in the fact that wasReportAccessibleRef.current still remains true, and its value does not change despite the fact that the report's state has been modified.

Previously, we did not set the value to false in the case where the condition if (!report || !report.reportID || shouldHideReport) returned true. Realizing this, I understood that we could fix this error in a more convenient and understandable way by leaving the implementation of shouldShowNotFoundPage untouched. To correct the incorrect state of wasReportAccessibleRef, we simply need to set wasReportAccessibleRef.current = false when it passes the check, like this:

useEffect(() => {
    if (!report || !report.reportID || shouldHideReport) {
        wasReportAccessibleRef.current = false;
        return;
    }
    wasReportAccessibleRef.current = true;
}, [shouldHideReport, report]);

Now, this adjustment ensures that the wasReportAccessibleRef state is correctly updated based on the conditions, addressing the issue you identified.

[dukenv0307]

@parasharrajat I just updated my proposal

[dylanexpensify]

@parasharrajat can you re-review?

[aimane-chnaif]

Taking this over

[parasharrajat]

@aimane-chnaif will be helping here as C+ while I am unavailable.

[melvin-bot]

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

[aimane-chnaif]

Announce room should be archived

The expected result is wrong. It should be not found page.

@dylanexpensify can you please check? And assign me here.

[aimane-chnaif]

@dukenv0307 I am confused about your RCA and solution which made things unnecessarily complex. You meant to show loading indicator instead of not found page?

[aimane-chnaif]

I agree with @Amoralchik's proposal. The RCA is correct and solution just fixes that root cause.

[dukenv0307]

You meant to show loading indicator instead of not found page?

@aimane-chnaif No. I mean that we should show not found page in this case

[dukenv0307]

@aimane-chnaif The solution #35751 (comment) need to make sure that it does not lead to the regression here

[aimane-chnaif]

Thanks. I already tested various cases with that solution but not found issue.
@Amoralchik please confirm that solution doesn't cause such regressions like "not found page briefly shows".

[dylanexpensify]

NICE!

[dylanexpensify]

Woot! Merged!

[Amoralchik]

The pull request has been reviewed, merged, and deployed. However, it is still labeled as "Reviewing." Is this correct?

[Amoralchik]

The pull request has been reviewed, merged, and deployed. However, it is still labeled as "Reviewing." Is this correct?

mention @dylanexpensify @MonilBhavsar

[aimane-chnaif]

PR was deployed to production on Feb 26

[dylanexpensify]

ah apologies, I've been out sick, but yes agree will work on payout

[dylanexpensify]

Payment summary:

• Contributor: @Amoralchik $500
• Contributor+: @aimane-chnaif $500

Please apply or request!

[aimane-chnaif]

Accepted this offer already

[Amoralchik]

I already applied to the Upwork job

[melvin-bot]

@Amoralchik, @MonilBhavsar, @dylanexpensify, @aimane-chnaif Eep! 4 days overdue now. Issues have feelings too...

[Amoralchik]

@dylanexpensify
Hey, I noticed the Upwork job posting is no longer available and my application wasn't accepted. Perhaps I should have asked about an offer? I also wanted to clarify that I meant to say that I had "submitted a proposal" for the job. I apologize if my previous message caused any confusion or misunderstanding.

[dylanexpensify]

Ahh I see! Let me get a new link @Amoralchik!

[dylanexpensify]

@Amoralchik sent offer again!

[dylanexpensify]

Done!

[Amoralchik]

@dylanexpensify Thank you