This is an automated email from the ASF dual-hosted git repository. msyavuz pushed a commit to branch msyavuz/fix/slack-channels-large-workspace in repository https://gitbox.apache.org/repos/asf/superset.git
commit fe3a25ada683b849a17e3af087f2638b2f08bd09 Author: Mehmet Salih Yavuz <[email protected]> AuthorDate: Mon Jul 13 15:33:16 2026 +0300 fix(reports): paginate Slack recipient picker for large workspaces The Alerts & Reports SlackV2 recipient picker eagerly enumerated every channel via conversations.list (no Slack search API) and rendered the full list. On workspaces with tens of thousands of channels this hit Slack rate limits (504) and froze the browser. Backend: paginate the /report/slack_channels/ endpoint (page/page_size) and return the full count, so the browser never receives the whole list. The search_string/exact_match params now actually reach the filter. Frontend: switch the picker to AsyncSelect with debounced server-side search (one page at a time) and allowNewOptions so a channel id can be pasted directly, matching the SlackV2 send path. Edit-mode ids resolve via a bounded exact_match lookup. Large workspaces should schedule the existing slack.cache_channels task to keep the channel cache warm. --- .../alerts/components/NotificationMethod.test.tsx | 69 +++++++ .../alerts/components/NotificationMethod.tsx | 222 +++++++++++---------- superset/config.py | 5 + superset/reports/api.py | 11 +- superset/reports/schemas.py | 3 + tests/unit_tests/reports/api_test.py | 22 ++ 6 files changed, 227 insertions(+), 105 deletions(-) diff --git a/superset-frontend/src/features/alerts/components/NotificationMethod.test.tsx b/superset-frontend/src/features/alerts/components/NotificationMethod.test.tsx index 5177e425bfa..736933c6481 100644 --- a/superset-frontend/src/features/alerts/components/NotificationMethod.test.tsx +++ b/superset-frontend/src/features/alerts/components/NotificationMethod.test.tsx @@ -752,6 +752,75 @@ describe('NotificationMethod', () => { ).toBeInTheDocument(); }); + test('fetches Slack channels lazily with pagination for SlackV2', async () => { + window.featureFlags = { [FeatureFlag.AlertReportSlackV2]: true }; + const getSpy = jest.spyOn(SupersetClient, 'get').mockResolvedValue({ + json: { + count: 1, + result: [ + { id: 'C123', name: 'general', is_private: false, is_member: true }, + ], + }, + } as unknown as JsonResponse); + + render( + <NotificationMethod + setting={{ ...mockSettingSlackV2, recipients: '' }} + index={0} + onUpdate={mockOnUpdate} + onRemove={mockOnRemove} + onInputChange={mockOnInputChange} + email_subject={mockEmailSubject} + defaultSubject={mockDefaultSubject} + setErrorSubject={mockSetErrorSubject} + />, + ); + + fireEvent.click(await screen.findByTestId('recipients-load-options')); + + expect(await screen.findByText('general')).toBeInTheDocument(); + + const slackEndpoint = getSpy.mock.calls + .map(([arg]) => (arg as { endpoint: string }).endpoint) + .find(endpoint => endpoint.includes('/report/slack_channels/')); + expect(slackEndpoint).toBeDefined(); + expect(rison.decode(slackEndpoint!.split('?q=')[1])).toMatchObject({ + search_string: '', + types: ['public_channel', 'private_channel'], + page: 0, + page_size: 25, + }); + }); + + test('allows entering a Slack channel id directly for SlackV2', async () => { + window.featureFlags = { [FeatureFlag.AlertReportSlackV2]: true }; + jest.spyOn(SupersetClient, 'get').mockResolvedValue({ + json: { count: 0, result: [] }, + } as unknown as JsonResponse); + + render( + <NotificationMethod + setting={{ ...mockSettingSlackV2, recipients: '' }} + index={0} + onUpdate={mockOnUpdate} + onRemove={mockOnRemove} + onInputChange={mockOnInputChange} + email_subject={mockEmailSubject} + defaultSubject={mockDefaultSubject} + setErrorSubject={mockSetErrorSubject} + />, + ); + + fireEvent.change(await screen.findByTestId('recipients'), { + target: { value: 'C0123456789' }, + }); + + expect(mockOnUpdate).toHaveBeenCalledWith( + 0, + expect.objectContaining({ recipients: 'C0123456789' }), + ); + }); + // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks describe('RefreshLabel functionality', () => { test('should call updateSlackOptions with force true when RefreshLabel is clicked', async () => { diff --git a/superset-frontend/src/features/alerts/components/NotificationMethod.tsx b/superset-frontend/src/features/alerts/components/NotificationMethod.tsx index 1a5df776491..240d50ab26a 100644 --- a/superset-frontend/src/features/alerts/components/NotificationMethod.tsx +++ b/superset-frontend/src/features/alerts/components/NotificationMethod.tsx @@ -20,8 +20,10 @@ import { FunctionComponent, useState, ChangeEvent, + useCallback, useEffect, useMemo, + useRef, } from 'react'; import rison from 'rison'; @@ -165,47 +167,6 @@ export const mapSlackValues = ({ .filter(val => !!val) as { label: string; value: string }[]; }; -export const mapChannelsToOptions = (result: SlackChannel[]) => { - const publicChannels: SlackChannel[] = []; - const privateChannels: SlackChannel[] = []; - - result.forEach(channel => { - if (channel.is_private) { - privateChannels.push(channel); - } else { - publicChannels.push(channel); - } - }); - - return [ - { - label: 'Public Channels', - options: publicChannels.map((channel: SlackChannel) => ({ - label: `${channel.name} ${ - channel.is_member ? '' : t('(Bot not in channel)') - }`, - value: channel.id, - key: channel.id, - })), - key: 'public', - }, - { - label: t('Private Channels (Bot in channel)'), - options: privateChannels.map((channel: SlackChannel) => ({ - label: channel.name, - value: channel.id, - key: channel.id, - })), - key: 'private', - }, - ]; -}; - -type SlackOptionsType = { - label: string; - options: { label: string; value: string }[]; -}[]; - type EmailRecipientField = 'recipients' | 'cc' | 'bcc'; type EmailRecipientOption = { @@ -310,12 +271,8 @@ export const NotificationMethod: FunctionComponent<NotificationMethodProps> = ({ const theme = useTheme(); const [methodOptionsLoading, setMethodOptionsLoading] = useState<boolean>(true); - const [slackOptions, setSlackOptions] = useState<SlackOptionsType>([ - { - label: '', - options: [], - }, - ]); + const [slackRefreshKey, setSlackRefreshKey] = useState<number>(0); + const forceRefreshSlackRef = useRef<boolean>(false); const recipientEmailOptions = useMemo( () => recipientStringToOptions(recipientValue), [recipientValue], @@ -355,83 +312,136 @@ export const NotificationMethod: FunctionComponent<NotificationMethodProps> = ({ } }; - const fetchSlackChannels = async ({ + const fetchSlackChannels = ({ searchString = '', types = [], exactMatch = false, force = false, + page, + pageSize, }: { - searchString?: string | undefined; + searchString?: string; types?: string[]; - exactMatch?: boolean | undefined; - force?: boolean | undefined; + exactMatch?: boolean; + force?: boolean; + page?: number; + pageSize?: number; } = {}): Promise<JsonResponse> => { const queryString = rison.encode({ - searchString, + search_string: searchString, types, - exactMatch, + exact_match: exactMatch, force, + ...(page !== undefined ? { page } : {}), + ...(pageSize !== undefined ? { page_size: pageSize } : {}), }); const endpoint = `/api/v1/report/slack_channels/?q=${queryString}`; return SupersetClient.get({ endpoint }); }; - const updateSlackOptions = async ({ - force, - }: { - force?: boolean | undefined; - } = {}) => { - setIsSlackChannelsLoading(true); - fetchSlackChannels({ types: ['public_channel', 'private_channel'], force }) - .then(({ json }) => { - const { result } = json; - const options: SlackOptionsType = mapChannelsToOptions(result); - - setSlackOptions(options); - - if (isFeatureEnabled(FeatureFlag.AlertReportSlackV2)) { - // for edit mode, map existing ids to names for display if slack v2 - // or names to ids if slack v1 - const [publicOptions, privateOptions] = options; - if ( - method && - [ - NotificationMethodOption.SlackV2, - NotificationMethodOption.Slack, - ].includes(method) - ) { - setSlackRecipients( - mapSlackValues({ - method, - recipientValue, - slackOptions: [ - ...publicOptions.options, - ...privateOptions.options, - ], - }), - ); - } - } - }) - .catch(() => { - // Fallback to slack v1 if slack v2 is not compatible - setUseSlackV1(true); - }) - .finally(() => { - setMethodOptionsLoading(false); - setIsSlackChannelsLoading(false); + // Lazily fetch one page of channels as the user searches, so workspaces with + // tens of thousands of channels never load the whole list into the browser. + const loadSlackChannels = useCallback( + async (search: string, page: number, pageSize: number) => { + const force = forceRefreshSlackRef.current; + forceRefreshSlackRef.current = false; + const { json } = await fetchSlackChannels({ + searchString: search, + types: ['public_channel', 'private_channel'], + force, + page, + pageSize, }); + const result = (json?.result ?? []) as SlackChannel[]; + return { + data: result.map(channel => ({ + label: + !channel.is_private && !channel.is_member + ? `${channel.name} ${t('(Bot not in channel)')}` + : channel.name, + value: channel.id, + })), + totalCount: json?.count ?? result.length, + }; + }, + // fetchSlackChannels only closes over stable refs/imports + // eslint-disable-next-line react-hooks/exhaustive-deps + [], + ); + + const onRefreshSlackChannels = () => { + forceRefreshSlackRef.current = true; + // Remount the AsyncSelect so its internal result cache is discarded and the + // forced (cache-busting) fetch runs. + setSlackRefreshKey(key => key + 1); }; + // Resolve the channel ids saved on an existing SlackV2 report into + // human-readable labels. Bounded by exact_match so it never enumerates the + // whole workspace. useEffect(() => { + let cancelled = false; const slackEnabled = options?.some( option => option === NotificationMethodOption.Slack || option === NotificationMethodOption.SlackV2, ); - if (slackEnabled && !slackOptions[0]?.options.length) { - updateSlackOptions(); + const savedIds = + method === NotificationMethodOption.SlackV2 && recipientValue + ? recipientValue + .split(',') + .map(value => value.trim()) + .filter(Boolean) + : []; + + const resolveSavedRecipients = async () => { + // Show the saved ids immediately so the existing selection is always + // visible, then enrich with channel names once the lookup resolves. + if (savedIds.length && !cancelled) { + setSlackRecipients(savedIds.map(id => ({ label: id, value: id }))); + } + try { + if (savedIds.length) { + const { json } = await fetchSlackChannels({ + searchString: recipientValue, + types: ['public_channel', 'private_channel'], + exactMatch: true, + }); + const result = (json?.result ?? []) as SlackChannel[]; + const namesById = new Map( + result.map(channel => [channel.id, channel.name]), + ); + if (!cancelled) { + setSlackRecipients( + savedIds.map(id => ({ + label: namesById.get(id) ?? id, + value: id, + })), + ); + } + } + } catch { + // Keep the raw ids as labels; the AsyncSelect onError handler drives + // the v1 fallback for the picker itself. + } finally { + if (!cancelled) { + setMethodOptionsLoading(false); + setIsSlackChannelsLoading(false); + } + } + }; + + if (slackEnabled) { + resolveSavedRecipients(); + } else { + setMethodOptionsLoading(false); + setIsSlackChannelsLoading(false); } + + return () => { + cancelled = true; + }; + // eslint-disable-next-line react-hooks/exhaustive-deps }, []); useEffect(() => { @@ -665,21 +675,25 @@ export const NotificationMethod: FunctionComponent<NotificationMethodProps> = ({ ) : ( // for SlackV2 <div className="input-container"> - <Select + <AsyncSelect + key={slackRefreshKey} ariaLabel={t('Select channels')} mode="multiple" name="recipients" value={slackRecipients} - options={slackOptions} + options={loadSlackChannels} onChange={onSlackRecipientsChange} + onError={() => setUseSlackV1(true)} allowClear + allowNewOptions data-test="recipients" - loading={isSlackChannelsLoading} - allowSelectAll={false} labelInValue + placeholder={t( + 'Search a channel by name, or paste a channel ID', + )} /> <RefreshLabel - onClick={() => updateSlackOptions({ force: true })} + onClick={onRefreshSlackChannels} tooltipContent={t('Force refresh Slack channels list')} disabled={isSlackChannelsLoading} /> diff --git a/superset/config.py b/superset/config.py index cf4a8707f1a..d3d7e6f1c89 100644 --- a/superset/config.py +++ b/superset/config.py @@ -2342,6 +2342,11 @@ SLACK_PROXY = None # ignored for workspace-level tokens, so leaving it as None preserves the default # single-workspace behavior. SLACK_TEAM_ID: Callable[[], str] | str | None = None +# How long the fetched channel list is cached. The Alerts & Reports recipient +# picker serves channels from this cache, so on very large workspaces (tens of +# thousands of channels) schedule the ``slack.cache_channels`` Celery task to +# warm the cache ahead of the TTL — enumerating that many channels inside a +# single interactive request will otherwise hit Slack rate limits and time out. SLACK_CACHE_TIMEOUT = int(timedelta(days=1).total_seconds()) # Maximum number of retries when Slack API returns rate limit errors diff --git a/superset/reports/api.py b/superset/reports/api.py index 3089af26f37..01e7b1226b2 100644 --- a/superset/reports/api.py +++ b/superset/reports/api.py @@ -683,13 +683,22 @@ class ReportScheduleRestApi(BaseSupersetModelRestApi): types = params.get("types", []) exact_match = params.get("exact_match", False) force = params.get("force", False) + page = params.get("page") + page_size = params.get("page_size") channels = get_channels_with_search( search_string=search_string, types=types, exact_match=exact_match, force=force, ) - return self.response(200, result=channels) + # Paginate at the API layer so large workspaces (tens of thousands of + # channels) never ship the full list to the browser at once. The + # filtered set is served from the warm cache, so slicing is cheap. + count = len(channels) + if page is not None and page_size is not None: + start = page * page_size + channels = channels[start : start + page_size] + return self.response(200, count=count, result=channels) except SupersetException as ex: logger.error("Error fetching slack channels %s", str(ex)) return self.response_422(message=str(ex)) diff --git a/superset/reports/schemas.py b/superset/reports/schemas.py index 0c5262aa7d3..d9d703206bd 100644 --- a/superset/reports/schemas.py +++ b/superset/reports/schemas.py @@ -59,6 +59,9 @@ get_slack_channels_schema = { "items": {"type": "string", "enum": ["public_channel", "private_channel"]}, }, "exact_match": {"type": "boolean"}, + "force": {"type": "boolean"}, + "page": {"type": "integer", "minimum": 0}, + "page_size": {"type": "integer", "minimum": 1}, }, } diff --git a/tests/unit_tests/reports/api_test.py b/tests/unit_tests/reports/api_test.py index c62bd7cfd8a..76278a1dad8 100644 --- a/tests/unit_tests/reports/api_test.py +++ b/tests/unit_tests/reports/api_test.py @@ -36,6 +36,28 @@ def test_slack_channels_success( assert rv.status_code == 200 data = rv.json assert data["result"] == [{"id": "C123", "name": "general"}] + assert data["count"] == 1 + + +@with_feature_flags(ALERT_REPORTS=True) +@patch("superset.reports.api.get_channels_with_search") +def test_slack_channels_paginates( + mock_search: Any, + client: Any, + full_api_access: None, +) -> None: + # A large workspace: the endpoint must return only the requested page while + # reporting the full count, so the browser never receives every channel. + mock_search.return_value = [ + {"id": f"C{i}", "name": f"channel-{i}"} for i in range(250) + ] + params = rison.dumps({"page": 1, "page_size": 100}) + rv = client.get(f"/api/v1/report/slack_channels/?q={params}") + assert rv.status_code == 200 + data = rv.json + assert data["count"] == 250 + assert len(data["result"]) == 100 + assert data["result"][0] == {"id": "C100", "name": "channel-100"} @with_feature_flags(ALERT_REPORTS=True)
