Skip to content

refactor(lint): converge the three view-container ladder traversals onto one shared walker (#6381) #6657

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-project-manager merged 1 commit into from
Aug 8, 2026
+440 −169
Merged

refactor(lint): converge the three view-container ladder traversals onto one shared walker (#6381) #6657

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
102 changes: 26 additions & 76 deletions packages/lint/src/validate-form-layout.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,15 @@
*
* Scope: every form view reachable from a `views[]` entry — the entry itself
* when it IS a bare form view, plus the container's default `form` and each
* `formViews.<key>` (see {@link formViewSites} for why reading only the first
* shape left both rules reporting clean on real app metadata, #6251). Forms
* embedded inside page component trees are a follow-up — the walker
* deliberately stays shallow so it never guesses at an arbitrary component's
* object binding.
* `formViews.<key>`, through the shared `view-walk.ts` ladder (#6381; see
* {@link formViewSites} for why reading only the first shape left both rules
* reporting clean on real app metadata, #6251). Forms embedded inside page
* component trees are a follow-up — the walk deliberately stays shallow so it
* never guesses at an arbitrary component's object binding.
*/

import { formViewSites } from './view-walk.js';

export const FORM_FIELD_UNKNOWN = 'form-field-unknown';
export const FORM_COLSPAN_ABSOLUTE = 'absolute-colspan-discouraged';

Expand Down Expand Up @@ -93,76 +95,21 @@ function collectionEntries(v: unknown, base: string): Array<{ rec: AnyRec; path:
}

/**
* Every FORM VIEW reachable from one `views[]` entry, with the path each sits at.
*
* **Copied from `validate-visibility-predicates.ts`'s `formViewSites` (#6248)**
* rather than re-derived: that file fixed this exact traversal hole on the
* sibling rule one PR earlier, and a second hand-rolled ladder is how two rules
* on one surface start disagreeing about which forms exist. The only thing added
* here is the object binding each site inherits (below) — this rule resolves a
* field reference, the visibility rules do not.
*
* Two shapes, and reading only the first is how BOTH rules in this file were
* dead on real app metadata until #6251 measured it. `os build` on
* `examples/app-showcase` emits its form sections at
* `views[0].formViews.edit.sections[…]`; the traversal read `views[0].sections`,
* found nothing, and reported clean on a stack that DOES carry form sections:
*
* - **View CONTAINER** (the runtime app shape). `ViewSchema` declares exactly
* `name` / `label` / `object` / `list` / `form` / `listViews` / `formViews`
* (`view.zod.ts:1890-1903` — the strict error map spells the container's own
* keys out in prose). Form sections therefore live one level down, under
* `form` and each `formViews.<key>`.
* - **A bare FORM VIEW** (`FormViewSchema`, `view.zod.ts:1623-1624`), whose
* `sections` / `groups` sit at the top.
*
* `list` / `listViews.<key>` are `ObjectListViewSchema`
* (`view.zod.ts:1838` — `ListViewSchema` minus `userFilters`) and carry no
* `sections` at all, so they are deliberately NOT walked. This is the one point
* where the other in-repo ladder, `validate-translatable-sections.ts`'s
* `collectViewSites`, is wider: it also visits `listViews.*.sections`. Measured
* against the schema, that rung can only ever read `undefined` — it costs
* nothing there and buys nothing here, so the narrower #6248 ladder is the one
* copied. Both agree on every rung that can hold a section.
*
* `objects[].views` is deliberately absent for the reason #6248 states:
* `object.zod.ts:1833` tombstones the key by name ("`views` is not an
* ObjectSchema field"), so a branch keyed on it could only fire for stacks the
* schema already rejects — the phantom check #4984 / #5017 removed elsewhere.
* The bare-form site (the `views[]` entry itself) is NOT a phantom check, and
* the distinction is worth keeping straight where this rule reads it: strict
* `ViewSchema` refuses a `views[]` entry carrying root `sections` — measured,
* `unrecognized_keys` naming `sections` — so on a parsed `defineStack` config
* only the container rungs can fire. But this rule is registered
* `input: 'parsed'`, and `os lint` never parses: `runAuthoringRules` hands
* `parsed` rules the NORMALIZED stack, where a raw (non-`defineStack`) config's
* root `sections` is still present and still the author's mistake to hear about.
*
* The bare-form site (the entry itself) is NOT such a phantom, and the
* distinction is worth keeping straight: strict `ViewSchema` refuses a `views[]`
* entry carrying root `sections` — measured, `unrecognized_keys` naming
* `sections` — so on a parsed `defineStack` config only the container rungs can
* fire. But this rule is registered `input: 'parsed'`, and `os lint` never
* parses: `runAuthoringRules` hands `parsed` rules the NORMALIZED stack, where a
* raw (non-`defineStack`) config's root `sections` is still present and still
* the author's mistake to hear about.
* The ladder itself — which rungs exist, which are filtered, and the schema
* proof behind each — lives once in `view-walk.ts` (#6381). It used to be a
* verbatim copy of `validate-visibility-predicates.ts`'s walker (#6248 → #6251);
* a third independent copy in `validate-translatable-sections.ts` made three,
* and three copies is how the next author fixes one and leaves two behind.
*/
function formViewSites(
view: AnyRec,
basePath: string,
): Array<{ form: AnyRec; path: string; surface: string }> {
// `surface` names the sub-container in the human-readable `where`. It earns
// its place on exactly the shape this traversal was extended for: a runtime
// container carries neither `name` nor `object` in the emitted artifact, so
// without it every finding under one view reads `view "views[0]"` and the
// author cannot tell the `edit` form from the `create` one.
const sites = [{ form: view, path: basePath, surface: '' }];
const dflt = view.form;
if (isRec(dflt)) {
sites.push({ form: dflt, path: `${basePath}.form`, surface: 'form' });
}
const named = view.formViews;
if (isRec(named)) {
for (const [key, sub] of Object.entries(named)) {
if (isRec(sub)) {
sites.push({ form: sub, path: `${basePath}.formViews.${key}`, surface: `formViews.${key}` });
}
}
}
return sites;
}

/** A section field entry is either a bare field name or `{ field, colSpan, … }`. */
function fieldNameOf(entry: unknown): string | null {
Expand Down Expand Up @@ -227,8 +174,11 @@ export function validateFormLayout(stack: AnyRec): FormLayoutFinding[] {
for (const site of formViewSites(view, viewPath)) {
// A sub-container declares its own binding (`form.data.object`) and
// otherwise inherits the container's — the resolution order every other
// view-walking rule in this package uses.
const objName = boundObject(site.form) ?? containerObject;
// view-walking rule in this package uses. Deliberately NOT folded into
// the shared walker: the three consumers compose this ladder differently
// (see `view-walk.ts`), and a refactor that changes a verdict is a failed
// refactor.
const objName = boundObject(site.view) ?? containerObject;
// Only reference-check when the bound object resolves; otherwise we can't.
const known = objName ? objectFields.get(objName) : undefined;
const where = site.surface ? `view "${viewName}" · ${site.surface}` : `view "${viewName}"`;
Expand All @@ -239,7 +189,7 @@ export function validateFormLayout(stack: AnyRec): FormLayoutFinding[] {
// the canonical spelling is silent on the legacy one, which is exactly the
// half-coverage this issue is about.
for (const bucket of ['sections', 'groups'] as const) {
const sections = Array.isArray(site.form[bucket]) ? (site.form[bucket] as unknown[]) : [];
const sections = Array.isArray(site.view[bucket]) ? (site.view[bucket] as unknown[]) : [];

for (let s = 0; s < sections.length; s++) {
const sec = sections[s];
Expand Down
68 changes: 32 additions & 36 deletions packages/lint/src/validate-translatable-sections.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,10 @@
*
* - a view container's `sections`, its DEFAULT `form.sections` (#5415 — the
* anchor that is neither a `formViews.*` entry nor the record's own), and
* every `listViews.*` / `formViews.*` sub-container's `sections`;
* every `listViews.*` / `formViews.*` sub-container's `sections` — reached
* through the shared `view-walk.ts` ladder (#6381; never a private copy,
* for the reason that file's header states — three copies of this descent
* had each been fixed separately, twice for the same missing rung);
* - the same three on views embedded in an object (`objects[].views`,
* `objects[].listViews`);
* - `record:details` sections nested anywhere in a page's component tree,
Expand All @@ -86,6 +89,7 @@
*/

import { walkPageComponents } from './page-walk.js';
import { viewContainerSites } from './view-walk.js';

export const TRANSLATION_SECTION_NAME_MISSING = 'translation-section-name-missing';

Expand Down Expand Up @@ -181,49 +185,41 @@ function joinWhere(...parts: string[]): string {
/**
* Register every `sections` array ONE view container declares.
*
* The binding ladder mirrors `validate-translation-references.ts`'s
* `collectViewRecord` exactly: a sub-container resolves its own object first
* and falls back to the record's, then to the default list's — because on the
* canonical shape the binding lives INSIDE the container (`list.data.object`),
* not at the record root.
* The DESCENT is the shared one (`view-walk.ts`, #6381) — the entry itself, the
* container's default `form` (#5415: the anchor that is neither a `formViews.*`
* entry nor the record's own), and every `listViews.*` / `formViews.*`
* sub-container. This rule takes the FULL ladder, `listViews.*` included: that
* rung is how it reaches an object's own `listViews` container, which the module
* docblock above declares as part of its section face.
*
* The BINDING ladder stays here, because it is this rule's own: it mirrors
* `validate-translation-references.ts`'s `collectViewRecord` — a sub-container
* resolves its own object first and falls back to the record's, then to the
* default list's, because on the canonical shape the binding lives INSIDE the
* container (`list.data.object`), not at the record root. The sibling rules
* compose their fallbacks differently and folding them together would change
* verdicts.
*
* One equivalence worth writing down, since it is what let the two branches
* collapse into one: the entry's OWN site used to resolve `recordObject ??
* listBinding` while sub-containers resolved `viewObjectName(sub) ??
* recordObject ?? listBinding`. For the entry, `viewObjectName(view)` IS
* `recordObject`, so the sub-container formula returns exactly the same answer
* on it — the uniform expression below is the old two-branch behaviour, not a
* widening of it.
*/
function collectViewSites(view: AnyRec, basePath: string, label: string, sites: SectionSite[]): void {
const recordObject = viewObjectName(view);
const listBinding = isRec(view.list) ? viewObjectName(view.list) ?? recordObject : undefined;
const bindingOf = (container: AnyRec): string | undefined =>
viewObjectName(container) ?? recordObject;

sites.push({
path: `${basePath}.sections`,
surface: label,
objectName: recordObject ?? listBinding,
sections: view.sections,
});

// The container's DEFAULT form — the one `defineView({ form: … })` declares
// and `ObjectForm` renders when no named form view is asked for (#5415).
if (isRec(view.form)) {
for (const site of viewContainerSites(view, basePath)) {
sites.push({
path: `${basePath}.form.sections`,
surface: joinWhere(label, 'form'),
objectName: bindingOf(view.form) ?? listBinding,
sections: view.form.sections,
path: `${site.path}.sections`,
surface: joinWhere(label, site.surface),
objectName: viewObjectName(site.view) ?? recordObject ?? listBinding,
sections: site.view.sections,
});
}

for (const key of ['listViews', 'formViews'] as const) {
const container = view[key];
if (!isRec(container)) continue;
for (const [subKey, sub] of Object.entries(container)) {
if (!isRec(sub)) continue;
sites.push({
path: `${basePath}.${key}.${subKey}.sections`,
surface: joinWhere(label, `${key}.${subKey}`),
objectName: bindingOf(sub) ?? listBinding,
sections: sub.sections,
});
}
}
}

/** Every object name some translation bundle carries a node for. */
Expand Down
65 changes: 8 additions & 57 deletions packages/lint/src/validate-visibility-predicates.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,10 +77,12 @@
*
* Scope: `views` — every form view reachable from a `views[]` entry (the entry
* itself when it IS a form view, plus the container's `form` and each
* `formViews.<key>`; see {@link formViewSites} for why reading only the first
* shape left this rule reporting clean on real metadata) — and `pages`, through
* the shared `walkPageComponents` traversal. Data-field `visibleWhen` is already
* covered by `validate-expressions` and is not re-checked here.
* `formViews.<key>`), through the shared `view-walk.ts` ladder (#6381; see
* {@link formViewSites} for why reading only the first shape left this rule
* reporting clean on real metadata, and why `listViews.<key>` is filtered out
* rather than absent) — and `pages`, through the shared `walkPageComponents`
* traversal. Data-field `visibleWhen` is already covered by
* `validate-expressions` and is not re-checked here.
*
* The predicate family is read off the schema, not guessed: `visibleWhen` is the
* canonical key on all three carriers (`FormFieldBaseSchema` `view.zod.ts:1416`,
Expand Down Expand Up @@ -226,6 +228,7 @@ import { collectCelRootIdentifiers, firstUndeclaredReference, parseCelToAst } fr
import type { CelAstNode } from '@objectstack/formula';

import { walkPageComponents } from './page-walk.js';
import { formViewSites } from './view-walk.js';

export const VISIBILITY_ROOT_MISLAYERED = 'visibility-root-mislayered';
export const VISIBILITY_BARE_IDENTIFIER = 'visibility-bare-identifier';
Expand Down Expand Up @@ -650,58 +653,6 @@ function isFieldObject(entry: unknown): entry is AnyRec {
return !!entry && typeof entry === 'object' && !Array.isArray(entry);
}

/**
* Every FORM VIEW reachable from one `views[]` entry, with the path each sits at.
*
* Two shapes, and reading only the first is how this rule was dead on real
* metadata until #6128 measured it. `os build` on `examples/app-showcase` emits
* its one form predicate at
* `views[0].formViews.edit.sections[0].fields[6].visibleWhen` — the traversal
* read `views[0].sections`, found nothing, and reported clean on a stack that
* DOES carry a view-form predicate:
*
* - **View CONTAINER** (the runtime app shape). `ViewSchema` declares exactly
* `name` / `label` / `object` / `list` / `form` / `listViews` / `formViews`
* (`view.zod.ts:1890-1903` — the strict error map spells the container's own
* keys out in prose). Form sections therefore live one level down, under
* `form` and each `formViews.<key>`; `list` / `listViews.<key>` are
* `ObjectListViewSchema` and carry no `sections`, so they are not walked.
* - **A bare FORM VIEW** (`FormViewSchema`, `view.zod.ts:1623-1624`), whose
* `sections` / `groups` sit at the top. This is the `defineForm` shape the
* `*.form.ts` metadata-editing forms use, i.e. the `layer: 'metadata'` caller.
*
* `objects[].views` is deliberately absent: `object.zod.ts:1833` tombstones the
* key ("`views` is not an ObjectSchema field"), so a branch keyed on it could
* only ever fire for stacks the schema already rejects by name — the phantom
* check #4984 / #5017 removed from two neighbouring rules. Object-level
* `listViews` (`object.zod.ts:1616`) is a list view, so it carries none of this
* either.
*/
function formViewSites(
view: AnyRec,
basePath: string,
): Array<{ form: AnyRec; path: string; surface: string }> {
// `surface` names the sub-container in the human-readable `where`. It earns
// its place on exactly the shape this traversal was extended for: a runtime
// container carries neither `name` nor `object` in the emitted artifact, so
// without it every finding under one view reads `view "views[0]"` and the
// author cannot tell the `edit` form from the `tabbed` one.
const sites = [{ form: view, path: basePath, surface: '' }];
const dflt = view.form;
if (dflt && typeof dflt === 'object' && !Array.isArray(dflt)) {
sites.push({ form: dflt as AnyRec, path: `${basePath}.form`, surface: 'form' });
}
const named = view.formViews;
if (named && typeof named === 'object' && !Array.isArray(named)) {
for (const [key, sub] of Object.entries(named as AnyRec)) {
if (sub && typeof sub === 'object' && !Array.isArray(sub)) {
sites.push({ form: sub as AnyRec, path: `${basePath}.formViews.${key}`, surface: `formViews.${key}` });
}
}
}
return sites;
}

/**
* Validate conditional-visibility predicates across authored views and pages.
*
Expand Down Expand Up @@ -743,7 +694,7 @@ export function validateVisibilityPredicates(
// `sections` (canonical) and `groups` (legacy alias → sections) both hold
// FormSection objects with an optional visibility predicate + `fields`.
for (const bucket of ['sections', 'groups'] as const) {
const sections = Array.isArray(site.form[bucket]) ? (site.form[bucket] as unknown[]) : [];
const sections = Array.isArray(site.view[bucket]) ? (site.view[bucket] as unknown[]) : [];
for (let s = 0; s < sections.length; s++) {
const sec = sections[s];
if (!sec || typeof sec !== 'object') continue;
Expand Down
Loading
Loading