Add isLoading guard to onboarding pages to prevent duplicate tasks#89991
Add isLoading guard to onboarding pages to prevent duplicate tasks#89991MelvinBot wants to merge 6 commits into
Conversation
…ation When clicking Continue multiple times during onboarding, each click fires completeOnboarding with fresh optimistic task IDs, creating duplicate tasks in Concierge. Add isLoading state to BaseOnboardingPersonalDetails, BaseOnboardingPurpose, and BaseOnboardingWorkspaceOptional, matching the existing pattern in BaseOnboardingInterestedFeatures. Co-authored-by: Linh Vo <linhvovan29546@users.noreply.github.com>
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.
|
|
@MelvinBot Please recheck this change to ensure it matches |
|
@MelvinBot Also please resolve conflict |
Match the pattern from BaseOnboardingInterestedFeatures: - Make completeOnboarding calls async with await - Add try/catch/finally to reset isLoading after completion - Add Log.warn for error cases Co-authored-by: Linh Vo <linhvovan29546@users.noreply.github.com>
|
Updated all three files to match the
Changes per file:
|
…icateTaskGuard Co-authored-by: Linh Vo <linhvovan29546@users.noreply.github.com> # Conflicts: # src/pages/OnboardingPersonalDetails/BaseOnboardingPersonalDetails.tsx # src/pages/OnboardingPurpose/BaseOnboardingPurpose.tsx # src/pages/OnboardingWorkspaceOptional/BaseOnboardingWorkspaceOptional.tsx
|
Resolved merge conflicts with main. The conflicts were in all three onboarding files where main replaced |
Replace try/catch/finally with try/catch, placing setIsLoading(false) in both the success path and catch block. React Compiler cannot handle TryStatement with a finalizer clause. Co-authored-by: Linh Vo <linhvovan29546@users.noreply.github.com>
|
Fixed the failing React Compiler Compliance check: replaced |
|
@MelvinBot Resolve conflict and mark PR as "Ready for review" |
Resolve conflicts: keep async/await + try/catch pattern from PR, adopt main's removal of betas parameter from onboarding calls. Co-authored-by: Linh Vo <linhvovan29546@users.noreply.github.com>
|
@linhvovan29546 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] |
|
Resolved merge conflicts and marked as ready for review. Conflicts resolved in 3 files — kept the async/await + try/catch pattern from this PR, adopted main's removal of the
|
|
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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ddef3b135b
ℹ️ 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".
| return; | ||
| } | ||
|
|
||
| setIsLoading(true); |
There was a problem hiding this comment.
Clear loading state when purpose is unset before submit
handleSubmit now sets isLoading to true before calling completeOnboarding, but completeOnboarding immediately returns when onboardingPurposeSelected is falsy. In that state (which can happen if the page is reached via direct navigation/rehydration without a selected purpose), the loading flag is never reset, so the form stays stuck in loading and Continue cannot be pressed again without a reload.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@MelvinBot Please address this. I think we can move setIsLoading to completeOnboarding.
There was a problem hiding this comment.
Done — moved setIsLoading(true) into the completeOnboarding callback (after the guards), matching the existing pattern in BaseOnboardingWorkspaceOptional. Also added isLoading to the guard and dependency array. The track onboarding path in handleSubmit keeps its own setIsLoading(true) since it does not go through this callback.
Moves setIsLoading(true) from handleSubmit into the completeOnboarding useCallback, after the guard checks. This prevents isLoading from being set when completeOnboarding returns early (e.g. when onboardingPurposeSelected is falsy), which would leave the form stuck in a loading state. Matches the existing pattern in BaseOnboardingWorkspaceOptional. Co-authored-by: Linh Vo <linhvovan29546@users.noreply.github.com>
|
Setting this as a draft until ready for review |
Explanation of Change
During onboarding, clicking "Continue" multiple times (after FormProvider's 1-second debounce window) fires
completeOnboardingrepeatedly, and each call generates fresh optimistic task IDs viaprepareOnboardingOnyxData— producing duplicate tasks in Concierge.This adds an
isLoadingstate guard to the three onboarding pages that callcompleteOnboardingwithout one:BaseOnboardingPersonalDetails— addsisLoadingstate and passes it toFormProviderBaseOnboardingPurpose— addsisLoadingstate to guard theonPresshandlersBaseOnboardingWorkspaceOptional— addsisLoadingstate to guard bothcompleteOnboardingandcreateWorkspaceAndCompleteOnboarding, and passes it to the Skip and Create Workspace buttonsThis matches the existing pattern in
BaseOnboardingInterestedFeatures, which already has this guard.Fixed Issues
$ #89904
PROPOSAL: #89904 (comment)
Tests
// TODO: The human co-author must fill out the tests you ran before marking this PR as "ready for review"
// Please describe what tests you performed that validates your changed worked.
Offline tests
// TODO: The human co-author must fill out the offline tests before marking this PR as "ready for review"
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
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)Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
AI Tests