[WEB-4457] refactor: decouple work item properties from mobx store#7363
[WEB-4457] refactor: decouple work item properties from mobx store#7363
Conversation
WalkthroughThis change refactors dropdown and select components for projects, modules, members, states, and labels to decouple them from internal store/state dependencies. Components now receive explicit data and lookup functions as props, and new wrapper components are introduced to bridge store data to these base components. The Changes
Sequence Diagram(s)sequenceDiagram
participant Store
participant WrapperComponent
participant BaseComponent
Store->>WrapperComponent: Provides data (IDs, lookup functions)
WrapperComponent->>BaseComponent: Passes IDs, lookup, and handlers as props
BaseComponent->>BaseComponent: Renders UI using provided data
BaseComponent-->>WrapperComponent: Calls event handlers (e.g., onDropdownOpen)
WrapperComponent-->>Store: Triggers data fetch if needed
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
|
Pull Request Linked with Plane Work Items Comment Automatically Generated by Plane |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (2)
apps/web/core/components/dropdowns/module/base.tsx (1)
167-167: Avoid usingas anytype casting.The
onChangeprop is being cast toany, which bypasses TypeScript's type checking and could hide potential type mismatches. Consider fixing the type definitions to ensure type safety.- onChange={onChange as any} + onChange={onChange}If there's a type mismatch, update the
ModuleButtonContentprops type to properly handle both single and multiple selection onChange signatures, or create a union type that accommodates both cases.apps/web/core/components/dropdowns/project/base.tsx (1)
93-110: Use filter-map pattern to avoid undefined values in array.The current implementation creates undefined values in the options array when
renderConditionreturns false, which are filtered out later. Consider using a filter-map pattern for cleaner code.- const options = projectIds?.map((projectId) => { - const projectDetails = getProjectById(projectId); - if (renderCondition && !renderCondition(projectId)) return; - return { - value: projectId, - query: `${projectDetails?.name}`, - content: ( - <div className="flex items-center gap-2"> - {projectDetails?.logo_props && ( - <span className="grid place-items-center flex-shrink-0 h-4 w-4"> - <Logo logo={projectDetails?.logo_props} size={12} /> - </span> - )} - <span className="flex-grow truncate">{projectDetails?.name}</span> - </div> - ), - }; - }); + const options = projectIds + ?.filter((projectId) => !renderCondition || renderCondition(projectId)) + ?.map((projectId) => { + const projectDetails = getProjectById(projectId); + return { + value: projectId, + query: `${projectDetails?.name}`, + content: ( + <div className="flex items-center gap-2"> + {projectDetails?.logo_props && ( + <span className="grid place-items-center flex-shrink-0 h-4 w-4"> + <Logo logo={projectDetails?.logo_props} size={12} /> + </span> + )} + <span className="flex-grow truncate">{projectDetails?.name}</span> + </div> + ), + }; + });This eliminates the need to filter out undefined values in
filteredOptions.
🧹 Nitpick comments (4)
apps/web/helpers/react-hook-form.helper.ts (1)
10-19: Consider edge case handling for path validation.The function handles object traversal well, but consider adding validation for edge cases like empty paths or malformed paths.
export const getNestedError = <T extends FieldValues>(errors: T, path: string): FieldError | undefined => { + // Handle empty or invalid paths + if (!path || !path.trim()) { + return undefined; + } + const keys = path.split("."); let current: unknown = errors;This would gracefully handle cases where an empty string or whitespace-only path is passed.
apps/web/core/components/dropdowns/module/dropdown.tsx (1)
52-52: Remove unnecessary nullish coalescing.The
moduleIds ?? []is unnecessary sincemoduleIdsis already guaranteed to be an array (either fromgetProjectModuleIds()or the fallback[]on line 42).- moduleIds={moduleIds ?? []} + moduleIds={moduleIds}apps/web/core/components/dropdowns/member/dropdown.tsx (1)
44-44: Remove unnecessary nullish coalescing.The
memberIds ?? []is unnecessary since the resolution logic in lines 30-34 already ensuresmemberIdsis not null/undefined (it falls back toworkspaceMemberIds).- memberIds={memberIds ?? []} + memberIds={memberIds}apps/web/core/components/dropdowns/module/button-content.tsx (1)
40-40: Consider decoupling from store hooks for consistency.The component uses
useModulehook internally, which is inconsistent with the overall refactoring pattern of decoupling components from store dependencies. Consider receivinggetModuleByIdas a prop instead.-// store hooks -const { getModuleById } = useModule(); +type ModuleButtonContentProps = { + getModuleById: (moduleId: string) => IModule | null; disabled: boolean; // ... other props };Then update the component to use the prop:
export const ModuleButtonContent: React.FC<ModuleButtonContentProps> = (props) => { const { + getModuleById, disabled, // ... other props } = props; - // store hooks - const { getModuleById } = useModule();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
apps/web/core/components/cycles/form.tsx(1 hunks)apps/web/core/components/dropdowns/index.ts(1 hunks)apps/web/core/components/dropdowns/member/base.tsx(3 hunks)apps/web/core/components/dropdowns/member/dropdown.tsx(1 hunks)apps/web/core/components/dropdowns/member/member-options.tsx(2 hunks)apps/web/core/components/dropdowns/module/base.tsx(6 hunks)apps/web/core/components/dropdowns/module/button-content.tsx(1 hunks)apps/web/core/components/dropdowns/module/dropdown.tsx(1 hunks)apps/web/core/components/dropdowns/module/module-options.tsx(3 hunks)apps/web/core/components/dropdowns/project/base.tsx(6 hunks)apps/web/core/components/dropdowns/project/dropdown.tsx(1 hunks)apps/web/core/components/dropdowns/state/base.tsx(4 hunks)apps/web/core/components/dropdowns/state/dropdown.tsx(1 hunks)apps/web/core/components/issues/issue-modal/components/project-select.tsx(1 hunks)apps/web/core/components/issues/issue-modal/form.tsx(4 hunks)apps/web/core/components/issues/select/base.tsx(3 hunks)apps/web/core/components/issues/select/dropdown.tsx(1 hunks)apps/web/core/components/issues/select/index.ts(1 hunks)apps/web/core/components/labels/create-label-modal.tsx(3 hunks)apps/web/core/components/modules/form.tsx(1 hunks)apps/web/helpers/react-hook-form.helper.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (12)
📓 Common learnings
Learnt from: prateekshourya29
PR: makeplane/plane#7188
File: web/app/(all)/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/cycles/(list)/header.tsx:40-45
Timestamp: 2025-06-18T09:46:08.566Z
Learning: When reviewing breadcrumb components that accept projectId or similar props, check if empty strings are being passed during loading states, which can result in invalid URLs. The preferred approach is to handle these loading states internally within the component rather than requiring each consumer to manage the loading logic.
apps/web/core/components/issues/issue-modal/components/project-select.tsx (2)
Learnt from: vamsikrishnamathala
PR: makeplane/plane#7214
File: web/core/store/issue/helpers/base-issues.store.ts:117-117
Timestamp: 2025-06-16T07:23:39.497Z
Learning: In the updateIssueDates method of BaseIssuesStore (web/core/store/issue/helpers/base-issues.store.ts), the projectId parameter is intentionally made optional to support override implementations in subclasses. The base implementation requires projectId and includes an early return check, but making it optional allows derived classes to override the method with different parameter requirements.
Learnt from: prateekshourya29
PR: makeplane/plane#7188
File: web/app/(all)/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/cycles/(list)/header.tsx:40-45
Timestamp: 2025-06-18T09:46:08.566Z
Learning: When reviewing breadcrumb components that accept projectId or similar props, check if empty strings are being passed during loading states, which can result in invalid URLs. The preferred approach is to handle these loading states internally within the component rather than requiring each consumer to manage the loading logic.
apps/web/core/components/modules/form.tsx (2)
Learnt from: vamsikrishnamathala
PR: makeplane/plane#7214
File: web/core/store/issue/helpers/base-issues.store.ts:117-117
Timestamp: 2025-06-16T07:23:39.497Z
Learning: In the updateIssueDates method of BaseIssuesStore (web/core/store/issue/helpers/base-issues.store.ts), the projectId parameter is intentionally made optional to support override implementations in subclasses. The base implementation requires projectId and includes an early return check, but making it optional allows derived classes to override the method with different parameter requirements.
Learnt from: prateekshourya29
PR: makeplane/plane#7188
File: web/app/(all)/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/cycles/(list)/header.tsx:40-45
Timestamp: 2025-06-18T09:46:08.566Z
Learning: When reviewing breadcrumb components that accept projectId or similar props, check if empty strings are being passed during loading states, which can result in invalid URLs. The preferred approach is to handle these loading states internally within the component rather than requiring each consumer to manage the loading logic.
apps/web/core/components/issues/issue-modal/form.tsx (2)
Learnt from: vamsikrishnamathala
PR: makeplane/plane#7214
File: web/core/store/issue/helpers/base-issues.store.ts:117-117
Timestamp: 2025-06-16T07:23:39.497Z
Learning: In the updateIssueDates method of BaseIssuesStore (web/core/store/issue/helpers/base-issues.store.ts), the projectId parameter is intentionally made optional to support override implementations in subclasses. The base implementation requires projectId and includes an early return check, but making it optional allows derived classes to override the method with different parameter requirements.
Learnt from: vamsikrishnamathala
PR: makeplane/plane#7061
File: web/core/components/workspace-notifications/root.tsx:18-18
Timestamp: 2025-05-14T13:16:23.323Z
Learning: In the Plane project codebase, the path alias `@/plane-web` resolves to the `ce` directory, making imports like `@/plane-web/hooks/use-notification-preview` correctly resolve to files in `web/ce/hooks/`.
apps/web/core/components/cycles/form.tsx (2)
Learnt from: vamsikrishnamathala
PR: makeplane/plane#7214
File: web/core/store/issue/helpers/base-issues.store.ts:117-117
Timestamp: 2025-06-16T07:23:39.497Z
Learning: In the updateIssueDates method of BaseIssuesStore (web/core/store/issue/helpers/base-issues.store.ts), the projectId parameter is intentionally made optional to support override implementations in subclasses. The base implementation requires projectId and includes an early return check, but making it optional allows derived classes to override the method with different parameter requirements.
Learnt from: prateekshourya29
PR: makeplane/plane#7188
File: web/app/(all)/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/cycles/(list)/header.tsx:40-45
Timestamp: 2025-06-18T09:46:08.566Z
Learning: When reviewing breadcrumb components that accept projectId or similar props, check if empty strings are being passed during loading states, which can result in invalid URLs. The preferred approach is to handle these loading states internally within the component rather than requiring each consumer to manage the loading logic.
apps/web/core/components/dropdowns/index.ts (2)
Learnt from: mathalav55
PR: makeplane/plane#6107
File: web/ce/components/workspace-notifications/sidebar/notification-card/options/read.tsx:11-12
Timestamp: 2024-11-28T07:02:15.514Z
Learning: Some components are still in core and have not been moved yet, so their import paths remain the same.
Learnt from: mathalav55
PR: makeplane/plane#6107
File: web/ce/components/workspace-notifications/sidebar/notification-card/options/archive.tsx:11-14
Timestamp: 2024-11-28T07:02:54.664Z
Learning: When components are still located in `core`, it's appropriate for files to import them using `@/components/...`, and the migration to the new import paths is not necessary in such cases.
apps/web/core/components/issues/select/dropdown.tsx (1)
Learnt from: vamsikrishnamathala
PR: makeplane/plane#7214
File: web/core/store/issue/helpers/base-issues.store.ts:117-117
Timestamp: 2025-06-16T07:23:39.497Z
Learning: In the updateIssueDates method of BaseIssuesStore (web/core/store/issue/helpers/base-issues.store.ts), the projectId parameter is intentionally made optional to support override implementations in subclasses. The base implementation requires projectId and includes an early return check, but making it optional allows derived classes to override the method with different parameter requirements.
apps/web/core/components/labels/create-label-modal.tsx (1)
Learnt from: vamsikrishnamathala
PR: makeplane/plane#7214
File: web/core/store/issue/helpers/base-issues.store.ts:117-117
Timestamp: 2025-06-16T07:23:39.497Z
Learning: In the updateIssueDates method of BaseIssuesStore (web/core/store/issue/helpers/base-issues.store.ts), the projectId parameter is intentionally made optional to support override implementations in subclasses. The base implementation requires projectId and includes an early return check, but making it optional allows derived classes to override the method with different parameter requirements.
apps/web/core/components/dropdowns/module/module-options.tsx (1)
Learnt from: vamsikrishnamathala
PR: makeplane/plane#7214
File: web/core/store/issue/helpers/base-issues.store.ts:117-117
Timestamp: 2025-06-16T07:23:39.497Z
Learning: In the updateIssueDates method of BaseIssuesStore (web/core/store/issue/helpers/base-issues.store.ts), the projectId parameter is intentionally made optional to support override implementations in subclasses. The base implementation requires projectId and includes an early return check, but making it optional allows derived classes to override the method with different parameter requirements.
apps/web/core/components/dropdowns/project/base.tsx (3)
Learnt from: vamsikrishnamathala
PR: makeplane/plane#7214
File: web/core/store/issue/helpers/base-issues.store.ts:117-117
Timestamp: 2025-06-16T07:23:39.497Z
Learning: In the updateIssueDates method of BaseIssuesStore (web/core/store/issue/helpers/base-issues.store.ts), the projectId parameter is intentionally made optional to support override implementations in subclasses. The base implementation requires projectId and includes an early return check, but making it optional allows derived classes to override the method with different parameter requirements.
Learnt from: vamsikrishnamathala
PR: makeplane/plane#7061
File: web/core/components/workspace-notifications/root.tsx:18-18
Timestamp: 2025-05-14T13:16:23.323Z
Learning: In the Plane project codebase, the path alias `@/plane-web` resolves to the `ce` directory, making imports like `@/plane-web/hooks/use-notification-preview` correctly resolve to files in `web/ce/hooks/`.
Learnt from: prateekshourya29
PR: makeplane/plane#7188
File: web/app/(all)/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/cycles/(list)/header.tsx:40-45
Timestamp: 2025-06-18T09:46:08.566Z
Learning: When reviewing breadcrumb components that accept projectId or similar props, check if empty strings are being passed during loading states, which can result in invalid URLs. The preferred approach is to handle these loading states internally within the component rather than requiring each consumer to manage the loading logic.
apps/web/core/components/issues/select/base.tsx (1)
Learnt from: vamsikrishnamathala
PR: makeplane/plane#7214
File: web/core/store/issue/helpers/base-issues.store.ts:117-117
Timestamp: 2025-06-16T07:23:39.497Z
Learning: In the updateIssueDates method of BaseIssuesStore (web/core/store/issue/helpers/base-issues.store.ts), the projectId parameter is intentionally made optional to support override implementations in subclasses. The base implementation requires projectId and includes an early return check, but making it optional allows derived classes to override the method with different parameter requirements.
apps/web/core/components/dropdowns/state/base.tsx (1)
Learnt from: prateekshourya29
PR: makeplane/plane#7188
File: web/app/(all)/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/cycles/(list)/header.tsx:40-45
Timestamp: 2025-06-18T09:46:08.566Z
Learning: When reviewing breadcrumb components that accept projectId or similar props, check if empty strings are being passed during loading states, which can result in invalid URLs. The preferred approach is to handle these loading states internally within the component rather than requiring each consumer to manage the loading logic.
🧬 Code Graph Analysis (7)
apps/web/core/components/dropdowns/project/dropdown.tsx (1)
apps/web/core/components/dropdowns/project/base.tsx (1)
ProjectDropdownBase(44-287)
apps/web/core/components/dropdowns/module/dropdown.tsx (1)
apps/web/core/components/dropdowns/module/base.tsx (1)
ModuleDropdownBase(45-198)
apps/web/core/components/dropdowns/module/button-content.tsx (2)
packages/utils/src/common.ts (1)
cn(8-8)packages/ui/src/tooltip/tooltip.tsx (1)
Tooltip(36-110)
apps/web/core/components/issues/select/dropdown.tsx (1)
apps/web/core/components/issues/select/base.tsx (2)
TWorkItemLabelSelectBaseProps(18-32)WorkItemLabelSelectBase(34-267)
apps/web/core/components/dropdowns/member/base.tsx (2)
packages/utils/src/common.ts (1)
cn(8-8)apps/web/core/components/dropdowns/member/member-options.tsx (1)
MemberOptions(30-161)
apps/web/core/components/dropdowns/module/base.tsx (1)
apps/web/core/components/dropdowns/module/button-content.tsx (1)
ModuleButtonContent(25-129)
apps/web/core/components/dropdowns/state/dropdown.tsx (1)
apps/web/core/components/dropdowns/state/base.tsx (2)
TWorkItemStateDropdownBaseProps(22-40)WorkItemStateDropdownBase(42-253)
🪛 Biome (1.9.4)
apps/web/core/components/dropdowns/state/base.tsx
[error] 80-80: 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)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (31)
apps/web/helpers/react-hook-form.helper.ts (2)
1-8: LGTM! Good documentation and imports.The imports are appropriate and the JSDoc documentation clearly explains the function's purpose and parameters.
9-27: Well-structured utility function that aligns with the refactoring goals.This helper function provides a clean, type-safe way to access nested form errors, which supports the broader architectural shift toward more controlled and decoupled form components mentioned in the PR objectives.
apps/web/core/components/issues/select/index.ts (1)
1-1: API change aligns with refactor objectives.The export change from
"./label"to"./dropdown"is part of the coordinated refactor to decouple dropdown components from store dependencies. This change ensures consumers receive the updated dropdown-based implementation.apps/web/core/components/issues/issue-modal/components/project-select.tsx (1)
49-49: API update correctly implements the refactor pattern.The
renderConditionprop change from accepting a project object to a project ID string is consistent with the broader refactor to decouple dropdown components from store dependencies. The logic correctly uses direct ID lookup inallowedProjectIds.includes(projectId).apps/web/core/components/issues/issue-modal/form.tsx (3)
35-35: Hook import supports the decoupling refactor.The
useLabelhook import is correctly added to support the new label creation pattern where the modal receives an explicitcreateLabelfunction instead of relying on internal store hooks.
127-127: Correct destructuring of createLabel function.The
createLabelfunction is properly destructured from theuseLabelhook to support the new prop-based label creation approach.
366-366: Proper function binding for decoupled modal.The
createLabelfunction is correctly bound withworkspaceSlugandprojectIdparameters before being passed toCreateLabelModal. This approach provides the necessary context while maintaining the decoupled API design.apps/web/core/components/cycles/form.tsx (1)
86-86: Correctly implements the standardized renderCondition API.The
renderConditionprop change from accepting a project object to a project ID string is consistent with the broader refactor. The permission check logic correctly uses direct ID lookup with!!projectsWithCreatePermissions?.[projectId].apps/web/core/components/modules/form.tsx (1)
96-96: Consistent implementation of the refactor pattern.The
renderConditionprop change from accepting a project object to a project ID string aligns with the coordinated refactor across all dropdown components. The permission check logic correctly uses!!projectsWithCreatePermissions?.[projectId]and maintains existing functionality.apps/web/core/components/dropdowns/index.ts (1)
1-1: Export paths correctly updated to reflect new component structure.The export paths have been properly updated to point to the new dropdown wrapper components in their respective subdirectories. This aligns with the refactoring pattern where base components are separated from store-connected wrapper components.
Also applies to: 7-7, 9-10
apps/web/core/components/issues/select/dropdown.tsx (2)
13-35: Well-structured wrapper component following the refactoring pattern.The component correctly implements the wrapper pattern by:
- Using observer for reactive updates
- Extracting store data and providing it to the base component
- Implementing conditional fetching logic
- Properly forwarding props
The implementation aligns well with the refactoring objectives.
22-25: Conditional fetching logic is correct and needs no changes
getProjectLabelIdsreturnsundefinedonly when labels haven’t been fetched yet and thereafter always returns an array (possibly empty). The current check (projectLabelIds === undefined) will reliably trigger exactly one fetch and never misinterpretnullor an empty array as “not fetched.”apps/web/core/components/dropdowns/project/dropdown.tsx (2)
30-35: Simple and correct wrapper implementation.The component correctly implements the wrapper pattern by using the project store hook and passing the data to the base component. The implementation is clean and follows the established pattern.
Note that unlike other dropdown wrappers, this one doesn't include conditional fetching logic, which suggests project data is already available when needed.
9-28: Well-designed union type for single/multiple selection modes.The union type correctly enforces that
multiple,onChange, andvalueprops are consistent with each other. This prevents invalid combinations likemultiple: truewith a non-array value.apps/web/core/components/dropdowns/module/dropdown.tsx (2)
17-17: Consider making projectId required.Since the component's functionality depends heavily on
projectId(for both getting module IDs and fetching modules), consider making it required rather than optional. An undefinedprojectIdresults in an empty array and no fetching capability.⛔ Skipped due to learnings
Learnt from: prateekshourya29 PR: makeplane/plane#7188 File: web/app/(all)/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/cycles/(list)/header.tsx:40-45 Timestamp: 2025-06-18T09:46:08.566Z Learning: When reviewing breadcrumb components that accept projectId or similar props, check if empty strings are being passed during loading states, which can result in invalid URLs. The preferred approach is to handle these loading states internally within the component rather than requiring each consumer to manage the loading logic.Learnt from: vamsikrishnamathala PR: makeplane/plane#7214 File: web/core/store/issue/helpers/base-issues.store.ts:117-117 Timestamp: 2025-06-16T07:23:39.497Z Learning: In the updateIssueDates method of BaseIssuesStore (web/core/store/issue/helpers/base-issues.store.ts), the projectId parameter is intentionally made optional to support override implementations in subclasses. The base implementation requires projectId and includes an early return check, but making it optional allows derived classes to override the method with different parameter requirements.
44-46: Fix the conditional fetching logic.The condition
!moduleIdswill betruewhenmoduleIdsis an empty array[], which is falsy in JavaScript. This means the component will attempt to fetch modules even when an empty array is intentionally returned (e.g., when a project has no modules).Consider checking for
undefinedornullspecifically:- const onDropdownOpen = () => { - if (!moduleIds && projectId && workspaceSlug) fetchModules(workspaceSlug.toString(), projectId); - }; + const onDropdownOpen = () => { + if (moduleIds === undefined && projectId && workspaceSlug) fetchModules(workspaceSlug.toString(), projectId); + };⛔ Skipped due to learnings
Learnt from: prateekshourya29 PR: makeplane/plane#7188 File: web/app/(all)/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/cycles/(list)/header.tsx:40-45 Timestamp: 2025-06-18T09:46:08.566Z Learning: When reviewing breadcrumb components that accept projectId or similar props, check if empty strings are being passed during loading states, which can result in invalid URLs. The preferred approach is to handle these loading states internally within the component rather than requiring each consumer to manage the loading logic.apps/web/core/components/dropdowns/member/dropdown.tsx (2)
30-34: Well-structured member ID resolution logic.The cascading logic for determining member IDs is well-structured:
- Use explicitly provided
memberIdsif available- Fall back to project member IDs if
projectIdis provided- Use workspace member IDs as final fallback
This provides good flexibility for different use cases.
36-38: Fix the conditional fetching logic.Similar to the ModuleDropdown, the condition
!memberIdswill betruewhenmemberIdsis an empty array[]. This could cause unnecessary fetching when a project intentionally has no members.- const onDropdownOpen = () => { - if (!memberIds && projectId && workspaceSlug) fetchProjectMembers(workspaceSlug.toString(), projectId); - }; + const onDropdownOpen = () => { + if (memberIds === undefined && projectId && workspaceSlug) fetchProjectMembers(workspaceSlug.toString(), projectId); + };Likely an incorrect or invalid review comment.
apps/web/core/components/dropdowns/member/member-options.tsx (2)
21-24: Good refactoring to decouple from store dependencies.The new props interface effectively separates data fetching from UI rendering by requiring
getUserDetailscallback and acceptingmemberIdsas a prop. This improves testability and component reusability.
79-95: Confirm Upstream Guest-User FilteringI wasn’t able to find any logic in MemberOptions or its parent components that excludes guest users. Before merging, please verify that guest accounts are still being filtered out upstream—otherwise they’ll appear in this dropdown.
Points to double-check:
- Where memberIds is populated (e.g. your data-fetch hook or selector that returns project members)
- Any service/API layers (e.g. fetchProjectMembers or GraphQL queries) that should omit guests
- The component that passes memberIds into (in base.tsx)
apps/web/core/components/dropdowns/module/module-options.tsx (2)
26-33: Consistent refactoring pattern with improved separation of concerns.The new props interface follows the same pattern as other dropdown components, receiving data via
getModuleByIdcallback andmoduleIdsprop. This effectively decouples the component from internal store dependencies.
81-93: Clean data transformation using callback pattern.The options construction now uses the
getModuleByIdcallback instead of internal store selectors, maintaining the same functionality while improving modularity.apps/web/core/components/labels/create-label-modal.tsx (2)
23-27: Excellent refactoring to a pure presentational component.The new
createLabelprop interface successfully decouples the modal from routing and store concerns, making it more testable and reusable. The component now focuses solely on UI logic and form handling.
70-83: Clean error handling with delegated label creation.The
onSubmithandler now uses the providedcreateLabelfunction directly, maintaining proper error handling while delegating the actual creation logic to the parent component.apps/web/core/components/dropdowns/state/dropdown.tsx (2)
18-47: Well-structured wrapper component with proper loading state management.The new
StateDropdowncomponent effectively separates data fetching concerns from UI rendering. The loading state (stateLoader) is properly managed during asynchronous operations, and the component correctly passes data down toWorkItemStateDropdownBase.
30-36: Efficient lazy loading implementation.The
onDropdownOpenfunction implements proper lazy loading by only fetching states when needed (whenstateIdsis undefined) and includes proper loading state management.apps/web/core/components/dropdowns/module/button-content.tsx (2)
25-41: Good UI logic extraction with comprehensive prop interface.The component effectively handles both single and multiple module selection scenarios with a well-designed props interface. The component supports various display options and interaction states.
43-116: Robust handling of multiple module selection with proper UX.The array handling logic properly supports both count display and individual module chips, with tooltips and remove functionality. The edge cases (empty arrays, single vs multiple items) are handled correctly.
apps/web/core/components/dropdowns/member/base.tsx (1)
1-183: Clean refactoring to decouple from store dependencies.The component has been successfully refactored to receive data and callbacks via props, making it more reusable and testable. The implementation follows the established pattern across other dropdown components.
apps/web/core/components/issues/select/base.tsx (1)
1-267: Well-structured refactoring following the established pattern.The component has been successfully decoupled from store dependencies and now receives data via props. The implementation is clean and maintains proper separation of concerns.
apps/web/core/components/dropdowns/state/base.tsx (1)
1-253: Excellent refactoring with comprehensive prop configuration.The component has been successfully refactored with a rich set of configuration props, making it highly flexible and reusable. The implementation maintains consistency with other dropdown components in the codebase.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/web/ce/components/issues/issue-modal/additional-properties.tsx(0 hunks)apps/web/ce/components/issues/issue-modal/index.ts(0 hunks)apps/web/ce/components/issues/issue-modal/modal-additional-properties.tsx(1 hunks)apps/web/ce/components/issues/issue-modal/provider.tsx(1 hunks)apps/web/core/components/issues/issue-modal/base.tsx(2 hunks)apps/web/core/components/issues/issue-modal/context/issue-modal-context.tsx(3 hunks)apps/web/core/components/issues/issue-modal/form.tsx(5 hunks)
💤 Files with no reviewable changes (2)
- apps/web/ce/components/issues/issue-modal/index.ts
- apps/web/ce/components/issues/issue-modal/additional-properties.tsx
✅ Files skipped from review due to trivial changes (1)
- apps/web/ce/components/issues/issue-modal/modal-additional-properties.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/core/components/issues/issue-modal/form.tsx
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: prateekshourya29
PR: makeplane/plane#7363
File: apps/web/core/components/issues/select/dropdown.tsx:9-11
Timestamp: 2025-07-08T13:41:01.610Z
Learning: The `getProjectLabelIds` function in the label store handles undefined projectId internally, so it's safe to pass undefined values to it without explicit checks in the calling component.
Learnt from: prateekshourya29
PR: makeplane/plane#7188
File: web/app/(all)/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/cycles/(list)/header.tsx:40-45
Timestamp: 2025-06-18T09:46:08.566Z
Learning: When reviewing breadcrumb components that accept projectId or similar props, check if empty strings are being passed during loading states, which can result in invalid URLs. The preferred approach is to handle these loading states internally within the component rather than requiring each consumer to manage the loading logic.
Learnt from: vamsikrishnamathala
PR: makeplane/plane#7214
File: web/core/store/issue/helpers/base-issues.store.ts:117-117
Timestamp: 2025-06-16T07:23:39.497Z
Learning: In the updateIssueDates method of BaseIssuesStore (web/core/store/issue/helpers/base-issues.store.ts), the projectId parameter is intentionally made optional to support override implementations in subclasses. The base implementation requires projectId and includes an early return check, but making it optional allows derived classes to override the method with different parameter requirements.
apps/web/core/components/issues/issue-modal/base.tsx (2)
Learnt from: vamsikrishnamathala
PR: makeplane/plane#7214
File: web/core/store/issue/helpers/base-issues.store.ts:117-117
Timestamp: 2025-06-16T07:23:39.497Z
Learning: In the updateIssueDates method of BaseIssuesStore (web/core/store/issue/helpers/base-issues.store.ts), the projectId parameter is intentionally made optional to support override implementations in subclasses. The base implementation requires projectId and includes an early return check, but making it optional allows derived classes to override the method with different parameter requirements.
Learnt from: prateekshourya29
PR: makeplane/plane#7363
File: apps/web/core/components/issues/select/dropdown.tsx:9-11
Timestamp: 2025-07-08T13:41:01.610Z
Learning: The `getProjectLabelIds` function in the label store handles undefined projectId internally, so it's safe to pass undefined values to it without explicit checks in the calling component.
apps/web/core/components/issues/issue-modal/context/issue-modal-context.tsx (1)
Learnt from: prateekshourya29
PR: makeplane/plane#7363
File: apps/web/helpers/react-hook-form.helper.ts:9-19
Timestamp: 2025-07-08T13:41:19.881Z
Learning: In the apps/web/helpers/react-hook-form.helper.ts file, the getNestedError function's FieldError detection logic using "message" property check is sufficient for their use case, and more robust type checking is not required.
🔇 Additional comments (5)
apps/web/ce/components/issues/issue-modal/provider.tsx (1)
47-47: LGTM: Consistent stub implementation patternThe no-op implementation of
handleCreateSubWorkItemfollows the established pattern of other handler functions in the CE provider. This is appropriate for a Community Edition stub.apps/web/core/components/issues/issue-modal/base.tsx (1)
71-71: LGTM: Proper extraction of handler functionThe
handleCreateSubWorkItemfunction is correctly extracted from the issue modal context.apps/web/core/components/issues/issue-modal/context/issue-modal-context.tsx (3)
4-4: LGTM: Import reorganizationMoving the
TIssueFieldsimport earlier in the import section is a minor reorganization that doesn't affect functionality.
31-35: LGTM: Well-defined type for sub work item creationThe
TCreateSubWorkItemPropstype is clearly defined with appropriate required properties. The type follows the existing naming and structure patterns in the codebase.
74-74: LGTM: Consistent interface extensionThe
handleCreateSubWorkItemmethod addition to theTIssueModalContextinterface is consistent with other handler functions and properly typed.
|
@prateekshourya29 This PR broke the web app build. Please fix first thing Monday. |
4 participants
Description
Type of Change
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Chores