diff --git a/scripts/__tests__/check-lucide-icon-record-names.test.ts b/scripts/__tests__/check-lucide-icon-record-names.test.ts index 6c7f34b9c6..57d5619284 100644 --- a/scripts/__tests__/check-lucide-icon-record-names.test.ts +++ b/scripts/__tests__/check-lucide-icon-record-names.test.ts @@ -93,8 +93,28 @@ interface FixtureOptions { declaredRecordReaders?: string[]; declaredDynamicReaders?: string[]; negativeControl?: string; + recordReadingTypes?: CensusTable; } +/** The census-table shape `analyze` accepts, derived from the gate, not restated. */ +type CensusTable = NonNullable[1]>['recordReadingTypes']>; +type CensusEntry = CensusTable[string]; + +/** + * The authored-node census with every `descendants` declaration stripped — + * the fixture default, for the same reason `anchors` defaults to `[]` here: + * a throwaway tree contains none of this repository's authored nodes, and a + * descent declaration carries a `min` that ERRORS when it reaches nothing + * (objectui#5992). A fixture inheriting the repo's declarations would be + * testing that error instead of what it means to test. Tests that are ABOUT + * descent pass their own table. + */ +const FIXTURE_TYPES: CensusTable = Object.fromEntries( + Object.entries(RECORD_READING_TYPES).map(([type, spec]): [string, CensusEntry] => ( + [type, { paths: spec.paths, resolver: spec.resolver }] + )), +); + function fixtureRepo(label: string, files: Record): string { const root = fs.mkdtempSync(path.join(os.tmpdir(), `icon-record-${label}-`)); fixtures.push(root); @@ -112,6 +132,7 @@ function judge(label: string, options: FixtureOptions) { declaredRecordReaders: options.declaredRecordReaders ?? [RESOLVER_FILE], declaredDynamicReaders: options.declaredDynamicReaders ?? [], negativeControl: options.negativeControl, + recordReadingTypes: options.recordReadingTypes ?? FIXTURE_TYPES, }); } @@ -245,6 +266,131 @@ describe('a name whose resolver this gate cannot identify is declined, not flagg }); }); +// ── 3b. icons on UNTYPED child items of a DECLARED container ───────────────── + +/** + * objectui#5992. Part 2 judged an `icon` only on a node whose OWN `type` was + * censused. `ui:dropdown-menu` menu items are child objects with no `type` key + * at all, so they were never judged — harmless while nothing resolved them, and + * a live hole the moment objectui#5930 routed them through `resolveIcon`. The + * published fixture that carries them is a declared AI few-shot retrieval + * source, so a dead spelling there teaches a dead name. + * + * The rule pinned here is the one that closed it: an untyped node's `icon` is + * judged against its NEAREST TYPED ANCESTOR, and only when that ancestor's + * census entry declares `descendants`. + */ +describe('an icon on an UNTYPED child of a container that declares descent', () => { + /** A fixture container declaring descent, so these tests own their table. */ + const DESCENT_TYPES: CensusTable = { + ...FIXTURE_TYPES, + 'fixture-menu': { + paths: [], + descendants: true, + min: 1, + resolver: RESOLVER_FILE, + }, + }; + + const menu = (items: unknown): string => JSON.stringify({ type: 'fixture-menu', items }, null, 2); + + it('goes RED, naming the child node and the container it answers to', () => { + const result = judge('descend-red', { + files: { 'examples/catalog/menu.json': menu([{ label: 'Edit', icon: 'edit' }]) }, + recordReadingTypes: DESCENT_TYPES, + }); + + expect(result.errors).toEqual([]); + expect(result.violations).toHaveLength(1); + expect(result.violations[0].where).toBe('examples/catalog/menu.json $.items[0].icon'); + expect(result.violations[0].site).toBe('fixture-menu (untyped child item)'); + expect(result.violations[0].detail).toContain('write `square-pen`'); + expect(result.counters.authoredDescendantJudged).toBe(1); + }); + + it('goes GREEN on a live name at the same site — and it was really judged', () => { + const result = judge('descend-green', { + files: { 'examples/catalog/menu.json': menu([{ label: 'Edit', icon: 'square-pen' }]) }, + recordReadingTypes: DESCENT_TYPES, + }); + + expect(result.violations).toEqual([]); + expect(result.errors).toEqual([]); + expect(result.counters.authoredDescendantJudged).toBe(1); + }); + + it('reaches ARBITRARY nesting depth — the case a `items[].icon` path cannot express', () => { + // Why the nearest-typed-ancestor rule was chosen over declaring child-item + // keys per container. `renderMenuItems` RECURSES into `item.children` and + // resolves the submenu trigger through the SAME `resolveIcon` call, so a + // retired name three levels down reaches the record exactly as the leaf + // does. The `paths` grammar is `^(\w+)\[\]\.icon$` — one level, by + // construction — so a key list would have closed the leaf and left the + // submenu open, which is the narrower version of the same bug objectui#5930 + // explicitly refused to ship. + const result = judge('descend-deep', { + files: { + 'examples/catalog/menu.json': menu([ + { label: 'More', children: [{ label: 'Deeper', children: [{ label: 'Deepest', icon: 'more-horizontal' }] }] }, + ]), + }, + recordReadingTypes: DESCENT_TYPES, + }); + + expect(result.violations).toHaveLength(1); + expect(result.violations[0].where).toBe('examples/catalog/menu.json $.items[0].children[0].children[0].icon'); + expect(result.violations[0].detail).toContain('write `ellipsis`'); + }); + + it('STOPS at a typed child — the child\'s own `type` still answers for it', () => { + // A `type: 'separator'` menu item is returned early by the renderer and + // draws no icon. Descent that ran through it would invent a violation no + // resolver would ever produce, and this gate would get suppressed. + const result = judge('descend-typed-child', { + files: { + 'examples/catalog/menu.json': menu([ + { type: 'separator', icon: 'edit' }, + { type: 'text', icon: 'filter' }, + ]), + }, + recordReadingTypes: DESCENT_TYPES, + }); + + expect(result.violations).toEqual([]); + expect(result.counters.authoredDescendantJudged).toBe(0); + // …and it SAW them: silence is a decision, not a miss. + expect(result.counters.authoredDeclined).toBe(2); + }); + + it('does NOT leak descent into a container that never declared it', () => { + // The reason `descendants` is opt-in per container rather than a blanket + // "nearest censused ancestor" rule: `breadcrumb`/`button-group`/`command` + // items were measured and none of their renderers reads `icon` at all. + const result = judge('descend-undeclared', { + files: { 'examples/catalog/crumbs.json': JSON.stringify({ type: 'breadcrumb', items: [{ icon: 'layout' }] }, null, 2) }, + recordReadingTypes: DESCENT_TYPES, + }); + + expect(result.violations).toEqual([]); + expect(result.counters.authoredDescendantJudged).toBe(0); + expect(result.counters.authoredDeclined).toBe(1); + }); + + it('ERRORS rather than passing when a descent declaration reaches NOTHING', () => { + // The vacuity this whole card is about, one level up: a declaration that + // stops reaching its nodes produces zero violations and reads exactly like + // a clean tree. Same precondition ANCHORED_MAPS states with `min`. + const result = judge('descend-vacuous', { + files: { 'examples/catalog/other.json': JSON.stringify({ type: 'text', label: 'hi' }, null, 2) }, + recordReadingTypes: DESCENT_TYPES, + }); + + expect(result.violations).toEqual([]); + expect(result.errors.join('\n')).toContain('`fixture-menu` declares its icon names on UNTYPED child items'); + expect(result.errors.join('\n')).toContain('reached 0 of them — fewer than the 1'); + }); +}); + // ── 4. the census is measured, not remembered ──────────────────────────────── describe('the surface census is re-derived on every run', () => { @@ -373,6 +519,28 @@ describe('this repository', () => { expect(repoResult.counters.anchoredJudged).toBeGreaterThan(30); }); + it('really judges the untyped child items objectui#5992 opened up', () => { + // Without this, the descent rule could stop reaching the catalog and the + // repository would stay green — which is precisely the shape of the defect + // that card was filed for. The gate carries a `min` for the same reason; + // this is the reading of it from outside. + expect(repoResult.counters.authoredDescendantJudged).toBeGreaterThan(0); + const declaring = Object.entries(RECORD_READING_TYPES) + .filter(([, spec]) => 'descendants' in spec && spec.descendants) + .map(([type]) => type); + expect(declaring).toContain('dropdown-menu'); + }); + + it('did NOT grow part 1 in the process — descent is a part-2 rule', () => { + // objectui#5930 routes menu icons through `resolveIcon`, not through a + // fresh `icons` import, so `renderers/overlay/dropdown-menu.tsx` correctly + // stays OUT of the record-reader census. A descent declaration that moved + // this number would have altered the wrong part of the gate. + expect(repoResult.discovered.record).toHaveLength(8); + expect(repoResult.discovered.record).not.toContain('packages/components/src/renderers/overlay/dropdown-menu.tsx'); + expect(RECORD_READING_TYPES['dropdown-menu'].resolver).toContain('renderers/action/resolve-icon.ts'); + }); + it('carries more record-reading resolvers than objectui#5633 catalogued by hand', () => { // The card's table listed four. Discovery found eight, which is the whole // argument for measuring the population instead of maintaining a list: the @@ -430,6 +598,21 @@ describe('the gate is wired and the local pins it subsumes are gone', () => { expect(anchored).toContain('packages/plugin-view/src/ObjectView.tsx::iconMap'); }); + it('no longer carries the parenthetical objectui#5930 falsified', () => { + // The header used to state, as a measurement, that the untyped catalog + // icons were "eight ... child items of `button-group`, `breadcrumb`, + // `command` and `dropdown-menu` — three of which never read `icon`, and the + // fourth renders it as raw text". objectui#5930 made the fourth resolve + // through the record, and the count was low by 53. A stale measurement in + // the header of a gate is what the next reader reasons from. + const header = fs.readFileSync(path.join(repoRoot, GATE), 'utf8'); + expect(header).not.toContain('the fourth renders it as raw text'); + expect(header).toContain('Re-measured over the schema catalog at objectui@ef2a3bd8d'); + for (const container of ['button-group', 'breadcrumb', 'command', 'context-menu', 'timeline', 'tree-view']) { + expect(header, `the re-measured table dropped ${container}`).toContain(container); + } + }); + it('the pin the gate does NOT subsume is kept, and says why', () => { // `ui:icon`'s registration meta: no first-party consumer of a // registration's `icon` exists in this repository, so the gate has no diff --git a/scripts/check-lucide-icon-record-names.mjs b/scripts/check-lucide-icon-record-names.mjs index fc45da4551..43b7afcedf 100644 --- a/scripts/check-lucide-icon-record-names.mjs +++ b/scripts/check-lucide-icon-record-names.mjs @@ -54,11 +54,48 @@ * schema object literals embedded in first-party TS) and checks the `icon` * names on nodes whose `type` is a censused record-reading renderer. The * node's own `type` is what answers "which resolver does this string - * reach?", which is why this check can be broad without being suppressible: - * an `icon` on an UNTYPED node is not judged at all (measured: the eight - * such names in the schema catalog are child items of `button-group`, - * `breadcrumb`, `command` and `dropdown-menu` — three of which never read - * `icon`, and the fourth renders it as raw text). + * reach?", which is why this check can be broad without being suppressible. + * + * An `icon` on an UNTYPED node is judged only when its NEAREST TYPED + * ANCESTOR is a censused container whose entry DECLARES that its child items + * carry icon names (`descendants: true`) — a fact read off that container's + * renderer, exactly the way every `paths` entry in the table below was. A + * typed node ENDS any descent it sits inside, because its own `type` is + * still the answer to which resolver its string reaches. Untyped icons under + * every other ancestor remain declined, not flagged. + * + * ── Re-measured over the schema catalog at objectui@ef2a3bd8d ──────────── + * The parenthetical this replaced read "the eight such names in the schema + * catalog are child items of `button-group`, `breadcrumb`, `command` and + * `dropdown-menu` — three of which never read `icon`, and the fourth renders + * it as raw text". objectui#5930 falsified its second half, and the count + * was low by 53. Re-measured, by reading each renderer (objectui#5992): + * + * 61 untyped `icon` names, across SEVEN containers. Exactly ONE of them + * reaches a record-reading resolver: + * + * dropdown-menu 3 RECORD — `resolveIcon(item.icon)` in + * `renderers/overlay/dropdown-menu.tsx` (objectui#5930). + * JUDGED HERE. Judged RECURSIVELY, because that + * renderer recurses into `item.children` for submenus + * and resolves the submenu trigger's icon through the + * same call — a depth no single-level `items[].icon` + * path can express. + * button-group 8 `renderers/basic/button-group.tsx` never reads + * `button.icon` at all; the names render nothing + * breadcrumb 3 renderer never reads `icon` + * command 9 renderer never reads `icon` + * context-menu 4 renderer never reads `icon` — dropdown-menu's twin, + * and NOT routed by objectui#5930 + * timeline 4 rendered as RAW TEXT, `{item.icon}`; + * the four authored names are emoji, not lucide names + * tree-view 30 read, but as a TWO-VALUED literal switch + * (`node.icon === 'folder'`) — never a record lookup + * + * `dropdown-menu.tsx` itself is correctly ABSENT from part 1's census: it + * imports `resolveIcon`, not `icons`, so the record read happens in + * `renderers/action/resolve-icon.ts`, which is already declared. Part 1 + * stays at eight resolvers; this is a part-2 rule, not a census change. * * 3. ANCHORED MAPS — the first-party const maps that feed a record-reading * resolver but are not authored nodes. This is the population the retired @@ -132,6 +169,16 @@ export const SCAN_ROOTS = ['packages', 'apps', 'examples']; // from this table is not judged, because nothing here knows which vocabulary // (if any) its icons reach — and a gate that guessed would be a gate that gets // suppressed. +// +// `descendants: true` extends that same declaration to the container's UNTYPED +// child items — see the part-2 paragraph in the header. It is deliberately +// OPT-IN per container rather than a blanket "nearest typed ancestor" rule: +// applying descent to every censused entry would extend a judgement that was +// measured against one shape (`action:bar`'s `actions[]`) to descendant shapes +// nobody read off a renderer, which is the guessing this table exists to +// refuse. `min` is the non-vacuity precondition ANCHORED_MAPS uses below: a +// descent that reaches NOTHING reports no violations and reads exactly like a +// clean tree. export const RECORD_READING_TYPES = { 'button': { paths: ['icon'], resolver: 'packages/components/src/renderers/form/button.tsx' }, 'action:bar': { paths: ['actions[].icon'], resolver: 'packages/components/src/renderers/action/resolve-icon.ts' }, @@ -140,6 +187,16 @@ export const RECORD_READING_TYPES = { 'action:icon': { paths: ['icon'], resolver: 'packages/components/src/renderers/action/resolve-icon.ts' }, 'action:menu': { paths: ['icon', 'actions[].icon'], resolver: 'packages/components/src/renderers/action/resolve-icon.ts' }, 'data-table': { paths: ['rowActionDefs[].icon'], resolver: 'packages/components/src/renderers/complex/data-table.tsx' }, + // The container's OWN `icon` is never read (`paths: []`); its item icons sit + // on untyped children, recursively, and every one of them goes through the + // single `resolveIcon(item.icon)` call that serves BOTH the leaf arm and the + // submenu-trigger arm (objectui#5930, objectui#5992). + 'dropdown-menu': { + paths: [], + descendants: true, + min: 1, + resolver: 'packages/components/src/renderers/action/resolve-icon.ts (via renderers/overlay/dropdown-menu.tsx)', + }, 'view-switcher': { paths: ['views[].icon', 'viewActions[].icon'], resolver: 'packages/plugin-view/src/ViewSwitcher.tsx' }, }; @@ -399,13 +456,17 @@ export function discoverResolvers(root, files) { // ── Part 2: authored nodes ─────────────────────────────────────────────────── const ARRAY_PATH = /^(\w+)\[\]\.icon$/; -function judgeAuthoredNodes(root, { sources, documents }) { +function judgeAuthoredNodes(root, { sources, documents }, types = RECORD_READING_TYPES) { const violations = []; + const errors = []; let judged = 0; let declined = 0; + let descendantJudged = 0; + /** Per declaring container `type`, how many untyped child icons descent reached. */ + const descentReach = new Map(); const judge = (typeName, gather, locate) => { - const spec = RECORD_READING_TYPES[typeName]; + const spec = types[typeName]; if (!spec) return; for (const path of spec.paths) { for (const found of gather(path)) { @@ -417,16 +478,43 @@ function judgeAuthoredNodes(root, { sources, documents }) { } }; + /** + * The descent site a node's children inherit. A node's OWN `type` is still + * the answer to "which resolver does this string reach?", so a typed node + * ENDS whatever descent it sits inside and opens a new one only if its own + * census entry declares one. That is what keeps a `type: 'separator'` menu + * item — which this renderer returns early for, drawing no icon — declined + * rather than judged. + */ + const descentBelow = (typeName, inherited) => { + if (!typeName) return inherited; + const spec = types[typeName]; + return spec?.descendants ? { type: typeName, spec } : null; + }; + + /** Judge one untyped child icon against the container that declared descent. */ + const judgeDescendant = (site, value, where) => { + judged += 1; + descendantJudged += 1; + descentReach.set(site.type, (descentReach.get(site.type) ?? 0) + 1); + if (!isLiveKey(value)) { + violations.push({ where, site: `${site.type} (untyped child item)`, resolver: site.spec.resolver, detail: describeName(value) }); + } + }; + for (const file of documents) { if (isTestPath(file)) continue; let document; try { document = JSON.parse(readFileSync(join(root, file), 'utf8')); } catch { continue; } - // One walker, carrying a JSON-pointer trail so a violation names the node. - const walk = (node, trail) => { - if (Array.isArray(node)) { node.forEach((child, index) => walk(child, `${trail}[${index}]`)); return; } + // One walker, carrying a JSON-pointer trail so a violation names the node, + // and the descent site (if any) its untyped children answer to. + const walk = (node, trail, descent) => { + if (Array.isArray(node)) { node.forEach((child, index) => walk(child, `${trail}[${index}]`, descent)); return; } if (!node || typeof node !== 'object') return; const typeName = typeof node.type === 'string' ? node.type : null; - if (typeof node.icon === 'string' && !(typeName && RECORD_READING_TYPES[typeName])) declined += 1; + const inherits = !typeName && descent && typeof node.icon === 'string'; + if (inherits) judgeDescendant(descent, node.icon, `${file} ${trail}.icon`); + if (typeof node.icon === 'string' && !inherits && !(typeName && types[typeName])) declined += 1; if (typeName) { judge(typeName, (path) => { const found = []; @@ -441,9 +529,10 @@ function judgeAuthoredNodes(root, { sources, documents }) { return found; }, (where) => `${file} ${where}`); } - for (const [key, value] of Object.entries(node)) walk(value, `${trail}.${key}`); + const below = descentBelow(typeName, descent); + for (const [key, value] of Object.entries(node)) walk(value, `${trail}.${key}`, below); }; - walk(document, '$'); + walk(document, '$', null); } for (const file of sources) { @@ -451,12 +540,17 @@ function judgeAuthoredNodes(root, { sources, documents }) { const text = readFileSync(join(root, file), 'utf8'); if (!text.includes('icon')) continue; const sf = parseSource(root, file); - const visit = (node) => { + const visit = (node, descent) => { + let below = descent; if (ts.isObjectLiteralExpression(node)) { const typeInit = objectProp(node, 'type'); const iconInit = objectProp(node, 'icon'); const typeName = typeInit && ts.isStringLiteral(typeInit) ? typeInit.text : null; - if (iconInit && ts.isStringLiteral(iconInit) && !(typeName && RECORD_READING_TYPES[typeName])) declined += 1; + below = descentBelow(typeName, descent); + const iconLiteral = iconInit && ts.isStringLiteral(iconInit) ? iconInit : null; + const inherits = !typeName && descent && iconLiteral; + if (inherits) judgeDescendant(descent, iconLiteral.text, `${file}:${lineOf(sf, iconLiteral)}`); + if (iconLiteral && !inherits && !(typeName && types[typeName])) declined += 1; if (typeName) { judge(typeName, (path) => { const found = []; @@ -477,12 +571,30 @@ function judgeAuthoredNodes(root, { sources, documents }) { }, (where) => `${file}:${lineOf(sf, where)}`); } } - ts.forEachChild(node, visit); + ts.forEachChild(node, (child) => visit(child, below)); }; - ts.forEachChild(sf, visit); + ts.forEachChild(sf, (child) => visit(child, null)); } - return { violations, judged, declined }; + // Non-vacuity, the precondition ANCHORED_MAPS states in full below: a descent + // declaration that reached NOTHING produces zero violations and reads exactly + // like a clean tree. That is the failure this whole card is about one level + // up, so it is an ERROR here rather than a shrug. + for (const [typeName, spec] of Object.entries(types)) { + if (!spec.descendants) continue; + const reached = descentReach.get(typeName) ?? 0; + const min = spec.min ?? 1; + if (reached < min) { + errors.push( + `\`${typeName}\` declares its icon names on UNTYPED child items (\`descendants: true\`), but the authored-node walk ` + + `reached ${reached} of them — fewer than the ${min} it is declared to carry. Either the authored nodes moved ` + + 'or the descent no longer reaches them; a descent that reaches nothing reports no violations and reads as green. ' + + `Resolved through: ${spec.resolver}`, + ); + } + } + + return { violations, errors, judged, declined, descendantJudged }; } // ── Part 3: anchored first-party maps ──────────────────────────────────────── @@ -547,11 +659,32 @@ function judgeAnchoredMaps(root, anchors) { } // ── The whole judgement ────────────────────────────────────────────────────── +/** + * One authored-node census entry. Spelled out as a type rather than left to be + * inferred from `RECORD_READING_TYPES`, because every override this function + * takes exists to be a DIFFERENT table from the declared one — inferring the + * parameter from the default makes the declared table the only one that fits, + * which is precisely backwards for a seam whose job is substitution. + * + * @typedef {{ paths: string[], resolver: string, descendants?: boolean, min?: number }} RecordReadingType + * + * @typedef {{ + * anchors?: readonly any[], + * declaredRecordReaders?: readonly string[], + * declaredDynamicReaders?: readonly string[], + * negativeControl?: string, + * recordReadingTypes?: Record, + * }} AnalyzeOptions + * + * @param {string} root + * @param {AnalyzeOptions} [options] + */ export function analyze(root, { anchors = ANCHORED_MAPS, declaredRecordReaders = DECLARED_RECORD_READERS, declaredDynamicReaders = DECLARED_DYNAMIC_READERS, negativeControl = DISCOVERY_NEGATIVE_CONTROL, + recordReadingTypes = RECORD_READING_TYPES, } = {}) { const errors = [...selfTest()]; const { sources, documents } = collectFiles(root); @@ -581,9 +714,9 @@ export function analyze(root, { errors.push(`discovery classified ${negativeControl} as a lucide resolver. It builds its OWN local \`icons\` object — discovery is matching the NAME rather than the IMPORT.`); } - const authored = judgeAuthoredNodes(root, { sources, documents }); + const authored = judgeAuthoredNodes(root, { sources, documents }, recordReadingTypes); const anchored = judgeAnchoredMaps(root, anchors); - errors.push(...anchored.errors); + errors.push(...authored.errors, ...anchored.errors); return { discovered, @@ -594,6 +727,7 @@ export function analyze(root, { documents: documents.length, authoredJudged: authored.judged, authoredDeclined: authored.declined, + authoredDescendantJudged: authored.descendantJudged, anchoredJudged: anchored.judged, }, }; @@ -613,7 +747,7 @@ if (invokedDirectly) { for (const file of discovered.record) console.log(` ${file}`); console.log(`dynamic-surface resolvers discovered (${discovered.dynamic.length}), NOT judged here:`); for (const file of discovered.dynamic) console.log(` ${file}`); - console.log(`authored icon names judged: ${counters.authoredJudged} | icon names on nodes this gate declines to judge: ${counters.authoredDeclined}`); + console.log(`authored icon names judged: ${counters.authoredJudged} (${counters.authoredDescendantJudged} of them on UNTYPED child items of a declared container) | icon names on nodes this gate declines to judge: ${counters.authoredDeclined}`); console.log(`anchored map entries judged: ${counters.anchoredJudged}`); console.log(''); }