Remove Onyx.connect() for the key: ONYXKEYS.NVP_ONBOARDING in src/libs/actions/Welcome/OnboardingFlow.ts#69071
Conversation
|
@shubham1206agra 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] |
|
|
|
Looking at the failing tests |
|
@shubham1206agra bump for review |
|
bump |
|
bump @shubham1206agra |
|
@shubham1206agra do i need to update anything else to get this one moving ? |
|
bump @shubham1206agra |
|
bump |
| const [currentOnboardingCompanySize] = useOnyx(ONYXKEYS.ONBOARDING_COMPANY_SIZE, {canBeMissing: true}); | ||
| const [onboardingInitialPath] = useOnyx(ONYXKEYS.ONBOARDING_LAST_VISITED_PATH, {canBeMissing: true}); | ||
|
|
||
| const [onboardingValues] = useOnyx(ONYXKEYS.NVP_ONBOARDING, {canBeMissing: true}); |
There was a problem hiding this comment.
@allgandalf The value of this onyx variable might not be present when we load this component as this is the first time the value of onboarding is being fetched and it may take some cycle to load the value.
Can we add this to OnyxListItemProvider to make sure this doesn't happen here?
There was a problem hiding this comment.
Also, why is this useOnyx is duplicated? There is one above too
const [isOnboardingCompleted = true] = useOnyx(ONYXKEYS.NVP_ONBOARDING, {
selector: hasCompletedGuidedSetupFlowSelector,
canBeMissing: true,
});
There was a problem hiding this comment.
@allgandalf The value of this onyx variable might not be present when we load this component as this is the first time the value of onboarding is being fetched and it may take some cycle to load the value.
We pass this to getOnboardingInitialPath, which handles the fallback as mentioned here https://github.com/Expensify/App/pull/69071/files#r2396972152, so i think it's fine even if we don't load the values but we can't wait for the onboarding modal to pop up till we get these values since we don't want the user to navigate unless he has completed onboarding
There was a problem hiding this comment.
Also, why is this useOnyx is duplicated? There is one above too
I missed this, i will update it now
There was a problem hiding this comment.
Oh actually @shubham1206agra we have the following selector:
function hasCompletedGuidedSetupFlowSelector(onboarding: OnyxValue<typeof ONYXKEYS.NVP_ONBOARDING>): boolean | undefined {
// Onboarding is an empty object for old accounts and accounts migrated from OldDot
if (isEmptyObject(onboarding)) {
return true;
}
if (!isEmptyObject(onboarding) && onboarding?.hasCompletedGuidedSetupFlow === undefined) {
return true;
}
return onboarding?.hasCompletedGuidedSetupFlow;
}
it handles accounts from OldDot, so i think we need to fetch the values specifically from selector and not get it directly from useOnyx
There was a problem hiding this comment.
I think that addresses both of your comments, let me know if something is not clear to you
There was a problem hiding this comment.
Can we add this to OnyxListItemProvider to make sure this doesn't happen here?
@allgandalf Can you do this anyway? This will work as failsafe in case logic changes in future.
There was a problem hiding this comment.
@allgandalf Can you do this anyway? This will work as failsafe in case logic changes in future.
@shubham1206agra If i use it via the provider, what will happen?
My main concern is that we shouldn't wait to display the onboarding modal until we have those values present from the provider. The modal should be displayed instantaneously, so if i use it via the provider, will it still work as intended or will it wait until we have the values from the provider?
There was a problem hiding this comment.
will it wait until we have the values from the provider
If you use provider, it will cache the value for common usage, and you won't see the difference.
|
@shubham1206agra ready for your review |
Codecov Report✅ All modified and coverable lines are covered by tests.
... and 7 files with indirect coverage changes 🚀 New features to boost your workflow:
|
Reviewer Checklist
Screenshots/VideosScreen.Recording.2025-10-31.at.6.00.41.PM.mov |
|
@allgandalf Please fix the ts error. |
done |
|
✋ 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/tgolen in version: 9.2.43-0 🚀
|
|
🚀 Deployed to production by https://github.com/luacmartins in version: 9.2.43-2 🚀
|
4 participants
Explanation of Change
Fixed Issues
$ #69028
PROPOSAL:
Tests
Offline tests
QA Steps
Verify that onboarding modal is displayed correctly and you can complete the onboarding
Offline tests
QA Steps
verify that you are able to copilot login into the account
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand 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.https://github.com/user-attachments/assets/4cb9cc4a-8140-404a-b054-58ffe963690d