Repository navigation
[Payment due @ahmedGaber93] Update MANAGE_TEAM onboarding: skip stale optimistic writes and navigate from API response - #91685
Conversation
…ant before Onyx update Reads the RHP variant directly from the API response instead of relying on Onyx, avoiding the race condition where SaveResponseInOnyx queues onyxData updates and the variant NVP is undefined when navigation fires.
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
Remove shouldUseFollowupsInsteadOfTasks — it was suppressing optimistic task data for ALL MANAGE_TEAM users, leaving a blank UI until Pusher delivered the real data. Tasks should appear immediately for the control and other non-inbAdminsWel variants. Keep optimisticConciergeReportActionID for MANAGE_TEAM so Auth has an idempotent ID for the bespoke welcome when the inbAdminsWel variant is assigned.
…10-inb-admins-wel
The server assigns the RHP variant at CompleteGuidedSetup time. For inbAdminsWel, Auth replaces guidedSetupData with "[]" so no task or welcome reportAction ever lands. The App was optimistically merging the task reports + intro welcome into Onyx anyway, which lingered forever for inbAdminsWel users (Pusher never delivers them because the server never created them). Wire payload (tasksForParameters, welcomeSignOff message) stays so the server can post them for non-inbAdminsWel variants — Pusher delivers ~1-2s after the response. inbAdminsWel users never see stale optimistic state.
… message-in-wire test guidedSetupData for MANAGE_TEAM now includes the text message and sign-off so the server can post them (inbAdminsWel variant fix). Five existing toHaveLength(1) checks were counting total entries; they now filter by type='task' so they stay precise. New test validates the core invariant: message appears in guidedSetupData (wire) but not in optimisticData (no flash before server responds). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Each comment led with a restatement of what the adjacent code already showed (pattern 1). Trimmed to WHY-only, one line each. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Optimistic writes to the admins room are skipped for MANAGE_TEAM users (shouldDeferOptimisticTasks=true) so the server can post authoritatively for the inbAdminsWel variant. The previous assertion (not.toBeUndefined) reflected the old behavior before deferral was introduced.
marcochavezf
changed the title
marcochavezf
changed the title
|
@ahmedGaber93 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] |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // this ID via addComment (idempotent on reportActionID), so we never add an optimistic action | ||
| // here — the real message arrives from the server without a flash. | ||
| const optimisticConciergeReportActionID: string | undefined = shouldUseFollowupsInsteadOfTasks ? rand64() : undefined; | ||
| // Auth needs this ID to post the inbAdminsWel welcome idempotently; the real message comes from the server. |
There was a problem hiding this comment.
❌ CONSISTENCY-3 (docs)
The expression engagementChoice === CONST.ONBOARDING_CHOICES.MANAGE_TEAM is duplicated here when the variable shouldDeferOptimisticTasks (defined at line 11651) already captures this exact condition.
Reuse the existing variable:
const optimisticConciergeReportActionID: string | undefined = shouldDeferOptimisticTasks ? rand64() : undefined;Reviewed at: 4c33cff | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
![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: 4c33cff5af
ℹ️ 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".
| value: { | ||
| hasOutstandingChildTask, | ||
| ...(shouldUseFollowupsInsteadOfTasks || (skipSignOff && !message) ? {} : {lastVisibleActionCreated}), | ||
| ...(skipSignOff && !message ? {} : {lastVisibleActionCreated}), |
There was a problem hiding this comment.
For MANAGE_TEAM, shouldDeferOptimisticTasks now prevents adding the optimistic text/sign-off report actions, but this merge still writes lastVisibleActionCreated from welcomeSignOffCommentAction. In the inbAdminsWel path where the server does not immediately return matching report actions, the admins/Concierge report points at a non-existent last visible action, leaving the report preview/order stale until a later server update or refresh. Keep this field unchanged when optimistic actions are deferred, as the old shouldUseFollowupsInsteadOfTasks guard did.
Useful? React with 👍 / 👎.
|
@marcochavezf Do you mind briefly summarizing what this is doing or if it needs product review? I don't really understand based on the description at the top of the PR. Thanks! |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariScreen.Recording.2026-06-19.at.12.23.38.AM.mov
|
|
Hi @joekaufmanexpensify, we're moving the welcome message to a new variant and restoring the onboarding tasks to the control variant (more details here), sorry if the PR description sounds a bit cryptic I will update it @ahmedGaber93 let's pause the testing for now please, I'm going to include more changes for other two variants that we're going to add, and I think it would be better to include them here |
marcochavezf
changed the title
…extractRHPVariantFromResponse validation - CONST.ONBOARDING_RHP_VARIANT.INBOX_ADMINS_BESPOKE = 'inboxAdminsBespoke' - OnboardingRHPVariant union updated - extractRHPVariantFromResponse: validate against known values, return undefined for unknown strings - Update comments in ReportUtils.ts and tests
…10-inb-admins-wel # Conflicts: # tests/unit/ReportUtilsTest.ts
…ctRHPVariantFromResponse Replace `raw as OnboardingRHPVariant` cast (and the intermediate string/string[] casts) with `typeof raw === 'string'` narrowing + `.find()` which returns the typed OnboardingRHPVariant | undefined directly. Brings the no-unsafe-type-assertion seatbelt back to baseline.
@ahmedGaber93 Yes it is, and independent of current changes. Could you continue with your review. |
|
The changes looks good, just a small note and update to QA steps Micro company size now open the RHP with concierge chat If this is expected, let’s update this test step to prevent QA from reporting it as a regression. - 2. Repeat with **Manage my team's expenses**. Verify the app navigates to the admins room without a placeholder flash and the standard welcome appears.
+ 2. Repeat with **Manage my team's expenses** and medium company size (11 - 50). Verify the app navigates to the admins room without a placeholder flash and the standard welcome appears.let’s update this test step since - 1. Create a fresh account
+ 1. Create a fresh account without `+` signWe need also to confirm that the QA steps is a web only since |
ahmedGaber93
left a comment
ahmedGaber93
left a comment
There was a problem hiding this comment.
LGTM! Just small note and QA step update #91685 (comment)
melvin-bot
Bot
changed the title
|
🎯 @ahmedGaber93, thanks for reviewing and testing this PR! 🎉 A payment issue will be created for your review once this PR is deployed to production. If payment is not needed (e.g., regression PR review fix etc), react with 👎 to this comment to prevent the payment issue from being created. |
Noted. QA steps now updated |
|
🚧 Valforte has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/Valforte in version: 9.4.19-1 🚀
Bundle Size Analysis (Sentry): |
|
🤖 Help site review: no changes required. I reviewed the changes in this PR and they do not require any updates to Expensify's help site files under Why: This PR is internal plumbing for the server-driven
None of this changes a documented feature, setting, label, or user-visible workflow — it prevents a placeholder flash and fixes navigation timing for an experiment whose variant assignment lives server-side. Since no public behavior is added, removed, or altered, there is nothing to document on the help site, so I did not create a draft docs PR. If a variant of this experiment later ships as the permanent onboarding behavior and changes what new admins see, that would be the point to revisit the getting-started articles. |
|
@marcochavezf This PR is failing in step 2 because of a regression issue #94575 
The issue is reproducible in: Android Bug7190258_1782336884360.WhatsApp_Video_2026-06-24_at_5.30.26_PM.mp4 |
|
This issue already on main before this PR, and affected mWeb and native platforms, so it looks not a regression from this PR. More details about the root cause on Melvin's proposal |
|
@mitarachim That behaviour is fine for now on Android and native platforms. Let's not block on this. |
|
🚀 Deployed to production by https://github.com/blimpich in version: 9.4.19-5 🚀
|
|
🤖 Payment issue created: #94633 |
Explanation of Change
Prepares the frontend for the
inbAdminsWelonboarding experiment, where the server decides at response time whether a MANAGE_TEAM user gets a bespoke welcome in their #admins room or the standard guided-setup tasks. The App reads the assigned variant directly from the API response and navigates accordingly — so whichever path the server picks, the user lands in the right place.Two issues are fixed to make this work correctly:
Variant assignment is handled in the Auth companion: Auth#21785.
Fixed Issues
$ https://github.com/Expensify/Expensify/issues/637600
PROPOSAL:
Tests
ReportTestandReportUtilsTestupdated to assert no optimistic actions are written to the admins room for MANAGE_TEAM, and thatguidedSetupDataincludes the correct entries for both paths.Regression steps (can be run on staging without companion PRs — variant won't fire, but existing behavior should be unaffected):
+sign and complete onboarding selecting Submit expenses (or Track my own expenses). Verify the welcome message and tasks appear in the Concierge chat.Offline tests
Not applicable — post-onboarding navigation requires a network connection.
QA Steps
Same as test steps.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, 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.ScrollViewcomponent to make it scrollable when more elements are added to the page.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
Tests using the companion PRs (onboarding tasks are moved to control and the welcome message appears in the new variant):
Without the companion PRs, the expectect behavior is the one described in the test section