Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mWeb - Chat - Not here page displayed when going online after deleting a thread message #51825

Closed
1 of 8 tasks
lanitochka17 opened this issue Oct 31, 2024 · 5 comments
Closed
1 of 8 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2

Comments

@lanitochka17
Copy link

lanitochka17 commented Oct 31, 2024

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: 9.0.56-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): [email protected]
Issue reported by: Applause - Internal Team

Issue found when executing PR #51422

Action Performed:

  1. Navigate to staging.new.expensify.com
  2. Open a chat report
  3. Go offline
  4. Send a message
  5. Reply in thread of that message
  6. Send a message and delete the message inside of the thread
  7. Go online

Expected Result:

Nothing happens

Actual Result:

Hmmm not here page is displayed

Workaround:

Unknown

Platforms:

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

  • Android: Standalone
  • Android: HybridApp
  • Android: mWeb Chrome
  • iOS: Standalone
  • iOS: HybridApp
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence
Bug6651361_1730393923738.Screen_Recording_20241031_195233_Chrome.mp4

View all open jobs on GitHub

@lanitochka17 lanitochka17 added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Oct 31, 2024
Copy link

melvin-bot bot commented Oct 31, 2024

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

@lanitochka17
Copy link
Author

@OfstadC FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

@bernhardoj
Copy link
Contributor

Proposal

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

Not found page is shown after going to a new thread while offline.

What is the root cause of that problem?

When we open the new thread while offline, there will be 3 open report request (but only the last one sent). The first one when we optimistically create the thread and then send the parent report action ID to the request (along with other data).

App/src/libs/actions/Report.ts

Lines 1099 to 1101 in 6763f1a

const participantLogins = PersonalDetailsUtils.getLoginsByAccountIDs(Object.keys(newChat.participants ?? {}).map(Number));
openReport(newChat.reportID, '', participantLogins, newChat, parentReportAction.reportActionID);
Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(newChat.reportID));

The 2nd one is when the report screen is mounted.

useEffect(() => {
// Call OpenReport only if we are not linking to a message or the report is not available yet
if (isLoadingReportOnyx || reportActionIDFromRoute) {
return;
}
fetchReportIfNeeded();
// eslint-disable-next-line react-compiler/react-compiler, react-hooks/exhaustive-deps
}, [isLoadingReportOnyx]);

And the 3rd one is in this effect. (there is an issue to prevent this to be called)

// before deciding that we shouldn't call OpenReport.
if (reportIDFromRoute === lastReportIDFromRoute && (!onyxReportID || onyxReportID === reportIDFromRoute)) {
return;
}
fetchReportIfNeeded();
ComposerActions.setShouldShowComposeInput(true);
// eslint-disable-next-line react-compiler/react-compiler, react-hooks/exhaustive-deps

The 2nd and 3rd calls shouldn't be made because it's an optimistic report.

if (!shouldFetchReport(report) && (isInitialPageReady || isLinkedMessagePageReady)) {
return;
}
fetchReport();
}, [report, fetchReport, reportIDFromRoute, isLoadingApp, isInitialPageReady, isLinkedMessagePageReady]);

shouldFetchReport will return false for optimistic report and report that has not found error.

export default function shouldFetchReport(report: OnyxEntry<Report>) {
// If the report is optimistic, there's no need to fetch it. The original action should create it.
// If there is an error for creating the chat, there's no need to fetch it since it doesn't exist
return !report?.isOptimisticReport && !report?.errorFields?.createChat;
}

However, the && (isInitialPageReady || isLinkedMessagePageReady) was added in #44488 which causes the OpenReport to be called. isInitialPageReady checks for the report actions length.

const isInitialPageReady = isOffline
? reportActions.length > 0
: reportActions.length >= CONST.REPORT.MIN_INITIAL_REPORT_ACTION_COUNT || isPendingActionExist || (doesCreatedActionExists() && reportActions.length > 0);

But when we open report, the report actions is still loading, so isInitialPageReady return false and an OpenReport request is made with a parentReportActionID as -1.

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

I think it doesn't make sense to tie shouldFetchReport with isInitialPageReady. If shouldFetchReport is false, then don't fetch the report. No need for the report actions to be available. shouldFetchReport only checks for the report object, so it doesn't depend on report actions. So, the solution is to remove, && (isInitialPageReady || isLinkedMessagePageReady)

if (!shouldFetchReport(report) && (isInitialPageReady || isLinkedMessagePageReady)) {
return;
}

@OfstadC
Copy link
Contributor

OfstadC commented Nov 4, 2024

I feel like this isn't a real world scenario - so closing

@OfstadC OfstadC closed this as completed Nov 4, 2024
@melvin-bot melvin-bot bot removed the Overdue label Nov 4, 2024
@bernhardoj
Copy link
Contributor

@OfstadC hi, just to clarify, you can reproduce this issue just by opening a new thread while offline (without deleting any message) and then go online which I think a real scenario that could happen commonly.

It also sometimes happen without going offline at all, but it's just easier to repro while offline.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2
Projects
None yet
Development

No branches or pull requests

3 participants