Migration Flow: Fix multiple stickers being added simultaneously when navigating to the "DIY" option #95612

m1r0 · 2024-10-23T11:35:36Z

Resolves #95560

Proposed Changes

This fixes a race condition causing multiple stickers to be added when the "I'll do it myself" option is selected. The problem is explained in detail here.

Why are these changes being made?

We need only one migration sticker to be present at all times.

Testing Instructions

Go to /setup/migration.
Select "WordPress" and the "I'll do it myself" option.
Check the blog stickers for the site (you can check it in the Blog RC), and make sure that only themigration-started-diy sticker is present.

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 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-10-23T11:35:49Z

Calypso Live (direct link)

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

Jetpack Cloud live (direct link)

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

Automattic for Agencies live (direct link)

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

matticbot · 2024-10-23T11:37:33Z

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

Sections (~27 bytes added 📈 [gzipped])

name                     parsed_size           gzip_size
import-hosted-site-flow       +111 B  (+0.0%)      +27 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.

renatho

Thanks @m1r0! In my tests, it seems the issue was fixed! 🙌

I just left some suggestions in the code.

renatho · 2024-10-23T18:27:03Z

.../stepper/declarative-flow/internals/steps-repository/site-migration-how-to-migrate/index.tsx

@@ -1,8 +1,9 @@
 import { StepContainer } from '@automattic/onboarding';
 import { useTranslate } from 'i18n-calypso';
-import { FC, useMemo } from 'react';
+import { FC, useMemo, useState } from 'react';


The component SiteMigrationHowToMigrate is getting a little complex (big). I started extracting part of the logic to another hook here. I think it would be nice to base this PR there and also add the loading logic there, returning the loading state. WDYT?

Sounds good!

I've updated the code but I'm a bit unsure if my implementation is aligned with your vision. 😅 Could you give it a look again?

Yes! That looks great!

.../stepper/declarative-flow/internals/steps-repository/site-migration-how-to-migrate/index.tsx

renatho · 2024-10-23T18:40:31Z

.../stepper/declarative-flow/internals/steps-repository/site-migration-how-to-migrate/index.tsx

@@ -1,8 +1,9 @@
 import { StepContainer } from '@automattic/onboarding';
 import { useTranslate } from 'i18n-calypso';
-import { FC, useMemo } from 'react';
+import { FC, useMemo, useState } from 'react';


And I thought of another scenario where we could have the same race condition issue.
If the user is in a slow connection and they click on one of the options quickly it would be registering the "pending" sticker, and could try to register the "pending-{TYPE}" at the same time.

If we rebase this PR, we could also add the loader for this scenario (when creating the first "pending" sticker).

If we follow the suggestion of the other comment, I think it would assume this behavior automatically.

I think I've managed to fix this scenario but I'm facing an issue with the component loading first and then the loader appearing.

aAeTa2.mp4

Can I fix this without having an extra state in the component?

Hm! I noticed that it's because this fetch depends on the siteId, and it's taking some time to be ready because it's coming from the store. I wonder if we could get it directly from the query string, so we would have this information immediately when the component is loaded to start already with the loader. WDYT?

Could you take a look at this approach? I wasn't able to avoid the site loading because of this check. Do you think it's good enough? 🙂

This approach looks good to me!

But I just noticed another flickering after clicking on the options ("Do it for me", for example). It finishes the loading (the sticker creating that comes from the click), and displays the step again very quickly before calling the onSubmit.

Screen.Recording.2024-10-28.at.13.02.34.mov

I think we'll probably need an extra state here, similar to your previous approach. Maybe something that we set as loading after clicking and keep this state unchanged, so onSubmit is called when it's still in the loading state.

Oh, I didn't noticed that one. I've fixed it here following your suggestion. Thank you for your patience!

m1r0 · 2024-10-25T18:05:01Z

@renatho, I've noticed that I've broken some of the tests. I'll fix those once I have your approval for the implementation approach. 🙂

.../stepper/declarative-flow/internals/steps-repository/site-migration-how-to-migrate/index.tsx

renatho · 2024-10-25T19:56:03Z

.../stepper/declarative-flow/internals/steps-repository/site-migration-how-to-migrate/index.tsx

@@ -1,8 +1,9 @@
 import { StepContainer } from '@automattic/onboarding';
 import { useTranslate } from 'i18n-calypso';
-import { FC, useMemo } from 'react';
+import { FC, useMemo, useState } from 'react';


Yes! That looks great!

.../stepper/declarative-flow/internals/steps-repository/site-migration-how-to-migrate/index.tsx

renatho · 2024-10-25T20:00:07Z

.../stepper/declarative-flow/internals/steps-repository/site-migration-how-to-migrate/index.tsx

@@ -1,8 +1,9 @@
 import { StepContainer } from '@automattic/onboarding';
 import { useTranslate } from 'i18n-calypso';
-import { FC, useMemo } from 'react';
+import { FC, useMemo, useState } from 'react';


