fix(js): ensure notification cache entries are proper Notification instances#10585
Draft
cursor[bot] wants to merge 1 commit intonextfrom
Draft
fix(js): ensure notification cache entries are proper Notification instances#10585cursor[bot] wants to merge 1 commit intonextfrom
cursor[bot] wants to merge 1 commit intonextfrom
Conversation
…stances Fixes DASHBOARD-2B5, DASHBOARD-2M6 When multiple tabs are open, BroadcastChannel.postMessage() uses structured cloning which strips class prototypes from Notification objects. This causes 'notification.read is not a function' and 'notification.archive is not a function' errors when users interact with inbox notifications on non-leader tabs. Two fixes applied: - CountContext: use notificationsCache.unshift() instead of direct cache.update() so incoming notifications are wrapped via createNotification - notifications-cache: add ensureNotificationInstance() guard in handleNotificationEvent to re-wrap plain objects that lost their prototype Co-authored-by: Dima Grossman <dima@grossman.io>
✅ Deploy Preview for dashboard-v2-novu-staging canceled.
|
Contributor
|
Hey there and thank you for opening this pull request! 👋 We require pull request titles to follow specific formatting rules and it looks like your proposed title needs to be adjusted. Your PR title is: Requirements:
Expected format: Details: PR title must end with 'fixes TICKET-ID' (e.g., 'fixes NOV-123') or include ticket ID in branch name |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed and why
Fixes DASHBOARD-2B5 (
TypeError: notification.read is not a function— 11 events, 3 users) and DASHBOARD-2M6 (TypeError: notification.archive is not a function— 1 event, 1 user).Root cause
When multiple browser tabs are open,
@novu/jsusesBroadcastChannelto share websocket events (like new notification received) across tabs. Only one tab (the "leader") holds the actual websocket connection; other tabs receive events viaBroadcastChannel.postMessage().The problem:
BroadcastChanneluses the browser's structured clone algorithm, which strips class prototypes. ANotificationclass instance (withread(),archive(),unread(), etc. on its prototype) becomes a plain object on the receiving tab — it has all the data properties but none of the methods.In
CountContext.tsx, the received notification was inserted directly into the notifications cache vianotificationsCache.update(), bypassing thecreateNotification()wrapper that constructs properNotificationclass instances. When the user then clicked "mark as read" or "archive" in the inbox, the code callednotification.read()on this plain object, producing the TypeError.Changes
CountContext.tsx: Changed fromnotificationsCache.update()(which accepts raw objects) tonotificationsCache.unshift()(which wraps viacreateNotification(), ensuring a properNotificationinstance).notifications-cache.ts: AddedensureNotificationInstance()guard inhandleNotificationEvent()— any notification data entering the cache from events is validated and re-wrapped if it lacks theNotificationprototype. This is a defense-in-depth measure for the same BroadcastChannel scenario on notification update/remove events.createNotification.ts: AddedensureNotificationInstance()utility that checksinstanceof Notificationand only wraps when needed (no-op for already-valid instances).Tests: Added 2 tests to
notifications-cache.test.tsverifying that plain objects (simulating BroadcastChannel deserialization) are properly wrapped intoNotificationinstances.