De-dupe network requests

src/libs/API/index.ts

@@ -9,7 +9,7 @@ import * as Request from '@libs/Request';
 import * as PersistedRequests from '@userActions/PersistedRequests';
 import CONST from '@src/CONST';
 import type OnyxRequest from '@src/types/onyx/Request';
-import type {PaginatedRequest, PaginationConfig} from '@src/types/onyx/Request';
+import type {PaginatedRequest, PaginationConfig, RequestConflictResolver} from '@src/types/onyx/Request';
 import type Response from '@src/types/onyx/Response';
 import type {ApiCommand, ApiRequestCommandParameters, ApiRequestType, CommandOfType, ReadCommand, SideEffectRequestCommand, WriteCommand} from './types';
 
@@ -45,7 +45,13 @@ type OnyxData = {
 /**
  * Prepare the request to be sent. Bind data together with request metadata and apply optimistic Onyx data.
  */
-function prepareRequest<TCommand extends ApiCommand>(command: TCommand, type: ApiRequestType, params: ApiRequestCommandParameters[TCommand], onyxData: OnyxData = {}): OnyxRequest {
+function prepareRequest<TCommand extends ApiCommand>(
+    command: TCommand,
+    type: ApiRequestType,
+    params: ApiRequestCommandParameters[TCommand],
+    onyxData: OnyxData = {},
+    conflictResolver: RequestConflictResolver = {},
+): OnyxRequest {
     Log.info('[API] Preparing request', false, {command, type});
 
     const {optimisticData, ...onyxDataWithoutOptimisticData} = onyxData;
@@ -71,6 +77,7 @@ function prepareRequest<TCommand extends ApiCommand>(command: TCommand, type: Ap
         command,
         data,
         ...onyxDataWithoutOptimisticData,
+        ...conflictResolver,
     };
 
     if (isWriteRequest) {
@@ -116,9 +123,14 @@ function processRequest(request: OnyxRequest, type: ApiRequestType): Promise<voi
  * @param [onyxData.failureData] - Onyx instructions that will be passed to Onyx.update() when the response has jsonCode !== 200.
  * @param [onyxData.finallyData] - Onyx instructions that will be passed to Onyx.update() when the response has jsonCode === 200 or jsonCode !== 200.
  */
-function write<TCommand extends WriteCommand>(command: TCommand, apiCommandParameters: ApiRequestCommandParameters[TCommand], onyxData: OnyxData = {}): void {
+function write<TCommand extends WriteCommand>(
+    command: TCommand,
+    apiCommandParameters: ApiRequestCommandParameters[TCommand],
+    onyxData: OnyxData = {},
+    conflictResolver: RequestConflictResolver = {},
+): void {
     Log.info('[API] Called API write', false, {command, ...apiCommandParameters});
-    const request = prepareRequest(command, CONST.API_REQUEST_TYPE.WRITE, apiCommandParameters, onyxData);
+    const request = prepareRequest(command, CONST.API_REQUEST_TYPE.WRITE, apiCommandParameters, onyxData, conflictResolver);
     processRequest(request, CONST.API_REQUEST_TYPE.WRITE);
 }
 

src/libs/Network/SequentialQueue.ts

@@ -24,7 +24,8 @@ let isReadyPromise = new Promise((resolve) => {
 resolveIsReadyPromise?.();
 
 let isSequentialQueueRunning = false;
-let currentRequest: Promise<void> | null = null;
+let currentRequest: OnyxRequest | null = null;
+let currentRequestPromise: Promise<void> | null = null;
 let isQueuePaused = false;
 
 /**
@@ -82,7 +83,8 @@ function process(): Promise<void> {
     const requestToProcess = persistedRequests[0];
 
     // Set the current request to a promise awaiting its processing so that getCurrentRequest can be used to take some action after the current request has processed.
-    currentRequest = Request.processWithMiddleware(requestToProcess, true)
+    currentRequest = requestToProcess;
+    currentRequestPromise = Request.processWithMiddleware(requestToProcess, true)
         .then((response) => {
             // A response might indicate that the queue should be paused. This happens when a gap in onyx updates is detected between the client and the server and
             // that gap needs resolved before the queue can continue.
@@ -112,7 +114,7 @@ function process(): Promise<void> {
                 });
         });
 
-    return currentRequest;
+    return currentRequestPromise;
 }
 
 function flush() {
@@ -158,6 +160,7 @@ function flush() {
                     resolveIsReadyPromise?.();
                 }
                 currentRequest = null;
+                currentRequestPromise = null;
                 flushOnyxUpdatesQueue();
             });
         },
@@ -192,8 +195,35 @@ function isPaused(): boolean {
 NetworkStore.onReconnection(flush);
 
 function push(request: OnyxRequest) {
+    // identify and handle any existing requests that conflict with the new one, ignoring the one that's currently processing if there is one
+    const requests = PersistedRequests.getAll().filter((persistedRequest) => persistedRequest !== currentRequest);
+    let hasConflict = false;
+    const {getConflictingRequests, handleConflictingRequest, shouldSkipThisRequestOnConflict} = request;
+    if (getConflictingRequests) {
+        // Identify conflicting requests according to logic bound to the new request
+        const conflictingRequests = getConflictingRequests(requests);
+        hasConflict = conflictingRequests.length > 0;
+
+        // Delete the conflicting requests
+        PersistedRequests.bulkRemove(conflictingRequests);
+
+        // Allow the new request to perform any additional cleanup for a cancelled request
+        if (handleConflictingRequest) {
+            for (const conflictingRequest of conflictingRequests) {
+                handleConflictingRequest(conflictingRequest);
+            }
+        }
+    }
+
+    // Don't try to serialize conflict resolution functions
+    delete request.getConflictingRequests;
+    delete request.handleConflictingRequest;
+    delete request.shouldSkipThisRequestOnConflict;
+
     // Add request to Persisted Requests so that it can be retried if it fails
-    PersistedRequests.save(request);
+    if (!(hasConflict && (shouldSkipThisRequestOnConflict?.() ?? false))) {
+        PersistedRequests.save(request);
+    }
 
     // If we are offline we don't need to trigger the queue to empty as it will happen when we come back online
     if (NetworkStore.isOffline()) {
@@ -210,10 +240,10 @@ function push(request: OnyxRequest) {
 }
 
 function getCurrentRequest(): Promise<void> {
-    if (currentRequest === null) {
+    if (currentRequestPromise === null) {
         return Promise.resolve();
     }
-    return currentRequest;
+    return currentRequestPromise;
 }
 
 /**

src/libs/actions/PersistedRequests.ts

@@ -47,6 +47,15 @@ function remove(requestToRemove: Request) {
     });
 }
 
+function bulkRemove(requestsToRemove: Request[]) {
+    if (requestsToRemove.length === 0) {
+        return;
+    }
+    const requests = persistedRequests.filter((request) => !requestsToRemove.includes(request));
+    persistedRequests = requests;
+    Onyx.set(ONYXKEYS.PERSISTED_REQUESTS, requests);
+}
+
 function update(oldRequestIndex: number, newRequest: Request) {
     const requests = [...persistedRequests];
     requests.splice(oldRequestIndex, 1, newRequest);
@@ -58,4 +67,4 @@ function getAll(): Request[] {
     return persistedRequests;
 }
 
-export {clear, save, getAll, remove, update, getLength};
+export {clear, save, getAll, remove, update, getLength, bulkRemove};

src/libs/actions/Report.ts

@@ -1505,7 +1505,24 @@ function deleteReportComment(reportID: string, reportAction: ReportAction) {
 
     CachedPDFPaths.clearByKey(reportActionID);
 
-    API.write(WRITE_COMMANDS.DELETE_COMMENT, parameters, {optimisticData, successData, failureData});
+    let shouldRequestHappen = true;
+    API.write(
+        WRITE_COMMANDS.DELETE_COMMENT,
+        parameters,
+        {optimisticData, successData, failureData},
+        {
+            getConflictingRequests: (persistedRequests) => {
+                const conflictingCommands = (
+                    isDeletedParentAction ? [WRITE_COMMANDS.UPDATE_COMMENT] : [WRITE_COMMANDS.ADD_COMMENT, WRITE_COMMANDS.ADD_ATTACHMENT, WRITE_COMMANDS.UPDATE_COMMENT]
+                ) as string[];
+                const conflictingRequests = persistedRequests.filter((request) => conflictingCommands.includes(request.command) && request.data?.reportActionID === reportActionID);
+                shouldRequestHappen = conflictingRequests.some((request) => request.command !== WRITE_COMMANDS.UPDATE_COMMENT);
+                return conflictingRequests;
+            },
+            handleConflictingRequest: () => Onyx.update(successData),
+            shouldSkipThisRequestOnConflict: () => !shouldRequestHappen,
+        },
+    );
 
     // if we are linking to the report action, and we are deleting it, and it's not a deleted parent action,
     // we should navigate to its report in order to not show not found page

src/types/onyx/Request.ts

@@ -55,8 +55,44 @@ type RequestData = {
     shouldSkipWebProxy?: boolean;
 };
 
+/**
+ * An object that describes how a new write request can identify any queued requests that may conflict with or be undone by the new request,
+ * whether the new request should be sent in the event of the new conflict, and what to do with the conflicting requests.
+ */
+type RequestConflictResolver = {
+    /**
+     * A callback that's provided with all the currently serialized functions in the sequential queue.
+     * Should return a subset of the requests passed in that conflict with the new request.
+     * Any conflicting requests will be cancelled and removed from the queue.
+     *
+     * @example - In ReconnectApp, you'd only want to have one instance of that command serialized to run on reconnect. The callback for that might look like this:
+     * (persistedRequests) => persistedRequests.filter((request) => request.command === 'ReconnectApp')
+     * */
+    getConflictingRequests?: (persistedRequests: Request[]) => Request[];
+
+    /**
+     * If other conflicting requests are found, should the new request still happen?
+     * This is useful if the new request and an existing request cancel each other out completely.
+     *
+     * @example - In DeleteComment, if you're deleting an optimistic comment, you'd want to cancel the optimistic AddComment call AND the DeleteComment call.
+     *            In this case you'd want shouldSkipThisRequestOnConflict to return true
+     *
+     * @example - If multiple OpenReport commands are queued for the same report offline (and none of them are creating the report optimistically),
+     *            then you'd want to cancel the other OpenReport commands and keep only the last one.
+     *            In this case, you'd want shouldSkipThisRequestOnConflict to return false
+     * */
+    shouldSkipThisRequestOnConflict?: () => boolean;
+
+    /**
+     * Callback to handle a single conflicting request.
+     * This is useful if you need to clean up some optimistic data for a request that was queue but never sent.
+     * In these cases the optimisticData will be applied immediately, but the successData, failureData, and/or finallyData will never be applied unless you do it manually in this callback.
+     */
+    handleConflictingRequest?: (persistedRequest: Request) => unknown;
+};
+
 /** Model of requests sent to the API */
-type Request = RequestData & OnyxData;
+type Request = RequestData & OnyxData & RequestConflictResolver;
 
 /**
  * An object used to describe how a request can be paginated.
@@ -85,4 +121,4 @@ type PaginatedRequest = Request &
     };
 
 export default Request;
-export type {OnyxData, RequestType, PaginationConfig, PaginatedRequest};
+export type {OnyxData, RequestType, PaginationConfig, PaginatedRequest, RequestConflictResolver};

tests/unit/PersistedRequests.ts

@@ -7,16 +7,15 @@ const request: Request = {
     failureData: [{key: 'reportMetadata_2', onyxMethod: 'merge', value: {}}],
 };
 
-beforeEach(() => {
-    PersistedRequests.clear();
-    PersistedRequests.save(request);
-});
+describe('PersistedRequests', () => {
+    beforeEach(() => {
+        PersistedRequests.save(request);
+    });
 
-afterEach(() => {
-    PersistedRequests.clear();
-});
+    afterEach(() => {
+        PersistedRequests.clear();
+    });
 
-describe('PersistedRequests', () => {
     it('save a request without conflicts', () => {
         PersistedRequests.save(request);
         expect(PersistedRequests.getAll().length).toBe(2);
@@ -26,4 +25,12 @@ describe('PersistedRequests', () => {
         PersistedRequests.remove(request);
         expect(PersistedRequests.getAll().length).toBe(0);
     });
+
+    it('bulk remove requests from the PersistedRequests array', () => {
+        const newRequest: Request = {...request};
+        PersistedRequests.save(newRequest);
+        expect(PersistedRequests.getAll().length).toBe(2);
+        PersistedRequests.bulkRemove([request, newRequest]);
+        expect(PersistedRequests.getAll().length).toBe(0);
+    });
 });

tests/unit/SequentialQueueTest.ts

@@ -0,0 +1,102 @@
+import Onyx from 'react-native-onyx';
+import * as PersistedRequests from '@userActions/PersistedRequests';
+import ONYXKEYS from '@src/ONYXKEYS';
+import * as SequentialQueue from '../../src/libs/Network/SequentialQueue';
+import type Request from '../../src/types/onyx/Request';
+import * as TestHelper from '../utils/TestHelper';
+import waitForBatchedUpdates from '../utils/waitForBatchedUpdates';
+
+const request: Request = {
+    command: 'OpenReport',
+    successData: [{key: 'reportMetadata_1', onyxMethod: 'merge', value: {}}],
+    failureData: [{key: 'reportMetadata_2', onyxMethod: 'merge', value: {}}],
+};
+
+describe('SequentialQueue', () => {
+    beforeAll(() => {
+        Onyx.init({
+            keys: ONYXKEYS,
+        });
+    });
+
+    beforeEach(() => {
+        PersistedRequests.save(request);
+
+        // @ts-expect-error TODO: Remove this once TestHelper (https://github.com/Expensify/App/issues/25318) is migrated to TypeScript.
+        global.fetch = TestHelper.getGlobalFetchMock();
+    });
+
+    afterEach(async () => {
+        await PersistedRequests.clear();
+        await Onyx.clear();
+        await waitForBatchedUpdates();
+    });
+
+    it('saves a request without conflicts', () => {
+        SequentialQueue.push(request);
+        expect(PersistedRequests.getAll().length).toBe(2);
+    });
+
+    it('save a new request with conflict resolution', () => {
+        expect(PersistedRequests.getAll().length).toBe(1);
+        const handleConflictingRequest = jest.fn();
+        const newRequest = {
+            command: 'ThingA',
+            getConflictingRequests: (requests: Request[]) => requests,
+            handleConflictingRequest,
+        };
+        SequentialQueue.push(newRequest);
+        expect(PersistedRequests.getAll().length).toBe(1);
+        expect(handleConflictingRequest).toHaveBeenCalledWith(request);
+        expect(handleConflictingRequest).toHaveBeenCalledTimes(1);
+    });
+
+    it('save a new request with conflict resolution and cancelling out new request', () => {
+        expect(PersistedRequests.getAll().length).toBe(1);
+        const newRequest = {
+            command: 'ThingA',
+            getConflictingRequests: (requests: Request[]) => requests,
+            shouldSkipThisRequestOnConflict: () => true,
+        };
+        SequentialQueue.push(newRequest);
+        expect(PersistedRequests.getAll().length).toBe(0);
+    });
+
+    it('a request should never conflict with itself if there are no other queued requests', () => {
+        PersistedRequests.remove(request);
+        expect(PersistedRequests.getAll().length).toBe(0);
+
+        const newRequest: Request = {...request, getConflictingRequests: (requests: Request[]) => requests};
+        SequentialQueue.push(newRequest);
+        expect(PersistedRequests.getAll().length).toBe(1);
+
+        PersistedRequests.remove(newRequest);
+        expect(PersistedRequests.getAll().length).toBe(0);
+
+        SequentialQueue.push({...request, getConflictingRequests: (requests: Request[]) => requests, shouldSkipThisRequestOnConflict: () => true});
+        expect(PersistedRequests.getAll().length).toBe(1);
+    });
+
+    it('should always ignore any requests that have already been sent', async () => {
+        expect(PersistedRequests.getAll().length).toBe(1);
+        SequentialQueue.flush();
+
+        // Wait for the queue to start processing (this is async because it uses an Onyx.connect callback, but don't wait for all async activity to finish
+        // We want to test the case where we try to synchronously add a request to the queue while there's another one still processing
+        // eslint-disable-next-line @typescript-eslint/unbound-method
+        await new Promise(process.nextTick);
+
+        // Ensure the first request is still processing
+        expect(PersistedRequests.getAll().length).toBe(1);
+
+        const newRequest = {
+            command: 'ThingA',
+            getConflictingRequests: (requests: Request[]) => requests,
+            shouldSkipThisRequestOnConflict: () => true,
+        };
+        SequentialQueue.push(newRequest);
+
+        // Verify that we ignored the conflict with any request that's already processing
+        expect(PersistedRequests.getAll().length).toBe(2);
+    });
+});