korbit-ai[bot] commented on code in PR #35832:
URL: https://github.com/apache/superset/pull/35832#discussion_r2460992726
##########
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:
### Unbounded cursor cache growth <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The cursor cache key generation creates potential memory leaks and
inefficient cache usage by storing cursors for every search term and page
combination without cleanup.
###### Why this matters
This unbounded cache growth can consume significant memory over time,
especially with frequent searches or long-running sessions, and may cause
performance degradation.
###### Suggested change ∙ *Feature Preview*
Implement cache size limits and cleanup strategy. Consider using a Map with
LRU eviction or limit cache entries:
```typescript
const MAX_CACHE_SIZE = 50;
const cacheKeys = Object.keys(cursorRef.current);
if (cacheKeys.length >= MAX_CACHE_SIZE) {
// Remove oldest entries
cacheKeys.slice(0, cacheKeys.length - MAX_CACHE_SIZE + 1)
.forEach(key => delete cursorRef.current[key]);
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bd979f1e-24f4-4b48-bca2-ff6594d5392f/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bd979f1e-24f4-4b48-bca2-ff6594d5392f?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bd979f1e-24f4-4b48-bca2-ff6594d5392f?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bd979f1e-24f4-4b48-bca2-ff6594d5392f?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bd979f1e-24f4-4b48-bca2-ff6594d5392f)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:86c82e0e-7d98-4f11-856f-737b47f5dcd2 -->
[](86c82e0e-7d98-4f11-856f-737b47f5dcd2)
##########
superset/reports/schemas.py:
##########
@@ -58,6 +58,9 @@
"items": {"type": "string", "enum": ["public_channel",
"private_channel"]},
},
"exact_match": {"type": "boolean"},
+ "cursor": {"type": ["string", "null"]},
+ "limit": {"type": "integer", "default": 100},
Review Comment:
### Missing limit validation constraints <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The limit field has a default value in the JSON schema but lacks validation
constraints to prevent potential abuse or system overload.
###### Why this matters
Without proper validation, clients could request extremely large limits
(e.g., millions of records) which could cause memory exhaustion, database
timeouts, or denial of service conditions when fetching Slack channels.
###### Suggested change ∙ *Feature Preview*
Add validation constraints to the limit field to ensure reasonable bounds:
```json
"limit": {
"type": "integer",
"default": 100,
"minimum": 1,
"maximum": 1000
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/03d07551-efa9-4692-99c9-55970331d4bc/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/03d07551-efa9-4692-99c9-55970331d4bc?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/03d07551-efa9-4692-99c9-55970331d4bc?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/03d07551-efa9-4692-99c9-55970331d4bc?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/03d07551-efa9-4692-99c9-55970331d4bc)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:a7901fcd-c828-4e9f-afbd-1769ffdf326d -->
[](a7901fcd-c828-4e9f-afbd-1769ffdf326d)
##########
superset/utils/slack.py:
##########
@@ -92,52 +92,100 @@ def get_channels_with_search(
search_string: str = "",
types: Optional[list[SlackChannelTypes]] = None,
exact_match: bool = False,
- force: bool = False,
-) -> list[SlackChannelSchema]:
+ cursor: Optional[str] = None,
+ limit: int = 100,
+) -> dict[str, Any]:
"""
- The slack api is paginated but does not include search, so we need to fetch
- all channels and filter them ourselves
- This will search by slack name or id
+ Fetches Slack channels with pagination and search support.
+
+ Returns a dict with:
+ - result: list of channels
+ - next_cursor: cursor for next page (None if no more pages)
+ - has_more: boolean indicating if more pages exist
+
+ The Slack API is paginated but does not include search. We handle two
cases:
+ 1. WITHOUT search: Fetch single page from Slack API (fast, no timeout)
+ 2. WITH search: Stream through Slack API pages until we find enough matches
+ (stops early to prevent timeouts on large workspaces)
"""
try:
- channels = get_channels(
- force=force,
- cache_timeout=app.config["SLACK_CACHE_TIMEOUT"],
+ client = get_slack_client()
+ channel_schema = SlackChannelSchema()
+
+ types_param = (
+ ",".join(types)
+ if types and len(types) < len(SlackChannelTypes)
+ else ",".join(SlackChannelTypes)
)
- except (SlackClientError, SlackApiError) as ex:
- raise SupersetException(f"Failed to list channels: {ex}") from ex
- if types and not len(types) == len(SlackChannelTypes):
- conditions: list[Callable[[SlackChannelSchema], bool]] = []
- if SlackChannelTypes.PUBLIC in types:
- conditions.append(lambda channel: not channel["is_private"])
- if SlackChannelTypes.PRIVATE in types:
- conditions.append(lambda channel: channel["is_private"])
-
- channels = [
- channel for channel in channels if any(cond(channel) for cond in
conditions)
- ]
-
- # The search string can be multiple channels separated by commas
- if search_string:
- search_array = recipients_string_to_list(search_string)
- channels = [
- channel
- for channel in channels
- if any(
- (
- search.lower() == channel["name"].lower()
- or search.lower() == channel["id"].lower()
- if exact_match
- else (
- search.lower() in channel["name"].lower()
- or search.lower() in channel["id"].lower()
- )
- )
- for search in search_array
+ if not search_string:
+ response = client.conversations_list(
+ limit=limit,
+ cursor=cursor,
+ exclude_archived=True,
+ types=types_param,
)
- ]
- return channels
+
+ channels = [
+ channel_schema.load(channel) for channel in
response.data["channels"]
+ ]
+
+ next_cursor = response.data.get("response_metadata",
{}).get("next_cursor")
+
+ return {
+ "result": channels,
+ "next_cursor": next_cursor,
+ "has_more": bool(next_cursor),
+ }
+
+ matches: list[SlackChannelSchema] = []
+ slack_cursor = cursor
+ max_pages_to_fetch = 50
+ pages_fetched = 0
+
+ while len(matches) < limit and pages_fetched < max_pages_to_fetch:
+ response = client.conversations_list(
+ limit=200,
+ cursor=slack_cursor,
+ exclude_archived=True,
+ types=types_param,
+ )
+
+ for channel_data in response.data["channels"]:
+ channel = channel_schema.load(channel_data)
+
+ if exact_match:
+ if (
+ search_string.lower() == channel["name"].lower()
+ or search_string.lower() == channel["id"].lower()
+ ):
+ matches.append(channel)
+ else:
+ if (
+ search_string.lower() in channel["name"].lower()
+ or search_string.lower() in channel["id"].lower()
+ ):
+ matches.append(channel)
Review Comment:
### Redundant string lowercasing in search loop <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Redundant string lowercasing operations are performed multiple times within
the search loop, causing unnecessary computational overhead.
###### Why this matters
The search_string.lower() operation is called repeatedly for each channel
comparison, which creates unnecessary CPU overhead when processing large
numbers of channels across multiple pages.
###### Suggested change ∙ *Feature Preview*
Cache the lowercased search string outside the loop to avoid repeated string
operations:
```python
search_lower = search_string.lower()
while len(matches) < limit and pages_fetched < max_pages_to_fetch:
# ... existing code ...
for channel_data in response.data["channels"]:
channel = channel_schema.load(channel_data)
channel_name_lower = channel["name"].lower()
channel_id_lower = channel["id"].lower()
if exact_match:
if search_lower == channel_name_lower or search_lower ==
channel_id_lower:
matches.append(channel)
else:
if search_lower in channel_name_lower or search_lower in
channel_id_lower:
matches.append(channel)
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/31a7fd96-f51c-4c60-9a89-077d7ca406c0/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/31a7fd96-f51c-4c60-9a89-077d7ca406c0?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/31a7fd96-f51c-4c60-9a89-077d7ca406c0?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/31a7fd96-f51c-4c60-9a89-077d7ca406c0?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/31a7fd96-f51c-4c60-9a89-077d7ca406c0)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:aa0b1ee3-bb2e-4ea9-8866-3de64bab1257 -->
[](aa0b1ee3-bb2e-4ea9-8866-3de64bab1257)
##########
superset/utils/slack.py:
##########
@@ -92,52 +92,100 @@ def get_channels_with_search(
search_string: str = "",
types: Optional[list[SlackChannelTypes]] = None,
exact_match: bool = False,
- force: bool = False,
-) -> list[SlackChannelSchema]:
+ cursor: Optional[str] = None,
+ limit: int = 100,
+) -> dict[str, Any]:
"""
- The slack api is paginated but does not include search, so we need to fetch
- all channels and filter them ourselves
- This will search by slack name or id
+ Fetches Slack channels with pagination and search support.
+
+ Returns a dict with:
+ - result: list of channels
+ - next_cursor: cursor for next page (None if no more pages)
+ - has_more: boolean indicating if more pages exist
+
+ The Slack API is paginated but does not include search. We handle two
cases:
+ 1. WITHOUT search: Fetch single page from Slack API (fast, no timeout)
+ 2. WITH search: Stream through Slack API pages until we find enough matches
+ (stops early to prevent timeouts on large workspaces)
"""
try:
- channels = get_channels(
- force=force,
- cache_timeout=app.config["SLACK_CACHE_TIMEOUT"],
+ client = get_slack_client()
+ channel_schema = SlackChannelSchema()
+
+ types_param = (
+ ",".join(types)
+ if types and len(types) < len(SlackChannelTypes)
+ else ",".join(SlackChannelTypes)
)
- except (SlackClientError, SlackApiError) as ex:
- raise SupersetException(f"Failed to list channels: {ex}") from ex
- if types and not len(types) == len(SlackChannelTypes):
- conditions: list[Callable[[SlackChannelSchema], bool]] = []
- if SlackChannelTypes.PUBLIC in types:
- conditions.append(lambda channel: not channel["is_private"])
- if SlackChannelTypes.PRIVATE in types:
- conditions.append(lambda channel: channel["is_private"])
-
- channels = [
- channel for channel in channels if any(cond(channel) for cond in
conditions)
- ]
-
- # The search string can be multiple channels separated by commas
- if search_string:
- search_array = recipients_string_to_list(search_string)
- channels = [
- channel
- for channel in channels
- if any(
- (
- search.lower() == channel["name"].lower()
- or search.lower() == channel["id"].lower()
- if exact_match
- else (
- search.lower() in channel["name"].lower()
- or search.lower() in channel["id"].lower()
- )
- )
- for search in search_array
+ if not search_string:
+ response = client.conversations_list(
+ limit=limit,
+ cursor=cursor,
+ exclude_archived=True,
+ types=types_param,
)
- ]
- return channels
+
+ channels = [
+ channel_schema.load(channel) for channel in
response.data["channels"]
+ ]
+
+ next_cursor = response.data.get("response_metadata",
{}).get("next_cursor")
+
+ return {
+ "result": channels,
+ "next_cursor": next_cursor,
+ "has_more": bool(next_cursor),
+ }
+
+ matches: list[SlackChannelSchema] = []
+ slack_cursor = cursor
+ max_pages_to_fetch = 50
+ pages_fetched = 0
+
+ while len(matches) < limit and pages_fetched < max_pages_to_fetch:
+ response = client.conversations_list(
+ limit=200,
+ cursor=slack_cursor,
+ exclude_archived=True,
+ types=types_param,
+ )
+
+ for channel_data in response.data["channels"]:
+ channel = channel_schema.load(channel_data)
+
+ if exact_match:
+ if (
+ search_string.lower() == channel["name"].lower()
+ or search_string.lower() == channel["id"].lower()
+ ):
+ matches.append(channel)
+ else:
+ if (
+ search_string.lower() in channel["name"].lower()
+ or search_string.lower() in channel["id"].lower()
+ ):
+ matches.append(channel)
+
+ if len(matches) >= limit:
+ break
Review Comment:
### Premature schema loading before search filtering <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Schema loading is performed for all channels even when they don't match the
search criteria, wasting processing time on irrelevant data.
###### Why this matters
The channel_schema.load() operation is computationally expensive and is
being called before checking if the channel matches the search criteria,
leading to unnecessary processing overhead for non-matching channels.
###### Suggested change ∙ *Feature Preview*
Move the schema loading after the search criteria check to avoid processing
irrelevant channels:
```python
for channel_data in response.data["channels"]:
# Check search criteria on raw data first
channel_name_lower = channel_data["name"].lower()
channel_id_lower = channel_data["id"].lower()
match_found = False
if exact_match:
match_found = search_lower == channel_name_lower or search_lower ==
channel_id_lower
else:
match_found = search_lower in channel_name_lower or search_lower in
channel_id_lower
if match_found:
channel = channel_schema.load(channel_data)
matches.append(channel)
if len(matches) >= limit:
break
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fc43f9aa-bd76-404a-b9b0-27ded4456fde/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fc43f9aa-bd76-404a-b9b0-27ded4456fde?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fc43f9aa-bd76-404a-b9b0-27ded4456fde?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fc43f9aa-bd76-404a-b9b0-27ded4456fde?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fc43f9aa-bd76-404a-b9b0-27ded4456fde)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:83de18d1-832a-4e1d-aa3e-227c44f6be6e -->
[](83de18d1-832a-4e1d-aa3e-227c44f6be6e)
--
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]
