Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/docs/development/llm-guide.md
Original file line number Diff line number Diff line change
Expand Up @@ -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`.
Expand Down
Original file line number Diff line number Diff line change
@@ -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<void>(() => {});
});

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);
});
});
62 changes: 60 additions & 2 deletions packages/appkit-ui/src/react/hooks/use-analytics-query.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<string, unknown>);
const bKeys = Object.keys(b as Record<string, unknown>);
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<string, unknown>)[key],
(b as Record<string, unknown>)[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<T>(value: T): T {
const ref = useRef<T>(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;
Expand Down Expand Up @@ -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(
Expand All @@ -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) {
Expand Down
2 changes: 1 addition & 1 deletion packages/appkit-ui/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
"compilerOptions": {
"outDir": "dist",
"jsx": "react-jsx",
"lib": ["ES2021", "DOM"],
"lib": ["ES2022", "DOM"],
"baseUrl": ".",
"paths": {
"@/*": ["./src/*"],
Expand Down
Loading