[WEB-2629] fix: workspace draft delete and move mutation #5822
Conversation
WalkthroughThe changes in this pull request involve the introduction of 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 (
|
![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 and nitpick comments (2)
web/core/store/issue/workspace-draft/issue.store.ts (2)
302-305: Improved implementation for removing issues.The new implementation is more explicit and easier to understand. It correctly removes the issue from both
issueMapIdsandissuesMap.Consider combining the operations to reduce the number of property accesses:
-this.issueMapIds[workspaceSlug] = (this.issueMapIds[workspaceSlug] || []).filter((id) => id !== issueId); -delete this.issuesMap[issueId]; +const workspaceIssueIds = this.issueMapIds[workspaceSlug]; +if (workspaceIssueIds) { + this.issueMapIds[workspaceSlug] = workspaceIssueIds.filter((id) => id !== issueId); + delete this.issuesMap[issueId]; +}This change ensures that we only perform the delete operation if the workspace actually exists in
issueMapIds, potentially avoiding unnecessary operations.
Line range hint
1-424: Summary of changes and their impactThe modifications to
deleteIssueandmoveIssuemethods improve the clarity of the code by using more explicit operations to remove issues fromissueMapIdsandissuesMap. These changes align with the PR objectives of fixing the mutation process for deleting and moving workspace draft issues.However, there are two points that require attention:
- The
deleteIssuemethod could be optimized to reduce property accesses.- The
moveIssuemethod's implementation is identical todeleteIssue, which may not be the intended behavior for a move operation.These changes should improve the overall functionality and maintainability of the workspace draft issue management. Once the
moveIssuebehavior is clarified and the suggested optimizations are considered, this update will significantly contribute to resolving the issues mentioned in [WEB-2629].Consider adding unit tests for these methods to ensure their correct behavior and prevent future regressions. This is especially important given the critical nature of these operations in managing workspace draft issues.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- web/core/components/issues/issue-layouts/filters/applied-filters/roots/workspace-draft-root.tsx (0 hunks)
- web/core/store/issue/workspace-draft/issue.store.ts (2 hunks)
💤 Files with no reviewable changes (1)
- web/core/components/issues/issue-layouts/filters/applied-filters/roots/workspace-draft-root.tsx
🧰 Additional context used
🔇 Additional comments (1)
web/core/store/issue/workspace-draft/issue.store.ts (1)
329-332:⚠️ Potential issueClarify the intended behavior of
moveIssue.The current implementation of
moveIssueis identical todeleteIssue, removing the issue entirely fromissueMapIdsandissuesMap. However, the method name suggests that the issue should be moved, not deleted.Please clarify if this is the intended behavior. If not, consider updating the implementation to reflect a move operation, such as updating the issue's position or associated data instead of removing it completely.
To verify the intended behavior, we can check the API implementation:
This will help us understand if the backend treats move and delete operations differently, which should inform our frontend implementation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
web/core/components/issues/workspace-draft/root.tsx (2)
32-34: LGTM: New hook usage implemented correctly.The
useWorkspaceIssuePropertieshook is correctly implemented with theworkspaceSlugprop. The comment provides clarity on its purpose.Consider updating the comment to be more specific:
-//swr hook for fetching issue properties +// Hook for fetching workspace issue propertiesThis change would more accurately describe the hook's specific functionality.
60-62: LGTM: Updated rendering logic for WorkspaceDraftIssuesLoader.The condition for rendering the loader has been appropriately updated to display only during the initial loading state. This change improves the component's behavior and user experience.
For consistency with the previous condition, consider using the same comparison operator:
-if (loader === "init-loader") { +if (loader === "init-loader") {This change maintains a consistent coding style throughout the component.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- web/core/components/issues/workspace-draft/root.tsx (3 hunks)
🧰 Additional context used
🔇 Additional comments (3)
web/core/components/issues/workspace-draft/root.tsx (3)
15-15: LGTM: New hook import added.The import for
useWorkspaceIssuePropertiesis correctly placed and seems relevant to the component's functionality.
58-59: LGTM: Simplified rendering logic for WorkspaceDraftEmptyState.The condition for rendering the empty state has been streamlined, making it more readable and straightforward. This change correctly handles the case when there are no issues to display.
Line range hint
1-96: Overall assessment: Changes improve component functionality and readability.The modifications to
WorkspaceDraftIssuesRootalign well with the PR objectives. The introduction of theuseWorkspaceIssuePropertieshook and the streamlined rendering logic for empty states and loaders enhance the component's functionality and readability. The changes are well-implemented and no major issues were identified.
SatishGandham
deleted the
fix-workspace-draft-delete-and-move-mutation
branch
Changes:
This PR includes following changes:
Reference:
[WEB-2629]
Summary by CodeRabbit
New Features
WorkspaceDraftIssuesRootcomponent for improved management of workspace draft issues.useWorkspaceIssueProperties, to fetch issue properties based on the workspace.Bug Fixes
Refactor