chore: issue filters refactor#6742
Conversation
WalkthroughThis pull request introduces several changes across the application. The property Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (9)
💤 Files with no reviewable changes (6)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (1)
✨ Finishing Touches
🪧 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: 1
🧹 Nitpick comments (1)
web/ce/components/issues/issue-details/additional-properties.tsx (1)
16-31: Good component abstractionThis new component effectively encapsulates the rendering of issue additional properties. The conditional rendering based on
workItemTypeis appropriate, and the props are properly passed down to the child component.According to the PR summary, this component replaces direct usage of
IssueAdditionalPropertyValuesUpdatein multiple locations, which improves code reusability and maintainability.However, I noticed that the
isPeekViewprop is defined in the type but not used in the component. Consider using this prop or removing it if it's not needed.- const { workItemId, projectId, workItemType, workspaceSlug, isEditable } = props; + const { workItemId, projectId, workItemType, workspaceSlug, isEditable, isPeekView } = props;Then pass it to the child component if needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
web/ce/components/issues/issue-details/additional-properties.tsx(1 hunks)web/ce/helpers/issue-filter.helper.ts(1 hunks)web/ce/store/issue/helpers/base-issue.store.ts(1 hunks)web/core/components/issues/issue-detail/root.tsx(1 hunks)web/core/components/issues/issue-detail/sidebar.tsx(2 hunks)web/core/components/issues/issue-layouts/spreadsheet/issue-column.tsx(2 hunks)web/core/components/issues/issue-layouts/spreadsheet/spreadsheet-header-column.tsx(2 hunks)web/core/components/issues/peek-overview/properties.tsx(2 hunks)web/core/store/issue/helpers/base-issues.store.ts(2 hunks)web/ee/components/issues/issue-details/additional-properties.tsx(1 hunks)web/ee/store/issue/helpers/base-issue.store.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (python)
🔇 Additional comments (16)
web/ee/store/issue/helpers/base-issue.store.ts (1)
1-1: Re-exporting module is a good practice for maintainabilityThis file re-exports all entities from the CE (Community Edition) module, which is a good pattern for maintaining separation between CE and EE (Enterprise Edition) code while allowing consumers to import from a consistent path.
web/ee/components/issues/issue-details/additional-properties.tsx (1)
1-1: Clean re-export follows established patternThis file correctly re-exports all entities from the corresponding CE file, following the same pattern seen elsewhere in the codebase. This maintains a clean separation between CE and EE implementations.
web/core/components/issues/issue-detail/root.tsx (1)
374-374: Removed overflow-hidden from sidebar containerRemoving the
overflow-hiddenclass from this container will allow content to overflow, which might be intentional but could also lead to layout issues. Verify that this change doesn't cause unwanted scrollbars or content overlaps in different viewport sizes.This change is likely related to the adoption of the new
WorkItemAdditionalSidebarPropertiescomponent mentioned in the PR summary, which might handle overflow differently than the previous component.web/core/components/issues/issue-layouts/spreadsheet/spreadsheet-header-column.tsx (2)
6-6: Good addition of centralized rendering logic helperThe import of the
shouldRenderColumnhelper function enhances code maintainability by centralizing column rendering logic.
31-31: Nice refactoring to use the shared helper functionReplacing inline logic with the
shouldRenderColumnhelper function follows the DRY principle and makes the code more maintainable. This change ensures consistent handling of column rendering across components.web/core/components/issues/issue-layouts/spreadsheet/issue-column.tsx (2)
10-10: Good addition of centralized rendering logic helperAdding the import for the
shouldRenderColumnhelper maintains consistency with other components and improves code organization.
30-30: Nice refactoring to use the shared helper functionReplacing the inline logic with the
shouldRenderColumnhelper function follows the DRY principle and improves maintainability. This change ensures column rendering logic is consistent with other parts of the application.web/ce/helpers/issue-filter.helper.ts (2)
20-23: Well-defined type for the helper functionGood practice to define a clear type with properly typed properties. The type signature makes it clear what parameters are required for the function.
25-33: Good implementation of the shared helper functionThe
shouldRenderColumnfunction is clean and focused. It centralizes the conditional logic for column rendering, making it easy to maintain and extend in the future. If more conditions need to be added for other column types, they can be easily incorporated into this switch statement.web/ce/components/issues/issue-details/additional-properties.tsx (1)
7-14: Well-structured type definitionThe type definition is clear and properly documents all required properties. The optional
isPeekViewproperty is nicely marked as such, allowing for flexibility in how the component is used.web/core/components/issues/peek-overview/properties.tsx (2)
27-27: Updated import statement for the new component.The import path reflects the new organizational structure, where the
WorkItemAdditionalSidebarPropertiescomponent is located in the issue-details directory, making the code structure more intuitive.
294-301: Good replacement of IssueAdditionalPropertyValuesUpdate with WorkItemAdditionalSidebarProperties.The refactoring replaces the conditional rendering of the
IssueAdditionalPropertyValuesUpdatecomponent with an unconditional rendering ofWorkItemAdditionalSidebarProperties. This change:
- Uses consistent "workItem" terminology instead of "issue"
- Improves readability with better prop naming (e.g.,
isEditableinstead of negatingdisabled)- Adds context awareness with the
isPeekViewpropThese changes align well with the refactoring goal mentioned in the PR objectives.
web/core/components/issues/issue-detail/sidebar.tsx (2)
29-29: Updated import statement for the new component.The import path for
WorkItemAdditionalSidebarPropertiesfollows the same pattern as in the peek-overview component, maintaining consistency across the codebase.
296-303: Good replacement of IssueAdditionalPropertyValuesUpdate with WorkItemAdditionalSidebarProperties.This change mirrors the one made in the peek-overview component, creating a consistent interface across different views. The component:
- Uses the same prop structure as the peek-overview implementation
- Maintains the appropriate state with
isEditable={isEditable}(directly passed through)- Applies the same view context with
isPeekViewThis consistent implementation across different views improves maintainability and reduces cognitive load for developers working on the codebase.
web/core/store/issue/helpers/base-issues.store.ts (2)
40-40: New import for extended work item sorting functionality.The import of
workItemSortWithOrderByExtendedintroduces a dependency on a more specialized sorting function that's now separated into its own module, improving code organization.
1995-1995:Details
❓ Verification inconclusive
Enhanced default case in issuesSortWithOrderBy to use the extended sorting function.
The default case now delegates to
workItemSortWithOrderByExtendedinstead of simply returninggetIssueIds(array). This change provides a more robust handling of sorting cases that don't match the predefined options, potentially supporting additional sorting criteria defined in the extended function.Let's verify the implementation of
workItemSortWithOrderByExtendedto ensure it handles all possible cases properly:
🏁 Script executed:
#!/bin/bash # Search for the implementation of workItemSortWithOrderByExtended rg -A 10 "workItemSortWithOrderByExtended" --type=tsLength of output: 2081
Attention: Extended Sorting Behavior Verification Needed
The default case now delegates to
workItemSortWithOrderByExtended, which, upon verification, is implemented as a simple pass-through togetIssueIds(array)without utilizing thekeyor any additional sorting logic. This means that although the change was intended to provide a more robust handling of sorting cases, it currently does not support extended sorting criteria.
- Confirm whether extended sorting logic based on the
keyis expected in this function.- If extended behavior is intended, please update the implementation accordingly.
- Otherwise, document that returning
getIssueIds(array)is the deliberate fallback behavior.
web/core/components/issues/issue-layouts/spreadsheet/issue-column.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
web/core/components/issues/issue-layouts/spreadsheet/issue-column.tsx (1)
14-21: 🛠️ Refactor suggestionRemove unused property from Props interface
The
isEstimateEnabledproperty is defined in the Props interface but has been removed from the destructured props. This aligns with the previous code review comment about accessing this value from the store directly.type Props = { displayProperties: IIssueDisplayProperties; issueDetail: TIssue; disableUserActions: boolean; property: keyof IIssueDisplayProperties; updateIssue: ((projectId: string | null, issueId: string, data: Partial<TIssue>) => Promise<void>) | undefined; - isEstimateEnabled: boolean; };
♻️ Duplicate comments (1)
web/core/components/issues/issue-detail/sidebar.tsx (1)
296-303: 🛠️ Refactor suggestionRename prop from
workItemTypetoworkItemTypeIdThe prop name should be
workItemTypeIdinstead ofworkItemTypeto maintain consistency with the renamed property across the codebase. This ensures type safety and aligns with the changes made in other components.<WorkItemAdditionalSidebarProperties workItemId={issue.id} - workItemType={issue.type_id} + workItemTypeId={issue.type_id} projectId={projectId} workspaceSlug={workspaceSlug} isEditable={isEditable} isPeekView />
🛑 Comments failed to post (4)
web/core/components/issues/issue-layouts/spreadsheet/spreadsheet-header-column.tsx (2)
24-24:
⚠️ Potential issueParameter mismatch with
shouldRenderColumnfunctionThe function call to
shouldRenderColumnonly passes thepropertyparameter, but according to the function definition inissue-filter.helper.ts, it expects an object of typeTShouldRenderColumnwith bothkeyandisEstimateEnabledproperties.This parameter mismatch will likely cause runtime errors. The function call should be updated to match the expected parameter structure, or the function definition should be modified to accept just the property key.
19-19: 🛠️ Refactor suggestion
Property removal requires function parameter alignment
The
isEstimateEnabledproperty has been removed from the destructured props, but the interface still includes it at line 13. This property is likely no longer needed as a prop since it seems the intention is to access it from the store context directly.- interface Props { - displayProperties: IIssueDisplayProperties; - property: keyof IIssueDisplayProperties; - isEstimateEnabled: boolean; - displayFilters: IIssueDisplayFilterOptions; - handleDisplayFilterUpdate: (data: Partial<IIssueDisplayFilterOptions>) => void; - isEpic?: boolean; - } + interface Props { + displayProperties: IIssueDisplayProperties; + property: keyof IIssueDisplayProperties; + displayFilters: IIssueDisplayFilterOptions; + handleDisplayFilterUpdate: (data: Partial<IIssueDisplayFilterOptions>) => void; + isEpic?: boolean; + }📝 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.interface Props { displayProperties: IIssueDisplayProperties; property: keyof IIssueDisplayProperties; displayFilters: IIssueDisplayFilterOptions; handleDisplayFilterUpdate: (data: Partial<IIssueDisplayFilterOptions>) => void; isEpic?: boolean; }web/core/components/issues/issue-layouts/spreadsheet/issue-column.tsx (1)
30-30:
⚠️ Potential issueParameter mismatch with
shouldRenderColumnfunctionThe function call to
shouldRenderColumnonly passes thepropertyparameter, but according to the function definition inissue-filter.helper.ts, it expects an object of typeTShouldRenderColumnwith bothkeyandisEstimateEnabledproperties.This parameter mismatch will likely cause runtime errors. The function call should be updated to match the expected parameter structure, or the function definition should be modified to accept just the property key.
web/ce/helpers/issue-filter.helper.ts (1)
20-33:
⚠️ Potential issueFunction implementation doesn't match its usage
The
shouldRenderColumnfunction is defined to take a parameter of typeTShouldRenderColumnwith propertieskeyandisEstimateEnabled, but it's being called with just a property key in the other files. Additionally, since the previous review mentioned accessingisEstimateEnabledfrom the store, this function should be updated to get that value directly from the store context.Update the function to handle a single parameter and access
isEstimateEnabledfrom the store:- export type TShouldRenderColumn = { - key: keyof IIssueDisplayProperties; - isEstimateEnabled: boolean; - }; - - export const shouldRenderColumn = (props: TShouldRenderColumn): boolean => { - const { key, isEstimateEnabled } = props; + export const shouldRenderColumn = (key: keyof IIssueDisplayProperties): boolean => { + const isEstimateEnabled = store.projectState.getProjectById(store.projectState?.currentProjectId)?.estimate; switch (key) { case "estimate": return isEstimateEnabled; default: return true; } };This change will align the function with how it's being called in the components and follows the suggestion to get
isEstimateEnabledfrom the store directly.📝 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.}; export const shouldRenderColumn = (key: keyof IIssueDisplayProperties): boolean => { const isEstimateEnabled = store.projectState.getProjectById(store.projectState?.currentProjectId)?.estimate; switch (key) { case "estimate": return isEstimateEnabled; default: return true; } };
* chore: issue filters refactor * chore: update helper funciton implementation * chore: removed redundant components
3 participants
Description
This update includes a code refactoring for issue properties.
Type of Change
Screenshots and Media (if applicable)
Test Scenarios
References
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
WorkItemAdditionalSidebarPropertiescomponent across various instances.Chores