fix: [M3-7249] - Linode Landing flickering #9836

bnussman-akamai · 2023-10-24T14:43:21Z

Description 📝

Fixes flickering on the Linodes landing page caused by the PreferenceToggle component
The rewrites the PreferenceToggle to not store duplicate state and rely on React Query for the state preference state directly.

Changes 🔄

Removed unused localStorageKey prop
No longer blocks PreferenceToggle.tsx's children from rendering if a preference value is

Preview 📷

Before	After
Screen.Recording.2023-10-24.at.10.35.29.AM.mov	Screen.Recording.2023-10-24.at.10.35.50.AM.mov

How to test 🧪

Reproduction steps

Click the Akamai logo while on the Linode Landing page to see the flickering

Verification steps

Verify that the flickering no longer happens when you click on the Akamai logo
Verify that preferences are still loaded and retained normally (for example, if you group by tag and reload, it should retain the group by tag setting)
Go to http://localhost:3000/profile/settings?preferenceEditor=true and clear your preferences ({})
Verify that no flickering happens even when your preferences are empty

As an Author I have considered 🤔

bnussman-akamai · 2023-10-24T14:45:08Z

packages/manager/src/features/Linodes/LinodesLanding/LinodesLanding.tsx

@@ -188,7 +188,6 @@ class ListLinodes extends React.Component<CombinedProps, State> {
        <DocumentTitleSegment segment="Linodes" />
        <ProductInformationBanner bannerLocation="Linodes" />
        <PreferenceToggle<boolean>
-          localStorageKey="GROUP_LINODES"


This prop was unused and did nothing

bnussman-akamai · 2023-10-24T16:07:25Z

packages/manager/src/features/Linodes/LinodesLanding/LinodesLanding.tsx

          preferenceKey="linodes_group_by_tag"
          preferenceOptions={[false, true]}
-          toggleCallbackFnDebounced={sendGroupByAnalytic}


Because we now rely on React Query's state directly, we won't debounce because React Query performs optimistic updates. I don't think this is an issue considering how and where this component is used.

bnussman-akamai · 2023-10-24T16:28:49Z

packages/manager/src/MainContent.test.ts

@@ -52,31 +43,3 @@ describe('checkPreferencesForBannerDismissal', () => {
    expect(checkPreferencesForBannerDismissal({}, 'key1')).toBe(false);
  });
 });
