HMS-2861: Use RTK for state management #1477

lucasgarfield · 2023-12-04T18:59:32Z

This is currently a WIP, there are a few bugs and some clean-up/refactoring is necessary

So far we have considered (thanks to the work of @lavocatt) two possible options for managing the wizard state: storing state in the highest level component via useState() hooks, and storing state via a useContext() hook.

However, there is a 3rd option - we can use Redux (with RTK).

One big advantage is we can take advantage of the Redux dev tools and use them to inspect the state of the wizard during development:

codecov · 2023-12-04T19:08:37Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (b3fade4) 77.69% compared to head (338c3b3) 77.52%.
Report is 4 commits behind head on main.

❗ Current head 338c3b3 differs from pull request most recent head 0224a31. Consider uploading reports for the commit 0224a31 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1477      +/-   ##
==========================================
- Coverage   77.69%   77.52%   -0.17%     
==========================================
  Files          69       69              
  Lines        2035     2016      -19     
  Branches      560      557       -3     
==========================================
- Hits         1581     1563      -18     
+ Misses        413      412       -1     
  Partials       41       41

see 5 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b3fade4...0224a31. Read the comment docs.

lavocatt · 2023-12-06T10:04:33Z

Thanks for working on this other way of doing things. I'm still reviewing you PR and will hopefully soon be able to write you my thoughts about it.

lavocatt · 2023-12-06T14:52:17Z

I'm in the middle of the review now and I've noticed a bug. The wizard keeps it's state across multiple spawns. To reproduce the issue, open the wizard, fill up a couple of fields, click on cancel and open it up again. The fields that have been selected in the previous wizard session are still active. The expected behavior is that on every new click on create image the wizard starts with its fields set to the default values.

lavocatt

Thanks @lucasgarfield this is a very interesting approach and I'm glad you did the experimentation. The Redux toolkit is quite interesting, and the fact that we can explore the global state at any point in time adds value to this contribution.

