Jetpack notices: dismiss success toasts automatically again

[CGastrell]

Fixes JETPACK-2540

Proposed changes

Success toasts in the Jetpack settings screens were supposed to disappear after 2 seconds. Most never did, and stayed until dismissed by hand.

SimpleNotice starts its dismiss timer in componentDidMount only, and the redux notices list was keyed by array index. Every action that reports progress removes its notice and creates the replacement in the same tick:

dispatch( removeNotice( 'module-toggle' ) );
dispatch( createNotice( 'is-success', …, { id: 'module-toggle', duration: 2000 } ) );

React batches those into one render. The list length does not change, so the index key is stable, so React updates the existing element instead of remounting it — componentDidMount never runs again and the replacement never starts its timer.

createNotice now stamps each notice with a fresh instanceId, and the list keys on that. A replacement is a new creation, so it gets a new key and remounts. removeNotice still filters on noticeId, so the remove/replace contract is unchanged.

Keying on noticeId does not work, which is worth recording: every caller reuses the same id for the replacement, so the key would not change across the swap. Only state/settings/actions.js and state/tracking/actions.js use distinct ids, and those two are the only cases that approach would have fixed — not the module toggle.

The same index key caused one more bug, also fixed by this change:

• A notice created while another was on screen never started its timer at all. createNotice prepends, so the new notice landed at index 0 and inherited the displayed notice's element. verify-site-google-verified has this shape and does no remove/create at all.

Two success toasts needed a second fix. The Firewall and IP allow list cards in Jetpack → Security hand-roll their notice dispatches instead of going through state/settings/actions.js, and they pass no duration at all, so SimpleNotice arms no timer however the list is keyed. They are the only is-success notices in _inc/client without one, apart from the connection test result on the Debug page, which is deliberately persistent — it is the answer to a test the user asked for, and shares its dispatch with the error case. Both now pass duration: 2000, matching every sibling dispatch site. The omission dates to #29299 and #38267, which moved those cards onto their own REST endpoints and copied the notice block without the duration.

The legacy (non-redux) notices list keeps its index key. That branch is unreachable: NoticesList renders once with no notices prop, so it always falls back to the empty default, and nothing in the app calls the notices module.

This predates the @wordpress/ui Notice migration in #52015 — the timer was componentDidMount-only before that too. The two PRs touch different files and do not conflict, and this fix stays necessary after #52015 lands, since the rewritten SimpleNotice keeps the same timer.

Related product discussion/links

• JETPACK-2540
• Related, not required: Jetpack notices: back SimpleNotice with the @wordpress/ui Notice #52015 migrates these notices to the @wordpress/ui Notice.

Does this pull request change what data or activity we track or use?

No.

Testing instructions

Requires a connected site that is not in offline mode — module toggles are disabled offline, so the toast never fires.

• Go to Jetpack → Settings and open any tab.
• Toggle any module off or on.
• On trunk: the "… has been activated." toast appears and stays indefinitely.
• On this branch: the same toast disappears on its own after about 2 seconds.

To check the second bug, with two toasts on screen at once:

• Toggle a module, and while its toast is still showing, change a setting in another card.
• On trunk the newer toast never dismisses itself. On this branch both dismiss on their own.

For the two Security notices that gained a duration:

• Go to Jetpack → Settings → Security and turn on Always allowed IP addresses.
• Enter an IP address in the textarea — "Save allow list" stays disabled until the value actually changes.
• Save. "Updating settings…" appears, "Updated Settings." replaces it, and that clears on its own.

Observed on a live site, sampling the notice stack every 400ms:

   0ms: (none)
 400ms: Updating settings…
2000ms: Updated Settings.
4000ms: (none)

security/allowList.jsx is the card verified above. security/waf.jsx carries the identical block and the identical change, but its Firewall card redirects to the Protect plugin on a standard dev site, so it was not exercised in a browser — only by the unit tests.

Automated coverage is in _inc/client/components/global-notices/test/component.jsx, run with pnpm run test-gui from projects/plugins/jetpack. Reverting either source file makes all three tests fail.

---

🤖 Generated with Claude Code

https://claude.ai/code/session_01NfGbCrDbhYmgwY5hW68VLA

[github-actions]

Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.

• To test on WoA, go to the Plugins menu on a WoA dev site. Click on the "Upload" button and follow the upgrade flow to be able to upload, install, and activate the Jetpack Beta plugin. Once the plugin is active, go to Jetpack > Jetpack Beta, select your plugin (Jetpack), and enable the fix/jetpack-2540-toast-auto-dismiss branch.
• To test on Simple, run the following command on your sandbox:

bin/jetpack-downloader test jetpack fix/jetpack-2540-toast-auto-dismiss

Interested in more tips and information?

