Skip to content

ref(ui): do not disable eslint-plugin-query for debouncing #103141

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+10 −15
Open

ref(ui): do not disable eslint-plugin-query for debouncing #103141

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9eb63c8
ref(ui): do not disable eslint-plugin-query for debouncing
TkDodo Nov 11, 2025
18e057d
Merge branch 'master' into tkdodo/ref/no-disable-eslint-plugin-query
TkDodo Nov 11, 2025
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
8 changes: 3 additions & 5 deletions static/app/components/searchQueryBuilder/tokens/filter/valueCombobox.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -365,18 +365,16 @@ function useFilterSuggestions({
);

const baseQueryKey = useMemo(
() => ['search-query-builder-tag-values', queryParams],
() => ['search-query-builder-tag-values', queryParams] as const,
[queryParams]
);
const queryKey = useDebouncedValue(baseQueryKey);
const isDebouncing = baseQueryKey !== queryKey;

// TODO(malwilley): Display error states
const {data, isFetching} = useQuery<string[]>({
// disable exhaustive deps because we want to debounce the query key above
// eslint-disable-next-line @tanstack/query/exhaustive-deps
const {data, isFetching} = useQuery({
queryKey,
queryFn: () => getTagValues(...queryParams),
queryFn: ctx => getTagValues(...ctx.queryKey[1]),
Comment on lines -368 to +377
Copy link
Collaborator Author

@TkDodo TkDodo Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here, we need the debounced key to calculate isDebouncing, so we can’t inline it. What we can do though is not rely on closures for the key, but use the provided queryFunctionContext: React Query gives you the QueryKey into the queryFn so that you can build self-contained functions.

To do this, we need to make sure types flow through correctly so we need:

  • as const on the actual key, otherwise it’s not a tuple
  • remove useQuery<string[]> to just useQuery. using <> is a type assertion in disguise and destroys type inference for the other 3 type parameters useQuery has, so please never do this unless absolutely necessary. Here, getTagValues already returns a Promise<string>, so type inference works nicely and with that, we also get type inference back for the queryFunctionContext. That means we can now access queryParams as ctx.queryKey[1].

placeholderData: keepPreviousData,
enabled: shouldFetchValues,
});
Expand Down
17 changes: 7 additions & 10 deletions static/app/views/dashboards/globalFilter/filterSelector.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,16 +83,13 @@ function FilterSelector({
? !fullTag.predefined && predefinedValues === null
: true;

const baseQueryKey = useMemo(
() => ['global-dashboard-filters-tag-values', tag, selection, searchQuery],
[tag, selection, searchQuery]
);
const queryKey = useDebouncedValue(baseQueryKey);

const queryResult = useQuery<string[]>({
// Disable exhaustive deps because we want to debounce the query key above
// eslint-disable-next-line @tanstack/query/exhaustive-deps
queryKey,
const queryResult = useQuery({
queryKey: useDebouncedValue(
useMemo(
() => ['global-dashboard-filters-tag-values', tag, selection, searchQuery],
[tag, selection, searchQuery]
)
),
Comment on lines +86 to +92
Copy link
Collaborator Author

@TkDodo TkDodo Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a limitation of the eslint plugin - it doesn’t track the usage through multiple hooks. The easiest way is to just inline the hook calls, which is fine - it doesn’t violate the rules of hooks as they are all still called top-level.

we can do this if we don’t need the debounced intermediate value anywhere else.

Copy link
Contributor

@cursor cursor bot Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Wrong Key, Wrong Data Cache Mismatch

The queryFn uses the non-debounced searchQuery from closure while the queryKey uses the debounced value. This mismatch causes React Query to cache results under the wrong key - the query executes with the current searchQuery value but gets stored under a key containing the debounced value, potentially returning stale or incorrect cached data.

Fix in Cursor Fix in Web

Copy link
Collaborator Author

@TkDodo TkDodo Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean yeah, but wasn’t this the case before, too?

queryFn: async () => {
const result = await searchBarData.getTagValues(tag, searchQuery);
return result ?? [];
Expand Down
Loading