From 01de981ddc71d30d58c657350120587612e01dba Mon Sep 17 00:00:00 2001 From: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Date: Thu, 30 Jul 2026 17:09:58 +0800 Subject: [PATCH] fix(plugin-form): swapping recordId no longer leaves the previous record on screen MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 (#2998) 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. Verified by mutation — dropping only that line fails the two guard tests with `expected true to be false`. New `recordSwapLoading.test.tsx` drives each container through a real record swap with a `findOne` the test resolves by id, so both the stale-record and the out-of-order cases are deterministic rather than timing-dependent; all 11 fail against the previous code (8 of them on the two core assertions). Verified: `npx vitest run packages/plugin-form/ packages/components/ packages/app-shell/` — 315 files / 2627 tests pass; `@object-ui/plugin-form` type-check clean, lint 0 errors (the 2 unused-disable warnings are pre-existing, in EmbeddableForm/MasterDetailForm). Co-Authored-By: Claude Opus 5 --- .../record-swap-hides-the-previous-record.md | 38 ++++ packages/plugin-form/src/DrawerForm.tsx | 32 ++- packages/plugin-form/src/ModalForm.tsx | 32 ++- packages/plugin-form/src/SplitForm.tsx | 27 ++- packages/plugin-form/src/TabbedForm.tsx | 31 ++- .../src/recordSwapLoading.test.tsx | 206 ++++++++++++++++++ 6 files changed, 358 insertions(+), 8 deletions(-) create mode 100644 .changeset/record-swap-hides-the-previous-record.md create mode 100644 packages/plugin-form/src/recordSwapLoading.test.tsx 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 0000000000..6e3ef0cf02 --- /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 3bf7921b9d..7f46269bea 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 ce97f7bbdb..b7a2894b9b 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 a164242aa0..b7d1748019 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 b4d764be96..f659cea098 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 0000000000..ec2be02759 --- /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(); + }); +});