From bbbbdc251927b7a0a4c8c9da90be847ff5cf1801 Mon Sep 17 00:00:00 2001 From: Tarrence van As Date: Mon, 29 Sep 2025 15:22:38 -0500 Subject: [PATCH 1/9] fix: improve theme preset loading and add tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Fix race conditions in theme preset loading - Add cleanup for async operations to prevent memory leaks - Simplify theme effect dependencies to avoid unnecessary re-renders - Add comprehensive tests for useConnectionValue hook - Ensure theme falls back correctly when preset is missing or invalid 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude --- .../keychain/src/hooks/connection.test.ts | 161 +++++++++++++++++- packages/keychain/src/hooks/connection.ts | 61 +++++-- 2 files changed, 203 insertions(+), 19 deletions(-) diff --git a/packages/keychain/src/hooks/connection.test.ts b/packages/keychain/src/hooks/connection.test.ts index afa2ef04ee..f4df001f1e 100644 --- a/packages/keychain/src/hooks/connection.test.ts +++ b/packages/keychain/src/hooks/connection.test.ts @@ -1,5 +1,64 @@ -import { isOriginVerified } from "./connection"; +import { ReactNode } from "react"; +import { renderHook, waitFor } from "@testing-library/react"; +import { MemoryRouter } from "react-router-dom"; import { vi } from "vitest"; +import { defaultTheme } from "@cartridge/presets"; +import { isOriginVerified, useConnectionValue } from "./connection"; + +const loadConfigMock = vi.fn(); +const useThemeEffectMock = vi.fn(); + +vi.mock("@cartridge/presets", async () => { + const actual = await vi.importActual< + typeof import("@cartridge/presets") + >("@cartridge/presets"); + + return { + ...actual, + loadConfig: loadConfigMock, + }; +}); + +vi.mock("@cartridge/ui", async () => { + const actual = await vi.importActual( + "@cartridge/ui", + ); + + return { + ...actual, + useThemeEffect: useThemeEffectMock, + }; +}); + +vi.mock("@cartridge/ui/utils", async () => { + const actual = await vi.importActual< + typeof import("@cartridge/ui/utils") + >("@cartridge/ui/utils"); + + return { + ...actual, + isIframe: () => true, + normalizeOrigin: (origin: string) => origin, + }; +}); + +vi.mock("@/components/connect/create/utils", () => ({ + fetchController: vi.fn(() => Promise.resolve({ controller: null })), +})); + +declare global { + // Extend window used within tests without importing app-level types + // eslint-disable-next-line @typescript-eslint/consistent-type-definitions + interface Window { + controller?: { + rpcUrl: () => string; + chainId: () => string; + disconnect: () => Promise | void; + username: () => string | undefined; + }; + keychain_wallets?: unknown; + } +} describe("isOriginVerified", () => { const allowedOrigins = ["example.com", "*.example.com", "sub.test.com"]; @@ -115,3 +174,103 @@ vi.mock("@/utils/connection", () => ({ destroy: vi.fn(), })), })); + +describe("useConnectionValue", () => { + const createWrapper = (entry: string) => + function Wrapper({ children }: { children: ReactNode }) { + return {children}; + }; + + beforeEach(() => { + loadConfigMock.mockReset(); + useThemeEffectMock.mockReset(); + useThemeEffectMock.mockImplementation(() => undefined); + mockGetChainId.mockReset(); + mockGetChainId.mockResolvedValue("0x1"); + + window.controller = { + rpcUrl: () => "https://rpc.example.com", + chainId: () => "0x534e5f534550", + disconnect: () => Promise.resolve(), + username: () => undefined, + }; + window.keychain_wallets = undefined; + }); + + it("keeps the default theme when no preset is provided", async () => { + const { result } = renderHook(() => useConnectionValue(), { + wrapper: createWrapper("/connect"), + }); + + await waitFor(() => expect(useThemeEffectMock).toHaveBeenCalled()); + + expect(loadConfigMock).not.toHaveBeenCalled(); + expect(result.current.theme.name).toBe(defaultTheme.name); + expect(result.current.theme.verified).toBe(true); + expect(result.current.verified).toBe(false); + + const lastCall = useThemeEffectMock.mock.calls.at(-1)?.[0]; + expect(lastCall?.theme.name).toBe(defaultTheme.name); + expect(lastCall?.theme.verified).toBe(true); + }); + + it("applies the preset theme when config resolves", async () => { + loadConfigMock.mockResolvedValue({ + origin: ["test.com"], + theme: { ...defaultTheme, name: "Test Theme" }, + }); + + const { result } = renderHook(() => useConnectionValue(), { + wrapper: createWrapper("/connect?preset=test"), + }); + + await waitFor(() => { + expect(loadConfigMock).toHaveBeenCalledWith("test"); + expect(result.current.theme.name).toBe("Test Theme"); + expect(result.current.verified).toBe(true); + }); + + const lastCall = useThemeEffectMock.mock.calls.at(-1)?.[0]; + expect(lastCall?.theme.name).toBe("Test Theme"); + expect(lastCall?.theme.verified).toBe(true); + }); + + it("falls back to the default theme when config lacks a theme", async () => { + loadConfigMock.mockResolvedValue({ + origin: ["test.com"], + }); + + const { result } = renderHook(() => useConnectionValue(), { + wrapper: createWrapper("/connect?preset=test"), + }); + + await waitFor(() => { + expect(loadConfigMock).toHaveBeenCalledWith("test"); + expect(result.current.verified).toBe(true); + expect(result.current.theme.name).toBe(defaultTheme.name); + }); + + const lastCall = useThemeEffectMock.mock.calls.at(-1)?.[0]; + expect(lastCall?.theme.name).toBe(defaultTheme.name); + expect(lastCall?.theme.verified).toBe(true); + }); + + it("marks the preset as unverified when config loading fails", async () => { + loadConfigMock.mockRejectedValue(new Error("network error")); + + const { result } = renderHook(() => useConnectionValue(), { + wrapper: createWrapper("/connect?preset=test"), + }); + + await waitFor(() => expect(loadConfigMock).toHaveBeenCalledWith("test")); + + await waitFor(() => { + expect(result.current.verified).toBe(false); + expect(result.current.theme.name).toBe(defaultTheme.name); + }); + + const lastCall = useThemeEffectMock.mock.calls.at(-1)?.[0]; + expect(lastCall?.theme.name).toBe(defaultTheme.name); + expect(lastCall?.theme.verified).toBe(true); + }); +}); diff --git a/packages/keychain/src/hooks/connection.ts b/packages/keychain/src/hooks/connection.ts index 0ceaf51b8c..d266bc2c3c 100644 --- a/packages/keychain/src/hooks/connection.ts +++ b/packages/keychain/src/hooks/connection.ts @@ -365,52 +365,77 @@ export function useConnectionValue() { // Check if preset is verified for the current origin, supporting wildcards useEffect(() => { - if (!urlParams.preset) { + const preset = urlParams.preset; + + if (!preset) { + setConfigData(null); + setVerified(false); return; } + let isActive = true; + setIsConfigLoading(true); - loadConfig(urlParams.preset) + loadConfig(preset) .then((config) => { + if (!isActive) return; + if (config && config.origin) { const allowedOrigins = toArray(config.origin as string | string[]); setVerified(isOriginVerified(origin, allowedOrigins)); - setConfigData(config as Record); + } else { + setVerified(false); } + + setConfigData( + config ? (config as Record) : null, + ); }) .catch((error: Error) => { + if (!isActive) return; console.error("Failed to load config:", error); + setConfigData(null); + setVerified(false); }) .finally(() => { + if (!isActive) return; setIsConfigLoading(false); }); + + return () => { + isActive = false; + }; }, [origin, urlParams]); // Handle theme configuration useEffect(() => { const { preset } = urlParams; - // Skip if the theme has already been set and preset is not defined - if (theme.name !== defaultTheme.name && !preset) return; + if (!preset) { + setTheme({ + verified: true, + ...defaultTheme, + }); + return; + } + + if (isConfigLoading) { + return; + } - if ( - preset && - !isConfigLoading && - configData && - configData && - "theme" in configData - ) { + if (configData && "theme" in configData) { setTheme({ verified, ...(configData.theme as ControllerTheme), }); - } else { - setTheme({ - verified: true, - ...defaultTheme, - }); + return; } - }, [urlParams, verified, configData, isConfigLoading, theme.name]); + + setTheme({ + verified: true, + ...defaultTheme, + }); + }, [urlParams, verified, configData, isConfigLoading]); useEffect(() => { if (urlParams.version) { From df37561c199c58ec475886c45e4b51c187189646 Mon Sep 17 00:00:00 2001 From: Tarrence van As Date: Mon, 29 Sep 2025 17:00:14 -0500 Subject: [PATCH 2/9] fix: move vitest mocks to top of file MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Vitest requires all vi.mock() calls to be at the top of the file before any other code execution. This fixes the syntax errors in CI. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude --- .../keychain/src/hooks/connection.test.ts | 98 +++++++++---------- 1 file changed, 49 insertions(+), 49 deletions(-) diff --git a/packages/keychain/src/hooks/connection.test.ts b/packages/keychain/src/hooks/connection.test.ts index f4df001f1e..e7b2eeac8d 100644 --- a/packages/keychain/src/hooks/connection.test.ts +++ b/packages/keychain/src/hooks/connection.test.ts @@ -46,6 +46,55 @@ vi.mock("@/components/connect/create/utils", () => ({ fetchController: vi.fn(() => Promise.resolve({ controller: null })), })); +// Mock RpcProvider +const mockGetChainId = vi.fn(); +vi.mock("starknet", async () => { + const actual = await vi.importActual("starknet"); + return { + ...actual, + RpcProvider: vi.fn().mockImplementation(() => ({ + getChainId: mockGetChainId, + })), + }; +}); + +// Mock Controller +const mockController = { + appId: () => "test-app", + classHash: () => "0x123", + chainId: () => "0x534e5f534550", + rpcUrl: () => "https://rpc.example.com", + address: () => "0x456", + username: () => "testuser", + owner: () => "0x789", +}; + +vi.mock("@/utils/controller", () => ({ + default: vi.fn().mockImplementation((options: Record) => ({ + ...mockController, + chainId: () => options.chainId || mockController.chainId(), + rpcUrl: () => options.rpcUrl || mockController.rpcUrl(), + })), +})); + +// Mock navigation hook +const mockNavigate = vi.fn(); +vi.mock("@/context/navigation", () => ({ + useNavigation: () => ({ + navigate: mockNavigate, + }), +})); + +// Mock other dependencies +vi.mock("@/utils/connection", () => ({ + connectToController: vi.fn(() => ({ + promise: Promise.resolve({ + origin: "https://test.com", + }), + destroy: vi.fn(), + })), +})); + declare global { // Extend window used within tests without importing app-level types // eslint-disable-next-line @typescript-eslint/consistent-type-definitions @@ -126,55 +175,6 @@ describe("isOriginVerified", () => { }); }); -// Mock RpcProvider -const mockGetChainId = vi.fn(); -vi.mock("starknet", async () => { - const actual = await vi.importActual("starknet"); - return { - ...actual, - RpcProvider: vi.fn().mockImplementation(() => ({ - getChainId: mockGetChainId, - })), - }; -}); - -// Mock Controller -const mockController = { - appId: () => "test-app", - classHash: () => "0x123", - chainId: () => "0x534e5f534550", - rpcUrl: () => "https://rpc.example.com", - address: () => "0x456", - username: () => "testuser", - owner: () => "0x789", -}; - -vi.mock("@/utils/controller", () => ({ - default: vi.fn().mockImplementation((options: Record) => ({ - ...mockController, - chainId: () => options.chainId || mockController.chainId(), - rpcUrl: () => options.rpcUrl || mockController.rpcUrl(), - })), -})); - -// Mock navigation hook -const mockNavigate = vi.fn(); -vi.mock("@/context/navigation", () => ({ - useNavigation: () => ({ - navigate: mockNavigate, - }), -})); - -// Mock other dependencies -vi.mock("@/utils/connection", () => ({ - connectToController: vi.fn(() => ({ - promise: Promise.resolve({ - origin: "https://test.com", - }), - destroy: vi.fn(), - })), -})); - describe("useConnectionValue", () => { const createWrapper = (entry: string) => function Wrapper({ children }: { children: ReactNode }) { From 89e1980eda2bf3b53bdc842bbc86dc26e4e70edf Mon Sep 17 00:00:00 2001 From: Tarrence van As Date: Mon, 29 Sep 2025 17:03:28 -0500 Subject: [PATCH 3/9] fix: rename test file to .tsx extension MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Test files using JSX syntax must have .tsx extension for TypeScript to properly parse them. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude --- .../src/hooks/{connection.test.ts => connection.test.tsx} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename packages/keychain/src/hooks/{connection.test.ts => connection.test.tsx} (100%) diff --git a/packages/keychain/src/hooks/connection.test.ts b/packages/keychain/src/hooks/connection.test.tsx similarity index 100% rename from packages/keychain/src/hooks/connection.test.ts rename to packages/keychain/src/hooks/connection.test.tsx From 60ecc1189ad865bd6cf2e531759c25bfecc2b734 Mon Sep 17 00:00:00 2001 From: Tarrence van As Date: Mon, 29 Sep 2025 17:05:53 -0500 Subject: [PATCH 4/9] fix: format test file with prettier MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Apply prettier formatting to pass CI format checks. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude --- .../keychain/src/hooks/connection.test.tsx | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/packages/keychain/src/hooks/connection.test.tsx b/packages/keychain/src/hooks/connection.test.tsx index e7b2eeac8d..60250f873f 100644 --- a/packages/keychain/src/hooks/connection.test.tsx +++ b/packages/keychain/src/hooks/connection.test.tsx @@ -9,9 +9,10 @@ const loadConfigMock = vi.fn(); const useThemeEffectMock = vi.fn(); vi.mock("@cartridge/presets", async () => { - const actual = await vi.importActual< - typeof import("@cartridge/presets") - >("@cartridge/presets"); + const actual = + await vi.importActual( + "@cartridge/presets", + ); return { ...actual, @@ -20,9 +21,8 @@ vi.mock("@cartridge/presets", async () => { }); vi.mock("@cartridge/ui", async () => { - const actual = await vi.importActual( - "@cartridge/ui", - ); + const actual = + await vi.importActual("@cartridge/ui"); return { ...actual, @@ -31,9 +31,9 @@ vi.mock("@cartridge/ui", async () => { }); vi.mock("@cartridge/ui/utils", async () => { - const actual = await vi.importActual< - typeof import("@cartridge/ui/utils") - >("@cartridge/ui/utils"); + const actual = await vi.importActual( + "@cartridge/ui/utils", + ); return { ...actual, From 94377728b558652e5d352d6449aced71b0406143 Mon Sep 17 00:00:00 2001 From: Tarrence van As Date: Mon, 29 Sep 2025 17:08:58 -0500 Subject: [PATCH 5/9] fix: format connection.ts with prettier MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Apply prettier formatting to all modified files. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude --- packages/keychain/src/hooks/connection.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/packages/keychain/src/hooks/connection.ts b/packages/keychain/src/hooks/connection.ts index d266bc2c3c..937620ba46 100644 --- a/packages/keychain/src/hooks/connection.ts +++ b/packages/keychain/src/hooks/connection.ts @@ -387,9 +387,7 @@ export function useConnectionValue() { setVerified(false); } - setConfigData( - config ? (config as Record) : null, - ); + setConfigData(config ? (config as Record) : null); }) .catch((error: Error) => { if (!isActive) return; From 5574ccdad5ffd1760a8d4f60ce0ce7edf93c68e2 Mon Sep 17 00:00:00 2001 From: Tarrence van As Date: Mon, 29 Sep 2025 17:28:04 -0500 Subject: [PATCH 6/9] fix: remove duplicate Window interface declaration MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Window interface with keychain_wallets is already declared in wallets.tsx. Declaring it again with a different type (unknown) causes a TypeScript compilation error. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude --- packages/keychain/src/hooks/connection.test.tsx | 14 +------------- 1 file changed, 1 insertion(+), 13 deletions(-) diff --git a/packages/keychain/src/hooks/connection.test.tsx b/packages/keychain/src/hooks/connection.test.tsx index 60250f873f..ec3a32c350 100644 --- a/packages/keychain/src/hooks/connection.test.tsx +++ b/packages/keychain/src/hooks/connection.test.tsx @@ -95,19 +95,7 @@ vi.mock("@/utils/connection", () => ({ })), })); -declare global { - // Extend window used within tests without importing app-level types - // eslint-disable-next-line @typescript-eslint/consistent-type-definitions - interface Window { - controller?: { - rpcUrl: () => string; - chainId: () => string; - disconnect: () => Promise | void; - username: () => string | undefined; - }; - keychain_wallets?: unknown; - } -} +// keychain_wallets type is defined in wallets.tsx, no need to redeclare it here describe("isOriginVerified", () => { const allowedOrigins = ["example.com", "*.example.com", "sub.test.com"]; From 03c4cc78b648c95d246f7cd18a3e46efdf24e409 Mon Sep 17 00:00:00 2001 From: Tarrence van As Date: Mon, 29 Sep 2025 17:32:14 -0500 Subject: [PATCH 7/9] fix: resolve vitest mock initialization issue MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Move mock variable declarations after vi.mock() calls and use vi.mocked() to get references to the mocked functions. This prevents "Cannot access before initialization" errors. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude --- packages/keychain/src/hooks/connection.test.tsx | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/packages/keychain/src/hooks/connection.test.tsx b/packages/keychain/src/hooks/connection.test.tsx index ec3a32c350..cbe1c2ffb1 100644 --- a/packages/keychain/src/hooks/connection.test.tsx +++ b/packages/keychain/src/hooks/connection.test.tsx @@ -2,12 +2,10 @@ import { ReactNode } from "react"; import { renderHook, waitFor } from "@testing-library/react"; import { MemoryRouter } from "react-router-dom"; import { vi } from "vitest"; -import { defaultTheme } from "@cartridge/presets"; +import { defaultTheme, loadConfig } from "@cartridge/presets"; +import { useThemeEffect } from "@cartridge/ui"; import { isOriginVerified, useConnectionValue } from "./connection"; -const loadConfigMock = vi.fn(); -const useThemeEffectMock = vi.fn(); - vi.mock("@cartridge/presets", async () => { const actual = await vi.importActual( @@ -16,7 +14,7 @@ vi.mock("@cartridge/presets", async () => { return { ...actual, - loadConfig: loadConfigMock, + loadConfig: vi.fn(), }; }); @@ -26,7 +24,7 @@ vi.mock("@cartridge/ui", async () => { return { ...actual, - useThemeEffect: useThemeEffectMock, + useThemeEffect: vi.fn(), }; }); @@ -97,6 +95,10 @@ vi.mock("@/utils/connection", () => ({ // keychain_wallets type is defined in wallets.tsx, no need to redeclare it here +// Get references to mocked functions +const loadConfigMock = vi.mocked(loadConfig); +const useThemeEffectMock = vi.mocked(useThemeEffect); + describe("isOriginVerified", () => { const allowedOrigins = ["example.com", "*.example.com", "sub.test.com"]; From dc171efbd7dd43103e18346382cc64745e7d34a2 Mon Sep 17 00:00:00 2001 From: Tarrence van As Date: Mon, 29 Sep 2025 17:35:13 -0500 Subject: [PATCH 8/9] fix: remove invalid theme.verified assertions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The theme object passed to useThemeEffect is a ControllerTheme, not a VerifiableControllerTheme, so it doesn't have a verified property. Remove the invalid assertions. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude --- packages/keychain/src/hooks/connection.test.tsx | 4 ---- 1 file changed, 4 deletions(-) diff --git a/packages/keychain/src/hooks/connection.test.tsx b/packages/keychain/src/hooks/connection.test.tsx index cbe1c2ffb1..f3a860bf56 100644 --- a/packages/keychain/src/hooks/connection.test.tsx +++ b/packages/keychain/src/hooks/connection.test.tsx @@ -201,7 +201,6 @@ describe("useConnectionValue", () => { const lastCall = useThemeEffectMock.mock.calls.at(-1)?.[0]; expect(lastCall?.theme.name).toBe(defaultTheme.name); - expect(lastCall?.theme.verified).toBe(true); }); it("applies the preset theme when config resolves", async () => { @@ -222,7 +221,6 @@ describe("useConnectionValue", () => { const lastCall = useThemeEffectMock.mock.calls.at(-1)?.[0]; expect(lastCall?.theme.name).toBe("Test Theme"); - expect(lastCall?.theme.verified).toBe(true); }); it("falls back to the default theme when config lacks a theme", async () => { @@ -242,7 +240,6 @@ describe("useConnectionValue", () => { const lastCall = useThemeEffectMock.mock.calls.at(-1)?.[0]; expect(lastCall?.theme.name).toBe(defaultTheme.name); - expect(lastCall?.theme.verified).toBe(true); }); it("marks the preset as unverified when config loading fails", async () => { @@ -261,6 +258,5 @@ describe("useConnectionValue", () => { const lastCall = useThemeEffectMock.mock.calls.at(-1)?.[0]; expect(lastCall?.theme.name).toBe(defaultTheme.name); - expect(lastCall?.theme.verified).toBe(true); }); }); From 152f01acb4ba4ef1e4e67b0c22a5b6df47c29d41 Mon Sep 17 00:00:00 2001 From: Tarrence van As Date: Mon, 29 Sep 2025 21:20:54 -0500 Subject: [PATCH 9/9] fix: simplify window.controller mock in tests Set window.controller to undefined instead of creating a partial mock object. This avoids TypeScript type errors while still allowing the tests to run correctly with mocked dependencies. --- packages/keychain/src/hooks/connection.test.tsx | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/packages/keychain/src/hooks/connection.test.tsx b/packages/keychain/src/hooks/connection.test.tsx index f3a860bf56..926720814f 100644 --- a/packages/keychain/src/hooks/connection.test.tsx +++ b/packages/keychain/src/hooks/connection.test.tsx @@ -178,12 +178,7 @@ describe("useConnectionValue", () => { mockGetChainId.mockReset(); mockGetChainId.mockResolvedValue("0x1"); - window.controller = { - rpcUrl: () => "https://rpc.example.com", - chainId: () => "0x534e5f534550", - disconnect: () => Promise.resolve(), - username: () => undefined, - }; + window.controller = undefined; window.keychain_wallets = undefined; });