fix(appkit-ui): stabilize useAnalyticsQuery params reference to avoid infinite refetch#321
Open
jamesbroadhead wants to merge 2 commits intomainfrom
Open
fix(appkit-ui): stabilize useAnalyticsQuery params reference to avoid infinite refetch#321jamesbroadhead wants to merge 2 commits intomainfrom
jamesbroadhead wants to merge 2 commits intomainfrom
Conversation
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 <jamesbroadhead@gmail.com>
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 <jamesbroadhead@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
useAnalyticsQuery(queryKey, params)invalidates its internal payload memo whenever theparamsobject reference changes. A typical call site looks like:{ limit: ... }is a new object literal every render, so the memo invalidates,startre-runs, the SSE connection is torn down and reopened — and on the next render the cycle repeats. Result: infinite refetch.The previous workaround was for every consumer to wrap params in
useMemo. The LLM guide even told users to "always memoize query parameters". That's a pure API tax that every team using AppKit will eventually trip over.Fix
Stabilize the
parametersreference inside the hook usinguseRef+ a shallow structural-equality check. If the new params are structurally equal to the previous render's, reuse the previous reference for downstream dependency comparisons. No public API change.Analytics query params are produced by the
sql.*builders and are always one-level objects keyed to primitives (string | number | boolean | null | undefined), so shallow equality is sufficient — and meaningfully cheaper than a full deep-equal.Options considered
Option 1 (selected): deep-compare params inside the hook.
Option 2: stable-stringify params and use the string as the internal cache key.
JSON.stringifyon every render and makes the internal cache-key indirection harder to reason about for the small-param common case.Option 1 wins on simplicity for the common case.
Changes
packages/appkit-ui/src/react/hooks/use-analytics-query.ts— addshallowEqualParams+useStableParamshelpers; routeparametersthroughuseStableParamsbefore the payload memo.packages/appkit-ui/src/react/hooks/__tests__/use-analytics-query.test.ts— new test file. Covers: structurally-equal fresh-literal params do NOT refetch, real param changes DO refetch, undefined params are stable, two empty{}literals are stable.docs/docs/development/llm-guide.md— replace the stale "always memoize query parameters" guidance with a note that params are now reference-stabilized internally.Test plan
pnpm test— all 1757 tests pass, including 4 new tests inuse-analytics-query.test.ts.pnpm check— Biome lint + format clean.pnpm -r typecheck— clean across all packages.This pull request and its description were written by Isaac.