From dcf1f609bebad01699b030f1c8173ae7bebe4d08 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 10 Aug 2026 02:06:49 +0000 Subject: [PATCH] refactor(lint): converge the triplicated `collectionEntries` and view binding ladder (#6662) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #6381 converged the "views[] entry to its real form/view sites" DESCENT onto one shared walker (`view-walk.ts`), and deliberately left two smaller helpers used by the very same rules at three copies each. This is that follow-up, now that #6422 has closed and `validate-translation-references.ts` is no longer held. 1. `collectionEntries` — 3 copies, now one (`collection-entries.ts`): validate-form-layout.ts / validate-translatable-sections.ts (byte-identical) validate-visibility-predicates.ts (same function, predicates open-coded) 2. The binding ladder `objectName -> object -> data.object` — 3 copies under two names, now one (`viewObjectName`, exported from `view-walk.ts`): boundObject in validate-form-layout.ts viewObjectName in validate-translatable-sections.ts viewObjectName in validate-translation-references.ts Only the BASE ladder is shared. Each rule's fallback COMPOSITION stays in its own file, because they differ on purpose and #6657 preserved that deliberately: form-layout falls back to the container, translatable-sections to the container and then to the default `list`'s binding, translation-references to the record, and visibility-predicates needs no binding at all. `lint-view-refs.ts` is untouched: its deeper ladder was judged reasoned difference rather than drift by #6381. Verdicts are unchanged, measured rather than asserted: a temporary differential (not committed) ran all four rules against their origin/main baselines over 2520 generated stacks each -- 10,080 rule runs, 11,624 findings compared with JSON.stringify so order counts -- byte-identical throughout. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01F8q5J1MQyocgtNspb15fSn --- packages/lint/src/collection-entries.test.ts | 187 ++++++++++++++++++ packages/lint/src/collection-entries.ts | 82 ++++++++ packages/lint/src/validate-form-layout.ts | 64 +----- .../src/validate-translatable-sections.ts | 44 +---- .../src/validate-translation-references.ts | 10 +- .../src/validate-visibility-predicates.ts | 27 +-- packages/lint/src/view-walk.test.ts | 164 ++++++++++++++- packages/lint/src/view-walk.ts | 56 +++++- 8 files changed, 497 insertions(+), 137 deletions(-) create mode 100644 packages/lint/src/collection-entries.test.ts create mode 100644 packages/lint/src/collection-entries.ts diff --git a/packages/lint/src/collection-entries.test.ts b/packages/lint/src/collection-entries.test.ts new file mode 100644 index 0000000000..d0b3359d22 --- /dev/null +++ b/packages/lint/src/collection-entries.test.ts @@ -0,0 +1,187 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * The shared stack-collection enumeration (#6662). + * + * Three rules had their own copy of this helper — `validate-form-layout`, + * `validate-translatable-sections`, `validate-visibility-predicates` — and two + * of the three were byte-identical while the third open-coded its record + * predicate inline. This file pins the behaviour ONCE, where it now lives, and + * then asserts the property the convergence buys: all three consumers report + * the same paths for the same collection. + */ + +import { describe, expect, it } from 'vitest'; + +import { collectionEntries } from './collection-entries.js'; +import { validateFormLayout } from './validate-form-layout.js'; +import { validateTranslatableSections } from './validate-translatable-sections.js'; +import { validateVisibilityPredicates } from './validate-visibility-predicates.js'; + +type AnyRec = Record; + +describe('collectionEntries — the array shape', () => { + it('yields each record at its index path', () => { + const a = { name: 'a' }; + const b = { name: 'b' }; + expect(collectionEntries([a, b], 'views')).toEqual([ + { rec: a, path: 'views[0]' }, + { rec: b, path: 'views[1]' }, + ]); + }); + + it('hands back the caller’s own record, not a copy', () => { + // Consumers mutate nothing, but they DO compare identity against the sites + // `view-walk.ts` yields, so a defensive copy here would break that. + const rec = { name: 'a' }; + expect(collectionEntries([rec], 'views')[0].rec).toBe(rec); + }); + + it('skips non-records but keeps the index of the records it keeps', () => { + // The index is the AUTHORED position, so a skipped entry must not shift the + // ones after it — the path has to be one the author can look up. + const rec = { name: 'real' }; + expect(collectionEntries([null, 'str', 42, rec], 'views')).toEqual([ + { rec, path: 'views[3]' }, + ]); + }); + + it('skips a NESTED array — an array is not a record', () => { + expect(collectionEntries([[{ name: 'a' }]], 'views')).toEqual([]); + }); +}); + +describe('collectionEntries — the name-keyed map shape', () => { + it('yields each record at its key path, with the key spread in as `name`', () => { + // The map key IS the entry's name on this shape. Reporting `views[0]` for + // it would name a position that does not exist in the author's file. + expect(collectionEntries({ case_views: { object: 'crm_case' } }, 'views')).toEqual([ + { rec: { name: 'case_views', object: 'crm_case' }, path: 'views.case_views' }, + ]); + }); + + it('lets an entry’s OWN `name` win over the map key', () => { + // `{ name, ...def }` — key first, so the spread overwrites it. + expect(collectionEntries({ keyed: { name: 'declared' } }, 'views')[0].rec).toEqual({ + name: 'declared', + }); + }); + + it('skips non-record values', () => { + const entries = collectionEntries({ a: null, b: 'str', c: 42, d: [], e: { ok: true } }, 'views'); + expect(entries).toEqual([{ rec: { name: 'e', ok: true }, path: 'views.e' }]); + }); +}); + +describe('collectionEntries — what is not a collection', () => { + it.each([ + ['null', null], + ['undefined', undefined], + ['a string', 'views'], + ['a number', 42], + ['a boolean', true], + ])('returns nothing for %s', (_label, v) => { + expect(collectionEntries(v, 'views')).toEqual([]); + }); + + it('returns nothing for an empty collection of either shape', () => { + expect(collectionEntries([], 'views')).toEqual([]); + expect(collectionEntries({}, 'views')).toEqual([]); + }); +}); + +/** + * The one textual divergence between the copies this file converged (#6662). + * + * `validate-visibility-predicates` open-coded its record predicate inline, and + * its MAP-branch guard read `v && typeof v === 'object'` with no + * `!Array.isArray(v)` — where the other two copies called `isRec`, which has + * that third clause. The two are the same function because the ARRAY branch + * returns unconditionally, so the map guard is only ever evaluated on a value + * that is already not an array. This asserts that domination directly: an array + * must never be enumerated as a map, however it is decorated. + */ +describe('collectionEntries — an array is never enumerated as a map', () => { + it('reads only index entries, never an array’s other own keys', () => { + const arr: unknown[] & { extra?: AnyRec } = [{ name: 'indexed' }]; + arr.extra = { name: 'not_an_entry' }; + + expect(collectionEntries(arr, 'views')).toEqual([ + { rec: { name: 'indexed' }, path: 'views[0]' }, + ]); + }); + + it('yields index paths for an empty-but-decorated array, not key paths', () => { + const arr: unknown[] & { case_views?: AnyRec } = []; + arr.case_views = { object: 'crm_case' }; + + expect(collectionEntries(arr, 'views')).toEqual([]); + }); +}); + +/** + * The property the convergence buys: ONE coercion, THREE consumers. + * + * Before #6662 each of these rules decided on its own what path to print for a + * map-shaped collection. Break the coercion now and all three columns go red + * together, which is the whole point — the failure this class of duplication + * produces is the next author fixing one copy and leaving two behind (#6381's + * own history: #6128 / #6248, then #6251). + */ +describe('one coercion, three consumers (#6662)', () => { + const objects = [{ name: 'crm_case', fields: { subject: {}, status: {} } }]; + const translations = [{ 'zh-CN': { objects: { crm_case: { label: '个案' } } } }]; + + /** A form body that trips all three rules at once, at one site. */ + const body = () => ({ + sections: [ + { + label: 'Basics', // → translatable-sections + fields: [ + 'ghost_field', // → form-layout (unknown field) + { field: 'subject', visibleWhen: 'status == "open"' }, // → visibility + ], + }, + ], + }); + + const view = () => ({ object: 'crm_case', ...body() }); + + it('all three report the ARRAY path for an array-shaped `views`', () => { + const stack = { objects, views: [view()], translations }; + + expect(validateVisibilityPredicates(stack).map((f) => f.path)).toEqual([ + 'views[0].sections[0].fields[1]', + ]); + expect(validateFormLayout(stack).map((f) => f.path)).toEqual([ + 'views[0].sections[0].fields[0]', + ]); + expect(validateTranslatableSections(stack).map((f) => f.path)).toEqual([ + 'views[0].sections[0]', + ]); + }); + + it('all three report the KEY path for a map-shaped `views`', () => { + const stack = { objects, views: { case_views: view() }, translations }; + + expect(validateVisibilityPredicates(stack).map((f) => f.path)).toEqual([ + 'views.case_views.sections[0].fields[1]', + ]); + expect(validateFormLayout(stack).map((f) => f.path)).toEqual([ + 'views.case_views.sections[0].fields[0]', + ]); + expect(validateTranslatableSections(stack).map((f) => f.path)).toEqual([ + 'views.case_views.sections[0]', + ]); + }); + + it('all three take the map KEY as the entry’s name in the `where` line', () => { + // The unnamed-but-keyed entry: nothing declares `name`, so the key is the + // only thing that can locate it for the author. + const stack = { objects, views: { case_views: view() }, translations }; + + expect(validateVisibilityPredicates(stack)[0].where).toContain('case_views'); + expect(validateFormLayout(stack)[0].where).toContain('case_views'); + expect(validateTranslatableSections(stack)[0].where).toContain('case_views'); + }); +}); diff --git a/packages/lint/src/collection-entries.ts b/packages/lint/src/collection-entries.ts new file mode 100644 index 0000000000..bddd7f2615 --- /dev/null +++ b/packages/lint/src/collection-entries.ts @@ -0,0 +1,82 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * Shared stack-collection enumeration (issue #6662) — the one coercion from a + * collection authored EITHER as an array OR as a name-keyed map into the + * records it holds, each carrying the config path it actually sits at. + * + * This helper had grown THREE independent copies in this package, all on the + * same view-walking rules that #6381 had just converged onto one descent + * (`view-walk.ts`): `validate-form-layout.ts`, `validate-translatable-sections.ts` + * and `validate-visibility-predicates.ts`. The duplication was already + * acknowledged in-tree — `validate-form-layout.ts`'s copy said out loud "Same + * helper, same reasoning as `validate-visibility-predicates.ts` and + * `validate-translatable-sections.ts`" — which records the cost without paying + * it. The copy COUNT is the argument for this file, exactly as it was for + * `view-walk.ts` and `page-walk.ts` (#3583): with three, the next author fixes + * one and the two survivors keep the old answer. + * + * ## Why the PATH is the point + * + * The sibling rules that do not report a location coerce with a local `asArray` + * and throw the path away. A rule that emits findings cannot: findings are + * consumed as EDIT TARGETS (`os lint --json`, Studio's finding renderer), so a + * map-shaped collection must not report a synthetic array index nobody can look + * up. `views[2]` is the honest path for the array shape and + * `views.contact_views` for the map, and this helper is the only place that + * decides which. + * + * ## Why the map shape injects `name` + * + * The map key IS the entry's name on that shape, so it is spread in as `name` + * (`{ name, ...def }`, key first so an entry's own `name` still wins). That is + * how an unnamed-but-keyed view still locates itself in a message — a rule that + * read only `rec.name` would otherwise print an anonymous finding for an entry + * the author named perfectly well. + * + * ## Non-records are skipped, not coerced + * + * On both shapes an entry that is not a record is dropped rather than repaired. + * Callers therefore receive records only, which is what lets + * `viewContainerSites` open with a defensive `isRec` guard it documents as + * unreachable from the in-repo callers. + */ + +type AnyRec = Record; + +/** One record of a collection, with the config path it sits at. */ +export interface CollectionEntry { + /** The record itself. On the map shape, with the map key spread in as `name`. */ + rec: AnyRec; + /** Config path — `views[2]` for the array shape, `views.contact_views` for the map. */ + path: string; +} + +function isRec(v: unknown): v is AnyRec { + return !!v && typeof v === 'object' && !Array.isArray(v); +} + +/** + * Every record in a collection authored either as an array or as a name-keyed + * map, each with its config path. `base` is the caller's path prefix for the + * collection itself (e.g. `views`, `objects[0].views`). + * + * Order is the collection's own order — array index order, or `Object.entries` + * insertion order for the map — because findings are emitted in walk order and + * every consumer's pinned output order depends on it. + */ +export function collectionEntries(v: unknown, base: string): CollectionEntry[] { + if (Array.isArray(v)) { + const out: CollectionEntry[] = []; + for (let i = 0; i < v.length; i++) { + if (isRec(v[i])) out.push({ rec: v[i] as AnyRec, path: `${base}[${i}]` }); + } + return out; + } + if (isRec(v)) { + return Object.entries(v) + .filter(([, def]) => isRec(def)) + .map(([name, def]) => ({ rec: { name, ...(def as AnyRec) }, path: `${base}.${name}` })); + } + return []; +} diff --git a/packages/lint/src/validate-form-layout.ts b/packages/lint/src/validate-form-layout.ts index b889cb0f3b..8b1388e55e 100644 --- a/packages/lint/src/validate-form-layout.ts +++ b/packages/lint/src/validate-form-layout.ts @@ -29,7 +29,8 @@ * never guesses at an arbitrary component's object binding. */ -import { formViewSites } from './view-walk.js'; +import { collectionEntries } from './collection-entries.js'; +import { formViewSites, viewObjectName } from './view-walk.js'; export const FORM_FIELD_UNKNOWN = 'form-field-unknown'; export const FORM_COLSPAN_ABSOLUTE = 'absolute-colspan-discouraged'; @@ -70,30 +71,6 @@ function strName(v: unknown): string | undefined { return typeof v === 'string' && v.length > 0 ? v : undefined; } -/** - * Every record in a collection authored either as an array or as a name-keyed - * map, each with its config PATH — `views[2]` for the array shape, - * `views.contact_views` for the map. Findings here are consumed as edit targets - * (`os lint --json`, Studio's finding renderer), so a map-shaped collection must - * not report a synthetic index nobody can look up. Same helper, same reasoning - * as `validate-visibility-predicates.ts` and `validate-translatable-sections.ts`. - */ -function collectionEntries(v: unknown, base: string): Array<{ rec: AnyRec; path: string }> { - if (Array.isArray(v)) { - const out: Array<{ rec: AnyRec; path: string }> = []; - for (let i = 0; i < v.length; i++) { - if (isRec(v[i])) out.push({ rec: v[i] as AnyRec, path: `${base}[${i}]` }); - } - return out; - } - if (isRec(v)) { - return Object.entries(v) - .filter(([, def]) => isRec(def)) - .map(([name, def]) => ({ rec: { name, ...(def as AnyRec) }, path: `${base}.${name}` })); - } - return []; -} - /** * 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 @@ -121,32 +98,6 @@ function fieldNameOf(entry: unknown): string | null { return null; } -/** - * The object a view — or one of its sub-containers — binds to, across the shapes - * it is authored in. - * - * The ladder is `objectName` → `object` → `data.object`, identical to - * `validate-translation-references.ts` and `validate-translatable-sections.ts`'s - * `viewObjectName` (and to the CLI i18n walker's), so all of them agree on which - * object a form belongs to. On the canonical container shape the binding lives - * INSIDE the sub-container (`form.data.object`) while the container itself - * carries `object`, which is why the caller resolves the site first and falls - * back to the container — a record-level lookup alone resolves to nothing on the - * shape real apps ship. - * - * `name` is deliberately NOT a rung. A stack-level container's `name` may be the - * object name (`view.zod.ts` says so for object-scoped containers), but a form - * view's `name` is its own — `contract_form`, not `contract` — and reading it - * here would bind the wrong object and report every field on the form as unknown. - */ -function boundObject(view: AnyRec): string | undefined { - return ( - strName(view.objectName) ?? - strName(view.object) ?? - (isRec(view.data) ? strName(view.data.object) : undefined) - ); -} - /** * Validate authored form-view layout. Returns findings (empty = clean). * Advisory only — the caller must never fail the build on these alone. @@ -169,16 +120,17 @@ export function validateFormLayout(stack: AnyRec): FormLayoutFinding[] { // A container names itself with `name`, or binds with `object` — and an // artifact-emitted one may carry neither, so the path is the last resort. const viewName = strName(view.name) ?? strName(view.object) ?? viewPath; - const containerObject = boundObject(view); + const containerObject = viewObjectName(view); 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. 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 + // view-walking rule in this package uses. The base rung is the shared + // `viewObjectName` (#6662); this FALLBACK is deliberately NOT folded into + // the shared walker, because the consumers compose it differently (see + // `view-walk.ts`) and a refactor that changes a verdict is a failed // refactor. - const objName = boundObject(site.view) ?? containerObject; + const objName = viewObjectName(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}"`; diff --git a/packages/lint/src/validate-translatable-sections.ts b/packages/lint/src/validate-translatable-sections.ts index a57a19403f..c4d1d763d6 100644 --- a/packages/lint/src/validate-translatable-sections.ts +++ b/packages/lint/src/validate-translatable-sections.ts @@ -88,8 +88,9 @@ * the map key IS the name. */ +import { collectionEntries } from './collection-entries.js'; import { walkPageComponents } from './page-walk.js'; -import { viewContainerSites } from './view-walk.js'; +import { viewContainerSites, viewObjectName } from './view-walk.js'; export const TRANSLATION_SECTION_NAME_MISSING = 'translation-section-name-missing'; @@ -120,42 +121,6 @@ function strName(v: unknown): string | undefined { return typeof v === 'string' && v.length > 0 ? v : undefined; } -/** - * The object a view (or one of its containers) binds to, across the shapes it - * is authored in. Same ladder as `validate-translation-references.ts` and the - * CLI walker's `viewObjectName`, so all three agree on which object a heading - * belongs to — a container retargeted at another object keys its headings - * there, and disagreeing here would mean warning about the wrong object. - */ -function viewObjectName(view: AnyRec): string | undefined { - return ( - strName(view.objectName) ?? - strName(view.object) ?? - (isRec(view.data) ? strName(view.data.object) : undefined) - ); -} - -/** - * Entries of a collection authored either as an array or as a name-keyed map, - * each with the config path it actually sits at. The sibling rules coerce with - * `asArray` and lose the path; a rule that reports a location cannot. - */ -function collectionEntries(v: unknown, base: string): Array<{ rec: AnyRec; path: string }> { - if (Array.isArray(v)) { - const out: Array<{ rec: AnyRec; path: string }> = []; - for (let i = 0; i < v.length; i++) { - if (isRec(v[i])) out.push({ rec: v[i] as AnyRec, path: `${base}[${i}]` }); - } - return out; - } - if (isRec(v)) { - return Object.entries(v) - .filter(([, def]) => isRec(def)) - .map(([name, def]) => ({ rec: { name, ...(def as AnyRec) }, path: `${base}.${name}` })); - } - return []; -} - /** One `sections` array, with where it sits and which object it renders under. */ interface SectionSite { /** Path of the `sections` array itself. */ @@ -192,13 +157,14 @@ function joinWhere(...parts: string[]): string { * 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 + * The binding COMPOSITION 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. + * verdicts. Only the base rung each of them starts from is shared + * (`viewObjectName`, `view-walk.ts`, #6662). * * 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 ?? diff --git a/packages/lint/src/validate-translation-references.ts b/packages/lint/src/validate-translation-references.ts index 0fcee13b5d..f76b88747f 100644 --- a/packages/lint/src/validate-translation-references.ts +++ b/packages/lint/src/validate-translation-references.ts @@ -69,6 +69,7 @@ import { expandViewContainer } from '@objectstack/spec'; import { hasPlatformObjectPrefix, isPlatformProvidedObjectName } from '@objectstack/spec/system'; import { walkPageComponents } from './page-walk.js'; import { SYSTEM_FIELDS } from './system-fields.js'; +import { viewObjectName } from './view-walk.js'; export const TRANSLATION_TARGET_UNKNOWN = 'translation-target-unknown'; export const TRANSLATION_OPTION_KEY_UNKNOWN = 'translation-option-key-unknown'; @@ -403,15 +404,6 @@ function namedViewKeys(container: AnyRec): { return { list: keysOf('list', listCount), form: keysOf('form', formCount) }; } -/** The object a view (or one of its containers) binds to, across the shapes it is authored in. */ -function viewObjectName(view: AnyRec): string | undefined { - return ( - strName(view.objectName) ?? - strName(view.object) ?? - (isRec(view.data) ? strName(view.data.object) : undefined) - ); -} - /** * Declared select options for a field, or `undefined` when the field declares * none at all. Handles the canonical `{value,label}[]` shape plus the two diff --git a/packages/lint/src/validate-visibility-predicates.ts b/packages/lint/src/validate-visibility-predicates.ts index bcdc0fbf61..cc1ab3549b 100644 --- a/packages/lint/src/validate-visibility-predicates.ts +++ b/packages/lint/src/validate-visibility-predicates.ts @@ -227,6 +227,7 @@ import { collectCelRootIdentifiers, firstUndeclaredReference, parseCelToAst } from '@objectstack/formula'; import type { CelAstNode } from '@objectstack/formula'; +import { collectionEntries } from './collection-entries.js'; import { walkPageComponents } from './page-walk.js'; import { formViewSites } from './view-walk.js'; @@ -278,32 +279,6 @@ type AnyRec = Record; */ const CANONICAL = 'visibleWhen'; -/** - * Every record in a collection authored either as an array or as a name-keyed - * map, each with its config PATH — `pages[2]` for the array shape, - * `pages.my_page` for the map. Findings on this surface are consumed as edit - * targets (`os lint --json`, Studio's finding renderer), so a map-shaped - * collection must not report a synthetic index nobody can look up. The map - * shape also contributes the entry's KEY as its `name`, which is how an - * unnamed-but-keyed view still locates itself in a message. - */ -function collectionEntries(v: unknown, base: string): Array<{ rec: AnyRec; path: string }> { - if (Array.isArray(v)) { - const out: Array<{ rec: AnyRec; path: string }> = []; - for (let i = 0; i < v.length; i++) { - const rec = v[i]; - if (rec && typeof rec === 'object' && !Array.isArray(rec)) out.push({ rec: rec as AnyRec, path: `${base}[${i}]` }); - } - return out; - } - if (v && typeof v === 'object') { - return Object.entries(v as AnyRec) - .filter(([, def]) => !!def && typeof def === 'object' && !Array.isArray(def)) - .map(([name, def]) => ({ rec: { name, ...(def as AnyRec) }, path: `${base}.${name}` })); - } - return []; -} - /** Extract the CEL source from a predicate value (string, or `{ source }` envelope). */ function predicateSource(v: unknown): string | undefined { if (typeof v === 'string') return v; diff --git a/packages/lint/src/view-walk.test.ts b/packages/lint/src/view-walk.test.ts index 39d8c12f11..817a8b801f 100644 --- a/packages/lint/src/view-walk.test.ts +++ b/packages/lint/src/view-walk.test.ts @@ -2,10 +2,11 @@ import { describe, it, expect } from 'vitest'; -import { viewContainerSites, formViewSites } from './view-walk.js'; +import { viewContainerSites, formViewSites, viewObjectName } from './view-walk.js'; import { validateVisibilityPredicates } from './validate-visibility-predicates.js'; import { validateFormLayout } from './validate-form-layout.js'; import { validateTranslatableSections } from './validate-translatable-sections.js'; +import { validateTranslationReferences } from './validate-translation-references.js'; type AnyRec = Record; @@ -176,3 +177,164 @@ describe('one ladder, three consumers (#6381)', () => { ]); }); }); + +/** + * The base binding ladder (#6662). + * + * `objectName` → `object` → `data.object`, in that order, on ONE record. It had + * three byte-identical copies under two names — `boundObject` + * (`validate-form-layout`) and `viewObjectName` (`validate-translatable-sections`, + * `validate-translation-references`) — which is the same copy count, on the same + * rules, that made the case for `view-walk.ts` itself. + * + * What is pinned here is the base rung ONLY. Each rule's own fallback + * composition is asserted by that rule's own suite, and is deliberately not + * folded in (see the module docblock). + */ +describe('viewObjectName — the base binding ladder (#6662)', () => { + it('reads `objectName` first', () => { + expect(viewObjectName({ objectName: 'a', object: 'b', data: { object: 'c' } })).toBe('a'); + }); + + it('falls to `object` when `objectName` is absent', () => { + expect(viewObjectName({ object: 'b', data: { object: 'c' } })).toBe('b'); + }); + + it('falls to `data.object` last — the canonical container shape', () => { + // On the shape real apps ship, the binding lives INSIDE the sub-container + // (`form.data.object`), so dropping this rung resolves nothing for them. + expect(viewObjectName({ data: { provider: 'object', object: 'c' } })).toBe('c'); + }); + + it('skips a rung authored as an empty string', () => { + // `strName` — an empty name is not a name, and binding to `''` would look + // up an object nobody declared instead of falling through. + expect(viewObjectName({ objectName: '', object: 'b' })).toBe('b'); + expect(viewObjectName({ objectName: '', object: '', data: { object: 'c' } })).toBe('c'); + }); + + it('skips a rung authored with the wrong type', () => { + expect(viewObjectName({ objectName: 42, object: 'b' })).toBe('b'); + expect(viewObjectName({ object: { nested: true }, data: { object: 'c' } })).toBe('c'); + }); + + it('reads `data.object` only when `data` is a record', () => { + expect(viewObjectName({ data: 'crm_case' })).toBeUndefined(); + expect(viewObjectName({ data: [{ object: 'crm_case' }] })).toBeUndefined(); + }); + + it('does NOT read `name` — a form view names itself, not its object', () => { + // `contract_form`, not `contract`. Reading it here would bind the wrong + // object and report every field on the form as unknown. + expect(viewObjectName({ name: 'contract_form' })).toBeUndefined(); + }); + + it('resolves to nothing when the record binds nothing', () => { + expect(viewObjectName({})).toBeUndefined(); + }); +}); + +/** + * The property this convergence buys: ONE base ladder, THREE binding consumers. + * + * The stack below binds its view through the DEEPEST rung only + * (`data.object`) — the rung a hand-written ladder is likeliest to drop, and the + * one the canonical container shape actually uses. All three rules must resolve + * `crm_case` from it, and each is asserted through an observable that only + * exists when the binding resolved: + * + * - `validate-form-layout` reference-checks fields ONLY when the binding + * resolves, so the unknown-field finding is proof it did; + * - `validate-translatable-sections` reports a section only when its bound + * object is translated, and prints that object in `where`; + * - `validate-translation-references` files the form's section names under the + * bound object, so a bundle translating one is accepted rather than reported + * as naming a section nothing declares. + * + * The negative half is asserted too: strip the binding and all three fall + * silent — without it, "the finding fired" would pass just as well for a rule + * that resolved the wrong object, or none. + */ +describe('one base ladder, three binding consumers (#6662)', () => { + const objects = [{ name: 'crm_case', fields: { subject: { type: 'text' } } }]; + + /** A container whose ONLY binding is the deepest rung, on the container itself. */ + const container = (data: unknown) => ({ + name: 'case_views', + ...(data === undefined ? {} : { data }), + formViews: { + edit: { + sections: [ + { name: 'basics', label: 'Basics', fields: ['ghost_field'] }, + ], + }, + }, + }); + + const translations = [ + { + en: { + objects: { + crm_case: { label: 'Case', _sections: { basics: { label: 'Basics' } } }, + }, + }, + }, + ]; + + const bound = { objects, views: [container({ provider: 'object', object: 'crm_case' })], translations }; + const unbound = { objects, views: [container(undefined)], translations }; + + it('validate-form-layout resolves it — and reference-checks against it', () => { + const findings = validateFormLayout(bound); + expect(findings.map((f) => f.path)).toEqual([ + 'views[0].formViews.edit.sections[0].fields[0]', + ]); + expect(findings[0].message).toContain('crm_case'); + + // Negative half: no binding, no reference check, no finding. + expect(validateFormLayout(unbound)).toEqual([]); + }); + + it('validate-translatable-sections resolves it — and names it in `where`', () => { + // The section is named AND translated, so the rule is silent on the bound + // stack; the unbound one cannot resolve an object to check against either. + // What is asserted is the resolution itself, through the labelled-but- + // nameless section the rule does report. + const nameless = { + objects, + views: [ + { + name: 'case_views', + data: { provider: 'object', object: 'crm_case' }, + formViews: { edit: { sections: [{ label: 'Basics' }] } }, + }, + ], + translations, + }; + const findings = validateTranslatableSections(nameless); + expect(findings.map((f) => f.path)).toEqual(['views[0].formViews.edit.sections[0]']); + expect(findings[0].where).toContain('object "crm_case"'); + + // Negative half: strip the binding and the rule has no object to judge + // against, so it reports nothing at all. + const stripped = { + ...nameless, + views: [{ name: 'case_views', formViews: { edit: { sections: [{ label: 'Basics' }] } } }], + }; + expect(validateTranslatableSections(stripped)).toEqual([]); + }); + + it('validate-translation-references resolves it — and files sections under it', () => { + // Accepted: `basics` is declared on a form bound to `crm_case` by the + // deepest rung, so the bundle translating it names something that renders. + expect(validateTranslationReferences(bound)).toEqual([]); + + // Negative half: with the binding gone the section is filed under no + // object, and the same bundle is reported as naming a section nothing + // declares. + const findings = validateTranslationReferences(unbound); + expect(findings.map((f) => f.path)).toEqual([ + 'translations[0].en.objects.crm_case._sections.basics', + ]); + }); +}); diff --git a/packages/lint/src/view-walk.ts b/packages/lint/src/view-walk.ts index 16fb51f0fd..ee3a3f9822 100644 --- a/packages/lint/src/view-walk.ts +++ b/packages/lint/src/view-walk.ts @@ -98,12 +98,15 @@ * record keeps that a consumer's choice instead of freezing one rule's answer * into the shared walk. * - * **Which object a site binds to.** The three consumers compose their binding - * ladders differently on purpose — `validate-form-layout` falls back to the - * container, `validate-translatable-sections` falls back to the container and - * then to the default `list`'s binding, and `validate-visibility-predicates` - * needs no binding at all. Folding those into one walk would change verdicts, - * which a refactor may not do. + * **How a site's binding is COMPOSED.** The BASE ladder — which keys on ONE + * record name an object — is shared, and lives here as {@link viewObjectName} + * (#6662). What each consumer wraps around it is its own and stays its own: + * `validate-form-layout` falls back to the container, + * `validate-translatable-sections` falls back to the container and then to the + * default `list`'s binding, `validate-translation-references` falls back to the + * record, and `validate-visibility-predicates` needs no binding at all. Folding + * those fallbacks into one walk would change verdicts, which a refactor may not + * do; sharing the rung they all start from cannot. */ type AnyRec = Record; @@ -141,6 +144,47 @@ function isRec(v: unknown): v is AnyRec { return !!v && typeof v === 'object' && !Array.isArray(v); } +function strName(v: unknown): string | undefined { + return typeof v === 'string' && v.length > 0 ? v : undefined; +} + +/** + * The object ONE record — a `views[]` entry, or one of its sub-containers — + * binds to, across the shapes it is authored in: `objectName` → `object` → + * `data.object`. + * + * This is the BASE rung only. It had three byte-identical copies under two + * names (#6662): `boundObject` in `validate-form-layout.ts`, `viewObjectName` + * in `validate-translatable-sections.ts` and in + * `validate-translation-references.ts`. The majority spelling wins here, and it + * is also the CLI i18n extractor's (`packages/cli/src/utils/i18n-extract.ts`) + * and the spec resolver's (`packages/spec/src/system/i18n-resolver.ts`), so + * every walker in the repo now agrees on which object a record belongs to by + * reading the same four lines. Disagreeing here would mean warning about the + * wrong object — a container retargeted at another object keys its headings + * there. + * + * What is NOT folded in is the per-rule FALLBACK composition (see the module + * docblock): on the canonical container shape the binding lives INSIDE the + * sub-container (`form.data.object` / `list.data.object`) while the container + * itself carries `object`, so each rule resolves the site first and then falls + * back its own way. A record-level lookup alone resolves to nothing on the + * shape real apps ship. + * + * `name` is deliberately NOT a rung. A stack-level container's `name` may be + * the object name (`view.zod.ts` says so for object-scoped containers), but a + * form view's `name` is its own — `contract_form`, not `contract` — and reading + * it here would bind the wrong object and report every field on the form as + * unknown. + */ +export function viewObjectName(view: AnyRec): string | undefined { + return ( + strName(view.objectName) ?? + strName(view.object) ?? + (isRec(view.data) ? strName(view.data.object) : undefined) + ); +} + /** * EVERY site one `views[]` entry can carry sections on, in ladder order: * the entry itself, its default `form`, its `listViews.*`, its `formViews.*`.