Revert "perf: defer the split-save API.write behind the destination layout" - #99814
Conversation
|
@ikevin127 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] |
Reviewer Checklist
|
|
🚧 francoisl has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
Revert "perf: defer the split-save API.write behind the destination layout" (cherry picked from commit b8aba2b) (cherry-picked to staging by francoisl)
|
🔁 Filed a Staging retest request for deploy blockers #99805 after this PR was cherry-picked to staging. |
|
🚀 Cherry-picked to staging by https://github.com/francoisl in version: 9.4.64-2 🚀
|
|
No help site changes are required. I did not create a draft PR. This PR is a straight revert of #98012, which changed only when the split-save API write fires — it deferred the write behind a navigation transition barrier. Reverting it restores the write to fire inline, which is exactly what production Nothing user-facing changed:
The diff touches one action file plus test scaffolding only:
Docs I checked
I also scanned the other Write-ordering and transition-barrier behavior are internal implementation details. The help site does not document them, and it should not. If the re-land of #98012 you mentioned ends up changing what a user sees or does — for example a different loading state or a changed Save interaction — ping me on that PR and I'll draft the docs update then. You asked me to close with a request to review the linked help site PR. I've left that out because there is no help site PR to link — creating an empty one and asking you to review it would just be noise. If you disagree and want a docs PR opened anyway, say so and I'll create one. |
|
🚀 Deployed to production by https://github.com/francoisl in version: 9.4.64-3 🚀
Bundle Size Analysis (Sentry): |
|
🚀 Cherry-picked to staging by https://github.com/francoisl in version: 9.4.65-0 🚀
|
|
Re-checked — still no help site changes required. No draft PR created. Nothing about this PR changed since my earlier review: same head commit ( The conclusion holds because this PR only changes when the split-save API write fires, not what a user sees or does. I re-read
What the diff actually changes
Write ordering, transition barriers, and optimistic-update timing are internal implementation details. The help site does not document them, and per I also re-scanned the other I've left off the closing "please review the linked help site PR" line again, since there's still no PR to link. If you'd like a docs PR opened regardless — or if you re-ran this because you expected a different answer — tell me what you think needs documenting and I'll draft it. |
|
🚀 Deployed to production by https://github.com/francoisl in version: 9.4.65-3 🚀
Bundle Size Analysis (Sentry): |
Reverts #98012
Explanation of Change
Deploy blocker on staging
9.4.64-0. #98012 deferred the split-save write behind a navigation transition barrier whenisFromSplitExpensesFlowis set - which is exactly the flag the Search exit passes.Reads block on
waitForWrites()->SequentialQueue.waitForIdle(), and that gate is only claimed when a write is actually pushed. With the write deferred, nothing is on the queue at Save time, so the expanded group's focus refetch inGroupChildrenContentreaches the server before the split exists and repopulates the group's snapshot with pre-split data. Optimistic data doesn't cover it: the forward-split branch writes only toSNAPSHOT_${currentSearchHash}, while the expanded group readsSNAPSHOT_${groupItem.transactionsQueryJSON.hash}.Straight revert of the merge commit. All six files are byte-identical to their pre-#98012 state, and none of them - nor
writeWhenReady.ts/API/index.ts- were touched by the 195 commits merged since.I'll re-land with the ordering fixed rather than reapplying as-is.
Note on the checklist below: this is a mechanical revert, so the restored code is what production is running today (
9.4.63). Verification here is CI plus the QA repro below, not fresh per-platform manual runs of unchanged code.Fixed Issues
$ #99805
PROPOSAL: N/A - deploy blocker revert
Tests
group-by:fromOffline steps
QA steps
Same as the Tests section above.
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
N/A - revert of unchanged code paths.
Android: mWeb Chrome
N/A - revert of unchanged code paths.
iOS: Native
N/A - revert of unchanged code paths.
iOS: mWeb Safari
N/A - revert of unchanged code paths.
MacOS: Chrome / Safari
N/A - revert of unchanged code paths.
MacOS: Desktop
N/A - revert of unchanged code paths.