[WEB-2724] fix: custom properties issue while moving to project #6090
Conversation
mathalav55
requested review from
SatishGandham,
sriramveeraghanta and
rahulramesha
WalkthroughThe changes introduce a new function, Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Warning Rate limit exceeded@mathalav55 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 1 minutes and 3 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 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: 2
🧹 Outside diff range and nitpick comments (1)
web/core/components/issues/issue-modal/form.tsx (1)
239-254: Consider implementing a transaction-like patternThe current implementation performs two separate operations (property update and issue move) that should ideally succeed or fail together.
Consider:
- Implementing a higher-level service method that wraps both operations in a single transaction-like pattern
- Adding compensation logic to rollback changes if one operation fails
- Moving this logic to a dedicated service/hook for better reusability across the application
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
web/core/components/issues/issue-modal/form.tsx(3 hunks)
🔇 Additional comments (1)
web/core/components/issues/issue-modal/form.tsx (1)
116-116: LGTM: Proper hook integration for property handling
The addition of handleCreateUpdatePropertyValues to the destructured hooks aligns with the PR's objective of managing custom properties during draft-to-project transitions.
| } as TWorkspaceDraftIssue); | ||
| } | ||
| }} | ||
| onClick={handleMoveToProjects} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update button to reflect loading state
The button should display the loading state during the move operation.
- onClick={handleMoveToProjects}
+ onClick={handleMoveToProjects}
+ loading={isMoving} // Add this prop after adding the state
+ disabled={isMoving} // Prevent multiple clicksCommittable suggestion skipped: line range outside the PR's diff.
| const handleMoveToProjects = async () => { | ||
| if( !data?.id ||!data?.project_id || !data) return | ||
| await handleCreateUpdatePropertyValues({ | ||
| issueId: data.id, | ||
| issueTypeId: data.type_id, | ||
| projectId: data.project_id, | ||
| workspaceSlug: workspaceSlug.toString(), | ||
| isDraft: true | ||
| }) | ||
|
|
||
| moveIssue(workspaceSlug.toString(), data.id, { | ||
| ...data, | ||
| ...getValues(), | ||
| } as TWorkspaceDraftIssue); | ||
|
|
||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and user feedback
While the function correctly handles the core operations, it needs better error handling and user feedback:
- Error handling for both operations
- Loading state management
- Success/error notifications
Consider this implementation:
const handleMoveToProjects = async () => {
if (!data?.id || !data?.project_id || !data) return;
+ try {
+ setIsMoving(true); // Add this state
await handleCreateUpdatePropertyValues({
issueId: data.id,
issueTypeId: data.type_id,
projectId: data.project_id,
workspaceSlug: workspaceSlug.toString(),
isDraft: true
});
await moveIssue(workspaceSlug.toString(), data.id, {
...data,
...getValues(),
} as TWorkspaceDraftIssue);
+ setToast({
+ type: TOAST_TYPE.SUCCESS,
+ title: "Success!",
+ message: "Issue successfully moved to project.",
+ });
+ } catch (error) {
+ setToast({
+ type: TOAST_TYPE.ERROR,
+ title: "Error!",
+ message: "Failed to move issue to project. Please try again.",
+ });
+ } finally {
+ setIsMoving(false); // Add this state
+ }
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
web/core/components/issues/issue-modal/form.tsx (1)
513-513: Consider separate loading state for move operationThe button's loading state is currently tied to the form's
isSubmittingstate. Since moving to project is a separate operation, it should have its own loading state.Consider this change:
- loading={isSubmitting} + loading={isMovingToProject} onClick={handleMoveToProjects}Add a new state variable at the component level:
const [isMovingToProject, setIsMovingToProject] = useState(false);Update the loading state in
handleMoveToProjects:setIsMovingToProject(true); try { // ... existing code ... } finally { setIsMovingToProject(false); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
web/core/components/issues/issue-modal/form.tsx(3 hunks)
🔇 Additional comments (2)
web/core/components/issues/issue-modal/form.tsx (2)
116-116: LGTM: Hook destructuring includes necessary property handler
The addition of handleCreateUpdatePropertyValues from useIssueModal hook is appropriate for managing property updates during draft-to-project transitions.
241-247: Verify property update implementation
Let's verify that handleCreateUpdatePropertyValues correctly updates all necessary properties during the move operation.
| const handleMoveToProjects = async () => { | ||
| if( !data?.id ||!data?.project_id || !data) return | ||
| await handleCreateUpdatePropertyValues({ | ||
| issueId: data.id, | ||
| issueTypeId: data.type_id, | ||
| projectId: data.project_id, | ||
| workspaceSlug: workspaceSlug.toString(), | ||
| isDraft: true | ||
| }) | ||
|
|
||
| moveIssue(workspaceSlug.toString(), data.id, { | ||
| ...data, | ||
| ...getValues(), | ||
| } as TWorkspaceDraftIssue); | ||
|
|
||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling and type safety in handleMoveToProjects
While the function implements the core functionality, it could benefit from several improvements:
- The early return condition should be more explicit
- Add error handling for async operations
- Add loading state management
- Improve type safety for workspaceSlug
Consider this implementation:
- const handleMoveToProjects = async () => {
- if( !data?.id ||!data?.project_id || !data) return
- await handleCreateUpdatePropertyValues({
- issueId: data.id,
- issueTypeId: data.type_id,
- projectId: data.project_id,
- workspaceSlug: workspaceSlug.toString(),
- isDraft: true
- })
-
- moveIssue(workspaceSlug.toString(), data.id, {
- ...data,
- ...getValues(),
- } as TWorkspaceDraftIssue);
- }
+ const handleMoveToProjects = async () => {
+ try {
+ // Early return with explicit conditions
+ if (!data?.id || !data?.project_id || !workspaceSlug) {
+ setToast({
+ type: TOAST_TYPE.ERROR,
+ title: "Error",
+ message: "Missing required data for moving to project",
+ });
+ return;
+ }
+
+ // Set loading state
+ setValue("isSubmitting", true);
+
+ // Update property values
+ await handleCreateUpdatePropertyValues({
+ issueId: data.id,
+ issueTypeId: data.type_id,
+ projectId: data.project_id,
+ workspaceSlug: workspaceSlug.toString(),
+ isDraft: true
+ });
+
+ // Move the issue
+ await moveIssue(workspaceSlug.toString(), data.id, {
+ ...data,
+ ...getValues(),
+ } as TWorkspaceDraftIssue);
+
+ setToast({
+ type: TOAST_TYPE.SUCCESS,
+ title: "Success",
+ message: "Issue moved to project successfully",
+ });
+ } catch (error) {
+ setToast({
+ type: TOAST_TYPE.ERROR,
+ title: "Error",
+ message: "Failed to move issue to project",
+ });
+ console.error("Error moving issue to project:", error);
+ } finally {
+ setValue("isSubmitting", false);
+ }
+ };Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
web/core/components/issues/issue-modal/form.tsx(4 hunks)
🔇 Additional comments (3)
web/core/components/issues/issue-modal/form.tsx (3)
104-104: LGTM: State variable for tracking loading state
The addition of isMoving state variable is appropriate for managing the loading state during the move operation.
117-122: LGTM: Destructured hook methods
The destructuring includes handleCreateUpdatePropertyValues which is essential for the move operation.
527-529: LGTM: Button state management
The button correctly implements loading and disabled states to prevent multiple clicks.
| const handleMoveToProjects = async () => { | ||
| if (!data?.id || !data?.project_id || !data) return; | ||
| setIsMoving(true); | ||
| try { | ||
| await handleCreateUpdatePropertyValues({ | ||
| issueId: data.id, | ||
| issueTypeId: data.type_id, | ||
| projectId: data.project_id, | ||
| workspaceSlug: workspaceSlug.toString(), | ||
| isDraft: true, | ||
| }); | ||
|
|
||
| await moveIssue(workspaceSlug.toString(), data.id, { | ||
| ...data, | ||
| ...getValues(), | ||
| } as TWorkspaceDraftIssue); | ||
| } catch (error) { | ||
| setToast({ | ||
| type: TOAST_TYPE.ERROR, | ||
| title: "Error!", | ||
| message: "Failed to move issue to project. Please try again.", | ||
| }); | ||
| } finally { | ||
| setIsMoving(false); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Add validation for workspaceSlug
The function should validate workspaceSlug before using it in API calls.
Apply this diff to add the validation:
const handleMoveToProjects = async () => {
- if (!data?.id || !data?.project_id || !data) return;
+ if (!data?.id || !data?.project_id || !data || !workspaceSlug) {
+ setToast({
+ type: TOAST_TYPE.ERROR,
+ title: "Error!",
+ message: "Missing required data for moving to project",
+ });
+ return;
+ }
setIsMoving(true);📝 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.
| const handleMoveToProjects = async () => { | |
| if (!data?.id || !data?.project_id || !data) return; | |
| setIsMoving(true); | |
| try { | |
| await handleCreateUpdatePropertyValues({ | |
| issueId: data.id, | |
| issueTypeId: data.type_id, | |
| projectId: data.project_id, | |
| workspaceSlug: workspaceSlug.toString(), | |
| isDraft: true, | |
| }); | |
| await moveIssue(workspaceSlug.toString(), data.id, { | |
| ...data, | |
| ...getValues(), | |
| } as TWorkspaceDraftIssue); | |
| } catch (error) { | |
| setToast({ | |
| type: TOAST_TYPE.ERROR, | |
| title: "Error!", | |
| message: "Failed to move issue to project. Please try again.", | |
| }); | |
| } finally { | |
| setIsMoving(false); | |
| } | |
| }; | |
| const handleMoveToProjects = async () => { | |
| if (!data?.id || !data?.project_id || !data || !workspaceSlug) { | |
| setToast({ | |
| type: TOAST_TYPE.ERROR, | |
| title: "Error!", | |
| message: "Missing required data for moving to project", | |
| }); | |
| return; | |
| } | |
| setIsMoving(true); | |
| try { | |
| await handleCreateUpdatePropertyValues({ | |
| issueId: data.id, | |
| issueTypeId: data.type_id, | |
| projectId: data.project_id, | |
| workspaceSlug: workspaceSlug.toString(), | |
| isDraft: true, | |
| }); | |
| await moveIssue(workspaceSlug.toString(), data.id, { | |
| ...data, | |
| ...getValues(), | |
| } as TWorkspaceDraftIssue); | |
| } catch (error) { | |
| setToast({ | |
| type: TOAST_TYPE.ERROR, | |
| title: "Error!", | |
| message: "Failed to move issue to project. Please try again.", | |
| }); | |
| } finally { | |
| setIsMoving(false); | |
| } | |
| }; |
Problem Statement
Missing properties while moving drafts to projects.
Solution
All properties will be updated while moving to projects without explicitly clicking on the update.
References
WEB-2724
Summary by CodeRabbit
New Features
Bug Fixes
Enhancements