Chore: progress chart changes #5707
Conversation
WalkthroughThe pull request introduces new exports for sidebar components in the cycle management system, specifically adding 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://github.com/avatars/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 28
🧹 Outside diff range and nitpick comments (24)
web/ce/components/cycles/analytics-sidebar/index.ts (1)
1-1: LGTM! Good step towards code splitting and modularity.This change aligns well with the PR objective of "progress chart code splitting". It improves code organization by separating the sidebar chart functionality into its own module, which can enhance maintainability and potentially allow for lazy loading of components.
A few points to consider:
- Using wildcard exports (*) can sometimes lead to naming conflicts. Ensure that the exported entities from "sidebar-chart" don't clash with other imports where this module is used.
- For better clarity, consider explicitly listing the main exports in a comment above this line. This can serve as quick documentation for developers using this module.
web/ce/components/cycles/index.ts (1)
1-2: LGTM! Consider adding explanatory comments.The creation of this index file is a good step towards better code organization and potential code splitting. It aligns well with the PR objective and follows common JavaScript/TypeScript practices.
Consider adding a brief comment at the top of the file to explain its purpose, which could help other developers understand the intent behind this file more quickly. For example:
// Central export point for cycle-related components. // This file facilitates easier imports and potential code splitting. export * from "./active-cycle"; export * from "./analytics-sidebar";This addition would enhance the file's self-documentation without changing its functionality.
packages/ui/src/icons/in-progress-icon.tsx (2)
5-5: Component definition looks good, but there's an unused prop.The component definition follows React best practices and uses TypeScript for type safety. However, the
colorprop is destructured but not used in the component.Consider removing the unused
colorprop or utilizing it in the SVG if it's intended for future use:-export const InProgressState: React.FC<ISvgIcons> = ({ width = "10", height = "11", className, color }) => ( +export const InProgressState: React.FC<ISvgIcons> = ({ width = "10", height = "11", className }) => (
6-17: SVG structure is good, but consider improving color customization.The SVG structure effectively creates an "in progress" icon using two circles with different stroke styles. The
classNameprop is correctly applied, allowing for external styling.To improve the component's reusability, consider making the stroke color customizable:
- Use the
colorprop (currently unused) to set the stroke color.- Provide a default color if none is specified.
Here's how you could modify the SVG:
<svg width={width} height={height} viewBox="0 0 12 13" fill="none" xmlns="http://www.w3.org/2000/svg" className={className} > - <circle cx="6" cy="6.5" r="4.4" stroke="#EA8900" stroke-width="1.2" /> - <circle cx="6" cy="6.5" r="2.4" stroke="#EA8900" stroke-width="1.2" stroke-dasharray="4 4" /> + <circle cx="6" cy="6.5" r="4.4" stroke={color || "#EA8900"} stroke-width="1.2" /> + <circle cx="6" cy="6.5" r="2.4" stroke={color || "#EA8900"} stroke-width="1.2" stroke-dasharray="4 4" /> </svg>This change would allow users of the component to customize the color while maintaining the default orange color if no color is specified.
packages/ui/src/icons/done-icon.tsx (1)
14-20: Use camelCase for SVG attributes to align with React conventions.The SVG structure is correct, but the attribute names use kebab-case instead of camelCase, which is the convention in React for DOM properties.
Consider updating the SVG attributes to use camelCase:
- <circle cx="5" cy="5.5" r="4.4" stroke="#15A34A" stroke-width="1.2" /> + <circle cx="5" cy="5.5" r="4.4" stroke="#15A34A" strokeWidth="1.2" /> <path - fill-rule="evenodd" - clip-rule="evenodd" + fillRule="evenodd" + clipRule="evenodd" d="M2.5 5.59375L3.82582 6.91957L4.26777 6.47763L2.94194 5.15181L2.5 5.59375ZM4.26777 7.36152L7.36136 4.26793L6.91942 3.82599L3.82583 6.91958L4.26777 7.36152Z" fill="#15A34A" />This change improves consistency with React conventions and prevents warnings in strict mode.
packages/ui/src/icons/index.ts (1)
31-33: LGTM! Consider grouping related icons.The new exports for planned, in-progress, and done icons are consistent with the existing pattern and support the progress chart functionality mentioned in the PR title. These additions will likely enhance the visualization of task progress in the UI.
Consider grouping related icons together in the file for better organization. For example, you could move these new exports next to other task-related icons if they exist, or add a comment to separate different categories of icons.
export * from "./favorite-folder-icon"; +// Task progress icons export * from "./planned-icon"; export * from "./in-progress-icon"; export * from "./done-icon";This grouping can improve code readability and make it easier to manage icons as the list grows.
web/ce/components/cycles/analytics-sidebar/sidebar-chart.tsx (3)
6-13: LGTM: Well-defined prop types with a minor suggestion.The
ProgressChartPropstype is well-defined and uses appropriate TypeScript features. Good job on using union types forchartDistributionDataand optional types for date properties.Consider using more specific types for
totalEstimatePoints,totalIssues, andplotType. For example:totalEstimatePoints: number; totalIssues: number; plotType: 'points' | 'issues';This would provide better type safety and autocompletion for the
plotTypeprop.
14-57: LGTM: Well-structured component with minor improvement suggestions.The
SidebarBaseChartcomponent is well-implemented, following React best practices. Good job on using optional chaining and conditional rendering based on data availability.Consider the following improvements:
- Extract the legend rendering into a separate component for better modularity:
const ChartLegend = () => ( <div className="relative flex items-center gap-2"> {/* ... legend items ... */} </div> );
- Use early return for the loading state to reduce nesting:
if (!cycleStartDate || !cycleEndDate || !completionChartDistributionData) { return ( <Loader className="w-full h-[160px] mt-4"> <Loader.Item width="100%" height="100%" /> </Loader> ); } // Rest of the component...
- Consider using a ternary operator instead of an if-else for rendering the ProgressChart based on
plotType:<ProgressChart distribution={completionChartDistributionData} startDate={cycleStartDate} endDate={cycleEndDate} totalIssues={plotType === "points" ? totalEstimatePoints : totalIssues} plotTitle={plotType} />These changes would improve readability and reduce code duplication.
1-57: Consider performance optimizations and error handling.The component is well-structured, but there are opportunities for improvement in terms of performance and error handling:
Memoization: Consider using
useMemofor expensive computations orReact.memofor the component if it's expected to re-render frequently with the same props.Error Handling: Implement error boundaries or add proper error handling for potential issues with data fetching or processing.
Accessibility: Ensure the chart is accessible by adding appropriate ARIA attributes and keyboard navigation support.
Here's an example of how you could implement these suggestions:
import React, { useMemo } from 'react'; import { ErrorBoundary } from 'react-error-boundary'; const SidebarBaseChart = React.memo((props: ProgressChartProps) => { const { chartDistributionData, cycleStartDate, cycleEndDate, totalEstimatePoints, totalIssues, plotType } = props; const completionChartDistributionData = useMemo(() => chartDistributionData?.completion_chart, [chartDistributionData] ); // ... rest of the component logic return ( <ErrorBoundary fallback={<ErrorFallback />}> {/* ... component JSX ... */} </ErrorBoundary> ); }); const ErrorFallback = () => ( <div role="alert"> <p>Something went wrong:</p> <pre>Error loading chart data</pre> </div> );This implementation adds memoization to prevent unnecessary re-renders and includes an error boundary for graceful error handling.
packages/ui/src/icons/planned-icon.tsx (2)
5-13: LGTM: Component declaration is well-structured. Consider using object destructuring for default values.The component declaration follows React best practices and uses TypeScript for improved type safety. However, you could simplify the props destructuring by using object default values.
Consider refactoring the component declaration as follows:
export const PlannedState: React.FC<ISvgIcons> = ({ width = "10", height = "11", className, color }) => ( <svg width={width} height={height} viewBox="0 0 12 13" fill="none" xmlns="http://www.w3.org/2000/svg" className={className} > {/* ... */} </svg> );This approach is more idiomatic in React and makes the default values more visible.
14-39: LGTM: SVG structure is well-formed. Consider allowing color customization.The SVG structure is correct and uses appropriate techniques like clipping paths for complex shapes. The consistent use of fill color across paths is good for maintainability.
Consider allowing color customization through props. You can modify the path elements to use the
colorprop:<path // ...other attributes fill={color || "#455068"} />This change would make the icon more flexible for different color schemes while maintaining the default color.
packages/ui/src/progress/circular-progress-indicator.tsx (3)
12-12: LGTM! Consider adding JSDoc for the new prop.The addition of the
strokeColorprop with a default value is a good way to allow customization while maintaining backward compatibility.Consider adding a JSDoc comment for the
strokeColorprop in theICircularProgressIndicatorinterface to describe its purpose and expected values. For example:interface ICircularProgressIndicator { // ... other props ... /** * Custom stroke color for the progress indicator. * @default "stroke-custom-primary-100" */ strokeColor?: string; }
Line range hint
30-56: Remove commented-out code and update related styles.The large commented-out
<defs>section should be removed entirely rather than left as comments. This will improve code readability and maintainability.Please make the following changes:
- Remove the commented-out
<defs>section (lines 30-56).- Update or remove the
styleprop on line 27 that references the now-removed filter:- style={{ filter: "url(#filter0_bi_377_19141)" }} + // Remove this line if the filter is no longer neededIf the filter functionality is still required, consider moving it to a separate SVG definitions file or implementing it using CSS filters instead.
58-58: LGTM! Consider adding a safeguard for direct color values.The change correctly applies the custom stroke color. However, it assumes that
strokeColoris always a CSS class name.To make the component more flexible, consider allowing both CSS class names and direct color values (e.g., "#FF0000" or "rgb(255, 0, 0)"). You can achieve this by updating the
classNameand adding astyleprop:className={`fill-none ${strokeColor.startsWith('stroke-') ? strokeColor : ''}`} style={{ stroke: strokeColor.startsWith('stroke-') ? undefined : strokeColor }}This change would allow users to pass either a CSS class name (prefixed with "stroke-") or a direct color value, making the component more versatile.
web/app/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/cycles/(detail)/[cycleId]/page.tsx (1)
73-80: Summary of changes and potential impact.The changes in this file involve:
- Reducing the sidebar width and padding, making it more compact.
- Simplifying the CycleDetailsSidebar component by removing the cycleId prop.
These modifications appear to be part of a UI optimization and component simplification effort. While they seem reasonable, please ensure that:
- The new sidebar dimensions work well across different screen sizes and don't negatively impact the user experience.
- The CycleDetailsSidebar component still functions correctly without the cycleId prop.
Consider adding or updating tests to cover these changes and prevent potential regressions.
web/core/components/cycles/analytics-sidebar/progress-stats.tsx (1)
344-351: LGTM: Addition of "States" Tab.Panel with minor suggestionThe addition of the "States"
Tab.Panelwith theStateStatComponentis well-implemented and consistent with the changes in theprogressStatsarray. This modification allows for the display of state-based statistics in the progress chart.For consistency with the other
Tab.Panelcomponents, consider adding thefiltersprop to theStateStatComponent:<StateStatComponent distribution={distributionStateData} totalIssuesCount={totalIssuesCount} isEditable={isEditable} + filters={filters} handleFiltersUpdate={handleFiltersUpdate} />This change would make the
StateStatComponentprops more consistent with theAssigneeStatComponentandLabelStatComponent.web/core/store/issue/cycle/issue.store.ts (1)
142-145: LGTM! Consider minor readability improvement.The addition of
fetchActiveCycleProgressProenhances the functionality by fetching active cycle progress alongside cycle details. This aligns well with the PR objective of progress chart code splitting.For improved readability, consider extracting the condition check into a separate variable:
+ const shouldFetchProgress = projectId && cycleId; - projectId && - cycleId && - this.rootIssueStore.rootStore.cycle.fetchActiveCycleProgressPro(workspaceSlug, projectId, cycleId); + if (shouldFetchProgress) { + this.rootIssueStore.rootStore.cycle.fetchActiveCycleProgressPro(workspaceSlug, projectId, cycleId); + }This change would make the code more readable and consistent with the style used for
fetchCycleDetails.web/helpers/date-time.helper.ts (2)
368-377: LGTM! Consider using date-fns for consistency.The
getTodayfunction is well-implemented and correctly handles both Date object and formatted string outputs. However, for consistency with the rest of the file, consider usingdate-fnsfunctions likestartOfDayandformat.Here's a suggested refactor using
date-fns:import { startOfDay, format } from 'date-fns'; export const getToday = (shouldFormat: boolean = false): Date | string => { const today = startOfDay(new Date()); return shouldFormat ? format(today, 'yyyy-MM-dd') : today; };This refactor maintains the same functionality while leveraging the
date-fnslibrary for consistency with other functions in the file.
Line range hint
1-432: Overall improvement suggestion for date-time helper moduleThe new functions (
getToday,dateFormatter,daysLeft, andgenerateDateArray) add valuable functionality to the date-time helper module. However, there's an opportunity to improve consistency across the entire file:
- Consistently use
date-fnsfunctions throughout the module. This will provide better timezone handling, reduce the risk of errors, and make the code more maintainable.- Standardize error handling and input validation across all functions.
- Ensure all functions have clear, accurate JSDoc comments with examples.
- Consider grouping related functions (e.g., all formatting functions together) for better organization.
To implement these improvements:
- Review all existing functions and refactor them to use
date-fnswhere appropriate.- Create a common error handling utility for date-related errors.
- Update all JSDoc comments to follow a consistent format with accurate examples.
- Reorganize the file structure to group related functions together.
These changes will enhance the overall quality, consistency, and maintainability of the date-time helper module.
web/ce/components/cycles/active-cycle/root.tsx (1)
37-88: Remove Redundant Fragment Wrapping the Disclosure ComponentThe fragment wrapping the
<Disclosure>component at lines 37 and 88 is unnecessary since<Disclosure>is a single root element in the return statement. Removing the fragment can simplify the code.Apply this diff to remove the redundant fragment:
- return ( - <> - <Disclosure as="div" className="flex flex-shrink-0 flex-col" defaultOpen> - {({ open }) => ( - <> - ... - </> - )} - </Disclosure> - </> - ); + return ( + <Disclosure as="div" className="flex flex-shrink-0 flex-col" defaultOpen> + {({ open }) => ( + <> + ... + </> + )} + </Disclosure> + );web/helpers/cycle.helper.ts (1)
8-20: Consider defining a separate interface for the progress chart data itemsTo enhance readability and reusability, consider defining an interface for the progress chart data items and then using it to define the array type. This can make the codebase more maintainable.
For example:
export interface IProgressChartDataItem { date: string; scope: number; completed: number; backlog: number; started: number; unstarted: number; cancelled: number; pending: number; ideal: number; actual: number; } export type TProgressChartData = IProgressChartDataItem[];web/core/components/cycles/analytics-sidebar/sidebar-details.tsx (1)
121-123: Fix typo in commentThere's a typographical error in the comment. It should be "the projects" instead of "he projects".
Apply this diff to fix the typo:
/** - * NOTE: Render this section when estimate points of he projects is enabled and the estimate system is points + * NOTE: Render this section when estimate points of the projects is enabled and the estimate system is points */web/core/components/cycles/analytics-sidebar/issue-progress.tsx (1)
Line range hint
95-101: Fix logical error inprogressHeaderPercentagecalculationThere is a logical error in the condition checking
completedIssuesandtotalIssues. The condition currently comparescompletedIssuesto itself, which is redundant and incorrect. It should check iftotalIssuesis not zero to prevent division by zero.Apply this diff to correct the condition:
: completedIssues != 0 && completedIssues != 0 + : completedIssues != 0 && totalIssues != 0web/core/components/cycles/analytics-sidebar/sidebar-header.tsx (1)
175-179: Improve the clarity of the error messageThe error message can be rephrased for better readability and to avoid run-on sentences.
Consider updating the message:
- "You already have a cycle on the given dates, if you want to create a draft cycle, you can do that by removing both the dates." + "A cycle already exists for the selected dates. To create a draft cycle, please remove both the start and end dates."
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (31)
- packages/types/src/cycle/cycle.d.ts (5 hunks)
- packages/ui/src/icons/done-icon.tsx (1 hunks)
- packages/ui/src/icons/in-progress-icon.tsx (1 hunks)
- packages/ui/src/icons/index.ts (1 hunks)
- packages/ui/src/icons/planned-icon.tsx (1 hunks)
- packages/ui/src/loader.tsx (1 hunks)
- packages/ui/src/progress/circular-progress-indicator.tsx (3 hunks)
- web/app/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/cycles/(detail)/[cycleId]/page.tsx (1 hunks)
- web/ce/components/cycles/active-cycle/index.ts (1 hunks)
- web/ce/components/cycles/active-cycle/root.tsx (1 hunks)
- web/ce/components/cycles/analytics-sidebar/index.ts (1 hunks)
- web/ce/components/cycles/analytics-sidebar/sidebar-chart.tsx (1 hunks)
- web/ce/components/cycles/index.ts (1 hunks)
- web/core/components/cycles/active-cycle/index.ts (0 hunks)
- web/core/components/cycles/active-cycle/productivity.tsx (6 hunks)
- web/core/components/cycles/active-cycle/use-cycles-details.ts (1 hunks)
- web/core/components/cycles/analytics-sidebar/index.ts (1 hunks)
- web/core/components/cycles/analytics-sidebar/issue-progress.tsx (11 hunks)
- web/core/components/cycles/analytics-sidebar/progress-stats.tsx (2 hunks)
- web/core/components/cycles/analytics-sidebar/root.tsx (2 hunks)
- web/core/components/cycles/analytics-sidebar/sidebar-details.tsx (1 hunks)
- web/core/components/cycles/analytics-sidebar/sidebar-header.tsx (1 hunks)
- web/core/components/cycles/cycle-peek-overview.tsx (1 hunks)
- web/core/components/cycles/list/cycle-list-item-action.tsx (5 hunks)
- web/core/components/cycles/list/cycles-list-item.tsx (0 hunks)
- web/core/components/cycles/list/root.tsx (2 hunks)
- web/core/services/cycle.service.ts (1 hunks)
- web/core/store/cycle.store.ts (12 hunks)
- web/core/store/issue/cycle/issue.store.ts (1 hunks)
- web/helpers/cycle.helper.ts (2 hunks)
- web/helpers/date-time.helper.ts (1 hunks)
💤 Files not reviewed due to no reviewable changes (2)
- web/core/components/cycles/active-cycle/index.ts
- web/core/components/cycles/list/cycles-list-item.tsx
🧰 Additional context used
🪛 Biome
web/core/components/cycles/analytics-sidebar/issue-progress.tsx
[error] 182-182: 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/cycles/analytics-sidebar/sidebar-details.tsx
[error] 42-44: 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)
[error] 45-47: 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/cycles/analytics-sidebar/sidebar-header.tsx
[error] 159-159: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🪛 GitHub Check: lint-web
web/core/components/cycles/analytics-sidebar/sidebar-details.tsx
[failure] 5-5:
lodash/isEmptyimport should occur before import ofmobx-react
[failure] 9-9:
@plane/typesimport should occur before import of@plane/uiweb/core/components/cycles/analytics-sidebar/sidebar-header.tsx
[failure] 6-6:
mobx-reactimport should occur before import ofreact-hook-formweb/core/components/cycles/list/cycle-list-item-action.tsx
[failure] 25-25:
@/helpers/router.helperimport should occur before import of@/hooks/store
[failure] 26-26:
@/hooks/use-app-routerimport should occur before import of@/hooks/use-platform-os
[failure] 27-27:
next/navigationimport should occur before import ofreact-hook-form
🔇 Additional comments (67)
web/ce/components/cycles/active-cycle/index.ts (2)
1-1: LGTM! This change facilitates better code organization and splitting.This new
index.tsfile, which re-exports all entities from therootmodule, is a good practice for several reasons:
- It provides a clear entry point for the
active-cyclemodule, making imports more consistent across the codebase.- It facilitates easier code splitting and lazy loading of components, which can lead to improved performance.
- It enhances maintainability by centralizing exports, making it easier to manage as the number of exports grows.
This change aligns well with the PR objective of "progress chart code splitting".
1-1: Verify consistent usage of index files across the project.To ensure this change is part of a broader, consistent approach to code organization:
- Check if similar index files exist in other directories.
- Verify that imports in other files are updated to use this new index file where applicable.
Run the following script to check for consistency:
This script will help identify if the new pattern is consistently applied and if there are any imports that might need updating.
web/core/components/cycles/analytics-sidebar/index.ts (1)
5-6: New exports align with code splitting objective.The addition of exports for "sidebar-header" and "sidebar-details" modules aligns well with the PR objective of progress chart code splitting. This change enhances modularity and potentially improves maintainability.
packages/ui/src/icons/in-progress-icon.tsx (2)
1-3: LGTM: Imports are appropriate.The imports are correct for a React component using TypeScript. Importing the
ISvgIconsinterface from a separate file is a good practice for maintaining type definitions.
1-17: Overall assessment: Well-implemented component with minor improvement opportunities.The
InProgressStatecomponent is well-structured and follows React and TypeScript best practices. It successfully creates a reusable "in progress" icon component. The use of SVG for the icon ensures scalability, and the component accepts customizable props for flexibility.To further improve the component:
- Remove or utilize the unused
colorprop.- Consider making the stroke color customizable for better reusability.
These minor adjustments will enhance the component's flexibility and adherence to best practices.
packages/ui/src/loader.tsx (3)
19-19: LGTM: Addition of className to ItemPropsThe addition of the optional
classNameproperty to theItemPropsinterface enhances the flexibility of theItemcomponent, allowing for custom styling. This change is consistent with the existing pattern in theLoadercomponent and maintains backward compatibility.
22-23: LGTM: Implementation of className in Item componentThe changes to the
Itemcomponent correctly implement the newclassNameprop. The use of thecnfunction ensures that the default class is always applied while allowing for additional custom classes. The default value of an empty string forclassNamemaintains backward compatibility. This implementation is consistent with howclassNameis handled in the parentLoadercomponent.
19-23: Summary: Improved flexibility of the Item componentThese changes enhance the
Itemcomponent by allowing custom CSS classes to be passed through the newclassNameprop. This improvement aligns well with the PR objective of code splitting and increases the reusability of the component. The implementation is consistent with existing patterns in the codebase and maintains backward compatibility.packages/ui/src/icons/done-icon.tsx (2)
1-3: LGTM: Imports are correct and necessary.The import statements are appropriate for the component. React is imported for the JSX syntax, and the ISvgIcons interface is imported for type checking.
1-22: Overall assessment: Well-structured component with minor improvements needed.The
DoneStatecomponent is a good implementation of a "done" icon as an SVG in React. It's properly typed with theISvgIconsinterface and allows for customization of width, height, and className.Summary of suggestions:
- Utilize the
colorprop to make the icon color dynamic.- Use camelCase for SVG attributes to align with React conventions.
These changes will enhance the component's flexibility and maintainability.
web/core/components/cycles/cycle-peek-overview.tsx (2)
44-49: Styling adjustments for the sidebarThe changes to the sidebar's styling look good. Reducing the maximum width from 24rem to 21.5rem and adjusting the padding from px-6 to px-4 will result in a slightly more compact sidebar. This should improve the overall layout without significantly impacting the user experience.
Line range hint
1-54: Overall assessment: Changes look good with minor verification neededThe changes in this file are relatively minor and seem to improve both the layout and code simplicity. The styling adjustments for the sidebar should result in a more compact design, while the simplified prop passing for
CycleDetailsSidebarreduces code complexity.However, it's important to verify that the
peekCyclevalue is always a string to ensure type safety with the new prop passing method. The verification script provided earlier will help confirm this.If the verification passes, these changes can be approved. Great job on the code splitting and simplification!
web/ce/components/cycles/analytics-sidebar/sidebar-chart.tsx (1)
1-4: LGTM: Imports are appropriate and concise.The imports are well-organized and relevant to the component's functionality. Good job on using local imports for custom components like
ProgressChart.packages/ui/src/icons/planned-icon.tsx (1)
1-3: LGTM: Imports are correct and well-structured.The necessary imports for React and the
ISvgIconsinterface are present and correctly implemented.packages/ui/src/progress/circular-progress-indicator.tsx (1)
Line range hint
1-83: Overall, good improvements to component flexibility.The changes to the
CircularProgressIndicatorcomponent successfully introduce the ability to customize the stroke color, which enhances its reusability. The implementation is generally sound, with good use of default values to maintain backward compatibility.Here's a summary of the suggested improvements:
- Add JSDoc comments for the new
strokeColorprop.- Remove the commented-out
<defs>section and update related styles.- Make the
strokeColorprop more flexible by allowing both CSS class names and direct color values.These changes will result in a cleaner, more robust, and more flexible component. Great work on improving the component's customizability!
web/core/components/cycles/list/root.tsx (3)
31-31: LGTM: Simplified prop passing forActiveCycleRootThe
workspaceSlugandprojectIdprops are now passed directly toActiveCycleRootwithout usingtoString(). This simplification is good, but we should ensure that theActiveCycleRootcomponent can handle these prop types correctly.To confirm that the
ActiveCycleRootcomponent is prepared for this change, please run the following script:#!/bin/bash # Description: Verify the prop types of ActiveCycleRoot component # Test: Check the prop types defined for ActiveCycleRoot ast-grep --lang typescript --pattern $'interface $_Props { $$$ workspaceSlug: $_ projectId: $_ $$$ } export const ActiveCycleRoot: FC<$_Props> = ($_) => { $$$ }'This script will help us verify the prop types defined for
ActiveCycleRootand ensure they match the new usage.
Line range hint
1-31: Summary: Minor changes as part of potential refactoringThe changes in this file are relatively minor but may be part of a larger refactoring effort:
- The import path for
ActiveCycleRoothas been updated, suggesting a reorganization of the project structure.- The prop passing for
ActiveCycleRoothas been simplified, which might indicate changes in how prop types are handled.While these changes don't affect the overall structure or logic of the
CyclesListcomponent, they could be part of broader changes across the project. It's important to ensure that these changes are consistent with other parts of the codebase and don't introduce any unintended side effects.To get a better understanding of the scope of these changes, please run the following script:
#!/bin/bash # Description: Check for similar changes across the project # Test: Look for other files that might have been affected by similar changes rg --type typescript --type tsx 'from\s+["\']@/plane-web/components/' # Test: Check for other instances where toString() might have been removed from prop passing rg --type typescript --type tsx '(?<!\.toString\(\))\s+(?:workspaceSlug|projectId)=\{[^}]+\}'This will help us understand if these changes are isolated or part of a larger refactoring effort.
8-8: LGTM: Import path updated forActiveCycleRootThe import path for
ActiveCycleRoothas been updated from "@/components/cycles" to "@/plane-web/components/cycles". This change likely reflects a project structure reorganization or a move towards more modular architecture.To ensure consistency across the project, please run the following script to verify the usage of the new import path:
✅ Verification successful
To address the syntax error in the previous shell script, please use the corrected scripts below to verify the usage of the new import path for
ActiveCycleRoot:
To resolve the issue with unrecognized file types, please use the updated scripts below to verify the usage of the new import path for
ActiveCycleRoot:
To resolve the issues with unrecognized file types, please use the updated scripts below to verify the usage of the new import path for
ActiveCycleRoot:
LGTM: Import path updated for
ActiveCycleRootThe import path for
ActiveCycleRoothas been successfully updated from@/components/cyclesto@/plane-web/components/cyclesinweb/core/components/cycles/list/root.tsx. No remaining imports from the old path were found in this file.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the new import path for ActiveCycleRoot # Test: Search for any remaining imports from the old path rg --type typescript --type tsx 'from\s+["\']@/components/cycles["\']' # Test: Confirm the usage of the new import path rg --type typescript --type tsx 'from\s+["\']@/plane-web/components/cycles["\']'Length of output: 230
Script:
#!/bin/bash # Description: Verify the usage of the new import path for ActiveCycleRoot # Test: Search for any remaining imports from the old path rg --type typescript --type tsx "from\s+['\"]@/components/cycles['\"]" # Test: Confirm the usage of the new import path rg --type typescript --type tsx "from\s+['\"]@/plane-web/components/cycles['\"]"Length of output: 233
Script:
#!/bin/bash # Description: Verify the usage of the new import path for ActiveCycleRoot # Test: Search for any remaining imports from the old path rg --type ts --type tsx "from\s+['\"]@/components/cycles['\"]" # Test: Confirm the usage of the new import path rg --type ts --type tsx "from\s+['\"]@/plane-web/components/cycles['\"]"Length of output: 203
Script:
#!/bin/bash # Description: Verify the usage of the new import path for ActiveCycleRoot # Test: Search for any remaining imports from the old path in .ts and .tsx files rg -g '*.ts' -g '*.tsx' "from\s+['\"]@/components/cycles['\"]" # Test: Confirm the usage of the new import path in .ts and .tsx files rg -g '*.ts' -g '*.tsx' "from\s+['\"]@/plane-web/components/cycles['\"]"Length of output: 3855
web/app/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/cycles/(detail)/[cycleId]/page.tsx (1)
73-73: LGTM! Verify UI appearance after sidebar size adjustment.The sidebar width has been reduced from 24rem to 21.5rem, and the padding has been changed from px-6 to px-4. This change makes the sidebar more compact, which could improve space utilization.
Please verify that this change doesn't negatively impact the UI appearance or user experience, especially on different screen sizes.
packages/types/src/cycle/cycle.d.ts (3)
46-57: LGTM: Well-structuredTCycleProgresstype definitionThe new
TCycleProgresstype is well-defined with clear and descriptive property names. It provides a comprehensive structure for tracking various aspects of cycle progress, which will be valuable for detailed cycle analysis and reporting.
132-132: LGTM: ClearTCycleEstimateTypedefinitionThe new
TCycleEstimateTypeis well-defined and aligns with common agile estimation practices. It provides a clear way to distinguish between issue-based and point-based estimations in the cycle context.
133-133: LGTM: EnhancedTCyclePlotTypewith burnup optionThe addition of "burnup" to
TCyclePlotTypeis a valuable enhancement. This change allows for more comprehensive cycle progress visualization, supporting both burndown and burnup charts which are common in agile project management.To ensure consistency, please verify if any documentation or related code needs to be updated to reflect this new option. You can use the following script to check for any occurrences of
TCyclePlotTypethat might need updating:web/core/components/cycles/active-cycle/use-cycles-details.ts (1)
13-13: LGTM! Verify impact on dependent code.The change to allow
cycleIdto beundefinedis backwards compatible and doesn't introduce immediate issues in this file. However, ensure that this change is reflected correctly in other parts of the codebase whereIActiveCycleDetailsorcycleIdfrom this hook is used.To verify the impact of this change, run the following script:
Also, consider updating any relevant documentation to reflect this change in the
cycleIdtype.web/core/components/cycles/active-cycle/productivity.tsx (8)
4-4: LGTM: Import statement updated correctlyThe import statement has been updated to include
TCycleEstimateType, which aligns with the component's new focus on estimate types instead of plot types.
22-22: LGTM: Chart options updated to reflect estimate typesThe
cycleBurnDownChartOptionsarray has been updated to use "issues" instead of "burndown", which is consistent with the component's new focus on estimate types.
29-29: LGTM: Hooks and derived values updated for estimate typesThe component's hooks and derived values have been correctly updated to use estimate type functions (
getEstimateTypeByCycleIdandsetEstimateType) instead of plot type functions. The defaultestimateTypeis appropriately set to "issues".Also applies to: 33-33
35-37: LGTM: onChange function updated for estimate typesThe
onChangefunction has been correctly updated to handle estimate types instead of plot types. It now uses thesetEstimateTypefunction, which is consistent with the component's new focus.
46-46: LGTM: Chart distribution data logic updatedThe chart distribution data logic has been correctly updated to use the estimate type. It now checks if the estimate type is "points" to determine which distribution data to use, which is consistent with the component's new focus on estimate types.
58-59: LGTM: CustomSelect component updated for estimate typesThe CustomSelect component has been correctly updated to use
estimateTypeinstead ofplotType. The value and label now reflect the new estimate type focus, which is consistent with the overall changes in the component.
Line range hint
88-93: LGTM: Rendering logic updated for estimate typesThe rendering logic for pending points or issues has been correctly updated to use the new estimate type. It now conditionally renders either pending points or pending issues based on the
estimateType, which is consistent with the component's new focus on estimate types.
Line range hint
1-134: Summary: Successful transition from plot types to estimate typesThe changes in this file consistently implement a transition from plot types to estimate types in the
ActiveCycleProductivitycomponent. This refactoring includes updates to imports, variable names, function calls, and rendering logic. The changes are well-implemented and maintain the component's functionality while shifting its focus.Key points:
- Import statements have been updated to include
TCycleEstimateType.- The
cycleBurnDownChartOptionsarray now uses "issues" instead of "burndown".- Hooks and derived values now use estimate type functions.
- The
onChangefunction has been updated to handle estimate types.- Chart distribution data logic now considers the estimate type.
- The CustomSelect component has been updated to use
estimateType.- Rendering logic for pending points or issues now uses the estimate type.
These changes align with the PR objective of "Chore: progress chart code splitting" and appear to be part of a larger refactoring effort. The implementation is consistent and maintains the component's functionality while improving its focus on estimate types.
🧰 Tools
🪛 Biome
[error] 40-40: 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/cycles/analytics-sidebar/progress-stats.tsx (2)
222-225: LGTM: Addition of "States" entry to progressStats arrayThe addition of the "States" entry to the beginning of the
progressStatsarray is consistent with the PR objective and aligns with the changes described in the AI-generated summary. This modification will allow for the display of state-based statistics in the progress chart.
Line range hint
1-368: Summary: Progress chart code splitting implementation looks goodThe changes made to the
progressStatsarray and theCycleProgressStatscomponent successfully implement the progress chart code splitting as intended. The addition of the "States" entry and correspondingTab.Panelallows for the display of state-based statistics in the progress chart.The implementation is consistent and well-structured. A minor suggestion was made to improve consistency in prop passing, but overall, the changes are approved and ready for merging.
web/core/components/cycles/analytics-sidebar/root.tsx (2)
50-51: Ensure safe conversion of parameters when calling.toString()As mentioned earlier, calling
.toString()on potentially undefinedworkspaceSlugandprojectIdcan lead to runtime errors. UseString()for safe conversion or ensure these parameters are defined.
61-62: Ensure safe conversion of parameters when calling.toString()web/ce/components/cycles/active-cycle/root.tsx (1)
27-89: Component Implementation Looks GoodThe
ActiveCycleRootcomponent is well-structured, and the logic for displaying and managing the active cycle details is clear and effective. The use of hooks and state management appears appropriate, and props are correctly passed to child components.web/helpers/cycle.helper.ts (1)
1-1: Imports are appropriate and necessaryThe imported modules are used correctly within the code.
Also applies to: 5-5
web/core/components/cycles/list/cycle-list-item-action.tsx (9)
6-6: Approve the addition of new icon importsThe import of
EyeandUsersicons fromlucide-reactis appropriate for the added functionality.
37-37: Approve addition of optionalisActivepropThe inclusion of the optional
isActiveboolean prop enhances the component's conditional rendering capabilities.
46-46: DestructureisActiveprop with a default valueDestructuring
isActivefrom props with a default value offalseis appropriate and ensures backward compatibility.
49-52: Approve addition of router hooksUsing
useAppRouter,useSearchParams, andusePathnamehooks effectively manages routing and query parameters.
195-205: Confirm correctness ofopenCycleOverviewhandlerThe
openCycleOverviewfunction correctly toggles the cycle overview based on thepeekCyclequery parameter.
209-216: Conditional rendering of the "More details" buttonThe button is appropriately displayed based on the
isMobileorisActivestate, enhancing user accessibility.
254-254: Approve conditional rendering ofButtonAvatarsRendering
ButtonAvatarswhencreatedByDetailsexists andisActiveis false is appropriate.
256-271: Approve conditional rendering of member avatarsThe
TooltipandAvatarGroupcomponents are correctly rendered based on theisActivestate and the presence ofassignee_ids.
276-277: Prevent default event behavior in favorite actionAdding
e.preventDefault()ande.stopPropagation()ensures that clicking the favorite star does not trigger unintended navigation or parent event handlers.web/core/components/cycles/analytics-sidebar/issue-progress.tsx (1)
181-181:⚠️ Potential issueSimplify conditional expression in
defaultOpenpropThe ternary operator
isCycleDateValid ? true : falseis unnecessary since it directly evaluates toisCycleDateValid. You can simplify the code by assigningisCycleDateValiddirectly.Apply this diff to simplify the expression:
- <Disclosure defaultOpen={isCycleDateValid ? true : false}> + <Disclosure defaultOpen={isCycleDateValid}>Likely invalid or redundant comment.
web/core/components/cycles/analytics-sidebar/sidebar-header.tsx (1)
302-302: Confirm if 'minDate' should restrict selecting past datesSetting
minDate={new Date()}restricts users from selecting past dates. If cycles can have start dates in the past, consider adjusting or removingminDate.web/core/store/cycle.store.ts (19)
14-15: Imports are correctly addedThe types
TCycleEstimateTypeandTCycleProgressare properly imported, which are essential for the new functionalities introduced.
18-18: ImportedformatActiveCyclehelper functionThe
formatActiveCyclefunction is correctly imported fromcycle.helper, ensuring that any formatting related to active cycles can be utilized.
32-32: AddedprogressLoaderproperty to interfaceThe
progressLoader: booleanproperty is appropriately added to theICycleStoreinterface to handle loading states for cycle progress operations.
37-37: IntroducedestimatedTypeproperty to interfaceThe
estimatedType: Record<string, TCycleEstimateType>property is properly added to manage estimate types for cycles.
51-51: DefinedgetActiveCycleProgressmethod in interfaceThe method
getActiveCycleProgressis correctly declared in theICycleStoreinterface, facilitating retrieval of active cycle progress details.
60-60: AddedgetEstimateTypeByCycleIdmethod to interfaceThe
getEstimateTypeByCycleIdmethod is properly added, allowing retrieval of the estimate type for a specific cycle.
65-65: IncludedsetEstimateTypemethod in interfaceThe method
setEstimateTypeis correctly included in theICycleStoreinterface to update the estimate type of a cycle.
74-78: DeclaredfetchActiveCycleProgressPromethod in interfaceThe
fetchActiveCycleProgressPromethod is added to the interface, preparing for enhanced progress fetching for pro users.
105-105: InitializedprogressLoaderpropertyThe
progressLoaderis initialized tofalse, ensuring a default non-loading state.
108-108: InitializedestimatedTypepropertyThe
estimatedTypeproperty is initialized as an empty object, ready to store estimate types keyed by cycle IDs.
124-124: MadeprogressLoaderobservableThe
progressLoaderproperty is correctly set as an observable reference, allowing reactive updates in the UI.
127-127: MadeestimatedTypeobservableThe
estimatedTypeproperty is properly set as an observable, enabling reactivity when estimate types change.
142-142: RegisteredsetEstimateTypeas an actionThe
setEstimateTypemethod is correctly registered as an action, allowing state modifications within MobX's rules.
149-149: RegisteredfetchActiveCycleProgressProas an actionThe
fetchActiveCycleProgressPromethod is properly registered as an action to perform asynchronous operations.
280-292: ImplementedgetActiveCycleProgressmethodThe
getActiveCycleProgressmethod is well implemented usingcomputedFn, efficiently computing values based on the cycle ID.
409-410: UpdatedgetPlotTypeByCycleIdmethodThe method correctly returns the plot type for a given cycle ID, defaulting to
"burndown"if not set.
411-421: ImplementedgetEstimateTypeByCycleIdmethodThe
getEstimateTypeByCycleIdmethod accurately determines the estimate type based on project settings and cycle-specific configurations.
431-438: ImplementedsetEstimateTypemethodThe
setEstimateTypemethod correctly updates theestimatedTypemap with the new estimate type for the given cycle ID.
530-539: EnhancedfetchActiveCycleProgresswith loading stateThe inclusion of
progressLoaderbefore and after the asynchronous call ensures proper loading state handling.
| @@ -1,3 +1,6 @@ | |||
| export * from "./root"; | |||
| export * from "./issue-progress"; | |||
| export * from "./progress-stats"; | |||
| export * from "./root"; | |||
There was a problem hiding this comment.
Remove redundant export statement.
The export statement for the "root" module is duplicated. This redundancy should be removed to maintain clean and efficient code.
Apply this diff to remove the redundant export:
export * from "./root";
export * from "./issue-progress";
export * from "./progress-stats";
-export * from "./root";
export * from "./sidebar-header";
export * from "./sidebar-details";📝 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 * from "./root"; | |
| export * from "./root"; | |
| export * from "./issue-progress"; | |
| export * from "./progress-stats"; | |
| export * from "./sidebar-header"; | |
| export * from "./sidebar-details"; |
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| export const DoneState: React.FC<ISvgIcons> = ({ width = "10", height = "11", className, color }) => ( | ||
| <svg | ||
| width={width} | ||
| height={height} | ||
| viewBox="0 0 10 11" | ||
| fill="none" | ||
| xmlns="http://www.w3.org/2000/svg" | ||
| className={className} | ||
| > |
There was a problem hiding this comment.
Address unused 'color' prop and consider making SVG colors dynamic.
The component declaration looks good, but there's an issue with the 'color' prop:
- The 'color' prop is destructured but not used in the component. This could lead to confusion for users of the component.
- The SVG uses hardcoded colors, which limits its reusability.
Consider the following improvements:
- Remove the unused 'color' prop if it's not needed.
- If color customization is intended, use the 'color' prop to set the SVG colors dynamically.
Here's a suggested implementation:
- export const DoneState: React.FC<ISvgIcons> = ({ width = "10", height = "11", className, color }) => (
+ export const DoneState: React.FC<ISvgIcons> = ({ width = "10", height = "11", className, color = "#15A34A" }) => (
<svg
width={width}
height={height}
viewBox="0 0 10 11"
fill="none"
xmlns="http://www.w3.org/2000/svg"
className={className}
>
- <circle cx="5" cy="5.5" r="4.4" stroke="#15A34A" stroke-width="1.2" />
+ <circle cx="5" cy="5.5" r="4.4" stroke={color} stroke-width="1.2" />
<path
fill-rule="evenodd"
clip-rule="evenodd"
d="M2.5 5.59375L3.82582 6.91957L4.26777 6.47763L2.94194 5.15181L2.5 5.59375ZM4.26777 7.36152L7.36136 4.26793L6.91942 3.82599L3.82583 6.91958L4.26777 7.36152Z"
- fill="#15A34A"
+ fill={color}
/>
</svg>
);This change allows users to customize the icon color while maintaining a default value.
📝 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 DoneState: React.FC<ISvgIcons> = ({ width = "10", height = "11", className, color }) => ( | |
| <svg | |
| width={width} | |
| height={height} | |
| viewBox="0 0 10 11" | |
| fill="none" | |
| xmlns="http://www.w3.org/2000/svg" | |
| className={className} | |
| > | |
| export const DoneState: React.FC<ISvgIcons> = ({ width = "10", height = "11", className, color = "#15A34A" }) => ( | |
| <svg | |
| width={width} | |
| height={height} | |
| viewBox="0 0 10 11" | |
| fill="none" | |
| xmlns="http://www.w3.org/2000/svg" | |
| className={className} | |
| > | |
| <circle cx="5" cy="5.5" r="4.4" stroke={color} stroke-width="1.2" /> | |
| <path | |
| fill-rule="evenodd" | |
| clip-rule="evenodd" | |
| d="M2.5 5.59375L3.82582 6.91957L4.26777 6.47763L2.94194 5.15181L2.5 5.59375ZM4.26777 7.36152L7.36136 4.26793L6.91942 3.82599L3.82583 6.91958L4.26777 7.36152Z" | |
| fill={color} | |
| /> | |
| </svg> | |
| ); |
| handleClose={handleClose} | ||
| isArchived={isArchived} | ||
| /> | ||
| <CycleDetailsSidebar handleClose={handleClose} isArchived={isArchived} cycleId={peekCycle} /> |
There was a problem hiding this comment.
💡 Codebase verification
Potential Type Issue with peekCycle in CycleDetailsSidebar
It appears that peekCycle can be null, but it's being passed directly to CycleDetailsSidebar as a prop without ensuring it's a string. This could introduce type-related issues.
- Review the prop types for
CycleDetailsSidebarto confirm thatcycleIdexpects a string. - Ensure that
peekCycleis always a string before passing it, possibly by adding type guards or default values.
🔗 Analysis chain
Simplified prop passing for CycleDetailsSidebar
The change in prop passing for cycleId looks good as it simplifies the code. However, we should verify that peekCycle is always a string to ensure this change doesn't introduce any type-related issues.
Let's verify the type of peekCycle and its usage:
This script will help us confirm that the change is safe and doesn't introduce any potential issues.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the type and usage of peekCycle
# Check the type of peekCycle
echo "Checking the type of peekCycle:"
ast-grep --lang typescript --pattern 'const peekCycle = searchParams.get("peekCycle")'
# Check if peekCycle is used with toString() elsewhere in the file
echo "Checking if peekCycle is used with toString() elsewhere:"
rg 'peekCycle.*toString\(\)' web/core/components/cycles/cycle-peek-overview.tsx
# Check if CycleDetailsSidebar expects a string for cycleId
echo "Checking CycleDetailsSidebar prop types:"
ast-grep --lang typescript --pattern 'type $Props = {
$$$
cycleId: $_
$$$
}'
Length of output: 713
|
|
||
| import { ISvgIcons } from "./type"; | ||
|
|
||
| export const PlannedState: React.FC<ISvgIcons> = ({ width = "10", height = "11", className, color }) => ( |
There was a problem hiding this comment.
Address unused color prop.
The color prop is destructured in the component parameters but not used in the SVG. This could lead to confusion for developers using this component.
Consider one of the following options:
- Remove the
colorprop if it's not needed. - Implement color customization using the
colorprop as suggested in the previous comment. - If the
colorprop is intended for future use, add a TODO comment explaining its purpose.
| )} | ||
| style={{ | ||
| boxShadow: | ||
| "0px 1px 4px 0px rgba(0, 0, 0, 0.06), 0px 2px 4px 0px rgba(16, 24, 40, 0.06), 0px 1px 8px -1px rgba(16, 24, 40, 0.06)", | ||
| }} | ||
| > | ||
| <CycleDetailsSidebar cycleId={cycleId.toString()} handleClose={toggleSidebar} /> | ||
| <CycleDetailsSidebar handleClose={toggleSidebar} /> |
There was a problem hiding this comment.
💡 Codebase verification
CycleDetailsSidebar still utilizes the cycleId prop.
The cycleId prop has been removed from the CycleDetailsSidebar component in web/app/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/cycles/(detail)/[cycleId]/page.tsx. However, the search results indicate that cycleId is still being passed to CycleDetailsSidebar in other parts of the codebase, which may lead to inconsistencies or runtime errors.
- Instances to review:
web/core/components/cycles/cycle-peek-overview.tsxweb/core/components/cycles/list/cycles-list-item-action.tsxweb/core/components/cycles/board/cycles-board-card.tsx
Please verify all usages of the CycleDetailsSidebar component to ensure that the removal of the cycleId prop does not negatively impact functionality.
🔗 Analysis chain
Verify CycleDetailsSidebar functionality after prop removal.
The cycleId prop has been removed from the CycleDetailsSidebar component. This simplification of the component's interface is good for maintainability.
Please ensure that the CycleDetailsSidebar component can still function correctly without the cycleId prop. Run the following script to check if cycleId is still being used within the CycleDetailsSidebar component:
If the script returns any results, it indicates that cycleId is still being used within the component, which may cause issues.
Please run the following script to verify if cycleId is still being used within the CycleDetailsSidebar component:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if cycleId is still being used within the CycleDetailsSidebar component.
# Test: Search for cycleId usage in CycleDetailsSidebar
ast-grep --lang typescript --pattern 'const CycleDetailsSidebar = ($_) => { $$$ cycleId $$$ }'
Length of output: 96
Script:
#!/bin/bash
# Description: Search for any occurrences of cycleId within the CycleDetailsSidebar component.
# Search for 'cycleId' usage in CycleDetailsSidebar
rg 'cycleId' $(fd 'CycleDetailsSidebar' --type f)
Length of output: 72381
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| import React, { FC, useEffect, useState } from "react"; | ||
| import { Controller, useForm } from "react-hook-form"; | ||
| import { ArchiveIcon, ArchiveRestoreIcon, ChevronRight, EllipsisIcon, LinkIcon, Trash2 } from "lucide-react"; | ||
| import { observer } from "mobx-react"; |
There was a problem hiding this comment.
Reorder imports to maintain proper sequencing
To adhere to the project's import organization standards, place the mobx-react import before the react-hook-form import.
Apply this diff to fix the import order:
import React, { FC, useEffect, useState } from "react";
+import { observer } from "mobx-react";
import { Controller, useForm } from "react-hook-form";
-import { observer } from "mobx-react";Committable suggestion was skipped due to low confidence.
🧰 Tools
🪛 GitHub Check: lint-web
[failure] 6-6:
mobx-reactimport should occur before import ofreact-hook-form
| title: "Restore success", | ||
| message: "Your cycle can be found in project cycles.", | ||
| }); | ||
| router.push(`/${workspaceSlug.toString()}/projects/${projectId.toString()}/archives/cycles`); |
There was a problem hiding this comment.
Redirect to the active cycles page after restoring
After restoring the cycle, the user is redirected to the archived cycles page. Since the cycle is now active, consider redirecting to the active cycles page to reflect the updated state and enhance user experience.
Adjust the router.push call to point to the active cycles:
- router.push(`/${workspaceSlug.toString()}/projects/${projectId.toString()}/archives/cycles`);
+ router.push(`/${workspaceSlug.toString()}/projects/${projectId.toString()}/cycles/${cycleDetails.id}`);Committable suggestion was skipped due to low confidence.
| }); | ||
| }, [cycleDetails, reset]); | ||
|
|
||
| const dateChecker = async (payload: any) => { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Specify the type of 'payload' parameter instead of 'any'
Using any defeats the purpose of TypeScript's type safety. Define a specific type for the payload parameter in the dateChecker function to enhance code robustness.
Update the function signature with a defined interface:
+interface DateCheckerPayload {
+ start_date: string;
+ end_date: string;
+ cycle_id?: string;
+}
- const dateChecker = async (payload: any) => {
+ const dateChecker = async (payload: DateCheckerPayload) => {📝 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.
| const dateChecker = async (payload: any) => { | |
| interface DateCheckerPayload { | |
| start_date: string; | |
| end_date: string; | |
| cycle_id?: string; | |
| } | |
| const dateChecker = async (payload: DateCheckerPayload) => { |
web/core/components/cycles/analytics-sidebar/sidebar-header.tsx
Outdated
Show resolved
Hide resolved
| const { currentActiveEstimateId, areEstimateEnabledByProjectId, estimateById } = useProjectEstimates(); | ||
|
|
||
| // derived values | ||
| const plotType: TCyclePlotType = (cycle && getPlotTypeByCycleId(cycle.id)) || "burndown"; | ||
| const estimateType: TCycleEstimateType = (cycle && getEstimateTypeByCycleId(cycle.id)) || "issues"; |
There was a problem hiding this comment.
Will progressUnit be a better name?
There was a problem hiding this comment.
Its a judgement call, to me estimateType looks better, not sure which one to go ahead with
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- web/core/components/cycles/analytics-sidebar/index.ts (1 hunks)
- web/core/components/cycles/analytics-sidebar/sidebar-details.tsx (1 hunks)
- web/core/components/cycles/analytics-sidebar/sidebar-header.tsx (1 hunks)
- web/core/components/cycles/list/cycle-list-item-action.tsx (5 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- web/core/components/cycles/analytics-sidebar/index.ts
- web/core/components/cycles/analytics-sidebar/sidebar-header.tsx
- web/core/components/cycles/list/cycle-list-item-action.tsx
🧰 Additional context used
🪛 Biome
web/core/components/cycles/analytics-sidebar/sidebar-details.tsx
[error] 42-44: 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)
[error] 45-47: 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)
🔇 Additional comments (5)
web/core/components/cycles/analytics-sidebar/sidebar-details.tsx (5)
1-18: LGTM: Imports and type definitions are well-organized.The imports are properly structured, and the type definition for the component props is clear and concise. The previously mentioned import order issues have been resolved in this version.
20-28: LGTM: Component definition and hook usage are well-implemented.The
CycleSidebarDetailscomponent is correctly defined as a functional component using MobX's observer. The hooks are used appropriately to fetch necessary data, and the initial calculations based on cycle status and estimates are logical and clear.
30-40: LGTM: Issue count and estimate point calculations are correct.The logic for calculating
issueCountandissueEstimatePointCountis complex but appears to handle all necessary cases correctly, including completed cycles and different progress snapshot scenarios.Also applies to: 48-56
57-134: LGTM: Component rendering logic is well-implemented.The rendering logic for the
CycleSidebarDetailscomponent is well-structured and uses appropriate UI components from@plane/ui. The conditional rendering for the description and estimate points is correctly implemented. The layout uses flexbox for responsive design, ensuring a good user experience across different screen sizes.
1-138: Overall implementation aligns well with PR objectives.The
CycleSidebarDetailscomponent is well-implemented and aligns with the PR objective of implementing code splitting for the progress chart component. The component is modular, follows React best practices, and effectively encapsulates the logic for displaying cycle details, including user and issue information.Key strengths of the implementation:
- Proper use of MobX for state management
- Effective use of hooks for data fetching
- Clear type definitions
- Responsive layout using flexbox
- Appropriate conditional rendering
The only suggested improvement is the simplification of the
isEstimatePointValidcalculation, as mentioned in a previous comment. Once that's addressed, the component will be in excellent shape.🧰 Tools
🪛 Biome
[error] 42-44: 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)
[error] 45-47: 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)
| const isEstimatePointValid = isEmpty(cycleDetails?.progress_snapshot || {}) | ||
| ? estimateType && estimateType?.type == EEstimateSystem.POINTS | ||
| ? true | ||
| : false | ||
| : isEmpty(cycleDetails?.progress_snapshot?.estimate_distribution || {}) | ||
| ? false | ||
| : true; |
There was a problem hiding this comment.
Simplify the isEstimatePointValid calculation.
The current implementation uses nested ternary operators with boolean literals, which can be simplified for better readability and maintainability.
Consider refactoring the isEstimatePointValid calculation as follows:
const isEstimatePointValid = isEmpty(cycleDetails?.progress_snapshot || {})
? estimateType?.type === EEstimateSystem.POINTS
: !isEmpty(cycleDetails?.progress_snapshot?.estimate_distribution || {});This simplification removes the unnecessary boolean literals and makes the logic more straightforward.
🧰 Tools
🪛 Biome
[error] 42-44: 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)
[error] 45-47: 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)
SatishGandham
changed the title
…re-progress-chart
Summary by CodeRabbit
Release Notes
New Features
CycleSidebarHeaderandCycleSidebarDetailscomponents for enhanced sidebar functionality in cycle management.Improvements
Bug Fixes
Documentation