Plans 2023: Account for siteId when updating storage add-ons UI state #87342

jeyip · 2024-02-09T18:25:14Z

Related to #85773

Proposed Changes

Persist storage add-on UI state on a per site basis

Testing Instructions

Navigate to /plans and select a non-standard storage add-on option
Switch sites
Verify that the storage add-on option has changed to the default

Pre-merge Checklist

Has the general commit checklist been followed? (PCYsg-hS-p2)
https://wpcalypso.wordpress.com/devdocs/docs/testing/index.md 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-ajp-p2)?

github-actions · 2024-02-09T18:25:38Z

Calypso Live (direct link)

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

Jetpack Cloud live (direct link)

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

Automattic for Agencies live (direct link)

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

matticbot · 2024-02-12T22:02:18Z

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

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

odyssey-stats
wpcom-block-editor

To test WordPress.com changes, run install-plugin.sh $pluginSlug update/storage-add-on-ui-store on your sandbox.

matticbot · 2024-02-12T22:06:29Z

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

Sections (~108 bytes added 📈 [gzipped])

name                  parsed_size           gzip_size
update-design-flow         +207 B  (+0.0%)      +44 B  (+0.0%)
plugins                    +207 B  (+0.0%)      +44 B  (+0.0%)
plans                      +207 B  (+0.0%)      +44 B  (+0.0%)
link-in-bio-tld-flow       +207 B  (+0.0%)      +44 B  (+0.0%)
jetpack-app                +207 B  (+0.1%)      +44 B  (+0.0%)
theme                       +65 B  (+0.0%)      +23 B  (+0.0%)
stats                       +65 B  (+0.0%)      +21 B  (+0.0%)
import-flow                 +65 B  (+0.0%)      +20 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.

Async-loaded Components (~44 bytes added 📈 [gzipped])

name                                             parsed_size           gzip_size
async-load-signup-steps-plans-theme-preselected       +207 B  (+0.1%)      +44 B  (+0.0%)
async-load-signup-steps-plans                         +207 B  (+0.1%)      +44 B  (+0.0%)

React components that are loaded lazily, when a certain part of UI is displayed for the first time.

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.

jeyip · 2024-02-12T22:39:23Z

packages/data-stores/src/plans/hooks/use-pricing-meta-for-grid-plans.ts

