pierrejeambrun commented on code in PR #64271:
URL: https://github.com/apache/airflow/pull/64271#discussion_r3116552105
##########
airflow-core/src/airflow/ui/src/context/groups/GroupsProvider.tsx:
##########
@@ -57,21 +54,17 @@ export const OpenGroupsProvider = ({ children, dagId }:
Props) => {
{ enabled: Boolean(dagId) && selectedVersion !== undefined },
);
- // Update allGroupIds whenever structure changes
- useEffect(() => {
- const observedGroupIds = flattenGraphNodes(structure.nodes).allGroupIds;
+ const { allGroupIds: observedGroupIds, allOperators } =
flattenGraphNodes(structure.nodes);
+ useEffect(() => {
if (JSON.stringify(observedGroupIds) !==
JSON.stringify(allGroupIdsRef.current)) {
Review Comment:
Double `JSON.stringify` on every render is pretty expensive for big DAGs.
Use `length` + `every()` element comparison, or compute `observedGroupIds`
outside the effect and use its reference as the dep.
##########
airflow-core/src/airflow/ui/src/layouts/Details/Graph/useGraphFilteredNodes.ts:
##########
@@ -0,0 +1,140 @@
+/*!
+ * 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 { Node as ReactFlowNode } from "@xyflow/react";
+import { useMemo } from "react";
+
+import type { CustomNodeProps } from "src/components/Graph/reactflowUtils";
+
+type GraphFilterValues = {
+ durationThreshold?: number;
+ mapIndex?: number;
+ selectedOperators: Array<string>;
+ selectedStates: Array<string>;
+ selectedTaskGroups: Array<string>;
+};
+
+const hasActiveFilters = (filters: GraphFilterValues): boolean =>
+ filters.selectedOperators.length > 0 ||
+ filters.selectedTaskGroups.length > 0 ||
+ filters.selectedStates.length > 0 ||
+ filters.durationThreshold !== undefined ||
+ filters.mapIndex !== undefined;
+
+const getTaskDurationSeconds = (
+ minStartDate: string | null | undefined,
+ maxEndDate: string | null | undefined,
+): number | undefined => {
+ if (
+ minStartDate === null ||
+ minStartDate === undefined ||
+ maxEndDate === null ||
+ maxEndDate === undefined
+ ) {
+ return undefined;
+ }
+ const start = new Date(minStartDate).getTime();
+ const end = new Date(maxEndDate).getTime();
+
+ if (Number.isNaN(start) || Number.isNaN(end)) {
+ return undefined;
+ }
+
+ return (end - start) / 1000;
+};
+
+const isNodeFiltered = (node: ReactFlowNode<CustomNodeProps>, filters:
GraphFilterValues): boolean => {
+ if (node.type !== "task") {
+ return false;
+ }
+
+ const { isMapped, operator, taskInstance } = node.data;
+
+ if (
+ filters.selectedOperators.length > 0 &&
+ (operator === undefined || operator === null ||
!filters.selectedOperators.includes(operator))
+ ) {
+ return true;
+ }
+
+ if (filters.selectedTaskGroups.length > 0) {
+ const matchesGroup = filters.selectedTaskGroups.some(
+ (group) => node.id === group || node.id.startsWith(`${group}.`),
+ );
+
+ if (!matchesGroup) {
+ return true;
+ }
+ }
+
+ if (filters.selectedStates.length > 0) {
+ const state = taskInstance?.state ?? "none";
+
+ if (!filters.selectedStates.includes(state)) {
+ return true;
+ }
+ }
+
+ if (filters.mapIndex !== undefined) {
+ if (!isMapped || taskInstance === undefined) {
+ return true;
+ }
+ const total = Object.values(taskInstance.child_states ?? {}).reduce(
+ (acc, currentValue) => acc + currentValue,
+ 0,
+ );
+
+ if (filters.mapIndex >= total) {
Review Comment:
The map index filter semantics aren't obvious: `filters.mapIndex >= total`
effectively means "show mapped tasks with at least N+1 expansions". Is the user
meant to read this as "show the task instance at map_index=N"? The label in the
UI is just "mapIndex" which is ambiguous. Clarify the UX or the label.
##########
airflow-core/src/airflow/ui/src/layouts/Details/Graph/useGraphFilteredNodes.ts:
##########
@@ -0,0 +1,140 @@
+/*!
+ * 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 { Node as ReactFlowNode } from "@xyflow/react";
+import { useMemo } from "react";
+
+import type { CustomNodeProps } from "src/components/Graph/reactflowUtils";
+
+type GraphFilterValues = {
+ durationThreshold?: number;
+ mapIndex?: number;
+ selectedOperators: Array<string>;
+ selectedStates: Array<string>;
+ selectedTaskGroups: Array<string>;
+};
+
+const hasActiveFilters = (filters: GraphFilterValues): boolean =>
+ filters.selectedOperators.length > 0 ||
+ filters.selectedTaskGroups.length > 0 ||
+ filters.selectedStates.length > 0 ||
+ filters.durationThreshold !== undefined ||
+ filters.mapIndex !== undefined;
+
+const getTaskDurationSeconds = (
+ minStartDate: string | null | undefined,
+ maxEndDate: string | null | undefined,
+): number | undefined => {
+ if (
+ minStartDate === null ||
+ minStartDate === undefined ||
+ maxEndDate === null ||
+ maxEndDate === undefined
+ ) {
+ return undefined;
+ }
+ const start = new Date(minStartDate).getTime();
+ const end = new Date(maxEndDate).getTime();
+
+ if (Number.isNaN(start) || Number.isNaN(end)) {
+ return undefined;
+ }
+
+ return (end - start) / 1000;
+};
+
+const isNodeFiltered = (node: ReactFlowNode<CustomNodeProps>, filters:
GraphFilterValues): boolean => {
+ if (node.type !== "task") {
+ return false;
+ }
+
+ const { isMapped, operator, taskInstance } = node.data;
+
+ if (
+ filters.selectedOperators.length > 0 &&
+ (operator === undefined || operator === null ||
!filters.selectedOperators.includes(operator))
+ ) {
+ return true;
+ }
+
+ if (filters.selectedTaskGroups.length > 0) {
+ const matchesGroup = filters.selectedTaskGroups.some(
+ (group) => node.id === group || node.id.startsWith(`${group}.`),
+ );
+
+ if (!matchesGroup) {
+ return true;
+ }
+ }
+
+ if (filters.selectedStates.length > 0) {
+ const state = taskInstance?.state ?? "none";
+
+ if (!filters.selectedStates.includes(state)) {
+ return true;
+ }
+ }
+
+ if (filters.mapIndex !== undefined) {
+ if (!isMapped || taskInstance === undefined) {
+ return true;
+ }
+ const total = Object.values(taskInstance.child_states ?? {}).reduce(
+ (acc, currentValue) => acc + currentValue,
+ 0,
+ );
+
+ if (filters.mapIndex >= total) {
+ return true;
+ }
+ }
+
+ if (filters.durationThreshold !== undefined) {
+ const duration = getTaskDurationSeconds(taskInstance?.min_start_date,
taskInstance?.max_end_date);
Review Comment:
For mapped tasks, `min_start_date` / `max_end_date` span ALL map indices. So
"show tasks that took > 10s" ends up matching any mapped task whose total span
exceeds 10s — not per-instance. Worth documenting or deciding whether the
threshold should be per-instance for mapped tasks.
##########
airflow-core/src/airflow/ui/src/context/groups/GroupsProvider.tsx:
##########
@@ -25,28 +25,25 @@ import { allGroupsKey, dependenciesKey, openGroupsKey }
from "src/constants/loca
import useSelectedVersion from "src/hooks/useSelectedVersion";
import { flattenGraphNodes } from "src/layouts/Details/Grid/utils";
-import { OpenGroupsContext, type OpenGroupsContextType } from "./Context";
+import { GroupsContext, type GroupsContextType } from "./Context";
type Props = {
readonly dagId: string;
} & PropsWithChildren;
-export const OpenGroupsProvider = ({ children, dagId }: Props) => {
+export const GroupsProvider = ({ children, dagId }: Props) => {
const [openGroupIds, setOpenGroupIds] =
useLocalStorage<Array<string>>(openGroupsKey(dagId), []);
const [allGroupIds, setAllGroupIds] =
useLocalStorage<Array<string>>(allGroupsKey(dagId), []);
Review Comment:
`allGroupIds` is derived from the DAG structure API — it shouldn't live in
localStorage. The source of truth is the structure response; persisting this
means stale data can linger across DAG changes (e.g., user removes a task group
in code, localStorage still has it until the effect at line 60 rewrites). Just
compute it from `structure.nodes` each render (memoized) and pass through
context.
##########
airflow-core/src/airflow/ui/src/layouts/Details/Graph/useGraphFilteredNodes.test.ts:
##########
@@ -0,0 +1,330 @@
+/* eslint-disable max-lines */
+
+/*!
+ * 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 { renderHook } from "@testing-library/react";
+import type { Node as ReactFlowNode } from "@xyflow/react";
+import { describe, expect, it } from "vitest";
+
+import type { CustomNodeProps } from "src/components/Graph/reactflowUtils";
+
+import { type GraphFilterValues, useGraphFilteredNodes } from
"./useGraphFilteredNodes";
+
+const makeNode = (
+ overrides: { id?: string; type?: string } & Partial<CustomNodeProps> = {},
+): ReactFlowNode<CustomNodeProps> => ({
+ data: {
+ id: overrides.id ?? "task_a",
+ label: overrides.id ?? "task_a",
+ type: overrides.type ?? "task",
+ ...overrides,
+ },
+ id: overrides.id ?? "task_a",
+ position: { x: 0, y: 0 },
+ type: overrides.type ?? "task",
+});
+
+const noFilters: GraphFilterValues = {
+ selectedOperators: [],
+ selectedStates: [],
+ selectedTaskGroups: [],
+};
+
+describe("useGraphFilteredNodes", () => {
Review Comment:
Good coverage on the pure hook (22 tests), but `GraphTaskFilters.tsx` itself
— popover open/close, URL param round-trip, clearFilters — has no tests. The
component owns non-trivial state mutation logic; worth adding a component-level
test.
##########
airflow-core/src/airflow/ui/src/components/GraphTaskFilters.tsx:
##########
@@ -0,0 +1,220 @@
+/*!
+ * 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 {
+ Button,
+ IconButton,
+ type NumberInputValueChangeDetails,
+ Portal,
+ Separator,
+ Text,
+ VStack,
+} from "@chakra-ui/react";
+import { useState } from "react";
+import { useHotkeys } from "react-hotkeys-hook";
+import { useTranslation } from "react-i18next";
+import { FiSearch } from "react-icons/fi";
+import { useParams, useSearchParams } from "react-router-dom";
+
+import type { TaskInstanceState } from "openapi/requests/types.gen";
+import { AttrSelectFilterMulti } from "src/components/AttrSelectFilterMulti";
+import { StateBadge } from "src/components/StateBadge";
+import { Select } from "src/components/ui";
+import { Menu } from "src/components/ui/Menu";
+import { NumberInputField, NumberInputRoot } from
"src/components/ui/NumberInput";
+import { SearchParamsKeys } from "src/constants/searchParams";
+import { taskInstanceStateOptions } from "src/constants/stateOptions";
+import { useGroups } from "src/context/groups";
+
+export const GraphTaskFilters = () => {
+ const { t: translate } = useTranslation(["dag", "tasks"]);
+ const { runId } = useParams();
+ const [searchParams, setSearchParams] = useSearchParams();
+
+ const { allGroupIds: allTaskGroups, allOperators } = useGroups();
+
+ const selectedOperators =
searchParams.getAll(SearchParamsKeys.GRAPH_OPERATOR);
+ const selectedTaskGroups =
searchParams.getAll(SearchParamsKeys.GRAPH_TASK_GROUP);
+ const selectedStates =
searchParams.getAll(SearchParamsKeys.GRAPH_TASK_STATE);
+ const mapIndexParam = searchParams.get(SearchParamsKeys.GRAPH_MAP_INDEX) ??
"";
+ const durationGteParam =
searchParams.get(SearchParamsKeys.GRAPH_DURATION_GTE) ?? "";
+
+ const hasActiveFilters =
+ selectedOperators.length > 0 ||
+ selectedTaskGroups.length > 0 ||
+ selectedStates.length > 0 ||
+ mapIndexParam !== "" ||
+ durationGteParam !== "";
+
+ const stateItems = taskInstanceStateOptions.items.filter(
+ (item) => item.value !== "all" && item.value !== "none",
+ );
+
+ const handleMultiChange = (key: string) => (values: Array<string> |
undefined) => {
+ searchParams.delete(key);
+ values?.forEach((val) => searchParams.append(key, val));
+ setSearchParams(searchParams);
+ };
+
+ const handleNumberChange =
+ (key: string, condition: (n: number) => boolean) => (details:
NumberInputValueChangeDetails) => {
+ if (condition(details.valueAsNumber)) {
+ searchParams.set(key, details.value);
+ } else {
+ searchParams.delete(key);
+ }
+ setSearchParams(searchParams);
+ };
+
+ const clearAllFilters = () => {
+ [
+ SearchParamsKeys.GRAPH_OPERATOR,
+ SearchParamsKeys.GRAPH_TASK_GROUP,
+ SearchParamsKeys.GRAPH_TASK_STATE,
+ SearchParamsKeys.GRAPH_MAP_INDEX,
+ SearchParamsKeys.GRAPH_DURATION_GTE,
+ ].forEach((key) => searchParams.delete(key));
+ setSearchParams(searchParams);
+ };
+
+ const [isOpen, setIsOpen] = useState(false);
+
+ useHotkeys("mod+shift+f", () => setIsOpen(true), { preventDefault: true });
Review Comment:
`mod+shift+f` isn't documented or namespaced. Airflow UI already has a few
global hotkeys; worth verifying no clash and adding this to the hotkeys
docs/help overlay if one exists.
##########
airflow-core/src/airflow/ui/src/context/groups/GroupsProvider.tsx:
##########
@@ -57,21 +54,17 @@ export const OpenGroupsProvider = ({ children, dagId }:
Props) => {
{ enabled: Boolean(dagId) && selectedVersion !== undefined },
);
- // Update allGroupIds whenever structure changes
- useEffect(() => {
- const observedGroupIds = flattenGraphNodes(structure.nodes).allGroupIds;
+ const { allGroupIds: observedGroupIds, allOperators } =
flattenGraphNodes(structure.nodes);
Review Comment:
`flattenGraphNodes(structure.nodes)` runs on every render of the provider,
which is a full tree walk. Wrap in `useMemo` keyed on `structure.nodes`.
##########
airflow-core/src/airflow/ui/src/components/GraphTaskFilters.tsx:
##########
@@ -0,0 +1,220 @@
+/*!
+ * 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 {
+ Button,
+ IconButton,
+ type NumberInputValueChangeDetails,
+ Portal,
+ Separator,
+ Text,
+ VStack,
+} from "@chakra-ui/react";
+import { useState } from "react";
+import { useHotkeys } from "react-hotkeys-hook";
+import { useTranslation } from "react-i18next";
+import { FiSearch } from "react-icons/fi";
+import { useParams, useSearchParams } from "react-router-dom";
+
+import type { TaskInstanceState } from "openapi/requests/types.gen";
+import { AttrSelectFilterMulti } from "src/components/AttrSelectFilterMulti";
+import { StateBadge } from "src/components/StateBadge";
+import { Select } from "src/components/ui";
+import { Menu } from "src/components/ui/Menu";
+import { NumberInputField, NumberInputRoot } from
"src/components/ui/NumberInput";
+import { SearchParamsKeys } from "src/constants/searchParams";
+import { taskInstanceStateOptions } from "src/constants/stateOptions";
+import { useGroups } from "src/context/groups";
+
+export const GraphTaskFilters = () => {
+ const { t: translate } = useTranslation(["dag", "tasks"]);
+ const { runId } = useParams();
+ const [searchParams, setSearchParams] = useSearchParams();
+
+ const { allGroupIds: allTaskGroups, allOperators } = useGroups();
+
+ const selectedOperators =
searchParams.getAll(SearchParamsKeys.GRAPH_OPERATOR);
+ const selectedTaskGroups =
searchParams.getAll(SearchParamsKeys.GRAPH_TASK_GROUP);
Review Comment:
When the user switches to a historical DAG version, a selected task group
from the URL may no longer exist in the new `allTaskGroups`. The filter stays
active with a stale value, leading to an "empty" filtered graph with no visual
indication why. Consider clearing invalid selections when `allGroupIds` /
`allOperators` change, or showing the invalid pill with a warning.
##########
airflow-core/src/airflow/ui/src/components/GraphTaskFilters.tsx:
##########
@@ -0,0 +1,220 @@
+/*!
+ * 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 {
+ Button,
+ IconButton,
+ type NumberInputValueChangeDetails,
+ Portal,
+ Separator,
+ Text,
+ VStack,
+} from "@chakra-ui/react";
+import { useState } from "react";
+import { useHotkeys } from "react-hotkeys-hook";
+import { useTranslation } from "react-i18next";
+import { FiSearch } from "react-icons/fi";
+import { useParams, useSearchParams } from "react-router-dom";
+
+import type { TaskInstanceState } from "openapi/requests/types.gen";
+import { AttrSelectFilterMulti } from "src/components/AttrSelectFilterMulti";
+import { StateBadge } from "src/components/StateBadge";
+import { Select } from "src/components/ui";
+import { Menu } from "src/components/ui/Menu";
+import { NumberInputField, NumberInputRoot } from
"src/components/ui/NumberInput";
+import { SearchParamsKeys } from "src/constants/searchParams";
+import { taskInstanceStateOptions } from "src/constants/stateOptions";
+import { useGroups } from "src/context/groups";
+
+export const GraphTaskFilters = () => {
+ const { t: translate } = useTranslation(["dag", "tasks"]);
+ const { runId } = useParams();
+ const [searchParams, setSearchParams] = useSearchParams();
+
+ const { allGroupIds: allTaskGroups, allOperators } = useGroups();
+
+ const selectedOperators =
searchParams.getAll(SearchParamsKeys.GRAPH_OPERATOR);
+ const selectedTaskGroups =
searchParams.getAll(SearchParamsKeys.GRAPH_TASK_GROUP);
+ const selectedStates =
searchParams.getAll(SearchParamsKeys.GRAPH_TASK_STATE);
+ const mapIndexParam = searchParams.get(SearchParamsKeys.GRAPH_MAP_INDEX) ??
"";
+ const durationGteParam =
searchParams.get(SearchParamsKeys.GRAPH_DURATION_GTE) ?? "";
+
+ const hasActiveFilters =
+ selectedOperators.length > 0 ||
+ selectedTaskGroups.length > 0 ||
+ selectedStates.length > 0 ||
+ mapIndexParam !== "" ||
+ durationGteParam !== "";
+
+ const stateItems = taskInstanceStateOptions.items.filter(
+ (item) => item.value !== "all" && item.value !== "none",
+ );
+
+ const handleMultiChange = (key: string) => (values: Array<string> |
undefined) => {
+ searchParams.delete(key);
+ values?.forEach((val) => searchParams.append(key, val));
+ setSearchParams(searchParams);
Review Comment:
`searchParams.delete(...); searchParams.append(...);
setSearchParams(searchParams)` mutates the object returned by
`useSearchParams()` directly and pushes to history on every toggle. Prefer
`setSearchParams((prev) => { const next = new URLSearchParams(prev); ...;
return next; }, { replace: true })` — avoids mutation and stops polluting the
back button every time a user clicks a checkbox.
--
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]
