Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[NV-3037] 🐛 Bug Report: Wrong Socket Close in useUnreadCount.ts #4582

Closed
2 tasks done
Khongchai opened this issue Oct 19, 2023 · 6 comments
Closed
2 tasks done

[NV-3037] 🐛 Bug Report: Wrong Socket Close in useUnreadCount.ts #4582

Khongchai opened this issue Oct 19, 2023 · 6 comments

Comments

@Khongchai
Copy link
Contributor

Khongchai commented Oct 19, 2023

📜 Description

In this hook, you're doing socket.on(WebSocketEventEnum.UNREAD) but you're doing socket.off(WebSocketEventEnum.UNSEEN).

👟 Reproduction steps

None

👍 Expected behavior

It should closes the correct socket channel.

👎 Actual Behavior with Screenshots

    socket.on(
      WebSocketEventEnum.UNREAD, // <<<<<<<<<<<<< here
      debounce((data?: { unreadCount: number }) => {
        if (Number.isInteger(data?.unreadCount)) {
          queryClient.setQueryData<{ count: number }>(queryKeysRef.current.unreadCountQueryKey, (oldData) => ({
            count: data?.unreadCount ?? oldData.count,
          }));

          // when unread count changes, we need to refetch unseen count
          queryClient.refetchQueries(queryKeysRef.current.unseenCountQueryKey, {
            exact: false,
          });
          queryClient.refetchQueries(queryKeysRef.current.fetchNotificationsQueryKey, {
            exact: false,
          });
          // refetch all feeds unseen count that is shown on the tabs
          queryClient.refetchQueries([...FEED_UNSEEN_COUNT_QUERY_KEY], {
            exact: false,
          });

          dispatchUnreadCountEvent(data.unreadCount);
        }
      }, DEBOUNCE_TIME)
    );

    return () => {
      socket.off(WebSocketEventEnum.UNSEEN); // <<<<<<<<<<<<< here
    };
  }, [socket, queryClient, setQueryKey]);

Novu version

0.17.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?

  • I checked and didn't find a similar issue

🏢 Have you read the Contributing Guidelines?

Are you willing to submit PR?

Yes

NV-3037

@Khongchai
Copy link
Contributor Author

Just noticed that the query being refetched is also the wrong one.

image

@p-fernandez p-fernandez changed the title 🐛 Bug Report: Wrong Socket Close in useUnreadCount.ts [NV-3037] 🐛 Bug Report: Wrong Socket Close in useUnreadCount.ts Oct 19, 2023
@p-fernandez
Copy link
Contributor

Thank you for raising this bug. Indeed you are right.
I am assigning the issue to you as you mention you are happy to raise a PR to fix it.

@krishanu7
Copy link

Please assigne me

@Khongchai
Copy link
Contributor Author

Khongchai commented Oct 19, 2023

@p-fernandez I'll open a PR for the socket one.

Perhaps @krishanu7 can have this other one?

image

These still references unseen count when they should probably be unread.

@p-fernandez
Copy link
Contributor

@p-fernandez I'll open a PR for the socket one.

Perhaps @krishanu7 can have this other one?

image These still references unseen count when they should probably be unread.

I think it is better if you can fix both issues in the same PR as you identified them. 🙂

@Khongchai
Copy link
Contributor Author

@p-fernandez just checked the code. It seems the other parts I mentioned were actually already correct, no?

image

It's just reflecting the change after having updated unread to unseen. Please let me know if my assumption is correct. If not, I'll add more changes.

Here's the PR #4590

@p-fernandez p-fernandez removed this from the Cycle 29 milestone Oct 22, 2023
@p-fernandez p-fernandez added this to the Cycle 30 milestone Oct 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants