Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions .changeset/view-expand-collision-warning.md
Original file line number Diff line number Diff line change
@@ -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 (`<object>.<key>` →
`<object>.<key>_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).
4 changes: 3 additions & 1 deletion examples/app-showcase/src/actions/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
7 changes: 6 additions & 1 deletion examples/app-showcase/src/views/task.view.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
// `<object>.<key>` 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: [
Expand Down
3 changes: 3 additions & 0 deletions packages/metadata/src/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
48 changes: 48 additions & 0 deletions packages/metadata/src/view-expand.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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();
});
});
3 changes: 3 additions & 0 deletions packages/objectql/src/engine.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
Expand Down
56 changes: 48 additions & 8 deletions packages/spec/src/ui/view.zod.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -1162,6 +1167,29 @@ function uniqueViewName(base: string, used: Set<string>): string {
return name;
}

/** Stamp a rename warning on an expanded item whose `<object>.<key>` 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 '<object>.<key>' namespace, and the `
+ `default 'list' implicitly claims '<object>.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);
Expand Down Expand Up @@ -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<any>(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;
Expand All @@ -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;
}
}
Expand All @@ -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<any>(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;
Expand Down