ryanahamilton commented on code in PR #72350:
URL: https://github.com/apache/airflow/pull/72350#discussion_r3898482830
##########
airflow-core/src/airflow/ui/src/utils/datetimeUtils.ts:
##########
@@ -139,12 +268,58 @@ export const formatDate = (
return dayjs(date).tz(timezone).format(format);
};
-export const getRelativeTime = (date: string | null | undefined): string => {
- if (date === null || date === "" || date === undefined) {
+// Ordered largest first so the first unit the difference reaches wins: "45
minutes ago" rather than
+// "2700 seconds ago". Months and years use the mean Gregorian lengths CLDR
assumes for relative
+// phrasing. Anything under a minute falls through to seconds.
+const RELATIVE_TIME_UNITS: Array<{ seconds: number; unit:
Intl.RelativeTimeFormatUnit }> = [
+ { seconds: 31_557_600, unit: "year" },
+ { seconds: 2_629_800, unit: "month" },
+ { seconds: SECONDS_PER_DAY * 7, unit: "week" },
+ { seconds: SECONDS_PER_DAY, unit: "day" },
+ { seconds: SECONDS_PER_HOUR, unit: "hour" },
+ { seconds: SECONDS_PER_MINUTE, unit: "minute" },
+ { seconds: 1, unit: "second" },
+];
+
+const RELATIVE_TIME_FALLBACK_UNIT = { seconds: 1, unit: "second" } as const;
+
+const relativeTimeFormatters = new Map<string, Intl.RelativeTimeFormat>();
+
+const getRelativeTimeFormatter = (locale: string): Intl.RelativeTimeFormat => {
+ const cached = relativeTimeFormatters.get(locale);
+
+ if (cached !== undefined) {
+ return cached;
+ }
+
+ const options: Intl.RelativeTimeFormatOptions = { numeric: "auto" };
+ let formatter: Intl.RelativeTimeFormat;
+
+ try {
+ formatter = new Intl.RelativeTimeFormat(locale, options);
+ } catch {
+ formatter = new Intl.RelativeTimeFormat(DEFAULT_LOCALE, options);
+ }
+
+ relativeTimeFormatters.set(locale, formatter);
+
+ return formatter;
+};
+
+export const getRelativeTime = (
+ date: string | null | undefined,
+ locale: string = i18n.language || DEFAULT_LOCALE,
+): string => {
+ if (date === null || date === "" || date === undefined ||
!dayjs(date).isValid()) {
return "";
}
- return dayjs(date).fromNow();
+ const elapsed = dayjs(date).diff(dayjs(), "second", true);
+ const magnitude = Math.abs(elapsed);
+ const { seconds, unit } =
+ RELATIVE_TIME_UNITS.find((candidate) => magnitude >= candidate.seconds) ??
RELATIVE_TIME_FALLBACK_UNIT;
+
+ return getRelativeTimeFormatter(locale).format(Math.round(elapsed /
seconds), unit);
Review Comment:
The unit is picked from the *unrounded* magnitude and `Math.round` is then
applied inside it, so anything that rounds up to a full next unit prints the
saturated form:
| elapsed | this PR | `dayjs().fromNow()` on main |
|---|---|---|
| −3582 s | `60 minutes ago` | `an hour ago` |
| −86000 s | `24 hours ago` | `a day ago` |
| −31557000 s | `12 months ago` | `a year ago` |
This is the same carry that `getDurationParts` handles deliberately a couple
of hundred lines up ("Rounding at a band's precision can spill into the next
band (59.96s is a minute, not '60.0s')") — it just isn't applied here.
Promoting after the rounding, the way the duration path does, would cover it.
Reaches users via the next-run tooltip in `DagRunInfo`,
`ClearTaskInstanceConfirmationDialog`, and `HITLReviewDetailSummary`.
---
Drafted-by: Claude Code (Opus 5) (no human review before posting)
##########
airflow-core/src/airflow/ui/src/utils/datetimeUtils.ts:
##########
@@ -20,6 +20,7 @@ import dayjs from "dayjs";
import dayjsDuration from "dayjs/plugin/duration";
import relativeTime from "dayjs/plugin/relativeTime";
import tz from "dayjs/plugin/timezone";
+import i18n from "i18next";
dayjs.extend(dayjsDuration);
dayjs.extend(relativeTime);
Review Comment:
`relativeTime` looks dead now — it was needed by `.fromNow()` (replaced by
`Intl.RelativeTimeFormat`) and by `duration.humanize()` (replaced by the CLDR
path), and neither is called anywhere in `src/` any more. The import on line 21
can go with it.
---
Drafted-by: Claude Code (Opus 5) (no human review before posting)
##########
airflow-core/src/airflow/ui/src/layouts/Details/Gantt/utils.ts:
##########
@@ -350,20 +350,16 @@ export type GanttAxisTick = {
};
/** Elapsed time from the chart origin (`minMs`), formatted like grid duration
labels (no wall-clock). */
-const formatElapsedMsForGanttAxis = (elapsedMs: number): string => {
+const formatElapsedMsForGanttAxis = (elapsedMs: number, locale?: string):
string => {
const seconds = Math.max(0, elapsedMs / 1000);
- if (seconds <= 0.01) {
- return "00:00:00";
- }
-
- return renderDuration(seconds, false) ?? "00:00:00";
+ return renderDuration(seconds, locale) ?? "0s";
Review Comment:
The Gantt axis places ticks at evenly-spaced *raw* values, unlike the
Chart.js axes which snap through `getDurationTickStep`. The old truncating
format masked that; the new precision exposes it on short spans:
```
7s span, 7 ticks: 0s | 1.17s | 2.33s | 3.5s | 4.67s | 5.83s | 7s
45s span, 9 ticks: 0s | 5.63s | 11.3s | 16.9s | 22.5s | 28.1s | 33.8s |
39.4s | 45s
```
Long spans read well (`0s | 30m | 1h | 1h 30m | …`). Snapping the Gantt
ticks the same way the other axes do — or rounding the label to the axis step's
own precision — would fix the short-span case.
---
Drafted-by: Claude Code (Opus 5) (no human review before posting)
##########
airflow-core/src/airflow/ui/src/utils/datetimeUtils.test.ts:
##########
@@ -23,55 +23,198 @@ import { describe, it, expect, vi, beforeAll, afterAll }
from "vitest";
import {
getDuration,
getDurationTickStep,
+ getElapsedSeconds,
humanizeSeconds,
- renderCompactDuration,
renderDuration,
getRelativeTime,
} from "./datetimeUtils";
dayjs.extend(dayjsDuration);
-describe("getDuration & formatDuration", () => {
- it("handles durations less than 60 seconds", () => {
- const start = "2024-03-14T10:00:00.000Z";
- const end = "2024-03-14T10:00:05.5111111Z";
+// CLDR's own strings shift between ICU releases — de narrow "1 Std." became
"1h" in ICU 78 — and the
+// runtime ICU differs across CI, contributor machines and browsers. So
localized cases assert the
+// composition we control (which units, what precision, which style, joined in
order) and leave the
+// wording to the platform. Only the English cases pin literals, as those
encode our band policy.
+const expectDuration = (
+ locale: string,
+ style: "long" | "narrow",
+ parts: Array<[Intl.NumberFormatOptions["unit"], number, number?]>,
+) => {
+ const formatted = parts.map(([unit, value, fractionDigits = 0]) =>
+ new Intl.NumberFormat(locale, {
+ maximumFractionDigits: fractionDigits,
+ style: "unit",
+ unit,
+ unitDisplay: style,
+ }).format(value),
+ );
- expect(getDuration(start, end)).toBe("00:00:05.511");
+ return formatted.length > 1
+ ? new Intl.ListFormat(locale, { style, type: "unit" }).format(formatted)
+ : formatted[0];
+};
+
+describe("renderDuration", () => {
+ it.each([
+ [0, "0s"],
+ [0.0000004, "<1ms"],
+ [0.0009, "<1ms"],
+ [0.001, "1ms"],
+ [0.083, "83ms"],
+ [0.9994, "999ms"],
+ // Rounding up out of the millisecond band must promote to seconds, not
print "1000ms".
+ [0.9996, "1s"],
+ [1, "1s"],
+ [1.5, "1.5s"],
+ [9.87456, "9.87s"],
+ // Three significant digits means one decimal from 10s up, two below it.
+ [14.846, "14.8s"],
+ [15, "15s"],
+ [45, "45s"],
+ [59.9, "59.9s"],
+ // Rounding at the band's precision spills into the next band.
+ [59.96, "1m"],
+ [60, "1m"],
+ [65.25, "1m 5s"],
+ [545, "9m 5s"],
+ [540, "9m"],
+ [3599.6, "1h"],
+ [3600, "1h"],
+ [3725.4, "1h 2m"],
+ [5400, "1h 30m"],
+ [86_399.6, "1d"],
+ [86_400, "1d"],
+ [90_061.2, "1d 1h"],
+ // Rounds rather than truncates: 1d 4h 30m is nearer 1d 5h.
+ [102_600, "1d 5h"],
+ [281_445, "3d 6h"],
+ ])("formats %s seconds as %s", (seconds, expected) => {
+ expect(renderDuration(seconds, "en")).toBe(expected);
});
- it("handles durations spanning multiple days", () => {
- const start = "2024-03-14T10:00:00.000Z";
- const end = "2024-03-17T15:30:45.000Z";
+ it.each([[null], [undefined], [Number.NaN], [Number.POSITIVE_INFINITY],
[-5]])(
+ "returns undefined without a usable duration (%s)",
+ (seconds) => {
+ expect(renderDuration(seconds, "en")).toBeUndefined();
+ },
+ );
- expect(getDuration(start, end)).toBe("3d05:30:45");
+ it("accepts dayjs durations as well as numbers", () => {
+ expect(renderDuration(dayjs.duration(10, "seconds"), "en")).toBe("10s");
+ expect(renderDuration(dayjs.duration(0.083, "seconds"),
"en")).toBe("83ms");
+ expect(renderDuration(dayjs.duration(3725.4, "seconds"), "en")).toBe("1h
2m");
});
- it("handles exactly 24 hours", () => {
- const start = "2024-03-14T10:00:00.000Z";
- const end = "2024-03-15T10:00:00.000Z";
+ it.each([
+ ["de", 0.083, [["millisecond", 83]]],
+ ["de", 14.846, [["second", 14.8, 1]]],
+ [
+ "de",
+ 3725.4,
+ [
+ ["hour", 1],
+ ["minute", 2],
+ ],
+ ],
+ [
+ "fr",
+ 281_445,
+ [
+ ["day", 3],
+ ["hour", 6],
+ ],
+ ],
+ [
+ "ru",
+ 3725.4,
+ [
+ ["hour", 1],
+ ["minute", 2],
+ ],
+ ],
+ [
+ "ja",
+ 65.25,
+ [
+ ["minute", 1],
+ ["second", 5],
+ ],
+ ],
+ [
+ "ar",
+ 3725.4,
+ [
+ ["hour", 1],
+ ["minute", 2],
+ ],
+ ],
+ [
+ "pl",
+ 545,
+ [
+ ["minute", 9],
+ ["second", 5],
+ ],
+ ],
+ [
+ "zh-CN",
+ 545,
+ [
+ ["minute", 9],
+ ["second", 5],
+ ],
+ ],
+ ["pt", 604_800, [["day", 7]]],
+ ["it", 604_800, [["day", 7]]],
+ ] as Array<[string, number, Array<[Intl.NumberFormatOptions["unit"], number,
number?]>]>)(
+ "localizes %s duration of %s seconds",
+ (locale, seconds, parts) => {
+ expect(renderDuration(seconds, locale)).toBe(expectDuration(locale,
"narrow", parts));
+ },
+ );
- expect(getDuration(start, end)).toBe("1d00:00:00");
+ // Properties CLDR has held stable for decades, unlike the unit
abbreviations themselves.
+ it("uses the locale's decimal separator and script", () => {
+ expect(renderDuration(14.846, "fr")).toContain("14,8");
+ expect(renderDuration(14.846, "en")).toContain("14.8");
+ expect(renderDuration(3725.4, "ru")).toMatch(/\p{Script=Cyrillic}/u);
+ expect(renderDuration(3725.4, "de")).not.toBe(renderDuration(3725.4,
"en"));
Review Comment:
This one asserts inequality between two locales' CLDR narrow forms, which is
the drift the header comment at the top of this file warns about ("de narrow '1
Std.' became '1h' in ICU 78"). It is safe today because German joins with `",
"` rather than a space, but it's the one assertion in the file that doesn't
follow the stated policy of pinning composition rather than wording.
---
Drafted-by: Claude Code (Opus 5) (no human review before posting)
##########
airflow-core/src/airflow/ui/src/layouts/Details/Gantt/GanttTimeline.tsx:
##########
@@ -148,7 +150,7 @@ export const GanttTimeline = ({
// Each "HH:MM:SS" label is ~8 chars at font-size xs; allow
MIN_TICK_SPACING_PX per tick.
const tickCount =
bodyWidthPx > 0 ? Math.max(2, Math.floor(bodyWidthPx /
MIN_TICK_SPACING_PX)) : GANTT_TIME_AXIS_TICK_COUNT;
- const timeTicks = buildGanttTimeAxisTicks(minMs, maxMs, tickCount);
+ const timeTicks = buildGanttTimeAxisTicks(minMs, maxMs, { locale, tickCount
});
Review Comment:
Two things follow from the label change here:
1. The comment three lines up still says `Each "HH:MM:SS" label is ~8 chars
at font-size xs` — no longer true.
2. `MIN_TICK_SPACING_PX = 80` was derived from that 8-char estimate. German
narrow runs ~14 chars (`"1 Std., 2 Min."`), which is roughly 84px at 12px — so
ticks can overlap in the wider locales.
---
Drafted-by: Claude Code (Opus 5) (no human review before posting)
##########
airflow-core/src/airflow/ui/src/utils/datetimeUtils.ts:
##########
@@ -29,71 +30,199 @@ export const DATE_FORMAT = "YYYY-MM-DD";
export const DEFAULT_DATETIME_FORMAT = `${DATE_FORMAT} HH:mm:ss`;
export const DEFAULT_DATETIME_FORMAT_WITH_TZ = `${DEFAULT_DATETIME_FORMAT} z`;
-export const renderDuration = (
- durationSeconds: dayjsDuration.Duration | number | null | undefined,
- withMilliseconds: boolean = true,
-): string | undefined => {
- if (durationSeconds === null || durationSeconds === undefined) {
- return undefined;
+const DEFAULT_LOCALE = "en";
+const SECONDS_PER_MINUTE = 60;
+const SECONDS_PER_HOUR = 3600;
+const SECONDS_PER_DAY = 86_400;
+
+type DurationUnit = "day" | "hour" | "millisecond" | "minute" | "second";
+
+type DurationPart = { fractionDigits?: number; unit: DurationUnit; value:
number };
+
+/** `narrow` ("1h 2m") suits dense tables and charts; `long` ("1 hour, 2
minutes") suits prose. */
+type DurationStyle = "long" | "narrow";
+
+// Durations render in every table row and chart tick callback, and Intl
formatters are costly to
+// construct, so instances are reused. A stored language Intl rejects must not
blank out every
+// duration in the UI, hence the fallback instead of letting the RangeError
escape.
+const unitFormatters = new Map<string, Intl.NumberFormat>();
+
+const getUnitFormatter = (locale: string, style: DurationStyle, part:
DurationPart): Intl.NumberFormat => {
+ const { fractionDigits = 0, unit } = part;
+ const key = `${locale}|${unit}|${fractionDigits}|${style}`;
+ const cached = unitFormatters.get(key);
+
+ if (cached !== undefined) {
+ return cached;
}
- // Handle floating point milliseconds
- const duration = dayjs.isDuration(durationSeconds)
- ? dayjs.duration(Math.round(durationSeconds.asMilliseconds()))
- : dayjs.duration(Number(durationSeconds.toFixed(3)), "seconds");
+ const options: Intl.NumberFormatOptions = {
+ maximumFractionDigits: fractionDigits,
+ style: "unit",
+ unit,
+ unitDisplay: style,
+ };
+ let formatter: Intl.NumberFormat;
- if (duration.asMilliseconds() < 1) {
- return undefined;
+ try {
+ formatter = new Intl.NumberFormat(locale, options);
+ } catch {
+ formatter = new Intl.NumberFormat(DEFAULT_LOCALE, options);
}
- // If under 60 seconds, render milliseconds
- if (duration.asSeconds() < 60 && duration.milliseconds() > 0 &&
withMilliseconds) {
- return duration.format("HH:mm:ss.SSS");
+ unitFormatters.set(key, formatter);
+
+ return formatter;
+};
+
+const listFormatters = new Map<string, Intl.ListFormat>();
+
+const getListFormatter = (locale: string, style: DurationStyle):
Intl.ListFormat => {
+ const key = `${locale}|${style}`;
+ const cached = listFormatters.get(key);
+
+ if (cached !== undefined) {
+ return cached;
}
- // If under 1 day, render as HH:mm:ss otherwise include the number of days
- return duration.asSeconds() < 86_400 ? duration.format("HH:mm:ss") :
duration.format("D[d]HH:mm:ss");
+ const options: Intl.ListFormatOptions = { style, type: "unit" };
+ let formatter: Intl.ListFormat;
+
+ try {
+ formatter = new Intl.ListFormat(locale, options);
+ } catch {
+ formatter = new Intl.ListFormat(DEFAULT_LOCALE, options);
+ }
+
+ listFormatters.set(key, formatter);
+
+ return formatter;
+};
+
+// Unit names, decimal separators, plural forms and the joiner all come from
CLDR, so "1h 2m" is
+// "1 ч 2 мин" in ru. This reproduces Intl.DurationFormat's narrow style
exactly (verified across
+// every locale we ship) without requiring it: that API needs Node 23+, above
this package's
+// engines floor, and Node 23 was never an LTS line. Exact wording also varies
by the runtime's ICU
+// version, so nothing may depend on a specific CLDR string.
+const formatParts = (parts: Array<DurationPart>, locale: string, style:
DurationStyle): string => {
+ const formatted = parts.map((part) => getUnitFormatter(locale, style,
part).format(part.value));
+
+ return formatted.length > 1 ? getListFormatter(locale,
style).format(formatted) : (formatted[0] ?? "");
};
-// dayjs humanizes a missing or non-finite input as "a few seconds", so
callers with no duration
-// to name get undefined instead of a made-up one.
-export const humanizeSeconds = (seconds: number | null | undefined): string |
undefined =>
- typeof seconds === "number" && Number.isFinite(seconds)
- ? dayjs.duration(seconds, "seconds").humanize()
- : undefined;
+// Durations carry roughly three significant digits at every magnitude, so a
83ms task and a
+// three-day backfill are both legible without decoding zero-padded clock
groups. Rounding at a
+// band's precision can spill into the next band (59.96s is a minute, not
"60.0s"), hence the
+// recursion on the promoted value. Callers needing the unrounded number
should surface it separately.
+const getDurationParts = (seconds: number): Array<DurationPart> => {
Review Comment:
Not a defect, but worth a decision: three significant digits means `1h 2m`
covers 3690–3750s and `1d 5h` covers a ±30-minute band. The list "Duration"
columns are often used to compare two similar runs against each other, and that
comparison gets lossier than the old `01:02:05`.
The comment below says callers needing the unrounded number should surface
it separately — but no caller currently does. A `title` attribute carrying the
exact value on the `DagRuns`/`TaskInstances` duration cells would keep the
scannability win without giving up the precision.
---
Drafted-by: Claude Code (Opus 5) (no human review before posting)
##########
airflow-core/src/airflow/ui/src/components/SlowestTaskInstancesChart.tsx:
##########
@@ -89,7 +86,7 @@ export const SlowestTaskInstancesChart = ({
const duration = durations[index];
if (duration !== undefined) {
- ctx.fillText(renderCompactDuration(duration), bar.x + 6, bar.y);
+ ctx.fillText(renderDuration(duration) ?? "0s", bar.x + 6, bar.y);
Review Comment:
`layout: { padding: { right: 64 } }` (line 122) was sized when
`renderCompactDuration` always emitted English `"1h 2m"` — about 30px at this
11px label font. CLDR narrow is considerably wider in several shipped locales:
```
de "1 Std., 2 Min." 14 chars, ~80px
hu "1 ó és 2 p" 11 chars
pl "1 h i 2 min" 11 chars
pt "3 dias 6 h" 10 chars
```
At `bar.x + 6` the longest bar's label will run past the 64px reserve in
German. Deriving the padding from `ctx.measureText` on the widest label would
keep it safe across locales.
---
Drafted-by: Claude Code (Opus 5) (no human review before posting)
##########
airflow-core/src/airflow/ui/src/utils/useDurationFormat.ts:
##########
@@ -0,0 +1,55 @@
+/*!
+ * 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 type dayjsDuration from "dayjs/plugin/duration";
+import { useMemo } from "react";
+import { useTranslation } from "react-i18next";
+
+import { getDuration, humanizeSeconds, renderDuration } from "./datetimeUtils";
+
+/**
+ * Duration formatters bound to the language currently on screen.
+ *
+ * The plain formatters read the language from the i18next singleton, which
React does not track, so
+ * a component that shows a duration but never subscribes to `languageChanged`
keeps rendering the
+ * previous locale. Reading the language through `useTranslation` here makes
it an ordinary render
+ * input: switching language re-renders every consumer, and `locale` can be
added to a caller's memo
+ * dependencies so derived columns, chart options and tick callbacks rebuild
with it.
+ *
+ * Components should always format durations through this hook. Reach for the
raw functions in
+ * `datetimeUtils` only outside React, and pass a locale explicitly there.
+ */
+export const useDurationFormat = () => {
+ const { i18n } = useTranslation();
+ const locale = i18n.language;
+
+ return useMemo(
+ () => ({
+ /** Elapsed time between two timestamps, counting an absent end as still
running. */
+ formatElapsed: (startDate?: string | null, endDate?: string | null) =>
+ getDuration(startDate, endDate, locale),
+ /** Spelled-out duration for prose, e.g. "1 hour, 2 minutes". */
+ humanizeDuration: (seconds: number | null | undefined) =>
humanizeSeconds(seconds, locale),
+ locale,
Review Comment:
`getRelativeTime` also gained a `locale` parameter in this PR, but it isn't
exposed here, and none of its three call sites (`DagRunInfo.tsx:47`,
`ClearTaskInstanceConfirmationDialog.tsx:127`,
`HITLReviewDetailSummary.tsx:66`) pass one — so it still reads the i18next
singleton, which is the exact failure this hook's doc comment describes. It
happens to work today only because all three components call `useTranslation`
for unrelated reasons, which is a fragile thing to depend on.
Adding a `formatRelative` member here would close it the same way the
duration formatters were closed.
---
Drafted-by: Claude Code (Opus 5) (no human review before posting)
##########
airflow-core/src/airflow/ui/src/utils/datetimeUtils.ts:
##########
@@ -29,71 +30,199 @@ export const DATE_FORMAT = "YYYY-MM-DD";
export const DEFAULT_DATETIME_FORMAT = `${DATE_FORMAT} HH:mm:ss`;
export const DEFAULT_DATETIME_FORMAT_WITH_TZ = `${DEFAULT_DATETIME_FORMAT} z`;
-export const renderDuration = (
- durationSeconds: dayjsDuration.Duration | number | null | undefined,
- withMilliseconds: boolean = true,
-): string | undefined => {
- if (durationSeconds === null || durationSeconds === undefined) {
- return undefined;
+const DEFAULT_LOCALE = "en";
+const SECONDS_PER_MINUTE = 60;
+const SECONDS_PER_HOUR = 3600;
+const SECONDS_PER_DAY = 86_400;
+
+type DurationUnit = "day" | "hour" | "millisecond" | "minute" | "second";
+
+type DurationPart = { fractionDigits?: number; unit: DurationUnit; value:
number };
+
+/** `narrow` ("1h 2m") suits dense tables and charts; `long` ("1 hour, 2
minutes") suits prose. */
+type DurationStyle = "long" | "narrow";
+
+// Durations render in every table row and chart tick callback, and Intl
formatters are costly to
+// construct, so instances are reused. A stored language Intl rejects must not
blank out every
+// duration in the UI, hence the fallback instead of letting the RangeError
escape.
+const unitFormatters = new Map<string, Intl.NumberFormat>();
+
+const getUnitFormatter = (locale: string, style: DurationStyle, part:
DurationPart): Intl.NumberFormat => {
+ const { fractionDigits = 0, unit } = part;
+ const key = `${locale}|${unit}|${fractionDigits}|${style}`;
+ const cached = unitFormatters.get(key);
+
+ if (cached !== undefined) {
+ return cached;
}
- // Handle floating point milliseconds
- const duration = dayjs.isDuration(durationSeconds)
- ? dayjs.duration(Math.round(durationSeconds.asMilliseconds()))
- : dayjs.duration(Number(durationSeconds.toFixed(3)), "seconds");
+ const options: Intl.NumberFormatOptions = {
+ maximumFractionDigits: fractionDigits,
+ style: "unit",
+ unit,
+ unitDisplay: style,
+ };
+ let formatter: Intl.NumberFormat;
- if (duration.asMilliseconds() < 1) {
- return undefined;
+ try {
+ formatter = new Intl.NumberFormat(locale, options);
+ } catch {
+ formatter = new Intl.NumberFormat(DEFAULT_LOCALE, options);
}
- // If under 60 seconds, render milliseconds
- if (duration.asSeconds() < 60 && duration.milliseconds() > 0 &&
withMilliseconds) {
- return duration.format("HH:mm:ss.SSS");
+ unitFormatters.set(key, formatter);
+
+ return formatter;
+};
+
+const listFormatters = new Map<string, Intl.ListFormat>();
+
+const getListFormatter = (locale: string, style: DurationStyle):
Intl.ListFormat => {
+ const key = `${locale}|${style}`;
+ const cached = listFormatters.get(key);
+
+ if (cached !== undefined) {
+ return cached;
}
- // If under 1 day, render as HH:mm:ss otherwise include the number of days
- return duration.asSeconds() < 86_400 ? duration.format("HH:mm:ss") :
duration.format("D[d]HH:mm:ss");
+ const options: Intl.ListFormatOptions = { style, type: "unit" };
+ let formatter: Intl.ListFormat;
+
+ try {
+ formatter = new Intl.ListFormat(locale, options);
+ } catch {
+ formatter = new Intl.ListFormat(DEFAULT_LOCALE, options);
+ }
+
+ listFormatters.set(key, formatter);
+
+ return formatter;
+};
+
+// Unit names, decimal separators, plural forms and the joiner all come from
CLDR, so "1h 2m" is
+// "1 ч 2 мин" in ru. This reproduces Intl.DurationFormat's narrow style
exactly (verified across
+// every locale we ship) without requiring it: that API needs Node 23+, above
this package's
+// engines floor, and Node 23 was never an LTS line. Exact wording also varies
by the runtime's ICU
+// version, so nothing may depend on a specific CLDR string.
+const formatParts = (parts: Array<DurationPart>, locale: string, style:
DurationStyle): string => {
+ const formatted = parts.map((part) => getUnitFormatter(locale, style,
part).format(part.value));
+
+ return formatted.length > 1 ? getListFormatter(locale,
style).format(formatted) : (formatted[0] ?? "");
};
-// dayjs humanizes a missing or non-finite input as "a few seconds", so
callers with no duration
-// to name get undefined instead of a made-up one.
-export const humanizeSeconds = (seconds: number | null | undefined): string |
undefined =>
- typeof seconds === "number" && Number.isFinite(seconds)
- ? dayjs.duration(seconds, "seconds").humanize()
- : undefined;
+// Durations carry roughly three significant digits at every magnitude, so a
83ms task and a
+// three-day backfill are both legible without decoding zero-padded clock
groups. Rounding at a
+// band's precision can spill into the next band (59.96s is a minute, not
"60.0s"), hence the
+// recursion on the promoted value. Callers needing the unrounded number
should surface it separately.
+const getDurationParts = (seconds: number): Array<DurationPart> => {
+ if (seconds === 0) {
+ return [{ unit: "second", value: 0 }];
+ }
+
+ if (seconds < 1) {
+ const milliseconds = Math.round(seconds * 1000);
+
+ return milliseconds < 1000 ? [{ unit: "millisecond", value: milliseconds
}] : getDurationParts(1);
+ }
+
+ if (seconds < SECONDS_PER_MINUTE) {
+ // Two decimals under 10s, one above, keeps three significant digits
either way.
+ const fractionDigits = seconds < 10 ? 2 : 1;
+ const rounded = Number(seconds.toFixed(fractionDigits));
+
+ return rounded < SECONDS_PER_MINUTE
+ ? [{ fractionDigits, unit: "second", value: rounded }]
+ : getDurationParts(SECONDS_PER_MINUTE);
+ }
+
+ if (seconds < SECONDS_PER_HOUR) {
+ const minutes = Math.floor(seconds / SECONDS_PER_MINUTE);
+ const remainingSeconds = Math.round(seconds - minutes *
SECONDS_PER_MINUTE);
+
+ if (remainingSeconds === SECONDS_PER_MINUTE) {
+ return getDurationParts((minutes + 1) * SECONDS_PER_MINUTE);
+ }
-// Chart axes need whole units at a glance; HH:mm:ss forces the reader to
decode
-// every tick to work out the magnitude.
-export const renderCompactDuration = (durationSeconds: number): string => {
- if (!Number.isFinite(durationSeconds) || durationSeconds <= 0) {
- return "0s";
+ return remainingSeconds > 0
+ ? [
+ { unit: "minute", value: minutes },
+ { unit: "second", value: remainingSeconds },
+ ]
+ : [{ unit: "minute", value: minutes }];
}
- if (durationSeconds < 1) {
- return `${Math.round(durationSeconds * 1000)}ms`;
+ if (seconds < SECONDS_PER_DAY) {
+ const hours = Math.floor(seconds / SECONDS_PER_HOUR);
+ const remainingMinutes = Math.round((seconds - hours * SECONDS_PER_HOUR) /
SECONDS_PER_MINUTE);
+
+ if (remainingMinutes === SECONDS_PER_MINUTE) {
Review Comment:
Nit: `remainingMinutes === SECONDS_PER_MINUTE` is numerically right but
reads wrong — this is 60 minutes *per hour*, not seconds per minute. Line 173
then drops to a bare `24` where the rest of the function uses named constants.
A `MINUTES_PER_HOUR` / `HOURS_PER_DAY` pair would make all three carry checks
read consistently.
---
Drafted-by: Claude Code (Opus 5) (no human review before posting)
##########
airflow-core/src/airflow/ui/src/layouts/Details/Gantt/utils.test.ts:
##########
@@ -40,22 +40,22 @@ describe("buildGanttTimeAxisTicks", () => {
expect(ticks).toHaveLength(GANTT_TIME_AXIS_TICK_COUNT);
expect(ticks[0]?.leftPct).toBe(0);
- expect(ticks[0]?.label).toBe("00:00:00");
+ expect(ticks[0]?.label).toBe("0s");
expect(ticks[0]?.labelAlign).toBe("left");
expect(ticks[GANTT_TIME_AXIS_TICK_COUNT - 1]?.leftPct).toBe(100);
expect(ticks[GANTT_TIME_AXIS_TICK_COUNT - 1]?.labelAlign).toBe("right");
- expect(ticks[GANTT_TIME_AXIS_TICK_COUNT - 1]?.label).toBe("00:01:00");
+ expect(ticks[GANTT_TIME_AXIS_TICK_COUNT - 1]?.label).toBe("1m");
expect(ticks[1]?.labelAlign).toBe("center");
expect(ticks.every((tick) => typeof tick.label === "string" &&
tick.label.length > 0)).toBe(true);
});
it("supports a single tick", () => {
- const ticks = buildGanttTimeAxisTicks(1000, 1000, 1);
+ const ticks = buildGanttTimeAxisTicks(1000, 1000, { tickCount: 1 });
Review Comment:
The signature change to an options object is covered for `tickCount`, but
the new `locale` option isn't exercised anywhere — a case passing a non-`en`
locale and asserting the tick label picks it up would cover the path
`GanttTimeline` actually uses.
---
Drafted-by: Claude Code (Opus 5) (no human review before posting)
--
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]
