Address comments

Author: Yueying Lu

pages/flashbar/persistence.page.tsx

@@ -1,14 +1,15 @@
 // Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 import * as React from 'react';
-import { useState } from 'react';
+import { useContext, useState } from 'react';
 
 import Button from '~components/button';
 import Flashbar, { FlashbarProps } from '~components/flashbar';
 import { setPersistenceFunctionsForTesting } from '~components/internal/persistence';
 import SpaceBetween from '~components/space-between';
 import Toggle from '~components/toggle';
 
+import AppContext, { AppContextType } from '../app/app-context';
 import ScreenshotArea from '../utils/screenshot-area';
 
 const params = new URLSearchParams(window.location.hash.split('?')[1] || '');
@@ -22,7 +23,10 @@ setPersistenceFunctionsForTesting({
   },
 });
 
+type PageContext = React.Context<AppContextType<{ stackItems: boolean }>>;
+
 export default function FlashbarTest() {
+  const { urlParams, setUrlParams } = useContext(AppContext as PageContext);
   const [items, setItems] = useState<FlashbarProps.MessageDefinition[]>([
     {
       type: 'success',
@@ -63,7 +67,7 @@ export default function FlashbarTest() {
       },
     },
   ]);
-  const [stackItems, setStackItems] = useState(params.get('stackItems') === 'true');
+  const stackItems = urlParams.stackItems ?? false;
 
   const addFlashItem = (withPersistence: boolean) => {
     const id = `message_${Date.now()}`;
@@ -100,11 +104,7 @@ export default function FlashbarTest() {
             data-id="stack-items"
             checked={stackItems}
             onChange={({ detail }) => {
-              setStackItems(detail.checked);
-              const url = new URL(window.location.href);
-              const params = new URLSearchParams(url.hash.split('?')[1] || '');
-              params.set('stackItems', detail.checked.toString());
-              window.history.replaceState({}, '', `${url.pathname}${url.hash.split('?')[0]}?${params}`);
+              setUrlParams({ stackItems: detail.checked });
             }}
           >
             Stack items

src/alert/__tests__/persistence-unit.test.tsx

@@ -35,7 +35,7 @@ describe('Alert Persistence', () => {
       renderAlert({ persistenceConfig, dismissible: true });
 
       await waitFor(() => {
-        expect(mockRetrieveAlertDismiss).toHaveBeenCalledWith(persistenceConfig);
+        expect(mockRetrieveAlertDismiss).toHaveBeenCalledWith({ uniqueKey: 'test-alert' });
       });
     });
 
@@ -88,7 +88,7 @@ describe('Alert Persistence', () => {
 
       getWrapper()!.findDismissButton()!.click();
 
-      expect(mockPersistAlertDismiss).toHaveBeenCalledWith(persistenceConfig);
+      expect(mockPersistAlertDismiss).toHaveBeenCalledWith({ uniqueKey: 'test-alert' });
       expect(onDismiss).toHaveBeenCalled();
     });
 
@@ -123,7 +123,10 @@ describe('Alert Persistence', () => {
 
       getWrapper()!.findDismissButton()!.click();
 
-      expect(mockPersistAlertDismiss).toHaveBeenCalledWith(persistenceConfig);
+      expect(mockPersistAlertDismiss).toHaveBeenCalledWith({
+        uniqueKey: 'test-alert',
+        crossServicePersistence: true,
+      });
     });
   });
 

src/alert/internal.tsx

@@ -130,16 +130,17 @@ const InternalAlert = React.forwardRef(
     );
 
     useEffect(() => {
-      if (persistenceConfig?.uniqueKey && persistenceConfig.uniqueKey.trim()) {
+      if (persistenceConfig?.uniqueKey) {
         retrieveAlertDismiss(persistenceConfig).then(dismissed => {
           setIsPersistentlyDismissed(!!dismissed);
         });
       }
-    }, [persistenceConfig]);
+      // eslint-disable-next-line react-hooks/exhaustive-deps
+    }, [persistenceConfig?.uniqueKey]);
 
     const dismiss = () => {
       fireNonCancelableEvent(onDismiss);
-      if (persistenceConfig && persistenceConfig.uniqueKey && persistenceConfig.uniqueKey.trim()) {
+      if (persistenceConfig && persistenceConfig.uniqueKey) {
         persistAlertDismiss(persistenceConfig);
       }
     };

src/flashbar/__tests__/persistence-unit.test.tsx

@@ -62,9 +62,15 @@ describe('Flashbar persistence', () => {
     });
 
     it('handles mixed persistent and non-persistent items', async () => {
-      mockRetrieveFlashbarDismiss
-        .mockResolvedValueOnce(true) // key-0: hidden
-        .mockResolvedValueOnce(false); // key-2: visible
+      mockRetrieveFlashbarDismiss.mockImplementation(config => {
+        if (config.uniqueKey === 'key-0') {
+          return Promise.resolve(true);
+        }
+        if (config.uniqueKey === 'key-2') {
+          return Promise.resolve(false);
+        }
+        throw new Error(`Unknown key ${config.uniqueKey}`);
+      });
 
       const items = createItems([true, false, true]);
 
@@ -196,5 +202,81 @@ describe('Flashbar persistence', () => {
         expect(mockRetrieveFlashbarDismiss).toHaveBeenLastCalledWith({ uniqueKey: 'new-key' });
       });
     });
+
+    it('checks persistence only for newly added items when start with empty array', async () => {
+      mockRetrieveFlashbarDismiss.mockResolvedValue(false);
+
+      // Start with empty array
+      const { rerender } = render(<Flashbar items={[]} />);
+
+      expect(mockRetrieveFlashbarDismiss).not.toHaveBeenCalled();
+
+      // Add one item, check that mockRetrieveFlashbarDismiss was called with that item id
+      const items = createItems([true]);
+      rerender(<Flashbar items={items} />);
+
+      await waitFor(() => {
+        expect(mockRetrieveFlashbarDismiss).toHaveBeenCalledTimes(1);
+        expect(mockRetrieveFlashbarDismiss).toHaveBeenCalledWith({ uniqueKey: 'key-0' });
+      });
+
+      mockRetrieveFlashbarDismiss.mockClear();
+
+      // Add one more item, check that mockRetrieveFlashbarDismiss was called with only the 2nd item id
+      const secondItem = { ...createItems([true])[0], id: 'item-1', persistenceConfig: { uniqueKey: 'key-1' } };
+      rerender(<Flashbar items={[...items, secondItem]} />);
+
+      await waitFor(() => {
+        expect(mockRetrieveFlashbarDismiss).toHaveBeenCalledTimes(1);
+        expect(mockRetrieveFlashbarDismiss).toHaveBeenCalledWith({ uniqueKey: 'key-1' });
+      });
+    });
+
+    it('handles concurrent async calls without race conditions', async () => {
+      let resolveFirst: (value: boolean) => void;
+      let resolveSecond: (value: boolean) => void;
+
+      const firstPromise = new Promise<boolean>(resolve => {
+        resolveFirst = resolve;
+      });
+      const secondPromise = new Promise<boolean>(resolve => {
+        resolveSecond = resolve;
+      });
+
+      mockRetrieveFlashbarDismiss.mockImplementation(config => {
+        if (config.uniqueKey === 'key-0') {
+          return firstPromise;
+        }
+        if (config.uniqueKey === 'key-2') {
+          return secondPromise;
+        }
+        throw new Error(`Unknown key ${config.uniqueKey}`);
+      });
+
+      const initialItems = createItems([true, false]); // Create item 0 with persistence, item 1 without persistence
+      const { rerender, container } = render(<Flashbar items={initialItems} />);
+
+      // Add third item while first item still loading
+      const updatedItems = [
+        ...initialItems,
+        { ...createItems([true])[0], content: 'Item 2', id: 'item-second', persistenceConfig: { uniqueKey: 'key-2' } },
+      ];
+      rerender(<Flashbar items={updatedItems} />);
+
+      // Resolve second call first (race condition scenario)
+      resolveSecond!(false); // Item 2 should be visible
+      await waitFor(() => {});
+
+      // Then resolve first call
+      resolveFirst!(true); // Item 0 should be hidden
+
+      const wrapper = createWrapper(container).findFlashbar()!;
+      await waitFor(() => {
+        const items = wrapper.findItems();
+        expect(items).toHaveLength(2);
+        expect(items[0].findContent()!.getElement()).toHaveTextContent('Item 1');
+        expect(items[1].findContent()!.getElement()).toHaveTextContent('Item 2');
+      });
+    });
   });
 });

