From 7ab8031d6f10e491ce51880bb2a235830d9516fd Mon Sep 17 00:00:00 2001 From: Dominik Buszowiecki Date: Wed, 27 May 2026 16:54:40 -0400 Subject: [PATCH 1/3] fix(dashboards): Move global filter loading spinner to dropdown footer The global filter dropdown trigger replaced the selected value with a spinner while tag values refetched as the user typed. This was confusing because the selected value had not changed. Keep the selected value visible in the trigger and surface the loading state as a small indicator at the bottom of the dropdown list instead. Refs DAIN-1636 Co-Authored-By: Claude --- .../globalFilter/filterSelector.tsx | 42 +++++++++++++------ .../globalFilter/filterSelectorTrigger.tsx | 39 +++++------------ 2 files changed, 41 insertions(+), 40 deletions(-) diff --git a/static/app/views/dashboards/globalFilter/filterSelector.tsx b/static/app/views/dashboards/globalFilter/filterSelector.tsx index ccf727c10e19..0bf9a24589d4 100644 --- a/static/app/views/dashboards/globalFilter/filterSelector.tsx +++ b/static/app/views/dashboards/globalFilter/filterSelector.tsx @@ -16,6 +16,7 @@ import {Flex} from '@sentry/scraps/layout'; import {OverlayTrigger} from '@sentry/scraps/overlayTrigger'; import {DropdownMenu} from 'sentry/components/dropdownMenu'; +import {LoadingIndicator} from 'sentry/components/loadingIndicator'; import {usePageFilters} from 'sentry/components/pageFilters/usePageFilters'; import {useStagedCompactSelect} from 'sentry/components/pageFilters/useStagedCompactSelect'; import { @@ -342,10 +343,15 @@ export function FilterSelector({ activeFilterValues={filterValues} operator={stagedOperator} options={translatedOptions} - queryResult={queryResult} /> ); + const loadingFooter = isFetching ? ( + + + + ) : null; + if (!canSelectMultipleValues) { return ( { setStagedFilterValues([]); }} + menuFooter={loadingFooter} menuTitle={ {t('%s Filter', getDatasetLabel(globalFilter.dataset))} @@ -421,17 +428,22 @@ export function FilterSelector({ isFetching ? t('Loading filter values...') : t('No filter values found') } menuFooter={ - hasStagedChanges ? ( - - dispatch({type: 'remove staged'})} - /> - { - dispatch({type: 'remove staged'}); - handleChange(stagedSelect.value); - }} - /> + hasStagedChanges || isFetching ? ( + + {loadingFooter} + {hasStagedChanges && ( + + dispatch({type: 'remove staged'})} + /> + { + dispatch({type: 'remove staged'}); + handleChange(stagedSelect.value); + }} + /> + + )} ) : null } @@ -517,6 +529,12 @@ export const MenuTitleWrapper = styled('span')` padding-bottom: ${p => p.theme.space.xs}; `; +const FooterLoadingIndicator = styled(LoadingIndicator)` + && { + margin: 0; + } +`; + const OperatorFlex = styled(Flex)` margin-left: -${p => p.theme.space.sm}; `; diff --git a/static/app/views/dashboards/globalFilter/filterSelectorTrigger.tsx b/static/app/views/dashboards/globalFilter/filterSelectorTrigger.tsx index e53dab57a1b7..32960f5711cf 100644 --- a/static/app/views/dashboards/globalFilter/filterSelectorTrigger.tsx +++ b/static/app/views/dashboards/globalFilter/filterSelectorTrigger.tsx @@ -1,12 +1,8 @@ -import styled from '@emotion/styled'; -import type {UseQueryResult} from '@tanstack/react-query'; - import {Badge} from '@sentry/scraps/badge'; import type {SelectOption} from '@sentry/scraps/compactSelect'; import {Container, Flex} from '@sentry/scraps/layout'; import {Text} from '@sentry/scraps/text'; -import {LoadingIndicator} from 'sentry/components/loadingIndicator'; import {OP_LABELS} from 'sentry/components/searchQueryBuilder/tokens/filter/utils'; import {TermOperator} from 'sentry/components/searchSyntax/parser'; import {t} from 'sentry/locale'; @@ -19,7 +15,6 @@ type FilterSelectorTriggerProps = { globalFilter: GlobalFilter; operator: TermOperator; options: Array>; - queryResult: UseQueryResult; }; export function FilterSelectorTrigger({ @@ -27,12 +22,10 @@ export function FilterSelectorTrigger({ activeFilterValues, operator, options, - queryResult, }: FilterSelectorTriggerProps) { - const {isFetching} = queryResult; const {tag} = globalFilter; - const shouldShowBadge = !isFetching && activeFilterValues.length > 1; + const shouldShowBadge = activeFilterValues.length > 1; // "All" means no filter is applied (empty selection). We intentionally avoid // comparing against options.length because when tag values fail to load, @@ -64,20 +57,17 @@ export function FilterSelectorTrigger({ {opLabel} - {!isFetching && - (isAllSelected ? ( - - {t('All')} + {isAllSelected ? ( + + {t('All')} + + ) : ( + + + {label} - ) : ( - - - {label} - - - ))} - - {isFetching && } + + )} {shouldShowBadge && ( @@ -87,10 +77,3 @@ export function FilterSelectorTrigger({ ); } - -const InlineLoadingIndicator = styled(LoadingIndicator)` - && { - margin: 0; - margin-left: ${p => p.theme.space.xs}; - } -`; From dca6f8d9b6e3ead7734405b479f4637efa3ce1b8 Mon Sep 17 00:00:00 2001 From: Dominik Buszowiecki Date: Wed, 27 May 2026 17:05:04 -0400 Subject: [PATCH 2/3] test(dashboards): Update global filter trigger accessible names The filter selector trigger now always displays the selected value (or "All"), so the accessible names of the trigger buttons include that text in tests. Co-Authored-By: Claude --- .../dashboards/globalFilter/filterSelector.spec.tsx | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/static/app/views/dashboards/globalFilter/filterSelector.spec.tsx b/static/app/views/dashboards/globalFilter/filterSelector.spec.tsx index 448259dfd665..2f4c5be2dcf9 100644 --- a/static/app/views/dashboards/globalFilter/filterSelector.spec.tsx +++ b/static/app/views/dashboards/globalFilter/filterSelector.spec.tsx @@ -38,7 +38,7 @@ describe('FilterSelector', () => { ); const button = screen.getByRole('button', { - name: `${mockGlobalFilter.tag.key} contains`, + name: `${mockGlobalFilter.tag.key} contains All`, }); await userEvent.click(button); @@ -58,7 +58,7 @@ describe('FilterSelector', () => { ); const button = screen.getByRole('button', { - name: `${mockGlobalFilter.tag.key} contains`, + name: `${mockGlobalFilter.tag.key} contains All`, }); await userEvent.click(button); @@ -90,7 +90,7 @@ describe('FilterSelector', () => { /> ); - const button = screen.getByRole('button', {name: mockGlobalFilter.tag.key + ' :'}); + const button = screen.getByRole('button', {name: /^browser :/}); await userEvent.click(button); expect(screen.getByRole('checkbox', {name: 'Select firefox'})).toBeChecked(); @@ -108,7 +108,7 @@ describe('FilterSelector', () => { ); const button = screen.getByRole('button', { - name: `${mockGlobalFilter.tag.key} contains`, + name: `${mockGlobalFilter.tag.key} contains All`, }); await userEvent.click(button); await userEvent.click(screen.getByRole('button', {name: 'Remove Filter'})); @@ -209,7 +209,7 @@ describe('FilterSelector', () => { ); const button = screen.getByRole('button', { - name: `${SpanFields.USER_GEO_SUBREGION} contains`, + name: `${SpanFields.USER_GEO_SUBREGION} contains All`, }); await userEvent.click(button); @@ -240,7 +240,7 @@ describe('FilterSelector', () => { ); const button = screen.getByRole('button', { - name: `${mockGlobalFilter.tag.key} contains`, + name: `${mockGlobalFilter.tag.key} contains All`, }); await userEvent.click(button); From dac010b2879fa1664c3413dd6511de90685e137f Mon Sep 17 00:00:00 2001 From: Dominik Buszowiecki Date: Thu, 28 May 2026 11:17:20 -0400 Subject: [PATCH 3/3] test(dashboards): Stop waiting on trigger loading indicator in filtersBar The global filter trigger no longer renders a loading indicator while tag values fetch, so waitForElementToBeRemoved on that testid errors immediately. Wait for the filter trigger button to appear instead. Co-Authored-By: Claude --- .../app/views/dashboards/filtersBar.spec.tsx | 19 +++++-------------- 1 file changed, 5 insertions(+), 14 deletions(-) diff --git a/static/app/views/dashboards/filtersBar.spec.tsx b/static/app/views/dashboards/filtersBar.spec.tsx index 653c3ca5d9f7..6d32ab62e01a 100644 --- a/static/app/views/dashboards/filtersBar.spec.tsx +++ b/static/app/views/dashboards/filtersBar.spec.tsx @@ -5,12 +5,7 @@ import {OrganizationFixture} from 'sentry-fixture/organization'; import {ReleaseFixture} from 'sentry-fixture/release'; import {TagsFixture} from 'sentry-fixture/tags'; -import { - render, - screen, - waitFor, - waitForElementToBeRemoved, -} from 'sentry-test/reactTestingLibrary'; +import {render, screen, waitFor} from 'sentry-test/reactTestingLibrary'; import type {Organization} from 'sentry/types/organization'; import {FieldKind} from 'sentry/utils/fields'; @@ -63,9 +58,8 @@ describe('FiltersBar', () => { }, }); renderFilterBar({location: newLocation}); - await waitForElementToBeRemoved(() => screen.queryByTestId('loading-indicator')); expect( - screen.getByRole('button', {name: /browser\.name.*Chrome/i}) + await screen.findByRole('button', {name: /browser\.name.*Chrome/i}) ).toBeInTheDocument(); }); @@ -80,8 +74,7 @@ describe('FiltersBar', () => { }, }); renderFilterBar({location: newLocation, hasUnsavedChanges: true}); - await waitForElementToBeRemoved(() => screen.queryByTestId('loading-indicator')); - expect(screen.getByRole('button', {name: 'Save'})).toBeInTheDocument(); + expect(await screen.findByRole('button', {name: 'Save'})).toBeInTheDocument(); expect(screen.getByRole('button', {name: 'Cancel'})).toBeInTheDocument(); }); @@ -98,9 +91,8 @@ describe('FiltersBar', () => { }); renderFilterBar({location: newLocation}); - await waitForElementToBeRemoved(() => screen.queryByTestId('loading-indicator')); expect( - screen.getByRole('button', {name: /browser\.name.*Chrome/i}) + await screen.findByRole('button', {name: /browser\.name.*Chrome/i}) ).toBeInTheDocument(); expect(screen.queryByRole('button', {name: 'Save'})).not.toBeInTheDocument(); expect(screen.queryByRole('button', {name: 'Cancel'})).not.toBeInTheDocument(); @@ -215,9 +207,8 @@ describe('FiltersBar', () => { hasUnsavedChanges: true, prebuiltDashboardId: PrebuiltDashboardId.FRONTEND_SESSION_HEALTH, }); - await waitForElementToBeRemoved(() => screen.queryByTestId('loading-indicator')); expect( - screen.getByRole('button', {name: /browser\.name.*Chrome/i}) + await screen.findByRole('button', {name: /browser\.name.*Chrome/i}) ).toBeInTheDocument(); expect(screen.getByRole('button', {name: 'Save for Everyone'})).toBeInTheDocument(); expect(screen.getByRole('button', {name: 'Cancel'})).toBeInTheDocument();