fix(core): Undo of an undoable update never writes null over a field the row did not carry

.changeset/10404-undo-absent-field-not-null.md

@@ -0,0 +1,35 @@
+---
+'@object-ui/core': minor
+'@object-ui/app-shell': minor
+---
+
+fix(core): Undo of an `undoable` update no longer writes `null` over a field the row did not carry
+
+An `undoable` update records the prior value of every field it writes, read off
+the row it ran on, so the success toast can offer Undo. On a list row projected
+by `$select` (`ListView`, `ObjectGrid`, `RelatedList`), a written field that no
+column shows was absent from the row. It was recorded as `null`, and Undo then
+wrote `null` over the value it existed to restore: close a task from a grid that
+shows only its name, press Undo, and its status became empty instead of `open`.
+
+Two changes:
+
+- The list harvest (`listViewPredicates`) now also names the fields an
+  `undoable` action writes: its `patch` keys, the key each param's value is
+  collected under (`name`, else `field`), and its `bodyExtra` keys. So a
+  projected row carries them, through the same gates as every harvested name:
+  a bare identifier, a field the object declares (or a platform column), and
+  field-level security once the permission answer has loaded. `recordId` is
+  not harvested; both writers strip it as the record's address.
+- The capture never records an absent field as `null`, in `ActionRunner`'s
+  `operation: 'update'` path and in the console `api` handler's data-source
+  branch alike. When the row does not carry every written field (for example a
+  field the principal may write but not read), the action still runs, but it
+  offers no Undo: the success toast has no Undo button, nothing is pushed onto
+  the undo stack, and a console warning names the missing fields. A `null` the
+  row does carry is a real empty value and is still restored as `null`.
+
+Behaviour to know about: on a backend that leaves null-valued fields out of a
+full record, an `undoable` update of such a field now offers no Undo instead of
+one that restores it to empty. Absent and empty cannot be told apart there, and
+no Undo is the side that cannot overwrite stored data.

packages/app-shell/src/hooks/__tests__/useConsoleActionRuntime.undoAbsentField-10404.test.tsx

@@ -0,0 +1,128 @@
+/**
+ * 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.
+ */
+
+/**
+ * objectui#10404 — the console `api` handler's generic (data-source) branch
+ * never records a field the row does not carry as `null` in its Undo snapshot.
+ *
+ * This branch writes `params` plus `bodyExtra` through `dataSource.update` and,
+ * for an `undoable` action, snapshots each written field's prior value off the
+ * row stashed under `params._rowRecord`. The snapshot used to be
+ * `rowRecord[k] ?? null`, so on a projected list row a written field no column
+ * shows was recorded as `null` and Undo overwrote the stored value with it —
+ * the same defect as `ActionRunner`'s `captureUpdateUndoData`, in the handler's
+ * own copy.
+ */
+
+import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest';
+import { renderHook, act } from '@testing-library/react';
+
+vi.mock('react-router-dom', () => ({ useNavigate: () => vi.fn() }));
+
+vi.mock('@object-ui/auth', async (importOriginal) => ({
+  ...(await importOriginal<Record<string, unknown>>()),
+  useAuth: () => ({ user: { id: 'u1', name: 'User', image: null }, activeOrganization: null }),
+  createAuthenticatedFetch: () => vi.fn(),
+}));
+
+vi.mock('@object-ui/i18n', async (importOriginal) => ({
+  ...(await importOriginal<Record<string, unknown>>()),
+  useObjectLabel: () => ({
+    fieldLabel: (_o: unknown, _n: unknown, l: unknown) => l,
+    fieldOptionLabel: (_o: unknown, _f: unknown, _v: unknown, l: unknown) => l,
+    actionParamText: (_o: unknown, _a: unknown, _p: unknown, _attr: unknown, fallback: unknown) => fallback,
+    actionParamOptionLabel: (_o: unknown, _a: unknown, _p: unknown, _v: unknown, fallback: unknown) => fallback,
+    actionDescription: (_o: unknown, _a: unknown, fallback: unknown) => fallback,
+  }),
+  useObjectTranslation: () => ({
+    t: (key: string, options?: { defaultValue?: string }) => String(options?.defaultValue ?? key),
+  }),
+}));
+
+// Imported for real these drag in the modal/form graph; nothing here renders them.
+vi.mock('../useActionModal', () => ({
+  useActionModal: () => ({
+    modalHandler: vi.fn(),
+    modalElement: null,
+    closeModal: () => {},
+    resolveModalTarget: vi.fn(async () => null),
+  }),
+}));
+vi.mock('../../views/ActionConfirmDialog', () => ({ ActionConfirmDialog: () => null }));
+vi.mock('../../views/ActionParamDialog', () => ({ ActionParamDialog: () => null }));
+vi.mock('../../views/ActionResultDialog', () => ({ ActionResultDialog: () => null }));
+vi.mock('../../views/FlowRunner', () => ({ FlowRunner: () => null }));
+vi.mock('sonner', () => ({ toast: Object.assign(vi.fn(), { error: vi.fn(), success: vi.fn() }) }));
+
+import type { ActionDef, ActionResult } from '@object-ui/core';
+import { useConsoleActionRuntime } from '../useConsoleActionRuntime';
+
+let warn: ReturnType<typeof vi.spyOn>;
+beforeEach(() => {
+  warn = vi.spyOn(console, 'warn').mockImplementation(() => {});
+});
+afterEach(() => vi.restoreAllMocks());
+
+/** Run the card's action through the generic branch against `row`. */
+async function closeTask(row: Record<string, unknown>, extra: Record<string, unknown> = {}) {
+  const dataSource = { update: vi.fn(async () => ({})) };
+  const { result } = renderHook(() =>
+    useConsoleActionRuntime({ dataSource, objects: [], objectName: 'task' }),
+  );
+  let res: ActionResult = { success: false };
+  await act(async () => {
+    res = await result.current.apiHandler({
+      type: 'api',
+      name: 'close_task',
+      label: 'Close',
+      target: 'close_task',
+      undoable: true,
+      bodyExtra: { status: 'closed' },
+      params: { _rowRecord: row },
+      ...extra,
+    } as ActionDef);
+  });
+  return { res, dataSource };
+}
+
+describe('console `api` handler — the Undo snapshot never records an absent field as `null` (objectui#10404)', () => {
+  it('offers no Undo when the row lacks the written field, and still performs the write', async () => {
+    // A projected row: `status` was not selected.
+    const { res, dataSource } = await closeTask({ id: 't_1', name: 'Ada' });
+
+    expect(dataSource.update).toHaveBeenCalledWith('task', 't_1', { status: 'closed' });
+    expect(res.success).toBe(true);
+    expect(res.undo).toBeUndefined();
+    expect(warn.mock.calls[0][1]).toEqual({ action: 'close_task', missing: ['status'] });
+  });
+
+  it('control: a row that carries the field snapshots its stored value', async () => {
+    const { res } = await closeTask({ id: 't_1', name: 'Ada', status: 'open' });
+
+    expect(res.undo?.undoData).toEqual({ status: 'open' });
+    expect(res.undo?.redoData).toEqual({ status: 'closed' });
+    expect(warn).not.toHaveBeenCalled();
+  });
+
+  it('still snapshots a `null` the row carries — a real empty value', async () => {
+    const { res } = await closeTask({ id: 't_1', status: null });
+
+    expect(res.undo?.undoData).toEqual({ status: null });
+  });
+
+  it('offers no partial Undo: a carried `bodyExtra` field beside an absent param field is no Undo at all', async () => {
+    const { res } = await closeTask(
+      { id: 't_1', status: 'open' },
+      { params: { _rowRecord: { id: 't_1', status: 'open' }, note: 'typed' } },
+    );
+
+    expect(res.success).toBe(true);
+    expect(res.undo).toBeUndefined();
+    expect(warn.mock.calls[0][1]).toEqual({ action: 'close_task', missing: ['note'] });
+  });
+});

