Copilot commented on code in PR #64335:
URL: https://github.com/apache/airflow/pull/64335#discussion_r3025334530
##########
airflow-core/src/airflow/ui/src/layouts/Details/DetailsLayout.tsx:
##########
@@ -77,7 +103,10 @@ export const DetailsLayout = ({ children, error, isLoading,
tabs }: Props) => {
const { data: dag } = useDagServiceGetDag({ dagId });
const [defaultDagView] = useLocalStorage<"graph" |
"grid">(DEFAULT_DAG_VIEW_KEY, "grid");
const panelGroupRef = useRef<ImperativePanelGroupHandle | null>(null);
- const [dagView, setDagView] = useLocalStorage<"graph" |
"grid">(dagViewKey(dagId), defaultDagView);
+ const [dagView, setDagView] = useLocalStorage<"gantt" | "graph" | "grid">(
+ dagViewKey(dagId),
+ defaultDagView,
+ );
const [limit, setLimit] = useLocalStorage<number>(dagRunsLimitKey(dagId),
10);
Review Comment:
`dagView` can be persisted as "gantt" even when the route has no `runId`. In
that situation the layout renders the Grid-only branch, but state still says
"gantt" (and the view toggle may not have a matching option). Consider adding a
small effect to coerce `dagView` back to "grid" (or the default view) when
`dagView === "gantt" && !runId` so the UI state always matches what is actually
rendered.
##########
airflow-core/src/airflow/ui/src/layouts/Details/Gantt/utils.ts:
##########
@@ -144,246 +135,138 @@ export const transformGanttData = ({
.filter((item): item is GanttDataItem => item !== undefined);
};
-export const createHandleBarClick =
- ({ dagId, data, location, navigate, runId }: HandleBarClickOptions) =>
- (_: ChartEvent, elements: Array<ActiveElement>) => {
- if (elements.length === 0 || !elements[0] || !runId) {
- return;
- }
-
- const clickedData = data[elements[0].index];
+/** One entry per flat node: segments to draw in that row (tries or
aggregate). */
+export const buildGanttRowSegments = (
+ flatNodes: Array<GridTask>,
+ items: Array<GanttDataItem>,
+): Array<Array<GanttDataItem>> => {
+ const byTaskId = new Map<string, Array<GanttDataItem>>();
- if (!clickedData) {
- return;
- }
+ for (const item of items) {
+ const list = byTaskId.get(item.taskId) ?? [];
- const { isGroup, isMapped, taskId, tryNumber } = clickedData;
-
- const taskUrl = buildTaskInstanceUrl({
- currentPathname: location.pathname,
- dagId,
- isGroup: Boolean(isGroup),
- isMapped: Boolean(isMapped),
- runId,
- taskId,
- });
-
- const searchParams = new URLSearchParams(location.search);
- const isOlderTry =
- tryNumber !== undefined &&
- tryNumber <
- Math.max(...data.filter((item) => item.taskId === taskId).map((item)
=> item.tryNumber ?? 1));
-
- if (isOlderTry) {
- searchParams.set(SearchParamsKeys.TRY_NUMBER, tryNumber.toString());
- } else {
- searchParams.delete(SearchParamsKeys.TRY_NUMBER);
- }
-
- void Promise.resolve(
- navigate(
- {
- pathname: taskUrl,
- search: searchParams.toString(),
- },
- { replace: true },
- ),
- );
- };
+ list.push(item);
+ byTaskId.set(item.taskId, list);
+ }
-export const createHandleBarHover = (
- data: Array<GanttDataItem>,
- setHoveredTaskId: (taskId: string | undefined) => void,
-) => {
- let lastHoveredTaskId: string | undefined = undefined;
-
- return (_: ChartEvent, elements: Array<ActiveElement>) => {
- // Clear previous hover styles
- if (lastHoveredTaskId !== undefined) {
- const previousTasks = document.querySelectorAll<HTMLDivElement>(
- `#${lastHoveredTaskId.replaceAll(".", "-")}`,
- );
-
- previousTasks.forEach((task) => {
- task.style.backgroundColor = "";
- });
- }
-
- if (elements.length > 0 && elements[0] && elements[0].index < data.length)
{
- const hoveredData = data[elements[0].index];
-
- if (hoveredData?.taskId !== undefined) {
- lastHoveredTaskId = hoveredData.taskId;
- setHoveredTaskId(hoveredData.taskId);
-
- // Apply new hover styles
- const tasks = document.querySelectorAll<HTMLDivElement>(
- `#${hoveredData.taskId.replaceAll(".", "-")}`,
- );
-
- tasks.forEach((task) => {
- task.style.backgroundColor = "var(--chakra-colors-info-subtle)";
- });
- }
- } else {
- lastHoveredTaskId = undefined;
- setHoveredTaskId(undefined);
- }
- };
+ return flatNodes.map((node) => byTaskId.get(node.id) ?? []);
};
-export const createChartOptions = ({
+export const computeGanttTimeRangeMs = ({
data,
- gridColor,
- handleBarClick,
- handleBarHover,
- hoveredId,
- hoveredItemColor,
- labels,
- selectedId,
- selectedItemColor,
selectedRun,
selectedTimezone,
- translate,
-}: ChartOptionsParams) => {
- const isActivePending = isStatePending(selectedRun?.state);
+}: {
+ data: Array<GanttDataItem>;
+ selectedRun?: GridRunsResponse;
+ selectedTimezone: string;
+}): { maxMs: number; minMs: number } => {
+ const isActivePending = selectedRun !== undefined &&
isStatePending(selectedRun.state);
const effectiveEndDate = isActivePending
? dayjs().tz(selectedTimezone).format("YYYY-MM-DD HH:mm:ss")
: selectedRun?.end_date;
+ if (data.length === 0) {
+ const minMs = new Date(formatDate(selectedRun?.start_date,
selectedTimezone)).getTime();
+ const maxMs = new Date(formatDate(effectiveEndDate,
selectedTimezone)).getTime();
+
Review Comment:
`computeGanttTimeRangeMs` converts the output of `formatDate(...)` back into
milliseconds via `new Date(...).getTime()`. `formatDate` returns a formatted
string (default "YYYY-MM-DD HH:mm:ss"), which is not reliably parseable across
browsers and also returns “now” when the input is null/invalid — both can skew
the axis range when `data.length === 0`. Prefer computing ms directly from the
raw timestamps with `dayjs(date).tz(selectedTimezone).valueOf()` (and handle
missing `selectedRun`/dates explicitly).
```suggestion
if (data.length === 0) {
const hasStart = Boolean(selectedRun?.start_date);
const hasEnd = Boolean(selectedRun?.end_date);
// When the run is still active/pending, use "now" as the effective end.
const nowMs = dayjs().tz(selectedTimezone).valueOf();
const minMs = hasStart
? dayjs(selectedRun!.start_date!).tz(selectedTimezone).valueOf()
: nowMs;
const maxMs = isActivePending
? nowMs
: hasEnd
? dayjs(selectedRun!.end_date!).tz(selectedTimezone).valueOf()
: minMs;
```
##########
airflow-core/src/airflow/ui/src/layouts/Details/Grid/Grid.tsx:
##########
@@ -141,7 +143,24 @@ export const Grid = ({
showVersionIndicatorMode,
});
- const { flatNodes } = useMemo(() => flattenNodes(dagStructure,
openGroupIds), [dagStructure, openGroupIds]);
+ const { flatNodes } = flattenNodes(dagStructure, openGroupIds);
+
+ const taskNameColumnWidthPx = showGantt ?
estimateTaskNameColumnWidthPx(flatNodes) : undefined;
+
+ const taskNameColumnStyles =
+ showGantt && taskNameColumnWidthPx !== undefined
+ ? {
+ flexGrow: 0,
+ flexShrink: 0,
+ maxW: `${taskNameColumnWidthPx}px`,
+ minW: `${taskNameColumnWidthPx}px`,
+ width: `${taskNameColumnWidthPx}px`,
+ }
+ : {
+ flexGrow: 1,
+ flexShrink: 0,
+ minW: "200px",
+ };
Review Comment:
`flattenNodes(...)` was previously memoized; it now runs on every render and
uses array spreads inside recursion, which can be expensive for large DAGs.
Since hover/selection updates can trigger frequent renders, this can become a
noticeable performance regression. Consider restoring `useMemo` around
`flattenNodes(dagStructure, openGroupIds)` (and the derived
`estimateTaskNameColumnWidthPx(flatNodes)`/`taskNameColumnStyles`) keyed on
`dagStructure`, `openGroupIds`, and `showGantt`.
##########
airflow-core/src/airflow/ui/src/layouts/Details/Gantt/utils.ts:
##########
@@ -144,246 +135,138 @@ export const transformGanttData = ({
.filter((item): item is GanttDataItem => item !== undefined);
};
-export const createHandleBarClick =
- ({ dagId, data, location, navigate, runId }: HandleBarClickOptions) =>
- (_: ChartEvent, elements: Array<ActiveElement>) => {
- if (elements.length === 0 || !elements[0] || !runId) {
- return;
- }
-
- const clickedData = data[elements[0].index];
+/** One entry per flat node: segments to draw in that row (tries or
aggregate). */
+export const buildGanttRowSegments = (
+ flatNodes: Array<GridTask>,
+ items: Array<GanttDataItem>,
+): Array<Array<GanttDataItem>> => {
+ const byTaskId = new Map<string, Array<GanttDataItem>>();
- if (!clickedData) {
- return;
- }
+ for (const item of items) {
+ const list = byTaskId.get(item.taskId) ?? [];
- const { isGroup, isMapped, taskId, tryNumber } = clickedData;
-
- const taskUrl = buildTaskInstanceUrl({
- currentPathname: location.pathname,
- dagId,
- isGroup: Boolean(isGroup),
- isMapped: Boolean(isMapped),
- runId,
- taskId,
- });
-
- const searchParams = new URLSearchParams(location.search);
- const isOlderTry =
- tryNumber !== undefined &&
- tryNumber <
- Math.max(...data.filter((item) => item.taskId === taskId).map((item)
=> item.tryNumber ?? 1));
-
- if (isOlderTry) {
- searchParams.set(SearchParamsKeys.TRY_NUMBER, tryNumber.toString());
- } else {
- searchParams.delete(SearchParamsKeys.TRY_NUMBER);
- }
-
- void Promise.resolve(
- navigate(
- {
- pathname: taskUrl,
- search: searchParams.toString(),
- },
- { replace: true },
- ),
- );
- };
+ list.push(item);
+ byTaskId.set(item.taskId, list);
+ }
-export const createHandleBarHover = (
- data: Array<GanttDataItem>,
- setHoveredTaskId: (taskId: string | undefined) => void,
-) => {
- let lastHoveredTaskId: string | undefined = undefined;
-
- return (_: ChartEvent, elements: Array<ActiveElement>) => {
- // Clear previous hover styles
- if (lastHoveredTaskId !== undefined) {
- const previousTasks = document.querySelectorAll<HTMLDivElement>(
- `#${lastHoveredTaskId.replaceAll(".", "-")}`,
- );
-
- previousTasks.forEach((task) => {
- task.style.backgroundColor = "";
- });
- }
-
- if (elements.length > 0 && elements[0] && elements[0].index < data.length)
{
- const hoveredData = data[elements[0].index];
-
- if (hoveredData?.taskId !== undefined) {
- lastHoveredTaskId = hoveredData.taskId;
- setHoveredTaskId(hoveredData.taskId);
-
- // Apply new hover styles
- const tasks = document.querySelectorAll<HTMLDivElement>(
- `#${hoveredData.taskId.replaceAll(".", "-")}`,
- );
-
- tasks.forEach((task) => {
- task.style.backgroundColor = "var(--chakra-colors-info-subtle)";
- });
- }
- } else {
- lastHoveredTaskId = undefined;
- setHoveredTaskId(undefined);
- }
- };
+ return flatNodes.map((node) => byTaskId.get(node.id) ?? []);
};
-export const createChartOptions = ({
+export const computeGanttTimeRangeMs = ({
data,
- gridColor,
- handleBarClick,
- handleBarHover,
- hoveredId,
- hoveredItemColor,
- labels,
- selectedId,
- selectedItemColor,
selectedRun,
selectedTimezone,
- translate,
-}: ChartOptionsParams) => {
- const isActivePending = isStatePending(selectedRun?.state);
+}: {
+ data: Array<GanttDataItem>;
+ selectedRun?: GridRunsResponse;
+ selectedTimezone: string;
+}): { maxMs: number; minMs: number } => {
+ const isActivePending = selectedRun !== undefined &&
isStatePending(selectedRun.state);
const effectiveEndDate = isActivePending
? dayjs().tz(selectedTimezone).format("YYYY-MM-DD HH:mm:ss")
: selectedRun?.end_date;
+ if (data.length === 0) {
+ const minMs = new Date(formatDate(selectedRun?.start_date,
selectedTimezone)).getTime();
+ const maxMs = new Date(formatDate(effectiveEndDate,
selectedTimezone)).getTime();
+
+ return { maxMs, minMs };
+ }
+
+ const maxTime = data.reduce((max, item) => Math.max(max, item.x[1]),
-Infinity);
+ const minTime = data.reduce((min, item) => Math.min(min, item.x[0]),
Infinity);
+ const totalDuration = maxTime - minTime;
+
return {
- animation: {
- duration: 150,
- easing: "linear" as const,
- },
- indexAxis: "y" as const,
- maintainAspectRatio: false,
- onClick: handleBarClick,
- onHover: (event: ChartEvent, elements: Array<ActiveElement>) => {
- const target = event.native?.target as HTMLElement | undefined;
-
- if (target) {
- target.style.cursor = elements.length > 0 ? "pointer" : "default";
- }
+ maxMs: maxTime + totalDuration * 0.05,
+ minMs: minTime - totalDuration * 0.02,
+ };
+};
- handleBarHover(event, elements);
- },
- plugins: {
- annotation: {
- annotations: [
- // Selected task annotation
- ...(selectedId === undefined || selectedId === "" || hoveredId ===
selectedId
- ? []
- : [
- {
- backgroundColor: selectedItemColor,
- borderWidth: 0,
- drawTime: "beforeDatasetsDraw" as const,
- type: "box" as const,
- xMax: "max" as const,
- xMin: "min" as const,
- yMax: labels.indexOf(selectedId) + 0.5,
- yMin: labels.indexOf(selectedId) - 0.5,
- },
- ]),
- // Hovered task annotation
- ...(hoveredId === null || hoveredId === undefined
- ? []
- : [
- {
- backgroundColor: hoveredItemColor,
- borderWidth: 0,
- drawTime: "beforeDatasetsDraw" as const,
- type: "box" as const,
- xMax: "max" as const,
- xMin: "min" as const,
- yMax: labels.indexOf(hoveredId) + 0.5,
- yMin: labels.indexOf(hoveredId) - 0.5,
- },
- ]),
- ],
- clip: false,
- },
- legend: {
- display: false,
- },
- tooltip: {
- callbacks: {
- afterBody(tooltipItems: Array<TooltipItem<"bar">>) {
- const taskInstance = data[tooltipItems[0]?.dataIndex ?? 0];
- const startDate = formatDate(taskInstance?.x[0], selectedTimezone);
- const endDate = formatDate(taskInstance?.x[1], selectedTimezone);
- const lines = [
- `${translate("startDate")}: ${startDate}`,
- `${translate("endDate")}: ${endDate}`,
- `${translate("duration")}: ${getDuration(taskInstance?.x[0],
taskInstance?.x[1])}`,
- ];
-
- if (taskInstance?.tryNumber !== undefined) {
- lines.unshift(`${translate("tryNumber")}:
${taskInstance.tryNumber}`);
- }
-
- return lines;
- },
- label(tooltipItem: TooltipItem<"bar">) {
- const taskInstance = data[tooltipItem.dataIndex];
+export const getGanttSegmentTo = ({
+ dagId,
+ data,
+ item,
+ location,
+ runId,
+}: GanttSegmentLinkParams): To | undefined => {
+ if (!runId) {
+ return undefined;
+ }
- return `${translate("state")}:
${translate(`states.${taskInstance?.state}`)}`;
- },
- },
- },
- },
- resizeDelay: 100,
- responsive: true,
- scales: {
- x: {
- grid: {
- color: gridColor,
- display: true,
- },
- max:
- data.length > 0
- ? (() => {
- const maxTime = Math.max(...data.map((item) => new
Date(item.x[1] ?? "").getTime()));
- const minTime = Math.min(...data.map((item) => new
Date(item.x[0] ?? "").getTime()));
- const totalDuration = maxTime - minTime;
-
- // add 5% to the max time to avoid the last tick being cut off
- return maxTime + totalDuration * 0.05;
- })()
- : formatDate(effectiveEndDate, selectedTimezone),
- min:
- data.length > 0
- ? (() => {
- const maxTime = Math.max(...data.map((item) => new
Date(item.x[1] ?? "").getTime()));
- const minTime = Math.min(...data.map((item) => new
Date(item.x[0] ?? "").getTime()));
- const totalDuration = maxTime - minTime;
-
- // subtract 2% from min time so background color shows before
data
- return minTime - totalDuration * 0.02;
- })()
- : formatDate(selectedRun?.start_date, selectedTimezone),
- position: "top" as const,
- stacked: true,
- ticks: {
- align: "start" as const,
- callback: (value: number | string) => formatDate(value,
selectedTimezone, "HH:mm:ss"),
- maxRotation: 8,
- maxTicksLimit: 8,
- minRotation: 8,
- },
- type: "time" as const,
- },
- y: {
- grid: {
- color: gridColor,
- display: true,
- },
- stacked: true,
- ticks: {
- display: false,
- },
- },
- },
+ const { isGroup, isMapped, taskId, tryNumber } = item;
+
+ const pathname = buildTaskInstanceUrl({
+ currentPathname: location.pathname,
+ dagId,
+ isGroup: Boolean(isGroup),
+ isMapped: Boolean(isMapped),
+ runId,
+ taskId,
+ });
+
+ const searchParams = new URLSearchParams(location.search);
+ const tryNumbersForTask = data
+ .filter((row: GanttDataItem) => row.taskId === taskId)
+ .map((row: GanttDataItem) => row.tryNumber ?? 1);
+ const maxTryForTask = tryNumbersForTask.length > 0 ?
Math.max(...tryNumbersForTask) : 1;
+ const isOlderTry = tryNumber !== undefined && tryNumber < maxTryForTask;
Review Comment:
`getGanttSegmentTo` does a full `data.filter(...).map(...)` to compute
`maxTryForTask`. In `GanttTimeline` this helper is called for every rendered
segment, so this becomes O(n^2) in the number of displayed segments/tries.
Consider precomputing a `Map<taskId, maxTryNumber>` once (e.g., alongside
`buildGanttRowSegments`) and passing it in, or storing `maxTryForTask` on each
`GanttDataItem`, so link generation is O(1) per segment.
--
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]
