Coalesce OnyxDerived recomputations via macrotask scheduling - #95287
roryabraham merged 21 commits into
Conversation
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0ebbee3bd5
ℹ️ 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".
| } | ||
|
|
||
| pendingDependencyIndexes.clear(); | ||
| runCompute(sourceValues); |
There was a problem hiding this comment.
Preserve secondary triggers in coalesced recomputes
When this flush passes all pending dependency changes to a config at once, several existing incremental configs still assume only one source key is present. For example, reportTransactionsAndViolations.compute uses if (transactionsUpdates) ... else if (transactionViolationsUpdates) ..., so a single Onyx.update that changes transaction A and violations for transaction B now only processes A; before this change the callbacks ran separately and B's violation map was updated. The same pattern exists in sortedReportActions when REPORT_ACTIONS is batched with REPORT/NETWORK, so coalescing needs to either force a full recompute for mixed triggers or update those configs to handle all triggers together.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch, fixed in 0842b78.
However I'm not implementing the general suggested fix (full recompute on any mixed trigger) because that would defeat the goal of this PR, so I'm instead fixing reportTransactionsAndViolations and sortedReportActions to account for this scenario. The other derived files were checked and they are already safe.
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
|
@shubham1206agra Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 roryabraham has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/roryabraham in version: 9.4.32-0 🚀
|
|
🤖 I reviewed the changes in this PR to determine whether any Expensify help site articles under No help site changes are required. This PR is a purely internal performance optimization — it coalesces per-dependency
There is no change to user-facing behavior: no new features, UI, tabs, settings, buttons, or workflows. Derived values remain eventually consistent within one macrotask of the originating write, and consumers re-render as before through Because no changes are needed, I did not create a draft docs PR. @fabioh8010, please review the linked help site PR and confirm it reflects the current behavior. Then mark the linked help site PR |
|
Deploy Blocker #95696 was identified to be related to this PR. |
|
Deploy Blocker #95698 was identified to be related to this PR. |
|
Deploy Blocker #95700 was identified to be related to this PR. |
Reverts the coalescing engine that caused deploy blockers, scoped to just the OnyxDerived changes on top of current main. Restores the synchronous, per- dependency recompute engine (Onyx sourceValue based) and keeps waitForCollectionCallback, matching main's current Onyx 3.0.86 (the onyx-store work that removed it, #93436, was already reverted separately). Removes getCollectionDelta, scheduleMacrotask, and the triggeredKeys/flush changes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Deploy Blocker #95721 was identified to be related to this PR. |
|
Deploy Blocker #95753 was identified to be related to this PR. |
|
🚀 Deployed to production by https://github.com/grgia in version: 9.4.32-3 🚀
Bundle Size Analysis (Sentry): |
This reverts commit 4c28035.
Explanation of Change
Slack proposal: https://expensify.slack.com/archives/C08CZDJFJ77/p1783094934084619?thread_ts=1782801937.464879&cid=C08CZDJFJ77
OnyxDerived values (e.g.
reportAttributes) open one Onyx connection per dependency, and each connection callback recomputes the derived value synchronously. Because Onyx delivers a single logical write (a server response touching reports + transactions + violations + report actions, etc.) as several separate broadcasts, one logical update recomputes each derived value many times back-to-back, blocking the JS thread during startup and report opens.reportAttributesalone has 13 dependencies and is one of the heaviest computes in the app.This PR coalesces the per-dependency recomputes for each derived value into a single compute per logical update.
How it works
Coalescing.
recomputeDerivedValueno longer computes inline — it records the triggering dependency in a pending set and, if no flush is already scheduled, schedules one on the next macrotask. Every dependency callback from the same logical update lands in that one pending flush, so N recomputes collapse to 1.Flush-time delta reconstruction. Instead of relying on Onyx's per-broadcast
sourceValue(which doesn't survive coalescing — you'd be merging N partial payloads — and is being phased out), the flush reconstructs the changed-member delta itself: for each collection dependency that fired, it diffs the current snapshot against a baseline captured at the previous flush, via a new sharedgetCollectionDeltahelper. Onyx's structural sharing keeps unchanged members reference-equal, so this is a cheap reference scan. The first flush has no baselines, so it computes from scratch and captures baselines for subsequent diffs.scheduleMacrotask(MessageChannel). The flush is scheduled with a smallscheduleMacrotaskhelper that usesMessageChannelon web — the same technique React's scheduler uses — becausesetTimeoutis throttled in background tabs (rules varies through different browsers) and clamped to 4ms after nested calls. It falls back tosetTimeouton React Native (noMessageChannel; the JS thread is fully suspended in the background there anyway) and in tests.Incremental configs hardened to honor every coalesced trigger. Configs that update incrementally previously assumed a single triggering dependency per compute. Coalescing can now deliver several changed dependencies in one compute, so any config that branched on "which one changed" was updated to process all of them:
reportAttributes— its personal-details fast-path short-circuits only when personal details are the sole trigger.reportTransactionsAndViolations— processes the union of transactions that changed directly and transactions whose violations changed (was anif/elsethat dropped the second trigger).sortedReportActions— takes its report-actions incremental path only when report actions are the sole trigger; a batchedREPORTchange falls back to a full recompute.With recomputes coalesced, one logical update produces exactly one compute of each derived value instead of one-per-dependency. Derived values become eventually consistent within one macrotask of the originating write; consumers re-render normally through
useOnyxwhen the single updated value lands.Fixed Issues
$ #95301
PROPOSAL:
Tests
MessageChannel testing (web-only)
I'm using Firefox as it's easier to demonstrate the differences in the latencies when comparing with
setTimeout.Before testing, apply these changes to
src/libs/actions/OnyxDerived/index.ts:await Onyx.merge('session', {email: 'test@email.com'});.[perf][OnyxDerived]logs. Assert the flush latency is lesser than 1000 ms.If you change
scheduleMacrotask.tsto always use thesetTimeoutfallback, the flush latency will be greater than 1000 ms.Offline tests
N/A
QA Steps
Same as Tests, except MessageChannel testing (web-only).
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)Avatar, 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
Screen.Recording.2026-07-06.at.10.38.55.mov
iOS: mWeb Safari
MacOS: Chrome / Safari
Screen.Recording.2026-07-06.at.10.32.24.mov
MessageChannel testing (web-only)
Screen.Recording.2026-07-06.at.10.21.14.mov