fix(reports): paginate Slack recipient picker for large workspaces#41998
Merged
Conversation
dosubot
Bot
added
alert-reports
Contributor
Code Review Agent Run #85a911Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
Comment on lines
+686
to
+687
| page = params.get("page") | ||
| page_size = params.get("page_size") |
Contributor
There was a problem hiding this comment.
Suggestion: Add explicit type annotations for the newly introduced pagination input variables to satisfy the type-hint requirement. [custom_rule]
Severity Level: Minor 🧹
Why it matters? ⭐
The added local variables page and page_size are unannotated Python variables in newly introduced code. Since they are used as pagination inputs and can be type-hinted, this matches the rule requiring type hints for relevant new or modified Python code.
Rule source 📖
.cursor/rules/dev-standard.mdc (line 28)
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/reports/api.py
**Line:** 686:687
**Comment:**
*Custom Rule: Add explicit type annotations for the newly introduced pagination input variables to satisfy the type-hint requirement.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+697
to
+699
| count = len(channels) | ||
| if page is not None and page_size is not None: | ||
| start = page * page_size |
Contributor
There was a problem hiding this comment.
Suggestion: Add type annotations for the new derived pagination variables so their numeric intent is explicit and type-checked. [custom_rule]
Severity Level: Minor 🧹
Why it matters? ⭐
The new local variables count and start are introduced without type annotations in Python code that can be annotated. This is a real violation of the type-hint requirement for relevant variables.
Rule source 📖
.cursor/rules/dev-standard.mdc (line 28)
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/reports/api.py
**Line:** 697:699
**Comment:**
*Custom Rule: Add type annotations for the new derived pagination variables so their numeric intent is explicit and type-checked.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| @@ -36,6 +36,28 @@ def test_slack_channels_success( | |||
| assert rv.status_code == 200 | |||
| data = rv.json | |||
Contributor
There was a problem hiding this comment.
Suggestion: Add an explicit type annotation for this response payload variable to comply with the type-hint requirement for relevant new variables. [custom_rule]
Severity Level: Minor 🧹
Why it matters? ⭐
The new test introduces a local variable assigned from rv.json without any type annotation. Since this is a newly added Python variable that can reasonably be annotated, it matches the type-hint requirement.
Rule source 📖
.cursor/rules/dev-standard.mdc (line 28)
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/reports/api_test.py
**Line:** 37:37
**Comment:**
*Custom Rule: Add an explicit type annotation for this response payload variable to comply with the type-hint requirement for relevant new variables.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #41998 +/- ##
==========================================
+ Coverage 64.49% 65.06% +0.57%
==========================================
Files 2747 2747
Lines 153843 153863 +20
Branches 35268 35279 +11
==========================================
+ Hits 99221 100116 +895
+ Misses 52712 51836 -876
- Partials 1910 1911 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
geido
reviewed
Jul 14, 2026
geido
left a comment
geido
left a comment
Member
There was a problem hiding this comment.
Can we look if we can trim down the payload also?
Member
Author
This is the current payload: { "id": "C123", "name": "general", "is_member": true, "is_private": false }If we trim the is_member and is_private we lose the (Bot is not in channel) hint we display |
Contributor
EnxDev's Review Agent — #41998 · HEAD fe3a25acomment — solid, well-tested fix; the async picker + pagination are correct and CI is green. Two minor items plus an answer to the open payload thread. Verified: backend paginates the warm-cached filtered set and returns full 🟡 Should-fix
🔵 Nits
🙌 Praise
|
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
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.
Remove the isSlackChannelsLoading state: after the move to AsyncSelect nothing set it back to true, so the RefreshLabel disable never fired. AsyncSelect shows its own spinner on the force-refresh remount. Add coverage for edit-mode id->name resolution, the force-refresh cache-busting fetch, and the page-without-page_size fall-through.
msyavuz
force-pushed
the
msyavuz/fix/slack-channels-large-workspace
branch
from
July 14, 2026 21:44
01fc09a to
3bbbe81
Compare
Contributor
Code Review Agent Run #ef0377Actionable Suggestions - 0Additional Suggestions - 1
Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
msyavuz
requested review from
betodealmeida,
eschutho,
michael-s-molina,
mistercrunch and
sadpandajoe
as code owners
July 15, 2026 09:15
| from alembic import op | ||
| from sqlalchemy import Column, Integer, String, Text | ||
| from sqlalchemy.ext.declarative import declarative_base | ||
| from sqlalchemy.orm import declarative_base |
Contributor
There was a problem hiding this comment.
Suggestion: Avoid introducing new raw SQLAlchemy migration patterns here and refactor this migration to use superset.migrations.shared.utils helpers for cross-dialect-safe data updates instead of adding ORM-base plumbing directly. [custom_rule]
Severity Level: Major
Why it matters? ⭐
This migration introduces direct SQLAlchemy ORM plumbing (declarative_base) instead of using helpers from superset.migrations.shared.utils, and the repository already provides migration helpers such as paginated_update for this pattern. That matches the migration-specific custom rule violation.
Rule source 📖
.github/copilot-instructions.md (line 294)
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/migrations/versions/2026-06-30_00-00_d24e6b0a9c7f_strip_metricsqlexpressions_from_ag_grid_params.py
**Line:** 36:36
**Comment:**
*Custom Rule: Avoid introducing new raw SQLAlchemy migration patterns here and refactor this migration to use `superset.migrations.shared.utils` helpers for cross-dialect-safe data updates instead of adding ORM-base plumbing directly.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
| 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) |
Contributor
There was a problem hiding this comment.
Suggestion: The endpoint now returns an extra count field, but the documented response schema in this API method still describes only result. This creates an API contract mismatch for OpenAPI consumers and generated clients; update the documented response schema to include count. [api mismatch]
Severity Level: Major ⚠️
- ⚠️ OpenAPI spec omits count field in response.
- ⚠️ Generated clients lack total-count pagination metadata.
- ⚠️ API consumers may misinterpret pagination capabilities.Steps of Reproduction ✅
1. Inspect the `slack_channels` endpoint implementation in
`superset/reports/api.py:628-639`; the method `slack_channels(self, **kwargs)` is
documented with an OpenAPI-style docstring starting at `superset/reports/api.py:640`,
under `get:` and `responses:`.
2. Within that docstring at `superset/reports/api.py:653-655`, observe that the documented
`200` response schema describes an `application/json` object with a single
`properties.result` array of channel objects (`id`, `name`) and no `count` property
mentioned.
3. Examine the implementation below the docstring at `superset/reports/api.py:641-662` (PR
hunk lines `681-701`); after computing `count = len(channels)` and optionally slicing, the
method returns `return self.response(200, count=count, result=channels)` at PR line `701`,
meaning the actual runtime JSON object contains both `result` and `count`.
4. Confirm the mismatch in the published OpenAPI spec by opening
`docs/static/resources/openapi.json` around lines `27880-27955`; the
`/api/v1/report/slack_channels/` path’s `200` response schema (real line ~27887) defines
an object with a `result` array but no `count` property, so OpenAPI-generated clients and
documentation describe a response without `count` even though the server always returns it
as verified by `tests/unit_tests/reports/api_test.py::test_slack_channels_success` at
lines `26-39`, which asserts `data["count"] == 1`.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/reports/api.py
**Line:** 701:701
**Comment:**
*Api Mismatch: The endpoint now returns an extra `count` field, but the documented response schema in this API method still describes only `result`. This creates an API contract mismatch for OpenAPI consumers and generated clients; update the documented response schema to include `count`.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
328
to
337
| 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 }); |
Contributor
There was a problem hiding this comment.
Suggestion: searchString is inserted into the URL using rison.encode, not URL-safe encoding. Inputs containing characters like #, &, or ? can break/truncate the q query parameter before it reaches the API, so server-side search behaves incorrectly. Use URI-safe Rison encoding for query params (as done in other fetchers) before interpolating into the endpoint. [api mismatch]
Severity Level: Major ⚠️
- ⚠️ SlackV2 channel search misbehaves for reserved characters.
- ⚠️ Large-workspace Slack recipient search becomes unreliable.
- ⚠️ Inconsistent Rison usage versus email recipient fetcher.Steps of Reproduction ✅
1. Open the Alerts and Reports modal and add a Slack notification method so that
NotificationMethod renders, as wired in AlertReportModal.tsx lines 2688-2727 where
<NotificationMethod /> is mapped over notificationSettings.
2. In NotificationMethod.tsx lines 253-267 and 679-12, select SlackV2 as the delivery
method so the SlackV2 AsyncSelect is rendered and begin typing a search string containing
URL-reserved characters such as `?`, `#`, or `&` in the recipients picker (this value is
passed as the `search` argument into loadSlackChannels at lines 123-145).
3. Observe that loadSlackChannels at NotificationMethod.tsx lines 123-145 delegates to
fetchSlackChannels at lines 94-119, which builds `queryString` using `rison.encode` at
line 109 with the raw `searchString` and interpolates it directly into the endpoint
`/api/v1/report/slack_channels/?q=${queryString}` at line 117.
4. Because `rison.encode` does not URI-escape reserved characters and the resulting string
is used verbatim in the query parameter, the browser issues a GET where the `q` parameter
is truncated or split at the reserved characters, so the backend receives a corrupted
Rison payload; server-side filtering for Slack channel search can misbehave or fail for
such inputs, unlike fetchEmailRecipientOptions at lines 217-247 which uses
`rison.encode_uri` specifically to avoid this issue.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/features/alerts/components/NotificationMethod.tsx
**Line:** 328:337
**Comment:**
*Api Mismatch: `searchString` is inserted into the URL using `rison.encode`, not URL-safe encoding. Inputs containing characters like `#`, `&`, or `?` can break/truncate the `q` query parameter before it reaches the API, so server-side search behaves incorrectly. Use URI-safe Rison encoding for query params (as done in other fetchers) before interpolating into the endpoint.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
380
to
441
| 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); | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| if (slackEnabled) { | ||
| resolveSavedRecipients(); | ||
| } else { | ||
| setMethodOptionsLoading(false); | ||
| } | ||
|
|
||
| return () => { | ||
| cancelled = true; | ||
| }; | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, []); |
Contributor
There was a problem hiding this comment.
Suggestion: The saved-recipient resolution effect runs only once on mount, but this component already supports recipients prop updates later. When setting.recipients changes after mount, slackRecipients labels are not re-resolved and can remain stale/incorrect. Re-run the resolution when relevant inputs (method, recipientValue, options) change. [stale reference]
Severity Level: Major ⚠️
- ⚠️ SlackV2 picker shows channels that are not persisted.
- ⚠️ Saved reports can lose SlackV2 recipients silently.
- ⚠️ Notification summary in backend diverges from UI state.Steps of Reproduction ✅
1. In AlertReportModal.tsx lines 2688-2727, open the notification panel so that
notificationSettings are mapped to <NotificationMethod /> instances, each receiving a
NotificationSetting with `method`, `recipients`, and `options`.
2. Configure a notification with `method` set to `NotificationMethodOption.SlackV2` and
select one or more channels in the SlackV2 AsyncSelect at NotificationMethod.tsx lines
253-267 and 679-12; this populates `slackRecipients` via onSlackRecipientsChange at lines
110-122 and persists a comma-separated channel id string into `setting.recipients` via
updateNotificationSetting in AlertReportModal.tsx lines 834-859.
3. Change the delivery method to Slack (SlackV1) and then back to SlackV2 using the Select
at NotificationMethod.tsx lines 141-157, which calls onMethodChange at lines 72-92;
onMethodChange resets `recipientValue`, `ccValue`, and `bccValue` and updates the parent
NotificationSetting (recipients cleared), but it does not clear `slackRecipients`, and the
effect that resolves saved SlackV2 recipients at lines 161-222 is declared with an empty
dependency array `[]` so it never re-runs when `method`, `recipientValue`, or `options`
change.
4. After this toggle, the SlackV2 AsyncSelect still renders using the stale
`slackRecipients` array while the underlying `setting.recipients` string is now empty;
when the user saves, AlertReportModal.tsx lines 65-82 and 115-133 build the recipients
payload from `setting.recipients`, so no SlackV2 recipient is sent even though the UI
shows selected channels, demonstrating stale label state caused by the mount-only
resolution effect.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/features/alerts/components/NotificationMethod.tsx
**Line:** 380:441
**Comment:**
*Stale Reference: The saved-recipient resolution effect runs only once on mount, but this component already supports `recipients` prop updates later. When `setting.recipients` changes after mount, `slackRecipients` labels are not re-resolved and can remain stale/incorrect. Re-run the resolution when relevant inputs (`method`, `recipientValue`, `options`) change.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| options={loadSlackChannels} | ||
| onChange={onSlackRecipientsChange} | ||
| onError={() => setUseSlackV1(true)} | ||
| allowClear |
Contributor
There was a problem hiding this comment.
Suggestion: Setting useSlackV1 in onError does not actually switch the active picker when the current method is SlackV2, so fetch failures can leave users stuck in the failing AsyncSelect path instead of falling back automatically. Ensure the fallback also updates the selected notification method (or render branch) so the UI truly moves to Slack V1 input on error. [incomplete implementation]
Severity Level: Major ⚠️
- ⚠️ SlackV2 picker does not auto-fallback on errors.
- ⚠️ Users stay on a broken SlackV2 input path.
- ⚠️ Manual reconfiguration required to reach Slack V1.Steps of Reproduction ✅
1. Ensure the SlackV2 feature flag is enabled so SlackV2 can be selected;
AlertReportModal.tsx lines 214-215 read allowedNotificationMethods, and
NotificationMethod.tsx lines 42-65 use `isFeatureEnabled(FeatureFlag.AlertReportSlackV2)`
to include SlackV2 in methodOptions when `useSlackV1` is false.
2. Open the Alerts or Reports modal and configure a notification whose NotificationSetting
has `method: NotificationMethodOption.SlackV2`, as exemplified by mockSettingSlackV2 in
NotificationMethod.test.tsx lines 225-233; this causes NotificationMethod to render the
SlackV2 AsyncSelect branch at lines 253-267 and 679-12.
3. Induce a failure in the SlackV2 channel fetch (for example by having SupersetClient.get
reject inside loadSlackChannels at NotificationMethod.tsx lines 123-145) so that the
AsyncSelect options loader throws; the AsyncSelect invokes its `onError` handler defined
at line 683 as `() => setUseSlackV1(true)`, which flips `useSlackV1` to true but leaves
`setting.method` as `NotificationMethodOption.SlackV2`.
4. After `useSlackV1` is true, methodOptions at NotificationMethod.tsx lines 42-65 filter
out SlackV2 and only include Slack, yet the current `method` prop still equals `SlackV2`;
the Select’s `value` is computed as `methodOptions.find(option => option.value ===
method)` at lines 143-156 and becomes undefined, and the recipients branch at lines
204-253 continues to render the SlackV2 AsyncSelect because it switches solely on
`method`, not `useSlackV1`, leaving the user on the failing SlackV2 picker instead of
automatically transitioning to the working SlackV1 textarea.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/features/alerts/components/NotificationMethod.tsx
**Line:** 683:683
**Comment:**
*Incomplete Implementation: Setting `useSlackV1` in `onError` does not actually switch the active picker when the current method is SlackV2, so fetch failures can leave users stuck in the failing AsyncSelect path instead of falling back automatically. Ensure the fallback also updates the selected notification method (or render branch) so the UI truly moves to Slack V1 input on error.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| "items": {"type": "string", "enum": ["public_channel", "private_channel"]}, | ||
| }, | ||
| "exact_match": {"type": "boolean"}, | ||
| "force": {"type": "boolean"}, |
Contributor
There was a problem hiding this comment.
Suggestion: Exposing force as an accepted query parameter allows any caller of the Slack channels endpoint to bypass the cached channel list and trigger a full Slack conversations.list crawl repeatedly. That reintroduces the same rate-limit/timeout failure mode this change is trying to solve. Keep force internal-only (not user-controllable from this API) or gate it behind a stricter privileged path. [cache]
Severity Level: Major ⚠️
❌ Slack channel fetch bypasses cache via force flag.
⚠️ Large workspaces hit Slack API rate limits.
⚠️ Slack recipient picker can freeze on repeated force.Steps of Reproduction ✅
1. Start Superset with Alerts & Reports enabled and a Slack bot token configured so Slack
channel listing is active (Slack client setup and channel enumeration are implemented in
`superset/utils/slack.py:79-187`).
2. Call the Slack channels endpoint exposed by `ReportScheduleRestApi.slack_channels` via
HTTP `GET /api/v1/report/slack_channels/` (route defined by `@expose("/slack_channels/",
methods=("GET",))` at `superset/reports/api.py:628` and protected with `@protect()` at
`superset/reports/api.py:63`).
3. Pass a `q` query parameter containing Rison/JSON with `"force": true`, e.g.
`q={"search_string":"","force":true}`; the schema `get_slack_channels_schema` at
`superset/reports/schemas.py:54-62` explicitly accepts a `force` boolean, and
`@parse_rison(get_slack_channels_schema)` at `superset/reports/api.py:64` deserializes it
into `params["force"]` used at `superset/reports/api.py:685`.
4. Inside `slack_channels`, the code at `superset/reports/api.py:681-127` calls
`get_channels_with_search(search_string=..., types=..., exact_match=..., force=force)`;
`get_channels_with_search` in `superset/utils/slack.py:197-213` forwards `force` into
`get_channels(force=force, cache_timeout=app.config["SLACK_CACHE_TIMEOUT"])`, and
`get_channels`’ docstring at `superset/utils/slack.py:115-133` states that `kwargs`
including `"force"` are forwarded to the memoized fetch `_get_channels`, allowing the
caller to bypass the cached Slack conversations list and trigger a full
`conversations.list` crawl on every such request, reintroducing rate-limit/timeout risks
for large workspaces.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/reports/schemas.py
**Line:** 62:62
**Comment:**
*Cache: Exposing `force` as an accepted query parameter allows any caller of the Slack channels endpoint to bypass the cached channel list and trigger a full Slack `conversations.list` crawl repeatedly. That reintroduces the same rate-limit/timeout failure mode this change is trying to solve. Keep `force` internal-only (not user-controllable from this API) or gate it behind a stricter privileged path.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| "exact_match": {"type": "boolean"}, | ||
| "force": {"type": "boolean"}, | ||
| "page": {"type": "integer", "minimum": 0}, | ||
| "page_size": {"type": "integer", "minimum": 1}, |
Contributor
There was a problem hiding this comment.
Suggestion: page_size is only bounded to be positive, but not capped, so callers can request extremely large pages and effectively force the backend to return the full filtered channel set in one response. This defeats the pagination protection and can still cause large payloads and browser/backend slowdowns. Add a strict maximum page size. [performance]
Severity Level: Critical 🚨
❌ Clients can request entire Slack channel list in one response.
⚠️ Large responses slow browser Slack channel picker significantly.
⚠️ Backend spends memory serializing huge channel arrays.Steps of Reproduction ✅
1. Run Superset with Alerts & Reports enabled and SlackV2 configured; the frontend Slack
recipient picker for Alerts & Reports uses the `slack_channels` API defined in
`superset/reports/api.py:628-135` to enumerate channels.
2. From a client (browser devtools, API script, or custom integration), issue `GET
/api/v1/report/slack_channels/` with query parameter `q` containing a large `page_size`,
for example `q={"page":0,"page_size":100000}`; the OpenAPI schema
`get_slack_channels_schema` at `superset/reports/schemas.py:54-65` defines `"page_size":
{"type": "integer", "minimum": 1}` without any maximum, so any positive integer is
accepted.
3. In `ReportScheduleRestApi.slack_channels` (`superset/reports/api.py:639-135`), `params
= kwargs.get("rison", {})` is read at line 681, then `page = params.get("page")` and
`page_size = params.get("page_size")` at `superset/reports/api.py:683-684`; the function
fetches the full filtered channel list via `get_channels_with_search(...)` at
`superset/reports/api.py:122-127`, which itself enumerates all cached channels in
`superset/utils/slack.py:197-256` before any slicing.
4. Because `page_size` is unbounded, when the client requests `page_size` larger than or
equal to the total channel count, the slicing logic `start = page * page_size` and
`channels = channels[start : start + page_size]` at `superset/reports/api.py:132-134`
returns nearly the entire `channels` list in a single response; this recreates the large
JSON payloads and frontend rendering slowdowns that the pagination change is intended to
prevent, especially on workspaces with tens of thousands of Slack channels.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/reports/schemas.py
**Line:** 64:64
**Comment:**
*Performance: `page_size` is only bounded to be positive, but not capped, so callers can request extremely large pages and effectively force the backend to return the full filtered channel set in one response. This defeats the pagination protection and can still cause large payloads and browser/backend slowdowns. Add a strict maximum page size.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
msyavuz
force-pushed
the
msyavuz/fix/slack-channels-large-workspace
branch
from
July 15, 2026 09:58
0e87405 to
3bbbe81
Compare
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SUMMARY
On workspaces with tens of thousands of Slack channels, the Alerts & Reports Slack recipient picker freezes the browser and the backend returns
504. The SlackV2 picker eagerly pulled every channel (conversations.listis paginated and has no search API), so it hit Slack rate limits during enumeration and rendered the entire list at once. The UI'ssearchString/exactMatchparams were also silently ignored (the endpoint readssearch_string/exact_match), so search never reached the server.Backend —
/report/slack_channels/now acceptspage/page_sizeand returns the requested page plus the fullcount, so the browser never receives the whole list.get_channels_with_searchis unchanged (caching andexecute.pyunaffected); slicing happens at the API layer over the warm-cached filtered set.Frontend — the SlackV2 picker moves from an eager
SelecttoAsyncSelect: debounced server-side search, one page at a time (can't freeze), plusallowNewOptionsso a channel ID can be pasted directly and used immediately. This is the guaranteed unblock for huge workspaces — SlackV2 send passes channel-ID strings straight tochat_postMessage, no list membership required. Edit-mode IDs resolve to names via a boundedexact_matchlookup.Large workspaces should schedule the existing
slack.cache_channelsCelery task to keep the cache warm (documented inconfig.py).BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — behavioral fix; no static screenshot captures the freeze. Manual repro/verification steps below.
TESTING INSTRUCTIONS
Automated:
pytest tests/unit_tests/reports/api_test.py(pagination + count) andnpm run test -- NotificationMethod(lazy pagination + direct channel-ID entry).Manual (with
ALERT_REPORT_SLACK_V2on and a Slack bot token configured):C0123456789) — it is accepted directly without the full list loading, and a test send delivers to that channel.slack.cache_channelstask once and confirm the dropdown browses quickly from the warm cache.ADDITIONAL INFORMATION
ALERT_REPORT_SLACK_V2(existing; unchanged)API change is additive/backwards-compatible: the endpoint gains optional
page/page_sizeand acountfield; the previously-ignoredsearch_string/exact_matchnow take effect.