[WEB-2924] improvement: optimize Treemap chart for large datasets #6369
Conversation
WalkthroughThis pull request introduces enhancements to the tree map chart components, focusing on improving type definitions, configuration options, and rendering flexibility. The changes span multiple files in the charts module, adding new types for content visibility, updating the Changes
Sequence DiagramsequenceDiagram
participant Chart as TreeMapChart
participant Content as CustomTreeMapContent
participant Tooltip as TreeMapTooltip
Chart->>Content: Render with dimensions
Content-->>Content: Calculate visibility
Content-->>Content: Truncate text
Content->>Chart: Render chart content
alt Tooltip Enabled
Chart->>Tooltip: Show tooltip
Tooltip-->>Tooltip: Process payload
Tooltip->>Chart: Display tooltip details
end
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
web/core/components/core/charts/tree-map/tooltip.tsx (1)
5-8: Improve type definitions forpayloadprop inTreeMapTooltipThe
payloadprop is currently typed asany[], which reduces type safety. Consider using the appropriate types fromrechartsor defining a specific type to enhance type checking and prevent potential runtime errors.You can import
TooltipPropsfromrechartsand use itspayloadtype:import { TooltipProps } from "recharts"; interface TreeMapTooltipProps { active: boolean | undefined; payload: TooltipProps["payload"]; }Adjust the component accordingly to use the proper types.
packages/types/src/charts.d.ts (2)
57-72: Consider making configuration properties optional for better flexibility.The new configuration types provide excellent granular control over content visibility, which aligns well with optimizing performance for large datasets. However, consider making some properties optional to provide more flexibility and better backward compatibility.
export type TTopSectionConfig = { - showIcon: boolean; - showName: boolean; - nameTruncated: boolean; + showIcon?: boolean; + showName?: boolean; + nameTruncated?: boolean; }; export type TBottomSectionConfig = { - show: boolean; - showValue: boolean; - showLabel: boolean; - labelTruncated: boolean; + show?: boolean; + showValue?: boolean; + showLabel?: boolean; + labelTruncated?: boolean; };This change would:
- Allow partial configurations
- Enable gradual adoption of new features
- Maintain backward compatibility
Line range hint
42-72: Good architectural changes for performance optimization.The type changes effectively support performance optimization for large datasets by:
- Providing clear separation between styling approaches (color vs. class-based)
- Adding granular control over tooltip visibility
- Introducing structured configuration for selective content rendering
These changes lay a good foundation for optimizing rendering performance through selective content display and flexible styling options.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/types/src/charts.d.ts(2 hunks)web/core/components/core/charts/tree-map/map-content.tsx(3 hunks)web/core/components/core/charts/tree-map/root.tsx(2 hunks)web/core/components/core/charts/tree-map/tooltip.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (python)
🔇 Additional comments (3)
web/core/components/core/charts/tree-map/root.tsx (1)
11-11: LGTM!The addition of the
showTooltipprop enhances the component's flexibility, and the implementation appears correct.packages/types/src/charts.d.ts (2)
54-55: LGTM! Good addition for tooltip control.The
showTooltipproperty follows the existing pattern inTStackedBarChartPropsand provides good control over chart interactivity.
42-45: Verify the mutual exclusivity of fillColor and fillClassName.The type definition enforces that a TreeMapItem can have either
fillColororfillClassName, but not both. Please confirm if this mutual exclusivity is intentional, as there might be cases where both a base color and additional styling classes are needed.✅ Verification successful
Mutual exclusivity between fillColor and fillClassName is correct and beneficial
The implementation in
map-content.tsxshows thatfillClassNameis used for comprehensive styling control, including transitions and hover effects. Having mutually exclusive styling properties prevents conflicts and provides clear, distinct paths for either direct color application (fillColor) or complex styling (fillClassName). This is a good TypeScript pattern that enforces clean styling practices.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for existing TreeMapItem usage to verify if this change impacts current implementations rg -t typescript -t javascript "fillColor.*fillClassName|fillClassName.*fillColor"Length of output: 119
Script:
#!/bin/bash # Search for TreeMapItem type references and usage rg --type-add 'ts:*.{ts,tsx}' --type-add 'js:*.{js,jsx}' -t ts -t js "TreeMapItem" # Search for files containing both fillColor and fillClassName rg --type-add 'ts:*.{ts,tsx}' --type-add 'js:*.{js,jsx}' -t ts -t js -l "fillColor" | xargs rg "fillClassName" # Search for treemap implementation files fd -e ts -e tsx -e js -e jsx treemap -iLength of output: 699
| return `${stringText.slice(0, maxChars - 3)}...`; | ||
| }; | ||
|
|
||
| export const CustomTreeMapContent: React.FC<any> = ({ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Define explicit prop types for CustomTreeMapContent instead of using any
Using any defeats TypeScript's type safety. Define a specific interface for the props of CustomTreeMapContent to enhance code robustness and maintainability.
Apply this diff to define explicit prop types:
-export const CustomTreeMapContent: React.FC<any> = ({
+interface CustomTreeMapContentProps {
+ x: number;
+ y: number;
+ width: number;
+ height: number;
+ name: string;
+ value: number;
+ label: string;
+ fillColor: string;
+ fillClassName: string;
+ textClassName: string;
+ icon: React.ReactNode;
+}
+
+export const CustomTreeMapContent: React.FC<CustomTreeMapContentProps> = ({📝 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 CustomTreeMapContent: React.FC<any> = ({ | |
| interface CustomTreeMapContentProps { | |
| x: number; | |
| y: number; | |
| width: number; | |
| height: number; | |
| name: string; | |
| value: number; | |
| label: string; | |
| fillColor: string; | |
| fillClassName: string; | |
| textClassName: string; | |
| icon: React.ReactNode; | |
| } | |
| export const CustomTreeMapContent: React.FC<CustomTreeMapContentProps> = ({ |
Description
Type of Change
Screenshots and Media (if applicable)
Test Scenarios
References
Summary by CodeRabbit
Release Notes
New Features
Improvements
Technical Updates