bito-code-review[bot] commented on code in PR #35832:
URL: https://github.com/apache/superset/pull/35832#discussion_r2461019908
##########
superset-frontend/src/features/alerts/components/NotificationMethod.tsx:
##########
@@ -249,84 +209,91 @@ export const NotificationMethod:
FunctionComponent<NotificationMethodProps> = ({
}
};
- const fetchSlackChannels = async ({
- searchString = '',
- types = [],
- exactMatch = false,
- force = false,
- }: {
- searchString?: string | undefined;
- types?: string[];
- exactMatch?: boolean | undefined;
- force?: boolean | undefined;
- } = {}): Promise<JsonResponse> => {
- const queryString = rison.encode({
- searchString,
- types,
- exactMatch,
- force,
- });
- const endpoint = `/api/v1/report/slack_channels/?q=${queryString}`;
- return SupersetClient.get({ endpoint });
- };
+ const fetchSlackChannels = async (
+ search: string,
+ page: number,
+ pageSize: number,
+ ): Promise<{
+ data: { label: string; value: string }[];
+ totalCount: number;
+ }> => {
+ try {
+ const cacheKey = `${search}:${page}`;
+ const cursor = page > 0 ? cursorRef.current[cacheKey] : null;
+
+ const params: Record<string, any> = {
+ types: ['public_channel', 'private_channel'],
+ limit: pageSize,
+ };
- 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(e => {
- // Fallback to slack v1 if slack v2 is not compatible
- setUseSlackV1(true);
- })
- .finally(() => {
- setMethodOptionsLoading(false);
- setIsSlackChannelsLoading(false);
- });
+ if (page === 0 && forceRefreshRef.current) {
+ params.force = true;
+ forceRefreshRef.current = false;
+ }
+
+ if (search) {
+ params.search_string = search;
+ }
+
+ if (cursor) {
+ params.cursor = cursor;
+ }
+
+ const queryString = rison.encode(params);
+ const endpoint = `/api/v1/report/slack_channels/?q=${queryString}`;
+ const response = await SupersetClient.get({ endpoint });
+
+ const { result, next_cursor, has_more } = response.json;
+
+ if (next_cursor) {
+ cursorRef.current[`${search}:${page + 1}`] = next_cursor;
+ }
+
+ const options = result.map((channel: SlackChannel) => ({
+ label: channel.name,
+ value: channel.id,
+ }));
+
+ const totalCount = has_more
+ ? (page + 1) * pageSize + 1 // Fake "has more"
+ : page * pageSize + options.length; // Last page
+
+ return {
+ data: options,
+ totalCount,
+ };
+ } catch (error) {
+ console.error('Failed to fetch Slack channels:', error);
+ setUseSlackV1(true);
+ throw error;
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing method update on SlackV2 error fallback</b></div>
<div id="fix">
When `fetchSlackChannels` fails, the code sets `useSlackV1` to true but
doesn't update the notification method from SlackV2 to Slack, leaving the
AsyncSelect component visible and broken instead of falling back to the Slack
input method. This breaks the recipients selection UI for Slack notifications.
To fix this, update the setting's method to `NotificationMethodOption.Slack` in
the error handler so the UI properly switches to the fallback input component.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
setUseSlackV1(true);
if (onUpdate && setting) {
onUpdate(index, {
...setting,
method: NotificationMethodOption.Slack,
});
}
throw error;
````
</div>
</details>
</div>
<small><i>Code Review Run <a
href=https://github.com/apache/superset/pull/35832#issuecomment-3443672626>#7b7415</a></i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
tests/unit_tests/utils/slack_test.py:
##########
@@ -211,8 +416,251 @@ def test_handle_pagination_multiple_pages(self, mocker):
]
mocker.patch("superset.utils.slack.get_slack_client",
return_value=mock_client)
+ # Search for "general" - should find 2 channels across 2 pages
+ result = get_channels_with_search(search_string="general", limit=100)
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Broken pagination for large limits in non-search
case</b></div>
<div id="fix">
The `get_channels_with_search` function fails to return the correct number
of channels when `limit > 200` in the non-search case, as it only fetches one
page from the Slack API which caps responses at 200 channels. This breaks
pagination for large limits and affects downstream consumers in
`superset.reports.api` and `superset.commands.report.execute`. The fix modifies
the non-search logic to stream through pages until the limit is reached,
ensuring complete results.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
@@ -115,18 +115,39 @@
if not search_string:
channels = []
slack_cursor = cursor
while len(channels) < limit:
response = client.conversations_list(
limit=min(200, limit - len(channels)),
cursor=slack_cursor,
exclude_archived=True,
types=types_param,
)
page_channels = [
channel_schema.load(channel) for channel in
response.data["channels"]
]
channels.extend(page_channels)
slack_cursor = response.data.get("response_metadata",
{}).get("next_cursor")
if not slack_cursor:
break
has_more = bool(slack_cursor) and len(channels) >= limit
return {
"result": channels[:limit],
"next_cursor": slack_cursor if has_more else None,
"has_more": has_more,
}
```
</div>
</details>
</div>
<small><i>Code Review Run <a
href=https://github.com/apache/superset/pull/35832#issuecomment-3443672626>#7b7415</a></i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
