diff --git a/.changeset/nav-viewname-resolution.md b/.changeset/nav-viewname-resolution.md new file mode 100644 index 0000000000..abf1023a4f --- /dev/null +++ b/.changeset/nav-viewname-resolution.md @@ -0,0 +1,24 @@ +--- +'@objectstack/lint': minor +--- + +Resolve an app navigation entry's `viewName` against its object's list views at `validate` and `build` + +`AppNavigationItemSchema.viewName` is documented as *"Default list view to open"*, so an +unresolvable name never failed — it **fell back**. A nav entry keeping its authored label and +icon would open a different view, and nothing said so: `os validate --json` reported +`valid: true` and `os build` was green. The decay mode was worse than the typo mode — renaming +a list view silently degraded every nav entry pointing at it, with every gate green and the +diff reading correctly in review. + +`lintViewRefs` now walks `app.navigation` (and the `areas[]` container) recursively and reports +`view-ref-nav-view-missing` as an **error** when a `viewName` resolves to no list view on the +object it names. This extends #2554's existing rule to the second, more travelled door into the +same `listViews` namespace rather than adding a new rule class. + +Resolution mirrors the runtime matcher (objectui's `resolveViewId`) in all three directions — +exact id, short name retried as `.`, and qualified name with the prefix stripped — +so a name that works at runtime is never reported. The accept set narrows only where the stack +itself declares the object's list views: an entry is skipped when the `viewName` is interpolated, +when `recordId` is set (the schema documents `viewName` as ignored there), when the item carries +`requiresObject`, or when this stack contributed no list view for that object. diff --git a/packages/lint/src/index.ts b/packages/lint/src/index.ts index 544ab3c974..f76e13717e 100644 --- a/packages/lint/src/index.ts +++ b/packages/lint/src/index.ts @@ -755,6 +755,7 @@ export { VIEW_KEY_COLLISION, VIEW_REF_FORM_TARGET_MISSING, VIEW_REF_FORM_TARGET_KIND, + VIEW_REF_NAV_VIEW_MISSING, } from './lint-view-refs.js'; export { diff --git a/packages/lint/src/lint-view-refs.test.ts b/packages/lint/src/lint-view-refs.test.ts index f7ec14e8f9..db93b1d2e0 100644 --- a/packages/lint/src/lint-view-refs.test.ts +++ b/packages/lint/src/lint-view-refs.test.ts @@ -6,7 +6,9 @@ import { VIEW_KEY_COLLISION, VIEW_REF_FORM_TARGET_MISSING, VIEW_REF_FORM_TARGET_KIND, + VIEW_REF_NAV_VIEW_MISSING, } from './lint-view-refs.js'; +import { runAuthoringRules, splitBySeverity } from './authoring-rules.js'; const listView = (object: string) => ({ type: 'grid', @@ -174,3 +176,233 @@ describe('lintViewRefs — form action target resolution', () => { expect(out[0].where).toContain("object 'task'"); // object-nested context retained }); }); + +// ───────────────────────────────────────────────────────────────────────────── +// #14108 — app navigation `viewName`, the SECOND door into the same `listViews` +// namespace the #2554 rules above guard. +// ───────────────────────────────────────────────────────────────────────────── + +/** Distinct configs so the expander does not dedupe the default `list` into a + * structurally identical `listViews` entry — the fixture needs BOTH so the + * default's real key is observable. */ +const navListView = (object: string, label: string, column: string) => ({ + type: 'grid', + label, + columns: [column], + data: { provider: 'object', object }, +}); + +const NAV_OBJECT = { + name: 'duly_task', + list: navListView('duly_task', 'All tasks', 'title'), + listViews: { + schedule: navListView('duly_task', 'Schedule', 'due_at'), + board: navListView('duly_task', 'Board', 'status'), + }, + formViews: { edit: formView('duly_task') }, +}; + +const navStack = (nav: Record, appExtra: Record = {}) => ({ + objects: [NAV_OBJECT], + apps: [{ name: 'duly', label: 'Duly', navigation: [nav], ...appExtra }], +}); + +const navFindings = (stack: Record) => + lintViewRefs(stack).filter((f) => f.rule === VIEW_REF_NAV_VIEW_MISSING); + +describe('lintViewRefs — navigation viewName resolution (#14108)', () => { + it('errors when a nav viewName names no list view on its object (the measured repro)', () => { + const out = navFindings( + navStack({ id: 'nav_schedule', type: 'object', objectName: 'duly_task', viewName: 'A4_no_such_view', label: 'Schedule' }), + ); + expect(out).toHaveLength(1); + expect(out[0].severity).toBe('error'); + expect(out[0].where).toBe("app 'duly' · nav 'nav_schedule'"); + // The available list views are listed the way the platform's other + // unknown-name findings list theirs — short (authorable) keys, not + // expanded `.` ids. + expect(out[0].message).toContain('board, default, schedule'); + expect(out[0].message).not.toContain('duly_task.schedule'); + }); + + it('suggests the near-miss name a rename left behind', () => { + const out = navFindings( + navStack({ id: 'n', type: 'object', objectName: 'duly_task', viewName: 'schedul', label: 'Schedule' }), + ); + expect(out[0].hint).toContain('Did you mean "schedule"?'); + }); + + it('accepts a viewName that names a declared listViews key', () => { + expect(navFindings(navStack({ id: 'n', type: 'object', objectName: 'duly_task', viewName: 'schedule', label: 'S' }))).toEqual([]); + }); + + it("accepts the default `list`'s real expansion key — which is `default`, not `all`", () => { + // The schema documents viewName as 'Defaults to "all"'. That describes the + // CONVENTION of declaring a `listViews.all`, not a magic fallback name: + // `expandViewContainer` keys a bare default `list` as `.default`, + // and objectui's `resolveViewId` has no special case for 'all' either. + expect(navFindings(navStack({ id: 'n', type: 'object', objectName: 'duly_task', viewName: 'default', label: 'D' }))).toEqual([]); + const undeclaredAll = navFindings( + navStack({ id: 'n', type: 'object', objectName: 'duly_task', viewName: 'all', label: 'A' }), + ); + expect(undeclaredAll).toHaveLength(1); + }); + + it("accepts 'all' once the object actually declares it (the app-crm convention)", () => { + const stack = { + objects: [{ ...NAV_OBJECT, listViews: { ...NAV_OBJECT.listViews, all: navListView('duly_task', 'All', 'title') } }], + apps: [{ name: 'duly', navigation: [{ id: 'n', type: 'object', objectName: 'duly_task', viewName: 'all', label: 'A' }] }], + }; + expect(navFindings(stack)).toEqual([]); + }); + + it('accepts a FULLY QUALIFIED viewName, exactly as the runtime matcher does', () => { + // objectui's `resolveViewId` accepts `.` as well as the short + // key. A lint stricter than the matcher would red a name that works. + expect( + navFindings(navStack({ id: 'n', type: 'object', objectName: 'duly_task', viewName: 'duly_task.schedule', label: 'S' })), + ).toEqual([]); + }); + + it('resolves against an independent (already-expanded) top-level ViewItem', () => { + const stack = { + objects: [{ name: 'duly_task' }], + views: [{ name: 'duly_task.mine', object: 'duly_task', viewKind: 'list', ...navListView('duly_task', 'Mine', 'title') }], + apps: [{ name: 'duly', navigation: [{ id: 'n', type: 'object', objectName: 'duly_task', viewName: 'mine', label: 'M' }] }], + }; + expect(navFindings(stack)).toEqual([]); + }); + + it('names the form-view case explicitly rather than reporting a bare miss', () => { + const out = navFindings(navStack({ id: 'n', type: 'object', objectName: 'duly_task', viewName: 'edit', label: 'E' })); + expect(out).toHaveLength(1); + expect(out[0].message).toContain('resolves to a FORM view'); + expect(out[0].hint).toContain("type:'form' action target"); + }); + + it('recurses into `group` children AND into an `object` item’s own children', () => { + const stack = { + objects: [NAV_OBJECT], + apps: [{ + name: 'duly', + navigation: [{ + id: 'grp', type: 'group', label: 'Work', + children: [{ + id: 'parent', type: 'object', objectName: 'duly_task', viewName: 'schedule', label: 'Tasks', + children: [{ id: 'deep', type: 'object', objectName: 'duly_task', viewName: 'A4_no_such_view', label: 'Deep' }], + }], + }], + }], + }; + const out = navFindings(stack); + expect(out).toHaveLength(1); + expect(out[0].where).toContain("nav 'deep'"); + }); + + it('walks the `areas[]` nav container too, not only `navigation`', () => { + const bad = { id: 'in_area', type: 'object', objectName: 'duly_task', viewName: 'A4_no_such_view', label: 'X' }; + for (const area of [{ name: 'a', items: [bad] }, { name: 'a', navigation: [bad] }]) { + const out = navFindings({ objects: [NAV_OBJECT], apps: [{ name: 'duly', areas: [area] }] }); + expect(out).toHaveLength(1); + expect(out[0].where).toContain("nav 'in_area'"); + } + }); + + it('reports each distinct nav entry once', () => { + const stack = { + objects: [NAV_OBJECT], + apps: [{ + name: 'duly', + navigation: [ + { id: 'a', type: 'object', objectName: 'duly_task', viewName: 'A4_no_such_view', label: 'A' }, + { id: 'b', type: 'object', objectName: 'duly_task', viewName: 'A4_no_such_view', label: 'B' }, + ], + }], + }; + expect(navFindings(stack).map((f) => f.where)).toEqual([ + "app 'duly' · nav 'a'", + "app 'duly' · nav 'b'", + ]); + }); +}); + +describe('lintViewRefs — navigation viewName exemptions (false positives stay near zero)', () => { + const bogus = 'A4_no_such_view'; + + it('skips an interpolated viewName (resolved at render time)', () => { + expect(navFindings(navStack({ id: 'n', type: 'object', objectName: 'duly_task', viewName: '${params.view}', label: 'X' }))).toEqual([]); + }); + + it('skips an entry carrying `requiresObject` — another package provides the object', () => { + expect( + navFindings(navStack({ id: 'n', type: 'object', objectName: 'duly_task', viewName: bogus, requiresObject: 'duly_task', label: 'X' })), + ).toEqual([]); + }); + + it('skips when `recordId` is set — the schema documents viewName as ignored there', () => { + expect( + navFindings(navStack({ id: 'n', type: 'object', objectName: 'duly_task', viewName: bogus, recordId: '{current_user_id}', label: 'X' })), + ).toEqual([]); + }); + + it('says nothing about an object this stack declares no list views for', () => { + // Indistinguishable from a cross-package object, so the rule stays silent + // rather than guessing — the precondition that lets it be an ERROR at all. + const stack = { + objects: [{ name: 'other_object' }], + apps: [{ name: 'duly', navigation: [{ id: 'n', type: 'object', objectName: 'not_here', viewName: bogus, label: 'X' }] }], + }; + expect(navFindings(stack)).toEqual([]); + }); + + it('says nothing when the nav entry names no view at all', () => { + expect(navFindings(navStack({ id: 'n', type: 'object', objectName: 'duly_task', label: 'X' }))).toEqual([]); + }); + + it('leaves the repo’s own shipped nav shapes alone (no error on a resolvable stack)', () => { + const stack = { + objects: [NAV_OBJECT], + apps: [{ + name: 'duly', + navigation: [ + { id: 'a', type: 'object', objectName: 'duly_task', viewName: 'schedule', label: 'S', icon: 'calendar' }, + { id: 'b', type: 'object', objectName: 'duly_task', label: 'Default landing' }, + { id: 'c', type: 'page', pageName: 'pricing', label: 'Pricing' }, + { id: 'd', type: 'url', url: 'https://example.com', label: 'Docs' }, + ], + }], + }; + expect(navFindings(stack)).toEqual([]); + }); +}); + +/** + * The card's binding acceptance criterion, pinned end-to-end rather than + * inferred from the registry entry (the #14148 / #14107 precedent): the + * measured repro must fail `validate` AND `build`. The card measured + * `os validate --json` returning `valid: true` and `os build` green, so a + * validate-only fix was not acceptable — and nothing else in this file would + * notice if the suite entry's `commands` were narrowed later. + */ +describe('#14108 acceptance — a nav viewName miss gates `validate` AND `build`', () => { + const repro = navStack({ + id: 'nav_schedule', type: 'object', objectName: 'duly_task', + viewName: 'A4_no_such_view', label: 'Schedule', icon: 'gantt-chart', + }); + const clean = navStack({ + id: 'nav_schedule', type: 'object', objectName: 'duly_task', + viewName: 'schedule', label: 'Schedule', icon: 'gantt-chart', + }); + + for (const command of ['validate', 'build'] as const) { + it(`the measured repro fails \`${command}\``, () => { + const { errors } = splitBySeverity(runAuthoringRules(command, { normalized: repro as never })); + expect(errors.map((f) => f.rule)).toContain(VIEW_REF_NAV_VIEW_MISSING); + }); + + it(`the corrected nav entry passes \`${command}\``, () => { + const { errors, advisories } = splitBySeverity(runAuthoringRules(command, { normalized: clean as never })); + expect([...errors, ...advisories].filter((f) => f.rule === VIEW_REF_NAV_VIEW_MISSING)).toEqual([]); + }); + } +}); diff --git a/packages/lint/src/lint-view-refs.ts b/packages/lint/src/lint-view-refs.ts index ce70fcbe00..5981ca029f 100644 --- a/packages/lint/src/lint-view-refs.ts +++ b/packages/lint/src/lint-view-refs.ts @@ -28,14 +28,57 @@ * may also be a view this lint failed to collect (a non-standard container * shape), so it warns rather than risk a false-positive build failure. * + * view-ref-nav-view-missing — ERROR (fails the build) + * An app navigation entry whose `viewName` resolves to no list view on its + * own object. See the section below — this is the SECOND door into the same + * `listViews` namespace the two rules above guard, and the more travelled one. + * * Deliberately conservative to keep false positives near zero: only `type:'form'` * targets are checked (the one type that unambiguously names a form view), * interpolated targets (`${…}`) are skipped as non-static, and non-qualified * targets (no `.`) are treated as opaque handler/modal refs rather than view * references. + * + * ## The navigation door (`view-ref-nav-view-missing`) + * + * A form action target is one way to name a view; an app's navigation is the + * other, and it is the one an end user travels every day. `ObjectNavItemSchema` + * documents `viewName` as *"Default list view to open. Defaults to 'all'"* — so + * an unresolvable name does not fail, it **falls back**. Measured end to end: + * mutating a real app's `viewName` to a name nothing declares leaves + * `os validate --json` reporting `valid: true`, and `os build` green. + * + * What the author gets instead is a nav entry that keeps its authored label and + * icon and opens a different view — a "Schedule" entry that opens the plain + * grid. The runtime does notice: objectui's `ObjectView` calls the shared + * `resolveViewId` matcher and, on a miss, `console.warn`s and falls back to + * `defaultViewId || views[0]`. A browser-console warning is not an author-time + * signal, which is why the check belongs here. The decay mode is worse than the + * typo mode: renaming a list view silently degrades every nav entry pointing at + * it, with every gate green and the diff reading correctly in review. + * + * **Resolution mirrors the runtime matcher exactly** (`resolveViewId` in + * objectui's `@object-ui/core`), all three directions: exact id, short name + * retried as `.`, and qualified name retried with the + * `.` prefix stripped. Reimplementing a *stricter* match here would red + * names that work at runtime; a looser one would bless names that do not. The + * `'all'` of the schema's doc line is not magic in that matcher either — it + * resolves only when the object actually declares it — so this rule does not + * special-case it. + * + * ERROR rather than the sibling's WARNING, because the false-positive vector + * that made `view-ref-form-target-missing` advisory is closed by construction + * here: the rule fires only when it has already collected a NON-EMPTY list-view + * namespace for that object out of this stack. If the object is absent, carries + * `requiresObject` (an explicit "another package provides this"), or contributed + * no list view this lint could expand, the entry is skipped rather than guessed + * at. What remains outside its knowledge is a runtime-SAVED view (`savedViews` + * in `ObjectView`), which no author-time pass over declared metadata can see — + * the same boundary every rule in this suite has. */ import { expandViewContainerWithDiagnostics, isAggregatedViewContainer } from '@objectstack/spec'; +import { listNames, suggestName } from './object-graph.js'; export interface ViewRefFinding { where: string; @@ -58,6 +101,32 @@ function asArray(v: unknown): AnyRec[] { export const VIEW_KEY_COLLISION = 'view-key-collision'; export const VIEW_REF_FORM_TARGET_MISSING = 'view-ref-form-target-missing'; export const VIEW_REF_FORM_TARGET_KIND = 'view-ref-form-target-kind'; +export const VIEW_REF_NAV_VIEW_MISSING = 'view-ref-nav-view-missing'; + +/** + * An interpolated name resolves at render time — the same conservative + * exemption `validate-nav-target-refs` and `validate-object-references` use to + * keep false positives near zero (ADR-0072 D1). + */ +const isInterpolated = (s: string): boolean => s.includes('${') || s.includes('{'); + +/** + * Resolve a requested view name against an object's actual view ids, in all + * three directions objectui's `resolveViewId` accepts. Kept deliberately in + * lock-step with that matcher (`@object-ui/core`, objectstack#2217): a name + * this returns false for is a name the runtime falls back on. + */ +function resolvesViewId(requested: string, ids: ReadonlySet, object: string): boolean { + if (ids.has(requested)) return true; + const prefix = `${object}.`; + if (!requested.includes('.') && ids.has(prefix + requested)) return true; + if (requested.startsWith(prefix) && ids.has(requested.slice(prefix.length))) return true; + return false; +} + +/** The short, author-facing spelling of an expanded view id (`task.mine` → `mine`). */ +const shortViewName = (id: string, object: string): string => + id.startsWith(`${object}.`) ? id.slice(object.length + 1) : id; /** Pull the view-container slots out of an object definition (ADR-0017 nested * "Object has-many View"). Absent slots stay undefined — the expander ignores @@ -93,12 +162,38 @@ export function lintViewRefs(stack: AnyRec): ViewRefFinding[] { s.add(kind); }; + // Per-object view ids, split by kind — what a navigation `viewName` resolves + // against. Kept beside `viewKinds` (which is keyed by expanded name alone) + // because navigation names a view WITHIN one object's namespace, so the + // owning object is part of the question. + const listViewIds = new Map>(); + const formViewIds = new Map>(); + const indexForObject = (object: string, name: string, kind: 'list' | 'form') => { + const m = kind === 'list' ? listViewIds : formViewIds; + let s = m.get(object); + if (!s) m.set(object, (s = new Set())); + s.add(name); + }; + + /** The object an already-expanded, independent ViewItem binds to. */ + const independentViewObject = (v: AnyRec): string | undefined => { + for (const c of [v.object, v.objectName, v.data?.object, v.list?.data?.object]) { + if (typeof c === 'string' && c) return c; + } + return undefined; + }; + // 1) Gather every aggregated container: top-level `views` + object-nested. const containers: Array<{ object: string; container: AnyRec }> = []; for (const v of asArray(stack.views)) { if (v.viewKind) { // Already an independent, expanded ViewItem — index it directly. - if (typeof v.name === 'string') indexKind(v.name, v.viewKind === 'form' ? 'form' : 'list'); + const kind = v.viewKind === 'form' ? 'form' : 'list'; + if (typeof v.name === 'string') { + indexKind(v.name, kind); + const owner = independentViewObject(v); + if (owner) indexForObject(owner, v.name, kind); + } continue; } if (!isAggregatedViewContainer(v)) continue; @@ -116,7 +211,13 @@ export function lintViewRefs(stack: AnyRec): ViewRefFinding[] { // 2) Expand each container: index names + report every collision as an error. for (const { object, container } of containers) { const { items, collisions } = expandViewContainerWithDiagnostics(object, container); - for (const it of items) indexKind(it.name, it.viewKind); + for (const it of items) { + indexKind(it.name, it.viewKind); + // Index under the item's OWN object (`it.object`), not the container's — + // they are the same here, but the expander is the authority on which + // object a produced item belongs to. + indexForObject(it.object ?? object, it.name, it.viewKind); + } for (const col of collisions) { findings.push({ where: `object '${object}' · view key '${col.key}'`, @@ -186,5 +287,95 @@ export function lintViewRefs(stack: AnyRec): ViewRefFinding[] { } for (const action of asArray(stack.actions)) checkAction(action); + // 4) Validate every app-navigation `viewName` against its object's list views. + // The second door into the same `listViews` namespace as (3) — see the + // header. An entry is reported only when this stack actually declares list + // views for the object it names. + const seenNavRefs = new Set(); + const checkNavItem = (nav: AnyRec, appName: string) => { + const objectName = typeof nav.objectName === 'string' ? nav.objectName : undefined; + const viewName = typeof nav.viewName === 'string' ? nav.viewName : undefined; + if (!objectName || !viewName) return; + if (isInterpolated(viewName)) return; // resolved at render time — not static + + // `recordId` navigates straight to a record; the schema documents + // `viewName` as IGNORED in that combination (and `app.test.ts` pins the + // legacy pairing as tolerated), so the reference is not live. + if (typeof nav.recordId === 'string' && nav.recordId) return; + + // `requiresObject` is the author's explicit "another package provides this + // object" opt-in — the same exemption `validate-object-references` and + // `stack.zod.ts`'s nav cross-reference block honour. Its views are not in + // this stack to resolve against. + if (nav.requiresObject) return; + + // Nothing collected for this object: it may live in another package, or + // carry a container shape this lint could not expand. Indistinguishable + // from a typo, so say nothing rather than guess. + const ids = listViewIds.get(objectName); + if (!ids || ids.size === 0) return; + + if (resolvesViewId(viewName, ids, objectName)) return; + + const navId = + (typeof nav.id === 'string' && nav.id) || (typeof nav.label === 'string' && nav.label) || '(unnamed)'; + // JSON, not a delimiter-joined string: injective without needing a + // separator byte that cannot appear in the parts. + const dedupeKey = JSON.stringify([appName, navId, objectName, viewName]); + if (seenNavRefs.has(dedupeKey)) return; + seenNavRefs.add(dedupeKey); + + const available = [...ids].map((id) => shortViewName(id, objectName)); + const formIds = formViewIds.get(objectName); + const isFormView = !!formIds && resolvesViewId(viewName, formIds, objectName); + + findings.push({ + where: `app '${appName}' · nav '${navId}'`, + message: + `Navigation entry opens view '${viewName}' on object '${objectName}', which declares no such ` + + `list view. ` + + (isFormView + ? `The name resolves to a FORM view of that object, which the object's view switcher never offers. ` + : '') + + `At runtime the name does not resolve and the entry falls back to the object's default view, ` + + `keeping its authored label and icon — so the sidebar still reads correctly while opening the ` + + `wrong view. List views on '${objectName}': ${listNames(available)}.`, + hint: + `Correct the name, declare '${viewName}' in the object's \`listViews\`, or drop \`viewName\` ` + + `to open the default view.` + + (isFormView + ? ` A form view is reachable from a type:'form' action target, not from navigation.` + : '') + + suggestName(viewName, available), + rule: VIEW_REF_NAV_VIEW_MISSING, + severity: 'error', + }); + }; + + const walkNav = (items: unknown, appName: string) => { + if (!Array.isArray(items)) return; + for (const raw of items) { + if (!raw || typeof raw !== 'object' || Array.isArray(raw)) continue; + const nav = raw as AnyRec; + checkNavItem(nav, appName); + // NOT gated on `type === 'group'`: an `object` nav item carries + // `children` too, and a targeted child nested under one would be skipped + // — the same reason `stack.zod.ts` and `validate-nav-target-refs` recurse + // unconditionally. + if (Array.isArray(nav.children)) walkNav(nav.children, appName); + } + }; + + for (const [ai, app] of asArray(stack.apps).entries()) { + const appName = typeof app.name === 'string' && app.name ? app.name : `#${ai}`; + walkNav(app.navigation, appName); + // `areas[]` is the other nav container; it was once skipped wholesale in + // `stack.zod.ts`, so an areas-based app got no nav validation at all. + for (const area of asArray(app.areas)) { + walkNav(area.items, appName); + walkNav(area.navigation, appName); + } + } + return findings; }