diff --git a/src/libs/Middleware/SaveResponseInOnyx.ts b/src/libs/Middleware/SaveResponseInOnyx.ts index 1a423540de20..fc5b2f96e884 100644 --- a/src/libs/Middleware/SaveResponseInOnyx.ts +++ b/src/libs/Middleware/SaveResponseInOnyx.ts @@ -1,5 +1,7 @@ import {READ_COMMANDS, SIDE_EFFECT_REQUEST_COMMANDS, WRITE_COMMANDS} from '@libs/API/types'; +import Log from '@libs/Log'; +import {reconnectApp} from '@userActions/App'; import * as OnyxUpdates from '@userActions/OnyxUpdates'; import CONST from '@src/CONST'; @@ -21,6 +23,15 @@ const requestsToIgnoreLastUpdateID = new Set([ SIDE_EFFECT_REQUEST_COMMANDS.GET_MISSING_ONYX_MESSAGES, ]); +// These requests already bring the client up to date (or end the session), so they never need a full ReconnectApp to cover a gap in update IDs. +// This is separate from requestsToIgnoreLastUpdateID because requests like DeleteMoneyRequest skip the OnyxUpdates flow but still move lastUpdateID forward. +const requestsExemptFromFullReconnect = new Set([ + WRITE_COMMANDS.OPEN_APP, + SIDE_EFFECT_REQUEST_COMMANDS.RECONNECT_APP, + SIDE_EFFECT_REQUEST_COMMANDS.GET_MISSING_ONYX_MESSAGES, + WRITE_COMMANDS.CLOSE_ACCOUNT, +]); + // A request belongs here when its successData/finallyData is what unblocks authentication, because parking that leaves the client unable to reauthenticate. const requestsToApplyWithoutAdvancingLastUpdateID = new Set([READ_COMMANDS.SIGN_IN_WITH_SHORT_LIVED_AUTH_TOKEN, READ_COMMANDS.SIGN_IN_WITH_SUPPORT_AUTH_TOKEN]); @@ -44,6 +55,31 @@ const SaveResponseInOnyx: Middleware = (requestResponse: P const shouldApplyWithoutAdvancingLastUpdateID = requestsToApplyWithoutAdvancingLastUpdateID.has(request.command); + // The server only keeps recent update IDs. When a client comes back after a long time, the server no longer knows its + // lastUpdateIDAppliedToClient and answers a request with a lastUpdateID but no previousUpdateID. Applying that response moves + // the client's lastUpdateID forward past updates it never received, so an incremental ReconnectApp can't recover them. + // Run a full ReconnectApp after applying so the client gets everything in the gap. + const clientLastUpdateID = OnyxUpdates.getPersistedLastUpdateID(); + const responseLastUpdateID = Number(response?.lastUpdateID ?? CONST.DEFAULT_NUMBER_ID); + const shouldFullReconnectAfterApply = + !shouldApplyWithoutAdvancingLastUpdateID && + !requestsExemptFromFullReconnect.has(request.command) && + !!clientLastUpdateID && + responseLastUpdateID > clientLastUpdateID && + !Number(response?.previousUpdateID ?? CONST.DEFAULT_NUMBER_ID); + + if (shouldFullReconnectAfterApply) { + Log.info('[SaveResponseInOnyx] Response has a lastUpdateID but no previousUpdateID, running a full ReconnectApp after applying it', false, { + command: request.command, + clientLastUpdateID, + responseLastUpdateID, + }); + return OnyxUpdates.apply(responseToApply).then((appliedResponse) => { + reconnectApp(); + return appliedResponse; + }); + } + if ( shouldApplyWithoutAdvancingLastUpdateID || requestsToIgnoreLastUpdateID.has(request.command) || diff --git a/src/libs/actions/User.ts b/src/libs/actions/User.ts index 418f1dcd1584..6c8f986e1f6c 100644 --- a/src/libs/actions/User.ts +++ b/src/libs/actions/User.ts @@ -86,7 +86,7 @@ Onyx.connectWithoutView({ }, }); -// This value is used in the handler for a RECONNECT_APP Pusher event (anonymous users), so it's not possible to use useOnyx(). +// This value is used in the handler for a RECONNECT_APP Pusher event, so it's not possible to use useOnyx(). let lastUpdateIDAppliedToClient: OnyxEntry; Onyx.connectWithoutView({ key: ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, @@ -943,14 +943,12 @@ function subscribeToUserEvents( // We have an event to reconnect the App. It is triggered when we detect that the user passed updateID // is not in the DB PusherUtils.subscribeToMultiEvent(Pusher.TYPE.MULTIPLE_EVENT_TYPE.RECONNECT_APP, () => { - // Anonymous public-room sessions already have report actions from OpenReport. A full ReconnectApp - // uses setCollection and clears them, which flashes the loading skeleton until GetNewerActions - // refills. Pass lastUpdateID so reconnect stays incremental for anonymous users (#97847). - if (isAnonymousUser()) { - reconnectApp(lastUpdateIDAppliedToClient); - } else { - reconnectApp(); - } + // This event is broadcast on the account-wide channel, so every signed-in device receives it, not only + // the one whose updateID is missing. A full ReconnectApp uses setCollection and clears report actions the + // device already holds (#97847, #100791). Pass lastUpdateID so the server can reply incrementally. It + // still returns full data when it can't serve that range, and we fall back to a full reconnect when the + // client has no lastUpdateID yet. + reconnectApp(lastUpdateIDAppliedToClient); return Promise.resolve(); }); } diff --git a/tests/actions/UserTest.ts b/tests/actions/UserTest.ts index 6bd8b104a21d..36ca52e9cd6a 100644 --- a/tests/actions/UserTest.ts +++ b/tests/actions/UserTest.ts @@ -1180,18 +1180,39 @@ describe('actions/User', () => { expect(reconnectAppMock).toHaveBeenCalledWith(lastUpdateID); }); - it('calls reconnectApp without updateIDFrom for authenticated users', async () => { + it('passes lastUpdateIDAppliedToClient when reconnecting as an authenticated user', async () => { + // Given a signed-in device that has already applied updates up to a known ID + const lastUpdateID = 42; + await Onyx.merge(ONYXKEYS.SESSION, {authTokenType: undefined, accountID: 1, authToken: 'token'}); + await Onyx.merge(ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, lastUpdateID); + await waitForBatchedUpdates(); + + UserActions.subscribeToUserEvents(1, 'test@test.com', () => {}, formatPhoneNumber, undefined); + await waitForBatchedUpdates(); + + // When the account-wide RECONNECT_APP event arrives (for example, because another device signed in) + await PusherUtils.triggerMultiEventHandler(Pusher.TYPE.MULTIPLE_EVENT_TYPE.RECONNECT_APP, []); + await waitForBatchedUpdates(); + + // Then the reconnect is incremental, so a full ReconnectApp doesn't wipe report actions this device already has + expect(reconnectAppMock).toHaveBeenCalledWith(lastUpdateID); + }); + + it('falls back to a full reconnect when the client has no lastUpdateIDAppliedToClient', async () => { + // Given a signed-in device that has not applied any updates yet await Onyx.merge(ONYXKEYS.SESSION, {authTokenType: undefined, accountID: 1, authToken: 'token'}); - await Onyx.merge(ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, 42); + await Onyx.set(ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, null); await waitForBatchedUpdates(); UserActions.subscribeToUserEvents(1, 'test@test.com', () => {}, formatPhoneNumber, undefined); await waitForBatchedUpdates(); + // When the RECONNECT_APP event arrives await PusherUtils.triggerMultiEventHandler(Pusher.TYPE.MULTIPLE_EVENT_TYPE.RECONNECT_APP, []); await waitForBatchedUpdates(); - expect(reconnectAppMock).toHaveBeenCalledWith(); + // Then no updateIDFrom is passed, so reconnectApp still does a full reconnect to recover + expect(reconnectAppMock).toHaveBeenCalledWith(undefined); }); }); }); diff --git a/tests/unit/MiddlewareTest.ts b/tests/unit/MiddlewareTest.ts index d6a6d79da16d..772aea316f2d 100644 --- a/tests/unit/MiddlewareTest.ts +++ b/tests/unit/MiddlewareTest.ts @@ -1,7 +1,8 @@ -import {READ_COMMANDS} from '@libs/API/types'; +import {READ_COMMANDS, SIDE_EFFECT_REQUEST_COMMANDS, WRITE_COMMANDS} from '@libs/API/types'; import SaveResponseInOnyx from '@libs/Middleware/SaveResponseInOnyx'; import CONST from '@src/CONST'; +import * as App from '@src/libs/actions/App'; import * as PersistedRequests from '@src/libs/actions/PersistedRequests'; import type MoveIOUReportToExistingPolicyParams from '@src/libs/API/parameters/MoveIOUReportToExistingPolicyParams'; import HttpUtils from '@src/libs/HttpUtils'; @@ -120,6 +121,75 @@ describe('Middleware', () => { expect(await getOnyxValue(ONYXKEYS.ONYX_UPDATES_FROM_SERVER)).toBeUndefined(); expect(await getOnyxValue(ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT)).toBe(clientLastUpdateID); }); + + describe('when the server no longer knows the client lastUpdateID', () => { + beforeEach(() => { + jest.spyOn(App, 'reconnectApp').mockImplementation(() => {}); + }); + + afterEach(() => { + jest.restoreAllMocks(); + }); + + test.each([ + ['AddComment', 'AddComment'], + // DeleteMoneyRequest skips the OnyxUpdates flow but still moves lastUpdateID forward, so it can leave the same gap + ['DeleteMoneyRequest', WRITE_COMMANDS.DELETE_MONEY_REQUEST], + ])('applies the response and runs a full ReconnectApp to cover the gap for %s', async (_case, command) => { + // Given a device that was backgrounded for a long time, so the server no longer has its lastUpdateIDAppliedToClient + await Onyx.merge(ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, 100); + await waitForBatchedUpdates(); + + Request.addMiddleware(SaveResponseInOnyx); + + // When a write returns a lastUpdateID but no previousUpdateID + jest.spyOn(HttpUtils, 'xhr').mockResolvedValueOnce({jsonCode: 200, lastUpdateID: 5001}); + const result = await Request.processWithMiddleware({ + command, + data: {apiRequestType: CONST.API_REQUEST_TYPE.WRITE}, + successData: [{onyxMethod: Onyx.METHOD.MERGE, key: ONYXKEYS.IS_LOADING_APP, value: false}], + }); + await waitForBatchedUpdates(); + + // Then the response is applied without pausing the queue + expect(result?.shouldPauseQueue).toBeUndefined(); + expect(await getOnyxValue(ONYXKEYS.ONYX_UPDATES_FROM_SERVER)).toBeUndefined(); + + // And a full ReconnectApp runs, because an incremental one would start after the skipped updates + expect(App.reconnectApp).toHaveBeenCalledTimes(1); + expect(App.reconnectApp).toHaveBeenCalledWith(); + }); + + test.each([ + ['the response has a previousUpdateID', 100, 'AddComment', {lastUpdateID: 101, previousUpdateID: 100}], + ['the client has no lastUpdateID yet', undefined, 'AddComment', {lastUpdateID: 5001}], + ['the response is not newer than the client', 100, 'AddComment', {lastUpdateID: 100}], + ['the request is a ReconnectApp', 100, SIDE_EFFECT_REQUEST_COMMANDS.RECONNECT_APP, {lastUpdateID: 5001}], + ['the request is an OpenApp', 100, WRITE_COMMANDS.OPEN_APP, {lastUpdateID: 5001}], + ['the request is a GetMissingOnyxMessages', 100, SIDE_EFFECT_REQUEST_COMMANDS.GET_MISSING_ONYX_MESSAGES, {lastUpdateID: 5001}], + ['the request is a CloseAccount', 100, WRITE_COMMANDS.CLOSE_ACCOUNT, {lastUpdateID: 5001}], + ])('does not run a full ReconnectApp when %s', async (_case, clientLastUpdateID, command, updateIDs) => { + // Given a client whose lastUpdateID is either still known to the server or has nothing to lose + if (clientLastUpdateID) { + await Onyx.merge(ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, clientLastUpdateID); + } + await waitForBatchedUpdates(); + + Request.addMiddleware(SaveResponseInOnyx); + jest.spyOn(HttpUtils, 'xhr').mockResolvedValueOnce({jsonCode: 200, ...updateIDs}); + + // When the response comes back + await Request.processWithMiddleware({ + command, + data: {apiRequestType: CONST.API_REQUEST_TYPE.WRITE}, + successData: [{onyxMethod: Onyx.METHOD.MERGE, key: ONYXKEYS.IS_LOADING_APP, value: false}], + }); + await waitForBatchedUpdates(); + + // Then no extra full ReconnectApp runs, since no updates were skipped or the request already returns full data + expect(App.reconnectApp).not.toHaveBeenCalled(); + }); + }); }); describe('HandleUnusedOptimisticID', () => {