src/flashbar/common.tsx

@@ -128,7 +128,7 @@ export function useFlashbar({
 
   const handleFlashDismissed = (dismissedId?: string, persistenceConfig?: FlashbarProps.PersistenceConfig) => {
     handleFlashDismissedInternal(dismissedId, items, ref.current, flashRefs.current);
-    if (persistenceConfig?.uniqueKey && persistenceConfig.uniqueKey.trim()) {
+    if (persistenceConfig?.uniqueKey) {
       persistFlashbarDismiss(persistenceConfig);
     }
   };
@@ -147,8 +147,8 @@ export function useFlashbar({
 
 // Hook for managing flashbar items visibility with persistence
 export function useFlashbarVisibility(items: ReadonlyArray<FlashbarProps.MessageDefinition>) {
-  const [checkedPersistenceKeys, setCheckedPersistenceKeys] = useState<Set<string>>(new Set());
-  const [persistentItemsVisibility, setPersistentItemsVisibility] = useState<Map<string, boolean>>(new Map());
+  const [checkedPersistenceKeys, setCheckedPersistenceKeys] = useState<Set<string>>(() => new Set());
+  const [persistentItemsVisibility, setPersistentItemsVisibility] = useState<Map<string, boolean>>(() => new Map());
 
   const visibleItems = useMemo(() => {
     return items.filter(
@@ -167,19 +167,27 @@ export function useFlashbarVisibility(items: ReadonlyArray<FlashbarProps.Message
     }
 
     const checkNewPersistentItems = async () => {
-      const newVisibility = new Map(persistentItemsVisibility);
-      const newKeys = new Set(checkedPersistenceKeys);
-
-      await Promise.all(
+      const results = await Promise.all(
         newPersistentItems.map(async item => {
           const isDismissed = await retrieveFlashbarDismiss(item.persistenceConfig!);
-          newVisibility.set(item.persistenceConfig!.uniqueKey, !isDismissed);
-          newKeys.add(item.persistenceConfig!.uniqueKey);
+          return {
+            key: item.persistenceConfig!.uniqueKey,
+            visible: !isDismissed,
+          };
         })
       );
 
-      setPersistentItemsVisibility(newVisibility);
-      setCheckedPersistenceKeys(newKeys);
+      setPersistentItemsVisibility(prev => {
+        const updated = new Map(prev);
+        results.forEach(({ key, visible }) => updated.set(key, visible));
+        return updated;
+      });
+
+      setCheckedPersistenceKeys(prev => {
+        const updated = new Set(prev);
+        results.forEach(({ key }) => updated.add(key));
+        return updated;
+      });
     };
 
     checkNewPersistentItems();

src/internal/persistence/index.ts

@@ -27,32 +27,34 @@ export function setPersistenceFunctionsForTesting(functions: PersistenceFunction
   }
 }
 
+// eslint-disable-next-line require-await
 export let persistFlashbarDismiss = async function (
   // eslint-disable-next-line @typescript-eslint/no-unused-vars
   persistenceConfig: FlashbarProps.PersistenceConfig
 ): Promise<void> {
-  // No-op
+  return Promise.resolve();
 };
 
+// eslint-disable-next-line require-await
 export let retrieveFlashbarDismiss = async function (
   // eslint-disable-next-line @typescript-eslint/no-unused-vars
   persistenceConfig: FlashbarProps.PersistenceConfig
 ): Promise<boolean> {
-  const result = await new Promise<boolean>(resolve => resolve(false));
-  return result;
+  return Promise.resolve(false);
 };
 
+// eslint-disable-next-line require-await
 export let persistAlertDismiss = async function (
   // eslint-disable-next-line @typescript-eslint/no-unused-vars
   persistenceConfig: AlertProps.PersistenceConfig
 ): Promise<void> {
-  // No-op
+  return Promise.resolve();
 };
 
+// eslint-disable-next-line require-await
 export let retrieveAlertDismiss = async function (
   // eslint-disable-next-line @typescript-eslint/no-unused-vars
   persistenceConfig: AlertProps.PersistenceConfig
 ): Promise<boolean> {
-  const result = await new Promise<boolean>(resolve => resolve(false));
-  return result;
+  return Promise.resolve(false);
 };