[WEB-2382]chore:notification files restructuring #6181
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces modifications to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 1
🧹 Outside diff range and nitpick comments (1)
web/core/store/notifications/notification.ts (1)
29-29: Consider adding constructor validationSince
idis now required, consider adding validation in the constructor to ensure it's provided.constructor( private store: CoreRootStore, private notification: TNotification ) { + if (!notification.id) { + throw new Error('Notification ID is required'); + } makeObservable(this, {Also applies to: 98-98
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
packages/types/src/workspace-notifications.d.ts(1 hunks)web/app/[workspaceSlug]/(projects)/notifications/layout.tsx(1 hunks)web/ce/components/workspace-notifications/index.ts(1 hunks)web/core/components/workspace-notifications/index.ts(1 hunks)web/core/components/workspace-notifications/root.tsx(1 hunks)web/core/components/workspace-notifications/sidebar/notification-card/index.ts(0 hunks)web/core/store/notifications/notification.ts(1 hunks)
💤 Files with no reviewable changes (1)
- web/core/components/workspace-notifications/sidebar/notification-card/index.ts
✅ Files skipped from review due to trivial changes (3)
- web/ce/components/workspace-notifications/index.ts
- web/app/[workspaceSlug]/(projects)/notifications/layout.tsx
- web/core/components/workspace-notifications/root.tsx
🔇 Additional comments (3)
web/core/components/workspace-notifications/index.ts (1)
3-3: LGTM! Verify root module exports.
The additional export follows the barrel file pattern correctly.
Let's verify the root module exports:
✅ Verification successful
LGTM! The root module exports are properly structured.
The root module exports the NotificationsSidebarRoot component and related notification components, which are properly structured React components following best practices. The exports are consistent with the codebase's architecture and there are no circular dependencies or issues found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the exports from the root module
# Check if the root module exists and examine its exports
fd -e tsx -e ts "root" web/core/components/workspace-notifications/ --exec cat {}
Length of output: 19796
packages/types/src/workspace-notifications.d.ts (1)
38-38:
Breaking Change: TNotification.id is now required
Changing id from string | undefined to string is a breaking change that requires all notification objects to have an ID. This could affect existing code that expects id to be optional.
Let's verify the impact:
web/core/store/notifications/notification.ts (1)
29-29: Verify error handling in notification service calls
The change to make id required might affect error handling in the notification service methods.
✅ Verification successful
Based on the analysis of the notification service and store implementations, I can now provide a final response:
Making id required is safe - error handling is properly implemented
The notification service and store have comprehensive error handling in place. All operations that involve the notification ID have proper error handling with try-catch blocks that:
- Preserve the previous state in case of errors through
runInAction - Properly propagate errors up the chain
- Handle state rollbacks for operations like read/unread, archive/unarchive, and snooze
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check error handling in notification service
# Look for error handling patterns in notification service
rg -A 5 "catch.*error" services/workspace-notification.service
Length of output: 144
Script:
#!/bin/bash
# Let's first find the correct path for notification-related services
fd -t f "notification.*service"
# Then search for error handling patterns in notification-related files
rg -A 5 "catch.*error" --type ts
Length of output: 73538
Script:
#!/bin/bash
# Let's check the notification service and error handling implementation
fd -t f "notification.*service.ts" web/core/services/
# Then examine the error handling in the found service file
rg -A 5 "catch.*error" web/core/services/workspace-notification.service.ts
# Also check the notification store implementation for error handling
rg -A 5 "catch.*error" web/core/store/notifications/notification.ts
Length of output: 2311
| export class Notification implements INotification { | ||
| // observables | ||
| id: string | undefined = undefined; | ||
| id: string; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor: Remove redundant ID null checks
Since id is now required, the null checks in methods like updateNotification, markNotificationAsRead, etc., are redundant.
Here's how to clean up the redundant checks:
- if (!this.id) return undefined;This change should be applied to all methods that have this check.
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
packages/types/src/workspace-notifications.d.ts (1)
Line range hint
38-58: Consider standardizing optional property definitions.The type currently mixes two styles of optional properties:
- Using union with undefined:
string | undefined- Using optional property marker:
property?: stringConsider standardizing to use optional properties (?) for better TypeScript idioms and consistency.
Example refactor for better consistency:
export type TNotification = { id: string; - title: string | undefined; - data: TNotificationData | undefined; + title?: string; + data?: TNotificationData; // ... apply similar changes to other properties };web/core/store/notifications/notification.ts (2)
Line range hint
165-166: Remove redundant ID null checksSince
idis now a required string property, theif (!this.id) return undefinedchecks in methods are redundant and can never be reached. Consider removing these checks from:
- updateNotification
- markNotificationAsRead
- markNotificationAsUnRead
- archiveNotification
- unArchiveNotification
- snoozeNotification
- unSnoozeNotification
Example diff for one method (apply similar pattern to others):
updateNotification = async ( workspaceSlug: string, payload: Partial<TNotification> ): Promise<TNotification | undefined> => { - if (!this.id) return undefined; try { const notification = await workspaceNotificationService.updateNotificationById(workspaceSlug, this.id, payload);Also applies to: 190-191, 217-218, 244-245, 271-272, 298-299, 325-326
29-29: LGTM: Improved type safetyMaking
ida required property is a good architectural decision that:
- Ensures data consistency
- Reduces potential runtime errors
- Aligns with typical notification system requirements where each notification must be uniquely identifiable
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
packages/types/src/workspace-notifications.d.ts(1 hunks)web/app/[workspaceSlug]/(projects)/notifications/layout.tsx(1 hunks)web/ce/components/workspace-notifications/index.ts(1 hunks)web/core/components/workspace-notifications/index.ts(1 hunks)web/core/components/workspace-notifications/root.tsx(1 hunks)web/core/components/workspace-notifications/sidebar/notification-card/index.ts(0 hunks)web/core/store/notifications/notification.ts(1 hunks)
💤 Files with no reviewable changes (1)
- web/core/components/workspace-notifications/sidebar/notification-card/index.ts
✅ Files skipped from review due to trivial changes (2)
- web/ce/components/workspace-notifications/index.ts
- web/app/[workspaceSlug]/(projects)/notifications/layout.tsx
🧰 Additional context used
📓 Learnings (1)
web/core/components/workspace-notifications/root.tsx (1)
Learnt from: mathalav55
PR: makeplane/plane#6107
File: web/ce/components/workspace-notifications/sidebar/notification-card/options/read.tsx:11-12
Timestamp: 2024-11-28T07:02:15.514Z
Learning: Some components are still in core and have not been moved yet, so their import paths remain the same.
🔇 Additional comments (4)
web/core/components/workspace-notifications/index.ts (1)
3-3: LGTM! Clean addition of root module export.
The new export follows the established pattern and supports the notification files restructuring objective.
packages/types/src/workspace-notifications.d.ts (1)
38-38: Verify handling of notification IDs across the codebase.
Making id required instead of optional is a good practice as notifications should always have an ID. However, we should verify that all notification creation/handling code properly sets the ID.
Let's verify the impact:
✅ Verification successful
The change to make id required is safe to proceed with
Based on the codebase analysis:
-
The main notification handling code in
workspace-notifications.store.tsalready checks for notification ID presence:if (!notification.id) return;is used as a guard clause- All notification operations require a valid ID
-
The UI components in
notification-card/item.tsxsafely handle potential undefined IDs:- Uses optional chaining
notification?.id - Has null checks
if (!notificationId || !notification?.id)
- Uses optional chaining
-
The notification class in
notification.tsalways expects and uses the ID property, suggesting it's already treated as required in practice.
The codebase is already handling notifications with the assumption that IDs should exist, making this type change align with the actual implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential places that might expect undefined notification IDs
# Search for notification object creation/handling
rg -t typescript -t javascript "new\s+(?:Type)?Notification|notification\.id|notifications\[.*\]\.id"
# Search for direct property access that might assume optional id
rg -t typescript -t javascript "notification(?:\s+)?\?\.id|notification!\.id"
Length of output: 253
Script:
#!/bin/bash
# Let's try with ripgrep without file type filters
# Search for notification object creation/handling
rg "new\s+(?:Type)?Notification|notification\.id|notifications\[.*\]\.id"
# Search for direct property access that might assume optional id
rg "notification(?:\s+)?\?\.id|notification!\.id"
# Also search for any files that might import or use TNotification type
rg "TNotification"
# And look for notification-related files
fd "notification" --type f
Length of output: 17917
web/core/store/notifications/notification.ts (2)
Line range hint 1-1: LGTM: Robust error handling pattern
The error handling pattern is well-implemented:
- Preserves previous state on error
- Allows error propagation to upper layers
- Consistently applied across all methods
Also applies to: 165-187
29-29: Verify handling of notification creation
The change from string | undefined to string for the id property is a breaking change that requires all notifications to have a defined ID. Please ensure that:
- All notification creation flows provide a valid ID
- Any existing code handling undefined IDs is updated
✅ Verification successful
Let me gather more information about the notification creation and handling to verify this change comprehensively.
Let me search for the notification type definition and its usage to verify this change comprehensively.
Based on the gathered information, I can now generate the final response:
Change from string | undefined to string for notification ID is safe
The change is safe because:
- The TNotification type in
packages/types/src/workspace-notifications.d.tsalready definesidas required string - The only notification creation occurs in
workspace-notifications.store.tswhere it's created with a notification object that must conform to TNotification type - No undefined checks for notification IDs were found in the codebase that would be broken by this change
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for notification creation patterns to verify ID handling
ast-grep --pattern 'new Notification($_, $_)'
# Search for potential undefined ID checks that might need updates
rg 'notification.*id.*undefined|notification.*id.*null' -t ts
Length of output: 646
Script:
#!/bin/bash
# Check the Notification class implementation
ast-grep --pattern 'class Notification {
$$$
}'
# Check notification interface/type definitions
ast-grep --pattern 'interface $_ {
id: $_
$$$
}'
# Look for notification creation in tests
rg "new Notification" -t test
Length of output: 26141
Script:
#!/bin/bash
# Search for notification type definition
ast-grep --pattern 'type $_ = {
id: $_
$$$
}'
# Search for notification interface definition
ast-grep --pattern 'interface $_ {
id: $_
$$$
}'
# Look for notification creation patterns
rg "new Notification\(" -A 2
Length of output: 55640
| import { NotificationCardListRoot } from "@/plane-web/components/workspace-notifications"; | ||
|
|
There was a problem hiding this comment.
💡 Codebase verification
Multiple components still using old import path need attention
The verification reveals that while NotificationCardListRoot has been moved to the new path, several other components in the file are still using the old import path @/components/workspace-notifications:
NotificationsLoaderNotificationEmptyStateNotificationSidebarHeaderAppliedFilters
Additionally, there are multiple other files in the codebase still importing components from the old path, suggesting this migration is incomplete.
🔗 Analysis chain
Import path change aligns with component restructuring.
The NotificationCardListRoot component has been moved to @/plane-web/components, which aligns with the ongoing restructuring effort. Based on the retrieved learning, this is part of a gradual migration where some components remain in core.
Let's verify if there are other notification components that might need similar updates:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other notification components that might need path updates
# Search for notification component imports
rg -t typescript -t javascript "from\s+['\"]@/components/workspace-notifications['\"]"
# Search for potential notification components still in the old location
fd -t f . "components/workspace-notifications" | grep -v "plane-web"
Length of output: 316
Script:
#!/bin/bash
# Let's try a different approach to find notification-related imports and components
# Search for imports containing notification-related paths
rg "from.*notification" --type=tsx --type=ts
# Find notification-related files
fd "notification" -e tsx -e ts
Length of output: 706
Script:
#!/bin/bash
# Let's check the content of these files to find any imports we need to verify
# Check imports in notification-related files
rg -l "@/components/workspace-notifications" web/core/components/workspace-notifications/notification-app-sidebar-option.tsx
# Let's also check the content of the root.tsx file to understand the full context
cat web/core/components/workspace-notifications/root.tsx
Length of output: 4420
Script:
#!/bin/bash
# Let's check the imports from @/components/workspace-notifications in the root.tsx file
# Search for the components that are still being imported from the old path
ast-grep --pattern 'import { $_ } from "@/components/workspace-notifications"'
# Also check if these components exist in the new location
fd -e tsx "NotificationsLoader|NotificationEmptyState|NotificationSidebarHeader|AppliedFilters" web/plane-web/components/workspace-notifications
Length of output: 1967
* chore: adjusted increment/decrement for unread count * chore: improved param handling for unread notification count function * chore:file restructuring * fix:notification types * chore:file restructuring * chore:modified notfication types * chore: modified types for notification * chore:removed redundant checks for id
Description
Restructured files for notifications.
Modified type for notification.
Type of Change
References
WEB-2382
Summary by CodeRabbit
Summary by CodeRabbit
New Features
rootmodule, expanding available components for notifications.Bug Fixes
Chores