fix(plugin-form): swapping recordId no longer leaves the previous record on screen (#3005)

`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: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com>
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>

Author: os-zhuang

.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.

packages/plugin-form/src/DrawerForm.tsx

@@ -216,8 +216,24 @@ export const DrawerForm: React.FC<DrawerFormProps> = ({
     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<string | number | undefined>(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<DrawerFormProps> = ({
         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

packages/plugin-form/src/ModalForm.tsx

@@ -290,8 +290,24 @@ export const ModalForm: React.FC<ModalFormProps> = ({
     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<string | number | undefined>(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<ModalFormProps> = ({
         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

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<SplitFormProps> = ({
     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<string | number | undefined>(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<SplitFormProps> = ({
         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

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<TabbedFormProps> = ({
     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<string | number | undefined>(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