refactor(api): delete only archived notifications in deleteAll #983
Conversation
WalkthroughThe changes in this pull request rename the GraphQL mutation Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 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 (
|
![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 (3)
api/src/unraid-api/graph/resolvers/notifications/notifications.service.ts (2)
320-326: RefactordeleteAllNotificationsto eliminate unnecessary recursionThe
deleteAllNotificationsmethod currently uses recursion to delete notifications when notypeis provided. This can be simplified by directly callingdeleteNotificationsfor bothARCHIVEandUNREADtypes without recursion.Apply this diff to simplify the method:
public async deleteAllNotifications(type?: NotificationType) { if (type) { await this.deleteNotifications(type); } else { - await this.deleteNotifications(NotificationType.ARCHIVE); - await this.deleteAllNotifications(NotificationType.UNREAD); + await this.deleteNotifications(NotificationType.ARCHIVE); + await this.deleteNotifications(NotificationType.UNREAD); } return this.getOverview(); }
711-711: Fix typo in commentThere's a typo in the comment: "non-numberS" should be "non-number".
api/src/graphql/schema/types/notifications/notifications.graphql (1)
23-23: Consider adding an optional type parameter for future flexibility.While the current implementation specifically targets archived notifications, consider making the mutation more flexible by adding an optional type parameter, similar to other mutations in the schema. This would make it easier to extend functionality in the future without breaking changes.
- deleteAllNotifications: NotificationOverview! + """ + Deletes all notifications of the specified type. Defaults to ARCHIVE if not provided. + """ + deleteAllNotifications(type: NotificationType): NotificationOverview!
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (1)
api/src/graphql/generated/api/types.tsis excluded by!**/generated/**
📒 Files selected for processing (4)
api/src/graphql/schema/types/notifications/notifications.graphql(1 hunks)api/src/unraid-api/graph/resolvers/notifications/notifications.resolver.ts(2 hunks)api/src/unraid-api/graph/resolvers/notifications/notifications.service.ts(2 hunks)web/components/Notifications/Sidebar.vue(1 hunks)
🔇 Additional comments (3)
api/src/unraid-api/graph/resolvers/notifications/notifications.resolver.ts (1)
77-77: Ensure deleteAllNotifications deletes only archived notifications
The deleteAllNotifications mutation now specifies NotificationType.ARCHIVE, ensuring only archived notifications are deleted, which aligns with the intended functionality.
web/components/Notifications/Sidebar.vue (1)
25-25: Confirmation message accurately reflects action
The updated confirmation message correctly specifies that only archived notifications will be permanently deleted.
api/src/graphql/schema/types/notifications/notifications.graphql (1)
20-22: LGTM! Clear documentation of the mutation's purpose.
The description accurately reflects that this mutation deletes archived notifications, aligning with the PR objective.
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
pujitm
force-pushed
the
fix/notif-delete
branch
from
a2a81db to
1e79e1a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
api/src/unraid-api/graph/resolvers/notifications/notifications.service.ts (1)
704-704: Fix typo in comment.There's a typo in the comment: "non-numberS" should be "non-number".
- // i.e. if unixStringSeconds is an empty string or represents a non-numberS + // i.e. if unixStringSeconds is an empty string or represents a non-number
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (1)
api/src/graphql/generated/api/types.tsis excluded by!**/generated/**
📒 Files selected for processing (4)
api/src/graphql/schema/types/notifications/notifications.graphql(1 hunks)api/src/unraid-api/graph/resolvers/notifications/notifications.resolver.ts(2 hunks)api/src/unraid-api/graph/resolvers/notifications/notifications.service.ts(2 hunks)web/components/Notifications/Sidebar.vue(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- api/src/graphql/schema/types/notifications/notifications.graphql
- web/components/Notifications/Sidebar.vue
- api/src/unraid-api/graph/resolvers/notifications/notifications.resolver.ts
🔇 Additional comments (1)
api/src/unraid-api/graph/resolvers/notifications/notifications.service.ts (1)
295-310: LGTM! Well-structured implementation of type-specific notification deletion.
The method correctly handles both the file system operations and state management, ensuring that the overview stats are properly reset after deletion.
api/src/unraid-api/graph/resolvers/notifications/notifications.service.ts
Show resolved
Hide resolved
pujitm
force-pushed
the
fix/notif-delete
branch
from
1e79e1a to
1878d3e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
api/src/unraid-api/graph/resolvers/notifications/notifications.service.ts (1)
Line range hint
704-708: Fix typo in comment.There's a typo in the comment: "non-numberS" should be "non-number".
Apply this diff to fix the typo:
- // i.e. if unixStringSeconds is an empty string or represents a non-numberS + // i.e. if unixStringSeconds is an empty string or represents a non-number
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (1)
api/src/graphql/generated/api/types.tsis excluded by!**/generated/**
📒 Files selected for processing (4)
api/src/graphql/schema/types/notifications/notifications.graphql(1 hunks)api/src/unraid-api/graph/resolvers/notifications/notifications.resolver.ts(2 hunks)api/src/unraid-api/graph/resolvers/notifications/notifications.service.ts(2 hunks)web/components/Notifications/Sidebar.vue(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- web/components/Notifications/Sidebar.vue
- api/src/graphql/schema/types/notifications/notifications.graphql
- api/src/unraid-api/graph/resolvers/notifications/notifications.resolver.ts
🔇 Additional comments (2)
api/src/unraid-api/graph/resolvers/notifications/notifications.service.ts (2)
295-310: LGTM! Well-structured implementation of type-specific notification deletion.
The method correctly handles both the file system operations and stats reset, with proper documentation.
311-321:
Implementation doesn't match PR objective.
The current implementation deletes both archived and unread notifications, but according to the PR objective, it should only delete archived notifications.
Apply this diff to align with the PR objective:
/**
- * Deletes all notifications from disk while preserving the directory structure.
+ * Deletes all archived notifications from disk while preserving the directory structure.
* Resets overview stats to zero.
*
* @returns The updated notification overview stats
*/
public async deleteAllNotifications() {
await this.deleteNotifications(NotificationType.ARCHIVE);
- await this.deleteNotifications(NotificationType.UNREAD);
return this.getOverview();
}Likely invalid or redundant comment.
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
web/components/Notifications/graphql/notification.query.ts (1)
71-73: Update GraphQL operation name to match mutation nameThe exported constant has been correctly renamed to
deleteArchivedNotifications, but the GraphQL operation name is stillDeleteAllNotifications. For better maintainability and consistency, consider updating the operation name as well.export const deleteArchivedNotifications = graphql(/* GraphQL */ ` - mutation DeleteAllNotifications { + mutation DeleteArchivedNotifications { deleteArchivedNotifications {web/composables/gql/gql.ts (1)
23-23: Maintain consistency with operation name changeRelated to the operation name change suggested in
notification.query.ts, update the operation name here as well for consistency."\n mutation DeleteAllNotifications {\n deleteArchivedNotifications {\n +"\n mutation DeleteArchivedNotifications {\n deleteArchivedNotifications {\nAlso applies to: 76-76
web/composables/gql/graphql.ts (1)
1700-1700: Maintain consistency with operation name changeRelated to the operation name changes suggested in
notification.query.tsandgql.ts, update the operation name here as well for consistency. Also consider renaming the type fromDeleteAllNotificationsMutationtoDeleteArchivedNotificationsMutationto better reflect its purpose.-export type DeleteAllNotificationsMutation = { __typename?: 'Mutation', +export type DeleteArchivedNotificationsMutation = { __typename?: 'Mutation',Also applies to: 1760-1760
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (1)
api/src/graphql/generated/api/types.tsis excluded by!**/generated/**
📒 Files selected for processing (6)
api/src/graphql/schema/types/notifications/notifications.graphql(1 hunks)api/src/unraid-api/graph/resolvers/notifications/notifications.resolver.ts(2 hunks)web/components/Notifications/Sidebar.vue(2 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 skipped from review as they are similar to previous changes (3)
- web/components/Notifications/Sidebar.vue
- api/src/graphql/schema/types/notifications/notifications.graphql
- api/src/unraid-api/graph/resolvers/notifications/notifications.resolver.ts
pujitm
force-pushed
the
fix/notif-delete
branch
from
b0386e6 to
b8c2f0e
Compare
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
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)
Line range hint
188-193: Add documentation for consistency with other mutation fields.The implementation looks good and follows the established patterns in the codebase. However, for consistency with other mutation fields like
archiveAllanddeleteNotification, consider adding documentation that explains the cache behavior.Add documentation above the merge function:
deleteArchivedNotifications: { + /** + * Clears the notifications cache when archived notifications are deleted + * to ensure that notification queries are invalidated and refetched. + * + * @param _ - Existing cache value (unused) + * @param incoming - Result from the server + * @param context - Apollo context containing cache instance + * @returns The incoming result to be cached + */ merge(_, incoming, { cache }) { cache.evict({ fieldName: 'notifications' }); cache.gc(); return incoming; }, },
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
Summary by CodeRabbit
New Features
Bug Fixes