packages/app-shell/src/hooks/useConsoleActionRuntime.tsx

@@ -522,21 +522,43 @@ export function useConsoleActionRuntime(opts: ConsoleActionRuntimeOptions): Cons
 
       // Undoable single-record update: capture the prior values of the changed
       // fields from the row record so the success toast can offer "Undo".
+      //
+      // ⛔ A field the row does not CARRY is never captured as `null`
+      // (objectui#10404) — the rule `captureUpdateUndoData` in
+      // `@object-ui/core`'s `ActionRunner` states, applied to this handler's
+      // written set (`params` plus `bodyExtra`). A list row is projected by
+      // `$select`, so a written field no column shows is absent while the
+      // server holds a real value; recording `null` made Undo overwrite it. A
+      // `null` the row carries is a real empty value and is captured as one.
+      // When any written field is not carried there is no Undo at all: the
+      // success toast then has no Undo button, and the warning names the cause.
       let undo: ActionResult['undo'];
       if (action.undoable && obj && recId && rowRecord && Object.keys(fields).length > 0
           && typeof dataSource?.update === 'function') {
-        const undoData: Record<string, unknown> = {};
-        for (const k of Object.keys(fields)) undoData[k] = rowRecord[k] ?? null;
-        undo = {
-          id: `undo-${obj}-${recId}-${Date.now()}`,
-          type: 'update',
-          objectName: obj,
-          recordId: String(recId),
-          timestamp: Date.now(),
-          description: action.label || `Undo ${obj}`,
-          undoData,
-          redoData: { ...fields },
-        };
+        const written = Object.keys(fields);
+        const missing = written.filter(
+          (k) => !Object.prototype.hasOwnProperty.call(rowRecord, k) || rowRecord[k] === undefined,
+        );
+        if (missing.length === 0) {
+          undo = {
+            id: `undo-${obj}-${recId}-${Date.now()}`,
+            type: 'update',
+            objectName: obj,
+            recordId: String(recId),
+            timestamp: Date.now(),
+            description: action.label || `Undo ${obj}`,
+            undoData: Object.fromEntries(written.map((k) => [k, rowRecord[k]])),
+            redoData: { ...fields },
+          };
+        } else {
+          console.warn(
+            '[useConsoleActionRuntime] `undoable` action succeeded but offers no Undo: the row it ran on '
+            + 'does not carry every field it wrote, so their prior values are unknown and an Undo would '
+            + 'overwrite stored data. A list row carries a written field when the object declares it and '
+            + 'the principal may read it.',
+            { action: action.name, missing },
+          );
+        }
       }
 
       const shouldRefresh = action.refreshAfter !== false;