In fairness to the other two approaches (the one with the Context and the one with the states lifted up to the wizard's root) let's not forget that the React dev toolkit already allows us to explore the current wizard state:

For the context:

For the prop lifting:

One advantage as you mentioned on slack for the Redux toolkit is that it allows you to get back in time, one inconvenient is that you always have to scroll a lot to get to the latest version of the global state and it gets mixed up with the states of the RTK queries.

There still work for this PR to be a drop in replacement but you're getting there 🚀

lavocatt · 2023-12-06T08:16:16Z

src/Components/CreateImageWizardV2/steps/ImageOutput/ImageOutput.tsx

      {release.match('centos-*') && <CentOSAcknowledgement />}
-      {isFetching && (


Where is the fetching spinner and error handling moved to? I can't find them

lavocatt · 2023-12-06T15:26:47Z

src/store/wizardSlice.ts

+  name: 'wizard',
+  initialState,
+  reducers: {
+    changeRelease: (state, action) => {


For each update a new function must be written right ? Can you try to come up with a generic one? Like we could have an extra information in the payload about which states gets to be updated and then to find it in the state on change time. That would definitely be a better sell to me.

These functions are just setters right?

They're reducers. They take a state and an action, and they return a state. In normal Redux state updates should be done immutably, but RTK uses the Immer library which allows us to write mutable state updates.

https://redux.js.org/tutorials/fundamentals/part-3-state-actions-reducers#writing-reducers

Thanks for the link to the doc. I meant that these reducers are equivalent to setters, each changeXyz function affects the action.payload value to a place in the state.

lavocatt · 2023-12-06T15:28:03Z

src/store/wizardSlice.ts

+    changeEnvironment: (state, action) => {
+      state.targetStep.targetEnvironment = action.payload;
+    },
+    initializeWizard: () => initialState,


Initialize could take a payload too. That way we could pass in a previous image request to initialize the state with.

lavocatt · 2023-12-06T15:39:30Z

src/store/wizardSlice.ts

+import { RHEL_9, X86_64 } from '../constants';
+
+type wizardState = {
+  targetStep: {


I'm understanding that your intention is to cluster data based on which step is supposed to update it. I think this kind of isolation from one step to another is essential for maintainability. We never want to have the XStep changing values of the YStep, that would lead to some buggy nightmares down the road.

My main concern here is that there is no actual software protection for that to happen, if in the future someone decides to call changeEnvironment within the ReviewStep they'll have very little friction to do so (just import changeEnvironment and call it later in the component`. This issue is already arising in the former DDFWizard we are refactoring (see https://github.com/RedHatInsights/image-builder-frontend/blob/3e07472826749bc665157178759741ea2c841de9/src/Components/CreateImageWizard/formComponents/ReviewStep.js#L31-L51 in that code chunk the review step is doing some changes to the wizard step, which is less than ideal for the state machine of the wizazrd. )

Outside of the naming convention that gives us a framework to apply the law in the future, can you think of any way of improving this to provide better security in that regard?

lavocatt · 2023-12-06T15:47:30Z

src/store/hooks.ts

+
+import type { RootState, AppDispatch } from './index';
+
+// Use throughout your app instead of plain `useDispatch` and `useSelector`


I don't understand this comment, can you elaborate please?

lavocatt · 2023-12-06T15:48:53Z

src/Components/CreateImageWizardV2/steps/ImageOutput/ReleaseSelect.tsx

  // By default the component doesn't show the Centos releases and only the RHEL
  // ones. The user has the option to click on a button to make them appear.
+  const release = useAppSelector((state) => state.wizard.targetStep.release);
+  const dispatch = useAppDispatch();


should dispatch be called appDispatch to avoid any confusion with the React dispatch?

lavocatt · 2023-12-06T15:52:28Z

src/Components/CreateImageWizardV2/steps/ImageOutput/Environment.tsx

  const prefetchSources = provisioningApi.usePrefetch('getSourceList');
  const { isBeta } = useGetEnvironment();
+  // TODO: Handle isFetching state (use skeletons)
+  // TODO: Handle isError state (very unlikely...)


I'm looking forward to these TODOs to be implemented.

lavocatt · 2023-12-06T16:00:56Z

src/store/wizardSlice.ts

+  };
+};
+
+const initialState: wizardState = {


The bug with the react state always being the same after closing and reopening is likely to come from here. See the initialState is a const, and we're updating that same object everytime we are calling the changeXyz functions. To avoid that issue you can either not have the initialState as a global const, or you could deep copy the object on re-initialization.

lavocatt · 2023-12-06T16:15:55Z

src/store/wizardSlice.ts

+
+type wizardState = {
+  targetStep: {
+    release: 'rhel-93';


Is it possible to correctly type the release and distributions as they were previously?

lavocatt · 2023-12-06T16:15:57Z

src/store/wizardSlice.ts

+  targetStep: {
+    release: 'rhel-93';
+    architecture: 'x86_64';
+    targetEnvironment: string[];


The targetEnvironment type is less safe than the predecessor it intends to replace.

lucasgarfield · 2024-01-05T13:22:07Z

Closing this for a fresh start.

lucasgarfield requested review from lavocatt and regexowl December 4, 2023 18:59

lucasgarfield force-pushed the rtk-wiz branch from d32af54 to 25524bf Compare December 4, 2023 19:02

lucasgarfield force-pushed the rtk-wiz branch from 25524bf to 909cade Compare December 5, 2023 18:17

lavocatt requested changes Dec 6, 2023

View reviewed changes

lucasgarfield added the ✅ ready for review label Dec 8, 2023

lucasgarfield force-pushed the rtk-wiz branch from 909cade to 905baf1 Compare December 13, 2023 18:48

lucasgarfield force-pushed the rtk-wiz branch from 905baf1 to 338c3b3 Compare December 22, 2023 23:59

lucasgarfield added 4 commits January 5, 2024 09:47

prep wizard slice

a340aaa

target environment step

a0b24f0

aws step

5034ea2

tmp

0224a31

lucasgarfield force-pushed the rtk-wiz branch from 338c3b3 to 0224a31 Compare January 5, 2024 13:21

lucasgarfield closed this Jan 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HMS-2861: Use RTK for state management #1477

HMS-2861: Use RTK for state management #1477

lucasgarfield commented Dec 4, 2023

codecov bot commented Dec 4, 2023 •

edited

Loading

lavocatt commented Dec 6, 2023

lavocatt commented Dec 6, 2023

lavocatt left a comment •

edited

Loading

lavocatt Dec 6, 2023

lavocatt Dec 6, 2023

lucasgarfield Dec 6, 2023

lavocatt Dec 7, 2023

lavocatt Dec 6, 2023

lavocatt Dec 6, 2023

lavocatt Dec 6, 2023

lavocatt Dec 6, 2023

lavocatt Dec 6, 2023 •

edited

Loading

lavocatt Dec 6, 2023 •

edited

Loading

lavocatt Dec 6, 2023

lavocatt Dec 6, 2023

lucasgarfield commented Jan 5, 2024

		{release.match('centos-*') && <CentOSAcknowledgement />}
		{isFetching && (


		import type { RootState, AppDispatch } from './index';

		// Use throughout your app instead of plain `useDispatch` and `useSelector`

HMS-2861: Use RTK for state management #1477

HMS-2861: Use RTK for state management #1477

Conversation

lucasgarfield commented Dec 4, 2023

codecov bot commented Dec 4, 2023 • edited Loading

Codecov Report

lavocatt commented Dec 6, 2023

lavocatt commented Dec 6, 2023

lavocatt left a comment • edited Loading

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

lavocatt Dec 6, 2023 • edited Loading

Choose a reason for hiding this comment

lavocatt Dec 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucasgarfield commented Jan 5, 2024

codecov bot commented Dec 4, 2023 •

edited

Loading

lavocatt left a comment •

edited

Loading

lavocatt Dec 6, 2023 •

edited

Loading

lavocatt Dec 6, 2023 •

edited

Loading