From fa5e24eb860d591ae5da6aa8333097ad51cc1e84 Mon Sep 17 00:00:00 2001 From: James Broadhead Date: Tue, 28 Apr 2026 17:59:06 +0000 Subject: [PATCH 1/2] fix(appkit-ui): stabilize useAnalyticsQuery params reference MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit useAnalyticsQuery treated `parameters` as a useMemo dep by reference. A typical call site passes a fresh object literal every render (`useAnalyticsQuery("k", { limit: sql.number(10) })`), which invalidated the payload memo and re-ran `start` -> infinite refetch. The workaround was for every consumer to wrap params in `useMemo` — a recurring footgun. Stabilize the params reference inside the hook using a useRef + shallow structural equality check. Analytics query params are produced by the `sql.*` builders and are always 1-level objects keyed to primitives, so shallow equality is sufficient and substantially cheaper than a full deep-equal. No public API change. Also considered: stable-stringify + cache-keying on the string. Rejected because params are tiny, structural equality is what users actually want, and TanStack Query / SWR ship the same approach. Updates the LLM guide which previously told consumers to always memoize params. Co-authored-by: Isaac Signed-off-by: James Broadhead --- docs/docs/development/llm-guide.md | 2 +- .../__tests__/use-analytics-query.test.ts | 93 +++++++++++++++++++ .../src/react/hooks/use-analytics-query.ts | 62 ++++++++++++- 3 files changed, 154 insertions(+), 3 deletions(-) create mode 100644 packages/appkit-ui/src/react/hooks/__tests__/use-analytics-query.test.ts diff --git a/docs/docs/development/llm-guide.md b/docs/docs/development/llm-guide.md index dec2f6815..aa1a3945d 100644 --- a/docs/docs/development/llm-guide.md +++ b/docs/docs/development/llm-guide.md @@ -33,7 +33,7 @@ This guide is designed to work even when you *do not* have access to the AppKit - **Do not invent APIs**. If unsure, stick to the patterns shown in the documentation and only use documented exports from `@databricks/appkit` and `@databricks/appkit-ui`. - **`createApp()` is async**. Prefer **top-level `await createApp(...)`**. If you can't, use `void createApp(...)` and do not ignore promise rejection. -- **Always memoize query parameters** passed to `useAnalyticsQuery` / charts to avoid refetch loops. +- **Query parameters are reference-stabilized** inside `useAnalyticsQuery` / charts via shallow structural equality, so passing a fresh object literal each render is safe and will not cause refetch loops. Manual `useMemo` wrapping is no longer required. - **Always handle loading/error/empty states** in UI (use `Skeleton`, error text, empty state). - **Always use `sql.*` helpers** for query parameters (do not pass raw strings/numbers unless the query expects none). - **Never construct SQL strings dynamically**. Use parameterized queries with `:paramName`. diff --git a/packages/appkit-ui/src/react/hooks/__tests__/use-analytics-query.test.ts b/packages/appkit-ui/src/react/hooks/__tests__/use-analytics-query.test.ts new file mode 100644 index 000000000..cfe5d6ce9 --- /dev/null +++ b/packages/appkit-ui/src/react/hooks/__tests__/use-analytics-query.test.ts @@ -0,0 +1,93 @@ +import { renderHook } from "@testing-library/react"; +import { afterEach, describe, expect, test, vi } from "vitest"; + +// Mock connectSSE so the hook does not attempt a real network request. +const mockConnectSSE = vi.fn().mockImplementation((_opts: unknown) => { + // Return a never-resolving promise; tests don't need the result. + return new Promise(() => {}); +}); + +vi.mock("@/js", () => ({ + ArrowClient: { + fetchArrow: vi.fn(), + processArrowBuffer: vi.fn(), + }, + connectSSE: (...args: unknown[]) => mockConnectSSE(...args), +})); + +// Stub useQueryHMR so we don't pull in import.meta.hot wiring. +vi.mock("../use-query-hmr", () => ({ + useQueryHMR: () => {}, +})); + +import { useAnalyticsQuery } from "../use-analytics-query"; + +describe("useAnalyticsQuery", () => { + afterEach(() => { + vi.clearAllMocks(); + }); + + test("does not refetch when params object is structurally equal across renders", () => { + // Each render passes a fresh object literal — the common footgun. + const { rerender } = renderHook( + ({ limit }: { limit: number }) => + // biome-ignore lint/suspicious/noExplicitAny: typed registry not available in tests + useAnalyticsQuery("test_query" as any, { limit } as any), + { initialProps: { limit: 10 } }, + ); + + // Initial render triggers exactly one connection. + expect(mockConnectSSE).toHaveBeenCalledTimes(1); + + // Re-render with structurally-equal-but-new-reference params. + rerender({ limit: 10 }); + rerender({ limit: 10 }); + rerender({ limit: 10 }); + + // Should NOT have refetched — the hook stabilized the params reference. + expect(mockConnectSSE).toHaveBeenCalledTimes(1); + }); + + test("does refetch when a param value actually changes", () => { + const { rerender } = renderHook( + ({ limit }: { limit: number }) => + // biome-ignore lint/suspicious/noExplicitAny: typed registry not available in tests + useAnalyticsQuery("test_query" as any, { limit } as any), + { initialProps: { limit: 10 } }, + ); + + expect(mockConnectSSE).toHaveBeenCalledTimes(1); + + rerender({ limit: 20 }); + + expect(mockConnectSSE).toHaveBeenCalledTimes(2); + }); + + test("does not refetch when params is undefined across renders", () => { + const { rerender } = renderHook(() => + // biome-ignore lint/suspicious/noExplicitAny: typed registry not available in tests + useAnalyticsQuery("test_query" as any), + ); + + expect(mockConnectSSE).toHaveBeenCalledTimes(1); + + rerender(); + rerender(); + + expect(mockConnectSSE).toHaveBeenCalledTimes(1); + }); + + test("treats two empty object literals as equal", () => { + const { rerender } = renderHook(() => + // biome-ignore lint/suspicious/noExplicitAny: typed registry not available in tests + useAnalyticsQuery("test_query" as any, {} as any), + ); + + expect(mockConnectSSE).toHaveBeenCalledTimes(1); + + rerender(); + rerender(); + + expect(mockConnectSSE).toHaveBeenCalledTimes(1); + }); +}); diff --git a/packages/appkit-ui/src/react/hooks/use-analytics-query.ts b/packages/appkit-ui/src/react/hooks/use-analytics-query.ts index 24e03ea3b..7ba2e788c 100644 --- a/packages/appkit-ui/src/react/hooks/use-analytics-query.ts +++ b/packages/appkit-ui/src/react/hooks/use-analytics-query.ts @@ -10,6 +10,55 @@ import type { } from "./types"; import { useQueryHMR } from "./use-query-hmr"; +/** + * Shallow structural equality for analytics query parameter objects. + * + * Analytics query parameters are produced by the `sql.*` builders and are + * always plain objects keyed to primitive values (string | number | boolean + * | null | undefined), so shallow equality is sufficient and substantially + * cheaper than a full deep-equal. + */ +function shallowEqualParams(a: unknown, b: unknown): boolean { + if (Object.is(a, b)) return true; + if ( + a === null || + b === null || + typeof a !== "object" || + typeof b !== "object" + ) { + return false; + } + const aKeys = Object.keys(a as Record); + const bKeys = Object.keys(b as Record); + if (aKeys.length !== bKeys.length) return false; + for (const key of aKeys) { + if (!Object.hasOwn(b, key)) return false; + if ( + !Object.is( + (a as Record)[key], + (b as Record)[key], + ) + ) { + return false; + } + } + return true; +} + +/** + * Stabilize a value's identity across renders when it is structurally equal + * to the previous value. Used to make object-literal parameters safe to pass + * directly to `useAnalyticsQuery` without forcing every consumer to wrap + * params in `useMemo`. + */ +function useStableParams(value: T): T { + const ref = useRef(value); + if (!shallowEqualParams(ref.current, value)) { + ref.current = value; + } + return ref.current; +} + function getDevMode() { const url = new URL(window.location.href); const searchParams = url.searchParams; @@ -79,9 +128,18 @@ export function useAnalyticsQuery< ); } + // Stabilize the parameters reference across renders. Without this, a fresh + // object literal at the call site (e.g. `useAnalyticsQuery("k", { limit: 10 })`) + // would change identity every render, invalidating the `payload` memo and + // re-running `start` -> infinite refetch loop. + const stableParameters = useStableParams(parameters); + const payload = useMemo(() => { try { - const serialized = JSON.stringify({ parameters, format }); + const serialized = JSON.stringify({ + parameters: stableParameters, + format, + }); const sizeInBytes = new Blob([serialized]).size; if (sizeInBytes > maxParametersSize) { throw new Error( @@ -94,7 +152,7 @@ export function useAnalyticsQuery< console.error("useAnalyticsQuery: Failed to serialize parameters", error); return null; } - }, [parameters, format, maxParametersSize]); + }, [stableParameters, format, maxParametersSize]); const start = useCallback(() => { if (payload === null) { From 6f13f1b41963be7e7bc0da07bf2c2a69f37dbf43 Mon Sep 17 00:00:00 2001 From: James Broadhead Date: Wed, 29 Apr 2026 17:06:38 +0000 Subject: [PATCH 2/2] fix(appkit-ui): bump tsconfig lib from ES2021 to ES2022 The override added DOM lib but accidentally downleveled inherited ES2022 from the root tsconfig, breaking Object.hasOwn (used in the shallowEqualParams helper). Signed-off-by: James Broadhead --- packages/appkit-ui/tsconfig.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/appkit-ui/tsconfig.json b/packages/appkit-ui/tsconfig.json index 3d33f3634..f7f3555f6 100644 --- a/packages/appkit-ui/tsconfig.json +++ b/packages/appkit-ui/tsconfig.json @@ -3,7 +3,7 @@ "compilerOptions": { "outDir": "dist", "jsx": "react-jsx", - "lib": ["ES2021", "DOM"], + "lib": ["ES2022", "DOM"], "baseUrl": ".", "paths": { "@/*": ["./src/*"],