Skip to content

ref(feedback): Refactor feedback query keys#112552

Open
ryan953 wants to merge 5 commits intomasterfrom
ryan953/ref-feedback-query-keys
Open

ref(feedback): Refactor feedback query keys#112552
ryan953 wants to merge 5 commits intomasterfrom
ryan953/ref-feedback-query-keys

Conversation

@ryan953
Copy link
Copy Markdown
Member

@ryan953 ryan953 commented Apr 8, 2026

We can call apiOptions now and to more easily get what we need to make infinite items requests for the feedback list, and the call to see if there are new items for the top of the list.

This PR removes listQueryKey and listPrefetchQueryKey from the <FeedbackQueryKeysProvider> because we can just grab the options/keys on demand where we need them.
I've also switched over to useQuery and useInfiniteQuery calls, along with updating the cache entries.

The cache management part is the hardest to update. There's lots of generics when we use the raw queryClient.* methods, so types aren't a reliable safeguard imo

@ryan953 ryan953 requested review from a team as code owners April 8, 2026 22:57
@ryan953 ryan953 requested a review from TkDodo April 8, 2026 22:57
@github-actions github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Apr 8, 2026
Comment thread static/app/components/feedback/useFeedbackCache.tsx
Comment on lines +23 to +27
const {data} = useQuery({
...listPrefetchQueryOptions,
refetchInterval: POLLING_INTERVAL_MS,
enabled: !foundData,
});
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The idea here is that we have stored a timestamp listHeadTime into react context when we first load the page and start the infinite list going.
We can scroll down the list and load more older items incrementally. As we scroll we get things older and older.

That listHeadTime is used to fetch and see if there are any items that are newer, something appeared since we loaded the page. We're fetching with a timerange like between now(aka payload) and tomorrow. Then if we find something we can clear caches, and reset the top of the list to an updated timestamp.

Copy link
Copy Markdown
Contributor

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 2 potential issues.

Autofix Details

Bugbot Autofix prepared fixes for both issues found in the latest run.

  • ✅ Fixed: Prefetch query missing limit: 1 constraint
    • Added limit: 1 to the prefetch return value to ensure only one item is fetched for polling instead of a full page.
  • ✅ Fixed: Optional chaining makes loadedRows potentially undefined
    • Removed optional chaining from deduplicateItems call since it's a required prop, preventing potential runtime crashes on loadedRows.length access.

Create PR

Or push these changes by commenting:

@cursor push 490af4b10c
Preview (490af4b10c)
diff --git a/static/app/components/infiniteList/infiniteListItems.tsx b/static/app/components/infiniteList/infiniteListItems.tsx
--- a/static/app/components/infiniteList/infiniteListItems.tsx
+++ b/static/app/components/infiniteList/infiniteListItems.tsx
@@ -54,7 +54,7 @@
   queryResult,
 }: Props<ListItem, Response>) {
   const {data, hasNextPage, isFetchingNextPage, fetchNextPage} = queryResult;
-  const loadedRows = deduplicateItems?.(data?.pages ?? []);
+  const loadedRows = deduplicateItems(data?.pages ?? []);
   const parentRef = useRef<HTMLDivElement>(null);
 
   const rowVirtualizer = useVirtualizer({

diff --git a/static/app/utils/feedback/coaleseIssueStatsPeriodQuery.tsx b/static/app/utils/feedback/coaleseIssueStatsPeriodQuery.tsx
--- a/static/app/utils/feedback/coaleseIssueStatsPeriodQuery.tsx
+++ b/static/app/utils/feedback/coaleseIssueStatsPeriodQuery.tsx
@@ -38,7 +38,7 @@
     const intervalMS = ONE_DAY_MS;
     const start = new Date(listHeadTime).toISOString();
     const end = new Date(listHeadTime + intervalMS).toISOString();
-    return statsPeriod ? {start, end, statsPeriod: undefined} : undefined;
+    return statsPeriod ? {limit: 1, start, end, statsPeriod: undefined} : undefined;
   }
 
   const intervalMS = intervalToMilliseconds(statsPeriod ?? '');

This Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.

Comment thread static/app/utils/feedback/coaleseIssueStatsPeriodQuery.tsx
Comment thread static/app/components/infiniteList/infiniteListItems.tsx Outdated
ryan953 and others added 2 commits April 8, 2026 16:18
The predicate incorrectly cast query.state.data as QueryState<...> and
accessed .data?.pages, but query.state.data is already the resolved
InfiniteData — so the extra .data dereference always yielded undefined,
silently skipping refetch for specific feedback IDs. Additionally, each
page is ApiResponse<T> ({json, headers}), not a raw array, so calling
.some() directly on the page object would throw a TypeError if the
first bug were fixed independently.

Co-Authored-By: Claude Sonnet 4 <noreply@example.com>
Made-with: Cursor
Copy link
Copy Markdown
Contributor

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 3 potential issues.

There are 4 total unresolved issues (including 1 from previous review).

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 44e0719. Configure here.

Comment thread static/app/utils/feedback/coaleseIssueStatsPeriodQuery.tsx
Comment thread static/app/components/feedback/list/useMailboxCounts.tsx Outdated
Comment thread static/app/components/feedback/useFeedbackHasNewItems.tsx Outdated
Comment on lines +24 to +29
const {statsPeriod} =
parseQueryKey(listPrefetchQueryOptions.queryKey).options?.query ?? {};
const {data} = useQuery({
...listPrefetchQueryOptions,
refetchInterval: POLLING_INTERVAL_MS,
enabled: statsPeriod && !foundData,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Bug: The enabled condition for the useFeedbackHasNewItems hook always evaluates to false because it checks for statsPeriod in the query key, which is stripped out during key construction.
Severity: HIGH

Suggested Fix

The logic for the enabled flag should be changed. Instead of checking for statsPeriod within the parsed query key, which will always be undefined, the logic should revert to a check similar to the previous implementation. For example, check if listPrefetchQueryKey is defined, as it will only be undefined for absolute date ranges where polling is not intended. This will correctly enable polling for relative date ranges.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: static/app/components/feedback/useFeedbackHasNewItems.tsx#L24-L29

Potential issue: A refactoring in `useFeedbackHasNewItems` introduced a logic error that
disables polling for new feedback items. The `enabled` condition now checks for the
`statsPeriod` property within the query key. However, the `stripUndefinedValues`
function removes this property before the query key is constructed. As a result, the
condition `statsPeriod && !foundData` always evaluates to `false`, preventing the
polling request from ever being made. This means users will never see the "Load new
feedback" banner, even when new items are available and a relative date range is
selected.

Copy link
Copy Markdown
Collaborator

@TkDodo TkDodo left a comment

Choose a reason for hiding this comment

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

when you’re moving an endpoint to queryOptions, it’s better to move all usages, especially when invalidation and cache updates is involved. Otherwise, we have two caches which might get out of sync.

I can find more places where /organizations/$organizationIdOrSlug/issues/ is used which hasn’t been touched in this PR.

search for:

getApiUrl('/organizations/$organizationIdOrSlug/issues/'

and you will find some more that would need to be moved to apiOptions!

@@ -58,13 +66,12 @@ export function useFeedbackCache() {
}
const listData = queryClient.getQueryData<ListCache>(listQueryKey);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
const listData = queryClient.getQueryData<ListCache>(listQueryKey);
const listData = queryClient.getQueryData(listQueryKey);

it’s important that we rely on type inference for getQueryData when using the queryKey from queryOptions.

@ryan953
Copy link
Copy Markdown
Member Author

ryan953 commented Apr 10, 2026

when you’re moving an endpoint to queryOptions, it’s better to move all usages, especially when invalidation and cache updates is involved. Otherwise, we have two caches which might get out of sync.

I can find more places where /organizations/$organizationIdOrSlug/issues/ is used which hasn’t been touched in this PR.

search for:

getApiUrl('/organizations/$organizationIdOrSlug/issues/'

and you will find some more that would need to be moved to apiOptions!

I'm probably not going to do that in this case. Usually yes i understand the reasons why.
In this case these queries are scoped to id's that are feedback items, and we should have queryParam discriminators to back that up too. So there should be no overlap. The other callsites around the app are going to be related to issues, and there should be many of them, which broadens the scope of things :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Scope: Frontend Automatically applied to PRs that change frontend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants