feat: add notifications widget in the navbar #16983
Conversation
BrunoQuaresma
requested review from
a team and
bcpeinhardt
and removed request for
a team
site/src/api/api.ts
Outdated
| const res = JSON.parse(event.data) as TypesGen.GetInboxNotificationResponse; | ||
| onNewNotification(res); |
There was a problem hiding this comment.
The OneWayWebSocket class handles this, but in the meantime, do we want to add error handling if JSON.parse ever throws an error?
There was a problem hiding this comment.
Good catch! I think it is always good to handle errors. In this case, I think we can just log a warn. What do you think?
There was a problem hiding this comment.
Just making a note for myself: this looks like a good candidate for swapping in the OneWayWebSocket class once that PR is merged in
| markAllAsRead={(): Promise<void> => { | ||
| throw new Error("Function not implemented."); | ||
| }} |
There was a problem hiding this comment.
Just to be on the extra safe side, the return type can be changed to never to indicate that it will always throw an error. Luckily all promise values are assignable to never, too, so you don't even need Promise<never>
|
|
||
| type InboxPopoverProps = { | ||
| notifications: Notification[] | undefined; | ||
| notifications: readonly InboxNotification[] | undefined; |
There was a problem hiding this comment.
Appreciate the readonly update!
| useEffect(() => { | ||
| const socket = watchInboxNotifications( | ||
| (res) => { | ||
| safeUpdateNotificationsCache((prev) => { |
There was a problem hiding this comment.
JavaScript rules will ensure that this works, but just to help make sure the code stays readable, could safeUpdateNotificationsCache be moved above the effect?
There was a problem hiding this comment.
Also, I'm really surprised that Biome didn't flag this as a missing effect dependency. I would expect that we would need to wrap the callback in useEffectEvent, too
| @@ -59,15 +78,15 @@ export const NotificationsInbox: FC<NotificationsInboxProps> = ({ | |||
|
|
|||
| const markNotificationAsReadMutation = useMutation({ | |||
| mutationFn: markNotificationAsRead, | |||
| onSuccess: (_, notificationId) => { | |||
| onSuccess: (res) => { | |||
| safeUpdateNotificationsCache((prev) => { | |||
There was a problem hiding this comment.
Same note, here, too. When I read the code top to bottom, I find it slightly harder to follow the logic when we reference values that haven't been explicitly declared yet
Preview:
Figma file
This PR adds:
What is next?
And about tests?
The notification widget components are well covered by the current stories, but we definitely want to have e2e tests for it. However, in my recent projects, I found more useful to ship the UI features first, get feedback, change whatever needs to be changed, and then, add the e2e tests to avoid major rework.
Related to coder/internal#336