Repository navigation
Queue CompleteGuidedSetup when offline during web onboarding - #102344
Conversation
…ct request On web, the Track onboarding flow used makeRequestWithSideEffects, which is never queued or retried. Offline it failed right away, rolled back the optimistic onboarding state, and added errors to the Concierge messages. Co-authored-by: Abdelrahman Khattab <abzokhattab@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. |
|
@MelvinBot the fix itself looks right. I applied the patch locally and reverted just the A few things to add before this is ready for review:
|
|
🤖 Web test steps verification results: I could not run the offline repro on web. The test tooling can't take the browser offline or send Ctrl+D, and the onboarding modal blocks the way to Troubleshoot's "Force offline". So the offline path this PR changes was not tested, and the ❌ rows mean "not run", not "the fix failed". As a regression check, I finished the "Track expenses for my business" flow online:
|
…e-effect path Assert the offline request is persisted in the queue and that no Concierge onboarding message gets errors. Offline fetch now fails like a real offline request, so the error assertion fails without the fix. Add a test that online CompleteGuidedSetup still uses makeRequestWithSideEffects. Co-authored-by: Abdelrahman Khattab <abzokhattab@users.noreply.github.com>
|
I've addressed all four points and pushed them in d04ad44.
All 314 tests in |
|
@MelvinBot @codex review the PR and check for regressions, breaking changes or bugs |
|
@MelvinBot complete the checklist and test steps, and mark screenshots as done since they’ll be added in the reviewer checklist. |
PR ReviewNo regressions or breaking changes found. The change is small and scoped to web, and CI passes except Gap: the connection can drop during Suggested fix: check again after the await, and fall through to if (shouldWaitForRHPVariantInitialization && !isOfflineNetwork()) {
await waitForWrites(SIDE_EFFECT_REQUEST_COMMANDS.COMPLETE_GUIDED_SETUP);
if (!isOfflineNetwork()) {
resetOnboardingStackToRoot();
return API.makeRequestWithSideEffects(...);
}
}What I checked
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d04ad44efc
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (shouldWaitForRHPVariantInitialization) { | ||
| // Side-effect requests are not queued or retried, so when offline we fall through to API.write | ||
| // to keep the optimistic data and replay the request once the user is back online | ||
| if (shouldWaitForRHPVariantInitialization && !isOfflineNetwork()) { |
There was a problem hiding this comment.
Recheck offline status after waiting for writes
When web onboarding starts online but connectivity drops while waitForWrites() is waiting for the preceding CreateWorkspace, this condition has already selected the side-effect path. The sequential queue releases its idle wait when it becomes offline, so execution then reaches makeRequestWithSideEffects and sends an unqueued request; its failure data rolls back onboarding just like the offline regression this change is meant to prevent. Recheck connectivity after the await and fall back to API.write if it changed.
Useful? React with 👍 / 👎.
|
@MelvinBot thanks, the four earlier points look good — and good catch that the original fetch mock succeeded while offline, so the new error assertion would have passed without the fix. Two more things, then the checklist.
|
|
🤖 I updated the PR description:
Before you mark it ready, please confirm the items I checked but couldn't test myself:
What I tested |
…Setup as a side-effect request waitForWrites resolves when the app goes offline even with requests still queued, so a connection drop while CreateWorkspace is in flight still reached makeRequestWithSideEffects. Check again after the wait and fall through to API.write. Add a test for that window and fix the spy comment in the online test. Co-authored-by: Abdelrahman Khattab <abzokhattab@users.noreply.github.com>
|
I pushed all three points in b2eabf0.
Not run, so please cover these in the reviewer checklist:
What did run:
|
…rWrites Co-authored-by: Abdelrahman Khattab <abzokhattab@users.noreply.github.com>
|
I combined the two offline checks into one in fbbb0dd. The only The tests and the spy comment fix from b2eabf0 are unchanged. I couldn't run Jest, lint or Prettier in this environment, so this relies on your local run of the 11 |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
Skipping the wait when already offline avoids blocking on an in-flight request that has no timeout. The post-wait re-check still covers going offline during the wait. Comments now explain each check's purpose. Co-authored-by: Abdelrahman Khattab <abzokhattab@users.noreply.github.com>
|
I restored the outer
I made no test changes. ESLint found no issues in the changed lines. Jest and Prettier couldn't run here, so this relies on your local run of the 11 |
|
MelvinBot has made 5 consecutive commits trying to fix CI failures. Automatic CI fix dispatches are now paused for this PR. Next steps: A human should review the recent commits and resolve the conflicting CI errors manually. |
|
@MelvinBot logic in 4dfe346 is right, but my wording was too long. Please keep both checks and collapse the two comment blocks into one two-liner above the outer // Side-effect requests are never queued or retried, so offline we fall through to API.write.
// Checked twice because waitForWrites also resolves when the connection drops mid-wait.
if (shouldWaitForRHPVariantInitialization && !isOfflineNetwork()) {
// Wait for the workspace to be created before completing the guided setup
await waitForWrites(SIDE_EFFECT_REQUEST_COMMANDS.COMPLETE_GUIDED_SETUP);
if (!isOfflineNetwork()) {Drop the "Skip the wait entirely…" and "Re-check, because…" blocks. No other changes, and no test changes. |
Co-authored-by: Abdelrahman Khattab <abzokhattab@users.noreply.github.com>
|
I collapsed the two comment blocks into your two-liner above the outer I couldn't run Prettier or cspell locally in this run, so CI will be the first check of those. |
|
🤖 I updated the PR description, but I couldn't mark the PR as ready. The "ready for review" action was blocked in this run, so the PR is still a draft. Changes to the description:
|
|
@MelvinBot you have access to mark it as ready .. retry |
|
🤖 I retried, but I still couldn't mark this PR as ready. This run's tool permissions block the "ready for review" command, so the PR is still a draft.
|
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppUntitled.movAndroid: mWeb ChromeiOS: HybridAppScreen.Recording.2026-09-26.at.16.54.27.moviOS: mWeb SafariScreen.Recording.2026-09-26.at.16.56.23.movMacOS: Chrome / SafariScreen.Recording.2026-09-26.at.13.48.44.mov |
|
@iwiznia melvin keeps refusing to mark the PR as ready .. can you please help me marking it as ready as i dont have access to i posted the checklist so that is ready for review |
|
@abzokhattab 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] |
|
🚧 iwiznia 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! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/iwiznia in version: 9.4.96-0 🚀
|
|
No help site update needed. This PR fixes a bug so onboarding completed offline on web now stays completed and shows no Concierge error, which is what users already expect, and no help article sentence becomes incorrect. |
|
🚀 Deployed to staging by https://github.com/iwiznia in version: 9.4.99-0 🚀
|
|
No help site update is needed. This PR fixes a bug so that finishing onboarding while offline queues the request and completes as users expect, and no existing article describes the old behavior. |
|
🚀 Deployed to production by https://github.com/Julesssss in version: 9.4.99-2 🚀
Bundle Size Analysis (Sentry): |















Explanation of Change
On web, onboarding calls
completeOnboardingwithshouldWaitForRHPVariantInitialization, which sendsCompleteGuidedSetupthroughAPI.makeRequestWithSideEffects. That request is never queued or retried. Offline, it fails right away and itsfailureDataadds "Unexpected error posting the comment" to the Concierge onboarding messages and resetshasCompletedGuidedSetupFlowtofalse, so the user keeps getting sent back to onboarding.This PR uses the side-effect path only when online (checked before and after waiting for queued writes). Offline,
completeOnboardingfalls back toAPI.write, so the request is queued and replays on reconnect. Offline users don't get a server-assigned RHP variant; Track flows fall back to the existingtrackExpensesWithConciergevariant.Added unit tests in
tests/actions/ReportTest.tsfor offline, going offline during the wait, and online.Fixed Issues
$ #102337
PROPOSAL: #102337 (comment)
Tests
"Track expenses for my business" offline
Offline tests
Same as Tests. Completing onboarding offline queues
CompleteGuidedSetup, onboarding stays completed, no "Unexpected error posting the comment" appears in Concierge, and the request replays when you reconnect (steps 9-10).QA Steps
Same as Tests. The bug is web-only, so web (Chrome and Safari) is the priority. On native, confirm the same flows still complete offline with no errors.
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