From 69d68e7a9b9fc20386a10c832fda15307911e54c Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 13 Aug 2026 07:59:57 +0000 Subject: [PATCH] =?UTF-8?q?fix(types):=20DashboardRenderer=20and=20ListVie?= =?UTF-8?q?w=20serve=20their=20declared=20props=20=E2=80=94=20the=20index?= =?UTF-8?q?=20signature=20stops=20erasing=20them=20(#4528)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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, 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 erased 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 `...['onWidgetClick']` was `any`, while the interface declared `(widgetId: string | null) => void`. plugin-list, asserted by inspection in the card, measured identically for `onRowClick`. Type-only: the emitted JS for both packages is byte-identical before and after (sha256 on dist/index.js and dist/index.umd.cjs), and both runtime suites are untouched and green (87 files, 939 tests). Props each component genuinely reads but never declared are now declared by name at the type each lands on. DashboardRenderer's DOM pass-through keys are derived from `toDomProps`' whitelist constant so declaration and runtime filter cannot drift. Three latent defects the erasure hid, each surfaced by the repo-wide canary: DashboardWithConfig typed its handler `(widgetId: string)` while the renderer calls `onWidgetClick(null)` to deselect; InterfaceListPage built a schema whose `viewType` was a bare `string`; StudioDesignSurface forwarded a `refreshKey` that nothing in the chain declares or reads. Per-package structural guards pin the shape, covering the public `forwardRef` that takes its props whole — the spelling #4438's schema-destructuring scan could not see. Refs #4422, #4438, #4426, #4040. Findings filed: #4548, #4549, #4550. --- .changeset/forwardref-prop-erasure-4528.md | 17 + .../app-shell/src/views/InterfaceListPage.tsx | 9 +- .../studio-design/StudioDesignSurface.tsx | 17 +- .../src/DashboardRenderer.tsx | 53 ++- .../src/DashboardWithConfig.tsx | 11 +- .../DashboardRenderer.domProps.test.tsx | 24 +- .../DashboardRenderer.propsResolution.test.ts | 91 +++++ .../forwardref-props-annotation.guard.test.ts | 310 +++++++++++++++++ packages/plugin-list/src/ListView.tsx | 72 +++- .../ListView.propsResolution.test.ts | 100 ++++++ .../forwardref-props-annotation.guard.test.ts | 314 ++++++++++++++++++ 11 files changed, 997 insertions(+), 21 deletions(-) create mode 100644 .changeset/forwardref-prop-erasure-4528.md create mode 100644 packages/plugin-dashboard/src/__tests__/DashboardRenderer.propsResolution.test.ts create mode 100644 packages/plugin-dashboard/src/__tests__/forwardref-props-annotation.guard.test.ts create mode 100644 packages/plugin-list/src/__tests__/ListView.propsResolution.test.ts create mode 100644 packages/plugin-list/src/__tests__/forwardref-props-annotation.guard.test.ts diff --git a/.changeset/forwardref-prop-erasure-4528.md b/.changeset/forwardref-prop-erasure-4528.md new file mode 100644 index 0000000000..46bc5cbe69 --- /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 a4e819aa4f..ae78e1cb09 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 5dd94610f8..10bfb1e507 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 256e71dd02..4d1738246b 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 4c8e92001a..fa264a3bbc 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 fb3a2b9173..7419597c90 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 0000000000..4e03266e0c --- /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 0000000000..ffc680b913 --- /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 902c0ecb67..2f0c718e89 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 0000000000..8678f99f4d --- /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 0000000000..410067ceed --- /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([]); + }); +});