Skip to content

Commit ea684a0

Browse files
fix(app-shell): a server error at an RLS clause's path holds the permission-set autosave (#27612)
<!-- claude-projects-attribution: {"github_login":"os-bill"} --> _Requested by **Bill** · [project thread](https://claude.ai/code/project/chan_016iMXWSZDzqWRWvWzdNo8wg?thread=cmsg_016iMXWSZDzqWRWvWzdNo8wg4i9JiKjEjwtuVzd4gKy9hB)_ ## 维护者速读 - 改了什么:Studio 权限集编辑器里,RLS 的 USING/CHECK 子句若被服务端草稿裁决判为发布时会拒绝(如 USING 读公式字段),现在会像 CEL 语法错误一样挡住自动保存,并在字段上显示服务端原话。 - 为什么改:之前服务端裁决只用于显示、不计入保存门,作者看着报错却一直 200 保存,到 Publish 才 422。 - 风险与回滚:只改 app-shell 三个文件;裁决只在「字段文本等于裁决所评判的文本」时挡保存,改掉文本即放行(下一次保存的重读带来新裁决),避免死锁。回滚即 revert 本 PR。 - 先看:`CelPredicateField.tsx` 的 `report` 与 `heldKey` 效果;`draftVerdict.ts` 的 `serverHoldAt`;`PermissionMatrixEditor.tsx` 里与 `servedVerdict` 同步设置的 `servedVerdictBody`。 - 两分钟验证:`pnpm --dir ui exec vitest run packages/app-shell/src/views/metadata-admin/PermissionMatrixEditor.serverVerdictHold-27482.test.tsx`,预期 4 条全绿。 - 需要你做的:无,席位落地。 Part of #27482 Clause-②: no Metadata-protocol: none This closes the RLS arm of the card. The Validations-tab arm is not done here and is listed under Acceptance notes, so the merge must not close the card. ## What changes - `draftVerdict.ts`: a second context, `DraftVerdictBodyContext`, carries the body the served verdict judged. `valueAtPath` reads a dotted `_diagnostics` path in that body, and `serverHoldAt` / `useServerHoldAt` answer the served errors at a field's path only while the field shows the text that body holds there. No rule is restated: the server's errors are taken whole. - `CelPredicateField.tsx`: a field with a `verdictPath` reports those held server errors through `onLintChange` as blocking errors, beside its lint findings. It re-reports when the hold starts, changes or lapses, keyed on the messages rather than on the array identity. Display is unchanged. - `PermissionMatrixEditor.tsx`: `servedVerdictBody` is set beside `servedVerdict` at each of its three reads (the load, the package door's post-save draft read, the environment door's post-save layered read) and provided to the RLS facets. The existing count, `celErrorCount`, now includes those errors, so the package door's autosave holds and the environment door's Save is disabled. ## Why The draft read's `_diagnostics` reached the RLS clause's display (#26858) but never `celErrorCount`, so a predicate the publish refuses (a USING over a formula field, `rls-predicate-unenforceable`) kept autosaving with the server's message on screen. Triage asked for exactly this: a server error at the field's own path counts in `celErrorCount`, with no second client copy of the publish rule. The verdict judges the stored draft and nothing else, and no door judges a text without storing it. Holding every edit while an error stands at the path would deadlock: the corrected clause could never be saved, so the next draft read could never clear it. So the hold applies while the clause still shows the judged text. Editing the clause away lets that one save go out, and its re-read brings the next verdict. ## Tests - `PermissionMatrixEditor.serverVerdictHold-27482.test.tsx` (new, 4 cases, fake door decides the refusal): (a) a stored refused draft holds another edit, no PUT, message stays; (b) the card flow: the refused clause saves once, its re-read holds the next edit; (c) editing the clause away releases, the save goes out and its clean re-read clears the verdict; (d) typing back to the judged text before the autosave fires is held again, no PUT. - `draftVerdict.test.ts`: four new cases for `valueAtPath` and `serverHoldAt` (judged text holds, other text does not, no body / no path / no error at that exact path holds nothing, an absent value reads as empty text). ## Acceptance notes - **File surface widened, seat-approved:** `ui/packages/app-shell/src/views/metadata-admin/draftVerdict.ts`, the home of the served-verdict helpers. `PermissionAdvancedFacets.tsx` is untouched: the held errors ride the existing `onLintChange` report. - **Validations tab, not done (deviation):** its hold counter exists (`ObjectValidationsPanel`'s per-rule map, reported through `onBlockingIssuesChange` to the Data pillar's Save gate), but no server verdict reaches the Data pillar at all. `DataPillar` in `StudioDesignSurface.tsx` reads the object draft through `readDraftForSave(client, 'object', …)` and keeps only `extractDraftBody`, dropping `_diagnostics`; it provides no `DraftVerdictContext`, and `ConditionBuilder` passes no `verdictPath` to its `CelPredicateField`. Wiring it is: keep the object draft read's `_diagnostics` and body in `DataPillar` (load and post-save), provide both contexts, and give `ConditionBuilder` a `verdictPath` per rule. The hold then applies with no further change to the field. The server's path spelling for a validation rule's condition is not measured here. That region belongs to a sibling card in flight, so it is left to a follow-up. - **The first save of a newly refused text still goes out.** Nothing on the client knows the refusal before the server stores and judges the text, and a client copy of the rule is ruled out. So in the card's step 5 the edit to another refused text saves once, and its re-read holds what follows. - **Status line wording:** the held line reads the existing `perm.cel.saveBlocked` ("Fix the CEL syntax errors before saving."), which also covers this server-verdict hold; the field itself shows the server's message. Rewording it is left alone (the studio namespace is in a sibling card's diff). - **Field-level `when` conditions** in the permission matrix pass no `verdictPath`, so they show and hold no server verdict, as before. ## Verification Readings name the commit they ran on. <details><summary>Tests</summary> - New and changed tests on 02780608: 2 files, 11/11 passed. - Neighbouring suites on bee1b7d (every metadata-admin test of PermissionMatrixEditor, CelPredicateField, draftVerdict, ConditionalFormatting, the ResourceEditPage verdict tests, and every ui test file that imports `CelPredicateField`, `draftVerdict` or `ConditionBuilder`): 92 files, 811/811 passed. - `pnpm --filter @object-ui/app-shell type-check`: exit 0. - Post-merge re-run on 48c334a (after `origin/main` merged, install and the app-shell dependency closure rebuilt): the same 92 files, 811/811 passed; `type-check` exit 0. </details> <details><summary>Ablations (each committed first, mutated through scripts/ablation-replace.mjs, restored with blob == HEAD and an empty git diff HEAD)</summary> 1. `CelPredicateField.tsx`, the report ignores the held server errors (the anchor `server.length === 0` made always true): cases (a), (b), (d) red, (c) green. Expected: (c) is the release path. 2. `draftVerdict.ts`, the hold ignores the text (the judged-text comparison replaced by a constant true): the unit case "holds on the judged text, and not on any other" red and integration case (c) red, the deadlock this design avoids. Everything else green. </details> <details><summary>Derived gates</summary> `node scripts/pm/dispatch-gates.mjs --commands` on this diff: 34 families, all run, all exit 0 (`--ran` reconciliation: 34 derived, 34 run, 0 NOT-MEASURED). `node scripts/check-sdui-manifest.mjs` exit 0. `pnpm check:new-line-citations` (ui): 0 new citations. Every family CI runs is read from this PR's check runs. </details> ## Human Minimum - [x] 1. Schemas start as Zod; `packages/spec` holds no runtime logic. (No spec change.) - [x] 2. Every name I added uses the casing for its kind. - [x] 3. No formatter ran; `pnpm lint` passes on the changed files (eslint, see the report). - [x] 4. One issue, one branch off an up-to-date `main`, one pull request. - [x] 5. A change that publishes carries a changeset, and both declaration lines are filled in here and in the changeset. - [x] 6. `packages/spec` touched: not touched. - [x] 7. No hand edits to `content/docs/references/`, `packages/*/CHANGELOG.md` or `content/docs/releases/`. - [x] 8. Title, body and comments are in English. --- _Generated by [Claude Code](https://claude.ai/code/session_01Utp6CaN9smHqkJcUWF4adR)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent e29a1b1 commit ea684a0

6 files changed

Lines changed: 371 additions & 33 deletions

File tree

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
---
2+
'@object-ui/app-shell': patch
3+
---
4+
5+
fix(app-shell): a server error at an RLS clause's path holds the permission-set autosave
6+
7+
Clause-②: no
8+
Metadata-protocol: none
9+
10+
In Studio's permission-set editor, an RLS `using` or `check` clause that the publish will refuse (for example a `using` that reads a formula field, refused as `rls-predicate-unenforceable`) showed the server's message on the field but kept autosaving: the message was displayed, and the save gate never counted it. Now the server's errors at the clause's own path hold the autosave, the same way a CEL syntax error does, while the clause still shows the text that verdict judged.
11+
12+
The verdict describes the stored draft and nothing else, and no door judges a text without saving it. So an edit that changes the clause's text is not held: it saves, and that save's re-read brings the next verdict, which holds or releases from there. The editor decides nothing itself: it takes the server's errors as served and restates no publish rule.

‎ui/packages/app-shell/src/views/metadata-admin/CelPredicateField.tsx‎

Lines changed: 44 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,7 @@ import {
5454
type CelSuggestionKind,
5555
type CelValueType,
5656
} from './celAuthoring.js';
57-
import { useServerErrorsAt } from './draftVerdict.js';
57+
import { useServerErrorsAt, useServerHoldAt, type ServerDiagnosticEntry } from './draftVerdict.js';
5858

5959
const LINT_DEBOUNCE_MS = 250;
6060

@@ -124,11 +124,21 @@ export interface CelPredicateFieldProps {
124124
* the page holds (`rowLevelSecurity.0.using`). When the page's served verdict
125125
* (`DraftVerdictContext`) has an error at exactly that path — what publish
126126
* will refuse — the editor shows the server's message and withholds "Valid
127-
* CEL". The lint, its findings and `onLintChange` are unaffected, as with
128-
* `scopeIssue`. Omitted: no server verdict is read.
127+
* CEL". The lint and its findings are unaffected, as with `scopeIssue`.
128+
* Omitted: no server verdict is read.
129+
*
130+
* objectstack#27482 — and while the text on screen is the text that verdict
131+
* judged (`DraftVerdictBodyContext`), each of those errors is also reported
132+
* through `onLintChange` as a blocking error, so the host's Save gate holds
133+
* on what publish will refuse exactly as on a parse fault. Once the author
134+
* edits the text away the verdict no longer describes it and holds nothing:
135+
* the save goes out, and its re-read brings the next verdict.
129136
*/
130137
verdictPath?: string;
131-
/** Reports the current lint issues up so the editor can gate Save on errors. */
138+
/**
139+
* Reports the current lint issues up so the editor can gate Save on errors —
140+
* plus the server's errors at {@link verdictPath} on the judged text.
141+
*/
132142
onLintChange?: (issues: CelLintIssue[]) => void;
133143
/**
134144
* `role="value"` only: reports the inferred result type after each
@@ -206,14 +216,42 @@ export function CelPredicateField({
206216
const fieldsKey = fieldNames.join(',');
207217
const rootsKey = roots ? roots.join(',') : '';
208218

219+
// objectstack#27482 — the server's errors at this editor's path while the
220+
// text on screen is the text they judged; they ride the lint report up.
221+
const held = useServerHoldAt(verdictPath, value);
222+
const heldKey = held.map((e) => e.message).join('\n');
223+
209224
// Keep the reporters out of the lint effect deps (parent may not memoize them).
210225
const onLintChangeRef = React.useRef(onLintChange);
211226
const onInferredTypeChangeRef = React.useRef(onInferredTypeChange);
227+
const heldRef = React.useRef<readonly ServerDiagnosticEntry[]>(held);
212228
React.useEffect(() => {
213229
onLintChangeRef.current = onLintChange;
214230
onInferredTypeChangeRef.current = onInferredTypeChange;
231+
heldRef.current = held;
215232
});
216233

234+
// The last lint result, so a verdict that arrives or lapses between two
235+
// lint passes is reported against it.
236+
const lintRef = React.useRef<CelLintIssue[]>([]);
237+
const report = React.useCallback((lint: CelLintIssue[]) => {
238+
lintRef.current = lint;
239+
const server = heldRef.current;
240+
onLintChangeRef.current?.(
241+
server.length === 0
242+
? lint
243+
: [...lint, ...server.map((e): CelLintIssue => ({ severity: 'error', message: e.message }))],
244+
);
245+
}, []);
246+
// Keyed on the messages, not on the array (rebuilt every render): report
247+
// when the hold starts, changes or lapses, never on mount with none.
248+
const reportedHeldRef = React.useRef('');
249+
React.useEffect(() => {
250+
if (heldKey === reportedHeldRef.current) return;
251+
reportedHeldRef.current = heldKey;
252+
report(lintRef.current);
253+
}, [heldKey, report]);
254+
217255
/* Debounced lint (and, for role="value", result-type inference). */
218256
React.useEffect(() => {
219257
let cancelled = false;
@@ -223,7 +261,7 @@ export function CelPredicateField({
223261
if (cancelled) return;
224262
setIssues(res);
225263
setLinted(true);
226-
onLintChangeRef.current?.(res);
264+
report(res);
227265
});
228266
if (role === 'value') {
229267
inferCelValueType(value, hint).then((res) => {
@@ -246,7 +284,7 @@ export function CelPredicateField({
246284
setIssues([]);
247285
setLinted(false);
248286
setInferredType(null);
249-
onLintChangeRef.current?.([]);
287+
report([]);
250288
if (role === 'value') onInferredTypeChangeRef.current?.(null);
251289
}
252290
// eslint-disable-next-line react-hooks/exhaustive-deps
Lines changed: 174 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,174 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* objectstack#27482 — a server error at an RLS clause's own path holds the
5+
* package door's autosave, the way a CEL parse fault does.
6+
*
7+
* objectstack#26858 made the clause SHOW the draft read's verdict; it never
8+
* reached `celErrorCount`, so an expression the publish refuses (a USING that
9+
* reads a formula field) kept autosaving with the server's message on screen.
10+
* The verdict judges the stored draft and nothing else, so it holds while the
11+
* clause still shows the text it judged, and lets the edit that changes that
12+
* text go out: only that save brings the next verdict. ⛔ The fake door below
13+
* is the only place the refusal is decided; the editor restates no rule.
14+
*/
15+
16+
import '@testing-library/jest-dom/vitest';
17+
import { afterEach, describe, expect, it, vi } from 'vitest';
18+
import { cleanup, fireEvent, render, screen, waitFor } from '@testing-library/react';
19+
import { MemoryRouter } from 'react-router-dom';
20+
21+
const REFUSED_USING = 'f_total > 5';
22+
const ACCEPTED_USING = 'owner_id == current_user.id';
23+
const REFUSAL = "rowLevelSecurity 'hot_deals' using reads 'f_total', a formula field the database cannot filter on";
24+
25+
const SET = {
26+
name: 'sales_perms',
27+
label: 'Sales',
28+
objects: { a_account: { allowRead: true } },
29+
fields: {},
30+
};
31+
32+
interface Server {
33+
draft: Record<string, unknown> | null;
34+
draftReads: number;
35+
/** Every body a save sent. */
36+
sent: Array<Record<string, unknown>>;
37+
}
38+
39+
/** The door's publish verdict on a stored draft: it refuses the one USING clause. */
40+
function verdictOf(body: Record<string, unknown>) {
41+
const rows = (body.rowLevelSecurity ?? []) as Array<{ using?: string }>;
42+
const errors = rows.flatMap((r, i) =>
43+
r.using === REFUSED_USING
44+
? [{ path: `rowLevelSecurity.${i}.using`, message: REFUSAL, code: 'rls-predicate-unenforceable' }]
45+
: [],
46+
);
47+
return errors.length ? { valid: false, errors } : { valid: true };
48+
}
49+
50+
// `withPreviewDrafts` answers the client itself, not a promise (objectstack#26840).
51+
type FakeClient = Record<string, (...args: never[]) => unknown>;
52+
53+
function makeClient(server: Server): FakeClient {
54+
return {
55+
layered: async () => ({ effective: SET, code: null, overlay: null, overlayScope: null }),
56+
getDraft: async () => {
57+
server.draftReads += 1;
58+
return server.draft
59+
? { type: 'permission', name: 'sales_perms', item: { ...server.draft, _draft: true, _diagnostics: verdictOf(server.draft) } }
60+
: null;
61+
},
62+
withPreviewDrafts() { return this; },
63+
list: async (type: string) => (type === 'object' ? [{ item: { name: 'a_account' } }] : []),
64+
get: async () => null,
65+
save: async (_type: string, _name: string, payload: Record<string, unknown>) => {
66+
const body = JSON.parse(JSON.stringify(payload)) as Record<string, unknown>;
67+
server.sent.push(body);
68+
server.draft = body;
69+
return payload;
70+
},
71+
};
72+
}
73+
74+
let clientImpl: FakeClient;
75+
76+
vi.mock('./useMetadata', () => ({
77+
useMetadataClient: () => clientImpl,
78+
useMetadataTypes: () => ({
79+
loading: false,
80+
error: null,
81+
entries: [{ type: 'permission', label: 'Permission', allowEnvironmentOverlay: true }],
82+
}),
83+
}));
84+
85+
vi.mock('./AssignedUsersSection', () => ({ AssignedUsersSection: () => null }));
86+
87+
import { PermissionMatrixEditPage } from './PermissionMatrixEditor';
88+
89+
afterEach(cleanup);
90+
91+
const USING_PLACEHOLDER = 'organization_id == current_user.organization_id';
92+
const CHECK_PLACEHOLDER = 'optional — defaults to USING';
93+
/** The shared autosave fires 1.5s after the last edit. */
94+
const AUTOSAVE = { timeout: 5000 };
95+
/** Longer than the autosave's pause. */
96+
const PAST_THE_PAUSE = 2500;
97+
const pause = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms));
98+
99+
const policy = (using: string) => ({
100+
...SET,
101+
rowLevelSecurity: [{ name: 'hot_deals', object: 'a_account', operation: 'select', using, enabled: true }],
102+
});
103+
104+
async function openPackageDoor(server: Server) {
105+
clientImpl = makeClient(server);
106+
render(
107+
<MemoryRouter>
108+
<PermissionMatrixEditPage type="permission" name="sales_perms" packageId="app.a" />
109+
</MemoryRouter>,
110+
);
111+
await screen.findByText('a_account');
112+
fireEvent.click(screen.getByRole('button', { name: /Row-Level Security/ }));
113+
}
114+
115+
const edit = (placeholder: string, value: string) =>
116+
fireEvent.change(screen.getByPlaceholderText(placeholder), { target: { value } });
117+
118+
describe('a server error at a clause path holds the autosave (objectstack#27482)', () => {
119+
it('(a) a stored draft the publish refuses: another edit is held, no PUT, the server message stays', async () => {
120+
const server: Server = { draft: policy(REFUSED_USING), draftReads: 0, sent: [] };
121+
await openPackageDoor(server);
122+
await screen.findByText(REFUSAL);
123+
124+
edit(CHECK_PLACEHOLDER, ACCEPTED_USING);
125+
126+
await waitFor(() => expect(screen.getByTestId('perm-autosave-held')).toBeInTheDocument(), AUTOSAVE);
127+
await pause(PAST_THE_PAUSE);
128+
expect(server.sent).toHaveLength(0);
129+
expect(screen.getByText(REFUSAL)).toBeInTheDocument();
130+
});
131+
132+
it('(b) the card flow: the refused clause saves once, its re-read holds the next edit', async () => {
133+
const server: Server = { draft: policy(ACCEPTED_USING), draftReads: 0, sent: [] };
134+
await openPackageDoor(server);
135+
await screen.findByText('Valid CEL', {}, { timeout: 3000 });
136+
137+
// No client copy of the rule: the lint passes it and the first save goes out.
138+
edit(USING_PLACEHOLDER, REFUSED_USING);
139+
await waitFor(() => expect(server.sent).toHaveLength(1), AUTOSAVE);
140+
expect(await screen.findByText(REFUSAL, {}, AUTOSAVE)).toBeInTheDocument();
141+
142+
edit(CHECK_PLACEHOLDER, ACCEPTED_USING);
143+
await waitFor(() => expect(screen.getByTestId('perm-autosave-held')).toBeInTheDocument(), AUTOSAVE);
144+
await pause(PAST_THE_PAUSE);
145+
expect(server.sent).toHaveLength(1);
146+
});
147+
148+
it('(c) editing the clause away releases: the save goes out and its clean re-read clears the verdict', async () => {
149+
const server: Server = { draft: policy(REFUSED_USING), draftReads: 0, sent: [] };
150+
await openPackageDoor(server);
151+
await screen.findByText(REFUSAL);
152+
153+
edit(USING_PLACEHOLDER, ACCEPTED_USING);
154+
155+
await waitFor(() => expect(server.sent).toHaveLength(1), AUTOSAVE);
156+
await waitFor(() => expect(screen.queryByText(REFUSAL)).toBeNull(), AUTOSAVE);
157+
expect(await screen.findByTestId('perm-saved-at', {}, AUTOSAVE)).toBeInTheDocument();
158+
expect(screen.queryByTestId('perm-autosave-held')).toBeNull();
159+
});
160+
161+
it('(d) typed back to the judged text before the autosave fires: held again, no PUT', async () => {
162+
const server: Server = { draft: policy(REFUSED_USING), draftReads: 0, sent: [] };
163+
await openPackageDoor(server);
164+
await screen.findByText(REFUSAL);
165+
166+
edit(CHECK_PLACEHOLDER, ACCEPTED_USING);
167+
edit(USING_PLACEHOLDER, 'f_total > 50');
168+
edit(USING_PLACEHOLDER, REFUSED_USING);
169+
170+
await waitFor(() => expect(screen.getByTestId('perm-autosave-held')).toBeInTheDocument(), AUTOSAVE);
171+
await pause(PAST_THE_PAUSE);
172+
expect(server.sent).toHaveLength(0);
173+
});
174+
});

0 commit comments

Comments
 (0)