refactor: [M3-8501] - AccessSelect Optimization: Use React Hook Form #10952

harsh-akamai · 2024-09-17T12:29:15Z

Description 📝

The goal is to see if we can eliminate the number of useState calls we're making in this file and potentially remove the useEffect by using React Hook Form and inferring values from other values.

Changes 🔄

Replaced the existing queries with react-query queries and mutations for managing state
Used React Hook Form for better state management

Target release date 🗓️

N/A

How to test 🧪

Verification steps

Check if the AccessSelect component's functionality is unaffected by the changes

As an Author I have considered 🤔

Check all that apply

… & React Query

abailly-akamai · 2024-09-18T13:36:47Z

@harsh-akamai can you make sure to fix your unit tests before people review the PR?

harsh-akamai · 2024-09-20T11:39:00Z

@abailly-akamai I have fixed the failing unit tests

github-actions · 2024-09-20T12:02:44Z

Coverage Report: ✅
Base Coverage: 86.95%
Current Coverage: 86.95%

abailly-akamai

Thanks for working on this. The change makes sense and the code cleanup looks good, and is well self documented and tested

There is a state issue in the form however. The cors toggle shows always as enabled on page load, no matter it's value.

Screen.Recording.2024-09-27.at.09.12.56.mov

Looks like e2e test failures are flakes. You may want to rebase your PRA again to get the latest from develop since those have been fixed there i believe

packages/manager/src/features/ObjectStorage/BucketDetail/AccessSelect.tsx

abailly-akamai

Looks good, thanks for addressing the fix.

Approving, and leaving a few optional UI improvements to consider, out of the scope but we may as well tackle them now:

jaalah-akamai

Really nice job here! Just some minor changes and this is looking 🔥

jaalah-akamai · 2024-10-02T16:29:50Z

packages/manager/src/features/ObjectStorage/BucketDetail/AccessSelect.tsx

+            onChange={(_, selected) => {
+              if (selected) {
+                field.onChange(selected.value);
+              }
+            }}


We can just set the value on the onChange itself

Suggested change

onChange={(_, selected) => {

if (selected) {

field.onChange(selected.value);

}

}}

onChange={(_, selected: { label: string; value: ACLType }) => {

if (selected) {

setValue('acl', selected.value);

}

}}

jaalah-akamai · 2024-10-02T16:31:46Z

packages/manager/src/features/ObjectStorage/BucketDetail/AccessSelect.tsx

+                <Toggle
+                  {...field}
+                  checked={field.value}
+                  disabled={bucketAccessIsFetching || objectAccessIsFetching}


Suggested change

disabled={bucketAccessIsFetching || objectAccessIsFetching}

disabled={bucketAccessIsFetching || objectAccessIsFetching}

onChange={(e) => setValue('cors_enabled', e.target.checked)}

jaalah-akamai · 2024-10-02T16:35:53Z

packages/manager/src/features/ObjectStorage/BucketDetail/AccessSelect.tsx

+    defaultValues: {
+      acl: 'private',
+      cors_enabled: true,
+    },


In correlation to changes from removing useEffect

Suggested change

defaultValues: {

acl: 'private',

cors_enabled: true,

},

defaultValues

jaalah-akamai · 2024-10-02T16:39:26Z