Hm! I noticed that it's because this fetch depends on the siteId, and it's taking some time to be ready because it's coming from the store. I wonder if we could get it directly from the query string, so we would have this information immediately when the component is loaded to start already with the loader. WDYT?

gabrielcaires · 2024-10-28T10:01:09Z

We are having page flickering, probably due to the
https://github.com/user-attachments/assets/5e9404f5-54a1-470a-8bd2-748cc0a965e5
#95612 (comment)

I suggest showing the content only when we are ready to show it, preventing any future race conditions.

m1r0 · 2024-10-28T15:16:04Z

@gabrielcaires, do you mean replacing the whole page content, not just the step content?

renatho

Looks good and works well!
I'm already approving, but pinging @gabrielcaires to check if he has anything else to add based on his last comment.

m1r0 · 2024-10-31T15:49:49Z

Merging to unblock a potential refactor.

m1r0 self-assigned this Oct 23, 2024

m1r0 marked this pull request as ready for review October 23, 2024 11:38

m1r0 requested a review from a team October 23, 2024 11:57

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

renatho reviewed Oct 23, 2024

View reviewed changes

Migration Flow: Fix race condition when adding the blog stickers

df6a9e0

m1r0 force-pushed the fix/multiple-migration-stickers-added branch from 080cdb6 to df6a9e0 Compare October 25, 2024 17:54

renatho reviewed Oct 25, 2024

View reviewed changes

m1r0 added 3 commits October 28, 2024 14:58

Remove excessive fragment tag

682f844

Fix loading state flickering

0252787

Fix tests

a9fd24e

Fix flickering after clicking an option

eff54d4

renatho approved these changes Oct 29, 2024

View reviewed changes

renatho mentioned this pull request Oct 31, 2024

Create pending sticker when loading the "how to migrate" step #95413

Merged

7 tasks

m1r0 merged commit 10c31f4 into trunk Oct 31, 2024
11 checks passed

m1r0 deleted the fix/multiple-migration-stickers-added branch October 31, 2024 15:48

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 Oct 31, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migration Flow: Fix multiple stickers being added simultaneously when navigating to the "DIY" option #95612

Migration Flow: Fix multiple stickers being added simultaneously when navigating to the "DIY" option #95612

m1r0 commented Oct 23, 2024 •

edited

Loading

github-actions bot commented Oct 23, 2024 •

edited by matticbot

Loading

matticbot commented Oct 23, 2024 •

edited

Loading

renatho left a comment

renatho Oct 23, 2024

m1r0 Oct 25, 2024

renatho Oct 25, 2024

renatho Oct 23, 2024 •

edited

Loading

m1r0 Oct 25, 2024 •

edited

Loading

renatho Oct 25, 2024

m1r0 Oct 28, 2024 •

edited

Loading

renatho Oct 28, 2024

m1r0 Oct 28, 2024 •

edited

Loading

m1r0 commented Oct 25, 2024

renatho Oct 25, 2024

renatho Oct 25, 2024

gabrielcaires commented Oct 28, 2024

m1r0 commented Oct 28, 2024

renatho left a comment

m1r0 commented Oct 31, 2024

Migration Flow: Fix multiple stickers being added simultaneously when navigating to the "DIY" option #95612

Migration Flow: Fix multiple stickers being added simultaneously when navigating to the "DIY" option #95612

Conversation

m1r0 commented Oct 23, 2024 • edited Loading

Proposed Changes

Why are these changes being made?

Testing Instructions

Pre-merge Checklist

github-actions bot commented Oct 23, 2024 • edited by matticbot Loading

matticbot commented Oct 23, 2024 • edited Loading

renatho left a comment

Choose a reason for hiding this comment

renatho Oct 23, 2024

Choose a reason for hiding this comment

m1r0 Oct 25, 2024

Choose a reason for hiding this comment

renatho Oct 25, 2024

Choose a reason for hiding this comment

renatho Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

m1r0 Oct 25, 2024 • edited Loading

Choose a reason for hiding this comment

renatho Oct 25, 2024

Choose a reason for hiding this comment

m1r0 Oct 28, 2024 • edited Loading

Choose a reason for hiding this comment

renatho Oct 28, 2024

Choose a reason for hiding this comment

m1r0 Oct 28, 2024 • edited Loading

Choose a reason for hiding this comment

m1r0 commented Oct 25, 2024

renatho Oct 25, 2024

Choose a reason for hiding this comment

renatho Oct 25, 2024

Choose a reason for hiding this comment

gabrielcaires commented Oct 28, 2024

m1r0 commented Oct 28, 2024

renatho left a comment

Choose a reason for hiding this comment

m1r0 commented Oct 31, 2024

m1r0 commented Oct 23, 2024 •

edited

Loading

github-actions bot commented Oct 23, 2024 •

edited by matticbot

Loading

matticbot commented Oct 23, 2024 •

edited

Loading

renatho Oct 23, 2024 •

edited

Loading

m1r0 Oct 25, 2024 •

edited

Loading

m1r0 Oct 28, 2024 •

edited

Loading

m1r0 Oct 28, 2024 •

edited

Loading