Skip to content

Commit afcb0e8

Browse files
baozhoutaoclaude
andcommitted
fix(spec): warn loudly on view-key collisions in expandViewContainer
list/form views share one <object>.<key> namespace during container expansion; a colliding key (e.g. formViews.default vs the implicit default list) was silently renamed to <key>_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 <noreply@anthropic.com>
1 parent 24b62ee commit afcb0e8

7 files changed

Lines changed: 126 additions & 10 deletions

File tree

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
---
2+
"@objectstack/spec": patch
3+
"@objectstack/objectql": patch
4+
"@objectstack/metadata": patch
5+
---
6+
7+
Surface view-key collisions during view container expansion instead of renaming silently.
8+
9+
`expandViewContainer` keeps its backward-compatible rename behaviour (`<object>.<key>`
10+
`<object>.<key>_2` on collision) but now stamps a machine-readable
11+
`_diagnostics.warnings` entry on the renamed `ExpandedViewItem`, explaining that
12+
references targeting the requested name (form action targets, navigation `viewName`s)
13+
will resolve to the *other* view. Both flattening loaders — the ObjectQL engine and the
14+
MetadataPlugin — log these warnings at boot so the collision is visible instead of
15+
manifesting as a form action opening a list view (#2554).

examples/app-showcase/src/actions/index.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -117,7 +117,9 @@ export const LogTimeAction = defineAction({
117117
icon: 'clock',
118118
objectName: task,
119119
type: 'form',
120-
target: 'showcase_task.default',
120+
// Targets the `edit` form view — `showcase_task.default` is the LIST view
121+
// (the container's main `list` implicitly claims the `default` key).
122+
target: 'showcase_task.edit',
121123
// `record_section` so it surfaces in the Task Detail quick-actions bar too.
122124
locations: ['record_header', 'record_related', 'record_section'],
123125
refreshAfter: true,

examples/app-showcase/src/views/task.view.ts

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -230,7 +230,12 @@ export const TaskViews = defineView({
230230

231231
formViews: {
232232
// simple ── single-section form ──────────────────────────────────────
233-
default: {
233+
// Keyed `edit`, NOT `default`: the container's main `list` implicitly
234+
// claims `showcase_task.default`, and list + form views share one
235+
// `<object>.<key>` namespace — a `default` form key would be renamed to
236+
// `default_2` at expansion (with a boot warning), breaking any action
237+
// `target` that references it. See framework issue #2554.
238+
edit: {
234239
type: 'simple',
235240
data,
236241
sections: [

packages/metadata/src/plugin.ts

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -558,6 +558,9 @@ export class MetadataPlugin implements Plugin {
558558
await this.manager.register('view', viewObject, item);
559559
totalRegistered++;
560560
for (const vi of expandViewContainer(viewObject, item)) {
561+
for (const w of vi._diagnostics?.warnings ?? []) {
562+
ctx.logger.warn(`[MetadataPlugin] View expansion warning for '${vi.name}': ${w.message}`);
563+
}
561564
applyProtection(vi as any, {
562565
packageId: manifestPackageId,
563566
packageVersion: manifestVersion,

packages/metadata/src/view-expand.test.ts

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -103,3 +103,51 @@ describe('expandViewContainer — default list with no listViews dup', () => {
103103
expect(items[0].isDefault).toBe(true);
104104
});
105105
});
106+
107+
describe('expandViewContainer — name collisions carry _diagnostics warnings (#2554)', () => {
108+
it('warns when formViews.default collides with the implicit default list', () => {
109+
const items = expandViewContainer('task', {
110+
list: { type: 'grid', label: 'All Tasks', columns: ['title'], data: { provider: 'object', object: 'task' } },
111+
formViews: {
112+
default: { type: 'simple', data: { provider: 'object', object: 'task' }, sections: [] },
113+
},
114+
});
115+
const list = items.find((i) => i.viewKind === 'list');
116+
const form = items.find((i) => i.viewKind === 'form');
117+
// Rename behaviour itself is unchanged (backward compat)…
118+
expect(list?.name).toBe('task.default');
119+
expect(form?.name).toBe('task.default_2');
120+
// …but the renamed item now carries a loud, machine-readable warning.
121+
expect(list?._diagnostics).toBeUndefined();
122+
expect(form?._diagnostics?.valid).toBe(true);
123+
expect(form?._diagnostics?.warnings).toHaveLength(1);
124+
expect(form?._diagnostics?.warnings[0].path).toBe('name');
125+
expect(form?._diagnostics?.warnings[0].message).toContain("'task.default'");
126+
expect(form?._diagnostics?.warnings[0].message).toContain("'task.default_2'");
127+
});
128+
129+
it('warns when a formViews key collides with a listViews key', () => {
130+
const items = expandViewContainer('task', {
131+
listViews: {
132+
mine: { type: 'grid', label: 'Mine', columns: ['title'], data: { provider: 'object', object: 'task' } },
133+
},
134+
formViews: {
135+
mine: { type: 'simple', data: { provider: 'object', object: 'task' }, sections: [] },
136+
},
137+
});
138+
const form = items.find((i) => i.viewKind === 'form');
139+
expect(form?.name).toBe('task.mine_2');
140+
expect(form?._diagnostics?.warnings?.[0].message).toContain("'task.mine'");
141+
});
142+
143+
it('does not stamp _diagnostics on collision-free expansions', () => {
144+
const items = expandViewContainer('task', {
145+
list: { type: 'grid', label: 'All', columns: ['title'], data: { provider: 'object', object: 'task' } },
146+
formViews: {
147+
edit: { type: 'simple', data: { provider: 'object', object: 'task' }, sections: [] },
148+
},
149+
});
150+
expect(items).toHaveLength(2);
151+
for (const item of items) expect(item._diagnostics).toBeUndefined();
152+
});
153+
});

packages/objectql/src/engine.ts

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1060,6 +1060,9 @@ export class ObjectQL implements IDataEngine {
10601060
// the per-view `package` layer the switcher + Studio consume.
10611061
if (key === 'views' && isAggregatedViewContainer(toRegister)) {
10621062
for (const vi of expandViewContainer(itemName, toRegister)) {
1063+
for (const w of vi._diagnostics?.warnings ?? []) {
1064+
this.logger.warn(`View expansion warning for '${vi.name}': ${w.message}`, { from: id });
1065+
}
10631066
this._registry.registerItem('view', vi, 'name' as any, id);
10641067
}
10651068
}

packages/spec/src/ui/view.zod.ts

Lines changed: 48 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1131,6 +1131,11 @@ export interface ExpandedViewItem {
11311131
isDefault?: boolean;
11321132
order: number;
11331133
scope: 'package';
1134+
/** Non-blocking expansion diagnostics (MetadataValidationResult wire shape).
1135+
* Present only when the item's name had to be rewritten to avoid a
1136+
* collision — loaders surface `warnings` in their boot/HMR logs and
1137+
* Studio can badge the view. */
1138+
_diagnostics?: { valid: boolean; warnings: Array<{ path: string; message: string }> };
11341139
}
11351140

11361141
/** True when a raw view artifact still uses the aggregated container shape
@@ -1162,6 +1167,29 @@ function uniqueViewName(base: string, used: Set<string>): string {
11621167
return name;
11631168
}
11641169

1170+
/** Stamp a rename warning on an expanded item whose `<object>.<key>` name was
1171+
* already taken (e.g. `formViews.default` vs the implicit default `list`).
1172+
* The rename itself is kept for backward compatibility — this makes it LOUD:
1173+
* loaders log the warning and Studio can render it, so authors discover that
1174+
* references to the requested name (form action `target`s, navigation
1175+
* `viewName`s) resolve to a DIFFERENT view. */
1176+
function stampRenameWarning(item: ExpandedViewItem, requestedName: string): void {
1177+
if (item.name === requestedName) return;
1178+
item._diagnostics = {
1179+
valid: true,
1180+
warnings: [{
1181+
path: 'name',
1182+
message:
1183+
`View key collision: '${requestedName}' is already registered by another view in this `
1184+
+ `defineView container (list and form views share one '<object>.<key>' namespace, and the `
1185+
+ `default 'list' implicitly claims '<object>.default'). This ${item.viewKind} view was `
1186+
+ `renamed to '${item.name}'. References targeting '${requestedName}' — form action `
1187+
+ `targets, navigation viewNames — will resolve to the OTHER view. Rename the view key `
1188+
+ `to something unique to remove this warning.`,
1189+
}],
1190+
};
1191+
}
1192+
11651193
function cloneViewConfig(v: any): any {
11661194
try {
11671195
return structuredClone(v);
@@ -1191,9 +1219,12 @@ export function expandViewContainer(object: string, container: any): ExpandedVie
11911219
container.listViews && typeof container.listViews === 'object' ? container.listViews : {};
11921220
for (const [k, v] of Object.entries<any>(listViews)) {
11931221
if (!v || typeof v !== 'object') continue;
1194-
const name = uniqueViewName(`${object}.${k}`, used);
1222+
const requested = `${object}.${k}`;
1223+
const name = uniqueViewName(requested, used);
11951224
listSigToName.set(viewSignature(v), name);
1196-
out.push({ name, object, viewKind: 'list', label: v.label, config: cloneViewConfig(v), order: order++, scope: 'package' });
1225+
const item: ExpandedViewItem = { name, object, viewKind: 'list', label: v.label, config: cloneViewConfig(v), order: order++, scope: 'package' };
1226+
stampRenameWarning(item, requested);
1227+
out.push(item);
11971228
}
11981229
const defaultList = container.list;
11991230
let defaultListName: string | undefined;
@@ -1203,8 +1234,11 @@ export function expandViewContainer(object: string, container: any): ExpandedVie
12031234
defaultListName = dup; // already represented by a named listViews entry
12041235
} else {
12051236
const key = typeof defaultList.name === 'string' && defaultList.name ? defaultList.name : 'default';
1206-
const name = uniqueViewName(`${object}.${key}`, used);
1207-
out.push({ name, object, viewKind: 'list', label: defaultList.label, config: cloneViewConfig(defaultList), order: order++, scope: 'package' });
1237+
const requested = `${object}.${key}`;
1238+
const name = uniqueViewName(requested, used);
1239+
const item: ExpandedViewItem = { name, object, viewKind: 'list', label: defaultList.label, config: cloneViewConfig(defaultList), order: order++, scope: 'package' };
1240+
stampRenameWarning(item, requested);
1241+
out.push(item);
12081242
defaultListName = name;
12091243
}
12101244
}
@@ -1220,16 +1254,22 @@ export function expandViewContainer(object: string, container: any): ExpandedVie
12201254
container.formViews && typeof container.formViews === 'object' ? container.formViews : {};
12211255
for (const [k, v] of Object.entries<any>(formViews)) {
12221256
if (!v || typeof v !== 'object') continue;
1223-
const name = uniqueViewName(`${object}.${k}`, used);
1257+
const requested = `${object}.${k}`;
1258+
const name = uniqueViewName(requested, used);
12241259
formSigSeen.add(viewSignature(v));
1225-
out.push({ name, object, viewKind: 'form', label: v.label, config: cloneViewConfig(v), order: order++, scope: 'package' });
1260+
const item: ExpandedViewItem = { name, object, viewKind: 'form', label: v.label, config: cloneViewConfig(v), order: order++, scope: 'package' };
1261+
stampRenameWarning(item, requested);
1262+
out.push(item);
12261263
}
12271264
const defaultForm = container.form;
12281265
let defaultFormName: string | undefined;
12291266
if (defaultForm && typeof defaultForm === 'object' && !formSigSeen.has(viewSignature(defaultForm))) {
12301267
const key = typeof defaultForm.name === 'string' && defaultForm.name ? defaultForm.name : 'form';
1231-
const name = uniqueViewName(`${object}.${key}`, used);
1232-
out.push({ name, object, viewKind: 'form', label: defaultForm.label, config: cloneViewConfig(defaultForm), order: order++, scope: 'package' });
1268+
const requested = `${object}.${key}`;
1269+
const name = uniqueViewName(requested, used);
1270+
const item: ExpandedViewItem = { name, object, viewKind: 'form', label: defaultForm.label, config: cloneViewConfig(defaultForm), order: order++, scope: 'package' };
1271+
stampRenameWarning(item, requested);
1272+
out.push(item);
12331273
defaultFormName = name;
12341274
}
12351275
if (!defaultFormName && out.length > formStart) defaultFormName = out[formStart].name;

0 commit comments

Comments
 (0)