Skip to content

MSD: wire up unseen notification dot icon logic in interim omnibar#109858

Open
fushar wants to merge 1 commit intotrunkfrom
omnibar-unread-notif
Open

MSD: wire up unseen notification dot icon logic in interim omnibar#109858
fushar wants to merge 1 commit intotrunkfrom
omnibar-unread-notif

Conversation

@fushar
Copy link
Copy Markdown
Contributor

@fushar fushar commented Apr 7, 2026

Fixes DOTMSD-1202

Proposed Changes

This PR implements the unseen notification count logic in the fake interim omnibar's redux store.

Testing Instructions

  1. Checkout this PR.
  2. With dashboard/omnibar flag on, visit Site List.
  3. Trigger new notification, e.g. by commenting on a post by another user of yours.
  4. Refresh the site list.
  5. Verify you see the dot in the notification bell icon.
  6. Click the bell icon.
  7. Verify the dot is gone.

Pre-merge Checklist

  • Has the general commit checklist been followed? (PCYsg-hS-p2)
  • Have you written new tests for your changes?
  • Have you tested the feature in Simple (P9HQHe-k8-p2), Atomic (P9HQHe-jW-p2), and self-hosted Jetpack sites (PCYsg-g6b-p2)?
  • Have you checked for TypeScript, React or other console errors?
  • Have you tested accessibility for your changes? Ensure the feature remains usable with various user agents (e.g., browsers), interfaces (e.g., keyboard navigation), and assistive technologies (e.g., screen readers) (PCYsg-S3g-p2).
  • Have you used memoizing on expensive computations? More info in Memoizing with create-selector and Using memoizing selectors and Our Approach to Data
  • Have we added the "[Status] String Freeze" label as soon as any new strings were ready for translation (p4TIVU-5Jq-p2)?
    • For UI changes, have we tested the change in various languages (for example, ES, PT, FR, or DE)? The length of text and words vary significantly between languages.
  • For changes affecting Jetpack: Have we added the "[Status] Needs Privacy Updates" label if this pull request changes what data or activity we track or use (p4TIVU-aUh-p2)?

@matticbot
Copy link
Copy Markdown
Contributor

matticbot commented Apr 7, 2026

This PR modifies the release build for the following Calypso Apps:

For info about this notification, see here: PCYsg-OT6-p2

  • help-center

To test WordPress.com changes, run install-plugin.sh $pluginSlug omnibar-unread-notif on your sandbox.

@fushar fushar marked this pull request as ready for review April 7, 2026 12:25
@fushar fushar requested a review from a team as a code owner April 7, 2026 12:25
@fushar fushar self-assigned this Apr 7, 2026
@matticbot matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Apr 7, 2026
Copy link
Copy Markdown
Member

@p-jackson p-jackson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, it was closer to working than I realised!

Our hacky wee Redux store is breaking more rules now. I believe the "rules of redux" are that the store reference shouldn't change between renders, and previously this was working because onToggleNotifications never changed and therefore the useMemo was never re-run. But now we're relying on the memo to be re-built whenever user changes 🤔 I think the technically correct way would be to update the data that gets returned by getState() and call the listeners.

But if it's not causing issues maybe this hack is still good enough for the interim solution. My main worry is that maybe it would leak memory or something? Would old components keep references to the old store through their listeners or something?

@fushar fushar force-pushed the omnibar-unread-notif branch 2 times, most recently from a3dc857 to 8d0456f Compare April 8, 2026 12:22
@fushar
Copy link
Copy Markdown
Contributor Author

fushar commented Apr 8, 2026

But now we're relying on the memo to be re-built whenever user changes 🤔 I think the technically correct way would be to update the data that gets returned by getState() and call the listeners.

Good catch. After playing around the code a bit more, it turns out the user object is only used for initial mount. I've removed user from the dependency list.

Now things get interesting here.

Subsequent updates are to be handled by listening to the APP_RENDER_NOTES action. In Dashboard, we have a local state to maintain the unseen count.. We don't have such thing in our interim omnibar, so I created another event (omnibarEvents.notificationsUnseenCount) that the notification app can emit to forward the count to our omnibar. Then, our omnibar just put that in the fake redux store, then notify the listeners when it changes.

But the thing is, I can't test this behavior. It should be correct but there's no way to verify it. It seems the whole web socket thing is broken. See: DOTMSD-1208

So this PR can only be tested by trying to get a new comment, then refresh the page and verify the dot appears.

@fushar fushar requested a review from p-jackson April 8, 2026 12:31
@fushar fushar force-pushed the omnibar-unread-notif branch 2 times, most recently from 8eba53b to a666548 Compare April 9, 2026 05:50
@p-jackson
Copy link
Copy Markdown
Member

I re-tested because I thought the PR looked like it would work (on refresh) in it's current state. The easiest way I found to get an unread notification is to leave a comment, and then have a testing user reply to that comment. I get the unread notifications icon on wordpress.com, but not on .calypso.live of this PR.

@fushar
Copy link
Copy Markdown
Contributor Author

fushar commented Apr 9, 2026

Thanks, interesting, it worked yesterday; but something might have messed up after today's refactor(s). Let me check... 😬

@fushar fushar force-pushed the omnibar-unread-notif branch from a666548 to 8a47f64 Compare April 9, 2026 07:35
@fushar
Copy link
Copy Markdown
Contributor Author

fushar commented Apr 9, 2026

@p-jackson Please retest. Sorry the commit history is lost because I rewrote the changes against latest trunk instead of rebasing 😬 but I essentially added this:

	// Dispatch the user's unseen note count to the store so the unread marker appears.
	useEffect( () => {
		store.dispatch( {
			type: 'NOTIFICATIONS_UNSEEN_COUNT_SET',
			unseenCount: Number( !! user.has_unseen_notes ),
		} );
	}, [ store, user.has_unseen_notes ] );

because after latest changes, user is first an empty fake user, not the real user, so it has no .has_unseen_notes initially.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants