fix(charts): Table chart shows an error on row limit#37218
fix(charts): Table chart shows an error on row limit#37218sadpandajoe merged 6 commits intoapache:masterfrom
Conversation
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Code Review Agent Run #5f2cf0Actionable Suggestions - 0Additional Suggestions - 1
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 |
| validator: (v: unknown, state?: any) => string | false, | ||
| label: string, | ||
| ) { | ||
| return (v: unknown, state?: any) => { |
There was a problem hiding this comment.
Types are a bit shallow. Can we improve?
|
Yes, the types can be improved by making the function generic over the value type to preserve type safety. This avoids using 'unknown' loosely and ensures the validator and returned function share the same input type. superset-frontend/packages/superset-ui-core/src/utils/withLabel.ts |
superset-frontend/packages/superset-ui-core/test/validator/validateMaxValue.test.ts
Show resolved
Hide resolved
superset-frontend/plugins/plugin-chart-echarts/src/Histogram/controlPanel.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Code Review Agent Run #0ea597
Actionable Suggestions - 1
-
superset-frontend/packages/superset-ui-core/src/validator/validateTimeComparisonRangeValues.ts - 1
- Logic Bug in Null Handling · Line 29-31
Review Details
-
Files reviewed - 17 · Commit Range:
1c5418e..84c1224- superset-frontend/packages/superset-ui-core/src/utils/index.ts
- superset-frontend/packages/superset-ui-core/src/utils/withLabel.ts
- superset-frontend/packages/superset-ui-core/src/validator/index.ts
- superset-frontend/packages/superset-ui-core/src/validator/legacyValidateInteger.ts
- superset-frontend/packages/superset-ui-core/src/validator/legacyValidateNumber.ts
- superset-frontend/packages/superset-ui-core/src/validator/types.ts
- superset-frontend/packages/superset-ui-core/src/validator/validateInteger.ts
- superset-frontend/packages/superset-ui-core/src/validator/validateMapboxStylesUrl.ts
- superset-frontend/packages/superset-ui-core/src/validator/validateMaxValue.ts
- superset-frontend/packages/superset-ui-core/src/validator/validateNonEmpty.ts
- superset-frontend/packages/superset-ui-core/src/validator/validateNumber.ts
- superset-frontend/packages/superset-ui-core/src/validator/validateServerPagination.ts
- superset-frontend/packages/superset-ui-core/src/validator/validateTimeComparisonRangeValues.ts
- superset-frontend/packages/superset-ui-core/test/validator/validateMaxValue.test.ts
- superset-frontend/plugins/plugin-chart-ag-grid-table/src/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/Histogram/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-table/src/controlPanel.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 evan@preset.io.
Documentation & Help
superset-frontend/packages/superset-ui-core/src/validator/validateTimeComparisonRangeValues.ts
Show resolved
Hide resolved
| clearable: false, | ||
| mapStateToProps: state => ({ | ||
| mapStateToProps: (state, controlState) => ({ | ||
| value: controlState?.value ?? 10000, |
There was a problem hiding this comment.
Config has a default field you can use. Wouldn't that be enough?
There was a problem hiding this comment.
The default value is 10000, but the issue persists even after changing it.
There was a problem hiding this comment.
Then do you think the issue might be in the control or the component?
There was a problem hiding this comment.
Let me revise the control again.
There was a problem hiding this comment.
I found this issue here with undefined & null. Also, there was another default prop outside the config in a json called override that in this particular case didn't have any effect. I deleted it.
| if (state.default != null && value == null) { | ||
| value = state.default; | ||
| } | ||
| // If a choice control went from multi=false to true, wrap value in array |
There was a problem hiding this comment.
Suggestion: Reinventing and duplicating array-wrapping logic: the code manually checks and wraps values for multi controls instead of using the existing ensureIsArray helper imported at the top; this can miss edge cases and is less clear. Replace the manual wrapping with ensureIsArray to consistently normalize to array when required. [code duplication]
Severity Level: Major ⚠️
- ❌ Control normalization bugs for multi-choice controls.
- ⚠️ Inconsistent value shapes across control initialization.
- ⚠️ Tests relying on array normalization may fail intermittently.
Code Review Agent Run #748e5aActionable 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 |
| // If no current value, set it as default | ||
| if (state.default && value === undefined) { | ||
| // Use loose equality to catch both null and undefined | ||
| if (state.default != null && value == null) { |
There was a problem hiding this comment.
This looks much cleaner, great work!
Test Summary
All above scenarios working as expected - QA Approved |
|
Testing done. QA approved. |
SUMMARY
The Table chart's "Row limit" field had two problems:
Solution:
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before:
Before.mov
After:
After.mov
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION