[WEB-4094]chore: workspace notifications refactor#7061
Merged
sriramveeraghanta merged 5 commits intopreviewfrom Jun 9, 2025
Merged
[WEB-4094]chore: workspace notifications refactor#7061sriramveeraghanta merged 5 commits intopreviewfrom
sriramveeraghanta merged 5 commits intopreviewfrom
Conversation
Contributor
## Walkthrough
This update introduces new types and components for managing notification previews and workspace notification sidebars. It refactors notification-related components to use encapsulated roots, adds MobX-based sidebar management, and standardizes issue subscription logic with explicit service type handling. Several exports are updated to expose new modules and types.
## Changes
| File(s) | Change Summary |
|--------------------------------------------------------------------------------------------|---------------|
| `packages/types/src/issues/issue.d.ts` | Added `IWorkItemPeekOverview` interface and updated imports to include `EIssuesStoreType`. |
| `web/app/[workspaceSlug]/(projects)/notifications/page.tsx` | Simplified the notification page to render only the `NotificationsRoot` component, removing all internal logic and state management. |
| `web/ce/components/workspace-notifications/index.ts` | Added export for `./list-root`. |
| `web/ce/components/workspace-notifications/list-root.tsx` | Introduced `NotificationListRoot` component and `TNotificationListRoot` type. |
| `web/ce/hooks/use-notification-preview.tsx` | Added `useNotificationPreview` hook and `TNotificationPreview` type for managing notification previews. |
| `web/core/components/issues/issue-detail/subscription.tsx` | Enhanced `IssueSubscription` to accept an optional `serviceType` prop and improved button disabled logic. |
| `web/core/components/issues/peek-overview/root.tsx` | Changed component props from local `IIssuePeekOverview` to imported `IWorkItemPeekOverview`. |
| `web/core/components/workspace-notifications/root.tsx` | Renamed and refactored to `NotificationsRoot`, streamlined logic, and updated props to accept optional `workspaceSlug`. |
| `web/core/components/workspace-notifications/sidebar/index.ts` | Added export for `./root`. |
| `web/core/components/workspace-notifications/sidebar/root.tsx` | Added new MobX-based `NotificationsSidebarRoot` component for workspace notifications. |
| `web/core/store/issue/issue-details/root.store.ts` | Updated `IssueDetail` constructor to pass `serviceType` to `IssueSubscriptionStore`. |
| `web/core/store/issue/issue-details/subscription.store.ts` | Modified `IssueSubscriptionStore` constructor to accept and use `serviceType`. |
| `web/core/components/workspace-notifications/sidebar/header/root.tsx` | Added `bg-custom-background-100` CSS class to header component. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant User
participant NotificationsSidebarRoot
participant NotificationListRoot
participant NotificationsRoot
participant useNotificationPreview
participant IssuePeekOverview
User->>NotificationsSidebarRoot: Navigates to workspace notifications
NotificationsSidebarRoot->>NotificationListRoot: Renders notification list
User->>NotificationListRoot: Selects a notification
NotificationListRoot->>NotificationsRoot: Notification selected
NotificationsRoot->>useNotificationPreview: Determines preview type
useNotificationPreview->>IssuePeekOverview: Provides preview component
NotificationsRoot->>IssuePeekOverview: Renders notification previewSuggested reviewers
Poem
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
web/ce/hooks/use-notification-preview.tsx (1)
18-25: Return value is recreated on each render – wrap withuseMemoEvery invocation of the hook recreates a brand-new object, needlessly invalidating all React memoisation further down the tree.
Wrapping the return value inuseMemo(keyed bypeekIssue) avoids avoidable re-renders, especially becausePeekOverviewComponentis stable.-import { useIssueDetail } from "@/hooks/store"; +import { useIssueDetail } from "@/hooks/store"; +import { useMemo } from "react"; @@ - const { peekIssue, setPeekIssue } = useIssueDetail(EIssueServiceType.ISSUES); - - return { - isWorkItem: Boolean(peekIssue), - PeekOverviewComponent: IssuePeekOverview, - setPeekWorkItem: setPeekIssue, - }; + const { peekIssue, setPeekIssue } = useIssueDetail(EIssueServiceType.ISSUES); + + return useMemo( + () => ({ + isWorkItem: Boolean(peekIssue), + PeekOverviewComponent: IssuePeekOverview, + setPeekWorkItem: setPeekIssue, + }), + [peekIssue, setPeekIssue], + );web/core/components/workspace-notifications/root.tsx (2)
76-81: Effect dependency list contains an unused entry
setCurrentSelectedNotificationIdis listed but never referenced inside the effect – this forces the cleanup to be re-registered whenever the setter identity changes (rare, but unnecessary).-useEffect( - () => () => { - setPeekWorkItem(undefined); - }, - [setCurrentSelectedNotificationId, setPeekWorkItem], -); +useEffect(() => () => setPeekWorkItem(undefined), [setPeekWorkItem]);
83-114: Minor: duplicated fragment wrapperLines 90–113 already live inside a fragment return; the extra
<>…</>is redundant and adds an extra node to the DOM.Removing it simplifies the markup:
- ) : ( - <> - {is_inbox_issue === true && ...} - </> - )} + ) : ( + is_inbox_issue === true && ... /* existing logic */ + )}web/core/components/workspace-notifications/sidebar/root.tsx (2)
53-53: Consider adding a more user-friendly empty state.Instead of returning an empty fragment when workspace data is missing, consider showing a helpful error message or a loading indicator to improve user experience.
- if (!workspaceSlug || !workspace) return <></>; + if (!workspaceSlug || !workspace) return ( + <div className="flex items-center justify-center h-full w-full"> + <p className="text-custom-text-400">Workspace not found or inaccessible</p> + </div> + );
104-107: Consider handling empty arrays with optional chaining.The conditional check for
notificationIds && notificationIds.length > 0can be made more concise with optional chaining.- {notificationIds && notificationIds.length > 0 ? ( + {notificationIds?.length > 0 ? ( <ContentWrapper variant={ERowVariant.HUGGING}> <NotificationListRoot workspaceSlug={workspaceSlug.toString()} workspaceId={workspace?.id} /> </ContentWrapper>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
packages/types/src/issues/issue.d.ts(2 hunks)web/app/[workspaceSlug]/(projects)/notifications/page.tsx(1 hunks)web/ce/components/workspace-notifications/index.ts(1 hunks)web/ce/components/workspace-notifications/list-root.tsx(1 hunks)web/ce/hooks/use-notification-preview.tsx(1 hunks)web/core/components/issues/issue-detail/subscription.tsx(4 hunks)web/core/components/issues/peek-overview/root.tsx(2 hunks)web/core/components/workspace-notifications/root.tsx(1 hunks)web/core/components/workspace-notifications/sidebar/index.ts(1 hunks)web/core/components/workspace-notifications/sidebar/root.tsx(1 hunks)web/core/store/issue/issue-details/root.store.ts(1 hunks)web/core/store/issue/issue-details/subscription.store.ts(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
web/ce/components/workspace-notifications/list-root.tsx (1)
web/ce/components/workspace-notifications/notification-card/root.tsx (1)
NotificationCardListRoot(19-59)
web/core/store/issue/issue-details/root.store.ts (1)
web/core/store/issue/issue-details/subscription.store.ts (1)
IssueSubscriptionStore(22-104)
web/core/components/issues/peek-overview/root.tsx (1)
packages/types/src/issues/issue.d.ts (1)
IWorkItemPeekOverview(185-190)
🔇 Additional comments (14)
web/ce/components/workspace-notifications/index.ts (1)
2-2: Appropriate export addition.The new export makes the NotificationListRoot component available for import, which is consistent with the refactoring approach.
web/core/components/workspace-notifications/sidebar/index.ts (1)
9-10: LGTM: Exports the sidebar root component.This addition properly exposes the sidebar root component for use in other parts of the application, which is essential for the notification components refactoring.
web/core/store/issue/issue-details/subscription.store.ts (1)
4-4: Good enhancement to improve type safety.The addition of
serviceTypeparameter to the constructor properly propagates the issue service type down to the underlyingIssueService. This enhances type safety and maintains consistency across the issue subscription system.Also applies to: 30-30, 43-43
web/core/store/issue/issue-details/root.store.ts (1)
207-207: Correctly updated constructor call.The update properly passes the
serviceTypeparameter to theIssueSubscriptionStoreconstructor, matching its updated signature and ensuring consistent propagation of service type information throughout the issue detail stores.web/core/components/issues/peek-overview/root.tsx (2)
17-17: Good refactoring of component's type importsProperly importing the interface from a centralized type definition in
@plane/typesrather than using a local interface improves code reusability and consistency across the codebase.
28-28: Appropriate type replacement for component propsUsing the standardized
IWorkItemPeekOverviewinterface from the shared types package maintains the same functionality while making the component more maintainable as part of the larger notification system.web/ce/components/workspace-notifications/list-root.tsx (1)
1-8: Well-structured component wrapperThis is a clean implementation of a wrapper component that follows good React patterns. The component properly types its props and simply forwards them to the underlying implementation, providing a clear API for the notification list.
packages/types/src/issues/issue.d.ts (2)
1-1: Appropriate import expansionIncluding
EIssuesStoreTypealongside the existing import ensures that the new interface has access to all required types.
185-190: Well-defined shared interfaceCreating a centralized interface for work item peek overview components is good practice. All properties are correctly marked as optional, matching their usage in components.
web/core/components/issues/issue-detail/subscription.tsx (3)
8-8: Enhanced component flexibility with service type parameterAdding the
serviceTypeprop with appropriate default value makes the component more reusable across different contexts (issues, epics, work items) while maintaining backward compatibility.Also applies to: 19-19, 23-23, 30-30
57-58: Improved error handlingThe error handling has been streamlined by removing the unused error parameter, and the translation key has been corrected from "commons.error.message" to "common.error.message".
Also applies to: 62-62
82-82: Better button state managementAdding the loading state to the disabled condition prevents users from triggering multiple concurrent subscription requests, which could lead to unpredictable behavior.
web/core/components/workspace-notifications/sidebar/root.tsx (2)
26-118: LGTM - Well-structured component with good separation of concerns.The NotificationsSidebarRoot component is well-implemented with:
- Proper state management using MobX
- Clean conditional rendering
- Responsive design considerations
- Good use of helper functions and custom hooks
I appreciate the clean organization of UI elements with clear separation between header, tabs, filters, and content areas.
24-24:Details
❌ Incorrect review comment
Double-check import path consistency.
The import path
@/plane-web/components/workspace-notifications/list-rootuses a different naming pattern than other imports. Ensure this is consistent with your project's import conventions.
🏁 Script executed:
#!/bin/bash # Check for import path patterns in the project echo "Checking import path patterns for workspace notifications components..." grep -r --include="*.tsx" --include="*.ts" "@/plane-web/components" . echo "Comparing with other import patterns..." grep -r --include="*.tsx" --include="*.ts" "@/components" . | head -n 10Length of output: 23824
Import path is consistent with existing
@/plane-web/componentsusages—no change needed.The
@/plane-web/components/workspace-notifications/list-rootimport follows the same pattern used throughout theplane-webcodebase. Please disregard the earlier consistency flag.Likely an incorrect or invalid review comment.
vamsikrishnamathala
changed the title
|
Pull Request Linked with Plane Work Items
Comment Automatically Generated by Plane |
anmolsinghbhatia
approved these changes
Jun 6, 2025
…pace_notifications_refactor
sriramveeraghanta
approved these changes
Jun 9, 2025
This was referenced Jul 21, 2025
Merged
This was referenced Jul 29, 2025
lifeiscontent
pushed a commit
that referenced
this pull request
Aug 18, 2025
* chore: workspace notifications refactor * fix: url params * fix: added null checks to avoid run time errors * fix: notifications header color fix
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Refactored code for workspace notification cards.
Type of Change
Screenshots and Media (if applicable)
Test Scenarios
References
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Documentation