[$250] 3 OpenReport API calls for the same report

[m-natarajan]

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.41-6
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):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: @iwiznia
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1727446418470879

Action Performed:

1. User A and user B are on a chat
2. User A adds a comment
3. User A creates a thread under their comment
4. User B clicks the thread

Expected Result:

Thread opens and no duplicate OpenReport API calls for same report

Actual Result:

3 OpenReport API calls for the same report

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021841330786260257123
• Upwork Job ID: 1841330786260257123
• Last Price Increase: 2024-11-13

Issue Owner

Current Issue Owner: @shubham1206agra

[melvin-bot]

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

[bernhardoj]

Proposal

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

There are 2 OpenReport calls when open a thread that we never loaded yet.

What is the root cause of that problem?

The 1st OpenReport is called in this effect. The purpose of this effect is to call the OpenReport on mount, but we only want to call it when the report onyx is already loaded. We did this after migrating from withOnyx to useOnyx here.

App/src/pages/home/ReportScreen.tsx

Lines 489 to 496 in 69483ff

	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]);

The 2nd and 3rd call is triggered from this effect.

App/src/pages/home/ReportScreen.tsx

Lines 577 to 588 in 69483ff

	// If you already have a report open and are deeplinking to a new report on native,
	// the ReportScreen never actually unmounts and the reportID in the route also doesn't change.
	// Therefore, we need to compare if the existing reportID is the same as the one in the route
	// before deciding that we shouldn't call OpenReport.
	if (onyxReportID === prevReport?.reportID && (!onyxReportID || onyxReportID === reportIDFromRoute)) {
	return;
	}
	fetchReportIfNeeded();
	ComposerActions.setShouldShowComposeInput(true);
	// eslint-disable-next-line react-compiler/react-compiler, react-hooks/exhaustive-deps
	}, [

When we open the thread that we haven't loaded yet, the report will initially be undefined. The 1st OpenReport will optimistically merge the reportName as Chat Report.

App/src/libs/actions/Report.ts

Lines 779 to 783 in 69483ff

	const optimisticReport = reportActionsExist(reportID)
	? {}
	: {
	reportName: ReportConnection.getAllReports()?.[`${ONYXKEYS.COLLECTION.REPORT}${reportID}`]?.reportName ?? CONST.REPORT.DEFAULT_REPORT_NAME,
	};

This will update the report to {reportName: 'Chat Report'}, and so, the report onyx now contains an empty reportID.

App/src/pages/home/ReportScreen.tsx

Lines 169 to 173 in 69483ff

	const report = useMemo(
	(): OnyxEntry<OnyxTypes.Report> =>
	reportOnyx && {
	lastReadTime: reportOnyx.lastReadTime,
	reportID: reportOnyx.reportID ?? '',

Because onyxReportID (empty string) is different from prevReport?.reportID (undefined), the 2nd OpenReport is called. After the 1st OpenReport is finished, the onyxReportID is updated to the correct reportID (X). Again, because the onyxReportID (X) is different from prevReport?.reportID (empty string), the 3rd OpenReport is called.

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

There are 2 solutions. First, if we see the comment here,

App/src/pages/home/ReportScreen.tsx

Lines 577 to 585 in 69483ff

	// If you already have a report open and are deeplinking to a new report on native,
	// the ReportScreen never actually unmounts and the reportID in the route also doesn't change.
	// Therefore, we need to compare if the existing reportID is the same as the one in the route
	// before deciding that we shouldn't call OpenReport.
	if (onyxReportID === prevReport?.reportID && (!onyxReportID || onyxReportID === reportIDFromRoute)) {
	return;
	}
	fetchReportIfNeeded();

it's mentioned that when deeplinking from a report to another report, the report screen is not unmounted and the reportID param is not updated, so the old conditions doesn't work when comparing prev reportID params to the current reportID params.

The report screen is not unmounted part is still true today because if we deeplink to the same screen with a different params, react-navigation will just update the screen params, so this part isn't true anymore.

the reportID in the route also doesn't change.

So, the first solution is to use the old conditions by comparing the prev reportID params and the current reportID params.

if (prevReportIDFromRoute === reportIDFromRoute) {
    return;
}

The second solution is to push a new screen if the reportID is different. To do that, we can override the deeplink here.

App/src/libs/Navigation/linkingConfig/subscribe/index.native.ts

Lines 21 to 29 in 69483ff

	if (path) {
	const stateFromPath = getStateFromPath(path, config);
	if (stateFromPath) {
	const focusedRoute = findFocusedRoute(stateFromPath);
	if (focusedRoute && focusedRoute.name === SCREENS.SEARCH.CENTRAL_PANE) {
	Navigation.navigate(path as Route);
	return;
	}
	}

if (focusedRoute?.name === SCREENS.REPORT && hasAuthToken()) {
    Navigation.navigate(path as Route);
    return;
}

Then, we can remove the OpenReport call here.

App/src/pages/home/ReportScreen.tsx

Lines 577 to 585 in 69483ff

	// If you already have a report open and are deeplinking to a new report on native,
	// the ReportScreen never actually unmounts and the reportID in the route also doesn't change.
	// Therefore, we need to compare if the existing reportID is the same as the one in the route
	// before deciding that we shouldn't call OpenReport.
	if (onyxReportID === prevReport?.reportID && (!onyxReportID || onyxReportID === reportIDFromRoute)) {
	return;
	}
	fetchReportIfNeeded();

[melvin-bot]

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

[melvin-bot]

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

[VictoriaExpensify]

I think #vip-vsb is the best place for this since it's related to threads

[melvin-bot]

@VictoriaExpensify, @shubham1206agra Huh... This is 4 days overdue. Who can take care of this?

[melvin-bot]

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

[iwiznia]

@shubham1206agra can you review @bernhardoj's proposal or should we reassign this?

[shubham1206agra]

@bernhardoj Your RCA looks correct but I don't think your solution is addressing the RCA.

[bernhardoj]

In my root cause, I explained the unnecessary calls are coming from here,

App/src/pages/home/ReportScreen.tsx

Lines 582 to 587 in 2987fc0

	if (onyxReportID === prevReport?.reportID && (!onyxReportID || onyxReportID === reportIDFromRoute)) {
	return;
	}
	fetchReportIfNeeded();
	ComposerActions.setShouldShowComposeInput(true);

which I offer 2 solutions to solve it. Do you care to explain why you think the solution doesn't address the RCA?

[shubham1206agra]

@bernhardoj Can you address this?

Because onyxReportID (empty string) is different from prevReport?.reportID (undefined), the 2nd OpenReport is called.

[bernhardoj]

That specifically happens after #49056 where they add a check for the report here.

App/src/pages/home/ReportScreen.tsx

Lines 169 to 173 in f0be8a7

	const report = useMemo(
	(): OnyxEntry<OnyxTypes.Report> =>
	reportOnyx && {
	lastReadTime: reportOnyx.lastReadTime,
	reportID: reportOnyx.reportID ?? '',

So, previously, even when the report is undefined, the reportID will still be an empty string, so the 2nd OpenReport isn't called.

Again, because the onyxReportID (X) is different from prevReport?.reportID (empty string), the 3rd OpenReport is called.

But I tried reverting it locally and the 3rd OpenReport is still called when the reportID changes from empty string to the correct reportID. That's why I didn't talk about that since the multiple OpenReport will still happen.

[melvin-bot]

@VictoriaExpensify @shubham1206agra this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

[melvin-bot]

@VictoriaExpensify, @shubham1206agra Eep! 4 days overdue now. Issues have feelings too...

[shubham1206agra]

@bernhardoj Tbh, this solution looks like a workaround. We should focus on the main cause of the problem.

[bernhardoj]

Why is it a workaround? The current code is the workaround for an issue that isn't happening anymore.

App/src/pages/home/ReportScreen.tsx

Lines 580 to 586 in 96ffbfe

	// If you already have a report open and are deeplinking to a new report on native,
	// the ReportScreen never actually unmounts and the reportID in the route also doesn't change.
	// Therefore, we need to compare if the existing reportID is the same as the one in the route
	// before deciding that we shouldn't call OpenReport.
	if (onyxReportID === prevReport?.reportID && (!onyxReportID || onyxReportID === reportIDFromRoute)) {
	return;
	}

I'm suggesting to revert back to the old condition which compares the prev and current reportID params. If my solution is a workaround, then the old code is also a workaround, which I don't think is true.

There is even a 2nd solution that will remove that condition and push a new screen instead of updating the current screen params.

I think removing a workaround is preferable compared to patching existing workaround.

[melvin-bot]

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

[bernhardoj]

Is it not happening anymore because of the workaround @bernhardoj ? i.e. if we removed the workaround would the issue resurface?

Yes, it's not happening anymore from my testing but not because of the workaround. I think it's a bug in the navigator (drawer navigator) that we used in the old code before.

[VictoriaExpensify]

OK if reverting the workaround from your code here will resolve this issue, and removing it will not cause further issues, I think that's the approach we should take

[melvin-bot]

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

[melvin-bot]

@VictoriaExpensify, @shubham1206agra Whoops! This issue is 2 days overdue. Let's get this updated quick!

[shubham1206agra]

@VictoriaExpensify Please close this issue.

[melvin-bot]

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

[shubham1206agra]

This is fixed already. Please close this issue.

[iwiznia]

Is it? IIRC someone mentioned that this was still happening... See this conversation https://expensify.slack.com/archives/C05LX9D6E07/p1730737982412859?thread_ts=1727884875.529299&cid=C05LX9D6E07

[melvin-bot]

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

[melvin-bot]

@VictoriaExpensify, @shubham1206agra Huh... This is 4 days overdue. Who can take care of this?

[melvin-bot]

@VictoriaExpensify, @shubham1206agra Still overdue 6 days?! Let's take care of this!

[VictoriaExpensify]

hey @shubham1206agra - can you please check the conversation Ioni linked here - #49948 (comment)

It looks like this issue is still happening

[melvin-bot]

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

[melvin-bot]

@VictoriaExpensify, @shubham1206agra Now this issue is 8 days overdue. Are you sure this should be a Daily? Feel free to change it!

[mvtglobally]

Issue not reproducible during KI retests. (First week)

[melvin-bot]

@VictoriaExpensify, @shubham1206agra 12 days overdue now... This issue's end is nigh!

[shubham1206agra]

@VictoriaExpensify Please close this

[muttmuure]

Fixed over here: #51680