fix: dedupe OpenApp against an in-flight OpenApp - #98266
Conversation
|
@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: a66841704c
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const openAppPromise = API.writeWithNoDuplicatesOpenAppConflictAction( | ||
| params, | ||
| getOnyxDataForOpenOrReconnect(true, undefined, shouldKeepPublicRooms, allReportsWithDraftComments), | ||
| shouldDedupeWithInFlight, |
There was a problem hiding this comment.
When an openApp(true) call arrives while another OpenApp is in flight, the default shouldDedupeWithInFlight remains true, so the new resolver drops the request even though this call's successData contains the public-room preservation merges assembled at lines 372–385. The existing in-flight request cannot apply those merges, so the sign-in flow in SignInModal.tsx can lose locally retained public rooms if another tab or caller starts an OpenApp after its waitForIdle() check. Derive the opt-out when shouldKeepPublicRooms or draft-comment preservation data is supplied rather than relying on callers to pass the fourth positional argument.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch, fixed: openApp() now derives the exclusion from its own arguments, so any call passing shouldKeepPublicRooms or draft-comment data is never dropped, and the explicit shouldDedupeWithInFlight flag stays for the priority-mode refetch whose reason the params cannot show.
|
@MelvinBot review |
Review: dedupe
|
Reviewer Checklist
Screenshots/Videos
|
mountiny
left a comment
mountiny
left a comment
There was a problem hiding this comment.
Ok thanks for adding the tests as well, looks like this follows the Reconnect resolution so looks good to me
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 mountiny 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/mountiny in version: 9.4.53-0 🚀
|
|
🤖 I reviewed the changes and no help site changes are required under Why: This PR is an internal request-queue change — it adds in-flight deduplication so a second Since nothing customer-facing changed, there is no help site PR to open, review, or mark ready. |
6 participants
Explanation of Change
OpenApphad no in-flight deduplication, so twoopenApp()calls that land close together both reach the server and each downloads the whole account.openApp()went throughAPI.writeWithNoDuplicatesConflictAction, whose resolver only searchespersistedRequests.processNextRequestmoves the request currently on the wire out ofpersistedRequestsand intoongoingRequest, so while the firstOpenAppis in flight the matcher finds nothing, the resolver returnspushinstead ofreplace, and the second one is queued and sent.ReconnectAppalready closes this gap by passinggetOngoingRequest()into its own resolver.This adds
resolveOpenAppDuplicationConflictAction, wired throughAPI.writeWithNoDuplicatesOpenAppConflictAction, which readsgetOngoingRequest()the same way. AnOpenAppthat arrives while another one is in flight is dropped (noAction). Everything else is unchanged: a queuedOpenAppis still replaced by the newer one, and anOpenAppthat lands while only aReconnectAppis in flight still runs, because a reconnect does not carryOpenApp's payload.The drop is opt-out through
shouldDedupeWithInFlightonopenApp(), because the params do not describe the whole response: the server picks the report set from the priority-mode NVP as it reads it, so thePriorityModeHandlerrefetch after leaving #focus needs its own round trip. It is the only caller that opts out today.In production over 24 hours the client pushed a second
OpenAppwhile one was already in flight 907 times across 771 accounts, out of 15080OpenApppushes, and every one of them became a full round trip.Fixed Issues
$ #98002
PROPOSAL:
Tests
OpenAppis still pending.OpenAppand find the second[API] Called API write. It should be followed byconflictType: "noAction"andNo action performed, request ignored, with no request sent for it. Onmainthe same spot readsconflictType: "push"withongoingRequest: "OpenApp", and a secondOpenAppgoes out.OpenAppstill goes out and the full chat list loads.Offline tests
OpenApppushed while offline is persisted and not sent, so the in-flight check never applies.OpenAppis flushed and the app loads.QA Steps
OpenApprequest for the new session.OpenApp, the inbox loads, and no loading state is stuck.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