[HOLD for payment 2024-04-03] [$500] Web - Thread - Console error ERR_FAILED 404 shows up when opening thread

[kbecciv]

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.51-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. Go to any chat.
3. Send a message.
4. Right click on the message > Reply in thread.

Expected Result:

No console error will show up.

Actual Result:

Console error ERR_FAILED 404 shows up.

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

Bug6411473_1710273044161.20240313_034855.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01bff0ded70edc9c84
• Upwork Job ID: 1768311805142413312
• Last Price Increase: 2024-03-14
• Automatic offers:
• Pujan92 | Reviewer | 0
• tienifr | Contributor | 0

[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 @luacmartins (Engineering), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

[kbecciv]

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

[luacmartins]

I'm gonna demote this to NAB. Nothing seems broken, we make a 2nd request to AuthenticatePusher that succeed. We reverted this PR that touched this logic around this subscribe logic that might have caused this.

[melvin-bot]

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

[dylanexpensify]

Reviewing today!

[luacmartins]

Found the issue in the logs. It seems like we're trying to subscribe to the report channel before the report is created, so there's a race condition somewhere.

[melvin-bot]

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

[melvin-bot]

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

[tienifr]

Proposal

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

Console error ERR_FAILED 404 shows up.

What is the root cause of that problem?

In here and here, we're not checking that the report is actually the current report in the route. When we navigate to a new page, the report could be outdated/not yet initialized.

Let's say we're on report 111, when we create a new report and navigate to report 222 the reportID is now 222 and the report there are still in pending creation. But the report here is actually still an empty report/or the 111 report because the 222 report wasn't loaded yet. So didCreateReportSuccessfully is true and we do the subscribing on the 222 report, which leads to 404 error because the 222 report wasn't loaded yet.

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

In the above places here and here, check that report.reportID === reportID before subscribing to pusher channel. If they are not equal, that means the report was not yet loaded/report was outdated and we should early return.

if (reportID !== report.reportID) {
    return;
}

Also in here, we should have report.reportID as dependency too.

While investigating, I also found there's another bug where in here, we don't use .current on the ref, so !didSubscribeToReportLeavingEvents will always evaluate to false and we'll always be unsubscribing the LeavingRoomReportChannel even though in some cases we never subscribed to it. To fix it we can just use .current on the ref correctly.

For the report typing subscription here, we never unsubscribes, we might want to add unsubscription logic similar to for the leaving room event here.

What alternative solutions did you explore? (Optional)

NA

[bernhardoj]

Proposal

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

We have authenticate pusher console error when opening a thread.

What is the root cause of that problem?

We try to subscribe to the pusher before the report is created. We currently have 2 subscriptions, one in the report screen and one in the report actions view.

App/src/pages/home/ReportScreen.tsx

Lines 437 to 441 in 00315b1

	const didCreateReportSuccessfully = !report.pendingFields || (!report.pendingFields.addWorkspaceRoom && !report.pendingFields.createChat);
	if (!didSubscribeToReportLeavingEvents.current && didCreateReportSuccessfully) {
	Report.subscribeToReportLeavingEvents(reportID);
	didSubscribeToReportLeavingEvents.current = true;
	}

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

Lines 146 to 150 in 00315b1

	const didCreateReportSuccessfully = !report.pendingFields || (!report.pendingFields.addWorkspaceRoom && !report.pendingFields.createChat);
	if (!didSubscribeToReportTypingEvents.current && didCreateReportSuccessfully) {
	Report.subscribeToReportTypingEvents(reportID);
	didSubscribeToReportTypingEvents.current = true;
	}

The problem is in the report screen. When we open a new thread, it's possible that the report onyx data is not available yet, so the didCreateReportSuccessfully condition will return true because the pending fields are empty.

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

We can add a new check to make sure the report object has a report ID.

const didCreateReportSuccessfully = report.reportID && (!report.pendingFields || (!report.pendingFields.addWorkspaceRoom && !report.pendingFields.createChat));

What alternative solutions did you explore? (Optional)

Move the subscription to the report actions view too because it's only rendered when the report is already loaded.

[tienifr]

Proposal updated to add example code changes

[Pujan92]

Thanks for the proposals. Both have covered the RCA correctly which seems to be Report onyx data might not be available post navigating to the thread. Regarding the solutions @tienifr I don't think we need to compare the route report id and onyx report id, because if it is still old report then didSubscribeToReportLeavingEvents ref value is enough to identify and avoid subscribing the events. Validating reportID of report onyx data is enough and seems correct to me. With that, we can go with @bernhardoj's proposal.

🎀👀🎀 C+ reviewed

[melvin-bot]

Current assignee @luacmartins is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

[tienifr]

Validating reportID of report onyx data is enough

@Pujan92 I believe comparing the reportID is safer and doesn’t cause any regressions right?

In this line, I don't think we'd feel confident saying didCreateReportSuccessfully is true here if the report we check is different from the report from the route.

Do you think just checking reportID exists is substantially different from my proposal, especially when there’s no harm (even safer) if we do the comparison as in my proposal? 😄

cc @luacmartins

[luacmartins]

I agree that we should minimize these changes to avoid other regressions

[bernhardoj]

This case might be rare but still I think it is a valid point.
Instead of breaking some other things I think it is better to validate only reportID here as a safer side.

Oh yes, that's technically possible. I agree to just validate the reportID.

[Pujan92]

@luacmartins Can you plz check the review comment and other followed comments to decide whom to assign? Also, both contributors have provided valuable inputs so we may consider the bounty split if you think the same.

[tienifr]

@luacmartins TLDR;

@tienifr's proposal is the first one posted, and suggests comparing the reportID of the report with the reportID from the route, and make sure they match before subscribing to the event. @tienifr also suggested to fix 1 additional bug on the event unsubscription, in the same flow.

@bernhardoj's proposal suggests to rely on the existence of report.reportID, if it exists, subscribe to the event. There's also an alternative solution, initially @bernhardoj favored that more but after a potential issue/regression was highlighted, now @bernhardoj favored the main solution.

@Pujan92 also prefers that report.reportID existence check, thinking that comparing the reportID of the report with the reportID from the route is not needed. Meanwhile @tienifr thinks the reportID comparison is stricter and safer, and doesn't have any drawback compared to the reportID existence check (besides, only @tienifr's proposal contains the unsubscription bug fix).

