-
Notifications
You must be signed in to change notification settings - Fork 16.6k
fix(charts): normalize server_page_length from string to number #37517
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
fix(charts): normalize server_page_length from string to number #37517
Conversation
Fixes backend type comparison error where SelectControl with freeForm stores values as strings. Added normalization in exploreReducer for server_page_length control.
Code Review Agent Run #7c4137Actionable 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 |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
/review |
Code Review Agent Run #95cc7dActionable 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 |
Code Review Agent Run #f27707Actionable 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes a backend type comparison error that occurs when using custom server page length values in Table charts. The issue arises because the SelectControl component with freeForm: true stores user-typed values as strings, but the Python backend expects numeric values for comparison operations.
Changes:
- Adds type normalization in the
SET_FIELD_VALUEaction handler to convertserver_page_lengthfrom string to number - Implements targeted normalization that only affects
server_page_length, preserving existing behavior for other controls - Adds comprehensive unit tests to verify the normalization behavior
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| superset-frontend/src/explore/reducers/exploreReducer.js | Adds normalization logic to convert server_page_length from string to number when the value is set, while preserving invalid strings for validation |
| superset-frontend/src/explore/reducers/exploreReducer.test.js | Adds test cases covering string-to-number conversion, numeric value preservation, non-normalization of other controls, and edge case handling for '0' |
| // Normalize server_page_length to number if it's a string | ||
| const normalizedValue = | ||
| controlName === 'server_page_length' && typeof value === 'string' |
Copilot
AI
Jan 28, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The normalization logic converts an empty string to the number 0, which may not be the intended behavior. Consider whether an empty string should be preserved for validation or converted to 0. If empty strings should remain for validation to catch, the logic should check for empty strings before conversion:
controlName === 'server_page_length' && typeof value === 'string' && value.trim() !== '' ? (isNaN(Number(value)) ? value : Number(value)) : value
This would align with the validateInteger function which checks v.trim().length > 0 before validation.
| // Normalize server_page_length to number if it's a string | |
| const normalizedValue = | |
| controlName === 'server_page_length' && typeof value === 'string' | |
| // Normalize server_page_length to number if it's a non-empty string | |
| const normalizedValue = | |
| controlName === 'server_page_length' && | |
| typeof value === 'string' && | |
| value.trim() !== '' |
Code Review Agent Run #fc04b0Actionable 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 |
fix(charts): normalize server_page_length from string to number
SUMMARY
Fixes a backend type comparison error (
'<' not supported between instances of 'str' and 'int') that occurs when using custom server page length values in Table charts.The
SelectControlcomponent withfreeForm: truestores user-typed values as strings. Whenserver_page_lengthis manually entered (e.g., "99"), it's saved as a string but the Python backend expects a number for comparison operations.This PR adds type normalization in
exploreReducer.jsto convertserver_page_lengthfrom string to number when theSET_FIELD_VALUEaction is dispatched. The fix is targeted - onlyserver_page_lengthis normalized, as other controls likerow_limitdon't exhibit this backend issue.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before:
TypeError: '<' not supported between instances of 'str' and 'int'After:
TESTING INSTRUCTIONS
cd superset-frontend && npm test -- exploreReducer.test.jsADDITIONAL INFORMATION