[$250] Accounting - Accounting tab loads infinitely when the workspace is created offline

[IuliiaHerets]

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.57-3
Reproducible in staging?: Y
Reproducible in production?: Y
Email or phone of affected tester (no customers): [email protected]
Issue reported by: Applause Internal Team

Action Performed:

1. Go to staging.new.expensify.com
2. Go offline.
3. Create a new workspace.
4. Go to workspace settings > More features.
5. Enable Accounting.
6. Go to Accounting.
7. Go online.

Expected Result:

Accounting tab will load after returning online.

Actual Result:

Accounting tab loads infinitely when the workspace is created offline.

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Bug6655070_1730782888555.20241105_125138.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021853787863203385081
• Upwork Job ID: 1853787863203385081
• Last Price Increase: 2024-11-05

Issue Owner

Current Issue Owner: @ntdiary

[melvin-bot]

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

Infinity loading in accounting page if creating the workspace while offline.

What is the root cause of that problem?

The loading will show if the condition below is met.

App/src/pages/workspace/withPolicyConnections.tsx

Lines 58 to 60 in b1bffa6

	if ((isConnectionDataFetchNeeded || isFetchingData || isOnyxDataLoading) && shouldBlockView) {
	return <FullScreenLoadingIndicator />;
	}

The one that we need to focus on is hasConnectionsDataBeenFetched.

App/src/pages/workspace/withPolicyConnections.tsx

Lines 32 to 35 in b1bffa6

	const [hasConnectionsDataBeenFetched, hasConnectionsDataBeenFetchedResult] = useOnyx(`${ONYXKEYS.COLLECTION.POLICY_HAS_CONNECTIONS_DATA_BEEN_FETCHED}${props.policy?.id ?? '-1'}`);
	const isOnyxDataLoading = isLoadingOnyxValue(hasConnectionsDataBeenFetchedResult);
	const isConnectionDataFetchNeeded =
	!isOnyxDataLoading && !isOffline && !!props.policy && (!!props.policy.areConnectionsEnabled || !isEmptyObject(props.policy.connections)) && !hasConnectionsDataBeenFetched;

The hasConnectionsDataBeenFetched will become true if the OPEN_POLICY_ACCOUNTING_PAGE read is completed. We request it only while online.

App/src/libs/actions/PolicyConnections.ts

