fix(detail): distinguish "in approval (editable)" from locked, and stop losing write warnings

.changeset/approval-band-truth-and-write-warnings.md

@@ -0,0 +1,53 @@
+---
+"@object-ui/plugin-detail": minor
+"@object-ui/app-shell": minor
+"@object-ui/data-objectstack": minor
+"@object-ui/i18n": minor
+---
+
+fix(detail): finish the approval-lock story, and warn on silently stripped fields (framework#3794)
+
+The Console reported record writability wrong in both directions during an
+approval, so a user had nothing to go on: what they *could* edit said "locked",
+and what they *couldn't* said "updated successfully".
+
+**The lock band told the truth; the Edit button did not.** objectui#2902 split
+the band into "in approval · editable" vs locked, but the header **Edit** CTA
+still keyed off nothing at all — on a genuinely locked record it stayed live, so
+the user opened the form, filled a screen, and got `RECORD_LOCKED` back on Save.
+It is now `disabled` on a locked record: visible-but-off, with the band beside it
+saying why. This is the LOCK, not the mere presence of an approval — a
+`lockRecord: false` node keeps Edit live, which is the point of that setting.
+
+**And the band could still re-lock itself.** `DetailView` OR-ed the record's own
+`approval_status` mirror into `isLocked` unconditionally. That mirror is written
+on submit by any flow configuring an `approvalStatusField`, *regardless of*
+`lockRecord` — so on a `lockRecord: false` node the host correctly resolved "not
+locked" from the request's `lock_record` while the mirror dragged the band back
+to "Locked for approval", with the pencils live and saves landing underneath it.
+The host is now authoritative whenever it threads `approvalPending`; the mirror
+is consulted only for bare/legacy `DetailView` hosts that thread nothing, where
+it still reads as locked (no node granularity — the safe direction).
+
+Recall's tooltip no longer promises to unlock a record the node never locked
+(`detail.cancelApprovalTooltipUnlocked`).
+
+**Silently stripped fields now surface on the record form's save path.** The
+adapter emitted a write-warning for `create`/`update` responses carrying
+`droppedFields`, but not for `batchTransaction` — which is how the record form
+saves a master-detail record, i.e. the one surface where a user actually edits a
+`readonlyWhen`-locked field. `batchTransaction` now emits one warning per event,
+resolving each back to its operation via the response's `index`.
+
+The toast itself was hardcoded English and called every strip "read-only". It is
+now localized (`detail.writeStripped*`, ten locales) and worded by reason:
+`readonly_when` says the field is not editable *in this record's current state*,
+which is what actually happened — the field is editable in other states and the
+form rendered it as an ordinary input, so "read-only" sent the user hunting for a
+permission problem that does not exist.
+
+**And it stopped crying wolf.** `createObjectStackUserStateAdapter` hand-stamped
+the server-managed `updated_at` on every recents/favorites write, which the
+server strips and reports — so the console popped "Some fields were not saved"
+about a field no user ever touched, on page loads, drowning the signal the toast
+exists for. It no longer sends the column; the server stamps it anyway.

packages/app-shell/src/providers/AdapterProvider.tsx

@@ -7,29 +7,59 @@
  * @module
  */
 
-import { useState, useEffect, type ReactNode } from 'react';
+import { useState, useEffect, useRef, type ReactNode } from 'react';
 import { toast } from 'sonner';
 import { ObjectStackAdapter, type WriteWarningEvent } from '@object-ui/data-objectstack';
 import { createAuthenticatedFetch } from '@object-ui/auth';
 import { AdapterCtx } from '@object-ui/react';
+import { useObjectTranslation } from '@object-ui/i18n';
 import { installSettleSignalGlobal, withSettleSignal } from '../observability/settleSignal';
 
 export { useAdapter } from '@object-ui/react';
 
+type TranslateFn = (key: string, options?: Record<string, unknown>) => string;
+
 /**
  * Turn a write-warning (framework #3431/#3455) into a human toast. The write
- * SUCCEEDED — some caller-supplied fields were legally stripped (read-only /
- * locked by state), so we tell the user rather than let it pass silently.
+ * SUCCEEDED — some caller-supplied fields were legally stripped, so we tell the
+ * user rather than let it pass silently.
+ *
+ * The REASON decides the wording (#3794). `readonly_when` is not "this field is
+ * read-only" — the field is editable in other states and the form rendered it
+ * as an ordinary input; what happened is that THIS record's current state locks
+ * it. Saying "read-only" there sends the user looking for a permission problem
+ * that doesn't exist. Fields are named by their machine name: the event carries
+ * no labels, and a wrong guess at a label is worse than an exact key.
  */
-function toastWriteWarning(ev: WriteWarningEvent): void {
-  const fields = Array.from(new Set(ev.droppedFields.flatMap((d) => d.fields)));
-  if (fields.length === 0) return;
-  const list = fields.join(', ');
-  const description =
-    fields.length === 1
-      ? `The read-only field “${list}” could not be changed and was not saved.`
-      : `${fields.length} read-only fields could not be changed and were not saved: ${list}.`;
-  toast.warning('Some fields were not saved', { description });
+function toastWriteWarning(ev: WriteWarningEvent, t: TranslateFn): void {
+  const byReason = new Map<string, string[]>();
+  for (const d of ev.droppedFields) {
+    const seen = byReason.get(d.reason) ?? [];
+    for (const f of d.fields) if (!seen.includes(f)) seen.push(f);
+    byReason.set(d.reason, seen);
+  }
+  const lines: string[] = [];
+  for (const [reason, fields] of byReason) {
+    if (fields.length === 0) continue;
+    const list = fields.join(', ');
+    lines.push(
+      reason === 'readonly_when'
+        ? t('detail.writeStrippedByState', {
+            fields: list,
+            defaultValue:
+              'Not editable in this record\'s current state, so it was not saved: {{fields}}',
+          })
+        : t('detail.writeStrippedReadonly', {
+            fields: list,
+            defaultValue: 'Read-only, so it was not saved: {{fields}}',
+          }),
+    );
+  }
+  if (lines.length === 0) return;
+  toast.warning(
+    t('detail.writeStrippedTitle', { defaultValue: 'Some fields were not saved' }),
+    { description: lines.join('\n') },
+  );
 }
 
 interface AdapterProviderProps {
@@ -44,6 +74,13 @@ interface AdapterProviderProps {
  */
 export function AdapterProvider({ children, adapter: externalAdapter }: AdapterProviderProps) {
   const [adapter, setAdapter] = useState<ObjectStackAdapter | null>(externalAdapter ?? null);
+  // The warning listener is registered ONCE (the adapter outlives a language
+  // switch), so read `t` through a ref instead of capturing it — otherwise a
+  // user who switches language mid-session keeps getting the old locale, and
+  // adding `t` to the effect deps would tear down and rebuild the adapter.
+  const { t } = useObjectTranslation();
+  const tRef = useRef(t);
+  tRef.current = t;
 
   useEffect(() => {
     if (externalAdapter) {
@@ -72,7 +109,9 @@ export function AdapterProvider({ children, adapter: externalAdapter }: AdapterP
 
         // Surface silently-stripped write fields (#3431/#3455) as a toast so a
         // read-only value the user typed doesn't just vanish on save.
-        unsubscribeWriteWarning = a.onWriteWarning(toastWriteWarning);
+        unsubscribeWriteWarning = a.onWriteWarning((ev) =>
+          toastWriteWarning(ev, tRef.current as TranslateFn),
+        );
 
         await a.connect();
 

packages/app-shell/src/views/RecordDetailView.tsx

@@ -1839,6 +1839,14 @@ export function RecordDetailView({ dataSource, objects, onEdit, objectNameOverri
         // can't be stacked on top of the draft (two competing edit sessions
         // with no reconciliation).
         disableDuringInlineEdit: true,
+        // framework#3794 — an approval-LOCKED record refuses every write (the
+        // server answers RECORD_LOCKED), so opening the form only to be
+        // rejected on Save after filling a screen is wasted work. Disabled
+        // rather than hidden: the user should see the affordance exists and is
+        // off, with the lock band next to it saying why. Note this is the
+        // LOCK, not the mere presence of an approval — a `lockRecord: false`
+        // node keeps Edit live, which is the point of that setting.
+        disabled: approvalLocked,
         onClick: () => onEdit({ id: pureRecordId }),
       } as any);
     }

packages/data-objectstack/src/index.ts

@@ -973,6 +973,41 @@ export class ObjectStackAdapter<T = unknown> implements DataSource<T> {
     this.emitWriteWarning({ operation, resource, ...(id !== undefined ? { id } : {}), droppedFields });
   }
 
+  /**
+   * Same, for the cross-object transactional batch (framework #3794). Its
+   * response hangs the events off a top-level `droppedFields` list, each tagged
+   * with the `index` of the operation it came from — `results` entries are bare
+   * record echoes with nowhere to hang a per-row list.
+   *
+   * This is the path that matters most for the warning: `batchTransaction` is
+   * how the console's record form saves a master-detail record, so a
+   * `readonlyWhen`-locked field edited in that form was stripped server-side
+   * while the UI reported a plain success. The operation kind is taken from the
+   * originating op so the toast doesn't call an update a create.
+   */
+  private notifyBatchDroppedFields(
+    operations: BatchTransactionOperation[],
+    payload: unknown,
+  ): void {
+    const dropped = (payload as { droppedFields?: unknown } | null | undefined)?.droppedFields;
+    if (!Array.isArray(dropped) || dropped.length === 0) return;
+    for (const entry of dropped) {
+      if (!entry || typeof entry !== 'object') continue;
+      const e = entry as DroppedFieldsEvent & { index?: number };
+      if (!Array.isArray(e.fields) || e.fields.length === 0) continue;
+      const op = typeof e.index === 'number' ? operations[e.index] : undefined;
+      // `delete` never drops fields; anything unexpected reads as an update,
+      // which is the truthful default for a batch that echoed a strip.
+      const operation: 'create' | 'update' = (op?.action ?? 'create') === 'create' ? 'create' : 'update';
+      this.emitWriteWarning({
+        operation,
+        resource: e.object ?? op?.object ?? '',
+        ...(op?.id !== undefined && op?.id !== null ? { id: op.id } : {}),
+        droppedFields: [{ object: e.object, fields: e.fields, reason: e.reason }],
+      });
+    }
+  }
+
   /**
    * Subscribe to write-warning events (a create/update dropped caller-supplied
    * fields — #3431/#3455). Returns an unsubscribe function. The app shell uses
@@ -1225,6 +1260,7 @@ export class ObjectStackAdapter<T = unknown> implements DataSource<T> {
       // dependency floor (framework #3271). No hand-rolled POST /api/v1/batch.
       const payload = await this.client.data.batchTransaction(operations);
       this.emitBatchMutations(operations, payload?.results);
+      this.notifyBatchDroppedFields(operations, payload);
       return payload;
     } catch (err) {
       // On a non-declaring backend, endpoint missing (404/405) or a runtime that

packages/data-objectstack/src/onWriteWarning.test.ts

@@ -94,6 +94,56 @@ describe('ObjectStackAdapter.onWriteWarning', () => {
     expect(events).toEqual([]);
   });
 
+  // ── cross-object batch (framework #3794) ─────────────────────────────────
+  //
+  // `batchTransaction` is the console record form's save path for a
+  // master-detail record, so this is where a `readonlyWhen` strip actually
+  // reaches a user editing a form. Its response tags each event with the index
+  // of the operation that produced it.
+
+  it('emits a write-warning per event on a cross-object batch, resolving the op', async () => {
+    const batchTransaction = vi.fn().mockResolvedValue({
+      results: [{ id: 'acc1' }, { id: 'inv1' }],
+      droppedFields: [
+        { object: 'invoice', fields: ['tax_rate'], reason: 'readonly_when', index: 1 },
+      ],
+    });
+    const ds = makeDS({ batchTransaction });
+    ds.atomicBatchCapability = true;
+    const events: WriteWarningEvent[] = [];
+    ds.onWriteWarning((e: WriteWarningEvent) => events.push(e));
+
+    await ds.batchTransaction([
+      { object: 'account', action: 'create', data: { name: 'Acme' } },
+      { object: 'invoice', action: 'update', id: 'inv1', data: { status: 'paid', tax_rate: 9 } },
+    ]);
+
+    expect(events).toEqual([
+      {
+        operation: 'update',
+        resource: 'invoice',
+        id: 'inv1',
+        droppedFields: [{ object: 'invoice', fields: ['tax_rate'], reason: 'readonly_when' }],
+      },
+    ]);
+  });
+
+  it('does NOT emit for a clean batch or a malformed droppedFields list', async () => {
+    const batchTransaction = vi
+      .fn()
+      .mockResolvedValueOnce({ results: [{ id: 'a' }] })
+      .mockResolvedValueOnce({ results: [{ id: 'a' }], droppedFields: [{ object: 'x', fields: [], index: 0 }] });
+    const ds = makeDS({ batchTransaction });
+    ds.atomicBatchCapability = true;
+    const events: WriteWarningEvent[] = [];
+    ds.onWriteWarning((e: WriteWarningEvent) => events.push(e));
+
+    await ds.batchTransaction([{ object: 'x', action: 'create', data: {} }]);
+    await ds.batchTransaction([{ object: 'x', action: 'create', data: {} }]);
+
+    expect(events).toEqual([]);
+  });
+
   it('stops delivering after unsubscribe', async () => {
     const create = vi.fn().mockResolvedValue({ record: { id: 'r1' }, droppedFields: ONE_DROP });
     const ds = makeDS({ create });

packages/data-objectstack/src/userState.test.ts

@@ -144,7 +144,6 @@ describe('createObjectStackUserStateAdapter', () => {
         user_id: 'u1',
         key: 'ui.favorites',
         value: [{ id: 'a' }],
-        updated_at: expect.any(String),
       });
     });
 
@@ -165,11 +164,35 @@ describe('createObjectStackUserStateAdapter', () => {
 
       expect(ds.update).toHaveBeenCalledWith('sys_user_preference', 'row-99', {
         value: [{ id: 'x' }],
-        updated_at: expect.any(String),
       });
       expect(ds.create).not.toHaveBeenCalled();
     });
 
+    // #3794 — `updated_at` is server-managed. A caller-supplied value is
+    // stripped (framework #2948) and reported back as a dropped field, which
+    // the console turns into a "Some fields were not saved" toast. Stamping it
+    // here fired that warning on every recents/favorites write, about a field
+    // no user ever touched — noise on the exact surface that is supposed to
+    // tell a user their edit did not land.
+    it('never sends the server-managed updated_at column', async () => {
+      const ds = mockDataSource({
+        find: vi.fn().mockResolvedValue({ data: [] }) as any,
+        create: vi.fn().mockResolvedValue({ id: 'row-1' }) as any,
+      });
+      const adapter = createObjectStackUserStateAdapter({
+        dataSource: ds,
+        userId: 'u',
+        key: 'ui.recent',
+      });
+
+      await adapter.save([{ id: 'a' } as any]);
+      await adapter.save([{ id: 'b' } as any]);
+
+      for (const call of [...(ds.create as any).mock.calls, ...(ds.update as any).mock.calls]) {
+        expect(call[call.length - 1]).not.toHaveProperty('updated_at');
+      }
+    });
+
     it('uses the cached row id from a previous load on subsequent saves', async () => {
       const ds = mockDataSource({
         find: vi.fn().mockResolvedValue({
@@ -242,7 +265,6 @@ describe('createObjectStackUserStateAdapter', () => {
       expect(ds.create).toHaveBeenCalledTimes(1);
       expect(ds.update).toHaveBeenCalledWith('sys_user_preference', 'row-7', {
         value: [{ id: 'q' }],
-        updated_at: expect.any(String),
       });
     });
 
@@ -270,7 +292,6 @@ describe('createObjectStackUserStateAdapter', () => {
       expect(ds.create).toHaveBeenCalledTimes(1);
       expect(ds.update).toHaveBeenCalledWith('sys_user_preference', 'row-1', {
         value: [{ id: 'a' }, { id: 'b' }],
-        updated_at: expect.any(String),
       });
     });
 

packages/data-objectstack/src/userState.ts

@@ -159,13 +159,18 @@ export function createObjectStackUserStateAdapter<T = unknown>(
   // (a concurrent writer created the row, or the backend dropped our find
   // predicate), recover by re-finding and updating rather than surfacing the
   // failed insert.
+  //
+  // Deliberately does NOT stamp `updated_at`: the column is server-managed, and
+  // a non-system caller's write to it is stripped (framework #2948) and reported
+  // back as a dropped field — which the console surfaces as a "Some fields were
+  // not saved" toast (#3431). Sending it made every recents/favorites write pop
+  // a scary warning about a field the user never touched, drowning the real
+  // signal the toast exists for (#3794). The server stamps it either way.
   const upsert = async (items: T[]): Promise<void> => {
-    const now = new Date().toISOString();
-
     // Fast path: we already know the row id from a previous load/save.
     if (cachedRowId !== null) {
       try {
-        await dataSource.update(resource, cachedRowId, { value: items, updated_at: now });
+        await dataSource.update(resource, cachedRowId, { value: items });
         return;
       } catch (updateError) {
         // Row may have been deleted server-side — fall through to find/insert.
@@ -177,7 +182,7 @@ export function createObjectStackUserStateAdapter<T = unknown>(
     const existing = await findExisting();
     if (existing && existing.id !== undefined && existing.id !== null) {
       cachedRowId = existing.id;
-      await dataSource.update(resource, existing.id, { value: items, updated_at: now });
+      await dataSource.update(resource, existing.id, { value: items });
       return;
     }
 
@@ -186,7 +191,6 @@ export function createObjectStackUserStateAdapter<T = unknown>(
         user_id: userId,
         key,
         value: items,
-        updated_at: now,
       });
       const newId = (created as UserPreferenceRecord | undefined)?.id;
       if (newId !== undefined && newId !== null) cachedRowId = newId;
@@ -196,7 +200,7 @@ export function createObjectStackUserStateAdapter<T = unknown>(
       const recovered = await findExisting();
       if (recovered && recovered.id !== undefined && recovered.id !== null) {
         cachedRowId = recovered.id;
-        await dataSource.update(resource, recovered.id, { value: items, updated_at: now });
+        await dataSource.update(resource, recovered.id, { value: items });
         return;
       }
       throw createError;

packages/i18n/src/locales/ar.ts

@@ -792,11 +792,15 @@ const ar = {
     saving: "جارٍ الحفظ…",
     lockedByApproval: "مقفل للموافقة",
     lockedTooltip: "يحتوي هذا السجل على طلب موافقة معلق؛ التعديل مقفل",
+    writeStrippedTitle: "لم يتم حفظ بعض الحقول",
+    writeStrippedReadonly: "للقراءة فقط، لذلك لم يتم حفظها: {{fields}}",
+    writeStrippedByState: "غير قابلة للتعديل في الحالة الحالية لهذا السجل، لذلك لم يتم حفظها: {{fields}}",
     approvalPendingEditable: "قيد الموافقة · قابل للتعديل",
     approvalPendingTooltip: "يحتوي هذا السجل على طلب موافقة معلق، لكن هذه الخطوة لا تزال تسمح بالتعديل",
     cancelApproval: "إلغاء الموافقة",
     cancelApprovalInFlight: "جارٍ الإلغاء…",
     cancelApprovalTooltip: "إلغاء طلب الموافقة المعلق لفتح قفل السجل",
+    cancelApprovalTooltipUnlocked: "إلغاء طلب الموافقة المعلق",
     cancelApprovalFailed: "فشل إلغاء الموافقة",
     cancelApprovalUnavailable: "إلغاء الموافقات غير مدعوم من مصدر البيانات هذا",
     linkCopied: "تم نسخ الرابط إلى الحافظة",