From cbbc14b6012a240ab8bf292d49cf0a97a3113294 Mon Sep 17 00:00:00 2001 From: Adam Horodyski Date: Thu, 16 Jul 2026 16:39:51 +0200 Subject: [PATCH] perf: scope reportAttributes error propagation to changed chats The propagate-errors-to-parent-chat pass walked every report on every compute, even when the incremental pass only touched one report. Keep a module-level chat -> children index (patched from report deltas, rebuilt on full recomputes, same pattern as reportTransactionsAndViolations) and re-evaluate propagation only for chats recomputed in the pass. Every path that enqueues a child already enqueues its parent chat; the one gap (policy-tag updates) is closed by moving the parent-chat enqueue after all branches. A child that is deleted or moves to another chat now re-enqueues the chat it left, fixing a stale error dot that main keeps until the old chat is next recomputed. Untouched errored chats keep their entry by reference instead of being restamped every compute. Co-Authored-By: Claude Fable 5 --- .../OnyxDerived/configs/reportAttributes.ts | 149 ++++++++++++------ tests/unit/reportAttributesTest.ts | 136 +++++++++++++++- 2 files changed, 236 insertions(+), 49 deletions(-) diff --git a/src/libs/actions/OnyxDerived/configs/reportAttributes.ts b/src/libs/actions/OnyxDerived/configs/reportAttributes.ts index c1c8f9d28268..af1cef052030 100644 --- a/src/libs/actions/OnyxDerived/configs/reportAttributes.ts +++ b/src/libs/actions/OnyxDerived/configs/reportAttributes.ts @@ -35,6 +35,52 @@ let previousDisplayNames: Record = {}; let previousPersonalDetails: OnyxEntry | undefined; let previousPolicies: OnyxCollection; +// Which chat each child report belongs to, and the reverse. Kept in sync incrementally so +// error propagation doesn't have to walk every report on each compute. +const childToChat = new Map(); +const childrenByChat = new Map>(); + +const rebuildChildChatIndex = (reports: OnyxCollection) => { + childToChat.clear(); + childrenByChat.clear(); + for (const report of Object.values(reports ?? {})) { + if (!report?.reportID || !report.chatReportID || report.reportID === report.chatReportID) { + continue; + } + childToChat.set(report.reportID, report.chatReportID); + const children = childrenByChat.get(report.chatReportID) ?? new Set(); + children.add(report.reportID); + childrenByChat.set(report.chatReportID, children); + } +}; + +// Points a child at its (possibly new) chat. Returns the chats whose child list changed — +// a chat a child left may need its propagated error cleared, so callers must re-enqueue them. +const updateChildChatIndex = (childReportID: string, chatReportID: string | undefined): string[] => { + const previousChatReportID = childToChat.get(childReportID); + if (previousChatReportID === chatReportID) { + return []; + } + const affectedChatIDs: string[] = []; + if (previousChatReportID) { + childToChat.delete(childReportID); + const previousChildren = childrenByChat.get(previousChatReportID); + previousChildren?.delete(childReportID); + if (previousChildren?.size === 0) { + childrenByChat.delete(previousChatReportID); + } + affectedChatIDs.push(previousChatReportID); + } + if (chatReportID) { + childToChat.set(childReportID, chatReportID); + const children = childrenByChat.get(chatReportID) ?? new Set(); + children.add(childReportID); + childrenByChat.set(chatReportID, children); + affectedChatIDs.push(chatReportID); + } + return affectedChatIDs; +}; + const RECOMPUTE_ALL = 'all' as const; const prepareReportKeys = (keys: string[]) => { @@ -157,7 +203,12 @@ const reportReferencesAccountIDs = (report: Report, accountIDs: Set): bo // Returns the report-preview action ID of the oldest child in `reportIDs` matching `predicate` // (oldest by preview-action creation time), or undefined when none match. -const getOldestPreviewActionID = (chatReportID: string, reportIDs: string[] | undefined, reports: OnyxCollection, predicate?: (childReport: OnyxEntry) => boolean) => { +const getOldestPreviewActionID = ( + chatReportID: string, + reportIDs: Iterable | undefined, + reports: OnyxCollection, + predicate?: (childReport: OnyxEntry) => boolean, +) => { let oldestCreated: string | undefined; let targetReportActionID: string | undefined; for (const childReportID of reportIDs ?? []) { @@ -310,6 +361,23 @@ export default createOnyxDerivedValueConfig({ const transactionViolationsUpdates = sourceValues?.[ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS]; const policyTagsUpdates = sourceValues?.[ONYXKEYS.COLLECTION.POLICY_TAGS]; + // Keep the chat → children index in sync. A child that was deleted or moved to another chat + // changes the error state of the chat it left, which nothing else re-enqueues — collect those + // chats so they are recomputed (and their stale propagated error cleared) in this pass. + const indexAffectedChatKeys: string[] = []; + if (useIncrementalUpdates) { + for (const reportKey of Object.keys(reportUpdates)) { + const report = reports[reportKey]; + const reportID = reportKey.replace(ONYXKEYS.COLLECTION.REPORT, ''); + const chatReportID = report?.chatReportID && report.chatReportID !== report.reportID ? report.chatReportID : undefined; + for (const affectedChatID of updateChildChatIndex(reportID, chatReportID)) { + indexAffectedChatKeys.push(`${ONYXKEYS.COLLECTION.REPORT}${affectedChatID}`); + } + } + } else { + rebuildChildChatIndex(reports); + } + let dataToIterate = Object.keys(reports); // check if there are any report-related updates @@ -347,6 +415,7 @@ export default createOnyxDerivedValueConfig({ ...Array.from(reportUpdatesRelatedToReportActions), ...policyChangedReportKeys, ...personalDetailsChangedReportKeys, + ...indexAffectedChatKeys, ]; if (useIncrementalUpdates) { @@ -355,18 +424,6 @@ export default createOnyxDerivedValueConfig({ dataToIterate = []; if (updates.length > 0) { dataToIterate = prepareReportKeys(updates); - - // When an IOU report changes, we need to re-evaluate its parent chat report as well. - const parentChatReportIDsToUpdate = new Set(); - for (const reportKey of dataToIterate) { - const report = reports[reportKey]; - if (report?.chatReportID && report.reportID !== report.chatReportID) { - parentChatReportIDsToUpdate.add(`${ONYXKEYS.COLLECTION.REPORT}${report.chatReportID}`); - } - } - if (parentChatReportIDsToUpdate.size > 0) { - dataToIterate.push(...Array.from(parentChatReportIDsToUpdate)); - } } if (!!transactionsUpdates || !!transactionViolationsUpdates) { let transactionReportIDs: string[] = []; @@ -405,15 +462,7 @@ export default createOnyxDerivedValueConfig({ transactionReportIDs = [...transactionReportIDs, ...violationReportIDs, ...chatReportIDs]; } - // A transaction change (e.g. a card expense going from pending to posted) can flip whether its - // expense report requires attention, but the to-do/GBR render on the parent workspace chat. - // Enqueue those parent chats too so their attributes don't stay stale after the transaction updates. - const transactionParentChatReportIDs = transactionReportIDs - .map((reportKey) => reports?.[reportKey]?.chatReportID) - .filter(Boolean) - .map((chatReportID) => `${ONYXKEYS.COLLECTION.REPORT}${chatReportID}`); - - dataToIterate.push(...prepareReportKeys([...transactionReportIDs, ...transactionParentChatReportIDs])); + dataToIterate.push(...prepareReportKeys(transactionReportIDs)); } if (policyTagsUpdates) { const changedPolicyIDs = new Set(Object.keys(policyTagsUpdates).map((key) => key.replace(ONYXKEYS.COLLECTION.POLICY_TAGS, ''))); @@ -422,6 +471,20 @@ export default createOnyxDerivedValueConfig({ .map((report) => `${ONYXKEYS.COLLECTION.REPORT}${report?.reportID}`); dataToIterate.push(...prepareReportKeys(affectedReportKeys)); } + + // Whatever caused a child report to be recomputed can also change whether its parent chat + // shows the error indicator — always recompute the parent chat in the same pass. The scoped + // error propagation below relies on this: it only re-checks chats present in dataToIterate. + const parentChatReportIDsToUpdate = new Set(); + for (const reportKey of dataToIterate) { + const report = reports[reportKey]; + if (report?.chatReportID && report.reportID !== report.chatReportID) { + parentChatReportIDsToUpdate.add(`${ONYXKEYS.COLLECTION.REPORT}${report.chatReportID}`); + } + } + if (parentChatReportIDsToUpdate.size > 0) { + dataToIterate.push(...Array.from(parentChatReportIDsToUpdate)); + } } else { // No updates to process, return current value to prevent unnecessary computation return currentValue ?? {reports: {}, locale: null}; @@ -547,45 +610,39 @@ export default createOnyxDerivedValueConfig({ currentValue?.reports ? {...currentValue.reports} : {}, ); - // Propagate errors from IOU reports to their parent chat reports. + // Propagate errors from IOU reports to their parent chat reports — but only for chats + // recomputed in this pass. Every path that enqueues a child report also enqueues its parent + // chat (see parentChatReportIDsToUpdate and indexAffectedChatKeys above), so any chat whose + // aggregate error state could have changed is in dataToIterate, freshly recomputed and + // unstamped — a chat with no remaining errored children needs no explicit clearing. const currentUserAccountID = session?.accountID ?? CONST.DEFAULT_NUMBER_ID; const currentUserEmail = session?.email ?? ''; - const erroredChildReportIDsByChat = new Map(); - const childReportIDsByChat = new Map(); - for (const report of Object.values(reports)) { - if (!report?.reportID || !report.chatReportID || report.reportID === report.chatReportID) { + for (const key of new Set(dataToIterate)) { + const chatReportID = key.replace(ONYXKEYS.COLLECTION.REPORT, ''); + const childReportIDs = childrenByChat.get(chatReportID); + const chatAttributes = reportAttributes[chatReportID]; + if (!childReportIDs || !chatAttributes) { continue; } - const childReportIDs = childReportIDsByChat.get(report.chatReportID) ?? []; - childReportIDs.push(report.reportID); - childReportIDsByChat.set(report.chatReportID, childReportIDs); - - // If this is an IOU report and its calculated attributes have an error, - // then we need to mark its parent chat report. // We read `needsParentChatErrorPropagation` rather than `brickRoadStatus` because the per-report // pass suppresses the child's own brickRoadStatus when the parent workspace chat is accessible — // we still need to propagate the error up so the parent shows the indicator. - const attributes = reportAttributes[report.reportID]; - if (attributes?.needsParentChatErrorPropagation || attributes?.brickRoadStatus === CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR) { - const erroredChildReportIDs = erroredChildReportIDsByChat.get(report.chatReportID) ?? []; - erroredChildReportIDs.push(report.reportID); - erroredChildReportIDsByChat.set(report.chatReportID, erroredChildReportIDs); + const erroredChildReportIDs: string[] = []; + for (const childReportID of childReportIDs) { + const attributes = reportAttributes[childReportID]; + if (attributes?.needsParentChatErrorPropagation || attributes?.brickRoadStatus === CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR) { + erroredChildReportIDs.push(childReportID); + } } - } - - // Apply the error status to the parent chat reports. - for (const [chatReportID, erroredChildReportIDs] of erroredChildReportIDsByChat) { - if (!reportAttributes[chatReportID]) { + if (erroredChildReportIDs.length === 0) { continue; } - const chatAttributes = reportAttributes[chatReportID]; let actionTargetReportActionID = chatAttributes.actionTargetReportActionID; - actionTargetReportActionID = getOldestPreviewActionID(chatReportID, erroredChildReportIDs, reports, isActionable) ?? - getOldestPreviewActionID(chatReportID, childReportIDsByChat.get(chatReportID), reports, (childReport) => + getOldestPreviewActionID(chatReportID, childReportIDs, reports, (childReport) => needsViolationFix(childReport, policies, transactionViolations, currentUserAccountID, currentUserEmail), ) ?? getOldestPreviewActionID(chatReportID, erroredChildReportIDs, reports) ?? diff --git a/tests/unit/reportAttributesTest.ts b/tests/unit/reportAttributesTest.ts index 7c76a9ac4116..c53825813a4c 100644 --- a/tests/unit/reportAttributesTest.ts +++ b/tests/unit/reportAttributesTest.ts @@ -1,5 +1,4 @@ -import type reportAttributesModuleDefault from '@userActions/OnyxDerived/configs/reportAttributes'; -import {hasPolicyRelevantFieldChanged} from '@userActions/OnyxDerived/configs/reportAttributes'; +import reportAttributesModuleDefault, {hasPolicyRelevantFieldChanged} from '@userActions/OnyxDerived/configs/reportAttributes'; import CONST from '@src/CONST'; import ONYXKEYS from '@src/ONYXKEYS'; @@ -26,12 +25,20 @@ jest.mock('@libs/ReportUtils', () => ({ isArchivedReport: jest.fn(() => false), isValidReport: jest.fn(() => true), parseReportRouteParams: jest.fn(() => ({reportID: ''})), + isOpenReport: jest.fn(() => true), + isProcessingReport: jest.fn(() => false), + isPolicyExpenseChat: jest.fn(() => false), + isPolicyAdmin: jest.fn(() => false), + hasViolations: jest.fn(() => false), })); +// Report IDs the mocked getReasonAndReportActionThatHasRedBrickRoad treats as errored. +const mockErroredReportIDs = new Set(); + jest.mock('@libs/SidebarUtils', () => ({ __esModule: true, default: { - getReasonAndReportActionThatHasRedBrickRoad: jest.fn(() => undefined), + getReasonAndReportActionThatHasRedBrickRoad: jest.fn((report: Report) => (mockErroredReportIDs.has(report.reportID) ? {reason: 'hasErrors', reportAction: undefined} : undefined)), }, })); @@ -371,3 +378,126 @@ describe('reportAttributes compute — policy change code flow', () => { expect(result?.reports.chat1?.reportName).toBe('Test Report'); }); }); + +describe('reportAttributes compute — error propagation to parent chats', () => { + // Static import instead of the re-require pattern above: every test here starts with a full + // compute (seedFullCompute), which rebuilds all of the config's module-level state anyway. + const config = reportAttributesModuleDefault; + + const chatA: Report = {...createRandomReport(1, CONST.REPORT.CHAT_TYPE.POLICY_EXPENSE_CHAT), reportID: 'chatA', policyID: 'policy1', chatReportID: undefined}; + const chatB: Report = {...createRandomReport(2, CONST.REPORT.CHAT_TYPE.POLICY_EXPENSE_CHAT), reportID: 'chatB', policyID: 'policy1', chatReportID: undefined}; + const childA1: Report = {...createRandomReport(3, undefined), reportID: 'childA1', policyID: 'policy1', chatReportID: 'chatA'}; + const childA2: Report = {...createRandomReport(4, undefined), reportID: 'childA2', policyID: 'policy1', chatReportID: 'chatA'}; + const childB1: Report = {...createRandomReport(5, undefined), reportID: 'childB1', policyID: 'policy1', chatReportID: 'chatB'}; + + const baseReports: OnyxCollection = { + [`${ONYXKEYS.COLLECTION.REPORT}chatA`]: chatA, + [`${ONYXKEYS.COLLECTION.REPORT}chatB`]: chatB, + [`${ONYXKEYS.COLLECTION.REPORT}childA1`]: childA1, + [`${ONYXKEYS.COLLECTION.REPORT}childA2`]: childA2, + [`${ONYXKEYS.COLLECTION.REPORT}childB1`]: childB1, + }; + + beforeEach(() => { + mockErroredReportIDs.clear(); + }); + + const buildArgs = (reportsArg: OnyxCollection): Parameters[0] => [ + reportsArg, + undefined, + undefined, + undefined, + undefined, + undefined, + undefined, + undefined, + undefined, + undefined, + undefined, + undefined, + undefined, + undefined, + ]; + + // A full compute (no currentValue) — seeds the module-level chat → children index too. + const seedFullCompute = (reportsArg: OnyxCollection) => config.compute(buildArgs(reportsArg), {currentValue: undefined, sourceValues: undefined}); + + const computeReportDelta = (reportsArg: OnyxCollection, currentValue: ReportAttributesDerivedValue, delta: OnyxCollection) => + config.compute(buildArgs(reportsArg), { + currentValue, + sourceValues: {[ONYXKEYS.COLLECTION.REPORT]: delta}, + }); + + it('flags the parent chat when a child report has errors', () => { + mockErroredReportIDs.add('childA1'); + + const result = seedFullCompute(baseReports); + + expect(result?.reports.chatA?.brickRoadStatus).toBe(CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR); + expect(result?.reports.chatA?.actionBadge).toBe(CONST.REPORT.ACTION_BADGE.FIX); + expect(result?.reports.chatB?.brickRoadStatus).toBeUndefined(); + }); + + it('keeps the parent flagged when one child clears but a sibling is still errored', () => { + mockErroredReportIDs.add('childA1'); + mockErroredReportIDs.add('childA2'); + const seeded = seedFullCompute(baseReports); + expect(seeded?.reports.chatA?.brickRoadStatus).toBe(CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR); + + mockErroredReportIDs.delete('childA1'); + const result = computeReportDelta(baseReports, seeded, {[`${ONYXKEYS.COLLECTION.REPORT}childA1`]: childA1}); + + expect(result?.reports.childA1?.brickRoadStatus).toBeUndefined(); + expect(result?.reports.chatA?.brickRoadStatus).toBe(CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR); + }); + + it('unflags the parent when the last errored child clears', () => { + mockErroredReportIDs.add('childA1'); + const seeded = seedFullCompute(baseReports); + expect(seeded?.reports.chatA?.brickRoadStatus).toBe(CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR); + + mockErroredReportIDs.delete('childA1'); + const result = computeReportDelta(baseReports, seeded, {[`${ONYXKEYS.COLLECTION.REPORT}childA1`]: childA1}); + + expect(result?.reports.chatA?.brickRoadStatus).toBeUndefined(); + expect(result?.reports.chatA?.actionBadge).toBeUndefined(); + }); + + it('moves the flag when an errored child moves to another chat', () => { + mockErroredReportIDs.add('childA1'); + const seeded = seedFullCompute(baseReports); + expect(seeded?.reports.chatA?.brickRoadStatus).toBe(CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR); + expect(seeded?.reports.chatB?.brickRoadStatus).toBeUndefined(); + + const movedChild: Report = {...childA1, chatReportID: 'chatB'}; + const movedReports: OnyxCollection = {...baseReports, [`${ONYXKEYS.COLLECTION.REPORT}childA1`]: movedChild}; + const result = computeReportDelta(movedReports, seeded, {[`${ONYXKEYS.COLLECTION.REPORT}childA1`]: movedChild}); + + expect(result?.reports.chatA?.brickRoadStatus).toBeUndefined(); + expect(result?.reports.chatB?.brickRoadStatus).toBe(CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR); + }); + + it('unflags the parent when its only errored child is deleted', () => { + mockErroredReportIDs.add('childA1'); + const seeded = seedFullCompute(baseReports); + expect(seeded?.reports.chatA?.brickRoadStatus).toBe(CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR); + + const {[`${ONYXKEYS.COLLECTION.REPORT}childA1`]: deletedChild, ...remainingReports} = baseReports; + const result = computeReportDelta(remainingReports, seeded, {[`${ONYXKEYS.COLLECTION.REPORT}childA1`]: undefined}); + + expect(result?.reports.childA1).toBeUndefined(); + expect(result?.reports.chatA?.brickRoadStatus).toBeUndefined(); + }); + + it('does not touch unrelated errored chats on a single-report update', () => { + mockErroredReportIDs.add('childB1'); + const seeded = seedFullCompute(baseReports); + const chatBBefore = seeded?.reports.chatB; + expect(chatBBefore?.brickRoadStatus).toBe(CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR); + + const result = computeReportDelta(baseReports, seeded, {[`${ONYXKEYS.COLLECTION.REPORT}childA1`]: childA1}); + + // chatB was not part of the update — its entry must be carried over by reference, not restamped. + expect(result?.reports.chatB).toBe(chatBBefore); + }); +});