[$500] Not found is shown when deeplinking to expense details

[rlinoz]

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:**v1.4.43-11
**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:
Slack conversation:

Action Performed:

Break down in numbered steps

1. Having account A and B send a money request from A to B
2. With account A, open the expense details and copy the URL
3. Paste the URL in a fresh new window and sign in with account A again

Expected Result:

Describe what you think should've happened
Expense details page should load

Actual Result:

Describe what actually happened
A not found view is shown

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?
Reloading the page works

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

Screen.Recording.2024-02-18.at.2.01.32.PM.mov

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01715c671a688b4a3b
• Upwork Job ID: 1760748556818485248
• Last Price Increase: 2024-02-22
• Automatic offers:
• s77rt | Contributor | 0

Issue Owner

Current Issue Owner: @deetergp

[melvin-bot]

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

[rlinoz]

This is coming from this other issue: #34346

@shahinyan11 posted a proposal here

But if you could extract the part that makes sense to this one.

[shahinyan11]

But if you could extract the part that makes sense to this one.

Now I'll add the extracted version here to make it easier to understand

[shahinyan11]

Proposal

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

Not found is shown when deeplinking to expense details

What is the root cause of that problem?

The OpenReport endpoint is called before OpenApp and so when we receive the response, the lastUpdateIDAppliedToClient is missing, causing the updates to be saved in Onyx on this line instead of being applied. Then it turns out that during applying updates the lastUpdateID and lastUpdateIDAppliedToClient are equal here which prevents applying updates . Since I think it also can happen in other cases and not only for openReport I think we should fix globally

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

Us < instead <= in this check. Because of this change, It is possible that sometimes we will overwrite the same data that already exists, but this will help us fix the problem

What alternative solutions did you explore? (Optional) (edited)

Always apply changes for specific endpoints . In this case for OpenReport. To do that we can change this check with below code to

(Number(lastUpdateID) <= lastUpdateIDAppliedToClient && request?.command !== "OpenReport")

As ay already said that it can be happen in other places we can create new constant for request commands list during which we will still overwrite the data . e.g ALWAYS_APPLAYING_COMMANDS = [ 'OpenReport', ... ] and use below option instead above one

(Number(lastUpdateID) <= lastUpdateIDAppliedToClient && !ALWAYS_APPLAYING_COMMANDS.includes(request?.command))

[melvin-bot]

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

[melvin-bot]

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

[paultsimura]

Hey @s77rt, would you like to take this one over as a C+ since you've worked on the original issue?🙏
I really want to wrap up my ongoing Contributor PRs this week before fully emerging into the C+ role.

[s77rt]

@paultsimura Sure! Taking this as C+

[s77rt]

@shahinyan11 Thanks for the proposal. The RCA seems incomplete. I have some questions/concerns:

1. If lastUpdateIDAppliedToClient is false then doesClientNeedToBeUpdated will be true and this will make our SaveResponseInOnyx middleware defer the updates and not just ignore them ¹. The updates are eventually applied by OnyxUpdateManager ². Why are we still not seeing the report in Onyx?
2. The report updates come from the OpenReport http response. What does the pusher events (and subscribeToUserEvents function) have to do here?

Footnotes

1. https://github.com/Expensify/App/blob/72bdedb226d0813a68e2b8caa909194064ed92a9/src/libs/Middleware/SaveResponseInOnyx.ts#L40-L45 ↩

2. https://github.com/Expensify/App/blob/72bdedb226d0813a68e2b8caa909194064ed92a9/src/libs/actions/OnyxUpdateManager.ts#L87-L93 ↩

[bernhardoj]

Proposal

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

Not found page is shown when open transaction thread report after login

What is the root cause of that problem?

Summary: The onyx data from the OpenReport request isn't applied because the data is considered outdated.

When we open (deeplink) a report after login, there are 2 requests are made, OpenApp and OpenReport. OpenApp is a read request, so both requests are run in parallel. When the OpenReport request finishes first, OnyxUpdates.doesClientNeedToBeUpdated will return true

App/src/libs/Middleware/SaveResponseInOnyx.ts

Lines 40 to 45 in 4a885f5

	if (requestsToIgnoreLastUpdateID.includes(request.command) || !OnyxUpdates.doesClientNeedToBeUpdated(Number(response?.previousUpdateID ?? 0))) {
	return OnyxUpdates.apply(responseToApply);
	}
	// Save the update IDs to Onyx so they can be used to fetch incremental updates if the client gets out of sync from the server
	OnyxUpdates.saveUpdateInformation(responseToApply);

