fix: PR comments

superset-frontend/src/features/alerts/AlertReportModal.tsx

@@ -2600,6 +2600,7 @@ const AlertReportModal: FunctionComponent<AlertReportModalProps> = ({
                         email_subject={currentAlert?.email_subject || ''}
                         defaultSubject={emailSubject || ''}
                         setErrorSubject={handleErrorUpdate}
+                        addDangerToast={addDangerToast}
                       />
                     </StyledNotificationMethodWrapper>
                   ))}

superset-frontend/src/features/alerts/components/NotificationMethod.test.tsx

@@ -38,6 +38,7 @@ const mockOnUpdate = jest.fn();
 const mockOnRemove = jest.fn();
 const mockOnInputChange = jest.fn();
 const mockSetErrorSubject = jest.fn();
+const mockAddDangerToast = jest.fn();
 
 const mockSetting: NotificationSetting = {
   method: NotificationMethodOption.Email,
@@ -78,6 +79,7 @@ test('NotificationMethod - should render the component', () => {
       email_subject={mockEmailSubject}
       defaultSubject={mockDefaultSubject}
       setErrorSubject={mockSetErrorSubject}
+      addDangerToast={mockAddDangerToast}
     />,
   );
 
@@ -97,6 +99,7 @@ test('NotificationMethod - should call onRemove when the delete button is clicke
       email_subject={mockEmailSubject}
       defaultSubject={mockDefaultSubject}
       setErrorSubject={mockSetErrorSubject}
+      addDangerToast={mockAddDangerToast}
     />,
   );
 
@@ -117,6 +120,7 @@ test('NotificationMethod - should update recipient value when input changes', ()
       email_subject={mockEmailSubject}
       defaultSubject={mockDefaultSubject}
       setErrorSubject={mockSetErrorSubject}
+      addDangerToast={mockAddDangerToast}
     />,
   );
 
@@ -142,6 +146,7 @@ test('NotificationMethod - should call onRecipientsChange when the recipients va
       email_subject={mockEmailSubject}
       defaultSubject={mockDefaultSubject}
       setErrorSubject={mockSetErrorSubject}
+      addDangerToast={mockAddDangerToast}
     />,
   );
 
@@ -326,6 +331,7 @@ test('shows the right combo when ff is false', async () => {
       email_subject={mockEmailSubject}
       defaultSubject={mockDefaultSubject}
       setErrorSubject={mockSetErrorSubject}
+      addDangerToast={mockAddDangerToast}
     />,
   );
 
@@ -363,6 +369,7 @@ test('shows the textbox when the fetch fails', async () => {
       email_subject={mockEmailSubject}
       defaultSubject={mockDefaultSubject}
       setErrorSubject={mockSetErrorSubject}
+      addDangerToast={mockAddDangerToast}
     />,
   );
 
@@ -405,6 +412,7 @@ test('shows the dropdown when ff is true and slackChannels succeed', async () =>
       email_subject={mockEmailSubject}
       defaultSubject={mockDefaultSubject}
       setErrorSubject={mockSetErrorSubject}
+      addDangerToast={mockAddDangerToast}
     />,
   );
 
@@ -440,6 +448,7 @@ test('shows the textarea when ff is true and slackChannels fail', async () => {
       email_subject={mockEmailSubject}
       defaultSubject={mockDefaultSubject}
       setErrorSubject={mockSetErrorSubject}
+      addDangerToast={mockAddDangerToast}
     />,
   );
 
@@ -475,6 +484,7 @@ test('shows the textarea when ff is true and slackChannels fail and slack is sel
       email_subject={mockEmailSubject}
       defaultSubject={mockDefaultSubject}
       setErrorSubject={mockSetErrorSubject}
+      addDangerToast={mockAddDangerToast}
     />,
   );
 
@@ -503,6 +513,7 @@ test('shows the textarea when ff is true, slackChannels fail and slack is select
       email_subject={mockEmailSubject}
       defaultSubject={mockDefaultSubject}
       setErrorSubject={mockSetErrorSubject}
+      addDangerToast={mockAddDangerToast}
     />,
   );
 
@@ -536,6 +547,7 @@ test('NotificationMethod - AsyncSelect should render for SlackV2 method', async
       email_subject={mockEmailSubject}
       defaultSubject={mockDefaultSubject}
       setErrorSubject={mockSetErrorSubject}
+      addDangerToast={mockAddDangerToast}
     />,
   );
 
@@ -574,6 +586,7 @@ test('NotificationMethod - AsyncSelect should handle SlackV2 with valid API resp
       email_subject={mockEmailSubject}
       defaultSubject={mockDefaultSubject}
       setErrorSubject={mockSetErrorSubject}
+      addDangerToast={mockAddDangerToast}
     />,
   );
 
@@ -615,6 +628,7 @@ test('NotificationMethod - AsyncSelect should render refresh button for SlackV2'
       email_subject={mockEmailSubject}
       defaultSubject={mockDefaultSubject}
       setErrorSubject={mockSetErrorSubject}
+      addDangerToast={mockAddDangerToast}
     />,
   );
 
@@ -626,7 +640,7 @@ test('NotificationMethod - AsyncSelect should render refresh button for SlackV2'
   });
 });
 
-test('NotificationMethod - AsyncSelect should clear recipients and reload channels when refresh button clicked', async () => {
+test('NotificationMethod - AsyncSelect should reload channels without clearing recipients when refresh button clicked', async () => {
   window.featureFlags = { [FeatureFlag.AlertReportSlackV2]: true };
   const mockChannels = [
     {
@@ -637,7 +651,7 @@ test('NotificationMethod - AsyncSelect should clear recipients and reload channe
     },
   ];
 
-  jest.spyOn(SupersetClient, 'get').mockResolvedValue({
+  const getSpy = jest.spyOn(SupersetClient, 'get').mockResolvedValue({
     json: {
       result: mockChannels,
       next_cursor: null,
@@ -658,6 +672,7 @@ test('NotificationMethod - AsyncSelect should clear recipients and reload channe
       email_subject={mockEmailSubject}
       defaultSubject={mockDefaultSubject}
       setErrorSubject={mockSetErrorSubject}
+      addDangerToast={mockAddDangerToast}
     />,
   );
 
@@ -666,19 +681,25 @@ test('NotificationMethod - AsyncSelect should clear recipients and reload channe
     expect(screen.getByTestId('refresh-slack-channels')).toBeInTheDocument();
   });
 
+  const initialCallCount = getSpy.mock.calls.length;
+
   // Click refresh button
   const refreshButton = screen.getByTestId('refresh-slack-channels');
   fireEvent.click(refreshButton);
 
-  // Verify onUpdate was called with empty recipients
+  // Verify the API was called again to reload channels
   await waitFor(() => {
-    expect(mockOnUpdate).toHaveBeenCalledWith(
-      0,
-      expect.objectContaining({
-        recipients: '',
-      }),
-    );
+    expect(getSpy.mock.calls.length).toBeGreaterThan(initialCallCount);
   });
+
+  // Verify onUpdate was NOT called with empty recipients (recipients should be preserved)
+  const updateCalls = mockOnUpdate.mock.calls;
+  const callsWithEmptyRecipients = updateCalls.filter(
+    call => call[1]?.recipients === '',
+  );
+  expect(callsWithEmptyRecipients).toHaveLength(0);
+
+  getSpy.mockRestore();
 });
 
 test('NotificationMethod - data cache prevents redundant API calls when selecting channels', async () => {
@@ -689,6 +710,16 @@ test('NotificationMethod - data cache prevents redundant API calls when selectin
     { name: 'random', id: 'C002', is_private: false, is_member: true },
   ];
 
+  const mockSettingWithoutRecipients: NotificationSetting = {
+    method: NotificationMethodOption.SlackV2,
+    recipients: '', // Empty to avoid initialization call
+    options: [
+      NotificationMethodOption.Email,
+      NotificationMethodOption.Slack,
+      NotificationMethodOption.SlackV2,
+    ],
+  };
+
   let callCount = 0;
   const getSpy = jest
     .spyOn(SupersetClient, 'get')
@@ -707,7 +738,7 @@ test('NotificationMethod - data cache prevents redundant API calls when selectin
 
   const { getByRole } = render(
     <NotificationMethod
-      setting={mockSettingSlackV2}
+      setting={mockSettingWithoutRecipients}
       index={0}
       onUpdate={mockOnUpdate}
       onRemove={mockOnRemove}
@@ -715,6 +746,7 @@ test('NotificationMethod - data cache prevents redundant API calls when selectin
       email_subject={mockEmailSubject}
       defaultSubject={mockDefaultSubject}
       setErrorSubject={mockSetErrorSubject}
+      addDangerToast={mockAddDangerToast}
     />,
   );
 
@@ -736,7 +768,287 @@ test('NotificationMethod - data cache prevents redundant API calls when selectin
 
   await new Promise(resolve => setTimeout(resolve, 200));
 
+  // Should still be 1 because of caching
   expect(callCount).toBe(1);
 
   getSpy.mockRestore();
 });
+
+test('NotificationMethod - should preserve selected channels on refresh', async () => {
+  window.featureFlags = { [FeatureFlag.AlertReportSlackV2]: true };
+  const mockChannels = [
+    { id: 'C001', name: 'general', is_private: false, is_member: true },
+    { id: 'C002', name: 'random', is_private: false, is_member: true },
+    { id: 'C003', name: 'engineering', is_private: false, is_member: true },
+  ];
+
+  const mockSettingWithRecipients: NotificationSetting = {
+    method: NotificationMethodOption.SlackV2,
+    recipients: 'C001,C002',
+    options: [
+      NotificationMethodOption.Email,
+      NotificationMethodOption.Slack,
+      NotificationMethodOption.SlackV2,
+    ],
+  };
+
+  const getSpy = jest.spyOn(SupersetClient, 'get').mockImplementation(
+    () =>
+      Promise.resolve({
+        json: {
+          result: mockChannels,
+          next_cursor: null,
+          has_more: false,
+        },
+      }) as unknown as Promise<Response | JsonResponse | TextResponse>,
+  );
+
+  const { getByTestId } = render(
+    <NotificationMethod
+      setting={mockSettingWithRecipients}
+      index={0}
+      onUpdate={mockOnUpdate}
+      onRemove={mockOnRemove}
+      onInputChange={mockOnInputChange}
+      email_subject={mockEmailSubject}
+      defaultSubject={mockDefaultSubject}
+      setErrorSubject={mockSetErrorSubject}
+      addDangerToast={mockAddDangerToast}
+    />,
+  );
+
+  // Wait for initial load
+  await waitFor(
+    () => {
+      expect(getByTestId('refresh-slack-channels')).toBeInTheDocument();
+    },
+    { timeout: 3000 },
+  );
+
+  const initialCallCount = getSpy.mock.calls.length;
+
+  // Find and click the refresh button
+  const refreshButton = getByTestId('refresh-slack-channels');
+  fireEvent.click(refreshButton);
+
+  // Wait for refresh to complete
+  await waitFor(
+    () => {
+      expect(getSpy.mock.calls.length).toBeGreaterThan(initialCallCount);
+    },
+    { timeout: 3000 },
+  );
+
+  // Verify that recipients are preserved (not cleared to empty string)
+  const updateCalls = mockOnUpdate.mock.calls;
+  const callsWithEmptyRecipients = updateCalls.filter(
+    call => call[1]?.recipients === '',
+  );
+  expect(callsWithEmptyRecipients).toHaveLength(0);
+
+  getSpy.mockRestore();
+});
+
+test('NotificationMethod - should initialize Slack recipients when editing existing alert', async () => {
+  window.featureFlags = { [FeatureFlag.AlertReportSlackV2]: true };
+  const mockChannels = [
+    { id: 'C001', name: 'general', is_private: false, is_member: true },
+    { id: 'C002', name: 'random', is_private: false, is_member: true },
+    { id: 'C003', name: 'engineering', is_private: false, is_member: true },
+  ];
+
+  const mockSettingWithRecipients: NotificationSetting = {
+    method: NotificationMethodOption.SlackV2,
+    recipients: 'C001,C002',
+    options: [
+      NotificationMethodOption.Email,
+      NotificationMethodOption.Slack,
+      NotificationMethodOption.SlackV2,
+    ],
+  };
+
+  const getSpy = jest.spyOn(SupersetClient, 'get').mockImplementation(
+    () =>
+      Promise.resolve({
+        json: {
+          result: mockChannels,
+          next_cursor: null,
+          has_more: false,
+        },
+      }) as unknown as Promise<Response | JsonResponse | TextResponse>,
+  );
+
+  const { getByTestId } = render(
+    <NotificationMethod
+      setting={mockSettingWithRecipients}
+      index={0}
+      onUpdate={mockOnUpdate}
+      onRemove={mockOnRemove}
+      onInputChange={mockOnInputChange}
+      email_subject={mockEmailSubject}
+      defaultSubject={mockDefaultSubject}
+      setErrorSubject={mockSetErrorSubject}
+      addDangerToast={mockAddDangerToast}
+    />,
+  );
+
+  // Wait for the component to render and recipients dropdown to be available
+  await waitFor(
+    () => {
+      expect(getByTestId('recipients')).toBeInTheDocument();
+    },
+    { timeout: 3000 },
+  );
+
+  // Verify that the component fetched channels for initialization
+  await waitFor(
+    () => {
+      expect(getSpy).toHaveBeenCalled();
+    },
+    { timeout: 3000 },
+  );
+
+  // Verify that the component attempted to initialize recipients
+  expect(getSpy).toHaveBeenCalledWith(
+    expect.objectContaining({
+      endpoint: expect.stringContaining('/api/v1/report/slack_channels/'),
+    }),
+  );
+
+  getSpy.mockRestore();
+});
+
+test('NotificationMethod - should preserve selected channels during search and pagination', async () => {
+  window.featureFlags = { [FeatureFlag.AlertReportSlackV2]: true };
+
+  // Mock channels for different pages and search results
+  const page1Channels = [
+    { id: 'C001', name: 'general', is_private: false, is_member: true },
+    { id: 'C002', name: 'random', is_private: false, is_member: true },
+    { id: 'C003', name: 'engineering', is_private: false, is_member: true },
+  ];
+
+  const page2Channels = [
+    { id: 'C004', name: 'design', is_private: false, is_member: true },
+    { id: 'C005', name: 'product', is_private: false, is_member: true },
+  ];
+
+  const searchChannels = [
+    { id: 'C003', name: 'engineering', is_private: false, is_member: true },
+    { id: 'C006', name: 'eng-team', is_private: false, is_member: true },
+  ];
+
+  // Setting with pre-selected recipients
+  const mockSettingWithRecipients: NotificationSetting = {
+    method: NotificationMethodOption.SlackV2,
+    recipients: 'C001,C002', // Pre-selected: general, random
+    options: [
+      NotificationMethodOption.Email,
+      NotificationMethodOption.Slack,
+      NotificationMethodOption.SlackV2,
+    ],
+  };
+
+  // Mock API to return different results based on search string
+  const getSpy = jest
+    .spyOn(SupersetClient, 'get')
+    .mockImplementation(({ endpoint }) => {
+      const url = endpoint as string;
+      const isSearch = url.includes('search_string');
+      const isPage2 = url.includes('cursor');
+
+      let result;
+      let next_cursor = null;
+      let has_more = false;
+
+      if (isSearch) {
+        // Search returns matching channels
+        result = searchChannels;
+      } else if (isPage2) {
+        // Page 2 returns additional channels
+        result = page2Channels;
+      } else {
+        // Page 1 returns initial channels
+        result = page1Channels;
+        next_cursor = 'page2cursor';
+        has_more = true;
+      }
+
+      return Promise.resolve({
+        json: {
+          result,
+          next_cursor,
+          has_more,
+        },
+      }) as unknown as Promise<Response | JsonResponse | TextResponse>;
+    });
+
+  const { getByTestId } = render(
+    <NotificationMethod
+      setting={mockSettingWithRecipients}
+      index={0}
+      onUpdate={mockOnUpdate}
+      onRemove={mockOnRemove}
+      onInputChange={mockOnInputChange}
+      email_subject={mockEmailSubject}
+      defaultSubject={mockDefaultSubject}
+      setErrorSubject={mockSetErrorSubject}
+      addDangerToast={mockAddDangerToast}
+    />,
+  );
+
+  // Wait for initial page to load
+  await waitFor(
+    () => {
+      expect(getByTestId('recipients')).toBeInTheDocument();
+    },
+    { timeout: 3000 },
+  );
+
+  // Verify initial API call was made
+  await waitFor(
+    () => {
+      expect(getSpy).toHaveBeenCalled();
+    },
+    { timeout: 3000 },
+  );
+
+  const initialCallCount = getSpy.mock.calls.length;
+
+  // Simulate search and pagination scenarios
+  // Note: AsyncSelect is complex to simulate directly, so we verify via API calls
+  // that selections aren't cleared during search and pagination operations
+
+  // Verify no calls to onUpdate cleared the recipients
+  let updateCalls = mockOnUpdate.mock.calls;
+  let callsWithEmptyRecipients = updateCalls.filter(
+    call => call[1]?.recipients === '',
+  );
+  expect(callsWithEmptyRecipients).toHaveLength(0);
+
+  // Simulate pagination by triggering additional API calls
+  // In the real component, this happens when user scrolls in AsyncSelect
+  await waitFor(
+    () => {
+      // The component should have made at least the initial call
+      expect(getSpy.mock.calls.length).toBeGreaterThanOrEqual(initialCallCount);
+    },
+    { timeout: 3000 },
+  );
+
+  // After pagination, verify recipients are still not cleared
+  updateCalls = mockOnUpdate.mock.calls;
+  callsWithEmptyRecipients = updateCalls.filter(
+    call => call[1]?.recipients === '',
+  );
+  expect(callsWithEmptyRecipients).toHaveLength(0);
+
+  // Verify that all onUpdate calls (if any) preserved the recipients
+  // Every call should either have no recipients field touched, or have recipients
+  const updateCallsWithEmptyRecipientsCheck = updateCalls.every(
+    call => !call[1]?.hasOwnProperty('recipients') || call[1].recipients !== '',
+  );
+  expect(updateCallsWithEmptyRecipientsCheck).toBe(true);
+
+  getSpy.mockRestore();
+});

superset-frontend/src/features/alerts/components/NotificationMethod.tsx

@@ -22,6 +22,7 @@ import {
   ChangeEvent,
   useMemo,
   useRef,
+  useEffect,
 } from 'react';
 import rison from 'rison';
 
@@ -29,6 +30,7 @@ import {
   FeatureFlag,
   SupersetClient,
   isFeatureEnabled,
+  logging,
   styled,
   t,
   useTheme,
@@ -136,6 +138,7 @@ interface NotificationMethodProps {
   email_subject: string;
   defaultSubject: string;
   setErrorSubject: (hasError: boolean) => void;
+  addDangerToast?: (msg: string) => void;
 }
 
 export const mapSlackValues = ({
@@ -168,6 +171,7 @@ export const NotificationMethod: FunctionComponent<NotificationMethodProps> = ({
   email_subject,
   defaultSubject,
   setErrorSubject,
+  addDangerToast,
 }) => {
   const { method, recipients, cc, bcc, options } = setting || {};
   const [recipientValue, setRecipientValue] = useState<string>(
@@ -193,6 +197,7 @@ export const NotificationMethod: FunctionComponent<NotificationMethodProps> = ({
   const [refreshKey, setRefreshKey] = useState(0);
   const forceRefreshRef = useRef(false);
   const [isRefreshing, setIsRefreshing] = useState(false);
+  const hasShownErrorToast = useRef(false); // Track if we've shown the error toast
 
   const onMethodChange = (selected: {
     label: string;
@@ -278,13 +283,40 @@ export const NotificationMethod: FunctionComponent<NotificationMethodProps> = ({
 
       return responseData;
     } catch (error) {
-      console.error('Failed to fetch Slack channels:', error);
+      logging.error('Failed to fetch Slack channels:', error);
+
+      // Show user-friendly error message
+      if (addDangerToast && !hasShownErrorToast.current) {
+        addDangerToast(
+          t(
+            'Unable to load Slack channels. Please check your Slack API token configuration. ' +
+              'Switching to manual channel input.',
+          ),
+        );
+        hasShownErrorToast.current = true;
+      }
+
+      // Fallback to Slack v1 without clearing recipients to prevent data loss
       setUseSlackV1(true);
-      onMethodChange({
-        label: NotificationMethodOption.Slack,
-        value: NotificationMethodOption.Slack,
-      });
-      throw error;
+
+      // Auto-switch to Slack V1 in the notification method dropdown
+      if (
+        onUpdate &&
+        setting &&
+        setting.method === NotificationMethodOption.SlackV2
+      ) {
+        onUpdate(index, {
+          ...setting,
+          method: NotificationMethodOption.Slack, // Switch from SlackV2 to Slack V1
+          // Keep recipients to preserve user data
+        });
+      }
+
+      // Return empty result instead of throwing to allow UI to continue
+      return {
+        data: [],
+        totalCount: 0,
+      };
     }
   };
 
@@ -292,29 +324,75 @@ export const NotificationMethod: FunctionComponent<NotificationMethodProps> = ({
     setIsRefreshing(true);
 
     try {
+      // Clear cache and cursor to force fresh data fetch
       forceRefreshRef.current = true;
       cursorRef.current = {};
       dataCache.current = {};
 
-      setSlackRecipients([]);
-
-      if (onUpdate && setting) {
-        onUpdate(index, {
-          ...setting,
-          recipients: '',
-        } as NotificationSetting);
-      }
-
+      // Fetch fresh channel list without clearing user selections
       await fetchSlackChannels('', 0, 100);
 
       setRefreshKey(prev => prev + 1);
     } catch (error) {
-      console.error('Error refreshing channels:', error);
+      logging.error('Error refreshing channels:', error);
+
+      // Show error toast if available
+      if (addDangerToast) {
+        addDangerToast(
+          t('Failed to refresh Slack channels. Please try again.'),
+        );
+      }
     } finally {
       setIsRefreshing(false);
     }
   };
 
+  // Reset error toast flag when method changes
+  useEffect(() => {
+    hasShownErrorToast.current = false;
+  }, [method]);
+
+  // Initialize slackRecipients when editing an existing alert with SlackV2
+  useEffect(() => {
+    const initializeSlackRecipients = async () => {
+      if (
+        method === NotificationMethodOption.SlackV2 &&
+        recipients &&
+        slackRecipients.length === 0
+      ) {
+        try {
+          // Fetch first page of channels to map IDs to names
+          const channelData = await fetchSlackChannels('', 0, 100);
+          const recipientIds = recipients.split(',').map(id => id.trim());
+
+          // Map recipient IDs to {label, value} format
+          const mappedRecipients = recipientIds
+            .map(id => {
+              const channel = channelData.data.find(
+                ch => ch.value.toLowerCase() === id.toLowerCase(),
+              );
+              // If channel found, use its label; otherwise fallback to ID
+              return channel || { label: id, value: id };
+            })
+            .filter(r => r.value); // Filter out empty values
+
+          setSlackRecipients(mappedRecipients);
+        } catch (error) {
+          // If fetching fails, use IDs as both label and value
+          const recipientIds = recipients.split(',').map(id => id.trim());
+          const fallbackRecipients = recipientIds.map(id => ({
+            label: id,
+            value: id,
+          }));
+          setSlackRecipients(fallbackRecipients);
+        }
+      }
+    };
+
+    initializeSlackRecipients();
+    // eslint-disable-next-line react-hooks/exhaustive-deps
+  }, [method, recipients]);
+
   const methodOptions = useMemo(
     () =>
       (options || [])