feat(web): delete notifications from archive view #941
Conversation
WalkthroughThe changes introduce a delete functionality for notifications in the system. This includes the addition of a Changes
Possibly related PRs
Suggested reviewers
Poem
🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (2)
web/components/Notifications/Item.vue (1)
42-54: Consider renaming deleteFromServer for consistency
The mutation setup is well-structured, and the renaming of loading to archiveInProgress improves clarity. However, for consistency with the archive mutation naming, consider renaming deleteFromServer to remove or delete.
-const { mutate: deleteFromServer, loading: deleting } = useMutation(
+const { mutate: delete, loading: deleting } = useMutation(web/composables/gql/gql.ts (1)
61-64: Enhance the documentation comment for DeleteNotification.
The current documentation comment is generic. Consider adding specific details about the mutation's purpose and parameters.
Example enhancement:
/**
* The graphql function is used to parse GraphQL queries into a document that can be used by GraphQL clients.
+ *
+ * @description Mutation to delete a notification by its ID and type from the archive view
+ * @param {string} id - The unique identifier of the notification to delete
+ * @param {NotificationType} type - The type of the notification
+ * @returns {Promise<{ archive: { total: number } }>} The updated archive total after deletion
*/📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- web/components/Notifications/Item.vue (3 hunks)
- web/components/Notifications/graphql/notification.query.ts (1 hunks)
- web/composables/gql/gql.ts (2 hunks)
- web/composables/gql/graphql.ts (3 hunks)
- web/helpers/apollo-cache/index.ts (2 hunks)
🔇 Additional comments (8)
web/components/Notifications/graphql/notification.query.ts (1)
51-59: Verify the NotificationType enum usage.
The mutation requires a NotificationType enum parameter. Let's verify its definition in the schema.
✅ Verification successful
NotificationType enum is properly defined and matches the mutation usage
The NotificationType enum is correctly defined in both frontend (web/composables/gql/graphql.ts) and backend (api/src/graphql/generated/api/types.ts) with matching values ARCHIVE and UNREAD. The mutation's type parameter usage is valid and consistent with the schema definition.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for NotificationType enum definition
ast-grep --pattern 'enum NotificationType {
$$$
}'
Length of output: 518
web/components/Notifications/Item.vue (1)
8-17: LGTM: Import changes are well-structured
The new imports for TrashIcon and deleteNotification mutation are properly organized and align with the feature requirements.
web/helpers/apollo-cache/index.ts (1)
53-55: LGTM! Clean formatting improvement.
The restructured format for passing the offset parameter improves readability while maintaining the same functionality.
web/composables/gql/gql.ts (1)
20-20: Verify GraphQL schema compatibility.
Let's verify that the mutation is properly defined in the GraphQL schema and that all required types are available.
Also applies to: 61-64
✅ Verification successful
GraphQL schema and mutation are correctly defined and implemented
The verification confirms that:
- The
NotificationTypeenum is properly defined in the schema withUNREADandARCHIVEvalues - The
deleteNotificationmutation is correctly defined in the schema with required parametersid: String!andtype: NotificationType! - The mutation returns
NotificationOverviewtype as specified in the code - The implementation in
gql.tsmatches the schema definition
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify GraphQL schema compatibility for DeleteNotification mutation
# Test 1: Check if NotificationType enum is defined in schema
rg -l "enum NotificationType" --type graphql
# Test 2: Check if deleteNotification mutation is defined in schema
rg -l "deleteNotification.*NotificationType!" --type graphql
# Test 3: Check for any existing usages of the mutation
rg "DeleteNotification.*mutation" --type typescript --type vue
Length of output: 273
Script:
#!/bin/bash
# Let's try different approaches to find GraphQL schema and related files
# Find potential GraphQL schema files
fd -e graphql -e gql
# Find TypeScript files that might contain the schema
fd -e ts -e tsx | xargs rg "NotificationType"
# Look for DeleteNotification mutation usage
fd -e ts -e tsx -e vue | xargs rg "DeleteNotification"
# Check for any schema.graphql or similar files
fd "schema" -e graphql -e gql
Length of output: 16729
Script:
#!/bin/bash
# Let's check the notifications GraphQL schema and resolver implementation
# Check notifications schema definition
cat api/src/graphql/schema/types/notifications/notifications.graphql
# Look for deleteNotification mutation implementation in resolver
rg -A 10 "deleteNotification.*NotificationType" api/src/unraid-api/graph/resolvers/notifications/notifications.resolver.ts
Length of output: 2247
web/composables/gql/graphql.ts (4)
824-824: LGTM: Optional formatted timestamp field added to Notification type
The addition of formattedTimestamp as an optional string field to the Notification type is well-defined and follows GraphQL schema best practices.
1687-1693: LGTM: DeleteNotification mutation properly defined
The mutation is well-structured with:
- Required parameters:
id(String) andtype(NotificationType) - Return type: NotificationOverview with archive count
1746-1746: LGTM: DeleteNotification document implementation is complete
The GraphQL document definition properly maps to the mutation type and includes the necessary selections for the response.
Line range hint 1687-1746: Verify the mutation's error handling
While the implementation looks correct, it's important to verify error handling for common scenarios:
- Deleting non-existent notifications
- Deleting already deleted notifications
- Permission-related errors
|
think we should add a confirmation dialog or something before actually deleting the notification? |
I think we're good because the user has already dropped into another view (Unread -> Archived). Plus most people's notifications are already in memory and non-permanent. |
pujitm
force-pushed
the
feat/delete-notifications
branch
from
e8f8df9 to
9f5a7f8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
web/components/Notifications/Item.vue (1)
92-92: Add success feedback for delete action.Consider adding a success toast or notification to provide feedback when the deletion is successful. This improves user experience by confirming their action was completed.
const deleteNotification = reactive( useMutation(deleteMutation, { variables: { id: props.id, type: props.type }, + onCompleted: () => { + // Assuming you have a toast/notification system + showToast({ + message: 'Notification deleted successfully', + type: 'success' + }); + } }) );Also applies to: 111-120
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
web/components/Notifications/Item.vue(3 hunks)
🧰 Additional context used
📓 Learnings (1)
web/components/Notifications/Item.vue (1)
Learnt from: pujitm
PR: unraid/api#941
File: web/components/Notifications/Item.vue:0-0
Timestamp: 2024-11-01T17:52:55.311Z
Learning: In the `web/components/Notifications/Item.vue` component, ensure that when delete or archive mutations fail, the notification remains displayed, and an error message is shown below the action buttons to inform the user.
🔇 Additional comments (2)
web/components/Notifications/Item.vue (2)
8-8: LGTM: Import changes are well-organized.
The imports are properly structured and include all necessary components for the new delete functionality.
Also applies to: 13-17
42-55: LGTM: Error handling implementation is robust.
The error handling implementation aligns with the learnings and properly displays errors from both mutations. The reactive wrappers and computed error property provide a clean way to manage mutation states.
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)
91-102: Add type safety and error handling.Consider adding TypeScript types for the args parameter and error handling for missing IDs:
- merge(_, incoming, { cache, args }) { + merge( + _: unknown, + incoming: unknown, + { cache, args }: { + cache: ApolloCache<unknown>; + args?: { id?: string | null } + } + ) { + if (!args?.id) { + console.warn('deleteNotification: No notification ID provided'); + return incoming; + } const id = cache.identify({ id: args.id, __typename: NotificationType, }); cache.evict({ id }); cache.gc(); return incoming; },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
web/composables/gql/typename.ts(1 hunks)web/helpers/apollo-cache/index.ts(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- web/composables/gql/typename.ts
🔇 Additional comments (3)
web/helpers/apollo-cache/index.ts (3)
3-3: LGTM!
The import is correctly added and used in the new cache configuration.
54-56: LGTM!
The formatting changes improve readability while maintaining the same functionality.
80-103: LGTM! Well-implemented cache management for notification deletion.
The implementation correctly:
- Uses
cache.identify()for proper cache key generation - Performs garbage collection to prevent dangling references
- Includes clear documentation
3 participants
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
These updates enhance the notification management system, providing users with more control and improved information visibility.