[WIKI-551] refactor: work item activity logic#7417
Conversation
WalkthroughThe changes rename and update a store method to return combined activity and comment data, update a React component to use this new method and handle loading and empty states, and introduce a new loader component for visual feedback during data fetching. Changes
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
|
Pull Request Linked with Plane Work Items
Comment Automatically Generated by Plane |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
apps/web/core/components/issues/issue-detail/issue-activity/activity-comment-root.tsx (1)
55-90: Fix missing key prop in mapped elements.The static analysis tool correctly identifies that the mapped elements are missing the
keyprop, which is required for React's reconciliation process.Apply this fix:
- {filteredActivityAndComments.map((activityComment, index) => { + {filteredActivityAndComments.map((activityComment, index) => { const comment = getCommentById(activityComment.id); return activityComment.activity_type === "COMMENT" ? ( <CommentCard key={activityComment.id}Add the key prop to all other rendered elements:
) : activityComment.activity_type === "ACTIVITY" ? ( <IssueActivityItem + key={activityComment.id} activityId={activityComment.id} ends={index === 0 ? "top" : index === filteredActivityAndComments.length - 1 ? "bottom" : undefined} /> ) : activityComment.activity_type === "ISSUE_ADDITIONAL_PROPERTIES_ACTIVITY" ? ( <IssueAdditionalPropertiesActivity + key={activityComment.id} activityId={activityComment.id} ends={index === 0 ? "top" : index === filteredActivityAndComments.length - 1 ? "bottom" : undefined} /> ) : activityComment.activity_type === "WORKLOG" ? ( <IssueActivityWorklog + key={activityComment.id} workspaceSlug={workspaceSlug} projectId={projectId} issueId={issueId} activityComment={activityComment} ends={index === 0 ? "top" : index === filteredActivityAndComments.length - 1 ? "bottom" : undefined} /> ) : ( - <></> + <div key={activityComment.id}></div> );
🧹 Nitpick comments (2)
apps/web/core/components/issues/issue-detail/issue-activity/loader.tsx (1)
6-29: Consider extracting repeated block structure to reduce duplication.The three loader blocks share identical structure with only minor width variations. Consider extracting this into a reusable sub-component or loop structure.
Example refactor:
+const LoaderBlock = ({ widths }: { widths: [string, string, string] }) => ( + <div className="flex items-start gap-3"> + <Loader.Item className="shrink-0" height="28px" width="28px" /> + <div className="space-y-2 w-full"> + <Loader.Item height="8px" width={widths[0]} /> + <Loader.Item height="8px" width={widths[1]} /> + <Loader.Item height="10px" width={widths[2]} /> + </div> + </div> +); export const IssueActivityLoader = () => ( <Loader className="space-y-8"> - <div className="flex items-start gap-3"> - <Loader.Item className="shrink-0" height="28px" width="28px" /> - <div className="space-y-2 w-full"> - <Loader.Item height="8px" width="60%" /> - <Loader.Item height="8px" width="40%" /> - <Loader.Item height="10px" width="100%" /> - </div> - </div> - <div className="flex items-start gap-3"> - <Loader.Item className="shrink-0" height="28px" width="28px" /> - <div className="space-y-2 w-full"> - <Loader.Item height="8px" width="40%" /> - <Loader.Item height="8px" width="60%" /> - <Loader.Item height="10px" width="80%" /> - </div> - </div> - <div className="flex items-start gap-3"> - <Loader.Item className="shrink-0" height="28px" width="28px" /> - <div className="space-y-2 w-full"> - <Loader.Item height="8px" width="60%" /> - <Loader.Item height="8px" width="40%" /> - <Loader.Item height="10px" width="100%" /> - </div> - </div> + <LoaderBlock widths={["60%", "40%", "100%"]} /> + <LoaderBlock widths={["40%", "60%", "80%"]} /> + <LoaderBlock widths={["60%", "40%", "100%"]} /> </Loader> );apps/web/core/components/issues/issue-detail/issue-activity/activity-comment-root.tsx (1)
87-89: Improve fallback case handling.The fallback case returns an empty fragment, which should either have a key prop or be replaced with null.
Consider this improvement:
- ) : ( - <></> - ); + ) : null;Or if you need to maintain the element for layout purposes:
- ) : ( - <></> - ); + ) : ( + <div key={activityComment.id} /> + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/web/ce/store/issue/issue-details/activity.store.ts(3 hunks)apps/web/core/components/issues/issue-detail/issue-activity/activity-comment-root.tsx(3 hunks)apps/web/core/components/issues/issue-detail/issue-activity/loader.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: prateekshourya29
PR: makeplane/plane#7188
File: web/app/(all)/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/cycles/(list)/header.tsx:40-45
Timestamp: 2025-06-18T09:46:08.566Z
Learning: When reviewing breadcrumb components that accept projectId or similar props, check if empty strings are being passed during loading states, which can result in invalid URLs. The preferred approach is to handle these loading states internally within the component rather than requiring each consumer to manage the loading logic.
apps/web/core/components/issues/issue-detail/issue-activity/loader.tsx (1)
Learnt from: lifeiscontent
PR: makeplane/plane#7164
File: packages/ui/.storybook/main.ts:24-47
Timestamp: 2025-06-04T16:22:44.344Z
Learning: In packages/ui/.storybook/main.ts, the webpackFinal function intentionally overrides the CSS loader strategy by finding and replacing existing CSS rules. This is a temporary workaround for a known upstream issue in Storybook's CSS handling that has been communicated to the Storybook maintainers. The current implementation should not be changed until the upstream issue is resolved.
apps/web/core/components/issues/issue-detail/issue-activity/activity-comment-root.tsx (1)
Learnt from: vamsikrishnamathala
PR: makeplane/plane#7061
File: web/core/components/workspace-notifications/root.tsx:18-18
Timestamp: 2025-05-14T13:16:23.323Z
Learning: In the Plane project codebase, the path alias `@/plane-web` resolves to the `ce` directory, making imports like `@/plane-web/hooks/use-notification-preview` correctly resolve to files in `web/ce/hooks/`.
apps/web/ce/store/issue/issue-details/activity.store.ts (1)
Learnt from: vamsikrishnamathala
PR: makeplane/plane#7214
File: web/core/store/issue/helpers/base-issues.store.ts:117-117
Timestamp: 2025-06-16T07:23:39.497Z
Learning: In the updateIssueDates method of BaseIssuesStore (web/core/store/issue/helpers/base-issues.store.ts), the projectId parameter is intentionally made optional to support override implementations in subclasses. The base implementation requires projectId and includes an early return check, but making it optional allows derived classes to override the method with different parameter requirements.
🪛 Biome (1.9.4)
apps/web/core/components/issues/issue-detail/issue-activity/activity-comment-root.tsx
[error] 75-78: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
🔇 Additional comments (8)
apps/web/core/components/issues/issue-detail/issue-activity/loader.tsx (1)
4-31: Well-implemented loading component with good visual hierarchy.The loader component follows established skeleton loading patterns with proper spacing, sizing, and visual variation. The repeated structure effectively simulates the expected content layout.
apps/web/core/components/issues/issue-detail/issue-activity/activity-comment-root.tsx (3)
41-41: Good refactoring to use the renamed store method.The update to use
getActivityAndCommentsByIssueIdaligns with the store changes and maintains consistency in the codebase.
47-47: Proper loading state handling with the new loader component.The integration of
IssueActivityLoaderprovides good visual feedback during data fetching and handles the undefined state appropriately.
49-49: Good improvement: using null instead of empty fragment.Returning
nullfor empty states is more semantically correct than an empty fragment and properly indicates "no content" to React.apps/web/ce/store/issue/issue-details/activity.store.ts (4)
46-46: Good method renaming for clarity.The renamed method
getActivityAndCommentsByIssueIdbetter describes its purpose of returning combined activity and comment data.
87-123: Improved data handling with proper undefined checks.The refactored method includes better error handling with early returns for missing data and uses optional chaining appropriately. The explicit
undefinedreturn enables proper loading state detection in consuming components.
100-118: Consistent use of optional chaining for safe iteration.The optional chaining usage in the forEach loops provides safe iteration over potentially undefined arrays, preventing runtime errors.
98-98: Verification complete: All references updated to the new method
rg confirms no occurrences of getActivityCommentByIssueId remain; only getActivityAndCommentsByIssueId is in use.
apps/web/core/components/issues/issue-detail/issue-activity/activity-comment-root.tsx
Show resolved
Hide resolved
3 participants
Description
This PR includes the following changes-
Type of Change
Summary by CodeRabbit
New Features
Refactor