diff --git a/.github/workflows/typos.toml b/.github/workflows/typos.toml index 011115e500..0f7f9a6cc9 100644 --- a/.github/workflows/typos.toml +++ b/.github/workflows/typos.toml @@ -36,6 +36,7 @@ typ = "typ" styl = "styl" edn = "edn" Inferrable = "Inferrable" +IIF = "IIF" [files] extend-exclude = [ diff --git a/site/scripts/check-compiler.mjs b/site/scripts/check-compiler.mjs index a4e193237f..2ea58a8e8c 100644 --- a/site/scripts/check-compiler.mjs +++ b/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; -} -} diff --git a/site/scripts/check-compiler.test.mjs b/site/scripts/check-compiler.test.mjs index 9dce97f81c..20c389d4db 100644 --- a/site/scripts/check-compiler.test.mjs +++ b/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 = ;", + "}", + ].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 = ;", + "}", + ].join("\n"); + expect(findUnmemoizedClosureDeps(code)).toEqual([]); + }); + + it("ignores primitives (not closures)", () => { + const code = [ + "const offset = (page - 1) * pageSize;", + "if ($[0] !== offset) {", + " t0 = ;", + "}", + ].join("\n"); + expect(findUnmemoizedClosureDeps(code)).toEqual([]); + }); + + it("ignores closures not referenced in any dep check", () => { + const code = [ + "const handler = () => console.log('hi');", + "return ;", + ].join("\n"); + expect(findUnmemoizedClosureDeps(code)).toEqual([]); + }); + + it("detects async closures", () => { + const code = [ + "const doWork = async (id) => {", + " await api.call(id);", + "};", + "if ($[0] !== doWork) {", + " t0 = ;", + "}", + ].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 = ;", + "}", + ].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 = ;", + "}", + ].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 }, + ]); + }); +}); diff --git a/site/src/pages/AgentsPage/AgentChatPage.tsx b/site/src/pages/AgentsPage/AgentChatPage.tsx index 9990a482d6..43580f86fa 100644 --- a/site/src/pages/AgentsPage/AgentChatPage.tsx +++ b/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; diff --git a/site/src/pages/AgentsPage/AgentsPage.tsx b/site/src/pages/AgentsPage/AgentsPage.tsx index 07f6cfd62c..541151d12f 100644 --- a/site/src/pages/AgentsPage/AgentsPage.tsx +++ b/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 + >({}); + 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 - >({}); 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(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(chatKey(activeChatId)) - ?.root_chat_id - : undefined, - ) - ) { - navigate("/agents"); - } - }; useEffect(() => { activeChatIDRef.current = agentId; });