Stepper: Fix the previous step of the user step may be wrong #97775

arthur791004 · 2024-12-25T07:06:20Z

Related to #

Proposed Changes

The flow relied on the firstAuthWalledStep and lastPreAuthWalledStep to get the previous step before auth and the next step after auth. However, it made the flow become liner because both of them are the result of the linear search in the steps array. So, the previous step and the next step might be wrong.
- For example, if there were 2 steps (A1, A2) go to auth and 2 steps (B1, B2) after the auth, then the back button might always go back to the A1 step, and user might always be redirected to B1 step after the auth.
```
A1 -> auth -> B1
A2 -> auth -> B2
```
This PR proposed
- Save the current previous step before navigating to the user step so we can know what's the correct previous step
- Save the current step that requires the auth as a next step so we can know what's the correct next step after the auth

Why are these changes being made?

The previous step & next step of the user step may be wrong.

Testing Instructions

Go to /setup/onboarding without logged-in
Select any goals
Select any design
On the user step,
- Click the back button
- Ensure you're back to the Design Picker screen
- Continue to the user step again
- Log in
- Ensure you can see the Domains screen

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 · 2024-12-25T07:06:32Z

Calypso Live (direct link)

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

Jetpack Cloud live (direct link)

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

Automattic for Agencies live (direct link)

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

matticbot · 2024-12-25T07:15:23Z

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

App Entrypoints (~5505 bytes added 📈 [gzipped])

name           parsed_size           gzip_size
entry-stepper     +75051 B  (+5.3%)    +5505 B  (+1.3%)

Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used.

Sections (~86088 bytes removed 📉 [gzipped])

name                        parsed_size            gzip_size
ai-assembler-flow              -74928 B  (-58.6%)    -6371 B  (-27.6%)
update-options-flow            -74915 B  (-97.6%)    -5988 B  (-87.7%)
update-design-flow             -74915 B   (-4.9%)    -6696 B   (-1.6%)
trial-wooexpress-flow          -74915 B  (-93.7%)    -5969 B  (-73.6%)
tailored-ecommerce-flow        -74915 B  (-91.7%)    -5741 B  (-72.1%)
site-setup-wg                  -74915 B  (-27.6%)    -6352 B   (-9.6%)
site-setup-flow                -74915 B  (-27.7%)    -6352 B   (-9.7%)
site-migration-flow            -74915 B  (-59.8%)    -6154 B  (-27.4%)
readymade-template-flow        -74915 B  (-27.6%)    -6190 B   (-9.3%)
onboarding-flow                -74915 B  (-76.3%)    -5789 B  (-44.1%)
migration-signup               -74915 B  (-84.2%)    -6153 B  (-55.0%)
migration-flow                 -74915 B  (-69.3%)    -6154 B  (-34.5%)
import-flow                    -74915 B  (-77.5%)    -6187 B  (-45.6%)
hosted-site-migration-flow     -74915 B  (-59.5%)    -6154 B  (-27.0%)
free-post-setup-flow           -74915 B  (-98.2%)    -5989 B  (-89.3%)
entrepreneur-flow              -74915 B  (-26.3%)    -6117 B   (-8.2%)
assembler-first-flow           -74915 B  (-31.4%)    -6392 B  (-11.2%)

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.

matticbot · 2024-12-25T07:16:25Z

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

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

notifications
wpcom-block-editor

To test WordPress.com changes, run install-plugin.sh $pluginSlug fix/stepper-auth-previous-step on your sandbox.

zaguiini

Works well for me as I get sent back to the design setup step.

alshakero

Hi Arthur! One of the main reasons we built Stepper is to avoid the assumption of linearity.

When things are assumed to be linear, people come up with all kinds of (bad) contraptions to create non-linear scenarios. The design of extraData, especially previousStep is antithetical to the Stepper design, and should be removed. Luckily, only 2–3 flows use it and I will remove it soon.

Could you please give me the reproduction steps of the bug you are describing?

alshakero · 2024-12-25T16:04:45Z

client/landing/stepper/declarative-flow/internals/steps.tsx

+	USER: {
+		slug: 'user',
+		asyncComponent: () =>
+			import( /* webpackChunkName: "stepper-user-step" */ './steps-repository/__user' ),
+	},


We don't want the user step in the steps library. It's a private step that should only used by the framework itself.

I don't think it makes sense because there are some components know what's the user step and use hardcoded slug, user, to determine whether it's a user step. It doesn't look like a private step. In addition, what's the benefit of making the user step private? If there is a flow that requires auth at the beginning, we can still make the user step to the first step explicitly.

