From f5f573d27a98de10450405b2c307f1934ec61e4c Mon Sep 17 00:00:00 2001 From: sumo_slonik Date: Tue, 14 Jul 2026 15:31:32 +0200 Subject: [PATCH 1/3] Persist reportAttributes recompute baselines so app restarts do not trigger full rescans Policy change detection now compares signatures stored in the derived value instead of a module-level snapshot that resets on every app restart, so the first post-startup policy merge no longer rescans every report. The same value-comparison guard is applied to conciergeReportID and introSelected, which previously forced a full recompute whenever the key was re-delivered. Each OnyxDerivedCompute span now carries a derived_trigger attribute. --- src/CONST/index.ts | 1 + .../OnyxDerived/configs/reportAttributes.ts | 97 +++++++++++++------ src/libs/actions/OnyxDerived/index.ts | 2 +- src/types/onyx/DerivedValues.ts | 13 +++ tests/unit/reportAttributesTest.ts | 89 ++++++++++++----- 5 files changed, 148 insertions(+), 54 deletions(-) diff --git a/src/CONST/index.ts b/src/CONST/index.ts index 52d0210840c1..2e5335686225 100644 --- a/src/CONST/index.ts +++ b/src/CONST/index.ts @@ -2211,6 +2211,7 @@ const CONST = { APP_OPEN: 'NavigationAppOpen', }, // Attribute names + ATTRIBUTE_DERIVED_TRIGGER: 'derived_trigger', ATTRIBUTE_IOU_TYPE: 'iou_type', ATTRIBUTE_IS_ONE_TRANSACTION_REPORT: 'is_one_transaction_report', ATTRIBUTE_IS_TRANSACTION_THREAD: 'is_transaction_thread', diff --git a/src/libs/actions/OnyxDerived/configs/reportAttributes.ts b/src/libs/actions/OnyxDerived/configs/reportAttributes.ts index c1c8f9d28268..05305ce9e832 100644 --- a/src/libs/actions/OnyxDerived/configs/reportAttributes.ts +++ b/src/libs/actions/OnyxDerived/configs/reportAttributes.ts @@ -33,7 +33,6 @@ import {isTrackIntentUserSelector} from '@selectors/Onboarding'; // The name-related fields we saw for each account last time, so we can spot which accounts changed. let previousDisplayNames: Record = {}; let previousPersonalDetails: OnyxEntry | undefined; -let previousPolicies: OnyxCollection; const RECOMPUTE_ALL = 'all' as const; @@ -50,22 +49,12 @@ const prepareReportKeys = (keys: string[]) => { ]; }; -const hasPolicyRelevantFieldChanged = (prev: Policy | null | undefined, next: Policy | null | undefined): boolean => { - if (!prev && !next) { - return false; - } - if (!prev || !next) { - return true; - } - return ( - prev.type !== next.type || - prev.approvalMode !== next.approvalMode || - prev.reimbursementChoice !== next.reimbursementChoice || - prev.autoReimbursementLimit !== next.autoReimbursementLimit || - prev.role !== next.role || - prev.autoReimbursement?.limit !== next.autoReimbursement?.limit - ); -}; +// A short string built from the policy fields the report attributes depend on. Signatures are stored in +// the derived value (like `locale`), so the change-detection baseline survives app restarts. +const policyRelevantSignature = (policy: Policy | null | undefined): string | null => + policy ? JSON.stringify([policy.type, policy.approvalMode, policy.reimbursementChoice, policy.autoReimbursementLimit, policy.role, policy.autoReimbursement?.limit]) : null; + +const hasPolicyRelevantFieldChanged = (prev: Policy | null | undefined, next: Policy | null | undefined): boolean => policyRelevantSignature(prev) !== policyRelevantSignature(next); // A short string built from the fields a report name can come from: displayName and firstName. const displayNameSignature = (details: PersonalDetails | null): string => JSON.stringify([details?.displayName ?? '', details?.firstName ?? '']); @@ -250,25 +239,50 @@ export default createOnyxDerivedValueConfig({ seedDisplayNamesBaseline(personalDetails); } + const nextIsTrackIntentUser = isTrackIntentUserSelector(introSelected); + // conciergeReportID and introSelected are re-delivered on every OpenApp/reconnect merge, so a full + // recompute happens only when the value differs from the one the attributes were computed with. + // A missing stored value (attributes written by an older app version) seeds the baseline instead. + const storedConciergeReportID = currentValue && 'conciergeReportID' in currentValue ? (currentValue.conciergeReportID ?? null) : undefined; + const storedIsTrackIntentUser = currentValue && 'isTrackIntentUser' in currentValue ? currentValue.isTrackIntentUser : undefined; + const hasConciergeReportIDChanged = + hasKeyTriggeredCompute(ONYXKEYS.CONCIERGE_REPORT_ID, sourceValues) && storedConciergeReportID !== undefined && storedConciergeReportID !== (conciergeReportID ?? null); + const hasIsTrackIntentUserChanged = + hasKeyTriggeredCompute(ONYXKEYS.NVP_INTRO_SELECTED, sourceValues) && storedIsTrackIntentUser !== undefined && storedIsTrackIntentUser !== nextIsTrackIntentUser; + // A full recompute is needed when locale changes (report names are locale-dependent) or display names change. // We compare preferredLocale against currentValue?.locale so that the first locale load on startup // (where both equal the same persisted value) does not trigger an unnecessary full recompute. const needsFullRecompute = (hasKeyTriggeredCompute(ONYXKEYS.NVP_PREFERRED_LOCALE, sourceValues) && preferredLocale !== currentValue?.locale) || displayNameChanges === RECOMPUTE_ALL || - hasKeyTriggeredCompute(ONYXKEYS.CONCIERGE_REPORT_ID, sourceValues) || - hasKeyTriggeredCompute(ONYXKEYS.NVP_INTRO_SELECTED, sourceValues); - + hasConciergeReportIDChanged || + hasIsTrackIntentUserChanged; + + // Policy change detection compares signatures stored in the derived value, so the baseline survives + // app restarts. Without a stored baseline (value written by an older app version) the first policy + // delivery only seeds the signatures — the stored attributes already reflect these policies. + const storedPolicySignatures = currentValue?.policySignatures; + let nextPolicySignatures = storedPolicySignatures; const policyChangedReportKeys: string[] = []; - if (hasKeyTriggeredCompute(ONYXKEYS.COLLECTION.POLICY, sourceValues)) { - if (!needsFullRecompute) { + if (hasKeyTriggeredCompute(ONYXKEYS.COLLECTION.POLICY, sourceValues) || (!storedPolicySignatures && policies)) { + if (!needsFullRecompute && storedPolicySignatures && hasKeyTriggeredCompute(ONYXKEYS.COLLECTION.POLICY, sourceValues)) { const changedPolicyIDs = new Set(); + const updatedSignatures = {...storedPolicySignatures}; for (const key of Object.keys(sourceValues?.[ONYXKEYS.COLLECTION.POLICY] ?? {})) { - if (hasPolicyRelevantFieldChanged(previousPolicies?.[key], policies?.[key])) { - changedPolicyIDs.add(key.replace(ONYXKEYS.COLLECTION.POLICY, '')); + const signature = policyRelevantSignature(policies?.[key]); + if ((storedPolicySignatures[key] ?? null) === signature) { + continue; + } + changedPolicyIDs.add(key.replace(ONYXKEYS.COLLECTION.POLICY, '')); + if (signature === null) { + delete updatedSignatures[key]; + } else { + updatedSignatures[key] = signature; } } if (changedPolicyIDs.size > 0) { + nextPolicySignatures = updatedSignatures; for (const [reportKey, report] of Object.entries(reports ?? {})) { if (!report) { continue; @@ -289,17 +303,39 @@ export default createOnyxDerivedValueConfig({ } } } + } else { + const signatures: Record = {}; + for (const [key, policy] of Object.entries(policies ?? {})) { + const signature = policyRelevantSignature(policy); + if (signature !== null) { + signatures[key] = signature; + } + } + nextPolicySignatures = signatures; } - previousPolicies = policies; } + // One-time baseline writes (policy signatures / concierge / intro seeds) must persist even when this + // pass otherwise returns the current value unchanged. + const metaPatch: Partial = {}; + if (nextPolicySignatures !== storedPolicySignatures) { + metaPatch.policySignatures = nextPolicySignatures; + } + if (storedConciergeReportID !== (conciergeReportID ?? null)) { + metaPatch.conciergeReportID = conciergeReportID ?? null; + } + if (storedIsTrackIntentUser !== nextIsTrackIntentUser) { + metaPatch.isTrackIntentUser = nextIsTrackIntentUser; + } + const withMetaPatch = (value: ReportAttributesDerivedValue): ReportAttributesDerivedValue => (Object.keys(metaPatch).length > 0 ? {...value, ...metaPatch} : value); + // Use incremental updates when currentValue is already populated and no full recompute is required. // If currentValue has no reports (fresh install or cleared storage), fall back to a full scan. const useIncrementalUpdates = !!currentValue?.reports && Object.keys(currentValue.reports).length > 0 && !needsFullRecompute; // if we already computed the report attributes and there is no new reports data, return the current value if ((useIncrementalUpdates && !sourceValues) || !reports) { - return currentValue ?? {reports: {}, locale: null}; + return withMetaPatch(currentValue ?? {reports: {}, locale: null}); } const reportUpdates = sourceValues?.[ONYXKEYS.COLLECTION.REPORT] ?? {}; @@ -424,7 +460,7 @@ export default createOnyxDerivedValueConfig({ } } else { // No updates to process, return current value to prevent unnecessary computation - return currentValue ?? {reports: {}, locale: null}; + return withMetaPatch(currentValue ?? {reports: {}, locale: null}); } } @@ -529,7 +565,7 @@ export default createOnyxDerivedValueConfig({ allPolicyTags: policyTags, conciergeReportID: conciergeReportID ?? undefined, reportAttributes: currentValue?.reports, - isTrackIntentUser: isTrackIntentUserSelector(introSelected), + isTrackIntentUser: nextIsTrackIntentUser, }) : '', isEmpty: generateIsEmptyReport(report, isReportArchived), @@ -604,8 +640,11 @@ export default createOnyxDerivedValueConfig({ return { reports: reportAttributes, locale: preferredLocale ?? null, + policySignatures: nextPolicySignatures, + conciergeReportID: conciergeReportID ?? null, + isTrackIntentUser: nextIsTrackIntentUser, }; }, }); -export {hasPolicyRelevantFieldChanged}; +export {hasPolicyRelevantFieldChanged, policyRelevantSignature}; diff --git a/src/libs/actions/OnyxDerived/index.ts b/src/libs/actions/OnyxDerived/index.ts index ecd0764882e4..0b0870d6a2d5 100644 --- a/src/libs/actions/OnyxDerived/index.ts +++ b/src/libs/actions/OnyxDerived/index.ts @@ -88,7 +88,7 @@ function init() { name: CONST.TELEMETRY.SPAN_ONYX_DERIVED_COMPUTE, op: CONST.TELEMETRY.SPAN_ONYX_DERIVED_COMPUTE, parentSpan: getSpan(CONST.TELEMETRY.SPAN_APP_STARTUP), - attributes: {derivedKey: key}, + attributes: {derivedKey: key, [CONST.TELEMETRY.ATTRIBUTE_DERIVED_TRIGGER]: sourceKey ?? 'initial'}, }); try { diff --git a/src/types/onyx/DerivedValues.ts b/src/types/onyx/DerivedValues.ts index 4223a0ed8183..82c70e9d4721 100644 --- a/src/types/onyx/DerivedValues.ts +++ b/src/types/onyx/DerivedValues.ts @@ -69,6 +69,19 @@ type ReportAttributesDerivedValue = { * The locale used to compute the report attributes. */ locale: string | null; + /** + * Signatures of the policy fields the attributes depend on, keyed by policy Onyx key. + * Persisted so a fresh app session can tell real policy changes from the first post-startup merge. + */ + policySignatures?: Record; + /** + * The conciergeReportID used to compute the report attributes. + */ + conciergeReportID?: string | null; + /** + * Whether the user was a track-intent user when the attributes were computed. + */ + isTrackIntentUser?: boolean; }; /** diff --git a/tests/unit/reportAttributesTest.ts b/tests/unit/reportAttributesTest.ts index 7c76a9ac4116..bef085604b75 100644 --- a/tests/unit/reportAttributesTest.ts +++ b/tests/unit/reportAttributesTest.ts @@ -1,5 +1,5 @@ import type reportAttributesModuleDefault from '@userActions/OnyxDerived/configs/reportAttributes'; -import {hasPolicyRelevantFieldChanged} from '@userActions/OnyxDerived/configs/reportAttributes'; +import {hasPolicyRelevantFieldChanged, policyRelevantSignature} from '@userActions/OnyxDerived/configs/reportAttributes'; import CONST from '@src/CONST'; import ONYXKEYS from '@src/ONYXKEYS'; @@ -160,6 +160,9 @@ describe('reportAttributes compute — policy change code flow', () => { [`${ONYXKEYS.COLLECTION.POLICY}policy2`]: policy2, }; + // policyRelevantSignature returns null only for a missing policy, so tests can rely on a string. + const signatureOf = (policy: Policy): string => policyRelevantSignature(policy) ?? ''; + beforeEach(() => { jest.resetModules(); @@ -192,15 +195,50 @@ describe('reportAttributes compute — policy change code flow', () => { expect(result?.reports).toHaveProperty('r2'); }); - it('scopes the first policy load to reports referencing the loaded policies when currentValue is already populated', () => { - // Reproduces the ReconnectApp-after-open case: attributes were already computed, then ~1k policies - // land. Only reports whose policy actually arrived should recompute — not every report. + it('seeds policy signatures without recomputing when the stored value has no baseline', () => { + // Reproduces the first policy delivery after startup on a value written by an older app version: + // the stored attributes already reflect these policies, so nothing recomputes — the pass only + // records the signatures so later deliveries can be diffed. + const report3: Report = {...createRandomReport(12, undefined), reportID: 'r3', policyID: 'policyOther', chatReportID: undefined}; + const reportsWithUnrelated: OnyxCollection = { + ...reports, + [`${ONYXKEYS.COLLECTION.REPORT}r3`]: report3, + }; + + const existingValue: ReportAttributesDerivedValue = { + reports: { + r1: {reportName: 'Old Name 1', isEmpty: false, brickRoadStatus: undefined, requiresAttention: false, reportErrors: {}}, + r2: {reportName: 'Old Name 2', isEmpty: false, brickRoadStatus: undefined, requiresAttention: false, reportErrors: {}}, + r3: {reportName: 'Old Name 3', isEmpty: false, brickRoadStatus: undefined, requiresAttention: false, reportErrors: {}}, + }, + locale: null, + }; + + const result = config.compute(buildArgs(policies, reportsWithUnrelated), { + currentValue: existingValue, + sourceValues: {[ONYXKEYS.COLLECTION.POLICY]: policies}, + }); + + expect(result?.reports.r1?.reportName).toBe('Old Name 1'); + expect(result?.reports.r2?.reportName).toBe('Old Name 2'); + expect(result?.reports.r3?.reportName).toBe('Old Name 3'); + expect(result?.policySignatures).toEqual({ + [`${ONYXKEYS.COLLECTION.POLICY}policy1`]: policyRelevantSignature(policy1), + [`${ONYXKEYS.COLLECTION.POLICY}policy2`]: policyRelevantSignature(policy2), + }); + }); + + it('scopes a policy delivery to reports whose stored signature differs when currentValue is already populated', () => { + // Reproduces the ReconnectApp-after-restart case: attributes and signatures were persisted, then + // ~1k policies land and some actually changed. Only reports whose policy signature differs should + // recompute — not every report. const report3: Report = {...createRandomReport(12, undefined), reportID: 'r3', policyID: 'policyOther', chatReportID: undefined}; const reportsWithUnrelated: OnyxCollection = { ...reports, [`${ONYXKEYS.COLLECTION.REPORT}r3`]: report3, }; + const stalePolicy1: Policy = {...policy1, approvalMode: CONST.POLICY.APPROVAL_MODE.OPTIONAL}; const existingValue: ReportAttributesDerivedValue = { reports: { r1: {reportName: 'Old Name 1', isEmpty: false, brickRoadStatus: undefined, requiresAttention: false, reportErrors: {}}, @@ -208,6 +246,10 @@ describe('reportAttributes compute — policy change code flow', () => { r3: {reportName: 'Old Name 3', isEmpty: false, brickRoadStatus: undefined, requiresAttention: false, reportErrors: {}}, }, locale: null, + policySignatures: { + [`${ONYXKEYS.COLLECTION.POLICY}policy1`]: signatureOf(stalePolicy1), + [`${ONYXKEYS.COLLECTION.POLICY}policy2`]: signatureOf(policy2), + }, }; const result = config.compute(buildArgs(policies, reportsWithUnrelated), { @@ -215,11 +257,14 @@ describe('reportAttributes compute — policy change code flow', () => { sourceValues: {[ONYXKEYS.COLLECTION.POLICY]: policies}, }); - // r1/r2 reference the loaded policies → recomputed (default mock name). + // r1's policy signature differs from the stored one → recomputed (default mock name). expect(result?.reports.r1?.reportName).toBe('Test Report'); - expect(result?.reports.r2?.reportName).toBe('Test Report'); + // r2's policy matches its stored signature → keeps its existing value. + expect(result?.reports.r2?.reportName).toBe('Old Name 2'); // r3 references a policy that did not load → keeps its existing value (not recomputed). expect(result?.reports.r3?.reportName).toBe('Old Name 3'); + // The changed policy's signature is refreshed in the stored baseline. + expect(result?.policySignatures?.[`${ONYXKEYS.COLLECTION.POLICY}policy1`]).toBe(policyRelevantSignature(policy1)); }); it('recomputes a child invoice report when only its receiver workspace policy loads', () => { @@ -243,18 +288,16 @@ describe('reportAttributes compute — policy change code flow', () => { [`${ONYXKEYS.COLLECTION.REPORT}invoiceChild`]: invoiceChild, }; - // Seed previousPolicies with just the sender policy, as if it arrived in an earlier batch. - config.compute(buildArgs({[`${ONYXKEYS.COLLECTION.POLICY}senderPolicy`]: senderPolicy}, invoiceReports), { - currentValue: undefined, - sourceValues: {[ONYXKEYS.COLLECTION.POLICY]: {[`${ONYXKEYS.COLLECTION.POLICY}senderPolicy`]: senderPolicy}}, - }); - + // The stored baseline knows only the sender policy, as if it arrived in an earlier batch. const existingValue: ReportAttributesDerivedValue = { reports: { invoiceRoom: {reportName: 'Old Room', isEmpty: false, brickRoadStatus: undefined, requiresAttention: false, reportErrors: {}}, invoiceChild: {reportName: 'Old Child', isEmpty: false, brickRoadStatus: undefined, requiresAttention: false, reportErrors: {}}, }, locale: null, + policySignatures: { + [`${ONYXKEYS.COLLECTION.POLICY}senderPolicy`]: signatureOf(senderPolicy), + }, }; // The receiver policy now arrives in its own batch. @@ -273,12 +316,6 @@ describe('reportAttributes compute — policy change code flow', () => { }); it('only recomputes reports for the changed policy when a tracked field changes', () => { - // Seed previousPolicies by doing an initial compute - config.compute(buildArgs(), { - currentValue: undefined, - sourceValues: {[ONYXKEYS.COLLECTION.POLICY]: policies as never}, - }); - const policy1Changed = {...policy1, approvalMode: CONST.POLICY.APPROVAL_MODE.OPTIONAL} as unknown as Policy; const updatedPolicies: OnyxCollection = { ...policies, @@ -291,6 +328,10 @@ describe('reportAttributes compute — policy change code flow', () => { r2: {reportName: 'Old Name 2', isEmpty: false, brickRoadStatus: undefined, requiresAttention: false, reportErrors: {}}, }, locale: null, + policySignatures: { + [`${ONYXKEYS.COLLECTION.POLICY}policy1`]: signatureOf(policy1), + [`${ONYXKEYS.COLLECTION.POLICY}policy2`]: signatureOf(policy2), + }, }; const computeReportNameMock = (jest.requireMock('@libs/ReportNameUtils') as unknown as {computeReportName: jest.Mock}).computeReportName; @@ -308,12 +349,6 @@ describe('reportAttributes compute — policy change code flow', () => { }); it('skips recompute when a non-tracked policy field changes', () => { - // Seed previousPolicies - config.compute(buildArgs(), { - currentValue: undefined, - sourceValues: {[ONYXKEYS.COLLECTION.POLICY]: policies as never}, - }); - const policy1WithNameChange = {...policy1, name: 'New Policy Name'} as unknown as Policy; const updatedPolicies: OnyxCollection = { ...policies, @@ -326,6 +361,12 @@ describe('reportAttributes compute — policy change code flow', () => { r2: {reportName: 'Existing r2', isEmpty: false, brickRoadStatus: undefined, requiresAttention: false, reportErrors: {}}, }, locale: null, + policySignatures: { + [`${ONYXKEYS.COLLECTION.POLICY}policy1`]: signatureOf(policy1), + [`${ONYXKEYS.COLLECTION.POLICY}policy2`]: signatureOf(policy2), + }, + conciergeReportID: null, + isTrackIntentUser: false, }; const result = config.compute(buildArgs(updatedPolicies), { From 82db1786face042ccb04a6f57158ce624bdde0c4 Mon Sep 17 00:00:00 2001 From: sumo_slonik Date: Tue, 14 Jul 2026 19:51:43 +0200 Subject: [PATCH 2/3] Harden persisted reportAttributes baselines against missed recomputes Policy signatures now hash the whole policy content (minus Onyx write-bookkeeping keys) via a stable stringify, so no consumed policy field can fall outside change detection; the old full-rescan-on-reconnect was the accidental healer for fields like policy.name. Signatures advance only when the recompute they imply actually ran (or had no reports to touch), a missing baseline with computed attributes falls back to recomputing the delivered policies' reports, a full scan snapshots the baseline even without a policy trigger, and the concierge/introSelected baselines advance only on full-recompute passes so a drifted value still heals on its next delivery. --- .../OnyxDerived/configs/reportAttributes.ts | 167 ++++++++--- src/libs/actions/OnyxDerived/index.ts | 2 +- tests/unit/reportAttributesTest.ts | 274 +++++++++++++----- 3 files changed, 314 insertions(+), 129 deletions(-) diff --git a/src/libs/actions/OnyxDerived/configs/reportAttributes.ts b/src/libs/actions/OnyxDerived/configs/reportAttributes.ts index 05305ce9e832..2889e48dce56 100644 --- a/src/libs/actions/OnyxDerived/configs/reportAttributes.ts +++ b/src/libs/actions/OnyxDerived/configs/reportAttributes.ts @@ -1,6 +1,7 @@ import type {LocalizedTranslate} from '@components/LocaleContextProvider'; import {getReportPreviewAction} from '@libs/actions/IOU/MoneyRequestBuilder'; +import hashCode from '@libs/hashCode'; import {translate as translateForLocale} from '@libs/Localize'; import {getIsOffline} from '@libs/NetworkState'; import {getLinkedTransactionID} from '@libs/ReportActionsUtils'; @@ -49,12 +50,38 @@ const prepareReportKeys = (keys: string[]) => { ]; }; -// A short string built from the policy fields the report attributes depend on. Signatures are stored in -// the derived value (like `locale`), so the change-detection baseline survives app restarts. -const policyRelevantSignature = (policy: Policy | null | undefined): string | null => - policy ? JSON.stringify([policy.type, policy.approvalMode, policy.reimbursementChoice, policy.autoReimbursementLimit, policy.role, policy.autoReimbursement?.limit]) : null; +// Onyx write-bookkeeping keys. They flip during optimistic writes without affecting the computed +// attributes, so they are excluded from policy signatures to avoid recompute churn. +const POLICY_SIGNATURE_EXCLUDED_KEYS = new Set(['pendingAction', 'pendingFields', 'errors', 'errorFields']); -const hasPolicyRelevantFieldChanged = (prev: Policy | null | undefined, next: Policy | null | undefined): boolean => policyRelevantSignature(prev) !== policyRelevantSignature(next); +// Deterministic stringify: object keys are sorted at every level so the same policy content always +// produces the same string regardless of key insertion order after Onyx merges. +const stableStringify = (value: unknown): string => { + if (value === null || typeof value !== 'object') { + return JSON.stringify(value) ?? 'undefined'; + } + if (Array.isArray(value)) { + return `[${value.map(stableStringify).join(',')}]`; + } + const entries = Object.entries(value) + .filter(([key]) => !POLICY_SIGNATURE_EXCLUDED_KEYS.has(key)) + .sort(([a], [b]) => (a < b ? -1 : 1)); + return `{${entries.map(([key, entryValue]) => `${JSON.stringify(key)}:${stableStringify(entryValue)}`).join(',')}}`; +}; + +// A signature of the whole policy content (minus write-bookkeeping keys), stored in the derived value +// (like `locale`) so the change-detection baseline survives app restarts. Hashing the full content +// instead of an enumerated field list means a policy field the compute reads can never silently fall +// outside change detection; over-triggering is safe because the recompute is scoped to that policy's +// reports. The serialized length rides along as a second dimension so a 32-bit hash collision alone +// cannot mask a change; a full collision would only skip one scoped recompute for one policy change. +const policyRelevantSignature = (policy: Policy | null | undefined): string | null => { + if (!policy) { + return null; + } + const serialized = stableStringify(policy); + return `${hashCode(serialized)}.${serialized.length}`; +}; // A short string built from the fields a report name can come from: displayName and firstName. const displayNameSignature = (details: PersonalDetails | null): string => JSON.stringify([details?.displayName ?? '', details?.firstName ?? '']); @@ -260,13 +287,53 @@ export default createOnyxDerivedValueConfig({ hasIsTrackIntentUserChanged; // Policy change detection compares signatures stored in the derived value, so the baseline survives - // app restarts. Without a stored baseline (value written by an older app version) the first policy - // delivery only seeds the signatures — the stored attributes already reflect these policies. + // app restarts. Signatures advance only when the recompute they imply actually runs (or vacuously + // has no reports to touch); otherwise the next delivery re-diffs against the old baseline. const storedPolicySignatures = currentValue?.policySignatures; + const hasComputedReports = !!currentValue?.reports && Object.keys(currentValue.reports).length > 0; let nextPolicySignatures = storedPolicySignatures; + // True when the signatures may persist through an early return: a pure baseline seed, or a diff + // whose changed policies have no reports referencing them (nothing to recompute). + let canPersistSignaturesWithoutRecompute = false; const policyChangedReportKeys: string[] = []; - if (hasKeyTriggeredCompute(ONYXKEYS.COLLECTION.POLICY, sourceValues) || (!storedPolicySignatures && policies)) { - if (!needsFullRecompute && storedPolicySignatures && hasKeyTriggeredCompute(ONYXKEYS.COLLECTION.POLICY, sourceValues)) { + + const buildAllPolicySignatures = () => { + const signatures: Record = {}; + for (const [key, policy] of Object.entries(policies ?? {})) { + const signature = policyRelevantSignature(policy); + if (signature !== null) { + signatures[key] = signature; + } + } + return signatures; + }; + const collectReportKeysForPolicies = (changedPolicyIDs: Set) => { + for (const [reportKey, report] of Object.entries(reports ?? {})) { + if (!report) { + continue; + } + // The report's own policy — the sender workspace for an invoice. + if (report.policyID && changedPolicyIDs.has(report.policyID)) { + policyChangedReportKeys.push(reportKey); + continue; + } + // An invoice follows its receiver workspace. The invoice room carries the receiver + // on itself; a child invoice report doesn't, so we read it from its parent room + // (chatReportID) — the same place computeReportName looks for the invoice name. + const ownReceiverPolicyID = report.invoiceReceiver && 'policyID' in report.invoiceReceiver ? report.invoiceReceiver.policyID : undefined; + const room = report.chatReportID ? reports?.[`${ONYXKEYS.COLLECTION.REPORT}${report.chatReportID}`] : undefined; + const roomReceiverPolicyID = room?.invoiceReceiver && 'policyID' in room.invoiceReceiver ? room.invoiceReceiver.policyID : undefined; + if ((ownReceiverPolicyID && changedPolicyIDs.has(ownReceiverPolicyID)) || (roomReceiverPolicyID && changedPolicyIDs.has(roomReceiverPolicyID))) { + policyChangedReportKeys.push(reportKey); + } + } + }; + + if (hasKeyTriggeredCompute(ONYXKEYS.COLLECTION.POLICY, sourceValues)) { + if (needsFullRecompute) { + // Every report recomputes with the current policies anyway — snapshot the full baseline. + nextPolicySignatures = buildAllPolicySignatures(); + } else if (storedPolicySignatures) { const changedPolicyIDs = new Set(); const updatedSignatures = {...storedPolicySignatures}; for (const key of Object.keys(sourceValues?.[ONYXKEYS.COLLECTION.POLICY] ?? {})) { @@ -283,50 +350,42 @@ export default createOnyxDerivedValueConfig({ } if (changedPolicyIDs.size > 0) { nextPolicySignatures = updatedSignatures; - for (const [reportKey, report] of Object.entries(reports ?? {})) { - if (!report) { - continue; - } - // The report's own policy — the sender workspace for an invoice. - if (report.policyID && changedPolicyIDs.has(report.policyID)) { - policyChangedReportKeys.push(reportKey); - continue; - } - // An invoice follows its receiver workspace. The invoice room carries the receiver - // on itself; a child invoice report doesn't, so we read it from its parent room - // (chatReportID) — the same place computeReportName looks for the invoice name. - const ownReceiverPolicyID = report.invoiceReceiver && 'policyID' in report.invoiceReceiver ? report.invoiceReceiver.policyID : undefined; - const room = report.chatReportID ? reports?.[`${ONYXKEYS.COLLECTION.REPORT}${report.chatReportID}`] : undefined; - const roomReceiverPolicyID = room?.invoiceReceiver && 'policyID' in room.invoiceReceiver ? room.invoiceReceiver.policyID : undefined; - if ((ownReceiverPolicyID && changedPolicyIDs.has(ownReceiverPolicyID)) || (roomReceiverPolicyID && changedPolicyIDs.has(roomReceiverPolicyID))) { - policyChangedReportKeys.push(reportKey); - } - } + collectReportKeysForPolicies(changedPolicyIDs); + // Vacuously satisfied only when the reports collection is actually available — + // with it missing, the recompute is skipped rather than unnecessary. + canPersistSignaturesWithoutRecompute = !!reports && policyChangedReportKeys.length === 0; } - } else { - const signatures: Record = {}; - for (const [key, policy] of Object.entries(policies ?? {})) { - const signature = policyRelevantSignature(policy); - if (signature !== null) { - signatures[key] = signature; - } + } else if (hasComputedReports) { + // No stored baseline but attributes exist (value written by an older app version, or computed + // in-session before policies loaded) — we cannot tell whether these policies match the ones the + // attributes were computed with, so recompute the delivered policies' reports like the + // pre-signature code did, then snapshot the full baseline. + const deliveredPolicyIDs = new Set(Object.keys(sourceValues?.[ONYXKEYS.COLLECTION.POLICY] ?? {}).map((key) => key.replace(ONYXKEYS.COLLECTION.POLICY, ''))); + collectReportKeysForPolicies(deliveredPolicyIDs); + canPersistSignaturesWithoutRecompute = !!reports && policyChangedReportKeys.length === 0; + if (reports) { + nextPolicySignatures = buildAllPolicySignatures(); } - nextPolicySignatures = signatures; + } else { + // No attributes computed yet — the pass below runs a full scan, so seeding is safe. + nextPolicySignatures = buildAllPolicySignatures(); } + } else if (!storedPolicySignatures && policies && hasComputedReports) { + // Baseline seeding on a pass that did not deliver policies: both the attributes and the policies + // come from the same disk-hydrated state (an in-session value computed without policies is + // impossible here — policies would have arrived as a policy trigger), so they are consistent. + nextPolicySignatures = buildAllPolicySignatures(); + canPersistSignaturesWithoutRecompute = true; } - // One-time baseline writes (policy signatures / concierge / intro seeds) must persist even when this - // pass otherwise returns the current value unchanged. + // Baseline writes that may ride an early return: only signature snapshots that require no recompute. + // conciergeReportID/isTrackIntentUser baselines advance exclusively in the final return, and only on + // passes that recomputed everything with the current values — advancing them on other passes would + // silently absorb a change and suppress the full recompute its next delivery should cause. const metaPatch: Partial = {}; - if (nextPolicySignatures !== storedPolicySignatures) { + if (canPersistSignaturesWithoutRecompute && nextPolicySignatures !== storedPolicySignatures) { metaPatch.policySignatures = nextPolicySignatures; } - if (storedConciergeReportID !== (conciergeReportID ?? null)) { - metaPatch.conciergeReportID = conciergeReportID ?? null; - } - if (storedIsTrackIntentUser !== nextIsTrackIntentUser) { - metaPatch.isTrackIntentUser = nextIsTrackIntentUser; - } const withMetaPatch = (value: ReportAttributesDerivedValue): ReportAttributesDerivedValue => (Object.keys(metaPatch).length > 0 ? {...value, ...metaPatch} : value); // Use incremental updates when currentValue is already populated and no full recompute is required. @@ -637,14 +696,26 @@ export default createOnyxDerivedValueConfig({ }; } + // A full scan just recomputed every report with the current policies, so the baseline can be + // snapshotted even when no policy trigger fired this pass (e.g. policies merged while computes + // were deferred during startup, or a locale-driven full recompute). + if (!useIncrementalUpdates && policies) { + nextPolicySignatures = buildAllPolicySignatures(); + } + + // The stored conciergeReportID/isTrackIntentUser must always equal the values the full attribute + // set was computed with, so they advance only when everything recomputed with the current values + // (or on the very first write). Incremental passes keep the stored baseline — if the current value + // drifted (e.g. a change landed while computes were deferred), the next delivery of that key still + // compares unequal and triggers the healing full recompute. return { reports: reportAttributes, locale: preferredLocale ?? null, policySignatures: nextPolicySignatures, - conciergeReportID: conciergeReportID ?? null, - isTrackIntentUser: nextIsTrackIntentUser, + conciergeReportID: storedConciergeReportID === undefined || needsFullRecompute ? (conciergeReportID ?? null) : storedConciergeReportID, + isTrackIntentUser: storedIsTrackIntentUser === undefined || needsFullRecompute ? nextIsTrackIntentUser : storedIsTrackIntentUser, }; }, }); -export {hasPolicyRelevantFieldChanged, policyRelevantSignature}; +export {policyRelevantSignature}; diff --git a/src/libs/actions/OnyxDerived/index.ts b/src/libs/actions/OnyxDerived/index.ts index 0b0870d6a2d5..73aafa4ad17c 100644 --- a/src/libs/actions/OnyxDerived/index.ts +++ b/src/libs/actions/OnyxDerived/index.ts @@ -88,7 +88,7 @@ function init() { name: CONST.TELEMETRY.SPAN_ONYX_DERIVED_COMPUTE, op: CONST.TELEMETRY.SPAN_ONYX_DERIVED_COMPUTE, parentSpan: getSpan(CONST.TELEMETRY.SPAN_APP_STARTUP), - attributes: {derivedKey: key, [CONST.TELEMETRY.ATTRIBUTE_DERIVED_TRIGGER]: sourceKey ?? 'initial'}, + attributes: {derivedKey: key, [CONST.TELEMETRY.ATTRIBUTE_DERIVED_TRIGGER]: sourceKey ?? 'unknown'}, }); try { diff --git a/tests/unit/reportAttributesTest.ts b/tests/unit/reportAttributesTest.ts index bef085604b75..85319af2243a 100644 --- a/tests/unit/reportAttributesTest.ts +++ b/tests/unit/reportAttributesTest.ts @@ -1,5 +1,5 @@ import type reportAttributesModuleDefault from '@userActions/OnyxDerived/configs/reportAttributes'; -import {hasPolicyRelevantFieldChanged, policyRelevantSignature} from '@userActions/OnyxDerived/configs/reportAttributes'; +import {policyRelevantSignature} from '@userActions/OnyxDerived/configs/reportAttributes'; import CONST from '@src/CONST'; import ONYXKEYS from '@src/ONYXKEYS'; @@ -51,81 +51,45 @@ const basePolicy: Policy = { autoReimbursement: {limit: 500}, } as unknown as Policy; -describe('hasPolicyRelevantFieldChanged', () => { +describe('policyRelevantSignature', () => { describe('null / undefined edge cases', () => { - it('returns false when both are null', () => { - expect(hasPolicyRelevantFieldChanged(null, null)).toBe(false); - }); - - it('returns false when both are undefined', () => { - expect(hasPolicyRelevantFieldChanged(undefined, undefined)).toBe(false); - }); - - it('returns false when both are null/undefined mix', () => { - expect(hasPolicyRelevantFieldChanged(null, undefined)).toBe(false); - expect(hasPolicyRelevantFieldChanged(undefined, null)).toBe(false); - }); - - it('returns true when prev is null and next has a policy', () => { - expect(hasPolicyRelevantFieldChanged(null, basePolicy)).toBe(true); - }); - - it('returns true when next is null and prev had a policy', () => { - expect(hasPolicyRelevantFieldChanged(basePolicy, null)).toBe(true); + it('returns null for missing policies', () => { + expect(policyRelevantSignature(null)).toBeNull(); + expect(policyRelevantSignature(undefined)).toBeNull(); }); }); - describe('identical policies', () => { - it('returns false when all tracked fields are the same', () => { - const copy = {...basePolicy}; - expect(hasPolicyRelevantFieldChanged(basePolicy, copy)).toBe(false); - }); - - it('returns false when only a non-tracked field changes', () => { - const updated = {...basePolicy, name: 'Updated Name'} as unknown as Policy; - expect(hasPolicyRelevantFieldChanged(basePolicy, updated)).toBe(false); - }); - }); - - describe('tracked field changes', () => { - it('returns true when type changes', () => { - const updated = {...basePolicy, type: CONST.POLICY.TYPE.TEAM} as unknown as Policy; - expect(hasPolicyRelevantFieldChanged(basePolicy, updated)).toBe(true); - }); - - it('returns true when approvalMode changes', () => { - const updated = {...basePolicy, approvalMode: CONST.POLICY.APPROVAL_MODE.OPTIONAL} as unknown as Policy; - expect(hasPolicyRelevantFieldChanged(basePolicy, updated)).toBe(true); - }); - - it('returns true when reimbursementChoice changes', () => { - const updated = {...basePolicy, reimbursementChoice: CONST.POLICY.REIMBURSEMENT_CHOICES.REIMBURSEMENT_NO} as unknown as Policy; - expect(hasPolicyRelevantFieldChanged(basePolicy, updated)).toBe(true); - }); - - it('returns true when autoReimbursementLimit changes', () => { - const updated = {...basePolicy, autoReimbursementLimit: 2000} as unknown as Policy; - expect(hasPolicyRelevantFieldChanged(basePolicy, updated)).toBe(true); - }); - - it('returns true when role changes', () => { - const updated = {...basePolicy, role: CONST.POLICY.ROLE.USER} as unknown as Policy; - expect(hasPolicyRelevantFieldChanged(basePolicy, updated)).toBe(true); + describe('identical content', () => { + it('produces the same signature for a shallow copy', () => { + expect(policyRelevantSignature({...basePolicy})).toBe(policyRelevantSignature(basePolicy)); }); - it('returns true when autoReimbursement.limit changes', () => { - const updated = {...basePolicy, autoReimbursement: {limit: 999}} as unknown as Policy; - expect(hasPolicyRelevantFieldChanged(basePolicy, updated)).toBe(true); + it('produces the same signature regardless of key insertion order', () => { + const reordered = Object.fromEntries(Object.entries(basePolicy).reverse()) as unknown as Policy; + expect(policyRelevantSignature(reordered)).toBe(policyRelevantSignature(basePolicy)); }); - it('returns true when autoReimbursement goes from defined to undefined', () => { - const updated = {...basePolicy, autoReimbursement: undefined} as unknown as Policy; - expect(hasPolicyRelevantFieldChanged(basePolicy, updated)).toBe(true); + it('ignores Onyx write-bookkeeping keys', () => { + const withWriteNoise = {...basePolicy, pendingAction: 'update', pendingFields: {name: 'update'}, errors: {a: 'err'}, errorFields: {name: {a: 'err'}}} as unknown as Policy; + expect(policyRelevantSignature(withWriteNoise)).toBe(policyRelevantSignature(basePolicy)); }); + }); - it('returns true when autoReimbursement goes from undefined to defined', () => { - const withoutAutoReimburse = {...basePolicy, autoReimbursement: undefined} as unknown as Policy; - expect(hasPolicyRelevantFieldChanged(withoutAutoReimburse, basePolicy)).toBe(true); + describe('content changes', () => { + const changes: Array<[string, Partial]> = [ + ['type', {type: CONST.POLICY.TYPE.TEAM}], + ['approvalMode', {approvalMode: CONST.POLICY.APPROVAL_MODE.OPTIONAL}], + ['reimbursementChoice', {reimbursementChoice: CONST.POLICY.REIMBURSEMENT_CHOICES.REIMBURSEMENT_NO}], + ['autoReimbursementLimit', {autoReimbursementLimit: 2000}], + ['role', {role: CONST.POLICY.ROLE.USER}], + ['autoReimbursement.limit', {autoReimbursement: {limit: 999}}], + ['autoReimbursement removed', {autoReimbursement: undefined}], + ['name', {name: 'Renamed Workspace'}], + ['employeeList', {employeeList: {['a@b.com' as string]: {submitsTo: 'c@d.com'}}}], + ]; + it.each(changes)('changes the signature when %s changes', (_label, change) => { + const updated = {...basePolicy, ...change} as unknown as Policy; + expect(policyRelevantSignature(updated)).not.toBe(policyRelevantSignature(basePolicy)); }); }); }); @@ -169,7 +133,12 @@ describe('reportAttributes compute — policy change code flow', () => { config = (require('@userActions/OnyxDerived/configs/reportAttributes') as {default: ReportAttributesConfig}).default; }); - const buildArgs = (overridePolicies?: OnyxCollection, overrideReports?: OnyxCollection, transactionsUpdate?: OnyxCollection | null) => + const buildArgs = ( + overridePolicies?: OnyxCollection, + overrideReports?: OnyxCollection, + transactionsUpdate?: OnyxCollection | null, + conciergeReportID?: string, + ) => [ overrideReports ?? reports, // reports null, // preferredLocale @@ -181,8 +150,8 @@ describe('reportAttributes compute — policy change code flow', () => { null, // session overridePolicies ?? policies, // policies null, // policyTags - null, // reportViolations - null, // reportMetadata + conciergeReportID ?? null, // conciergeReportID + null, // introSelected ] as unknown as Parameters[0]; it('computes every report on a cold start (no currentValue) when policies load', () => { @@ -195,10 +164,11 @@ describe('reportAttributes compute — policy change code flow', () => { expect(result?.reports).toHaveProperty('r2'); }); - it('seeds policy signatures without recomputing when the stored value has no baseline', () => { - // Reproduces the first policy delivery after startup on a value written by an older app version: - // the stored attributes already reflect these policies, so nothing recomputes — the pass only - // records the signatures so later deliveries can be diffed. + it('recomputes reports of delivered policies when the stored value has no signature baseline', () => { + // Reproduces the first policy delivery on a value with no baseline (written by an older app + // version, or computed in-session before policies loaded): we cannot tell whether the stored + // attributes match these policies, so the delivered policies' reports recompute — exactly like + // the pre-signature code did on its first in-session delivery — and the full baseline is recorded. const report3: Report = {...createRandomReport(12, undefined), reportID: 'r3', policyID: 'policyOther', chatReportID: undefined}; const reportsWithUnrelated: OnyxCollection = { ...reports, @@ -219,8 +189,10 @@ describe('reportAttributes compute — policy change code flow', () => { sourceValues: {[ONYXKEYS.COLLECTION.POLICY]: policies}, }); - expect(result?.reports.r1?.reportName).toBe('Old Name 1'); - expect(result?.reports.r2?.reportName).toBe('Old Name 2'); + // r1/r2 reference the delivered policies → recomputed (default mock name). + expect(result?.reports.r1?.reportName).toBe('Test Report'); + expect(result?.reports.r2?.reportName).toBe('Test Report'); + // r3 references a policy that was not delivered → keeps its existing value. expect(result?.reports.r3?.reportName).toBe('Old Name 3'); expect(result?.policySignatures).toEqual({ [`${ONYXKEYS.COLLECTION.POLICY}policy1`]: policyRelevantSignature(policy1), @@ -228,6 +200,30 @@ describe('reportAttributes compute — policy change code flow', () => { }); }); + it('seeds policy signatures without recomputing on a pass that did not deliver policies', () => { + // Disk-hydrated policies and a disk-restored value are consistent, so a non-policy pass just + // records the baseline; only the report from this pass's own update recomputes. + const existingValue: ReportAttributesDerivedValue = { + reports: { + r1: {reportName: 'Old Name 1', isEmpty: false, brickRoadStatus: undefined, requiresAttention: false, reportErrors: {}}, + r2: {reportName: 'Old Name 2', isEmpty: false, brickRoadStatus: undefined, requiresAttention: false, reportErrors: {}}, + }, + locale: null, + }; + + const result = config.compute(buildArgs(), { + currentValue: existingValue, + sourceValues: {[ONYXKEYS.COLLECTION.REPORT]: {[`${ONYXKEYS.COLLECTION.REPORT}r1`]: report1}}, + }); + + expect(result?.reports.r1?.reportName).toBe('Test Report'); + expect(result?.reports.r2?.reportName).toBe('Old Name 2'); + expect(result?.policySignatures).toEqual({ + [`${ONYXKEYS.COLLECTION.POLICY}policy1`]: policyRelevantSignature(policy1), + [`${ONYXKEYS.COLLECTION.POLICY}policy2`]: policyRelevantSignature(policy2), + }); + }); + it('scopes a policy delivery to reports whose stored signature differs when currentValue is already populated', () => { // Reproduces the ReconnectApp-after-restart case: attributes and signatures were persisted, then // ~1k policies land and some actually changed. Only reports whose policy signature differs should @@ -348,11 +344,11 @@ describe('reportAttributes compute — policy change code flow', () => { expect(result?.reports.r2?.reportName).toBe('Old Name 2'); }); - it('skips recompute when a non-tracked policy field changes', () => { - const policy1WithNameChange = {...policy1, name: 'New Policy Name'} as unknown as Policy; + it('skips recompute when only Onyx write-bookkeeping keys change on a policy', () => { + const policy1WithWriteNoise = {...policy1, pendingFields: {generalSettings: 'update'}, errorFields: {generalSettings: {a: 'err'}}} as unknown as Policy; const updatedPolicies: OnyxCollection = { ...policies, - [`${ONYXKEYS.COLLECTION.POLICY}policy1`]: policy1WithNameChange, + [`${ONYXKEYS.COLLECTION.POLICY}policy1`]: policy1WithWriteNoise, }; const existingValue: ReportAttributesDerivedValue = { @@ -371,13 +367,131 @@ describe('reportAttributes compute — policy change code flow', () => { const result = config.compute(buildArgs(updatedPolicies), { currentValue: existingValue, - sourceValues: {[ONYXKEYS.COLLECTION.POLICY]: {[`${ONYXKEYS.COLLECTION.POLICY}policy1`]: policy1WithNameChange} as never}, + sourceValues: {[ONYXKEYS.COLLECTION.POLICY]: {[`${ONYXKEYS.COLLECTION.POLICY}policy1`]: policy1WithWriteNoise} as never}, }); - // No tracked fields changed → return currentValue unchanged + // No content change → return currentValue unchanged expect(result).toEqual(existingValue); }); + it('recomputes reports of a policy whose name changes', () => { + const policy1Renamed = {...policy1, name: 'Renamed Workspace'} as unknown as Policy; + const updatedPolicies: OnyxCollection = { + ...policies, + [`${ONYXKEYS.COLLECTION.POLICY}policy1`]: policy1Renamed, + }; + + const existingValue: ReportAttributesDerivedValue = { + reports: { + r1: {reportName: 'Old Name 1', isEmpty: false, brickRoadStatus: undefined, requiresAttention: false, reportErrors: {}}, + r2: {reportName: 'Old Name 2', isEmpty: false, brickRoadStatus: undefined, requiresAttention: false, reportErrors: {}}, + }, + locale: null, + policySignatures: { + [`${ONYXKEYS.COLLECTION.POLICY}policy1`]: signatureOf(policy1), + [`${ONYXKEYS.COLLECTION.POLICY}policy2`]: signatureOf(policy2), + }, + }; + + const result = config.compute(buildArgs(updatedPolicies), { + currentValue: existingValue, + sourceValues: {[ONYXKEYS.COLLECTION.POLICY]: {[`${ONYXKEYS.COLLECTION.POLICY}policy1`]: policy1Renamed} as never}, + }); + + // Report names embed the workspace name, so a rename recomputes that policy's reports. + expect(result?.reports.r1?.reportName).toBe('Test Report'); + expect(result?.reports.r2?.reportName).toBe('Old Name 2'); + expect(result?.policySignatures?.[`${ONYXKEYS.COLLECTION.POLICY}policy1`]).toBe(signatureOf(policy1Renamed)); + }); + + it('keeps the concierge baseline on passes that did not recompute everything', () => { + const existingValue: ReportAttributesDerivedValue = { + reports: { + r1: {reportName: 'Old Name 1', isEmpty: false, brickRoadStatus: undefined, requiresAttention: false, reportErrors: {}}, + r2: {reportName: 'Old Name 2', isEmpty: false, brickRoadStatus: undefined, requiresAttention: false, reportErrors: {}}, + }, + locale: null, + policySignatures: { + [`${ONYXKEYS.COLLECTION.POLICY}policy1`]: signatureOf(policy1), + [`${ONYXKEYS.COLLECTION.POLICY}policy2`]: signatureOf(policy2), + }, + conciergeReportID: 'conciergeOld', + isTrackIntentUser: false, + }; + + // An incremental pass while the current conciergeReportID already drifted (it changed while + // computes were deferred): the baseline must NOT advance, or the change would be absorbed. + const incrementalResult = config.compute(buildArgs(policies, undefined, null, 'conciergeNew'), { + currentValue: existingValue, + sourceValues: {[ONYXKEYS.COLLECTION.REPORT]: {[`${ONYXKEYS.COLLECTION.REPORT}r1`]: report1}}, + }); + expect(incrementalResult?.conciergeReportID).toBe('conciergeOld'); + expect(incrementalResult?.reports.r2?.reportName).toBe('Old Name 2'); + + // The next delivery of the key compares against the preserved baseline → full recompute + advance. + const deliveryResult = config.compute(buildArgs(policies, undefined, null, 'conciergeNew'), { + currentValue: incrementalResult, + sourceValues: {[ONYXKEYS.CONCIERGE_REPORT_ID]: 'conciergeNew' as never}, + }); + expect(deliveryResult?.conciergeReportID).toBe('conciergeNew'); + expect(deliveryResult?.reports.r2?.reportName).toBe('Test Report'); + }); + + it('does not persist advanced policy signatures when the reports collection is unavailable', () => { + const policy1Changed = {...policy1, approvalMode: CONST.POLICY.APPROVAL_MODE.OPTIONAL} as unknown as Policy; + const updatedPolicies: OnyxCollection = { + ...policies, + [`${ONYXKEYS.COLLECTION.POLICY}policy1`]: policy1Changed, + }; + const existingValue: ReportAttributesDerivedValue = { + reports: { + r1: {reportName: 'Old Name 1', isEmpty: false, brickRoadStatus: undefined, requiresAttention: false, reportErrors: {}}, + }, + locale: null, + policySignatures: { + [`${ONYXKEYS.COLLECTION.POLICY}policy1`]: signatureOf(policy1), + [`${ONYXKEYS.COLLECTION.POLICY}policy2`]: signatureOf(policy2), + }, + }; + + const argsWithoutReports = [ + undefined, // reports + null, // preferredLocale + null, // transactionViolations + null, // reportActions + null, // reportNameValuePairs + null, // transactions + null, // personalDetails + null, // session + updatedPolicies, // policies + null, // policyTags + null, // conciergeReportID + null, // introSelected + ] as unknown as Parameters[0]; + + const result = config.compute(argsWithoutReports, { + currentValue: existingValue, + sourceValues: {[ONYXKEYS.COLLECTION.POLICY]: {[`${ONYXKEYS.COLLECTION.POLICY}policy1`]: policy1Changed} as never}, + }); + + // The scoped recompute could not run, so the old baseline must survive — the next delivery + // re-diffs against it and retries the recompute. + expect(result?.policySignatures?.[`${ONYXKEYS.COLLECTION.POLICY}policy1`]).toBe(signatureOf(policy1)); + }); + + it('snapshots the policy baseline after a full scan even without a policy trigger', () => { + const result = config.compute(buildArgs(), { + currentValue: undefined, + sourceValues: {[ONYXKEYS.COLLECTION.REPORT]: {[`${ONYXKEYS.COLLECTION.REPORT}r1`]: report1}}, + }); + + expect(result?.reports.r1?.reportName).toBe('Test Report'); + expect(result?.policySignatures).toEqual({ + [`${ONYXKEYS.COLLECTION.POLICY}policy1`]: signatureOf(policy1), + [`${ONYXKEYS.COLLECTION.POLICY}policy2`]: signatureOf(policy2), + }); + }); + it('recomputes the parent workspace chat when a transaction on its expense report changes', () => { const expenseReport: Report = {...createRandomReport(10, undefined), reportID: 'expense1', policyID: 'policy3', chatReportID: 'chat1'}; const chatReport: Report = {...createRandomReport(11, CONST.REPORT.CHAT_TYPE.POLICY_EXPENSE_CHAT), reportID: 'chat1', policyID: 'policy3', chatReportID: undefined}; From 31db952e73ea16f18e9fbdf8877fb243731362ab Mon Sep 17 00:00:00 2001 From: sumo_slonik Date: Tue, 14 Jul 2026 20:23:55 +0200 Subject: [PATCH 3/3] Address second-review findings on persisted reportAttributes baselines The concierge baseline sentinel is an empty string because Onyx.set strips nested null values on persist. Policy signatures additionally exclude loading flags, lastModified and the accounting connections subtree (volatile and never read by the compute), and carry a format version so older stored signatures degrade to one scoped recompute. The all-policies snapshot is built once per pass and useIncrementalUpdates reuses hasComputedReports. --- .../OnyxDerived/configs/reportAttributes.ts | 50 ++++++++++++++++--- src/types/onyx/DerivedValues.ts | 5 +- tests/unit/reportAttributesTest.ts | 34 ++++++++++++- 3 files changed, 78 insertions(+), 11 deletions(-) diff --git a/src/libs/actions/OnyxDerived/configs/reportAttributes.ts b/src/libs/actions/OnyxDerived/configs/reportAttributes.ts index 2889e48dce56..4332e80762d4 100644 --- a/src/libs/actions/OnyxDerived/configs/reportAttributes.ts +++ b/src/libs/actions/OnyxDerived/configs/reportAttributes.ts @@ -50,9 +50,27 @@ const prepareReportKeys = (keys: string[]) => { ]; }; -// Onyx write-bookkeeping keys. They flip during optimistic writes without affecting the computed -// attributes, so they are excluded from policy signatures to avoid recompute churn. -const POLICY_SIGNATURE_EXCLUDED_KEYS = new Set(['pendingAction', 'pendingFields', 'errors', 'errorFields']); +// Keys excluded from policy signatures because they change without affecting the computed attributes: +// Onyx write-bookkeeping keys and loading flags flip during every optimistic write / API call, and +// `connections` (accounting integration configs, incl. per-sync timestamps) is both volatile and large +// while the attribute computation never reads it. Excluding them prevents recompute churn. +const POLICY_SIGNATURE_EXCLUDED_KEYS = new Set([ + 'pendingAction', + 'pendingFields', + 'errors', + 'errorFields', + 'connections', + 'isLoading', + 'isLoadingWorkspaceReimbursement', + 'isLoadingReceiptPartners', + 'isChangeOwnerSuccessful', + 'isChangeOwnerFailed', + 'lastModified', +]); + +// Bump when the signature format or exclusion set changes: stored signatures from an older format then +// mismatch, which safely degrades to a scoped recompute of the delivered policies' reports once. +const POLICY_SIGNATURE_VERSION = '2'; // Deterministic stringify: object keys are sorted at every level so the same policy content always // produces the same string regardless of key insertion order after Onyx merges. @@ -80,7 +98,7 @@ const policyRelevantSignature = (policy: Policy | null | undefined): string | nu return null; } const serialized = stableStringify(policy); - return `${hashCode(serialized)}.${serialized.length}`; + return `${POLICY_SIGNATURE_VERSION}|${hashCode(serialized)}.${serialized.length}`; }; // A short string built from the fields a report name can come from: displayName and firstName. @@ -270,10 +288,16 @@ export default createOnyxDerivedValueConfig({ // conciergeReportID and introSelected are re-delivered on every OpenApp/reconnect merge, so a full // recompute happens only when the value differs from the one the attributes were computed with. // A missing stored value (attributes written by an older app version) seeds the baseline instead. - const storedConciergeReportID = currentValue && 'conciergeReportID' in currentValue ? (currentValue.conciergeReportID ?? null) : undefined; + // "No concierge report yet" is stored as an empty string, NOT null — Onyx.set strips nested null + // values on persist, so a null baseline would read back as missing after a restart and seed + // instead of detecting the change. + // eslint-disable-next-line rulesdir/no-default-id-values -- '' is a persistable baseline sentinel for "computed without a concierge report ID", never used as a lookup ID + const nextConciergeReportID = conciergeReportID ?? ''; + // eslint-disable-next-line rulesdir/no-default-id-values -- same sentinel when reading values persisted before the field became non-nullable + const storedConciergeReportID = currentValue && 'conciergeReportID' in currentValue ? (currentValue.conciergeReportID ?? '') : undefined; const storedIsTrackIntentUser = currentValue && 'isTrackIntentUser' in currentValue ? currentValue.isTrackIntentUser : undefined; const hasConciergeReportIDChanged = - hasKeyTriggeredCompute(ONYXKEYS.CONCIERGE_REPORT_ID, sourceValues) && storedConciergeReportID !== undefined && storedConciergeReportID !== (conciergeReportID ?? null); + hasKeyTriggeredCompute(ONYXKEYS.CONCIERGE_REPORT_ID, sourceValues) && storedConciergeReportID !== undefined && storedConciergeReportID !== nextConciergeReportID; const hasIsTrackIntentUserChanged = hasKeyTriggeredCompute(ONYXKEYS.NVP_INTRO_SELECTED, sourceValues) && storedIsTrackIntentUser !== undefined && storedIsTrackIntentUser !== nextIsTrackIntentUser; @@ -297,7 +321,13 @@ export default createOnyxDerivedValueConfig({ let canPersistSignaturesWithoutRecompute = false; const policyChangedReportKeys: string[] = []; + // Lazily built and cached — a full-recompute pass with a policy trigger would otherwise + // serialize every policy twice (once here, once in the pre-return snapshot). + let allPolicySignaturesCache: Record | undefined; const buildAllPolicySignatures = () => { + if (allPolicySignaturesCache) { + return allPolicySignaturesCache; + } const signatures: Record = {}; for (const [key, policy] of Object.entries(policies ?? {})) { const signature = policyRelevantSignature(policy); @@ -305,6 +335,7 @@ export default createOnyxDerivedValueConfig({ signatures[key] = signature; } } + allPolicySignaturesCache = signatures; return signatures; }; const collectReportKeysForPolicies = (changedPolicyIDs: Set) => { @@ -374,6 +405,9 @@ export default createOnyxDerivedValueConfig({ // Baseline seeding on a pass that did not deliver policies: both the attributes and the policies // come from the same disk-hydrated state (an in-session value computed without policies is // impossible here — policies would have arrived as a policy trigger), so they are consistent. + // Known one-time limitation: on the FIRST session after this feature ships, a policy change + // merged while startup computes are still deferred is seeded here without a recompute; from the + // next value write onward the persisted baseline closes that window. nextPolicySignatures = buildAllPolicySignatures(); canPersistSignaturesWithoutRecompute = true; } @@ -390,7 +424,7 @@ export default createOnyxDerivedValueConfig({ // Use incremental updates when currentValue is already populated and no full recompute is required. // If currentValue has no reports (fresh install or cleared storage), fall back to a full scan. - const useIncrementalUpdates = !!currentValue?.reports && Object.keys(currentValue.reports).length > 0 && !needsFullRecompute; + const useIncrementalUpdates = hasComputedReports && !needsFullRecompute; // if we already computed the report attributes and there is no new reports data, return the current value if ((useIncrementalUpdates && !sourceValues) || !reports) { @@ -712,7 +746,7 @@ export default createOnyxDerivedValueConfig({ reports: reportAttributes, locale: preferredLocale ?? null, policySignatures: nextPolicySignatures, - conciergeReportID: storedConciergeReportID === undefined || needsFullRecompute ? (conciergeReportID ?? null) : storedConciergeReportID, + conciergeReportID: storedConciergeReportID === undefined || needsFullRecompute ? nextConciergeReportID : storedConciergeReportID, isTrackIntentUser: storedIsTrackIntentUser === undefined || needsFullRecompute ? nextIsTrackIntentUser : storedIsTrackIntentUser, }; }, diff --git a/src/types/onyx/DerivedValues.ts b/src/types/onyx/DerivedValues.ts index 82c70e9d4721..9c56ea5083f5 100644 --- a/src/types/onyx/DerivedValues.ts +++ b/src/types/onyx/DerivedValues.ts @@ -75,9 +75,10 @@ type ReportAttributesDerivedValue = { */ policySignatures?: Record; /** - * The conciergeReportID used to compute the report attributes. + * The conciergeReportID used to compute the report attributes. An empty string means the attributes + * were computed without one (null is not stored — Onyx.set strips nested null values on persist). */ - conciergeReportID?: string | null; + conciergeReportID?: string; /** * Whether the user was a track-intent user when the attributes were computed. */ diff --git a/tests/unit/reportAttributesTest.ts b/tests/unit/reportAttributesTest.ts index 85319af2243a..0ff12c3e4a0f 100644 --- a/tests/unit/reportAttributesTest.ts +++ b/tests/unit/reportAttributesTest.ts @@ -361,7 +361,7 @@ describe('reportAttributes compute — policy change code flow', () => { [`${ONYXKEYS.COLLECTION.POLICY}policy1`]: signatureOf(policy1), [`${ONYXKEYS.COLLECTION.POLICY}policy2`]: signatureOf(policy2), }, - conciergeReportID: null, + conciergeReportID: '', isTrackIntentUser: false, }; @@ -479,6 +479,38 @@ describe('reportAttributes compute — policy change code flow', () => { expect(result?.policySignatures?.[`${ONYXKEYS.COLLECTION.POLICY}policy1`]).toBe(signatureOf(policy1)); }); + it('persists advanced signatures without recompute when the changed policy has no reports', () => { + const policyNoReports: Policy = {...basePolicy, id: 'policyNoReports'}; + const policyNoReportsChanged: Policy = {...policyNoReports, approvalMode: CONST.POLICY.APPROVAL_MODE.OPTIONAL}; + const policiesWithExtra: OnyxCollection = { + ...policies, + [`${ONYXKEYS.COLLECTION.POLICY}policyNoReports`]: policyNoReportsChanged, + }; + const existingValue: ReportAttributesDerivedValue = { + reports: { + r1: {reportName: 'Old Name 1', isEmpty: false, brickRoadStatus: undefined, requiresAttention: false, reportErrors: {}}, + r2: {reportName: 'Old Name 2', isEmpty: false, brickRoadStatus: undefined, requiresAttention: false, reportErrors: {}}, + }, + locale: null, + policySignatures: { + [`${ONYXKEYS.COLLECTION.POLICY}policy1`]: signatureOf(policy1), + [`${ONYXKEYS.COLLECTION.POLICY}policy2`]: signatureOf(policy2), + [`${ONYXKEYS.COLLECTION.POLICY}policyNoReports`]: signatureOf(policyNoReports), + }, + }; + + const result = config.compute(buildArgs(policiesWithExtra), { + currentValue: existingValue, + sourceValues: {[ONYXKEYS.COLLECTION.POLICY]: {[`${ONYXKEYS.COLLECTION.POLICY}policyNoReports`]: policyNoReportsChanged} as never}, + }); + + // No report references the changed policy, so the recompute is vacuous — nothing recomputes, + // but the advanced signature persists so the same delivery is not re-diffed forever. + expect(result?.reports.r1?.reportName).toBe('Old Name 1'); + expect(result?.reports.r2?.reportName).toBe('Old Name 2'); + expect(result?.policySignatures?.[`${ONYXKEYS.COLLECTION.POLICY}policyNoReports`]).toBe(signatureOf(policyNoReportsChanged)); + }); + it('snapshots the policy baseline after a full scan even without a policy trigger', () => { const result = config.compute(buildArgs(), { currentValue: undefined,