diff --git a/AGENTS.md b/AGENTS.md index 0804392af9..40a6ae6e49 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -174,6 +174,14 @@ pnpm verify:workspace # deep opt-in verification (lint -> test:full -> build); - Reuse existing systems, helpers, and hooks after searching for an equivalent before adding a new one. If a new primitive is genuinely necessary, justify it in the change and first check documented patterns in `docs/solutions/`. - Authoritative references: [Styling Guide — Design tokens and Component classes](docs/dashboard-guide.md#styling-guide), `packages/dashboard/app/styles.css` (token/primitive source of truth), and `docs/solutions/`. +### Standing Rule: Never Declare a Component Inside Another Component + +- A React component declared inside another component's render is a **new element type on every render**, so React unmounts and remounts its whole subtree on each parent update — focused inputs are destroyed mid-typing, expanded/scrolled rows reset, and local state is silently discarded. +- Hoist it to module scope and pass what it needs as props. If it is only markup, make it a **lowercase render function** (`renderModalShell(children)`): that returns elements without introducing an element type, so the subtree reconciles in place. +- Enforced by `fusion-react/no-nested-component-definitions` (defined in `eslint.config.mjs`, covered by `packages/dashboard/app/__tests__/eslint-no-nested-components.test.ts`). Escape hatch: a preceding `// nested-component-allowlist: ` comment. +- Motivating incidents: FN-8606's `ModalShell` made Planning Mode and Settings untypable (each keystroke remounted the composer, so only the first character survived), and `MailboxModal`'s `ReplyContextExpandable` collapsed already-expanded reply rows whenever another row was expanded. +- Testing note: `fireEvent.change` sets a value without needing the node to stay mounted, so it **cannot** catch this. Assert with real per-character `userEvent.type`, or assert DOM node identity across an unrelated re-render. + ### Standing Rule: Fix the Invariant, Not the Repro (FN-5893) - When fixing a bug, the regression test must assert the general invariant across ALL known surfaces — not only the single reported reproduction. diff --git a/eslint.config.mjs b/eslint.config.mjs index 6fff687614..205e81c4e5 100644 --- a/eslint.config.mjs +++ b/eslint.config.mjs @@ -69,6 +69,133 @@ const detachedSpawnGuard = { }, }; +/* +FNXC:LintConfig 2026-07-26-21:05: +Ban React components declared inside another component's render. A component declared in render is a NEW element +type on every render, so React unmounts and remounts its entire subtree each time the parent updates: focused +inputs are destroyed mid-typing, expanded/scrolled rows reset, and local state is silently discarded. + +This has shipped three times. FN-8606's `ModalShell` made Planning Mode and Settings untypable (every keystroke +remounted the composer, so only the first character survived), and MailboxModal's `ReplyContextExpandable` +collapsed already-expanded reply rows whenever any other row was expanded. None of it was caught by review or by +tests using fireEvent.change, which sets a value without needing the node to stay mounted. + +The fix is always the same: hoist the component to module scope and pass what it needs as props, or — when it is +just markup, not a component — make it a plain render function (`renderModalShell(children)`), whose returned +element types stay stable. Lowercase render helpers are therefore not reported. + +Escape hatch: a preceding `// nested-component-allowlist: ` comment within 3 lines. +*/ +export const noNestedComponentDefinitions = { + meta: { + type: "problem", + docs: { + description: "ban React component definitions nested inside another component", + }, + schema: [], + }, + create(context) { + const sourceCode = context.sourceCode; + const visitorKeys = sourceCode.visitorKeys; + const allowlistMarker = "nested-component-allowlist:"; + const FUNCTION_TYPES = new Set(["FunctionDeclaration", "FunctionExpression", "ArrowFunctionExpression"]); + const COMPONENT_WRAPPERS = new Set(["memo", "forwardRef"]); + /** Stack of enclosing functions; `rendersJsx` marks the ones that are components. */ + const functionStack = []; + + function hasAllowlistComment(node) { + if (!node.loc) return false; + const startLine = node.loc.start.line; + const windowStart = Math.max(0, startLine - 3); + return sourceCode.lines.slice(windowStart, startLine).some((line) => line.includes(allowlistMarker)); + } + + /** Walk `node` and its descendants, never descending into nested functions (they own their own JSX). */ + function walkOwnBody(node, visit) { + if (!node || typeof node.type !== "string") return; + visit(node); + for (const key of visitorKeys[node.type] ?? []) { + const value = node[key]; + for (const child of Array.isArray(value) ? value : [value]) { + if (!child || typeof child.type !== "string" || FUNCTION_TYPES.has(child.type)) continue; + walkOwnBody(child, visit); + } + } + } + + function containsJsx(node) { + let found = false; + walkOwnBody(node, (candidate) => { + if (candidate.type === "JSXElement" || candidate.type === "JSXFragment") found = true; + }); + return found; + } + + function rendersJsx(fnNode) { + if (fnNode.type === "ArrowFunctionExpression" && fnNode.body.type !== "BlockStatement") { + return containsJsx(fnNode.body); + } + let found = false; + walkOwnBody(fnNode.body, (candidate) => { + if (candidate.type === "ReturnStatement" && candidate.argument && containsJsx(candidate.argument)) found = true; + }); + return found; + } + + /* + A function is component-NAMED when it is bound to a PascalCase name, directly or through memo()/forwardRef(). + Lowercase bindings are render helpers: they return elements without introducing an element type, so they are + safe inside a render and are deliberately not reported. + */ + function componentName(fnNode) { + if (fnNode.type === "FunctionDeclaration") { + return fnNode.id?.name ?? null; + } + let current = fnNode; + let parent = current.parent; + if (parent?.type === "CallExpression" && parent.arguments[0] === current) { + const callee = parent.callee; + const calleeName = callee.type === "MemberExpression" && !callee.computed && callee.property.type === "Identifier" + ? callee.property.name + : callee.type === "Identifier" ? callee.name : null; + if (!calleeName || !COMPONENT_WRAPPERS.has(calleeName)) return null; + current = parent; + parent = current.parent; + } + if (parent?.type === "VariableDeclarator" && parent.init === current && parent.id.type === "Identifier") { + return parent.id.name; + } + return null; + } + + function enterFunction(node) { + const name = componentName(node); + const isComponent = Boolean(name) && /^[A-Z]/.test(name) && rendersJsx(node); + if (isComponent && functionStack.some((entry) => entry.isComponent) && !hasAllowlistComment(node)) { + context.report({ + node, + message: + `Component "${name}" is defined inside another component. React treats it as a new element type on every render, remounting its subtree and destroying focus, scroll, and local state. Hoist it to module scope and pass what it needs as props, or make it a lowercase render function (e.g. render${name}(...)) if it is only markup.`, + }); + } + functionStack.push({ isComponent: isComponent || rendersJsx(node) }); + } + + function exitFunction() { + functionStack.pop(); + } + + return { + FunctionDeclaration: enterFunction, + "FunctionDeclaration:exit": exitFunction, + FunctionExpression: enterFunction, + "FunctionExpression:exit": exitFunction, + ArrowFunctionExpression: enterFunction, + "ArrowFunctionExpression:exit": exitFunction, + }; + }, +}; + const noPluginViewReexport = { meta: { type: "problem", @@ -607,6 +734,31 @@ export default tseslint.config( }, }, + // ───────────────────────────────────────────────────────────── + // REACT SOURCE — no component definitions nested inside a render + // (see noNestedComponentDefinitions above for the incident history) + // ───────────────────────────────────────────────────────────── + { + files: [ + "packages/*/src/**/*.tsx", + "packages/dashboard/app/**/*.tsx", + "plugins/**/*.tsx", + ], + ignores: ["**/__tests__/**", "**/*.test.tsx", "**/*.gen.tsx"], + plugins: { + // Distinct namespace: the `fusion` plugin name is already claimed for these same + // files by the detached-spawn block, and flat config forbids redefining it. + "fusion-react": { + rules: { + "no-nested-component-definitions": noNestedComponentDefinitions, + }, + }, + }, + rules: { + "fusion-react/no-nested-component-definitions": "error", + }, + }, + // ───────────────────────────────────────────────────────────── // SERVICE WORKER FILES — browser service worker globals // (packages/dashboard/app/public/sw.js uses self, caches, fetch, etc.) diff --git a/packages/dashboard/app/__tests__/eslint-no-nested-components.test.ts b/packages/dashboard/app/__tests__/eslint-no-nested-components.test.ts new file mode 100644 index 0000000000..3ab3ac219b --- /dev/null +++ b/packages/dashboard/app/__tests__/eslint-no-nested-components.test.ts @@ -0,0 +1,99 @@ +import { describe, expect, it } from "vitest"; +import { Linter } from "eslint"; +import tseslint from "typescript-eslint"; +import { noNestedComponentDefinitions } from "../../../../eslint.config.mjs"; + +/* +FNXC:LintConfig 2026-07-26-21:05: +Guards the `fusion-react/no-nested-component-definitions` rule that keeps components out of other +components' render bodies. The rule exists because that pattern shipped three times (FN-8606's +ModalShell left Planning Mode and Settings untypable; MailboxModal's ReplyContextExpandable collapsed +expanded reply rows), so the rule itself needs coverage — a silently-broken guard is worse than none. +Both halves matter: it must flag real nested components AND leave the sanctioned escapes alone, or +the codebase routes around it. +*/ + +const linter = new Linter(); + +function lint(code: string): string[] { + const config = { + files: ["**/*.tsx"], + languageOptions: { + parser: tseslint.parser, + parserOptions: { ecmaVersion: "latest", sourceType: "module", ecmaFeatures: { jsx: true } }, + }, + plugins: { "fusion-react": { rules: { "no-nested-component-definitions": noNestedComponentDefinitions } } }, + rules: { "fusion-react/no-nested-component-definitions": "error" }, + } as unknown as Linter.Config; + const messages = linter.verify(code, config, "probe.tsx"); + return messages.map((message) => message.message); +} + +describe("fusion-react/no-nested-component-definitions", () => { + it("flags a PascalCase arrow component declared inside a component", () => { + const messages = lint(` + export function Parent() { + const Badge = ({ label }: { label: string }) => {label}; + return
; + } + `); + expect(messages).toHaveLength(1); + expect(messages[0]).toContain('Component "Badge" is defined inside another component'); + }); + + it("flags a nested function declaration component", () => { + const messages = lint(` + export function Parent() { + function Row() { + return
  • row
  • ; + } + return ; + } + `); + expect(messages).toHaveLength(1); + expect(messages[0]).toContain('Component "Row" is defined inside another component'); + }); + + it("flags memo()/forwardRef()-wrapped nested components", () => { + const messages = lint(` + import { memo } from "react"; + export function Parent() { + const Row = memo(() =>
  • row
  • ); + return ; + } + `); + expect(messages).toHaveLength(1); + expect(messages[0]).toContain('Component "Row" is defined inside another component'); + }); + + /* + The sanctioned fixes. A lowercase render function returns elements without introducing an element + type, so the subtree reconciles in place — that is exactly the shape PlanningModeModal/SettingsModal + were moved to (`renderModalShell(children)`), and it must never be reported. + */ + it("allows lowercase render helpers, inline callbacks, and module-scope components", () => { + expect(lint(` + function Badge({ label }: { label: string }) { + return {label}; + } + export function Parent({ items }: { items: string[] }) { + const renderShell = (children: unknown) =>
    {children}
    ; + return ( +
    console.log("noop")}> + {renderShell(items.map((item) => ))} +
    + ); + } + `)).toEqual([]); + }); + + it("honors the nested-component-allowlist escape hatch", () => { + expect(lint(` + export function Parent() { + // nested-component-allowlist: deliberate, documented exception + const Badge = () => x; + return
    ; + } + `)).toEqual([]); + }); +}); diff --git a/packages/dashboard/app/components/ModelOnboardingModal.tsx b/packages/dashboard/app/components/ModelOnboardingModal.tsx index 676aee96b8..cff94c615b 100644 --- a/packages/dashboard/app/components/ModelOnboardingModal.tsx +++ b/packages/dashboard/app/components/ModelOnboardingModal.tsx @@ -636,6 +636,55 @@ interface GitHubActionViewModel { readyVia: "oauth" | "gh-cli" | null; } +/* +FNXC:ModelOnboarding 2026-07-26-21:05: +Status badges stay at module scope like the other onboarding subcomponents. Declared inside +ModelOnboardingModal's render they were a new element type per render, remounting each badge on every +onboarding state change; `fusion-react/no-nested-component-definitions` now enforces this. +Both read their copy from useTranslation directly, so they need no props beyond `status`. +*/ +function ProviderStatusBadge({ status }: { status: ProviderConnectionStatus }) { + const { t } = useTranslation("app"); + const config: Record = { + connected: { text: t("setup.statusConnected", "✓ Connected"), className: "auth-status-badge connected" }, + "not-connected": { text: t("setup.statusNotConnected", "Not connected"), className: "auth-status-badge not-connected" }, + skipped: { text: t("setup.statusSkipped", "Skipped"), className: "auth-status-badge skipped" }, + retry: { text: t("setup.statusRetry", "Retry"), className: "auth-status-badge retry" }, + }; + const { text, className: badgeClassName } = config[status]; + return ( + + {text} + + ); +} + +function GitHubStatusBadge({ status }: { status: GitHubConnectionStatus }) { + const { t } = useTranslation("app"); + const config: Record = { + connected: { text: t("setup.statusConnected", "✓ Connected"), className: "auth-status-badge connected" }, + pending: { text: t("setup.statusConnecting", "⏳ Connecting…"), className: "auth-status-badge pending" }, + failed: { text: t("setup.statusConnectionFailed", "✗ Connection failed"), className: "auth-status-badge retry" }, + skipped: { text: t("setup.statusSkipped", "Skipped"), className: "auth-status-badge skipped" }, + "not-connected": { text: t("setup.statusNotConnected", "Not connected"), className: "auth-status-badge not-connected" }, + }; + + const { text, className: badgeClassName } = config[status]; + return ( + + {text} + + ); +} + const GIT_INSTALL_URL = "https://git-scm.com/downloads"; const GH_CLI_INSTALL_URL = "https://github.com/cli/cli/releases/latest"; @@ -1050,26 +1099,6 @@ export function ModelOnboardingModal({ return "not-connected"; }, [loginOutcomes, skippedProviders]); - // Status badge component for provider connection status - function ProviderStatusBadge({ status }: { status: ProviderConnectionStatus }) { - const config: Record = { - connected: { text: t("setup.statusConnected", "✓ Connected"), className: "auth-status-badge connected" }, - "not-connected": { text: t("setup.statusNotConnected", "Not connected"), className: "auth-status-badge not-connected" }, - skipped: { text: t("setup.statusSkipped", "Skipped"), className: "auth-status-badge skipped" }, - retry: { text: t("setup.statusRetry", "Retry"), className: "auth-status-badge retry" }, - }; - const { text, className: badgeClassName } = config[status]; - return ( - - {text} - - ); - } - const getGitHubStatus = useCallback((): GitHubConnectionStatus => { if (githubActionState.ready) { return "connected"; @@ -1088,27 +1117,6 @@ export function ModelOnboardingModal({ return "not-connected"; }, [githubActionState]); - function GitHubStatusBadge({ status }: { status: GitHubConnectionStatus }) { - const config: Record = { - connected: { text: t("setup.statusConnected", "✓ Connected"), className: "auth-status-badge connected" }, - pending: { text: t("setup.statusConnecting", "⏳ Connecting…"), className: "auth-status-badge pending" }, - failed: { text: t("setup.statusConnectionFailed", "✗ Connection failed"), className: "auth-status-badge retry" }, - skipped: { text: t("setup.statusSkipped", "Skipped"), className: "auth-status-badge skipped" }, - "not-connected": { text: t("setup.statusNotConnected", "Not connected"), className: "auth-status-badge not-connected" }, - }; - - const { text, className: badgeClassName } = config[status]; - return ( - - {text} - - ); - } - // Load models const loadModels = useCallback(async () => { try {