liveblocks / liveblocks

Liveblocks is the platform for adding collaborative editing, comments, and notifications into your application.
https://liveblocks.io
Apache License 2.0
3.62k stars 293 forks source link

Error after deleting a notification through a React hook #1998

Open barbaldo opened 1 month ago

barbaldo commented 1 month ago

Describe the bug

Client error after deleting a notification that was rendered using data retrieved from the NodeJS SDK.

To Reproduce

Steps to reproduce the behavior:

  1. Initialize Liveblocks on the client
  2. Get data from Backend using liveblocks.getInboxNotifications
  3. Render notifications like in this doc: https://liveblocks.io/docs/api-reference/liveblocks-react-ui#Notifications
  4. Delete notification using useDeleteInboxNotification();
  5. Unexpected Application Error! Inbox notification with ID "in_Ad0RowtYhch5NEJN9J9wF" not found

Expected behavior

There should not be an error (maybe I don't get how notifications work)

Illustrations

N / D

Environment (please complete the following information):

Additional context

N / D

nvie commented 1 month ago

Hi @barbaldo – I'm a bit surprised this works in the first place! In order to render an inbox notification, the component will look up the associated thread from the cache. That cache is populated by the useInboxNotifications() and useThreads() hooks, so the assumption here is that you use those hooks. Passing an inbox notification JSON value you obtained using the backend won't be enough to render the component on screen. Doing so would have to trigger a waterfall of lookups to pull the necessary thread data.

Instead of using the Node SDK, have you considered using the useInboxNotifications() hook? The added benefit is that your app will automatically receive live updates as new notifications arrive, in addition to being able to render the notification using the InboxNotification UI component.

Please let us know if that approach works for you instead. If not, we'd love to better understand your use case! Thank you! 🙏

barbaldo commented 1 month ago

@nvie Hi! Sorry for not replying earlier, we're developing quite extensively on your platform so I'd actually love to get in contact we have a number of questions, My superiors have been trying to get in contact with sales for a while now :)

Anyway, let me address your reply point by point:

nvie commented 1 month ago

Again please, we've been trying to get in touch with you guys we'd love to use more features (and pay for them too), so feel free to get in touch!

I have passed this on to our sales team! 😇

nvie commented 1 month ago

As in, it looks like it's working on my end, or maybe the cache gets populated in some other parts of the app I'm forgetting about.

If you're also using useThreads() and/or useInboxNotifications() (even in other parts of your app), then it may happen to work because of that. It also depends on whether you're using "thread" inbox notifications or "custom" inbox notifications. While custom notifications contain all the data needed to display, they will likely work, but notifications for threads/comments require a populated cache to pull, for example, the thread text contents from to display in the notification.

(Think of the component as if it would look like: <InboxNotifications id="in_Ad0RowtYhch5NEJN9J9wF" />, and all the contents are being pulled from the cache. When you only use our backend SDK, the cache would not get populated, and this component won't be able to guarantee it has all the data to display. I understand this isn't completely obvious, and I may take this feedback back to our API design sessions.)

nvie commented 1 month ago

I have considered the hook, and honestly we ended up using it but as I was mentioning in the original post we have a need to further filter the notifications a user gets […] We used a workaround which is not scalable […]

Are you using this snippet to filter? This should work without any issues, and is completely supported:

const { inboxNotifications } = useInboxNotifications();
const filteredNotifications = React.useMemo(
  () => inboxNotifications.filter(n => n.roomId === roomId),
  [inboxNotifications, roomId]
);

Please let us know what's not scalable about your workaround, we're here to help!

barbaldo commented 1 month ago

Answering to both points: yes that was the case, it was working because notifications got populated in another part of the app. Second point: yes we ended up doing something like that and it does work quite well!