Copilot commented on code in PR #38949:
URL: https://github.com/apache/superset/pull/38949#discussion_r3017400111
##########
superset-frontend/packages/superset-ui-core/test/time-format/TimeFormatter.test.ts:
##########
@@ -67,6 +67,12 @@ describe('TimeFormatter', () => {
test('handles number, treating it as a timestamp', () => {
expect(formatter.format(PREVIEW_TIME.getTime())).toEqual('2017');
});
+ test('handles numeric string, treating it as a timestamp', () => {
+ // PivotData.processRecord coerces values with String(), turning numeric
+ // timestamps into strings.
+ const timestamp = PREVIEW_TIME.getTime().toString();
+ expect(formatter.format(timestamp)).toEqual('2017');
+ });
Review Comment:
The new string-handling path is only tested for numeric strings. Consider
adding a test for a non-numeric date string (e.g. ISO 8601) to ensure it still
formats correctly and isn’t accidentally coerced via the numeric-detection
logic in the future.
```suggestion
});
test('handles non-numeric ISO date string', () => {
const isoString = PREVIEW_TIME.toISOString();
expect(formatter.format(isoString)).toEqual('2017');
});
```
##########
superset-frontend/packages/superset-ui-core/src/time-format/utils/stringifyTimeInput.ts:
##########
@@ -18,12 +18,17 @@
*/
export default function stringifyTimeInput(
- value: Date | number | undefined | null,
+ value: Date | number | string | undefined | null,
fn: (time: Date) => string,
) {
if (value === null || value === undefined) {
return `${value}`;
}
+ if (typeof value === 'string') {
+ const num = Number(value);
+ return fn(new Date(Number.isFinite(num) ? num : value));
Review Comment:
`Number.isFinite(Number(value))` treats some non-timestamp strings as
numeric (e.g. `''` or `' '` become `0`, and `'1e3'`/`'0x10'` are also
finite), which would unexpectedly format as epoch/other dates. If the intent is
to only re-coerce *digit-only* timestamp strings, consider using a stricter
check (e.g. trim + regex like `^-?\d+$`) before converting to `number`, and
otherwise fall back to `new Date(value)`.
```suggestion
const trimmed = value.trim();
const isIntegerString = /^-?\d+$/.test(trimmed);
if (isIntegerString) {
const num = Number(trimmed);
return fn(new Date(num));
}
return fn(new Date(value));
```
--
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]
