[HOLD for payment 2024-04-05] [$500] Workspace - #admin room with messages shows skeleton loading after workspace is deleted

[kbecciv]

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.56-0
Reproducible in staging?: y
Reproducible in production?: n
Issue reported by: Applause - Internal Team

Action Performed:

1. Go to staging.new.expensify.com.
2. Create a new workspace.
3. Go to #admin room.
4. Send a message.
5. Delete the workspace.
6. Go to #admin room.

Expected Result:

#admin room will not show skeleton loading after the workspace is deleted.

Actual Result:

#admin room shows skeleton loading after the workspace is deleted.

Workaround:

n/a

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

Bug6423448_1711129084965.20240323_013502.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0190baa2ce2890e3e7
• Upwork Job ID: 1772371688906227712
• Last Price Increase: 2024-03-25
• Automatic offers:
• ishpaul777 | Contributor | 0

[kbecciv]

We think that this bug might be related to #vip-vsb

[github-actions]

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

1. Identify the pull request that introduced this issue and revert it.
2. Find someone who can quickly fix the issue.
3. Fix the issue yourself.

[melvin-bot]

Triggered auto assignment to @aldo-expensify (Engineering), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

[aldo-expensify]

The GetOlderActions command is erroring out, sample logs: 86894ae17a5e2d72-YVR

[aldo-expensify]

Seems like the functions loadOlderChats and loadNewerChats gets called in a infinite loop:

The InvertedFlatList is calling them here:

App/src/pages/home/report/ReportActionsList.tsx

Lines 610 to 613 in 81787f5

	onEndReached={loadOlderChats}
	onEndReachedThreshold={0.75}
	onStartReached={loadNewerChats}
	onStartReachedThreshold={0.75}

[aldo-expensify]

Adding external and bug to see if we can get a contributor to help here

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[mallenexpensify]

@aldo-expensify , does this need to be a deploy blocker? I have a feeling it might be related to comment linking, multiple bugs have been reported, like here https://expensify.slack.com/archives/C049HHMV9SM/p1711147499521539

cc @roryabraham , you likely have a better idea if it's related.

[aldo-expensify]

@aldo-expensify , does this need to be a deploy blocker?

I think so, it is causing a regression that doesn't allow opening the admin room.

[abzokhattab]

Proposal

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

The app keeps calling the loading skeleton after a workspace is deleted

What is the root cause of that problem?

the reportActions returned from the getContinuousReportActionChain always have fewer actions after deleting the workspace

this happens because the optimistic value of the delete workspace action doesn't have a previousReportActionID so the conditions here fails after one iteration

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

we can modify the endIndex loop so that it skips if the current action is close

    while (
        (endIndex < sortedReportActions.length - 1 && sortedReportActions[endIndex].previousReportActionID === sortedReportActions[endIndex + 1].reportActionID) ||
        !!sortedReportActions[endIndex + 1]?.whisperedToAccountIDs?.length ||
        !!sortedReportActions[endIndex]?.whisperedToAccountIDs?.length ||
        sortedReportActions[endIndex]?.actionName === CONST.REPORT.ACTIONS.TYPE.ROOMCHANGELOG.INVITE_TO_ROOM ||
        sortedReportActions[endIndex]?.actionName === CONST.REPORT.ACTIONS.TYPE.CLOSED ||
        sortedReportActions[endIndex + 1]?.actionName === CONST.REPORT.ACTIONS.TYPE.CLOSED ||
        sortedReportActions[endIndex + 1]?.actionName === CONST.REPORT.ACTIONS.TYPE.CREATED
    ) {

POC:

Screen.Recording.2024-03-26.at.6.40.04.AM.mov

alternatively:

or we can pass a previousReportActionID to the buildOptimisticClosedReportAction and set it to the last action id so that the first condition passes for the optimistic action.

[sobitneupane]

Thanks for the proposal @abzokhattab

There are two Closed action which looks like the reason for the issue. Why do we have two Closed Actions in the first place? One of them don't have previousReportActionID which probably is the one optimistically added.

This PR might have caused the regression.

Does reverting the PR and adding previousReportActionID optimistically solves both this issue and #35174 issue?

[aldo-expensify]

@roryabraham @perunt when you have some time, can you check if this solution makes sense to you? 🙏

[roryabraham]

PR is being CP'd presently

[mallenexpensify]

This hit production two days ago 🎉
Gonna close this, doesn't look like @sobitneupane reviewed the PR. If I'm wrong, please comment, reopen.

[aldo-expensify]

Created issue to handle the follow up work for this: https://github.com/Expensify/Expensify/issues/383435

[aldo-expensify]

@mallenexpensify : @ishpaul777 ended up reviewing

[melvin-bot]

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

[ishpaul777]

hmm, this doesn't require a payment i was c+ on the offending PR

[aldo-expensify]

ohh thanks for clearing that up!

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.57-5 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Remove optimistic CLOSED actions on success #39008

If no regressions arise, payment will be issued on 2024-04-05. 🎊

For reference, here are some details about the assignees on this issue:

• @ishpaul777 requires payment automatic offer (Contributor)

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@aldo-expensify] The PR that introduced the bug has been identified. Link to the PR: HIGH: (Comment linking: Step 4) Main "glue" PR for Comment Linking #30269
• [@aldo-expensify] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: HIGH: (Comment linking: Step 4) Main "glue" PR for Comment Linking #30269 (comment)
• [@aldo-expensify] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: No need, the linked PR was huge and caused many deploy blockers
• [@aldo-expensify] Determine if we should create a regression test for this bug: I think this is already caught by existing regression tests
• [@aldo-expensify] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again. N/a
• [@mallenexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon: N/a