feat(web): add count labels to notification tabs #975
Conversation
WalkthroughThe pull request introduces a new recipe called Changes
Possibly related PRs
Suggested reviewers
🪧 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-counts
branch
from
fa0e280 to
01a1e95
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (3)
justfile (1)
19-21: Enhance the restore-notifications recipe.The recipe could benefit from better error handling and user feedback.
Consider this implementation:
-# restore notification files under api/dev -restore-notifications: - git checkout ./api/dev/notifications +# Restore notification files to their original state from git +restore-notifications: + #!/usr/bin/env bash + if [ ! -d "./api/dev/notifications" ]; then + echo "Error: notifications directory not found" + exit 1 + fi + git checkout ./api/dev/notifications + echo "✓ Notification files restored successfully"web/components/Notifications/Indicator.vue (1)
9-11: Consider increasing the polling intervalThe current 2-second polling interval might be too aggressive for server resources, especially with multiple active users. Consider increasing it to 5-10 seconds for a better balance between responsiveness and server load.
- pollInterval: 2_000, // 2 seconds + pollInterval: 5_000, // 5 secondsweb/components/Notifications/Sidebar.vue (1)
53-53: Consider enhancing the confirmation UXThe current implementation uses basic browser confirm dialogs. Consider using a more user-friendly custom confirmation dialog that matches your application's design system.
Example implementation:
<template> <Dialog> <DialogTrigger> <Button variant="link" size="sm">Archive All</Button> </DialogTrigger> <DialogContent> <DialogHeader> <DialogTitle>Confirm Archive</DialogTitle> <DialogDescription> This will archive all notifications on your Unraid server. Continue? </DialogDescription> </DialogHeader> <DialogFooter> <Button variant="outline" @click="dialog.value = false">Cancel</Button> <Button variant="destructive" @click="handleConfirm">Continue</Button> </DialogFooter> </DialogContent> </Dialog> </template>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
justfile(1 hunks)web/components/Loading/Error.vue(0 hunks)web/components/Notifications/Indicator.vue(1 hunks)web/components/Notifications/Sidebar.vue(1 hunks)web/components/Notifications/TabList.vue(1 hunks)web/components/Notifications/graphql/notification.query.ts(1 hunks)web/composables/gql/gql.ts(2 hunks)web/composables/gql/graphql.ts(2 hunks)
💤 Files with no reviewable changes (1)
- web/components/Loading/Error.vue
🔇 Additional comments (6)
web/components/Notifications/graphql/notification.query.ts (1)
84-100: LGTM! Well-structured query implementation.
The notificationsOverview query is well-designed:
- Efficiently retrieves both unread and archive counts in a single query
- Properly structured with clear field organization
- Follows GraphQL best practices
web/components/Notifications/Indicator.vue (2)
Line range hint 13-19: LGTM! Well-structured computed properties
The computed properties for overview and indicatorLevel are well-implemented with proper null checks and clear logic flow.
Also applies to: 21-35
Line range hint 48-52: LGTM! Robust watch implementation
The watch implementation for new notifications includes proper cleanup and timeout management.
web/components/Notifications/Sidebar.vue (1)
Line range hint 54-90: LGTM! Well-structured notification management
The implementation properly handles loading states and provides clear user feedback for archive/delete operations.
web/composables/gql/gql.ts (1)
24-24: LGTM! Well-structured GraphQL query updates
The Overview query has been properly updated to include archive counts and notification IDs while maintaining backward compatibility.
Also applies to: 80-80
web/composables/gql/graphql.ts (1)
1705-1705: LGTM! The OverviewQuery type has been properly updated.
The changes to the OverviewQuery type correctly add the necessary fields for notification counts:
- Added
idfield to the notifications object for proper identification - Added
archivefield with complete notification count structure - Maintained type safety with proper nesting of NotificationCounts type
Also applies to: 1761-1761
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
web/components/Notifications/Indicator.vue (1)
10-21: Consider enhancing maintainability of indicator level logicWhile the logic is correct, consider extracting the importance thresholds into a constant map for better maintainability.
+const IMPORTANCE_THRESHOLDS = [ + { level: Importance.Alert, check: (unread) => unread.alert > 0 }, + { level: Importance.Warning, check: (unread) => unread.warning > 0 }, + { level: 'UNREAD', check: (unread) => unread.total > 0 } +] as const; const indicatorLevel = computed(() => { if (!props.overview?.unread) { return undefined; } - switch (true) { - case props.overview.unread.alert > 0: - return Importance.Alert; - case props.overview.unread.warning > 0: - return Importance.Warning; - case props.overview.unread.total > 0: - return 'UNREAD'; - default: - return undefined; - } + return IMPORTANCE_THRESHOLDS.find(({ check }) => + check(props.overview.unread))?.level; });web/components/Notifications/Sidebar.vue (1)
69-74: Enhance accessibility of notification countsWhile the count labels are implemented correctly, consider improving accessibility by adding screen reader text and ARIA labels.
<TabsTrigger value="unread"> - Unread <span v-if="overview">({{ overview.unread.total }})</span> + Unread + <span v-if="overview" + :aria-label="`${overview.unread.total} unread notifications`" + role="status"> + ({{ overview.unread.total }}) + </span> </TabsTrigger> <TabsTrigger value="archived"> - Archived <span v-if="overview">({{ overview.archive.total }})</span> + Archived + <span v-if="overview" + :aria-label="`${overview.archive.total} archived notifications`" + role="status"> + ({{ overview.archive.total }}) + </span> </TabsTrigger>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
web/components/Notifications/Indicator.vue(2 hunks)web/components/Notifications/Sidebar.vue(3 hunks)
🔇 Additional comments (2)
web/components/Notifications/Indicator.vue (1)
4-4: LGTM: Good architectural improvement
The transition from direct GraphQL queries to prop-based data passing improves component reusability and testability while maintaining type safety.
Also applies to: 7-7
web/components/Notifications/Sidebar.vue (1)
47-47: LGTM: Clean implementation of prop passing
The NotificationsIndicator component correctly receives the overview data via props, aligning with the architectural changes.
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
web/helpers/apollo-cache/index.ts (4)
84-87: Consider more granular cache invalidation.Instead of evicting the entire notifications field, consider evicting only the affected parts of the cache to improve performance.
-cache.evict({ fieldName: 'notifications' }); +// Evict only the unread notifications list +cache.evict({ + fieldName: 'notifications', + args: { filter: { type: 'UNREAD' } } +});
74-78: Add explicit return type to merge function.Adding an explicit return type improves type safety and documentation.
merge( existing: NotificationOverview | undefined, incoming: NotificationOverview | undefined, { cache } -) { +): NotificationOverview | undefined {
65-67: Decouple cache logic from UI components.The comment mentions the sidebar, which couples this cache logic to UI components. Consider making the comment more generic.
-* This allows incoming notifications to appear in the sidebar without reloading the page. +* This allows the UI to reflect new notifications without requiring a page reload.
63-90: Consider implementing cache updates via GraphQL subscriptions.The AI summary mentions that
Sidebar.vuepolls for notifications every 2 seconds. While the current cache implementation handles this well, consider using GraphQL subscriptions for real-time updates instead of polling. This would be more efficient and reduce server load.Benefits:
- Real-time updates without polling
- Reduced server load
- Better user experience
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
web/helpers/apollo-cache/index.ts(3 hunks)
🔇 Additional comments (1)
web/helpers/apollo-cache/index.ts (1)
27-28: LGTM! Well-structured type definitions.
The types are well-defined using TypeScript's Partial type and properly model the notification count data structure.
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
pujitm
force-pushed
the
feat/notif-counts
branch
from
38bffbc to
86c7145
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
web/helpers/apollo-cache/index.ts (1)
81-84: Consider adding error handling for cache operations.The cache operations could potentially fail silently. Consider wrapping the cache operations in a try-catch block to handle potential errors gracefully.
if (hasNewUnreads) { - cache.evict({ fieldName: 'notifications' }); - cache.gc(); + try { + cache.evict({ fieldName: 'notifications' }); + cache.gc(); + } catch (error) { + console.error('Failed to update notifications cache:', error); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
web/helpers/apollo-cache/index.ts(2 hunks)
🧰 Additional context used
📓 Learnings (1)
web/helpers/apollo-cache/index.ts (1)
Learnt from: pujitm
PR: unraid/api#975
File: web/helpers/apollo-cache/index.ts:79-82
Timestamp: 2024-12-10T15:57:42.593Z
Learning: The `isDefined` function is autoloaded and placed on `globalThis`; it does not need to be imported or defined explicitly in files.
🔇 Additional comments (2)
web/helpers/apollo-cache/index.ts (2)
2-2: LGTM!
The import statement is correctly typed and properly references the generated GraphQL types.
60-87: LGTM! Verify cache eviction behavior.
The overview field policy implementation is well-documented and correctly handles cache invalidation for new unread notifications. The type safety and cache operations are properly implemented.
Let's verify the cache eviction behavior:
✅ Verification successful
Cache eviction behavior is correctly implemented and verified
The cache eviction implementation is well-designed and properly integrated with the notification system:
- The
overviewfield policy correctly evicts notifications when new unread items are detected - The Sidebar component polls the overview query every 2 seconds, ensuring timely updates
- The cache eviction affects both the List and Sidebar components that use notification queries
- The implementation is consistent with other notification-related cache operations (archiveAll, deleteNotification, etc.)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if notifications query is used consistently across components
# This helps verify that cache eviction affects all notification-related components
# Search for notification query usage
rg -A 5 'notifications.*\{' --type vue
Length of output: 68
Script:
#!/bin/bash
# Let's try a broader search for notification-related queries and components
# First, let's find files that might contain notification-related code
fd -e ts -e tsx -e js -e jsx | xargs rg -l "notifications?"
# Then, let's look for specific GraphQL query usage
rg -A 5 "query.*notifications?"
# And also check for cache-related imports and usage
rg "import.*apollo.*cache"
Length of output: 19397
Script:
#!/bin/bash
# Let's examine the notification queries and cache implementation more closely
# Check the notification query implementation
cat web/components/Notifications/graphql/notification.query.ts
# Look at the Apollo cache setup
cat web/helpers/apollo-cache/index.ts
# Check for any notification-related components that might be affected by cache
rg -A 5 "useQuery.*Overview"
rg -A 5 "useQuery.*Notifications"
Length of output: 8866
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Chores