packages/core/src/actions/ActionRunner.ts

@@ -883,16 +883,36 @@ function isUpdateOperationAction(action: ActionDef): boolean {
  *
  * Prior values come from the row record the invoking surface stashed under
  * `params._rowRecord` (the same client-side stash the record-id dance reads).
- * A field absent from that row is captured as `null`, which is what clearing it
- * back to empty means on the data plane.
+ * A field the row carries as `null` is captured as `null`: it was empty, and
+ * clearing it back to empty is what restoring it means on the data plane.
+ *
+ * ⛔ A field the row does not CARRY is never captured as `null`
+ * (objectui#10404). Absent is not empty: a list row is projected by `$select`,
+ * and a written field no column shows is missing from the row while the server
+ * holds a real value for it. Recording `null` there made Undo write `null` over
+ * the value it existed to restore. The list harvest (`listViewPredicates`) now
+ * asks for the written fields of an `undoable` action, so a projected row
+ * carries them; this is the backstop for the ones it cannot ask for (a field
+ * the object does not declare or the principal may not read, a row no harvest
+ * shaped). "Carries" means an own key whose value is not `undefined`: JSON
+ * cannot send `undefined`, so such a value says nothing about what is stored.
+ *
+ * Answers `undefined` when any written field is not carried, and the caller
+ * then offers no Undo at all. Capturing only the carried fields is not an
+ * option, for the reason above: a partial restore reported as a full one is
+ * worse than no Undo.
  */
 function captureUpdateUndoData(
   writtenFields: readonly string[],
   rowRecord: Record<string, unknown>,
-): Record<string, unknown> {
-  const undoData: Record<string, unknown> = {};
-  for (const field of writtenFields) undoData[field] = rowRecord[field] ?? null;
-  return undoData;
+): Record<string, unknown> | undefined {
+  if (!writtenFields.every((field) => rowCarries(rowRecord, field))) return undefined;
+  return Object.fromEntries(writtenFields.map((field) => [field, rowRecord[field]]));
+}
+
+/** Whether the row CARRIES `field`: an own key whose value is not `undefined`. */
+function rowCarries(rowRecord: Record<string, unknown>, field: string): boolean {
+  return Object.prototype.hasOwnProperty.call(rowRecord, field) && rowRecord[field] !== undefined;
 }
 
 /*
@@ -1558,21 +1578,37 @@ export class ActionRunner {
 
     // Undo: prior values of exactly the fields this action wrote. Needs a row
     // record to read them from — without one there is nothing to restore, so
-    // the affordance is correctly not offered rather than offered empty.
+    // the affordance is correctly not offered rather than offered empty. The
+    // same holds when the row lacks a written field (objectui#10404): no Undo,
+    // never one that writes `null` over the stored value.
     if (action.undoable && rowRecord && writtenFields.length > 0) {
       const objectName = action.objectName || readContextObjectName(this.context);
       const recordId = collected.recordId ?? rowRecord.id;
       if (objectName && recordId != null) {
-        result.undo = {
-          id: `undo-${objectName}-${String(recordId)}-${Date.now()}`,
-          type: 'update',
-          objectName,
-          recordId: String(recordId),
-          timestamp: Date.now(),
-          description: action.label || `Undo ${objectName}`,
-          undoData: captureUpdateUndoData(writtenFields, rowRecord),
-          redoData: Object.fromEntries(writtenFields.map((k) => [k, params[k]])),
-        };
+        const undoData = captureUpdateUndoData(writtenFields, rowRecord);
+        if (undoData) {
+          result.undo = {
+            id: `undo-${objectName}-${String(recordId)}-${Date.now()}`,
+            type: 'update',
+            objectName,
+            recordId: String(recordId),
+            timestamp: Date.now(),
+            description: action.label || `Undo ${objectName}`,
+            undoData,
+            redoData: Object.fromEntries(writtenFields.map((k) => [k, params[k]])),
+          };
+        } else {
+          // The success toast then carries no Undo button, which is how the
+          // user learns this one cannot be undone; this names the cause for
+          // the author.
+          console.warn(
+            '[ActionRunner] `undoable` update succeeded but offers no Undo: the row it ran on does not '
+            + 'carry every field it wrote, so their prior values are unknown and an Undo would overwrite '
+            + 'stored data. A list row carries a written field when the object declares it and the '
+            + 'principal may read it.',
+            { action: action.name, missing: writtenFields.filter((k) => !rowCarries(rowRecord, k)) },
+          );
+        }
       }
     }