fix(TableChart): render cell bars for columns with NULL values #36819
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 · |
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}
| .map(row => row[key]) | ||
| .filter(value => typeof value === 'number') as number[]; |
There was a problem hiding this comment.
Suggestion: The current code will exclude Number boxed objects (e.g., new Number(5)) and doesn't explicitly remove NaN/Infinity; normalize boxed numbers and explicitly filter by Number.isFinite to avoid NaN/Infinity contaminating range calculations. [possible bug]
Severity Level: Critical 🚨
| .map(row => row[key]) | |
| .filter(value => typeof value === 'number') as number[]; | |
| .map(row => { | |
| const raw = row[key]; | |
| return raw instanceof Number ? raw.valueOf() : raw; | |
| }) | |
| .filter((value): value is number => | |
| typeof value === 'number' && Number.isFinite(value), | |
| ) as number[]; |
Why it matters? ⭐
The existing code ignores boxed Number objects (new Number(5)) and doesn't explicitly exclude NaN/Infinity. Normalizing boxed numbers via valueOf() and guarding with Number.isFinite prevents non-finite values from contaminating the range calculation — a sensible, small correctness fix.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/useColDefs.ts
**Line:** 71:72
**Comment:**
*Possible Bug: The current code will exclude Number boxed objects (e.g., `new Number(5)`) and doesn't explicitly remove NaN/Infinity; normalize boxed numbers and explicitly filter by `Number.isFinite` to avoid NaN/Infinity contaminating range calculations.
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.| return ( | ||
| alignPositiveNegative ? [0, d3Max(nums.map(Math.abs))] : d3Extent(nums) | ||
| ) as ValueRange; |
There was a problem hiding this comment.
Suggestion: d3Max can return undefined in some cases (type definitions allow it); using [0, d3Max(...)] can produce a range with undefined which will break consumers—compute the max separately and fall back to 0, and ensure d3Extent fallback is handled when it returns undefined. [possible bug]
Severity Level: Critical 🚨
| return ( | |
| alignPositiveNegative ? [0, d3Max(nums.map(Math.abs))] : d3Extent(nums) | |
| ) as ValueRange; | |
| const maxAbs = d3Max(nums.map(Math.abs)); | |
| if (alignPositiveNegative) { | |
| return [0, maxAbs ?? 0] as ValueRange; | |
| } | |
| const extent = d3Extent(nums) as ValueRange | undefined; | |
| return extent ?? [0, 0]; |
Why it matters? ⭐
d3's typings allow d3Max/d3Extent to return undefined even though with a non-empty numeric array they should return numbers at runtime. Handling the potential undefined explicitly (compute max separately and nullish-coalesce / fallback) removes a fragile cast and makes the function safer and clearer to readers and TypeScript. This is a real, low-risk defensive improvement.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/useColDefs.ts
**Line:** 74:76
**Comment:**
*Possible Bug: d3Max can return undefined in some cases (type definitions allow it); using `[0, d3Max(...)]` can produce a range with undefined which will break consumers—compute the max separately and fall back to 0, and ensure d3Extent fallback is handled when it returns undefined.
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.| cells = document.querySelectorAll('td'); | ||
| }); | ||
|
|
||
| test('render cell bars even when column contains NULL values', () => { |
There was a problem hiding this comment.
Suggestion: The test function performs DOM assertions that may depend on async rendering or effect-driven updates but the test is synchronous. Make the test async (add async) so you can await waitFor when asserting DOM nodes that can appear after state/effects; this prevents flaky failures where the assertion runs before the cell bars are rendered. [race condition]
Severity Level: Minor
| test('render cell bars even when column contains NULL values', () => { | |
| test('render cell bars even when column contains NULL values', async () => { |
Why it matters? ⭐
Making the test async (and awaiting any async UI updates) reduces flakiness when DOM updates happen via effects or async layout; it's a safe improvement to test stability.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx
**Line:** 807:807
**Comment:**
*Race Condition: The test function performs DOM assertions that may depend on async rendering or effect-driven updates but the test is synchronous. Make the test async (add `async`) so you can `await` `waitFor` when asserting DOM nodes that can appear after state/effects; this prevents flaky failures where the assertion runs before the cell bars are rendered.
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 row2Cells = rows[1].querySelectorAll('td'); | ||
| const value4Cell = row2Cells[4]; // 5th column (0-indexed) | ||
| const value4Bar = value4Cell.querySelector('div.cell-bar'); |
There was a problem hiding this comment.
Suggestion: Directly indexing into row cells with a hard-coded index (row2Cells[4]) can be out-of-bounds if the table contains different/extra columns (grouping headers, hidden columns, or additional leading columns). If row2Cells[4] is undefined, calling .querySelector on it will throw. Find the column index dynamically from the table headers (or locate the column by header text) and assert the cell exists before querying its contents. [null pointer]
Severity Level: Minor
| const row2Cells = rows[1].querySelectorAll('td'); | |
| const value4Cell = row2Cells[4]; // 5th column (0-indexed) | |
| const value4Bar = value4Cell.querySelector('div.cell-bar'); | |
| // Resolve the column index dynamically from the table headers to avoid brittle hard-coded indices. | |
| const headerCells = container.querySelectorAll('thead th'); | |
| const value4Index = Array.from(headerCells).findIndex(h => | |
| (h.textContent || '').trim() === 'value4', | |
| ); | |
| expect(value4Index).toBeGreaterThanOrEqual(0); | |
| const row2Cells = rows[1].querySelectorAll('td'); | |
| const value4Cell = row2Cells[value4Index]; | |
| expect(value4Cell).toBeDefined(); | |
| const value4Bar = value4Cell!.querySelector('div.cell-bar'); |
Why it matters? ⭐
The test currently assumes a fixed column ordering which is brittle; resolving the column index from headers makes the assertion robust and prevents potential runtime TypeError when table columns change. This is a valid, low-risk improvement to test resilience.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx
**Line:** 872:874
**Comment:**
*Null Pointer: Directly indexing into row cells with a hard-coded index (row2Cells[4]) can be out-of-bounds if the table contains different/extra columns (grouping headers, hidden columns, or additional leading columns). If `row2Cells[4]` is undefined, calling `.querySelector` on it will throw. Find the column index dynamically from the table headers (or locate the column by header text) and assert the cell exists before querying its contents.
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 getValueRange = useCallback( | ||
| function getValueRange(key: string, alignPositiveNegative: boolean) { | ||
| const nums = data |
There was a problem hiding this comment.
Suggestion: Runtime error when data is undefined: nums is derived with optional chaining on data but .filter(...) is called directly after, so if data is undefined nums will be undefined and nums.length will throw. Fix by ensuring nums is always an array (e.g., default data to []) before calling .map/.filter. [null pointer]
Severity Level: Minor
| height: 100%; | ||
| display: block; | ||
| top: 0; | ||
| ${valueRange && |
There was a problem hiding this comment.
Suggestion: Inconsistent conditional: the PR added a typeof value === 'number' guard inside the CSS template but the cell-bar div is still rendered whenever valueRange is truthy. That leaves an inert/empty div (and incorrect 'positive' class for non-number values). Make the render conditional also require typeof value === 'number' (or have the CSS hide the element) so non-numeric/null cells don't render an empty bar element. [logic error]
Severity Level: Minor
|
CodeAnt AI finished reviewing your PR. |
![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 #0fa8d5
Actionable Suggestions - 1
-
superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx - 1
- Runtime Error Risk · Line 388-388
Additional Suggestions - 1
-
superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx - 1
-
Loose Test Assertion · Line 865-865The test assertion uses toBeGreaterThan(0), but the comment indicates an expected count of 10; update to toBe(10) for precision.
Code suggestion
diff - expect(cellBars.length).toBeGreaterThan(0); + expect(cellBars.length).toBe(10);
-
Review Details
-
Files reviewed - 3 · Commit Range:
5e50e73..5e50e73- superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/useColDefs.ts
- superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx
- superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx
-
Files skipped - 0
-
Tools
- Eslint (Linter) - ✔︎ Successful
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ 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 [email protected].
Documentation & Help
AI Code Review powered by
| ?.map(row => row?.[key]) | ||
| .filter(value => typeof value === 'number') as number[]; | ||
| if (data && nums.length === data.length) { | ||
| if (nums.length > 0) { |
There was a problem hiding this comment.
The condition change from checking 'data && nums.length === data.length' to 'nums.length > 0' can cause a runtime TypeError if data is undefined, since nums would be undefined and accessing its length throws an error. The optional chaining on data suggests it can be undefined in some cases.
Code Review Run #0fa8d5
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
1 participant
User description
SUMMARY
Bug: Table chart cell bars didn't render for columns containing NULL values, even when other rows had valid numeric data.
Root Cause: The
getValueRangefunction required all values in a column to be numeric (nums.length === data.length). If any cell was NULL, the column returnednulland no bars were rendered.Solution:
nums.length > 0) instead of all values being numeric.typeof value === 'number') before rendering cell bars to preventBigIntvalues from causingMath.abserrors.Result: Columns with NULL values now render bars for non-NULL numeric cells, and
BigIntvalues are safely excluded from bar rendering.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before
After
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION
CodeAnt-AI Description
Render cell bars for numeric cells even when the column contains NULLs
What Changed
Impact
✅ Cell bars appear in columns with mixed NULL and numeric values✅ Fewer rendering errors caused by non-number values (e.g., BigInt)✅ Reduced regression risk via automated test coverage💡 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.