From 295960f15ed4c6d5dfd3d22176d9cce22616a0ad Mon Sep 17 00:00:00 2001 From: David Crespo Date: Wed, 24 Jan 2024 16:06:31 -0600 Subject: [PATCH 1/7] notes on async form submit and nav block --- app/components/form/FullPageForm.tsx | 17 ++++++++++------- app/forms/instance-create.tsx | 4 +++- libs/api-mocks/msw/handlers.ts | 5 ++++- 3 files changed, 17 insertions(+), 9 deletions(-) diff --git a/app/components/form/FullPageForm.tsx b/app/components/form/FullPageForm.tsx index e673af7a5..85ed3da8c 100644 --- a/app/components/form/FullPageForm.tsx +++ b/app/components/form/FullPageForm.tsx @@ -53,7 +53,7 @@ export function FullPageForm({ onSubmit, submitError, }: FullPageFormProps) { - const { isSubmitting, isDirty } = form.formState + const { isSubmitting, isDirty, isSubmitSuccessful } = form.formState /* Confirms with the user if they want to navigate away @@ -61,7 +61,11 @@ export function FullPageForm({ refreshes or closing the tab but serves to reduce the possibility of a user accidentally losing their progress */ - const blocker = useBlocker(isDirty) + // I think this would work if the nav didn't happen until after handleSubmit completes. + // as it stands it's in a race because both the end of handleSubmit and the onSuccess + // of the instance create mutation are kicked off by the completion of the create request + const blocker = useBlocker(isDirty && !isSubmitSuccessful) + console.log({ isSubmitting, isDirty, isSubmitSuccessful, blockerState: blocker.state }) // Reset blocker if form is no longer dirty useEffect(() => { @@ -81,17 +85,16 @@ export function FullPageForm({
{ + onSubmit={async (e) => { // This modal being in a portal doesn't prevent the submit event // from bubbling up out of the portal. Normally that's not a // problem, but sometimes (e.g., instance create) we render the // SideModalForm from inside another form, in which case submitting // the inner form submits the outer form unless we stop propagation e.stopPropagation() - // This resets `isDirty` whilst keeping the values meaning - // we are not prevented from navigating away by the blocker - form.reset({} as TFieldValues, { keepValues: true }) - form.handleSubmit(onSubmit)(e) + console.log('a') + await form.handleSubmit(onSubmit)(e) + console.log('b') }} autoComplete="off" > diff --git a/app/forms/instance-create.tsx b/app/forms/instance-create.tsx index cec3644ed..76e40cdd6 100644 --- a/app/forms/instance-create.tsx +++ b/app/forms/instance-create.tsx @@ -126,6 +126,8 @@ export function CreateInstanceForm() { instance ) addToast({ content: 'Your instance has been created' }) + // this navigate does not wait for the form to switch to isSubmitSuccessful, + // so it can get blocked navigate(pb.instancePage({ ...projectSelector, instance: instance.name })) }, }) @@ -188,7 +190,7 @@ export function CreateInstanceForm() { ? await readBlobAsBase64(values.userData) : undefined - createInstance.mutate({ + await createInstance.mutateAsync({ query: projectSelector, body: { name: values.name, diff --git a/libs/api-mocks/msw/handlers.ts b/libs/api-mocks/msw/handlers.ts index 7736f4013..8193601c1 100644 --- a/libs/api-mocks/msw/handlers.ts +++ b/libs/api-mocks/msw/handlers.ts @@ -39,6 +39,8 @@ import { unavailableErr, } from './util' +export const sleep = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms)) + // Note the *JSON types. Those represent actual API request and response bodies, // the snake-cased objects coming straight from the API before the generated // client camel-cases the keys and parses date fields. Inside the mock API everything @@ -289,7 +291,7 @@ export const handlers = makeHandlers({ const instances = db.instances.filter((i) => i.project_id === project.id) return paginated(query, instances) }, - instanceCreate({ body, query }) { + async instanceCreate({ body, query }) { const project = lookup.project(query) errIfExists(db.instances, { name: body.name, project_id: project.id }, 'instance') @@ -409,6 +411,7 @@ export const handlers = makeHandlers({ time_run_state_updated: new Date().toISOString(), } db.instances.push(newInstance) + await sleep(2000) return json(newInstance, { status: 201 }) }, instanceView: ({ path, query }) => lookup.instance({ ...path, ...query }), From 215a83ad48e6290f18ddb5d6df65862aed47d3e1 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Thu, 1 Feb 2024 14:15:55 -0600 Subject: [PATCH 2/7] version that seems to work --- app/components/form/FullPageForm.tsx | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/app/components/form/FullPageForm.tsx b/app/components/form/FullPageForm.tsx index 85ed3da8c..a455c9e69 100644 --- a/app/components/form/FullPageForm.tsx +++ b/app/components/form/FullPageForm.tsx @@ -69,10 +69,10 @@ export function FullPageForm({ // Reset blocker if form is no longer dirty useEffect(() => { - if (blocker.state === 'blocked' && !isDirty) { - blocker.reset() + if (blocker.state === 'blocked' && isSubmitSuccessful) { + blocker.proceed() } - }, [blocker, isDirty]) + }, [blocker, isSubmitSuccessful]) const childArray = flattenChildren(children) const actions = pluckFirstOfType(childArray, Form.Actions) @@ -93,6 +93,7 @@ export function FullPageForm({ // the inner form submits the outer form unless we stop propagation e.stopPropagation() console.log('a') + // important to await await form.handleSubmit(onSubmit)(e) console.log('b') }} @@ -101,7 +102,7 @@ export function FullPageForm({ {childArray} - {blocker ? : null} + {!isSubmitSuccessful && } {actions && ( From cf6702f7b99732665d449cf57fa77999e8333d25 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Thu, 1 Feb 2024 14:18:20 -0600 Subject: [PATCH 3/7] upgrade RHF --- package-lock.json | 14 +++++++------- package.json | 2 +- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/package-lock.json b/package-lock.json index 8ffcc5207..31e107b46 100644 --- a/package-lock.json +++ b/package-lock.json @@ -36,7 +36,7 @@ "react-aria": "^3.31.0", "react-dom": "^18.2.0", "react-error-boundary": "^4.0.12", - "react-hook-form": "^7.47.0", + "react-hook-form": "^7.50.0", "react-is": "^18.2.0", "react-merge-refs": "^2.1.1", "react-router-dom": "^6.21.1", @@ -17918,9 +17918,9 @@ } }, "node_modules/react-hook-form": { - "version": "7.47.0", - "resolved": "https://registry.npmjs.org/react-hook-form/-/react-hook-form-7.47.0.tgz", - "integrity": "sha512-F/TroLjTICipmHeFlMrLtNLceO2xr1jU3CyiNla5zdwsGUGu2UOxxR4UyJgLlhMwLW/Wzp4cpJ7CPfgJIeKdSg==", + "version": "7.50.0", + "resolved": "https://registry.npmjs.org/react-hook-form/-/react-hook-form-7.50.0.tgz", + "integrity": "sha512-AOhuzM3RdP09ZCnq+Z0yvKGHK25yiOX5phwxjV9L7U6HMla10ezkBnvQ+Pk4GTuDfsC5P2zza3k8mawFwFLVuQ==", "engines": { "node": ">=12.22.0" }, @@ -34226,9 +34226,9 @@ } }, "react-hook-form": { - "version": "7.47.0", - "resolved": "https://registry.npmjs.org/react-hook-form/-/react-hook-form-7.47.0.tgz", - "integrity": "sha512-F/TroLjTICipmHeFlMrLtNLceO2xr1jU3CyiNla5zdwsGUGu2UOxxR4UyJgLlhMwLW/Wzp4cpJ7CPfgJIeKdSg==", + "version": "7.50.0", + "resolved": "https://registry.npmjs.org/react-hook-form/-/react-hook-form-7.50.0.tgz", + "integrity": "sha512-AOhuzM3RdP09ZCnq+Z0yvKGHK25yiOX5phwxjV9L7U6HMla10ezkBnvQ+Pk4GTuDfsC5P2zza3k8mawFwFLVuQ==", "requires": {} }, "react-hotkeys-hook": { diff --git a/package.json b/package.json index 61d1b4635..c30734b43 100644 --- a/package.json +++ b/package.json @@ -57,7 +57,7 @@ "react-aria": "^3.31.0", "react-dom": "^18.2.0", "react-error-boundary": "^4.0.12", - "react-hook-form": "^7.47.0", + "react-hook-form": "^7.50.0", "react-is": "^18.2.0", "react-merge-refs": "^2.1.1", "react-router-dom": "^6.21.1", From 1437ecd5adfebb38d8d51a8520dd74ac9a82344c Mon Sep 17 00:00:00 2001 From: David Crespo Date: Thu, 1 Feb 2024 14:22:36 -0600 Subject: [PATCH 4/7] cleanup --- app/components/form/FullPageForm.tsx | 30 +++++++++++++++------------- app/forms/instance-create.tsx | 2 -- 2 files changed, 16 insertions(+), 16 deletions(-) diff --git a/app/components/form/FullPageForm.tsx b/app/components/form/FullPageForm.tsx index a455c9e69..c229f21a0 100644 --- a/app/components/form/FullPageForm.tsx +++ b/app/components/form/FullPageForm.tsx @@ -55,19 +55,18 @@ export function FullPageForm({ }: FullPageFormProps) { const { isSubmitting, isDirty, 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 but serves to reduce - the possibility of a user accidentally losing their progress - */ - // I think this would work if the nav didn't happen until after handleSubmit completes. - // as it stands it's in a race because both the end of handleSubmit and the onSuccess - // of the instance create mutation are kicked off by the completion of the create request + // 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 + // but serves to reduce the possibility of a user accidentally losing their + // progress. const blocker = useBlocker(isDirty && !isSubmitSuccessful) - console.log({ isSubmitting, isDirty, isSubmitSuccessful, blockerState: blocker.state }) - // Reset blocker if form is no longer dirty + // Gating on !isSubmitSuccessful above makes the blocker stop blocking nav + // after a successful submit. However, this can take a little time (there is a + // render in between when isSubmitSuccessful is true but the blocker is still + // ready to block), so we also have this useEffect that lets blocked requests + // through if submit is succesful but the blocker hasn't gotten a chance to + // stop blocking yet. useEffect(() => { if (blocker.state === 'blocked' && isSubmitSuccessful) { blocker.proceed() @@ -92,16 +91,19 @@ export function FullPageForm({ // SideModalForm from inside another form, in which case submitting // the inner form submits the outer form unless we stop propagation e.stopPropagation() - console.log('a') - // important to await + // Important to await here so isSubmitSuccessful doesn't become true + // until the submit is actually successful. Note you must use await + // mutateAsync() inside onSubmit in order to make this wait await form.handleSubmit(onSubmit)(e) - console.log('b') }} autoComplete="off" > {childArray} + {/* rendering of the modal must be gated on isSubmitSuccessful because + there is a brief moment where isSubmitSuccessful is true but the proceed() + hasn't fired yet, which means we get a brief flash of this modal */} {!isSubmitSuccessful && } {actions && ( diff --git a/app/forms/instance-create.tsx b/app/forms/instance-create.tsx index 76e40cdd6..e3d5b4bec 100644 --- a/app/forms/instance-create.tsx +++ b/app/forms/instance-create.tsx @@ -126,8 +126,6 @@ export function CreateInstanceForm() { instance ) addToast({ content: 'Your instance has been created' }) - // this navigate does not wait for the form to switch to isSubmitSuccessful, - // so it can get blocked navigate(pb.instancePage({ ...projectSelector, instance: instance.name })) }, }) From 22cfd7b5a11ad7caddc53c10610491387f1f01f5 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Thu, 1 Feb 2024 14:50:51 -0600 Subject: [PATCH 5/7] delete sleep in the mock handler --- libs/api-mocks/msw/handlers.ts | 3 --- 1 file changed, 3 deletions(-) diff --git a/libs/api-mocks/msw/handlers.ts b/libs/api-mocks/msw/handlers.ts index 8193601c1..669ae550f 100644 --- a/libs/api-mocks/msw/handlers.ts +++ b/libs/api-mocks/msw/handlers.ts @@ -39,8 +39,6 @@ import { unavailableErr, } from './util' -export const sleep = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms)) - // Note the *JSON types. Those represent actual API request and response bodies, // the snake-cased objects coming straight from the API before the generated // client camel-cases the keys and parses date fields. Inside the mock API everything @@ -411,7 +409,6 @@ export const handlers = makeHandlers({ time_run_state_updated: new Date().toISOString(), } db.instances.push(newInstance) - await sleep(2000) return json(newInstance, { status: 201 }) }, instanceView: ({ path, query }) => lookup.instance({ ...path, ...query }), From 974b34e6e86cc92bafc87df63b611afd92c3f5c1 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Wed, 7 Feb 2024 14:20:46 -0600 Subject: [PATCH 6/7] bump RHF again --- package-lock.json | 14 +++++++------- package.json | 2 +- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/package-lock.json b/package-lock.json index 31e107b46..0d34467d3 100644 --- a/package-lock.json +++ b/package-lock.json @@ -36,7 +36,7 @@ "react-aria": "^3.31.0", "react-dom": "^18.2.0", "react-error-boundary": "^4.0.12", - "react-hook-form": "^7.50.0", + "react-hook-form": "^7.50.1", "react-is": "^18.2.0", "react-merge-refs": "^2.1.1", "react-router-dom": "^6.21.1", @@ -17918,9 +17918,9 @@ } }, "node_modules/react-hook-form": { - "version": "7.50.0", - "resolved": "https://registry.npmjs.org/react-hook-form/-/react-hook-form-7.50.0.tgz", - "integrity": "sha512-AOhuzM3RdP09ZCnq+Z0yvKGHK25yiOX5phwxjV9L7U6HMla10ezkBnvQ+Pk4GTuDfsC5P2zza3k8mawFwFLVuQ==", + "version": "7.50.1", + "resolved": "https://registry.npmjs.org/react-hook-form/-/react-hook-form-7.50.1.tgz", + "integrity": "sha512-3PCY82oE0WgeOgUtIr3nYNNtNvqtJ7BZjsbxh6TnYNbXButaD5WpjOmTjdxZfheuHKR68qfeFnEDVYoSSFPMTQ==", "engines": { "node": ">=12.22.0" }, @@ -34226,9 +34226,9 @@ } }, "react-hook-form": { - "version": "7.50.0", - "resolved": "https://registry.npmjs.org/react-hook-form/-/react-hook-form-7.50.0.tgz", - "integrity": "sha512-AOhuzM3RdP09ZCnq+Z0yvKGHK25yiOX5phwxjV9L7U6HMla10ezkBnvQ+Pk4GTuDfsC5P2zza3k8mawFwFLVuQ==", + "version": "7.50.1", + "resolved": "https://registry.npmjs.org/react-hook-form/-/react-hook-form-7.50.1.tgz", + "integrity": "sha512-3PCY82oE0WgeOgUtIr3nYNNtNvqtJ7BZjsbxh6TnYNbXButaD5WpjOmTjdxZfheuHKR68qfeFnEDVYoSSFPMTQ==", "requires": {} }, "react-hotkeys-hook": { diff --git a/package.json b/package.json index c30734b43..5bd31ed9c 100644 --- a/package.json +++ b/package.json @@ -57,7 +57,7 @@ "react-aria": "^3.31.0", "react-dom": "^18.2.0", "react-error-boundary": "^4.0.12", - "react-hook-form": "^7.50.0", + "react-hook-form": "^7.50.1", "react-is": "^18.2.0", "react-merge-refs": "^2.1.1", "react-router-dom": "^6.21.1", From 3256c2a4fce4037e212b4ff3ce4f70b061365a7f Mon Sep 17 00:00:00 2001 From: David Crespo Date: Wed, 7 Feb 2024 14:24:59 -0600 Subject: [PATCH 7/7] lil tweak --- app/components/form/FullPageForm.tsx | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/app/components/form/FullPageForm.tsx b/app/components/form/FullPageForm.tsx index c229f21a0..2c388b92f 100644 --- a/app/components/form/FullPageForm.tsx +++ b/app/components/form/FullPageForm.tsx @@ -25,7 +25,11 @@ interface FullPageFormProps { error?: Error form: UseFormReturn loading?: boolean - onSubmit: (values: TFieldValues) => void + /** + * Use await mutateAsync(), otherwise you'll break the logic below that relies + * on knowing when the submit is done. + */ + onSubmit: (values: TFieldValues) => Promise /** Error from the API call */ submitError: ApiError | null /**