From dc873e025d2bae00f3581421e493bae4f2e9c2e2 Mon Sep 17 00:00:00 2001 From: rory Date: Tue, 26 Mar 2024 15:23:32 -0700 Subject: [PATCH 01/32] Create generic network layer logic for conflicting write requests --- src/libs/Network/SequentialQueue.ts | 42 +++++++++-------------------- src/types/onyx/Request.ts | 2 ++ 2 files changed, 15 insertions(+), 29 deletions(-) diff --git a/src/libs/Network/SequentialQueue.ts b/src/libs/Network/SequentialQueue.ts index fee8a0b437c7..6dd82355729d 100644 --- a/src/libs/Network/SequentialQueue.ts +++ b/src/libs/Network/SequentialQueue.ts @@ -50,40 +50,24 @@ function flushOnyxUpdatesQueue() { * Identifies and removes conflicting requests from the queue */ function reconcileRequests(persistedRequests: OnyxRequest[], commands: string[]) { - const requestsByActionID: Record = {}; - - // Group requests by reportActionID persistedRequests.forEach((request) => { - const {data} = request; - const reportActionID = data?.reportActionID as string; - if (reportActionID) { - if (!requestsByActionID[reportActionID]) { - requestsByActionID[reportActionID] = []; - } - requestsByActionID[reportActionID].push(request); + const {getConflictingRequests, handleConflictingRequest} = request; + if (!getConflictingRequests || !handleConflictingRequest) { + return; } - }); - // Process requests with conflicting actions - Object.values(requestsByActionID).forEach((requests) => { - const conflictingRequests: OnyxRequest[] = []; - commands.forEach((command) => { - const conflictingRequest = requests.find((request) => request.command === command); - if (conflictingRequest) { - conflictingRequests.push(conflictingRequest); + // Identify conflicting requests according to logic bound to the request + const conflictingRequests = getConflictingRequests(persistedRequests); + conflictingRequests.forEach((conflictingRequest) => { + // delete the conflicting request + const index = persistedRequests.findIndex((req) => req === conflictingRequest); + if (index !== -1) { + persistedRequests.splice(index, 1); } - }); - if (conflictingRequests.length > 1) { - // Remove all requests as they cancel each other out - conflictingRequests.forEach((request) => { - // Perform action: Remove request from persisted requests - const index = persistedRequests.findIndex((req) => req === request); - if (index !== -1) { - persistedRequests.splice(index, 1); - } - }); - } + // Allow the request to perform any additional cleanup for a cancelled request + handleConflictingRequest(conflictingRequest); + }); }); } diff --git a/src/types/onyx/Request.ts b/src/types/onyx/Request.ts index 03d44b1ce94c..d3d3a79a9c8c 100644 --- a/src/types/onyx/Request.ts +++ b/src/types/onyx/Request.ts @@ -20,6 +20,8 @@ type RequestData = { failureData?: OnyxUpdate[]; finallyData?: OnyxUpdate[]; idempotencyKey?: string; + getConflictingRequests?: (persistedRequests: Request[]) => Request[]; + handleConflictingRequest?: (persistedRequest: Request) => void; resolve?: (value: Response) => void; reject?: (value?: unknown) => void; From b78f4cdb8be45e0e73b7d6b110a2840a580a02b2 Mon Sep 17 00:00:00 2001 From: rory Date: Wed, 27 Mar 2024 10:26:22 -0700 Subject: [PATCH 02/32] Use RequireAllOrNone for the type --- src/types/onyx/Request.ts | 35 ++++++++++++++++++++--------------- 1 file changed, 20 insertions(+), 15 deletions(-) diff --git a/src/types/onyx/Request.ts b/src/types/onyx/Request.ts index d3d3a79a9c8c..e21502fbbd1e 100644 --- a/src/types/onyx/Request.ts +++ b/src/types/onyx/Request.ts @@ -1,4 +1,5 @@ import type {OnyxUpdate} from 'react-native-onyx'; +import type {RequireAllOrNone} from 'type-fest'; import type Response from './Response'; type OnyxData = { @@ -10,22 +11,26 @@ type OnyxData = { type RequestType = 'get' | 'post'; -type RequestData = { - command: string; - commandName?: string; - data?: Record; - type?: RequestType; - shouldUseSecure?: boolean; - successData?: OnyxUpdate[]; - failureData?: OnyxUpdate[]; - finallyData?: OnyxUpdate[]; - idempotencyKey?: string; - getConflictingRequests?: (persistedRequests: Request[]) => Request[]; - handleConflictingRequest?: (persistedRequest: Request) => void; +type RequestData = RequireAllOrNone< + { + command: string; + commandName?: string; + data?: Record; + type?: RequestType; + shouldUseSecure?: boolean; + successData?: OnyxUpdate[]; + failureData?: OnyxUpdate[]; + finallyData?: OnyxUpdate[]; + idempotencyKey?: string; - resolve?: (value: Response) => void; - reject?: (value?: unknown) => void; -}; + getConflictingRequests: (persistedRequests: Request[]) => Request[]; + handleConflictingRequest: (persistedRequest: Request) => void; + + resolve?: (value: Response) => void; + reject?: (value?: unknown) => void; + }, + 'getConflictingRequests' | 'handleConflictingRequest' +>; type Request = RequestData & OnyxData; From dc8be360d0fb9f036915486f7d31dd1afebe81fd Mon Sep 17 00:00:00 2001 From: rory Date: Wed, 27 Mar 2024 10:37:36 -0700 Subject: [PATCH 03/32] Remove unused commands param --- src/libs/Network/SequentialQueue.ts | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/libs/Network/SequentialQueue.ts b/src/libs/Network/SequentialQueue.ts index 6dd82355729d..6de5af329e49 100644 --- a/src/libs/Network/SequentialQueue.ts +++ b/src/libs/Network/SequentialQueue.ts @@ -1,6 +1,5 @@ import Onyx from 'react-native-onyx'; import * as ActiveClientManager from '@libs/ActiveClientManager'; -import {WRITE_COMMANDS} from '@libs/API/types'; import * as Request from '@libs/Request'; import * as RequestThrottle from '@libs/RequestThrottle'; import * as PersistedRequests from '@userActions/PersistedRequests'; @@ -49,7 +48,7 @@ function flushOnyxUpdatesQueue() { /** * Identifies and removes conflicting requests from the queue */ -function reconcileRequests(persistedRequests: OnyxRequest[], commands: string[]) { +function reconcileRequests(persistedRequests: OnyxRequest[]) { persistedRequests.forEach((request) => { const {getConflictingRequests, handleConflictingRequest} = request; if (!getConflictingRequests || !handleConflictingRequest) { @@ -91,8 +90,7 @@ function process(): Promise { } // Remove conflicting requests from the queue to avoid processing them - const commands = [WRITE_COMMANDS.ADD_COMMENT, WRITE_COMMANDS.DELETE_COMMENT]; - reconcileRequests(persistedRequests, commands); + reconcileRequests(persistedRequests); const requestToProcess = persistedRequests[0]; From c2311a870b55ecdefcd6a8ceb18b67180333c017 Mon Sep 17 00:00:00 2001 From: rory Date: Wed, 27 Mar 2024 11:01:11 -0700 Subject: [PATCH 04/32] Add necessary param to API.write --- src/libs/API/index.ts | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/libs/API/index.ts b/src/libs/API/index.ts index dbbcf790edf0..0e26f3c78b6d 100644 --- a/src/libs/API/index.ts +++ b/src/libs/API/index.ts @@ -1,5 +1,6 @@ import type {OnyxUpdate} from 'react-native-onyx'; import Onyx from 'react-native-onyx'; +import type {RequireAllOrNone} from 'type-fest'; import Log from '@libs/Log'; import * as Middleware from '@libs/Middleware'; import * as SequentialQueue from '@libs/Network/SequentialQueue'; @@ -51,8 +52,14 @@ type OnyxData = { * @param [onyxData.successData] - Onyx instructions that will be passed to Onyx.update() when the response has jsonCode === 200. * @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. + * @param [conflictResolver] - callbacks used in special cases to detect and handle conflicting requests in the sequential queue */ -function write(command: TCommand, apiCommandParameters: ApiRequestCommandParameters[TCommand], onyxData: OnyxData = {}) { +function write( + command: TCommand, + apiCommandParameters: ApiRequestCommandParameters[TCommand], + onyxData: OnyxData = {}, + conflictResolver: RequireAllOrNone, 'getConflictingRequests' | 'handleConflictingRequest'> = {}, +) { Log.info('Called API write', false, {command, ...apiCommandParameters}); const {optimisticData, ...onyxDataWithoutOptimisticData} = onyxData; @@ -83,6 +90,7 @@ function write(command: TCommand, apiCommandParam canCancel: true, }, ...onyxDataWithoutOptimisticData, + ...conflictResolver, }; // Write commands can be saved and retried, so push it to the SequentialQueue From 93df0af941de5feecf021141e252a3e8ffdff206 Mon Sep 17 00:00:00 2001 From: rory Date: Wed, 27 Mar 2024 11:45:31 -0700 Subject: [PATCH 05/32] DRY up types a bit --- src/libs/API/index.ts | 3 ++- src/types/onyx/Request.ts | 43 ++++++++++++++++++++++----------------- 2 files changed, 26 insertions(+), 20 deletions(-) diff --git a/src/libs/API/index.ts b/src/libs/API/index.ts index 0e26f3c78b6d..82150b16f758 100644 --- a/src/libs/API/index.ts +++ b/src/libs/API/index.ts @@ -8,6 +8,7 @@ import * as Pusher from '@libs/Pusher/pusher'; import * as Request from '@libs/Request'; import CONST from '@src/CONST'; import type OnyxRequest from '@src/types/onyx/Request'; +import type {RequestConflictResolver} from '@src/types/onyx/Request'; import type Response from '@src/types/onyx/Response'; import pkg from '../../../package.json'; import type {ApiRequest, ApiRequestCommandParameters, ReadCommand, SideEffectRequestCommand, WriteCommand} from './types'; @@ -58,7 +59,7 @@ function write( command: TCommand, apiCommandParameters: ApiRequestCommandParameters[TCommand], onyxData: OnyxData = {}, - conflictResolver: RequireAllOrNone, 'getConflictingRequests' | 'handleConflictingRequest'> = {}, + conflictResolver: RequireAllOrNone = {}, ) { Log.info('Called API write', false, {command, ...apiCommandParameters}); const {optimisticData, ...onyxDataWithoutOptimisticData} = onyxData; diff --git a/src/types/onyx/Request.ts b/src/types/onyx/Request.ts index e21502fbbd1e..f500d53df5d5 100644 --- a/src/types/onyx/Request.ts +++ b/src/types/onyx/Request.ts @@ -1,5 +1,5 @@ import type {OnyxUpdate} from 'react-native-onyx'; -import type {RequireAllOrNone} from 'type-fest'; +import type {RequireAllOrNone, Spread} from 'type-fest'; import type Response from './Response'; type OnyxData = { @@ -9,30 +9,35 @@ type OnyxData = { optimisticData?: OnyxUpdate[]; }; +type RequestConflictResolver = { + getConflictingRequests: (persistedRequests: Request[]) => Request[]; + handleConflictingRequest: (persistedRequest: Request) => void; +}; + type RequestType = 'get' | 'post'; type RequestData = RequireAllOrNone< - { - command: string; - commandName?: string; - data?: Record; - type?: RequestType; - shouldUseSecure?: boolean; - successData?: OnyxUpdate[]; - failureData?: OnyxUpdate[]; - finallyData?: OnyxUpdate[]; - idempotencyKey?: string; - - getConflictingRequests: (persistedRequests: Request[]) => Request[]; - handleConflictingRequest: (persistedRequest: Request) => void; + Spread< + { + command: string; + commandName?: string; + data?: Record; + type?: RequestType; + shouldUseSecure?: boolean; + successData?: OnyxUpdate[]; + failureData?: OnyxUpdate[]; + finallyData?: OnyxUpdate[]; + idempotencyKey?: string; - resolve?: (value: Response) => void; - reject?: (value?: unknown) => void; - }, - 'getConflictingRequests' | 'handleConflictingRequest' + resolve?: (value: Response) => void; + reject?: (value?: unknown) => void; + }, + RequestConflictResolver + >, + keyof RequestConflictResolver >; type Request = RequestData & OnyxData; export default Request; -export type {OnyxData, RequestType}; +export type {OnyxData, RequestType, RequestConflictResolver}; From 6192cc482ae06096161aa944c089b8ab9c457fa1 Mon Sep 17 00:00:00 2001 From: rory Date: Wed, 27 Mar 2024 11:52:17 -0700 Subject: [PATCH 06/32] Make return type of handleConflictingRequest unknown --- src/types/onyx/Request.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/types/onyx/Request.ts b/src/types/onyx/Request.ts index f500d53df5d5..7aaae5afbb6d 100644 --- a/src/types/onyx/Request.ts +++ b/src/types/onyx/Request.ts @@ -11,7 +11,7 @@ type OnyxData = { type RequestConflictResolver = { getConflictingRequests: (persistedRequests: Request[]) => Request[]; - handleConflictingRequest: (persistedRequest: Request) => void; + handleConflictingRequest: (persistedRequest: Request) => unknown; }; type RequestType = 'get' | 'post'; From 20629dd2b82b81cdb219703215ac4dee90b9201c Mon Sep 17 00:00:00 2001 From: rory Date: Wed, 27 Mar 2024 11:52:35 -0700 Subject: [PATCH 07/32] Implement conflict resolution logic for deleteReportComment --- src/libs/actions/Report.ts | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/src/libs/actions/Report.ts b/src/libs/actions/Report.ts index 93154bfff16b..f6c7001daa9f 100644 --- a/src/libs/actions/Report.ts +++ b/src/libs/actions/Report.ts @@ -1238,7 +1238,20 @@ function deleteReportComment(reportID: string, reportAction: ReportAction) { }; CachedPDFPaths.clearByKey(reportActionID); - API.write(WRITE_COMMANDS.DELETE_COMMENT, parameters, {optimisticData, successData, failureData}); + API.write( + WRITE_COMMANDS.DELETE_COMMENT, + parameters, + {optimisticData, successData, failureData}, + { + getConflictingRequests: (persistedRequests) => + persistedRequests.filter( + (request) => + [WRITE_COMMANDS.ADD_COMMENT, WRITE_COMMANDS.ADD_ATTACHMENT, WRITE_COMMANDS.DELETE_COMMENT, WRITE_COMMANDS.UPDATE_COMMENT].includes(request.command) && + request.data?.reportActionID === reportActionID, + ), + handleConflictingRequest: () => Onyx.update(successData), + }, + ); } /** From f9d7395b09714bd63d2825e2998ce2c63cd553e5 Mon Sep 17 00:00:00 2001 From: rory Date: Wed, 27 Mar 2024 13:00:41 -0700 Subject: [PATCH 08/32] Fix TS error --- src/libs/actions/Report.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libs/actions/Report.ts b/src/libs/actions/Report.ts index f6c7001daa9f..8dc4bcf76633 100644 --- a/src/libs/actions/Report.ts +++ b/src/libs/actions/Report.ts @@ -1246,7 +1246,7 @@ function deleteReportComment(reportID: string, reportAction: ReportAction) { getConflictingRequests: (persistedRequests) => persistedRequests.filter( (request) => - [WRITE_COMMANDS.ADD_COMMENT, WRITE_COMMANDS.ADD_ATTACHMENT, WRITE_COMMANDS.DELETE_COMMENT, WRITE_COMMANDS.UPDATE_COMMENT].includes(request.command) && + ([WRITE_COMMANDS.ADD_COMMENT, WRITE_COMMANDS.ADD_ATTACHMENT, WRITE_COMMANDS.DELETE_COMMENT, WRITE_COMMANDS.UPDATE_COMMENT] as string[]).includes(request.command) && request.data?.reportActionID === reportActionID, ), handleConflictingRequest: () => Onyx.update(successData), From 4d4117ae10be4f41fa2ace5d0035d32e75de37bf Mon Sep 17 00:00:00 2001 From: rory Date: Wed, 27 Mar 2024 13:12:38 -0700 Subject: [PATCH 09/32] Move reconcileRequests from process to push --- src/libs/Network/SequentialQueue.ts | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/libs/Network/SequentialQueue.ts b/src/libs/Network/SequentialQueue.ts index 6de5af329e49..d55f99df145f 100644 --- a/src/libs/Network/SequentialQueue.ts +++ b/src/libs/Network/SequentialQueue.ts @@ -48,8 +48,9 @@ function flushOnyxUpdatesQueue() { /** * Identifies and removes conflicting requests from the queue */ -function reconcileRequests(persistedRequests: OnyxRequest[]) { - persistedRequests.forEach((request) => { +function reconcileRequests(newRequest: OnyxRequest) { + const persistedRequests = PersistedRequests.getAll(); + [...persistedRequests, newRequest].reverse().forEach((request) => { const {getConflictingRequests, handleConflictingRequest} = request; if (!getConflictingRequests || !handleConflictingRequest) { return; @@ -89,9 +90,6 @@ function process(): Promise { return Promise.resolve(); } - // Remove conflicting requests from the queue to avoid processing them - reconcileRequests(persistedRequests); - 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. @@ -188,6 +186,9 @@ function isRunning(): boolean { NetworkStore.onReconnection(flush); function push(request: OnyxRequest) { + // Remove conflicting requests from the queue to avoid processing them + reconcileRequests(request); + // Add request to Persisted Requests so that it can be retried if it fails PersistedRequests.save(request); From 6e1895a5e8eb1b84bcac047ed6cb484f7beb06de Mon Sep 17 00:00:00 2001 From: rory Date: Wed, 27 Mar 2024 17:06:33 -0700 Subject: [PATCH 10/32] Move reconcileRequests down a layer into PersistedRequests.save --- src/libs/Network/SequentialQueue.ts | 29 ------------------- src/libs/actions/PersistedRequests.ts | 41 +++++++++++++++++++++------ 2 files changed, 32 insertions(+), 38 deletions(-) diff --git a/src/libs/Network/SequentialQueue.ts b/src/libs/Network/SequentialQueue.ts index d55f99df145f..38b0549b28bc 100644 --- a/src/libs/Network/SequentialQueue.ts +++ b/src/libs/Network/SequentialQueue.ts @@ -45,32 +45,6 @@ function flushOnyxUpdatesQueue() { QueuedOnyxUpdates.flushQueue(); } -/** - * Identifies and removes conflicting requests from the queue - */ -function reconcileRequests(newRequest: OnyxRequest) { - const persistedRequests = PersistedRequests.getAll(); - [...persistedRequests, newRequest].reverse().forEach((request) => { - const {getConflictingRequests, handleConflictingRequest} = request; - if (!getConflictingRequests || !handleConflictingRequest) { - return; - } - - // Identify conflicting requests according to logic bound to the request - const conflictingRequests = getConflictingRequests(persistedRequests); - conflictingRequests.forEach((conflictingRequest) => { - // delete the conflicting request - const index = persistedRequests.findIndex((req) => req === conflictingRequest); - if (index !== -1) { - persistedRequests.splice(index, 1); - } - - // Allow the request to perform any additional cleanup for a cancelled request - handleConflictingRequest(conflictingRequest); - }); - }); -} - /** * Process any persisted requests, when online, one at a time until the queue is empty. * @@ -186,9 +160,6 @@ function isRunning(): boolean { NetworkStore.onReconnection(flush); function push(request: OnyxRequest) { - // Remove conflicting requests from the queue to avoid processing them - reconcileRequests(request); - // Add request to Persisted Requests so that it can be retried if it fails PersistedRequests.save(request); diff --git a/src/libs/actions/PersistedRequests.ts b/src/libs/actions/PersistedRequests.ts index fcc6450c6072..2cef6fe1e42a 100644 --- a/src/libs/actions/PersistedRequests.ts +++ b/src/libs/actions/PersistedRequests.ts @@ -18,17 +18,40 @@ function clear() { } function save(requestToPersist: Request) { - const requests = [...persistedRequests]; - const existingRequestIndex = requests.findIndex((request) => request.data?.idempotencyKey && request.data?.idempotencyKey === requestToPersist.data?.idempotencyKey); - if (existingRequestIndex > -1) { - // Merge the new request into the existing one, keeping its place in the queue - requests.splice(existingRequestIndex, 1, requestToPersist); - } else { - // If not, push the new request to the end of the queue - requests.push(requestToPersist); + const requests = [...persistedRequests, requestToPersist]; + for (let i = requests.length - 1; i > 0; i--) { + // since we're deleting elements from the array, it may be reindexed. + // In this case, we need to make sure our index is still in bounds + if (i >= requests.length) { + i--; + // eslint-disable-next-line no-continue + continue; + } + + const request = requests[i]; + + // identify and handle any existing requests that conflict with the new one + const {getConflictingRequests, handleConflictingRequest} = request; + if (!getConflictingRequests || !handleConflictingRequest) { + // eslint-disable-next-line no-continue + continue; + } + + // Identify conflicting requests according to logic bound to the request + const conflictingRequests = getConflictingRequests(requests); + conflictingRequests.forEach((conflictingRequest) => { + // delete the conflicting request + const index = requests.findIndex((req) => req === conflictingRequest); + if (index !== -1) { + requests.splice(index, 1); + } + + // Allow the request to perform any additional cleanup for a cancelled request + handleConflictingRequest(conflictingRequest); + }); } - persistedRequests = requests; + // Save the updated set of requests Onyx.set(ONYXKEYS.PERSISTED_REQUESTS, requests); } From 100bd60b2923c23f3bf047089999cbdce619f188 Mon Sep 17 00:00:00 2001 From: rory Date: Wed, 27 Mar 2024 17:27:44 -0700 Subject: [PATCH 11/32] Don't include current request in getConflictingRequests --- src/libs/actions/PersistedRequests.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/libs/actions/PersistedRequests.ts b/src/libs/actions/PersistedRequests.ts index 2cef6fe1e42a..912a3417a83a 100644 --- a/src/libs/actions/PersistedRequests.ts +++ b/src/libs/actions/PersistedRequests.ts @@ -37,8 +37,11 @@ function save(requestToPersist: Request) { continue; } + // Get all the remaining requests, excluding the one we're adding, which will always be at the end of the array + const remainingRequests = requests.slice(0, requests.length - 1); + // Identify conflicting requests according to logic bound to the request - const conflictingRequests = getConflictingRequests(requests); + const conflictingRequests = getConflictingRequests(remainingRequests); conflictingRequests.forEach((conflictingRequest) => { // delete the conflicting request const index = requests.findIndex((req) => req === conflictingRequest); From eb17f3f9ed52136d804d25e57e8d4ae045bc0dcb Mon Sep 17 00:00:00 2001 From: rory Date: Wed, 27 Mar 2024 21:57:17 -0700 Subject: [PATCH 12/32] Dont require both getConflictingRequests and handleConflictingRequest --- src/libs/API/index.ts | 4 +- src/libs/API/parameters/ReconnectAppParams.ts | 1 - src/types/onyx/Request.ts | 43 +++++++------------ 3 files changed, 17 insertions(+), 31 deletions(-) diff --git a/src/libs/API/index.ts b/src/libs/API/index.ts index 82150b16f758..6fec161046a9 100644 --- a/src/libs/API/index.ts +++ b/src/libs/API/index.ts @@ -1,6 +1,5 @@ import type {OnyxUpdate} from 'react-native-onyx'; import Onyx from 'react-native-onyx'; -import type {RequireAllOrNone} from 'type-fest'; import Log from '@libs/Log'; import * as Middleware from '@libs/Middleware'; import * as SequentialQueue from '@libs/Network/SequentialQueue'; @@ -8,7 +7,6 @@ import * as Pusher from '@libs/Pusher/pusher'; import * as Request from '@libs/Request'; import CONST from '@src/CONST'; import type OnyxRequest from '@src/types/onyx/Request'; -import type {RequestConflictResolver} from '@src/types/onyx/Request'; import type Response from '@src/types/onyx/Response'; import pkg from '../../../package.json'; import type {ApiRequest, ApiRequestCommandParameters, ReadCommand, SideEffectRequestCommand, WriteCommand} from './types'; @@ -59,7 +57,7 @@ function write( command: TCommand, apiCommandParameters: ApiRequestCommandParameters[TCommand], onyxData: OnyxData = {}, - conflictResolver: RequireAllOrNone = {}, + conflictResolver: Pick = {}, ) { Log.info('Called API write', false, {command, ...apiCommandParameters}); const {optimisticData, ...onyxDataWithoutOptimisticData} = onyxData; diff --git a/src/libs/API/parameters/ReconnectAppParams.ts b/src/libs/API/parameters/ReconnectAppParams.ts index d8c1da4f0887..8c5b7d6c0da9 100644 --- a/src/libs/API/parameters/ReconnectAppParams.ts +++ b/src/libs/API/parameters/ReconnectAppParams.ts @@ -2,7 +2,6 @@ type ReconnectAppParams = { mostRecentReportActionLastModified?: string; updateIDFrom?: number; policyIDList: string[]; - idempotencyKey?: string; }; export default ReconnectAppParams; diff --git a/src/types/onyx/Request.ts b/src/types/onyx/Request.ts index 7aaae5afbb6d..d85e48b05da7 100644 --- a/src/types/onyx/Request.ts +++ b/src/types/onyx/Request.ts @@ -1,5 +1,4 @@ import type {OnyxUpdate} from 'react-native-onyx'; -import type {RequireAllOrNone, Spread} from 'type-fest'; import type Response from './Response'; type OnyxData = { @@ -9,35 +8,25 @@ type OnyxData = { optimisticData?: OnyxUpdate[]; }; -type RequestConflictResolver = { - getConflictingRequests: (persistedRequests: Request[]) => Request[]; - handleConflictingRequest: (persistedRequest: Request) => unknown; -}; - type RequestType = 'get' | 'post'; -type RequestData = RequireAllOrNone< - Spread< - { - command: string; - commandName?: string; - data?: Record; - type?: RequestType; - shouldUseSecure?: boolean; - successData?: OnyxUpdate[]; - failureData?: OnyxUpdate[]; - finallyData?: OnyxUpdate[]; - idempotencyKey?: string; - - resolve?: (value: Response) => void; - reject?: (value?: unknown) => void; - }, - RequestConflictResolver - >, - keyof RequestConflictResolver ->; +type RequestData = { + command: string; + commandName?: string; + data?: Record; + type?: RequestType; + shouldUseSecure?: boolean; + successData?: OnyxUpdate[]; + failureData?: OnyxUpdate[]; + finallyData?: OnyxUpdate[]; + idempotencyKey?: string; + getConflictingRequests?: (persistedRequests: Request[]) => Request[]; + handleConflictingRequest?: (persistedRequest: Request) => unknown; + resolve?: (value: Response) => void; + reject?: (value?: unknown) => void; +}; type Request = RequestData & OnyxData; export default Request; -export type {OnyxData, RequestType, RequestConflictResolver}; +export type {OnyxData, RequestType}; From 709fbcff801d253474996cdade911c9a35f58dda Mon Sep 17 00:00:00 2001 From: rory Date: Wed, 27 Mar 2024 21:58:35 -0700 Subject: [PATCH 13/32] Implement conflict resolution for reconnectApp --- src/libs/actions/App.ts | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/libs/actions/App.ts b/src/libs/actions/App.ts index 302e4048d0e8..3a9ab4c52b3c 100644 --- a/src/libs/actions/App.ts +++ b/src/libs/actions/App.ts @@ -222,10 +222,7 @@ function openApp() { function reconnectApp(updateIDFrom: OnyxEntry = 0) { console.debug(`[OnyxUpdates] App reconnecting with updateIDFrom: ${updateIDFrom}`); getPolicyParamsForOpenOrReconnect().then((policyParams) => { - const params: ReconnectAppParams = { - ...policyParams, - idempotencyKey: `${WRITE_COMMANDS.RECONNECT_APP}`, - }; + const params: ReconnectAppParams = policyParams; // When the app reconnects we do a fast "sync" of the LHN and only return chats that have new messages. We achieve this by sending the most recent reportActionID. // we have locally. And then only update the user about chats with messages that have occurred after that reportActionID. @@ -242,7 +239,9 @@ function reconnectApp(updateIDFrom: OnyxEntry = 0) { params.updateIDFrom = updateIDFrom; } - API.write(WRITE_COMMANDS.RECONNECT_APP, params, getOnyxDataForOpenOrReconnect()); + API.write(WRITE_COMMANDS.RECONNECT_APP, params, getOnyxDataForOpenOrReconnect(), { + getConflictingRequests: (persistedRequests) => persistedRequests.filter((request) => request?.command === WRITE_COMMANDS.RECONNECT_APP), + }); }); } From 37ce6133373c468d146ef5ecd9027a5db7d7b37d Mon Sep 17 00:00:00 2001 From: rory Date: Wed, 27 Mar 2024 22:27:58 -0700 Subject: [PATCH 14/32] Implement conflict resolution for OpenReport --- src/libs/API/parameters/OpenReportParams.ts | 1 - src/libs/actions/Report.ts | 17 ++++++++++++++--- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/src/libs/API/parameters/OpenReportParams.ts b/src/libs/API/parameters/OpenReportParams.ts index 8eaed6bc0fde..dbaf6c07444c 100644 --- a/src/libs/API/parameters/OpenReportParams.ts +++ b/src/libs/API/parameters/OpenReportParams.ts @@ -7,7 +7,6 @@ type OpenReportParams = { shouldRetry?: boolean; createdReportActionID?: string; clientLastReadTime?: string; - idempotencyKey?: string; }; export default OpenReportParams; diff --git a/src/libs/actions/Report.ts b/src/libs/actions/Report.ts index 8dc4bcf76633..69455bff42db 100644 --- a/src/libs/actions/Report.ts +++ b/src/libs/actions/Report.ts @@ -612,7 +612,6 @@ function openReport( emailList: participantLoginList ? participantLoginList.join(',') : '', accountIDList: participantAccountIDList ? participantAccountIDList.join(',') : '', parentReportActionID, - idempotencyKey: `${SIDE_EFFECT_REQUEST_COMMANDS.OPEN_REPORT}_${reportID}`, }; if (isFromDeepLink) { @@ -711,7 +710,6 @@ function openReport( // Add the createdReportActionID parameter to the API call parameters.createdReportActionID = optimisticCreatedAction.reportActionID; - parameters.idempotencyKey = `${parameters.idempotencyKey}_NewReport_${optimisticCreatedAction.reportActionID}`; // If we are creating a thread, ensure the report action has childReportID property added if (newReportObject.parentReportID && parentReportActionID) { @@ -737,7 +735,20 @@ function openReport( }); } else { // eslint-disable-next-line rulesdir/no-multiple-api-calls - API.write(WRITE_COMMANDS.OPEN_REPORT, parameters, {optimisticData, successData, failureData}); + API.write( + WRITE_COMMANDS.OPEN_REPORT, + parameters, + {optimisticData, successData, failureData}, + { + getConflictingRequests: (persistedRequests) => + persistedRequests.filter((request) => { + if (request.command !== WRITE_COMMANDS.OPEN_REPORT) { + return true; + } + return isEmptyObject(newReportObject) ? request.data?.reportID === reportID : false; + }), + }, + ); } } From f16e9b7d0ee9076434f549faf8eff284d2910eec Mon Sep 17 00:00:00 2001 From: rory Date: Wed, 27 Mar 2024 22:31:41 -0700 Subject: [PATCH 15/32] Remove idempotencyKey from RequestData --- src/libs/Network/enhanceParameters.ts | 3 --- src/types/onyx/Request.ts | 1 - 2 files changed, 4 deletions(-) diff --git a/src/libs/Network/enhanceParameters.ts b/src/libs/Network/enhanceParameters.ts index b8b8993a52b0..5c4590baded3 100644 --- a/src/libs/Network/enhanceParameters.ts +++ b/src/libs/Network/enhanceParameters.ts @@ -36,8 +36,5 @@ export default function enhanceParameters(command: string, parameters: Record Request[]; handleConflictingRequest?: (persistedRequest: Request) => unknown; resolve?: (value: Response) => void; From a88c45c5386ffd473b4828b4a5dc7ab69d4bdb4a Mon Sep 17 00:00:00 2001 From: rory Date: Wed, 27 Mar 2024 22:41:19 -0700 Subject: [PATCH 16/32] Update persistedRequests cache before saving to disk --- src/libs/actions/PersistedRequests.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/src/libs/actions/PersistedRequests.ts b/src/libs/actions/PersistedRequests.ts index 912a3417a83a..a672c3828288 100644 --- a/src/libs/actions/PersistedRequests.ts +++ b/src/libs/actions/PersistedRequests.ts @@ -55,6 +55,7 @@ function save(requestToPersist: Request) { } // Save the updated set of requests + persistedRequests = requests; Onyx.set(ONYXKEYS.PERSISTED_REQUESTS, requests); } From 416099e3a41637058f67163403bff4cda97d0db6 Mon Sep 17 00:00:00 2001 From: rory Date: Wed, 27 Mar 2024 22:48:51 -0700 Subject: [PATCH 17/32] separate checks for getConflictingRequests and handleConflictingRequest --- src/libs/actions/PersistedRequests.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libs/actions/PersistedRequests.ts b/src/libs/actions/PersistedRequests.ts index a672c3828288..ce79f1e2b590 100644 --- a/src/libs/actions/PersistedRequests.ts +++ b/src/libs/actions/PersistedRequests.ts @@ -32,7 +32,7 @@ function save(requestToPersist: Request) { // identify and handle any existing requests that conflict with the new one const {getConflictingRequests, handleConflictingRequest} = request; - if (!getConflictingRequests || !handleConflictingRequest) { + if (!getConflictingRequests) { // eslint-disable-next-line no-continue continue; } @@ -50,7 +50,7 @@ function save(requestToPersist: Request) { } // Allow the request to perform any additional cleanup for a cancelled request - handleConflictingRequest(conflictingRequest); + handleConflictingRequest?.(conflictingRequest); }); } From 941bdc1fbea368e74f96586296d38888daa77bac Mon Sep 17 00:00:00 2001 From: rory Date: Wed, 27 Mar 2024 22:49:54 -0700 Subject: [PATCH 18/32] Update unit tests --- tests/unit/PersistedRequests.ts | 39 +++++---------------------------- 1 file changed, 5 insertions(+), 34 deletions(-) diff --git a/tests/unit/PersistedRequests.ts b/tests/unit/PersistedRequests.ts index b1d05c96f12a..a858aa5904ee 100644 --- a/tests/unit/PersistedRequests.ts +++ b/tests/unit/PersistedRequests.ts @@ -3,9 +3,6 @@ import type Request from '../../src/types/onyx/Request'; const request: Request = { command: 'OpenReport', - data: { - idempotencyKey: 'OpenReport_1', - }, successData: [{key: 'reportMetadata_1', onyxMethod: 'merge', value: {}}], failureData: [{key: 'reportMetadata_2', onyxMethod: 'merge', value: {}}], }; @@ -20,44 +17,18 @@ afterEach(() => { }); describe('PersistedRequests', () => { - it('save a new request with an idempotency key which currently exists in the PersistedRequests array', () => { + it('save a request without conflicts', () => { PersistedRequests.save(request); - expect(PersistedRequests.getAll().length).toBe(1); - }); - - it('save a new request with a new idempotency key', () => { - const newRequest = { - command: 'OpenReport', - data: { - idempotencyKey: 'OpenReport_2', - }, - }; - PersistedRequests.save(newRequest); expect(PersistedRequests.getAll().length).toBe(2); }); - it('replace a request existing in the PersistedRequests array with a new one', () => { - const newRequest: Request = { + it('save a new request with conflict resolution', () => { + const newRequest = { command: 'OpenReport', - data: { - idempotencyKey: 'OpenReport_1', - }, - successData: [{key: 'reportMetadata_3', onyxMethod: 'merge', value: {}}], - failureData: [{key: 'reportMetadata_4', onyxMethod: 'merge', value: {}}], + getConflictingRequests: (requests: Request[]) => requests, }; - PersistedRequests.save(newRequest); - - const persistedRequests = PersistedRequests.getAll(); - - expect(persistedRequests.length).toBe(1); - - const mergedRequest = persistedRequests[0]; - - expect(mergedRequest.successData?.length).toBe(1); - expect(mergedRequest.failureData?.length).toBe(1); - expect(mergedRequest.successData?.[0]?.key).toBe('reportMetadata_3'); - expect(mergedRequest.failureData?.[0]?.key).toBe('reportMetadata_4'); + expect(PersistedRequests.getAll().length).toBe(1); }); it('remove a request from the PersistedRequests array', () => { From 60f75bd69ffa72cea6db184ffed6c2988eac9d3e Mon Sep 17 00:00:00 2001 From: rory Date: Wed, 27 Mar 2024 22:53:19 -0700 Subject: [PATCH 19/32] Add assertion to make sure handler is called --- tests/unit/PersistedRequests.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tests/unit/PersistedRequests.ts b/tests/unit/PersistedRequests.ts index a858aa5904ee..08c4901d7dd9 100644 --- a/tests/unit/PersistedRequests.ts +++ b/tests/unit/PersistedRequests.ts @@ -23,12 +23,16 @@ describe('PersistedRequests', () => { }); it('save a new request with conflict resolution', () => { + const handleConflictingRequest = jest.fn(); const newRequest = { - command: 'OpenReport', + command: 'ReconnectApp', getConflictingRequests: (requests: Request[]) => requests, + handleConflictingRequest, }; PersistedRequests.save(newRequest); expect(PersistedRequests.getAll().length).toBe(1); + expect(handleConflictingRequest).toHaveBeenCalledWith(request); + expect(handleConflictingRequest).toHaveBeenCalledTimes(1); }); it('remove a request from the PersistedRequests array', () => { From 49bc4d62804131b139a066bc6d4e1c14984b2b49 Mon Sep 17 00:00:00 2001 From: rory Date: Wed, 27 Mar 2024 23:19:04 -0700 Subject: [PATCH 20/32] Add shouldIncludeCurrentRequest flag --- src/libs/API/index.ts | 3 ++- src/libs/actions/PersistedRequests.ts | 6 +++--- src/types/onyx/Request.ts | 10 ++++++++-- 3 files changed, 13 insertions(+), 6 deletions(-) diff --git a/src/libs/API/index.ts b/src/libs/API/index.ts index 6fec161046a9..7d8cb348afbe 100644 --- a/src/libs/API/index.ts +++ b/src/libs/API/index.ts @@ -7,6 +7,7 @@ import * as Pusher from '@libs/Pusher/pusher'; import * as Request from '@libs/Request'; import CONST from '@src/CONST'; import type OnyxRequest from '@src/types/onyx/Request'; +import type {RequestConflictResolver} from '@src/types/onyx/Request'; import type Response from '@src/types/onyx/Response'; import pkg from '../../../package.json'; import type {ApiRequest, ApiRequestCommandParameters, ReadCommand, SideEffectRequestCommand, WriteCommand} from './types'; @@ -57,7 +58,7 @@ function write( command: TCommand, apiCommandParameters: ApiRequestCommandParameters[TCommand], onyxData: OnyxData = {}, - conflictResolver: Pick = {}, + conflictResolver: RequestConflictResolver = {}, ) { Log.info('Called API write', false, {command, ...apiCommandParameters}); const {optimisticData, ...onyxDataWithoutOptimisticData} = onyxData; diff --git a/src/libs/actions/PersistedRequests.ts b/src/libs/actions/PersistedRequests.ts index ce79f1e2b590..48f2e020fd11 100644 --- a/src/libs/actions/PersistedRequests.ts +++ b/src/libs/actions/PersistedRequests.ts @@ -31,14 +31,14 @@ function save(requestToPersist: Request) { const request = requests[i]; // identify and handle any existing requests that conflict with the new one - const {getConflictingRequests, handleConflictingRequest} = request; + const {getConflictingRequests, handleConflictingRequest, shouldIncludeCurrentRequest} = request; if (!getConflictingRequests) { // eslint-disable-next-line no-continue continue; } - // Get all the remaining requests, excluding the one we're adding, which will always be at the end of the array - const remainingRequests = requests.slice(0, requests.length - 1); + // Get all the remaining requests, potentially excluding the one we're adding, which will always be at the end of the array + const remainingRequests = shouldIncludeCurrentRequest ? requests : requests.slice(0, requests.length - 1); // Identify conflicting requests according to logic bound to the request const conflictingRequests = getConflictingRequests(remainingRequests); diff --git a/src/types/onyx/Request.ts b/src/types/onyx/Request.ts index e7bddf2ded44..9a2d29493810 100644 --- a/src/types/onyx/Request.ts +++ b/src/types/onyx/Request.ts @@ -25,7 +25,13 @@ type RequestData = { reject?: (value?: unknown) => void; }; -type Request = RequestData & OnyxData; +type RequestConflictResolver = { + getConflictingRequests?: (persistedRequests: Request[]) => Request[]; + shouldIncludeCurrentRequest?: boolean; + handleConflictingRequest?: (persistedRequest: Request) => unknown; +}; + +type Request = RequestData & OnyxData & RequestConflictResolver; export default Request; -export type {OnyxData, RequestType}; +export type {OnyxData, RequestType, RequestConflictResolver}; From 5dca71e3e47fd9994a63daf6de2a556bb3a244cc Mon Sep 17 00:00:00 2001 From: rory Date: Wed, 27 Mar 2024 23:24:40 -0700 Subject: [PATCH 21/32] Make sure the DeleteComment request is cancelled when it conflicts with an UpdateComment request --- src/libs/actions/Report.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/src/libs/actions/Report.ts b/src/libs/actions/Report.ts index 69455bff42db..cbf965303791 100644 --- a/src/libs/actions/Report.ts +++ b/src/libs/actions/Report.ts @@ -1261,6 +1261,7 @@ function deleteReportComment(reportID: string, reportAction: ReportAction) { request.data?.reportActionID === reportActionID, ), handleConflictingRequest: () => Onyx.update(successData), + shouldIncludeCurrentRequest: true, }, ); } From 2420552a4cf29ef9e5f1f00f2d7099899bf6c256 Mon Sep 17 00:00:00 2001 From: rory Date: Wed, 27 Mar 2024 23:30:58 -0700 Subject: [PATCH 22/32] Add test for shouldIncludeCurrentRequest --- tests/unit/PersistedRequests.ts | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/tests/unit/PersistedRequests.ts b/tests/unit/PersistedRequests.ts index 08c4901d7dd9..f7b740d5e875 100644 --- a/tests/unit/PersistedRequests.ts +++ b/tests/unit/PersistedRequests.ts @@ -25,11 +25,17 @@ describe('PersistedRequests', () => { it('save a new request with conflict resolution', () => { const handleConflictingRequest = jest.fn(); const newRequest = { - command: 'ReconnectApp', + command: 'ThingA', getConflictingRequests: (requests: Request[]) => requests, handleConflictingRequest, }; + const secondRequest = { + command: 'ThingB', + getConflictingRequests: (requests: Request[]) => requests, + shouldIncludeCurrentRequest: true, + }; PersistedRequests.save(newRequest); + PersistedRequests.save(secondRequest); expect(PersistedRequests.getAll().length).toBe(1); expect(handleConflictingRequest).toHaveBeenCalledWith(request); expect(handleConflictingRequest).toHaveBeenCalledTimes(1); From 4b9120555e6c5c015610d0dc749fdee808a1c3e7 Mon Sep 17 00:00:00 2001 From: rory Date: Thu, 28 Mar 2024 06:51:58 -0700 Subject: [PATCH 23/32] Don't try to serialize callbacks --- src/libs/actions/PersistedRequests.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/libs/actions/PersistedRequests.ts b/src/libs/actions/PersistedRequests.ts index 48f2e020fd11..5dcbf08d8a04 100644 --- a/src/libs/actions/PersistedRequests.ts +++ b/src/libs/actions/PersistedRequests.ts @@ -55,7 +55,12 @@ function save(requestToPersist: Request) { } // Save the updated set of requests - persistedRequests = requests; + persistedRequests = requests.map((request) => { + delete request.getConflictingRequests; + delete request.handleConflictingRequest; + delete request.shouldIncludeCurrentRequest; + return request; + }); Onyx.set(ONYXKEYS.PERSISTED_REQUESTS, requests); } From 0e9bffd97e157f7005199af5984afb452fb6d7a4 Mon Sep 17 00:00:00 2001 From: rory Date: Thu, 28 Mar 2024 07:03:47 -0700 Subject: [PATCH 24/32] Add some documentation --- src/libs/actions/PersistedRequests.ts | 1 + src/types/onyx/Request.ts | 21 +++++++++++++++++++++ 2 files changed, 22 insertions(+) diff --git a/src/libs/actions/PersistedRequests.ts b/src/libs/actions/PersistedRequests.ts index 5dcbf08d8a04..46a49a7ff66d 100644 --- a/src/libs/actions/PersistedRequests.ts +++ b/src/libs/actions/PersistedRequests.ts @@ -56,6 +56,7 @@ function save(requestToPersist: Request) { // Save the updated set of requests persistedRequests = requests.map((request) => { + // Don't try to serialize conflict resolution functions delete request.getConflictingRequests; delete request.handleConflictingRequest; delete request.shouldIncludeCurrentRequest; diff --git a/src/types/onyx/Request.ts b/src/types/onyx/Request.ts index 9a2d29493810..74904a0f7194 100644 --- a/src/types/onyx/Request.ts +++ b/src/types/onyx/Request.ts @@ -26,8 +26,29 @@ type RequestData = { }; 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[]; + + /** + * Should the requests provided to getConflictingRequests include the new request? + * This is useful if the new request and an existing request cancel eachother 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. + * */ shouldIncludeCurrentRequest?: 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; }; From 605397ccffc9967743f1b5de3982df0a3daab156 Mon Sep 17 00:00:00 2001 From: rory Date: Thu, 28 Mar 2024 07:06:14 -0700 Subject: [PATCH 25/32] Don't decrement twice --- src/libs/actions/PersistedRequests.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/src/libs/actions/PersistedRequests.ts b/src/libs/actions/PersistedRequests.ts index 46a49a7ff66d..92031c145268 100644 --- a/src/libs/actions/PersistedRequests.ts +++ b/src/libs/actions/PersistedRequests.ts @@ -23,7 +23,6 @@ function save(requestToPersist: Request) { // since we're deleting elements from the array, it may be reindexed. // In this case, we need to make sure our index is still in bounds if (i >= requests.length) { - i--; // eslint-disable-next-line no-continue continue; } From 4e22825a4f845c0c320368861f7be16d2b9bd1ce Mon Sep 17 00:00:00 2001 From: rory Date: Thu, 28 Mar 2024 07:14:20 -0700 Subject: [PATCH 26/32] Simplify PersistedRequests.save --- src/libs/actions/PersistedRequests.ts | 31 +++++++++------------------ 1 file changed, 10 insertions(+), 21 deletions(-) diff --git a/src/libs/actions/PersistedRequests.ts b/src/libs/actions/PersistedRequests.ts index 92031c145268..7d53c424decd 100644 --- a/src/libs/actions/PersistedRequests.ts +++ b/src/libs/actions/PersistedRequests.ts @@ -19,28 +19,16 @@ function clear() { function save(requestToPersist: Request) { const requests = [...persistedRequests, requestToPersist]; - for (let i = requests.length - 1; i > 0; i--) { - // since we're deleting elements from the array, it may be reindexed. - // In this case, we need to make sure our index is still in bounds - if (i >= requests.length) { - // eslint-disable-next-line no-continue - continue; - } - const request = requests[i]; - - // identify and handle any existing requests that conflict with the new one - const {getConflictingRequests, handleConflictingRequest, shouldIncludeCurrentRequest} = request; - if (!getConflictingRequests) { - // eslint-disable-next-line no-continue - continue; - } - - // Get all the remaining requests, potentially excluding the one we're adding, which will always be at the end of the array - const remainingRequests = shouldIncludeCurrentRequest ? requests : requests.slice(0, requests.length - 1); + // identify and handle any existing requests that conflict with the new one + const {getConflictingRequests, handleConflictingRequest, shouldIncludeCurrentRequest} = requestToPersist; + if (getConflictingRequests) { + // Get all the potentially excluding the one we're adding, which will always be at the end of the array + const potentiallyConflictingRequests = shouldIncludeCurrentRequest ? requests : requests.slice(0, requests.length - 1); // Identify conflicting requests according to logic bound to the request - const conflictingRequests = getConflictingRequests(remainingRequests); + const conflictingRequests = getConflictingRequests(potentiallyConflictingRequests); + conflictingRequests.forEach((conflictingRequest) => { // delete the conflicting request const index = requests.findIndex((req) => req === conflictingRequest); @@ -53,14 +41,15 @@ function save(requestToPersist: Request) { }); } - // Save the updated set of requests + // Don't try to serialize conflict resolution functions persistedRequests = requests.map((request) => { - // Don't try to serialize conflict resolution functions delete request.getConflictingRequests; delete request.handleConflictingRequest; delete request.shouldIncludeCurrentRequest; return request; }); + + // Save the updated set of requests Onyx.set(ONYXKEYS.PERSISTED_REQUESTS, requests); } From 4cbbe9b87a5f2821e37d038f44f75e5993366877 Mon Sep 17 00:00:00 2001 From: rory Date: Thu, 28 Mar 2024 07:16:01 -0700 Subject: [PATCH 27/32] fix comment --- src/libs/actions/PersistedRequests.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libs/actions/PersistedRequests.ts b/src/libs/actions/PersistedRequests.ts index 7d53c424decd..cdb260722d81 100644 --- a/src/libs/actions/PersistedRequests.ts +++ b/src/libs/actions/PersistedRequests.ts @@ -23,7 +23,7 @@ function save(requestToPersist: Request) { // identify and handle any existing requests that conflict with the new one const {getConflictingRequests, handleConflictingRequest, shouldIncludeCurrentRequest} = requestToPersist; if (getConflictingRequests) { - // Get all the potentially excluding the one we're adding, which will always be at the end of the array + // Get all the requests, potentially including the one we're adding, which will always be at the end of the array const potentiallyConflictingRequests = shouldIncludeCurrentRequest ? requests : requests.slice(0, requests.length - 1); // Identify conflicting requests according to logic bound to the request From 305a73d8f899b9b0984deac5b4496aa8b1eee91a Mon Sep 17 00:00:00 2001 From: rory Date: Thu, 28 Mar 2024 07:17:07 -0700 Subject: [PATCH 28/32] Clarify comments --- src/libs/actions/PersistedRequests.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libs/actions/PersistedRequests.ts b/src/libs/actions/PersistedRequests.ts index cdb260722d81..8adb59824693 100644 --- a/src/libs/actions/PersistedRequests.ts +++ b/src/libs/actions/PersistedRequests.ts @@ -26,7 +26,7 @@ function save(requestToPersist: Request) { // Get all the requests, potentially including the one we're adding, which will always be at the end of the array const potentiallyConflictingRequests = shouldIncludeCurrentRequest ? requests : requests.slice(0, requests.length - 1); - // Identify conflicting requests according to logic bound to the request + // Identify conflicting requests according to logic bound to the new request const conflictingRequests = getConflictingRequests(potentiallyConflictingRequests); conflictingRequests.forEach((conflictingRequest) => { @@ -36,7 +36,7 @@ function save(requestToPersist: Request) { requests.splice(index, 1); } - // Allow the request to perform any additional cleanup for a cancelled request + // Allow the new request to perform any additional cleanup for a cancelled request handleConflictingRequest?.(conflictingRequest); }); } From f494f26d7299afad60b48678a24b352f8303d482 Mon Sep 17 00:00:00 2001 From: rory Date: Thu, 28 Mar 2024 07:27:01 -0700 Subject: [PATCH 29/32] Simplify callback for OpenReport --- src/libs/actions/Report.ts | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/src/libs/actions/Report.ts b/src/libs/actions/Report.ts index 04d62c7b2bcb..7972809dc986 100644 --- a/src/libs/actions/Report.ts +++ b/src/libs/actions/Report.ts @@ -629,7 +629,8 @@ function openReport( } // If we are creating a new report, we need to add the optimistic report data and a report action - if (!isEmptyObject(newReportObject)) { + const isCreatingNewReport = !isEmptyObject(newReportObject); + if (isCreatingNewReport) { // Change the method to set for new reports because it doesn't exist yet, is faster, // and we need the data to be available when we navigate to the chat page optimisticData[0].onyxMethod = Onyx.METHOD.SET; @@ -744,12 +745,7 @@ function openReport( {optimisticData, successData, failureData}, { getConflictingRequests: (persistedRequests) => - persistedRequests.filter((request) => { - if (request.command !== WRITE_COMMANDS.OPEN_REPORT) { - return true; - } - return isEmptyObject(newReportObject) ? request.data?.reportID === reportID : false; - }), + persistedRequests.filter((request) => request.command === WRITE_COMMANDS.OPEN_REPORT && !isCreatingNewReport && request.data?.reportID === reportID), }, ); } From e3952611c07eae8ef2b6902866d24ee001198328 Mon Sep 17 00:00:00 2001 From: rory Date: Thu, 28 Mar 2024 12:27:21 -0700 Subject: [PATCH 30/32] Dont cancel addcomment requests when deleting a thread parent action --- src/libs/actions/Report.ts | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/src/libs/actions/Report.ts b/src/libs/actions/Report.ts index 726763da86bb..fbd1674b92e9 100644 --- a/src/libs/actions/Report.ts +++ b/src/libs/actions/Report.ts @@ -1195,6 +1195,7 @@ function deleteReportComment(reportID: string, reportAction: ReportAction) { return; } + const isDeletedParentAction = ReportActionsUtils.isThreadParentMessage(reportAction, reportID); const deletedMessage: Message[] = [ { translationKey: '', @@ -1202,7 +1203,7 @@ function deleteReportComment(reportID: string, reportAction: ReportAction) { html: '', text: '', isEdited: true, - isDeletedParentAction: ReportActionsUtils.isThreadParentMessage(reportAction, reportID), + isDeletedParentAction, }, ]; const optimisticReportActions: NullishDeep = { @@ -1299,19 +1300,22 @@ function deleteReportComment(reportID: string, reportAction: ReportAction) { }; CachedPDFPaths.clearByKey(reportActionID); + API.write( WRITE_COMMANDS.DELETE_COMMENT, parameters, {optimisticData, successData, failureData}, { - getConflictingRequests: (persistedRequests) => - persistedRequests.filter( - (request) => - ([WRITE_COMMANDS.ADD_COMMENT, WRITE_COMMANDS.ADD_ATTACHMENT, WRITE_COMMANDS.DELETE_COMMENT, WRITE_COMMANDS.UPDATE_COMMENT] as string[]).includes(request.command) && - request.data?.reportActionID === reportActionID, - ), + getConflictingRequests: (persistedRequests) => { + const conflictingCommands = ( + isDeletedParentAction + ? [WRITE_COMMANDS.UPDATE_COMMENT] + : [WRITE_COMMANDS.ADD_COMMENT, WRITE_COMMANDS.ADD_ATTACHMENT, WRITE_COMMANDS.UPDATE_COMMENT, WRITE_COMMANDS.DELETE_COMMENT] + ) as string[]; + return persistedRequests.filter((request) => conflictingCommands.includes(request.command) && request.data?.reportActionID === reportActionID); + }, handleConflictingRequest: () => Onyx.update(successData), - shouldIncludeCurrentRequest: true, + shouldIncludeCurrentRequest: !isDeletedParentAction, }, ); } From e4a13056092324a1c4aa9fdd876b5f95cce4a980 Mon Sep 17 00:00:00 2001 From: rory Date: Thu, 28 Mar 2024 15:11:11 -0700 Subject: [PATCH 31/32] Don't cancel any request in the queue that creates a new report --- src/libs/actions/Report.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/libs/actions/Report.ts b/src/libs/actions/Report.ts index fbd1674b92e9..cd5a9b320ba8 100644 --- a/src/libs/actions/Report.ts +++ b/src/libs/actions/Report.ts @@ -779,7 +779,9 @@ function openReport( {optimisticData, successData, failureData}, { getConflictingRequests: (persistedRequests) => - persistedRequests.filter((request) => request.command === WRITE_COMMANDS.OPEN_REPORT && !isCreatingNewReport && request.data?.reportID === reportID), + persistedRequests.filter( + (request) => request.command === WRITE_COMMANDS.OPEN_REPORT && request.data?.reportID === reportID && !isCreatingNewReport && !request.data?.createdReportActionID, + ), }, ); } From be315d6cf336a448e330ccc752e395ccf2695629 Mon Sep 17 00:00:00 2001 From: rory Date: Thu, 28 Mar 2024 15:13:43 -0700 Subject: [PATCH 32/32] Simplify conditions --- src/libs/actions/Report.ts | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/libs/actions/Report.ts b/src/libs/actions/Report.ts index cd5a9b320ba8..775d2459b0f3 100644 --- a/src/libs/actions/Report.ts +++ b/src/libs/actions/Report.ts @@ -779,9 +779,11 @@ function openReport( {optimisticData, successData, failureData}, { getConflictingRequests: (persistedRequests) => - persistedRequests.filter( - (request) => request.command === WRITE_COMMANDS.OPEN_REPORT && request.data?.reportID === reportID && !isCreatingNewReport && !request.data?.createdReportActionID, - ), + // requests conflict only if: + // 1. they are OpenReport commands + // 2. they have the same reportID + // 3. they are not creating a report - all calls to OpenReport that create a report will be unique and have a unique createdReportActionID + persistedRequests.filter((request) => request.command === WRITE_COMMANDS.OPEN_REPORT && request.data?.reportID === reportID && !request.data?.createdReportActionID), }, ); }