[WEB-2388] chore: workspace draft issue improvements#5808
[WEB-2388] chore: workspace draft issue improvements#5808sriramveeraghanta merged 1 commit intopreviewfrom
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces enhancements to draft issue management within the application. Key updates include modifications to the Changes
Possibly related PRs
Suggested reviewers
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 (
|
sriramveeraghanta
deleted the
chore-workspace-draft-issue-improvements
branch
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (6)
web/core/components/issues/issue-modal/modal.tsx (1)
Line range hint
1-33: Consider grouping related properties inIssuesModalPropsThe
IssuesModalPropsinterface is well-defined with a mix of required and optional properties. To improve readability and maintainability, consider grouping related properties together. For example:export interface IssuesModalProps { // Modal control isOpen: boolean; onClose: () => void; // Issue data data?: Partial<TIssue>; isDraft?: boolean; fetchIssueDetails?: boolean; // Callbacks and behavior onSubmit?: (res: TIssue) => Promise<void>; moveToIssue?: boolean; // Rendering options withDraftIssueWrapper?: boolean; storeType?: EIssuesStoreType; }This grouping can make it easier for developers to understand the purpose of each property at a glance.
web/core/components/issues/workspace-draft/quick-action.tsx (1)
81-87: LGTM with a minor suggestionThe changes to the "move-to-issues" menu item action are well-implemented. They correctly set the
moveToIssuestate to true and open the modal whenhandleMoveToIssuesis available.Consider extracting the action logic into a separate function for better readability:
const handleMoveToIssuesAction = () => { if (handleMoveToIssues) { setMoveToIssue(true); setIssueToEdit(issue); setCreateUpdateIssueModal(true); } }; // Then in MENU_ITEMS: { key: "move-to-issues", title: "Move to issues", icon: SquareStackIcon, action: handleMoveToIssuesAction, },web/core/components/issues/issue-modal/draft-issue-layout.tsx (2)
32-32: LGTM! Consider adding documentation for the new property.The addition of the
moveToIssueproperty to theDraftIssuePropsinterface is well-structured and follows TypeScript best practices. It's optional, which maintains backward compatibility.Consider adding a brief comment explaining the purpose of this new property to improve code maintainability:
/** Flag indicating whether the issue can be moved. Used for [brief explanation of its purpose]. */ moveToIssue?: boolean;
47-47: LGTM! Consider using object property shorthand for consistency.The addition of the
moveToIssueprop with a default value and its passing to theIssueFormRootcomponent is implemented correctly.For consistency with modern JavaScript practices, consider using object property shorthand in the
IssueFormRootprops:- moveToIssue={moveToIssue} + moveToIssueThis change is optional and doesn't affect functionality, but it aligns with the style used for other props in this component.
Also applies to: 161-161
web/core/components/issues/issue-modal/base.tsx (2)
33-33: LGTM. Consider adding prop documentation.The addition of the
moveToIssueprop with a default value offalseis good. It allows for optional functionality related to moving issues.Consider adding JSDoc or a comment to explain the purpose and usage of this new prop for better maintainability.
329-329: LGTM. Consider refactoring for DRY principle.The
moveToIssueprop is correctly passed down to theIssueFormRootcomponent, consistent with its usage inDraftIssueLayout.Consider extracting the common props passed to both
DraftIssueLayoutandIssueFormRootinto a separate object to adhere to the DRY (Don't Repeat Yourself) principle. This would make future additions or modifications easier to manage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- apiserver/plane/app/views/workspace/draft.py (1 hunks)
- web/core/components/issues/issue-modal/base.tsx (3 hunks)
- web/core/components/issues/issue-modal/draft-issue-layout.tsx (3 hunks)
- web/core/components/issues/issue-modal/form.tsx (7 hunks)
- web/core/components/issues/issue-modal/modal.tsx (1 hunks)
- web/core/components/issues/workspace-draft/quick-action.tsx (3 hunks)
🧰 Additional context used
🔇 Additional comments (9)
web/core/components/issues/issue-modal/modal.tsx (2)
Line range hint
1-33: Overall, the changes and existing code look goodThe addition of the
moveToIssueproperty toIssuesModalPropsis a minor change that doesn't affect the current component's functionality. The overall structure of the file is well-organized and follows good practices for React and TypeScript development. The use of the observer pattern and conditional rendering are appropriate for this modal component.Remember to update any relevant documentation to reflect the new
moveToIssueproperty and its purpose in the issue management workflow.
23-23: New propertymoveToIssueadded toIssuesModalPropsThe addition of the
moveToIssueoptional boolean property to theIssuesModalPropsinterface is noted. While this change doesn't affect the current component's implementation, it may introduce new behavior in child components or other parts of the application that use this interface.To ensure proper usage and understand the implications of this new property, please run the following script:
This will help us verify where and how the new property is being used across the codebase.
✅ Verification successful
Verification of
moveToIssueProperty IntegrationThe
moveToIssueoptional boolean property has been successfully added to theIssuesModalPropsinterface and is utilized in the following components:
web/core/components/issues/workspace-draft/quick-action.tsxweb/core/components/issues/issue-modal/base.tsxweb/core/components/issues/issue-modal/draft-issue-layout.tsxAll usages appear consistent and appropriate, with no issues detected based on the current implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find usages of the new 'moveToIssue' property # Search for components using IssuesModalProps echo "Components using IssuesModalProps:" rg --type typescript "IssuesModalProps" -l # Search for usage of 'moveToIssue' prop echo "\nUsage of 'moveToIssue' prop:" rg --type typescript "moveToIssue[=:]" -A 5Length of output: 314
Script:
#!/bin/bash # Description: Find usages of the new 'moveToIssue' property # Search for components using IssuesModalProps echo "Components using IssuesModalProps:" rg --type ts --type tsx "IssuesModalProps" -l # Search for usage of 'moveToIssue' prop echo "\nUsage of 'moveToIssue' prop:" rg --type ts --type tsx "moveToIssue[=:]" -A 5Length of output: 306
Script:
#!/bin/bash # Description: Find usages of the new 'moveToIssue' property using glob patterns # Search for components using IssuesModalProps in .ts and .tsx files echo "Components using IssuesModalProps:" rg "IssuesModalProps" --glob '*.ts' --glob '*.tsx' -l # Search for usage of 'moveToIssue' prop in .ts and .tsx files echo "\nUsage of 'moveToIssue' prop:" rg "moveToIssue[=:]" --glob '*.ts' --glob '*.tsx' -A 5Length of output: 2057
web/core/components/issues/workspace-draft/quick-action.tsx (3)
45-45: LGTM: New state variable added correctlyThe addition of the
moveToIssuestate variable is well-implemented. It's initialized tofalse, which is an appropriate default value, and the naming is clear and descriptive.
112-112: LGTM: Proper state reset on modal closeThe addition of
setMoveToIssue(false)in the modal'sonClosehandler is a good practice. It ensures that themoveToIssuestate is reset when the modal is closed, maintaining consistent state management.
120-120: LGTM, but additional context neededThe addition of the
moveToIssueprop to the CreateUpdateIssueModal component is consistent with the changes made earlier in the file. This prop correctly passes themoveToIssuestate to the modal.To ensure this change is properly implemented, could you provide information on how the
moveToIssueprop is used within the CreateUpdateIssueModal component? This will help verify that the prop is being utilized correctly.web/core/components/issues/issue-modal/draft-issue-layout.tsx (1)
Line range hint
1-164: Overall, the changes look good and are well-implemented.The addition of the
moveToIssueproperty to both theDraftIssuePropsinterface and theDraftIssueLayoutcomponent is consistent and follows best practices. The changes are minimal and focused, introducing no apparent issues or bugs.To ensure that the
moveToIssueprop is being used correctly throughout the codebase, you may want to run the following verification:This will help ensure that the new prop is being used consistently and correctly in related components.
web/core/components/issues/issue-modal/base.tsx (2)
312-312: LGTM. Prop correctly passed to child component.The
moveToIssueprop is appropriately passed down to theDraftIssueLayoutcomponent, ensuring the new functionality is available where needed.
Line range hint
1-332: Overall implementation ofmoveToIssueprop looks good.The addition of the
moveToIssueprop to theCreateUpdateIssueModalBasecomponent and its propagation to child components (DraftIssueLayoutandIssueFormRoot) has been implemented correctly. This change appears to introduce new functionality related to moving issues, which aligns with the PR objective of improving workspace draft issues.A few suggestions for further improvement:
- Add documentation for the new prop to enhance maintainability.
- Consider refactoring common props passed to child components to adhere to the DRY principle.
These changes should not introduce any breaking changes and seem to enhance the component's functionality as intended.
To ensure that the
moveToIssueprop is used correctly in the child components, you may want to verify its implementation inDraftIssueLayoutandIssueFormRoot. Here's a script to check for its usage:✅ Verification successful
Verified usage of
moveToIssueprop in all relevant components.The
moveToIssueprop is correctly defined and utilized in bothDraftIssueLayoutandIssueFormRoot(form.tsx). The implementation adheres to React best practices and aligns with the PR's objective of enhancing workspace draft issues.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for the usage of moveToIssue prop in DraftIssueLayout and IssueFormRoot components echo "Checking DraftIssueLayout for moveToIssue usage:" rg "moveToIssue" "web/core/components/issues/issue-modal/draft-issue-layout.tsx" echo "\nChecking IssueFormRoot for moveToIssue usage:" rg "moveToIssue" "web/core/components/issues/issue-modal/form/root.tsx"Length of output: 537
Script:
#!/bin/bash # Description: Locate the IssueFormRoot component in the codebase echo "Searching for IssueFormRoot component definition:" rg "IssueFormRoot" --glob "*.tsx" --files-with-matchesLength of output: 425
Script:
#!/bin/bash # Description: Check for the usage of moveToIssue prop in issue-modal/form.tsx echo "Checking IssueFormRoot in issue-modal/form.tsx for moveToIssue usage:" rg "moveToIssue" "web/core/components/issues/issue-modal/form.tsx"Length of output: 357
apiserver/plane/app/views/workspace/draft.py (1)
150-150: Includingtype_idin the response is appropriate.The addition of
type_idenhances the response data and aligns with the expected fields.
| "created_by", | ||
| "updated_by", | ||
| "type_id", | ||
| "description_html", |
There was a problem hiding this comment.
Ensure description_html is properly sanitized before including in the response.
To prevent potential XSS vulnerabilities, verify that description_html is sanitized before being sent to the client. This helps protect against any malicious content that may have been injected.
| onClick={() => | ||
| data?.id && data && moveIssue(workspaceSlug.toString(), data?.id, data as TWorkspaceDraftIssue) | ||
| } |
There was a problem hiding this comment.
Add Error Handling to the moveIssue Operation
Currently, the moveIssue function call lacks error handling, which could result in unhandled exceptions and a poor user experience if the operation fails.
Wrap the moveIssue function call in a try-catch block to handle potential errors and provide feedback to the user:
onClick={async () => {
setIsMovingIssue(true);
try {
if (data?.id && data) {
await moveIssue(workspaceSlug.toString(), data.id, data as TWorkspaceDraftIssue);
}
} catch (error) {
setToast({
type: TOAST_TYPE.ERROR,
title: "Error!",
message: "Failed to move the issue to the project. Please try again.",
});
} finally {
setIsMovingIssue(false);
}
}}Committable suggestion was skipped due to low confidence.
| loading={isSubmitting} | ||
| onClick={() => |
There was a problem hiding this comment.
Use Dedicated Loading State for the "Add to project" Button
The loading prop for the "Add to project" button uses isSubmitting, which is associated with form submission. This may not accurately reflect the loading state of the moveIssue action, potentially causing confusion.
Introduce a separate loading state for the moveIssue action to ensure accurate feedback to the user:
+const [isMovingIssue, setIsMovingIssue] = useState(false);
...
<Button
variant="primary"
type="button"
size="sm"
- loading={isSubmitting}
+ loading={isMovingIssue}
onClick={async () => {
+ setIsMovingIssue(true);
if (data?.id && data) {
- moveIssue(workspaceSlug.toString(), data.id, data as TWorkspaceDraftIssue);
+ await moveIssue(workspaceSlug.toString(), data.id, data as TWorkspaceDraftIssue);
}
+ setIsMovingIssue(false);
}}
>
Add to project
</Button>Committable suggestion was skipped due to low confidence.
| {moveToIssue && ( | ||
| <Button | ||
| variant="primary" | ||
| type="button" | ||
| size="sm" | ||
| loading={isSubmitting} | ||
| onClick={() => | ||
| data?.id && data && moveIssue(workspaceSlug.toString(), data?.id, data as TWorkspaceDraftIssue) | ||
| } | ||
| > | ||
| Add to project | ||
| </Button> | ||
| )} |
There was a problem hiding this comment.
Ensure Type Safety When Casting data to TWorkspaceDraftIssue
In the onClick handler for the "Add to project" button, data is cast to TWorkspaceDraftIssue without proper validation. Since data is of type Partial<TIssue>, directly casting it may lead to runtime errors if required fields are missing. It's important to ensure that data conforms to TWorkspaceDraftIssue before passing it to moveIssue.
To address this, explicitly construct a TWorkspaceDraftIssue object with the necessary fields:
onClick={() => {
if (data?.id && data) {
- moveIssue(workspaceSlug.toString(), data.id, data as TWorkspaceDraftIssue);
+ const draftIssue: TWorkspaceDraftIssue = {
+ id: data.id,
+ // Add all required fields here, ensuring they come from 'data' and are not undefined
+ // For example:
+ name: data.name ?? '',
+ description_html: data.description_html ?? '<p></p>',
+ // Include other necessary fields
+ };
+ moveIssue(workspaceSlug.toString(), data.id, draftIssue);
}
}}Committable suggestion was skipped due to low confidence.
3 participants
Summary by CodeRabbit
New Features
moveToIssuefor improved functionality in issue modal components.Bug Fixes
Documentation