[WEB-4130] fix: cycle charts minor optimizations#7123
[WEB-4130] fix: cycle charts minor optimizations#7123sriramveeraghanta merged 2 commits intopreviewfrom
Conversation
WalkthroughThe changes update UI labeling for estimate options, enhance the Changes
Sequence Diagram(s)sequenceDiagram
participant UI as User Interface
participant Dropdown as EstimateTypeDropdown (observer)
participant Store as ProjectEstimateStore
UI->>Dropdown: Render component
Dropdown->>Store: Access areEstimateEnabledByProjectId, currentProjectEstimateType
Store-->>Dropdown: Return observable values
Dropdown-->>UI: Conditionally render dropdown based on store state and estimate type
Suggested labels
Suggested reviewers
Poem
✨ 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: 0
🧹 Nitpick comments (1)
web/core/components/cycles/dropdowns/estimate-type-dropdown.tsx (1)
21-21: Simplify the unnecessary ternary operator.The ternary operator is redundant since the condition already evaluates to a boolean.
Apply this diff to simplify the expression:
- const isCurrentProjectEstimateEnabled = projectId && areEstimateEnabledByProjectId(projectId) ? true : false; + const isCurrentProjectEstimateEnabled = Boolean(projectId && areEstimateEnabledByProjectId(projectId));Or even simpler, since
areEstimateEnabledByProjectIdlikely returns a boolean:- const isCurrentProjectEstimateEnabled = projectId && areEstimateEnabledByProjectId(projectId) ? true : false; + const isCurrentProjectEstimateEnabled = projectId && areEstimateEnabledByProjectId(projectId);🧰 Tools
🪛 Biome (1.9.4)
[error] 21-21: 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)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
web/core/components/cycles/analytics-sidebar/issue-progress.tsx(1 hunks)web/core/components/cycles/dropdowns/estimate-type-dropdown.tsx(3 hunks)web/core/store/estimates/project-estimate.store.ts(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
web/core/store/estimates/project-estimate.store.ts (1)
packages/types/src/estimate.d.ts (1)
TEstimateSystemKeys(17-17)
web/core/components/cycles/dropdowns/estimate-type-dropdown.tsx (2)
web/core/hooks/store/use-cycle.ts (1)
useCycle(7-11)web/core/store/estimates/project-estimate.store.ts (1)
currentProjectEstimate(79-81)
🪛 Biome (1.9.4)
web/core/components/cycles/dropdowns/estimate-type-dropdown.tsx
[error] 21-21: 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)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (python)
🔇 Additional comments (9)
web/core/store/estimates/project-estimate.store.ts (3)
30-30: LGTM! Well-defined computed property interface.The addition of
currentProjectEstimateto the interface is properly typed and follows consistent patterns with other computed properties.
67-67: LGTM! Proper MobX configuration.The computed property is correctly included in the
makeObservableconfiguration, ensuring reactive updates when dependencies change.
79-81: LGTM! Correct implementation with proper null safety.The computed property correctly:
- Uses optional chaining to safely access the estimate type
- Returns undefined when no active estimate exists
- Leverages the existing
currentActiveEstimateIdcomputed propertyThis implementation supports the UI enhancement in the dropdown component.
web/core/components/cycles/analytics-sidebar/issue-progress.tsx (1)
35-35: LGTM! Improved terminology for estimate types.Changing the label from "Points" to "Estimates" better reflects the broader estimate type handling and is more inclusive of different estimate systems. This aligns well with the overall PR objective to improve estimate type management.
web/core/components/cycles/dropdowns/estimate-type-dropdown.tsx (5)
2-2: LGTM! Proper MobX integration.Adding the
observerwrapper enables reactive updates when the underlying MobX store state changes, which is essential for the new estimate type logic.
4-4: LGTM! Necessary import for estimate system check.The
EEstimateSystemenum import is required for comparing againstEEstimateSystem.CATEGORIESin the rendering condition.
17-17: LGTM! Proper observer pattern implementation.The component is correctly wrapped with
observerto ensure reactive updates when MobX observables change.Also applies to: 42-42
20-20: LGTM! Accessing the new computed property.Destructuring
currentProjectEstimatefromuseProjectEstimates()correctly accesses the new computed property added to the MobX store.
22-23: LGTM! Smart conditional rendering for estimate types.The enhanced rendering condition correctly hides the dropdown when the current project estimate system is set to
CATEGORIES, which aligns with the PR objective to improve estimate type handling. The logic properly combines multiple conditions:
- Points data availability OR estimates enabled for the project
- AND the estimate system is NOT categories
This prevents showing the dropdown when it's not applicable for category-based estimates.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
web/core/components/cycles/dropdowns/estimate-type-dropdown.tsx (1)
21-21: Simplify the unnecessary ternary operator.The ternary operator is redundant since the expression already evaluates to a boolean value.
Apply this diff to simplify the code:
- const isCurrentProjectEstimateEnabled = projectId && areEstimateEnabledByProjectId(projectId) ? true : false; + const isCurrentProjectEstimateEnabled = !!(projectId && areEstimateEnabledByProjectId(projectId));Alternatively, if you're confident that
areEstimateEnabledByProjectIdalways returns a boolean:- const isCurrentProjectEstimateEnabled = projectId && areEstimateEnabledByProjectId(projectId) ? true : false; + const isCurrentProjectEstimateEnabled = projectId && areEstimateEnabledByProjectId(projectId);🧰 Tools
🪛 Biome (1.9.4)
[error] 21-21: 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)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
web/core/components/cycles/dropdowns/estimate-type-dropdown.tsx(3 hunks)web/core/store/estimates/project-estimate.store.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- web/core/store/estimates/project-estimate.store.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
web/core/components/cycles/dropdowns/estimate-type-dropdown.tsx (2)
web/core/hooks/store/use-cycle.ts (1)
useCycle(7-11)web/core/store/estimates/project-estimate.store.ts (1)
currentProjectEstimateType(79-81)
🪛 Biome (1.9.4)
web/core/components/cycles/dropdowns/estimate-type-dropdown.tsx
[error] 21-21: 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)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (4)
web/core/components/cycles/dropdowns/estimate-type-dropdown.tsx (4)
2-2: LGTM! Imports are properly used.The new imports for
observerandEEstimateSystemare correctly utilized in the component for MobX reactivity and conditional rendering logic.Also applies to: 4-4
17-17: LGTM! Proper MobX observer implementation.Wrapping the component with
observeris correct since it now usescurrentProjectEstimateTypefrom the MobX store, ensuring reactive updates when the observable state changes.Also applies to: 42-42
20-20: LGTM! Proper use of the new computed property.The addition of
currentProjectEstimateTypeto the destructuring is correct and aligns with the computed property defined in theProjectEstimateStoreclass.
22-23: LGTM! Conditional logic correctly implements the requirement.The addition of
currentProjectEstimateType !== EEstimateSystem.CATEGORIESproperly hides the dropdown when the estimate type is set to "category", which aligns perfectly with the PR objectives.
3 participants
Description
References
[WEB-4130]
Summary by CodeRabbit
New Features
Style