[WEB-2382] chore: notifications file rearranging #6107
Conversation
WalkthroughThis pull request involves modifications to the notifications system within the application. Key changes include the renaming of the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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: 8
🧹 Outside diff range and nitpick comments (8)
web/app/[workspaceSlug]/(projects)/notifications/layout.tsx (1)
6-12: Consider adding prop types documentationThe layout component accepts children as a prop but lacks documentation about its purpose and usage. Consider adding JSDoc comments to improve maintainability.
+/** + * Layout component for the notifications page that renders a sidebar and content area + * @param {Object} props - Component props + * @param {React.ReactNode} props.children - Content to render in the main area + */ export default function ProjectInboxIssuesLayout({ children }: { children: React.ReactNode }) {web/ce/store/root.store.ts (1)
6-14: Clean integration with existing store architecture.The addition of workspace notifications follows the established pattern of extending
CoreRootStoreand maintains consistency with the existing architecture. The approach aligns well with the notification system restructuring objectives.web/ce/components/workspace-notifications/sidebar/notification-card/options/archive.tsx (1)
Line range hint
29-39: Enhance error handling for better user feedbackWhile the success case is well handled, the error case could be improved to provide better user feedback and error tracking.
Consider this enhancement:
try { const request = data.archived_at ? unArchiveNotification : archiveNotification; await request(workspaceSlug); captureEvent(NOTIFICATION_ARCHIVED, { issue_id: data?.data?.issue?.id, tab: currentNotificationTab, state: "SUCCESS", }); setToast({ title: data.archived_at ? "Notification un-archived" : "Notification archived", type: TOAST_TYPE.SUCCESS, }); } catch (e) { - console.error(e); + captureEvent(NOTIFICATION_ARCHIVED, { + issue_id: data?.data?.issue?.id, + tab: currentNotificationTab, + state: "FAILED", + error: e instanceof Error ? e.message : "Unknown error", + }); + setToast({ + title: "Failed to update notification", + message: "Please try again later", + type: TOAST_TYPE.ERROR, + }); }web/ce/components/workspace-notifications/sidebar/notification-card/root.tsx (1)
Line range hint
1-56: Consider documenting the CE/EE component splitSince this component is now being used in both CE (Community Edition) and EE (Enterprise Edition), consider adding a comment explaining the shared nature of this component and any special considerations for maintaining CE/EE compatibility.
Consider adding a comment at the top of the file:
"use client"; +// This component is shared between CE and EE editions. +// Ensure changes maintain compatibility with both editions. + import { FC } from "react";web/ce/components/workspace-notifications/sidebar/root.tsx (2)
25-40: Consider extracting state management logic into a custom hook.The component handles multiple state management concerns. Consider extracting the workspace and notifications state logic into a custom hook like
useNotificationsSidebarto improve maintainability and reusability.Example implementation:
const useNotificationsSidebar = (workspaceSlug: string) => { const { getWorkspaceBySlug } = useWorkspace(); const { currentSelectedNotificationId, unreadNotificationsCount, loader, notificationIdsByWorkspaceId, currentNotificationTab, setCurrentNotificationTab, } = useWorkspaceNotifications(); const workspace = workspaceSlug ? getWorkspaceBySlug(workspaceSlug) : undefined; const notificationIds = workspace ? notificationIdsByWorkspaceId(workspace.id) : undefined; return { workspace, notificationIds, currentSelectedNotificationId, unreadNotificationsCount, loader, currentNotificationTab, setCurrentNotificationTab, }; };
41-41: Consider adding a more graceful fallback UI.Instead of returning an empty fragment, consider showing a fallback UI to prevent layout shifts and provide better user feedback.
- if (!workspaceSlug || !workspace) return <></>; + if (!workspaceSlug || !workspace) return ( + <div className="flex items-center justify-center h-full"> + <p className="text-custom-text-400">Workspace not found</p> + </div> + );web/ce/store/notifications/notification.ts (2)
55-55: LGTM! Critical store dependency properly handledThe store parameter type change is correctly implemented. The code maintains proper error handling and state rollback mechanisms, particularly for the critical unread notification count management.
Consider documenting the store dependency and its critical role in notification state management, especially for the unread count feature, to help future maintainers understand the importance of this coupling.
Line range hint
156-162: Consider enhancing type safety in the mutation helperThe
mutateNotificationhelper uses a flexible but potentially type-unsafe approach withlodash/set. Consider adding runtime type checks or using a more type-safe mutation approach.mutateNotification = (notification: Partial<TNotification>) => { Object.entries(notification).forEach(([key, value]) => { - if (key in this) { + if (key in this && typeof value === typeof this[key as keyof this]) { set(this, key, value); + } else { + console.warn(`Invalid mutation attempt: ${key}`); } }); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (33)
web/app/[workspaceSlug]/(projects)/notifications/layout.tsx(1 hunks)web/ce/components/workspace-notifications/index.ts(1 hunks)web/ce/components/workspace-notifications/sidebar/index.ts(1 hunks)web/ce/components/workspace-notifications/sidebar/notification-card/item.tsx(1 hunks)web/ce/components/workspace-notifications/sidebar/notification-card/options/archive.tsx(1 hunks)web/ce/components/workspace-notifications/sidebar/notification-card/options/read.tsx(1 hunks)web/ce/components/workspace-notifications/sidebar/notification-card/options/root.tsx(1 hunks)web/ce/components/workspace-notifications/sidebar/notification-card/options/snooze/root.tsx(1 hunks)web/ce/components/workspace-notifications/sidebar/notification-card/root.tsx(1 hunks)web/ce/components/workspace-notifications/sidebar/root.tsx(1 hunks)web/ce/store/notifications/notification.ts(2 hunks)web/ce/store/notifications/workspace-notifications.store.ts(2 hunks)web/ce/store/root.store.ts(1 hunks)web/core/components/workspace-notifications/sidebar/index.ts(1 hunks)web/core/components/workspace-notifications/sidebar/root.tsx(1 hunks)web/core/hooks/store/notifications/use-notification.ts(1 hunks)web/core/hooks/store/notifications/use-workspace-notifications.ts(1 hunks)web/core/store/root.store.ts(0 hunks)web/ee/components/workspace-notifications/index.ts(1 hunks)web/ee/components/workspace-notifications/sidebar/index.ts(1 hunks)web/ee/components/workspace-notifications/sidebar/notification-card/index.ts(1 hunks)web/ee/components/workspace-notifications/sidebar/notification-card/item.tsx(1 hunks)web/ee/components/workspace-notifications/sidebar/notification-card/options/archive.tsx(1 hunks)web/ee/components/workspace-notifications/sidebar/notification-card/options/button.tsx(1 hunks)web/ee/components/workspace-notifications/sidebar/notification-card/options/index.ts(1 hunks)web/ee/components/workspace-notifications/sidebar/notification-card/options/read.tsx(1 hunks)web/ee/components/workspace-notifications/sidebar/notification-card/options/root.tsx(1 hunks)web/ee/components/workspace-notifications/sidebar/notification-card/options/snooze/index.ts(1 hunks)web/ee/components/workspace-notifications/sidebar/notification-card/options/snooze/modal.tsx(1 hunks)web/ee/components/workspace-notifications/sidebar/notification-card/options/snooze/root.tsx(1 hunks)web/ee/components/workspace-notifications/sidebar/notification-card/root.tsx(1 hunks)web/ee/components/workspace-notifications/sidebar/root.tsx(1 hunks)web/ee/store/workspace-notifications/workspace-notifications.store.ts(1 hunks)
💤 Files with no reviewable changes (1)
- web/core/store/root.store.ts
✅ Files skipped from review due to trivial changes (23)
- web/ce/components/workspace-notifications/index.ts
- web/ce/components/workspace-notifications/sidebar/index.ts
- web/ce/components/workspace-notifications/sidebar/notification-card/item.tsx
- web/ce/components/workspace-notifications/sidebar/notification-card/options/root.tsx
- web/ce/components/workspace-notifications/sidebar/notification-card/options/snooze/root.tsx
- web/core/components/workspace-notifications/sidebar/index.ts
- web/core/components/workspace-notifications/sidebar/root.tsx
- web/core/hooks/store/notifications/use-workspace-notifications.ts
- web/ee/components/workspace-notifications/index.ts
- web/ee/components/workspace-notifications/sidebar/index.ts
- web/ee/components/workspace-notifications/sidebar/notification-card/index.ts
- web/ee/components/workspace-notifications/sidebar/notification-card/item.tsx
- web/ee/components/workspace-notifications/sidebar/notification-card/options/archive.tsx
- web/ee/components/workspace-notifications/sidebar/notification-card/options/button.tsx
- web/ee/components/workspace-notifications/sidebar/notification-card/options/index.ts
- web/ee/components/workspace-notifications/sidebar/notification-card/options/read.tsx
- web/ee/components/workspace-notifications/sidebar/notification-card/options/root.tsx
- web/ee/components/workspace-notifications/sidebar/notification-card/options/snooze/index.ts
- web/ee/components/workspace-notifications/sidebar/notification-card/options/snooze/modal.tsx
- web/ee/components/workspace-notifications/sidebar/notification-card/options/snooze/root.tsx
- web/ee/components/workspace-notifications/sidebar/notification-card/root.tsx
- web/ee/components/workspace-notifications/sidebar/root.tsx
- web/ee/store/workspace-notifications/workspace-notifications.store.ts
🔇 Additional comments (12)
web/app/[workspaceSlug]/(projects)/notifications/layout.tsx (1)
9-9: LGTM: Component usage updated consistently
The component usage has been updated to match the new import, maintaining consistency with the architectural changes.
web/core/hooks/store/notifications/use-notification.ts (2)
Line range hint 7-13: LGTM! Hook implementation remains clean and functional
The hook's implementation maintains good practices:
- Proper context validation
- Handles undefined cases gracefully
- Type-safe return value
5-5: Verify the updated import path across the codebase
The import path change aligns with the notification file rearrangement objective. Let's verify the consistency of this change.
✅ Verification successful
Import path update is consistent across the codebase
The new import path @/plane-web/store/notifications/notification is being used consistently across all files that import the INotification interface. All related files have been updated to use the new path structure, and there are no remaining instances of the old import path. The change aligns with the broader pattern of plane-web/store imports throughout the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the new import path exists and check for any missed imports
# that might still be using the old path
# Check if the new path exists
fd "notification.ts" | grep "plane-web/store/notifications"
# Look for any remaining old import paths that might have been missed
rg "from ['\"]@/store/notifications/notification['\"]"
Length of output: 118
Script:
#!/bin/bash
# Let's try a broader search to understand the notification-related files and imports
# Find all notification.ts files
echo "=== All notification.ts files ==="
fd "notification.ts"
# Search for any imports containing notifications/notification
echo -e "\n=== All imports with notifications/notification ==="
rg "from.*notifications/notification['\"]" --type ts
# Search specifically for plane-web store imports
echo -e "\n=== Plane-web store imports ==="
rg "from.*plane-web/store" --type ts
Length of output: 3838
web/ce/store/root.store.ts (2)
4-4: Verify the new import path exists after file rearrangement.
Let's ensure the new notifications file structure is properly set up.
✅ Verification successful
Import path is correctly structured and implemented
The file web/ce/store/notifications/workspace-notifications.store.ts exists and properly exports both the WorkspaceNotificationStore class and IWorkspaceNotificationStore interface, which matches the import statement in root.store.ts. The implementation is complete and follows the expected store pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and structure of notification-related files
# Check if the workspace notifications store file exists
fd "workspace-notifications.store.ts" web/ce/store
# Verify the export of WorkspaceNotificationStore
rg -t typescript "export.*WorkspaceNotificationStore" web/ce/store
Length of output: 214
Script:
#!/bin/bash
# Check the content of the workspace notifications store file
cat web/ce/store/notifications/workspace-notifications.store.ts
Length of output: 14145
8-14: Consider documenting initialization order dependencies.
The workspaceNotification store is initialized before timelineStore. If there are any dependencies between these stores, it would be helpful to document them.
web/ce/components/workspace-notifications/sidebar/notification-card/options/read.tsx (1)
Line range hint 1-54: LGTM! Well-structured component implementation
The component follows React best practices with:
- Proper TypeScript types
- Error handling
- Event tracking
- Toast notifications for user feedback
- Clear separation of concerns
web/ce/components/workspace-notifications/sidebar/notification-card/options/archive.tsx (1)
Line range hint 20-53: Implementation looks good!
The component is well-structured with:
- Proper TypeScript typing
- Clean separation of concerns
- Correct implementation of MobX observer pattern
- Clear and consistent UI feedback
web/ce/components/workspace-notifications/sidebar/root.tsx (2)
1-22: LGTM! Well-organized imports with clear grouping.
The imports are properly organized with descriptive comments separating different types of imports (components, constants, helpers, hooks).
86-102: LGTM! Well-structured conditional rendering.
The loader and empty state handling is well-implemented with appropriate UI feedback for different states.
web/ce/store/notifications/notification.ts (1)
5-6: Verify store migration consistency across the codebase
The change from CoreRootStore to RootStore suggests a structural change in the store architecture. Let's verify this migration is consistent across the codebase.
✅ Verification successful
Migration from CoreRootStore to RootStore appears to be incomplete
The codebase analysis reveals an architectural pattern where RootStore extends CoreRootStore, but the migration is inconsistent:
web/ce/store/notifications/notification.tsimportsRootStorefrom "@/plane-web/store/root.store"- Many files still use
CoreRootStoredirectly - Some files have started using
RootStorefrom "@/plane-web/store/root.store"
Let's verify if this is the intended pattern:
The import change in notification.ts is correct and follows the intended pattern
The analysis reveals a deliberate architectural pattern where:
RootStoreextendsCoreRootStore(as seen in web/ce/store/root.store.ts)- New files in the web/ce directory correctly use
RootStorefrom "@/plane-web/store/root.store" - The timeline and notification stores in web/ce consistently use this pattern
The change in web/ce/store/notifications/notification.ts aligns with this architecture, where CE-specific stores use the extended RootStore while core stores continue to use CoreRootStore. This is a correct implementation of inheritance hierarchy.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining CoreRootStore references and verify RootStore usage
# Check for any remaining CoreRootStore references
echo "Checking for remaining CoreRootStore references..."
rg "CoreRootStore"
# Check RootStore import pattern consistency
echo "Checking RootStore import patterns..."
rg "import.*RootStore.*from.*store/root\.store"
Length of output: 13275
Script:
#!/bin/bash
# Check the inheritance structure of RootStore
echo "Checking RootStore implementation..."
ast-grep --pattern 'class RootStore extends $_'
# Check if there are any direct RootStore constructor parameters
echo "Checking RootStore constructor usage..."
rg "constructor.*\(.*RootStore.*\)"
Length of output: 6987
web/ce/store/notifications/workspace-notifications.store.ts (2)
Line range hint 88-379: Implementation follows best practices
The implementation:
- Properly uses MobX patterns (observables, actions, computed)
- Has consistent error handling
- Is well-structured and maintainable
- Includes comprehensive documentation
87-87:
Verify constructor parameter type change impact
The constructor parameter type has changed from CoreRootStore to RootStore. This is a breaking change that requires verification of all instantiations of WorkspaceNotificationStore.
web/ce/store/root.store.ts
Outdated
| resetOnSignOut() { | ||
| this.workspaceNotification = new WorkspaceNotificationStore(this); | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Ensure resetOnSignOut behavior aligns with parent class implementation
The parent class CoreRootStore implements a comprehensive reset of all stores during sign-out, but the child class only resets the workspaceNotification store. This could lead to state inconsistencies. You should either:
- Reset all stores by calling the parent's
resetOnSignOutusingsuper.resetOnSignOut() - Or document why only
workspaceNotificationneeds to be reset in this context
🔗 Analysis chain
Review sign-out reset behavior across all stores.
The resetOnSignOut method only resets the workspaceNotification store. Consider whether other stores (like timelineStore) should also be reset during sign-out to maintain state consistency.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check sign-out behavior in related stores
# Look for resetOnSignOut implementations in other stores
rg -t typescript "resetOnSignOut|reset.*sign.*out" web/ce/store
# Check parent class sign-out handling
ast-grep --pattern 'class CoreRootStore {
$$$
resetOnSignOut() {
$$$
}
$$$
}'
Length of output: 6191
| // components | ||
| import { NotificationItemOptionButton } from "@/plane-web/components/workspace-notifications"; |
There was a problem hiding this comment.
💡 Codebase verification
Import paths need to be updated for consistency
Several files are still using the old import path @/components/workspace-notifications instead of the new @/plane-web/components/workspace-notifications:
web/core/components/workspace/sidebar/user-menu.tsxweb/core/components/workspace-notifications/sidebar/header/root.tsxweb/core/components/workspace-notifications/sidebar/header/options/root.tsxweb/core/components/workspace-notifications/sidebar/header/options/menu-option/root.tsxweb/core/components/workspace-notifications/sidebar/header/options/menu-option/menu-item.tsxweb/core/components/workspace-notifications/sidebar/filters/menu/root.tsxweb/ce/components/workspace-notifications/sidebar/root.tsxweb/app/[workspaceSlug]/(projects)/notifications/layout.tsx
🔗 Analysis chain
Verify consistency of import path changes
The import paths have been updated to use the new @/plane-web/ namespace, which aligns with the file restructuring objective.
Let's verify the consistency of these changes across related files:
Also applies to: 14-14
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that related notification components use consistent import paths
# Check for any remaining old import paths
rg "from \"@/components/workspace-notifications\""
# Verify consistent usage of new import paths
rg "from \"@/plane-web/components/workspace-notifications\""
# Check for any inconsistencies in notification store imports
rg "from \"@/plane-web/store/notifications/notification\""
Length of output: 3669
There was a problem hiding this comment.
Other imported components are still in core not moved.
There was a problem hiding this comment.
@mathalav55, understood. Thanks for the clarification that the other imported components are still in core and have not been moved yet.
（・・）?
✏️ Learnings added
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.337Z
Learning: Some components are still in core and have not been moved yet, so their import paths remain the same.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| // components | ||
| import { NotificationItemOptionButton } from "@/plane-web/components/workspace-notifications"; | ||
| // store | ||
| import { INotification } from "@/store/notifications/notification"; | ||
| import { INotification } from "@/plane-web/store/notifications/notification"; |
There was a problem hiding this comment.
💡 Codebase verification
Migration to new import paths is incomplete
The codebase shows inconsistent usage of import paths for notification-related components:
-
Several files still use the old path
@/components/workspace-notifications:web/core/components/workspace/sidebar/user-menu.tsxweb/core/components/workspace-notifications/sidebar/header/*web/core/components/workspace-notifications/sidebar/filters/*web/app/[workspaceSlug]/(projects)/notifications/layout.tsx
-
The new path
@/plane-web/components/workspace-notificationsis being used in other files, primarily in the CE components. -
The
INotificationinterface imports are consistently using the new path@/plane-web/store/notifications/notification, which is correct.
🔗 Analysis chain
Verify consistent usage of new import paths across the codebase
The import paths have been updated to use the new @/plane-web prefix. Let's ensure this change is consistent across all notification-related components.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent usage of new import paths
# Check for any remaining old import paths
echo "Checking for old import paths..."
rg -l "@/components/workspace-notifications" --type typescript
# Check for consistent usage of new import paths
echo "Checking new import path usage..."
rg -l "@/plane-web/components/workspace-notifications" --type typescript
# Check for any mixed usage of INotification imports
echo "Checking INotification import consistency..."
rg -l "import.*INotification.*from.*@/store/notifications/notification" --type typescript
rg -l "import.*INotification.*from.*@/plane-web/store/notifications/notification" --type typescript
Length of output: 720
Script:
#!/bin/bash
# Description: Verify consistent usage of new import paths (corrected file type)
# Check for any remaining old import paths
echo "Checking for old import paths..."
rg -l "@/components/workspace-notifications" -t ts -t tsx
# Check for consistent usage of new import paths
echo "Checking new import path usage..."
rg -l "@/plane-web/components/workspace-notifications" -t ts -t tsx
# Check for any mixed usage of INotification imports
echo "Checking INotification import consistency..."
rg -l "import.*INotification.*from.*@/store/notifications/notification" -t ts -t tsx
rg -l "import.*INotification.*from.*@/plane-web/store/notifications/notification" -t ts -t tsx
Length of output: 672
Script:
#!/bin/bash
# Description: Verify consistent usage of new import paths (without file type flags)
# Check for any remaining old import paths
echo "Checking for old import paths..."
rg "@/components/workspace-notifications"
# Check for consistent usage of new import paths
echo "Checking new import path usage..."
rg "@/plane-web/components/workspace-notifications"
# Check for any mixed usage of INotification imports
echo "Checking INotification import consistency..."
rg "import.*INotification.*from.*@/store/notifications/notification"
rg "import.*INotification.*from.*@/plane-web/store/notifications/notification"
Length of output: 3995
There was a problem hiding this comment.
Same here. Imported components are still in core.
There was a problem hiding this comment.
@mathalav55, thank you for the clarification. Since the imported components are still in core, it's appropriate for these files to use the old import paths. Apologies for the confusion.
✏️ Learnings added
Learnt from: mathalav55
PR: makeplane/plane#6107
File: web/ce/components/workspace-notifications/sidebar/notification-card/options/archive.tsx:11-14
Timestamp: 2024-11-28T07:02:54.415Z
Learning: When components are still located in `core`, it's appropriate for files to import them using `@/components/...`, and the migration to the new import paths is not necessary in such cases.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| // components | ||
| import { NotificationItem } from "@/plane-web/components/workspace-notifications"; |
There was a problem hiding this comment.
💡 Codebase verification
Import path inconsistency detected across the codebase
The verification reveals mixed usage of both old and new import paths:
- Several files still use the old path
@/components/workspace-notifications - Multiple files have been updated to use the new path
@/plane-web/components/workspace-notifications - The target import path
web/plane-web/componentsdoesn't exist in the repository
Files still using old import path:
web/app/[workspaceSlug]/(projects)/notifications/layout.tsx- Multiple files under
web/core/components/workspace-notifications/ web/core/components/workspace/sidebar/user-menu.tsx
🔗 Analysis chain
Verify the new import path structure
The import path change from @/components/workspace-notifications to @/plane-web/components/workspace-notifications aligns with the file rearrangement objective.
Let's verify the new import path structure:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the new import path exists and check for consistency
# Check if the new path exists
fd "workspace-notifications" "web/plane-web/components"
# Check for any remaining old imports that need updating
rg -l "@/components/workspace-notifications" "web"
# Check for mixed usage of old and new paths
rg -l "@/plane-web/components/workspace-notifications" "web"
Length of output: 1554
| {NOTIFICATION_TABS.map((tab) => ( | ||
| <div | ||
| key={tab.value} | ||
| className="h-full px-3 relative cursor-pointer" | ||
| onClick={() => currentNotificationTab != tab.value && setCurrentNotificationTab(tab.value)} | ||
| > | ||
| <div | ||
| className={cn( | ||
| `relative h-full flex justify-center items-center gap-1 text-sm transition-all`, | ||
| currentNotificationTab === tab.value | ||
| ? "text-custom-primary-100" | ||
| : "text-custom-text-100 hover:text-custom-text-200" | ||
| )} | ||
| > | ||
| <div className="font-medium">{tab.label}</div> | ||
| {tab.count(unreadNotificationsCount) > 0 && ( | ||
| <CountChip count={getNumberCount(tab.count(unreadNotificationsCount))} /> | ||
| )} | ||
| </div> | ||
| {currentNotificationTab === tab.value && ( | ||
| <div className="border absolute bottom-0 right-0 left-0 rounded-t-md border-custom-primary-100" /> | ||
| )} | ||
| </div> | ||
| ))} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize tab click handler to prevent unnecessary rerenders.
The click handler function is recreated on each render. Consider memoizing it or moving it outside the map function.
+ const handleTabClick = useCallback((tabValue: string) => {
+ if (currentNotificationTab !== tabValue) {
+ setCurrentNotificationTab(tabValue);
+ }
+ }, [currentNotificationTab, setCurrentNotificationTab]);
{NOTIFICATION_TABS.map((tab) => (
<div
key={tab.value}
className="h-full px-3 relative cursor-pointer"
- onClick={() => currentNotificationTab != tab.value && setCurrentNotificationTab(tab.value)}
+ onClick={() => handleTabClick(tab.value)}
>Committable suggestion skipped: line range outside the PR's diff.
| // hooks | ||
| import { useWorkspace, useWorkspaceNotifications } from "@/hooks/store"; | ||
|
|
||
| import { NotificationCardListRoot } from "@/plane-web/components/workspace-notifications"; |
There was a problem hiding this comment.
💡 Codebase verification
Import path appears to be incorrect
The import path @/plane-web/components/workspace-notifications seems incorrect as:
- The component is exported from:
web/ce/components/workspace-notifications/sidebar/notification-card/root.tsxweb/ee/components/workspace-notifications/sidebar/notification-card/root.tsx
- The attempted import path
web/plane-web/components/does not exist in the codebase
The import statement should be updated to use the correct path based on the CE/EE component location.
🔗 Analysis chain
Verify the integration with NotificationCardListRoot component.
Since this PR involves file rearrangement, ensure that the import path for NotificationCardListRoot is correct and the component is properly exported from its new location.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the NotificationCardListRoot component's export and usage
# Check if the component is properly exported
rg -g '*.{ts,tsx}' -l 'export.*NotificationCardListRoot'
# Check for any other imports of this component to ensure they're updated
rg -g '*.{ts,tsx}' 'import.*NotificationCardListRoot'
Length of output: 418
Script:
#!/bin/bash
# Let's verify the actual export in plane-web components
rg -g '*.{ts,tsx}' -A 5 'export.*NotificationCardListRoot' 'web/plane-web/components/'
# Also check the barrel exports in workspace-notifications
rg -g '*.{ts,tsx}' 'export.*NotificationCardListRoot' 'web/plane-web/components/workspace-notifications/'
Length of output: 348
| // store | ||
| import { Notification, INotification } from "@/plane-web/store/notifications/notification"; | ||
| // services | ||
| import workspaceNotificationService from "@/services/workspace-notification.service"; | ||
| // store | ||
| import { Notification, INotification } from "@/store/notifications/notification"; | ||
| import { CoreRootStore } from "@/store/root.store"; | ||
| import { RootStore } from "../root.store"; |
There was a problem hiding this comment.
💡 Codebase verification
Import paths need to be updated for consistency
Based on the search results, there are inconsistencies in the import patterns that need to be addressed:
- Multiple files still use the old import path
@/store/root.storefor CoreRootStore - The new
@/plane-web/store/notifications/notificationimport is used consistently in the notification-related components - The relative import
from "../root.store"for RootStore is used in several files, but there's inconsistency with some files using@/plane-web/store/root.store
Please update the following:
- Standardize the RootStore imports to use
@/plane-web/store/root.storeinstead of relative paths - Update remaining old imports of
@/store/root.storeto use the new path structure
🔗 Analysis chain
Verify import path consistency across the codebase
The import paths have been restructured:
- RootStore now uses a relative path
- Notification uses an aliased path from plane-web
Let's verify these changes are consistent across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any inconsistent import patterns
# that might have been missed during the restructuring
# Check for any remaining old import patterns for RootStore
rg --type typescript "from ['\"]@/store/root\.store['\"]"
# Check for consistency in Notification imports
rg --type typescript "from ['\"]@/plane-web/store/notifications/notification['\"]"
# Look for potential circular dependencies with the new relative import
ast-grep --pattern 'from "../root.store"'
Length of output: 266
Script:
#!/bin/bash
# Let's try different search patterns without type filters
# Check for any remaining old import patterns for RootStore
rg "from ['\"]@/store/root\.store['\"]"
# Check for consistency in Notification imports
rg "from ['\"]@/plane-web/store/notifications/notification['\"]"
# Look for imports of root.store to analyze the pattern
rg "from ['\"].*root\.store['\"]"
# Search for files that might contain notification imports
fd "notification" --type f
Length of output: 13236
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
web/ce/components/workspace-notifications/sidebar/root.tsx (1)
62-87: Consider extracting tab navigation into a separate componentThe tab navigation logic (lines 62-87) could be extracted into a separate component for better maintainability and reusability. This would:
- Reduce the complexity of the main component
- Make the code more maintainable
- Enable reuse of the tab navigation elsewhere if needed
Example structure:
interface NotificationTabsProps { currentTab: TNotificationTab; unreadCount: number; onTabChange: (tab: TNotificationTab) => void; } const NotificationTabs: FC<NotificationTabsProps> = ({ currentTab, unreadCount, onTabChange, }) => { return ( <Header variant={EHeaderVariant.SECONDARY} className="justify-start"> {NOTIFICATION_TABS.map((tab) => ( // ... existing tab rendering logic ))} </Header> ); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
web/ce/components/workspace-notifications/sidebar/root.tsx(1 hunks)web/ce/store/notifications/workspace-notifications.store.ts(2 hunks)web/ce/store/root.store.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- web/ce/store/notifications/workspace-notifications.store.ts
- web/ce/store/root.store.ts
🔇 Additional comments (2)
web/ce/components/workspace-notifications/sidebar/root.tsx (2)
23-23: Update import path for NotificationCardListRoot
The import path @/plane-web/components/workspace-notifications appears to be incorrect. The component should be imported from either:
web/ce/components/workspace-notifications/sidebar/notification-card/root.tsxweb/ee/components/workspace-notifications/sidebar/notification-card/root.tsx
25-40: LGTM: Well-structured component setup
The component follows React best practices with proper type annotations, state management, and undefined case handling.
| const handleTabClick = useCallback((tabValue: TNotificationTab)=>{ | ||
| if(currentSelectedNotificationId !== tabValue) { | ||
| setCurrentNotificationTab(tabValue); | ||
| } | ||
| },[ setCurrentNotificationTab, currentSelectedNotificationId ]); |
There was a problem hiding this comment.
Fix incorrect comparison in handleTabClick
The condition is comparing currentSelectedNotificationId with tabValue, but these appear to be different types:
currentSelectedNotificationId: likely a notification IDtabValue: a tab value of typeTNotificationTab
The correct comparison should be with currentNotificationTab.
- if(currentSelectedNotificationId !== tabValue) {
+ if(currentNotificationTab !== tabValue) {
setCurrentNotificationTab(tabValue);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleTabClick = useCallback((tabValue: TNotificationTab)=>{ | |
| if(currentSelectedNotificationId !== tabValue) { | |
| setCurrentNotificationTab(tabValue); | |
| } | |
| },[ setCurrentNotificationTab, currentSelectedNotificationId ]); | |
| const handleTabClick = useCallback((tabValue: TNotificationTab)=>{ | |
| if(currentNotificationTab !== tabValue) { | |
| setCurrentNotificationTab(tabValue); | |
| } | |
| },[ setCurrentNotificationTab, currentSelectedNotificationId ]); |
pushya22
changed the title
| @@ -7,4 +7,4 @@ export * from "./header"; | |||
|
|
|||
| export * from "./filters"; | |||
|
|
|||
| export * from "./notification-card"; | |||
| // export * from "./notification-card"; | |||
There was a problem hiding this comment.
@mathalav55 please check
| ); | ||
| }); | ||
| import { NotificationsSidebar } from "@/plane-web/components/workspace-notifications"; | ||
| export const NotificationsSidebarRoot = () => <NotificationsSidebar/> |
There was a problem hiding this comment.
This Change feel unnecessary. Please check
|
Closing this PR. raised another PR with minimal component re structuring. |
Summary
Rearranged files for notifications
Reference
WEB-2382
Summary by CodeRabbit
Release Notes
New Features
NotificationsSidebarcomponent for improved notification management.NotificationsSidebarRootas a wrapper for theNotificationsSidebar.Enhancements
NotificationItem,NotificationCardListRoot, and various notification options.Bug Fixes
These updates enhance the organization and accessibility of notification components, streamlining user interaction with notifications.