pierrejeambrun commented on code in PR #68544: URL: https://github.com/apache/airflow/pull/68544#discussion_r3492446434
########## airflow-core/src/airflow/ui/src/pages/DagsList/DagFolderTree/DagFolderTree.tsx: ########## @@ -0,0 +1,178 @@ +/*! + * 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 { Box, Flex, Heading, Skeleton, Text, VStack } from "@chakra-ui/react"; +import { useMemo, useState } from "react"; +import { useTranslation } from "react-i18next"; +import { FiChevronDown, FiChevronRight, FiFolder } from "react-icons/fi"; + +import { buildFolderTree, type FolderNode } from "./buildFolderTree"; + +type Props = { + readonly folders: ReadonlyArray<string>; + readonly isLoading?: boolean; + readonly onSelectFolder: (path: string | undefined) => void; + readonly selectedFolder: string | undefined; +}; + +// Every ancestor folder of the selected path, so the tree opens to reveal the selection. +const ancestorPaths = (folder: string | undefined): Array<string> => { + if (folder === undefined || folder === "") { + return []; + } + + const segments = folder.split("/"); + + return segments.map((_, index) => segments.slice(0, index + 1).join("/")); +}; + +type RowProps = { + readonly expanded: Set<string>; + readonly node: FolderNode; + readonly onSelectFolder: (path: string | undefined) => void; + readonly onToggle: (path: string) => void; + readonly selectedFolder: string | undefined; +}; + +const FolderRow = ({ expanded, node, onSelectFolder, onToggle, selectedFolder }: RowProps) => { + const hasChildren = node.children.length > 0; + const isExpanded = expanded.has(node.path); + const isSelected = selectedFolder === node.path; + const depth = node.path.split("/").length - 1; + + return ( + <Box> + <Flex + _hover={{ bg: isSelected ? "blue.subtle" : "bg.muted" }} + alignItems="center" + bg={isSelected ? "blue.subtle" : undefined} + borderRadius="sm" + cursor="pointer" + gap={1} + onClick={() => onSelectFolder(node.path)} + pl={`${depth * 16 + 4}px`} + py={1} + > + <Box + aria-label={isExpanded ? "Collapse" : "Expand"} + as="span" + color="fg.muted" + onClick={(event) => { + event.stopPropagation(); + if (hasChildren) { + onToggle(node.path); + } + }} + visibility={hasChildren ? "visible" : "hidden"} + > + {isExpanded ? <FiChevronDown /> : <FiChevronRight />} + </Box> + <Box as="span" color="fg.muted"> + <FiFolder /> + </Box> + <Text fontWeight={isSelected ? "bold" : "normal"} truncate> + {node.name} + </Text> + </Flex> + {hasChildren && isExpanded ? ( + <Box> + {node.children.map((child) => ( + <FolderRow + expanded={expanded} + key={child.path} + node={child} + onSelectFolder={onSelectFolder} + onToggle={onToggle} + selectedFolder={selectedFolder} + /> + ))} + </Box> + ) : undefined} + </Box> + ); +}; + +export const DagFolderTree = ({ folders, isLoading = false, onSelectFolder, selectedFolder }: Props) => { + const { t: translate } = useTranslation("dags"); + const tree = useMemo(() => buildFolderTree(folders), [folders]); Review Comment: `useMemo` probably not needed thanks to `react-compiler`. Repo convention is 'no manual useMemo, useCallback'. ########## airflow-core/src/airflow/api_fastapi/core_api/routes/ui/dags.py: ########## @@ -253,6 +258,36 @@ def get_dags( ) +@dags_router.get( + "/folders", + dependencies=[Depends(requires_access_dag(method="GET"))], + operation_id="get_dag_folders", +) +def get_dag_folders( + readable_dags_filter: ReadableDagsFilterDep, + session: SessionDep, +) -> DagFolderCollectionResponse: + """ + Get the distinct folders the readable Dags live in. Review Comment: Since this is a 'list' endpoint but it isn't paginated (in comparison to other list endpoint), I'm curious how this could go out of hands if there are a high number of dags. Should we set a manual limit here just in case? ########## airflow-core/tests/unit/api_fastapi/core_api/routes/ui/test_dags.py: ########## @@ -418,3 +418,92 @@ def test_is_favorite_field_user_specific(self, test_client, session): # Verify that DAG1 is not marked as favorite for the test user dag1_data = next(dag for dag in body["dags"] if dag["dag_id"] == DAG1_ID) assert dag1_data["is_favorite"] is False + + +# Maps dag_id -> relative_fileloc. ``team_alpha`` exists to prove that a folder +# name is never matched as a prefix of another (``team_a`` must not catch it), +# and ``root_dag.py`` lives at the bundle root (no folder). +FOLDER_DAGS = { + "folder_dag_a_etl_extract": "team_a/etl/extract.py", + "folder_dag_a_etl_load": "team_a/etl/load.py", + "folder_dag_a_report": "team_a/report.py", + "folder_dag_b_ml_train": "team_b/ml/train.py", + "folder_dag_alpha": "team_alpha/x.py", + "folder_dag_root": "root_dag.py", +} + + +class TestDagFolders(TestPublicDagEndpoint): Review Comment: For tests, add an `assert_queries_count` just to lock in that this endpoint should only make a finite number of request (a couple for auth, + 1 for the payload), and should never scale with the number of dags. -- 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]