@@ -77,7 +77,7 @@ const usePricingMetaForGridPlans = ( {
 		purchaseId: currentPlan?.purchaseId,
 	} );
 	const selectedStorageOptions = useSelect(
-		( select ) => select( WpcomPlansUI.store ).getSelectedStorageOptions(),
+		( select ) => select( WpcomPlansUI.store ).getSelectedStorageOptions( selectedSiteId ?? 0 ),


TODO: Consider empty state for siteId ( Ex. onboarding for new site )

It looks like 0 is used to represent a non-existent siteId throughout the codebase. I believe it makes sense here as well, but feel free to chime in if it doesn't

I think it's mainly used to bypass typescript when used with hooks for existing hooks. But I think the right representation of a non existing site id is either null or undefined. We should probably follow it where possible, especially when introducing new code. Because just sending 0 is trying to trick the type system where actually the underlying hook or function should be handling the empty value case.

I know sometimes for existing hooks we do this to just move things forward. And sometimes it's not possible to fix them due to the nature of hooks. But in other places I think we should avoid introducing them.

~~Correct me if I am wrong here though. Because seems like we might have to do a lot of extra work to support the 'Empty' value for the selected site id.~~

Looks like it can be done by changing line at
https://github.com/Automattic/wp-calypso/pull/87342/files#diff-2acc0cb518c786c436317cfc21f6d624bb922f4b372df93dc57534b10f6786beR10-R11

IIRC typescript gets upset at us for attempting to index an object with a null or undefined value. I'll double check tomorrow though

After reading over #87342 (comment), just wanted to say thanks again for the insight @ddc22.

I removed usage of 0 to represent a non-existent site id in 07e8ae7

southp · 2024-03-05T00:49:23Z

There is a dedicated issue for the bug now: #86152 :)

southp · 2024-03-15T10:36:01Z

@jeyip When you have a moment, could you help me clarify the status of this? I'd be grateful :)

jeyip · 2024-03-18T21:32:03Z

Hi @southp! Thanks for pinging this. I'll check later today

jeyip · 2024-03-19T19:02:16Z

packages/data-stores/src/wpcom-plans-ui/selectors.ts

+) => {
+	// @ts-expect-error TS is unhappy if we index by a null or an undefined value. We, however,
+	// expect siteId to be null or undefined here before site creation ( like during onboarding ).
+	return state.selectedStorageOptionForPlans?.[ siteId ][ planSlug ];


@ddc22 To maintain the representation of a non-existent siteId as null or undefined, we now index state by null or undefined values. This gives me pause, but since we're doing this intentionally, and since JS coerces these false-y values into strings before indexing ( Ex. state[null] becomes state['null'] ) , I believe it's fine. Lemme know if you have thoughts about this though.

jeyip · 2024-03-20T18:52:32Z

Merging this PR for the time being. Happy to return and make any further changes requested by folks

jeyip added the 2023 Pricing Page ⚡ Changes for pricing page optimisation 2023 label Feb 9, 2024

jeyip self-assigned this Feb 9, 2024

jeyip force-pushed the update/storage-add-on-ui-store branch from 1bd14e3 to 4845441 Compare February 12, 2024 21:34

jeyip marked this pull request as ready for review February 12, 2024 22:38

jeyip requested a review from a team as a code owner February 12, 2024 22:38

jeyip requested a review from chriskmnds February 12, 2024 22:38

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

jeyip commented Feb 12, 2024

View reviewed changes

jeyip changed the title ~~Plans 2023: Update storage add-ons UI store~~ Plans 2023: Account for siteId when updating storage add-ons UI state Feb 12, 2024

jeyip mentioned this pull request Feb 12, 2024

Auditing: storage add-on UI issues #85773

Open

jeyip force-pushed the update/storage-add-on-ui-store branch 2 times, most recently from 3c1ede6 to 205c44a Compare March 19, 2024 18:08

jeyip commented Mar 19, 2024

View reviewed changes

jeyip added 7 commits March 20, 2024 11:04

Update storage add-ons UI store

83bcd27

Update wpcom storage add-on UI store with siteId

cd427d2

Add siteId to upgradeClickHandler

258bc74

Add planslug type

24bce96

Add siteId type

3b4d082

Remove usage of 0 as representation of nonexistant site id

0ec8ad6

Add ts expect error comment

d1e60b5

jeyip force-pushed the update/storage-add-on-ui-store branch from 8e83652 to 22fa91e Compare March 20, 2024 18:05

Make selectors index state by null or undefined site ids

28a1781

jeyip force-pushed the update/storage-add-on-ui-store branch from 22fa91e to 28a1781 Compare March 20, 2024 18:16

jeyip added 2 commits March 20, 2024 12:21

Fix null propagation operator bug

90f4cee

Fix state override with siteId

cb56cee

jeyip merged commit 1219c81 into trunk Mar 20, 2024
11 checks passed

jeyip deleted the update/storage-add-on-ui-store branch March 20, 2024 22:11

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 Mar 20, 2024

jeyip mentioned this pull request Mar 20, 2024

Storage add-on: The drop-down state is shared between sites. #86152

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Plans 2023: Account for siteId when updating storage add-ons UI state #87342

Plans 2023: Account for siteId when updating storage add-ons UI state #87342

jeyip commented Feb 9, 2024 •

edited

Loading

github-actions bot commented Feb 9, 2024 •

edited by matticbot

Loading

matticbot commented Feb 12, 2024 •

edited

Loading

matticbot commented Feb 12, 2024 •

edited

Loading

jeyip Feb 12, 2024

jeyip Feb 12, 2024

ddc22 Feb 13, 2024

ddc22 Feb 13, 2024 •

edited

Loading

jeyip Feb 13, 2024

jeyip Mar 19, 2024

southp commented Mar 5, 2024

southp commented Mar 15, 2024

jeyip commented Mar 18, 2024

jeyip Mar 19, 2024 •

edited

Loading

jeyip commented Mar 20, 2024

Plans 2023: Account for siteId when updating storage add-ons UI state #87342

Plans 2023: Account for siteId when updating storage add-ons UI state #87342

Conversation

jeyip commented Feb 9, 2024 • edited Loading

Proposed Changes

Testing Instructions

Pre-merge Checklist

github-actions bot commented Feb 9, 2024 • edited by matticbot Loading

matticbot commented Feb 12, 2024 • edited Loading

matticbot commented Feb 12, 2024 • edited Loading

jeyip Feb 12, 2024

Choose a reason for hiding this comment

jeyip Feb 12, 2024

Choose a reason for hiding this comment

ddc22 Feb 13, 2024

Choose a reason for hiding this comment

ddc22 Feb 13, 2024 • edited Loading

Choose a reason for hiding this comment

jeyip Feb 13, 2024

Choose a reason for hiding this comment

jeyip Mar 19, 2024

Choose a reason for hiding this comment

southp commented Mar 5, 2024

southp commented Mar 15, 2024

jeyip commented Mar 18, 2024

jeyip Mar 19, 2024 • edited Loading

Choose a reason for hiding this comment

jeyip commented Mar 20, 2024

jeyip commented Feb 9, 2024 •

edited

Loading

github-actions bot commented Feb 9, 2024 •

edited by matticbot

Loading

matticbot commented Feb 12, 2024 •

edited

Loading

matticbot commented Feb 12, 2024 •

edited

Loading

ddc22 Feb 13, 2024 •

edited

Loading

jeyip Mar 19, 2024 •

edited

Loading