- Notifications
You must be signed in to change notification settings - Fork907
feat: add notifications widget in the navbar#16983
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Looks really good to me overall! Just had a few comments/questions
site/src/api/api.ts Outdated
| const res = JSON.parse(event.data) as TypesGen.GetInboxNotificationResponse; | ||
| onNewNotification(res); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
TheOneWayWebSocket class handles this, but in the meantime, do we want to add error handling ifJSON.parse ever throws an error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Just making a note for myself: this looks like a good candidate for swapping in theOneWayWebSocket class oncethat PR is merged in
| markAllAsRead={(): Promise<void> => { | ||
| throw new Error("Function not implemented."); | ||
| }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Just to be on the extra safe side, the return type can be changed tonever to indicate that it will always throw an error. Luckily all promise values are assignable tonever, too, so you don't even needPromise<never>
| type InboxPopoverProps = { | ||
| notifications:Notification[] | undefined; | ||
| notifications:readonly InboxNotification[] | undefined; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Appreciate thereadonly update!
| useEffect(() => { | ||
| const socket = watchInboxNotifications( | ||
| (res) => { | ||
| safeUpdateNotificationsCache((prev) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
JavaScript rules will ensure that this works, but just to help make sure the code stays readable, couldsafeUpdateNotificationsCache be moved above the effect?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 inuseEffectEvent, 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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
ab8ba96 intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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 tocoder/internal#336