Stepper: Avoid calling the submit function of the process step multiple times #98197

arthur791004 · 2025-01-10T05:22:39Z

Related to #98167

Proposed Changes

There is a long-term bug here that the submit function will be called multiple times if we
call resetOnboardStoreWithSkipFlags after the submit function (e.g.: exitFlow) to reset states
that are listed as the dependencies of the recordSignupComplete, e.g.: goals, selectedDesign, etc.

Here is a possible flow:

The Design Picker step submits, sets a pending action, and goes to this step
Run the pending action, and then submit first
The submit may trigger the exitFlow function that is defined by each flow
The exitFlow function may set another pending action, and call resetOnboardStoreWithSkipFlags function
The effect of calling the submit runs again since the recordSignupComplete function changes

It's also a reason why we have a hacky to set a pending action to return a Promise that is never resolved.

The issue, #98167, was the exitFlow might be called multiple times. However, we cleared the onboard state on the first call so the active theme would be reset to the default of the write intent when the pending action was triggered again (the selectedDesign became null on the second call.

As a result, this PR proposed to define a flag to avoid calling the submit function multiple times in the short term.

Another way is to remove the recordSignupComplete function from the dependencies of the effect that
is called when the hasActionSuccessfullyRun flag turns on. But it seems to be better to use an explicit
flag to avoid the issue and describe it here.

The current solution will affect all flows. If we want to avoid any regression, we can simply add a flag like isFlowCompleted to the specific flow (e.g.: site-setup) to return immediately when the exitFlow function calls multiple times.

Why are these changes being made?

Testing Instructions

Go to /setup/onboarding with/without goals-first exp
On the Goals screen, select Publish a blog goal
On the Design Picker screen, select any design
On the Domain screen, select any domain
On the Plan screen, select free plan
When you land on the Launchpad, ensure the preview looks like the design you selected

Pre-merge Checklist

Has the general commit checklist been followed? (PCYsg-hS-p2)
Have you written new tests for your changes?
Have you tested the feature in Simple (P9HQHe-k8-p2), Atomic (P9HQHe-jW-p2), and self-hosted Jetpack sites (PCYsg-g6b-p2)?
Have you checked for TypeScript, React or other console errors?
Have you used memoizing on expensive computations? More info in Memoizing with create-selector and Using memoizing selectors and Our Approach to Data
Have we added the "[Status] String Freeze" label as soon as any new strings were ready for translation (p4TIVU-5Jq-p2)?
- For UI changes, have we tested the change in various languages (for example, ES, PT, FR, or DE)? The length of text and words vary significantly between languages.
For changes affecting Jetpack: Have we added the "[Status] Needs Privacy Updates" label if this pull request changes what data or activity we track or use (p4TIVU-aUh-p2)?

github-actions · 2025-01-10T05:22:51Z

Calypso Live (direct link)

	https://calypso.live?image=registry.a8c.com/calypso/app:build-129538

Jetpack Cloud live (direct link)

	https://calypso.live?image=registry.a8c.com/calypso/app:build-129538&env=jetpack

Automattic for Agencies live (direct link)

	https://calypso.live?image=registry.a8c.com/calypso/app:build-129538&env=a8c-for-agencies

matticbot · 2025-01-10T05:30:58Z

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

Sections (~185 bytes added 📈 [gzipped])

name                           parsed_size           gzip_size
write-flow                           +99 B  (+0.0%)      +30 B  (+0.0%)
transferring-hosted-site-flow        +99 B  (+0.0%)      +30 B  (+0.0%)
plugin-bundle-flow                   +99 B  (+0.0%)      +30 B  (+0.0%)
link-in-bio-tld-flow                 +99 B  (+0.0%)      +30 B  (+0.0%)
import-hosted-site-flow              +99 B  (+0.0%)      +30 B  (+0.0%)
copy-site-flow                       +99 B  (+0.0%)      +29 B  (+0.0%)
build-flow                           +99 B  (+0.0%)      +36 B  (+0.0%)

Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to.

Legend

What is parsed and gzip size?

Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

candy02058912

This prevents the exitFlow in site-setup to run twice so that it isn't executed again after the resetOnboardStoreWithSkipFlags has been called.

candy02058912 · 2025-01-10T12:16:50Z

client/landing/stepper/declarative-flow/internals/steps-repository/processing-step/index.tsx

+	 * 4. The `exitFlow` function may set another pending action, and call `resetOnboardStoreWithSkipFlags` function
+	 * 5. The effect to call the `submit` runs again since the `recordSignupComplete` function changes
+	 *
+	 * It's also a reason why we have a hacky to set a pending action to return a Promise that is never resolved.


Suggested change

* It's also a reason why we have a hacky to set a pending action to return a Promise that is never resolved.

* It's also a reason why we have a hack to set a pending action to return a Promise that is never resolved.

