feat(web): group notifications list by importance #965
Conversation
WalkthroughThe changes in Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
pujitm
force-pushed
the
feat/notif-grouping
branch
from
af2c81f to
79bd9a6
Compare
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
web/components/Notifications/List.vue (2)
37-37: Consider enhancing error logging with additional contextWhile the component-specific prefix is good, consider including more context such as the current filter parameters to aid in debugging.
- console.log('[NotificationsList] getNotifications error:', newVal); + console.log('[NotificationsList] getNotifications error:', { + error: newVal, + type: props.type, + importance: props.importance, + offset: notifications.value?.length + });
84-96: Consider reducing template repetitionThe current implementation has repeated NotificationsItem blocks for each importance level. This could be simplified to be more maintainable.
Consider using a single loop over importance levels:
- <NotificationsItem - v-for="notification in notificationGroups.ALERT" - :key="notification.id" - v-bind="notification" - /> - <NotificationsItem - v-for="notification in notificationGroups.WARNING" - :key="notification.id" - v-bind="notification" - /> - <NotificationsItem - v-for="notification in notificationGroups.INFO" - :key="notification.id" - v-bind="notification" - /> + <template v-for="importance in ['ALERT', 'WARNING', 'INFO']" :key="importance"> + <NotificationsItem + v-for="notification in notificationGroups[importance]" + :key="notification.id" + v-bind="notification" + /> + </template>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
web/components/Notifications/List.vue(5 hunks)
🔇 Additional comments (3)
web/components/Notifications/List.vue (3)
6-6: LGTM! Good use of TypeScript's explicit type imports
The use of the type keyword in imports is a TypeScript best practice that improves code clarity and bundle size optimization.
24-26: LGTM! Well-documented loading state management
The canLoadMore ref is properly documented and correctly integrated with the infinite scroll functionality.
48-51: 🛠️ Refactor suggestion
Handle potential undefined importance values
While the grouping logic is clean, it might need to handle cases where importance is undefined or null.
Consider adding a fallback:
const notificationGroups = computed(() => {
- return Object.groupBy(notifications.value, ({ importance }) => importance);
+ return Object.groupBy(notifications.value, ({ importance }) => importance ?? 'OTHER');
});
|
What happens when we load a bunch more notifications? Does the grouping continue to work and things get added to the top, or is it ignored when you have many notifications to scroll through? |
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
the grouping continues to work and things get added to the top, but tbh i think it's a bad user experience. added a comment to asana explaining: https://app.asana.com/0/0/1208791188826610 |
|
we decided not to implement grouping (for now). see asana task for discussion |
2 participants
Soft-groups notification lists by severity (most severe first). each severity level is sorted by date. Example:
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Improvements