chore: added common component for project activity #6212
Conversation
WalkthroughThis pull request introduces a set of new React components and utilities for managing and displaying project activities. Notable additions include the Changes
Sequence DiagramsequenceDiagram
participant User as User Component
participant ActivityItem as Activity Item
participant ActivityBlock as Activity Block Component
participant Helper as Activity Helper
User->>ActivityItem: Render with activity data
ActivityItem->>Helper: Get icon and message
Helper-->>ActivityItem: Return icon and message
ActivityItem->>ActivityBlock: Pass icon, activity, message
ActivityBlock->>ActivityBlock: Render activity block with tooltip
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (1)packages/types/src/enums.ts (1)
The additions follow the established naming conventions and align with the PR's objective of enhancing project activity functionality. Let's verify the usage of these new enum members: 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: 7
🧹 Nitpick comments (4)
web/core/components/common/activity/activity-item.tsx (1)
10-14: Consider adding runtime prop validationFor better development experience and runtime safety, consider adding prop-types validation.
Example implementation:
import PropTypes from 'prop-types'; // ... component code ... ActivityItem.propTypes = { activity: PropTypes.shape({ field: PropTypes.string.isRequired, // add other required activity properties }).isRequired, showProject: PropTypes.bool, ends: PropTypes.oneOf(['top', 'bottom', undefined]) };web/core/components/common/activity/activity-block.tsx (1)
12-18: Props interface could be improved with better type safetyThe
endsprop type could be more strictly typed using a union literal type.Consider this improvement:
type TActivityBlockComponent = { icon?: ReactNode; activity: TProjectActivity; - ends: "top" | "bottom" | undefined; + ends?: "top" | "bottom"; children: ReactNode; customUserName?: string; };web/core/components/common/activity/helper.tsx (2)
31-66: Consider organizing icons by category for better maintainabilityThe icon mapping, while comprehensive, could be better organized by grouping related icons together.
Consider restructuring the icons map:
const ACTIVITY_ICONS = { project: { name: <Type className="h-3.5 w-3.5 text-custom-text-200" />, description: <AlignLeft className="h-3.5 w-3.5 text-custom-text-200" />, // ... other project-related icons }, features: { is_epic_enabled: <LayoutGrid className="h-3.5 w-3.5 text-custom-text-200" />, is_workflow_enabled: <GitBranch className="h-3.5 w-3.5 text-custom-text-200" />, // ... other feature-related icons }, // ... other categories } as const; export const iconsMap: ActivityIconMap = { ...ACTIVITY_ICONS.project, ...ACTIVITY_ICONS.features, // ... spread other categories default: <Network className="h-3.5 w-3.5 text-custom-text-200" />, };
74-78: Improve type safety of boolean action text helperThe
getBooleanActionTextfunction could benefit from stricter typing.Consider this improvement:
const getBooleanActionText = (value: 'true' | 'false' | string): string => { if (value === "true") return "enabled"; if (value === "false") return "disabled"; return value; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
web/core/components/common/activity/activity-block.tsx(1 hunks)web/core/components/common/activity/activity-item.tsx(1 hunks)web/core/components/common/activity/helper.tsx(1 hunks)web/core/components/common/activity/index.ts(1 hunks)web/core/components/common/activity/user.tsx(1 hunks)web/core/components/common/index.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- web/core/components/common/activity/index.ts
🧰 Additional context used
🪛 Biome (1.9.4)
web/core/components/common/activity/activity-item.tsx
[error] 27-27: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
🔇 Additional comments (1)
web/core/components/common/index.ts (1)
8-8: LGTM! Export follows established pattern
The new export follows the existing pattern and correctly exposes the activity module.
| {customUserName || activity.actor_detail?.display_name.includes("-intake") ? ( | ||
| <span className="text-custom-text-100 font-medium">{customUserName || "Plane"}</span> | ||
| ) : ( |
There was a problem hiding this comment.
Add null checks for actor_detail properties
The current implementation could throw runtime errors if actor_detail or display_name is undefined.
Suggested fix:
- {customUserName || activity.actor_detail?.display_name.includes("-intake") ? (
+ {customUserName || (activity.actor_detail?.display_name || "").includes("-intake") ? (📝 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.
| {customUserName || activity.actor_detail?.display_name.includes("-intake") ? ( | |
| <span className="text-custom-text-100 font-medium">{customUserName || "Plane"}</span> | |
| ) : ( | |
| {customUserName || (activity.actor_detail?.display_name || "").includes("-intake") ? ( | |
| <span className="text-custom-text-100 font-medium">{customUserName || "Plane"}</span> | |
| ) : ( |
| <Link | ||
| href={`/${activity?.workspace_detail?.slug}/profile/${activity?.actor_detail?.id}`} | ||
| className="hover:underline text-custom-text-100 font-medium" | ||
| > | ||
| {activity.actor_detail?.display_name} | ||
| </Link> | ||
| )} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add safety checks for profile link and enhance accessibility
The profile link construction needs safety checks for undefined values, and the link should have proper accessibility attributes.
Suggested improvements:
<Link
- href={`/${activity?.workspace_detail?.slug}/profile/${activity?.actor_detail?.id}`}
+ href={activity?.workspace_detail?.slug && activity?.actor_detail?.id
+ ? `/${activity.workspace_detail.slug}/profile/${activity.actor_detail.id}`
+ : "#"}
- className="hover:underline text-custom-text-100 font-medium"
+ className="hover:underline text-custom-text-100 font-medium"
+ aria-label={`View ${activity.actor_detail?.display_name}'s profile`}
>
{activity.actor_detail?.display_name}
</Link>📝 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.
| <Link | |
| href={`/${activity?.workspace_detail?.slug}/profile/${activity?.actor_detail?.id}`} | |
| className="hover:underline text-custom-text-100 font-medium" | |
| > | |
| {activity.actor_detail?.display_name} | |
| </Link> | |
| )} | |
| <Link | |
| href={activity?.workspace_detail?.slug && activity?.actor_detail?.id | |
| ? `/${activity.workspace_detail.slug}/profile/${activity.actor_detail.id}` | |
| : "#"} | |
| className="hover:underline text-custom-text-100 font-medium" | |
| aria-label={`View ${activity.actor_detail?.display_name}'s profile`} | |
| > | |
| {activity.actor_detail?.display_name} | |
| </Link> | |
| )} |
| <ActivityBlockComponent icon={icon} activity={activity} ends={ends} customUserName={customUserName}> | ||
| <>{message}</> | ||
| </ActivityBlockComponent> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unnecessary fragment and add error handling
- The fragment wrapper is unnecessary as noted by static analysis.
- Consider adding error boundary protection for message generation.
Suggested improvements:
<ActivityBlockComponent icon={icon} activity={activity} ends={ends} customUserName={customUserName}>
- <>{message}</>
+ {message}
</ActivityBlockComponent>Also, consider wrapping the message generation in a try-catch:
- const { message, customUserName } = messages(activity);
+ let message, customUserName;
+ try {
+ ({ message, customUserName } = messages(activity));
+ } catch (error) {
+ console.error('Failed to generate activity message:', error);
+ message = 'Activity details unavailable';
+ customUserName = undefined;
+ }📝 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.
| <ActivityBlockComponent icon={icon} activity={activity} ends={ends} customUserName={customUserName}> | |
| <>{message}</> | |
| </ActivityBlockComponent> | |
| <ActivityBlockComponent icon={icon} activity={activity} ends={ends} customUserName={customUserName}> | |
| {message} | |
| </ActivityBlockComponent> |
🧰 Tools
🪛 Biome (1.9.4)
[error] 27-27: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
| // hooks | ||
| const { isMobile } = usePlatformOS(); | ||
|
|
||
| if (!activity) return <></>; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error boundary or proper error handling
The early return with empty fragment when activity is null could be improved with proper error handling.
Consider adding error boundary or proper error state:
- if (!activity) return <></>;
+ if (!activity) {
+ console.warn("ActivityBlockComponent: No activity provided");
+ return null;
+ }📝 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.
| if (!activity) return <></>; | |
| if (!activity) { | |
| console.warn("ActivityBlockComponent: No activity provided"); | |
| return null; | |
| } |
| <div className="absolute left-[13px] top-0 bottom-0 w-0.5 bg-custom-background-80" aria-hidden /> | ||
| <div className="flex-shrink-0 ring-6 w-7 h-7 rounded-full overflow-hidden flex justify-center items-center z-[4] bg-custom-background-80 text-custom-text-200"> | ||
| {icon ? icon : <Network className="w-3.5 h-3.5" />} | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve accessibility for activity timeline
The timeline implementation could benefit from better accessibility attributes.
Add proper ARIA attributes and semantic HTML:
- <div className="absolute left-[13px] top-0 bottom-0 w-0.5 bg-custom-background-80" aria-hidden />
+ <div
+ className="absolute left-[13px] top-0 bottom-0 w-0.5 bg-custom-background-80"
+ role="separator"
+ aria-label="Activity timeline"
+ />📝 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.
| <div className="absolute left-[13px] top-0 bottom-0 w-0.5 bg-custom-background-80" aria-hidden /> | |
| <div className="flex-shrink-0 ring-6 w-7 h-7 rounded-full overflow-hidden flex justify-center items-center z-[4] bg-custom-background-80 text-custom-text-200"> | |
| {icon ? icon : <Network className="w-3.5 h-3.5" />} | |
| </div> | |
| <div | |
| className="absolute left-[13px] top-0 bottom-0 w-0.5 bg-custom-background-80" | |
| role="separator" | |
| aria-label="Activity timeline" | |
| /> | |
| <div className="flex-shrink-0 ring-6 w-7 h-7 rounded-full overflow-hidden flex justify-center items-center z-[4] bg-custom-background-80 text-custom-text-200"> | |
| {icon ? icon : <Network className="w-3.5 h-3.5" />} | |
| </div> |
| ) : ( | ||
| <span className="font-medium text-custom-text-100">{activity.old_value || "Unknown cycle"}</span> | ||
| )} | ||
| </> | ||
| ), | ||
| }; | ||
| case "modules": | ||
| return { | ||
| message: ( | ||
| <> | ||
| <span> | ||
| {verb} this project {verb === "removed" ? "from" : "to"} the module{" "} | ||
| </span> | ||
| <span className="font-medium text-custom-text-100"> | ||
| {verb === "removed" ? oldValue : newValue || "Unknown module"} | ||
| </span> | ||
| </> | ||
| ), | ||
| }; | ||
| case "labels": | ||
| return { | ||
| message: ( | ||
| <> | ||
| {verb} the label{" "} | ||
| <span className="font-medium text-custom-text-100">{newValue || oldValue || "Untitled label"}</span> | ||
| </> | ||
| ), | ||
| }; | ||
| case "inbox": | ||
| return { | ||
| message: <>{newValue ? "enabled" : "disabled"} inbox</>, | ||
| }; | ||
| case "page": | ||
| return { | ||
| message: ( | ||
| <> | ||
| {newValue ? "created" : "removed"} the project page{" "} | ||
| <span className="font-medium text-custom-text-100">{newValue || oldValue || "Untitled page"}</span> | ||
| </> | ||
| ), | ||
| }; | ||
| case "network": | ||
| return { | ||
| message: <>{newValue ? "enabled" : "disabled"} network access</>, | ||
| }; | ||
| case "identifier": | ||
| return { | ||
| message: ( | ||
| <> | ||
| updated project identifier to <span className="font-medium text-custom-text-100">{newValue || "none"}</span> | ||
| </> | ||
| ), | ||
| }; | ||
| case "timezone": | ||
| return { | ||
| message: ( | ||
| <> | ||
| changed project timezone to{" "} | ||
| <span className="font-medium text-custom-text-100">{newValue || "default"}</span> | ||
| </> | ||
| ), | ||
| }; | ||
| case "module_view": | ||
| case "cycle_view": | ||
| case "issue_views_view": | ||
| case "page_view": | ||
| case "intake_view": | ||
| return { | ||
| message: ( | ||
| <> | ||
| {getBooleanActionText(newValue)} {activityType.replace(/_view$/, "").replace(/_/g, " ")} view | ||
| </> | ||
| ), | ||
| }; | ||
| case "is_project_updates_enabled": | ||
| return { | ||
| message: <>{getBooleanActionText(newValue)} project updates</>, | ||
| }; | ||
| case "is_epic_enabled": | ||
| return { | ||
| message: <>{getBooleanActionText(newValue)} epics</>, | ||
| }; | ||
| case "is_workflow_enabled": | ||
| return { | ||
| message: <>{getBooleanActionText(newValue)} custom workflow</>, | ||
| }; | ||
| case "is_time_tracking_enabled": | ||
| return { | ||
| message: <>{getBooleanActionText(newValue)} time tracking</>, | ||
| }; | ||
| case "is_issue_type_enabled": | ||
| return { | ||
| message: <>{getBooleanActionText(newValue)} issue types</>, | ||
| }; | ||
| default: | ||
| return { | ||
| message: `${verb} ${activityType.replace(/_/g, " ")} `, | ||
| }; | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider breaking down the message generation logic
The message generation function is quite large and handles many cases. This could be split into smaller, more manageable functions.
Consider breaking down the switch statement into separate handlers:
const messageHandlers = {
priority: (activity: TProjectActivity) => ({
message: (
<>
set the priority to <span className="font-medium text-custom-text-100">{activity.new_value || "none"}</span>
</>
),
}),
// ... other handlers
};
export const messages = (activity: TProjectActivity) => {
const handler = messageHandlers[activity.field];
return handler ? handler(activity) : {
message: `${activity.verb} ${activity.field.replace(/_/g, " ")} `,
};
};This would make the code more maintainable and easier to test.
| <a | ||
| href={`/${activity.workspace_detail?.slug}/projects/${activity.project}/cycles/${activity.new_identifier}`} | ||
| target="_blank" | ||
| rel="noopener noreferrer" | ||
| className="inline-flex font-medium text-custom-text-100" | ||
| > | ||
| {activity.new_value} | ||
| </a> |
There was a problem hiding this comment.
Add error handling for navigation links
The cycle link implementation could benefit from proper error handling for missing slugs or identifiers.
Add proper checks:
{verb !== "removed" && activity.workspace_detail?.slug && activity.new_identifier ? (
<a
href={`/${activity.workspace_detail.slug}/projects/${activity.project}/cycles/${activity.new_identifier}`}
target="_blank"
rel="noopener noreferrer"
className="inline-flex font-medium text-custom-text-100"
>
{activity.new_value}
</a>
) : (
<span className="font-medium text-custom-text-100">
{activity.new_value || "Unknown cycle"}
</span>
)}📝 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.
| <a | |
| href={`/${activity.workspace_detail?.slug}/projects/${activity.project}/cycles/${activity.new_identifier}`} | |
| target="_blank" | |
| rel="noopener noreferrer" | |
| className="inline-flex font-medium text-custom-text-100" | |
| > | |
| {activity.new_value} | |
| </a> | |
| {verb !== "removed" && activity.workspace_detail?.slug && activity.new_identifier ? ( | |
| <a | |
| href={`/${activity.workspace_detail.slug}/projects/${activity.project}/cycles/${activity.new_identifier}`} | |
| target="_blank" | |
| rel="noopener noreferrer" | |
| className="inline-flex font-medium text-custom-text-100" | |
| > | |
| {activity.new_value} | |
| </a> | |
| ) : ( | |
| <span className="font-medium text-custom-text-100"> | |
| {activity.new_value || "Unknown cycle"} | |
| </span> | |
| )} |
* chore: added common component for project activity * fix: added enum * fix: added enum for initiatives
Summary
Summary by CodeRabbit
New Features
ActivityBlockComponentfor displaying structured activity information.ActivityItemcomponent to showcase individual activity items.Usercomponent for displaying user-related information based on activity.INITIATIVE_DESCRIPTIONandPROJECT_DESCRIPTIONto enhance activity categorization.Documentation