Fix duplicate OpenApp on deep link by recording full reconnect time immediately - #100547
Fix duplicate OpenApp on deep link by recording full reconnect time immediately#100547neerajbachani wants to merge 2 commits into
Conversation
|
@QichenZhu 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] |
|
Hey @QichenZhu , Could you please run an adhoc build on this branch for QA? I tried to manually test the Automated coverage: I added unit tests on this branch that model the boot race - on Thanks! |
|
@neerajbachani, you can open You need to do the same even with ad hoc builds, so I'd suggest testing locally and finishing the checklist first. |
|
Thanks @QichenZhu that works! I tested the Here’s a video of one of the runs: Screen.Recording.2026-09-09.at.10.17.19.AM.online-video-cutter.com.mp4I’m working through the rest of the PR checklist now as well. Thanks again! |
| * | ||
| * The entry goes right before the delivered cutoff entry, so the reconnect subscription never sees | ||
| * a new cutoff next to an old reconnect time. That assumes Onyx broadcasts a batch's merges in | ||
| * array order: true today because both keys are cache-resident (see subscribeToFullReconnect.ts), | ||
| * and pinned by the SubscribeToFullReconnect e2e test. If it ever broke, the worst case is one | ||
| * transient extra reconnect, never a loop. | ||
| * The time is written directly to Onyx (not spliced into the deferred response batch) so |
There was a problem hiding this comment.
(not spliced into the deferred response batch)
Is there a reason to mention an approach that will no longer exist after this PR?
| // Written directly to Onyx (not via an action) so subscribeToFullReconnect observes LAST_FULL_RECONNECT_TIME before any cutoff from the same response can land. | ||
| // eslint-disable-next-line rulesdir/prefer-actions-set-data |
There was a problem hiding this comment.
The comment doesn't explain why you suppress the rule instead of following it. I don't see a reason either.
Explanation of Change
When opening a report via an OldDot
/transitiondeep link,subscribeToFullReconnectcould fire an unnecessaryReconnectAppduring boot becauseLAST_FULL_RECONNECT_TIMEwas spliced into the deferredOpenAppresponse batch. The cutoff became visible in Onyx before the recorded reconnect time, soreconnectApp()fell back to a secondOpenApp, leaving the report stuck on a semi-transparent skeleton.This PR removes the broken splice in
recordFullReconnectTimeFromResponseand writesLAST_FULL_RECONNECT_TIMEdirectly to Onyx inside theRecordFullReconnectTimemiddleware, awaited beforeSaveResponseInOnyxcan stage the cutoff. That prevents the duplicate full download without adding ahasLoadedAppguard around the reconnect subscription.Fixed Issues
$ #97159
PROPOSAL: #97159 (comment)
Tests
npm test -- tests/unit/SubscribeToFullReconnectTest.ts tests/unit/FullReconnectUtilsTest.ts tests/unit/RecordFullReconnectTimeMiddlewareTest.tsOpenAppfires when the cutoff arrives while the first response is still queued.https://www.expensify.com/report?param={"pageReportID":"<reportID>"}(or use the transition link from the issue)./transitionand lands on/search/r/<reportID>.https://staging.new.expensify.com/search/r/<reportID>directly (no/transitionhop) and confirm the report still loads normally.Offline tests
N/A — this change affects the OpenApp/reconnect ordering during initial app load over the network. No offline-specific behavior was changed.
QA Steps
https://www.expensify.com/report?param={"pageReportID":"<reportID>"}./transition?...&exitTo=search/r/<reportID>and opens the report in the RHP.https://staging.new.expensify.com/search/r/<reportID>and confirm the report still opens normally.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
Screen.Recording.2026-09-09.at.10.17.19.AM.online-video-cutter.com.mp4