Lines 8 to 33 in b1bffa6

	function openPolicyAccountingPage(policyID: string) {
	const hasConnectionsDataBeenFetchedKey = `${ONYXKEYS.COLLECTION.POLICY_HAS_CONNECTIONS_DATA_BEEN_FETCHED}${policyID}` as const;
	const successData: OnyxUpdate[] = [
	{
	onyxMethod: Onyx.METHOD.MERGE,
	key: hasConnectionsDataBeenFetchedKey,
	value: true,
	},
	];
	const failureData: OnyxUpdate[] = [
	{
	onyxMethod: Onyx.METHOD.MERGE,
	key: hasConnectionsDataBeenFetchedKey,
	value: false,
	},
	];
	const parameters: OpenPolicyAccountingPageParams = {
	policyID,
	};
	API.read(READ_COMMANDS.OPEN_POLICY_ACCOUNTING_PAGE, parameters, {
	successData,
	failureData,
	});

When we make a read request, it will wait for all write requests to complete first, by using the isReadyPromise.

App/src/libs/API/index.ts

Lines 170 to 179 in b1bffa6

	* Ensure all write requests on the sequential queue have finished responding before running read requests.
	* Responses from read requests can overwrite the optimistic data inserted by
	* write requests that use the same Onyx keys and haven't responded yet.
	*/
	function waitForWrites<TCommand extends ReadCommand>(command: TCommand) {
	if (PersistedRequests.getLength() > 0) {
	Log.info(`[API] '${command}' is waiting on ${PersistedRequests.getLength()} write commands`);
	}
	return SequentialQueue.waitForIdle();
	}

App/src/libs/Network/SequentialQueue.ts

Lines 270 to 272 in b1bffa6

	function waitForIdle(): Promise<unknown> {
	return isReadyPromise;
	}

However, in our case, the promise is never resolved even after all write requests are completed. That's because, while waiting for the promise to be resolved, the promise is recreated, so the old one is never resolved.

App/src/libs/Network/SequentialQueue.ts

Lines 155 to 158 in b1bffa6

	// Reset the isReadyPromise so that the queue will be flushed as soon as the request is finished
	isReadyPromise = new Promise((resolve) => {
	resolveIsReadyPromise = resolve;
	});

It's recreated every time flush is called. After the flush is called, isSequentialQueueRunning is set to true which prevents any subsequent flush from being processed when there is one already running.

App/src/libs/Network/SequentialQueue.ts

Line 153 in b1bffa6

isSequentialQueueRunning = true;

App/src/libs/Network/SequentialQueue.ts

Lines 136 to 139 in b1bffa6

	if (isSequentialQueueRunning) {
	Log.info('[SequentialQueue] Unable to flush. Queue is already running.');
	return;
	}

But in our case, isSequentialQueueRunning becomes false. That's because when we create the workspace and enable the accounting while offline, the BE will return shouldPauseQueue for EnablePolicyConnections which pause the queue.

App/src/libs/Network/SequentialQueue.ts

Lines 95 to 98 in b1bffa6

	if (response?.shouldPauseQueue) {
	Log.info("[SequentialQueue] Handled 'shouldPauseQueue' in response. Pausing the queue.");
	pause();
	}

When it processes the next request, the promise is resolved because the queue is paused,

App/src/libs/Network/SequentialQueue.ts

Lines 68 to 71 in b1bffa6

	if (isQueuePaused) {
	Log.info('[SequentialQueue] Unable to process. Queue is paused.');
	return Promise.resolve();
	}

which sets isSequentialQueueRunning to false.

App/src/libs/Network/SequentialQueue.ts

Lines 168 to 170 in b1bffa6

	process().finally(() => {
	Log.info('[SequentialQueue] Finished processing queue.');
	isSequentialQueueRunning = false;

When the queue is unpaused, we call flush again.

App/src/libs/Network/SequentialQueue.ts

Lines 188 to 198 in b1bffa6

	function unpause() {
	if (!isQueuePaused) {
	Log.info('[SequentialQueue] Unable to unpause queue. We are already processing.');
	return;
	}
	const numberOfPersistedRequests = PersistedRequests.getAll().length || 0;
	Log.info(`[SequentialQueue] Unpausing the queue and flushing ${numberOfPersistedRequests} requests`);
	isQueuePaused = false;
	flush();
	}

And because isSequentialQueueRunning is false, the isReadyPromise is recreated.

flush -> process (CreateWorkspace) -> process (EnablePolicyConnections) -> pause -> GetMissingOnyxMessages -> unpause -> flush (new isReadyPromise) -> process (ReconnectApp)

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

We shouldn't reset the isReadyPromise when unpausing the queue. To do that, we can accept a new param for flush called resume. If it's true, then don't recreate isReadyPromise.

if (!resume) {
    // Reset the isReadyPromise so that the queue will be flushed as soon as the request is finished
    isReadyPromise = new Promise((resolve) => {
        resolveIsReadyPromise = resolve;
    });
}

And pass true when flushing from unpause.

App/src/libs/Network/SequentialQueue.ts

Lines 188 to 198 in b1bffa6

	function unpause() {
	if (!isQueuePaused) {
	Log.info('[SequentialQueue] Unable to unpause queue. We are already processing.');
	return;
	}
	const numberOfPersistedRequests = PersistedRequests.getAll().length || 0;
	Log.info(`[SequentialQueue] Unpausing the queue and flushing ${numberOfPersistedRequests} requests`);
	isQueuePaused = false;
	flush();
	}

[melvin-bot]

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

[melvin-bot]

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

[ntdiary]

Under review.

still verifying the RCA, will provide an update tomorrow.

[ntdiary]

@bernhardoj's RCA is accurate, with only a slight difference from what I checked. :)
pause is called here, and shouldPauseQueue is also set to true here:

App/src/libs/Middleware/SaveResponseInOnyx.ts

Lines 38 to 45 in e453cfd

	// 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);
	// Ensure the queue is paused while the client resolves the gap in onyx updates so that updates are guaranteed to happen in a specific order.
	return Promise.resolve({
	...response,
	shouldPauseQueue: true,
	});

---

BTW, just a note, in our current SequentialQueue, we don’t have an exact concept of "current batch"

App/src/libs/Network/SequentialQueue.ts

Lines 251 to 257 in e000d80

	// If the queue is running this request will run once it has finished processing the current batch
	if (isSequentialQueueRunning) {
	isReadyPromise.then(flush);
	return;
	}
	flush();

we just ensure that all requests in persistedRequests are executed in order by calling flush in multiple places.

[ntdiary]

Additionally, @bernhardoj, just a small question: since our code calls unpause in several places, do you think adding this parameter will be safe enough to avoid causing regressions? 😂