fix(dashboard): add top padding to "Create new chart" button in builder pane#40033
Conversation
dosubot
Bot
added
dashboard:design
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #4ef44b
Actionable Suggestions - 1
-
superset-frontend/plugins/plugin-chart-echarts/src/Heatmap/transformProps.ts - 1
- CWE-1321: Generic Object Injection Sink vulnerability · Line 385-398
Review Details
-
Files reviewed - 3 · Commit Range:
c131dd2..567a12b- superset-frontend/plugins/plugin-chart-echarts/src/Heatmap/transformProps.ts
- superset-frontend/plugins/plugin-chart-echarts/test/Heatmap/transformProps.test.ts
- superset-frontend/src/dashboard/components/SliceAdder.tsx
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Eslint (Linter) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
| let suffix = 'heatmap'; | ||
| if (typeof value === 'number') { | ||
| if (normalizeAcross === 'x') { | ||
| percentage = value / totals.x[String(xValue)]; | ||
| // Convert xValue to a key type (string or number) for totals lookup | ||
| const xKey = xValue as string | number; | ||
| percentage = value / totals.x[xKey]; | ||
| suffix = formattedX; | ||
| } else if (normalizeAcross === 'y') { | ||
| percentage = value / totals.y[String(yValue)]; | ||
| // Convert yValue to a key type (string or number) for totals lookup | ||
| const yKey = yValue as string | number; | ||
| percentage = value / totals.y[yKey]; | ||
| suffix = formattedY; | ||
| } else { | ||
| percentage = value / totals.total; |
There was a problem hiding this comment.
Two instances of unsafe object property access using unsanitized keys (xKey and yKey) at lines 390 and 395. This creates a Generic Object Injection Sink vulnerability (CWE-1321). Validate or sanitize keys before using them for object property access.
Code suggestion
Check the AI-generated fix before applying
| let suffix = 'heatmap'; | |
| if (typeof value === 'number') { | |
| if (normalizeAcross === 'x') { | |
| percentage = value / totals.x[String(xValue)]; | |
| // Convert xValue to a key type (string or number) for totals lookup | |
| const xKey = xValue as string | number; | |
| percentage = value / totals.x[xKey]; | |
| suffix = formattedX; | |
| } else if (normalizeAcross === 'y') { | |
| percentage = value / totals.y[String(yValue)]; | |
| // Convert yValue to a key type (string or number) for totals lookup | |
| const yKey = yValue as string | number; | |
| percentage = value / totals.y[yKey]; | |
| suffix = formattedY; | |
| } else { | |
| percentage = value / totals.total; | |
| let suffix = 'heatmap'; | |
| if (typeof value === 'number') { | |
| if (normalizeAcross === 'x') { | |
| // Convert xValue to a key type (string or number) for totals lookup | |
| const xKey = xValue as string | number; | |
| if (Object.prototype.hasOwnProperty.call(totals.x, xKey)) { | |
| percentage = value / totals.x[xKey]; | |
| } | |
| suffix = formattedX; | |
| } else if (normalizeAcross === 'y') { | |
| // Convert yValue to a key type (string or number) for totals lookup | |
| const yKey = yValue as string | number; | |
| if (Object.prototype.hasOwnProperty.call(totals.y, yKey)) { | |
| percentage = value / totals.y[yKey]; | |
| } | |
| suffix = formattedY; | |
| } else { | |
| percentage = value / totals.total; |
Code Review Run #4ef44b
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
The PR title and description indicate a small CSS fix to add top padding to the "Create new chart" button in the dashboard builder pane. However, the diff also contains substantial unrelated changes to the Heatmap chart's transformProps.ts and its test file.
Changes:
- Adds
sizeUnit * 3top padding toNewChartButtonContainerinSliceAdder.tsx. - Refactors Heatmap
transformPropsto usexAxisColumnName/yAxisColumnName(fromcolnames) instead ofgetColumnLabel(xAxis/groupby)for tooltip totals lookup, removing related imports/destructuring. - Adds five new tests covering tooltip formatter behavior with various sort orders, percentage calculations, and numeric axes.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| superset-frontend/src/dashboard/components/SliceAdder.tsx | Adds top/bottom padding to the new-chart button container. |
| superset-frontend/plugins/plugin-chart-echarts/src/Heatmap/transformProps.ts | Switches tooltip totals lookup keys from form-data column labels to query colnames. |
| superset-frontend/plugins/plugin-chart-echarts/test/Heatmap/transformProps.test.ts | Adds tooltip formatter tests across sort orders, normalization modes, and numeric axes. |
| xAxisColumnName, | ||
| yAxisColumnName, |
| expect(tooltipHtml).toContain('11'); | ||
| // Should not contain raw indices | ||
| expect(tooltipHtml).not.toMatch(/\b1\s*\(/); | ||
| expect(tooltipHtml).not.toMatch(/\(\s*2\b/); |
| expect(tooltipHtml).toContain('2021'); | ||
| expect(tooltipHtml).toContain('2'); |
…er pane Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
yousoph
force-pushed
the
fix/dashboard-create-chart-button-spacing
branch
from
56e85a0 to
6cfb10d
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #40033 +/- ##
=======================================
Coverage 64.14% 64.15%
=======================================
Files 2590 2590
Lines 138104 138044 -60
Branches 32039 32028 -11
=======================================
- Hits 88593 88560 -33
+ Misses 47990 47966 -24
+ Partials 1521 1518 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Code Review Agent Run #062977Actionable 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 |
SUMMARY
The
NewChartButtonContainerhad no top padding, causing the "+ Create new chart" button in the dashboard builder pane to sit flush against the tab bar divider. AddssizeUnit * 3(12px) top padding for proper visual separation.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before:
After:
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION