From afcb0e8a1cfd91927692ec40a3314f299607fa95 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=8C=85=E5=91=A8=E6=B6=9B?= Date: Sat, 4 Jul 2026 11:09:05 -0700 Subject: [PATCH] fix(spec): warn loudly on view-key collisions in expandViewContainer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit list/form views share one . namespace during container expansion; a colliding key (e.g. formViews.default vs the implicit default list) was silently renamed to _2, so action targets and navigation viewNames referencing the requested name resolved to the OTHER view — the root cause of the showcase "Log Time" form action opening the list view and rendering a black/empty form. - spec: ExpandedViewItem gains optional _diagnostics; uniqueViewName renames now stamp a warning naming both views and the blast radius. - objectql engine + metadata plugin: log expansion warnings at boot. - app-showcase: rename formViews.default -> edit and point showcase_log_time's target at showcase_task.edit (with comments documenting the collision). - tests: 3 new cases covering collision warnings and the clean path. Closes #2554 Co-Authored-By: Claude --- .changeset/view-expand-collision-warning.md | 15 ++++++ examples/app-showcase/src/actions/index.ts | 4 +- examples/app-showcase/src/views/task.view.ts | 7 ++- packages/metadata/src/plugin.ts | 3 ++ packages/metadata/src/view-expand.test.ts | 48 +++++++++++++++++ packages/objectql/src/engine.ts | 3 ++ packages/spec/src/ui/view.zod.ts | 56 +++++++++++++++++--- 7 files changed, 126 insertions(+), 10 deletions(-) create mode 100644 .changeset/view-expand-collision-warning.md diff --git a/.changeset/view-expand-collision-warning.md b/.changeset/view-expand-collision-warning.md new file mode 100644 index 0000000000..6e0f7a3c11 --- /dev/null +++ b/.changeset/view-expand-collision-warning.md @@ -0,0 +1,15 @@ +--- +"@objectstack/spec": patch +"@objectstack/objectql": patch +"@objectstack/metadata": patch +--- + +Surface view-key collisions during view container expansion instead of renaming silently. + +`expandViewContainer` keeps its backward-compatible rename behaviour (`.` → +`._2` on collision) but now stamps a machine-readable +`_diagnostics.warnings` entry on the renamed `ExpandedViewItem`, explaining that +references targeting the requested name (form action targets, navigation `viewName`s) +will resolve to the *other* view. Both flattening loaders — the ObjectQL engine and the +MetadataPlugin — log these warnings at boot so the collision is visible instead of +manifesting as a form action opening a list view (#2554). diff --git a/examples/app-showcase/src/actions/index.ts b/examples/app-showcase/src/actions/index.ts index b263d1ff1b..90f00ce96d 100644 --- a/examples/app-showcase/src/actions/index.ts +++ b/examples/app-showcase/src/actions/index.ts @@ -117,7 +117,9 @@ export const LogTimeAction = defineAction({ icon: 'clock', objectName: task, type: 'form', - target: 'showcase_task.default', + // Targets the `edit` form view — `showcase_task.default` is the LIST view + // (the container's main `list` implicitly claims the `default` key). + target: 'showcase_task.edit', // `record_section` so it surfaces in the Task Detail quick-actions bar too. locations: ['record_header', 'record_related', 'record_section'], refreshAfter: true, diff --git a/examples/app-showcase/src/views/task.view.ts b/examples/app-showcase/src/views/task.view.ts index 0d2c243a0c..046c5cc238 100644 --- a/examples/app-showcase/src/views/task.view.ts +++ b/examples/app-showcase/src/views/task.view.ts @@ -230,7 +230,12 @@ export const TaskViews = defineView({ formViews: { // simple ── single-section form ────────────────────────────────────── - default: { + // Keyed `edit`, NOT `default`: the container's main `list` implicitly + // claims `showcase_task.default`, and list + form views share one + // `.` namespace — a `default` form key would be renamed to + // `default_2` at expansion (with a boot warning), breaking any action + // `target` that references it. See framework issue #2554. + edit: { type: 'simple', data, sections: [ diff --git a/packages/metadata/src/plugin.ts b/packages/metadata/src/plugin.ts index f627b7591c..11a89a953c 100644 --- a/packages/metadata/src/plugin.ts +++ b/packages/metadata/src/plugin.ts @@ -558,6 +558,9 @@ export class MetadataPlugin implements Plugin { await this.manager.register('view', viewObject, item); totalRegistered++; for (const vi of expandViewContainer(viewObject, item)) { + for (const w of vi._diagnostics?.warnings ?? []) { + ctx.logger.warn(`[MetadataPlugin] View expansion warning for '${vi.name}': ${w.message}`); + } applyProtection(vi as any, { packageId: manifestPackageId, packageVersion: manifestVersion, diff --git a/packages/metadata/src/view-expand.test.ts b/packages/metadata/src/view-expand.test.ts index ef128c6eb7..d9c106e471 100644 --- a/packages/metadata/src/view-expand.test.ts +++ b/packages/metadata/src/view-expand.test.ts @@ -103,3 +103,51 @@ describe('expandViewContainer — default list with no listViews dup', () => { expect(items[0].isDefault).toBe(true); }); }); + +describe('expandViewContainer — name collisions carry _diagnostics warnings (#2554)', () => { + it('warns when formViews.default collides with the implicit default list', () => { + const items = expandViewContainer('task', { + list: { type: 'grid', label: 'All Tasks', columns: ['title'], data: { provider: 'object', object: 'task' } }, + formViews: { + default: { type: 'simple', data: { provider: 'object', object: 'task' }, sections: [] }, + }, + }); + const list = items.find((i) => i.viewKind === 'list'); + const form = items.find((i) => i.viewKind === 'form'); + // Rename behaviour itself is unchanged (backward compat)… + expect(list?.name).toBe('task.default'); + expect(form?.name).toBe('task.default_2'); + // …but the renamed item now carries a loud, machine-readable warning. + expect(list?._diagnostics).toBeUndefined(); + expect(form?._diagnostics?.valid).toBe(true); + expect(form?._diagnostics?.warnings).toHaveLength(1); + expect(form?._diagnostics?.warnings[0].path).toBe('name'); + expect(form?._diagnostics?.warnings[0].message).toContain("'task.default'"); + expect(form?._diagnostics?.warnings[0].message).toContain("'task.default_2'"); + }); + + it('warns when a formViews key collides with a listViews key', () => { + const items = expandViewContainer('task', { + listViews: { + mine: { type: 'grid', label: 'Mine', columns: ['title'], data: { provider: 'object', object: 'task' } }, + }, + formViews: { + mine: { type: 'simple', data: { provider: 'object', object: 'task' }, sections: [] }, + }, + }); + const form = items.find((i) => i.viewKind === 'form'); + expect(form?.name).toBe('task.mine_2'); + expect(form?._diagnostics?.warnings?.[0].message).toContain("'task.mine'"); + }); + + it('does not stamp _diagnostics on collision-free expansions', () => { + const items = expandViewContainer('task', { + list: { type: 'grid', label: 'All', columns: ['title'], data: { provider: 'object', object: 'task' } }, + formViews: { + edit: { type: 'simple', data: { provider: 'object', object: 'task' }, sections: [] }, + }, + }); + expect(items).toHaveLength(2); + for (const item of items) expect(item._diagnostics).toBeUndefined(); + }); +}); diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index cc309ec825..fa247740c0 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -1060,6 +1060,9 @@ export class ObjectQL implements IDataEngine { // the per-view `package` layer the switcher + Studio consume. if (key === 'views' && isAggregatedViewContainer(toRegister)) { for (const vi of expandViewContainer(itemName, toRegister)) { + for (const w of vi._diagnostics?.warnings ?? []) { + this.logger.warn(`View expansion warning for '${vi.name}': ${w.message}`, { from: id }); + } this._registry.registerItem('view', vi, 'name' as any, id); } } diff --git a/packages/spec/src/ui/view.zod.ts b/packages/spec/src/ui/view.zod.ts index 327b6375ed..512a0d9030 100644 --- a/packages/spec/src/ui/view.zod.ts +++ b/packages/spec/src/ui/view.zod.ts @@ -1131,6 +1131,11 @@ export interface ExpandedViewItem { isDefault?: boolean; order: number; scope: 'package'; + /** Non-blocking expansion diagnostics (MetadataValidationResult wire shape). + * Present only when the item's name had to be rewritten to avoid a + * collision — loaders surface `warnings` in their boot/HMR logs and + * Studio can badge the view. */ + _diagnostics?: { valid: boolean; warnings: Array<{ path: string; message: string }> }; } /** True when a raw view artifact still uses the aggregated container shape @@ -1162,6 +1167,29 @@ function uniqueViewName(base: string, used: Set): string { return name; } +/** Stamp a rename warning on an expanded item whose `.` name was + * already taken (e.g. `formViews.default` vs the implicit default `list`). + * The rename itself is kept for backward compatibility — this makes it LOUD: + * loaders log the warning and Studio can render it, so authors discover that + * references to the requested name (form action `target`s, navigation + * `viewName`s) resolve to a DIFFERENT view. */ +function stampRenameWarning(item: ExpandedViewItem, requestedName: string): void { + if (item.name === requestedName) return; + item._diagnostics = { + valid: true, + warnings: [{ + path: 'name', + message: + `View key collision: '${requestedName}' is already registered by another view in this ` + + `defineView container (list and form views share one '.' namespace, and the ` + + `default 'list' implicitly claims '.default'). This ${item.viewKind} view was ` + + `renamed to '${item.name}'. References targeting '${requestedName}' — form action ` + + `targets, navigation viewNames — will resolve to the OTHER view. Rename the view key ` + + `to something unique to remove this warning.`, + }], + }; +} + function cloneViewConfig(v: any): any { try { return structuredClone(v); @@ -1191,9 +1219,12 @@ export function expandViewContainer(object: string, container: any): ExpandedVie container.listViews && typeof container.listViews === 'object' ? container.listViews : {}; for (const [k, v] of Object.entries(listViews)) { if (!v || typeof v !== 'object') continue; - const name = uniqueViewName(`${object}.${k}`, used); + const requested = `${object}.${k}`; + const name = uniqueViewName(requested, used); listSigToName.set(viewSignature(v), name); - out.push({ name, object, viewKind: 'list', label: v.label, config: cloneViewConfig(v), order: order++, scope: 'package' }); + const item: ExpandedViewItem = { name, object, viewKind: 'list', label: v.label, config: cloneViewConfig(v), order: order++, scope: 'package' }; + stampRenameWarning(item, requested); + out.push(item); } const defaultList = container.list; let defaultListName: string | undefined; @@ -1203,8 +1234,11 @@ export function expandViewContainer(object: string, container: any): ExpandedVie defaultListName = dup; // already represented by a named listViews entry } else { const key = typeof defaultList.name === 'string' && defaultList.name ? defaultList.name : 'default'; - const name = uniqueViewName(`${object}.${key}`, used); - out.push({ name, object, viewKind: 'list', label: defaultList.label, config: cloneViewConfig(defaultList), order: order++, scope: 'package' }); + const requested = `${object}.${key}`; + const name = uniqueViewName(requested, used); + const item: ExpandedViewItem = { name, object, viewKind: 'list', label: defaultList.label, config: cloneViewConfig(defaultList), order: order++, scope: 'package' }; + stampRenameWarning(item, requested); + out.push(item); defaultListName = name; } } @@ -1220,16 +1254,22 @@ export function expandViewContainer(object: string, container: any): ExpandedVie container.formViews && typeof container.formViews === 'object' ? container.formViews : {}; for (const [k, v] of Object.entries(formViews)) { if (!v || typeof v !== 'object') continue; - const name = uniqueViewName(`${object}.${k}`, used); + const requested = `${object}.${k}`; + const name = uniqueViewName(requested, used); formSigSeen.add(viewSignature(v)); - out.push({ name, object, viewKind: 'form', label: v.label, config: cloneViewConfig(v), order: order++, scope: 'package' }); + const item: ExpandedViewItem = { name, object, viewKind: 'form', label: v.label, config: cloneViewConfig(v), order: order++, scope: 'package' }; + stampRenameWarning(item, requested); + out.push(item); } const defaultForm = container.form; let defaultFormName: string | undefined; if (defaultForm && typeof defaultForm === 'object' && !formSigSeen.has(viewSignature(defaultForm))) { const key = typeof defaultForm.name === 'string' && defaultForm.name ? defaultForm.name : 'form'; - const name = uniqueViewName(`${object}.${key}`, used); - out.push({ name, object, viewKind: 'form', label: defaultForm.label, config: cloneViewConfig(defaultForm), order: order++, scope: 'package' }); + const requested = `${object}.${key}`; + const name = uniqueViewName(requested, used); + const item: ExpandedViewItem = { name, object, viewKind: 'form', label: defaultForm.label, config: cloneViewConfig(defaultForm), order: order++, scope: 'package' }; + stampRenameWarning(item, requested); + out.push(item); defaultFormName = name; } if (!defaultFormName && out.length > formStart) defaultFormName = out[formStart].name;