novuhq / novu

Open-Source Notification Platform. Embeddable Notification Center, E-mail, Push and Slack Integrations.
https://novu.co
Other
34.01k stars 3.47k forks source link

๐Ÿ› Bug Report: @novu/notification-center Mark all as seen undesirable behavior #4846

Open Khongchai opened 9 months ago

Khongchai commented 9 months ago

๐Ÿ“œ Description

In the @novu/notification-center package, when you mark something as "seen", that message's read state will temporarily be marked as unread when the data is invalidated.

This causes an issue where, if I had a custom notification card whose message is read == true. Let's say that this card shows some type of indicator when a message is unread. When the user clicks on mark all as seen, which calls the markAllAsSeen method from the useNotification hook, the indicator would flash for a few milliseconds.

This is because the read state flips to unread, before switching back to read again when the new data arrives.

novu/packages/notification-center/src/hooks/useMarkNotificationAsSeen.ts at caac4b58775a6884a7e85b0055562f6ea62ee3a3 ยท novuhq/novu (github.com)

image

๐Ÿ‘Ÿ Reproduction steps

  1. Create one notification.
  2. Mark it as read. At this point, the message associated with the notification should now have read === true
  3. console.log the read state, and then call markAllAsSeen.
  4. You will see that the read state briefly switches to false before switching back to true.

๐Ÿ‘ Expected behavior

Should retain the actual read state.

I think the ideal behavior would be to use the read state of the current notification instead.

So, from this

//...
(infiniteData) => {
            const pages = infiniteData.pages.map((page) => {
              const data = page.data.map((message) => {
                return { ...message, read: false, seen: true };
 //...

we change to this

//...
(infiniteData) => {
            const pages = infiniteData.pages.map((page) => {
              const data = page.data.map((message) => {
                return { ...message, seen: true };
 //...

๐Ÿ‘Ž Actual Behavior with Screenshots

So I made a mock notification following the steps listed above:

The response is always

image

But the UI is like this (I'm repeatedly clicking mark all as read in this short clip). Green dot means the notification is unread.

https://github.com/novuhq/novu/assets/56601823/7b13f289-7361-4e7c-a5f8-01e5d40a4055

Novu version

0.21.0

npm version

No response

node version

No response

๐Ÿ“ƒ Provide any additional context for the Bug.

No response

๐Ÿ‘€ Have you spent some time to check if this bug has been raised before?

๐Ÿข Have you read the Contributing Guidelines?

Are you willing to submit PR?

Yes I am willing to submit a PR!

nick2432 commented 9 months ago

can i work on this?

Khongchai commented 4 months ago

Any update on this, folks? I can open a PR, if my understanding about the issue is sound.

I had to keep applying this patch for every new Novu version.

@@ -21,7 +21,7 @@ export const useMarkNotificationsAsSeen = (_a = {}) => {
             queryClient.setQueriesData({ queryKey: fetchNotificationsQueryKey, exact: false }, (infiniteData) => {
                 const pages = infiniteData.pages.map((page) => {
                     const data = page.data.map((message) => {
-                        return Object.assign(Object.assign({}, message), { read: false, seen: true });
+                        return Object.assign(Object.assign({}, message), { seen: true });
                     });
                     return Object.assign(Object.assign({}, page), { data });
                 });

We also have a custom indicator that uses unseen count, so it flashes all the time unless I apply this patch. In the video, it shows 1 every time I clicked on it because the read state of the message (there's only one) inside got set to true.

https://github.com/novuhq/novu/assets/56601823/3c92fef5-d0d3-47da-a297-9c857131ce7f

quan-nh2 commented 4 months ago

Do you happen to have any update on this one guys? I am facing the same issue ๐Ÿšฌ