Add form navigation guard to prevent accidental form content loss

[charliepark]

We currently use the useBlocker hook (specifically, FullPageForm) to prevent the user from accidentally navigating away from an in-the-works form. We don't have the same protection on a sidebar modal form. This PR creates a "guard" that we can load in to each form type, allowing us to add the useBlocker hook just by adding the <FormNavGuard> component.

Fixes #2186

[vercel]

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

Name	Status	Preview	Updated (UTC)
console	✅ Ready (Inspect)	Visit Preview	Jul 16, 2024 9:21pm

[david-crespo]

This doesn't need to be generic since we're not actually using the form values. I think you can do form: UseFormReturn with no param and it'll work.

[charliepark]

Both of the callsites for this have form as a UseFormReturn<TFieldValues>. I think it'd be easier to just pass it in as-is and set the type within the new component to also have the generic? (TS complains at the callsites if I strip it from the new component's type) Or is there an aspect to generics that I'm missing?

[david-crespo]

Yep, I was wrong. I thought that leaving it off like this

-export function FormNavGuard<TFieldValues extends FieldValues>({
-  form,
-}: {
-  form: UseFormReturn<TFieldValues>
-}) {
+export function FormNavGuard({ form }: { form: UseFormReturn }) {
   const { isDirty, isSubmitting, isSubmitSuccessful } = form.formState
   // Confirms with the user if they want to navigate away if the form is
   // dirty. Does not intercept everything e.g. refreshes or closing the tab

would be more like UseFormReturn<any> (which does work btw because it doesn't care about types anymore after), but it's actually UseFormReturn<FieldValues> (the default value for that generic param) and surprisingly, that is incompatible with our TFieldValues extends FieldValues. I think you're right, the best thing is to leave it as-is.

[charliepark]

One or two test issues I'll have to work out as well

[charliepark]

Test seems flaky; running playwright locally in both chrome and firefox seems to be fine; merged in main and trying again.

[david-crespo]

That just flaked on main, too: https://github.com/oxidecomputer/console/actions/runs/9876960315/job/27277408168

[charliepark]

This is in a pretty good spot, though when there isn't a route change (for example, the Create New Disk form in the Instance Create form … in fact, that might be the only one), we don't get the benefit of useBlocker preventing the dismissal of the side modal. One possible approach would be to use the onDismiss function for the SideModalForm, and to check if the form is dirty before firing the onDismiss. When doing that, if it is dirty, open the confirmation modal. I'll need to revisit this, though, as I'm a bit foggy.

[charliepark]

If we do decide to add a guard for the side modal form, #2328 is a much simpler implementation, so I don't think this particular branch / implementation makes sense to consider further. (Can always re-open if we decide to.)