From cff8fe9a6ae6d798752684e1b096ca1c7c404526 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Sat, 3 Oct 2026 16:38:46 +0200 Subject: [PATCH] test(frontend): a React Compiler ratchet over everything the build ships compilerCoverage.contract.test.ts used to check two files. It now runs the real compiler over all of src (minus tests) and holds the exact set it skips today, file -> function -> category: 93 functions. A new skip or a category change is red, and so is a listed skip that has gone, so the list only shrinks. Channels and DataTable must also show up as compiled, so a harness that compiled nothing cannot pass. Measured against the audit's scan: no function this sprint touched started to skip. PlexPersonView and useOverlayOrder now compile. reactPlugin.ts claimed the compiler's rules were linted, which was false while react-hooks/refs was off. That comment and the refs block in eslint.config.js now say what holds the line and why refs is still off: 245 real hits, pinned by the ratchet, burned down from S3. --- frontend/eslint.config.js | 18 +- frontend/reactPlugin.ts | 10 +- .../compilerCoverage.contract.test.ts | 190 ++++++++++++------ .../src/components/compilerSkips.testkit.ts | 124 ++++++++++++ 4 files changed, 274 insertions(+), 68 deletions(-) create mode 100644 frontend/src/components/compilerSkips.testkit.ts diff --git a/frontend/eslint.config.js b/frontend/eslint.config.js index cf5cc369..4fd99ee6 100644 --- a/frontend/eslint.config.js +++ b/frontend/eslint.config.js @@ -569,15 +569,15 @@ export default tseslint.config( // bulk `--suppress-all` would hide the next violation instead of // answering it, and it cannot hold a warning-level rule anyway. // - // `refs` is off because it fires on this app's popover architecture, not on its history, so - // suppressing it would not help the next popover anyone writes. Measured over the 217: - // 79 are `pop.refs.setReference` / `getReferenceProps` — @floating-ui/react's callback-ref - // setters, which are the documented way to use it — and 74 more are passing the popover handle - // down as a prop (`popover={pop}`, `ref={fade.ref}`), which is what `components/ui`'s popover - // engine is FOR. That is 153 of 217 flagged for having the word `refs` in an object that is not - // a React ref. The remaining ~64 are genuine render-phase `someRef.current = x` writes and are - // real (E27 S2 answered the ones the other rules could see, at their sites). - // Re-evaluate when the rule can: it shipped in 1.0.0 and this looks like over-approximation. + // `refs` is OFF — the one rule here switched off while it has real hits, and recorded as such + // in docs/gates.md. E27 turned it off calling 153 of its 217 hits false positives (the popover + // engine's `pop.refs.setReference` / `getReferenceProps()`, a popover handle passed as a prop) + // and the other ~64 real render-phase `someRef.current = x` writes. The E32 audit showed the + // triage was wrong: the rule and the React Compiler share one analysis, so every one of those + // "false positives" is a component the compiler SKIPS — 21 popover components among them, and + // Channels' 0.9 s row menu was the bill. It stays off only because 245 hits (measured E32 S1) cannot be fixed in + // one sprint; until they are, compilerCoverage.contract.test.ts pins every skipped function + // (none may be added), and E32 S3+ burns them down to turn this back on. "react-hooks/refs": "off", "local/jsx-comment-text": "error", "local/focus-safe-disabled": "error", diff --git a/frontend/reactPlugin.ts b/frontend/reactPlugin.ts index ce3ddc66..8a1f2843 100644 --- a/frontend/reactPlugin.ts +++ b/frontend/reactPlugin.ts @@ -11,8 +11,14 @@ import reactCompiler from "babel-plugin-react-compiler"; // // The React Compiler auto-memoises: it infers what `useCallback`/`useMemo` were being hand-written // for and applies the equivalent itself. It is enabled repo-wide with no opt-out list because it -// bails out on its own for anything it cannot prove safe (its rules are also linted, via -// eslint-plugin-react-hooks). `target` follows the installed React major and must move with it. +// bails out on its own for anything it cannot prove safe — SILENTLY: `panicThreshold` is "none" and +// there is no logger here, so a skipped component still works and simply re-renders everything under +// it. This comment used to say "its rules are also linted"; it was false while `react-hooks/refs` was +// off, and 95 functions sat uncompiled behind it (E32). What actually holds the line now is +// src/components/compilerCoverage.contract.test.ts, which runs THIS compiler over the shipped source +// and fails on any skip not on its list; the react-hooks lint rules are the early warning, with the +// one still switched off recorded in docs/gates.md. `target` follows the installed React major and +// must move with it. // // The plugin is IMPORTED rather than named by the usual `"babel-plugin-react-compiler"` string. Two // reasons, both about the gate: a string is invisible to knip, which then reports the dependency as diff --git a/frontend/src/components/compilerCoverage.contract.test.ts b/frontend/src/components/compilerCoverage.contract.test.ts index 2cd61f43..3950bdbe 100644 --- a/frontend/src/components/compilerCoverage.contract.test.ts +++ b/frontend/src/components/compilerCoverage.contract.test.ts @@ -1,66 +1,142 @@ -import { readFileSync } from "node:fs"; -import { transformSync } from "@babel/core"; -import reactCompiler from "babel-plugin-react-compiler"; import { describe, expect, it } from "vitest"; +import { compileReport, shippedSources, type CompileReport } from "./compilerSkips.testkit"; // The React Compiler bails out of a component it cannot prove safe WITHOUT a word: the component -// still works, it just re-renders everything under it on every render. Its main tripwire, a ref read -// or written during render, is not linted here (`react-hooks/refs` is off, see eslint.config.js), so -// nothing caught it. The channel manager is where it hurt (the owner, 2026-10-03): Channels and -// DataTable both bailed out, every open, close and reposition of a row's right-click menu rebuilt -// the rows and columns, and on "All" rows the menu took ~0.9 s to open and ~0.6 s to close. +// still works, it just re-renders everything under it on every render. The channel manager is where +// it hurt (the owner, 2026-10-03): Channels and DataTable both bailed out, every open, close and +// reposition of a row's right-click menu rebuilt the rows and columns, and on "All" rows the menu +// took ~0.9 s to open and ~0.6 s to close. The E32 audit then found 95 such functions across src. // -// This runs the real compiler (the options reactPlugin.ts gives it) over the files that must stay -// compiled and fails on any function it skips, with the compiler's own reason. -const MUST_COMPILE = ["components/Channels.tsx", "components/ui/DataTable.tsx"]; +// A RATCHET over everything the build compiles (E32 S1). KNOWN_SKIPS is the exact set the compiler +// skips today, `file → function → the compiler's category`; everything else must compile. Both +// directions are red: +// • a NEW skip (or a known one changing category) — fix the function; if it truly cannot be +// compiled, adding it here is a decision for the owner, with the reason in docs/gates.md; +// • a skip that is GONE — delete its entry, so the list only ever shrinks. +// E32 S3+ burns it down: the popover engine (Refs: `getReferenceProps()` / `refs.setReference` +// passed during render), render-phase ref writes, try/finally (Todo), the two exhaustive-deps +// suppressions. The end state is IncompatibleLibrary alone (VirtualGrid on TanStack Virtual). +const KNOWN_SKIPS: CompileReport["skipped"] = { + "components/AboutCell.tsx": { AboutCell: "Refs" }, + "components/ActiveFilterChips.tsx": { OverflowPopover: "Refs" }, + "components/AddToPlaylist.tsx": { AddToPlaylist: "Refs", MembershipPanel: "Todo" }, + "components/BootGate.tsx": { BootGate: "Refs" }, + "components/ChannelStatsPopover.tsx": { ChannelStatsPopover: "Refs" }, + "components/ChannelTags.tsx": { + ChannelTagsControl: "Refs", + SuggestedChipsLane: "Refs", + TagEditorMenu: "Refs", + }, + "components/ChannelsProvider.tsx": { ChannelsProvider: "Refs" }, + "components/ChatThread.tsx": { ChatThread: "Refs" }, + "components/ConfigPanel.tsx": { ConfigPanel: "Refs" }, + "components/DateFilterSelect.tsx": { DateFilterSelect: "Refs" }, + "components/DownloadCenter.tsx": { DownloadCenter: "Todo" }, + "components/DownloadDialog.tsx": { DownloadDialog: "Todo" }, + "components/Feed.tsx": { Feed: "Immutability" }, + "components/IssueComposer.tsx": { Attachments: "Immutability", ComposerBody: "Refs" }, + "components/KeyGate.tsx": { KeyGate: "Todo" }, + "components/LanguageSwitcher.tsx": { LanguageSwitcher: "Refs" }, + "components/Messages.tsx": { ConversationList: "Refs" }, + "components/NavSidebar.tsx": { NavSidebar: "Refs" }, + "components/NavigationProvider.tsx": { NavigationProvider: "Refs" }, + "components/OnboardingWizard.tsx": { OnboardingWizard: "Refs" }, + "components/PersonalViewsModal.tsx": { PersonalViewsModal: "Refs" }, + "components/PlayerModal.tsx": { PlayerModal: "Todo" }, + "components/PlaylistLimitsHint.tsx": { PlaylistLimitsHint: "Refs" }, + "components/Playlists.tsx": { Playlists: "Immutability" }, + "components/PlaylistsRail.tsx": { PlaylistsRail: "Todo" }, + "components/PlexBrowse.tsx": { PlexBrowse: "Immutability" }, + "components/PlexCollectionEditor.tsx": { PlexCollectionEditor: "Todo" }, + "components/PlexCollectionView.tsx": { PlexCollectionView: "Todo" }, + "components/PlexPlayer.tsx": { PlexPlayer: "Suppression" }, + "components/PlexPlaylistAdd.tsx": { PlexPlaylistAdd: "Todo" }, + "components/PlexPlaylistView.tsx": { PlexPlaylistView: "Refs" }, + "components/PlexStatsOverlay.tsx": { PlexStatsOverlay: "Todo" }, + "components/SavedViewsPopover.tsx": { SavedViewsPopover: "Refs" }, + "components/SavedViewsProvider.tsx": { SavedViewsProvider: "Todo" }, + "components/SettingsPanel.tsx": { SignInMethods: "Todo" }, + "components/SetupWizard.tsx": { SetupWizard: "Todo" }, + "components/SyncStatus.tsx": { SyncStatus: "Refs" }, + "components/TagFilterChips.tsx": { TagFilterChips: "Refs" }, + "components/TagManager.tsx": { TagManager: "Refs", TagRow: "Refs" }, + "components/UpNextCard.tsx": { UpNextCard: "Refs" }, + "components/UploadDialog.tsx": { UploadDialog: "Todo" }, + "components/VideoEditor.tsx": { VideoEditor: "Refs" }, + "components/ViewSwitcher.tsx": { ViewSwitcher: "Refs" }, + "components/WatchPage.tsx": { WatchPage: "Todo", useMediaSession: "Todo" }, + "components/Welcome.tsx": { AuthCard: "Todo", Lightbox: "Refs" }, + "components/YearFilter.tsx": { YearFilter: "Refs" }, + "components/plexViews.tsx": { PlexSeasonView: "Todo", PlexShowView: "Todo" }, + "components/ui/ActionMenu.tsx": { ActionMenu: "Refs" }, + "components/ui/AnchoredPopover.tsx": { PopoverSurfaceInner: "Refs" }, + "components/ui/Combobox.tsx": { Combobox: "Refs" }, + "components/ui/FilterSelect.tsx": { FilterSelect: "Refs" }, + "components/ui/FocusSafeButton.tsx": { FocusSafeButton: "Refs" }, + "components/ui/Modal.tsx": { Modal: "Refs" }, + "components/ui/NavRail.tsx": { NavRailGroup: "Refs" }, + "components/ui/PageScroller.tsx": { PageScroller: "Refs" }, + "components/ui/PriorityStepper.tsx": { PriorityStepper: "Refs" }, + "components/ui/SidePanel.tsx": { SidePanel: "Refs" }, + "components/ui/SortControl.tsx": { SortControl: "Refs" }, + "components/ui/Toaster.tsx": { Toaster: "Globals" }, + "components/ui/Tooltip.tsx": { Tooltip: "Refs" }, + "components/ui/VirtualGrid.tsx": { VirtualGrid: "IncompatibleLibrary" }, + "components/ui/anchoredPopoverHooks.ts": { useAnchoredPopover: "Refs" }, + "components/ui/sortableA11y.ts": { useSortableA11y: "Immutability" }, + "components/ui/tableHeader.tsx": { FilterFunnel: "Refs", FilterOptionList: "Refs" }, + "components/useDebugRecording.ts": { useDebugRecording: "Todo" }, + "components/useDownloadActions.tsx": { useDownloadActions: "Todo" }, + "components/usePlaylistActions.tsx": { usePlaylistActions: "Todo" }, + "components/usePlexPlaylistEdits.ts": { usePlexPlaylistEdits: "Todo" }, + "lib/history.ts": { useBackToClose: "Refs", useBackToExit: "Refs", useHistorySubview: "Refs" }, + "lib/keymap.ts": { useGlobalKeymap: "Refs" }, + "lib/layerStack.ts": { useLayer: "Refs" }, + "lib/listNav.ts": { useListNav: "Refs" }, + "lib/playerModalHooks.ts": { useYtBigView: "Refs", useYtProgress: "Suppression" }, + "lib/plexPlayerHooks.ts": { useAutoSkip: "Refs", usePlexTracks: "Todo" }, + "lib/useChannelPriority.ts": { useChannelPriority: "Refs" }, + "lib/useChipOverflowFit.ts": { useChipOverflowFit: "Refs" }, + "lib/useDismiss.ts": { useDismiss: "Refs" }, + "lib/usePref.ts": { usePref: "Refs" }, + "lib/useUndoable.ts": { useUndoable: "Refs" }, + "lib/useUrlMirror.ts": { useUrlMirror: "Refs" }, +}; -function bailouts(path: string): string[] { - const source = readFileSync(new URL(`../${path}`, import.meta.url), "utf8"); - const found: string[] = []; - transformSync(source, { - filename: path, - babelrc: false, - configFile: false, - parserOpts: { plugins: ["jsx", "typescript"] }, - plugins: [ - [ - reactCompiler, - { - target: "19", - logger: { - logEvent: ( - _file: string | null, - e: { kind: string; fnLoc?: unknown; detail?: unknown }, - ) => { - if ( - e.kind !== "CompileError" && - e.kind !== "CompileSkip" && - e.kind !== "PipelineError" - ) - return; - const loc = e.fnLoc as { start?: { line?: number } } | undefined; - const d = e.detail as - | { options?: { reason?: string; details?: { message?: string }[] } } - | { reason?: string; details?: { message?: string }[] } - | undefined; - const o = - d && "options" in d - ? d.options - : (d as { reason?: string; details?: { message?: string }[] }); - found.push( - `line ${loc?.start?.line}: ${o?.details?.[0]?.message ?? o?.reason ?? e.kind}`, - ); - }, - }, - }, - ], - ], - }); - return found; -} +// The files a regression hurt most: compiled, and they must stay so (a positive check too, so a +// harness that compiled nothing could not pass as "no skips"). +const MUST_COMPILE: Record = { + "components/Channels.tsx": ["Channels"], + "components/ui/DataTable.tsx": ["DataTable"], +}; + +type Row = [file: string, fn: string, category: string]; +const rows = (skips: CompileReport["skipped"]): Row[] => + Object.entries(skips).flatMap(([file, fns]) => + Object.entries(fns).map(([fn, category]): Row => [file, fn, category]), + ); describe("the React Compiler", () => { - it.each(MUST_COMPILE)("compiles every function in %s", (path) => { - expect(bailouts(path)).toEqual([]); + // ~8 s for the whole tree, once, shared by the checks below. + const report = compileReport(shippedSources()); + const actual = rows(report.skipped); + const known = rows(KNOWN_SKIPS); + const key = ([f, fn]: Row) => `${f} → ${fn}`; + + it("skips nothing new (fix it, or the owner decides to list it)", () => { + const knownAs = new Map(known.map((r) => [key(r), r[2]])); + expect( + actual.filter((r) => knownAs.get(key(r)) !== r[2]).map((r) => `${key(r)}: ${r[2]}`), + ).toEqual([]); + }); + + it("lists no skip that has gone (delete it — the list only shrinks)", () => { + const actualAs = new Map(actual.map((r) => [key(r), r[2]])); + expect(known.filter((r) => !actualAs.has(key(r))).map(key)).toEqual([]); + }); + + it.each(Object.entries(MUST_COMPILE))("compiles %s", (file, fns) => { + expect(report.skipped[file] ?? {}).toEqual({}); + for (const fn of fns) expect(report.compiled[file]).toContain(fn); }); }); diff --git a/frontend/src/components/compilerSkips.testkit.ts b/frontend/src/components/compilerSkips.testkit.ts new file mode 100644 index 00000000..d42731e3 --- /dev/null +++ b/frontend/src/components/compilerSkips.testkit.ts @@ -0,0 +1,124 @@ +import { readdirSync, readFileSync } from "node:fs"; +import { parseSync, transformSync, type types as t } from "@babel/core"; +import reactCompiler from "babel-plugin-react-compiler"; + +// Runs the real React Compiler — the options reactPlugin.ts gives it — over the shipped source and +// reports every function it SKIPS, keyed `file → function → category`. The compiler skips without a +// word (`panicThreshold` is "none" and the build has no logger), so this is the only place a skip is +// visible at all. Used by compilerCoverage.contract.test.ts. + +const SRC = new URL("../", import.meta.url); + +/** Every file the production build compiles: src, minus tests and test kits. */ +export function shippedSources(): string[] { + return (readdirSync(SRC, { recursive: true }) as string[]) + .map((p) => p.split("\\").join("/")) + .filter( + (p) => /\.(ts|tsx)$/.test(p) && !/\.(test|testkit)\.tsx?$/.test(p) && !p.endsWith(".d.ts"), + ) + .sort(); +} + +type Node = t.Node & { [k: string]: unknown }; +const isFn = (n: Node) => + n.type === "FunctionDeclaration" || + n.type === "FunctionExpression" || + n.type === "ArrowFunctionExpression"; + +/** The name a reader would look a function up by: its own, the variable it is assigned to (through + * a `memo(…)` / `forwardRef(…)` wrapper), or `default` for an anonymous default export. */ +function functionNames(ast: t.File): Map { + const names = new Map(); + const walk = (node: Node, parents: Node[]) => { + if (isFn(node) && node.loc) { + let name = (node as t.FunctionDeclaration).id?.name ?? null; + for (let i = parents.length - 1; !name && i >= 0; i--) { + const p = parents[i]!; + if (p.type === "VariableDeclarator" && p.id.type === "Identifier") name = p.id.name; + else if (p.type === "ExportDefaultDeclaration") name = "default"; + else if (p.type !== "CallExpression" && p.type !== "TSAsExpression") break; + } + names.set( + `${node.loc.start.line}:${node.loc.start.column}`, + name ?? ``, + ); + } + for (const key of Object.keys(node)) { + if (key === "loc" || key === "leadingComments" || key === "trailingComments") continue; + const v = node[key]; + if (Array.isArray(v)) { + for (const c of v) + if (c && typeof c === "object" && "type" in c) walk(c as Node, [...parents, node]); + } else if (v && typeof v === "object" && "type" in v) walk(v as Node, [...parents, node]); + } + }; + walk(ast as unknown as Node, []); + return names; +} + +type Detail = { + category?: string; + reason?: string; + options?: { category?: string; reason?: string }; +}; + +export interface CompileReport { + /** file → function → the compiler's category for the skip (Refs, Todo, Immutability, …). */ + skipped: Record>; + /** file → the functions it did compile. */ + compiled: Record; +} + +export function compileReport(files: string[]): CompileReport { + const skipped: CompileReport["skipped"] = {}; + const compiled: CompileReport["compiled"] = {}; + for (const file of files) { + const source = readFileSync(new URL(file, SRC), "utf8"); + const opts = { filename: file, babelrc: false, configFile: false } as const; + const ast = parseSync(source, { ...opts, parserOpts: { plugins: ["jsx", "typescript"] } }); + if (!ast) throw new Error(`could not parse ${file}`); + const names = functionNames(ast); + const nameAt = (loc: t.SourceLocation | null) => + loc + ? (names.get(`${loc.start.line}:${loc.start.column}`) ?? ``) + : ""; + transformSync(source, { + ...opts, + parserOpts: { plugins: ["jsx", "typescript"] }, + plugins: [ + [ + reactCompiler, + { + target: "19", + logger: { + logEvent: ( + _f: string | null, + e: { + kind: string; + fnLoc?: t.SourceLocation | null; + detail?: Detail; + reason?: string; + }, + ) => { + if (e.kind === "CompileSuccess") { + (compiled[file] ??= []).push(nameAt(e.fnLoc ?? null)); + return; + } + let category: string; + if (e.kind === "CompileError") + category = e.detail?.options?.category ?? e.detail?.category ?? "Unknown"; + else if (e.kind === "CompileSkip") category = `Skip: ${e.reason}`; + else if (e.kind === "PipelineError") category = "PipelineError"; + else return; + const fns = (skipped[file] ??= {}); + // The first reason is the one that stopped it; later ones are the same function. + fns[nameAt(e.fnLoc ?? null)] ??= category; + }, + }, + }, + ], + ], + }); + } + return { skipped, compiled }; +}