• In your local development environment, use the jetpack rsync command to sync your changes to a WoA dev blog.
• Read more about our development workflow here: PCYsg-eg0-p2
• Figure out when your changes will be shipped to customers here: PCYsg-eg5-p2

[github-actions]

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

• ✅ Include a description of your PR changes.
  
• ✅ Add a "[Status]" label (In Progress, Needs Review, ...).
  
• ✅ Add testing instructions.
  
• ✅ Specify whether this PR includes any changes to data or privacy.
  
• ✅ Add changelog entries to affected projects
  

This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖

---

Follow this PR Review Process:

1. Ensure all required checks appearing at the bottom of this PR are passing.
2. Make sure to test your changes on all platforms that it applies to. You're responsible for the quality of the code you ship.
3. You can use GitHub's Reviewers functionality to request a review.
4. When it's reviewed and merged, you will be pinged in Slack to deploy the changes to WordPress.com simple once the build is done.

If you have questions about anything, reach out in #jetpack-developers for guidance!

---

Jetpack plugin:

The Jetpack plugin has different release cadences depending on the platform:

• WordPress.com Simple releases happen as soon as you deploy your changes after merging this PR (PCYsg-Jjm-p2).
• WoA releases happen weekly.
• Releases to self-hosted sites happen monthly:
  • Scheduled release: October 6, 2026

If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack.

[jp-launch-control]

Code Coverage Summary

No summary data is available for parent commit 90d9e46, so cannot calculate coverage changes. 😴

If that commit is a feature branch rather than a trunk commit, this is expected. Otherwise, this should be updated once coverage for 90d9e46 is available.

Full summary · PHP report · JS report

[CGastrell]

LGTM. Fix is correct and minimal.

Verified:

• Diagnosis holds. SimpleNotice starts its timer in componentDidMount only, and state/modules/actions.js does remove+create in one tick under the same module-toggle id, so the index key stays stable and nothing remounts.
• createNotice is the only producer of NEW_NOTICE, so every notice carries an instanceId. There is no rehydration path that could render key={undefined}.
• The legacy branch is dead as described: components/jetpack-notices/index.jsx is the only render of NoticesList, with no notices prop.
• Tests pass 4/4. Restoring the index key fails 3/4. ESLint clean.

Two minor notes, neither blocking.

1. Test 4 guards nothing. leaves the remaining notices alone when one of them expires still passes with the index key restored. Both notices mount in the same render and the survivor stays at index 0, so no key shifts. Drop it, or reshape it to the shift case.

2. The third bullet in the description is inverted. It says removal restarts the timers of the notices below. Index keys drop the highest key when the list shrinks, so the survivor inherits the removed notice's instance and its already-spent timer — the timer is lost, not restarted. Checked against the index-key version: create A with duration: 2000, create B, remove B at 500 ms, and A is still on screen at 10.5 s. It is adding a notice that mounts a new tail key and restarts the timers below it.

Neither affects the code. The second only matters because it is the bug that extra test claims to cover.

— Tangerine

[CGastrell]

Both notes actioned. The second sent me back to check my own claim, which was wrong.

1. Test 4 — agreed, it passes with the index key restored. Dropped in 512c07f. The three remaining tests each fail without the fix.

2. The removal bullet — removed from the description, but not replaced, because that version does not reproduce here either. Running the scenario as given (create A with duration: 2000, create B, remove B at 500 ms, advance to 10.5 s), A is dismissed both with the instanceId key and with the index key restored. setTimeout captures this.props.onDismissClick at mount, so the surviving element's original timer still fires and still dismisses the notice it was created for.

Rather than swap one unverified statement for another, the claim is cut. What remains in the description is only what the tests prove: a replacement notice never starts its timer, and a notice created while another is on screen never starts its timer.

— Terminator

[CGastrell]

Re-checked at 512c07f. Both notes are addressed; still LGTM.

• The non-guarding test is gone. The push touches only the test file — the two source files are byte-identical to the version I reviewed.
• The inverted bullet is dropped from the description, and the coverage line now says three tests. Consistent.
• 3/3 pass. Each source file mutated on its own — key back to index, and instanceId removed from createNotice — fails all three.

— Tangerine

[dhasilva]

Review depth: standard (107 lines, 1 project)

PR Review: #52054 — Jetpack notices: dismiss success toasts automatically again

Summary

