chore: refactor project states to make way for new features#6156
chore: refactor project states to make way for new features#6156
Conversation
WalkthroughThe changes introduced in this pull request include the addition of new components and properties across various files, enhancing the functionality of tooltips and workflow management. A new optional property 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: 9
🧹 Outside diff range and nitpick comments (16)
web/core/hooks/store/use-project-state.ts (1)
1-8: Consider documenting the state transition workflowThe introduction of
IStateTransitionandIStateWorkFlowtypes suggests a significant enhancement to state management. Consider adding documentation (e.g., in README or TSDoc comments) explaining:
- The purpose and lifecycle of state transitions
- The role of actors in state transitions
- Example usage of these interfaces
This will help team members understand the new state management system and facilitate the implementation of future features.
web/helpers/array.helper.ts (1)
111-119: Consider optimizing and adding input validationWhile the implementation is functional, it could be improved for robustness and performance.
Consider this optimized version with input validation:
export const convertStringArrayToBooleanObject = (arrayStrings: string[]) => { + if (!Array.isArray(arrayStrings)) return {}; + - const obj: { [key: string]: boolean } = {}; - - for (const arrayString of arrayStrings) { - obj[arrayString] = true; - } - - return obj; + return arrayStrings.reduce((acc, str) => { + if (typeof str === 'string') { + acc[str] = true; + } + return acc; + }, {} as Record<string, boolean>); };web/core/store/root.store.ts (1)
77-78: Type assertions indicate potential architectural issuesThe use of
as unknown as RootStoretype assertions in both the constructor and resetOnSignOut method suggests potential issues in the type hierarchy betweenCoreRootStoreandRootStore.Consider these alternatives:
- Properly extend
CoreRootStorefromRootStore- Create an interface that both stores implement
- Refactor the stores to eliminate the need for type assertions
Example approach:
// Option 1: Proper inheritance export class CoreRootStore extends RootStore { // ... existing properties and methods } // Option 2: Common interface interface IBaseRootStore { // ... common properties and methods } export class CoreRootStore implements IBaseRootStore { // ... existing properties and methods } export class RootStore implements IBaseRootStore { // ... existing properties and methods }Also applies to: 108-109
web/core/store/issue/issue-details/sub_issues.store.ts (1)
Line range hint
324-349: Consider enhancing fetchOtherProjectProperties with error handling and parallel fetchingWhile the method comprehensively fetches all necessary project data, consider these improvements:
- Add error handling for individual fetch operations
- Use Promise.all for parallel fetching to improve performance
- Add progress tracking for better observability
Here's a suggested implementation:
fetchOtherProjectProperties = async (workspaceSlug: string, projectIds: string[]) => { if (projectIds.length > 0) { for (const projectId of projectIds) { - // fetching other project states - this.rootIssueDetailStore.rootIssueStore.rootStore.state.fetchProjectStates(workspaceSlug, projectId); - // fetching other project members - this.rootIssueDetailStore.rootIssueStore.rootStore.memberRoot.project.fetchProjectMembers( - workspaceSlug, - projectId - ); - // remaining sequential fetches... + try { + await Promise.all([ + this.rootIssueDetailStore.rootIssueStore.rootStore.state.fetchProjectStates(workspaceSlug, projectId), + this.rootIssueDetailStore.rootIssueStore.rootStore.memberRoot.project.fetchProjectMembers( + workspaceSlug, + projectId + ), + this.rootIssueDetailStore.rootIssueStore.rootStore.label.fetchProjectLabels(workspaceSlug, projectId), + this.rootIssueDetailStore.rootIssueStore.rootStore.cycle.fetchAllCycles(workspaceSlug, projectId), + this.rootIssueDetailStore.rootIssueStore.rootStore.module.fetchModules(workspaceSlug, projectId), + this.rootIssueDetailStore.rootIssueStore.rootStore.projectEstimate.getProjectEstimates( + workspaceSlug, + projectId + ) + ]); + } catch (error) { + console.error(`Failed to fetch properties for project ${projectId}:`, error); + // Consider implementing retry logic or error reporting + } } } };web/core/components/issues/issue-layouts/kanban/kanban-group.tsx (1)
124-130: Consider refactoring duplicated code inonDragEnterandonDragStarthandlersThe logic inside the
onDragEnterandonDragStarthandlers is identical. To adhere to the DRY (Don't Repeat Yourself) principle, consider extracting the shared logic into a separate function.web/ce/components/workflow/add-state-transition.tsx (1)
14-18: Remove unnecessary fragment in JSXThe fragment
<>...</>is unnecessary inside thedivas it's already wrapping the content. Removing it simplifies the code.Apply this diff to remove the fragment:
- <> <Plus className="h-4 w-4" color="#8591AD" /> <span className="text-custom-text-400 font-medium"> Add Transition</span> <div className="text-white bg-custom-background-80 font-semibold px-2 rounded-lg">Pro</div> - </>web/ce/components/workflow/state-item-child.tsx (1)
1-8: Improve import organizationThe import statements could be better organized:
- Comments between imports break consistency
- SetStateAction type should be explicitly imported from 'react'
Consider this organization:
-import { SetStateAction } from "react"; +import type { SetStateAction } from "react"; import { observer } from "mobx-react"; -// Plane import { IState } from "@plane/types"; -// components import { StateItemTitle } from "@/components/project-states/state-item-title"; -// import { AddStateTransition } from "./add-state-transition";web/core/components/issues/issue-layouts/kanban/headers/sub-group-by-card.tsx (1)
Line range hint
23-26: Enhance accessibility for interactive elementsThe div element is clickable but lacks proper accessibility attributes.
Consider adding these accessibility improvements:
<div className={`relative flex w-full flex-shrink-0 flex-row items-center gap-1 rounded-sm py-1.5 cursor-pointer`} onClick={() => handleCollapsedGroups("sub_group_by", column_id)} + role="button" + aria-label={`Toggle ${title} group`} + tabIndex={0} + onKeyDown={(e) => { + if (e.key === 'Enter' || e.key === ' ') { + e.preventDefault(); + handleCollapsedGroups("sub_group_by", column_id); + } + }} >web/core/components/issues/issue-layouts/group-drag-overlay.tsx (1)
45-75: Consider extracting message rendering logicThe conditional rendering logic could be simplified by extracting the message content into separate components for better maintainability.
Consider refactoring like this:
- {workflowDisabledSource ? ( - <WorkFlowDisabledMessage parentStateId={workflowDisabledSource} className="my-2" /> - ) : ( - <div className={cn("p-3 my-8 flex flex-col rounded items-center", - {"text-custom-text-200": shouldOverlayBeVisible}, - {"text-custom-text-error": isDropDisabled} - )}> - {dropErrorMessage ? ( - <div className="flex items-center"> - <AlertCircle width={13} height={13} /> - <span>{dropErrorMessage}</span> - </div> - ) : ( - <> - {readableOrderBy && ( - <span> - The layout is ordered by <span className="font-semibold">{readableOrderBy}</span>. - </span> - )} - <span>Drop here to move the issue.</span> - </> - )} - </div> - )} + <OverlayMessage + workflowDisabledSource={workflowDisabledSource} + dropErrorMessage={dropErrorMessage} + readableOrderBy={readableOrderBy} + shouldOverlayBeVisible={shouldOverlayBeVisible} + isDropDisabled={isDropDisabled} + />packages/ui/src/tooltip/tooltip.tsx (1)
84-99: Consider adding type guard for jsxContentWhile the implementation is correct, adding a type guard could improve type safety.
Consider adding:
+ const isJSXContent = (content: string | React.ReactNode): content is React.ReactNode => + typeof content !== 'string'; content={ - jsxContent ? ( + jsxContent && isJSXContent(jsxContent) ? ( <>{jsxContent}</> ) : (web/core/components/project-states/state-item-title.tsx (2)
55-55: Simplify boolean expressionThe ternary operator with boolean literals can be simplified.
- isDefault={state.default ? true : false} + isDefault={Boolean(state.default)}🧰 Tools
🪛 Biome (1.9.4)
[error] 55-55: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
26-71: Consider breaking down the component for better maintainabilityThe component has multiple logical sections that could be extracted into separate components.
Consider splitting into:
- StateIcon (lines 35-37)
- StateInfo (lines 39-42)
- StateActions (lines 47-69)
This would improve readability and make the component easier to maintain.
🧰 Tools
🪛 Biome (1.9.4)
[error] 55-55: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
web/core/components/project-states/state-item.tsx (1)
131-143: Clean refactoring of state item presentation logic!The extraction of presentation logic into
StateItemChildimproves modularity while maintaining the core drag-and-drop functionality. The component structure is now more maintainable and follows the Single Responsibility Principle.Consider documenting the responsibilities of
StateItemChildin a comment block to help future maintainers understand the separation of concerns.web/core/components/issues/issue-layouts/kanban/headers/group-by-card.tsx (1)
135-136: Consider handling vertical alignment for WorkFlowGroupTreeWhile the WorkFlowGroupTree integration looks good, it should respect the vertical alignment state when
verticalAlignPositionis true.Consider wrapping WorkFlowGroupTree with the same conditional styling:
- <WorkFlowGroupTree groupBy={group_by} groupId={column_id} /> + <div className={verticalAlignPosition ? 'flex flex-col items-center' : ''}> + <WorkFlowGroupTree groupBy={group_by} groupId={column_id} /> + </div>web/core/components/dropdowns/state.tsx (2)
35-36: LGTM! Consider adding JSDoc comments.The new optional props enhance the component's flexibility for state filtering. Consider adding JSDoc comments to document their purpose and usage.
+/** List of specific state IDs to filter the dropdown options */ stateIds?: string[]; +/** Flag to control whether to filter available state IDs */ filterAvailableStateIds?: boolean;
91-91: Simplify boolean coercion.The double negation (
!!value) is redundant as the value will already be coerced to a boolean in this context.-const stateValue = !!value ? value : showDefaultState ? defaultState?.id : undefined; +const stateValue = value ? value : showDefaultState ? defaultState?.id : undefined;🧰 Tools
🪛 Biome (1.9.4)
[error] 91-91: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation(lint/complexity/noExtraBooleanCast)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (38)
packages/ui/src/tooltip/tooltip.tsx(3 hunks)web/ce/components/workflow/add-state-transition.tsx(1 hunks)web/ce/components/workflow/index.tsx(1 hunks)web/ce/components/workflow/state-item-child.tsx(1 hunks)web/ce/components/workflow/state-option.tsx(1 hunks)web/ce/components/workflow/state-transition-count.tsx(1 hunks)web/ce/components/workflow/use-workflow-drag-n-drop.ts(1 hunks)web/ce/components/workflow/workflow-disabled-message.tsx(1 hunks)web/ce/components/workflow/workflow-group-tree.tsx(1 hunks)web/ce/services/index.ts(1 hunks)web/ce/services/project/project-state.service.ts(1 hunks)web/ce/store/state.store.ts(1 hunks)web/ce/types/index.ts(1 hunks)web/ce/types/state.d.ts(1 hunks)web/core/components/dropdowns/state.tsx(6 hunks)web/core/components/issues/issue-layouts/group-drag-overlay.tsx(3 hunks)web/core/components/issues/issue-layouts/kanban/headers/group-by-card.tsx(3 hunks)web/core/components/issues/issue-layouts/kanban/headers/sub-group-by-card.tsx(3 hunks)web/core/components/issues/issue-layouts/kanban/kanban-group.tsx(7 hunks)web/core/components/issues/issue-layouts/kanban/swimlanes.tsx(1 hunks)web/core/components/issues/issue-layouts/list/headers/group-by-card.tsx(5 hunks)web/core/components/issues/issue-layouts/list/list-group.tsx(8 hunks)web/core/components/project-states/index.ts(1 hunks)web/core/components/project-states/root.tsx(1 hunks)web/core/components/project-states/state-item-title.tsx(1 hunks)web/core/components/project-states/state-item.tsx(2 hunks)web/core/hooks/store/use-project-state.ts(1 hunks)web/core/layouts/auth-layout/project-wrapper.tsx(3 hunks)web/core/store/issue/issue-details/issue.store.ts(2 hunks)web/core/store/issue/issue-details/sub_issues.store.ts(3 hunks)web/core/store/issue/root.store.ts(0 hunks)web/core/store/root.store.ts(3 hunks)web/core/store/state.store.ts(6 hunks)web/ee/components/workflow/index.ts(1 hunks)web/ee/services/project/project-state.service.ts(1 hunks)web/ee/store/state.store.ts(1 hunks)web/ee/types/index.ts(1 hunks)web/helpers/array.helper.ts(1 hunks)
💤 Files with no reviewable changes (1)
- web/core/store/issue/root.store.ts
✅ Files skipped from review due to trivial changes (11)
- web/ee/store/state.store.ts
- web/ee/components/workflow/index.ts
- web/ce/services/index.ts
- web/ee/services/project/project-state.service.ts
- web/ce/components/workflow/state-transition-count.tsx
- web/ce/components/workflow/workflow-disabled-message.tsx
- web/ce/services/project/project-state.service.ts
- web/ce/types/index.ts
- web/ce/store/state.store.ts
- web/ce/components/workflow/workflow-group-tree.tsx
- web/ce/components/workflow/index.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
web/core/components/project-states/state-item-title.tsx
[error] 55-55: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
web/core/components/dropdowns/state.tsx
[error] 91-91: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
(lint/complexity/noExtraBooleanCast)
🔇 Additional comments (35)
web/ee/types/index.ts (1)
3-3: Verify import path and potential circular dependencies
The import path ce/types/state.d seems to reference a sibling directory. Consider using a more explicit import path to avoid potential issues with module resolution.
✅ Verification successful
Let me verify the import path and check for the actual state.d.ts file location.
Let me check the content of state.d.ts to understand what's being exported and verify if there are any circular dependencies.
Import path is valid and no circular dependencies found
The import path "ce/types/state.d" is valid as:
- The target file exists at
web/ce/types/state.d.ts - The file contains pure type definitions without any imports from "ee/"
- While many EE files import from CE, the CE file doesn't import from EE, avoiding circular dependencies
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for potential circular dependencies between ee and ce
rg -l "from \"ee/" web/ce/
rg -l "from \"ce/" web/ee/
Length of output: 3179
Script:
#!/bin/bash
# Check if state.d.ts exists in the expected location and its content
fd state.d.ts web/ce/types/
fd state.d.ts web/ce/
Length of output: 101
Script:
#!/bin/bash
# Check the content of state.d.ts and look for any imports from ee
cat web/ce/types/state.d.ts
rg "from \"ee/" web/ce/types/state.d.ts
Length of output: 240
web/ce/types/state.d.ts (2)
1-4: LGTM: Well-structured state transition interface
The IStateTransition interface clearly defines the structure for state transitions with appropriate types.
6-8: Verify transition state ID references
The IStateWorkFlow interface looks good, but let's ensure that the transition_state_id references are properly handled in the codebase.
✅ Verification successful
Interface and its usage are properly typed and consistent
Based on the search results, the IStateTransition interface and its usage in IStateWorkFlow are properly defined and consistently used throughout the codebase. The interface correctly defines transition_state_id as a string type, and the workflow interface properly maps transition IDs to state transitions. The codebase shows appropriate usage in components like AddStateTransition and state management through fetchProjectStateTransitions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for transition_state_id usage to ensure consistent typing
ast-grep --pattern 'transition_state_id: $_'
Length of output: 46
Script:
#!/bin/bash
# Let's search for any usage of transitionId or transition patterns in the codebase
rg -i "transition.*id|transitionId" -A 2
# Also search for IStateTransition interface definition
ast-grep --pattern 'interface IStateTransition {
$$$
}'
Length of output: 9582
web/core/components/project-states/root.tsx (2)
19-19: LGTM: Added state transitions fetching capability
The addition of fetchProjectStateTransitions aligns with the PR objective of refactoring project states.
23-29: Consider error handling for parallel fetches
While fetching both states and transitions in parallel is efficient, consider handling potential failures in either fetch operation.
Consider adding error handling:
workspaceSlug && projectId
? () => {
- fetchProjectStates(workspaceSlug.toString(), projectId.toString());
- fetchProjectStateTransitions(workspaceSlug.toString(), projectId.toString());
+ return Promise.all([
+ fetchProjectStates(workspaceSlug.toString(), projectId.toString()),
+ fetchProjectStateTransitions(workspaceSlug.toString(), projectId.toString())
+ ]).catch((error) => {
+ console.error("Failed to fetch project states:", error);
+ });
}
: null,web/core/store/root.store.ts (1)
2-4: Verify import dependencies
The addition of imports from "@/plane-web/store" suggests a potential circular dependency risk.
web/core/layouts/auth-layout/project-wrapper.tsx (1)
54-54: Verify implementation of fetchProjectStateTransitions
The fetchProjectStateTransitions function is called but appears to be a dummy implementation in the state store. This could lead to missing functionality in the state transition management.
Also applies to: 108-113
web/core/store/state.store.ts (1)
57-57: LGTM! Store restructuring and type safety improvements
The changes improve code organization and type safety by:
- Explicitly typing the stateService
- Properly integrating with the root store
Also applies to: 59-59, 61-61, 81-81
web/core/store/issue/issue-details/sub_issues.store.ts (1)
221-221: State retrieval refactoring looks good!
The changes consistently update the state retrieval path to use rootStore.state instead of accessing it through rootIssueStore. This centralization of state management aligns with the PR's objective and follows good practices.
Let's verify that this refactoring pattern is consistent across the codebase:
Also applies to: 226-226, 258-258, 293-293
✅ Verification successful
State retrieval pattern is consistently implemented
The codebase search confirms that all state retrievals follow the same pattern, accessing state through rootStore.state. No instances of direct state access through rootIssueStore.state were found, indicating successful and complete refactoring.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining direct state access through rootIssueStore
# that might have been missed during refactoring
# Search for potential missed refactoring spots
rg "rootIssueStore\.state\." --type ts
# Search for the new pattern to ensure consistency
rg "rootStore\.state\." --type ts
Length of output: 1501
web/core/components/issues/issue-layouts/kanban/kanban-group.tsx (6)
29-31: Approved import of useWorkFlowFDragNDrop
The import statement correctly adds useWorkFlowFDragNDrop from the specified path.
109-113: Correct initialization of useWorkFlowFDragNDrop hook
The useWorkFlowFDragNDrop hook is properly initialized with group_by and sub_group_by, and the returned values are correctly destructured.
Line range hint 150-157: Proper handling of drop disabled state
Including isWorkflowDropDisabled in the drop condition ensures that drops are correctly disabled when workflow constraints apply. The warning toast provides appropriate user feedback.
Line range hint 179-189: Updated dependency array in useEffect hook
Adding isWorkflowDropDisabled, dropErrorMessage, and handleOnDrop to the dependency array ensures that the effect responds correctly to changes in these variables.
259-260: Correctly updating overlay visibility logic
Adjusting canOverlayBeVisible to include isWorkflowDropDisabled ensures the overlay behaves appropriately under workflow constraints.
275-276: Properly propagating isDropDisabled prop
The GroupDragOverlay component now correctly receives the updated isDropDisabled prop, reflecting both workflow and drop states.
web/core/components/issues/issue-layouts/list/list-group.tsx (10)
29-31: Approved import of useWorkFlowFDragNDrop
The import statement correctly adds useWorkFlowFDragNDrop from the specified path.
98-98: Verify safe usage of optional chaining in isExpanded
Ensure that collapsedGroups?.group_by is always defined or consider providing a default value to prevent potential runtime errors when group_by is undefined.
110-111: Correct initialization of useWorkFlowFDragNDrop hook
The useWorkFlowFDragNDrop hook is properly initialized with group_by, and the returned values are correctly destructured.
192-193: Handling workflow state in onDrag event
The call to handleWorkFlowState within the onDrag event appropriately manages the workflow state during drag operations.
Line range hint 210-217: Proper handling of drop disabled state
Including isWorkflowDropDisabled in the drop condition ensures that drops are correctly disabled when workflow constraints apply. The warning toast provides appropriate user feedback.
224-225: Automatically expanding collapsed groups on drop
Expanding collapsed groups when an item is dropped enhances user experience by ensuring the dropped item is visible.
230-238: Updated dependency array in useEffect hook
Adding isWorkflowDropDisabled to the dependency array ensures that the effect responds correctly to changes in this variable.
242-242: Correctly updating overlay visibility logic
Adjusting canOverlayBeVisible to include isWorkflowDropDisabled ensures the overlay behaves appropriately under workflow constraints.
262-262: Passing groupBy prop to HeaderGroupByCard
Including the groupBy prop enhances the component's functionality in relation to the workflow grouping.
279-280: Properly propagating isDropDisabled prop
The GroupDragOverlay component now correctly receives the updated isDropDisabled prop, reflecting both workflow and drop states.
web/core/components/project-states/index.ts (1)
13-13: Approved addition of state-item-title export
The export statement correctly exposes state-item-title, enhancing modularity and access to this component.
web/ce/components/workflow/state-option.tsx (1)
20-35: LGTM! Well-structured component with proper accessibility
The component implementation is clean and follows best practices:
- Uses Headless UI for accessibility
- Properly handles active/selected states
- Clean JSX structure with meaningful class names
web/ce/components/workflow/state-item-child.tsx (1)
19-35: LGTM! Well-structured component with clear responsibilities
The component implementation is clean and follows best practices:
- Clear separation of concerns
- Proper props destructuring
- Meaningful class names for styling
web/core/components/issues/issue-layouts/kanban/headers/sub-group-by-card.tsx (1)
43-44: LGTM! Good integration of workflow grouping
The WorkFlowGroupTree integration is clean and properly typed with the necessary props.
web/core/components/issues/issue-layouts/group-drag-overlay.tsx (1)
13-13: LGTM: New prop enhances workflow state management
The addition of workflowDisabledSource prop aligns with the PR's objective of refactoring project states.
packages/ui/src/tooltip/tooltip.tsx (1)
26-26: LGTM: New prop enhances tooltip flexibility
The addition of jsxContent prop allows for richer tooltip content, which is beneficial for the project state refactoring.
web/core/components/issues/issue-layouts/list/headers/group-by-card.tsx (1)
119-119: Verify WorkFlowGroupTree integration with existing group handling
The addition of WorkFlowGroupTree enhances group visualization, but we should verify its interaction with the existing group collapse functionality.
✅ Verification successful
Based on the gathered information, I can now generate the final response:
WorkFlowGroupTree integration is safe and non-conflicting
The WorkFlowGroupTree component is a non-intrusive empty component (<></>) that coexists with the existing group collapse functionality. The collapse behavior is handled separately through handleCollapsedGroups and doesn't interfere with the tree visualization.
- The group collapse functionality remains intact through
handleCollapsedGroups(groupID) - WorkFlowGroupTree is rendered as a sibling element and doesn't affect the collapse behavior
- The empty implementation (
<></>) ensures no conflicts with existing functionality
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for potential conflicts between WorkFlowGroupTree and group collapse handling
ast-grep --pattern 'handleCollapsedGroups($groupId)'
Length of output: 54
Script:
#!/bin/bash
# Let's search for group collapse related functionality and WorkFlowGroupTree usage
rg -A 5 "WorkFlowGroupTree"
# Also search for any group collapse related state or handlers
rg -A 5 "collaps.*group" -i
# Check the WorkFlowGroupTree component implementation
fd -e tsx -e ts "WorkFlowGroupTree" --exec cat {}
Length of output: 69499
Script:
#!/bin/bash
# Let's check for any group collapse state management in the component
rg -A 5 "collapsedGroups|handleCollapse" web/core/components/issues/issue-layouts/list/headers/group-by-card.tsx
# Also check for any state management related to groups
rg -A 5 "groupBy|groupID" web/core/components/issues/issue-layouts/list/headers/group-by-card.tsx
# Check the implementation of WorkFlowGroupTree to understand its functionality
rg -A 10 "export const WorkFlowGroupTree" web/ce/components/workflow/workflow-group-tree.tsx
Length of output: 2333
web/core/components/issues/issue-layouts/kanban/headers/group-by-card.tsx (1)
18-19: Maintain import grouping consistency
The new import for WorkFlowGroupTree maintains proper separation in a "Plane-web" section, which is good for organization.
web/core/components/dropdowns/state.tsx (1)
87-89: LGTM! Clean and efficient state list computation.
The conditional computation with null filtering is a good approach for handling potentially invalid state IDs.
web/core/components/issues/issue-layouts/kanban/swimlanes.tsx (1)
173-173: LGTM! Consistent prop passing.
The addition of sub_group_by prop to HeaderSubGroupByCard maintains consistency with the parent component's interface.
| // Plane-web | ||
| import { IStateStore } from "@/plane-web/store/state.store"; |
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent import paths detected for state.store
Multiple files are still using the old import path @/store/state.store while some have been updated to use @/plane-web/store/state.store. This inconsistency needs to be addressed.
Locations requiring attention:
space/core/hooks/store/use-state.tsspace/core/components/issues/issue-layouts/utils.tsxweb/core/components/issues/issue-layouts/utils.tsxweb/ee/store/state.store.tsweb/ce/store/state.store.ts
🔗 Analysis chain
Verify consistency of @/plane-web imports
The import path has been updated to use @/plane-web/store/state.store. Let's ensure this pattern is consistent across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for inconsistent import patterns
rg "from \"@/store/state.store\""
rg "from \"@/plane-web/store/state.store\""
Length of output: 705
| // fetching states | ||
| // TODO: check if this function is required | ||
| this.rootIssueDetailStore.rootIssueStore.state.fetchProjectStates(workspaceSlug, projectId); | ||
| this.rootIssueDetailStore.rootIssueStore.rootStore.state.fetchProjectStates(workspaceSlug, projectId); |
There was a problem hiding this comment.
💡 Codebase verification
Based on the context gathered, I can now provide the final response:
Remove redundant fetchProjectStates call
The fetchProjectStates call appears to be redundant here as:
- Project states are already being fetched in multiple UI components when needed (dropdowns, project-states, project-wrapper, hooks)
- The TODO comment and the surrounding code context shows this fetch isn't directly related to the issue details being processed
- The state_id is only being used as a pass-through property, not requiring immediate state data
🔗 Analysis chain
Review necessity of fetchProjectStates call
There's a TODO comment questioning if this function call is required. Additionally, the store access path has been modified to use rootStore. Consider:
- Evaluating if this state fetch is necessary at this point
- Documenting why it's needed if it is determined to be required
Let me gather more context about the specific location where this fetch is being called to understand its necessity.
Let me check one more thing to understand if the states are being used immediately after this fetch.
Let me try a different pattern to get the method implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check where else project states are being fetched
# to determine if this fetch is redundant
# Search for fetchProjectStates calls
ast-grep --pattern 'fetchProjectStates($$$)'
Length of output: 545
Script:
#!/bin/bash
# Get context around the fetchProjectStates call in issue.store.ts
rg -B 10 -A 2 "fetchProjectStates" web/core/store/issue/issue-details/issue.store.ts
# Also check for any state-related operations nearby
rg -B 5 -A 5 "state" web/core/store/issue/issue-details/issue.store.ts
Length of output: 1832
Script:
#!/bin/bash
# Get the full method implementation to understand the context
ast-grep --pattern 'fetchIssueDetails($$$) {
$$$
}'
Length of output: 55
Script:
#!/bin/bash
# Get the full method implementation to understand the context
ast-grep --pattern 'fetchIssueDetails = async ($$$) => {
$$$
}'
Length of output: 67
| getAvailableProjectStateIdMap: ( | ||
| projectId: string | null | undefined, | ||
| currStateId: string | null | undefined | ||
| ) => { [key: string]: boolean }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance getAvailableProjectStateIdMap implementation
The current implementation has several limitations:
- The
currStateIdparameter is unused but could be valuable for determining valid state transitions - The method returns all states as available without considering transition rules or permissions
Consider enhancing the implementation to:
getAvailableProjectStateIdMap = computedFn(
(projectId: string | null | undefined, currStateId: string | null | undefined) => {
const projectStates = this.getProjectStates(projectId);
+ const currentState = this.getStateById(currStateId);
if (!projectStates) return {};
- return convertStringArrayToBooleanObject(projectStates.map((projectState) => projectState.id));
+ // Filter states based on transition rules and permissions
+ const availableStates = projectStates.filter((state) => {
+ // Add transition rules and permission checks here
+ return true;
+ });
+
+ return convertStringArrayToBooleanObject(availableStates.map((state) => state.id));
}
);Also applies to: 131-143
|
|
||
| // Dummy method | ||
| fetchProjectStateTransitions = (workspaceSlug: string, projectId: string) => {}; |
There was a problem hiding this comment.
Implement fetchProjectStateTransitions functionality
The method is currently a no-op placeholder. This needs to be implemented to support the state transition management being introduced.
Would you like help implementing this method? I can provide a basic implementation that:
- Fetches state transitions from the API
- Updates the store with the transition rules
- Integrates with the getAvailableProjectStateIdMap method
| import { TIssueGroupByOptions } from "@plane/types"; | ||
|
|
||
| export const useWorkFlowFDragNDrop = ( | ||
| groupBy: TIssueGroupByOptions | undefined, | ||
| subGroupBy?: TIssueGroupByOptions | ||
| ) => ({ | ||
| workflowDisabledSource: undefined, | ||
| isWorkflowDropDisabled: false, | ||
| handleWorkFlowState: ( | ||
| sourceGroupId: string, | ||
| destinationGroupId: string, | ||
| sourceSubGroupId?: string, | ||
| destinationSubGroupId?: string | ||
| ) => {}, | ||
| }); |
There was a problem hiding this comment.
Empty implementation in useWorkFlowFDragNDrop hook
The useWorkFlowFDragNDrop hook currently returns default values, and the handleWorkFlowState function lacks implementation. Ensure that the necessary logic is implemented to manage workflow drag-and-drop states effectively.
| export const AddStateTransition = (props: Props) => ( | ||
| <div className={cn("flex w-full px-3 h-6 items-center justify-start gap-2 text-sm bg-custom-background-90")}> | ||
| <> | ||
| <Plus className="h-4 w-4" color="#8591AD" /> | ||
| <span className="text-custom-text-400 font-medium"> Add Transition</span> | ||
| <div className="text-white bg-custom-background-80 font-semibold px-2 rounded-lg">Pro</div> | ||
| </> | ||
| </div> | ||
| ); |
There was a problem hiding this comment.
AddStateTransition component lacks interaction handling
The onTransitionAdd prop is defined but not utilized. Consider adding an onClick handler to invoke onTransitionAdd when the component is clicked.
Apply this diff to add the onClick handler:
export const AddStateTransition = (props: Props) => (
<div
className={cn("flex w-full px-3 h-6 items-center justify-start gap-2 text-sm bg-custom-background-90")}
+ onClick={props.onTransitionAdd}
>
<Plus className="h-4 w-4" color="#8591AD" />
<span className="text-custom-text-400 font-medium"> Add Transition</span>
<div className="text-white bg-custom-background-80 font-semibold px-2 rounded-lg">Pro</div>
</div>
);Committable suggestion skipped: line range outside the PR's diff.
| type Props = { | ||
| projectId: string | null | undefined; | ||
| option: { | ||
| value: string | undefined; | ||
| query: string; | ||
| content: JSX.Element; | ||
| }; | ||
| filterAvailableStateIds: boolean; | ||
| selectedValue: string | null | undefined; | ||
| className?: string; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider strengthening prop types and removing unused props
The Props type definition has some potential improvements:
projectIdandselectedValuebeing nullable/undefined might lead to runtime issuesfilterAvailableStateIdsis defined but not used in the component
Consider this improvement:
type Props = {
- projectId: string | null | undefined;
+ projectId: string;
option: {
- value: string | undefined;
+ value: string;
query: string;
content: JSX.Element;
};
- filterAvailableStateIds: boolean;
- selectedValue: string | null | undefined;
+ selectedValue: string;
className?: string;
};📝 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.
| type Props = { | |
| projectId: string | null | undefined; | |
| option: { | |
| value: string | undefined; | |
| query: string; | |
| content: JSX.Element; | |
| }; | |
| filterAvailableStateIds: boolean; | |
| selectedValue: string | null | undefined; | |
| className?: string; | |
| }; | |
| type Props = { | |
| projectId: string; | |
| option: { | |
| value: string; | |
| query: string; | |
| content: JSX.Element; | |
| }; | |
| selectedValue: string; | |
| className?: string; | |
| }; |
| {!disabled && stateCount != 1 && ( | ||
| <div className="flex-shrink-0 w-3 h-3 rounded-sm absolute left-0 hidden group-hover:flex justify-center items-center transition-colors bg-custom-background-90 cursor-pointer text-custom-text-200 hover:text-custom-text-100"> | ||
| <GripVertical className="w-3 h-3" /> | ||
| </div> | ||
| )} |
There was a problem hiding this comment.
💡 Codebase verification
Verify drag handle accessibility
The drag handle implementation lacks proper keyboard accessibility and ARIA attributes. It should be enhanced with:
role="button"attributearia-labelto describe the drag actiontabIndexto make it keyboard focusable- Keyboard event handlers for Enter/Space keys
Here are the specific locations that need attention:
web/core/components/project-states/state-item-title.tsx: Lines 29-33<div className="flex-shrink-0 w-3 h-3 rounded-sm absolute left-0 hidden group-hover:flex justify-center items-center transition-colors bg-custom-background-90 cursor-pointer text-custom-text-200 hover:text-custom-text-100"> <GripVertical className="w-3 h-3" /> </div>
🔗 Analysis chain
Verify drag handle accessibility
The drag handle should be keyboard accessible and have proper ARIA attributes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for other drag handle implementations in the codebase
rg -A 2 'role="button".*drag' --type tsx
Length of output: 70
Script:
#!/bin/bash
# Check for drag handle implementations and accessibility patterns
rg -t ts -t tsx -A 5 'GripVertical|draggable|drag'
Length of output: 80
Script:
#!/bin/bash
# Look for aria attributes and role assignments in the codebase
ast-grep --pattern 'role="$_"'
Length of output: 32
Script:
#!/bin/bash
# Find keyboard event handlers
rg -t ts -t tsx -A 3 'onKeyDown|onKeyPress|onKeyUp'
Length of output: 81
Script:
#!/bin/bash
# Search for any file content with grip, drag, or keyboard handlers
rg "GripVertical|draggable|drag|onKeyDown|onKeyPress|onKeyUp|role=\"button\""
# Also check the specific file content
cat "web/core/components/project-states/state-item-title.tsx"
Length of output: 52004
| <StateOption | ||
| key={option.value} | ||
| value={option.value} | ||
| className={({ active, selected }) => | ||
| `flex w-full cursor-pointer select-none items-center justify-between gap-2 truncate rounded px-1 py-1.5 ${ | ||
| active ? "bg-custom-background-80" : "" | ||
| } ${selected ? "text-custom-text-100" : "text-custom-text-200"}` | ||
| } | ||
| > | ||
| {({ selected }) => ( | ||
| <> | ||
| <span className="flex-grow truncate">{option.content}</span> | ||
| {selected && <Check className="h-3.5 w-3.5 flex-shrink-0" />} | ||
| </> | ||
| )} | ||
| </Combobox.Option> | ||
| option={option} | ||
| projectId={projectId} | ||
| filterAvailableStateIds={filterAvailableStateIds} | ||
| selectedValue={value} | ||
| className="flex w-full cursor-pointer select-none items-center justify-between gap-2 truncate rounded px-1 py-1.5" | ||
| /> |
There was a problem hiding this comment.
💡 Codebase verification
StateOption integration looks correct but props mismatch detected
The StateOption component in web/ce/components/workflow/state-option.tsx only accepts option and className props, but it's being called with additional props (projectId, filterAvailableStateIds, selectedValue) that aren't used in its implementation. This could lead to potential issues:
- The component ignores
projectId,filterAvailableStateIds, andselectedValueprops that are being passed to it - If these props are meant to affect the component's behavior, the functionality is currently missing
🔗 Analysis chain
LGTM! Verify StateOption component integration.
The integration of the custom StateOption component looks good. However, let's verify its implementation to ensure it handles all edge cases properly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check StateOption component implementation
ast-grep --pattern 'export const StateOption = ({ option, projectId, filterAvailableStateIds, selectedValue, className }) => {
$$$
}'
Length of output: 137
Script:
#!/bin/bash
# Let's try a broader search for StateOption component
ast-grep --pattern 'const StateOption = $_'
# Also search for any file containing StateOption
rg "StateOption" -A 10
Length of output: 4245
2 participants
This PR refactors the project states to make way for upcoming features.
Summary by CodeRabbit
Release Notes
New Features
jsxContentproperty.AddStateTransition,StateItemChild,StateOption, andStateTransitionCountcomponents to enhance workflow management.WorkFlowDisabledMessageandWorkFlowGroupTreecomponents for better workflow state representation.Bug Fixes
Documentation
Chores