candy02058912 · 2025-01-10T12:34:10Z

client/landing/stepper/declarative-flow/internals/steps-repository/processing-step/index.tsx

@@ -49,6 +49,28 @@ const ProcessingStep: React.FC< ProcessingStepProps > = function ( props ) {
 	const [ hasEmptyActionRun, setHasEmptyActionRun ] = useState( false );
 	const [ destinationState, setDestinationState ] = useState( {} );

+	/**
+	 * There is a long-term bug here that the `submit` function will be called multiple times if we
+	 * call `resetOnboardStoreWithSkipFlags` after the submit function (e.g.: exitFlow) to reset states


Note to myself: like here

paulopmt1

Hey @arthur791004, thanks for prioritizing this!
My main concern is the fact that this will change all flows, so I tested a few more flows to try to catch any possible issue, and all of them worked as expected:

Free /start/onboarding with the treatment flag
Free /start/onboarding with the control flag worked as expected fixing the bug
Paid plan on /start/onboarding on both treatment and control
/setup/newsletter also worked as expected
/setup/start-writing
Add a new site inside /sites on both treatment and control flags

alshakero

This looks great and a general precaution that is worth adding. But I think the original issue is that exitFlow is setting a pending action, then redirecting to the processing step. This is certainly not exiting.

exitFlow should be a synchronous function that just redirects. This issue emerged because something rightfully assumed exitFlow will be the last thing to run and reset the store there. But then something else made exitFlow do some non-exiting stuff, and this broke the original assumption.

It would be great if we can make exitFlow not do anything besides exiting. and set the pending action in another function.

…le times

matticbot · 2025-01-10T23:40:13Z

This PR modifies the release build for the following Calypso Apps:

For info about this notification, see here: PCYsg-OT6-p2

notifications

To test WordPress.com changes, run install-plugin.sh $pluginSlug fix/onboarding-write-selected-theme on your sandbox.

arthur791004 self-assigned this Jan 10, 2025

arthur791004 requested review from a team January 10, 2025 05:23

matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jan 10, 2025

arthur791004 mentioned this pull request Jan 10, 2025

Selected theme is ignored in the write flow #98167

Open

taipeicoder requested a review from alshakero January 10, 2025 05:52

candy02058912 approved these changes Jan 10, 2025

View reviewed changes

paulopmt1 approved these changes Jan 10, 2025

View reviewed changes

alshakero approved these changes Jan 10, 2025

View reviewed changes

arthur791004 and others added 2 commits January 10, 2025 20:10

Stepper: Avoid calling the submit function of the process step multip…

2e57b62

…le times

Gate ref workaround to the onboarding flow

01647de

zaguiini force-pushed the fix/onboarding-write-selected-theme branch from c5caaaf to 01647de Compare January 10, 2025 23:10

Gate it to site-setup flow instead

60b5104

zaguiini merged commit 3bd70a3 into trunk Jan 11, 2025
11 checks passed

zaguiini deleted the fix/onboarding-write-selected-theme branch January 11, 2025 00:07

github-actions bot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jan 11, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stepper: Avoid calling the submit function of the process step multiple times #98197

Stepper: Avoid calling the submit function of the process step multiple times #98197

arthur791004 commented Jan 10, 2025 •

edited

Loading

github-actions bot commented Jan 10, 2025 •

edited by matticbot

Loading

matticbot commented Jan 10, 2025 •

edited

Loading

candy02058912 left a comment

candy02058912 Jan 10, 2025

candy02058912 Jan 10, 2025

paulopmt1 left a comment •

edited

Loading

alshakero left a comment

matticbot commented Jan 10, 2025

	* It's also a reason why we have a hacky to set a pending action to return a Promise that is never resolved.
	* It's also a reason why we have a hack to set a pending action to return a Promise that is never resolved.

Stepper: Avoid calling the submit function of the process step multiple times #98197

Stepper: Avoid calling the submit function of the process step multiple times #98197

Conversation

arthur791004 commented Jan 10, 2025 • edited Loading

Proposed Changes

Why are these changes being made?

Testing Instructions

Pre-merge Checklist

github-actions bot commented Jan 10, 2025 • edited by matticbot Loading

matticbot commented Jan 10, 2025 • edited Loading

candy02058912 left a comment

Choose a reason for hiding this comment

candy02058912 Jan 10, 2025

Choose a reason for hiding this comment

candy02058912 Jan 10, 2025

Choose a reason for hiding this comment

paulopmt1 left a comment • edited Loading

Choose a reason for hiding this comment

alshakero left a comment

Choose a reason for hiding this comment

matticbot commented Jan 10, 2025

arthur791004 commented Jan 10, 2025 •

edited

Loading

github-actions bot commented Jan 10, 2025 •

edited by matticbot

Loading

matticbot commented Jan 10, 2025 •

edited

Loading

paulopmt1 left a comment •

edited

Loading