Copilot commented on code in PR #64689: URL: https://github.com/apache/airflow/pull/64689#discussion_r3066485349
########## airflow-core/src/airflow/ui/src/components/Graph/TriggerNode.tsx: ########## @@ -0,0 +1,137 @@ +/*! + * 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 { Flex, HStack, Text } from "@chakra-ui/react"; +import type { NodeProps, Node as NodeType } from "@xyflow/react"; +import { memo } from "react"; +import { useTranslation } from "react-i18next"; +import { FiZap } from "react-icons/fi"; + +import { NodeWrapper } from "./NodeWrapper"; +import type { CustomNodeProps } from "./reactflowUtils"; + +/** + * Trigger node component for DAG graph visualization. + * + * Displays a trigger task node in the DAG graph with a lightning bolt icon, + * node label, and descriptive subtitle. Supports internationalization for + * 21 languages. + * + * @component + * @example + * <TriggerNode + * data={{ label: "Manual Trigger", height: 100, width: 150, isSelected: false }} + * id="trigger-1" + * /> + * + * @param {NodeProps<NodeType<CustomNodeProps, "trigger">>} props + * @param {Object} props.data - Node data object + * @param {number} [props.data.height=100] - Node height in pixels (clamped: 40-1000) + * @param {number} [props.data.width=100] - Node width in pixels (clamped: 40-1000) + * @param {string} [props.data.label="Unknown"] - Trigger node label/name + * @param {boolean} [props.data.isSelected=false] - Whether the node is selected + * @returns {React.ReactElement} Rendered trigger node component + */ +export const TriggerNode = memo( + ({ + data: { + height = 100, + isSelected = false, + label = "Unknown", + width = 100, + }, + }: NodeProps<NodeType<CustomNodeProps, "trigger">>) => { + const { t } = useTranslation("components"); Review Comment: In this Graph area, other components alias `t` as `translate` for readability/consistency (e.g. `AssetNode.tsx:34`, `TaskNode.tsx:51`, `DownloadButton.tsx:28`). Consider following the same pattern here to match established conventions. ########## airflow-core/src/airflow/ui/public/i18n/locales/en/components.json: ########## @@ -91,7 +91,8 @@ "otherDagRuns": "+Other Dag Runs", "taskCount_one": "{{count}} Task", "taskCount_other": "{{count}} Tasks", - "taskGroup": "Task Group" + "taskGroup": "Task Group", + "triggerDagRun": "TriggerDagRunOperator" Review Comment: The `graph.triggerDagRun` translation value is set to `"TriggerDagRunOperator"`, which reads like a class name and doesn’t match the UI subtitle/defaultValue (`"Trigger DAG Run"`). This will surface a confusing label in the graph; please change the English string to the human-readable text intended for display. ########## airflow-core/src/airflow/ui/src/components/Graph/TriggerNode.tsx: ########## @@ -0,0 +1,137 @@ +/*! + * 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 { Flex, HStack, Text } from "@chakra-ui/react"; +import type { NodeProps, Node as NodeType } from "@xyflow/react"; +import { memo } from "react"; +import { useTranslation } from "react-i18next"; +import { FiZap } from "react-icons/fi"; + +import { NodeWrapper } from "./NodeWrapper"; +import type { CustomNodeProps } from "./reactflowUtils"; + +/** + * Trigger node component for DAG graph visualization. + * + * Displays a trigger task node in the DAG graph with a lightning bolt icon, + * node label, and descriptive subtitle. Supports internationalization for + * 21 languages. + * + * @component + * @example + * <TriggerNode + * data={{ label: "Manual Trigger", height: 100, width: 150, isSelected: false }} + * id="trigger-1" + * /> + * + * @param {NodeProps<NodeType<CustomNodeProps, "trigger">>} props + * @param {Object} props.data - Node data object + * @param {number} [props.data.height=100] - Node height in pixels (clamped: 40-1000) + * @param {number} [props.data.width=100] - Node width in pixels (clamped: 40-1000) + * @param {string} [props.data.label="Unknown"] - Trigger node label/name + * @param {boolean} [props.data.isSelected=false] - Whether the node is selected + * @returns {React.ReactElement} Rendered trigger node component + */ +export const TriggerNode = memo( + ({ + data: { + height = 100, + isSelected = false, + label = "Unknown", + width = 100, + }, + }: NodeProps<NodeType<CustomNodeProps, "trigger">>) => { + const { t } = useTranslation("components"); + + // Validate and sanitize inputs + const validHeight = Math.max(40, Math.min(Math.floor(height || 100), 1000)); + const validWidth = Math.max(40, Math.min(Math.floor(width || 100), 1000)); + const validLabel = (label || "") + .trim() + .slice(0, 100) || "Unknown"; // Max 100 chars, prevent XSS Review Comment: `useGraphLayout` computes `node.width/node.height` for ELK layout; the node components generally render using those exact values. Here the width/height are clamped (max 1000) and floored, and the label is truncated to 100 chars. Any divergence from the layout-provided dimensions/label can cause mismatches (overlaps/edge routing issues) and silently changes what the user sees. Consider rendering with the provided `height/width` directly and rely on CSS ellipsis + `title` (without slicing/clamping). ########## airflow-core/src/airflow/ui/src/components/Graph/TriggerNode.tsx: ########## @@ -0,0 +1,137 @@ +/*! + * 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 { Flex, HStack, Text } from "@chakra-ui/react"; +import type { NodeProps, Node as NodeType } from "@xyflow/react"; +import { memo } from "react"; +import { useTranslation } from "react-i18next"; +import { FiZap } from "react-icons/fi"; + +import { NodeWrapper } from "./NodeWrapper"; +import type { CustomNodeProps } from "./reactflowUtils"; + +/** + * Trigger node component for DAG graph visualization. + * + * Displays a trigger task node in the DAG graph with a lightning bolt icon, + * node label, and descriptive subtitle. Supports internationalization for + * 21 languages. + * + * @component + * @example + * <TriggerNode + * data={{ label: "Manual Trigger", height: 100, width: 150, isSelected: false }} + * id="trigger-1" + * /> + * + * @param {NodeProps<NodeType<CustomNodeProps, "trigger">>} props + * @param {Object} props.data - Node data object + * @param {number} [props.data.height=100] - Node height in pixels (clamped: 40-1000) + * @param {number} [props.data.width=100] - Node width in pixels (clamped: 40-1000) + * @param {string} [props.data.label="Unknown"] - Trigger node label/name + * @param {boolean} [props.data.isSelected=false] - Whether the node is selected + * @returns {React.ReactElement} Rendered trigger node component + */ +export const TriggerNode = memo( + ({ + data: { + height = 100, + isSelected = false, + label = "Unknown", + width = 100, + }, + }: NodeProps<NodeType<CustomNodeProps, "trigger">>) => { + const { t } = useTranslation("components"); Review Comment: New node type/component behavior is being introduced (rendering `trigger` nodes in graphs), but there’s no test coverage to ensure it renders and uses the expected translation key. Please add a small unit test for `TriggerNode` (similar to other UI component tests) to prevent regressions where an unregistered/misrendered node blanks the graph again. ########## airflow-core/src/airflow/ui/src/components/Graph/TriggerNode.tsx: ########## @@ -0,0 +1,137 @@ +/*! + * 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 { Flex, HStack, Text } from "@chakra-ui/react"; +import type { NodeProps, Node as NodeType } from "@xyflow/react"; +import { memo } from "react"; +import { useTranslation } from "react-i18next"; +import { FiZap } from "react-icons/fi"; + +import { NodeWrapper } from "./NodeWrapper"; +import type { CustomNodeProps } from "./reactflowUtils"; + +/** + * Trigger node component for DAG graph visualization. + * + * Displays a trigger task node in the DAG graph with a lightning bolt icon, + * node label, and descriptive subtitle. Supports internationalization for + * 21 languages. Review Comment: This JSDoc claims “Supports internationalization for 21 languages.” That’s brittle (the number of locales can change) and currently inaccurate since this PR’s locale changes appear to only add the key in `en` (and some strings here are still hard-coded English). Suggest rephrasing to just state it uses i18n translation keys. ########## airflow-core/src/airflow/ui/public/i18n/locales/en/components.json: ########## @@ -91,7 +91,8 @@ "otherDagRuns": "+Other Dag Runs", "taskCount_one": "{{count}} Task", "taskCount_other": "{{count}} Tasks", - "taskGroup": "Task Group" + "taskGroup": "Task Group", + "triggerDagRun": "TriggerDagRunOperator" Review Comment: PR description says the new `graph.triggerDagRun` key was added to all 21 locale files, but a repo-wide search only finds it in `locales/en/components.json`. Either update the remaining locale `components.json` files to include the key (even if temporarily in English) or adjust the PR description accordingly. ########## airflow-core/src/airflow/ui/src/components/Graph/TriggerNode.tsx: ########## @@ -0,0 +1,137 @@ +/*! + * 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 { Flex, HStack, Text } from "@chakra-ui/react"; +import type { NodeProps, Node as NodeType } from "@xyflow/react"; +import { memo } from "react"; +import { useTranslation } from "react-i18next"; +import { FiZap } from "react-icons/fi"; + +import { NodeWrapper } from "./NodeWrapper"; +import type { CustomNodeProps } from "./reactflowUtils"; + +/** + * Trigger node component for DAG graph visualization. + * + * Displays a trigger task node in the DAG graph with a lightning bolt icon, + * node label, and descriptive subtitle. Supports internationalization for + * 21 languages. + * + * @component + * @example + * <TriggerNode + * data={{ label: "Manual Trigger", height: 100, width: 150, isSelected: false }} + * id="trigger-1" + * /> + * + * @param {NodeProps<NodeType<CustomNodeProps, "trigger">>} props + * @param {Object} props.data - Node data object + * @param {number} [props.data.height=100] - Node height in pixels (clamped: 40-1000) + * @param {number} [props.data.width=100] - Node width in pixels (clamped: 40-1000) + * @param {string} [props.data.label="Unknown"] - Trigger node label/name + * @param {boolean} [props.data.isSelected=false] - Whether the node is selected + * @returns {React.ReactElement} Rendered trigger node component + */ +export const TriggerNode = memo( + ({ + data: { + height = 100, + isSelected = false, + label = "Unknown", + width = 100, + }, + }: NodeProps<NodeType<CustomNodeProps, "trigger">>) => { + const { t } = useTranslation("components"); + + // Validate and sanitize inputs + const validHeight = Math.max(40, Math.min(Math.floor(height || 100), 1000)); + const validWidth = Math.max(40, Math.min(Math.floor(width || 100), 1000)); + const validLabel = (label || "") + .trim() + .slice(0, 100) || "Unknown"; // Max 100 chars, prevent XSS + + // Get translated text with fallback + const triggerLabel = t("graph.triggerDagRun", { + defaultValue: "Trigger DAG Run", + }); + + return ( + <NodeWrapper> + <Flex + bg="bg" + borderColor={isSelected ? "border.inverted" : "border"} + borderRadius={5} + borderWidth={isSelected ? 4 : 2} + cursor="default" + flexDirection="column" + height={validHeight} + px={3} + py={1} + width={validWidth} + role="article" + aria-selected={isSelected} + aria-label={`${validLabel} trigger node, ${isSelected ? "selected" : "not selected"}`} + data-testid="trigger-node" + data-label={validLabel} + data-selected={isSelected} + > + <HStack alignItems="center" data-testid="trigger-node-header"> + <FiZap + aria-hidden="true" + data-testid="trigger-icon" + title="Trigger node indicator" + /> Review Comment: The `aria-label` and the icon `title` are hard-coded English strings, so they won’t be localized even though this component is meant to be i18n-ready. Prefer using `t(...)` for any user-facing/accessibility text (and you can likely drop the “selected/not selected” text since `aria-selected` already communicates state). -- 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]
