From 75be29217699f668a767a221e846cc99d3eb7d62 Mon Sep 17 00:00:00 2001 From: ConnorYoh <40631091+ConnorYoh@users.noreply.github.com> Date: Fri, 7 Aug 2026 09:53:13 +0100 Subject: [PATCH] feat(editor): move app config onto TanStack Query (#7283) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # Description of Changes > Stacked on #7264 — review that first. This diff is against its branch. ## The problem `AppConfigContext` hand-rolled a query client: a `fetchCountRef` dedupe guard, an exponential-backoff retry loop with its own `sleep()`, a `hasResolvedConfig` flag, and manual 401/5xx branching. All to fetch one endpoint that 80 files read. ## End state Same provider, same public contract, React Query underneath. **250 lines to 142**, and no consumer file changes. | | Before | After | |---|---|---| | Dedupe | `fetchCountRef` guard | query key | | Retry | `for` loop + `sleep()` + backoff maths | `retry` + `retryDelay` | | 401 | caught in the component, sets default config | `fetchAppConfig` returns the default — the retry predicate and error state only see real failures | | Auth pages | early return inside the fetch | `enabled` | | Resolved-yet tracking | `hasResolvedConfig` state | derived from the query | `fetchAppConfig` moves to `core/api/config.ts` with the simulation hook and request options, so the context no longer knows how config is fetched. **Behaviour change:** config survives a provider remount instead of refetching. That matters on desktop, where a connection-mode switch remounts the tree — and #7264's cache reset already clears it on exactly that transition. ## Testing The existing 12-case contract test passes unchanged apart from the query wrapper. It caught a real mistake: `failureCount` is 0-based in v5, so `<= maxRetries` gave one attempt too many. Four cases added — cached remount, `maxRetries` honoured, 4xx not retried, `autoFetch` off. `task frontend:check` green: 1672 tests across 191 files, typecheck on all five flavours, eslint, dpdm, prettier. ## Coming next | PR | Scope | |---|---| | 3 | `useEndpointConfig` — core (251 lines) plus a 482-line desktop override with its own dependency polling. Split out of this PR; different risk profile, and it deserves its own review. | | 4 | `useAdminSettings` (20 consumers) and the config sections | | 5 | Polling loops → `refetchInterval` | | 6 | Finish the Processor, collapse to one client | | 7 | Tool execution — mutation state only | --------- Co-authored-by: Reece Browne <74901996+reecebrowne@users.noreply.github.com> --- frontend/editor/src/core/api/config.ts | 23 ++ .../core/contexts/AppConfigContext.test.tsx | 233 +++++++++++++++- .../src/core/contexts/AppConfigContext.tsx | 256 ++++++------------ .../core/hooks/usePosthogTracking.test.tsx | 33 ++- frontend/editor/src/core/query/keys.ts | 1 + 5 files changed, 357 insertions(+), 189 deletions(-) diff --git a/frontend/editor/src/core/api/config.ts b/frontend/editor/src/core/api/config.ts index 61f74b2330..f0ba730e8c 100644 --- a/frontend/editor/src/core/api/config.ts +++ b/frontend/editor/src/core/api/config.ts @@ -1,4 +1,27 @@ import apiClient from "@app/services/apiClient"; +import { getSimulatedAppConfig } from "@app/testing/serverExperienceSimulations"; +import type { AppConfig } from "@app/types/appConfig"; + +/** Unauthenticated and unreachable both mean "assume login is on". */ +export const DEFAULT_APP_CONFIG: AppConfig = { enableLogin: true }; + +export async function fetchAppConfig(): Promise { + const simulated = getSimulatedAppConfig(); + if (simulated) return simulated; + + try { + const response = await apiClient.get( + "/api/v1/config/app-config", + { suppressErrorToast: true, skipAuthRedirect: true }, + ); + return response.data; + } catch (error) { + // 401 is an answer, not a failure: the app runs unauthenticated. + if ((error as { response?: { status?: number } })?.response?.status === 401) + return DEFAULT_APP_CONFIG; + throw error; + } +} export interface FooterInfo { analyticsEnabled?: boolean; diff --git a/frontend/editor/src/core/contexts/AppConfigContext.test.tsx b/frontend/editor/src/core/contexts/AppConfigContext.test.tsx index 5e512ec69e..93c860d823 100644 --- a/frontend/editor/src/core/contexts/AppConfigContext.test.tsx +++ b/frontend/editor/src/core/contexts/AppConfigContext.test.tsx @@ -6,6 +6,8 @@ import { } from "@app/contexts/AppConfigContext"; import apiClient from "@app/services/apiClient"; import { allowConsole, expectConsole } from "@app/tests/failOnConsole"; +import { TestQueryProvider } from "@app/tests/utils/TestQueryProvider"; +import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; import { ReactNode } from "react"; // Mock apiClient @@ -26,7 +28,9 @@ describe("AppConfigContext", () => { }); const wrapper = ({ children }: { children: ReactNode }) => ( - {children} + + {children} + ); it("should fetch and provide app config on non-auth pages", async () => { @@ -261,9 +265,11 @@ describe("AppConfigContext", () => { }; const customWrapper = ({ children }: { children: ReactNode }) => ( - - {children} - + + + {children} + + ); const { result } = renderHook(() => useAppConfig(), { @@ -279,6 +285,225 @@ describe("AppConfigContext", () => { expect(apiClient.get).toHaveBeenCalled(); }); + it("fetches on an auth page once a JWT arrives, flipping loading", async () => { + // Signing in on /login must load the config the first-login password + // prompt reads. useOnboardingOrchestrator keys its effect on + // [config?.enableLogin, configLoading] — both primitives — so the config + // value changing is not enough. loading has to go true then false, or the + // prompt never opens. + Object.defineProperty(window, "location", { + value: { pathname: "/login" }, + writable: true, + }); + let resolveFetch: (value: unknown) => void = () => {}; + vi.mocked(apiClient.get).mockReturnValue( + new Promise((resolve) => { + resolveFetch = resolve; + }) as never, + ); + + const { result } = renderHook(() => useAppConfig(), { wrapper }); + expect(apiClient.get).not.toHaveBeenCalled(); + expect(result.current.loading).toBe(false); + + await act(async () => { + window.dispatchEvent(new CustomEvent("jwt-available")); + await new Promise((resolve) => setTimeout(resolve, 0)); + }); + + await waitFor(() => expect(result.current.loading).toBe(true)); + + await act(async () => { + resolveFetch({ status: 200, data: { enableLogin: true, isAdmin: true } }); + await new Promise((resolve) => setTimeout(resolve, 0)); + }); + + await waitFor(() => expect(result.current.loading).toBe(false)); + expect(result.current.config).toEqual({ + enableLogin: true, + isAdmin: true, + }); + }); + + it("flips loading after sign-in even when a pre-login 401 already cached a config", async () => { + // The live sequence: the app loads on "/" while logged out, app-config 401s + // and resolves to the login-enabled default, then the user signs in on + // /login. That cached default means isPending is already false, so loading + // has to track isFetching or useOnboardingOrchestrator — whose effect deps + // are [config?.enableLogin, configLoading], both unchanged here — never + // re-runs, and the first-login password prompt never opens. + const client = new QueryClient({ + defaultOptions: { queries: { retry: false } }, + }); + const sharedWrapper = ({ children }: { children: ReactNode }) => ( + + {children} + + ); + + // Logged out on a non-auth page: 401 → default config, cached. + vi.mocked(apiClient.get).mockRejectedValueOnce( + Object.assign(new Error("Unauthorized"), { + response: { status: 401, data: {} }, + }), + ); + const loggedOut = renderHook(() => useAppConfig(), { + wrapper: sharedWrapper, + }); + await waitFor(() => + expect(loggedOut.result.current.config).toEqual({ enableLogin: true }), + ); + expect(loggedOut.result.current.loading).toBe(false); + loggedOut.unmount(); + + // Now on /login, with that default still cached. + Object.defineProperty(window, "location", { + value: { pathname: "/login" }, + writable: true, + }); + let resolveFetch: (value: unknown) => void = () => {}; + vi.mocked(apiClient.get).mockReturnValue( + new Promise((resolve) => { + resolveFetch = resolve; + }) as never, + ); + + const { result } = renderHook(() => useAppConfig(), { + wrapper: sharedWrapper, + }); + expect(result.current.loading).toBe(false); + + await act(async () => { + window.dispatchEvent(new CustomEvent("jwt-available")); + await new Promise((resolve) => setTimeout(resolve, 0)); + }); + + await waitFor(() => expect(result.current.loading).toBe(true)); + + await act(async () => { + resolveFetch({ + status: 200, + data: { enableLogin: true, isAdmin: true }, + }); + await new Promise((resolve) => setTimeout(resolve, 0)); + }); + + await waitFor(() => expect(result.current.loading).toBe(false)); + expect(result.current.config).toEqual({ + enableLogin: true, + isAdmin: true, + }); + }); + + it("serves a remounted provider from cache", async () => { + vi.mocked(apiClient.get).mockResolvedValue({ + status: 200, + data: { enableLogin: false }, + } as any); + + const client = new QueryClient({ + defaultOptions: { queries: { retry: false } }, + }); + const sharedWrapper = ({ children }: { children: ReactNode }) => ( + + {children} + + ); + + const first = renderHook(() => useAppConfig(), { wrapper: sharedWrapper }); + await waitFor(() => expect(first.result.current.loading).toBe(false)); + first.unmount(); + + const second = renderHook(() => useAppConfig(), { wrapper: sharedWrapper }); + // Cached, so no loading flash and no second request. + expect(second.result.current.loading).toBe(false); + expect(second.result.current.config).toEqual({ enableLogin: false }); + expect(apiClient.get).toHaveBeenCalledTimes(1); + }); + + it("honours maxRetries for network failures", async () => { + expectConsole.error(/\[AppConfig\] Failed to fetch app config/); + vi.mocked(apiClient.get).mockRejectedValue(new Error("boom")); + + const retryWrapper = ({ children }: { children: ReactNode }) => ( + + + {children} + + + ); + + const { result } = renderHook(() => useAppConfig(), { + wrapper: retryWrapper, + }); + + await waitFor(() => expect(result.current.error).toBe("boom")); + expect(apiClient.get).toHaveBeenCalledTimes(3); + }); + + it("does not retry a 4xx", async () => { + expectConsole.error(/\[AppConfig\] Failed to fetch app config/); + vi.mocked(apiClient.get).mockRejectedValue( + Object.assign(new Error("nope"), { response: { status: 403, data: {} } }), + ); + + const retryWrapper = ({ children }: { children: ReactNode }) => ( + + + {children} + + + ); + + const { result } = renderHook(() => useAppConfig(), { + wrapper: retryWrapper, + }); + + await waitFor(() => expect(result.current.error).toBe("nope")); + expect(apiClient.get).toHaveBeenCalledTimes(1); + }); + + it("stays loading when autoFetch is off and nothing seeds a config", async () => { + // Unresolved, not in-flight. Matches the pre-migration provider, which + // seeded loading from !hasResolvedConfig and never cleared it without a + // fetch. Reporting false here would tell consumers a null config is final. + const offWrapper = ({ children }: { children: ReactNode }) => ( + + {children} + + ); + + const { result } = renderHook(() => useAppConfig(), { + wrapper: offWrapper, + }); + + expect(result.current.loading).toBe(true); + expect(result.current.config).toBeNull(); + expect(apiClient.get).not.toHaveBeenCalled(); + }); + + it("does not fetch when autoFetch is off", async () => { + const offWrapper = ({ children }: { children: ReactNode }) => ( + + + {children} + + + ); + + const { result } = renderHook(() => useAppConfig(), { + wrapper: offWrapper, + }); + + expect(result.current.loading).toBe(false); + expect(result.current.config).toEqual({ enableLogin: false }); + expect(apiClient.get).not.toHaveBeenCalled(); + }); + it("should use suppressErrorToast for all config requests", async () => { const mockConfig = { enableLogin: true }; diff --git a/frontend/editor/src/core/contexts/AppConfigContext.tsx b/frontend/editor/src/core/contexts/AppConfigContext.tsx index 3672f6cdfa..112ca92bb8 100644 --- a/frontend/editor/src/core/contexts/AppConfigContext.tsx +++ b/frontend/editor/src/core/contexts/AppConfigContext.tsx @@ -2,23 +2,18 @@ import React, { createContext, useContext, useMemo, - useState, useEffect, - ReactNode, useCallback, + useState, + ReactNode, } from "react"; -import apiClient from "@app/services/apiClient"; -import { getSimulatedAppConfig } from "@app/testing/serverExperienceSimulations"; +import { useQuery, useQueryClient } from "@tanstack/react-query"; +import { DEFAULT_APP_CONFIG, fetchAppConfig } from "@app/api/config"; +import { qk } from "@app/query/keys"; +import { CONFIG_STALE_TIME } from "@app/query/staleTime"; import type { AppConfig, AppConfigBootstrapMode } from "@app/types/appConfig"; import { useJwtConfigSync } from "@app/hooks/useJwtConfigSync"; -/** - * Sleep utility for delays - */ -function sleep(ms: number): Promise { - return new Promise((resolve) => setTimeout(resolve, ms)); -} - export interface AppConfigRetryOptions { maxRetries?: number; initialDelay?: number; @@ -40,10 +35,6 @@ const AppConfigContext = createContext({ refetch: async () => {}, }); -/** - * Provider component that fetches and provides app configuration - * Should be placed at the top level of the app, before any components that need config - */ export interface AppConfigProviderProps { children: ReactNode; retryOptions?: AppConfigRetryOptions; @@ -53,6 +44,22 @@ export interface AppConfigProviderProps { onConfigLoaded?: (config: AppConfig) => void; } +function statusOf(error: unknown): number | undefined { + return (error as { response?: { status?: number } })?.response?.status; +} + +function errorMessage(error: unknown): string { + const axiosLike = error as { + response?: { data?: { message?: string } }; + message?: string; + }; + return ( + axiosLike?.response?.data?.message || + axiosLike?.message || + "Unknown error occurred" + ); +} + export const AppConfigProvider: React.FC = ({ children, retryOptions, @@ -61,171 +68,84 @@ export const AppConfigProvider: React.FC = ({ autoFetch = true, onConfigLoaded, }) => { - const isBlockingMode = bootstrapMode === "blocking"; - const [config, setConfig] = useState(initialConfig); - const [error, setError] = useState(null); - // Track how many times we've attempted to fetch. useRef avoids re-renders that can trigger loops. - const fetchCountRef = React.useRef(0); - const [hasResolvedConfig, setHasResolvedConfig] = useState( - Boolean(initialConfig) && !isBlockingMode, - ); - const [loading, setLoading] = useState(!hasResolvedConfig); + const maxRetries = retryOptions?.maxRetries ?? 0; + const initialDelay = retryOptions?.initialDelay ?? 1000; + // Non-blocking mode treats initialConfig as good enough to render on. + const seeded = Boolean(initialConfig) && bootstrapMode !== "blocking"; const onConfigLoadedRef = React.useRef(onConfigLoaded); onConfigLoadedRef.current = onConfigLoaded; - const maxRetries = retryOptions?.maxRetries ?? 0; - const initialDelay = retryOptions?.initialDelay ?? 1000; + const queryClient = useQueryClient(); + // Auth pages skip the fetch until sign-in asks for one. + const [signedIn, setSignedIn] = useState(false); + // fetchQuery, not refetchQueries: the latter skips a disabled query, and + // enabling one whose cache is still fresh doesn't fetch either. Sign-in has + // to force the request — a pre-login 401 leaves a cached default behind. + const refetch = useCallback(async () => { + setSignedIn(true); + await queryClient.fetchQuery({ + queryKey: qk.appConfig(), + queryFn: fetchAppConfig, + staleTime: 0, + }); + }, [queryClient]); - const fetchConfig = useCallback( - async (force = false) => { - // Prevent duplicate fetches unless forced - if (!force && fetchCountRef.current > 0) { - console.debug("[AppConfig] Already fetched, skipping"); - return; - } + const { isAuthPage } = useJwtConfigSync(refetch); + const fetching = autoFetch && (!isAuthPage || signedIn); - // Mark that we've attempted a fetch to prevent repeated auto-fetch loops - fetchCountRef.current += 1; - - const shouldBlockUI = !hasResolvedConfig || isBlockingMode; - if (shouldBlockUI) { - setLoading(true); - } - setError(null); - - const startTime = performance.now(); - for (let attempt = 0; attempt <= maxRetries; attempt++) { - try { - const testConfig = getSimulatedAppConfig(); - if (testConfig) { - setConfig(testConfig); - setHasResolvedConfig(true); - setLoading(false); - return; - } - - if (attempt > 0) { - const delay = initialDelay * Math.pow(2, attempt - 1); - console.debug( - `[AppConfig] Retry attempt ${attempt}/${maxRetries} after ${delay}ms delay...`, - ); - await sleep(delay); - } - - // apiClient automatically adds JWT header if available via interceptors - // Always suppress error toast - we handle 401 errors locally - console.debug("[AppConfig] Fetching app config", { - attempt, - force, - path: window.location.pathname, - }); - const response = await apiClient.get( - "/api/v1/config/app-config", - { - suppressErrorToast: true, - skipAuthRedirect: true, - } as any, - ); - const data = response.data; - - console.debug("[AppConfig] Config fetched successfully:", data); - console.debug( - "[AppConfig] Fetch duration ms:", - (performance.now() - startTime).toFixed(2), - ); - setConfig(data); - setHasResolvedConfig(true); - setLoading(false); - onConfigLoadedRef.current?.(data); - return; // Success - exit function - } catch (err: any) { - const status = err?.response?.status; - - // On 401 (not authenticated), use default config with login enabled - // This allows the app to work even without authentication - if (status === 401) { - console.debug( - "[AppConfig] 401 error - using default config (login enabled)", - ); - console.debug( - "[AppConfig] Fetch duration ms:", - (performance.now() - startTime).toFixed(2), - ); - setConfig({ enableLogin: true }); - setHasResolvedConfig(true); - setLoading(false); - return; - } - - // Check if we should retry (network errors or 5xx errors) - const shouldRetry = - (!status || status >= 500) && attempt < maxRetries; - - if (shouldRetry) { - console.debug( - `[AppConfig] Attempt ${attempt + 1} failed (status ${status || "network error"}):`, - err.message, - "- will retry...", - ); - continue; - } - - // Final attempt failed or non-retryable error (4xx) - const errorMessage = - err?.response?.data?.message || - err?.message || - "Unknown error occurred"; - setError(errorMessage); - console.error( - `[AppConfig] Failed to fetch app config after ${attempt + 1} attempts:`, - err, - ); - console.debug( - "[AppConfig] Fetch duration ms:", - (performance.now() - startTime).toFixed(2), - ); - // Preserve existing config (initial default or previous fetch). If nothing is set, assume login enabled. - setConfig((current) => current ?? { enableLogin: true }); - setHasResolvedConfig(true); - break; - } - } - - setLoading(false); + const { data, error, isFetching } = useQuery({ + queryKey: qk.appConfig(), + queryFn: fetchAppConfig, + enabled: fetching, + staleTime: CONFIG_STALE_TIME, + // Network and 5xx only; failureCount is 0-based, so `<` gives maxRetries retries. + retry: (failureCount, err) => { + const status = statusOf(err); + return (!status || status >= 500) && failureCount < maxRetries; }, - [hasResolvedConfig, isBlockingMode, maxRetries, initialDelay], - ); - - const { isAuthPage } = useJwtConfigSync(fetchConfig); + retryDelay: (attempt) => initialDelay * 2 ** attempt, + }); useEffect(() => { - if (isAuthPage) { - console.debug( - "[AppConfig] On auth page - using default config, skipping fetch", - { path: window.location.pathname }, - ); - setConfig({ enableLogin: true }); - setHasResolvedConfig(true); - setLoading(false); - return; - } + if (error) console.error("[AppConfig] Failed to fetch app config:", error); + }, [error]); - if (autoFetch) { - fetchConfig(); - } - }, [autoFetch, fetchConfig, isAuthPage]); - - const refetch = useCallback(() => fetchConfig(true), [fetchConfig]); + useEffect(() => { + if (data) onConfigLoadedRef.current?.(data); + }, [data]); const value = useMemo( () => ({ - config, - loading, - error, + config: + data ?? + initialConfig ?? + (isAuthPage || error ? DEFAULT_APP_CONFIG : null), + // "Config not settled yet": in flight, or never going to be fetched at + // all. isFetching rather than isPending because a pre-login 401 resolves + // to the default config, so isPending is already false by the time the + // post-sign-in fetch runs — and consumers key effects on the flip. + loading: seeded + ? false + : !autoFetch + ? true + : fetching + ? isFetching + : false, + error: error ? errorMessage(error) : null, refetch, }), - [config, loading, error, refetch], + [ + data, + error, + isFetching, + initialConfig, + isAuthPage, + seeded, + autoFetch, + fetching, + refetch, + ], ); return ( @@ -235,16 +155,10 @@ export const AppConfigProvider: React.FC = ({ ); }; -/** - * Hook to access application configuration - * Must be used within AppConfigProvider - */ export function useAppConfig(): AppConfigContextValue { const context = useContext(AppConfigContext); - if (context === undefined) { throw new Error("useAppConfig must be used within AppConfigProvider"); } - return context; } diff --git a/frontend/editor/src/core/hooks/usePosthogTracking.test.tsx b/frontend/editor/src/core/hooks/usePosthogTracking.test.tsx index 41f5fad7dc..b2e88a0ddd 100644 --- a/frontend/editor/src/core/hooks/usePosthogTracking.test.tsx +++ b/frontend/editor/src/core/hooks/usePosthogTracking.test.tsx @@ -2,6 +2,7 @@ import { ReactNode } from "react"; import { renderHook, waitFor } from "@testing-library/react"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { AppConfigProvider } from "@app/contexts/AppConfigContext"; +import { TestQueryProvider } from "@app/tests/utils/TestQueryProvider"; const posthogState = vi.hoisted(() => ({ loaded: false })); const posthogMock = vi.hoisted(() => ({ @@ -40,13 +41,15 @@ describe("usePosthogTracking", () => { it("does not initialize PostHog when analytics is disabled", async () => { const wrapper = ({ children }: { children: ReactNode }) => ( - - {children} - + + + {children} + + ); renderHook(() => usePosthogTracking(), { wrapper }); @@ -58,13 +61,15 @@ describe("usePosthogTracking", () => { it("initializes PostHog when analytics is enabled", async () => { const wrapper = ({ children }: { children: ReactNode }) => ( - - {children} - + + + {children} + + ); renderHook(() => usePosthogTracking(), { wrapper }); diff --git a/frontend/editor/src/core/query/keys.ts b/frontend/editor/src/core/query/keys.ts index 84a0206c8f..6c6bacd552 100644 --- a/frontend/editor/src/core/query/keys.ts +++ b/frontend/editor/src/core/query/keys.ts @@ -1,5 +1,6 @@ /** Editor query keys: ["editor", , ...params]. */ export const qk = { + appConfig: () => ["editor", "appConfig"] as const, footerInfo: () => ["editor", "footerInfo"] as const, groupEnabled: (group: string) => ["editor", "groupEnabled", group] as const, users: () => ["editor", "users"] as const,