feat(auto-save): Auto-save on custom / page type & slice builder #1258

xrutayisire · 2023-12-28T20:09:21Z

Context

Completes DT-1872, DT-1832, DT-1831, DT-1833

The Solution

Auto-save for page / custom type:

Remove selectedCustomTypeReducer reducer
Remove watchSelectedCustomTypeSagas saga
Remove old redux sectors
Remove old redux actions
Make the custom type builder working without redux with it's own data locally
Support Enter key to submit a new field
Ensure that when renaming a tab, the existing name is already filled
Create a local context for custom type builder
Ensure you cannot add a field if the form is already displayed
Persist save to local custom type builder context
Create a custom hook useAutoSave to ensure we have the proper way to handle multiple changes
Create a component to display saving status
Create domain functions to manage custom type model
Add unit tests for custom type domain functions
Add e2e tests for custom / page type builder
Add option to delay response for e2e mocking procedures management

TO DO

Discussions with Benjamin on the design of toaster on error
Discussions with Benjamin for the delay on error retry

Impact / Dependencies

The local custom type builder will not be updated automatically on window focus or anything else

Checklist before requesting a review

I hereby declare my code ready for review.
If it is a critical feature, I have added tests.
The CI is successful.
If there could backward compatibility issues, it has been discussed and planned.

Preview

Screen.Recording.2023-12-29.at.18.53.35.mov

linear · 2023-12-28T20:09:23Z

DT-1872 Auto-save on custom / page type builder

DT-1832 [e2e][tests] Page type builder fields tests

DT-1831 [e2e][tests] Page type builder common tests

DT-1833 [e2e][tests] Page type builder slice zone tests

vercel · 2023-12-28T20:09:26Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
slice-machine	✅ Ready (Inspect)	Visit Preview	Jan 18, 2024 2:49pm

bapmrl

Nice useAutoSave hook 😊 Just have some questions and comments as I don't understand all the intricacies of it.

bapmrl · 2024-01-16T09:04:49Z

packages/slice-machine/src/features/autoSave/useAutoSave.tsx

+  nextSave: (() => Promise<unknown>) | null;
+};
+
+export type AutoSaveStatusType = "saving" | "saved" | "error";


⚠️ In my opinion, there's no need to append Type to a type.

💡 Maybe failed would be better than error? That way, you'd be using past tense like saved.

I did that because I have a visual component named AutoSaveStatus that also refer to the type. So I wanted to be clear about the difference. WDYT?

Good idea for failed, I will rename it 🙂

Okay, what do you think about naming the component AutoSaveStatusIndicator then? 😊

That's a good idea, yes!

bapmrl · 2024-01-16T09:10:52Z

packages/slice-machine/src/features/autoSave/useAutoSave.tsx

💡 I wonder if a new core/autoSave folder would be a better fit. Indeed, I feel like we're putting too many things into features. For me, features should ideally be "optional" parts/pages/modules and core should contain the shared domain bits that allow the features to work.

Oh, I see. I really like it, we kind of discussed a bit that at the beginning I remember. But maybe it will also create a problem with too many first level folder in src with a dev never knowing where to put stuff?
For me, I "feel" it's ok, but I wanted to warn about that.

If we create a core folder, we could put:

autoSave

documentation

environments

inAppGuide

legacyState

slicesTemplates

And so in features we leave:

changes

cusotmTypes

labs

slices

WDYT?

Btw I guess since it's already a +3000/-3000 PR I can do that right after my PRs are merged and only do the autoSave now. So I ease the review for Hugo. Ok for you?

Okay for me, but I'm not sure about the partitioning you propose. We'll discuss it again ^^

Also, what would you think of moving all the "old" code into a separate top-level old directory? 😜

What old code you refer to? I'm not a fan of this, then you start to have again more confusion and folders. The old folders and old code. I feel It's just more pain to maintain.

Following our discussion, will remove 2 folders after my PRs

bapmrl · 2024-01-16T09:11:39Z

