Repository navigation
Keep ReconnectApp incremental for signed-in users so other devices don't lose chat messages - #102247
Conversation
…P Pusher event Co-authored-by: Nyoman Jyotisa <nyomanjyotisa@users.noreply.github.com>
|
I created this PR from an upstream branch since I don't have push access to your fork. To take ownership of this branch and be able to push updates, run: Then you can close this PR and open a new one from your fork. |
|
🤖 Web test steps verification results: I sent 3 messages in the Concierge chat. All 3 were still visible after 30 seconds, with no visible errors. I could not sign in to the same account from a second device, so the scenario this PR fixes was not tested. The PR's Tests section is still a TODO, so I used the steps from the linked issue.
|
|
@MelvinBot Please merge the latest |
|
Platform recordings for head
|
…crementalForSignedInUsers Co-authored-by: Nyoman Jyotisa <nyomanjyotisa@users.noreply.github.com>
|
@MelvinBot Could you remove “(anonymous users)” from this comment? It now applies to signed-in users too. |
…ient comment Co-authored-by: Nyoman Jyotisa <nyomanjyotisa@users.noreply.github.com>
|
I removed "(anonymous users)" from the comment in src/libs/actions/User.ts:89 and pushed commit 5a2106a. This commit changes only that comment. |
Updated testing, checklist, and recordingsTests
Offline tests
QA StepsFollow the Tests and Offline tests above.
PR Author Checklist
Screenshots/VideosAndroid: NativePR-102247-android-native-online-redacted.mp4Android: mWeb ChromePR-102247-android-mweb-chrome-online-redacted.mp4iOS: NativePR-102247-ios-native-online-redacted.mp4iOS: mWeb SafariPR-102247-ios-mweb-safari-online-redacted.mp4MacOS: Chrome / SafariPR-102247-desktop-chrome-three-session-online-redacted.mp4@MelvinBot please replace the sections from |
|
I updated the PR description. Everything from |
|
@MelvinBot please mark this PR ready for review. |
|
I couldn't mark this PR ready for review. The command that does it isn't permitted in my current environment, so the PR is still a draft.
|
|
@MelvinBot I can’t mark this PR ready for review because GitHub denied permission. Could you retry? |
|
I retried, but I still can't mark this PR ready for review. The action is blocked in my environment, so the PR is still a draft. I was wrong earlier when I said you could do it as an assignee. GitHub only lets the PR author or someone with write access change draft status. You have read access to this repo, so GitHub denied you. To unblock this, ask an Expensify employee with write access to click Ready for review, for example the assigned internal engineer. |
|
@Julesssss Could you help mark this PR ready for review? Melvin’s attempt was blocked, and I don’t have permission. |
|
@nyomanjyotisa 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] |
|
@MelvinBot please merge the latest |
…crementalForSignedInUsers Co-authored-by: Nyoman Jyotisa <nyomanjyotisa@users.noreply.github.com> # Conflicts: # tests/unit/MiddlewareTest.ts
|
I merged the latest One change from your request: I kept 2 of the 3 imports in
The formatter fixed a missing space before Checks: The middleware and user tests pass (67 of 67). Lint passes on both files, and |
|
@Julesssss could you please re-review? |
|
Sorry @nyomanjyotisa could you resolve blockers by merging main? |
|
@MelvinBot please merge |
…crementalForSignedInUsers Co-authored-by: Nyoman Jyotisa <nyomanjyotisa@users.noreply.github.com> # Conflicts: # tests/unit/MiddlewareTest.ts
|
Merged The only conflict was the import line in Checks:
|
|
@Julesssss main is merged and conflicts are resolved. Could you please re-review? |
Julesssss
left a comment
There was a problem hiding this comment.
@nyomanjyotisa unfortunately there is a failing check now. Could you merge main again and hopefully this is the last time.
|
@MelvinBot please merge |
…crementalForSignedInUsers Co-authored-by: Nyoman Jyotisa <nyomanjyotisa@users.noreply.github.com>
|
I merged
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 Julesssss 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/Julesssss in version: 9.5.10-0 🚀
|
|
No help site update is needed. This PR fixes a sync bug so devices that are already signed in keep their chat messages when another device signs in. That matches what users already expect, and no help article describes the old behavior. |
|
🚀 Deployed to staging by https://github.com/Julesssss in version: 9.5.12-0 🚀
|
|
No help site update is needed. This PR fixes a sync bug so signed-in devices keep their chat messages when another device signs in, which is what users already expect, and no help article describes the old behavior. |
|
🚀 Deployed to production by https://github.com/cristipaval in version: 9.5.12-1 🚀
Bundle Size Analysis (Sentry): |






Explanation of Change
The server sends the
reconnectAppPusher event on the account-wide channel, so every signed-in device of the account receives it, not only the device whoseupdateIDFromis missing. For signed-in users, the handler always calledreconnectApp()with no argument. That triggers a fullReconnectApp, which skips the "older than our state" guard and replaces the report-action collections. As a result, when a third device signed in, the two devices that were already chatting lost messages they held.This PR makes the handler pass
lastUpdateIDAppliedToClientfor all users, as it already did for anonymous users (#97847). Recovery still works:lastUpdateIDAppliedToClientyet,reconnectAppreceivesundefinedand still does a full reconnect.Unit tests in
tests/actions/UserTest.tsnow cover the signed-in incremental case and the no-watermark fallback.Open backend questions from the proposal (not addressed here, need an internal engineer): whether
reconnectAppshould be scoped to the affected device instead of broadcast account-wide, and confirmation that a fullReconnectAppusessetcollectionforreportActions_.AI tests run locally:
npm run lint-changed✅,npm run typecheck✅,npm run spell-changed✅,npm test -- tests/actions/UserTest.ts tests/actions/AppTest.ts✅ (57 passed). The full Jest suite, fullnpm run lint, formatter check, and Storybook smoke test were not run locally; CI covers them.Fixed Issues
$ #100791
PROPOSAL: #100791 (comment)
Tests
Offline tests
QA Steps
Follow the Tests and Offline tests 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
PR-102247-android-native-online-redacted.mp4
Android: mWeb Chrome
PR-102247-android-mweb-chrome-online-redacted.mp4
iOS: Native
PR-102247-ios-native-online-redacted.mp4
iOS: mWeb Safari
PR-102247-ios-mweb-safari-online-redacted.mp4
MacOS: Chrome / Safari
PR-102247-desktop-chrome-three-session-online-redacted.mp4