Repository navigation
Keep the leader's unsent writes when another tab syncs the queue - #105420
Open
adhorodyski wants to merge 4 commits into
Open
adhorodyski wants to merge 4 commits into
adhorodyski wants to merge 4 commits into
Conversation
A follower tab wrote its whole copy of the queue back to shared storage whenever it merged a request from another tab. That copy could be older than the leader's. The leader then treated the stale copy as proof the missing requests were done and dropped them from memory, so a queued ApproveMoneyRequest was never sent. Now a tab only adds other tabs' requests to its own memory. Only followers drop requests that are gone from storage. The leader removes a request only after it sends it and the server answers.
Jest loads the mobile ActiveClientManager, where every tab is the leader, so the tests had to mock isClientTheLeader. They now load the web module and make leader and follower tabs through its init() and the ACTIVE_CLIENTS key, the same way browser tabs register.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
melvin-bot
Bot
requested review from
garrettmknight
and removed request for
a team
October 30, 2026 17:26
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
2 of 7 tasks
Contributor
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
Moves the reconcile block into adoptLeaderDeletions, which returns early on the leader, and names the pending-writes check isCrossTabWrite. The leader has no leader to adopt deletions from, so any queue another tab writes can only be a stale copy. Following it would drop unsent writes. Moves the leader cross-tab test into SequentialQueueTest, which already mocks isClientTheLeader, and deletes SequentialQueueCrossTabTest. The guard only reads that boolean, so the real web election added no coverage. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
Author
|
@FitseTLT are you able to continue the review here? |
Contributor
|
Reviewing |
garrettmknight
removed their request for review
November 2, 2026 19:35
Contributor
|
@adhorodyski Regarding the Rca in the pr description isn't the onyx.set writing App/src/libs/actions/PersistedRequests.ts Lines 91 to 92 in 90fb28b or is it some sort of racing condition between the onyx writes that caused it? How did you come up with the RCA? Do you have a concrete evidence may be from the logs? I am not reproducing it on main from the steps listed, do you? I am not saying the change is wrong but I am feeling like we are removing a no-op operation but could we be making a real fix to the issue, that's my doubt. |
This branch has not been deployed
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
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
A follower tab wrote its old copy of the queue back to shared storage, and the leader read that copy as proof its newer, unsent requests were done, so it dropped a queued ApproveMoneyRequest without ever sending it. Now a tab keeps other tabs' requests in its own memory only, and only followers drop requests that are gone from storage, so the leader removes a request only after it sends it and the server answers.
Fixed Issues
$ #105174
PROPOSAL: N/A
Tests
The new unit test places a follower's older copy of the queue, without the approval, in storage. It checks that the leader still sends ReadNewestAction, OpenReport and ApproveMoneyRequest.
Before and after, from a two-tab run of the customer's case. The leader pauses its queue with ReadNewestAction and OpenReport and queues ApproveMoneyRequest. The second tab merges and writes its copy, then the queue resumes. The log lines come from the logging in #105372.
Before this change:
Merging requests from other tabswith ReadNewestAction and OpenReport.[ReadNewestAction, OpenReport]to shared storage.Reconciled deletions from leader tabwithremovedCommands: ["ApproveMoneyRequest"].After this change:
The second tab logs
Merging requests from other tabsbut writes nothing to shared storage.There is no
Reconciled deletions from leader tabline.The queue resumes with 3 requests.
All three are sent, ApproveMoneyRequest last.
Verify that no errors appear in the JS console
Offline tests
QA Steps
Same as Tests and Offline tests.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionNo new error UI. The failure case is the stale cross-tab write, which the new unit test covers.
Not run by hand. The unit tests cover the two-tab case.
Not run. The change only touches the multi-tab write queue.
No UI change. The bug only happens with two web tabs.
Single client, always the leader. Behavior unchanged.
Covered by the unit tests. Manual check pending.
Single client, always the leader. Behavior unchanged.
Covered by the unit tests. Manual check pending.
Covered by the unit tests. Manual two-tab check pending.
The unit tests log no errors. I will check the browser console during the manual run.
No copy change.
No new pattern. It reuses
isClientTheLeaderfromActiveClientManager.Avatar, I verified the components usingAvatarare working as expected)All 25 test suites that touch the persisted request queue pass.
npm run compress-svg)No assets.
No assets.
No change to message content. Only which tab drops queued requests.
Avataris modified, I verified thatAvataris working as expected in all cases)No component change.
No component change.
No page change.
No UI change.
Designlabel and/or tagged@Expensify/designso the design team can review the changes.No UI change.
No UI change.
No UI change.
No UI change.
No UI change.
mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.No review yet.
Screenshots/Videos
Android: Native
No UI change.
Android: mWeb Chrome
No UI change.
iOS: Native
No UI change.
iOS: mWeb Safari
No UI change.
MacOS: Chrome / Safari
No UI change.