perf: reorder declarations to fix React Compiler scope pruning (#24098)

.github/workflows/typos.toml

@@ -36,6 +36,7 @@ typ = "typ"
 styl = "styl"
 edn = "edn"
 Inferrable = "Inferrable"
+IIF = "IIF"
 
 [files]
 extend-exclude = [

site/scripts/check-compiler.mjs

@@ -27,6 +27,15 @@ const skipPatterns = [".test.", ".stories.", ".jest."];
 // Maximum length for truncated error messages in the report.
 const MAX_ERROR_LENGTH = 120;
 
+// Patterns that identify a function/closure value on the RHS of an
+// assignment. Primitives (strings, numbers, booleans) are fine without
+// memoization because `!==` compares them by value. Only reference types
+// (closures, objects, arrays) cause problems.
+const CLOSURE_RHS = /^\s*(?:const|let)\s+(\w+)\s*=\s*(?:async\s+)?(?:\([^)]*\)\s*=>|\w+\s*=>|function\s*\()/;
+
+// Matches a `$[N] !== name` fragment inside an `if (...)` guard.
+const DEP_CHECK = /\$\[\d+\]\s*!==\s*(\w+)/g;
+
 // ---------------------------------------------------------------------------
 // File collection
 // ---------------------------------------------------------------------------
@@ -69,8 +78,8 @@ function collectFiles(dir) {
 //
 // We use transformSync deliberately. The React Compiler plugin is
 // CPU-bound (parse-only takes ~2s vs ~19s with the compiler over all
-// of site/src), so transformAsync + Promise.all gives no speedup —
-// Node still runs all transforms on a single thread. Benchmarked
+// of site/src), so transformAsync + Promise.all gives no speedup
+// because Node still runs all transforms on a single thread. Benchmarked
 // sync, async-sequential, and async-parallel: all land within noise
 // of each other. The sync API keeps the code simple.
 // ---------------------------------------------------------------------------
@@ -153,11 +162,13 @@ function compileFile(file) {
 
 		return {
 			compiled: compiledCount,
+			code: result?.code ?? "",
 			diagnostics: deduplicateDiagnostics(diagnostics),
 		};
 	} catch (e) {
 		return {
 			compiled: 0,
+			code: "",
 			diagnostics: [{
 				line: 0,
 				// Truncate to keep the one-line report readable.
@@ -167,6 +178,70 @@ function compileFile(file) {
 	}
 }
 
+// ---------------------------------------------------------------------------
+// Scope-pruning detection
+//
+// The compiler's flattenScopesWithHooksOrUse pass silently drops
+// memoization scopes that span across hook calls. A closure whose
+// scope is pruned appears as a bare `const name = (...) =>` with
+// no `$[N]` guard, yet it may still be listed as a dependency in a
+// downstream JSX memoization block (`$[N] !== name`). That means
+// the JSX cache check fails every render because `name` is a new
+// function reference each time.
+//
+// findUnmemoizedClosureDeps detects this pattern in compiled output:
+// 1. Collect every name that appears in a `$[N] !== name` dep check.
+// 2. For each, check if the name is assigned a function value
+//    (arrow or function expression) outside any `$[N]` guard.
+// 3. If so, the closure is unmemoized but used as a reactive dep,
+//    which defeats the downstream memoization.
+// ---------------------------------------------------------------------------
+
+/**
+ * Scan compiled output for closures that appear as dependencies in
+ * memoization guards but are not themselves memoized. Returns an
+ * array of `{ name, line }` objects for each finding.
+ */
+export function findUnmemoizedClosureDeps(code) {
+	if (!code) return [];
+
+	const lines = code.split("\n");
+
+	// Pass 1: collect every name used in a $[N] !== name dep check.
+	const depNames = new Set();
+	for (const line of lines) {
+		for (const m of line.matchAll(DEP_CHECK)) {
+			depNames.add(m[1]);
+		}
+	}
+	if (depNames.size === 0) return [];
+
+	// Pass 2: find closure definitions that are directly assigned a
+	// function value (not assigned from a temp like `const x = t1`).
+	// A memoized closure uses the temp pattern:
+	//   if ($[N] !== dep) { t1 = () => {...}; } else { t1 = $[N]; }
+	//   const name = t1;
+	// An unmemoized closure is assigned the function directly:
+	//   const name = () => {...};
+	const findings = [];
+	for (let i = 0; i < lines.length; i++) {
+		const match = lines[i].match(CLOSURE_RHS);
+		if (!match) continue;
+
+		const name = match[1];
+		if (!depNames.has(name)) continue;
+
+		// Compiler temporaries are named t0, t1, ... tN. If the
+		// variable name matches that pattern it's an intermediate,
+		// not a user-visible declaration.
+		if (/^t\d+$/.test(name)) continue;
+
+		findings.push({ name, line: i + 1 });
+	}
+
+	return findings;
+}
+
 // ---------------------------------------------------------------------------
 // Report
 // ---------------------------------------------------------------------------
@@ -206,27 +281,47 @@ function printReport(failures, totalCompiled, fileCount, hadErrors) {
 // Main
 // ---------------------------------------------------------------------------
 
+// Tracks whether collectFiles encountered a missing directory.
+// Module-scoped so the function can set it and the main block can
+// read it after collection finishes.
+let hadCollectionErrors = false;
+
 // Only run the main block when executed directly, not when imported
 // by tests for the exported pure functions.
 if (process.argv[1] === fileURLToPath(import.meta.url)) {
-	let hadCollectionErrors = false;
 
 	const files = targetDirs.flatMap((d) => collectFiles(join(siteDir, d)));
 
-let totalCompiled = 0;
-const failures = [];
+	let totalCompiled = 0;
+	const failures = [];
 
-for (const file of files) {
-	const { compiled, diagnostics } = compileFile(file);
-	totalCompiled += compiled;
-	if (diagnostics.length > 0) {
-		failures.push({ file, compiled, diagnostics });
+	const scopePruned = [];
+
+	for (const file of files) {
+		const { compiled, code, diagnostics } = compileFile(file);
+		totalCompiled += compiled;
+		if (diagnostics.length > 0) {
+			failures.push({ file, compiled, diagnostics });
+		}
+		const pruned = findUnmemoizedClosureDeps(code);
+		if (pruned.length > 0) {
+			scopePruned.push({ file, closures: pruned });
+		}
+	}
+
+	printReport(failures, totalCompiled, files.length, hadCollectionErrors);
+
+	if (scopePruned.length > 0) {
+		console.log("\nUnmemoized closures used as reactive dependencies:");
+		console.log("(Move these after all hook calls to restore memoization)\n");
+		for (const { file, closures } of scopePruned) {
+			for (const c of closures) {
+				console.log(`  ✗ ${shortPath(file)}: ${c.name}`);
+			}
+		}
+	}
+
+	if (failures.length > 0 || hadCollectionErrors || scopePruned.length > 0) {
+		process.exitCode = 1;
 	}
 }
-
-printReport(failures, totalCompiled, files.length, hadCollectionErrors);
-
-if (failures.length > 0 || hadCollectionErrors) {
-	process.exitCode = 1;
-}
-}

site/scripts/check-compiler.test.mjs

@@ -1,6 +1,7 @@
 import { describe, expect, it } from "vitest";
 import {
 	deduplicateDiagnostics,
+	findUnmemoizedClosureDeps,
 	shortPath,
 	shortenMessage,
 } from "./check-compiler.mjs";
@@ -93,3 +94,110 @@ describe("shortPath", () => {
 			.toBe("src/utils/helper.ts");
 	});
 });
+
+describe("findUnmemoizedClosureDeps", () => {
+	it("detects a bare closure used in a dep check", () => {
+		const code = [
+			"const urlTransform = url => {",
+			"  return rewrite(url);",
+			"};",
+			"let t0;",
+			"if ($[0] !== urlTransform) {",
+			"  t0 = <View urlTransform={urlTransform} />;",
+			"}",
+		].join("\n");
+		expect(findUnmemoizedClosureDeps(code)).toEqual([
+			{ name: "urlTransform", line: 1 },
+		]);
+	});
+
+	it("ignores a memoized closure (preceded by else branch)", () => {
+		const code = [
+			"let t1;",
+			"if ($[0] !== proxyHost) {",
+			"  t1 = url => rewrite(url, proxyHost);",
+			"  $[0] = proxyHost;",
+			"  $[1] = t1;",
+			"} else {",
+			"  t1 = $[1];",
+			"}",
+			"const urlTransform = t1;",
+			"if ($[2] !== urlTransform) {",
+			"  t2 = <View urlTransform={urlTransform} />;",
+			"}",
+		].join("\n");
+		expect(findUnmemoizedClosureDeps(code)).toEqual([]);
+	});
+
+	it("ignores primitives (not closures)", () => {
+		const code = [
+			"const offset = (page - 1) * pageSize;",
+			"if ($[0] !== offset) {",
+			"  t0 = <View offset={offset} />;",
+			"}",
+		].join("\n");
+		expect(findUnmemoizedClosureDeps(code)).toEqual([]);
+	});
+
+	it("ignores closures not referenced in any dep check", () => {
+		const code = [
+			"const handler = () => console.log('hi');",
+			"return <View />;",
+		].join("\n");
+		expect(findUnmemoizedClosureDeps(code)).toEqual([]);
+	});
+
+	it("detects async closures", () => {
+		const code = [
+			"const doWork = async (id) => {",
+			"  await api.call(id);",
+			"};",
+			"if ($[0] !== doWork) {",
+			"  t0 = <View handler={doWork} />;",
+			"}",
+		].join("\n");
+		expect(findUnmemoizedClosureDeps(code)).toEqual([
+			{ name: "doWork", line: 1 },
+		]);
+	});
+
+	it("returns empty for empty input", () => {
+		expect(findUnmemoizedClosureDeps("")).toEqual([]);
+		expect(findUnmemoizedClosureDeps(null)).toEqual([]);
+		expect(findUnmemoizedClosureDeps(undefined)).toEqual([]);
+	});
+
+	it("detects multiple unmemoized closures", () => {
+		const code = [
+			"const fn1 = (x) => x + 1;",
+			"const fn2 = (y) => y * 2;",
+			"if ($[0] !== fn1 || $[1] !== fn2) {",
+			"  t0 = <View a={fn1} b={fn2} />;",
+			"}",
+		].join("\n");
+		const result = findUnmemoizedClosureDeps(code);
+		expect(result).toHaveLength(2);
+		expect(result[0].name).toBe("fn1");
+		expect(result[1].name).toBe("fn2");
+	});
+
+	// The CLOSURE_RHS regex also matches IIFEs like `const x = (() => {...})();`.
+	// The compiler does not emit IIFEs in compiled output, so this is not
+	// a real-world false positive today. This test documents the assumption
+	// so it breaks visibly if the compiler changes its output shape.
+	it("matches IIFEs (documents known regex limitation)", () => {
+		const code = [
+			"const config = (() => {",
+			"  return { theme: 'dark' };",
+			"})();",
+			"if ($[0] !== config) {",
+			"  t0 = <View config={config} />;",
+			"}",
+		].join("\n");
+		// CLOSURE_RHS matches the IIFE because it starts with `(() =>`.
+		// This is a known false positive that does not occur in practice.
+		expect(findUnmemoizedClosureDeps(code)).toEqual([
+			{ name: "config", line: 1 },
+		]);
+	});
+});

site/src/pages/AgentsPage/AgentChatPage.tsx

@@ -573,22 +573,6 @@ const AgentChatPage: FC = () => {
 	const workspaceAgent = getWorkspaceAgent(workspace, undefined);
 	const { proxy } = useProxy();
 
-	// Extract the primitive fields used by the transform so the
-	// compiler can see the real dependencies and avoid invalidating
-	// the closure when the workspace object reference changes but
-	// the relevant fields haven't.
-	const proxyHost = proxy.preferredWildcardHostname;
-	const agentName = workspaceAgent?.name;
-	const wsName = workspace?.name;
-	const wsOwner = workspace?.owner_name;
-
-	const urlTransform: UrlTransform = (url) => {
-		if (!proxyHost || !agentName || !wsName || !wsOwner) {
-			return url;
-		}
-		return rewriteLocalhostURL(url, proxyHost, agentName, wsName, wsOwner);
-	};
-
 	const chatRecord = chatQuery.data;
 
 	// Initialize MCP selection from chat record or defaults.
@@ -1090,6 +1074,19 @@ const AgentChatPage: FC = () => {
 		agentId,
 	]);
 
+	// Primitives extracted from proxy/workspace so the compiler
+	// tracks stable strings, not object identity.
+	const proxyHost = proxy.preferredWildcardHostname;
+	const agentName = workspaceAgent?.name;
+	const wsName = workspace?.name;
+	const wsOwner = workspace?.owner_name;
+	const urlTransform: UrlTransform = (url) => {
+		if (!proxyHost || !agentName || !wsName || !wsOwner) {
+			return url;
+		}
+		return rewriteLocalhostURL(url, proxyHost, agentName, wsName, wsOwner);
+	};
+
 	const handleRegenerateTitle = () => {
 		if (!agentId || isRegenerateTitleDisabled || !onRegenerateTitle) {
 			return;

site/src/pages/AgentsPage/AgentsPage.tsx

@@ -155,6 +155,43 @@ const AgentsPage: FC = () => {
 	// deduplicates the requests.
 	const chatModelsQuery = useQuery(chatModels());
 	const chatModelConfigsQuery = useQuery(chatModelConfigs());
+	const [chatErrorReasons, setChatErrorReasons] = useState<
+		Record<string, ChatDetailError>
+	>({});
+	const setChatErrorReason = (chatId: string, reason: ChatDetailError) => {
+		const trimmedMessage = reason.message.trim();
+		if (!chatId || !trimmedMessage) {
+			return;
+		}
+		const nextReason: ChatDetailError = {
+			...reason,
+			message: trimmedMessage,
+		};
+		setChatErrorReasons((current) => {
+			const existing = current[chatId];
+			if (chatDetailErrorsEqual(existing, nextReason)) {
+				return current;
+			}
+			return {
+				...current,
+				[chatId]: nextReason,
+			};
+		});
+	};
+	const clearChatErrorReason = (chatId: string) => {
+		if (!chatId) {
+			return;
+		}
+		setChatErrorReasons((current) => {
+			if (!(chatId in current)) {
+				return current;
+			}
+			const next = { ...current };
+			delete next[chatId];
+			return next;
+		});
+	};
+
 	const archiveChatBase = archiveChat(queryClient);
 	const archiveAgentMutation = useMutation({
 		...archiveChatBase,
@@ -245,46 +282,10 @@ const AgentsPage: FC = () => {
 		readonly string[]
 	>([]);
 	const [isSidebarCollapsed, setIsSidebarCollapsed] = useState(false);
-	const [chatErrorReasons, setChatErrorReasons] = useState<
-		Record<string, ChatDetailError>
-	>({});
 	const catalogModelOptions = getModelOptionsFromConfigs(
 		chatModelConfigsQuery.data,
 		chatModelsQuery.data,
 	);
-	const setChatErrorReason = (chatId: string, reason: ChatDetailError) => {
-		const trimmedMessage = reason.message.trim();
-		if (!chatId || !trimmedMessage) {
-			return;
-		}
-		const nextReason: ChatDetailError = {
-			...reason,
-			message: trimmedMessage,
-		};
-		setChatErrorReasons((current) => {
-			const existing = current[chatId];
-			if (chatDetailErrorsEqual(existing, nextReason)) {
-				return current;
-			}
-			return {
-				...current,
-				[chatId]: nextReason,
-			};
-		});
-	};
-	const clearChatErrorReason = (chatId: string) => {
-		if (!chatId) {
-			return;
-		}
-		setChatErrorReasons((current) => {
-			if (!(chatId in current)) {
-				return current;
-			}
-			const next = { ...current };
-			delete next[chatId];
-			return next;
-		});
-	};
 	const chatList = chatsQuery.data?.pages.flat() ?? [];
 	const isArchiving =
 		archiveAgentMutation.isPending || archiveAndDeleteMutation.isPending;
@@ -320,6 +321,32 @@ const AgentsPage: FC = () => {
 			},
 		});
 	};
+
+	// Track the active chat ID in a ref so the watchChats
+	// WebSocket handler can read it without re-subscribing
+	// on every navigation.
+	const activeChatIDRef = useRef(agentId);
+	const navigateAfterArchive = (archivedChatId: string) => {
+		const activeChatId = activeChatIDRef.current;
+		if (
+			shouldNavigateAfterArchive(
+				activeChatId,
+				archivedChatId,
+				// Read root_chat_id from the per-chat cache, which
+				// survives WebSocket eviction of sub-agents (only the
+				// parent's chatKey is removed). This must be read at
+				// callback time so it reflects the user's current
+				// location.
+				activeChatId
+					? queryClient.getQueryData<TypesGen.Chat>(chatKey(activeChatId))
+							?.root_chat_id
+					: undefined,
+			)
+		) {
+			navigate("/agents");
+		}
+	};
+
 	const requestArchiveAndDeleteWorkspace = async (
 		chatId: string,
 		workspaceId: string,
@@ -435,30 +462,6 @@ const AgentsPage: FC = () => {
 		navigate("/agents");
 	};
 
-	// Track the active chat ID in a ref so the watchChats
-	// WebSocket handler can read it without re-subscribing on
-	// every navigation.
-	const activeChatIDRef = useRef(agentId);
-	const navigateAfterArchive = (archivedChatId: string) => {
-		const activeChatId = activeChatIDRef.current;
-		if (
-			shouldNavigateAfterArchive(
-				activeChatId,
-				archivedChatId,
-				// Read root_chat_id from the per-chat cache, which
-				// survives WebSocket eviction of sub-agents (only the
-				// parent's chatKey is removed). This must be read at
-				// callback time so it reflects the user's current
-				// location.
-				activeChatId
-					? queryClient.getQueryData<TypesGen.Chat>(chatKey(activeChatId))
-							?.root_chat_id
-					: undefined,
-			)
-		) {
-			navigate("/agents");
-		}
-	};
 	useEffect(() => {
 		activeChatIDRef.current = agentId;
 	});