diff --git a/.changeset/record-swap-hides-the-previous-record.md b/.changeset/record-swap-hides-the-previous-record.md new file mode 100644 index 000000000..6e3ef0cf0 --- /dev/null +++ b/.changeset/record-swap-hides-the-previous-record.md @@ -0,0 +1,38 @@ +--- +"@object-ui/plugin-form": patch +--- + +fix(plugin-form): swapping `recordId` no longer leaves the previous record on screen + +`loading` in `ModalForm` / `DrawerForm` / `TabbedForm` / `SplitForm` was only ever +set `true` once, by `useState(true)`, and thereafter only ever set `false`. A +`recordId` change therefore re-entered the fetch effect **without** going back +through the loading branch: the form stayed mounted showing — and accepting edits +to — record A's values, with nothing indicating a different record had been asked +for, until B's response landed and replaced them in place. Anything typed in that +window read as A's on screen and would have been submitted against B. + +The same effect had no staleness guard either, so two overlapping reads landed in +**completion** order rather than request order: ask for B then C, and a slow B +arriving last left the form showing B while the caller had asked for C. + +Both are the same defect from the user's side — the form displays a record nobody +asked for — so both are fixed: + +- a change of record re-enters the loading state before the read, so the previous + record is off screen while the next one is in flight. Gated on the record + actually changing: the effect also re-runs on `initialData`/`initialValues` + identity churn (callers rebuild those objects every render), and flashing the + loading state for that would thrash; +- the effect's cleanup marks its read stale, so a response that is no longer the + one being awaited is dropped instead of overwriting a newer record. + +`ObjectForm` already re-entered loading before its fetch, which is why this only +ever reproduced on the four sectioned variants. + +**Also fixed, a consequence of the above:** hiding the form unmounts the inner +renderer, and that renderer is the only thing that reports dirtiness via +`onDirtyChange` — it gets no chance to report `false` on the way out. Without +clearing the flag, the overlay's unsaved-input guards would stay armed for input +belonging to a record no longer on screen: a plain refresh would prompt, and +closing would offer to discard nothing. diff --git a/packages/plugin-form/src/DrawerForm.tsx b/packages/plugin-form/src/DrawerForm.tsx index 3bf7921b9..7f46269be 100644 --- a/packages/plugin-form/src/DrawerForm.tsx +++ b/packages/plugin-form/src/DrawerForm.tsx @@ -216,8 +216,24 @@ export const DrawerForm: React.FC = ({ fetchSchema(); }, [schema.objectName, dataSource]); + // The record whose data `formData` currently holds. The fetch effect reads it + // to tell a genuine record SWAP from a re-run of its own making — + // `initialData`/`initialValues` are objects callers commonly rebuild every + // render, and flashing the loading state for those would thrash. + const loadedRecordIdRef = useRef(undefined); + // Fetch initial data useEffect(() => { + // A `recordId` change re-enters this effect with the form still MOUNTED on + // the previous record, which needs handling on two fronts (pinned by + // recordSwapLoading.test.tsx): + // - go back to the loading state, so record A's values are not left on + // screen AND EDITABLE while B is in flight, to be swapped underneath in + // place when it lands. Anything typed there read as A's on screen but + // would have been submitted against B. + // - ignore a response that is no longer the one being awaited, so two + // overlapping reads land in REQUEST order, not completion order. + let cancelled = false; const fetchData = async () => { if (schema.mode === 'create' || !schema.recordId) { setFormData(schema.initialData || schema.initialValues || {}); @@ -231,19 +247,33 @@ export const DrawerForm: React.FC = ({ return; } + // Only a change of RECORD hides the form. Hiding it UNMOUNTS the inner + // renderer, which is the only thing that reports dirtiness — it cannot + // emit a final `onDirtyChange(false)` on its way out, so clear the flag + // here or the close/unload guards stay armed for input that belongs to a + // record no longer on screen. + if (loadedRecordIdRef.current !== schema.recordId) { + setLoading(true); + setIsDirty(false); + } + try { const data = await dataSource.findOne(schema.objectName, schema.recordId); + if (cancelled) return; + loadedRecordIdRef.current = schema.recordId; setFormData(data || {}); } catch (err) { + if (cancelled) return; setError(err as Error); } finally { - setLoading(false); + if (!cancelled) setLoading(false); } }; if (objectSchema || !dataSource) { fetchData(); } + return () => { cancelled = true; }; }, [objectSchema, schema.mode, schema.recordId, schema.initialData, schema.initialValues, dataSource, schema.objectName]); // Build form fields from section config diff --git a/packages/plugin-form/src/ModalForm.tsx b/packages/plugin-form/src/ModalForm.tsx index ce97f7bbd..b7a2894b9 100644 --- a/packages/plugin-form/src/ModalForm.tsx +++ b/packages/plugin-form/src/ModalForm.tsx @@ -290,8 +290,24 @@ export const ModalForm: React.FC = ({ fetchSchema(); }, [schema.objectName, dataSource]); + // The record whose data `formData` currently holds. The fetch effect reads it + // to tell a genuine record SWAP from a re-run of its own making — + // `initialData`/`initialValues` are objects callers commonly rebuild every + // render, and flashing the loading state for those would thrash. + const loadedRecordIdRef = useRef(undefined); + // Fetch initial data useEffect(() => { + // A `recordId` change re-enters this effect with the form still MOUNTED on + // the previous record, which needs handling on two fronts (pinned by + // recordSwapLoading.test.tsx): + // - go back to the loading state, so record A's values are not left on + // screen AND EDITABLE while B is in flight, to be swapped underneath in + // place when it lands. Anything typed there read as A's on screen but + // would have been submitted against B. + // - ignore a response that is no longer the one being awaited, so two + // overlapping reads land in REQUEST order, not completion order. + let cancelled = false; const fetchData = async () => { if (schema.mode === 'create' || !schema.recordId) { setFormData(schema.initialData || schema.initialValues || {}); @@ -305,19 +321,33 @@ export const ModalForm: React.FC = ({ return; } + // Only a change of RECORD hides the form. Hiding it UNMOUNTS the inner + // renderer, which is the only thing that reports dirtiness — it cannot + // emit a final `onDirtyChange(false)` on its way out, so clear the flag + // here or the close/unload guards stay armed for input that belongs to a + // record no longer on screen. + if (loadedRecordIdRef.current !== schema.recordId) { + setLoading(true); + setIsDirty(false); + } + try { const data = await dataSource.findOne(schema.objectName, schema.recordId); + if (cancelled) return; + loadedRecordIdRef.current = schema.recordId; setFormData(data || {}); } catch (err) { + if (cancelled) return; setError(err as Error); } finally { - setLoading(false); + if (!cancelled) setLoading(false); } }; if (objectSchema || !dataSource) { fetchData(); } + return () => { cancelled = true; }; }, [objectSchema, schema.mode, schema.recordId, schema.initialData, schema.initialValues, dataSource, schema.objectName]); // Build form fields from section config diff --git a/packages/plugin-form/src/SplitForm.tsx b/packages/plugin-form/src/SplitForm.tsx index a164242aa..b7d174801 100644 --- a/packages/plugin-form/src/SplitForm.tsx +++ b/packages/plugin-form/src/SplitForm.tsx @@ -14,7 +14,7 @@ * Aligns with @objectstack/spec FormView type: 'split' */ -import React, { useState, useCallback, useEffect, useMemo } from 'react'; +import React, { useState, useCallback, useEffect, useMemo, useRef } from 'react'; import type { FormField, DataSource } from '@object-ui/types'; import { ResizablePanelGroup, @@ -114,8 +114,24 @@ export const SplitForm: React.FC = ({ fetchSchema(); }, [schema.objectName, dataSource]); + // The record whose data `formData` currently holds. The fetch effect reads it + // to tell a genuine record SWAP from a re-run of its own making — + // `initialData`/`initialValues` are objects callers commonly rebuild every + // render, and flashing the loading state for those would thrash. + const loadedRecordIdRef = useRef(undefined); + // Fetch initial data useEffect(() => { + // A `recordId` change re-enters this effect with the form still MOUNTED on + // the previous record, which needs handling on two fronts (pinned by + // recordSwapLoading.test.tsx): + // - go back to the loading state, so record A's values are not left on + // screen AND EDITABLE while B is in flight, to be swapped underneath in + // place when it lands. Anything typed there read as A's on screen but + // would have been submitted against B. + // - ignore a response that is no longer the one being awaited, so two + // overlapping reads land in REQUEST order, not completion order. + let cancelled = false; const fetchData = async () => { if (schema.mode === 'create' || !schema.recordId) { setFormData(schema.initialData || schema.initialValues || {}); @@ -129,19 +145,26 @@ export const SplitForm: React.FC = ({ return; } + // Only a change of RECORD hides the form. + if (loadedRecordIdRef.current !== schema.recordId) setLoading(true); + try { const data = await dataSource.findOne(schema.objectName, schema.recordId); + if (cancelled) return; + loadedRecordIdRef.current = schema.recordId; setFormData(data || {}); } catch (err) { + if (cancelled) return; setError(err as Error); } finally { - setLoading(false); + if (!cancelled) setLoading(false); } }; if (objectSchema || !dataSource) { fetchData(); } + return () => { cancelled = true; }; }, [objectSchema, schema.mode, schema.recordId, schema.initialData, schema.initialValues, dataSource, schema.objectName]); // Build form fields from section config diff --git a/packages/plugin-form/src/TabbedForm.tsx b/packages/plugin-form/src/TabbedForm.tsx index b4d764be9..f659cea09 100644 --- a/packages/plugin-form/src/TabbedForm.tsx +++ b/packages/plugin-form/src/TabbedForm.tsx @@ -13,7 +13,7 @@ * Aligns with @objectstack/spec FormView type: 'tabbed' */ -import React, { useState, useCallback } from 'react'; +import React, { useState, useCallback, useRef } from 'react'; import type { FormField, DataSource } from '@object-ui/types'; import { cn } from '@object-ui/components'; import { SchemaRenderer, useSafeFieldLabel } from '@object-ui/react'; @@ -210,28 +210,51 @@ export const TabbedForm: React.FC = ({ fetchSchema(); }, [schema.objectName, dataSource]); + // The record whose data `formData` currently holds. The fetch effect reads it + // to tell a genuine record SWAP from a re-run of its own making — + // `initialData`/`initialValues` are objects callers commonly rebuild every + // render, and flashing the loading state for those would thrash. + const loadedRecordIdRef = useRef(undefined); + // Fetch initial data for edit/view modes React.useEffect(() => { + // A `recordId` change re-enters this effect with the form still MOUNTED on + // the previous record, which needs handling on two fronts (pinned by + // recordSwapLoading.test.tsx): + // - go back to the loading state, so record A's values are not left on + // screen AND EDITABLE while B is in flight, to be swapped underneath in + // place when it lands. Anything typed there read as A's on screen but + // would have been submitted against B. + // - ignore a response that is no longer the one being awaited, so two + // overlapping reads land in REQUEST order, not completion order. + let cancelled = false; const fetchData = async () => { if (schema.mode === 'create' || !schema.recordId || !dataSource) { setFormData(schema.initialData || schema.initialValues || {}); setLoading(false); return; } - + + // Only a change of RECORD hides the form. + if (loadedRecordIdRef.current !== schema.recordId) setLoading(true); + try { const data = await dataSource.findOne(schema.objectName, schema.recordId); + if (cancelled) return; + loadedRecordIdRef.current = schema.recordId; setFormData(data || {}); } catch (err) { + if (cancelled) return; setError(err as Error); } finally { - setLoading(false); + if (!cancelled) setLoading(false); } }; - + if (objectSchema || !dataSource) { fetchData(); } + return () => { cancelled = true; }; }, [objectSchema, schema.mode, schema.recordId, schema.initialData, schema.initialValues, dataSource, schema.objectName]); // Build form fields from section config diff --git a/packages/plugin-form/src/recordSwapLoading.test.tsx b/packages/plugin-form/src/recordSwapLoading.test.tsx new file mode 100644 index 000000000..ec2be0275 --- /dev/null +++ b/packages/plugin-form/src/recordSwapLoading.test.tsx @@ -0,0 +1,206 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +/** + * Swapping `recordId` on a MOUNTED sectioned form must not show the old record. + * + * `loading` in these four containers was only ever set `true` once, by + * `useState(true)`, and thereafter only ever set `false`. So a `recordId` change + * re-entered the fetch effect WITHOUT going back through the loading branch: the + * form stayed mounted showing — and accepting edits to — record A's values, with + * no indication that a different record had been asked for, until B's response + * landed and replaced them in place. Anything typed in that window belonged to A + * on screen and to B on submit. + * + * The same effect also had no staleness guard, so two overlapping fetches landed + * in COMPLETION order rather than request order: ask for B then C, and a slow B + * arriving last left the form showing B while the caller had asked for C. + * + * Both are the same defect from the user's side — the form displays a record + * nobody asked for — so both are pinned here. `ObjectForm` already did the right + * thing (it re-enters loading before the fetch), which is why this only ever + * reproduced on the sectioned variants. + */ + +import React from 'react'; +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { render, fireEvent, waitFor, cleanup } from '@testing-library/react'; +import { registerAllFields } from '@object-ui/fields'; +import { ModalForm } from './ModalForm'; +import { DrawerForm } from './DrawerForm'; +import { TabbedForm } from './TabbedForm'; +import { SplitForm } from './SplitForm'; + +registerAllFields(); + +const SECTIONS = [{ name: 'basics', label: 'Basics', fields: ['title'] }]; + +/** A `findOne` whose every response is resolved by the test, by record id. */ +function deferredDataSource() { + const pending = new Map void>(); + return { + ds: { + getObjectSchema: vi.fn().mockResolvedValue({ + name: 'task', + fields: { title: { type: 'text', label: 'Title' } }, + }), + create: vi.fn(), + update: vi.fn(), + findOne: vi.fn( + (_obj: string, id: string) => + new Promise((resolve) => { + pending.set(id, resolve as (v: unknown) => void); + }), + ), + } as any, + /** + * Resolve the read for `id` with that record's payload, waiting for the + * request to actually be issued first — the fetch effect is gated on + * `getObjectSchema` resolving, so `findOne` is a tick or two behind render. + */ + async land(id: string, title: string) { + const resolve = await waitFor(() => { + const r = pending.get(id); + if (!r) throw new Error(`no in-flight findOne for ${id}`); + return r; + }); + pending.delete(id); + resolve({ id, title }); + // Let the resulting state updates flush. + await waitFor(() => {}); + }, + }; +} + +const titleInput = () => + document.body.querySelector('[data-field="title"] input'); + +const schemaFor = (formType: string, recordId: string) => + ({ + type: 'object-form', + formType, + objectName: 'task', + mode: 'edit', + recordId, + sections: SECTIONS, + // Overlay variants only. + open: true, + onOpenChange: vi.fn(), + }) as any; + +beforeEach(() => vi.clearAllMocks()); +afterEach(() => cleanup()); + +describe.each([ + ['ModalForm', ModalForm, 'modal'], + ['DrawerForm', DrawerForm, 'drawer'], + ['TabbedForm', TabbedForm, 'tabbed'], + ['SplitForm', SplitForm, 'split'], +] as const)('%s — recordId swap', (_name, Form, formType) => { + it('does not keep showing the previous record while the new one loads', async () => { + const { ds, land } = deferredDataSource(); + const { rerender } = render( +
, + ); + + await land('r1', 'Record One'); + await waitFor(() => expect(titleInput()?.value).toBe('Record One')); + + // The caller asks for a different record. B's read is still in flight. + rerender(); + + // Record A must be off screen — showing it here invites the user to edit + // values that will be submitted against record B. + expect(titleInput()?.value ?? null).not.toBe('Record One'); + + await land('r2', 'Record Two'); + await waitFor(() => expect(titleInput()?.value).toBe('Record Two')); + }); + + it('ignores a stale response that lands after a newer one', async () => { + const { ds, land } = deferredDataSource(); + const { rerender } = render( + , + ); + await land('r1', 'Record One'); + await waitFor(() => expect(titleInput()?.value).toBe('Record One')); + + // Two swaps in flight; the SECOND is what the caller wants. + rerender(); + rerender(); + + // Responses come back out of order — r3 first, then the slower r2. + await land('r3', 'Record Three'); + await land('r2', 'Record Two'); + + // Request order wins, not completion order. + await waitFor(() => expect(titleInput()?.value).toBe('Record Three')); + expect(titleInput()?.value).not.toBe('Record Two'); + }); +}); + +/** + * Hiding the form to load the next record UNMOUNTS the inner renderer, and that + * renderer is the only thing that reports dirtiness (`onDirtyChange`) — it gets + * no chance to report `false` on its way out. Left alone, the overlay's guards + * (#2998) would stay armed for input belonging to a record no longer on screen: + * a plain refresh would prompt, and closing would ask to discard nothing. + */ +describe.each([ + ['ModalForm', ModalForm, 'modal'], + ['DrawerForm', DrawerForm, 'drawer'], +] as const)('%s — swapping records clears the unsaved-input guard', (_name, Form, formType) => { + it('stops blocking unload once the previous record is gone', async () => { + const { ds, land } = deferredDataSource(); + const { rerender } = render( + , + ); + await land('r1', 'Record One'); + await waitFor(() => expect(titleInput()?.value).toBe('Record One')); + + // Dirty record A, and prove the guard is armed. + fireEvent.change(titleInput() as HTMLInputElement, { target: { value: 'edited' } }); + await waitFor(() => { + const evt = new Event('beforeunload', { cancelable: true }); + window.dispatchEvent(evt); + expect(evt.defaultPrevented).toBe(true); + }); + + rerender(); + + // A's edits went with A — nothing unsaved is on screen to protect. + const armed = new Event('beforeunload', { cancelable: true }); + window.dispatchEvent(armed); + expect(armed.defaultPrevented).toBe(false); + }); +}); + +describe('create mode is unaffected', () => { + it('never enters the loading state for a form with no recordId', async () => { + const { ds } = deferredDataSource(); + render( + , + ); + + // Renders straight to an empty form; `findOne` is never called. + await waitFor(() => expect(titleInput()).not.toBeNull()); + expect(titleInput()?.value).toBe(''); + expect(ds.findOne).not.toHaveBeenCalled(); + }); +});