[WEB-2388] fix: workspace draft issues#5800
Conversation
…ne into fix-workspace-draft-issues
WalkthroughThe changes in this pull request encompass updates to several components and serializers related to draft issues within the application. Key modifications include the addition of new fields in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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: 5
🧹 Outside diff range and nitpick comments (8)
web/app/[workspaceSlug]/(projects)/drafts/header.tsx (2)
40-48: LGTM: UI enhancement with draft issue count.The addition of the
CountChipcomponent improves the UI by displaying the count of draft issues. The conditional rendering is implemented correctly.Consider a minor optimization to simplify the conditional rendering:
- {paginationInfo?.count && paginationInfo?.count > 0 ? <CountChip count={paginationInfo?.count} /> : <></>} + {paginationInfo?.count > 0 && <CountChip count={paginationInfo.count} />}This change removes the redundant check and empty fragment, making the code more concise.
Line range hint
1-65: Overall changes look good and align with PR objectives.The
WorkspaceDraftHeadercomponent has been successfully updated to include the count of draft issues. The changes are well-integrated, maintaining the existing structure while adding new functionality. The use of hooks and conditional rendering is appropriate and consistent with React best practices.As the component grows in complexity, consider breaking it down into smaller, more focused components in the future. This will improve maintainability and reusability.
web/core/components/issues/issue-modal/draft-issue-layout.tsx (2)
56-56: LGTM: New hookuseWorkspaceDraftIssuesutilizedThe usage of
createIssuefromuseWorkspaceDraftIssuesis a good implementation of the newly imported hook. This change aligns well with the modifications in thehandleCreateDraftIssuefunction.Consider renaming the destructured function to
createDraftIssuefor better clarity:const { createIssue: createDraftIssue } = useWorkspaceDraftIssues();This would make it more explicit that we're dealing with draft issues throughout the component.
Line range hint
98-130: LGTM:handleCreateDraftIssueupdated to use new hookThe modification of
handleCreateDraftIssueto usecreateIssuefrom theuseWorkspaceDraftIssueshook is a good change. It maintains the existing functionality while leveraging the new hook-based approach.Consider enhancing the error handling by logging the error or providing more specific error messages:
.catch((error) => { console.error("Failed to create draft issue:", error); setToast({ type: TOAST_TYPE.ERROR, title: "Error!", message: `Issue could not be created: ${error.message || "Please try again."}`, }); // ... rest of the error handling });This would provide more detailed information for debugging and potentially give users more context about the error.
apiserver/plane/app/serializers/draft.py (2)
279-280: LGTM! Consider adding docstring for new fields.The addition of
type_idanddescription_htmlto theDraftIssueSerializeris appropriate and consistent with the existing pattern. These fields enhance the serializer's capability to handle issue types and HTML-formatted descriptions.Consider adding a docstring to the
DraftIssueSerializerclass to explain the purpose of these new fields, especiallytype_id, as it might not be immediately clear to other developers what type of issue it represents.class DraftIssueSerializer(BaseSerializer): """ Serializer for draft issues. Fields: - type_id: Represents the type of the issue (e.g., bug, feature, task). - description_html: HTML-formatted description of the issue. ... """ # ... rest of the code
Line range hint
288-291: Remove redundant field declaration.The
description_htmlfield is already inherited fromDraftIssueSerializer. Explicitly adding it to thefieldslist inDraftIssueDetailSerializeris redundant and might cause confusion.Consider removing the redundant field declaration:
class DraftIssueDetailSerializer(DraftIssueSerializer): description_html = serializers.CharField() class Meta(DraftIssueSerializer.Meta): - fields = DraftIssueSerializer.Meta.fields + [ - "description_html", - ] + fields = DraftIssueSerializer.Meta.fields read_only_fields = fieldsThis change will maintain the same functionality while reducing redundancy in the code.
web/core/components/issues/issue-modal/base.tsx (2)
Line range hint
126-133: LGTM with a minor suggestion.The refactoring of the
handleClosefunction looks good. It simplifies the logic by removing local storage handling and directly callinghandleCreateIssuewhen necessary. This aligns well with the removal of local storage functionality for draft issues.Consider renaming the
saveAsDraftparameter toshouldSaveAsDraftfor better clarity:-const handleClose = (saveAsDraft?: boolean) => { - if (changesMade && saveAsDraft && !data) { +const handleClose = (shouldSaveAsDraft?: boolean) => { + if (changesMade && shouldSaveAsDraft && !data) {This small change would make the parameter's purpose more explicit.
Line range hint
261-263: LGTM with a minor suggestion for improved clarity.The removal of
sourceIssueIdfrom the payload is a good cleanup step. It ensures that this temporary property doesn't persist in the created or updated issue.To improve code clarity, consider using the optional chaining operator:
-if (data?.sourceIssueId) delete data.sourceIssueId; +delete data?.sourceIssueId;This change simplifies the code while maintaining the same functionality, as
deleteis safe to use on non-existent properties.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- apiserver/plane/app/serializers/draft.py (1 hunks)
- web/app/[workspaceSlug]/(projects)/drafts/header.tsx (3 hunks)
- web/core/components/issues/issue-modal/base.tsx (2 hunks)
- web/core/components/issues/issue-modal/draft-issue-layout.tsx (3 hunks)
- web/core/components/issues/workspace-draft/root.tsx (4 hunks)
- web/core/store/issue/root.store.ts (2 hunks)
- web/core/store/issue/workspace-draft/issue.store.ts (8 hunks)
🧰 Additional context used
🔇 Additional comments (18)
web/app/[workspaceSlug]/(projects)/drafts/header.tsx (2)
9-9: LGTM: New imports are correctly added.The new imports for
CountChipanduseWorkspaceDraftIssuesare correctly placed and consistent with the changes described in the summary.Also applies to: 14-14
23-23: LGTM: New hook is correctly used.The
useWorkspaceDraftIssueshook is correctly implemented to retrieve thepaginationInfo.To ensure the hook is implemented correctly, you may want to verify its implementation. Here's a script to check the hook's definition:
✅ Verification successful
Verified: Hook implementation is correct.
The
useWorkspaceDraftIssueshook is correctly implemented to retrieve thepaginationInfo.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of useWorkspaceDraftIssues hook # Test: Search for the hook definition ast-grep --lang typescript --pattern 'export const useWorkspaceDraftIssues = $_'Length of output: 763
web/core/components/issues/issue-modal/draft-issue-layout.tsx (2)
17-17: LGTM: Import statement updated to includeuseWorkspaceDraftIssuesThe addition of
useWorkspaceDraftIssuesfrom@/hooks/storeis a good change. It suggests a move towards using a custom hook for managing draft issues, which can lead to better code organization and reusability.
Line range hint
1-158: Overall assessment: Good improvements in code organizationThe changes in this file represent a positive step towards better code organization and potential reusability. By introducing the
useWorkspaceDraftIssueshook and updating thehandleCreateDraftIssuefunction to use it, the component now delegates the draft issue creation logic to a dedicated hook. This approach can lead to more maintainable and testable code.The modifications are well-integrated into the existing component structure, maintaining its overall functionality while improving its implementation. The consistent use of the new hook throughout the component shows good attention to detail.
These changes are approved and ready for merging, pending the minor suggestions made in previous comments.
web/core/store/issue/root.store.ts (3)
206-206: LGTM: WorkspaceDraftIssues initialization updatedThe change to pass
thisto theWorkspaceDraftIssuesconstructor is consistent with other store initializations and aligns with the PR objectives related to workspace draft issues. This update allowsWorkspaceDraftIssuesto access theIssueRootStorecontext, which can improve data flow and interactivity.
227-227: Verify the need for CalendarStore updateThe change to pass
thisto theCalendarStoreconstructor is consistent with other store initializations. However, this change wasn't explicitly mentioned in the PR objectives related to workspace draft issues.Could you please confirm if this change is intentional and necessary for the current PR objectives? If it's related to a different feature or improvement, consider splitting it into a separate PR for better traceability.
Line range hint
1-229: Overall assessment: Minor improvements to store initializationsThe changes in this file are minimal and focused on improving the consistency of store initializations. They potentially enhance data flow between stores by providing access to the
IssueRootStorecontext. TheWorkspaceDraftIssueschange aligns well with the PR objectives. However, theCalendarStorechange, while consistent with the pattern, might benefit from additional context or justification if it's not directly related to the current PR objectives.apiserver/plane/app/serializers/draft.py (1)
Line range hint
1-291: Overall, the changes improve the draft issue serializers.The additions to
DraftIssueSerializerandDraftIssueDetailSerializerenhance their capabilities to handle issue types and HTML-formatted descriptions. The changes are consistent with the existing code structure and don't introduce any major issues.A few minor improvements have been suggested:
- Adding a docstring to
DraftIssueSerializerto explain the new fields.- Removing a redundant field declaration in
DraftIssueDetailSerializer.These changes will improve code clarity and maintainability without affecting functionality.
web/core/components/issues/issue-modal/base.tsx (1)
321-321: LGTM: Improved prop passing.The update to directly pass
handleCloseas theonCloseprop is a good improvement. It simplifies the code by removing an unnecessary anonymous function wrapper, which enhances readability and potentially improves performance by avoiding the creation of a new function on each render.web/core/components/issues/workspace-draft/root.tsx (1)
28-28: Handle potential undefined return fromuseProjecthookEnsure that the
useProjecthook returns a definedworkspaceProjectIds. If there's a possibility thatworkspaceProjectIdscould benullorundefined, it might be beneficial to set a default value to prevent potential runtime errors.Run the following script to check if
workspaceProjectIdscan beundefined:web/core/store/issue/workspace-draft/issue.store.ts (8)
27-28: ImportingIIssueRootStoreThe addition of
IIssueRootStoreimport from"../root.store"is appropriate for injecting the root issue store intoWorkspaceDraftIssues.
35-37: AddingissueMapIdsto the interfaceIncluding
issueMapIdsin theIWorkspaceDraftIssuesinterface enhances the tracking of issue IDs per workspace, promoting better data organization.
119-123: InitializeissueMapIdsand injectissueStoreInitializing
issueMapIdsand injectingissueStorethrough the constructor improves state management and ensures access to the workspace slug.
126-126: MakingissueMapIdsobservableAdding
issueMapIdsto the observable properties ensures that any changes to the issue ID mappings are properly tracked and reacted to within the application.Also applies to: 128-128
144-148: ComputeissueIdsbased on current workspaceThe
issueIdscomputed property correctly retrieves and orders issue IDs for the current workspace by creation date in descending order.
225-228: UpdateissueMapIdsafter fetching issuesUpon fetching issues, updating
issueMapIdswith the new issue IDs maintains the mapping between workspace slugs and their corresponding issue IDs.
252-255: Add new issue toissueMapIdsafter creationIn the
createIssuemethod, adding the new issue ID to the beginning ofissueMapIdsensures that the most recently created issues appear first.
271-275: Update issue inissuesMapwith new dataThe
updateIssuemethod correctly merges the existing issue data with the new payload and updates theupdated_attimestamp.
|
|
||
| {paginationInfo?.next_page_results && ( | ||
| <Fragment> | ||
| {loader === "pagination" && issueIds.length >= 0 ? ( |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify conditional rendering by removing redundant check
The condition issueIds.length >= 0 at line 70 is always true because an array's length is always greater than or equal to zero. You can simplify the conditional rendering by removing this redundant check.
Apply this diff to simplify the condition:
- {loader === "pagination" && issueIds.length >= 0 ? (
+ {loader === "pagination" ? (📝 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.
| {loader === "pagination" && issueIds.length >= 0 ? ( | |
| {loader === "pagination" ? ( |
| if (workspaceProjectIds?.length === 0) | ||
| return ( | ||
| <EmptyState | ||
| type={EmptyStateType.WORKSPACE_NO_PROJECTS} | ||
| size="sm" | ||
| primaryButtonOnClick={() => { | ||
| toggleCreateProjectModal(true); | ||
| }} | ||
| /> | ||
| ); | ||
|
|
There was a problem hiding this comment.
Ensure proper handling when workspaceProjectIds is undefined
At line 47, the condition workspaceProjectIds?.length === 0 checks for an empty array. However, if workspaceProjectIds is undefined, workspaceProjectIds?.length will be undefined, and the condition will evaluate to false. If you intend to show the EmptyState when workspaceProjectIds is undefined or empty, consider updating the condition.
Apply this diff to handle both undefined and empty arrays:
- if (workspaceProjectIds?.length === 0)
+ if (!workspaceProjectIds || workspaceProjectIds.length === 0)📝 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 (workspaceProjectIds?.length === 0) | |
| return ( | |
| <EmptyState | |
| type={EmptyStateType.WORKSPACE_NO_PROJECTS} | |
| size="sm" | |
| primaryButtonOnClick={() => { | |
| toggleCreateProjectModal(true); | |
| }} | |
| /> | |
| ); | |
| if (!workspaceProjectIds || workspaceProjectIds.length === 0) | |
| return ( | |
| <EmptyState | |
| type={EmptyStateType.WORKSPACE_NO_PROJECTS} | |
| size="sm" | |
| primaryButtonOnClick={() => { | |
| toggleCreateProjectModal(true); | |
| }} | |
| /> | |
| ); | |
| <Fragment> | ||
| {loader === "pagination" && issueIds.length >= 0 ? ( | ||
| <WorkspaceDraftIssuesLoader items={1} /> | ||
| ) : ( | ||
| <div | ||
| className={cn( | ||
| "h-11 pl-6 p-3 text-sm font-medium bg-custom-background-100 border-b border-custom-border-200 transition-all", | ||
| { | ||
| "text-custom-primary-100 hover:text-custom-primary-200 cursor-pointer underline-offset-2 hover:underline": | ||
| paginationInfo?.next_page_results, | ||
| } | ||
| )} | ||
| onClick={handleNextIssues} | ||
| > | ||
| Load More ↓ | ||
| </div> | ||
| )} | ||
| onClick={handleNextIssues} | ||
| > | ||
| Load More ↓ | ||
| </div> | ||
| </Fragment> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider removing unnecessary Fragment wrapper
The <Fragment> wrapper introduced at lines 69 to 86 is unnecessary since it only contains a single child element. You can remove the Fragment to simplify the code.
Apply this diff to remove the Fragment:
{paginationInfo?.next_page_results && (
- <Fragment>
{loader === "pagination" ? (
<WorkspaceDraftIssuesLoader items={1} />
) : (
<div
className={cn(
"h-11 pl-6 p-3 text-sm font-medium bg-custom-background-100 border-b border-custom-border-200 transition-all",
{
"text-custom-primary-100 hover:text-custom-primary-200 cursor-pointer underline-offset-2 hover:underline":
paginationInfo?.next_page_results,
}
)}
onClick={handleNextIssues}
>
Load More ↓
</div>
)}
- </Fragment>
)}Since Fragment is no longer used elsewhere, you can also remove the import statement at line 3:
-import { FC, Fragment } from "react";
+import { FC } from "react";Committable suggestion was skipped due to low confidence.
| runInAction(() => { | ||
| unset(this.issueMapIds[workspaceSlug], issueId); | ||
| unset(this.issuesMap, issueId); | ||
| }); |
There was a problem hiding this comment.
Fix issue removal in moveIssue: incorrect use of unset on arrays
Similar to deleteIssue, the moveIssue method incorrectly uses unset on an array to remove an issueId. This does not effectively remove the issue ID from the list.
Suggested fix:
runInAction(() => {
- unset(this.issueMapIds[workspaceSlug], issueId);
+ update(this.issueMapIds, [workspaceSlug], (issueIds = []) => issueIds.filter(id => id !== issueId));
unset(this.issuesMap, issueId);
});📝 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.
| runInAction(() => { | |
| unset(this.issueMapIds[workspaceSlug], issueId); | |
| unset(this.issuesMap, issueId); | |
| }); | |
| runInAction(() => { | |
| update(this.issueMapIds, [workspaceSlug], (issueIds = []) => issueIds.filter(id => id !== issueId)); | |
| unset(this.issuesMap, issueId); | |
| }); |
| runInAction(() => { | ||
| unset(this.issueMapIds[workspaceSlug], issueId); | ||
| unset(this.issuesMap, issueId); | ||
| }); |
There was a problem hiding this comment.
Fix issue removal: incorrect use of unset on arrays
The deleteIssue method uses unset on an array, which does not remove the issueId as intended. Since issueMapIds[workspaceSlug] is an array, you should remove the issueId using array manipulation methods.
Suggested fix:
runInAction(() => {
- unset(this.issueMapIds[workspaceSlug], issueId);
+ update(this.issueMapIds, [workspaceSlug], (issueIds = []) => issueIds.filter(id => id !== issueId));
unset(this.issuesMap, issueId);
});📝 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.
| runInAction(() => { | |
| unset(this.issueMapIds[workspaceSlug], issueId); | |
| unset(this.issuesMap, issueId); | |
| }); | |
| runInAction(() => { | |
| update(this.issueMapIds, [workspaceSlug], (issueIds = []) => issueIds.filter(id => id !== issueId)); | |
| unset(this.issuesMap, issueId); | |
| }); |
4 participants
Summary:
This PR included bug fixes and improvement for workspace draft issues.
Summary by CodeRabbit
New Features
type_idanddescription_htmlto draft issue serializers for enhanced data representation.CountChipcomponent to display the count of draft issues in the header.Bug Fixes
Refactor
Documentation