diff --git a/.changeset/forwardref-prop-erasure-4528.md b/.changeset/forwardref-prop-erasure-4528.md new file mode 100644 index 000000000..46bc5cbe6 --- /dev/null +++ b/.changeset/forwardref-prop-erasure-4528.md @@ -0,0 +1,17 @@ +--- +'@object-ui/plugin-dashboard': minor +'@object-ui/plugin-list': minor +'@object-ui/app-shell': patch +--- + +`DashboardRenderer` and `ListView` serve the props they declare — the index signature stops erasing them + +Both components declared a full props interface and neither was enforced. A `[key: string]: any` on `DashboardRendererProps` and `ListViewProps` puts `string` into `keyof Props`, so `'ref' extends keyof Props` is always true and React's `PropsWithoutRef` takes its `Omit` branch — and `Omit` over a type carrying a string index signature keeps only the index signature. Every declared property was dropped from the resolved type, on both sides: the render function received `{ [x: string]: any }` (so even `schema` was `any` inside the component), and every JSX call site was unchecked. Measured on the pre-fix source, `keyof ComponentProps` was `string | number` and `ComponentProps['onWidgetClick']` was `any`, while the interface went on declaring `(widgetId: string | null) => void`. `ListView` measured identically for `onRowClick`. This is objectui#4422 / PR #4438's trap in the two packages that issue left unswept. + +Graded **minor, not major**: the interfaces have always DECLARED these props; the index signature erased them from the resolved type. Restoring what the interface documents is a FIX to the published contract, not a contract break — no documented capability is removed, and `any`-typed accidental passthrough was never the documented surface. Nothing in either package's README or docs endorses relying on it. + +The props each component genuinely reads but never declared are now declared by name, at the type each one lands on: `dataSource` on both, plus `onAddRecord` / `onBulkAction` / `onPageSizeChange` / `onEdit` / `onDelete` / `onBulkDelete` on `ListView`. `DashboardRenderer`'s DOM pass-through keys are derived from `toDomProps`' whitelist constant itself, so the declaration and the runtime filter cannot drift — the "declare it and forward it by name" direction `@object-ui/core`'s `dom-props` doctrine asks for, rather than reopening the spread. + +Type-only: the emitted JS for both packages is byte-identical before and after (verified by sha256 on `dist/index.js` and `dist/index.umd.cjs`), and both packages' runtime suites are untouched and green. + +Three latent defects the erasure had been hiding are fixed with it, each surfaced by the repo-wide type-check: `DashboardWithConfig` typed its widget-select handler `(widgetId: string)` while `DashboardRenderer` calls `onWidgetClick(null)` to deselect; `InterfaceListPage` built a list schema whose `viewType` was a bare `string`; and `StudioDesignSurface` forwarded a `refreshKey` prop that no component in the chain declares or reads, so it was silently dropped. Per-package structural guards now pin the shape in both packages, covering the public `forwardRef` that takes its props whole — the spelling objectui#4438's `schema`-destructuring scan could not see. diff --git a/packages/app-shell/src/views/InterfaceListPage.tsx b/packages/app-shell/src/views/InterfaceListPage.tsx index a4e819aa4..ae78e1cb0 100644 --- a/packages/app-shell/src/views/InterfaceListPage.tsx +++ b/packages/app-shell/src/views/InterfaceListPage.tsx @@ -23,6 +23,7 @@ import { Empty, EmptyTitle, EmptyDescription, NavigationOverlay } from '@object- import { Database } from 'lucide-react'; import { useObjectTranslation } from '@object-ui/i18n'; import { isSystemManagedField } from '@object-ui/types'; +import type { ListViewSchema } from '@object-ui/types'; import { useMetadata } from '../providers/MetadataProvider'; import { useTenancyPosture } from '../hooks/useTenancyPosture'; import { parseUserFilterParams, applyUserFilterParams } from './userFilterUrlState'; @@ -372,7 +373,13 @@ export function InterfaceListPage({ page, className, onConfigChange, reserveEdit return { type: 'list-view' as const, objectName: objectDef.name, - viewType: (allowed[0] ?? view.type ?? 'grid'), + // Narrowed to the schema's declared union rather than left as `string`: + // `allowedVisualizations` arrives as `string[]`, so this expression is a + // bare `string` and only type-checked against `ListViewSchema` from + // objectui#4528 onwards — before that, `ListViewProps` carried a + // `[key: string]: any` that erased `schema` to `any` at this call site. + // The assertion changes no value; the runtime string is what it was. + viewType: (allowed[0] ?? view.type ?? 'grid') as ListViewSchema['viewType'], columns, ...(filters.length ? { filter: filters } : {}), ...(sort?.length ? { sort } : {}), diff --git a/packages/app-shell/src/views/studio-design/StudioDesignSurface.tsx b/packages/app-shell/src/views/studio-design/StudioDesignSurface.tsx index 5dd94610f..10bfb1e50 100644 --- a/packages/app-shell/src/views/studio-design/StudioDesignSurface.tsx +++ b/packages/app-shell/src/views/studio-design/StudioDesignSurface.tsx @@ -1885,10 +1885,24 @@ function renderStudioGridList(props: { dataSource: unknown; onEdit?: (record: Record) => void; className?: string; + /** + * Supplied by the plugin ObjectView's `renderListView` slot, and deliberately + * NOT read here. It used to be forwarded as `refreshKey={refreshKey}` to + * `ListView`, which reaches nothing: neither `ListView` nor any view + * component it renders declares or reads a `refreshKey` prop, so it rode the + * `{...props}` forward and was dropped. That was invisible until + * objectui#4528 stopped `ListViewProps` erasing itself. + * + * Removing the dead prop is behaviour-preserving; WIRING it is not, so it is + * deliberately left for triage rather than fixed under a type-only card. The + * working precedent is `app-shell/src/views/ObjectView.tsx`, which folds the + * slot's `refreshKey` into React's `key` to force a remount. Filed as a + * separate finding. + */ refreshKey?: number; onAddRecord?: () => void; }): React.ReactElement { - const { schema: listSchema, dataSource: ds, onEdit, className, refreshKey, onAddRecord } = props; + const { schema: listSchema, dataSource: ds, onEdit, className, onAddRecord } = props; return ( ); } diff --git a/packages/plugin-dashboard/src/DashboardRenderer.tsx b/packages/plugin-dashboard/src/DashboardRenderer.tsx index 256e71dd0..4d1738246 100644 --- a/packages/plugin-dashboard/src/DashboardRenderer.tsx +++ b/packages/plugin-dashboard/src/DashboardRenderer.tsx @@ -9,7 +9,7 @@ import type { DashboardComponentSchema, DashboardWidgetSchema } from '@object-ui/types'; import { SchemaRenderer, useActionEngine, useObjectLabel, PageVariablesProvider, usePageVariables } from '@object-ui/react'; import { useObjectTranslation, pickLocalized } from '@object-ui/i18n'; -import type { ActionDef, ActionResult, ActionContext, ModalHandler } from '@object-ui/core'; +import type { ActionDef, ActionResult, ActionContext, ModalHandler, SduiDomPassThroughKey } from '@object-ui/core'; import { resolveDashboardFilterDefs, dashboardFilterVariableDefs, @@ -19,6 +19,7 @@ import { } from '@object-ui/core'; import { cn, Card, CardHeader, CardTitle, CardContent, Button, getLazyIcon } from '@object-ui/components'; import { forwardRef, useState, useEffect, useCallback, useMemo, useRef, Fragment } from 'react'; +import type { HTMLAttributes } from 'react'; import { RefreshCw } from 'lucide-react'; import { DndContext, @@ -153,9 +154,52 @@ const LEGACY_RETIRED_WIDGET_SCHEMA = { className: 'flex h-full w-full items-center justify-center rounded border border-dashed border-destructive/40 bg-destructive/5 p-4 text-center text-destructive', } as const; -export interface DashboardRendererProps { +/** + * The dashboard renderer's props. + * + * ## Why there is no `[key: string]: any` here (objectui#4528) + * + * There used to be one, and it erased this entire interface. A string index + * signature puts `string` into `keyof Props`, so `'ref' extends keyof Props` is + * always true and React's `PropsWithoutRef` takes its `Omit` branch — and + * `Omit` over a type carrying a string index signature keeps ONLY the index + * signature. Every declared property above was dropped from the resolved type: + * `keyof React.ComponentProps< typeof DashboardRenderer >` measured as + * `string | number`, and `onWidgetClick` measured as `any` at every JSX call + * site, while this interface went on declaring + * `(widgetId: string | null) => void`. The declaration was right and no + * consumer was held to it — a prop typo or a wrong-arity handler type-checked. + * + * The same trap, in `packages/components`, is objectui#4422 / PR #4438; this is + * the sweep of the two packages that issue left unswept. + * + * ## How the DOM pass-through survives without it + * + * `SchemaRenderer` hands this component the authored node's own keys, so the + * render function still collects an open rest object — but what may reach the + * element is decided by `toDomProps`' WHITELIST (objectui#4432), not by this + * type. The keys that whitelist forwards are therefore declared here BY NAME, + * derived from the whitelist constant itself so the two cannot drift, which is + * exactly what `@object-ui/core`'s `dom-props` doctrine asks for: "Deliberate + * DOM pass-through beyond this set stays available the objectui#4435 way — + * DECLARE it and forward it by name. Do not reopen the spread." + */ +export interface DashboardRendererProps + extends Pick, SduiDomPassThroughKey> { schema: DashboardComponentSchema; className?: string; + /** + * Data-source adapter for the widgets this dashboard renders. + * + * Destructured by the render function and handed to `DatasetWidget` / + * `SchemaRenderer`, and passed by both in-repo hosts (`DashboardView`, + * `DashboardPreview`) — but never declared until objectui#4528, because the + * index signature above was answering for it. Typed `any` deliberately: that + * is precisely what it resolved to before, so declaring it changes what is + * DECLARED without changing what any call site is held to. Narrowing it to a + * real adapter type is a separate change with its own consumer sweep. + */ + dataSource?: any; /** Callback invoked when dashboard refresh is triggered (manual or auto) */ onRefresh?: () => void; /** Total record count to display */ @@ -186,11 +230,10 @@ export interface DashboardRendererProps { * title/subtitle so we don't display them twice. */ hideHeaderText?: boolean; - [key: string]: any; } const DashboardRendererInner = forwardRef( - ({ schema, className, dataSource, onRefresh, recordCount, userActions, designMode, selectedWidgetId, onWidgetClick, onWidgetsReorder, modalHandler, scriptHandlers, hideHeaderText, ...props }, ref) => { + ({ schema, className, dataSource, onRefresh, recordCount, userActions, designMode, selectedWidgetId, onWidgetClick, onWidgetsReorder, modalHandler, scriptHandlers, hideHeaderText, ...props }: DashboardRendererProps & { [key: string]: any }, ref) => { // Auto-infer the grid column count when the dashboard schema doesn't // specify one. Spec convention is a 12-column grid (widgets use w: 3 for // quarter-row KPIs, w: 6 for half-row charts, etc.). If we always default @@ -426,7 +469,7 @@ const DashboardRendererInner = forwardRef { + const handleHostClick = (e: React.MouseEvent) => { handleBackgroundClick(e); if (typeof props.onClick === 'function') props.onClick(e); }; diff --git a/packages/plugin-dashboard/src/DashboardWithConfig.tsx b/packages/plugin-dashboard/src/DashboardWithConfig.tsx index 4c8e92001..fa264a3bb 100644 --- a/packages/plugin-dashboard/src/DashboardWithConfig.tsx +++ b/packages/plugin-dashboard/src/DashboardWithConfig.tsx @@ -123,8 +123,17 @@ export function DashboardWithConfig({ // eslint-disable-next-line react-hooks/exhaustive-deps }, [selectedWidgetId, configVersion]); + // `string | null`, not `string`: `DashboardRenderer` calls `onWidgetClick(null)` + // to DESELECT when a design-mode click lands on the dashboard background, and + // this handler has always received that `null` at runtime — `selectedWidgetId` + // is a `useState< string | null >` precisely so it can hold it. The narrower + // `(widgetId: string)` type-checked only because a `[key: string]: any` on + // `DashboardRendererProps` erased every declared prop from the resolved type, + // so this call site was never held to the declared + // `(widgetId: string | null) => void` (objectui#4528). Widening the annotation + // is what the contract always said; nothing about the behaviour changes. const handleWidgetSelect = useCallback( - (widgetId: string) => { + (widgetId: string | null) => { setSelectedWidgetId(widgetId); setConfigOpen(true); }, diff --git a/packages/plugin-dashboard/src/__tests__/DashboardRenderer.domProps.test.tsx b/packages/plugin-dashboard/src/__tests__/DashboardRenderer.domProps.test.tsx index fb3a2b917..7419597c9 100644 --- a/packages/plugin-dashboard/src/__tests__/DashboardRenderer.domProps.test.tsx +++ b/packages/plugin-dashboard/src/__tests__/DashboardRenderer.domProps.test.tsx @@ -237,14 +237,22 @@ describe("the grid's click channel has one carrier (objectui#4432)", () => { ` and every NAMED prop — - // `onWidgetClick` included — collapses to the index signature at the - // JSX call site. The annotation restates the type the interface itself - // still declares, `(widgetId: string | null) => void`, so this callback - // is checked even though the element around it is not (objectui#4040). + // `id` is annotated rather than inferred. This used to be load-bearing: + // `DashboardRendererProps` carried a `[key: string]: any`, which made + // `'ref' extends keyof Props` true, so React's `PropsWithoutRef` + // resolved to `Pick` and every NAMED prop — + // `onWidgetClick` included — collapsed to the index signature at the + // JSX call site. The annotation restated the type the interface itself + // still declared, so this callback was checked even though the element + // around it was not (objectui#4040). + // + // objectui#4528 removed that index signature, so the annotation is now + // REDUNDANT rather than load-bearing — `onWidgetClick` resolves to the + // declared `(widgetId: string | null) => void` on its own, and + // `DashboardRenderer.propsResolution.test.ts` pins exactly that. It is + // kept because restating a declared type costs nothing and this call + // site reads more clearly with the contract spelled out; deleting it + // would also be correct. onWidgetClick={(id: string | null) => selections.push(id)} onClick={() => hostClicks.push('host')} />, diff --git a/packages/plugin-dashboard/src/__tests__/DashboardRenderer.propsResolution.test.ts b/packages/plugin-dashboard/src/__tests__/DashboardRenderer.propsResolution.test.ts new file mode 100644 index 000000000..4e03266e0 --- /dev/null +++ b/packages/plugin-dashboard/src/__tests__/DashboardRenderer.propsResolution.test.ts @@ -0,0 +1,91 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +/** + * objectui#4528 — COMPILE-TIME pins on the props a `DashboardRenderer` JSX call + * site is actually held to. + * + * These assertions are erased at runtime; `tsc` is the only thing that can + * check them, which is why this file is carried by + * `packages/plugin-dashboard/tsconfig.test.json` and why the `expect`s below + * are deliberately trivial — the real assertions are the `Assert< Equal< … > >` + * types, and a violation is a compile error, not a red test. + * + * ## What was measured before the fix + * + * `DashboardRendererProps` carried a `[key: string]: any`, which puts `string` + * into `keyof Props`, so React's `PropsWithoutRef` took its `Omit` branch and + * `Omit` over a string index signature keeps ONLY the index signature. On the + * pre-fix source, compiled through this same project: + * + * keyof React.ComponentProps< typeof DashboardRenderer > -> string | number + * React.ComponentProps< typeof DashboardRenderer >['onWidgetClick'] -> any + * DashboardRendererProps['onWidgetClick'] -> ((widgetId: string | null) => void) | undefined + * + * i.e. the interface declared the contract and no consumer was held to it. The + * pins below are exactly those three reads, in their fixed direction. + */ + +import { describe, it, expect } from 'vitest'; +import type { ComponentProps } from 'react'; +import { DashboardRenderer, type DashboardRendererProps } from '../DashboardRenderer'; + +type Assert = T; +type Equal = (() => T extends A ? 1 : 2) extends () => T extends B ? 1 : 2 ? true : false; +type IsAny = 0 extends 1 & T ? true : false; + +/** The props a JSX call site is held to. */ +type CallSiteProps = ComponentProps; + +// 1. The declared callback survives to the call site with its REAL signature. +// Before the fix this was `any`, so a wrong-arity or wrong-typed handler — +// and any prop typo next to it — passed silently. +type _OnWidgetClickIsDeclared = Assert< + Equal void) | undefined> +>; + +// 2. …and that is not vacuously true because the whole thing is `any`. +type _OnWidgetClickIsNotAny = Assert, false>>; + +// 3. The call-site type and the DECLARED interface agree key for key, once the +// `ref` / `key` that `RefAttributes` contributes are set aside. +type _DeclaredKeysAgree = Assert< + Equal, keyof DashboardRendererProps> +>; + +// 4. `keyof` is a union of literal keys, NOT the erased `string | number`. THIS +// is the assertion that discriminates: on the pre-fix shape +// `keyof CallSiteProps` was `string | number`, so `string` extended it and +// this pin was `true` — measured, and it is the whole defect in one line. +// (Note assertion 3 alone would NOT have caught it: pre-fix BOTH sides were +// erased to `string | number`, so they agreed with each other while agreeing +// with nothing the interface declared.) +type _KeysAreNotWidened = Assert>; + +// 5. A named prop the interface declares is reachable and correctly typed. +type _SchemaSurvives = Assert>; +type _DesignModeSurvives = Assert>; + +// 6. The DOM pass-through keys the `toDomProps` whitelist forwards are DECLARED +// rather than reachable through an index signature (objectui#4432 + #4528). +type _OnClickIsDeclared = Assert, false>>; + +// 7. `dataSource` is declared — the render function destructures it and both +// in-repo hosts pass it, but it was only ever reachable through the index +// signature. `'dataSource' extends keyof …` is false the moment it is not. +type _DataSourceIsDeclared = Assert<'dataSource' extends keyof DashboardRendererProps ? true : false>; + +describe('objectui#4528 — DashboardRenderer serves its declared props', () => { + it('pins the resolved call-site props at compile time', () => { + // The assertions are the types above; this body only keeps the file a test. + const probe: CallSiteProps['onWidgetClick'] = (widgetId: string | null) => { + void widgetId; + }; + expect(typeof probe).toBe('function'); + }); +}); diff --git a/packages/plugin-dashboard/src/__tests__/forwardref-props-annotation.guard.test.ts b/packages/plugin-dashboard/src/__tests__/forwardref-props-annotation.guard.test.ts new file mode 100644 index 000000000..ffc680b91 --- /dev/null +++ b/packages/plugin-dashboard/src/__tests__/forwardref-props-annotation.guard.test.ts @@ -0,0 +1,310 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +/** + * objectui#4528 structural guard — the `packages/plugin-dashboard` sibling of + * `packages/components/src/__tests__/forwardref-props-annotation.guard.test.ts` + * (objectui#4422 / PR #4438). + * + * ## Why a sibling and not a widened original + * + * The original resolves its scan root as `path.resolve(here, '..')`, i.e. + * `packages/components/src`, and its own header says so. That ratchet therefore + * structurally could not see this package, and would not have caught the + * offender arriving — which is exactly how `DashboardRenderer` kept the defect + * for the whole life of #4438. objectui#4528 direction 3 (one guard over every + * package's `src`) stays open and is NOT done here: a repo-wide widening goes + * red on `packages/react/src/SchemaRenderer.tsx`, which is outside this card's + * surface. Filed separately; see that finding before widening. + * + * ## The trap + * + * `forwardRef< T, P >` routes `P` through `PropsWithoutRef`, defined in + * `@types/react` as: + * + * Props extends any ? ('ref' extends keyof Props ? Omit< Props, 'ref' > : Props) : Props + * + * A string index signature puts `string` into `keyof Props`, so + * `'ref' extends keyof Props` is ALWAYS true and the `Omit` branch always runs. + * `Omit` over a type carrying a string index signature keeps only the index + * signature — every declared property is erased, on BOTH sides: + * + * * the render function receives `{ [x: string]: any }`, so every prop it + * reads (`schema` included) is `any`; and + * * `ForwardRefExoticComponent`'s public props come through the same alias, + * so every JSX CALL SITE is unchecked too. Measured on the pre-fix source: + * `keyof React.ComponentProps< typeof DashboardRenderer >` was + * `string | number` and `...['onWidgetClick']` was `any`, while + * `DashboardRendererProps` went on declaring + * `(widgetId: string | null) => void`. + * + * It is SILENT: the props type is right there in the source, so the component + * reads as typed to every reviewer and every tool, and `noImplicitAny` does not + * fire because the `any` is supplied EXPLICITLY by the index signature. + * + * ## Scope — WIDER than the original's, deliberately + * + * The original judges only `forwardRef` calls whose render function + * DESTRUCTURES a `schema` prop. That heuristic misses this package's public + * component: `DashboardRenderer` takes `(props, ref)` whole and forwards it, so + * it destructures nothing — yet it is precisely the call-site half objectui#4528 + * measured. So this guard's population is instead "every `forwardRef` whose + * props TYPE ARGUMENT is a type declared in the SAME file", i.e. every site + * where this package owns the props contract and a source scan can actually + * read it. A props type imported from elsewhere stays out of reach of a source + * scan and is not claimed to be covered (`ListViewBlock` in the sibling package + * is the worked example) — it is covered at its declaration site instead. + * + * ## If this fails + * + * Do not add the file to an allowlist, and do not delete a parameter annotation + * to make the error go away — that silently untypes every prop the render + * function reads. Keep the string index signature OFF the `forwardRef` type + * argument, and annotate the render function's first parameter when it + * destructures. Both halves move together: once `Omit` has erased the props, a + * required prop in the annotation is a TS2345 on the render function itself. + */ + +import { describe, it, expect } from 'vitest'; +import { readdirSync, readFileSync, statSync } from 'node:fs'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; +import ts from 'typescript'; + +const here = path.dirname(fileURLToPath(import.meta.url)); +// packages/plugin-dashboard/src/__tests__ -> packages/plugin-dashboard/src +const srcRoot = path.resolve(here, '..'); + +function collectSourceFiles(root: string): string[] { + const out: string[] = []; + const walk = (dir: string) => { + for (const entry of readdirSync(dir, { withFileTypes: true })) { + const name = entry.name; + if (name === 'node_modules' || name === 'dist' || name === '__tests__') continue; + const full = path.join(dir, name); + if (entry.isDirectory()) walk(full); + else if (/\.tsx?$/.test(name) && !/\.(test|spec)\.tsx?$/.test(name)) out.push(full); + } + }; + if (statSync(root).isDirectory()) walk(root); + return out; +} + +/** A `forwardRef` call site, reduced to the facts this guard judges. */ +interface Site { + file: string; + line: number; + /** The render function's first parameter is an object binding pattern. */ + destructures: boolean; + /** The render function's first parameter carries a direct type annotation. */ + annotated: boolean; + /** The props TYPE ARGUMENT syntactically carries a string index signature. */ + indexSignatureOnTypeArg: boolean; +} + +/** Does this type node syntactically carry a string index signature? */ +function hasStringIndexSignature( + node: ts.TypeNode | undefined, + localTypes: Map, + seen = new Set(), +): boolean { + if (!node) return false; + const members = (n: ts.Node): readonly ts.TypeElement[] | undefined => + ts.isTypeLiteralNode(n) || ts.isInterfaceDeclaration(n) ? n.members : undefined; + + const scan = (n: ts.Node): boolean => { + const ms = members(n); + if (ms) { + for (const m of ms) { + if (ts.isIndexSignatureDeclaration(m)) { + const p = m.parameters[0]; + if (p?.type && p.type.kind === ts.SyntaxKind.StringKeyword) return true; + } + } + // an interface may inherit one + if (ts.isInterfaceDeclaration(n) && n.heritageClauses) { + for (const h of n.heritageClauses) { + for (const t of h.types) { + if (ts.isIdentifier(t.expression) && localTypes.has(t.expression.text)) { + const target = localTypes.get(t.expression.text)!; + if (!seen.has(t.expression.text)) { + seen.add(t.expression.text); + if (scan(target)) return true; + } + } + } + } + } + return false; + } + if (ts.isTypeAliasDeclaration(n)) return scan(n.type); + if (ts.isIntersectionTypeNode(n) || ts.isUnionTypeNode(n)) return n.types.some(scan); + if (ts.isParenthesizedTypeNode(n)) return scan(n.type); + if (ts.isTypeReferenceNode(n) && ts.isIdentifier(n.typeName)) { + const name = n.typeName.text; + if (seen.has(name)) return false; + seen.add(name); + const decl = localTypes.get(name); + return decl ? scan(decl) : false; + } + return false; + }; + return scan(node); +} + +/** Every `forwardRef(...)` whose props type argument is declared in this file. */ +function collectSitesFrom(file: string, text: string): Site[] { + const sf = ts.createSourceFile(file, text, ts.ScriptTarget.Latest, true, ts.ScriptKind.TSX); + + const localTypes = new Map(); + const indexDecls = (n: ts.Node): void => { + if (ts.isInterfaceDeclaration(n) || ts.isTypeAliasDeclaration(n)) localTypes.set(n.name.text, n); + ts.forEachChild(n, indexDecls); + }; + indexDecls(sf); + + const sites: Site[] = []; + const visit = (node: ts.Node): void => { + if (ts.isCallExpression(node)) { + const callee = node.expression; + const isForwardRef = + (ts.isIdentifier(callee) && callee.text === 'forwardRef') || + (ts.isPropertyAccessExpression(callee) && callee.name.text === 'forwardRef'); + if (isForwardRef) { + const typeArg = node.typeArguments?.[1]; + // In scope only when THIS file owns the props contract: an inline type + // literal, or a named type declared here. A props type imported from + // elsewhere is out of a source scan's reach and is covered where it is + // declared instead. + let ownsProps = false; + if (typeArg) { + ownsProps = + !ts.isTypeReferenceNode(typeArg) || + !ts.isIdentifier(typeArg.typeName) || + localTypes.has(typeArg.typeName.text); + } + const render = node.arguments[0]; + if (ownsProps && render && (ts.isArrowFunction(render) || ts.isFunctionExpression(render))) { + const first = render.parameters[0]; + sites.push({ + file, + line: sf.getLineAndCharacterOfPosition(node.getStart()).line + 1, + destructures: !!first && ts.isObjectBindingPattern(first.name), + annotated: !!first?.type, + indexSignatureOnTypeArg: hasStringIndexSignature(typeArg, localTypes), + }); + } + } + } + ts.forEachChild(node, visit); + }; + visit(sf); + return sites; +} + +const collectSites = (file: string): Site[] => collectSitesFrom(file, readFileSync(file, 'utf8')); + +const ALL_SITES = collectSourceFiles(srcRoot).flatMap(collectSites); +const rel = (s: Site) => `${path.relative(srcRoot, s.file)}:${s.line}`; + +describe('objectui#4528 — plugin-dashboard forwardRef props must not be erased', () => { + it('finds the population (guards against a broken scan)', () => { + // If this collapses, the walk or the AST matcher has gone stale and the + // guard would silently pass on nothing. Two sites exist at the time of + // writing — `DashboardRendererInner` and the public `DashboardRenderer`, + // both on `DashboardRendererProps`. The floor is deliberately loose so + // adding or removing one component does not fail the wrong assertion. + expect(ALL_SITES.length).toBeGreaterThanOrEqual(2); + }); + + it('detects the shapes it is meant to ban (guards against a dead matcher)', () => { + // A guard whose matcher silently stops matching is worse than no guard, so + // pin it against the exact pre-fix shapes plus the spellings that erase + // props identically. Compiled in memory — no fixture files. + const scan = (src: string): Site[] => + collectSitesFrom(path.join(srcRoot, '__inmemory__.tsx'), src); + + // 1. This package's exact PRE-FIX shape: a named interface carrying the + // index signature, handed straight to `forwardRef`, render function + // destructuring with no annotation. This is what erased every declared + // prop of `DashboardRendererProps`. + const named = scan( + 'interface P { schema: XSchema; onWidgetClick?: (id: string | null) => void; [key: string]: any }\n' + + 'const C = forwardRef(({ schema, ...props }, ref) => null);', + ); + expect(named).toHaveLength(1); + expect(named[0].indexSignatureOnTypeArg).toBe(true); + expect(named[0].annotated).toBe(false); + + // 2. The PUBLIC half, which the objectui#4422 guard's `schema`-destructuring + // heuristic cannot see: props taken whole and forwarded. `DashboardRenderer` + // itself is spelled exactly this way, so a guard blind to it would have + // passed on the very component this card is about. + const whole = scan( + 'interface P { schema: XSchema; [key: string]: any }\n' + + 'const C = forwardRef((props, ref) => null);', + ); + expect(whole).toHaveLength(1); + expect(whole[0].destructures).toBe(false); + expect(whole[0].indexSignatureOnTypeArg).toBe(true); + + // 3. Hidden one level further, behind a type alias and an intersection. + const aliased = scan( + 'type Pass = { [key: string]: any };\n' + + 'type P = { schema: XSchema } & Pass;\n' + + 'const C = forwardRef(({ schema }, ref) => null);', + ); + expect(aliased[0].indexSignatureOnTypeArg).toBe(true); + + // 4. And the compliant shape must read as compliant: signature off the type + // argument, on the parameter annotation, so the spread still collects + // arbitrary keys while the declared props survive. + const fixed = scan( + 'interface P { schema: XSchema; className?: string }\n' + + 'const C = forwardRef(' + + '({ schema, className, ...props }: P & { [key: string]: any }, ref) => null);', + ); + expect(fixed).toHaveLength(1); + expect(fixed[0].annotated).toBe(true); + expect(fixed[0].indexSignatureOnTypeArg).toBe(false); + + // 5. A NUMBER index signature does not trigger the collapse (`keyof` still + // excludes the string `'ref'`), so it must not be reported. + const numeric = scan( + 'const C = forwardRef(({ schema }, ref) => null);', + ); + expect(numeric[0].indexSignatureOnTypeArg).toBe(false); + + // 6. A props type IMPORTED from another file is out of a source scan's + // reach, and this guard does not pretend otherwise — it reports nothing + // rather than reporting a false verdict. + const imported = scan( + "import type { P } from './elsewhere';\n" + + 'const C = forwardRef(({ schema }, ref) => null);', + ); + expect(imported).toEqual([]); + }); + + it('no forwardRef carries a string index signature on its props type argument', () => { + // If this fails: take the `[key: string]: any` OFF the type argument. On it, + // `PropsWithoutRef` collapses the props to the bare index signature, which + // erases every declared property from the render function AND from every + // JSX call site. Move it to the render function's parameter annotation if + // the component really does forward arbitrary keys. Do not allowlist. + const offenders = ALL_SITES.filter(s => s.indexSignatureOnTypeArg).map(rel); + expect(offenders).toEqual([]); + }); + + it('every destructuring forwardRef annotates its props parameter', () => { + // If this fails: annotate the render function's FIRST PARAMETER directly. + // Without it the parameter's type comes from `PropsWithoutRef` of the type + // argument, and every declared prop the render function reads is `any`. + const offenders = ALL_SITES.filter(s => s.destructures && !s.annotated).map(rel); + expect(offenders).toEqual([]); + }); +}); diff --git a/packages/plugin-list/src/ListView.tsx b/packages/plugin-list/src/ListView.tsx index 902c0ecb6..2f0c718e8 100644 --- a/packages/plugin-list/src/ListView.tsx +++ b/packages/plugin-list/src/ListView.tsx @@ -15,6 +15,7 @@ import { ViewSwitcherDropdown, ViewType } from './ViewSwitcher'; import { ViewSettingsPopover } from './components/ViewSettingsPopover'; import { UserFilters } from './UserFilters'; import { SchemaRenderer, useNavigationOverlay } from '@object-ui/react'; +import type { NavigationConfig } from '@object-ui/react'; import { useDensityMode } from '@object-ui/react'; import type { ListViewSchema } from '@object-ui/types'; import { detectStatusField } from '@object-ui/types'; @@ -23,9 +24,46 @@ import { resolveConditionalFormatting, buildExpandFields, buildExportFileName, r import { useObjectTranslation, useObjectLabel, useSafeFieldLabel, createSafeTranslation } from '@object-ui/i18n'; import { usePermissions } from '@object-ui/permissions'; +/** + * The list view's props. + * + * ## Why there is no `[key: string]: any` here (objectui#4528) + * + * There used to be one, and it erased this entire interface. A string index + * signature puts `string` into `keyof Props`, so `'ref' extends keyof Props` is + * always true and React's `PropsWithoutRef` takes its `Omit` branch — and + * `Omit` over a type carrying a string index signature keeps ONLY the index + * signature. Every declared property below was dropped from the resolved type. + * Measured on the pre-fix source: + * `keyof React.ComponentProps< typeof ListView >` was `string | number`, and + * `React.ComponentProps< typeof ListView >['onRowClick']` was `any`, while this + * interface went on declaring `(record: Record< string, unknown >) => void`. + * + * The same trap, in `packages/components`, is objectui#4422 / PR #4438; this is + * the sweep of the two packages that issue left unswept. + * + * ## The props that were only ever reachable through it + * + * `ListView` reads several props it never declared, because the index signature + * was answering for them — `dataSource`, `onAddRecord`, `onBulkAction`, + * `onPageSizeChange` are read directly, and `onEdit` / `onDelete` / + * `onBulkDelete` ride the `{...props}` forward into the active view component + * (`ObjectGrid` declares all three). They are declared by name below, at the + * type each one actually lands on, rather than left to an index signature that + * types them `any` and erases everything around them. + */ export interface ListViewProps { schema: ListViewSchema; className?: string; + /** + * Data-source adapter. Read directly (`dataSource.find`, + * `dataSource.getObjectSchema`, `dataSource.onMutation`) and forwarded to the + * active view component. Typed `any` deliberately: that is what it resolved + * to before objectui#4528, so declaring it changes what is DECLARED without + * changing what any call site is held to. Narrowing it to a real adapter type + * is a separate change with its own consumer sweep. + */ + dataSource?: any; onViewChange?: (view: ViewType) => void; onFilterChange?: (filters: any) => void; onSortChange?: (sort: any) => void; @@ -53,7 +91,21 @@ export interface ListViewProps { initialFilters?: FilterGroup; /** Initial search term to restore at mount (same one-shot semantics as `initialFilters`). */ initialSearchTerm?: string; - [key: string]: any; + /** Called when the user asks for a new record (toolbar "+ New" and the empty-state CTA). */ + onAddRecord?: () => void; + /** Called with a non-delete bulk action key and the currently selected rows. */ + onBulkAction?: (action: string, records: any[]) => void; + /** Called when the user picks a different page size in the pager. */ + onPageSizeChange?: (pageSize: number) => void; + /** + * Row-level affordances forwarded to the active view component. `ObjectGrid` + * declares all three with exactly these signatures; they are named here so the + * hosts that pass them (`ObjectView`, `StudioDesignSurface`) are held to a + * contract instead of to an index signature. + */ + onEdit?: (record: any) => void; + onDelete?: (record: any) => void; + onBulkDelete?: (records: any[]) => void; } // Helper to convert FilterBuilder group to ObjectStack AST. @@ -648,7 +700,7 @@ export const ListView = React.forwardRef(({ initialFilters, initialSearchTerm, ...props -}, ref) => { +}: ListViewProps & { [key: string]: any }, ref) => { // The switcher can be enabled either by the host component (prop) or by // the schema itself (ADR-0047 — ObjectView/InterfaceListPage stamp it on // the schema when appearance.allowedVisualizations whitelists >1 type). @@ -1065,7 +1117,12 @@ export const ListView = React.forwardRef(({ // nowhere (declined platform-side — objectstack#1301). Declared-but-dead // formats used to render as menu items whose click did nothing; now they're // dropped from the menu (with a one-time warning for the app author). - const exportableFormats = React.useMemo(() => { + // Annotated `string[]` rather than inferred: `resolvedExportOptions.formats` + // is the spec's literal union, so the inferred element type made the + // `exportableFormats.includes(f)` below (whose `f` is a plain `string`) a + // TS2345. Only visible since objectui#4528 gave this render function a real + // `schema` type — the index signature used to resolve it to `any`. + const exportableFormats = React.useMemo(() => { const declared = resolvedExportOptions?.formats || ['csv', 'json']; const serverAvailable = typeof dataSource?.exportDownload === 'function' && !!schema.objectName @@ -1670,8 +1727,15 @@ export const ListView = React.forwardRef(({ }, [onSearchChange]); // --- NavigationConfig support --- + // The assertion bridges two spellings of ONE spec object and changes no + // value: `@object-ui/react`'s `NavigationConfig` alias re-declares `mode` as + // NON-optional, while the spec-derived `ListViewSchema['navigation']` leaves + // it optional. The hook's own body defaults it (`navigation?.mode ?? 'page'`), + // so a spec-shaped value is valid input and only the alias is tighter than + // its implementation. Surfaced by objectui#4528: this call used to type-check + // for the wrong reason, because the erased props type made `schema` `any`. const navigation = useNavigationOverlay({ - navigation: schema.navigation, + navigation: schema.navigation as NavigationConfig | undefined, objectName: schema.objectName, onNavigate: schema.onNavigate, onRowClick, diff --git a/packages/plugin-list/src/__tests__/ListView.propsResolution.test.ts b/packages/plugin-list/src/__tests__/ListView.propsResolution.test.ts new file mode 100644 index 000000000..8678f99f4 --- /dev/null +++ b/packages/plugin-list/src/__tests__/ListView.propsResolution.test.ts @@ -0,0 +1,100 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +/** + * objectui#4528 — COMPILE-TIME pins on the props a `ListView` JSX call site is + * actually held to. + * + * These assertions are erased at runtime; `tsc` is the only thing that can + * check them, which is why this file is carried by + * `packages/plugin-list/tsconfig.test.json` and why the `expect` below is + * deliberately trivial — the real assertions are the `Assert< Equal< … > >` + * types, and a violation is a compile error, not a red test. + * + * ## What was measured before the fix + * + * The card objectui#4528 asserted this package's shape BY INSPECTION rather + * than by measurement. It was then measured, on the pre-fix source, compiled + * through this same project — and matched the sibling package exactly: + * + * keyof React.ComponentProps< typeof ListView > -> string | number + * React.ComponentProps< typeof ListView >['onRowClick'] -> any + * ListViewProps['onRowClick'] -> ((record: Record< string, unknown >) => void) | undefined + * + * `ListViewProps` carried a `[key: string]: any`, which puts `string` into + * `keyof Props`, so React's `PropsWithoutRef` took its `Omit` branch and `Omit` + * over a string index signature keeps ONLY the index signature. The interface + * declared the contract and no consumer was held to it. The pins below are + * exactly those reads, in their fixed direction. + */ + +import { describe, it, expect } from 'vitest'; +import type { ComponentProps } from 'react'; +import { ListView, type ListViewProps } from '../ListView'; + +type Assert = T; +type Equal = (() => T extends A ? 1 : 2) extends () => T extends B ? 1 : 2 ? true : false; +type IsAny = 0 extends 1 & T ? true : false; + +/** The props a JSX call site is held to. */ +type CallSiteProps = ComponentProps; + +// 1. The declared callback survives to the call site with its REAL signature. +// Before the fix this was `any`, so a wrong-typed handler — and any prop +// typo next to it — passed silently. +type _OnRowClickIsDeclared = Assert< + Equal) => void) | undefined> +>; + +// 2. …and that is not vacuously true because the whole thing is `any`. +type _OnRowClickIsNotAny = Assert, false>>; + +// 3. The call-site type and the DECLARED interface agree key for key, once the +// `ref` / `key` that `RefAttributes` contributes are set aside. +type _DeclaredKeysAgree = Assert< + Equal, keyof ListViewProps> +>; + +// 4. `keyof` is a union of literal keys, NOT the erased `string | number`. THIS +// is the assertion that discriminates: on the pre-fix shape +// `keyof CallSiteProps` was `string | number`, so `string` extended it and +// this pin was `true` — measured, and it is the whole defect in one line. +// (Note assertion 3 alone would NOT have caught it: pre-fix BOTH sides were +// erased to `string | number`, so they agreed with each other while agreeing +// with nothing the interface declared.) +type _KeysAreNotWidened = Assert>; + +// 5. Named props the interface declares are reachable and correctly typed. +type _SchemaSurvives = Assert>; +type _ShowViewSwitcherSurvives = Assert>; +type _InitialSearchTermSurvives = Assert>; + +// 6. The props this component READS off its rest object are declared by name +// rather than reachable only through an index signature (objectui#4528). +type _OnAddRecordIsDeclared = Assert void) | undefined>>; +type _OnPageSizeChangeIsDeclared = Assert< + Equal void) | undefined> +>; +type _DataSourceIsDeclared = Assert<'dataSource' extends keyof ListViewProps ? true : false>; + +// 7. The row affordances forwarded to the active view component are declared +// with the signatures `ObjectGrid` receives them at. +type _OnEditIsDeclared = Assert void) | undefined>>; +type _OnBulkDeleteIsDeclared = Assert< + Equal void) | undefined> +>; + +describe('objectui#4528 — ListView serves its declared props', () => { + it('pins the resolved call-site props at compile time', () => { + // The assertions are the types above; this body only keeps the file a test. + const probe: CallSiteProps['onRowClick'] = (record: Record) => { + void record; + }; + expect(typeof probe).toBe('function'); + }); +}); diff --git a/packages/plugin-list/src/__tests__/forwardref-props-annotation.guard.test.ts b/packages/plugin-list/src/__tests__/forwardref-props-annotation.guard.test.ts new file mode 100644 index 000000000..410067cee --- /dev/null +++ b/packages/plugin-list/src/__tests__/forwardref-props-annotation.guard.test.ts @@ -0,0 +1,314 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +/** + * objectui#4528 structural guard — the `packages/plugin-list` sibling of + * `packages/components/src/__tests__/forwardref-props-annotation.guard.test.ts` + * (objectui#4422 / PR #4438). + * + * ## Why a sibling and not a widened original + * + * The original resolves its scan root as `path.resolve(here, '..')`, i.e. + * `packages/components/src`, and its own header says so. That ratchet therefore + * structurally could not see this package, and would not have caught the + * offender arriving — which is exactly how `ListView` kept the defect for the + * whole life of #4438. objectui#4528 direction 3 (one guard over every + * package's `src`) stays open and is NOT done here: a repo-wide widening goes + * red on `packages/react/src/SchemaRenderer.tsx`, which is outside this card's + * surface. Filed separately; see that finding before widening. + * + * ## The trap + * + * `forwardRef< T, P >` routes `P` through `PropsWithoutRef`, defined in + * `@types/react` as: + * + * Props extends any ? ('ref' extends keyof Props ? Omit< Props, 'ref' > : Props) : Props + * + * A string index signature puts `string` into `keyof Props`, so + * `'ref' extends keyof Props` is ALWAYS true and the `Omit` branch always runs. + * `Omit` over a type carrying a string index signature keeps only the index + * signature — every declared property is erased, on BOTH sides: + * + * * the render function receives `{ [x: string]: any }`, so every prop it + * reads (`schema` included) is `any` — which is how two genuine type + * defects inside `ListView` survived until objectui#4528 annotated the + * parameter and gave `schema` a real type; and + * * `ForwardRefExoticComponent`'s public props come through the same alias, + * so every JSX CALL SITE is unchecked too. Measured on the pre-fix source: + * `keyof React.ComponentProps< typeof ListView >` was `string | number` and + * `...['onRowClick']` was `any`, while `ListViewProps` went on declaring + * `(record: Record< string, unknown >) => void`. + * + * It is SILENT: the props type is right there in the source, so the component + * reads as typed to every reviewer and every tool, and `noImplicitAny` does not + * fire because the `any` is supplied EXPLICITLY by the index signature. + * + * ## Scope — WIDER than the original's, deliberately + * + * The original judges only `forwardRef` calls whose render function + * DESTRUCTURES a `schema` prop. That heuristic misses components that take + * their props whole and forward them, which is the shape of the sibling + * package's public `DashboardRenderer` — and it is precisely the call-site half + * objectui#4528 measured. So this guard's population is instead "every + * `forwardRef` whose props TYPE ARGUMENT is a type declared in the SAME file", + * i.e. every site where this package owns the props contract and a source scan + * can actually read it. + * + * `ListViewBlock` is the worked example of what that leaves out: it is a + * `forwardRef` on `ListViewProps` IMPORTED from `./ListView`, so a source scan + * cannot see the declaration and this guard does not pretend to judge it. It is + * covered where the type is declared — here, on `ListView.tsx` — which is the + * only place the index signature could come back. + * + * ## If this fails + * + * Do not add the file to an allowlist, and do not delete a parameter annotation + * to make the error go away — that silently untypes every prop the render + * function reads. Keep the string index signature OFF the `forwardRef` type + * argument, and annotate the render function's first parameter when it + * destructures. Both halves move together: once `Omit` has erased the props, a + * required prop in the annotation is a TS2345 on the render function itself. + */ + +import { describe, it, expect } from 'vitest'; +import { readdirSync, readFileSync, statSync } from 'node:fs'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; +import ts from 'typescript'; + +const here = path.dirname(fileURLToPath(import.meta.url)); +// packages/plugin-list/src/__tests__ -> packages/plugin-list/src +const srcRoot = path.resolve(here, '..'); + +function collectSourceFiles(root: string): string[] { + const out: string[] = []; + const walk = (dir: string) => { + for (const entry of readdirSync(dir, { withFileTypes: true })) { + const name = entry.name; + if (name === 'node_modules' || name === 'dist' || name === '__tests__') continue; + const full = path.join(dir, name); + if (entry.isDirectory()) walk(full); + else if (/\.tsx?$/.test(name) && !/\.(test|spec)\.tsx?$/.test(name)) out.push(full); + } + }; + if (statSync(root).isDirectory()) walk(root); + return out; +} + +/** A `forwardRef` call site, reduced to the facts this guard judges. */ +interface Site { + file: string; + line: number; + /** The render function's first parameter is an object binding pattern. */ + destructures: boolean; + /** The render function's first parameter carries a direct type annotation. */ + annotated: boolean; + /** The props TYPE ARGUMENT syntactically carries a string index signature. */ + indexSignatureOnTypeArg: boolean; +} + +/** Does this type node syntactically carry a string index signature? */ +function hasStringIndexSignature( + node: ts.TypeNode | undefined, + localTypes: Map, + seen = new Set(), +): boolean { + if (!node) return false; + const members = (n: ts.Node): readonly ts.TypeElement[] | undefined => + ts.isTypeLiteralNode(n) || ts.isInterfaceDeclaration(n) ? n.members : undefined; + + const scan = (n: ts.Node): boolean => { + const ms = members(n); + if (ms) { + for (const m of ms) { + if (ts.isIndexSignatureDeclaration(m)) { + const p = m.parameters[0]; + if (p?.type && p.type.kind === ts.SyntaxKind.StringKeyword) return true; + } + } + // an interface may inherit one + if (ts.isInterfaceDeclaration(n) && n.heritageClauses) { + for (const h of n.heritageClauses) { + for (const t of h.types) { + if (ts.isIdentifier(t.expression) && localTypes.has(t.expression.text)) { + const target = localTypes.get(t.expression.text)!; + if (!seen.has(t.expression.text)) { + seen.add(t.expression.text); + if (scan(target)) return true; + } + } + } + } + } + return false; + } + if (ts.isTypeAliasDeclaration(n)) return scan(n.type); + if (ts.isIntersectionTypeNode(n) || ts.isUnionTypeNode(n)) return n.types.some(scan); + if (ts.isParenthesizedTypeNode(n)) return scan(n.type); + if (ts.isTypeReferenceNode(n) && ts.isIdentifier(n.typeName)) { + const name = n.typeName.text; + if (seen.has(name)) return false; + seen.add(name); + const decl = localTypes.get(name); + return decl ? scan(decl) : false; + } + return false; + }; + return scan(node); +} + +/** Every `forwardRef(...)` whose props type argument is declared in this file. */ +function collectSitesFrom(file: string, text: string): Site[] { + const sf = ts.createSourceFile(file, text, ts.ScriptTarget.Latest, true, ts.ScriptKind.TSX); + + const localTypes = new Map(); + const indexDecls = (n: ts.Node): void => { + if (ts.isInterfaceDeclaration(n) || ts.isTypeAliasDeclaration(n)) localTypes.set(n.name.text, n); + ts.forEachChild(n, indexDecls); + }; + indexDecls(sf); + + const sites: Site[] = []; + const visit = (node: ts.Node): void => { + if (ts.isCallExpression(node)) { + const callee = node.expression; + const isForwardRef = + (ts.isIdentifier(callee) && callee.text === 'forwardRef') || + (ts.isPropertyAccessExpression(callee) && callee.name.text === 'forwardRef'); + if (isForwardRef) { + const typeArg = node.typeArguments?.[1]; + // In scope only when THIS file owns the props contract: an inline type + // literal, or a named type declared here. A props type imported from + // elsewhere (`ListViewBlock`) is out of a source scan's reach and is + // covered where it is declared instead. + let ownsProps = false; + if (typeArg) { + ownsProps = + !ts.isTypeReferenceNode(typeArg) || + !ts.isIdentifier(typeArg.typeName) || + localTypes.has(typeArg.typeName.text); + } + const render = node.arguments[0]; + if (ownsProps && render && (ts.isArrowFunction(render) || ts.isFunctionExpression(render))) { + const first = render.parameters[0]; + sites.push({ + file, + line: sf.getLineAndCharacterOfPosition(node.getStart()).line + 1, + destructures: !!first && ts.isObjectBindingPattern(first.name), + annotated: !!first?.type, + indexSignatureOnTypeArg: hasStringIndexSignature(typeArg, localTypes), + }); + } + } + } + ts.forEachChild(node, visit); + }; + visit(sf); + return sites; +} + +const collectSites = (file: string): Site[] => collectSitesFrom(file, readFileSync(file, 'utf8')); + +const ALL_SITES = collectSourceFiles(srcRoot).flatMap(collectSites); +const rel = (s: Site) => `${path.relative(srcRoot, s.file)}:${s.line}`; + +describe('objectui#4528 — plugin-list forwardRef props must not be erased', () => { + it('finds the population (guards against a broken scan)', () => { + // If this collapses, the walk or the AST matcher has gone stale and the + // guard would silently pass on nothing. One site exists at the time of + // writing — `ListView` on `ListViewProps`; `ListViewBlock` is deliberately + // out of scope (imported props type, see the header). + expect(ALL_SITES.length).toBeGreaterThanOrEqual(1); + }); + + it('detects the shapes it is meant to ban (guards against a dead matcher)', () => { + // A guard whose matcher silently stops matching is worse than no guard, so + // pin it against the exact pre-fix shapes plus the spellings that erase + // props identically. Compiled in memory — no fixture files. + const scan = (src: string): Site[] => + collectSitesFrom(path.join(srcRoot, '__inmemory__.tsx'), src); + + // 1. This package's exact PRE-FIX shape: a named interface carrying the + // index signature, handed straight to `forwardRef`, render function + // destructuring with no annotation. This is what erased every declared + // prop of `ListViewProps`. + const named = scan( + 'interface P { schema: XSchema; onRowClick?: (r: Record) => void; [key: string]: any }\n' + + 'const C = React.forwardRef(({ schema, ...props }, ref) => null);', + ); + expect(named).toHaveLength(1); + expect(named[0].indexSignatureOnTypeArg).toBe(true); + expect(named[0].annotated).toBe(false); + + // 2. Props taken whole and forwarded — the shape the objectui#4422 guard's + // `schema`-destructuring heuristic cannot see. The sibling package's + // public `DashboardRenderer` is spelled exactly this way, so a guard + // blind to it would pass on a fully erased public component. + const whole = scan( + 'interface P { schema: XSchema; [key: string]: any }\n' + + 'const C = React.forwardRef((props, ref) => null);', + ); + expect(whole).toHaveLength(1); + expect(whole[0].destructures).toBe(false); + expect(whole[0].indexSignatureOnTypeArg).toBe(true); + + // 3. Hidden one level further, behind a type alias and an intersection. + const aliased = scan( + 'type Pass = { [key: string]: any };\n' + + 'type P = { schema: XSchema } & Pass;\n' + + 'const C = React.forwardRef(({ schema }, ref) => null);', + ); + expect(aliased[0].indexSignatureOnTypeArg).toBe(true); + + // 4. And the compliant shape must read as compliant: signature off the type + // argument, on the parameter annotation, so the spread still collects + // arbitrary keys while the declared props survive. + const fixed = scan( + 'interface P { schema: XSchema; className?: string }\n' + + 'const C = React.forwardRef(' + + '({ schema, className, ...props }: P & { [key: string]: any }, ref) => null);', + ); + expect(fixed).toHaveLength(1); + expect(fixed[0].annotated).toBe(true); + expect(fixed[0].indexSignatureOnTypeArg).toBe(false); + + // 5. A NUMBER index signature does not trigger the collapse (`keyof` still + // excludes the string `'ref'`), so it must not be reported. + const numeric = scan( + 'const C = React.forwardRef(({ schema }, ref) => null);', + ); + expect(numeric[0].indexSignatureOnTypeArg).toBe(false); + + // 6. A props type IMPORTED from another file — `ListViewBlock`'s shape — is + // out of a source scan's reach, and this guard does not pretend + // otherwise: it reports nothing rather than reporting a false verdict. + const imported = scan( + "import type { P } from './ListView';\n" + + 'const C = React.forwardRef((props, ref) => null);', + ); + expect(imported).toEqual([]); + }); + + it('no forwardRef carries a string index signature on its props type argument', () => { + // If this fails: take the `[key: string]: any` OFF the type argument. On it, + // `PropsWithoutRef` collapses the props to the bare index signature, which + // erases every declared property from the render function AND from every + // JSX call site. Move it to the render function's parameter annotation if + // the component really does forward arbitrary keys. Do not allowlist. + const offenders = ALL_SITES.filter(s => s.indexSignatureOnTypeArg).map(rel); + expect(offenders).toEqual([]); + }); + + it('every destructuring forwardRef annotates its props parameter', () => { + // If this fails: annotate the render function's FIRST PARAMETER directly. + // Without it the parameter's type comes from `PropsWithoutRef` of the type + // argument, and every declared prop the render function reads is `any`. + const offenders = ALL_SITES.filter(s => s.destructures && !s.annotated).map(rel); + expect(offenders).toEqual([]); + }); +});