alshakero · 2024-12-25T16:05:00Z

client/landing/stepper/utils/enhanceFlowWithAuth.ts

-const USER_STEP: StepperStep = {
-	slug: 'user',
-	asyncComponent: () =>
-		import(
-			/* webpackChunkName: "stepper-user-step" */ '../declarative-flow/internals/steps-repository/__user'
-		),
-};


Let's keep this please 🙏🏼

alshakero · 2024-12-25T16:06:49Z

client/landing/stepper/declarative-flow/internals/components/step-route/index.tsx

+		const extraData = {
+			previousStep: stepData?.previousStep,
+			nextStep: step.slug,
+		};


I think the extraData feature is a source of bugs and I was hoping we should remove it. Luckily, only 2-3 flows use it at this point. Adding state in general should be an elaborate and careful design. The main reason I prefer Stepper to Start is the statelessness. It would be amazing if we can come up with another way.

What's the bug of the extraData? I agreed we can remove it if it's not a great design or re-design the structure of the step data. However, I think it's an urgent issue for the onboarding flow and we can come up with a better solution in the future.

alshakero · 2024-12-25T16:46:49Z

Here's an alternative take, let me know what you think! #97786

arthur791004 · 2024-12-26T03:19:51Z

Could you please give me the reproduction steps of the bug you are describing?

Sorry for not being clear. I've updated the description 🙏

arthur791004 requested a review from a team as a code owner December 25, 2024 07:06

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

arthur791004 requested review from a team December 25, 2024 07:06

arthur791004 self-assigned this Dec 25, 2024

Stepper: Fix the previous step of the user step may be wrong

d4fb88d

arthur791004 force-pushed the fix/stepper-auth-previous-step branch from 2c90218 to d4fb88d Compare December 25, 2024 07:14

Fix types

53eb109

arthur791004 force-pushed the fix/stepper-auth-previous-step branch from 7ecc074 to 53eb109 Compare December 25, 2024 10:24

zaguiini approved these changes Dec 25, 2024

View reviewed changes

Avoid showing the back button if the first step requires auth

08aba3d

alshakero requested changes Dec 25, 2024

View reviewed changes

alshakero mentioned this pull request Dec 25, 2024

Stepper: Fix back button in user step #97786

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stepper: Fix the previous step of the user step may be wrong #97775

Stepper: Fix the previous step of the user step may be wrong #97775

arthur791004 commented Dec 25, 2024 •

edited

Loading

github-actions bot commented Dec 25, 2024 •

edited by matticbot

Loading

matticbot commented Dec 25, 2024 •

edited

Loading

matticbot commented Dec 25, 2024 •

edited

Loading

zaguiini left a comment

alshakero left a comment

alshakero Dec 25, 2024

arthur791004 Dec 26, 2024

alshakero Dec 25, 2024

alshakero Dec 25, 2024

arthur791004 Dec 26, 2024

alshakero commented Dec 25, 2024

arthur791004 commented Dec 26, 2024

Stepper: Fix the previous step of the user step may be wrong #97775

Are you sure you want to change the base?

Stepper: Fix the previous step of the user step may be wrong #97775

Conversation

arthur791004 commented Dec 25, 2024 • edited Loading

Proposed Changes

Why are these changes being made?

Testing Instructions

Pre-merge Checklist

github-actions bot commented Dec 25, 2024 • edited by matticbot Loading

matticbot commented Dec 25, 2024 • edited Loading

matticbot commented Dec 25, 2024 • edited Loading

zaguiini left a comment

Choose a reason for hiding this comment

alshakero left a comment

Choose a reason for hiding this comment

alshakero Dec 25, 2024

Choose a reason for hiding this comment

arthur791004 Dec 26, 2024

Choose a reason for hiding this comment

alshakero Dec 25, 2024

Choose a reason for hiding this comment

alshakero Dec 25, 2024

Choose a reason for hiding this comment

arthur791004 Dec 26, 2024

Choose a reason for hiding this comment

alshakero commented Dec 25, 2024

arthur791004 commented Dec 26, 2024

arthur791004 commented Dec 25, 2024 •

edited

Loading

github-actions bot commented Dec 25, 2024 •

edited by matticbot

Loading

matticbot commented Dec 25, 2024 •

edited

Loading

matticbot commented Dec 25, 2024 •

edited

Loading