because lastUpdateIDAppliedToClient doesn't exist yet, the OpenReport response is saved to ONYX_UPDATES_FROM_SERVER.

App/src/libs/actions/OnyxUpdates.ts

Lines 136 to 139 in 4a885f5

	// If we don't have any value in lastUpdateIDAppliedToClient, this is the first time we're receiving anything, so we need to do a last reconnectApp
	if (!lastUpdateIDAppliedToClient) {
	return true;
	}

if OpenApp finishes first, this issue won't happen

This will trigger a ReconnectApp request.

App/src/libs/actions/OnyxUpdateManager.ts

Lines 59 to 75 in 4a885f5

	// 1. This is the first time we're receiving an lastUpdateID, so we need to do a final reconnectApp before
	// fully migrating to the reliable updates mode.
	// 2. This client already has the reliable updates mode enabled, but it's missing some updates and it
	// needs to fetch those.
	//
	// For both of those, we need to pause the sequential queue. This is important so that the updates are
	// applied in their correct and specific order. If this queue was not paused, then there would be a lot of
	// onyx data being applied while we are fetching the missing updates and that would put them all out of order.
	SequentialQueue.pause();
	let canUnpauseQueuePromise;
	// The flow below is setting the promise to a reconnect app to address flow (1) explained above.
	if (!lastUpdateIDAppliedToClient) {
	Log.info('Client has not gotten reliable updates before so reconnecting the app to start the process');
	// Since this is a full reconnectApp, we'll not apply the updates we received - those will come in the reconnect app request.
	canUnpauseQueuePromise = App.finalReconnectAppAfterActivatingReliableUpdates();

ReconnectApp will return a lastUpdateID and the response will be applied immediately (requestsToIgnoreLastUpdateID contains ReconnectApp & OpenApp)

App/src/libs/Middleware/SaveResponseInOnyx.ts

Lines 40 to 42 in 4a885f5

	if (requestsToIgnoreLastUpdateID.includes(request.command) || !OnyxUpdates.doesClientNeedToBeUpdated(Number(response?.previousUpdateID ?? 0))) {
	return OnyxUpdates.apply(responseToApply);
	}

After it's done, the saved OpenReport response is processed.

App/src/libs/actions/OnyxUpdateManager.ts

Lines 87 to 88 in 4a885f5

	canUnpauseQueuePromise.finally(() => {
	OnyxUpdates.apply(updateParams).finally(() => {

However, the OpenReport lastUpdateID is smaller than the lastUpdateIDAppliedToClient (that is updated from ReconnectApp lastUpdateID)

App/src/libs/actions/OnyxUpdates.ts

Lines 81 to 85 in 4a885f5

	function apply({lastUpdateID, type, request, response, updates}: OnyxUpdatesFromServer): Promise<void | Response> | undefined {
	Log.info(`[OnyxUpdateManager] Applying update type: ${type} with lastUpdateID: ${lastUpdateID}`, false, {command: request?.command});
	if (lastUpdateID && lastUpdateIDAppliedToClient && Number(lastUpdateID) <= lastUpdateIDAppliedToClient) {
	Log.info('[OnyxUpdateManager] Update received was older than or the same as current state, returning without applying the updates other than successData and failureData');

so the response data is ignored.

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

I think ideally we should wait for the OpenApp request to be finished first before doing the OpenReport request because ReconnectApp doesn't return all report that a user has.

To achieve that, first we need to change the OpenApp API from READ to WRITE, just like we did for ReconnectApp, so it's processed sequentially.

However, this isn't enough because OpenApp is called after a promise and onyx connection which delays the call.

App/src/libs/actions/App.ts

Lines 210 to 216 in 4a885f5

	function openApp() {
	getPolicyParamsForOpenOrReconnect().then((policyParams: PolicyParamsForOpenOrReconnect) => {
	const params: OpenAppParams = {enablePriorityModeFilter: true, ...policyParams};
	API.read(READ_COMMANDS.OPEN_APP, params, getOnyxDataForOpenOrReconnect(true));
	});
	}

App/src/libs/actions/App.ts

Lines 149 to 158 in 4a885f5

	function getPolicyParamsForOpenOrReconnect(): Promise<PolicyParamsForOpenOrReconnect> {
	return new Promise((resolve) => {
	isReadyToOpenApp.then(() => {
	const connectionID = Onyx.connect({
	key: ONYXKEYS.COLLECTION.POLICY,
	waitForCollectionCallback: true,
	callback: (policies) => {
	Onyx.disconnect(connectionID);
	resolve({policyIDList: getNonOptimisticPolicyIDs(policies)});
	},

To solve this, we can follow a similar pattern of the promise here

App/src/libs/actions/App.ts

Lines 80 to 87 in 4a885f5

	let resolveIsReadyPromise: () => void;
	const isReadyToOpenApp = new Promise<void>((resolve) => {
	resolveIsReadyPromise = resolve;
	});
	function confirmReadyToOpenApp() {
	resolveIsReadyPromise();
	}

let resolveIsOpenAppCalledPromise: () => void;
const isOpenAppCalled = new Promise<void>((resolve) => {
    resolveIsOpenAppCalledPromise = resolve;
});

function waitForOpenApp() {
    return isOpenAppCalled;
}

Then, we will resolve the promise once the OpenApp/ReconnectApp api is pushed to the queue

App/src/libs/actions/App.ts

Lines 214 to 215 in 4a885f5

	API.read(READ_COMMANDS.OPEN_APP, params, getOnyxDataForOpenOrReconnect(true));
	});

App/src/libs/actions/App.ts

Lines 242 to 243 in 4a885f5

	API.write(WRITE_COMMANDS.RECONNECT_APP, params, getOnyxDataForOpenOrReconnect());
	});

why ReconnectApp too? because ReconnectApp will be called instead of OpenApp if we already logged in previously

Or we can resolve the promise immediately when the ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT exists.

Onyx.connect({
    key: ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT,
    callback: (val) => {
        if (val) resolveIsOpenAppCalledPromise();
    },
});

Then, we will wait for the promise here:

App/src/libs/actions/Report.ts

Lines 724 to 727 in 4a885f5

	} else {
	// eslint-disable-next-line rulesdir/no-multiple-api-calls
	API.write(WRITE_COMMANDS.OPEN_REPORT, parameters, {optimisticData, successData, failureData});
	}

App.waitForOpenApp(() => // open report request)

[melvin-bot]

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

[rlinoz]

Oh, that wasn't my intention, just switching the C+ per this comment

[s77rt]

@bernhardoj Thanks for the proposal. Your RCA is correct. Regarding the solution:

To achieve that, first we need to change the OpenApp API from READ to WRITE, just like #12125, so it's processed sequentially.

After doing that, will the OpenReport request wait for the OpenApp to finish if the OpenReport request type is makeRequestWithSideEffects and not read?

[bernhardoj]

No, it won't. Only write requests wait for each other.

Btw, I think we can keep the OpenApp as READ if we use this solution

[s77rt]

@bernhardoj If we deeplink into a report the OpenReport request type would be makeRequestWithSideEffects thus it won't wait for OpenApp

App/src/libs/actions/Report.ts

Lines 719 to 727 in 4a885f5

	if (isFromDeepLink) {
	// eslint-disable-next-line rulesdir/no-api-side-effects-method
	API.makeRequestWithSideEffects(SIDE_EFFECT_REQUEST_COMMANDS.OPEN_REPORT, parameters, {optimisticData, successData, failureData}).finally(() => {
	Onyx.set(ONYXKEYS.IS_CHECKING_PUBLIC_ROOM, false);
	});
	} else {
	// eslint-disable-next-line rulesdir/no-multiple-api-calls
	API.write(WRITE_COMMANDS.OPEN_REPORT, parameters, {optimisticData, successData, failureData});
	}

[bernhardoj]

Yes, that would be intentional since isFromDeepLink is only true when we open the report while logged out (to open the public room). If we wait, then we won't be able to open a public room.

[s77rt]

@bernhardoj In that case you suggestion solution shouldn't work as we won't wait for OpenApp after all.

[bernhardoj]

We don't need to wait for OpenApp (or specifically lastUpdateIDAppliedToClient) when opening a public room because it's not a WRITE request, so doesClientNeedToBeUpdated will always be false.

App/src/libs/actions/OnyxUpdates.ts

Lines 130 to 134 in 89d5689

	function doesClientNeedToBeUpdated(previousUpdateID = 0): boolean {
	// If no previousUpdateID is sent, this is not a WRITE request so we don't need to update our current state
	if (!previousUpdateID) {
	return false;
	}

App/src/libs/Middleware/SaveResponseInOnyx.ts

Lines 40 to 42 in 89d5689

	if (requestsToIgnoreLastUpdateID.includes(request.command) || !OnyxUpdates.doesClientNeedToBeUpdated(Number(response?.previousUpdateID ?? 0))) {
	return OnyxUpdates.apply(responseToApply);
	}

[shahinyan11]

@shahinyan11 Thanks for the proposal. The RCA seems incomplete. I have some questions/concerns:

1. If lastUpdateIDAppliedToClient is false then doesClientNeedToBeUpdated will be true and this will make our SaveResponseInOnyx middleware defer the updates and not just ignore them 1. The updates are eventually applied by OnyxUpdateManager 2. Why are we still not seeing the report in Onyx?
2. The report updates come from the OpenReport http response. What does the pusher events (and subscribeToUserEvents function) have to do here?

Sorry. I mixed up some files and went the wrong way

[shahinyan11]

Proposal

Updated

[s77rt]

@bernhardoj That's not entirely true and contradicts with your RCA. OpenReport does return previousUpdateID (treated as a WRITE request). In this case we need to wait for OpenApp otherwise the onyx update will be just ignored.

I have noticed that OpenReport sometimes return previousUpdateID and sometimes it does not. I see we have a chance of fixing this in the BE.

[s77rt]

@shahinyan11 Thanks for the update. The RCA makes sense but the suggested solutions are workarounds.

[s77rt]

🎀 👀 🎀 C+ reviewed

This could be internal. Can you please check when or what are the cases where OpenReport would return previousUpdateID in its response?

It seems that if we remove previousUpdateID from the server's response we could fix the bug as doesClientNeedToBeUpdated will be false in this case.

App/src/libs/actions/OnyxUpdates.ts

Lines 130 to 134 in 87eeb99

	function doesClientNeedToBeUpdated(previousUpdateID = 0): boolean {
	// If no previousUpdateID is sent, this is not a WRITE request so we don't need to update our current state
	if (!previousUpdateID) {
	return false;
	}

[melvin-bot]

Triggered auto assignment to @deetergp, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[s77rt]

@deetergp Adding to the above ^ I think OpenReport should return previousUpdateID only if we are creating a new report. In our case here we are not, we are accessing a report that already exists, thus previousUpdateID should not be a part of the response.

[bernhardoj]

@s77rt can you try again checking the response? I don't see previousReportID when opening a public room while logged out.

[s77rt]

@bernhardoj Ah sorry I missed the part that we pass isFromDeepLink = true only when opening public rooms. So the request type for money request report would be write and waiting for OpenApp makes sense. However I still don't see why we return previousUpdateID when opening an existing report. This may need to be fixed in the BE.

[garrettmknight]

@deetergp can you help confirm if this needs to be internal via this comment when you get a chance?

[deetergp]

@s77rt Getting rid of previousUpdateID is a non-starter. It's part of our Reliable Updates initiative and is only going to show up in more places as we move where we send onyx updates from our PHP layer, further back into our C++ layer.

[s77rt]

@deetergp I'm not suggesting the removal of previousUpdateID as whole. According to this comment

App/src/libs/actions/OnyxUpdates.ts

Line 131 in 87eeb99

// If no previousUpdateID is sent, this is not a WRITE request so we don't need to update our current state

that field should only be present if the request is a write request. OpenReport command can act as both a write and a read request, write request if creating a new report and a read request otherwise.

Example 1: Response when reading an existing report

Example 2: Response when creating a new report (chat with a user you have not chatted with before, etc.)

The problem is that we are encountering a case where we are reading an existing report (the money request report) yet get the previousUpdateID field which is unexpected. What I'm asking for is to check why is that field present i.e. why is the request treated as a write request?

[Julesssss]

Hey @deetergp, it seems possible that this newly raised bug is a dupe of this issue, would you agree?

[melvin-bot]

@deetergp, @garrettmknight, @s77rt Whoops! This issue is 2 days overdue. Let's get this updated quick!

[tienifr]

I believe this is a dupe of #34528, we should reopen the original issue, there're some history there

[melvin-bot]

@deetergp, @garrettmknight, @s77rt Whoops! This issue is 2 days overdue. Let's get this updated quick!

[garrettmknight]

@tienifr nice catch - agreed that this is a dupe of #34528. @shahinyan11 please add your proposal there if you'd still like to put it forward.