From bc2a9a38b113291dedf7742171e32f3a35f4981c Mon Sep 17 00:00:00 2001 From: Fini Date: Sun, 19 Apr 2026 15:39:42 +0800 Subject: [PATCH] fix(mcp): assign ids to every node in add_scroll_row_v0 subtree MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex stop-hook review: child nodes were saved without ids. batch_design only assigns an id to the top-level inserted node — nested children (inner row, cards, texts, icon_fonts) come through the DSL unchanged, which breaks any later tree operation that resolves by id (update / delete / move / post-processing / spatial index lookup). Fix: assignIdsRecursively walks the wrapper subtree before serializing to DSL and stamps every node with generateId(). batch_design's own overwrite of the top-level id is harmless. Test: 4 new id-coverage tests (one per children_type + uniqueness across a 3-card tree). Total 17 tests in add-scroll-row-v0, 74 in pen-mcp suite. format + tsc green. --- .../src/__tests__/add-scroll-row-v0.test.ts | 93 +++++++++++++++++++ .../pen-mcp/src/tools/add-scroll-row-v0.ts | 18 ++++ 2 files changed, 111 insertions(+) diff --git a/packages/pen-mcp/src/__tests__/add-scroll-row-v0.test.ts b/packages/pen-mcp/src/__tests__/add-scroll-row-v0.test.ts index 611f5e8e7..2d182c04b 100644 --- a/packages/pen-mcp/src/__tests__/add-scroll-row-v0.test.ts +++ b/packages/pen-mcp/src/__tests__/add-scroll-row-v0.test.ts @@ -243,6 +243,99 @@ describe('add_scroll_row_v0 — overrides', () => { }); }); +describe('add_scroll_row_v0 — id assignment (every node must have a valid id)', () => { + function collectIds( + node: Record, + out: { total: number; missing: string[] }, + path = 'root', + ): void { + const id = node.id; + if (typeof id !== 'string' || id.length === 0) { + out.missing.push(`${path} (type=${String(node.type)} name=${String(node.name)})`); + } + out.total += 1; + const children = node.children; + if (Array.isArray(children)) { + children.forEach((c, i) => { + if (c && typeof c === 'object') { + collectIds(c as Record, out, `${path}/${i}`); + } + }); + } + } + + it('every node in a card row (wrapper+row+cards+grandchildren) has a non-empty id', async () => { + const fp = await fresh('cards.op'); + await handleAddScrollRowV0({ + filePath: fp, + children_type: 'card', + items: [{ title: 'A', subtitle: 'x', icon: 'i' }, { title: 'B' }], + }); + const wrapper = getRoot(await readDoc(fp)); + const collected = { total: 0, missing: [] as string[] }; + collectIds(wrapper, collected); + expect(collected.missing).toEqual([]); + // wrapper + row + 2 cards + (3 + 1) card children = 8 + expect(collected.total).toBe(8); + }); + + it('every node in a metric_tile row has a non-empty id', async () => { + const fp = await fresh('metrics.op'); + await handleAddScrollRowV0({ + filePath: fp, + children_type: 'metric_tile', + items: [ + { title: 'Steps', subtitle: '8,432' }, + { title: 'Kcal', subtitle: '512', icon: 'flame' }, + ], + }); + const wrapper = getRoot(await readDoc(fp)); + const collected = { total: 0, missing: [] as string[] }; + collectIds(wrapper, collected); + expect(collected.missing).toEqual([]); + }); + + it('every node in a nav_item row has a non-empty id', async () => { + const fp = await fresh('nav.op'); + await handleAddScrollRowV0({ + filePath: fp, + children_type: 'nav_item', + items: [ + { title: 'Home', icon: 'home' }, + { title: 'Search', icon: 'search' }, + ], + }); + const wrapper = getRoot(await readDoc(fp)); + const collected = { total: 0, missing: [] as string[] }; + collectIds(wrapper, collected); + expect(collected.missing).toEqual([]); + }); + + it('all ids in the tree are unique', async () => { + const fp = await fresh('cards.op'); + await handleAddScrollRowV0({ + filePath: fp, + children_type: 'card', + items: [ + { title: 'A', subtitle: 'x', icon: 'i' }, + { title: 'B', subtitle: 'y', icon: 'j' }, + { title: 'C' }, + ], + }); + const wrapper = getRoot(await readDoc(fp)); + const ids: string[] = []; + function collect(n: Record): void { + if (typeof n.id === 'string') ids.push(n.id); + const kids = n.children; + if (Array.isArray(kids)) { + for (const k of kids) if (k && typeof k === 'object') collect(k as Record); + } + } + collect(wrapper); + expect(new Set(ids).size).toBe(ids.length); + }); +}); + describe('add_scroll_row_v0 — persistence', () => { it('result.nodeCount reflects full node tree (wrapper+row+cards+children)', async () => { const fp = await fresh('persist.op'); diff --git a/packages/pen-mcp/src/tools/add-scroll-row-v0.ts b/packages/pen-mcp/src/tools/add-scroll-row-v0.ts index fb351ab26..4c0109436 100644 --- a/packages/pen-mcp/src/tools/add-scroll-row-v0.ts +++ b/packages/pen-mcp/src/tools/add-scroll-row-v0.ts @@ -1,4 +1,5 @@ import { handleBatchDesign } from './batch-design'; +import { generateId } from '../utils/id'; export interface AddScrollRowV0Item { title: string; @@ -37,6 +38,11 @@ export async function handleAddScrollRowV0( const gap = params.gap ?? 12; const cardWidth = params.card_width ?? defaultCardWidth(params.children_type); const wrapper = buildWrapperNode(params, gap, cardWidth); + // batch_design only assigns an id to the top-level inserted node; nested + // children come through untouched. Without ids on nested nodes, later + // tree traversal / update / delete / move all fail. Assign ids to the + // entire subtree here (batch_design harmlessly overwrites the top id). + assignIdsRecursively(wrapper); const parentRef = params.parent_id ? `"${params.parent_id}"` : 'null'; const dsl = `row=I(${parentRef}, ${JSON.stringify(wrapper)})`; return handleBatchDesign({ @@ -47,6 +53,18 @@ export async function handleAddScrollRowV0( }); } +function assignIdsRecursively(node: Record): void { + if (typeof node.id !== 'string') node.id = generateId(); + const children = node.children; + if (Array.isArray(children)) { + for (const child of children) { + if (child && typeof child === 'object') { + assignIdsRecursively(child as Record); + } + } + } +} + function defaultCardWidth(kind: AddScrollRowV0ChildrenType): number { switch (kind) { case 'card':