Let us know what you think!

[luacmartins]

I agree that both contributors added value to this issue, so we can split the bounty. As for the proposal, I like @tienifr's solution more since it's stricter.

[melvin-bot]

📣 @Pujan92 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

[melvin-bot]

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

[dylanexpensify]

nice!

[tienifr]

PR ready for review #38657.

[dylanexpensify]

Merged, waiting for regression period then paying out!

[tienifr]

@dylanexpensify PR hit production 5 days ago. Payment due should be on April 03.

[tienifr]

@dylanexpensify Bump on the above ^. @Pujan92 Please complete the checklist once you have time.

[Pujan92]

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:

• [@Pujan92] The PR that introduced the bug has been identified. Link to the PR: Wildan/fix/21518/client pusher method #24407
• [@Pujan92] 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: https://github.com/Expensify/App/pull/24407/files#r1576748976
• [@Pujan92] 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: NA
• [@Pujan92] Determine if we should create a regression test for this bug. Yes
• [@Pujan92] 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.

Regression Test Steps

1. Open report
2. On any message(which isn't replied before) open context menu and select reply in thread
3. Verify thread report opens and isn't showing a AuthenticatePusher 404 error in the console

Do you agree 👍 or 👎 ?

[Pujan92]

@dylanexpensify seems automation is broken for this issue.

[luacmartins]

Updated the issue. Bump @dylanexpensify for payment

[dylanexpensify]

Paid out!

[dylanexpensify]

done!