Copilot commented on code in PR #37517:
URL: https://github.com/apache/superset/pull/37517#discussion_r2737949413
##########
superset-frontend/src/explore/reducers/exploreReducer.js:
##########
@@ -124,7 +124,12 @@ export default function exploreReducer(state = {}, action)
{
},
[actions.SET_FIELD_VALUE]() {
const { controlName, value, validationErrors } = action;
- let new_form_data = { ...state.form_data, [controlName]: value };
+ // Normalize server_page_length to number if it's a string
+ const normalizedValue =
+ controlName === 'server_page_length' && typeof value === 'string'
Review Comment:
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.
```suggestion
// Normalize server_page_length to number if it's a non-empty string
const normalizedValue =
controlName === 'server_page_length' &&
typeof value === 'string' &&
value.trim() !== ''
```
##########
superset-frontend/src/explore/reducers/exploreReducer.test.js:
##########
@@ -43,3 +43,53 @@ test('skips updates when the field is already updated on
SET_STASH_FORM_DATA', (
const newState = exploreReducer(initialState, restoreAction);
expect(newState).toBe(initialState);
});
+
+test('normalizes server_page_length from string to number on SET_FIELD_VALUE',
() => {
+ const initialState = {
+ form_data: { viz_type: 'table' },
+ controls: {},
+ };
+
+ const action = setControlValue('server_page_length', '100');
+ const newState = exploreReducer(initialState, action);
+
+ expect(newState.form_data.server_page_length).toBe(100);
+ expect(typeof newState.form_data.server_page_length).toBe('number');
+});
+
+test('preserves server_page_length when already numeric', () => {
+ const initialState = {
+ form_data: { viz_type: 'table' },
+ controls: {},
+ };
+
+ const action = setControlValue('server_page_length', 100);
+ const newState = exploreReducer(initialState, action);
+
+ expect(newState.form_data.server_page_length).toBe(100);
+});
+
+test('does not normalize other string control values', () => {
+ const initialState = {
+ form_data: { viz_type: 'table' },
+ controls: {},
+ };
+
+ const action = setControlValue('row_limit', '50');
+ const newState = exploreReducer(initialState, action);
+
+ expect(newState.form_data.row_limit).toBe('50');
+});
+
+test('normalizes server_page_length from string "0" to number 0', () => {
+ const initialState = {
+ form_data: { viz_type: 'table' },
+ controls: {},
+ };
+
+ const action = setControlValue('server_page_length', '0');
+ const newState = exploreReducer(initialState, action);
+
+ expect(newState.form_data.server_page_length).toBe(0);
+ expect(typeof newState.form_data.server_page_length).toBe('number');
+});
Review Comment:
Consider adding test cases for additional edge cases:
1. Empty string input (e.g., `''`) - currently would convert to 0
2. Whitespace-only input (e.g., `' '`) - currently would convert to 0
3. Invalid string input (e.g., `'abc'`) - to document that it's preserved
for validation
4. Decimal string input (e.g., `'10.5'`) - to verify validator catches it
These tests would provide better documentation of the normalization behavior
and catch potential regressions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
