fix(treemap): restore sequential coloring + remove label box, 1px gap (fixes #36807) #36847
Conversation
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
Code Review Agent Run #d0ad56Actionable Suggestions - 0Additional Suggestions - 2
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
codeant-ai-for-open-source
bot
added
the
size:M
Nitpicks 🔍
|
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| const metricValues = (data || []) | ||
| .map(row => { | ||
| const v = row[metricLabel as string]; | ||
| return typeof v === 'number' ? v : Number(v); |
There was a problem hiding this comment.
Suggestion: Null/undefined metric values are converted to 0 by using Number(null) which yields 0, so missing values are treated as valid numeric zeros and will skew min/max and enable sequential coloring incorrectly; exclude null/undefined before converting to Number so only real numeric-like values are considered. [logic error]
Severity Level: Minor
| return typeof v === 'number' ? v : Number(v); | |
| // treat null/undefined as missing values (NaN), so they are filtered out | |
| if (v === null || v === undefined) { | |
| return NaN; | |
| } |
Why it matters? ⭐
This is a valid logic bug in the PR: Number(null) === 0, so null/undefined metric cells get treated as numeric zeros and will skew min/max and enableSequential detection.
The improved code correctly treats null/undefined as missing (NaN) which the subsequent Number.isFinite filter removes. This fixes the statistics used to decide sequential coloring.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-echarts/src/Treemap/transformProps.ts
**Line:** 172:172
**Comment:**
*Logic Error: Null/undefined metric values are converted to 0 by using Number(null) which yields 0, so missing values are treated as valid numeric zeros and will skew min/max and enable sequential coloring incorrectly; exclude null/undefined before converting to Number so only real numeric-like values are considered.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| const minMetricValue = metricValues.length ? Math.min(...metricValues) : 0; | ||
| const maxMetricValue = metricValues.length ? Math.max(...metricValues) : 0; |
There was a problem hiding this comment.
Suggestion: Using Math.min(...metricValues) / Math.max(...metricValues) with the spread operator can blow the call stack or consume excessive memory for large datasets; compute min/max with an iterative reduce to avoid spreading a large array into a function call. [performance]
Severity Level: Minor
| const minMetricValue = metricValues.length ? Math.min(...metricValues) : 0; | |
| const maxMetricValue = metricValues.length ? Math.max(...metricValues) : 0; | |
| const minMetricValue = | |
| metricValues.length > 0 | |
| ? metricValues.reduce((acc, val) => (val < acc ? val : acc), metricValues[0]) | |
| : 0; | |
| const maxMetricValue = | |
| metricValues.length > 0 | |
| ? metricValues.reduce((acc, val) => (val > acc ? val : acc), metricValues[0]) | |
| : 0; |
Why it matters? ⭐
Using reduce to compute min/max avoids creating huge argument lists for Math.min/Math.max, which can blow the call stack for very large datasets. The suggested reduce-based calculation is safer and more memory-friendly.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-echarts/src/Treemap/transformProps.ts
**Line:** 175:176
**Comment:**
*Performance: Using Math.min(...metricValues) / Math.max(...metricValues) with the spread operator can blow the call stack or consume excessive memory for large datasets; compute min/max with an iterative reduce to avoid spreading a large array into a function call.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
superset-frontend/plugins/plugin-chart-echarts/src/Treemap/transformProps.ts
Outdated
Show resolved
Hide resolved
superset-frontend/plugins/plugin-chart-echarts/test/Treemap/transformProps.test.ts
Outdated
Show resolved
Hide resolved
|
CodeAnt AI finished reviewing your PR. |
…ansformProps.test.ts Co-authored-by: codeant-ai-for-open-source[bot] <244253245+codeant-ai-for-open-source[bot]@users.noreply.github.com>
…nsformProps.ts Co-authored-by: codeant-ai-for-open-source[bot] <244253245+codeant-ai-for-open-source[bot]@users.noreply.github.com>
Code Review Agent Run #1e540bActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
1 participant
User description
SUMMARY
Restore sequential (continuous) coloring for Treemap when a numeric metric with varying values is present, while preserving categorical coloring for categorical/constant metrics.
Remove boxed label appearance and eliminate the 1px gap by default.
Ensure de-emphasized (filtered-out) nodes are muted via opacity rather than overriding their color so gradients remain visible.
Add unit tests to prevent regressions.
BEFORE/AFTER SCREENSHOTS
-- BEFORE :
-- AFTER :
TESTING INSTRUCTIONS / FILES MODIFIED
transformProps.ts— core logic and label/de-emphasis changesconstants.ts— adjusted defaults forBORDER_WIDTH/GAP_WIDTHto remove thin gaptransformProps.test.ts— new/updated testsWhat I Changed
Behavior: Treemap now:
min !== max).UI: Labels no longer display a visible border/background by default, and that 1px gap.
Typing: Replaced
@ts-ignorewith a typed assignment forvisualMap.Tests: Added Jest tests validating
visualMapexistence for numeric-varying metric and absence for constant metrics.Fixes
CodeAnt-AI Description
Restore continuous coloring for numeric treemaps and remove boxed labels
What Changed
Impact
✅ Clearer sequential color gradients for numeric treemaps✅ No boxed labels or thin gaps around treemap labels✅ Filtered nodes retain underlying color gradients (better visual fidelity)💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.