[WEB-2388] dev: workspace draft issues #5772
Conversation
WalkthroughThe pull request introduces significant refactoring and new features across various components related to workspace draft issues. Key changes include the enhancement of 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 (
|
…ane into dev-workspace-draft-issues
sriramveeraghanta
force-pushed
the
preview
branch
from
45880b3 to
d97ca68
Compare
| e.stopPropagation(); | ||
| }} | ||
| > | ||
| <WorkspaceDraftIssueQuickActions |
There was a problem hiding this comment.
Why is there two WorkspaceDraftIssueQuickActions
There was a problem hiding this comment.
This looks like a bug please check
There was a problem hiding this comment.
@rahulramesha We have that because we have to render in two different places based on the responsiveness
| @@ -428,7 +428,7 @@ export const SidebarProjectsListItem: React.FC<Props> = observer((props) => { | |||
| </div> | |||
| </CustomMenu.MenuItem> | |||
| )} | |||
| {isAuthorized && ( | |||
There was a problem hiding this comment.
Remove commented code if not required
| @@ -102,7 +102,7 @@ export const SidebarQuickActions = observer(() => { | |||
| <PenSquare className="size-4" /> | |||
| {!isSidebarCollapsed && <span className="text-sm font-medium">New issue</span>} | |||
| </button> | |||
| {!disabled && workspaceDraftIssue && ( | |||
| ) => Promise<TWorkspaceDraftIssue | undefined>; | ||
| } | ||
|
|
||
| export class WorkspaceDraftIssues implements IWorkspaceDraftIssues { |
There was a problem hiding this comment.
This should extend base issues store and use the methods within that for pagination, this looks like it is different from the pattern we have
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 33
🧹 Outside diff range and nitpick comments (55)
web/core/constants/workspace-drafts.ts (1)
1-1: Consider adding documentation for the enum.To improve code maintainability and make it easier for other developers to understand the purpose and usage of this enum, consider adding a brief documentation comment above the enum declaration. This could include a description of when each state is used in the pagination process.
Here's an example of how you might document this enum:
/** * Represents the different states of pagination for draft issues. * * INIT: Used when initializing the pagination, typically for the first page load. * NEXT: Used when moving to the next page of draft issues. * PREV: Used when moving to the previous page of draft issues. * CURRENT: Used to refresh or stay on the current page. */ export enum EDraftIssuePaginationType { // ... (enum members remain the same) }web/core/components/issues/issue-layouts/quick-action-dropdowns/index.ts (1)
7-7: LGTM! Consider grouping related exports.The addition of the new export for workspace draft quick actions is appropriate and consistent with the existing code style. This change enhances the modularity of the component's interface by making the workspace draft quick actions available through this index file.
For better code organization, consider grouping related exports together. You might want to move this new export next to the
draft-issueexport if they are closely related. This would improve readability and make it easier to maintain the file in the future.web/app/[workspaceSlug]/(projects)/drafts/layout.tsx (2)
6-6: LGTM: Component definition is clear and well-typed.The component is correctly defined as a default export using function declaration syntax. The prop type is appropriately defined using TypeScript.
Consider using a separate interface for the props to improve reusability and maintainability:
interface WorkspaceDraftLayoutProps { children: React.ReactNode; } export default function WorkspaceDraftLayout({ children }: WorkspaceDraftLayoutProps) { // ... }
7-13: LGTM: Component render logic is clean and well-structured.The component's render logic is clear and follows good practices:
- Use of a fragment to avoid unnecessary DOM nesting
- Logical structure with a header and content area
- Good separation of concerns with a specific
WorkspaceDraftHeaderFor consistency, consider using self-closing tags for components without children:
<AppHeader header={<WorkspaceDraftHeader />} />instead of:
<AppHeader header={<WorkspaceDraftHeader></WorkspaceDraftHeader>} />web/core/hooks/store/workspace-draft/use-workspace-draft-issue.ts (2)
7-12: LGTM: Well-implemented custom hook with good error handling.The
useWorkspaceDraftIssueshook is correctly implemented, following React's conventions and best practices. It properly handles the case where the context is undefined, which is a good defensive programming approach.Consider adding a type assertion for the context to improve type safety:
const context = useContext(StoreContext); if (context === undefined) throw new Error("useWorkspaceDraftIssues must be used within StoreProvider"); return (context as { issue: { workspaceDraftIssues: IWorkspaceDraftIssues } }).issue.workspaceDraftIssues;This change ensures that TypeScript knows the exact shape of the context object when it's defined, potentially catching errors earlier in the development process.
1-12: Great addition: Custom hook enhances code reusability and maintainability.The introduction of
useWorkspaceDraftIssuesis a positive addition to the codebase. This custom hook encapsulates the logic for accessing workspace draft issues from the MobX store, promoting code reuse and separation of concerns. Its implementation follows React best practices and includes proper error handling.As this hook is likely to be used across multiple components, consider the following to further improve its robustness and usefulness:
- Add unit tests to ensure the hook behaves correctly in various scenarios.
- Consider adding memoization if the returned data is complex or if you notice performance issues in components using this hook frequently.
- Document the hook's usage and any prerequisites (like required provider wrappers) in a comment above the function to aid other developers.
web/core/hooks/store/workspace-draft/use-workspace-draft-issue-filters.ts (1)
7-12: LGTM: Well-implemented custom hook with proper error handling.The
useWorkspaceDraftIssueFiltershook is correctly implemented, following React conventions and best practices. It properly accesses the context and includes error handling for incorrect usage.A minor suggestion for improvement:
Consider making the error message more specific by mentioning the
StoreProvider:- throw new Error("useWorkspaceDraftIssueFilters must be used within StoreProvider"); + throw new Error("useWorkspaceDraftIssueFilters must be used within a StoreProvider");This change makes it clearer which provider is required.
web/core/components/issues/workspace-draft/loader.tsx (1)
13-19: LGTM! Component structure is well-implemented, with a minor optimization suggestion.The JSX structure is clean and efficient. The use of Array constructor with map to render multiple
ListLoaderItemRowcomponents is a good approach.Consider using
Array.from()instead of[...Array(items)]for slightly better readability:- {[...Array(items)].map((_, index) => ( + {Array.from({ length: items }).map((_, index) => ( <ListLoaderItemRow key={index} /> ))}This change doesn't affect functionality but might be considered more idiomatic JavaScript.
web/app/[workspaceSlug]/(projects)/drafts/page.tsx (1)
8-25: LGTM: Component logic is well-structured. Consider adding type annotations.The component logic is clean, easy to understand, and follows React and Next.js best practices. The early return for handling the case when workspaceSlug is not available is a good practice.
Consider adding type annotations to improve type safety:
- const WorkspaceDraftPage = () => { + const WorkspaceDraftPage: React.FC = () => { // ... - const workspaceSlug = (routeWorkspaceSlug as string) || undefined; + const workspaceSlug = routeWorkspaceSlug ? String(routeWorkspaceSlug) : undefined;This change ensures that
workspaceSlugis always of typestring | undefined, improving type safety.web/core/components/issues/issue-layouts/list/list-view-types.d.ts (1)
12-12: LGTM! Consider adding a brief JSDoc comment.The addition of
handleMoveToIssuesmethod is consistent with the existing interface structure and naming conventions. It's correctly defined as optional and returns a Promise, which aligns well with other handler methods.Consider adding a brief JSDoc comment to describe the purpose of this method, for example:
/** Handles moving the issue to the main issues list */ handleMoveToIssues?: () => Promise<void>;This would improve the self-documentation of the interface.
packages/types/src/issues/base.d.ts (1)
Line range hint
28-28: LGTM with a minor suggestion: Consider a more specific nameThe
TIssuestype is well-defined as a union ofTGroupedIssuesandTSubGroupedIssues, allowing for flexibility in representing issues. However, the nameTIssuesmight be slightly ambiguous as it doesn't immediately convey the grouped nature of the type.Consider renaming to
TGroupedOrSubGroupedIssuesfor more clarity, although this is a minor suggestion and the current name is acceptable.packages/types/src/index.d.ts (1)
Line range hint
24-24: Consider addressing the TODO comment.There's an existing TODO comment on line 24 about removing an export after development. While not directly related to your current changes, it might be worth considering if this TODO can be addressed in this PR or if it should be tracked for future cleanup.
Would you like me to create a GitHub issue to track this TODO for future cleanup?
web/core/components/issues/workspace-draft/empty-state.tsx (2)
17-22: LGTM: CreateUpdateIssueModal is well-implemented with a minor suggestion.The modal component is correctly implemented with appropriate props. For improved type safety, consider using a boolean literal type for the
isDraftprop:isDraft={true as const}This ensures that the prop is always
truefor this usage, which aligns with the component's purpose of creating draft issues.
15-33: LGTM: Overall component structure is well-implemented with a minor suggestion.The component structure, including the use of Fragment and the EmptyState implementation, is well-done. For consistency and to avoid potential issues with automatic semicolon insertion, consider adding a semicolon after the component's closing curly brace:
export const WorkspaceDraftEmptyState: FC = () => { // ... component implementation ... };This follows a common style guide practice and can prevent subtle bugs in certain situations.
packages/types/src/workspace-draft-issues/base.d.ts (4)
3-29: LGTM! Consider using a more specific type for date fields.The
TWorkspaceDraftIssuetype is well-defined and comprehensive. It covers various aspects of a draft issue, including identifiers, state, priority, assignments, and timestamps.Consider using a more specific type for date fields (e.g.,
start_date,target_date,completed_at,created_at,updated_at) instead ofstring | undefined. You could useDate | string | undefinedto allow for both Date objects and string representations, improving type safety and flexibility.
31-44: LGTM! Consider refining theextra_statsfield.The
TWorkspaceDraftPaginationInfo<T>type is well-structured and covers essential pagination information. The use of a generic type parameterTallows for flexibility in the type of paginated results.Consider refining the
extra_statsfield. Currently, it's typed asstring | undefined, which is quite generic. If possible, define a more specific type or interface forextra_statsto provide better type safety and documentation of its expected structure.
46-49: LGTM! Consider future extensibility of query parameters.The
TWorkspaceDraftQueryParamstype provides basic pagination parameters, which is good for simple use cases.Consider the potential need for additional query parameters in the future. You might want to make this type more extensible by either:
- Adding an optional
additionalParamsfield of typeRecord<string, unknown>, or- Using a mapped type with optional properties for known parameters.
This would allow for easier addition of new query parameters without breaking existing code.
51-61: LGTM! Consider removing 'undefined' from the union type.The
TWorkspaceDraftIssueLoadertype effectively captures various loading states for draft issues, which is crucial for managing UI states and user feedback.Consider removing
undefinedfrom the union type. Havingundefinedas a possible value might lead to ambiguous states. If you need to represent a "not set" or "initial" state, it's often clearer to use an explicit string literal like"initial"or"not-set"instead ofundefined. This approach can help prevent potential bugs and improve type safety.web/app/[workspaceSlug]/(projects)/drafts/header.tsx (2)
19-28: Consider using a more descriptive variable name for authorization check.The permissions check is implemented correctly. However, to improve code readability, consider renaming
isAuthorizedUserto something more specific, such ascanCreateDraftIssue. This would make the purpose of the check more immediately clear to other developers.Here's a suggested change:
- const isAuthorizedUser = allowPermissions( + const canCreateDraftIssue = allowPermissions( [EUserPermissions.ADMIN, EUserPermissions.MEMBER], EUserPermissionsLevel.WORKSPACE );
38-61: Consider extracting the button's onClick handler to a separate function.The header rendering and button implementation look good overall. The conditional disabling of the button based on user permissions is a good security practice, and the responsive text is a nice touch for better mobile experience.
However, to improve code maintainability and make it easier to add potential logic in the future, consider extracting the button's onClick handler to a separate function.
Here's a suggested change:
+ const handleCreateDraftIssue = () => { + setIsDraftIssueModalOpen(true); + // Any additional logic can be added here in the future + }; // ... (rest of the component) <Button variant="primary" size="sm" className="items-center gap-1" - onClick={() => setIsDraftIssueModalOpen(true)} + onClick={handleCreateDraftIssue} - disabled={!isAuthorizedUser} + disabled={!canCreateDraftIssue} > Draft <span className="hidden sm:inline-block">issue</span> </Button>This change also incorporates the previously suggested variable name change for consistency.
web/core/components/workspace/sidebar/quick-actions.tsx (1)
Residual References Found After Code Removal
The removal of the last drafted issue button in
quick-actions.tsxhas left several references toworkspaceDraftIssueandremoveWorkspaceDraftIssueacross the codebase:
web/core/store/issue/root.store.tsweb/core/hooks/use-issues-actions.tsxweb/core/hooks/store/use-issues.tsweb/core/hooks/store/workspace-draft/use-workspace-draft-issue.tsweb/core/hooks/store/workspace-draft/use-workspace-draft-issue-filters.tsAction Items:
Cleanup Unused Code:
- Remove or update the unused variables and functions related to
workspaceDraftIssueandremoveWorkspaceDraftIssuein the aforementioned files.Verify Feature Deprecation:
- Ensure that the removal aligns with the PR's objectives and that this feature is intended to be deprecated.
Update Documentation:
- If the feature is permanently removed, update any related documentation or user guides to reflect this change.
Run Tests:
- Execute relevant tests to confirm that the removal does not negatively impact other functionalities.
🔗 Analysis chain
Line range hint
105-130: Removal of last drafted issue button: Clarification neededThe code for rendering the last drafted issue button has been commented out. While this simplifies the component, it also removes functionality that might be valuable to users.
Could you please clarify the rationale behind removing this feature? Does it align with the PR objectives or is it part of a larger redesign?
If this feature is being permanently removed:
a. Consider cleaning up unused code, such as theworkspaceDraftIssuevariable and theremoveWorkspaceDraftIssuefunction.
b. Update any related documentation or user guides that might reference this feature.If this is a temporary change or part of a phased approach, consider adding a TODO comment explaining the plan for this functionality.
To ensure there are no lingering references to this removed feature, you can run the following command:
This will help identify any other places in the codebase that might need to be updated following this change.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for references to workspaceDraftIssue or removeWorkspaceDraftIssue rg 'workspaceDraftIssue|removeWorkspaceDraftIssue' --type typescript --type tsxLength of output: 116
Script:
#!/bin/bash # Search for references to workspaceDraftIssue or removeWorkspaceDraftIssue rg 'workspaceDraftIssue|removeWorkspaceDraftIssue' -t ts -t tsxLength of output: 93
Script:
#!/bin/bash # Search for references to workspaceDraftIssue or removeWorkspaceDraftIssue in .ts and .tsx files rg 'workspaceDraftIssue|removeWorkspaceDraftIssue' -g '*.ts' -g '*.tsx'Length of output: 2031
web/core/components/issues/issue-layouts/list/base-list-root.tsx (2)
28-29: LGTM! Consider adding comments for new store types.The addition of
EIssuesStoreType.ARCHIVEDandEIssuesStoreType.WORKSPACE_DRAFTtoListStoreTypeis appropriate and aligns with the PR objectives. This expansion allows for more versatile issue management.Consider adding brief comments explaining the purpose of these new store types to improve code readability and maintainability.
127-129: LGTM! Consider using object spread for clarity.The changes to the
handleCollapsedGroupsfunction improve code readability and type safety. The multi-line formatting of theupdateFilterscall and the explicit type annotation are good practices.For even better clarity, consider using object spread syntax:
updateFilters(projectId?.toString() ?? "", EIssueFilterType.KANBAN_FILTERS, { ...issuesFilter?.issueFilters?.kanbanFilters, group_by: collapsedGroups, } as TIssueKanbanFilters);This approach ensures that any existing properties in
kanbanFiltersare preserved while updatinggroup_by.web/core/constants/dashboard.ts (1)
332-339: LGTM: New "Drafts" menu item added correctly.The new "Drafts" menu item has been added to the
SIDEBAR_USER_MENU_ITEMSarray with appropriate properties. The structure is consistent with other menu items, and the access permissions are set correctly for ADMIN and MEMBER roles.Minor suggestion for consistency:
Consider updating the
hrefproperty to be consistent with other menu items that use template literals:- href: `/drafts`, + href: `/drafts`,This change doesn't affect functionality but maintains consistency with other menu items.
web/core/store/issue/root.store.ts (1)
117-119: LGTM: Class implementation updated to support workspace draft issues.The addition of
workspaceDraftIssuesFilterandworkspaceDraftIssuesproperties to theIssueRootStoreclass, along with their initialization in the constructor, properly implements the interface changes. This ensures that the store can handle workspace draft issues as intended.Consider grouping related properties and their initializations together for better code organization. For example, you could move the
workspaceDraftIssues*properties next to theworkspaceIssues*properties in both the class declaration and the constructor.Also applies to: 200-202
web/core/components/issues/issue-modal/form.tsx (1)
410-418: LGTM: Submit button text update for draft issuesThe submit button text now correctly includes "Create draft issue" when creating a new draft, which is consistent with the title changes and provides clear feedback to the user about the action they're taking.
Consider adjusting the indentation for better readability:
{data?.id ? isSubmitting ? "Updating" : "Update" : isSubmitting ? "Creating" - : isDraft - ? "Create draft issue" - : "Create"} + : isDraft + ? "Create draft issue" + : "Create"}This change aligns the indentation of the nested ternary operators, making the code structure more apparent.
web/core/components/issues/issue-layouts/properties/all-properties.tsx (3)
Line range hint
4-4: Address TODO comment: Add testsThere's a TODO comment about adding tests. It's important to ensure proper test coverage for the component. Consider implementing the required tests to improve the overall quality and maintainability of the code.
Line range hint
325-330: Remove commented-out codeThere's a commented-out
router.pushcall. If this code is no longer needed, it should be removed to keep the codebase clean and prevent confusion. If it's kept for future reference, consider adding a comment explaining why it's being preserved.
Line range hint
1-577: Consider component decompositionThe
IssuePropertiescomponent is quite large and complex, handling multiple properties and actions. Consider breaking it down into smaller, more focused components to improve readability, maintainability, and reusability. This could involve creating separate components for different sections of the issue properties or extracting some of the logic into custom hooks.web/core/components/workspace/sidebar/projects-list-item.tsx (1)
Line range hint
431-440: Remove commented code for better maintainabilityThe "Draft issues" menu item has been commented out. To improve code cleanliness and maintainability, it's recommended to remove commented-out code entirely rather than leaving it in the codebase. This aligns with the previous review comment as well.
Please remove the commented-out code block for the "Draft issues" menu item. If this functionality needs to be preserved for future reference, consider documenting it in a separate file or in the project's documentation.
- {/* {isAuthorized && ( - <CustomMenu.MenuItem> - <Link href={`/${workspaceSlug}/projects/${project?.id}/draft-issues/`}> - <div className="flex items-center justify-start gap-2"> - <PenSquare className="h-3.5 w-3.5 stroke-[1.5] text-custom-text-300" /> - <span>Draft issues</span> - </div> - </Link> - </CustomMenu.MenuItem> - )} */}apiserver/plane/utils/paginator.py (2)
Line range hint
391-391: Remove debug print statementThe print statement on line 391 should be removed before merging to production. Leaving debug statements in production code can lead to unnecessary console output and potential performance issues.
Remove the following line:
print(limit, "limit")
Line range hint
1-831: Summary of review findings
Several debug print statements were added throughout the file. These should be removed or replaced with proper logging before merging to production.
A change in the OffsetPaginator's slicing logic (lines 154-155) needs careful review, as it might lead to unexpected behavior.
No other significant issues were found in the changes.
Consider implementing a consistent logging strategy across the project to facilitate debugging without relying on print statements. This could involve:
- Setting up a logging configuration that can be easily adjusted for different environments (development, staging, production).
- Using appropriate log levels (DEBUG, INFO, WARNING, ERROR) throughout the code.
- Including relevant context in log messages to aid in troubleshooting.
Example:
import logging logger = logging.getLogger(__name__) class OffsetPaginator: def get_result(self, limit=1000, cursor=None): logger.debug(f"get_result called with limit={limit}, cursor={cursor}") # ... rest of the methodThis approach will make it easier to manage debug output and troubleshoot issues in various environments.
web/core/constants/empty-state.ts (2)
109-109: LGTM! Consider adding a more specific path.The addition of the
WORKSPACE_DRAFT_ISSUESenum and its corresponding entry inemptyStateDetailsis well-structured and consistent with the existing pattern. Good job!Consider making the
pathproperty more specific. Instead of/empty-state/workspace-draft/issue, you might want to use something like/empty-state/workspace/draft-issuesto maintain consistency with other workspace-related paths.Also applies to: 762-772
Line range hint
1-772: Consider refactoring for improved maintainability and consistency.While the current implementation works, there are a few areas where we could improve maintainability and consistency:
File length: This file is quite long. Consider splitting it into smaller, more focused files (e.g., one for enums, one for details).
Reduce duplication: There's some repetition in descriptions and titles. Consider using variables or functions to generate common parts of these strings.
String consistency: Standardize on either string literals or template literals for multi-line strings throughout the file.
Type safety: The use of
as constis good for type safety. Consider using TypeScript's satisfies operator for additional type checking.Here's a small example of how you might reduce duplication:
const createEmptyStateDetail = ( key: EmptyStateType, title: string, description: string, path: string, buttonText: string, access: EUserPermissions[] = [EUserPermissions.ADMIN, EUserPermissions.MEMBER] ): EmptyStateDetails => ({ key, title, description, path, primaryButton: { text: buttonText }, accessType: "workspace", access, }); // Usage: [EmptyStateType.WORKSPACE_DRAFT_ISSUES]: createEmptyStateDetail( EmptyStateType.WORKSPACE_DRAFT_ISSUES, "No Draft Issues Yet", "There are no draft issues in your workspace right now. Begin by adding your first one.", "/empty-state/workspace/draft-issues", "Create draft issue" ),This approach could help reduce repetition and make it easier to maintain consistent structures across different empty states.
web/core/components/issues/issue-layouts/filters/applied-filters/roots/workspace-draft-root.tsx (1)
18-18: Ensure the usage ofuseWorkspaceIssuePropertiesThe hook
useWorkspaceIssueProperties(workspaceSlug);is called but its return value is not captured. If the hook returns data needed for the component, consider capturing and utilizing it. If it's used solely for side effects, ensure that this is intentional.web/core/services/issue/workspace_draft.service.ts (1)
13-14: Specify a more precise type for thequeryparameterIn the
getIssuesmethod, thequeryparameter is currently typed asobject. This is too general and doesn't provide type safety. Consider specifying a more precise type, such asRecord<string, any>, or defining an interface that represents the expected query parameters.Apply this diff:
- query: object = {} + query: Record<string, any> = {}web/core/components/issues/workspace-draft/root.tsx (4)
28-30: Refactor to avoid repeating the same condition inuseSWRThe condition
workspaceSlug && issueIds.length <= 0is used twice in theuseSWRcall. Assigning it to a variable would improve readability and reduce repetition.Apply this diff to refactor:
+ const shouldFetchIssues = workspaceSlug && issueIds.length === 0; useSWR( - workspaceSlug && issueIds.length <= 0 ? `WORKSPACE_DRAFT_ISSUES_${workspaceSlug}` : null, - workspaceSlug && issueIds.length <= 0 ? async () => await fetchIssues(workspaceSlug, "init-loader") : null + shouldFetchIssues ? `WORKSPACE_DRAFT_ISSUES_${workspaceSlug}` : null, + shouldFetchIssues ? async () => await fetchIssues(workspaceSlug, "init-loader") : null );
38-42: Simplify conditions by usingissueIds.length === 0Since array lengths cannot be negative,
issueIds.length <= 0can be replaced withissueIds.length === 0for clarity.Apply this diff to simplify the conditions:
- if (loader === "init-loader" && issueIds.length <= 0) { + if (loader === "init-loader" && issueIds.length === 0) { ... - if (loader === "empty-state" && issueIds.length <= 0) return <WorkspaceDraftEmptyState />; + if (loader === "empty-state" && issueIds.length === 0) return <WorkspaceDraftEmptyState />;
51-53: Simplify condition:issueIds.length >= 0is always trueThe condition
issueIds.length >= 0will always be true because array lengths are never negative. You can simplify the ternary condition by removing this check.Apply this diff to simplify the condition:
- {loader === "pagination" && issueIds.length >= 0 ? ( + {loader === "pagination" ? (
24-24: Unused variableissuesMapThe
issuesMapvariable is extracted fromuseWorkspaceDraftIssues()but is not used within the component. Consider removing it if it's not needed to keep the code clean.Apply this diff to remove the unused variable:
- const { loader, paginationInfo, fetchIssues, issuesMap, issueIds } = useWorkspaceDraftIssues(); + const { loader, paginationInfo, fetchIssues, issueIds } = useWorkspaceDraftIssues();web/core/components/issues/workspace-draft/delete-modal.tsx (2)
65-87: Handle cases whenonSubmitis undefined.Since
onSubmitis optional, there should be a fallback or appropriate handling when it is not provided to prevent unexpected behavior.Apply this diff to handle the absence of
onSubmit:if (onSubmit) await onSubmit() .then(() => { // success handling }) .catch((errors) => { // error handling }) .finally(() => onClose()); +else { + onClose(); +}
98-101: Correct the modal content formatting.There's an unnecessary
{""}in the modal content which might result in extra spacing or unintended rendering.Apply this diff to clean up the content:
Are you sure you want to delete issue{" "} <span className="break-words font-medium text-custom-text-100">{projectDetails?.identifier}</span> - {""}? All of the data related to the issue will be permanently removed. This action cannot be undone. + ? All of the data related to the issue will be permanently removed. This action cannot be undone.web/core/components/issues/issue-modal/draft-issue-layout.tsx (2)
Line range hint
54-54: Fix the variable naming to prevent overwritingThe variable
usePathnameis being reassigned, which overwrites the imported hook and may cause unexpected behavior.Apply this diff to correct the variable name:
- const usePathname = usePathname(); + const pathname = usePathname();This change assigns the result of
usePathname()topathname, avoiding the conflict with the imported hook.
96-99: Simplify thenameassignment in the payloadThe current ternary expression can be simplified for better readability and maintainability.
Apply this diff to streamline the code:
const payload = { ...changesMade, - name: - changesMade?.name && changesMade?.name?.trim() !== "" - ? changesMade.name?.trim() - : "Untitled", + name: changesMade?.name?.trim() || "Untitled", project_id: projectId, };This uses the logical OR operator to assign
nameto the trimmed value or"Untitled"if the trimmed name is falsy.web/core/components/issues/workspace-draft/draft-issue-block.tsx (3)
36-36: Remove unnecessary space in classNameThere's an extra space at the beginning of the
classNamestring.Apply this change:
- <div id={`issue-${issue.id}`} className=" relative border-b border-b-custom-border-200 w-full cursor-pointer"> + <div id={`issue-${issue.id}`} className="relative border-b border-b-custom-border-200 w-full cursor-pointer">
70-73: Remove commented-out code for cleaner codebaseLines 70 and 72 contain commented-out properties. If these are no longer needed, consider removing them to keep the codebase clean.
100-102: Remove unused 'projectId' parameter in 'updateIssue' functionThe
projectIdparameter is unused in theupdateIssuefunction and can be removed for clarity.Apply this change:
- updateIssue={async (projectId, issueId, data) => { + updateIssue={async (issueId, data) => { await updateIssue(workspaceSlug, issueId, data); }}web/core/components/issues/workspace-draft/draft-issue-properties.tsx (4)
68-89: Reevaluate the necessity ofuseMemoforissueOperationsThe
issueOperationsobject is memoized usinguseMemo, but its dependencies includeissue, which may change frequently. This might negate the performance benefits of memoization.Consider defining
issueOperationswithoutuseMemoif memoization does not provide significant benefits:- const issueOperations = useMemo( - () => ({ ... }), - [workspaceSlug, issue, addCycleToIssue, addModulesToIssue] - ); + const issueOperations = { + ... // same operations + };
155-158: Review event handling inhandleEventPropagationThe
handleEventPropagationfunction both prevents the default action and stops propagation for all events. While stopping propagation might be necessary, preventing the default action could interfere with expected behaviors like form submissions or link navigation.Assess whether
e.preventDefault()is necessary, or if onlye.stopPropagation()suffices:const handleEventPropagation = (e: React.MouseEvent) => { e.stopPropagation(); - e.preventDefault(); };
243-244: Avoid assigning empty string tobuttonClassNameIn the
MemberDropdowncomponent, whenissue.assignee_idsis empty or undefined,buttonClassNameis set to an empty string. This might override default styles unintentionally.Instead of using an empty string, consider setting
buttonClassNameconditionally only when needed:- buttonClassName={issue.assignee_ids?.length > 0 ? "hover:bg-transparent px-0" : ""} + buttonClassName={issue.assignee_ids?.length > 0 ? "hover:bg-transparent px-0" : undefined}
55-55: Unnecessary string conversion ofproject_idIn the
areEstimateEnabledByProjectIdcall,issue.project_idis converted to a string usingtoString(). Ifproject_idis already a string, this conversion is redundant.Simplify the code by removing the unnecessary conversion:
- areEstimateEnabledByProjectId(issue.project_id?.toString()) + areEstimateEnabledByProjectId(issue.project_id)apiserver/plane/app/views/workspace/draft.py (3)
Line range hint
159-162: Update@allow_permissiondecorator to referenceDraftIssueinpartial_update.The
@allow_permissiondecorator specifiesmodel=Issue, but thepartial_updatemethod operates onDraftIssueinstances. Updating themodeltoDraftIssueensures correct permission checks.Apply this diff:
@allow_permission( allowed_roles=[ROLE.ADMIN, ROLE.MEMBER], creator=True, - model=Issue, + model=DraftIssue, level="WORKSPACE", )
Line range hint
190-193: Update@allow_permissiondecorator to referenceDraftIssueinretrieve.The
@allow_permissiondecorator specifiesmodel=Issue, but theretrievemethod operates onDraftIssueinstances. Updating themodeltoDraftIssueensures correct permission checks.Apply this diff:
@allow_permission( allowed_roles=[ROLE.ADMIN], creator=True, - model=Issue, + model=DraftIssue, level="WORKSPACE", )
Line range hint
214-217: Update@allow_permissiondecorator to referenceDraftIssueindestroy.The
@allow_permissiondecorator specifiesmodel=Issue, but thedestroymethod operates onDraftIssueinstances. Updating themodeltoDraftIssueensures correct permission checks.Apply this diff:
@allow_permission( allowed_roles=[ROLE.ADMIN], creator=True, - model=Issue, + model=DraftIssue, level="WORKSPACE", )web/core/hooks/use-issues-actions.tsx (1)
752-755: SimplifyworkspaceSlugusage infetchIssuesSince
workspaceSlugis already a string after applying.toString(), calling.toString()again is redundant. Consider removing the extra.toString()call.Update the line as follows:
-return issues.fetchIssues(workspaceSlug.toString(), loadType, options); +return issues.fetchIssues(workspaceSlug, loadType, options);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (43)
- apiserver/plane/app/views/workspace/draft.py (6 hunks)
- apiserver/plane/utils/paginator.py (3 hunks)
- packages/types/src/index.d.ts (1 hunks)
- packages/types/src/issues/base.d.ts (1 hunks)
- packages/types/src/workspace-draft-issues/base.d.ts (1 hunks)
- web/app/[workspaceSlug]/(projects)/drafts/header.tsx (1 hunks)
- web/app/[workspaceSlug]/(projects)/drafts/layout.tsx (1 hunks)
- web/app/[workspaceSlug]/(projects)/drafts/page.tsx (1 hunks)
- web/core/components/issues/issue-layouts/filters/applied-filters/roots/workspace-draft-root.tsx (1 hunks)
- web/core/components/issues/issue-layouts/list/base-list-root.tsx (3 hunks)
- web/core/components/issues/issue-layouts/list/default.tsx (2 hunks)
- web/core/components/issues/issue-layouts/list/list-view-types.d.ts (1 hunks)
- web/core/components/issues/issue-layouts/properties/all-properties.tsx (1 hunks)
- web/core/components/issues/issue-layouts/quick-action-dropdowns/index.ts (1 hunks)
- web/core/components/issues/issue-modal/base.tsx (8 hunks)
- web/core/components/issues/issue-modal/draft-issue-layout.tsx (2 hunks)
- web/core/components/issues/issue-modal/form.tsx (2 hunks)
- web/core/components/issues/workspace-draft/delete-modal.tsx (1 hunks)
- web/core/components/issues/workspace-draft/draft-issue-block.tsx (1 hunks)
- web/core/components/issues/workspace-draft/draft-issue-properties.tsx (1 hunks)
- web/core/components/issues/workspace-draft/empty-state.tsx (1 hunks)
- web/core/components/issues/workspace-draft/index.ts (1 hunks)
- web/core/components/issues/workspace-draft/loader.tsx (1 hunks)
- web/core/components/issues/workspace-draft/quick-action.tsx (1 hunks)
- web/core/components/issues/workspace-draft/root.tsx (1 hunks)
- web/core/components/workspace/sidebar/projects-list-item.tsx (2 hunks)
- web/core/components/workspace/sidebar/quick-actions.tsx (2 hunks)
- web/core/constants/dashboard.ts (2 hunks)
- web/core/constants/empty-state.ts (2 hunks)
- web/core/constants/issue.ts (1 hunks)
- web/core/constants/workspace-drafts.ts (1 hunks)
- web/core/hooks/store/index.ts (1 hunks)
- web/core/hooks/store/use-issues.ts (3 hunks)
- web/core/hooks/store/workspace-draft/index.ts (1 hunks)
- web/core/hooks/store/workspace-draft/use-workspace-draft-issue-filters.ts (1 hunks)
- web/core/hooks/store/workspace-draft/use-workspace-draft-issue.ts (1 hunks)
- web/core/hooks/use-issues-actions.tsx (3 hunks)
- web/core/services/issue/index.ts (1 hunks)
- web/core/services/issue/workspace_draft.service.ts (1 hunks)
- web/core/store/issue/root.store.ts (4 hunks)
- web/core/store/issue/workspace-draft/filter.store.ts (1 hunks)
- web/core/store/issue/workspace-draft/index.ts (1 hunks)
- web/core/store/issue/workspace-draft/issue.store.ts (1 hunks)
✅ Files skipped from review due to trivial changes (4)
- web/core/components/issues/issue-layouts/list/default.tsx
- web/core/components/issues/workspace-draft/index.ts
- web/core/hooks/store/workspace-draft/index.ts
- web/core/store/issue/workspace-draft/index.ts
🧰 Additional context used
🪛 Biome
web/core/components/issues/workspace-draft/draft-issue-block.tsx
[error] 32-32: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
web/core/components/issues/workspace-draft/quick-action.tsx
[error] 80-80: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
web/core/hooks/store/use-issues.ts
[error] 85-85: Duplicate case label.
The first similar label is here:
(lint/suspicious/noDuplicateCase)
🪛 GitHub Check: lint-web
web/core/store/issue/workspace-draft/issue.store.ts
[failure] 20-20:
lodashimport should occur before import oflodash/orderBy
🔇 Additional comments (50)
web/core/constants/workspace-drafts.ts (1)
1-6: LGTM! Well-structured enum implementation.The
EDraftIssuePaginationTypeenum is well-implemented, following TypeScript best practices. The naming conventions are correct, and the string values match the member names, which enhances clarity.web/app/[workspaceSlug]/(projects)/drafts/layout.tsx (2)
1-4: LGTM: Directive and imports are well-structured.The "use client" directive is correctly placed, and imports are appropriately organized. The use of destructuring in imports enhances code readability.
1-13: Overall assessment: Well-implemented layout component.The
WorkspaceDraftLayoutcomponent is well-structured and follows React best practices. It provides a clear and logical layout for workspace drafts, with good separation of concerns between the header and content areas. The use of TypeScript for prop types enhances code reliability.Minor suggestions were made for improving prop type definitions and tag consistency, but these are not critical issues. The component is ready for use and should integrate well with the rest of the application.
web/core/services/issue/index.ts (1)
10-10: LGTM: New export statement is consistent with existing pattern.The addition of
export * from "./workspace_draft.service";is in line with the file's structure and purpose. It correctly exposes the workspace draft service alongside other issue-related services, maintaining consistency and improving modularity.web/core/hooks/store/workspace-draft/use-workspace-draft-issue.ts (1)
1-5: LGTM: Imports are correct and necessary.The imports are well-organized and include all necessary dependencies for the hook's implementation. The use of absolute imports is consistent with the project's style.
web/core/hooks/store/workspace-draft/use-workspace-draft-issue-filters.ts (1)
1-5: LGTM: Imports are well-organized and appropriate.The imports are correctly structured, using absolute imports for local modules and separating types. This approach enhances code readability and maintainability.
web/core/components/issues/workspace-draft/loader.tsx (2)
1-9: LGTM! Imports and type definition are well-structured.The "use client" directive, imports, and type definition are correctly implemented. The optional
itemsprop in theTWorkspaceDraftIssuesLoadertype provides flexibility for the component's usage.
11-12: LGTM! Component declaration and props handling are well-implemented.The component is correctly exported and typed. The props destructuring with a default value for
itemsis a clean and effective approach.web/app/[workspaceSlug]/(projects)/drafts/page.tsx (2)
1-6: LGTM: Imports and setup are well-structured.The "use client" directive is correctly placed, and all necessary imports are present. The code follows Next.js and React best practices for importing components and hooks.
27-27: LGTM: Export statement is correct.The default export of the WorkspaceDraftPage component is properly implemented.
packages/types/src/issues/base.d.ts (7)
Line range hint
13-18: LGTM: Well-defined loader typeThe
TLoadertype is well-defined and covers various loading states, including an uninitialized state (undefined). The naming is clear and follows TypeScript conventions.
Line range hint
20-22: LGTM: Clear and appropriate grouping typeThe
TGroupedIssuestype is well-defined for grouping issues by a string identifier. The use of an index signature is appropriate, and the naming clearly indicates the purpose of the type.
Line range hint
24-26: LGTM: Well-structured nested grouping typeThe
TSubGroupedIssuestype is well-defined for nested grouping of issues. It effectively builds upon theTGroupedIssuestype, demonstrating good use of composition. The naming clearly indicates the hierarchical nature of the grouping.
Line range hint
30-34: LGTM: Well-structured pagination data typeThe
TPaginationDatatype is well-defined with clear and self-explanatory property names. It includes essential information for pagination (next and previous cursors, and a flag for next page results). The type name accurately represents its purpose.
Line range hint
36-38: LGTM: Well-structured group-specific pagination data typeThe
TIssuePaginationDatatype is well-defined for storing group-specific pagination data. It effectively builds upon theTPaginationDatatype, demonstrating good use of composition. The naming clearly indicates its purpose of managing pagination data for grouped issues.
Line range hint
40-42: LGTM: Clear and appropriate grouped issue count typeThe
TGroupedIssueCounttype is well-defined for storing the count of issues for each group. The use of an index signature is appropriate, and the naming clearly indicates the purpose of the type.
Line range hint
44-44: LGTM with a consideration: Simple ungrouped issues typeThe
TUnGroupedIssuestype is clearly defined as an array of strings, representing a list of ungrouped issues. The naming accurately reflects its purpose.Consider if this simple array structure will be sufficient for future needs. If there's a possibility that more information might be needed for each ungrouped issue in the future, it might be worth exploring a more structured approach. To verify the current usage and potential future needs, you could run the following script:
This script will help identify current usage patterns and any existing notes about potential changes to ungrouped issues handling.
✅ Verification successful
LGTM
The
TUnGroupedIssuestype is appropriately defined as an array of strings, and its usage within the codebase aligns with this structure. No TODO or FIXME comments related to ungrouped issues were found, indicating that the current implementation meets the requirements.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check usage of TUnGroupedIssues and look for potential expansion points # Search for usage of TUnGroupedIssues echo "Usage of TUnGroupedIssues:" rg --type typescript "TUnGroupedIssues" # Look for TODO comments or FIXME related to ungrouped issues echo "\nTODO or FIXME comments related to ungrouped issues:" rg --type typescript -i "TODO|FIXME" -C 2 | rg -i "ungrouped.*issues"Length of output: 361
Script:
#!/bin/bash # Description: Check usage of TUnGroupedIssues and look for potential expansion points using glob patterns # Search for usage of TUnGroupedIssues in .ts and .tsx files echo "Usage of TUnGroupedIssues:" rg "TUnGroupedIssues" --glob "*.ts" --glob "*.tsx" # Look for TODO or FIXME comments related to ungrouped issues in .ts and .tsx files echo "\nTODO or FIXME comments related to ungrouped issues:" rg -i "TODO|FIXME" --glob "*.ts" --glob "*.tsx" -C 2 | rg -i "ungrouped.*issues"Length of output: 707
Script:
#!/bin/bash # Description: Further analyze TUnGroupedIssues usage and related structures # Search for any types that are used alongside TUnGroupedIssues echo "Types used with TUnGroupedIssues:" rg "TUnGroupedIssues" --glob "*.ts" --glob "*.tsx" -A 2 # Check for interfaces or types that extend or utilize TUnGroupedIssues echo "\nInterfaces or types extending TUnGroupedIssues:" rg "extends .*TUnGroupedIssues" --glob "*.ts" --glob "*.tsx" # Look for any functions or methods that manipulate TUnGroupedIssues echo "\nFunctions or methods manipulating TUnGroupedIssues:" rg "function .*\(.*TUnGroupedIssues.*\)" --glob "*.ts" --glob "*.tsx"Length of output: 1393
packages/types/src/index.d.ts (1)
32-32: LGTM: New export for workspace draft issues added.The new export statement for "./workspace-draft-issues/base" has been correctly added, which aligns with the PR objectives for addressing workspace draft issues. This addition will make the types or interfaces related to workspace draft issues available for use in other parts of the project.
web/core/components/issues/workspace-draft/empty-state.tsx (2)
1-9: LGTM: Imports and "use client" directive are well-organized.The "use client" directive is correctly placed, and imports are appropriately organized. This structure enhances readability and maintainability.
11-13: LGTM: Component declaration and state management are well-implemented.The
WorkspaceDraftEmptyStatecomponent is correctly declared as a functional component and exported. The use of theuseStatehook for managing the modal's visibility is appropriate and follows React best practices.web/core/hooks/store/index.ts (1)
34-34: LGTM: New export follows existing patternThe addition of
export * from "./workspace-draft";is consistent with the existing export pattern in this file. It appears to be a valid inclusion for a new module related to workspace drafts.To ensure the exported module exists and is correctly implemented, let's verify its presence:
web/app/[workspaceSlug]/(projects)/drafts/header.tsx (2)
1-18: LGTM: Imports and component declaration are well-structured.The use of the 'use client' directive, imports, and component declaration with MobX's
observerare all appropriate for this client-side component.
32-37: LGTM: CreateUpdateIssueModal implementation is well-structured.The modal implementation is correct and follows best practices. The use of a separate component for creating/updating issues promotes code reusability.
web/core/components/issues/issue-layouts/list/base-list-root.tsx (1)
65-67: LGTM! Improved code structure and type safety.The changes to the
BaseListRootcomponent are well-implemented:
- Destructuring
updateFiltersfromuseIssuesActions(storeType)improves code readability.- The initialization of
collapsedGroupswith a fallback object enhances type safety and prevents potential runtime errors.These refinements align with best practices for React and TypeScript development.
web/core/constants/dashboard.ts (1)
5-5: LGTM: Import statement updated correctly.The
PenSquareicon has been added to the import statement fromlucide-react, which is consistent with its usage in the new "Drafts" menu item.web/core/store/issue/root.store.ts (2)
26-27: LGTM: Import statements are well-organized.The consolidation of workspace-related imports and the addition of workspace draft issues imports improve code organization and readability. These changes align with the PR objectives of addressing workspace draft issues.
59-61: LGTM: Interface updated to support workspace draft issues.The addition of
workspaceDraftIssuesFilterandworkspaceDraftIssuesproperties to theIIssueRootStoreinterface properly extends the store's capabilities to handle workspace draft issues. This change is consistent with the PR objectives.web/core/constants/issue.ts (1)
33-33: LGTM! Verify usage of the new enum value.The addition of
WORKSPACE_DRAFTtoEIssuesStoreTypeis consistent with the existing structure and aligns with the PR objectives. It expands the enum's capability to represent workspace-level draft issues.To ensure proper integration, please verify the usage of this new enum value across the codebase:
✅ Verification successful
Verified: The
WORKSPACE_DRAFTenum value is appropriately used across the codebase without any detected issues.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for usage of EIssuesStoreType.WORKSPACE_DRAFT # Test: Look for direct usage of WORKSPACE_DRAFT echo "Searching for direct usage of WORKSPACE_DRAFT:" rg --type typescript --type javascript "EIssuesStoreType\.WORKSPACE_DRAFT" # Test: Look for string literal "WORKSPACE_DRAFT" in case it's used without the enum echo "Searching for string literal 'WORKSPACE_DRAFT':" rg --type typescript --type javascript "'WORKSPACE_DRAFT'|\"WORKSPACE_DRAFT\"" # Test: Look for potential switch statements or if-else chains handling EIssuesStoreType echo "Searching for potential switch statements or if-else chains:" rg --type typescript --type javascript "switch.*EIssuesStoreType|case.*:"Length of output: 687
Script:
#!/bin/bash # Description: Search for usage of EIssuesStoreType.WORKSPACE_DRAFT # Test: Look for direct usage of WORKSPACE_DRAFT echo "Searching for direct usage of WORKSPACE_DRAFT:" rg --type=ts --type=tsx --type=js --type=jsx "EIssuesStoreType\.WORKSPACE_DRAFT" # Test: Look for string literal "WORKSPACE_DRAFT" in case it's used without the enum echo "Searching for string literal 'WORKSPACE_DRAFT':" rg --type=ts --type=tsx --type=js --type=jsx "'WORKSPACE_DRAFT'|\"WORKSPACE_DRAFT\"" # Test: Look for potential switch statements or if-else chains handling EIssuesStoreType echo "Searching for potential switch statements or if-else chains:" rg --type=ts --type=tsx --type=js --type=jsx "switch.*EIssuesStoreType|case.*:"Length of output: 684
Script:
#!/bin/bash # Description: Search for usage of EIssuesStoreType.WORKSPACE_DRAFT # Test: Look for direct usage of WORKSPACE_DRAFT echo "Searching for direct usage of WORKSPACE_DRAFT:" rg --glob "*.ts" --glob "*.tsx" --glob "*.js" --glob "*.jsx" "EIssuesStoreType\.WORKSPACE_DRAFT" # Test: Look for string literal "WORKSPACE_DRAFT" in case it's used without the enum echo "Searching for string literal 'WORKSPACE_DRAFT':" rg --glob "*.ts" --glob "*.tsx" --glob "*.js" --glob "*.jsx" "'WORKSPACE_DRAFT'|\"WORKSPACE_DRAFT\"" # Test: Look for potential switch statements or if-else chains handling EIssuesStoreType echo "Searching for potential switch statements or if-else chains:" rg --glob "*.ts" --glob "*.tsx" --glob "*.js" --glob "*.jsx" "switch.*EIssuesStoreType|case.*:"Length of output: 27425
web/core/components/issues/issue-modal/form.tsx (2)
269-271: LGTM: Title update for draft issuesThe title now correctly includes "Create draft" when
isDraftis true, providing clear feedback to the user about the current action. This change enhances the user experience by differentiating between creating a regular issue and a draft issue.
Line range hint
1-418: Overall assessment: Draft issue handling implemented successfullyThe changes to the
IssueFormRootcomponent effectively implement the handling of draft issues. The modifications to the title display and submit button text provide clear feedback to users about whether they are creating a regular issue or a draft issue. These changes are well-integrated into the existing component and consistent with its overall structure.A few key points:
- The title now includes "Create draft" when appropriate.
- The submit button text reflects the draft status when creating a new draft issue.
- The changes are minimal and focused, reducing the risk of introducing new bugs.
The implementation successfully addresses the requirements for draft issues while maintaining the existing functionality of the component.
web/core/components/issues/issue-layouts/properties/all-properties.tsx (1)
40-40: LGTM: Improved readability ofupdateIssuepropertyThe refactoring of the
updateIssueproperty in theIIssuePropertiesinterface improves code readability by condensing the multiline declaration into a single line. This change maintains the same functionality while making the code more concise.web/core/components/issues/workspace-draft/delete-modal.tsx (1)
45-46: Confirm thatissueandcurrentUserare defined before use.Ensure that
issueandcurrentUserare not undefined or null to prevent potential runtime errors when accessing their properties.Run the following script to check for null or undefined checks on
issueandcurrentUser:web/core/hooks/store/use-issues.ts (2)
28-31: Addition ofWORKSPACE_DRAFTtoTStoreIssuesis correctThe
WORKSPACE_DRAFTstore type has been correctly added toTStoreIssueswith appropriate type definitions.
80-84: Implementation ofWORKSPACE_DRAFTcase inuseIssuesfunction is correctThe
WORKSPACE_DRAFTcase has been properly added to theuseIssuesfunction, returning the merged store with the correctissuesandissuesFilter.web/core/components/issues/issue-modal/draft-issue-layout.tsx (1)
19-19: Ensure the new service is correctly integratedThe import statement for
workspaceDraftServiceappears to be correct. Ensure that all previous references toIssueDraftServicehave been updated throughout the code to prevent any inconsistencies.web/core/store/issue/workspace-draft/issue.store.ts (1)
66-66: Consider extending the base issues store for consistencyAs previously mentioned, this class should extend the base issues store and utilize its pagination methods to adhere to the established pattern in the codebase. This helps avoid code duplication and ensures consistency across stores.
web/core/components/issues/workspace-draft/draft-issue-properties.tsx (3)
85-86: Confirm cycle removal logic inremoveIssueFromCycleThere's a TODO comment
// TODO: To be checkedin theremoveIssueFromCyclefunction. Please verify that setting the cycle ID to an empty string ("") correctly removes the issue from the cycle. Depending on the backend implementation, you might need to passnullor omit the field entirely to properly remove the cycle association.
145-146: Handle cases whereissue.project_idis undefinedThe component returns
nullif!issue.project_id. This could lead to the component not rendering whenproject_idis undefined or null. Ensure that this is the intended behavior and that the parent component handles this scenario appropriately.
255-266:⚠️ Potential issueConditional rendering of
ModuleDropdownmay cause layout shiftsThe
ModuleDropdownis conditionally rendered based onprojectDetails?.module_view. This could lead to layout shifts ifprojectDetailsupdates after the initial render.Consider rendering a placeholder or reserving space to prevent layout shifts while
projectDetailsis loading:{projectDetails ? ( projectDetails.module_view && ( <div className="h-5" onClick={handleEventPropagation}> {/* ModuleDropdown component */} </div> ) ) : ( // Placeholder or loading indicator <div className="h-5" /> )}Likely invalid or redundant comment.
apiserver/plane/app/views/workspace/draft.py (1)
Line range hint
49-88: Great refactoring with theget_querysetmethod.Centralizing the queryset logic in the
get_querysetmethod enhances maintainability and reduces code redundancy across the viewset methods.web/core/components/issues/issue-modal/base.tsx (6)
53-53: Update store type toEIssuesStoreType.WORKSPACE_DRAFTis appropriate.Changing the store type to
EIssuesStoreType.WORKSPACE_DRAFTensures that the component interacts with the correct draft issues store, which aligns with the updated logic for managing workspace draft issues.
73-75: Setting description when issue details are not fetched is correct.When
fetchIssueDetailsis false or other conditions are met, setting the description fromdata?.description_htmlor a default<p></p>ensures that the description field is populated appropriately in the absence of fetched issue details.
215-216: ConfirmcreateMorefunctionality enhances user experience.The logic correctly handles closing the modal or focusing on the issue title field based on the
createMoreflag, allowing users to efficiently create multiple issues in succession.
316-316: PassingisDraftflag tohandleFormSubmitensures correct handling.By passing the
isDraftflag tohandleFormSubmit, the submission logic correctly differentiates between draft and regular issues, which is essential for proper issue handling inDraftIssueLayout.
334-334: VerifyisDraftusage inIssueFormRootsubmission.In the
IssueFormRootcomponent, passingisDrafttohandleFormSubmitmight not be necessary if it's intended for non-draft issues. Ensure thatisDraftaccurately reflects the issue type to prevent unintended behavior.Consider reviewing the
isDraftflag usage to confirm it aligns with the expected submission process for regular issues.
156-156:⚠️ Potential issueAdjust condition to allow creating draft issues without a project ID.
The current precondition
if (!workspaceSlug || !payload.project_id) return;prevents creating draft issues whenpayload.project_idis undefined. Since draft issues may not require a project ID, consider modifying the condition to allow draft issue creation without it.Apply this diff to adjust the condition:
- if (!workspaceSlug || !payload.project_id) return; + if (!workspaceSlug || (!payload.project_id && !is_draft_issue)) return;This change ensures that draft issues can be created even if
payload.project_idis not provided.Likely invalid or redundant comment.
web/core/hooks/use-issues-actions.tsx (4)
48-48: ImportingworkspaceDraftIssueActionsThe addition of
workspaceDraftIssueActionsintegrates well with the existing actions.
65-66: AddingWORKSPACE_DRAFTcase to the switch statementIncluding
EIssuesStoreType.WORKSPACE_DRAFTensures that the correct actions are returned for workspace draft issues.
758-762: Consistent dependency arrays inuseCallbackhooksSimilar to
fetchIssues, verify that alluseCallbackhooks include all necessary dependencies, such as specific functions fromissues.Review the dependency arrays of all
useCallbackhooks in this hook to ensure completeness. Consider using ESLint's exhaustive-deps rule to automate this verification.
746-748: Handle potential absence ofglobalViewIdIn the
useWorkspaceDraftIssueActionshook,globalViewIdis derived fromuseParams(). Ensure that the absence ofglobalViewIdis appropriately handled if it's essential for any operations.Confirm whether
globalViewIdis required in this context and add necessary checks or remove it if unnecessary.
| export const WorkspaceDraftIssuesLoader: FC<TWorkspaceDraftIssuesLoader> = (props) => { | ||
| const { items = 14 } = props; | ||
| return ( | ||
| <div className="relative h-full w-full"> | ||
| {[...Array(items)].map((_, index) => ( | ||
| <ListLoaderItemRow key={index} /> | ||
| ))} | ||
| </div> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing accessibility and customization options.
While the component effectively renders a loading state, consider the following enhancements:
- Improve accessibility by adding an aria-label to the container div:
- <div className="relative h-full w-full">
+ <div className="relative h-full w-full" aria-label="Loading workspace draft issues">- To increase flexibility, you might want to allow customization of individual
ListLoaderItemRowcomponents. This could be achieved by accepting a render prop:
type TWorkspaceDraftIssuesLoader = {
items?: number;
renderItem?: (index: number) => React.ReactNode;
};
export const WorkspaceDraftIssuesLoader: FC<TWorkspaceDraftIssuesLoader> = (props) => {
const { items = 14, renderItem } = props;
return (
<div className="relative h-full w-full" aria-label="Loading workspace draft issues">
{Array.from({ length: items }).map((_, index) => (
renderItem ? renderItem(index) : <ListLoaderItemRow key={index} />
))}
</div>
);
};This change would allow users of the component to customize the appearance of individual loader items if needed, while still providing a default implementation.
| print(limit, "limit") | ||
| if cursor.value != limit: | ||
| results = results[-(limit + 1) :] |
There was a problem hiding this comment.
Remove debug print and review slicing logic
-
The print statement on line 153 should be removed before merging to production.
-
The modification to the slicing logic on lines 154-155 needs careful review:
if cursor.value != limit: results = results[-(limit + 1):]
This change might lead to unexpected behavior if
cursor.valueis significantly different fromlimit. It could potentially return fewer results than expected or cause issues with pagination consistency.
Consider removing the print statement and reverting the slicing logic to its previous implementation unless there's a specific reason for this change that's not apparent from the context provided.
| @@ -761,7 +761,7 @@ | |||
| ): | |||
| """Paginate the request""" | |||
| per_page = self.get_per_page(request, default_per_page, max_per_page) | |||
|
|
|||
| print(per_page, "per_page") | |||
There was a problem hiding this comment.
Replace debug prints with proper logging
There are two print statements added for debugging purposes:
- Line 764:
print(per_page, "per_page") - Line 791:
print(per_page, "per_page 2")
These should be removed or replaced with proper logging before merging to production.
Replace the print statements with appropriate logging:
import logging
logger = logging.getLogger(__name__)
# ... (in the paginate method)
logger.debug(f"Initial per_page value: {per_page}")
# ... (before calling paginator.get_result)
logger.debug(f"per_page value before get_result: {per_page}")This approach allows for better control over log levels and output in different environments.
Also applies to: 791-791
| return allowPermissions( | ||
| [EUserPermissions.ADMIN, EUserPermissions.MEMBER], | ||
| EUserPermissionsLevel.PROJECT, | ||
| workspaceSlug.toString(), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid unnecessary .toString() on workspaceSlug
The workspaceSlug variable is already a string returned from useParams(), so calling .toString() is unnecessary and could lead to errors if workspaceSlug is undefined.
Consider removing the .toString() call:
EUserPermissionsLevel.PROJECT,
- workspaceSlug.toString(),
+ workspaceSlug,
projectId📝 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.
| workspaceSlug.toString(), | |
| workspaceSlug, |
| (projectId: string | undefined) => { | ||
| if (!projectId) return false; | ||
| return allowPermissions( | ||
| [EUserPermissions.ADMIN, EUserPermissions.MEMBER], | ||
| EUserPermissionsLevel.PROJECT, | ||
| workspaceSlug.toString(), | ||
| projectId | ||
| ); | ||
| }, |
There was a problem hiding this comment.
Handle potential undefined values for workspaceSlug
There is no check to ensure that workspaceSlug is defined before using it in canEditProperties. If workspaceSlug can be undefined, this may lead to runtime errors.
Consider adding a check for workspaceSlug:
(projectId: string | undefined) => {
+ if (!workspaceSlug) return false;
if (!projectId) return false;
return allowPermissions(📝 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.
| (projectId: string | undefined) => { | |
| if (!projectId) return false; | |
| return allowPermissions( | |
| [EUserPermissions.ADMIN, EUserPermissions.MEMBER], | |
| EUserPermissionsLevel.PROJECT, | |
| workspaceSlug.toString(), | |
| projectId | |
| ); | |
| }, | |
| (projectId: string | undefined) => { | |
| if (!workspaceSlug) return false; | |
| if (!projectId) return false; | |
| return allowPermissions( | |
| [EUserPermissions.ADMIN, EUserPermissions.MEMBER], | |
| EUserPermissionsLevel.PROJECT, | |
| workspaceSlug.toString(), | |
| projectId | |
| ); | |
| }, |
| if (isDraft) await draftIssues.updateIssue(workspaceSlug.toString(), data.id, payload); | ||
| else if (updateIssue) await updateIssue(payload.project_id, data.id, payload); |
There was a problem hiding this comment.
Modify condition to allow updating draft issues without a project ID.
In handleUpdateIssue, the condition if (!workspaceSlug || !payload.project_id || !data?.id) return; can prevent updating draft issues when payload.project_id is undefined. Since draft issues may not have a project ID, adjust the condition to accommodate this scenario.
Apply this diff to adjust the condition:
- if (!workspaceSlug || !payload.project_id || !data?.id) return;
+ if (!workspaceSlug || (!payload.project_id && !isDraft) || !data?.id) return;This ensures that draft issues can be updated even if payload.project_id is not specified.
Committable suggestion was skipped due to low confidence.
| [workspaceSlug, issues] | ||
| ); |
There was a problem hiding this comment.
Ensure all dependencies are included in useCallback
In the dependency array of the fetchIssues useCallback, issues.fetchIssues should be included to ensure that the callback updates appropriately when issues.fetchIssues changes.
Update the dependency array:
-[workspaceSlug, issues]
+[workspaceSlug, issues.fetchIssues]📝 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.
| [workspaceSlug, issues] | |
| ); | |
| [workspaceSlug, issues.fetchIssues] | |
| ); |
| async (workspaceSlug: string, issueId: string, data: Partial<TIssue>) => { | ||
| if (!workspaceSlug || !issueId || !data) return; | ||
| return await issues.moveToIssues(workspaceSlug, issueId, data); | ||
| }, | ||
| [issues] | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor moveToIssue function parameter
The moveToIssue function accepts workspaceSlug as a parameter, which is unnecessary since workspaceSlug is already available in the scope. Additionally, to maintain consistency with other functions, consider removing workspaceSlug from the parameters.
Refactor the function as follows:
-async (workspaceSlug: string, issueId: string, data: Partial<TIssue>) => {
+async (issueId: string, data: Partial<TIssue>) => {
if (!workspaceSlug || !issueId || !data) return;
return await issues.moveToIssues(workspaceSlug, issueId, data);
},Ensure to update the dependency array and any calls to this function accordingly.
Committable suggestion was skipped due to low confidence.
| const updateFilters = useCallback( | ||
| async ( | ||
| projectId: string, | ||
| filterType: EIssueFilterType, | ||
| filters: IIssueFilterOptions | IIssueDisplayFilterOptions | IIssueDisplayProperties | TIssueKanbanFilters | ||
| ) => { | ||
| filters = filters as IIssueFilterOptions | IIssueDisplayFilterOptions | IIssueDisplayProperties; | ||
| if (!globalViewId || !workspaceSlug) return; | ||
| return await issuesFilter.updateFilters(workspaceSlug, filterType, filters); | ||
| }, | ||
| [globalViewId, workspaceSlug, issuesFilter] | ||
| ); |
There was a problem hiding this comment.
Eliminate unnecessary projectId parameter in updateFilters
The updateFilters function includes a projectId parameter that is not utilized within the function. Removing this parameter will streamline the function signature.
Apply this change to remove the unused projectId parameter:
-async (
- projectId: string,
filterType: EIssueFilterType,
filters: IIssueFilterOptions | IIssueDisplayFilterOptions | IIssueDisplayProperties | TIssueKanbanFilters
) => {
if (!globalViewId || !workspaceSlug) return;
return await issuesFilter.updateFilters(workspaceSlug, filterType, filters);
},Committable suggestion was skipped due to low confidence.
| const useWorkspaceDraftIssueActions = () => { | ||
| // router | ||
| const { workspaceSlug: routerWorkspaceSlug, globalViewId: routerGlobalViewId } = useParams(); | ||
| const workspaceSlug = routerWorkspaceSlug?.toString(); | ||
| const globalViewId = routerGlobalViewId?.toString(); | ||
| // store hooks | ||
| const { issues, issuesFilter } = useIssues(EIssuesStoreType.WORKSPACE_DRAFT); | ||
| const fetchIssues = useCallback( | ||
| async (loadType: TLoader, options: IssuePaginationOptions) => { | ||
| if (!workspaceSlug) return; | ||
| return issues.fetchIssues(workspaceSlug.toString(), loadType, options); | ||
| }, | ||
| [workspaceSlug, issues] | ||
| ); | ||
|
|
||
| const fetchNextIssues = useCallback(async () => { | ||
| if (!workspaceSlug) return; | ||
| return issues.fetchNextIssues(workspaceSlug.toString()); | ||
| }, [workspaceSlug, issues]); | ||
|
|
||
| const createIssue = useCallback( | ||
| async (projectId: string | undefined | null, data: Partial<TIssue>) => { | ||
| if (!workspaceSlug || !projectId) return; | ||
| return await issues.createWorkspaceDraftIssue(workspaceSlug, data); | ||
| }, | ||
| [issues, workspaceSlug] | ||
| ); | ||
| const updateIssue = useCallback( | ||
| async (projectId: string | undefined | null, issueId: string, data: Partial<TIssue>) => { | ||
| if (!workspaceSlug || !projectId) return; | ||
| return await issues.updateWorkspaceDraftIssue(workspaceSlug, issueId, data); | ||
| }, | ||
| [issues, workspaceSlug] | ||
| ); | ||
| const removeIssue = useCallback( | ||
| async (projectId: string | undefined | null, issueId: string) => { | ||
| if (!workspaceSlug || !projectId) return; | ||
| return await issues.deleteWorkspaceDraftIssue(workspaceSlug, issueId); | ||
| }, | ||
| [issues, workspaceSlug] | ||
| ); | ||
|
|
||
| const moveToIssue = useCallback( | ||
| async (workspaceSlug: string, issueId: string, data: Partial<TIssue>) => { | ||
| if (!workspaceSlug || !issueId || !data) return; | ||
| return await issues.moveToIssues(workspaceSlug, issueId, data); | ||
| }, | ||
| [issues] | ||
| ); | ||
|
|
||
| const updateFilters = useCallback( | ||
| async ( | ||
| projectId: string, | ||
| filterType: EIssueFilterType, | ||
| filters: IIssueFilterOptions | IIssueDisplayFilterOptions | IIssueDisplayProperties | TIssueKanbanFilters | ||
| ) => { | ||
| filters = filters as IIssueFilterOptions | IIssueDisplayFilterOptions | IIssueDisplayProperties; | ||
| if (!globalViewId || !workspaceSlug) return; | ||
| return await issuesFilter.updateFilters(workspaceSlug, filterType, filters); | ||
| }, | ||
| [globalViewId, workspaceSlug, issuesFilter] | ||
| ); | ||
|
|
||
| return useMemo( | ||
| () => ({ | ||
| fetchIssues, | ||
| fetchNextIssues, | ||
| createIssue, | ||
| updateIssue, | ||
| removeIssue, | ||
| updateFilters, | ||
| moveToIssue, | ||
| }), | ||
| [fetchIssues, fetchNextIssues, createIssue, updateIssue, removeIssue, updateFilters, moveToIssue] | ||
| ); | ||
| }; |
There was a problem hiding this comment.
Remove unused parameter projectId in functions
In the useWorkspaceDraftIssueActions hook, the functions createIssue, updateIssue, and removeIssue accept projectId as a parameter, but it is not used within these functions. Removing the unused projectId parameter will clean up the code and prevent potential confusion.
Apply this diff to remove the unused projectId parameter:
-async (projectId: string | undefined | null, data: Partial<TIssue>) => {
+async (data: Partial<TIssue>) => {
if (!workspaceSlug) return;
return await issues.createWorkspaceDraftIssue(workspaceSlug, data);
},Repeat the same change for updateIssue and removeIssue functions.
Committable suggestion was skipped due to low confidence.
Summary by CodeRabbit
Release Notes
New Features
WorkspaceDraftHeader,WorkspaceDraftLayout, andDraftIssueBlock.Enhancements
Bug Fixes
Documentation