Persist reportAttributes recompute baselines so app restarts do not t… - #96071
Closed
sumo-slonik wants to merge 13 commits into
Closed
Persist reportAttributes recompute baselines so app restarts do not t…#96071sumo-slonik wants to merge 13 commits into
sumo-slonik wants to merge 13 commits into
Conversation
…rigger 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.
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.
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.
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
A value persisted before these fields existed left them permanently undefined if the key that should detect a change landed on a pass with no other updates, since the early-return metaPatch only seeded policySignatures.
…anged The propagation pass rebuilt parent/child maps by scanning every report on every compute call, even tiny incremental updates touching one or two reports. On a 22k-report account this ran unconditionally on each of the several recomputes fired during OpenApp hydration, adding real overhead on top of the already-fixed policy_ full-rescan. Skips the scan when none of the reports just recomputed have a different needsParentChatErrorPropagation or brickRoadStatus than before, which is the only state the propagation depends on.
…s-policy-signatures # Conflicts: # src/libs/actions/OnyxDerived/configs/reportAttributes.ts
Contributor
Author
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9796fb2dee
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Comment on lines
+328
to
+331
| const hasConciergeReportIDChanged = | ||
| hasKeyTriggeredCompute(ONYXKEYS.CONCIERGE_REPORT_ID, sourceValues) && storedConciergeReportID !== undefined && storedConciergeReportID !== nextConciergeReportID; | ||
| const hasIsTrackIntentUserChanged = | ||
| hasKeyTriggeredCompute(ONYXKEYS.NVP_INTRO_SELECTED, sourceValues) && storedIsTrackIntentUser !== undefined && storedIsTrackIntentUser !== nextIsTrackIntentUser; |
There was a problem hiding this comment.
When upgrading from a persisted reportAttributes value that predates these new baseline fields, an explicit CONCIERGE_REPORT_ID or NVP_INTRO_SELECTED delivery is now treated as unchanged because the stored baseline is undefined; the later meta patch then stores the delivered value without recomputing reports. If that value actually changed while the app was not running (or was first populated after the old value was computed), the only delivery that should update Concierge/track-dependent report names is absorbed, and later identical deliveries are skipped because the baseline now matches. Consider forcing a full recompute when one of these keys triggers and its stored baseline is missing, or only seeding on non-trigger passes.
Useful? React with 👍 / 👎.
…livery pass On the upgrade path (a persisted reportAttributes value that predates these baseline fields), the pass delivering CONCIERGE_REPORT_ID / NVP_INTRO_SELECTED was seeded without recomputing, so a value that changed while the app was closed could be absorbed silently and later identical deliveries skipped. A missing baseline on the delivery pass now counts as a change and forces a recompute (matching the policy-signature and display-name baselines); a missing baseline is seeded only on passes that did not deliver the key. Adds unit tests for both paths.
…s-policy-signatures # Conflicts: # src/libs/actions/OnyxDerived/configs/reportAttributes.ts # tests/unit/reportAttributesTest.ts
- Document that policy signature key exclusion applies at every nesting depth - Lead the signature comment with its invariant instead of a rejected alternative - Skip the full report walk when a coalesced policy trigger delivers an empty delta - Sort derived_trigger keys for stable Sentry group-bys, stamp the from-scratch first compute as 'initial', and hoist that value into CONST.TELEMETRY - Add a triggeredBy() helper for the repeated triggeredKeys sets in tests
Contributor
Author
|
Closing this — #97093 fixed the root cause this PR was targeting. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Explanation of Change
On large accounts (thousands of reports), cold-opening the app and going to Reports/Inbox could freeze the UI for up to ~20s.
The report list metadata (report names, unread/error/GBR badges) is produced by the
reportAttributesderived value. To stay fast it only recomputes when something relevant changed — in particular, when a policy changed. But the "what did the policies look like last time?" baseline was stored in an in-memory variable that resets on every app restart. Because OpenApp/Reconnect always delivers a policy update right after boot, that first update always looked like "everything changed" → the app rescanned every report. On a 22k-report account that's a single ~14s synchronous compute that blocks the main thread.Fix
Persist the change-detection baseline inside the stored derived value (the same way
localealready is), so it survives restarts:policySignaturesmap. A real policy change still recomputes only the reports it affects.conciergeReportID/introSelected→ persisted too; they recompute only when the value actually changes.Plus a smaller, always-on win: the parent-chat error-propagation pass (which scans every report) now runs only when a recomputed report's error status actually changed, instead of on every update.
The
OnyxDerivedComputespan also gains aderived_triggerattribute (the dependency keys that fired the compute), so production telemetry can attribute long computes to their triggers.Small/medium accounts are unaffected — the rescan was already cheap at that report count.
Performance results
Seeded 22k-report account, cold restart, web + Playwright, 50 runs each:
reportAttributesrecompute — medianreportAttributesrecompute — p90Re-validated on 2026-07-27, after merging
main— which meanwhile landed derived-value compute coalescing (queueMicrotaskbatching) and moved trigger detection totriggeredKeys; this PR's checks were adapted to that engine. Same seeded 22k-report account, cold restart + immediate Reports-tab tap, 3 runs per side:reportAttributesrecompute — longestpolicy_triggerCoalescing on
mainbatches computes but does not remove the false full rescan — only the persisted baseline does, so the two changes are complementary. The first restart after upgrading to the new persisted format runs a one-time reconciliation pass that seeds the baselines; measured at ~27 ms, not a rescan.Fixed Issues
$
PROPOSAL:
Tests
Offline tests
Unnecessary — only changes when the local
reportAttributesderived value recomputes; no network/persistence logic beyond the derived value itself.QA Steps
Same as tests.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari