-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
feat(issues): top issues experiment #103773
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.
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
Conversation
c2355a8 to
b8a87de
Compare
b8a87de to
5d232c5
Compare
Add a feature flag for dynamic grouping experiment of top issues. See #103773
5d232c5 to
24ddeb2
Compare
24ddeb2 to
58047e0
Compare
| localStorage.removeItem(STORAGE_KEY); | ||
| } | ||
| }, 300); // Match the animation duration | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Race condition in cluster removal
handleRemoveCluster uses a timeout for animation but captures a stale clusterData closure in the callback. If multiple clusters are removed quickly, the state updates will overwrite each other, causing previously removed items to reappear. Use the functional update pattern setClusterData(prev => ...) to ensure updates are based on the latest state.
| } catch (error) { | ||
| setParseError(error instanceof Error ? error.message : t('Invalid JSON format')); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Missing JSON schema validation causes crash
The JSON input validation only checks for an array, but the code expects objects with specific fields like group_ids. Entering invalid objects (e.g. missing group_ids) causes the app to crash when accessing properties of undefined. Please add schema validation to ensure required fields exist before setting state.
| import {useUser} from 'sentry/utils/useUser'; | ||
| import {useUserTeams} from 'sentry/utils/useUserTeams'; | ||
|
|
||
| const STORAGE_KEY = 'dynamic-grouping-cluster-data'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Data leak across organizations in local storage
The STORAGE_KEY is static and not namespaced by the organization. If a user switches organizations, they will see the cluster data from the previous organization. This can lead to confusion and potential leakage of sensitive issue titles/descriptions across organization boundaries on shared devices. Append organization.slug to the storage key.
| flex-direction: column; | ||
| gap: ${space(1.5)}; | ||
| `; | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Excessive use of styled components contrary to guidelines (Bugbot Rules)
The new file defines numerous styled components (e.g., PageContainer, PageHeader, CardContainer) despite importing Container, Flex, and Heading. Sentry's frontend guidelines (AGENTS.md) explicitly state to use these Core Components for layout and typography instead of creating new styled wrappers.
| ))} | ||
| </Flex> | ||
| ); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Unbounded concurrent API requests causing network flood
The ClusterIssues component triggers a separate useApiQuery request for each rendered cluster to fetch issue details. When visualizing a large number of clusters (e.g., pasted from a bulk analysis), this will trigger N concurrent API requests to the issues endpoint, leading to browser stalling or rate limiting. Requests should be batched.
| group: previewGroupIds, | ||
| query: `issue.id:[${previewGroupIds.join(',')}]`, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Invalid group parameter sent to issues endpoint
The API query in ClusterIssues passes a group parameter (array of IDs) to the issues endpoint. The Sentry API typically uses the query string parameter (e.g., issue.id:[...]) for ID filtering, not a top-level group parameter. This parameter is likely incorrect/ignored and should be removed to ensure correct API usage.
scttcper
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense for the demo. keeping it all in one file helps
Add UI for the top issues experiment. Hidden behind
organizations:top-issues-ui, see https://github.com/getsentry/sentry-options-automator/pull/5780