packages/manager/src/features/ObjectStorage/BucketDetail/AccessSelect.tsx

 import { capitalize } from 'src/utilities/capitalize';
 import { getErrorStringOrDefault } from 'src/utilities/errorUtils';

 import { bucketACLOptions, objectACLOptions } from '../utilities';
 import { copy } from './AccessSelect.data';

 import type {
-  ACLType,


We'll need this back with my suggested changes

jaalah-akamai · 2024-10-02T16:40:07Z

packages/manager/src/features/ObjectStorage/BucketDetail/AccessSelect.tsx

+    control,
+    formState: { errors, isSubmitting },
+    handleSubmit,
+    reset,


Replacing reset with setValue for more granular control in new approach

Suggested change

reset,

setValue,

jaalah-akamai · 2024-10-02T16:44:30Z

packages/manager/src/features/ObjectStorage/BucketDetail/AccessSelect.tsx

+  React.useEffect(() => {
+    const data = variant === 'object' ? objectAccessData : bucketAccessData;
+    if (data) {
+      const { acl } = data;
+      // Don't show "public-read-write" for Objects here; use "custom" instead
+      // since "public-read-write" Objects are basically the same as "public-read".
+      const _acl =
+        variant === 'object' && acl === 'public-read-write' ? 'custom' : acl;
+      const cors_enabled = isUpdateObjectStorageBucketAccessPayload(data)
+        ? data.cors_enabled ?? undefined
+        : true;
+      reset({ acl: _acl || undefined, cors_enabled });
+    }
+  }, [bucketAccessData, objectAccessData, variant, reset]);


No idea what happened to my previous comments, so I'll just restate things.

We can remove this useEffect and use the internals to calculate the defaultValues based on the latest API data.

const data = variant === 'object' ? objectAccessData : bucketAccessData; const defaultValues = React.useMemo(() => { if (data) { const { acl } = data; const _acl = variant === 'object' && acl === 'public-read-write' ? 'custom' : acl; const cors_enabled = isUpdateObjectStorageBucketAccessPayload(data) ? data.cors_enabled ?? true : true; return { acl: _acl as ACLType, cors_enabled }; } return { acl: 'private' as ACLType, cors_enabled: true }; }, [data, variant]);

I think removing useEffect might not solve the issue in this case since the useForm hook reads the defaultValues only on the initial render. Updating defaultValues to the values returned from the api wouldn't update the form state. We will have to use the reset() to update the form and I feel we will have to run reset() inside a useEffect.

Looks like passing the formValues to the values parameter of useForm() removes the necessity of useEffect(). I have pushed these changes in the latest commit(9a2430f)

mjac0bs

Coming late to this PR, but wanted to prevent it from getting stale. I didn't notice any major regressions in the Access Select and test are passing. I did notice a minor regression in loading state (see video clips).

Also re-requesting Jaalah since this PR is blocked by requested changes that, I think, have all been addressed now.

mjac0bs · 2024-10-14T23:02:23Z

packages/manager/src/features/ObjectStorage/BucketDetail/AccessSelect.tsx

+              }
+            }}
+            placeholder={
+              bucketAccessIsFetching || objectAccessIsFetching


This doesn't seem to be working as expected. We have a small regression in the loading state but I wasn't seeing why right off. We have the loading prop and the fetching state of the two queries. Autocomplete has the loadingText prop, but I don't think that makes a difference.

Prod This Branch

autocompleteLoadingState.mov

autocompleteNoLoadingState.mov

Thanks for pointing this out! The issue stems from setting default values for the form. One possible workaround is to add an option to the Options array with the placeholder text 'Loading access..', though I am uncertain if this would be the most optimal solution for maintaining clean and efficient code.

refactor: [M3-8501] - AccessSelect Optimization: Use React Hook Form

2bf0c78

harsh-akamai added the Work in Progress label Sep 17, 2024

harsh-akamai requested a review from jaalah-akamai September 17, 2024 12:29

harsh-akamai self-assigned this Sep 17, 2024

harsh-akamai requested a review from a team as a code owner September 17, 2024 12:29

harsh-akamai requested review from mjac0bs and abailly-akamai and removed request for a team September 17, 2024 12:29

harsh-akamai added 2 commits September 17, 2024 18:01

Added changeset: Optimize AccessSelect component: Use React Hook Form…

60955c2

… & React Query

Merge branch 'develop' into feature/M3-8501-access-select-optimizations

a80ff96

harsh-akamai added the Help Wanted Teamwork makes the dream work! label Sep 17, 2024

refactor: [M3-8501] - Updated tests

3f1c8ca

harsh-akamai added Ready for Review and removed Work in Progress labels Sep 27, 2024

abailly-akamai requested changes Sep 27, 2024

View reviewed changes

packages/manager/src/features/ObjectStorage/BucketDetail/AccessSelect.tsx Outdated Show resolved Hide resolved

harsh-akamai added 2 commits September 30, 2024 16:07

change: [M3-8501] - Tiny bug fixes

cd6919b

Merge branch 'develop' into feature/M3-8501-access-select-optimizations

feb78c3

harsh-akamai removed the Help Wanted Teamwork makes the dream work! label Sep 30, 2024

abailly-akamai approved these changes Oct 1, 2024

View reviewed changes

Merge branch 'develop' into feature/M3-8501-access-select-optimizations

6d41cff

jaalah-akamai requested changes Oct 2, 2024

View reviewed changes

jaalah-akamai reviewed Oct 2, 2024

View reviewed changes

harsh-akamai added 2 commits October 3, 2024 15:58

change: [M3-8501] - Styling changes

ef02189

refactor: [M3-8501] - Replacing useEffect with useMemo

9a2430f

harsh-akamai added Work in Progress and removed Ready for Review Work in Progress labels Oct 8, 2024

harsh-akamai added Ready for Review Add'tl Approval Needed Waiting on another approval! labels Oct 8, 2024

mjac0bs approved these changes Oct 14, 2024

View reviewed changes

mjac0bs requested a review from jaalah-akamai October 14, 2024 23:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: [M3-8501] - AccessSelect Optimization: Use React Hook Form #10952

refactor: [M3-8501] - AccessSelect Optimization: Use React Hook Form #10952

harsh-akamai commented Sep 17, 2024

abailly-akamai commented Sep 18, 2024

harsh-akamai commented Sep 20, 2024

github-actions bot commented Sep 20, 2024 •

edited

Loading

abailly-akamai left a comment

abailly-akamai left a comment

jaalah-akamai left a comment

jaalah-akamai Oct 2, 2024

jaalah-akamai Oct 2, 2024

jaalah-akamai Oct 2, 2024

jaalah-akamai Oct 2, 2024

jaalah-akamai Oct 2, 2024

jaalah-akamai Oct 2, 2024

harsh-akamai Oct 3, 2024

harsh-akamai Oct 8, 2024

mjac0bs left a comment

mjac0bs Oct 14, 2024

harsh-akamai Oct 15, 2024

	disabled={bucketAccessIsFetching \|\| objectAccessIsFetching}
	disabled={bucketAccessIsFetching \|\| objectAccessIsFetching}
	onChange={(e) => setValue('cors_enabled', e.target.checked)}

refactor: [M3-8501] - AccessSelect Optimization: Use React Hook Form #10952

Are you sure you want to change the base?

refactor: [M3-8501] - AccessSelect Optimization: Use React Hook Form #10952

Conversation

harsh-akamai commented Sep 17, 2024

Description 📝

Changes 🔄

Target release date 🗓️

How to test 🧪

Verification steps

As an Author I have considered 🤔

abailly-akamai commented Sep 18, 2024

harsh-akamai commented Sep 20, 2024

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

abailly-akamai left a comment

Choose a reason for hiding this comment

abailly-akamai left a comment

Choose a reason for hiding this comment

jaalah-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

mjac0bs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Sep 20, 2024 •

edited

Loading