Skip to content

Commit 1cf0de7

Browse files
baozhoutaoclaudeos-zhuang
authored
fix(detail): distinguish "in approval (editable)" from locked, and stop losing write warnings (#2914)
* fix(detail): distinguish "in approval (editable)" from locked, and stop losing write warnings (framework#3794) The record page told the user the opposite of what the backend does, in both directions. The approval band rendered "Locked for approval" on any pending request, ignoring the node's `lockRecord`. A `lockRecord: false` node is pending WITHOUT locking — that is how an approver amends a record while deciding on it — so the band hid the feature; meanwhile the header Edit button stayed enabled on a genuinely locked record, letting the user fill a form the save would reject with RECORD_LOCKED. `useRecordApprovals` now reads `locks_record` off the pending request (framework#3794), `RecordDetailView` derives `approvalPending` and `approvalLocked` separately and threads both through `InlineEditProvider`, and the band picks the amber lock or a neutral "In approval (editable)" from the host's signals. With no approvals-aware host the `approval_status` fallback is unchanged and still assumes a lock. The write-warning toast (framework #3431/#3455) never fired on `batchTransaction`, which is the record form's save path for a master-detail record — precisely where a user edits a `readonlyWhen`-locked field. It now emits per event, resolving each back to its operation via the response `index`. The toast is localized (en + zh) and worded by reason: a `readonly_when` strip says the field is not editable in this record's current state, not that it is read-only. Finally, the user-state adapter no longer hand-stamps the server-managed `updated_at`, which the server strips and reports — that fired a "Some fields were not saved" warning about a field no user touched, on ordinary page loads. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(i18n): backfill the new detail keys into all ten locale packs (framework#3794) The full ten-pack parity ratchet (#2872) landed after this branch was cut, so the six new `detail.*` keys existed only in en/zh. Translated into ja, ko, de, fr, es, pt, ru and ar. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com>
1 parent 9867281 commit 1cf0de7

20 files changed

Lines changed: 326 additions & 31 deletions

File tree

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

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

Lines changed: 52 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -7,29 +7,59 @@
77
* @module
88
*/
99

10-
import { useState, useEffect, type ReactNode } from 'react';
10+
import { useState, useEffect, useRef, type ReactNode } from 'react';
1111
import { toast } from 'sonner';
1212
import { ObjectStackAdapter, type WriteWarningEvent } from '@object-ui/data-objectstack';
1313
import { createAuthenticatedFetch } from '@object-ui/auth';
1414
import { AdapterCtx } from '@object-ui/react';
15+
import { useObjectTranslation } from '@object-ui/i18n';
1516
import { installSettleSignalGlobal, withSettleSignal } from '../observability/settleSignal';
1617

1718
export { useAdapter } from '@object-ui/react';
1819

20+
type TranslateFn = (key: string, options?: Record<string, unknown>) => string;
21+
1922
/**
2023
* Turn a write-warning (framework #3431/#3455) into a human toast. The write
21-
* SUCCEEDED — some caller-supplied fields were legally stripped (read-only /
22-
* locked by state), so we tell the user rather than let it pass silently.
24+
* SUCCEEDED — some caller-supplied fields were legally stripped, so we tell the
25+
* user rather than let it pass silently.
26+
*
27+
* The REASON decides the wording (#3794). `readonly_when` is not "this field is
28+
* read-only" — the field is editable in other states and the form rendered it
29+
* as an ordinary input; what happened is that THIS record's current state locks
30+
* it. Saying "read-only" there sends the user looking for a permission problem
31+
* that doesn't exist. Fields are named by their machine name: the event carries
32+
* no labels, and a wrong guess at a label is worse than an exact key.
2333
*/
24-
function toastWriteWarning(ev: WriteWarningEvent): void {
25-
const fields = Array.from(new Set(ev.droppedFields.flatMap((d) => d.fields)));
26-
if (fields.length === 0) return;
27-
const list = fields.join(', ');
28-
const description =
29-
fields.length === 1
30-
? `The read-only field “${list}” could not be changed and was not saved.`
31-
: `${fields.length} read-only fields could not be changed and were not saved: ${list}.`;
32-
toast.warning('Some fields were not saved', { description });
34+
function toastWriteWarning(ev: WriteWarningEvent, t: TranslateFn): void {
35+
const byReason = new Map<string, string[]>();
36+
for (const d of ev.droppedFields) {
37+
const seen = byReason.get(d.reason) ?? [];
38+
for (const f of d.fields) if (!seen.includes(f)) seen.push(f);
39+
byReason.set(d.reason, seen);
40+
}
41+
const lines: string[] = [];
42+
for (const [reason, fields] of byReason) {
43+
if (fields.length === 0) continue;
44+
const list = fields.join(', ');
45+
lines.push(
46+
reason === 'readonly_when'
47+
? t('detail.writeStrippedByState', {
48+
fields: list,
49+
defaultValue:
50+
'Not editable in this record\'s current state, so it was not saved: {{fields}}',
51+
})
52+
: t('detail.writeStrippedReadonly', {
53+
fields: list,
54+
defaultValue: 'Read-only, so it was not saved: {{fields}}',
55+
}),
56+
);
57+
}
58+
if (lines.length === 0) return;
59+
toast.warning(
60+
t('detail.writeStrippedTitle', { defaultValue: 'Some fields were not saved' }),
61+
{ description: lines.join('\n') },
62+
);
3363
}
3464

3565
interface AdapterProviderProps {
@@ -44,6 +74,13 @@ interface AdapterProviderProps {
4474
*/
4575
export function AdapterProvider({ children, adapter: externalAdapter }: AdapterProviderProps) {
4676
const [adapter, setAdapter] = useState<ObjectStackAdapter | null>(externalAdapter ?? null);
77+
// The warning listener is registered ONCE (the adapter outlives a language
78+
// switch), so read `t` through a ref instead of capturing it — otherwise a
79+
// user who switches language mid-session keeps getting the old locale, and
80+
// adding `t` to the effect deps would tear down and rebuild the adapter.
81+
const { t } = useObjectTranslation();
82+
const tRef = useRef(t);
83+
tRef.current = t;
4784

4885
useEffect(() => {
4986
if (externalAdapter) {
@@ -72,7 +109,9 @@ export function AdapterProvider({ children, adapter: externalAdapter }: AdapterP
72109

73110
// Surface silently-stripped write fields (#3431/#3455) as a toast so a
74111
// read-only value the user typed doesn't just vanish on save.
75-
unsubscribeWriteWarning = a.onWriteWarning(toastWriteWarning);
112+
unsubscribeWriteWarning = a.onWriteWarning((ev) =>
113+
toastWriteWarning(ev, tRef.current as TranslateFn),
114+
);
76115

77116
await a.connect();
78117

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

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

packages/data-objectstack/src/index.ts

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -973,6 +973,41 @@ export class ObjectStackAdapter<T = unknown> implements DataSource<T> {
973973
this.emitWriteWarning({ operation, resource, ...(id !== undefined ? { id } : {}), droppedFields });
974974
}
975975

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

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

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,56 @@ describe('ObjectStackAdapter.onWriteWarning', () => {
9494
expect(events).toEqual([]);
9595
});
9696

97+
// ── cross-object batch (framework #3794) ─────────────────────────────────
98+
//
99+
// `batchTransaction` is the console record form's save path for a
100+
// master-detail record, so this is where a `readonlyWhen` strip actually
101+
// reaches a user editing a form. Its response tags each event with the index
102+
// of the operation that produced it.
103+
104+
it('emits a write-warning per event on a cross-object batch, resolving the op', async () => {
105+
const batchTransaction = vi.fn().mockResolvedValue({
106+
results: [{ id: 'acc1' }, { id: 'inv1' }],
107+
droppedFields: [
108+
{ object: 'invoice', fields: ['tax_rate'], reason: 'readonly_when', index: 1 },
109+
],
110+
});
111+
const ds = makeDS({ batchTransaction });
112+
ds.atomicBatchCapability = true;
113+
const events: WriteWarningEvent[] = [];
114+
ds.onWriteWarning((e: WriteWarningEvent) => events.push(e));
115+
116+
await ds.batchTransaction([
117+
{ object: 'account', action: 'create', data: { name: 'Acme' } },
118+
{ object: 'invoice', action: 'update', id: 'inv1', data: { status: 'paid', tax_rate: 9 } },
119+
]);
120+
121+
expect(events).toEqual([
122+
{
123+
operation: 'update',
124+
resource: 'invoice',
125+
id: 'inv1',
126+
droppedFields: [{ object: 'invoice', fields: ['tax_rate'], reason: 'readonly_when' }],
127+
},
128+
]);
129+
});
130+
131+
it('does NOT emit for a clean batch or a malformed droppedFields list', async () => {
132+
const batchTransaction = vi
133+
.fn()
134+
.mockResolvedValueOnce({ results: [{ id: 'a' }] })
135+
.mockResolvedValueOnce({ results: [{ id: 'a' }], droppedFields: [{ object: 'x', fields: [], index: 0 }] });
136+
const ds = makeDS({ batchTransaction });
137+
ds.atomicBatchCapability = true;
138+
const events: WriteWarningEvent[] = [];
139+
ds.onWriteWarning((e: WriteWarningEvent) => events.push(e));
140+
141+
await ds.batchTransaction([{ object: 'x', action: 'create', data: {} }]);
142+
await ds.batchTransaction([{ object: 'x', action: 'create', data: {} }]);
143+
144+
expect(events).toEqual([]);
145+
});
146+
97147
it('stops delivering after unsubscribe', async () => {
98148
const create = vi.fn().mockResolvedValue({ record: { id: 'r1' }, droppedFields: ONE_DROP });
99149
const ds = makeDS({ create });

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

Lines changed: 25 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -144,7 +144,6 @@ describe('createObjectStackUserStateAdapter', () => {
144144
user_id: 'u1',
145145
key: 'ui.favorites',
146146
value: [{ id: 'a' }],
147-
updated_at: expect.any(String),
148147
});
149148
});
150149

@@ -165,11 +164,35 @@ describe('createObjectStackUserStateAdapter', () => {
165164

166165
expect(ds.update).toHaveBeenCalledWith('sys_user_preference', 'row-99', {
167166
value: [{ id: 'x' }],
168-
updated_at: expect.any(String),
169167
});
170168
expect(ds.create).not.toHaveBeenCalled();
171169
});
172170

171+
// #3794 — `updated_at` is server-managed. A caller-supplied value is
172+
// stripped (framework #2948) and reported back as a dropped field, which
173+
// the console turns into a "Some fields were not saved" toast. Stamping it
174+
// here fired that warning on every recents/favorites write, about a field
175+
// no user ever touched — noise on the exact surface that is supposed to
176+
// tell a user their edit did not land.
177+
it('never sends the server-managed updated_at column', async () => {
178+
const ds = mockDataSource({
179+
find: vi.fn().mockResolvedValue({ data: [] }) as any,
180+
create: vi.fn().mockResolvedValue({ id: 'row-1' }) as any,
181+
});
182+
const adapter = createObjectStackUserStateAdapter({
183+
dataSource: ds,
184+
userId: 'u',
185+
key: 'ui.recent',
186+
});
187+
188+
await adapter.save([{ id: 'a' } as any]);
189+
await adapter.save([{ id: 'b' } as any]);
190+
191+
for (const call of [...(ds.create as any).mock.calls, ...(ds.update as any).mock.calls]) {
192+
expect(call[call.length - 1]).not.toHaveProperty('updated_at');
193+
}
194+
});
195+
173196
it('uses the cached row id from a previous load on subsequent saves', async () => {
174197
const ds = mockDataSource({
175198
find: vi.fn().mockResolvedValue({
@@ -242,7 +265,6 @@ describe('createObjectStackUserStateAdapter', () => {
242265
expect(ds.create).toHaveBeenCalledTimes(1);
243266
expect(ds.update).toHaveBeenCalledWith('sys_user_preference', 'row-7', {
244267
value: [{ id: 'q' }],
245-
updated_at: expect.any(String),
246268
});
247269
});
248270

@@ -270,7 +292,6 @@ describe('createObjectStackUserStateAdapter', () => {
270292
expect(ds.create).toHaveBeenCalledTimes(1);
271293
expect(ds.update).toHaveBeenCalledWith('sys_user_preference', 'row-1', {
272294
value: [{ id: 'a' }, { id: 'b' }],
273-
updated_at: expect.any(String),
274295
});
275296
});
276297

packages/data-objectstack/src/userState.ts

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -159,13 +159,18 @@ export function createObjectStackUserStateAdapter<T = unknown>(
159159
// (a concurrent writer created the row, or the backend dropped our find
160160
// predicate), recover by re-finding and updating rather than surfacing the
161161
// failed insert.
162+
//
163+
// Deliberately does NOT stamp `updated_at`: the column is server-managed, and
164+
// a non-system caller's write to it is stripped (framework #2948) and reported
165+
// back as a dropped field — which the console surfaces as a "Some fields were
166+
// not saved" toast (#3431). Sending it made every recents/favorites write pop
167+
// a scary warning about a field the user never touched, drowning the real
168+
// signal the toast exists for (#3794). The server stamps it either way.
162169
const upsert = async (items: T[]): Promise<void> => {
163-
const now = new Date().toISOString();
164-
165170
// Fast path: we already know the row id from a previous load/save.
166171
if (cachedRowId !== null) {
167172
try {
168-
await dataSource.update(resource, cachedRowId, { value: items, updated_at: now });
173+
await dataSource.update(resource, cachedRowId, { value: items });
169174
return;
170175
} catch (updateError) {
171176
// Row may have been deleted server-side — fall through to find/insert.
@@ -177,7 +182,7 @@ export function createObjectStackUserStateAdapter<T = unknown>(
177182
const existing = await findExisting();
178183
if (existing && existing.id !== undefined && existing.id !== null) {
179184
cachedRowId = existing.id;
180-
await dataSource.update(resource, existing.id, { value: items, updated_at: now });
185+
await dataSource.update(resource, existing.id, { value: items });
181186
return;
182187
}
183188

@@ -186,7 +191,6 @@ export function createObjectStackUserStateAdapter<T = unknown>(
186191
user_id: userId,
187192
key,
188193
value: items,
189-
updated_at: now,
190194
});
191195
const newId = (created as UserPreferenceRecord | undefined)?.id;
192196
if (newId !== undefined && newId !== null) cachedRowId = newId;
@@ -196,7 +200,7 @@ export function createObjectStackUserStateAdapter<T = unknown>(
196200
const recovered = await findExisting();
197201
if (recovered && recovered.id !== undefined && recovered.id !== null) {
198202
cachedRowId = recovered.id;
199-
await dataSource.update(resource, recovered.id, { value: items, updated_at: now });
203+
await dataSource.update(resource, recovered.id, { value: items });
200204
return;
201205
}
202206
throw createError;

packages/i18n/src/locales/ar.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -792,11 +792,15 @@ const ar = {
792792
saving: "جارٍ الحفظ…",
793793
lockedByApproval: "مقفل للموافقة",
794794
lockedTooltip: "يحتوي هذا السجل على طلب موافقة معلق؛ التعديل مقفل",
795+
writeStrippedTitle: "لم يتم حفظ بعض الحقول",
796+
writeStrippedReadonly: "للقراءة فقط، لذلك لم يتم حفظها: {{fields}}",
797+
writeStrippedByState: "غير قابلة للتعديل في الحالة الحالية لهذا السجل، لذلك لم يتم حفظها: {{fields}}",
795798
approvalPendingEditable: "قيد الموافقة · قابل للتعديل",
796799
approvalPendingTooltip: "يحتوي هذا السجل على طلب موافقة معلق، لكن هذه الخطوة لا تزال تسمح بالتعديل",
797800
cancelApproval: "إلغاء الموافقة",
798801
cancelApprovalInFlight: "جارٍ الإلغاء…",
799802
cancelApprovalTooltip: "إلغاء طلب الموافقة المعلق لفتح قفل السجل",
803+
cancelApprovalTooltipUnlocked: "إلغاء طلب الموافقة المعلق",
800804
cancelApprovalFailed: "فشل إلغاء الموافقة",
801805
cancelApprovalUnavailable: "إلغاء الموافقات غير مدعوم من مصدر البيانات هذا",
802806
linkCopied: "تم نسخ الرابط إلى الحافظة",

0 commit comments

Comments
 (0)