[WEB-2625] chore: workspace favorite and draft improvement #5855
Conversation
…plane/plane into chore-app-sidebar-item-improvements
WalkthroughThe changes in this pull request introduce a new read-only field, Changes
Possibly related PRs
Suggested labels
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: 2
🧹 Outside diff range and nitpick comments (6)
web/core/components/workspace/sidebar/user-menu.tsx (2)
53-54: LGTM: Added draftIssueCount extraction.This change correctly extracts the draft issue count for the current workspace, which is essential for the new functionality to hide empty categories in the sidebar.
Consider adding a default value to handle cases where
workspaceUserInfo[workspaceSlug.toString()]is undefined:const draftIssueCount = workspaceUserInfo[workspaceSlug.toString()]?.draft_issue_count ?? 0;This ensures
draftIssueCountis always a number, simplifying the condition check later.
Line range hint
61-88: LGTM: Implemented conditional rendering for "drafts" menu item.This change successfully implements the logic to hide the "Draft" option when there are no draft issues, which aligns with the PR objectives.
To improve code readability, consider extracting the condition into a separate variable:
{SIDEBAR_USER_MENU_ITEMS.map((link) => { const shouldRenderDrafts = link.key !== "drafts" || draftIssueCount > 0; if (!shouldRenderDrafts) return null; return ( allowPermissions(link.access, EUserPermissionsLevel.WORKSPACE, workspaceSlug.toString()) && ( // ... rest of the code ) ); })}This separation of concerns makes the condition more explicit and easier to understand at a glance.
web/app/[workspaceSlug]/(projects)/sidebar.tsx (2)
30-30: LGTM: Handling favorites stateThe addition of
groupedFavoritesandisFavoriteEmptyis well-implemented. The use of theuseFavoritehook andisEmptyfunction provides a robust way to manage and check the state of favorites.Consider memoizing
isFavoriteEmptyifgroupedFavoritesdoesn't change frequently:const isFavoriteEmpty = useMemo(() => isEmpty(groupedFavorites), [groupedFavorites]);This optimization could prevent unnecessary re-renders if the component re-renders for reasons unrelated to favorites.
Also applies to: 54-55
99-99: LGTM: Improved rendering condition for SidebarFavoritesMenuThe modified rendering condition for
SidebarFavoritesMenucorrectly implements the PR objective. It ensures that the favorites menu is only displayed when there are favorites and the user has the necessary permissions.For improved readability, consider extracting the condition into a descriptive variable:
const shouldRenderFavoritesMenu = canPerformWorkspaceMemberActions && !isFavoriteEmpty; {shouldRenderFavoritesMenu && <SidebarFavoritesMenu />}This change would make the code more self-documenting and easier to understand at a glance.
packages/types/src/workspace.d.ts (1)
94-94: LGTM! Consider adding JSDoc comment for clarity.The addition of
draft_issue_countto theIWorkspaceMemberMeinterface is appropriate and aligns with the PR objectives. The property type (number) is suitable for representing a count.For improved clarity, consider adding a JSDoc comment to describe the purpose of this property. For example:
/** The number of draft issues for the workspace member */ draft_issue_count: number;apiserver/plane/app/serializers/workspace.py (1)
68-68: LGTM! Consider adding a docstring for clarity.The addition of the
draft_issue_countfield as a read-only integer is a good approach to expose this information through the API. This aligns well with the PR objective of improving the display of draft information.To improve code documentation, consider adding a brief docstring to explain the purpose of this field. For example:
class WorkspaceMemberMeSerializer(BaseSerializer): draft_issue_count = serializers.IntegerField(read_only=True) """ The number of draft issues associated with the workspace member. This field is read-only and calculated on the server. """This addition would help other developers understand the purpose and behavior of the field without needing to look at other parts of the codebase.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- apiserver/plane/app/serializers/workspace.py (1 hunks)
- apiserver/plane/app/views/workspace/member.py (3 hunks)
- packages/types/src/workspace.d.ts (1 hunks)
- web/app/[workspaceSlug]/(projects)/sidebar.tsx (4 hunks)
- web/core/components/workspace/sidebar/user-menu.tsx (3 hunks)
- web/core/store/issue/workspace-draft/issue.store.ts (4 hunks)
🧰 Additional context used
🔇 Additional comments (9)
web/core/components/workspace/sidebar/user-menu.tsx (2)
27-27: LGTM: Added workspaceUserInfo to useUserPermissions hook.This change allows access to workspace-specific user information, which is necessary for the new functionality to hide empty categories in the sidebar.
Line range hint
1-92: Overall assessment: Changes successfully implement the PR objectives.The modifications to the
SidebarUserMenucomponent effectively implement the requirement to hide the "Draft" option when there are no draft issues. The code is well-structured and consistent with React and MobX best practices.A few minor suggestions have been provided to further improve code readability and robustness. These changes enhance the user experience by removing empty categories from the sidebar, as intended.
web/app/[workspaceSlug]/(projects)/sidebar.tsx (3)
2-2: LGTM: ImportingisEmptyfrom lodashThe addition of
isEmptyfrom lodash is a good choice for checking if an object or array is empty. It's more robust than simple length checks and handles various data types consistently.
20-20: LGTM: ImportinguseFavoritehookThe addition of the
useFavoritehook import is appropriate and consistent with the existing import structure. This custom hook will likely provide the necessary state and actions for managing favorites in the sidebar.
Line range hint
1-107: Overall assessment: Well-implemented changesThe modifications to the
AppSidebarcomponent successfully implement the PR objective of not showing the "Favorite" option when there are no items in that category. The code is well-structured, and the changes are implemented correctly.Key points:
- Proper use of
isEmptyfrom lodash for robust empty checks.- Correct implementation of the
useFavoritehook.- Improved rendering condition for the
SidebarFavoritesMenu.The suggestions provided for memoization and readability are minor optimizations that could further enhance the code quality.
packages/types/src/workspace.d.ts (1)
Line range hint
1-94: Summary: Appropriate type definition added for draft issue countThe addition of the
draft_issue_countproperty to theIWorkspaceMemberMeinterface is a focused and appropriate change. It aligns well with the PR objectives of improving workspace favorite and draft functionality. This change enhances the type definitions to support the new feature for tracking draft issue counts.The modification is minimal and doesn't affect other parts of the file, maintaining the existing structure and consistency of the type definitions.
apiserver/plane/app/serializers/workspace.py (1)
68-68: Verify draft issue count calculation and consider performance implications.While the addition of the
draft_issue_countfield is straightforward, it's important to ensure that the underlying implementation is correct and efficient.Please run the following script to check for the implementation of the draft issue count calculation:
Consider the following points:
- Ensure that the calculation of
draft_issue_countis efficient, especially if it's being computed for multiple workspace members simultaneously.- If the calculation is expensive, consider implementing caching mechanisms or denormalization strategies to improve performance.
- Update any relevant API documentation to reflect this new field in the serializer output.
✅ Verification successful
Re-run verification scripts using file extensions instead of file types.
The previous scripts failed to recognize the
pythonfile type. Please use file extensions to search for the draft issue count implementation.
Re-run verification scripts with broader search patterns to locate
draft_issue_countimplementation.The previous search did not yield results. Please use the following scripts to search for any occurrences of
draft_issue_countand related terms to identify its implementation.
draft_issue_countis correctly implemented and integrated.The
draft_issue_countfield is calculated using ORM annotations inapiserver/plane/app/views/workspace/member.py, ensuring it is accurately reflected in the serializer.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Look for the implementation of draft issue count calculation # Search for functions or methods that might be calculating the draft issue count rg -t python -g '!*/migrations/*' 'def .*draft.*issue.*count' ./apiserver # Search for queries that might be related to counting draft issues rg -t python -g '!*/migrations/*' 'filter.*draft.*count\(' ./apiserverLength of output: 210
Script:
#!/bin/bash # Description: Look for the implementation of draft issue count calculation using file extensions # Search for functions or methods that might be calculating the draft issue count rg -g '*.py' 'def .*draft.*issue.*count' ./apiserver # Search for queries that might be related to counting draft issues rg -g '*.py' 'filter.*draft.*count\(' ./apiserverLength of output: 106
Script:
#!/bin/bash # Description: Search for any occurrences of 'draft_issue_count' and related terms # Search for the definition or usage of 'draft_issue_count' rg -g '*.py' 'draft_issue_count' ./apiserver # Search for related terms that might be involved in calculating draft issue counts rg -g '*.py' 'draft_issues' ./apiserver # Search for ORM annotations or aggregations related to draft issues rg -g '*.py' 'annotate.*draft' ./apiserverLength of output: 1789
web/core/store/issue/workspace-draft/issue.store.ts (2)
269-270: Correctly incrementingdraft_issue_countincreateIssueThe
updateWorkspaceUserDraftIssueCountmethod is appropriately called to increment thedraft_issue_countwhen a new draft issue is created.
322-323: Verify decrementingdraft_issue_countindeleteIssueandmoveIssueIn both
deleteIssueandmoveIssue, thedraft_issue_countis decremented by 1. Ensure that this decrement is appropriate in all cases, and that the issue being deleted or moved was indeed counted towards the draft issues for the workspace. This helps maintain accurate counts.Also applies to: 351-352
| draft_issue_count = ( | ||
| DraftIssue.objects.filter( | ||
| created_by=request.user, | ||
| workspace_id=OuterRef("workspace_id"), | ||
| ) | ||
| .values("workspace_id") | ||
| .annotate(count=Count("id")) | ||
| .values("count") | ||
| ) | ||
|
|
||
| workspace_member = ( | ||
| WorkspaceMember.objects.filter( | ||
| member=request.user, workspace__slug=slug, is_active=True | ||
| ) | ||
| .annotate( | ||
| draft_issue_count=Coalesce( | ||
| Subquery(draft_issue_count, output_field=IntegerField()), 0 | ||
| ) | ||
| ) | ||
| .first() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify the query by removing unnecessary Subquery and OuterRef
Since you are fetching a single WorkspaceMember object for the current user and workspace, using a Subquery and OuterRef adds unnecessary complexity. You can simplify the code by directly querying the count of DraftIssue objects and assigning it to the WorkspaceMember instance.
Apply this diff to simplify the code:
def get(self, request, slug):
- draft_issue_count = (
- DraftIssue.objects.filter(
- created_by=request.user,
- workspace_id=OuterRef("workspace_id"),
- )
- .values("workspace_id")
- .annotate(count=Count("id"))
- .values("count")
- )
-
- workspace_member = (
- WorkspaceMember.objects.filter(
- member=request.user, workspace__slug=slug, is_active=True
- )
- .annotate(
- draft_issue_count=Coalesce(
- Subquery(draft_issue_count, output_field=IntegerField()), 0
- )
- )
- .first()
- )
+ # Retrieve the workspace member
+ workspace_member = WorkspaceMember.objects.get(
+ member=request.user, workspace__slug=slug, is_active=True
+ )
+
+ # Count the draft issues
+ draft_issue_count = DraftIssue.objects.filter(
+ created_by=request.user,
+ workspace__slug=slug,
+ ).count()
+
+ # Assign the count to the instance
+ workspace_member.draft_issue_count = draft_issue_count
+
serializer = WorkspaceMemberMeSerializer(workspace_member)
return Response(serializer.data, status=status.HTTP_200_OK)Note: Ensure that the WorkspaceMemberMeSerializer is updated to handle the draft_issue_count attribute appropriately. You might need to modify the serializer to include this field if it's not already defined.
📝 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.
| draft_issue_count = ( | |
| DraftIssue.objects.filter( | |
| created_by=request.user, | |
| workspace_id=OuterRef("workspace_id"), | |
| ) | |
| .values("workspace_id") | |
| .annotate(count=Count("id")) | |
| .values("count") | |
| ) | |
| workspace_member = ( | |
| WorkspaceMember.objects.filter( | |
| member=request.user, workspace__slug=slug, is_active=True | |
| ) | |
| .annotate( | |
| draft_issue_count=Coalesce( | |
| Subquery(draft_issue_count, output_field=IntegerField()), 0 | |
| ) | |
| ) | |
| .first() | |
| # Retrieve the workspace member | |
| workspace_member = WorkspaceMember.objects.get( | |
| member=request.user, workspace__slug=slug, is_active=True | |
| ) | |
| # Count the draft issues | |
| draft_issue_count = DraftIssue.objects.filter( | |
| created_by=request.user, | |
| workspace__slug=slug, | |
| ).count() | |
| # Assign the count to the instance | |
| workspace_member.draft_issue_count = draft_issue_count | |
| serializer = WorkspaceMemberMeSerializer(workspace_member) | |
| return Response(serializer.data, status=status.HTTP_200_OK) |
| private updateWorkspaceUserDraftIssueCount(workspaceSlug: string, increment: number) { | ||
| const workspaceUserInfo = this.issueStore.rootStore.user.permission.workspaceUserInfo; | ||
| const currentCount = workspaceUserInfo[workspaceSlug]?.draft_issue_count ?? 0; | ||
|
|
||
| set(workspaceUserInfo, [workspaceSlug, "draft_issue_count"], currentCount + increment); | ||
| } | ||
|
|
There was a problem hiding this comment.
Ensure draft_issue_count does not become negative
The updateWorkspaceUserDraftIssueCount method updates draft_issue_count by adding an increment, which could be negative. To prevent potential issues with negative counts, consider ensuring that draft_issue_count does not fall below zero.
Apply this diff to enforce a non-negative draft_issue_count:
private updateWorkspaceUserDraftIssueCount(workspaceSlug: string, increment: number) {
const workspaceUserInfo = this.issueStore.rootStore.user.permission.workspaceUserInfo;
const currentCount = workspaceUserInfo[workspaceSlug]?.draft_issue_count ?? 0;
+ const updatedCount = Math.max(0, currentCount + increment);
+ set(workspaceUserInfo, [workspaceSlug, "draft_issue_count"], updatedCount);
- set(workspaceUserInfo, [workspaceSlug, "draft_issue_count"], currentCount + increment);
}📝 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.
| private updateWorkspaceUserDraftIssueCount(workspaceSlug: string, increment: number) { | |
| const workspaceUserInfo = this.issueStore.rootStore.user.permission.workspaceUserInfo; | |
| const currentCount = workspaceUserInfo[workspaceSlug]?.draft_issue_count ?? 0; | |
| set(workspaceUserInfo, [workspaceSlug, "draft_issue_count"], currentCount + increment); | |
| } | |
| private updateWorkspaceUserDraftIssueCount(workspaceSlug: string, increment: number) { | |
| const workspaceUserInfo = this.issueStore.rootStore.user.permission.workspaceUserInfo; | |
| const currentCount = workspaceUserInfo[workspaceSlug]?.draft_issue_count ?? 0; | |
| const updatedCount = Math.max(0, currentCount + increment); | |
| set(workspaceUserInfo, [workspaceSlug, "draft_issue_count"], updatedCount); | |
| } |
Changes:
This PR includes following changes:
Reference:
[WEB-2625]
Summary by CodeRabbit
New Features
draft_issue_countfield to provide users with the count of draft issues associated with workspace members.AppSidebarandSidebarUserMenucomponents to conditionally display draft-related information based on the draft issue count.Bug Fixes
Documentation
draft_issue_countproperty for better clarity on workspace member data.