upcoming: [DI-20360] - Updated preferences logic to remove flickering issue #10853

nikhagra-akamai · 2024-08-29T13:31:11Z

Description 📝

Changed the current implementation of UserPreference to avoid flickering issue in widget component

Changes 🔄

List any change relevant to the reviewer.

Created a custom hook useAclpPreference in UserPreference.ts file
Removed useEffect from CloudPulseWidget component
Updated CloudPulseDashboard component to use default time_granularity if preference is not present

Target release date 🗓️

5th September 2024

Preview 📷

Include a screenshot or screen recording of the change

💡 Use <video src="" /> tag when including recordings in table.

Before	After

How to test 🧪

Switch to mock user as APIs are not live.
Change global filters value & you can check if UI is flickering or not because of component re-renderings

As an Author I have considered 🤔

Check all that apply

…erings

…-pref-redesign

nikhagra-akamai · 2024-09-03T13:57:23Z

Hi @jaalah-akamai / @bnussman-akamai / @abailly-akamai, I've updated the logics to reduce re-renderings. Please start reviewing the PR

github-actions · 2024-09-03T14:32:46Z

Coverage Report: ✅
Base Coverage: 86.98%
Current Coverage: 86.98%

bnussman-akamai

My two main pieces of feedback are:

useRef in UserPreference.ts should not be necessary. If it is necessary to make CloudPulse work, is indicative of a larger issues.
I see preferences and updatePreferences getting passed down via props to a ton of components. Rather than passing it down via props, you can just use the hooks in the component itself. Doing this will allow us to minimize the number of props and could help with the number of components re-rendering

nikhagra-akamai · 2024-09-04T14:34:14Z

My two main pieces of feedback are:

useRef in UserPreference.ts should not be necessary. If it is necessary to make CloudPulse work, is indicative of a larger issues.

I see preferences and updatePreferences getting passed down via props to a ton of components. Rather than passing it down via props, you can just use the hooks in the component itself. Doing this will allow us to minimize the number of props and could help with the number of components re-rendering

Yes refs are needed because we are passing preference when the component initially renders and then we only allow component to render externally when major props change. If we remove refs then we have to call user preference in every component instead of props in this way we can completely remove internal state variables but my only concern is re-renderings will increase compare to what it is now. And if number of filters increased then it may cause impact application performance

jaalah-akamai · 2024-09-05T16:37:50Z

As discussed async - The plan is to move forward with removing the preferences and updatePreferences from being passed down via props and call the hook directly in the components that use preferences where it makes sense. For the components that only rely on preferences on initial load, we can just pass down the specific value they need instead of the entire preferences object which should help with re-rendering (since they're memoized components).

nikhagra-akamai · 2024-09-24T16:48:35Z

@cpathipa I've modulaized the code as per the suggestions please re-review

nikhagra-akamai · 2024-09-26T10:31:42Z

@abailly-akamai / @hkhalil-akamai I've updated all the changes and test cases as well. Please review it once & approve if all good

bnussman-akamai · 2024-09-26T14:39:06Z

packages/manager/src/features/CloudPulse/shared/CloudPulseDashboardSelect.tsx

@@ -123,5 +117,6 @@ export const CloudPulseDashboardSelect = React.memo(
        value={selectedDashboard ?? null} // Undefined is not allowed for uncontrolled component
      />
    );
-  }
+  },
+  () => true


Should this be here? useMemo should only be used if necessary. Is it necessary here?

Yes this compare function is not required, I've removed it from few components. Though useMemo will required as we don't want component to re-render if some other preference value changes

bnussman-akamai · 2024-09-26T14:40:54Z

packages/manager/src/features/CloudPulse/Utils/UserPreference.ts

+  };
+  return {
+    isLoading,
+    preferences: { ...(preferences?.aclpPreference ?? {}) },


What is going on here? Can this be

Suggested change

preferences: { ...(preferences?.aclpPreference ?? {}) },

preferences: preferences?.aclpPreference ?? {},

bnussman-akamai · 2024-09-26T14:41:59Z

packages/manager/src/features/CloudPulse/Utils/utils.ts

+export const arrayDeepEqual = (
+  oldValue: number[] | string[],
+  newValue: number[] | string[]
+): boolean => {


Utils like this need to be unit tested.

I've removed this method, it is not required

bnussman-akamai · 2024-09-26T14:43:58Z

packages/manager/src/features/CloudPulse/Dashboard/CloudPulseDashboardRenderer.tsx

+  (oldProps: DashboardProp, newProps: DashboardProp) => {
+    if (oldProps.dashboard?.id !== newProps.dashboard?.id) {
+      return false;
+    }
+
+    if (
+      oldProps.timeDuration?.unit !== newProps.timeDuration?.unit ||
+      oldProps.timeDuration?.value !== newProps.timeDuration?.value
+    ) {
+      return false;
+    }
+
+    const oldKeys = Object.keys(oldProps.filterValue);
+    const newKeys = Object.keys(newProps.filterValue);
+
+    if (oldKeys.length !== newKeys.length) {
+      return false;
+    }
+
+    for (const key of oldKeys) {
+      const oldValue = oldProps.filterValue[key];
+      const newValue = newProps.filterValue[key];
+
+      if (
+        Array.isArray(oldValue) &&
+        Array.isArray(newValue) &&
+        !arrayDeepEqual(oldValue, newValue)
+      ) {
+        return false;
+      }
+
+      if (oldValue !== newValue) {
+        return false;
+      }
+    }
+    return true;
+  }


I'm not saying this is wrong, but in my time writing React, I've never had to be this granular with memoization. I feel like things shouldn't have to be like this.

I also wonder how the React 19 compiler could affect stuff like this

I just checked this compare function will not be required, only surrounded with memo works

nikhagra-akamai · 2024-09-26T15:19:27Z

@bnussman-akamai thanks for suggestions. Alot of not required code I've removed and pushed.

nikhagra-akamai · 2024-09-30T12:56:45Z

@jaalah-akamai / @abailly-akamai / @hkhalil-akamai can we have another approval & merge

hkhalil-akamai

Thanks for responding to feedback.

nikhagra-akamai · 2024-09-30T14:38:20Z

@cpathipa @hkhalil-akamai thanks for the approvals

cypress · 2024-09-30T21:30:18Z

Cloud Manager E2E Run #6596

Run Properties: Passed #6596 • 033099ccf7: upcoming: [DI-20360] - Updated preferences logic to remove flickering issue (#10...

Project	`Cloud Manager E2E`
Branch Review	`develop`
Run status	`Passed #6596`
Run duration	`25m 12s`
Commit	`033099ccf7: upcoming: [DI-20360] - Updated preferences logic to remove flickering issue (#10...`
Committer	`Nikhil Agrawal`
View all properties for this run ↗︎

Test results
Failures	`0`
Flaky	`4`
Pending	`2`
Skipped	`0`
Passing	`409`
View all changes introduced in this branch ↗︎

nikhagra-akamai requested a review from a team as a code owner August 29, 2024 13:31

nikhagra-akamai requested review from bnussman-akamai and abailly-akamai and removed request for a team August 29, 2024 13:31

nikhagra-akamai added 4 commits August 29, 2024 19:31

upcoming: [DI-20360] - Modified user preference logic

1cda196

upcoming: [DI-20360] - Modified user preference logic

f54ca49

upcoming: [DI-20360] - Removed useEffect from widget component

1096463

upcoming: [DI-20360] - Updated request.tsx file

6e9f899

nikhagra-akamai force-pushed the feature/user-pref-redesign branch from 5a5e8fd to 6e9f899 Compare August 29, 2024 14:05

nikhagra-akamai marked this pull request as draft August 29, 2024 14:09

upcoming: [DI-20360] - remove sensitive information

a7ce52f

coliu-akamai assigned nikhagra-akamai Aug 29, 2024

coliu-akamai added the CloudPulse label Aug 29, 2024

nikhagra-akamai added 4 commits September 2, 2024 18:32

upcoming: [DI-20360] - Update preference logic to avoid extra re-rend…

12fcb4b

…erings

Merge branch 'develop' of github.com:linode/manager into feature/user…

c82dcc3

…-pref-redesign

upcoming: [DI-20360] - Updated preference logic in custom select

7d14f46

upcoming: [DI-20360] - Metrics api hardcoded url removed

a32940d

nikhagra-akamai marked this pull request as ready for review September 2, 2024 13:10

nikhagra-akamai added 3 commits September 3, 2024 18:22

upcoming: [DI-20360] - Updated jsdocs & removed un-used variable

0545491

upcoming: [DI-20360] - Test cases updated

33938a5

upcoming: [DI-20360] - added changeset

c279885

nikhagra-akamai added 2 commits September 3, 2024 22:14

upcoming: [DI-20360] - Removed unused variable

21496fb

upcoming: [DI-20360] - merge conflicts resolved

32a571a

bnussman-akamai reviewed Sep 4, 2024

View reviewed changes

jaalah-akamai self-requested a review September 5, 2024 16:34

jaalah-akamai added the Requires Changes label Sep 5, 2024

nikhagra-akamai requested review from venkymano-akamai and cpathipa September 24, 2024 16:47

upcoming: [DI-20360] - updated eslint issues

a8ee8c9

nikhagra-akamai added Ready for Review and removed Requires Changes labels Sep 24, 2024

nikhagra-akamai added 2 commits September 25, 2024 12:52

upcoming: [DI-20360] - Updated widget preferences logic

da521cd

upcoming: [DI-20360] - Updated deep equal logic

432f48d

nikhagra-akamai requested a review from hkhalil-akamai September 25, 2024 13:19

venkymano-akamai approved these changes Sep 26, 2024

View reviewed changes

nikhagra-akamai added 3 commits September 26, 2024 13:20

Updated aggregate function type

b51e591

upcoming: [DI-20360] - Updated global filter test cases

7a1e419

upcoming: [DI-20360] - Updated eslint issue

88c8c90

bnussman-akamai reviewed Sep 26, 2024

View reviewed changes

upcoming: [DI-20360] - Removed unnecessary compare functions

e25a627

nikhagra-akamai requested a review from bnussman-akamai September 26, 2024 15:19

bnussman-akamai approved these changes Sep 27, 2024

View reviewed changes

cpathipa approved these changes Sep 30, 2024

View reviewed changes

hkhalil-akamai approved these changes Sep 30, 2024

View reviewed changes

nikhagra-akamai added Approved Multiple approvals and ready to merge! and removed Ready for Review labels Sep 30, 2024

jaalah-akamai approved these changes Sep 30, 2024

View reviewed changes

jaalah-akamai merged commit 033099c into linode:develop Sep 30, 2024
20 checks passed

This was referenced Oct 10, 2024

Release v1.130.0 - release → staging #11087

Merged

Release v1.130.0- staging → master #11095

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

upcoming: [DI-20360] - Updated preferences logic to remove flickering issue #10853

upcoming: [DI-20360] - Updated preferences logic to remove flickering issue #10853

nikhagra-akamai commented Aug 29, 2024 •

edited

Loading

nikhagra-akamai commented Sep 3, 2024

github-actions bot commented Sep 3, 2024 •

edited

Loading

bnussman-akamai left a comment

nikhagra-akamai commented Sep 4, 2024 •

edited

Loading

jaalah-akamai commented Sep 5, 2024

nikhagra-akamai commented Sep 24, 2024

nikhagra-akamai commented Sep 26, 2024

bnussman-akamai Sep 26, 2024

nikhagra-akamai Sep 26, 2024

bnussman-akamai Sep 26, 2024

bnussman-akamai Sep 26, 2024

nikhagra-akamai Sep 26, 2024

bnussman-akamai Sep 26, 2024

nikhagra-akamai Sep 26, 2024

nikhagra-akamai commented Sep 26, 2024

nikhagra-akamai commented Sep 30, 2024

hkhalil-akamai left a comment

nikhagra-akamai commented Sep 30, 2024

cypress bot commented Sep 30, 2024

	preferences: { ...(preferences?.aclpPreference ?? {}) },
	preferences: preferences?.aclpPreference ?? {},

upcoming: [DI-20360] - Updated preferences logic to remove flickering issue #10853

upcoming: [DI-20360] - Updated preferences logic to remove flickering issue #10853

Conversation

nikhagra-akamai commented Aug 29, 2024 • edited Loading

Description 📝

Changes 🔄

Target release date 🗓️

Preview 📷

How to test 🧪

As an Author I have considered 🤔

nikhagra-akamai commented Sep 3, 2024

github-actions bot commented Sep 3, 2024 • edited Loading

bnussman-akamai left a comment

Choose a reason for hiding this comment

nikhagra-akamai commented Sep 4, 2024 • edited Loading

jaalah-akamai commented Sep 5, 2024

nikhagra-akamai commented Sep 24, 2024

nikhagra-akamai commented Sep 26, 2024

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

nikhagra-akamai commented Sep 26, 2024

nikhagra-akamai commented Sep 30, 2024

hkhalil-akamai left a comment

Choose a reason for hiding this comment

nikhagra-akamai commented Sep 30, 2024

cypress bot commented Sep 30, 2024

nikhagra-akamai commented Aug 29, 2024 •

edited

Loading

github-actions bot commented Sep 3, 2024 •

edited

Loading

nikhagra-akamai commented Sep 4, 2024 •

edited

Loading