-
Notifications
You must be signed in to change notification settings - Fork 3.5k
[WEB-2382] chore: notifications file rearranging #6107
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| export * from './sidebar' |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| export * from './notification-card' | ||
| export * from './root' |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,14 +4,14 @@ import { FC } from "react"; | |
| import { observer } from "mobx-react"; | ||
| import { ArchiveRestore } from "lucide-react"; | ||
| import { ArchiveIcon, TOAST_TYPE, setToast } from "@plane/ui"; | ||
| // components | ||
| import { NotificationItemOptionButton } from "@/components/workspace-notifications"; | ||
| // constants | ||
| import { NOTIFICATION_ARCHIVED } from "@/constants/event-tracker"; | ||
| // hooks | ||
| import { useEventTracker, useWorkspaceNotifications } from "@/hooks/store"; | ||
| // components | ||
| import { NotificationItemOptionButton } from "@/plane-web/components/workspace-notifications"; | ||
| // store | ||
| import { INotification } from "@/store/notifications/notification"; | ||
| import { INotification } from "@/plane-web/store/notifications/notification"; | ||
|
Comment on lines
+11
to
+14
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Codebase verification Migration to new import paths is incomplete The codebase shows inconsistent usage of import paths for notification-related components:
🔗 Analysis chainVerify consistent usage of new import paths across the codebase The import paths have been updated to use the new 🏁 Scripts executedThe 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
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here. Imported components are still in core.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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
|
||
|
|
||
| type TNotificationItemArchiveOption = { | ||
| workspaceSlug: string; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,14 +4,14 @@ import { FC } from "react"; | |
| import { observer } from "mobx-react"; | ||
| import { MessageSquare } from "lucide-react"; | ||
| import { TOAST_TYPE, setToast } from "@plane/ui"; | ||
| // components | ||
| import { NotificationItemOptionButton } from "@/components/workspace-notifications"; | ||
| // constants | ||
| import { NOTIFICATIONS_READ } from "@/constants/event-tracker"; | ||
| // hooks | ||
| import { useEventTracker, useWorkspaceNotifications } from "@/hooks/store"; | ||
| // components | ||
| import { NotificationItemOptionButton } from "@/plane-web/components/workspace-notifications"; | ||
|
Comment on lines
+11
to
+12
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Codebase verification Import paths need to be updated for consistency Several files are still using the old import path
🔗 Analysis chainVerify consistency of import path changes The import paths have been updated to use the new Let's verify the consistency of these changes across related files: Also applies to: 14-14 🏁 Scripts executedThe 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
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Other imported components are still in core not moved.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mathalav55, understood. Thanks for the clarification that the other imported components are still in (・・)? ✏️ Learnings added
|
||
| // store | ||
| import { INotification } from "@/store/notifications/notification"; | ||
| import { INotification } from "@/plane-web/store/notifications/notification"; | ||
|
|
||
| type TNotificationItemReadOption = { | ||
| workspaceSlug: string; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,12 +2,12 @@ | |
|
|
||
| import { FC } from "react"; | ||
| import { observer } from "mobx-react"; | ||
| // components | ||
| import { NotificationItem } from "@/components/workspace-notifications"; | ||
| // constants | ||
| import { ENotificationLoader, ENotificationQueryParamType } from "@/constants/notification"; | ||
| // hooks | ||
| import { useWorkspaceNotifications } from "@/hooks/store"; | ||
| // components | ||
| import { NotificationItem } from "@/plane-web/components/workspace-notifications"; | ||
|
Comment on lines
+9
to
+10
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Codebase verification Import path inconsistency detected across the codebase The verification reveals mixed usage of both old and new import paths:
Files still using old import path:
🔗 Analysis chainVerify the new import path structure The import path change from Let's verify the new import path structure: 🏁 Scripts executedThe 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 |
||
|
|
||
| type TNotificationCardListRoot = { | ||
| workspaceSlug: string; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,106 @@ | ||
| "use client"; | ||
|
|
||
| import { FC } from "react"; | ||
| import { observer } from "mobx-react"; | ||
| import { useParams } from "next/navigation"; | ||
| // components | ||
| import { Header, Row, ERowVariant, EHeaderVariant, ContentWrapper } from "@plane/ui"; | ||
| import { CountChip } from "@/components/common"; | ||
| import { | ||
| NotificationsLoader, | ||
| NotificationEmptyState, | ||
| NotificationSidebarHeader, | ||
| AppliedFilters, | ||
| } from "@/components/workspace-notifications"; | ||
| // constants | ||
| import { NOTIFICATION_TABS } from "@/constants/notification"; | ||
| // helpers | ||
| import { cn } from "@/helpers/common.helper"; | ||
| import { getNumberCount } from "@/helpers/string.helper"; | ||
| // hooks | ||
| import { useWorkspace, useWorkspaceNotifications } from "@/hooks/store"; | ||
|
|
||
| import { NotificationCardListRoot } from "@/plane-web/components/workspace-notifications"; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Codebase verification Import path appears to be incorrect The import path
The import statement should be updated to use the correct path based on the CE/EE component location. 🔗 Analysis chainVerify 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 executedThe 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 |
||
|
|
||
| export const NotificationsSidebar: FC = observer(() => { | ||
| const { workspaceSlug } = useParams(); | ||
| // hooks | ||
| const { getWorkspaceBySlug } = useWorkspace(); | ||
| const { | ||
| currentSelectedNotificationId, | ||
| unreadNotificationsCount, | ||
| loader, | ||
| notificationIdsByWorkspaceId, | ||
| currentNotificationTab, | ||
| setCurrentNotificationTab, | ||
| } = useWorkspaceNotifications(); | ||
| // derived values | ||
| const workspace = workspaceSlug ? getWorkspaceBySlug(workspaceSlug.toString()) : undefined; | ||
| const notificationIds = workspace ? notificationIdsByWorkspaceId(workspace.id) : undefined; | ||
|
|
||
| if (!workspaceSlug || !workspace) return <></>; | ||
|
|
||
| return ( | ||
| <div | ||
| className={cn( | ||
| "relative border-0 md:border-r border-custom-border-200 z-[10] flex-shrink-0 bg-custom-background-100 h-full transition-all overflow-hidden", | ||
| currentSelectedNotificationId ? "w-0 md:w-2/6" : "w-full md:w-2/6" | ||
| )} | ||
| > | ||
| <div className="relative w-full h-full overflow-hidden flex flex-col"> | ||
| <Row className="h-[3.75rem] border-b border-custom-border-200 flex"> | ||
| <NotificationSidebarHeader workspaceSlug={workspaceSlug.toString()} /> | ||
| </Row> | ||
|
|
||
| <Header variant={EHeaderVariant.SECONDARY} className="justify-start"> | ||
| {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> | ||
| ))} | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ 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)}
>
|
||
| </Header> | ||
|
|
||
| {/* applied filters */} | ||
| <AppliedFilters workspaceSlug={workspaceSlug.toString()} /> | ||
|
|
||
| {/* rendering notifications */} | ||
| {loader === "init-loader" ? ( | ||
| <div className="relative w-full h-full overflow-hidden"> | ||
| <NotificationsLoader /> | ||
| </div> | ||
| ) : ( | ||
| <> | ||
| {notificationIds && notificationIds.length > 0 ? ( | ||
| <ContentWrapper variant={ERowVariant.HUGGING}> | ||
| <NotificationCardListRoot workspaceSlug={workspaceSlug.toString()} workspaceId={workspace?.id} /> | ||
| </ContentWrapper> | ||
| ) : ( | ||
| <div className="relative w-full h-full flex justify-center items-center"> | ||
| <NotificationEmptyState /> | ||
| </div> | ||
| )} | ||
| </> | ||
| )} | ||
| </div> | ||
| </div> | ||
| ); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,11 +21,11 @@ import { | |
| } from "@/constants/notification"; | ||
| // helpers | ||
| import { convertToEpoch } from "@/helpers/date-time.helper"; | ||
| // 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"; | ||
|
||
|
|
||
| type TNotificationLoader = ENotificationLoader | undefined; | ||
| type TNotificationQueryParamType = ENotificationQueryParamType; | ||
|
|
@@ -84,7 +84,7 @@ export class WorkspaceNotificationStore implements IWorkspaceNotificationStore { | |
| read: false, | ||
| }; | ||
|
|
||
| constructor(private store: CoreRootStore) { | ||
| constructor(private store: RootStore) { | ||
| makeObservable(this, { | ||
| // observables | ||
| loader: observable.ref, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,13 +1,19 @@ | ||
| // store | ||
| import { CoreRootStore } from "@/store/root.store"; | ||
| import { ITimelineStore, TimeLineStore } from "./timeline"; | ||
| import { IWorkspaceNotificationStore, WorkspaceNotificationStore } from "./notifications/workspace-notifications.store"; | ||
|
|
||
| export class RootStore extends CoreRootStore { | ||
| timelineStore: ITimelineStore; | ||
| workspaceNotification: IWorkspaceNotificationStore; | ||
|
|
||
| constructor() { | ||
| super(); | ||
|
|
||
| this.workspaceNotification = new WorkspaceNotificationStore(this) | ||
| this.timelineStore = new TimeLineStore(this); | ||
| } | ||
|
|
||
| resetOnSignOut() { | ||
| this.workspaceNotification = new WorkspaceNotificationStore(this); | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Codebase verification Ensure resetOnSignOut behavior aligns with parent class implementation The parent class
🔗 Analysis chainReview sign-out reset behavior across all stores. The 🏁 Scripts executedThe 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 |
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,4 +7,4 @@ export * from "./header"; | |
|
|
||
| export * from "./filters"; | ||
|
|
||
| export * from "./notification-card"; | ||
| // export * from "./notification-card"; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mathalav55 please check |
||
Uh oh!
There was an error while loading. Please reload this page.