-
-describe('Databases menu item for a restricted user', () => {
-  it('should not render the menu item', async () => {


The intentions of this test are not clear. We have tests for this case in the PrimaryNav test and AddNewMenu so I'm not sure what this one was intended to check. It was flakey because it tries to render the entire app

jdamore-linode · 2023-10-24T17:07:10Z

It looks like the test failure in managed-navigation.spec.ts is legitimate -- For Managed users, Cloud doesn't seem to be redirecting from / to /managed/summary and is always redirecting to /linodes instead.

bnussman-akamai · 2023-10-24T17:25:50Z

packages/manager/src/components/AuthenticationWrapper/AuthenticationWrapper.tsx

@@ -142,6 +140,7 @@ export class AuthenticationWrapper extends React.Component<CombinedProps> {
      /** We choose to do nothing, relying on the Redux error state. */
    } finally {
      this.props.markAppAsDoneLoading();
+      this.setState({ showChildren: true });


Fixes e2e failure @jdamore-linode mentioned.

PreferenceToggle use to render-block all children until preferences were loaded. Now we are render-blocking at this level, which makes more sense anyway.

/** We choose to do nothing, relying on the Redux error state. */

It's my first time visiting this code in a long time. I understand we to load content and render tree in the case of a fetching error, but what does the redux error do exactly? The user does not get any feedback something fails (ex: try blocking any API request). It's probably beyond the scope of this PR but wondering what your thoughts are about it.

bnussman-akamai · 2023-10-24T22:32:49Z

I spun up devenv to verify that authentication still works as expected. I set the login expiry to 10 seconds to confirm Cloud Manager re-auths as expected

Screen.Recording.2023-10-24.at.6.30.15.PM.mov

abailly-akamai

Thanks for working on this! This is a sensitive area 💅

Flicker is gone and did not notice any bug/regression ✅

Will approve once getting some clarity on the items I commented about

abailly-akamai · 2023-10-25T14:26:47Z

packages/manager/src/components/AuthenticationWrapper/AuthenticationWrapper.tsx

@@ -90,8 +82,12 @@ export class AuthenticationWrapper extends React.Component<CombinedProps> {
  render() {
    const { children } = this.props;
    const { showChildren } = this.state;


Any thoughts on renaming showChildren? Isn't this pretty much "renderTree"? wondering what could make a tad more clear the fact we're really loading the application.

I was considering something more like isLoading but I decided to just leave as is for now. I definitely plan to follow this PR up with some others that improve our general render tree/ entry points. It's an absolute mess.

abailly-akamai · 2023-10-25T14:46:39Z

packages/manager/src/components/AuthenticationWrapper/AuthenticationWrapper.tsx

@@ -142,6 +140,7 @@ export class AuthenticationWrapper extends React.Component<CombinedProps> {
      /** We choose to do nothing, relying on the Redux error state. */
    } finally {
      this.props.markAppAsDoneLoading();
+      this.setState({ showChildren: true });


/** We choose to do nothing, relying on the Redux error state. */

It's my first time visiting this code in a long time. I understand we to load content and render tree in the case of a fetching error, but what does the redux error do exactly? The user does not get any feedback something fails (ex: try blocking any API request). It's probably beyond the scope of this PR but wondering what your thoughts are about it.

abailly-akamai · 2023-10-25T14:49:09Z

packages/manager/src/components/PreferenceToggle/PreferenceToggle.tsx

-    setPreference(newPreferenceToSet);
+    updateUserPreferences({
+      [preferenceKey]: newPreferenceToSet,
+    }).catch(() => /** swallow the error */ null);


Swallow the error? What does it mean for the end user?

It means their updated preferences won't be sent to the API. (this is existing code)

Right, and they won't know anything about it :( How about a TODO follow up item and associated ticket to display a toast (or whatever we decide) to handle this better?

Yeah, we could toast. I think we decided to swallow the errors because the idea of preferences is that is should be a low-impact failure if that makes any sense.

Created M3-7326

bnussman-akamai · 2023-10-25T15:12:00Z

@abailly-akamai I think that comment refers to our Axios interceptors that populate something called "globalErrors" in redux. From there we unblock rendering and surface whatever the UI would show. We definitely should consider adding a fallback for cases when the account/profile fail to load. The neat thing is that the app still somewhat functions if these fail and other requests succeed.

bnussman-akamai · 2023-10-25T16:49:06Z

I also confirmed this page still renders 👍🏼

jdamore-linode · 2023-10-25T17:15:09Z

packages/manager/cypress/e2e/core/general/account-activation.spec.ts

@@ -0,0 +1,27 @@
+import { apiMatcher } from 'support/util/intercepts';


Awesome!!!

Edit: Whoops, meant for this to be for the entire file lol

jdamore-linode

Nice work, @bnussman-akamai

Not seeing any flickering
Cypress and Jest tests are passing

Really enthusiastic about the account activation test, too! Great idea!

dwiley-akamai

Flickering no longer happens when you click on the Akamai logo ✅
Group by tag & Summary view preferences ✅
No flickering even when preferences are empty ✅
Code review ✅

packages/manager/src/IdentifyUser.tsx

bnussman-akamai · 2023-10-25T20:51:17Z

Planning to merge after the release is ✂️

Co-authored-by: Dajahi Wiley <114682940+dwiley-akamai@users.noreply.github.com>

dont block children from rendering

ee56a25

bnussman-akamai added the Bug Fixes for regressions or bugs label Oct 24, 2023

bnussman-akamai requested a review from a team as a code owner October 24, 2023 14:43

bnussman-akamai self-assigned this Oct 24, 2023

bnussman-akamai requested review from dwiley-akamai and abailly-akamai and removed request for a team October 24, 2023 14:43

Added changeset: Linodes Landing flickering

a87259a

bnussman-akamai commented Oct 24, 2023

View reviewed changes

clean up more

abaf5ae

bnussman-akamai marked this pull request as draft October 24, 2023 14:59

fix preference toggle

e8c1be5

bnussman-akamai marked this pull request as ready for review October 24, 2023 15:56

comment and clean up types

473c710

bnussman-akamai commented Oct 24, 2023

View reviewed changes

remove ambiguous test

ac73415

bnussman-akamai commented Oct 24, 2023

View reviewed changes

show children after inital fetches

eaecf6a

bnussman-akamai commented Oct 24, 2023

View reviewed changes

bnussman added 5 commits October 24, 2023 13:54

fix AuthenticationWrapper logic and test

3966d0b

revert change

ec5f134

removed unused state

91e98b1

fix lish

67adbf4

fix AuthenticationWrapper more

bd1967d

bnussman added 2 commits October 24, 2023 19:52

fix flags flickering

2832482

Merge branch 'develop' into M3-7249-fix-linode-landing-flickering

eb7ae37

abailly-akamai reviewed Oct 25, 2023

View reviewed changes

add some e2e coverage

d8af7a1

bnussman-akamai requested a review from jdamore-linode October 25, 2023 17:12

jdamore-linode reviewed Oct 25, 2023

View reviewed changes

jdamore-linode approved these changes Oct 25, 2023

View reviewed changes

bnussman-akamai added the Add'tl Approval Needed Waiting on another approval! label Oct 25, 2023

dwiley-akamai approved these changes Oct 25, 2023

View reviewed changes

packages/manager/src/IdentifyUser.tsx Outdated Show resolved Hide resolved

bnussman-akamai removed the Add'tl Approval Needed Waiting on another approval! label Oct 25, 2023

Update packages/manager/src/IdentifyUser.tsx

ab8bcfe

Co-authored-by: Dajahi Wiley <114682940+dwiley-akamai@users.noreply.github.com>

mjac0bs added the Approved Multiple approvals and ready to merge! label Oct 25, 2023

bnussman-akamai merged commit f26f788 into linode:develop Oct 25, 2023
12 of 13 checks passed

This was referenced Nov 8, 2023

Release v1.107.0 - release → staging #9889

Merged

Release v1.107.0 - staging → master #9898

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: [M3-7249] - Linode Landing flickering #9836

fix: [M3-7249] - Linode Landing flickering #9836

bnussman-akamai commented Oct 24, 2023 •

edited

Loading

bnussman-akamai Oct 24, 2023

bnussman-akamai Oct 24, 2023 •

edited

Loading

bnussman-akamai Oct 24, 2023

jdamore-linode commented Oct 24, 2023

bnussman-akamai Oct 24, 2023

abailly-akamai Oct 25, 2023

bnussman-akamai commented Oct 24, 2023

abailly-akamai left a comment

abailly-akamai Oct 25, 2023

bnussman-akamai Oct 25, 2023

abailly-akamai Oct 25, 2023

abailly-akamai Oct 25, 2023

bnussman-akamai Oct 25, 2023

abailly-akamai Oct 25, 2023

bnussman-akamai Oct 25, 2023

bnussman-akamai Oct 25, 2023

bnussman-akamai commented Oct 25, 2023

bnussman-akamai commented Oct 25, 2023

jdamore-linode Oct 25, 2023 •

edited

Loading

jdamore-linode left a comment

dwiley-akamai left a comment

bnussman-akamai commented Oct 25, 2023

		@@ -0,0 +1,27 @@
		import { apiMatcher } from 'support/util/intercepts';

fix: [M3-7249] - Linode Landing flickering #9836

fix: [M3-7249] - Linode Landing flickering #9836

Conversation

bnussman-akamai commented Oct 24, 2023 • edited Loading

Description 📝

Changes 🔄

Preview 📷

How to test 🧪

Reproduction steps

Verification steps

As an Author I have considered 🤔

Choose a reason for hiding this comment

bnussman-akamai Oct 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdamore-linode commented Oct 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnussman-akamai commented Oct 24, 2023

abailly-akamai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnussman-akamai commented Oct 25, 2023

bnussman-akamai commented Oct 25, 2023

jdamore-linode Oct 25, 2023 • edited Loading

Choose a reason for hiding this comment

jdamore-linode left a comment

Choose a reason for hiding this comment

dwiley-akamai left a comment

Choose a reason for hiding this comment

bnussman-akamai commented Oct 25, 2023

bnussman-akamai commented Oct 24, 2023 •

edited

Loading

bnussman-akamai Oct 24, 2023 •

edited

Loading

jdamore-linode Oct 25, 2023 •

edited

Loading