- Notifications
You must be signed in to change notification settings - Fork3k
[WEB-5574]chore: notification card refactor#8234
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:preview
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
coderabbitaibot commentedDec 4, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
WalkthroughRefactoring introduces an extensible map-based notification content system for core notifications with CE-specific extensions, while extracting base activity filter types to a shared constants package for reuse across components. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for usingCodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Linked to Plane Work Item(s) This comment was auto-generated byPlane |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/constants/src/issue/filter.ts (1)
357-362:Consider addingas constfor precise literal inference.Per the TypeScript 5.0+ coding guidelines, using
consttype parameters provides more precise literal inference.Apply this diff:
-export const BASE_ACTIVITY_FILTER_TYPES = [+export const BASE_ACTIVITY_FILTER_TYPES = [ EActivityFilterType.ACTIVITY, EActivityFilterType.STATE, EActivityFilterType.ASSIGNEE, EActivityFilterType.DEFAULT,-];+] as const;apps/web/core/components/workspace-notifications/sidebar/notification-card/content.tsx (1)
126-131:Type assertion for comment handler is necessary but could be improved.The type assertion is required because the comment handler has a special signature with an optional
renderCommentBoxparameter. While functional, this creates a type safety gap.Consider one of these approaches:
Option 1: Add renderCommentBox to the handler signature:
-export type TNotificationContentHandler = (data: TNotificationFieldData) => TNotificationContentDetails | null;+export type TNotificationContentHandler = (+ data: TNotificationFieldData,+ renderCommentBox?: boolean+) => TNotificationContentDetails | null;Option 2: Create a separate type for special handlers:
exporttypeTNotificationContentHandlerWithOptions=(data:TNotificationFieldData,renderCommentBox?:boolean)=>TNotificationContentDetails|null;exporttypeTNotificationContentMap={[key:string]:TNotificationContentHandler|TNotificationContentHandlerWithOptions;};
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/web/ce/components/workspace-notifications/notification-card/content.ts(1 hunks)apps/web/core/components/issues/issue-detail/issue-activity/activity-comment-root.tsx(1 hunks)apps/web/core/components/workspace-notifications/sidebar/notification-card/content.tsx(2 hunks)packages/constants/src/issue/filter.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,mts,cts}
📄 CodeRabbit inference engine (.github/instructions/typescript.instructions.md)
**/*.{ts,tsx,mts,cts}: Useconsttype parameters for more precise literal inference in TypeScript 5.0+
Use thesatisfiesoperator to validate types without widening them
Leverage inferred type predicates to reduce the need for explicitisreturn types in filter/check functions
UseNoInfer<T>utility to block inference for specific type arguments when they should be determined by other arguments
Utilize narrowing inswitch(true)blocks for control flow analysis (TypeScript 5.3+)
Rely on narrowing from direct boolean comparisons for type guards
Trust preserved narrowing in closures when variables aren't modified after the check (TypeScript 5.4+)
Use constant indices to narrow object/array properties (TypeScript 5.5+)
Use standard ECMAScript decorators (Stage 3) instead of legacyexperimentalDecorators
Useusingdeclarations for explicit resource management with Disposable pattern instead of manual cleanup (TypeScript 5.2+)
Usewith { type: "json" }for import attributes; avoid deprecatedassertsyntax (TypeScript 5.3/5.8+)
Useimport typeexplicitly when importing types to ensure they are erased during compilation, respectingverbatimModuleSyntaxflag
Use.ts,.mts,.ctsextensions inimport typestatements (TypeScript 5.2+)
Useimport type { Type } from "mod" with { "resolution-mode": "import" }for specific module resolution contexts (TypeScript 5.3+)
Use new iterator methods (map, filter, etc.) if targeting modern environments (TypeScript 5.6+)
Utilize newSetmethods likeunion,intersection, etc., when available (TypeScript 5.5+)
UseObject.groupBy/Map.groupBystandard methods for grouping instead of external libraries (TypeScript 5.4+)
UsePromise.withResolvers()for creating promises with exposed resolve/reject functions (TypeScript 5.7+)
Use copying array methods (toSorted,toSpliced,with) for immutable array operations (TypeScript 5.2+)
Avoid accessing instance fields viasuperin classes (TypeScript 5....
Files:
packages/constants/src/issue/filter.tsapps/web/ce/components/workspace-notifications/notification-card/content.tsapps/web/core/components/issues/issue-detail/issue-activity/activity-comment-root.tsxapps/web/core/components/workspace-notifications/sidebar/notification-card/content.tsx
🧠 Learnings (2)
📚 Learning: 2025-10-01T15:30:17.605Z
Learnt from: lifeiscontentRepo: makeplane/plane PR: 7888File: packages/propel/src/avatar/avatar.stories.tsx:2-3Timestamp: 2025-10-01T15:30:17.605ZLearning: In the makeplane/plane repository, avoid suggesting inline type imports (e.g., `import { Avatar, type TAvatarSize }`) due to bundler compatibility issues. Keep type imports and value imports as separate statements.Applied to files:
apps/web/core/components/issues/issue-detail/issue-activity/activity-comment-root.tsx
📚 Learning: 2025-10-09T20:42:31.843Z
Learnt from: lifeiscontentRepo: makeplane/plane PR: 7922File: apps/admin/app/(all)/(dashboard)/ai/form.tsx:19-19Timestamp: 2025-10-09T20:42:31.843ZLearning: In the makeplane/plane repository, React types are globally available through TypeScript configuration. Type annotations like React.FC, React.ReactNode, etc. can be used without explicitly importing the React namespace. The codebase uses the modern JSX transform, so React imports are not required for JSX or type references.Applied to files:
apps/web/core/components/issues/issue-detail/issue-activity/activity-comment-root.tsx
🧬 Code graph analysis (1)
apps/web/ce/components/workspace-notifications/notification-card/content.ts (1)
apps/web/core/components/workspace-notifications/sidebar/notification-card/content.tsx (1)
TNotificationContentMap(36-38)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build and lint web apps
🔇 Additional comments (9)
apps/web/core/components/issues/issue-detail/issue-activity/activity-comment-root.tsx (2)
3-4:LGTM! Correct import structure.The imports correctly separate type-only imports from value imports, following the repository's bundler compatibility requirements.
71-71:Remove the suggestion to eliminate the type assertion — it is necessary.The assertion
as EActivityFilterTypeis required becauseTIssueActivityComment.activity_typeis a discriminated union of string literals ("COMMENT" | "ACTIVITY" | "STATE" | "ASSIGNEE"), whileBASE_ACTIVITY_FILTER_TYPESis typed asEActivityFilterType[]. TypeScript distinguishes between literal string types and enum types, so the assertion bridges this gap. The discriminated union pattern inTIssueActivityCommentis the correct design and should not be changed to use the enum directly.Likely an incorrect or invalid review comment.
apps/web/ce/components/workspace-notifications/notification-card/content.ts (2)
14-18:Unused parameters can be removed or prefixed with underscore.Parameters
_notificationFieldand_oldValueare intentionally unused (marked with underscore prefix). This is acceptable for maintaining a consistent function signature.
20-23:LGTM! Clear field exclusion logic.The connector visibility logic is straightforward and handles the appropriate fields.
apps/web/core/components/workspace-notifications/sidebar/notification-card/content.tsx (5)
21-38:LGTM! Well-structured type definitions.The type definitions provide a clean abstraction for the map-based notification content system.
41-112:Excellent refactor to map-based rendering.The centralized content map improves maintainability and extensibility. The structure is clear with consistent handler signatures, and the special cases (comment, partial handlers) are well-documented through comments.
163-168:LGTM! Clean data structure for field information.The
fieldDataobject provides a clear, reusable structure for passing notification field information.
181-187:Verify fallback behavior consistency.The action rendering logic falls through to
renderAdditionalActionfor fields not in the map. Ensure that the fallback behavior from the CE file (which produces strings with potential leading spaces) is acceptable for all unmapped fields.Based on the issues identified in the CE file review, verify that the fallback rendering produces correctly formatted action strings.
198-199:LGTM! Proper fallback chain for connector visibility.The connector visibility logic correctly prioritizes the map value and falls back to the helper function.
| export const renderAdditionalAction = (notificationField: string, verb: string | undefined) => { | ||
| const baseAction = !["comment", "archived_at"].includes(notificationField) ? verb : ""; | ||
| return `${baseAction} ${replaceUnderscoreIfSnakeCase(notificationField)}`; | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Fix potential leading space in action string.
WhenbaseAction is an empty string (for "comment" or "archived_at" fields), the string concatenation on line 10 produces a leading space.
Apply this diff:
export const renderAdditionalAction = (notificationField: string, verb: string | undefined) => { const baseAction = !["comment", "archived_at"].includes(notificationField) ? verb : "";- return `${baseAction} ${replaceUnderscoreIfSnakeCase(notificationField)}`;+ return baseAction+ ? `${baseAction} ${replaceUnderscoreIfSnakeCase(notificationField)}`+ : replaceUnderscoreIfSnakeCase(notificationField); };🤖 Prompt for AI Agents
In apps/web/ce/components/workspace-notifications/notification-card/content.tsaround lines 8 to 11, the concatenation `\`${baseAction}${replaceUnderscoreIfSnakeCase(notificationField)}\`` produces a leading spacewhen baseAction is an empty string; change the return to conditionally join theparts so you only include the space when baseAction is non-empty (e.g., returnbaseAction ? `${baseAction} ${replaceUnderscoreIfSnakeCase(notificationField)}`: replaceUnderscoreIfSnakeCase(notificationField)).
Uh oh!
There was an error while loading.Please reload this page.
Description
This update refactors notification card render to use map improving the structure and extensibility of the render logic.
Type of Change
Screenshots and Media (if applicable)
Test Scenarios
References
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.