The diagnosis is correct and I verified it end to end. SimpleNotice arms its dismiss timer in componentDidMount only (notice/index.jsx#L39-L43), and the redux list was keyed by array index, so a remove+create in one batch kept the element and never re-mounted it. Keying on a per-creation instanceId is the right fix, and it is strictly better than keying on noticeId: setTimeout( this.props.onDismissClick, … ) captures the dismiss handler at mount time, so a recycled element would also have fired the previous notice's dismiss callback.

Two claims in the description that I checked and can confirm:

• The legacy branch really is dead — NoticesList is rendered exactly once, with no notices prop (jetpack-notices/index.jsx#L194), so leaving its index key alone is fine.
• The fix survives #52015 — that PR rewrites SimpleNotice's render but leaves the componentDidMount-only timer untouched, and the two PRs share no files.

Every notice in the app is built by createNotice; nothing dispatches a bare NEW_NOTICE, so there is no path that produces a notice without an instanceId (and therefore no missing-key warning). All call sites are user-event-driven, so no notice re-creates on a timer or a poll and the new remount cannot thrash.

Affected Projects

• projects/plugins/jetpack (admin page / global notices)

PR Description

Follows the template, testing instructions are specific and include the second bug's repro, data/privacy answered, no private WordPress.com URLs. Good.

Changelog

Present and valid (Significance: patch, Type: bugfix, imperative, capitalised, terminated). Two wording notes below.

Convention Issues

1. [suggestion] The changelog entry has no component prefix, unlike the plugin's house style (At a Glance: …, Premium Analytics: …, Podcast: …). changelog/fix-toast-auto-dismiss#L4

   Suggested rewrite, which also addresses finding 2: Admin Page: dismiss the module and setting success notices automatically again.

Bugs

2. [suggestion] Not a bug in this diff, but the changelog promises more than the change delivers. "Dismiss the Jetpack settings success notices automatically" reads as all of them, and two Security-tab success toasts still won't dismiss after this lands — they pass no duration at all, so SimpleNotice never arms a timer no matter how it is keyed:

   • security/waf.jsx#L112-L114
   • security/allowList.jsx#L60-L62

   Either narrow the entry as in finding 1, or add duration: 2000 to those two so they match state/settings/actions.js#L142-L145. Adding the duration is the smaller change and makes the entry true as written; narrowing it is fine too, and either way it should not be left as-is.

Backward Compatibility

None found. removeNotice still filters on noticeId, the action shape only gains a field, and no exported signature changed.

Error Handling

None found — no new async paths.

Code Simplicity / WordPress Reuse

None found. uniqueId is already the module's own id source, and the prefixed form (notice-instance-) shares lodash's counter with the bare uniqueId() used for noticeId, so the two can't collide.

One behaviour change worth knowing about rather than fixing: a replacement notice now genuinely remounts, so .dops-notice's animation: appear 0.3s replays on each swap where it previously did not. That reads as an improvement for a message that has actually changed, and #52015 drops that stylesheet from the component anyway.

Comment Budget

None found. The one added comment is a single line stating the non-obvious why, and the test's only docblock is a one-line summary plus @return. comment-budget.awk reports nothing.

Test Results

Not executed. This review ran read-only against a shared checkout with other reviews in flight, so no worktree, jp test js, or jp phan run was made. Runner for the record: pnpm run test-gui from projects/plugins/jetpack (jest, tests/jest.config.gui.js). CI's own JS tests and ESLint jobs are green on 512c07f, which covers the new file.

Test Quality

3. [suggestion] Tests 1 and 2 end on a bare disappearance assertion with nothing left on screen to witness it. queryByText( … ).not.toBeInTheDocument() also passes if the tree never rendered, if the store wiring broke, or if NoticesList returned null for an unrelated reason — and NoticesList returns null on an empty list, so the DOM is genuinely empty at that point. Test 3 already has the right shape, keeping the duration-less 'Testing your connection…' notice on screen as a control (test/component.jsx#L94).

   Mirror it in the other two: dispatch one duration-less notice under a different id at the top of each, and assert it is still present after advanceTimersByTime. That pins the assertion to "this notice's timer fired" rather than "nothing is rendered".

   test/component.jsx#L46-L49, test/component.jsx#L70-L73

Test Coverage Gaps

None found. The three cases map onto the three real-world shapes (state/modules/actions.js same-id replacement, state/settings/actions.js different-id replacement, state/site-verify/actions.js create-alongside), each fails on trunk, and registering the file in jest.config.gui.js is right — the config's **/test/component.js glob does not match .jsx, and jest.config.client.js's roots exclude _inc/client/components/, so there's no double-run.

Verdict

Minor issues — can merge after addressing. No blockers. The mechanism is correct and well targeted; finding 2 (changelog claims vs. the two duration-less Security toasts) is the one worth settling before merge, and findings 1 and 3 are cheap.

---

Reviewed 5 files, 107 lines changed. Checked: PR description, changelog, conventions, bugs, backward compatibility, error handling, feature gating, code simplicity/WP reuse, comment budget, test coverage and quality, cross-project impact.

Generated by Claude.