Copilot commented on code in PR #39330: URL: https://github.com/apache/superset/pull/39330#discussion_r3190483827
########## superset-frontend/packages/superset-ui-core/src/number-format/utils/getIntlDurationFormatter.ts: ########## @@ -0,0 +1,30 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +export function getIntlDurationFormatter( + locale?: string, + options?: Intl.DurationFormatOptions, +): Intl.DurationFormat { + const normalizedLocale = locale?.replace('_', '-'); Review Comment: Locale normalization only replaces the first underscore. Locales with multiple underscores (e.g. `zh_Hans_CN`) will become `zh-Hans_CN`, which can still be invalid and unnecessarily trigger the fallback. Use a global replacement (e.g. `/_/g`) or `replaceAll('_', '-')` to normalize the full locale string. ########## superset-frontend/packages/superset-ui-core/src/number-format/factories/createDurationFormatter.ts: ########## @@ -17,23 +17,58 @@ * under the License. */ -import prettyMilliseconds, { Options } from 'pretty-ms'; import NumberFormatter from '../NumberFormatter'; +import { getIntlDurationFormatter } from '../utils/getIntlDurationFormatter'; +import { parseMilliseconds } from '../utils/parseMilliseconds'; export default function createDurationFormatter( config: { description?: string; id?: string; label?: string; multiplier?: number; - } & Options = {}, + locale?: string; + formatSubMilliseconds?: boolean; + } & Intl.DurationFormatOptions = {}, ) { - const { description, id, label, multiplier = 1, ...prettyMsOptions } = config; - + const { + description, + id, + label, + multiplier = 1, + locale, + formatSubMilliseconds = false, + ...intlOptions + } = config; + const durationFormatter = getIntlDurationFormatter(locale, { + secondsDisplay: 'auto', + style: 'narrow', + ...intlOptions, + }); + const zeroDurationFormatter = getIntlDurationFormatter(locale, { + secondsDisplay: 'always', + style: 'narrow', + ...intlOptions, + }); return new NumberFormatter({ description, - formatFunc: value => - prettyMilliseconds(value * multiplier, prettyMsOptions), + formatFunc: value => { + const durObject = parseMilliseconds(value * multiplier); + + if (!formatSubMilliseconds) { + durObject.milliseconds = 0; + durObject.microseconds = 0; + durObject.nanoseconds = 0; + } + + const isAllUnitsZero = Object.values(durObject).every( + value => value === 0, + ); + + return ( + isAllUnitsZero ? zeroDurationFormatter : durationFormatter + ).format(durObject); + }, Review Comment: Negative durations are likely broken here. `parse-ms` (commonly) parses `Math.abs(ms)` and does not preserve sign, but the formatter tests still expect outputs like `-1s`. Consider explicitly handling sign: parse `Math.abs(value * multiplier)`, then apply the sign to the resulting duration object (all units must share the same sign for `Intl.DurationFormat`) or prefix the formatted output accordingly. ########## superset-frontend/packages/superset-ui-core/src/number-format/factories/createDurationFormatter.ts: ########## @@ -17,23 +17,58 @@ * under the License. */ -import prettyMilliseconds, { Options } from 'pretty-ms'; import NumberFormatter from '../NumberFormatter'; +import { getIntlDurationFormatter } from '../utils/getIntlDurationFormatter'; +import { parseMilliseconds } from '../utils/parseMilliseconds'; export default function createDurationFormatter( config: { description?: string; id?: string; label?: string; multiplier?: number; - } & Options = {}, + locale?: string; + formatSubMilliseconds?: boolean; + } & Intl.DurationFormatOptions = {}, ) { - const { description, id, label, multiplier = 1, ...prettyMsOptions } = config; - + const { + description, + id, + label, + multiplier = 1, + locale, + formatSubMilliseconds = false, + ...intlOptions + } = config; + const durationFormatter = getIntlDurationFormatter(locale, { + secondsDisplay: 'auto', + style: 'narrow', + ...intlOptions, + }); + const zeroDurationFormatter = getIntlDurationFormatter(locale, { + secondsDisplay: 'always', + style: 'narrow', + ...intlOptions, + }); return new NumberFormatter({ description, - formatFunc: value => - prettyMilliseconds(value * multiplier, prettyMsOptions), + formatFunc: value => { + const durObject = parseMilliseconds(value * multiplier); + + if (!formatSubMilliseconds) { + durObject.milliseconds = 0; Review Comment: This changes default behavior compared to `pretty-ms`: values like `10500` ms or `0.5` seconds (with `multiplier: 1000`) now become `10s` / `0s` instead of `10.5s` / `500ms` (sub-second precision is dropped unless `formatSubMilliseconds` is enabled). If the goal is to be a drop-in replacement for existing duration formats, consider preserving fractional seconds by default (or introducing a clearer option such as `rounding`/`fractionalDigits` defaults) and only truncating when explicitly requested. ########## superset-frontend/src/features/tasks/timeUtils.ts: ########## @@ -29,20 +30,41 @@ const MAX_ETA_SECONDS = 86400; * Format a duration in seconds to a human-readable string. * * @param seconds - Duration in seconds + * @param locale - Current locale * @returns Formatted string like "1m 30s" or "2h 15m", or null if invalid */ export function formatDuration( seconds: number | null | undefined, + locale?: string, ): string | null { if (seconds === null || seconds === undefined || seconds <= 0) { return null; } - return prettyMs(seconds * 1000, { - unitCount: 2, - secondsDecimalDigits: 0, - keepDecimalsOnWholeSeconds: false, - }); + const durObject = parseMilliseconds(seconds * 1000); + const unitOrder = [ + 'years', + 'days', + 'hours', + 'minutes', + 'seconds', + 'milliseconds', + 'microseconds', + 'nanoseconds', + ] as const; + const nonZeroUnits = unitOrder + .filter(unit => durObject[unit] > 0) + .slice(0, 2) + .reduce( + (obj, unit) => { + obj[unit] = durObject[unit]; + return obj; + }, + {} as Record<string, number>, + ); + return getIntlDurationFormatter(locale, { style: 'narrow' }).format( + nonZeroUnits, + ); Review Comment: `getIntlDurationFormatter()` creates a new `Intl.DurationFormat` instance on every call to `formatDuration`, which can be hot (task lists/tables/tooltip rendering). Consider memoizing/caching formatters by `(locale, options)` at module scope, or reusing the registry-based `createDurationFormatter` approach so the formatter instance is constructed once. ########## superset-frontend/src/features/tasks/TaskStatusIcon.tsx: ########## @@ -86,6 +88,9 @@ export default function TaskStatusIcon({ exceptionType, }: TaskStatusIconProps) { const theme = useTheme(); + const locale = useSelector( + (state: ExplorePageState) => state?.common?.locale, + ); Review Comment: Same issue as TaskList: typing the selector state as `ExplorePageState` in a shared tasks component couples this component to a specific page state model. Use the global/root Redux state type and select locale from the appropriate slice to keep the component reusable and correctly typed. ########## superset-frontend/packages/superset-ui-core/test/number-format/utils/getIntlDurationFormatter.test.ts: ########## @@ -0,0 +1,32 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { getIntlDurationFormatter } from '@superset-ui/core/number-format/utils//getIntlDurationFormatter'; Review Comment: The import path contains a double slash (`utils//...`). While it may resolve in some setups, it’s easy to break tooling (linters, path normalizers, jest module maps). Update it to a single slash for consistency and robustness. ########## superset-frontend/src/pages/TaskList/index.tsx: ########## @@ -77,6 +79,9 @@ interface TaskListProps { function TaskList({ addDangerToast, addSuccessToast, user }: TaskListProps) { const theme = useTheme(); + const locale = useSelector( + (state: ExplorePageState) => state?.common?.locale, + ); Review Comment: `useSelector` is typed with `ExplorePageState`, which is a page-specific state type and is unlikely to represent the global Redux state used on the Task List page. This can hide type errors and makes the selector contract unclear. Prefer the app's root store state type (typically `RootState`/`SupersetRootState`) and select the locale from the correct slice. -- 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]
