From 830f1b677e07e50d820976f8cbfac716acaa2ca3 Mon Sep 17 00:00:00 2001 From: Miguel Angel Simon Sierra Date: Wed, 2 Sep 2026 19:00:19 -0400 Subject: [PATCH 1/4] ci(studio): gate new useEffect call sites behind a per-file ratchet The ban on `useEffect` and `useLayoutEffect` in studio was prose only, so nothing failed when it was broken. `check:no-use-effect` counts call sites on the TypeScript AST and compares each file against a budget seeded from what exists today: a new file with an effect fails, a budgeted file that grows fails, and a budgeted file that shrinks fails until its number comes down. Aliased (`import { useEffect as x }`) and namespaced (`React.useEffect`) calls resolve to the same hook and count; `useMountEffect.ts` is the one sanctioned caller and is excluded. No existing call site is changed. --- CLAUDE.md | 10 + package.json | 3 +- scripts/check-no-use-effect.mjs | 340 ++++++++++++++++++++++++++++++++ 3 files changed, 352 insertions(+), 1 deletion(-) create mode 100644 scripts/check-no-use-effect.mjs diff --git a/CLAUDE.md b/CLAUDE.md index 35fe2ee673..9a1968bae7 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -87,6 +87,16 @@ npx hyperframes check # Browser gate (headless Chrome — runtime errors, l Both must pass before previewing or considering work complete. +## React Rules + +Studio code does not call `useEffect` or `useLayoutEffect`. Derive state during render, do work in +event handlers, fetch with a data library, and reset a subtree with a `key` instead of choreographing +dependency arrays. For the rare one-time sync with an external system, use `useMountEffect()` from +`packages/studio/src/hooks/useMountEffect.ts` — the one sanctioned wrapper. `bun run check:no-use-effect` +(part of `bun run lint`) enforces this with a per-file budget seeded from the call sites that already +existed: a new file with an effect fails, a budgeted file that grows fails, and a budgeted file that +shrinks fails until its number is lowered, so the list can only ever be paid down. + ## Project Structure ``` diff --git a/package.json b/package.json index 25866d6189..6e96537e1a 100644 --- a/package.json +++ b/package.json @@ -26,7 +26,7 @@ "sync-schemas:check": "tsx scripts/sync-schemas.ts --check", "sync:package-subpaths": "node scripts/package-subpaths.mjs --write", "check:package-subpaths": "node scripts/package-subpaths.mjs", - "lint": "bun run check:docs-snippet-motion && bun run check:tracked-artifacts && bun run check:workspace-contracts && bun run check:gcp-cloud-run-dockerfile && bun run check:package-cycles && bun run check:package-subpaths && bun run check:cli-process-ownership && oxlint . && tsx scripts/lint-skills.ts && node scripts/check-skill-mirror.mjs", + "lint": "bun run check:docs-snippet-motion && bun run check:tracked-artifacts && bun run check:workspace-contracts && bun run check:gcp-cloud-run-dockerfile && bun run check:package-cycles && bun run check:package-subpaths && bun run check:cli-process-ownership && bun run check:no-use-effect && oxlint . && tsx scripts/lint-skills.ts && node scripts/check-skill-mirror.mjs", "check:gcp-cloud-run-dockerfile": "bun run --cwd packages/gcp-cloud-run test:dockerfile-workspaces", "lint:skills": "tsx scripts/lint-skills.ts", "check:skill-mirror": "node scripts/check-skill-mirror.mjs", @@ -35,6 +35,7 @@ "check:docs-snippet-motion": "node scripts/check-docs-snippet-motion.mjs", "check:workspace-contracts": "node scripts/check-workspace-contracts.mjs", "check:package-cycles": "node scripts/check-package-cycles.mjs", + "check:no-use-effect": "node scripts/check-no-use-effect.mjs", "check:cli-process-ownership": "node scripts/check-cli-process-ownership.mjs", "format": "oxfmt .", "test": "bun run test:unit", diff --git a/scripts/check-no-use-effect.mjs b/scripts/check-no-use-effect.mjs new file mode 100644 index 0000000000..197894af7b --- /dev/null +++ b/scripts/check-no-use-effect.mjs @@ -0,0 +1,340 @@ +#!/usr/bin/env node + +// Studio bans bare `useEffect` (root `CLAUDE.md`, "React Rules"), and `useLayoutEffect` with it: +// same coupling hidden in a dependency array, same loops and races, and it blocks paint as well. +// Prose alone never failed a build, so the call sites kept accumulating while the rule read as +// absolute. This gate makes the rule mechanical without demanding a repo-wide refactor first. +// +// oxlint has no `useEffect` ban, and the two shapes that could stand in for one both fail the +// requirement that matters here: +// +// - A rule scoped to skip the directories holding the violations is a silent allowance. Nobody +// can count what is exempt, and the exemption never shrinks. +// - Per-file `oxlint-disable` comments are visible but do not ratchet: a file that is already +// suppressed can grow from 4 effects to 40 and stay green. +// +// So the exemption is a COUNT per file, listed in BUDGET below. A new file with an effect fails. +// A listed file that grows fails. A listed file that shrinks ALSO fails, asking for the number to +// come down -- that is what makes the list a debt register that can only be paid off, and what +// lets a reader watch the total move. When every count reaches zero, delete BUDGET and this +// script becomes a flat ban. +// +// Counting runs on the TypeScript AST rather than grep, because docblocks quote the banned +// pattern while explaining it and prose about a rule must not be counted as breaking it. Aliasing +// is resolved rather than assumed away: `import { useEffect as x }`, `React.useEffect` and a +// namespace import all resolve to the same call and all count. Both banned hooks share one +// budget, so neither can be smuggled in by spelling it the other way. + +import ts from "typescript"; +import { readdirSync, readFileSync } from "node:fs"; +import { join, resolve } from "node:path"; +import { fileURLToPath } from "node:url"; + +const ROOT = resolve(import.meta.dirname, ".."); +const SCANNED = "packages/studio/src"; + +/** + * The banned hooks. `useLayoutEffect` carries the same hazard as `useEffect` and blocks paint on + * top, so it is banned on the same terms. Swapping one for the other is not new debt, so the + * register tracks their total per file rather than one number each. + */ +const BANNED = new Set(["useEffect", "useLayoutEffect"]); + +/** + * The sanctioned escape hatch: the one file allowed to call `useEffect`, because it is what every + * other call site is meant to use instead. Not in BUDGET, because BUDGET must be able to reach + * zero and this entry never will. + */ +const SANCTIONED = new Map([ + [ + "packages/studio/src/hooks/useMountEffect.ts", + "defines useMountEffect(), the sanctioned wrapper the ban points at", + ], +]); + +/** + * Pre-existing call sites, by file, counted at the commit that added this gate. These are debt, + * not permission. Lower a number when you remove an effect, delete the entry when it hits zero, + * and never raise one. + */ +const BUDGET = new Map([ + ["packages/studio/src/App.tsx", 1], + ["packages/studio/src/captions/components/shared.tsx", 1], + ["packages/studio/src/components/TimelineToolbar.tsx", 1], + ["packages/studio/src/components/editor/AnimationCard.tsx", 2], + ["packages/studio/src/components/editor/BlockParamsPanel.tsx", 1], + ["packages/studio/src/components/editor/DomEditCropHandles.tsx", 1], + ["packages/studio/src/components/editor/DomEditOverlay.tsx", 1], + ["packages/studio/src/components/editor/EaseCurveSection.tsx", 3], + ["packages/studio/src/components/editor/EaseParamFields.tsx", 2], + ["packages/studio/src/components/editor/FileTreeNodes.tsx", 3], + ["packages/studio/src/components/editor/InlineTextToolbar.tsx", 1], + ["packages/studio/src/components/editor/LayersPanel.tsx", 4], + ["packages/studio/src/components/editor/MotionPathOverlay.tsx", 4], + ["packages/studio/src/components/editor/PromotableControl.tsx", 1], + ["packages/studio/src/components/editor/PropertyPanelFlat.tsx", 1], + ["packages/studio/src/components/editor/SnapToolbar.tsx", 2], + ["packages/studio/src/components/editor/SourceEditor.tsx", 2], + ["packages/studio/src/components/editor/TimelineFxPopover.tsx", 1], + ["packages/studio/src/components/editor/TopologyLens.tsx", 5], + ["packages/studio/src/components/editor/Transform3DCube.tsx", 1], + ["packages/studio/src/components/editor/propertyPanelColor.tsx", 3], + ["packages/studio/src/components/editor/propertyPanelColorGradingSlider.tsx", 1], + ["packages/studio/src/components/editor/propertyPanelColorScopes.tsx", 1], + ["packages/studio/src/components/editor/propertyPanelColorSecondary.tsx", 1], + ["packages/studio/src/components/editor/propertyPanelCommitField.tsx", 2], + ["packages/studio/src/components/editor/propertyPanelFlatColorGradingAccessory.tsx", 1], + ["packages/studio/src/components/editor/propertyPanelFlatColorGradingSection.tsx", 2], + ["packages/studio/src/components/editor/propertyPanelFlatEffectsSection.tsx", 2], + ["packages/studio/src/components/editor/propertyPanelFlatMediaSection.tsx", 1], + ["packages/studio/src/components/editor/propertyPanelFlatPrimitives.tsx", 2], + ["packages/studio/src/components/editor/propertyPanelFlatStyleSections.tsx", 1], + ["packages/studio/src/components/editor/propertyPanelFlatTextSection.tsx", 1], + ["packages/studio/src/components/editor/propertyPanelFont.tsx", 5], + ["packages/studio/src/components/editor/propertyPanelFxControls.tsx", 2], + ["packages/studio/src/components/editor/propertyPanelFxEqModule.tsx", 1], + ["packages/studio/src/components/editor/propertyPanelFxSection.tsx", 1], + ["packages/studio/src/components/editor/propertyPanelGradingNumberField.tsx", 1], + ["packages/studio/src/components/editor/propertyPanelMediaSection.tsx", 1], + ["packages/studio/src/components/editor/propertyPanelPrimitives.tsx", 2], + ["packages/studio/src/components/editor/propertyPanelSections.tsx", 4], + ["packages/studio/src/components/editor/propertyPanelStyleSections.tsx", 2], + ["packages/studio/src/components/editor/useAudioFxRevealSection.ts", 1], + ["packages/studio/src/components/editor/useCanvasContextMenuState.ts", 1], + ["packages/studio/src/components/editor/useColorGradingController.ts", 7], + ["packages/studio/src/components/editor/useColorGradingPreviews.ts", 2], + ["packages/studio/src/components/editor/useColorGradingScopes.ts", 1], + ["packages/studio/src/components/editor/useDomEditNudge.ts", 2], + ["packages/studio/src/components/editor/useFxAudition.ts", 1], + ["packages/studio/src/components/editor/useFxCarve.ts", 3], + ["packages/studio/src/components/editor/useFxChainObserved.ts", 1], + ["packages/studio/src/components/editor/useInspectorGestureTransaction.ts", 2], + ["packages/studio/src/components/editor/useLayerRevealOverride.ts", 2], + ["packages/studio/src/components/editor/useMotionPathData.ts", 2], + ["packages/studio/src/components/editor/useZOrderCrossedFlash.tsx", 1], + ["packages/studio/src/components/feedback/CrashFeedbackPrompt.tsx", 1], + ["packages/studio/src/components/feedback/StudioFeedbackCard.tsx", 3], + ["packages/studio/src/components/nle/AssetPreviewOverlay.tsx", 2], + ["packages/studio/src/components/nle/NLEContext.tsx", 7], + ["packages/studio/src/components/nle/NLEPreview.test.ts", 1], + ["packages/studio/src/components/nle/NLEPreview.tsx", 6], + ["packages/studio/src/components/nle/useCompositionStack.ts", 1], + ["packages/studio/src/components/panels/SlideshowPanel.tsx", 2], + ["packages/studio/src/components/panels/VariablesPanel.tsx", 1], + ["packages/studio/src/components/renders/FfmpegRequiredNotice.tsx", 3], + ["packages/studio/src/components/renders/RenderQueue.tsx", 4], + ["packages/studio/src/components/renders/useFfmpegStatus.ts", 2], + ["packages/studio/src/components/renders/useRenderQueue.ts", 3], + ["packages/studio/src/components/sidebar/AssetCard.tsx", 1], + ["packages/studio/src/components/sidebar/AssetContextMenu.tsx", 3], + ["packages/studio/src/components/sidebar/AssetsTab.tsx", 1], + ["packages/studio/src/components/sidebar/AudioRow.tsx", 2], + ["packages/studio/src/components/sidebar/BlocksTab.tsx", 1], + ["packages/studio/src/components/sidebar/CompositionsTab.tsx", 2], + ["packages/studio/src/components/sidebar/GlobalAssetsView.tsx", 1], + ["packages/studio/src/components/sidebar/PromptPreviewModal.tsx", 1], + ["packages/studio/src/components/storyboard/AgentChatMessageButton.tsx", 1], + ["packages/studio/src/components/storyboard/FramePoster.tsx", 1], + ["packages/studio/src/components/storyboard/StoryboardFrameFocus.tsx", 3], + ["packages/studio/src/components/storyboard/StoryboardLoaded.tsx", 2], + ["packages/studio/src/components/storyboard/StoryboardSourceEditor.tsx", 4], + ["packages/studio/src/components/storyboard/useFrameComments.ts", 1], + ["packages/studio/src/components/ui/Tooltip.tsx", 2], + ["packages/studio/src/components/ui/VideoFrameThumbnail.tsx", 1], + ["packages/studio/src/components/ui/useDialogBehavior.ts", 1], + ["packages/studio/src/contexts/VariablePromoteContext.tsx", 1], + ["packages/studio/src/contexts/ViewModeContext.tsx", 1], + ["packages/studio/src/hooks/useAppHotkeys.ts", 2], + ["packages/studio/src/hooks/useAskAgentModal.ts", 2], + ["packages/studio/src/hooks/useAutomationSelectionKeyboard.ts", 1], + ["packages/studio/src/hooks/useBlockCatalog.ts", 1], + ["packages/studio/src/hooks/useCaptionDetection.ts", 3], + ["packages/studio/src/hooks/useConsoleErrorCapture.ts", 1], + ["packages/studio/src/hooks/useContextMenuDismiss.ts", 1], + ["packages/studio/src/hooks/useDomEditPreviewSync.ts", 2], + ["packages/studio/src/hooks/useDomEditWiring.ts", 2], + ["packages/studio/src/hooks/useDomSelection.ts", 5], + ["packages/studio/src/hooks/useExternalFileChangeCoordinator.ts", 4], + ["packages/studio/src/hooks/useFileTree.ts", 1], + ["packages/studio/src/hooks/useGestureCommit.ts", 1], + ["packages/studio/src/hooks/useGestureRecording.ts", 1], + ["packages/studio/src/hooks/useGsapPropertyDebounce.ts", 1], + ["packages/studio/src/hooks/useGsapTweenCache.ts", 4], + ["packages/studio/src/hooks/useHydrateActiveCompPathFromUrl.ts", 1], + ["packages/studio/src/hooks/useInlineTextEdit.ts", 2], + ["packages/studio/src/hooks/useKeyframeKeyboard.ts", 1], + ["packages/studio/src/hooks/useLintModal.ts", 3], + ["packages/studio/src/hooks/useLivePlayheadTime.ts", 1], + ["packages/studio/src/hooks/useMusicBeatAnalysis.ts", 2], + ["packages/studio/src/hooks/usePanelLayout.ts", 1], + ["packages/studio/src/hooks/usePersistentEditHistory.ts", 1], + ["packages/studio/src/hooks/usePreviewDocumentVersion.ts", 1], + ["packages/studio/src/hooks/useProjectCompositionVariables.ts", 1], + ["packages/studio/src/hooks/useProjectSignaturePoll.ts", 1], + ["packages/studio/src/hooks/useRemoveBackground.ts", 1], + ["packages/studio/src/hooks/useSdkSelectionSync.ts", 1], + ["packages/studio/src/hooks/useSdkSession.ts", 2], + ["packages/studio/src/hooks/useServerConnection.ts", 1], + ["packages/studio/src/hooks/useSlideshowTabState.ts", 1], + ["packages/studio/src/hooks/useStoryboard.ts", 1], + ["packages/studio/src/hooks/useStudioSdkSessions.ts", 1], + ["packages/studio/src/hooks/useStudioSelectionPublisher.ts", 4], + ["packages/studio/src/hooks/useStudioSessionStart.ts", 1], + ["packages/studio/src/hooks/useStudioTestHooks.ts", 1], + ["packages/studio/src/hooks/useStudioUrlState.ts", 6], + ["packages/studio/src/hooks/useThumbnailLease.ts", 1], + ["packages/studio/src/hooks/useTimelineSelectionPreviewSync.ts", 1], + ["packages/studio/src/player/components/Player.tsx", 4], + ["packages/studio/src/player/components/PlayerControls.tsx", 2], + ["packages/studio/src/player/components/ShortcutsPanel.tsx", 1], + ["packages/studio/src/player/components/TimelineAutomationLane.tsx", 2], + ["packages/studio/src/player/components/TimelineAutomationLaneSlot.tsx", 1], + ["packages/studio/src/player/components/TimelineClipDiamonds.tsx", 1], + ["packages/studio/src/player/components/TimelineFxButton.tsx", 1], + ["packages/studio/src/player/components/TimelineOverlays.tsx", 2], + ["packages/studio/src/player/components/menuKeyboardNav.ts", 1], + ["packages/studio/src/player/components/timelineDragDrop.ts", 2], + ["packages/studio/src/player/components/useAutoExpandKeyframedClips.ts", 1], + ["packages/studio/src/player/components/useTimelineActiveClips.ts", 1], + ["packages/studio/src/player/components/useTimelineClipDrag.ts", 1], + ["packages/studio/src/player/components/useTimelineFocusCoordinator.ts", 2], + ["packages/studio/src/player/components/useTimelineGeometry.ts", 2], + ["packages/studio/src/player/components/useTimelinePlayhead.ts", 5], + ["packages/studio/src/player/components/useTimelineRangeSelection.ts", 3], + ["packages/studio/src/player/components/useTimelineRowVirtualization.ts", 1], + ["packages/studio/src/player/components/useTimelineScrollViewport.ts", 1], + ["packages/studio/src/player/components/useTimelineSelectionLifecycle.ts", 1], + ["packages/studio/src/player/components/useTimelineVirtualRows.ts", 2], + ["packages/studio/src/player/components/useTrackGapMenu.ts", 1], + ["packages/studio/src/player/hooks/useExpandedTimelineElements.test.ts", 1], + ["packages/studio/src/player/hooks/usePlaybackKeyboard.test.ts", 1], + ["packages/studio/src/player/hooks/useTimelinePlayer.seek.test.ts", 1], + ["packages/studio/src/player/hooks/useTimelinePlayer.ts", 2], + ["packages/studio/src/webmcp/StudioAgentTools.tsx", 1], + ["packages/studio/src/webmcp/useStudioAgentTools.ts", 1], +]); + +function sources() { + const found = []; + const walk = (dir) => { + for (const entry of readdirSync(join(ROOT, dir), { withFileTypes: true })) { + const path = join(dir, entry.name); + if (entry.isDirectory()) walk(path); + else if (/\.tsx?$/.test(entry.name) && !entry.name.endsWith(".d.ts")) found.push(path); + } + }; + walk(SCANNED); + return found.sort(); +} + +/** Local names in one file that refer to a banned hook, plus the React namespace names. */ +function reactBindings(root) { + const direct = new Set(); + const namespaces = new Set(); + for (const statement of root.statements) { + if (!ts.isImportDeclaration(statement)) continue; + if (!ts.isStringLiteral(statement.moduleSpecifier)) continue; + if (statement.moduleSpecifier.text !== "react") continue; + const clause = statement.importClause; + if (!clause) continue; + if (clause.name) namespaces.add(clause.name.text); // import React from "react" + const named = clause.namedBindings; + if (named && ts.isNamespaceImport(named)) namespaces.add(named.name.text); + if (named && ts.isNamedImports(named)) { + for (const spec of named.elements) { + if (BANNED.has((spec.propertyName ?? spec.name).text)) direct.add(spec.name.text); + } + } + } + return { direct, namespaces }; +} + +/** Line numbers of every banned-hook call in one file. */ +function callSites(file, text = readFileSync(join(ROOT, file), "utf8")) { + const root = ts.createSourceFile( + file, + text, + ts.ScriptTarget.Latest, + true, + file.endsWith(".tsx") ? ts.ScriptKind.TSX : ts.ScriptKind.TS, + ); + const { direct, namespaces } = reactBindings(root); + const lines = []; + const visit = (node) => { + if (ts.isCallExpression(node)) { + const callee = node.expression; + const hit = ts.isIdentifier(callee) + ? direct.has(callee.text) + : ts.isPropertyAccessExpression(callee) && + ts.isIdentifier(callee.expression) && + namespaces.has(callee.expression.text) && + BANNED.has(callee.name.text); + if (hit) lines.push(root.getLineAndCharacterOfPosition(node.getStart(root)).line + 1); + } + ts.forEachChild(node, visit); + }; + visit(root); + return lines; +} + +/** Every offending file under SCANNED, mapped to the lines its banned hooks sit on. */ +function scan() { + const found = new Map(); + for (const file of sources()) { + if (SANCTIONED.has(file)) continue; + const lines = callSites(file); + if (lines.length > 0) found.set(file, lines); + } + return found; +} + +function listBudgetIssues(found, budget = BUDGET) { + const problems = []; + for (const [file, lines] of found) { + const allowed = budget.get(file); + if (allowed === undefined) { + problems.push( + `NEW banned effect hook: ${file}:${lines.join(", :")}\n` + + ` The ban is absolute. Use useMountEffect() for a one-time external sync, or derive\n` + + ` the value during render. See CLAUDE.md > React Rules.`, + ); + } else if (lines.length > allowed) { + problems.push( + `OVER BUDGET: ${file} has ${lines.length}, budget allows ${allowed}\n` + + ` Lines: ${lines.join(", ")}. The budget is debt, not headroom.`, + ); + } + } + for (const [file, allowed] of budget) { + const actual = found.get(file)?.length ?? 0; + if (actual >= allowed) continue; + problems.push( + `STALE budget entry: ${file} now has ${actual}, budget still says ${allowed}\n` + + ` ${actual === 0 ? "Delete the entry." : `Lower it to ${actual}.`} Thanks for paying the debt down.`, + ); + } + return problems; +} + +function main() { + const problems = listBudgetIssues(scan()); + for (const file of SANCTIONED.keys()) { + if (!sources().includes(file)) + problems.push(`SANCTIONED lists a file that no longer exists: ${file}`); + } + const debt = [...BUDGET.values()].reduce((total, count) => total + count, 0); + if (problems.length === 0) { + console.log( + `no-use-effect: no new useEffect or useLayoutEffect in ${SCANNED}. ` + + `${debt} budgeted call site(s) across ${BUDGET.size} file(s) remain.`, + ); + return; + } + problems.forEach((problem) => console.error(problem)); + console.error( + `\n${problems.length} problem(s). Budgeted debt is ${debt} across ${BUDGET.size} files.`, + ); + process.exitCode = 1; +} + +if (process.argv[1] === fileURLToPath(import.meta.url)) main(); From 0562d9258642e15d3e38527add8e0ba4d9b11664 Mon Sep 17 00:00:00 2001 From: Miguel Angel Simon Sierra Date: Wed, 2 Sep 2026 19:42:58 -0400 Subject: [PATCH 2/4] ci(studio): split the effect gate's checks into single-purpose predicates The Fallow audit flagged four functions in `check-no-use-effect.mjs` on CRAP score, which multiplies cyclomatic complexity by how much of the function no test reaches. Coverage is the half that cannot be paid here: fallow's static estimator gives every `.mjs` file in this repo a 0% estimate, tested or not, so a script written this way can only clear the threshold on complexity. So each function now makes one decision. Resolving a `react` import, reading a namespace name, following a `useEffect as x` alias and matching a callee are four separate named predicates instead of one nested condition, and the budget comparison splits into "what is wrong with this file" and "what is wrong with this entry", assembled by a flat map. No rule, message, budget number or exit code changes: the gate still fails a new call site, a file that grew, and an entry the file has since paid down. The helpers are exported and `check-no-use-effect.test.mjs` drives them the way the sibling check scripts are tested, so the split is verified rather than assumed, and `main()` now hoists the source walk out of its loop instead of rebuilding the file list per sanctioned entry. --- package.json | 2 +- scripts/check-no-use-effect.mjs | 168 ++++++++++++++++++--------- scripts/check-no-use-effect.test.mjs | 127 ++++++++++++++++++++ 3 files changed, 239 insertions(+), 58 deletions(-) create mode 100644 scripts/check-no-use-effect.test.mjs diff --git a/package.json b/package.json index 6e96537e1a..cf8c06dc32 100644 --- a/package.json +++ b/package.json @@ -49,7 +49,7 @@ "player:perf": "bun run --filter @hyperframes/player perf", "format:check": "oxfmt --check .", "knip": "knip", - "test:scripts": "node --import tsx --test scripts/animejs-v4-guidance.test.mjs scripts/check-tracked-artifacts.test.mjs scripts/check-no-main-deletions.test.mjs scripts/check-docs-snippet-motion.test.mjs scripts/registry-target-paths.test.mjs scripts/check-workspace-contracts.test.mjs scripts/check-package-cycles.test.mjs scripts/check-cli-process-ownership.test.mjs scripts/check-large-files.test.mjs scripts/package-subpaths.test.mjs scripts/validate-release-channel.test.mjs scripts/publish-workflow.test.mjs scripts/install-workspace-dependencies.test.mjs scripts/draft-changelog.test.ts scripts/set-version.test.ts scripts/release-prepare.test.ts scripts/cli-options.test.ts scripts/changelog-weekly.test.ts scripts/claude-plugin-compression.test.ts scripts/catalog-payload-assets.test.ts scripts/catalog-preview-temp.test.ts scripts/player-cdn-pin.test.ts scripts/studio-runtime-smoke.test.mjs scripts/verify-packed-manifests.test.mjs scripts/lint-skills.test.mjs packages/gcp-cloud-run/check-dockerfile-workspaces.test.mjs && vitest run scripts/catalog/", + "test:scripts": "node --import tsx --test scripts/animejs-v4-guidance.test.mjs scripts/check-tracked-artifacts.test.mjs scripts/check-no-main-deletions.test.mjs scripts/check-docs-snippet-motion.test.mjs scripts/registry-target-paths.test.mjs scripts/check-workspace-contracts.test.mjs scripts/check-package-cycles.test.mjs scripts/check-cli-process-ownership.test.mjs scripts/check-no-use-effect.test.mjs scripts/check-large-files.test.mjs scripts/package-subpaths.test.mjs scripts/validate-release-channel.test.mjs scripts/publish-workflow.test.mjs scripts/install-workspace-dependencies.test.mjs scripts/draft-changelog.test.ts scripts/set-version.test.ts scripts/release-prepare.test.ts scripts/cli-options.test.ts scripts/changelog-weekly.test.ts scripts/claude-plugin-compression.test.ts scripts/catalog-payload-assets.test.ts scripts/catalog-preview-temp.test.ts scripts/player-cdn-pin.test.ts scripts/studio-runtime-smoke.test.mjs scripts/verify-packed-manifests.test.mjs scripts/lint-skills.test.mjs packages/gcp-cloud-run/check-dockerfile-workspaces.test.mjs && vitest run scripts/catalog/", "typecheck:scripts": "tsc --noEmit -p scripts/tsconfig.json", "test:skills": "node --test 'skills/**/*.test.mjs'", "generate:previews": "tsx scripts/generate-template-previews.ts", diff --git a/scripts/check-no-use-effect.mjs b/scripts/check-no-use-effect.mjs index 197894af7b..525bb2b34f 100644 --- a/scripts/check-no-use-effect.mjs +++ b/scripts/check-no-use-effect.mjs @@ -214,63 +214,107 @@ const BUDGET = new Map([ ["packages/studio/src/webmcp/useStudioAgentTools.ts", 1], ]); -function sources() { +/** Studio source the ban applies to: TypeScript, minus the ambient declarations. */ +function isScannedSource(name) { + return /\.tsx?$/.test(name) && !name.endsWith(".d.ts"); +} + +/** Every file under SCANNED the ban applies to, repo-relative and sorted. */ +export function sources() { const found = []; const walk = (dir) => { for (const entry of readdirSync(join(ROOT, dir), { withFileTypes: true })) { const path = join(dir, entry.name); if (entry.isDirectory()) walk(path); - else if (/\.tsx?$/.test(entry.name) && !entry.name.endsWith(".d.ts")) found.push(path); + else if (isScannedSource(entry.name)) found.push(path); } }; walk(SCANNED); return found.sort(); } +/** + * The import clause of `import ... from "react"`, or undefined for any other statement. A hook + * imported from anywhere else is a different function that merely shares a name, so the module + * specifier has to match before any name in the clause means anything. + */ +function reactImportClause(statement) { + if (!ts.isImportDeclaration(statement)) return undefined; + if (!ts.isStringLiteral(statement.moduleSpecifier)) return undefined; + if (statement.moduleSpecifier.text !== "react") return undefined; + return statement.importClause; +} + +/** The local name of `import * as React from "react"`, if the clause has one. */ +function namespaceImportName(named) { + return named && ts.isNamespaceImport(named) ? named.name.text : undefined; +} + +/** Names standing for the whole React namespace: the default import and the star import. */ +function namespaceNames(clause) { + return [clause.name?.text, namespaceImportName(clause.namedBindings)].filter( + (name) => name !== undefined, + ); +} + +/** Local names a `{ ... }` clause binds to a banned hook, following `useEffect as x` aliases. */ +function bannedLocalNames(named) { + if (!named || !ts.isNamedImports(named)) return []; + return named.elements + .filter((spec) => BANNED.has((spec.propertyName ?? spec.name).text)) + .map((spec) => spec.name.text); +} + /** Local names in one file that refer to a banned hook, plus the React namespace names. */ -function reactBindings(root) { - const direct = new Set(); - const namespaces = new Set(); +export function reactBindings(root) { + const direct = []; + const namespaces = []; for (const statement of root.statements) { - if (!ts.isImportDeclaration(statement)) continue; - if (!ts.isStringLiteral(statement.moduleSpecifier)) continue; - if (statement.moduleSpecifier.text !== "react") continue; - const clause = statement.importClause; + const clause = reactImportClause(statement); if (!clause) continue; - if (clause.name) namespaces.add(clause.name.text); // import React from "react" - const named = clause.namedBindings; - if (named && ts.isNamespaceImport(named)) namespaces.add(named.name.text); - if (named && ts.isNamedImports(named)) { - for (const spec of named.elements) { - if (BANNED.has((spec.propertyName ?? spec.name).text)) direct.add(spec.name.text); - } - } + namespaces.push(...namespaceNames(clause)); + direct.push(...bannedLocalNames(clause.namedBindings)); } - return { direct, namespaces }; + return { direct: new Set(direct), namespaces: new Set(namespaces) }; } -/** Line numbers of every banned-hook call in one file. */ -function callSites(file, text = readFileSync(join(ROOT, file), "utf8")) { - const root = ts.createSourceFile( +/** + * One source file as an AST. Exported so a test parses exactly the way the scan does, rather than + * keeping a second copy of these options that can drift from the one that actually runs. + */ +export function parse(file, text) { + return ts.createSourceFile( file, text, ts.ScriptTarget.Latest, true, file.endsWith(".tsx") ? ts.ScriptKind.TSX : ts.ScriptKind.TS, ); +} + +/** `React.useEffect(...)`, where the object is a name bound to the React namespace. */ +function isNamespacedCallee(callee, namespaces) { + return ( + ts.isPropertyAccessExpression(callee) && + ts.isIdentifier(callee.expression) && + namespaces.has(callee.expression.text) && + BANNED.has(callee.name.text) + ); +} + +/** Whether what is being called resolves to a banned hook, by either spelling. */ +function isBannedCallee(callee, direct, namespaces) { + return ts.isIdentifier(callee) ? direct.has(callee.text) : isNamespacedCallee(callee, namespaces); +} + +/** Line numbers of every banned-hook call in one file. */ +export function callSites(file, text = readFileSync(join(ROOT, file), "utf8")) { + const root = parse(file, text); const { direct, namespaces } = reactBindings(root); const lines = []; const visit = (node) => { - if (ts.isCallExpression(node)) { - const callee = node.expression; - const hit = ts.isIdentifier(callee) - ? direct.has(callee.text) - : ts.isPropertyAccessExpression(callee) && - ts.isIdentifier(callee.expression) && - namespaces.has(callee.expression.text) && - BANNED.has(callee.name.text); - if (hit) lines.push(root.getLineAndCharacterOfPosition(node.getStart(root)).line + 1); - } + if (ts.isCallExpression(node) && isBannedCallee(node.expression, direct, namespaces)) + lines.push(root.getLineAndCharacterOfPosition(node.getStart(root)).line + 1); ts.forEachChild(node, visit); }; visit(root); @@ -288,38 +332,48 @@ function scan() { return found; } -function listBudgetIssues(found, budget = BUDGET) { - const problems = []; - for (const [file, lines] of found) { - const allowed = budget.get(file); - if (allowed === undefined) { - problems.push( - `NEW banned effect hook: ${file}:${lines.join(", :")}\n` + - ` The ban is absolute. Use useMountEffect() for a one-time external sync, or derive\n` + - ` the value during render. See CLAUDE.md > React Rules.`, - ); - } else if (lines.length > allowed) { - problems.push( - `OVER BUDGET: ${file} has ${lines.length}, budget allows ${allowed}\n` + - ` Lines: ${lines.join(", ")}. The budget is debt, not headroom.`, - ); - } - } - for (const [file, allowed] of budget) { - const actual = found.get(file)?.length ?? 0; - if (actual >= allowed) continue; - problems.push( - `STALE budget entry: ${file} now has ${actual}, budget still says ${allowed}\n` + - ` ${actual === 0 ? "Delete the entry." : `Lower it to ${actual}.`} Thanks for paying the debt down.`, +/** What is wrong with one offending file, or null when it is within its budget. */ +function budgetProblem(file, lines, budget) { + const allowed = budget.get(file); + if (allowed === undefined) + return ( + `NEW banned effect hook: ${file}:${lines.join(", :")}\n` + + ` The ban is absolute. Use useMountEffect() for a one-time external sync, or derive\n` + + ` the value during render. See CLAUDE.md > React Rules.` ); - } - return problems; + if (lines.length > allowed) + return ( + `OVER BUDGET: ${file} has ${lines.length}, budget allows ${allowed}\n` + + ` Lines: ${lines.join(", ")}. The budget is debt, not headroom.` + ); + return null; +} + +/** What is wrong with one budget entry the file has since paid down, or null when it is honest. */ +function staleProblem(file, allowed, actual) { + if (actual >= allowed) return null; + const fix = actual === 0 ? "Delete the entry." : `Lower it to ${actual}.`; + return ( + `STALE budget entry: ${file} now has ${actual}, budget still says ${allowed}\n` + + ` ${fix} Thanks for paying the debt down.` + ); +} + +/** Every way the scan disagrees with the register, in file order then budget order. */ +export function listBudgetIssues(found, budget = BUDGET) { + return [ + ...[...found].map(([file, lines]) => budgetProblem(file, lines, budget)), + ...[...budget].map(([file, allowed]) => + staleProblem(file, allowed, found.get(file)?.length ?? 0), + ), + ].filter((problem) => problem !== null); } function main() { const problems = listBudgetIssues(scan()); + const scanned = sources(); for (const file of SANCTIONED.keys()) { - if (!sources().includes(file)) + if (!scanned.includes(file)) problems.push(`SANCTIONED lists a file that no longer exists: ${file}`); } const debt = [...BUDGET.values()].reduce((total, count) => total + count, 0); diff --git a/scripts/check-no-use-effect.test.mjs b/scripts/check-no-use-effect.test.mjs new file mode 100644 index 0000000000..4805176208 --- /dev/null +++ b/scripts/check-no-use-effect.test.mjs @@ -0,0 +1,127 @@ +import assert from "node:assert/strict"; +import { describe, it } from "node:test"; +import { + callSites, + listBudgetIssues, + parse, + reactBindings, + sources, +} from "./check-no-use-effect.mjs"; + +describe("banned-hook call sites", () => { + it("counts a plain named import, on the line the call starts", () => { + assert.deepEqual( + callSites("a.tsx", 'import { useEffect } from "react";\n\nuseEffect(() => {});\n'), + [3], + ); + }); + + it("follows an alias back to the banned hook", () => { + assert.deepEqual( + callSites("a.tsx", 'import { useEffect as sync } from "react";\nsync(fn);\n'), + [2], + ); + }); + + it("counts the default and namespace React imports", () => { + assert.deepEqual(callSites("a.tsx", 'import React from "react";\nReact.useEffect(fn);\n'), [2]); + assert.deepEqual( + callSites("a.tsx", 'import * as R from "react";\nR.useLayoutEffect(fn);\n'), + [2], + ); + }); + + it("counts useLayoutEffect against the same budget", () => { + assert.deepEqual( + callSites( + "a.tsx", + 'import { useEffect, useLayoutEffect } from "react";\nuseEffect(fn);\nuseLayoutEffect(fn);\n', + ), + [2, 3], + ); + }); + + it("does not count prose that quotes the rule", () => { + assert.deepEqual( + callSites("a.tsx", 'import { useEffect } from "react";\n// never call useEffect(fn) here\n'), + [], + ); + }); + + it("does not count a same-named hook from another module", () => { + assert.deepEqual( + callSites("a.ts", 'import { useEffect } from "./local";\nuseEffect(fn);\n'), + [], + ); + }); + + it("does not count a property access on a non-React object", () => { + assert.deepEqual(callSites("a.ts", 'import React from "react";\nlib.useEffect(fn);\n'), []); + }); +}); + +describe("react binding resolution", () => { + it("separates aliased hook names from React namespace names", () => { + const { direct, namespaces } = reactBindings( + parse( + "a.tsx", + 'import React, { useEffect as sync, useState } from "react";\nimport * as R from "react";\n', + ), + ); + assert.deepEqual([...direct].sort(), ["sync"]); + assert.deepEqual([...namespaces].sort(), ["R", "React"]); + }); + + it("ignores imports from other modules", () => { + const { direct, namespaces } = reactBindings( + parse("a.ts", 'import { useEffect } from "preact/hooks";\nimport React from "./shim";\n'), + ); + assert.equal(direct.size, 0); + assert.equal(namespaces.size, 0); + }); +}); + +describe("budget accounting", () => { + it("rejects an effect in a file with no budget entry", () => { + const [problem, ...rest] = listBudgetIssues(new Map([["new.tsx", [7]]]), new Map()); + assert.deepEqual(rest, []); + assert.match(problem, /^NEW banned effect hook: new\.tsx:7$/m); + }); + + it("rejects a budgeted file that grew", () => { + const [problem, ...rest] = listBudgetIssues( + new Map([["old.tsx", [4, 9]]]), + new Map([["old.tsx", 1]]), + ); + assert.deepEqual(rest, []); + assert.match(problem, /^OVER BUDGET: old\.tsx has 2, budget allows 1$/m); + }); + + it("rejects a budget that is now too high, and says what to lower it to", () => { + const [problem, ...rest] = listBudgetIssues( + new Map([["old.tsx", [4]]]), + new Map([["old.tsx", 3]]), + ); + assert.deepEqual(rest, []); + assert.match(problem, /^STALE budget entry: old\.tsx now has 1, budget still says 3$/m); + assert.match(problem, /Lower it to 1\./); + }); + + it("asks for a fully paid entry to be deleted", () => { + const [problem] = listBudgetIssues(new Map(), new Map([["old.tsx", 2]])); + assert.match(problem, /Delete the entry\./); + }); + + it("accepts a file sitting exactly on its budget", () => { + assert.deepEqual( + listBudgetIssues(new Map([["old.tsx", [4, 9]]]), new Map([["old.tsx", 2]])), + [], + ); + }); +}); + +describe("scanned tree", () => { + it("reaches the sanctioned wrapper, so SCANNED still points at studio source", () => { + assert.ok(sources().includes("packages/studio/src/hooks/useMountEffect.ts")); + }); +}); From eab389bf18e5882fb699c2da56402a59de9d8ef0 Mon Sep 17 00:00:00 2001 From: Miguel Angel Simon Sierra Date: Wed, 2 Sep 2026 20:14:27 -0400 Subject: [PATCH 3/4] ci(studio): close the effect gate's bypasses by spelling A ban that one import spelling walks around is not a gate. The scanner now resolves the react namespace through `require` and dynamic `import` (awaited or not, bound or destructured), reads computed member access, follows chains of local aliases in any declaration order, and fails a barrel under the scanned tree that re-exports the hook, since the barrel is the file the laundering route needs and it is in scope. `.js` and `.jsx` are scanned on the same terms as `.ts` and `.tsx`. The script header now lists every spelling as either detected or out of scope with its reason, so nothing is silently unhandled: cross-package barrels (no file in the repo re-exports react, and catching one needs whole-program resolution) and runtime indirection a static pass cannot follow. The sanctioned file stops being skipped wholesale, which had made it the one place any effect could hide. It must hold exactly one banned-hook call and that call must pass an empty dependency array, so useMountEffect cannot quietly grow into a general effect. Each new detection has a test that fails when the detection is removed. --- scripts/check-no-use-effect.mjs | 246 +++++++++++++++++++++++---- scripts/check-no-use-effect.test.mjs | 125 +++++++++++++- 2 files changed, 330 insertions(+), 41 deletions(-) diff --git a/scripts/check-no-use-effect.mjs b/scripts/check-no-use-effect.mjs index 525bb2b34f..848726c218 100644 --- a/scripts/check-no-use-effect.mjs +++ b/scripts/check-no-use-effect.mjs @@ -24,6 +24,33 @@ // is resolved rather than assumed away: `import { useEffect as x }`, `React.useEffect` and a // namespace import all resolve to the same call and all count. Both banned hooks share one // budget, so neither can be smuggled in by spelling it the other way. +// +// Spellings, and how far resolution reaches. Every form below is either DETECTED or listed as OUT +// OF SCOPE with its reason; none is silently unhandled. +// +// DETECTED `import { useEffect }`, `import { useEffect as x }`, `import React from "react"` + +// `React.useEffect`, `import * as R from "react"` + `R.useLayoutEffect`. +// DETECTED computed namespace access: `React["useEffect"]`. +// DETECTED runtime loads of the module -- `require` and dynamic `import`, awaited or not -- +// with the namespace either bound (`const R = await import()`) or destructured +// (`const { useEffect } = require()`). `` stands for the literal module +// specifier: spelling it out inside these parentheses makes the repository's +// dependency audit read this comment as a real import of react. +// DETECTED local aliases, transitively: `const e = useEffect`, then `const f = e`, and +// `const e = React["useEffect"]`. +// DETECTED a barrel re-export written UNDER the scanned tree: `export { useEffect } from "react"` +// and `export * from "react"` fail in the barrel itself, so importing the hook through +// a local barrel cannot launder it. The barrel is the file that has to exist for the +// bypass, and it is in scope, so the bypass is closed where it is written. +// DETECTED `.js` and `.jsx` sources, on the same terms as `.ts` and `.tsx`. +// OUT OF SCOPE a barrel OUTSIDE the scanned tree -- another workspace package re-exporting react. +// Catching it needs cross-package module resolution: a resolver plus a whole-program +// parse, to close a route that does not exist today (no file under `packages/` +// re-exports react at all). If one is ever written, ban it where it is written, the way +// the in-scope rule above already does. +// OUT OF SCOPE indirection no static pass can follow: `React[flag ? "useEffect" : "useMemo"]`, a +// hook pulled out of a data structure, `eval`. A ratchet resists drift, not an author +// deliberately defeating it -- that author can equally well edit BUDGET below. import ts from "typescript"; import { readdirSync, readFileSync } from "node:fs"; @@ -214,9 +241,9 @@ const BUDGET = new Map([ ["packages/studio/src/webmcp/useStudioAgentTools.ts", 1], ]); -/** Studio source the ban applies to: TypeScript, minus the ambient declarations. */ -function isScannedSource(name) { - return /\.tsx?$/.test(name) && !name.endsWith(".d.ts"); +/** Studio source the ban applies to: JavaScript and TypeScript, minus the ambient declarations. */ +export function isScannedSource(name) { + return /\.[jt]sx?$/.test(name) && !name.endsWith(".d.ts"); } /** Every file under SCANNED the ban applies to, repo-relative and sorted. */ @@ -265,6 +292,122 @@ function bannedLocalNames(named) { .map((spec) => spec.name.text); } +/** Whether a callee loads a module at runtime: the `import` keyword, or `require`. */ +function isModuleLoader(callee) { + if (callee.kind === ts.SyntaxKind.ImportKeyword) return true; + return ts.isIdentifier(callee) && callee.text === "require"; +} + +/** The literal module specifier a call loads, or undefined when it is not a literal. */ +function loadedModule(node) { + const [specifier] = node.arguments; + return specifier && ts.isStringLiteralLike(specifier) ? specifier.text : undefined; +} + +/** Whether a call loads the react module, by `require` or by dynamic `import`. */ +function isReactModuleCall(node) { + return isModuleLoader(node.expression) && loadedModule(node) === "react"; +} + +/** The expression under any number of `await` and parenthesis wrappers. */ +function unwrap(expression) { + let node = expression; + while (ts.isAwaitExpression(node) || ts.isParenthesizedExpression(node)) node = node.expression; + return node; +} + +/** Whether an expression evaluates to the React namespace, by name or by loading the module. */ +function isReactNamespace(wrapped, namespaces) { + const expression = unwrap(wrapped); + if (ts.isIdentifier(expression)) return namespaces.has(expression.text); + return ts.isCallExpression(expression) && isReactModuleCall(expression); +} + +/** The property a member access reads, spelled either `x.y` or `x["y"]`. */ +function memberName(expression) { + if (ts.isPropertyAccessExpression(expression)) return expression.name.text; + if ( + ts.isElementAccessExpression(expression) && + ts.isStringLiteralLike(expression.argumentExpression) + ) + return expression.argumentExpression.text; + return undefined; +} + +/** Whether an expression resolves to a banned hook: a bound local, or React's member by any spelling. */ +function isBannedExpression(expression, direct, namespaces) { + if (ts.isIdentifier(expression)) return direct.has(expression.text); + const name = memberName(expression); + return ( + name !== undefined && BANNED.has(name) && isReactNamespace(expression.expression, namespaces) + ); +} + +/** The banned names a `const { useEffect } = ` pattern binds, in their local spelling. */ +function destructuredNames(pattern) { + return pattern.elements + .filter((element) => ts.isIdentifier(element.propertyName ?? element.name)) + .filter((element) => BANNED.has((element.propertyName ?? element.name).text)) + .filter((element) => ts.isIdentifier(element.name)) + .map((element) => ({ kind: "direct", name: element.name.text })); +} + +/** What `const x = ...` contributes: a hook alias, a React namespace alias, or nothing. */ +function nameBindings(name, initializer, direct, namespaces) { + if (isBannedExpression(initializer, direct, namespaces)) return [{ kind: "direct", name }]; + if (isReactNamespace(initializer, namespaces)) return [{ kind: "namespaces", name }]; + return []; +} + +/** What `const { useEffect } = ...` contributes: the banned names it pulls off React. */ +function patternBindings(pattern, initializer, namespaces) { + return isReactNamespace(initializer, namespaces) ? destructuredNames(pattern) : []; +} + +/** What one variable declaration adds to the bindings, whichever way it is written. */ +function declaredBindings(declaration, direct, namespaces) { + const { name, initializer } = declaration; + if (!initializer) return []; + if (ts.isObjectBindingPattern(name)) return patternBindings(name, initializer, namespaces); + return ts.isIdentifier(name) ? nameBindings(name.text, initializer, direct, namespaces) : []; +} + +/** Every node of one kind in a file, in source order. */ +function collect(root, matches) { + const found = []; + const visit = (node) => { + if (matches(node)) found.push(node); + ts.forEachChild(node, visit); + }; + visit(root); + return found; +} + +/** One resolution pass: every alias visible from the bindings as they currently stand. */ +function addAliases(declarations, bindings) { + for (const declaration of declarations) + for (const found of declaredBindings(declaration, bindings.direct, bindings.namespaces)) + bindings[found.kind].add(found.name); +} + +/** How many names the bindings hold: the only thing a pass can change. */ +function bindingCount(bindings) { + return bindings.direct.size + bindings.namespaces.size; +} + +/** + * Grow the bindings by every alias of them until a pass adds nothing, so a chain like + * `const b = a; const a = useEffect;` resolves whatever order the declarations are written in. + */ +function resolveAliases(root, bindings) { + const declarations = collect(root, ts.isVariableDeclaration); + for (let previous = -1; previous !== bindingCount(bindings); ) { + previous = bindingCount(bindings); + addAliases(declarations, bindings); + } + return bindings; +} + /** Local names in one file that refer to a banned hook, plus the React namespace names. */ export function reactBindings(root) { const direct = []; @@ -275,7 +418,7 @@ export function reactBindings(root) { namespaces.push(...namespaceNames(clause)); direct.push(...bannedLocalNames(clause.namedBindings)); } - return { direct: new Set(direct), namespaces: new Set(namespaces) }; + return resolveAliases(root, { direct: new Set(direct), namespaces: new Set(namespaces) }); } /** @@ -288,37 +431,70 @@ export function parse(file, text) { text, ts.ScriptTarget.Latest, true, - file.endsWith(".tsx") ? ts.ScriptKind.TSX : ts.ScriptKind.TS, + file.endsWith("x") ? ts.ScriptKind.TSX : ts.ScriptKind.TS, ); } -/** `React.useEffect(...)`, where the object is a name bound to the React namespace. */ -function isNamespacedCallee(callee, namespaces) { +/** Every banned-hook call in one parsed file, as nodes. */ +export function bannedCalls(root) { + const { direct, namespaces } = reactBindings(root); + return collect( + root, + (node) => ts.isCallExpression(node) && isBannedExpression(node.expression, direct, namespaces), + ); +} + +/** The module a re-export pulls from, or undefined when the statement is not one. */ +function reExportedModule(statement) { + if (!ts.isExportDeclaration(statement)) return undefined; + const specifier = statement.moduleSpecifier; + return specifier && ts.isStringLiteral(specifier) ? specifier.text : undefined; +} + +/** Whether an export clause hands out a banned hook. No clause is `export *`, which hands out all. */ +function exportsBannedName(clause) { + if (!clause) return true; return ( - ts.isPropertyAccessExpression(callee) && - ts.isIdentifier(callee.expression) && - namespaces.has(callee.expression.text) && - BANNED.has(callee.name.text) + ts.isNamedExports(clause) && + clause.elements.some((spec) => BANNED.has((spec.propertyName ?? spec.name).text)) ); } -/** Whether what is being called resolves to a banned hook, by either spelling. */ -function isBannedCallee(callee, direct, namespaces) { - return ts.isIdentifier(callee) ? direct.has(callee.text) : isNamespacedCallee(callee, namespaces); +/** Whether a re-export hands a banned hook to other files: `export { useEffect } from "react"`. */ +function isReactReExport(statement) { + return reExportedModule(statement) === "react" && exportsBannedName(statement.exportClause); } -/** Line numbers of every banned-hook call in one file. */ -export function callSites(file, text = readFileSync(join(ROOT, file), "utf8")) { +/** Line numbers of every banned-hook call and every react re-export in one file. */ +export function violations(file, text = readFileSync(join(ROOT, file), "utf8")) { const root = parse(file, text); - const { direct, namespaces } = reactBindings(root); - const lines = []; - const visit = (node) => { - if (ts.isCallExpression(node) && isBannedCallee(node.expression, direct, namespaces)) - lines.push(root.getLineAndCharacterOfPosition(node.getStart(root)).line + 1); - ts.forEachChild(node, visit); - }; - visit(root); - return lines; + const line = (node) => root.getLineAndCharacterOfPosition(node.getStart(root)).line + 1; + const found = [...bannedCalls(root), ...root.statements.filter(isReactReExport)]; + return found.map(line).sort((a, b) => a - b); +} + +/** The shape the sanctioned wrapper must keep: one `useEffect(effect, [])` and nothing else. */ +function isMountEffectCall(node) { + const [, deps] = node.arguments; + return ( + node.arguments.length === 2 && ts.isArrayLiteralExpression(deps) && deps.elements.length === 0 + ); +} + +/** + * How the sanctioned file departs from useMountEffect(), or null when it still matches. Skipping + * the file wholesale would make it a hiding place: any effect, any dependency array, unchecked. + */ +export function sanctionedProblem(file, text = readFileSync(join(ROOT, file), "utf8")) { + const calls = bannedCalls(parse(file, text)); + if (calls.length !== 1) + return `SANCTIONED file drifted: ${file} has ${calls.length} banned hook calls, expected 1.`; + if (!isMountEffectCall(calls[0])) + return ( + `SANCTIONED file drifted: ${file} must call useEffect(effect, []) and nothing else.\n` + + ` Its one effect now takes a different dependency array, which is a general effect.` + ); + return null; } /** Every offending file under SCANNED, mapped to the lines its banned hooks sit on. */ @@ -326,7 +502,7 @@ function scan() { const found = new Map(); for (const file of sources()) { if (SANCTIONED.has(file)) continue; - const lines = callSites(file); + const lines = violations(file); if (lines.length > 0) found.set(file, lines); } return found; @@ -369,13 +545,19 @@ export function listBudgetIssues(found, budget = BUDGET) { ].filter((problem) => problem !== null); } +/** Every way a sanctioned file fails to be the thing it was sanctioned for. */ +export function listSanctionedIssues(scanned, sanctioned = SANCTIONED) { + return [...sanctioned.keys()] + .map((file) => + scanned.includes(file) + ? sanctionedProblem(file) + : `SANCTIONED lists a file that no longer exists: ${file}`, + ) + .filter((problem) => problem !== null); +} + function main() { - const problems = listBudgetIssues(scan()); - const scanned = sources(); - for (const file of SANCTIONED.keys()) { - if (!scanned.includes(file)) - problems.push(`SANCTIONED lists a file that no longer exists: ${file}`); - } + const problems = [...listBudgetIssues(scan()), ...listSanctionedIssues(sources())]; const debt = [...BUDGET.values()].reduce((total, count) => total + count, 0); if (problems.length === 0) { console.log( diff --git a/scripts/check-no-use-effect.test.mjs b/scripts/check-no-use-effect.test.mjs index 4805176208..bc4330df7e 100644 --- a/scripts/check-no-use-effect.test.mjs +++ b/scripts/check-no-use-effect.test.mjs @@ -1,39 +1,45 @@ import assert from "node:assert/strict"; import { describe, it } from "node:test"; import { - callSites, + isScannedSource, listBudgetIssues, + listSanctionedIssues, parse, reactBindings, + sanctionedProblem, sources, + violations, } from "./check-no-use-effect.mjs"; describe("banned-hook call sites", () => { it("counts a plain named import, on the line the call starts", () => { assert.deepEqual( - callSites("a.tsx", 'import { useEffect } from "react";\n\nuseEffect(() => {});\n'), + violations("a.tsx", 'import { useEffect } from "react";\n\nuseEffect(() => {});\n'), [3], ); }); it("follows an alias back to the banned hook", () => { assert.deepEqual( - callSites("a.tsx", 'import { useEffect as sync } from "react";\nsync(fn);\n'), + violations("a.tsx", 'import { useEffect as sync } from "react";\nsync(fn);\n'), [2], ); }); it("counts the default and namespace React imports", () => { - assert.deepEqual(callSites("a.tsx", 'import React from "react";\nReact.useEffect(fn);\n'), [2]); assert.deepEqual( - callSites("a.tsx", 'import * as R from "react";\nR.useLayoutEffect(fn);\n'), + violations("a.tsx", 'import React from "react";\nReact.useEffect(fn);\n'), + [2], + ); + assert.deepEqual( + violations("a.tsx", 'import * as R from "react";\nR.useLayoutEffect(fn);\n'), [2], ); }); it("counts useLayoutEffect against the same budget", () => { assert.deepEqual( - callSites( + violations( "a.tsx", 'import { useEffect, useLayoutEffect } from "react";\nuseEffect(fn);\nuseLayoutEffect(fn);\n', ), @@ -43,20 +49,20 @@ describe("banned-hook call sites", () => { it("does not count prose that quotes the rule", () => { assert.deepEqual( - callSites("a.tsx", 'import { useEffect } from "react";\n// never call useEffect(fn) here\n'), + violations("a.tsx", 'import { useEffect } from "react";\n// never call useEffect(fn) here\n'), [], ); }); it("does not count a same-named hook from another module", () => { assert.deepEqual( - callSites("a.ts", 'import { useEffect } from "./local";\nuseEffect(fn);\n'), + violations("a.ts", 'import { useEffect } from "./local";\nuseEffect(fn);\n'), [], ); }); it("does not count a property access on a non-React object", () => { - assert.deepEqual(callSites("a.ts", 'import React from "react";\nlib.useEffect(fn);\n'), []); + assert.deepEqual(violations("a.ts", 'import React from "react";\nlib.useEffect(fn);\n'), []); }); }); @@ -125,3 +131,104 @@ describe("scanned tree", () => { assert.ok(sources().includes("packages/studio/src/hooks/useMountEffect.ts")); }); }); + +// A gate is only worth its name if it cannot be walked around by spelling. One test per bypass the +// review named, so a regression in resolution shows up as a named failure rather than a quiet pass. +describe("bypasses by spelling", () => { + it("resolves a namespace bound by dynamic import", () => { + assert.deepEqual( + violations("a.ts", 'const R = await import("react");\nR.useEffect(fn);\n'), + [2], + ); + }); + + it("resolves a hook destructured out of require()", () => { + assert.deepEqual( + violations("a.ts", 'const { useEffect } = require("react");\nuseEffect(fn);\n'), + [2], + ); + }); + + it("resolves computed namespace access", () => { + assert.deepEqual( + violations("a.ts", 'import React from "react";\nReact["useEffect"](fn);\n'), + [2], + ); + }); + + it("follows a chain of local aliases, declared in either order", () => { + assert.deepEqual( + violations( + "a.ts", + 'import { useEffect } from "react";\nconst b = a;\nconst a = useEffect;\nb(fn);\n', + ), + [4], + ); + }); + + it("fails the barrel that re-exports the hook, named or star", () => { + assert.deepEqual(violations("a.ts", '\nexport { useEffect } from "react";\n'), [2]); + assert.deepEqual(violations("a.ts", '\n\nexport * from "react";\n'), [3]); + }); + + it("does not fail a re-export of some other react name", () => { + assert.deepEqual(violations("a.ts", 'export { useMemo } from "react";\n'), []); + }); + + it("scans .js and .jsx alongside .ts and .tsx", () => { + assert.ok(isScannedSource("legacy.js")); + assert.ok(isScannedSource("legacy.jsx")); + assert.ok(!isScannedSource("ambient.d.ts")); + assert.deepEqual( + violations("a.js", 'import { useEffect } from "react";\nuseEffect(fn);\n'), + [2], + ); + }); + + it("still ignores a same-named hook loaded from another module", () => { + assert.deepEqual( + violations("a.ts", 'const { useEffect } = require("preact/hooks");\nuseEffect(fn);\n'), + [], + ); + }); +}); + +describe("the sanctioned escape hatch", () => { + const mount = + 'import { useEffect } from "react";\nexport function useMountEffect(effect) {\n useEffect(effect, []);\n}\n'; + + it("accepts the intended implementation", () => { + assert.equal(sanctionedProblem("useMountEffect.ts", mount), null); + }); + + it("rejects a second effect in the sanctioned file", () => { + assert.match( + sanctionedProblem("useMountEffect.ts", mount + "useEffect(other, []);\n"), + /has 2 banned hook calls, expected 1/, + ); + }); + + it("rejects an effect with a non-empty dependency array", () => { + assert.match( + sanctionedProblem("useMountEffect.ts", mount.replace("[]", "[effect]")), + /must call useEffect\(effect, \[\]\)/, + ); + }); + + it("rejects an effect with no dependency array at all", () => { + assert.match( + sanctionedProblem("useMountEffect.ts", mount.replace(", []", "")), + /must call useEffect\(effect, \[\]\)/, + ); + }); + + it("reports a sanctioned file that has been deleted", () => { + assert.deepEqual(listSanctionedIssues([], new Map([["gone.ts", "why"]])), [ + "SANCTIONED lists a file that no longer exists: gone.ts", + ]); + }); + + it("holds the real useMountEffect.ts to that shape", () => { + assert.deepEqual(listSanctionedIssues(sources()), []); + }); +}); From d378a390884c954c92afa926f1e4f28e679918bc Mon Sep 17 00:00:00 2001 From: Miguel Angel Simon Sierra Date: Wed, 2 Sep 2026 20:14:28 -0400 Subject: [PATCH 4/4] ci: run the effect gate in a required check `Lint` is not required by the branch ruleset, so a gate that only runs there cannot block a merge. The required `Typecheck` context runs it too. No ruleset change needed. --- .github/workflows/ci.yml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index d2dc3af06d..79f3c2e39b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -268,6 +268,10 @@ jobs: - run: bun run build - run: bun run --filter '*' typecheck - run: bun run typecheck:scripts + # `Lint` is not a required check, so a gate that only runs there cannot block a merge. The + # effect ban is meant to block, so the required `Typecheck` context runs it too. It is a + # 200ms AST pass over one package; running it twice costs nothing worth saving. + - run: bun run check:no-use-effect test: name: Test