Skip to content

Collapse views.test.ts's binding stopgap into metadata-bindings.test.ts #97

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
os-warren merged 1 commit into from
Sep 1, 2026
Merged

Collapse views.test.ts's binding stopgap into metadata-bindings.test.ts #97

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
92 changes: 69 additions & 23 deletions test/metadata-bindings.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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<string> = new Set(Object.values(SystemFieldName));

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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',
Expand Down Expand Up @@ -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' }] })] });
Expand Down
119 changes: 29 additions & 90 deletions test/views.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,29 +3,40 @@
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:
*
* 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
Expand All @@ -35,21 +46,6 @@ import { dulyViews } from '../src/views/index.js';

type Rec = Record<string, unknown>;

const objectFields = new Map<string, Set<string>>(
(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;
Expand All @@ -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
Expand Down Expand Up @@ -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', () => {
Expand Down Expand Up @@ -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 = [
Expand Down
Loading