codeant-ai-for-open-source[bot] commented on code in PR #42074: URL: https://github.com/apache/superset/pull/42074#discussion_r3584615805
########## superset-frontend/src/features/alerts/hooks/useSlackChannels.ts: ########## @@ -0,0 +1,191 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { useCallback, useRef, useState } from 'react'; +import { logging, SupersetClient, t } from '@superset-ui/core'; +import rison from 'rison'; +import { SlackChannel } from '../types'; + +export interface SlackChannelOption { + label: string; + value: string; +} + +export interface SlackChannelsResult { + data: SlackChannelOption[]; + totalCount: number; + has_more?: boolean; + next_cursor?: string | null; +} + +export interface FetchChannelsParams { + search: string; + page: number; + pageSize: number; + force?: boolean; +} + +export interface UseSlackChannelsResult { + fetchChannels: (params: FetchChannelsParams) => Promise<SlackChannelsResult>; + refreshChannels: () => Promise<void>; + isRefreshing: boolean; +} + +/** + * Cache types for managing Slack channel data + */ +type CursorCache = Record<string, string | null>; +type DataCache = Record<string, SlackChannelsResult>; +type PendingRequestsCache = Record<string, Promise<SlackChannelsResult>>; + +/** + * Custom hook for managing Slack channels with caching and pagination + */ +export function useSlackChannels( + onError?: (message: string) => void, +): UseSlackChannelsResult { + const cursorRef = useRef<CursorCache>({}); + const dataCache = useRef<DataCache>({}); + const pendingRequests = useRef<PendingRequestsCache>({}); + const [isRefreshing, setIsRefreshing] = useState(false); + + const fetchChannels = useCallback( + async ({ + search, + page, + pageSize, + force = false, + }: FetchChannelsParams): Promise<SlackChannelsResult> => { + const cacheKey = `${search}:${page}`; + + if (!force && dataCache.current[cacheKey]) { + return dataCache.current[cacheKey]; Review Comment: **Suggestion:** The cache key omits `pageSize`, so different query shapes collide in the same cache entry. After `refreshChannels` fetches page 0 with `pageSize: 999`, later normal reads for page 0 with `pageSize: 100` can incorrectly return the cached 999-sized payload, which breaks pagination assumptions and can flood the dropdown with an unexpectedly large result set. [cache] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Slack dropdown may load far more channels than expected. - ⚠️ Pagination counts become inconsistent after a refresh. - ⚠️ Performance degrades when large cached pages are reused. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. The Slack channels hook `useSlackChannels` defines a cache map `dataCache` keyed by `cacheKey = `${search}:${page}`` (superset-frontend/src/features/alerts/hooks/useSlackChannels.ts:74) and reused across calls to `fetchChannels`. 2. `fetchChannels` first checks `if (!force && dataCache.current[cacheKey]) { return dataCache.current[cacheKey]; }` (useSlackChannels.ts:76-78), meaning any prior result for the same `search` and `page` combination will be reused, regardless of the requested `pageSize`. 3. The `refreshChannels` helper (useSlackChannels.ts:166-184) clears caches and then calls `await fetchChannels({ search: '', page: 0, pageSize: 999, force: true });`, populating `dataCache.current['':0]` with a large payload sized for `pageSize: 999`. 4. Later, normal dropdown usage in `NotificationMethod` invokes `fetchSlackChannels('', 0, 100)` (NotificationMethod.tsx:173-183 inside the `initializeSlackRecipients` effect, and similarly for user searches), which internally calls `fetchSlackChannelsFromHook({ search: '', page: 0, pageSize: 100 })` with `force` defaulting to false. Because `dataCache.current['':0]` already holds the 999-sized result, `fetchChannels` returns that cached dataset without hitting the API, causing the Slack channel dropdown to receive an unexpectedly large page (999+ items) instead of the intended 100-item page, breaking pagination expectations and potentially impacting performance. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=b749a374396844c890a8f5bbcf721a4d&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=b749a374396844c890a8f5bbcf721a4d&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/features/alerts/hooks/useSlackChannels.ts **Line:** 74:77 **Comment:** *Cache: The cache key omits `pageSize`, so different query shapes collide in the same cache entry. After `refreshChannels` fetches page 0 with `pageSize: 999`, later normal reads for page 0 with `pageSize: 100` can incorrectly return the cached 999-sized payload, which breaks pagination assumptions and can flood the dropdown with an unexpectedly large result set. 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 ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42074&comment_hash=9e33e41bb6b2fbc31531d7f3a82cdcd0b66389f274f387762f328de80ade4990&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42074&comment_hash=9e33e41bb6b2fbc31531d7f3a82cdcd0b66389f274f387762f328de80ade4990&reaction=dislike'>👎</a> ########## superset-frontend/src/features/alerts/AlertReportModal.tsx: ########## @@ -2700,17 +2700,17 @@ const AlertReportModal: FunctionComponent<AlertReportModalProps> = ({ children: ( <> {notificationSettings.map((notificationSetting, i) => ( - <StyledNotificationMethodWrapper> + <StyledNotificationMethodWrapper key={`notification-${i}`}> Review Comment: **Suggestion:** Using the array index as the React key for notification rows causes component state reuse after deletions/reordering. Since each `NotificationMethod` keeps local state for recipients/CC/BCC, removing one row can shift indices and display/edit stale state in the wrong notification entry. [stale reference] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ Notification rows show recipients from wrong configuration. - ⚠️ Users may send alerts to unintended recipients/channels. - ⚠️ Editing multi-method alerts becomes confusing and error-prone. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Open the Alerts & Reports modal implemented in `AlertReportModal` (superset-frontend/src/features/alerts/AlertReportModal.tsx:69, export default withToasts(AlertReportModal) as seen via BulkRead). 2. In the Notification section, add at least two notification methods so `notificationSettings` contains multiple entries; they are rendered via `notificationSettings.map((notificationSetting, i) => <StyledNotificationMethodWrapper key={`notification-${i}`}> <NotificationMethod setting={notificationSetting} index={i} ... /></StyledNotificationMethodWrapper>)` (AlertReportModal.tsx:33-45 from BulkRead). 3. Each `NotificationMethod` row maintains local React state (e.g. `recipientValue`, `slackRecipients`, `ccValue`, `bccValue`) as shown in `NotificationMethod` (superset-frontend/src/features/alerts/components/NotificationMethod.tsx:18-29, 142-149). 4. Delete a notification in the middle of the list using `removeNotificationSetting(index)` (AlertReportModal.tsx:41-47) which does `settings.splice(index, 1); setNotificationSettings(settings);`. Because the parent uses the array index `i` as the React key (`key={`notification-${i}`}`) instead of a stable identifier, React reuses the component instance for the shifted row, so the `NotificationMethod` at the new index retains the previous row’s local state (recipients/CC/BCC), causing stale or incorrect data to appear in the wrong notification entry. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=d5df15531a1a4d05b14e9dbb0ca41cf6&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=d5df15531a1a4d05b14e9dbb0ca41cf6&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/features/alerts/AlertReportModal.tsx **Line:** 2703:2703 **Comment:** *Stale Reference: Using the array index as the React key for notification rows causes component state reuse after deletions/reordering. Since each `NotificationMethod` keeps local state for recipients/CC/BCC, removing one row can shift indices and display/edit stale state in the wrong notification entry. 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 ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42074&comment_hash=bc2f7a147e604588f685ebe072ae136c03111c3a97afa49c01bff412502db19d&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42074&comment_hash=bc2f7a147e604588f685ebe072ae136c03111c3a97afa49c01bff412502db19d&reaction=dislike'>👎</a> ########## superset-frontend/src/features/alerts/hooks/useSlackChannels.ts: ########## @@ -0,0 +1,191 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { useCallback, useRef, useState } from 'react'; +import { logging, SupersetClient, t } from '@superset-ui/core'; +import rison from 'rison'; +import { SlackChannel } from '../types'; + +export interface SlackChannelOption { + label: string; + value: string; +} + +export interface SlackChannelsResult { + data: SlackChannelOption[]; + totalCount: number; + has_more?: boolean; + next_cursor?: string | null; +} + +export interface FetchChannelsParams { + search: string; + page: number; + pageSize: number; + force?: boolean; +} + +export interface UseSlackChannelsResult { + fetchChannels: (params: FetchChannelsParams) => Promise<SlackChannelsResult>; + refreshChannels: () => Promise<void>; + isRefreshing: boolean; +} + +/** + * Cache types for managing Slack channel data + */ +type CursorCache = Record<string, string | null>; +type DataCache = Record<string, SlackChannelsResult>; +type PendingRequestsCache = Record<string, Promise<SlackChannelsResult>>; + +/** + * Custom hook for managing Slack channels with caching and pagination + */ +export function useSlackChannels( + onError?: (message: string) => void, +): UseSlackChannelsResult { + const cursorRef = useRef<CursorCache>({}); + const dataCache = useRef<DataCache>({}); + const pendingRequests = useRef<PendingRequestsCache>({}); + const [isRefreshing, setIsRefreshing] = useState(false); + + const fetchChannels = useCallback( + async ({ + search, + page, + pageSize, + force = false, + }: FetchChannelsParams): Promise<SlackChannelsResult> => { + const cacheKey = `${search}:${page}`; + + if (!force && dataCache.current[cacheKey]) { + return dataCache.current[cacheKey]; + } + + if (!force && cacheKey in pendingRequests.current) { + return pendingRequests.current[cacheKey]; + } + + const cursor = page > 0 ? cursorRef.current[cacheKey] : null; + + const params: Record<string, any> = { + types: ['public_channel', 'private_channel'], + limit: pageSize, + }; + + if (search) { + params.search_string = search; + } + + if (cursor) { + params.cursor = cursor; + } + + if (force) { + params.force = true; + } + + const queryString = rison.encode(params); + const endpoint = `/api/v1/report/slack_channels/?q=${queryString}`; + + const fetchPromise = (async () => { + try { + const response = await SupersetClient.get({ endpoint }); + + const { + result, + next_cursor: nextCursor, + has_more: hasMore, + } = response.json; + + if (nextCursor) { + cursorRef.current[`${search}:${page + 1}`] = nextCursor; + } + + const options = result.map((channel: SlackChannel) => ({ + label: channel.name, + value: channel.id, + })); + + const totalCount = hasMore + ? (page + 1) * pageSize + 1 + : page * pageSize + options.length; + + const responseData = { + data: options, + totalCount, + has_more: hasMore, + next_cursor: nextCursor, + }; + + dataCache.current[cacheKey] = responseData; + + return responseData; + } catch (error) { + logging.error('Failed to fetch Slack channels:', error); + + if (onError) { + onError( + t( + 'Unable to load Slack channels. Please check your Slack API token configuration.', + ), + ); + } + + return { + data: [], + totalCount: 0, + }; Review Comment: **Suggestion:** `fetchChannels` swallows all request failures and resolves with empty data, so callers cannot detect a hard failure and execute fallback behavior. In this PR, `NotificationMethod` expects a rejected promise to switch from Slack V2 to manual Slack input, but this catch-and-return path prevents that transition and leaves users stuck on an empty dropdown. [api mismatch] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ Slack V2 dropdown stays empty on API/token failures. - ⚠️ Manual Slack input fallback never activates as designed. - ⚠️ Users cannot select channels when Slack is misconfigured. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. In `NotificationMethod` (superset-frontend/src/features/alerts/components/NotificationMethod.tsx:48-52), Slack channel loading is wired via `useSlackChannels(addDangerToast)`, which returns `fetchChannels` as `fetchSlackChannelsFromHook`. 2. The wrapper `fetchSlackChannels` in `NotificationMethod` (NotificationMethod.tsx:75-123) calls `fetchSlackChannelsFromHook({ search, page, pageSize })` inside a `try` block and relies on catching thrown errors (`catch (error) { ... setUseSlackV1(true); onUpdate(...) to switch method from SlackV2 to Slack; }`) to trigger the fallback from Slack V2 dropdown to manual Slack input (Slack V1). 3. The underlying hook implementation `useSlackChannels` defines `fetchChannels` (superset-frontend/src/features/alerts/hooks/useSlackChannels.ts:67-163), which wraps the HTTP request in `fetchPromise`. On failure of `SupersetClient.get({ endpoint })` (e.g. invalid Slack API token or network error) the `catch (error)` block at lines 139-153 logs the error, optionally calls `onError`, and then returns a resolved value `{ data: [], totalCount: 0 }` instead of rethrowing. 4. Because `fetchChannels` always resolves successfully (even on hard failures), `fetchSlackChannelsFromHook` never rejects, so `NotificationMethod`’s `catch` block is never executed. In a real Slack API failure (misconfigured token, as suggested by the error message text at useSlackChannels.ts:145-146), the user sees an empty Slack channel dropdown while remaining in Slack V2 mode, and the intended manual-input fallback (Slack V1 method, error toast) is not triggered. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=933f3a5fa6564fd1bfc93e60aa2ed54d&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=933f3a5fa6564fd1bfc93e60aa2ed54d&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/features/alerts/hooks/useSlackChannels.ts **Line:** 139:153 **Comment:** *Api Mismatch: `fetchChannels` swallows all request failures and resolves with empty data, so callers cannot detect a hard failure and execute fallback behavior. In this PR, `NotificationMethod` expects a rejected promise to switch from Slack V2 to manual Slack input, but this catch-and-return path prevents that transition and leaves users stuck on an empty dropdown. 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 ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42074&comment_hash=806d0889bafdd9eb94e47194e08e3ebcf981534f380749d4d9e86ec48dcb930e&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42074&comment_hash=806d0889bafdd9eb94e47194e08e3ebcf981534f380749d4d9e86ec48dcb930e&reaction=dislike'>👎</a> -- 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]
