feat(web): notification settings link & delete all button #962
Conversation
WalkthroughThis pull request introduces a new mutation, 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 (
|
|
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: 2
🧹 Outside diff range and nitpick comments (8)
web/components/Notifications/graphql/notification.query.ts (1)
70-81: Consider adding a safety parameter to prevent accidental deletions.Since this is a destructive operation affecting all notifications, consider adding a confirmation parameter to prevent accidental deletions.
-export const deleteAllNotifications = graphql(/* GraphQL */ ` - mutation DeleteAllNotifications { - deleteAllNotifications { +export const deleteAllNotifications = graphql(/* GraphQL */ ` + mutation DeleteAllNotifications($confirmation: String!) { + deleteAllNotifications(confirmation: $confirmation) { archive { total } unread { total } } } `);api/src/graphql/schema/types/notifications/notifications.graphql (1)
20-20: Consider adding an optional importance filter parameter.For consistency with
archiveAllandunarchiveAllmutations, consider adding an optionalimportanceparameter to allow selective deletion of notifications by importance level.- deleteAllNotifications: NotificationOverview! + """ + Deletes all notifications. If importance is specified, only deletes notifications of that importance. + """ + deleteAllNotifications(importance: Importance): NotificationOverview!web/components/Notifications/Sidebar.vue (2)
24-30: Consider using route constantsThe hard-coded path
/Settings/Notificationscould be fragile if routes change. Consider using route constants for better maintainability.- <a href="/Settings/Notifications"> + <a :href="ROUTES.SETTINGS_NOTIFICATIONS">
27-29: Enhance accessibility for new interactive elementsConsider adding ARIA labels or screen reader text to improve accessibility:
- Edit Settings + <span class="sr-only">Edit Notification Settings</span> + <span aria-hidden="true">Edit Settings</span>- Delete All + <span class="sr-only">Delete All Notifications</span> + <span aria-hidden="true">Delete All</span>Also applies to: 54-62
api/src/unraid-api/graph/resolvers/notifications/notifications.resolver.ts (1)
80-84: Add error handling for robustness.Consider adding error handling similar to the
recalculateOverviewmutation to ensure failed operations are properly communicated to the client.Here's a suggested implementation:
@Mutation() public async deleteAllNotifications(): Promise<NotificationOverview> { - return this.notificationsService.deleteAllNotifications(); + const { overview, error } = await this.notificationsService.deleteAllNotifications(); + if (error) { + throw new AppError('Failed to delete all notifications', 500); + } + return overview; }web/helpers/apollo-cache/index.ts (2)
104-110: Add JSDoc documentation for consistency.Unlike other mutation fields,
deleteAllNotificationslacks documentation explaining its purpose and parameters. Consider adding JSDoc comments similar to thearchiveAllmutation.Here's a suggested documentation block:
deleteAllNotifications: { + /** + * Clears the notifications cache when `deleteAllNotifications` is called to + * ensure that notification queries are invalidated and refetched. + * + * @param _ existing value in cache (irrelevant to this operation) + * @param incoming result from the server + * @param apolloContext contains the cache object + * @returns the value to cache for this operation + */ merge(_, incoming, { cache }) {
75-78: Consider refactoring duplicate cache clearing logic.The
deleteAllNotificationsandarchiveAllmutations share identical cache clearing logic. Consider extracting this into a shared utility function to improve maintainability.Here's a suggested refactor:
const clearNotificationsCache = (cache: ApolloCache<any>, incoming: any) => { cache.evict({ fieldName: 'notifications' }); cache.gc(); return incoming; }; // Usage in type policies: archiveAll: { merge(_, incoming, { cache }) { return clearNotificationsCache(cache, incoming); } }, deleteAllNotifications: { merge(_, incoming, { cache }) { return clearNotificationsCache(cache, incoming); } }Also applies to: 104-110
web/composables/gql/graphql.ts (1)
Consider adding user confirmation before bulk deletion
The code review reveals that the
deleteAllmutation is directly called from a "Delete All" button in the Notifications Sidebar without any confirmation dialog. This could lead to accidental deletion of all archived notifications. Specific findings:
- The deletion is triggered directly via
@click="deleteAll"in Sidebar.vue- No confirmation dialog or modal is present before executing the deletion
- This contrasts with the destructive nature of a bulk delete operation
Suggested locations for implementing confirmation:
web/components/Notifications/Sidebar.vue: Add a confirmation dialog before callingdeleteAll🔗 Analysis chain
Line range hint
1697-1760: Consider adding user confirmation before bulk deletionSince this mutation deletes all notifications in a single operation, consider implementing a confirmation dialog in the UI layer to prevent accidental deletions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for existing confirmation patterns in notification-related components rg -A 5 "confirm.*delete" --type vueLength of output: 66
Script:
#!/bin/bash # Search for confirmation patterns in all files rg -A 5 "confirm.*delete|delete.*confirm" # Search for files related to notifications UI fd -e ts -e js -e vue -e jsx -e tsx notificationLength of output: 659
Script:
#!/bin/bash # Check the notification components and store for existing delete operations ast-grep --pattern 'deleteAllNotifications' # Check the content of notification-related components cat web/components/Notifications/graphql/notification.query.ts cat web/store/notifications.tsLength of output: 2829
Script:
#!/bin/bash # Let's check for any UI components that use deleteAllNotifications rg -l "deleteAllNotifications" # Check for any modal or dialog components that might be used for confirmation fd -e vue -e tsx -e jsx modal dialog confirmLength of output: 749
Script:
#!/bin/bash # Check the Notifications Sidebar component for delete implementation cat "web/components/Notifications/Sidebar.vue" # Search for any confirmation dialogs in the codebase rg -A 5 "confirm.*modal|modal.*confirm"Length of output: 4346
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (3)
api/src/graphql/generated/api/types.tsis excluded by!**/generated/**api/src/graphql/generated/client/fragment-masking.tsis excluded by!**/generated/**api/src/graphql/generated/client/gql.tsis excluded by!**/generated/**
📒 Files selected for processing (7)
api/src/graphql/schema/types/notifications/notifications.graphql(1 hunks)api/src/unraid-api/graph/resolvers/notifications/notifications.resolver.ts(1 hunks)web/components/Notifications/Sidebar.vue(3 hunks)web/components/Notifications/graphql/notification.query.ts(1 hunks)web/composables/gql/gql.ts(2 hunks)web/composables/gql/graphql.ts(2 hunks)web/helpers/apollo-cache/index.ts(4 hunks)
🔇 Additional comments (9)
web/components/Notifications/graphql/notification.query.ts (1)
70-81: LGTM! The mutation is well-structured and consistent.
The implementation follows existing patterns and properly returns both archive and unread counts.
api/src/graphql/schema/types/notifications/notifications.graphql (1)
20-20: LGTM! The mutation follows GraphQL best practices.
The deleteAllNotifications mutation is well-designed, follows consistent naming patterns, and returns the appropriate type (NotificationOverview!).
web/components/Notifications/Sidebar.vue (1)
6-6: LGTM: Clean mutation setup
The mutation setup follows Vue Apollo best practices with proper loading state tracking.
Also applies to: 9-9
api/src/unraid-api/graph/resolvers/notifications/notifications.resolver.ts (1)
80-84: Verify service implementation and schema definition.
Let's ensure the implementation is consistent across the codebase.
✅ Verification successful
Implementation verified successfully
The implementation is consistent across the codebase:
- The resolver's
deleteAllNotifications()method matches the GraphQL schema definition, returningNotificationOverview - The service's implementation exists and handles both UNREAD and ARCHIVE notifications
- The schema correctly defines the mutation and return type
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify service method signature and GraphQL schema definition
# Check service method signature
echo "Checking service method signature..."
ast-grep --pattern 'class NotificationsService {
$$$
deleteAllNotifications($$$): Promise<$$$> {
$$$
}
$$$
}'
# Check GraphQL schema definition
echo "Checking GraphQL schema definition..."
rg "deleteAllNotifications.*:.*NotificationOverview" --type graphql
Length of output: 393
Script:
#!/bin/bash
# Let's try a different approach to verify the implementation
# 1. Find the service implementation
echo "Looking for NotificationsService implementation..."
rg "class NotificationsService" -A 5
# 2. Search for deleteAllNotifications method
echo -e "\nLooking for deleteAllNotifications method..."
rg "deleteAllNotifications.*Promise"
# 3. Look for GraphQL schema files with different extensions
echo -e "\nLooking for GraphQL schema files..."
fd "\.(graphql|gql|graphqls)$"
# 4. If we find schema files, check their content
echo -e "\nChecking schema content..."
fd "\.(graphql|gql|graphqls)$" --exec cat {}
Length of output: 34661
Script:
#!/bin/bash
# Let's check the service implementation
echo "Checking NotificationsService implementation..."
rg "deleteAllNotifications.*Promise.*NotificationOverview" -A 5 api/src/unraid-api/graph/resolvers/notifications/notifications.service.ts
Length of output: 244
Script:
#!/bin/bash
# Let's try a broader search for the service method
echo "Checking for deleteAllNotifications method in service..."
rg "deleteAllNotifications" api/src/unraid-api/graph/resolvers/notifications/notifications.service.ts -A 5
# Also verify the schema definition matches
echo -e "\nVerifying GraphQL schema definition..."
cat api/src/graphql/schema/types/notifications/notifications.graphql
Length of output: 2752
web/helpers/apollo-cache/index.ts (1)
39-39: LGTM! Well-documented cache key configuration.
The keyArgs configuration correctly handles notification filtering and pagination by including both the filter and its sub-fields.
web/composables/gql/gql.ts (2)
73-76: LGTM! The function overload maintains type safety.
The GraphQL function overload is correctly implemented, maintaining consistency with other overloads and ensuring proper type checking for the DeleteAllNotifications mutation.
23-23: LGTM! The mutation is properly structured.
The DeleteAllNotifications mutation follows the established pattern and is correctly mapped to its TypeScript type.
Let's verify the mutation's integration with the notification system:
✅ Verification successful
The DeleteAllNotifications mutation is properly integrated and implemented
The verification confirms that:
- The mutation is correctly defined in
web/components/Notifications/graphql/notification.query.ts - The resolver is properly implemented in
notifications.resolver.tswith thedeleteAllNotificationsmethod - The service implementation in
notifications.service.tsincludes a robustdeleteAllNotificationsmethod that:- Empties both unread and archive directories
- Resets the notification overview counters
- Maintains proper error handling and directory structure
- Returns the updated overview stats
The implementation follows the established patterns and maintains consistency with the existing notification system architecture.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the mutation's integration and usage
# Check for proper usage of the mutation in components
rg -l "DeleteAllNotifications" --type vue
# Check for proper error handling in the resolver
ast-grep --pattern 'class $_ implements $_ {
$$$
async deleteAllNotifications() {
try {
$$$
} catch $_
}
$$$
}'
Length of output: 205
Script:
#!/bin/bash
# Let's try different approaches to verify the mutation's integration
# 1. Search for DeleteAllNotifications usage across all files
rg -l "DeleteAllNotifications"
# 2. Look for the resolver implementation with a more generic pattern
ast-grep --pattern 'async deleteAllNotifications() {
$$$
}'
# 3. Check for any notification-related components
rg -l "notification" --type-list
rg -l "notification"
Length of output: 2771
Script:
#!/bin/bash
# Let's check the implementation details of the mutation
# 1. Check the notification query file for the mutation definition
cat web/components/Notifications/graphql/notification.query.ts
# 2. Check the resolver implementation
cat api/src/unraid-api/graph/resolvers/notifications/notifications.resolver.ts
# 3. Check the service implementation
cat api/src/unraid-api/graph/resolvers/notifications/notifications.service.ts
Length of output: 35302
web/composables/gql/graphql.ts (2)
1697-1701: LGTM: New mutation types for bulk notification deletion
The types are correctly defined:
- Empty variables type since no arguments are needed
- Response type properly captures the notification counts in the overview
1760-1760: LGTM: GraphQL document for deleteAllNotifications mutation
The mutation document is properly structured to:
- Delete all notifications in a single operation
- Return both archive and unread notification counts
api/src/unraid-api/graph/resolvers/notifications/notifications.resolver.ts
Show resolved
Hide resolved
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
web/components/Notifications/Sidebar.vue(3 hunks)
🧰 Additional context used
🪛 GitHub Check: Build Web App
web/components/Notifications/Sidebar.vue
[failure] 8-8:
'archiveAll' is assigned a value but never used. Allowed unused vars must match /^_/u
🔇 Additional comments (2)
web/components/Notifications/Sidebar.vue (2)
6-6: LGTM: Mutation setup is correct
The import and mutation setup for delete functionality are properly implemented. Note: The static analysis warning about unused archiveAll is a false positive as it's used in the template.
Also applies to: 9-9
40-46: LGTM: Settings link implementation
The settings link is well-placed in the header and properly implemented.
pujitm
force-pushed
the
feat/edit-notif-settings
branch
from
8761d05 to
1f220e6
Compare
|
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 (2)
web/components/Notifications/Sidebar.vue (2)
6-6: Consider adding explicit TypeScript types for mutationsWhile the code works, adding explicit types would improve type safety and maintainability.
-const { mutate: archiveAll, loading: loadingArchiveAll } = useMutation(archiveAllNotifications); -const { mutate: deleteAll, loading: loadingDeleteAll } = useMutation(deleteAllNotifications); +const { mutate: archiveAll, loading: loadingArchiveAll } = useMutation<{ archiveAllNotifications: NotificationOverview }>(archiveAllNotifications); +const { mutate: deleteAll, loading: loadingDeleteAll } = useMutation<{ deleteAllNotifications: NotificationOverview }>(deleteAllNotifications);Also applies to: 9-9
42-46: Improve accessibility for interactive elementsWhile the implementation is functionally correct, consider these accessibility improvements:
- <a href="/Settings/Notifications"> + <a + href="/Settings/Notifications" + aria-label="Edit notification settings" + class="inline-flex items-center" + > <Button variant="link" size="sm" class="text-muted-foreground text-base p-0"> Edit Settings </Button> </a> <TabsContent value="unread"> <Button :disabled="loadingArchiveAll" variant="link" size="sm" class="text-muted-foreground text-base p-0" + aria-label="Archive all notifications" + :aria-busy="loadingArchiveAll" @click="confirmAndArchiveAll" > Archive All </Button> </TabsContent> <TabsContent value="archived"> <Button :disabled="loadingDeleteAll" variant="link" size="sm" class="text-muted-foreground text-base p-0" + aria-label="Delete all notifications" + :aria-busy="loadingDeleteAll" @click="confirmAndDeleteAll" > Delete All </Button> </TabsContent>Also applies to: 58-79
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
deleteAllNotificationsmutation.