bbovenzi commented on code in PR #64271:
URL: https://github.com/apache/airflow/pull/64271#discussion_r3046457881
##########
airflow-core/src/airflow/ui/src/components/Graph/graphTypes.ts:
##########
@@ -25,6 +25,8 @@ import Edge from "./Edge";
import { JoinNode } from "./JoinNode";
import { TaskNode } from "./TaskNode";
+export const filteredOpacityTransition = "opacity 0.2s";
Review Comment:
Actually, this would always be matched with `opacity={isFiltered ? 0.2 1}`
then we should probably make this into object and keep both styles together.
Then we can be less verbose with a variable name like `opacityStyle`
##########
airflow-core/src/airflow/ui/src/components/Graph/JoinNode.tsx:
##########
@@ -28,6 +28,8 @@ export const JoinNode = ({ data }:
NodeProps<NodeType<CustomNodeProps, "join">>)
bg="border.inverted"
borderRadius={`${data.width}px`}
height={`${data.height}px`}
+ opacity={data.isFiltered ? 0.2 : 1}
+ style={{ transition: "opacity 0.2s" }}
Review Comment:
We didn't use the fiteredOpacity value here
##########
airflow-core/src/airflow/ui/public/i18n/locales/ru/dag.json:
##########
@@ -122,6 +122,14 @@
"graphDirection": {
"label": "Направление графа"
},
+ "graphFilters": {
Review Comment:
We still need to remove any placeholder translations
##########
airflow-core/src/airflow/ui/src/components/GraphTaskFilters.tsx:
##########
@@ -0,0 +1,246 @@
+/*!
+ * 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 { useMemo, 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 { useStructureServiceStructureData } from "openapi/queries";
+import type { NodeResponse } from "openapi/requests/types.gen";
+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 useSelectedVersion from "src/hooks/useSelectedVersion";
+import { AttrSelectFilterMulti } from
"src/pages/Dag/Tasks/TaskFilters/AttrSelectFilterMulti";
+
+const collectOperators = (nodes: Array<NodeResponse>): Array<string> => {
+ const operators = new Set<string>();
+
+ const walk = (nodeList: Array<NodeResponse>) => {
+ for (const node of nodeList) {
+ if (node.operator !== undefined && node.operator !== null) {
+ operators.add(node.operator);
+ }
+ if (node.children) {
+ walk(node.children);
+ }
+ }
+ };
+
+ walk(nodes);
+
+ return [...operators].sort();
+};
+
+const collectTaskGroups = (nodes: Array<NodeResponse>): Array<string> => {
+ const groups: Array<string> = [];
+
+ const walk = (nodeList: Array<NodeResponse>) => {
+ for (const node of nodeList) {
+ if (node.children) {
+ groups.push(node.id);
+ walk(node.children);
+ }
+ }
+ };
+
+ walk(nodes);
+
+ return groups.sort();
+};
+
+export const GraphTaskFilters = () => {
+ const { t: translate } = useTranslation(["dag", "tasks"]);
+ const { dagId = "", runId } = useParams();
+ const [searchParams, setSearchParams] = useSearchParams();
+ const selectedVersion = useSelectedVersion();
+
+ const { data: graphData } = useStructureServiceStructureData(
+ { dagId, versionNumber: selectedVersion },
+ undefined,
+ { enabled: selectedVersion !== undefined },
+ );
+ const graphNodes = useMemo(() => graphData?.nodes ?? [], [graphData?.nodes]);
+ const allOperators = useMemo(() => collectOperators(graphNodes),
[graphNodes]);
+ const allTaskGroups = useMemo(() => collectTaskGroups(graphNodes),
[graphNodes]);
Review Comment:
Why do we need allTaskGroups again? We already have `useOpenGroups()` which
will give you all ids. it would be nice to not have to walk through the graph
many times.
##########
airflow-core/src/airflow/ui/src/components/GraphTaskFilters.tsx:
##########
@@ -0,0 +1,246 @@
+/*!
+ * 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 { useMemo, 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 { useStructureServiceStructureData } from "openapi/queries";
+import type { NodeResponse } from "openapi/requests/types.gen";
+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 useSelectedVersion from "src/hooks/useSelectedVersion";
+import { AttrSelectFilterMulti } from
"src/pages/Dag/Tasks/TaskFilters/AttrSelectFilterMulti";
+
+const collectOperators = (nodes: Array<NodeResponse>): Array<string> => {
+ const operators = new Set<string>();
+
+ const walk = (nodeList: Array<NodeResponse>) => {
+ for (const node of nodeList) {
+ if (node.operator !== undefined && node.operator !== null) {
+ operators.add(node.operator);
+ }
+ if (node.children) {
+ walk(node.children);
+ }
+ }
+ };
+
+ walk(nodes);
+
+ return [...operators].sort();
+};
+
+const collectTaskGroups = (nodes: Array<NodeResponse>): Array<string> => {
+ const groups: Array<string> = [];
+
+ const walk = (nodeList: Array<NodeResponse>) => {
+ for (const node of nodeList) {
+ if (node.children) {
+ groups.push(node.id);
+ walk(node.children);
+ }
+ }
+ };
+
+ walk(nodes);
+
+ return groups.sort();
+};
+
+export const GraphTaskFilters = () => {
+ const { t: translate } = useTranslation(["dag", "tasks"]);
+ const { dagId = "", runId } = useParams();
+ const [searchParams, setSearchParams] = useSearchParams();
+ const selectedVersion = useSelectedVersion();
+
+ const { data: graphData } = useStructureServiceStructureData(
+ { dagId, versionNumber: selectedVersion },
+ undefined,
+ { enabled: selectedVersion !== undefined },
+ );
+ const graphNodes = useMemo(() => graphData?.nodes ?? [], [graphData?.nodes]);
+ const allOperators = useMemo(() => collectOperators(graphNodes),
[graphNodes]);
+ const allTaskGroups = useMemo(() => collectTaskGroups(graphNodes),
[graphNodes]);
+
+ const selectedOperators = searchParams.getAll(SearchParamsKeys.OPERATOR);
+ const selectedTaskGroups = searchParams.getAll(SearchParamsKeys.TASK_GROUP);
+ const selectedStates = searchParams.getAll(SearchParamsKeys.TASK_STATE);
+ const mapIndexParam = searchParams.get(SearchParamsKeys.MAP_INDEX) ?? "";
+ const durationGteParam = searchParams.get(SearchParamsKeys.DURATION_GTE) ??
"";
+
+ const hasActiveFilters =
+ selectedOperators.length > 0 ||
+ selectedTaskGroups.length > 0 ||
+ selectedStates.length > 0 ||
+ mapIndexParam !== "" ||
+ durationGteParam !== "";
+
+ const stateValues = useMemo(
+ () => taskInstanceStateOptions.items.filter((item) => item.value !==
"all").map((item) => item.value),
+ [],
+ );
+
+ 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.OPERATOR,
+ SearchParamsKeys.TASK_GROUP,
+ SearchParamsKeys.TASK_STATE,
+ SearchParamsKeys.MAP_INDEX,
+ SearchParamsKeys.DURATION_GTE,
+ ].forEach((key) => searchParams.delete(key));
+ setSearchParams(searchParams);
+ };
+
+ const [isOpen, setIsOpen] = useState(false);
+
+ useHotkeys("mod+f", () => setIsOpen(true), { preventDefault: true });
+
+ const tooltipContent = translate("dag:panel.graphFilters.title");
+
+ return (
+ <Menu.Root
+ onOpenChange={({ open: nextOpen }) => setIsOpen(nextOpen)}
+ open={isOpen}
+ positioning={{ placement: "bottom-end" }}
+ >
+ <Menu.Trigger asChild>
+ <IconButton
+ aria-label={tooltipContent}
+ colorPalette="brand"
+ size="md"
+ title={tooltipContent}
+ variant={hasActiveFilters ? "solid" : "ghost"}
+ >
+ <FiSearch />
+ </IconButton>
+ </Menu.Trigger>
+ <Portal>
+ <Menu.Positioner>
+ <Menu.Content alignItems="start" display="flex"
flexDirection="column" gap={2} p={4}>
+ <Text fontSize="sm" fontWeight="semibold">
+ {translate("dag:panel.graphFilters.title")}
+ </Text>
+
+ <Separator my={2} />
+
+ {allOperators.length > 1 ? (
+ <VStack alignItems="flex-start" width="100%">
+ <Text fontSize="xs">{translate("tasks:selectOperator")}</Text>
+ <AttrSelectFilterMulti
+ displayPrefix={undefined}
+ handleSelect={handleMultiChange(SearchParamsKeys.OPERATOR)}
+ placeholderText={translate("tasks:selectOperator")}
+ selectedValues={selectedOperators}
+ values={allOperators}
+ />
+ </VStack>
+ ) : undefined}
+
+ {allTaskGroups.length > 0 ? (
+ <VStack alignItems="flex-start" width="100%">
+ <Text
fontSize="xs">{translate("dag:panel.graphFilters.selectTaskGroup")}</Text>
+ <AttrSelectFilterMulti
+ displayPrefix={undefined}
+ handleSelect={handleMultiChange(SearchParamsKeys.TASK_GROUP)}
+
placeholderText={translate("dag:panel.graphFilters.selectTaskGroup")}
+ selectedValues={selectedTaskGroups}
+ values={allTaskGroups}
+ />
+ </VStack>
+ ) : undefined}
+
+ {runId === undefined ? undefined : (
+ <>
+ <VStack alignItems="flex-start" width="100%">
+ <Text
fontSize="xs">{translate("dag:panel.graphFilters.selectStatus")}</Text>
+ <AttrSelectFilterMulti
+ displayPrefix={undefined}
+
handleSelect={handleMultiChange(SearchParamsKeys.TASK_STATE)}
+
placeholderText={translate("dag:panel.graphFilters.selectStatus")}
+ selectedValues={selectedStates}
+ values={stateValues}
+ />
+ </VStack>
+ <VStack alignItems="flex-start" width="100%">
+ <Text
fontSize="xs">{translate("dag:panel.graphFilters.mapIndex")}</Text>
+ <NumberInputRoot
+ min={0}
+
onValueChange={handleNumberChange(SearchParamsKeys.MAP_INDEX, (num) => num >=
0)}
+ size="sm"
+ value={mapIndexParam}
+ w="100%"
+ >
+ <NumberInputField
placeholder={translate("dag:panel.graphFilters.mapIndex")} />
+ </NumberInputRoot>
+ </VStack>
+ <VStack alignItems="flex-start" width="100%">
+ <Text
fontSize="xs">{translate("dag:panel.graphFilters.durationGte")}</Text>
+ <NumberInputRoot
+ min={0}
Review Comment:
We actually might want the duration to have a step of "0.1"
##########
airflow-core/src/airflow/ui/src/components/GraphTaskFilters.tsx:
##########
@@ -0,0 +1,246 @@
+/*!
+ * 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 { useMemo, 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 { useStructureServiceStructureData } from "openapi/queries";
+import type { NodeResponse } from "openapi/requests/types.gen";
+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 useSelectedVersion from "src/hooks/useSelectedVersion";
+import { AttrSelectFilterMulti } from
"src/pages/Dag/Tasks/TaskFilters/AttrSelectFilterMulti";
+
+const collectOperators = (nodes: Array<NodeResponse>): Array<string> => {
Review Comment:
We are walking through the nested graph nodes many times. We should try to
do it only once when we're collecting all this information.
See how we are already getting all task group ids. It's probably best to use
our existing `useOpenGroups()` hook and expand it to handle all our
NodeResponse filtering and walking.
##########
airflow-core/src/airflow/ui/src/components/GraphTaskFilters.tsx:
##########
@@ -0,0 +1,246 @@
+/*!
+ * 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 { useMemo, useState } from "react";
Review Comment:
We are still using useMemo in too many places. I added a new agents.md file,
please rebase and make sure your AI agent reads it next time.
##########
airflow-core/src/airflow/ui/src/components/GraphTaskFilters.tsx:
##########
@@ -0,0 +1,246 @@
+/*!
+ * 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 { useMemo, 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 { useStructureServiceStructureData } from "openapi/queries";
+import type { NodeResponse } from "openapi/requests/types.gen";
+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 useSelectedVersion from "src/hooks/useSelectedVersion";
+import { AttrSelectFilterMulti } from
"src/pages/Dag/Tasks/TaskFilters/AttrSelectFilterMulti";
Review Comment:
No, I said to move it into `src/components`
##########
airflow-core/src/airflow/ui/src/components/GraphTaskFilters.tsx:
##########
@@ -0,0 +1,246 @@
+/*!
+ * 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 { useMemo, 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 { useStructureServiceStructureData } from "openapi/queries";
+import type { NodeResponse } from "openapi/requests/types.gen";
+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 useSelectedVersion from "src/hooks/useSelectedVersion";
+import { AttrSelectFilterMulti } from
"src/pages/Dag/Tasks/TaskFilters/AttrSelectFilterMulti";
+
+const collectOperators = (nodes: Array<NodeResponse>): Array<string> => {
+ const operators = new Set<string>();
+
+ const walk = (nodeList: Array<NodeResponse>) => {
+ for (const node of nodeList) {
+ if (node.operator !== undefined && node.operator !== null) {
+ operators.add(node.operator);
+ }
+ if (node.children) {
+ walk(node.children);
+ }
+ }
+ };
+
+ walk(nodes);
+
+ return [...operators].sort();
+};
+
+const collectTaskGroups = (nodes: Array<NodeResponse>): Array<string> => {
+ const groups: Array<string> = [];
+
+ const walk = (nodeList: Array<NodeResponse>) => {
+ for (const node of nodeList) {
+ if (node.children) {
+ groups.push(node.id);
+ walk(node.children);
+ }
+ }
+ };
+
+ walk(nodes);
+
+ return groups.sort();
+};
+
+export const GraphTaskFilters = () => {
+ const { t: translate } = useTranslation(["dag", "tasks"]);
+ const { dagId = "", runId } = useParams();
+ const [searchParams, setSearchParams] = useSearchParams();
+ const selectedVersion = useSelectedVersion();
+
+ const { data: graphData } = useStructureServiceStructureData(
+ { dagId, versionNumber: selectedVersion },
+ undefined,
+ { enabled: selectedVersion !== undefined },
+ );
+ const graphNodes = useMemo(() => graphData?.nodes ?? [], [graphData?.nodes]);
+ const allOperators = useMemo(() => collectOperators(graphNodes),
[graphNodes]);
+ const allTaskGroups = useMemo(() => collectTaskGroups(graphNodes),
[graphNodes]);
+
+ const selectedOperators = searchParams.getAll(SearchParamsKeys.OPERATOR);
+ const selectedTaskGroups = searchParams.getAll(SearchParamsKeys.TASK_GROUP);
+ const selectedStates = searchParams.getAll(SearchParamsKeys.TASK_STATE);
+ const mapIndexParam = searchParams.get(SearchParamsKeys.MAP_INDEX) ?? "";
+ const durationGteParam = searchParams.get(SearchParamsKeys.DURATION_GTE) ??
"";
+
+ const hasActiveFilters =
+ selectedOperators.length > 0 ||
+ selectedTaskGroups.length > 0 ||
+ selectedStates.length > 0 ||
+ mapIndexParam !== "" ||
+ durationGteParam !== "";
+
+ const stateValues = useMemo(
+ () => taskInstanceStateOptions.items.filter((item) => item.value !==
"all").map((item) => item.value),
+ [],
+ );
Review Comment:
We already have a select that uses our state badges. We should try to reuse
it here instead of having plain text dropdown options.
<img width="556" height="494" alt="Image"
src="https://github.com/user-attachments/assets/c1db34e3-2888-481a-a9cb-deb08f8b42cb";
/>
--
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]
