MYJP-268 Update My Jetpack red bubble notifications #46396
Conversation
github-actions
bot
added
[Package] My Jetpack
[Plugin] Backup
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
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:
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:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Backup plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Boost plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Search plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Social plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Starter Plugin plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Protect plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Videopress plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
github-actions
bot
added
the
[Status] Needs Author Reply
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
Code Coverage SummaryCoverage changed in 2 files.
1 file is newly checked for coverage.
Full summary · PHP report · JS report Coverage check overridden by
I don't care about code coverage for this PR
|
|
There seems to be some mistake about the linked issue. SOCIAL-268 is a different issue. Is it MYJP-268 instead? |
grzegorz-cp
changed the title
Yes, it's related to MYJP-268. Thank you noticing this. |
grzegorz-cp
added
I don't care about code coverage for this PR
| } ) | ||
| .catch( ( error: Error ) => { | ||
| // eslint-disable-next-line no-console | ||
| console.error( '[My Jetpack] Failed to fetch notification alerts:', error ); |
There was a problem hiding this comment.
I'm not sure if this is bringing any value.
There was a problem hiding this comment.
Pull request overview
This PR improves the performance of My Jetpack's red bubble notifications by deferring expensive external API calls to an asynchronous REST request that runs after page load. The change prevents blocking page rendering when the notification cache expires, while preserving the existing synchronous behavior on the My Jetpack page itself.
- Introduces async loading of red bubble notifications via JavaScript when cache is unavailable
- Adds a new
get_cached_alerts()method to check for cached data without triggering API calls - Updates logic to use cached data only on non-My Jetpack admin pages to avoid blocking
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| projects/packages/my-jetpack/webpack.config.js | Adds new webpack entry point for async notification bubble script |
| projects/packages/my-jetpack/src/class-red-bubble-notifications.php | Adds get_cached_alerts() method to retrieve cached notifications without expensive computation |
| projects/packages/my-jetpack/src/class-initializer.php | Updates maybe_show_red_bubble() to use cached data on non-My Jetpack pages and enqueue async script when cache is empty; adds enqueue_red_bubble_script() method |
| projects/packages/my-jetpack/_inc/utils/async-notification-bubble.ts | New TypeScript utility that fetches alerts via REST API and updates the menu bubble DOM asynchronously |
| projects/plugins/*/changelog/SOCIAL-268-bubble-async | Changelog entries documenting the performance improvement (9 files total) |
| projects/packages/my-jetpack/changelog/SOCIAL-268-bubble-async | Package-level changelog entry |
| /** | ||
| * My Jetpack Notification Bubble async loader. | ||
| * Fetches fresh alert data via REST API without blocking page load. | ||
| */ | ||
| import apiFetch from '@wordpress/api-fetch'; | ||
|
|
||
| // Minimal type for counting non-silent alerts. | ||
| type Alert = { | ||
| is_silent?: boolean; | ||
| }; | ||
|
|
||
| type AlertsResponse = Record< string, Alert >; | ||
|
|
||
| apiFetch< AlertsResponse >( { | ||
| path: 'my-jetpack/v1/red-bubble-notifications', | ||
| method: 'POST', | ||
| } ) | ||
| .then( alerts => { | ||
| const count = Object.values( alerts ).filter( a => ! a.is_silent ).length; | ||
| const menuItem = document.querySelector( '#toplevel_page_jetpack .wp-menu-name' ); | ||
|
|
||
| if ( ! menuItem ) { | ||
| return; | ||
| } | ||
|
|
||
| const bubble = menuItem.querySelector( '.awaiting-mod' ); | ||
|
|
||
| if ( count > 0 ) { | ||
| if ( bubble ) { | ||
| bubble.className = 'awaiting-mod'; | ||
| bubble.textContent = String( count ); | ||
| } else { | ||
| const span = document.createElement( 'span' ); | ||
| span.className = 'awaiting-mod'; | ||
| span.textContent = String( count ); | ||
| menuItem.appendChild( document.createTextNode( ' ' ) ); | ||
| menuItem.appendChild( span ); | ||
| } | ||
| } else if ( bubble ) { | ||
| bubble.remove(); | ||
| } | ||
| } ) | ||
| .catch( ( error: Error ) => { | ||
| // eslint-disable-next-line no-console | ||
| console.error( '[My Jetpack] Failed to fetch notification alerts:', error ); | ||
| } ); |
There was a problem hiding this comment.
The new async-notification-bubble.ts utility lacks test coverage. Consider adding unit tests to verify the DOM manipulation logic (bubble creation, update, and removal) and the error handling for API failures. This is particularly important since this code runs on every admin page when the cache is empty.
| /** | ||
| * Get cached red bubble alerts without triggering expensive computation. | ||
| * Returns the cached transient value or an empty array if not cached. | ||
| * | ||
| * @return array Cached alerts or empty array. | ||
| */ | ||
| public static function get_cached_alerts() { | ||
| $stored_alerts = get_transient( self::MY_JETPACK_RED_BUBBLE_TRANSIENT_KEY ); | ||
| return $stored_alerts !== false ? $stored_alerts : array(); | ||
| } |
There was a problem hiding this comment.
The new get_cached_alerts() method lacks test coverage. Consider adding unit tests to verify it correctly returns cached values when available and an empty array when the cache doesn't exist.
| * Returns the cached transient value or an empty array if not cached. | ||
| * | ||
| * @return array Cached alerts or empty array. | ||
| */ | ||
| public static function get_cached_alerts() { | ||
| $stored_alerts = get_transient( self::MY_JETPACK_RED_BUBBLE_TRANSIENT_KEY ); | ||
| return $stored_alerts !== false ? $stored_alerts : array(); |
There was a problem hiding this comment.
The get_cached_alerts() method returns an empty array when there's no cache, making it impossible to distinguish between "cache doesn't exist" and "cache exists but contains no alerts". This causes an unnecessary async script load when the cache legitimately contains an empty array (no alerts to show). The check if ( empty( $cached_alerts ) ) will trigger an async fetch in both cases. Consider modifying get_cached_alerts() to return false when the cache doesn't exist, preserving the ability to distinguish these two states.
| * Returns the cached transient value or an empty array if not cached. | |
| * | |
| * @return array Cached alerts or empty array. | |
| */ | |
| public static function get_cached_alerts() { | |
| $stored_alerts = get_transient( self::MY_JETPACK_RED_BUBBLE_TRANSIENT_KEY ); | |
| return $stored_alerts !== false ? $stored_alerts : array(); | |
| * Returns the cached transient value or false if not cached. | |
| * | |
| * @return array|false Cached alerts array or false if no cache exists. | |
| */ | |
| public static function get_cached_alerts() { | |
| return get_transient( self::MY_JETPACK_RED_BUBBLE_TRANSIENT_KEY ); |
manzoorwanijk
left a comment
manzoorwanijk
left a comment
There was a problem hiding this comment.
It works as expected. I have some minor suggestions for code quality improvements.
| } | ||
|
|
||
| $rbn = new Red_Bubble_Notifications(); | ||
| // Check if we're on the My Jetpack page |
There was a problem hiding this comment.
Let us avoid adding PHPCS errors. This file already has too many of them.
| // Check if we're on the My Jetpack page | |
| // Check if we're on the My Jetpack page. |
|
|
||
| // filters for the items in this file | ||
| add_filter( 'my_jetpack_red_bubble_notification_slugs', array( $rbn, 'add_red_bubble_alerts' ) ); | ||
| // Filter out silent alerts |
There was a problem hiding this comment.
| // Filter out silent alerts | |
| // Filter out silent alerts. |
| */ | ||
| public static function enqueue_red_bubble_script() { | ||
| Assets::register_script( | ||
| 'my_jetpack_notification_bubble', |
There was a problem hiding this comment.
Handles are usually with a hyphen instead of underscore.
| 'my_jetpack_notification_bubble', | |
| 'my-jetpack-notification-bubble', |
There was a problem hiding this comment.
I haven't seen a string with mixed hyphens and underscores in the codebase, but there are places where everything has underscores, even if it contains "my_jetpack", so I think it's better to keep the proposed value.
Co-authored-by: Manzoor Wani <[email protected]>
Co-authored-by: Copilot <[email protected]>
| method: 'POST', | ||
| } ) | ||
| .then( alerts => { | ||
| const count = Object.values( alerts ).filter( a => ! a.is_silent ).length; |
There was a problem hiding this comment.
If the API returns null, undefined, or a non-object value, calling Object.values() will throw a runtime error. While apiFetch with proper typing should prevent this, consider adding a defensive check to ensure alerts is a valid object before processing it.
| const count = Object.values( alerts ).filter( a => ! a.is_silent ).length; | |
| const alertValues = alerts && typeof alerts === 'object' ? Object.values( alerts ) : []; | |
| const count = alertValues.filter( a => ! a.is_silent ).length; |
Fixes MYJP-268
Proposed changes:
maybe_show_red_bubble()function runs on every WordPress admin page load and triggers external API calls to WordPress.com (that might timeout and delay page rendering) when the cache expires. The solution defers these API calls to an asynchronous REST request that runs after the page has loaded. My Jetpack page maintains existing logic to preserve React and window object implementation.Other information:
Jetpack product discussion
Does this pull request change what data or activity we track or use?
Testing instructions:
jetpack docker wp option delete _transient_my-jetpack-red-bubble-transient
jetpack docker wp option delete _transient_timeout_my-jetpack-red-bubble-transient
red-bubble-notificationsendpoint was made, but HTML doesn't haveasync-notification-bubble.jsscript appended.red-bubble-notifications(cache should be used)jetpack docker wp option delete _transient_my-jetpack-red-bubble-transient
jetpack docker wp option delete _transient_timeout_my-jetpack-red-bubble-transient
red-bubble-notifications(cache should be used)async-notification-bubble.jsscript appended