diff --git a/test/metadata-bindings.test.ts b/test/metadata-bindings.test.ts index a49b764..03aa7c4 100644 --- a/test/metadata-bindings.test.ts +++ b/test/metadata-bindings.test.ts @@ -42,33 +42,43 @@ import { dulyViews } from '../src/views/index.js'; * `viewName` does not error, it falls back to the object's default view and * KEEPS THE AUTHORED LABEL. The screen looks right and shows the wrong rows. * - * ── Relationship to `test/views.test.ts` — declared, not hidden ────────── - * That file's stopgap half already resolves the SIMPLE view surface (bare - * `columns` / `filter` / `sort` / `grouping` / `rowColor` / binding-block keys - * against the bound object) and the nav `objectName` / `viewName` pair. This - * file is a strict superset of that half, and exists because the superset is - * where the remaining holes are: + * ── Relationship to `test/views.test.ts` — collapsed, not duplicated ──── + * Reference RESOLUTION on views and nav is owned HERE and nowhere else. + * `test/views.test.ts` used to carry a second, weaker copy of it; duly#58 + * deleted that copy after measuring, mutation by mutation, that this walk + * reports every defect the copy did. What stayed there is binding-block + * PRESENCE (objectstack#14106 — "is there a `gantt` block at all") and the + * product pins, neither of which this file checks. Presence and resolution + * are different properties; do not move either one across. * - * 1. **Datasets are not covered there at all**, and `test/datasets.test.ts` + * The five reasons the superset was worth having, and what each is worth now: + * + * 1. **Datasets were not covered there at all**, and `test/datasets.test.ts` * pins caliber, date-macro grammar and the load-bearing absences — never * that a `field` path names anything real. - * 2. **Dotted paths are skipped there by construction**: its checker opens + * 2. **Dotted paths were skipped there by construction**: that checker opened * with `if (!name || name.includes('.')) return`. So `duty.frequency` — - * the one joined path this app ships — is resolved by NOTHING today. - * 3. **Its system-column list is hand-copied** (and therefore already drifted: - * it carries `business_unit_id`, which is not a platform column, and omits + * the one joined path this app ships — was resolved by NOTHING. + * 3. **Its system-column list was hand-copied, and had drifted**: it carried + * `business_unit_id`, which is not a platform column, and omitted * `owning_business_unit_id`, `tenant_id`, `user_id` and `deleted_at`, - * which are). This file reads the platform's own `SystemFieldName`. + * which are. Measured both ways before the deletion — a view column of + * `business_unit_id` passed there and fails here, and one of `deleted_at` + * failed there (a false positive) and passes here. This file reads the + * platform's own `SystemFieldName`, so it cannot drift again. * 4. **A view bound to a platform object would FAIL there**, because the - * bound object must be in `dulyObjects`. Platform objects are resolved + * bound object had to be in `dulyObjects`. Platform objects are resolved * here from the platform's own registry. * 5. **Neither file had a self-test.** A guard that has never been observed * failing is indistinguishable from a guard that cannot fail; the * synthetic fixtures at the bottom pin both directions permanently. * - * Collapsing the two into one is a follow-up, not this card: deleting another - * card's guard is not a rider on this one. Until then the overlap is benign — - * this file is the superset, so any disagreement reds HERE first. + * One thing the older copy DID cover that this walk did not: a nav entry + * nested under an `object` entry rather than a `group`. That was a real hole, + * not a redundancy — `walkNav` recursed on `group` alone — so it was closed + * here (see the note in `walkNav`) BEFORE the copy was deleted, and pinned by + * `reaches nav entries nested under an OBJECT entry` below. A collapse is + * only sound once the surviving guard is a genuine superset. * * ── Narrowings, stated rather than hidden ──────────────────────────────── * A guard people learn to ignore is worse than no guard, so this one only @@ -108,7 +118,9 @@ interface DeclaredObject { /** * Columns the platform puts on every object. Imported from the spec's own * `SystemFieldName` rather than transcribed: a second hand-maintained copy of - * platform knowledge drifts, and `test/views.test.ts`'s copy already has. + * platform knowledge drifts, and the copy `test/views.test.ts` used to carry + * had — it listed `business_unit_id`, which is not a platform column, and + * omitted four that are. That copy is gone (duly#58); this is the only one. */ const SYSTEM_FIELDS: ReadonlySet = new Set(Object.values(SystemFieldName)); @@ -632,10 +644,20 @@ export const metadataBindingFindings = (stack: Stack): WalkResult => { const where = `app ${appName} · nav '${id}'`; const type = String(item.type ?? ''); - if (type === 'group') { - walkNav(item.children as Rec[] | undefined, appName); - continue; - } + /** + * Children are walked for EVERY item type, not only `group`. The spec + * ties the recursive knot on the object branch as well — + * `NavigationItem` is `(ObjectNavItem & { children?: NavigationItem[] }) + * | … | GroupNavItem` — so a nav entry nested under an OBJECT entry is + * legal metadata that the shell renders. Recursing only on `group` left + * those children unvisited: measured on this app by hanging a child + * carrying `viewName: 'ghost_view'` off `nav_log`, this walk stayed + * GREEN while the reference resolved to nothing (duly#58). A `group` + * carries no binding of its own, so it stops here; everything else + * falls through and is resolved below. + */ + walkNav(item.children as Rec[] | undefined, appName); + if (type === 'group') continue; if (type === 'dashboard') { /** * `DashboardNavItemSchema` carries `dashboardName`, not an object, so @@ -804,8 +826,9 @@ describe('metadata bindings — every reference resolves (stopgap for objectstac it('resolves the joined path this app ships, through the lookup to the target field', () => { // `duty.frequency` is the one multi-hop path in the stack, and it is the - // path `test/views.test.ts` skips by construction (`name.includes('.')`). - // Asserting it is REACHED, not merely that nothing failed. + // path the deleted copy in `test/views.test.ts` skipped by construction + // (`if (!name || name.includes('.')) return`). Asserting it is REACHED, + // not merely that nothing failed. expect( result.resolved.some((r) => r.endsWith('→ duty.frequency')), 'the joined path duly_task→duty.frequency was never resolved — the multi-hop walk is not running', @@ -1107,6 +1130,29 @@ describe('metadata bindings — the guard can fail (self-test on synthetic metad expect(r.findings.map((f) => f.where)).toEqual([expect.stringContaining("nav 'deep'")]); }); + it('reaches nav entries nested under an OBJECT entry, which the schema also allows', () => { + // `NavigationItem` ties the `children` knot on the object branch too, so + // this shape is legal metadata. Recursing only on `group` left it + // unvisited and the walk read GREEN on a ghost `viewName` (duly#58) — + // this is the case that kept `test/views.test.ts`'s nav assertion alive + // until the walk was fixed. + const r = run({ + apps: [ + { + name: 'fx_app', + navigation: [ + { + id: 'parent', type: 'object', objectName: 'fx_task', viewName: 'lens', + children: [{ id: 'nested', type: 'object', objectName: 'fx_task', viewName: 'ghost' }], + }, + ], + }, + ], + }); + expect(r.findings.map((f) => f.reference)).toEqual(['ghost']); + expect(r.findings[0]!.where).toContain("nav 'nested'"); + }); + // ── Must NOT fire ────────────────────────────────────────────────────── it('does not fire on a platform-object reference — it records a boundary instead', () => { const r = run({ datasets: [dataset({ include: ['owner'], dimensions: [{ name: 'o', field: 'owner.full_name' }] })] }); diff --git a/test/views.test.ts b/test/views.test.ts index ee973e5..edaaff2 100644 --- a/test/views.test.ts +++ b/test/views.test.ts @@ -3,14 +3,13 @@ import { describe, expect, it } from 'vitest'; import { dulyApps } from '../src/apps/index.js'; -import { dulyObjects } from '../src/objects/index.js'; import { dulyViews } from '../src/views/index.js'; /** * View tests — two jobs, kept apart on purpose. * * ── 1. A STOPGAP for what `pnpm validate` does not see ─────────────────── - * ⚠️ Delete the stopgap half when the upstream rules land. It is not a house + * ⚠️ Delete the stopgap half when the upstream rule lands. It is not a house * rule that wants maintaining forever; it is author-time coverage every * ObjectStack app would otherwise re-implement, and it is written to be * removed rather than kept in step with the platform: @@ -18,14 +17,26 @@ import { dulyViews } from '../src/views/index.js'; * objectstack-ai/objectstack#14106 — `view/layout-without-binding` covers * `kanban` / `calendar` / `gantt` only. `timeline`, `tree` and `map` have * the same literal-default fallback in the renderer and no gate. - * objectstack-ai/objectstack#14107 — NO field reference on a list view is - * resolved at author time: not `columns`, `filter`, `sort`, `grouping`, - * nor any binding block. Neither `os validate` NOR `os build` sees it. - * objectstack-ai/objectstack#14108 — a nav `viewName` naming a view that - * does not exist silently opens the default view instead. * - * All three were measured against this repo on `@objectstack/cli` 17.2.0 by - * mutating a view and re-running the gates. The readings are in the issues. + * Measured against this repo on `@objectstack/cli` 17.2.0 by mutating a view + * and re-running the gates. The reading is in the issue. + * + * What is left here is binding-block PRESENCE and nothing else: "is there a + * `gantt` block at all", which is what #14106 is about. + * + * ── Reference RESOLUTION lives in `test/metadata-bindings.test.ts` ─────── + * This file used to carry a second, weaker copy of it — one assertion + * resolving view field names, one resolving nav `viewName` (#14107 / #14108). + * duly#58 deleted both after measuring, mutation by mutation, that the walk + * in `test/metadata-bindings.test.ts` reports every defect they did. It is a + * genuine superset: it resolves dotted paths (the copy here returned early on + * every one), reads the platform's own `SystemFieldName` registry instead of + * a hand-copied list that had already drifted, and covers datasets, bulk + * actions and dashboards besides. + * + * Presence and resolution are DIFFERENT properties. Do not move the check + * below across to that file, and do not re-grow a resolution check here: two + * guards on one rule is the cost both headers warn against. * * ── 2. PRODUCT pins that outlive the platform gaps ─────────────────────── * The gantt starting at `visible_from`, the timeline ordering by @@ -35,21 +46,6 @@ import { dulyViews } from '../src/views/index.js'; type Rec = Record; -const objectFields = new Map>( - (dulyObjects as unknown as Array<{ name: string; fields: Rec }>).map( - (o) => [o.name, new Set(Object.keys(o.fields))] as const, - ), -); - -/** - * Fields the platform provides on every object. A view may legitimately name - * one, and they are not in the authored `fields` map. - */ -const SYSTEM_FIELDS = new Set([ - 'id', 'created_at', 'updated_at', 'created_by', 'updated_by', 'owner_id', - 'organization_id', 'business_unit_id', -]); - interface NamedView { /** e.g. `duly_task › listViews.board` — the string an assertion failure prints. */ where: string; @@ -75,9 +71,6 @@ const byName = (name: string): NamedView => { return found; }; -const fieldOf = (col: unknown): string | undefined => - typeof col === 'string' ? col : typeof (col as Rec)?.field === 'string' ? (col as Rec).field as string : undefined; - describe('every non-grid lens is bound to real fields', () => { /** * The binding block each view type needs, and the keys inside it that MUST @@ -119,47 +112,11 @@ describe('every non-grid lens is bound to real fields', () => { }); /** - * #14107: a misspelt field name anywhere on a view is parse-clean, publishes - * green, and renders blank. Resolve every one of them here instead. + * Whether the names INSIDE that block resolve is a different property, and + * it is checked in `test/metadata-bindings.test.ts` (#14107) — over dotted + * paths, the platform's real system columns, and every binding-block key + * the Zod schemas declare, none of which this file did. */ - it('names only fields that exist on the object it is bound to', () => { - const bindingFieldKeys = [ - 'groupByField', 'summarizeField', 'startDateField', 'endDateField', 'titleField', - 'colorField', 'labelField', 'parentField', 'progressField', 'dependenciesField', - 'assigneeField', 'effortField', 'baselineStartField', 'baselineEndField', - 'locationField', 'latitudeField', 'longitudeField', 'coverField', 'allDayField', - ]; - - for (const { where, object, view } of allViews) { - const known = objectFields.get(object); - expect(known, `${where}: bound to "${object}", which no object in this stack defines`).toBeDefined(); - const check = (name: string | undefined, at: string) => { - if (!name || name.includes('.')) return; - expect( - known!.has(name) || SYSTEM_FIELDS.has(name), - `${where}: ${at} names "${name}", which is not a field on ${object}. ` - + `Fields: ${[...known!].sort().join(', ')}`, - ).toBe(true); - }; - - for (const col of (view.columns as unknown[]) ?? []) check(fieldOf(col), 'columns[]'); - for (const rule of (view.filter as Rec[]) ?? []) check(rule.field as string, 'filter[].field'); - if (Array.isArray(view.sort)) for (const s of view.sort as Rec[]) check(s.field as string, 'sort[].field'); - for (const g of ((view.grouping as Rec | undefined)?.fields as Rec[]) ?? []) { - check(g.field as string, 'grouping.fields[].field'); - } - check((view.rowColor as Rec | undefined)?.field as string, 'rowColor.field'); - - for (const spec of Object.values(REQUIRED_BINDINGS)) { - const block = view[spec.block] as Rec | undefined; - if (!block) continue; - for (const key of bindingFieldKeys) check(block[key] as string, `${spec.block}.${key}`); - for (const col of (block.columns as unknown[]) ?? []) check(fieldOf(col), `${spec.block}.columns[]`); - for (const f of (block.fields as unknown[]) ?? []) check(fieldOf(f), `${spec.block}.fields[]`); - for (const t of (block.tooltipFields as unknown[]) ?? []) check(fieldOf(t), `${spec.block}.tooltipFields[]`); - } - } - }); }); describe('the lenses say what the product means', () => { @@ -271,30 +228,12 @@ describe('navigation', () => { for (const app of dulyApps as unknown as Array<{ navigation?: NavItem[] }>) walk(app.navigation); /** - * #14108: a nav entry naming a view that does not exist does not fail — it - * falls back to the default view, keeps its authored label, and looks right - * in review. This is the check that would have caught it. + * That a nav entry's `objectName` / `viewName` RESOLVE (#14108) is checked + * in `test/metadata-bindings.test.ts`, which also reaches entries nested + * under an `object` parent and resolves `dashboardName` and `filters` keys. + * The two tests below are the other question — reachability and placement — + * and no reference walk can answer either. */ - it('every nav entry resolves to a view that exists on the object it names', () => { - for (const item of navItems) { - if (item.type !== 'object' || !item.objectName) continue; - expect( - objectFields.has(item.objectName), - `nav "${item.id}" targets object "${item.objectName}", which this stack does not define`, - ).toBe(true); - if (!item.viewName) continue; // no viewName = the object's default list - const known = allViews - .filter((v) => v.object === item.objectName) - .map((v) => v.where.split('listViews.')[1]) - .filter(Boolean); - expect( - known, - `nav "${item.id}" opens viewName "${item.viewName}", which ${item.objectName} does not declare — ` - + 'the shell silently falls back to the default view', - ).toContain(item.viewName); - } - }); - it('every lens this app adds is reachable from navigation', () => { const reachable = new Set(navItems.map((i) => `${i.objectName}.${i.viewName}`)); const expected = [