Skip to content

Commit 40fa433

Browse files
fix(app-shell): publish review sheet lists no Options change on a picklist-bound field nobody edited (#29949)
<!-- claude-projects-attribution: {"github_login":"os-support-ai"} --> _Requested by **Yibo Wang** · [project thread](https://claude.ai/code/project/chan_01BVUyyPie8gHwoPM18iABwB?thread=cmsg_01BVUyyPie8gHwoPM18iABwB4xjiquevJqfBXiUoqfQwa1)_ Fixes #29899 ## 维护者速读 - **改了什么:** Studio 发布审阅面板在比较对象的已发布版和草稿时,把绑定了 picklist 的字段都按"存储形"(不带 `options`)来比,不再把服务端解析出来的选项当成作者改动。 - **为什么改:** 只要对象里有绑定 picklist 的字段,每次发布面板都会多出一行"~ Tier · Options",作者根本没改过,面板失去可信度,真改了也分辨不出来。 - **风险与回滚:** 只动 `@object-ui/app-shell` 的面板 diff,不改任何写入、协议或存储;只影响声明了 `picklist` 的字段的 `options` 比较。回滚即 revert 本 PR。 - **先看:** `ui/packages/app-shell/src/preview/DraftChangesPanel.tsx` 的 `computeChangeDetail` 开头的 `stored(...)` 两行;新测试 `DraftChangesPanel.picklistServedOptions-29899.test.tsx`。 - **两分钟验证:** `pnpm --dir ui exec vitest run packages/app-shell/src/preview/__tests__/DraftChangesPanel.picklistServedOptions-29899.test.tsx`,预期 3 passed。 - **需要你做的:** 无,席位落地。 Clause-②: no Metadata-protocol: none ## What changes `computeChangeDetail` (the diff behind Studio's publish review sheet) now reads both versions of an `object` in the form the publish door **stores** a picklist-bound field: without `options`. It does so by passing each envelope-free body through `dropServedPicklistOptions` from `@object-ui/data-objectstack`, the function the Studio and Setup writers already use to turn a served object read back into the authored form before a PUT (objectui#10202, objectui#11692). The step runs only when the entry's type is `object`. The panel already imports from `@object-ui/data-objectstack` (`readDraftChanges`), so the console's eager graph gains no new module. ### Why "compare like with like (both stored)" and not "skip `options` in the diff" The card offered both. Normalizing to the stored form keeps every real change visible; skipping a key would hide some. - `dropServedPicklistOptions` touches a field only when **that definition** names a picklist (`picklist !== undefined`, the `FieldSchema` refinement's own test). A field with inline `options` is returned by reference, so a real options change there is still listed. The CONTROL test proves it. - A field moving between a picklist and inline options still lists **both** `picklist` and `options`, in either direction, because the side with inline options keeps them. A diff-level "ignore `options` when the field declares `picklist`" rule would hide the `options` half of that move. It would also have to choose which side's `picklist` counts. - The authoring door refuses `picklist` and `options` together, so the stored form of a bound field never carries `options`. Normalizing to it loses nothing an author could have written. ### Mechanism, measured against `origin/main` at `21510bba` The card's PM assumptions hold: - **The live read carries the resolved options; the draft read does not.** `readMetaItem` (`packages/metadata-protocol/src/protocol-meta-item-reads.ts`) returns early for `state: 'draft'` ("Draft reads stop here"). It serves the converted draft row through `governServedObject` and never reaches the object fold. The active read folds the object through `foldObjectExtendersFromRegistry` → `SchemaRegistry.foldObjectExtendersOnto` → `finishServedObject` → `resolvePicklistFieldsOnto` (`packages/objectql/src/picklist-resolution.ts`). That last step writes the resolved `options` onto each bound field and keeps `picklist`. - **No path found where the draft itself carries resolved options.** Studio's writers drop them before a save (`dropServedPicklistOptions`), and the authoring door refuses a body that carries both keys. A body that did carry both would also be normalized here on both sides. - **The fix is not server-side.** Serving the resolved options on a bound field is the declared contract (`PicklistServedFieldSchema`, `@objectstack/spec` `data/picklist.zod.ts`): every object read serves it, and the write door judges values by the same served field. The draft read serving the stored row is ADR-0005's draft lifecycle. Neither read is wrong. The diff was comparing two forms, so the ui lane owns the fix. ## Tests New file `ui/packages/app-shell/src/preview/__tests__/DraftChangesPanel.picklistServedOptions-29899.test.tsx`. It renders the real `DraftChangesPanel` against routes where the published read serves `tier` with resolved options and the draft read serves it stored: 1. A picklist-bound field `tier`, and a draft that changes only `notes.label`: the drill-in renders exactly one row, `~ Again notes notes · Label`, and no `Options`. 2. **CONTROL:** an inline-options select `status` whose options really changed, beside the same bound `tier`: exactly one row, `~ Status status · Options`. 3. `computeChangeDetail` direct: a bound field reads unchanged; bound → inline and inline → bound each list both `picklist` and `options`; the array `fields` shape reads the same. <details><summary>Ablation (fix removed → new tests red → restored)</summary> Fix committed first (`f4d64a21`). Then `node scripts/ablation-replace.mjs` swapped the anchor `type === 'object' ? dropServedPicklistOptions(body) : body;` for `body; // ABLATION-29899` and ran the new file under the verify lock. The test imports the panel by relative path from `src`, so no `dist` rebuild sits between the mutation and the run. ``` ablation-replace: ok mutation landed: anchor 1 -> 0, blob 694ec68 -> eb00fb8b831c × lists only the label change when the draft edits another field of an object with a bound field × CONTROL: still lists an Options change on a field with inline options × reads a bound field as unchanged in computeChangeDetail, and keeps a move to or from inline options AssertionError: expected [ '~ Again notes notes · Label', …(1) ] to have a length of 1 but got 2 AssertionError: expected [ '~ Status status · Options', …(1) ] to have a length of 1 but got 2 AssertionError: expected [ { name: 'tier', …(1) } ] to deeply equal [] Test Files 1 failed (1) Tests 3 failed (3) ablation-replace: ok restored: blob == HEAD (694ec68) and `git diff HEAD` is empty ``` Without the fix, case 1 renders a second row beside the label row, and case 3 lists `tier` as changed. The CONTROL fails only on its row count: its own `Options` row is still there first, and one more row follows it. </details> ## Acceptance notes - **Scope:** this changes only what the review sheet **displays**. The publish door's own no-op test (content hashes of the stored bodies) never read served options and is unchanged. - **`type` gate:** only `object` bodies are normalized. Its one caller passes `entry.type`. The served picklist form is a property of object reads. - No out-of-scope findings. ## Verification All runs at head `f4d64a21`, from the `ui/` root as `ui/AGENTS.md` prescribes, under `scripts/pm/os-verify-lock.sh`. <details><summary>Local runs</summary> | Command | Verdict | | --- | --- | | `turbo run build --filter='@object-ui/app-shell^...' --concurrency=2` (closure) | `VERDICT command-exit 0`, 64/64 tasks | | `pnpm --filter @object-ui/app-shell build` | `VERDICT command-exit 0` | | `pnpm --filter @object-ui/app-shell type-check` | `VERDICT command-exit 0` | | `pnpm --dir ui exec vitest run --maxWorkers=2` over the 84 test files that name `DraftChangesPanel` | `VERDICT command-exit 0`, Test Files 84 passed (84), Tests 536 passed (536) | | `node scripts/check-sdui-manifest.mjs` | exit 0, manifest intact (106 components) | | New test file alone | 3 passed (3) | **Narrowed:** the full `@object-ui/app-shell` vitest suite is not run locally. It needs more than an hour on the shared box. The 84 files are every test that names the edited component. CI's Test UI runs the rest. </details> <details><summary>Derived gates (`dispatch-gates.mjs --commands`)</summary> 35 derived, 35 run, 0 NOT-MEASURED, 0 UNRUN (`dispatch-gates --ran` exit 0) at `f4d64a21`. Each line is `CMD :: exit N`. ``` node scripts/check-adr-0087-registration.mjs --base origin/main :: exit 0 node scripts/check-adr-0087-registration.mjs --self-test :: exit 0 node scripts/check-changeset-no-major.mjs --base origin/main :: exit 0 node scripts/check-closing-keyword-parity.mjs :: exit 0 node scripts/check-closing-keyword-parity.mjs --self-test :: exit 0 node scripts/check-comment-mask-corpus.mjs :: exit 0 node scripts/check-empty-changeset.mjs --base origin/main :: exit 0 node scripts/check-empty-changeset.mjs --self-test :: exit 0 node scripts/check-sdui-manifest.mjs :: exit 0 node scripts/check-sdui-manifest.mjs --self-test :: exit 0 node scripts/pm/release-rehearsal-clone.mjs --self-test :: exit 0 node scripts/release-pending-publish.mjs --self-test :: exit 0 pnpm --dir ui exec vitest run scripts/ :: exit 0 pnpm --filter @objectstack/spec run check:duration-unit-keys :: exit 0 pnpm check:changeset-gate-self-tests :: exit 0 pnpm check:closed-ceremony-text :: exit 0 pnpm check:cross-package-test-inputs :: exit 0 pnpm check:dispatcher-error-vocabulary :: exit 0 pnpm check:driver-memory-census :: exit 0 pnpm check:engine-double-contract :: exit 0 pnpm check:gitlink-declared :: exit 0 pnpm check:nul-bytes :: exit 0 pnpm check:published-files :: exit 0 pnpm check:query-options-erasure :: exit 0 pnpm check:refd-timer-probe :: exit 0 pnpm check:registry-log-declared :: exit 0 pnpm check:rest-log-spy-declared :: exit 0 pnpm check:skill-identifier-liveness :: exit 0 pnpm check:test-source-alias :: exit 0 pnpm check:tier-file-adoption :: exit 0 pnpm check:type-check-coverage :: exit 0 pnpm check:type-check-debt :: exit 0 pnpm check:undeclared-dep-imports :: exit 0 pnpm check:watch-hint-literal :: exit 0 pnpm check:where-matcher :: exit 0 ``` </details> 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01WGufzfC4w4tZzdzVyV5FGX Co-authored-by: Claude <noreply@anthropic.com>
1 parent 83fc096 commit 40fa433

3 files changed

Lines changed: 206 additions & 3 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): Studio's publish review sheet no longer lists an `Options` change on a picklist-bound field nobody edited (#29899)
6+
7+
Clause-②: no
8+
Metadata-protocol: none
9+
10+
On an object with a field bound to a package picklist (`{ type: 'select', picklist: 'NAME' }`), the publish review sheet listed `~ FIELD · Options` on every publish, beside the change the author actually made. The draft is stored without the field's `options`, while the live read serves the options the runtime resolved from the picklist, and the sheet compared the two as read.
11+
12+
The sheet now compares both versions of an object in the form the publish door stores: a field that names a picklist is read without `options` on both sides. A field with its own inline `options` is untouched, so a real options change there is still listed, and a field moved between a picklist and inline options still lists both keys.

‎ui/packages/app-shell/src/preview/DraftChangesPanel.tsx‎

Lines changed: 24 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,12 @@ import {
4747
} from '@object-ui/components';
4848
import { useObjectTranslation } from '@object-ui/i18n';
4949
import { useAdapter } from '@object-ui/react';
50-
import { readDraftChanges, type DraftChange, type MetadataClient } from '@object-ui/data-objectstack';
50+
import {
51+
dropServedPicklistOptions,
52+
readDraftChanges,
53+
type DraftChange,
54+
type MetadataClient,
55+
} from '@object-ui/data-objectstack';
5156
// The `/meta` URL-spelling fold (objectstack#7894, objectstack-ai/objectstack#8424), applied once in
5257
// `listPendingDrafts` below.
5358
//
@@ -664,6 +669,19 @@ function fieldLabelOf(def: Record<string, unknown> | undefined): string | undefi
664669
* an author row: the platform injects the same definition on both reads, so a
665670
* difference there is an edit someone made.
666671
*
672+
* An `object`'s two bodies are compared in the form the publish door STORES
673+
* for a picklist-bound field (objectstack#29899): without `options`. The live
674+
* read serves such a field with the options the runtime resolved from its
675+
* picklist (`PicklistServedFieldSchema`), while the strict draft read serves
676+
* the draft row as stored, and the authoring door refuses `picklist` and
677+
* `options` together, so a stored draft never carries them. Compared as read,
678+
* every bound field listed `~ FIELD · Options` on every publish, an edit
679+
* nobody made. Both sides go through `dropServedPicklistOptions`, the writers'
680+
* own served-to-authored step, rather than skipping a key in the diff: it
681+
* touches a field only when that definition names a picklist, so a field with
682+
* inline `options` keeps every options change, and a field moving between a
683+
* picklist and inline options still lists both keys.
684+
*
667685
* Why not `computeDiffRows` from `LayeredDiff.tsx`, which splits top-level keys
668686
* the same way: that module imports the metadata-admin string table
669687
* (`./i18n.js`) at module scope, and this panel sits on the console's EAGER
@@ -675,8 +693,11 @@ export function computeChangeDetail(
675693
draft: Record<string, unknown> | null,
676694
type?: string,
677695
): EntryChangeDetail {
678-
const pub = withoutProtectionEnvelope(published ?? {});
679-
const cur = withoutProtectionEnvelope(draft ?? {});
696+
// objectstack#29899 — the stored form of a picklist-bound field, on both sides.
697+
const stored = (body: Record<string, unknown>): Record<string, unknown> =>
698+
type === 'object' ? dropServedPicklistOptions(body) : body;
699+
const pub = stored(withoutProtectionEnvelope(published ?? {}));
700+
const cur = stored(withoutProtectionEnvelope(draft ?? {}));
680701
let fields: EntryChangeDetail['fields'] = null;
681702
const generatedFields: EntryChangeDetail['generated'] = [];
682703
if (pub.fields != null || cur.fields != null) {
Lines changed: 170 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,170 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* The publish-review sheet lists no `Options` change on a picklist-bound field
5+
* nobody edited (objectstack#29899).
6+
*
7+
* ## The defect
8+
*
9+
* A select field may name a package picklist instead of carrying its own
10+
* options (`{ type: 'select', picklist: 'NAME' }`). The draft row stores it so,
11+
* without `options`: the authoring door refuses the two keys together. The
12+
* live (published) read serves the same field with the options the runtime
13+
* resolved from the picklist written onto it (`PicklistServedFieldSchema`).
14+
* The sheet diffed the two as read, so on every publish of such an object it
15+
* listed `~ Tier tier · Options` beside the author's real edit.
16+
*
17+
* The routes below serve exactly that pair: the published read carries the
18+
* resolved options on `tier`, the strict draft read does not.
19+
*
20+
* ## The controls
21+
*
22+
* - A field with INLINE `options` whose options really changed still lists
23+
* `Options`: the fix compares the stored form, it does not skip the key.
24+
* - A field moving between a picklist and inline options still lists both
25+
* keys, in either direction.
26+
*/
27+
28+
import '@testing-library/jest-dom/vitest';
29+
import * as React from 'react';
30+
import { afterEach, describe, expect, it, vi } from 'vitest';
31+
import { render, screen, fireEvent, waitFor } from '@testing-library/react';
32+
33+
vi.mock('@object-ui/i18n', async (importOriginal) => {
34+
const mod = await importOriginal<typeof import('@object-ui/i18n')>();
35+
return {
36+
...mod,
37+
useObjectTranslation: () => ({
38+
t: (_k: string, o?: { defaultValue?: string; count?: number }) =>
39+
(o?.defaultValue ?? _k).replace('{{count}}', String(o?.count ?? '')),
40+
}),
41+
};
42+
});
43+
44+
import { DraftChangesPanel, computeChangeDetail } from '../DraftChangesPanel';
45+
46+
afterEach(() => {
47+
vi.restoreAllMocks();
48+
});
49+
50+
type Body = Record<string, unknown>;
51+
52+
const TIER_OPTIONS = [
53+
{ label: 'Bronze', value: 'bronze' },
54+
{ label: 'Silver', value: 'silver' },
55+
{ label: 'Gold', value: 'gold' },
56+
];
57+
58+
/** `tier` as the draft row stores it: bound to the picklist, no `options`. */
59+
const STORED_TIER = { type: 'select', label: 'Tier', picklist: 'showcase_tier' };
60+
/** `tier` as the live read serves it: the picklist's options resolved onto it. */
61+
const SERVED_TIER = { ...STORED_TIER, options: TIER_OPTIONS };
62+
63+
function mockRoutes(type: string, name: string, draft: Body, published: Body) {
64+
global.fetch = vi.fn(async (input: RequestInfo | URL) => {
65+
const url = String(input);
66+
const ok = (body: unknown) => ({ ok: true, status: 200, json: async () => body });
67+
const notFound = { ok: false, status: 404, json: async () => ({}) };
68+
if (url.includes('/_drafts')) return ok([{ type, name, packageId: 'com.test.showcase' }]);
69+
if (url.includes('state=draft')) return ok({ type, name, item: { ...draft, _draft: true } });
70+
if (url.includes(`/meta/${type}/${name}`)) return ok({ type, name, item: published });
71+
if (new RegExp(`/meta/${type}(\\?|$)`).test(url)) return ok([{ name }]);
72+
return notFound;
73+
}) as unknown as typeof fetch;
74+
}
75+
76+
async function openEntryDetail() {
77+
render(<DraftChangesPanel open onOpenChange={() => {}} packageId="com.test.showcase" onPublish={vi.fn()} />);
78+
fireEvent.click(await screen.findByTestId('draft-entry-toggle'));
79+
}
80+
81+
/** The text of every field and key row the drill-in rendered. */
82+
function renderedRows(): string[] {
83+
return [
84+
...screen
85+
.getByTestId('draft-entry-detail')
86+
.querySelectorAll('[data-testid="draft-field-row"], [data-testid="draft-key-row"]'),
87+
].map((row) => (row.textContent ?? '').trim());
88+
}
89+
90+
describe('DraftChangesPanel — a picklist-bound field compares in its stored form (objectstack#29899)', () => {
91+
it('lists only the label change when the draft edits another field of an object with a bound field', async () => {
92+
mockRoutes(
93+
'object',
94+
'showcase_member',
95+
{
96+
name: 'showcase_member',
97+
label: 'Member',
98+
fields: { tier: STORED_TIER, notes: { type: 'textarea', label: 'Again notes' } },
99+
},
100+
{
101+
name: 'showcase_member',
102+
label: 'Member',
103+
fields: { tier: SERVED_TIER, notes: { type: 'textarea', label: 'Notes' } },
104+
},
105+
);
106+
await openEntryDetail();
107+
108+
await waitFor(() => expect(renderedRows().length).toBeGreaterThan(0), { timeout: 4000 });
109+
const rows = renderedRows();
110+
expect(rows).toHaveLength(1);
111+
expect(rows[0]).toMatch(/^~ Again notes notes/);
112+
expect(rows[0]).toContain('Label');
113+
expect(screen.getByTestId('draft-entry-detail').textContent).not.toContain('Options');
114+
});
115+
116+
it('CONTROL: still lists an Options change on a field with inline options', async () => {
117+
mockRoutes(
118+
'object',
119+
'showcase_member',
120+
{
121+
name: 'showcase_member',
122+
label: 'Member',
123+
fields: {
124+
tier: STORED_TIER,
125+
status: { type: 'select', label: 'Status', options: [{ label: 'Open', value: 'open' }, { label: 'Closed', value: 'closed' }] },
126+
},
127+
},
128+
{
129+
name: 'showcase_member',
130+
label: 'Member',
131+
fields: {
132+
tier: SERVED_TIER,
133+
status: { type: 'select', label: 'Status', options: [{ label: 'Open', value: 'open' }] },
134+
},
135+
},
136+
);
137+
await openEntryDetail();
138+
139+
await waitFor(() => expect(renderedRows().length).toBeGreaterThan(0), { timeout: 4000 });
140+
const rows = renderedRows();
141+
expect(rows).toHaveLength(1);
142+
expect(rows[0]).toMatch(/^~ Status status/);
143+
expect(rows[0]).toContain('Options');
144+
});
145+
146+
it('reads a bound field as unchanged in computeChangeDetail, and keeps a move to or from inline options', () => {
147+
const pub = { name: 'm', fields: { tier: SERVED_TIER } };
148+
expect(computeChangeDetail(pub, { name: 'm', fields: { tier: STORED_TIER } }, 'object').fields?.changed).toEqual([]);
149+
150+
// Bound → inline: the field's `picklist` leaves and its `options` arrive.
151+
const inline = { type: 'select', label: 'Tier', options: TIER_OPTIONS };
152+
expect(computeChangeDetail(pub, { name: 'm', fields: { tier: inline } }, 'object').fields?.changed).toEqual([
153+
{ name: 'tier', keys: expect.arrayContaining(['picklist', 'options']) },
154+
]);
155+
// Inline → bound, read back as the runtime serves it.
156+
expect(
157+
computeChangeDetail({ name: 'm', fields: { tier: inline } }, { name: 'm', fields: { tier: STORED_TIER } }, 'object')
158+
.fields?.changed,
159+
).toEqual([{ name: 'tier', keys: expect.arrayContaining(['picklist', 'options']) }]);
160+
161+
// The array `fields` shape a body can arrive in reads the same.
162+
expect(
163+
computeChangeDetail(
164+
{ name: 'm', fields: [{ name: 'tier', ...SERVED_TIER }] },
165+
{ name: 'm', fields: [{ name: 'tier', ...STORED_TIER }] },
166+
'object',
167+
).fields?.changed,
168+
).toEqual([]);
169+
});
170+
});

0 commit comments

Comments
 (0)