chore: Add Flashbar and Alert dismiss persistence #4035

YueyingLu · 2025-11-13T16:46:35Z

Description

Add code to support Persistence feature in Flashbar and Alert [qIQZAtdAL9Xj].
The home of core function of Persistence is CR-199098474.

Related links, issue #, if available: n/a

How has this been tested?

Review checklist

The following items are to be evaluated by the author(s) and the reviewer(s).

Correctness

Changes include appropriate documentation updates.
Changes are backward-compatible if not indicated, see CONTRIBUTING.md.
Changes do not include unsupported browser features, see CONTRIBUTING.md.
Changes were manually tested for accessibility, see accessibility guidelines.

Security

If the code handles URLs: all URLs are validated through the checkSafeUrl function.

Testing

Changes are covered with new/existing unit tests?
Changes are covered with new/existing integration tests?

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

codecov · 2025-11-14T08:47:36Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.99%. Comparing base (f98fa63) to head (ba0873c).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #4035   +/-   ##
=======================================
  Coverage   96.99%   96.99%           
=======================================
  Files         861      861           
  Lines       25214    25257   +43     
  Branches     9094     9103    +9     
=======================================
+ Hits        24456    24499   +43     
  Misses        711      711           
  Partials       47       47

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

just-boris

Legend

⛳ – blocking comments, possible bugs
Other comments are optional code style suggestions

pages/flashbar/persistence.page.tsx

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

src/alert/internal.tsx

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

src/flashbar/common.tsx

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

src/flashbar/common.tsx

src/internal/persistence/index.ts

just-boris

All blocking comments resolved, thanks

just-boris · 2025-11-14T16:20:57Z

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

+
+      // Resolve second call first (race condition scenario)
+      resolveSecond!(false); // Item 2 should be visible
+      await waitFor(() => {});


Is this needed for anything?

just-boris · 2025-11-14T16:21:43Z

src/flashbar/common.tsx

+  const [checkedPersistenceKeys, setCheckedPersistenceKeys] = useState<Set<string>>(new Set());
+  const [persistentItemsVisibility, setPersistentItemsVisibility] = useState<Map<string, boolean>>(new Map());
+
+  const visibleItems = useMemo(() => {


This benefit is not measurable in any way. This is why typically I recommend to keep the code simple without extra things

gethinwebster · 2025-11-17T17:14:38Z

src/alert/internal.tsx

+          setIsPersistentlyDismissed(!!dismissed);
+        });
+      }
+      // eslint-disable-next-line react-hooks/exhaustive-deps


why? what if persistenceConfig.crossServicePersistence changes?

gethinwebster · 2025-11-17T17:15:58Z

src/alert/internal.tsx

+
+    const dismiss = () => {
+      fireNonCancelableEvent(onDismiss);
+      if (persistenceConfig && persistenceConfig.uniqueKey) {


[minor] for consistency

Suggested change

if (persistenceConfig && persistenceConfig.uniqueKey) {

if (persistenceConfig?.uniqueKey) {

gethinwebster · 2025-11-17T17:20:10Z

src/alert/internal.tsx

    const headerRef = useMergeRefs(headerRefAction, headerRefContent);
    const contentRef = useMergeRefs(contentRefAction, contentRefContent);
+    const [isPersistentlyDismissed, setIsPersistentlyDismissed] = useState(
+      !!(persistenceConfig && persistenceConfig.uniqueKey)


default-to-dismissed seems dangerous here, especially combined with promise without error handling in the useEffect below: if something within retrieveAlertDismiss fails then user will never see the alert.

At the very minimum the promise should have error handling, but I would strongly prefer an approach of default-to-visible

gethinwebster · 2025-11-17T17:24:42Z

src/flashbar/common.tsx

+    }
+
+    const checkNewPersistentItems = async () => {
+      const results = await Promise.all(


needs some error handling around awaits

gethinwebster · 2025-11-17T17:28:20Z

src/flashbar/common.tsx

+    );
+  }, [items, persistentItemsVisibility]);
+
+  useEffect(() => {


useEffect with async code needs cleanup function

gethinwebster · 2025-11-17T17:28:43Z

src/alert/internal.tsx

      i18nStrings?.dismissAriaLabel ?? i18n('dismissAriaLabel', deprecatedDismissAriaLabel)
    );

+    useEffect(() => {


useEffect with async code needs cleanup function

gethinwebster · 2025-11-17T17:36:34Z

src/internal/persistence/index.ts

+  }
+}
+
+// eslint-disable-next-line require-await


[minor] you could probably avoid these with return await Promise.resolve();?

gethinwebster · 2025-11-17T17:37:36Z

src/internal/persistence/index.ts

+  retrieveAlertDismiss?: (persistenceConfig: AlertProps.PersistenceConfig) => Promise<boolean>;
+}
+
+export function setPersistenceFunctionsForTesting(functions: PersistenceFunction) {


why not just use mocking?

gethinwebster · 2025-11-17T17:41:29Z

src/flashbar/common.tsx

 }
+
+// Hook for managing flashbar items visibility with persistence
+export function useFlashbarVisibility(items: ReadonlyArray<FlashbarProps.MessageDefinition>) {


could we move the complexity of this into the plugin rather than in the component itself? so just have a single call to retrieveFlashbarDismiss(items) which then internally handles which ones need to be fetched and which not?

YueyingLu changed the title ~~Implement Flashbar and Alert persistence~~ feat: Flashbar and Alert dismiss persistence Nov 13, 2025

YueyingLu force-pushed the persistence-flashbar-alert branch from 1dec203 to f0eff84 Compare November 14, 2025 08:38

YueyingLu deployed to dev-pages-react16 November 14, 2025 08:48 — with GitHub Actions View deployment

YueyingLu deployed to dev-pages-react18 November 14, 2025 08:48 — with GitHub Actions View deployment

YueyingLu force-pushed the persistence-flashbar-alert branch from f0eff84 to d9dfbdb Compare November 14, 2025 09:54

YueyingLu deployed to dev-pages-react18 November 14, 2025 10:03 — with GitHub Actions View deployment

YueyingLu deployed to dev-pages-react16 November 14, 2025 10:03 — with GitHub Actions View deployment

YueyingLu force-pushed the persistence-flashbar-alert branch from d9dfbdb to 842c792 Compare November 14, 2025 10:32

YueyingLu deployed to dev-pages-react18 November 14, 2025 10:41 — with GitHub Actions View deployment

YueyingLu deployed to dev-pages-react16 November 14, 2025 10:41 — with GitHub Actions View deployment

YueyingLu marked this pull request as ready for review November 14, 2025 11:47

YueyingLu requested a review from a team as a code owner November 14, 2025 11:47

YueyingLu requested review from ClaudioGSDB and removed request for a team November 14, 2025 11:47

just-boris reviewed Nov 14, 2025

View reviewed changes

YueyingLu force-pushed the persistence-flashbar-alert branch from 842c792 to 0730c73 Compare November 14, 2025 15:42

Yueying Lu added 2 commits November 14, 2025 16:44

Implement Flashbar and Alert persistence

7b5dce3

Address comments

ba0873c

YueyingLu force-pushed the persistence-flashbar-alert branch from 0730c73 to ba0873c Compare November 14, 2025 15:45

YueyingLu deployed to dev-pages-react16 November 14, 2025 15:54 — with GitHub Actions View deployment

YueyingLu deployed to dev-pages-react18 November 14, 2025 15:54 — with GitHub Actions View deployment

YueyingLu requested a review from just-boris November 14, 2025 16:09

just-boris approved these changes Nov 14, 2025

View reviewed changes

YueyingLu changed the title ~~feat: Flashbar and Alert dismiss persistence~~ chore: Add Flashbar and Alert dismiss persistence Nov 14, 2025

gethinwebster reviewed Nov 17, 2025

View reviewed changes

	if (persistenceConfig && persistenceConfig.uniqueKey) {
	if (persistenceConfig?.uniqueKey) {

chore: Add Flashbar and Alert dismiss persistence #4035

Are you sure you want to change the base?

chore: Add Flashbar and Alert dismiss persistence #4035

Uh oh!

Conversation

YueyingLu commented Nov 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

How has this been tested?

Correctness

Security

Testing

Uh oh!

codecov bot commented Nov 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

just-boris left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

just-boris left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

YueyingLu commented Nov 13, 2025 •

edited

Loading

codecov bot commented Nov 14, 2025 •

edited

Loading