packages/slice-machine/src/features/autoSave/useAutoSave.tsx

+};
+
+type AutoSaveState = {
+  pendingSave: (() => Promise<unknown>) | null;


⚠️ Let's favor undefined over null for consistency.

Yep my bad, I prefer also to use undefined, was too fast here. Good catch, thanks 🙂
I will also add it to the best practices notion page.

bapmrl · 2024-01-16T09:15:24Z

packages/slice-machine/src/features/autoSave/useAutoSave.tsx

+    errorMessage = "An error happened while saving",
+    retryDelay = 2000,
+    retryMessage = "Retry",
+  } = args;


💡 A line break here would improve readability.

bapmrl · 2024-01-16T09:17:24Z

packages/slice-machine/src/features/autoSave/useAutoSave.tsx

+
+export type AutoSaveStatusType = "saving" | "saved" | "error";
+
+export const useAutoSave = (args: UseAutoSaveArgs = {}) => {


⚠️ I think it's important to always define the return type of top-level functions in order to help developers quickly understand what the function does.

Yep good catch, I will do it, and also it's already in the best practices 🎉

packages/slice-machine/src/features/autoSave/useAutoSave.tsx

bapmrl · 2024-01-16T09:51:31Z

packages/slice-machine/src/features/autoSave/useAutoSave.tsx

+        await autoSaveState.pendingSave();
+
+        setAutoSaveStatusInternal("saved");
+        setAutoSaveState((prevState) => ({


❓ Is this necessary as it's already done in the useEffect below?

Hum good thinking. I will try to see if it's possible and test it. It may simplify a bit the useEffect bellow even maybe.

But I'm not sure. I will try and come back to you 🙂

Yes!

So I fixed the bug of the retry, and I really took time to check that one.

When a save fail, we want to trigger a retry with the latest data we have (saw with Benjamin), meaning we don't want to just reply the pending. We may want to retry with the next, but if there is no next, meaning the user did not do any action in the meantime, we will retry with the pending.
Because of this it's important in case of a success to remove the pending, if not the useEffect will detect that there is a pending operation and will try to do it again, creating an endless loop.

I added comments for the tricky parts, btw.

packages/slice-machine/src/features/autoSave/useAutoSave.tsx

bapmrl · 2024-01-16T09:54:46Z

packages/slice-machine/src/features/autoSave/useAutoSave.tsx

+                toast.dismiss(toastId);
+                setAutoSaveStatusInternal("saving");
+                setTimeout(() => {
+                  void executeSave();


❓ I'm not sure I understand all the execution branches that could happen. For instance, what happens if:

There's an error.

The user doesn't click on the retry button, but performs another action in the UI.

The user clicks on the retry button.

Haha, that's something I totally tested (NO), so it's adding a double error if you really have a problem or it's saving the second thing you're doing BUT keep the retry visible.

I need to rework this! THANKS!

Fixed, I don't trigger the next save if they is an error. The retry will do it only. But any changes done after a fail will still but stacked.

bapmrl · 2024-01-16T10:00:32Z

packages/slice-machine/src/features/autoSave/useAutoSave.tsx

+      autoSaveStatusInternal !== "saving" &&
+      (autoSaveState.pendingSave || autoSaveState.nextSave)
+    ) {
+      setAutoSaveState((prevState) => ({


❓ I'm not sure I understand well this side effect. It seems it will run if autoSaveStatusInternal is error and the pendingSave corresponding to that error would be replaced by nextSave. Is it intended? Should the toast be dismissed as executeSave will automatically be called?

Ok, this one is the one that actually trigger every next save.

Meaning, when we want to perform a next save we check, is there any pendingSave or nextSave WHILE the status is not "saving" and so doing something. If yes, let's run the nextSave or just remove the pending one.

About the error, yes I think I need to dismiss somewhere to prevent the error above

useEffect updated with the fix on the error and it should be clearer now. With a comment explaining the code.

bapmrl

Again, excellent work Xavier 😊

bapmrl · 2024-01-16T12:49:57Z

packages/slice-machine/src/features/customTypes/customTypesBuilder/CustomTypeProvider.tsx

+
+type CustomTypeContext = {
+  customType: CustomType;
+  autoSaveStatus: AutoSaveStatusType;


❓ It means for now we have one autosave state for custom types. Is it going to be a different state for slices? How are we going to combine both autosave states to display the error toasts and status messages consistently/once?

For me, I was planning to have 2 providers. As the provider, don't have the save logic since it's the hook and only contain things purely related to custom types or slices.

There are some duplications, but I would say healthy one that help us maintain the project without risking crashing everything.

You can see what I'm planning to do here: https://github.com/prismicio/slice-machine/blob/xru/auto-save-slice-builder/packages/slice-machine/src/features/slices/sliceBuilder/SliceBuilderProvider.tsx
And you can see that there are difference related to slices.

So for me 2 providers that reuse the hook yes.

Okay, I just wonder how the 2 autoSaveStatus will be combined to provide a clear visual indicator to the users.

Hum, they will never be combined, since it's display from within the builder you never have both in the same view.

bapmrl · 2024-01-16T12:51:48Z

packages/slice-machine/src/features/customTypes/customTypesBuilder/CustomTypeProvider.tsx

+
+type CustomTypeProviderProps = {
+  children: ReactNode | ((value: CustomTypeContext) => ReactNode);
+  defaultCustomType: CustomType;


💡 I think initialCustomType would be more in line with the parameter name of the useState hook.

Yes good point! I think naming is my nemesis! 😈

packages/slice-machine/src/features/customTypes/customTypesBuilder/CustomTypeProvider.tsx

bapmrl · 2024-01-16T12:54:56Z

packages/slice-machine/src/features/customTypes/customTypesBuilder/CustomTypeProvider.tsx

+
+const CustomTypeContextValue = createContext<CustomTypeContext>({
+  autoSaveStatus: "saved",
+  customType: {} as CustomType,


⚠️ I think it's a weird optimization. If the consumer of that context didn't use the CustomTypeProvider, he'll get a customType that doesn't conform to the CustomType interface. I think it's both safer and clearer to initialize the context with undefined and throw if the provider cannot be found.

Yeah, I was never a fan of how it's done in React. I would just love to have a provider where I initialize with undefined but every time I'm getting the context typescript knows that it's full. And React handle the error if I didn't setup the Prodiver.

Will do it 👍

bapmrl · 2024-01-16T12:55:58Z

packages/slice-machine/src/features/customTypes/customTypesBuilder/CustomTypeProvider.tsx

+});
+
+export function CustomTypeProvider(props: CustomTypeProviderProps) {
+  const { children, defaultCustomType } = props;


💡 Adding a new line after the props spreading would make the code clearer in my opinion.

I will add it, maybe it's something I will look for now. I tend to group it like that.

bapmrl · 2024-01-16T13:07:36Z

packages/slice-machine/src/features/customTypes/customTypesBuilder/CustomTypeProvider.tsx

+
+export function CustomTypeProvider(props: CustomTypeProviderProps) {
+  const { children, defaultCustomType } = props;
+  const isMounted = useRef(false);


💡 Maybe you could use the useIsFirstRender hook from @prismicio/editor-support?

100%, I didn't even know about it. It's something we (expect you haha) are not doing. More looking for simple stuff in editor packages. I will use it!!

bapmrl · 2024-01-16T13:09:00Z

packages/slice-machine/src/features/customTypes/customTypesBuilder/CustomTypeProvider.tsx

+        isMounted.current = true;
+      }
+    },
+    // Prevent saveCustomTypeSuccess to trigger infinite loop


💡 Maybe "from triggering an infinite loop" would be better?

hypervillain · 2024-01-16T15:06:34Z

packages/slice-machine/src/features/autoSave/useAutoSave.tsx

+        console.error("Error in save effect:", error),
+      );
+    }
+  }, [autoSaveStatusInternal, autoSaveState, executeSave]);


I believe every property of autoSaveState should be listed in here

Hey, it's not necessary here since it's immutable, we are talking about a state so each time the values changed the state itself 100% change. You never modify a state sub value directly.

The linter is pretty good on that and tell us here that it's good.

hypervillain · 2024-01-17T11:19:26Z

packages/slice-machine/src/features/customTypes/customTypesBuilder/CustomTypeProvider.tsx

+export function CustomTypeProvider(props: CustomTypeProviderProps) {
+  const { children, initialCustomType } = props;
+
+  const isFirstRender = useIsFirstRender();


hypervillain · 2024-01-17T11:23:02Z

packages/slice-machine/src/features/customTypes/customTypesBuilder/CustomTypeProvider.tsx

+    },
+    // Prevent saveCustomTypeSuccess from triggering an infinite loop
+    // eslint-disable-next-line react-hooks/exhaustive-deps
+    [customType, setNextSave],


Is it better to remove it from the deps or the wrap it with a useCallback?

I could try to use useStableCallback from the editor support, good idea!

It's working with useStableCallback:

I did it in the second PR with both providers to freeze this one, since the review is done now.

xrutayisire force-pushed the xru/auto-save-type-builder branch from 9e71dfa to 4c92702 Compare December 28, 2023 20:40

vercel bot deployed to Preview December 28, 2023 20:46 View deployment

xrutayisire force-pushed the xru/auto-save-type-builder branch from 4c92702 to b412472 Compare December 28, 2023 21:42

vercel bot deployed to Preview December 28, 2023 21:47 View deployment

xrutayisire force-pushed the xru/auto-save-type-builder branch from b412472 to 9a85c8a Compare December 29, 2023 15:55

vercel bot deployed to Preview December 29, 2023 16:00 View deployment

xrutayisire force-pushed the xru/auto-save-type-builder branch from 9a85c8a to 8aa625c Compare December 29, 2023 18:23

xrutayisire marked this pull request as ready for review December 29, 2023 18:25

vercel bot deployed to Preview December 29, 2023 18:33 View deployment

xrutayisire force-pushed the xru/auto-save-type-builder branch from 8aa625c to 2f17bed Compare December 29, 2023 18:37

xrutayisire marked this pull request as draft December 29, 2023 18:38

vercel bot deployed to Preview December 29, 2023 18:44 View deployment

xrutayisire force-pushed the xru/auto-save-type-builder branch from 2f17bed to eafae8c Compare December 29, 2023 18:56

vercel bot deployed to Preview December 29, 2023 19:01 View deployment

xrutayisire force-pushed the xru/auto-save-type-builder branch from eafae8c to 8b7d010 Compare December 29, 2023 23:29

vercel bot deployed to Preview December 29, 2023 23:35 View deployment

xrutayisire force-pushed the xru/auto-save-type-builder branch from 8b7d010 to 621123a Compare December 29, 2023 23:38

vercel bot deployed to Preview December 29, 2023 23:43 View deployment

xrutayisire force-pushed the xru/auto-save-type-builder branch from 621123a to bc680c1 Compare January 2, 2024 15:12

vercel bot deployed to Preview January 2, 2024 15:17 View deployment

xrutayisire force-pushed the xru/auto-save-type-builder branch from bc680c1 to c130f36 Compare January 2, 2024 18:50

vercel bot deployed to Preview January 2, 2024 18:55 View deployment

xrutayisire force-pushed the xru/auto-save-type-builder branch from c130f36 to cc3118f Compare January 2, 2024 19:20

vercel bot deployed to Preview January 2, 2024 19:26 View deployment

xrutayisire force-pushed the xru/auto-save-type-builder branch from cc3118f to 6449ea8 Compare January 3, 2024 11:50

vercel bot deployed to Preview January 3, 2024 11:59 View deployment

xrutayisire force-pushed the xru/auto-save-type-builder branch from 6449ea8 to 6beab0a Compare January 3, 2024 12:22

vercel bot deployed to Preview January 3, 2024 12:27 View deployment

vercel bot deployed to Preview January 4, 2024 17:22 View deployment

xrutayisire force-pushed the xru/auto-save-type-builder branch from 8606db8 to b9a8a9b Compare January 9, 2024 10:00

vercel bot deployed to Preview January 9, 2024 10:06 View deployment

xrutayisire force-pushed the xru/auto-save-type-builder branch from b9a8a9b to 1bc7048 Compare January 9, 2024 10:25

vercel bot deployed to Preview January 9, 2024 10:30 View deployment

xrutayisire force-pushed the xru/auto-save-type-builder branch from 1bc7048 to 5ff0f6e Compare January 9, 2024 22:39

vercel bot deployed to Preview January 9, 2024 22:44 View deployment

bapmrl requested changes Jan 16, 2024

View reviewed changes

hypervillain reviewed Jan 16, 2024

View reviewed changes

xrutayisire force-pushed the xru/auto-save-type-builder branch from 5ff0f6e to 9da363e Compare January 16, 2024 21:54

vercel bot deployed to Preview January 16, 2024 21:59 View deployment

xrutayisire force-pushed the xru/auto-save-type-builder branch from 9da363e to 23667df Compare January 16, 2024 22:10

vercel bot deployed to Preview January 16, 2024 22:15 View deployment

xrutayisire force-pushed the xru/auto-save-type-builder branch 2 times, most recently from 916f31f to 9d769bd Compare January 17, 2024 08:57

vercel bot deployed to Preview January 17, 2024 09:07 View deployment

xrutayisire force-pushed the xru/auto-save-type-builder branch from 9d769bd to 6694bd6 Compare January 17, 2024 09:29

vercel bot deployed to Preview January 17, 2024 09:34 View deployment

xrutayisire force-pushed the xru/auto-save-type-builder branch from 6694bd6 to 00a2b21 Compare January 17, 2024 10:34

vercel bot deployed to Preview January 17, 2024 10:40 View deployment

hypervillain reviewed Jan 17, 2024

View reviewed changes

xrutayisire added 2 commits January 18, 2024 15:39

feat(auto-save): Auto-save on custom / page type builder

8cba3fd

feat(auto-save): Auto-save on slice builder (#1263)

02dc287

xrutayisire force-pushed the xru/auto-save-type-builder branch from 51a9163 to 02dc287 Compare January 18, 2024 14:40

xrutayisire changed the title ~~feat(auto-save): Auto-save on custom / page type builder~~ feat(auto-save): Auto-save on custom / page type & slice builder Jan 18, 2024

vercel bot deployed to Preview January 18, 2024 14:49 View deployment

xrutayisire merged commit d531c16 into main Jan 18, 2024
35 checks passed

xrutayisire deleted the xru/auto-save-type-builder branch January 18, 2024 14:56


		export type AutoSaveStatusType = "saving" \| "saved" \| "error";

		export const useAutoSave = (args: UseAutoSaveArgs = {}) => {

feat(auto-save): Auto-save on custom / page type & slice builder #1258

feat(auto-save): Auto-save on custom / page type & slice builder #1258

Conversation

xrutayisire commented Dec 28, 2023 • edited Loading

Context

The Solution

TO DO

Impact / Dependencies

Checklist before requesting a review

Preview

linear bot commented Dec 28, 2023

vercel bot commented Dec 28, 2023 • edited Loading

bapmrl 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

bapmrl Jan 16, 2024 • 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bapmrl 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

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

xrutayisire Jan 16, 2024 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xrutayisire commented Dec 28, 2023 •

edited

Loading

vercel bot commented Dec 28, 2023 •

edited

Loading

bapmrl Jan 16, 2024 •

edited

Loading

xrutayisire Jan 16, 2024 •

edited

Loading