kaxil commented on code in PR #74471: URL: https://github.com/apache/airflow/pull/74471#discussion_r4223373630
########## airflow-core/src/airflow/ui/src/components/SearchSelect.tsx: ########## @@ -0,0 +1,110 @@ +/*! + * 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 { ReactNode } from "react"; + +import { Box, Field } from "@chakra-ui/react"; +import type { + ChakraStylesConfig, + ControlProps, + GroupBase, + OptionsOrGroups, + SingleValue, +} from "chakra-react-select"; +import { AsyncSelect, chakraComponents } from "chakra-react-select"; +import { FiSearch } from "react-icons/fi"; + +/** + * Leads the input with the search affordance. react-select only renders indicators after the value + * container, so an icon on the start side has to come from the control itself. + */ +const SearchControl = <Option,>({ children, ...props }: ControlProps<Option, false>) => ( + <chakraComponents.Control {...props}> + <Box alignItems="center" as="span" color="fg.muted" display="flex" flexShrink={0} pe={1.5}> + <FiSearch /> + </Box> + {children} + </chakraComponents.Control> +); + +/** `DropdownIndicator` accepts `null` to opt out; `LoadingIndicator` only accepts a component. */ +const NoIndicator = () => null; + +type Props<Option> = { + /** + * The options to list before anything is typed. Always an array, never `true`: `true` makes + * react-select call `loadOptions` from a mount effect and start out `isLoading`, so every open + * of a panel that unmounts on close is a spinner over an empty list. A fresh array replaces what + * is listed, which is how a live query keeps an open panel current. + */ + readonly defaultOptions: Array<Option>; + readonly formatOptionLabel: (option: Option) => ReactNode; + readonly loadOptions: ( + inputValue: string, + callback: (options: OptionsOrGroups<Option, GroupBase<Option>>) => void, + ) => void; + readonly onChange: (selected: SingleValue<Option>) => void; + readonly placeholder: string; +}; + +/** + * The search a breadcrumb switcher drops open. Its results are listed from the moment it mounts, + * since the panel exists only to show them. + */ +export const SearchSelect = <Option,>({ + defaultOptions, + formatOptionLabel, + loadOptions, + onChange, + placeholder, +}: Props<Option>) => { + // The popover is the card. Drop the floating menu's own positioning and chrome so the results + // flow inside it directly under the input, instead of reading as a second card. + const chakraStyles: ChakraStylesConfig<Option, false, GroupBase<Option>> = { + menu: () => ({ marginTop: 2, width: "100%" }), + menuList: (provided) => ({ + ...provided, + background: "transparent", + borderRadius: 0, + boxShadow: "none", + paddingInline: 0, + zIndex: "auto", + }), + }; + + return ( + <Field.Root> + <AsyncSelect<Option> + backspaceRemovesValue={true} + chakraStyles={chakraStyles} + // No loading affordance anywhere: the listed options are in hand before the panel opens, + // and a search replaces them once it answers rather than blanking them first. + components={{ Control: SearchControl, DropdownIndicator: null, LoadingIndicator: NoIndicator }} + defaultOptions={defaultOptions} + filterOption={undefined} + formatOptionLabel={formatOptionLabel} + loadingMessage={() => null} Review Comment: I don't think this matches the comment above. On the first keystroke (and the first one after clearing the input) react-select's `useAsync` sets `passEmptyOptions` because nothing has loaded yet, so `options` is `[]` while the request is in flight. With no options and `isLoading` true, `renderMenu` calls `loadingMessage`, and `null` here makes it render no menu at all. So the listed runs vanish for the 300ms debounce plus the request. Dropping this override would at least show a loading message in that window (an array `defaultOptions` never starts out loading), and the comment on line 95 would need updating either way. ########## airflow-core/src/airflow/ui/src/layouts/Details/DagRunSwitcherButton.tsx: ########## @@ -0,0 +1,76 @@ +/*! + * 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 ReactNode, useState } from "react"; + +import { useTranslation } from "react-i18next"; + +import { useDagRunServiceGetDagRuns } from "openapi/queries"; + +import { CrumbSwitcher, type CrumbShape } from "src/components/Breadcrumb"; + +import { useAutoRefresh } from "src/utils"; +import type { DagRunSearchOption } from "src/utils/option"; + +import { SearchDagRuns } from "./SearchDagRuns"; +import { NEWEST_FIRST, SEARCH_LIMIT, buildDagRunOption } from "./searchOptions"; + +const NO_RUNS: Array<DagRunSearchOption> = []; + +type Props = { + readonly children: ReactNode; + readonly dagId: string; + readonly shape: CrumbShape; + readonly to: string; +}; + +/** + * The Dag run level of the breadcrumb, with a search over the Dag's runs behind its chevron. It + * stands in for both a named run and the "all runs" level, so `to` comes from the crumb itself. + * + * The runs the panel lists are loaded here rather than inside it: a query that only starts when + * the panel opens has nothing to show until it answers. Here it is already loaded by the time + * anyone opens the panel, and the page's auto-refresh keeps it that way. Unlike the grid, this + * list has to notice runs that do not exist yet, so `checkPendingRuns` scales the interval back + * once they have all finished rather than stopping. + */ +export const DagRunSwitcherButton = ({ children, dagId, shape, to }: Props) => { + const { t: translate } = useTranslation(); + const [open, setOpen] = useState(false); + const refetchInterval = useAutoRefresh({ checkPendingRuns: true, dagId }); + const { data } = useDagRunServiceGetDagRuns( + { dagId, limit: SEARCH_LIMIT, orderBy: NEWEST_FIRST }, + undefined, + { refetchInterval }, + ); + const runs = data === undefined ? NO_RUNS : data.dag_runs.map(buildDagRunOption); Review Comment: This builds new option objects on every render. react-select keeps keyboard focus only if the focused option is still in `options` by reference (`getNextFocusedOption`), otherwise it jumps to the first one. So if someone arrows down to the fifth run and a poll lands with a changed run state, Enter opens the newest run instead. Building the options in the query's `select` would keep unchanged items as the same objects, since TanStack structurally shares `select` results. ########## airflow-core/src/airflow/ui/src/layouts/Details/SearchDagRuns.tsx: ########## @@ -0,0 +1,120 @@ +/*! + * 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, Text } from "@chakra-ui/react"; +import { useQueryClient } from "@tanstack/react-query"; +import type { GroupBase, OptionsOrGroups, SingleValue } from "chakra-react-select"; +import { useTranslation } from "react-i18next"; +import { useNavigate, useParams } from "react-router-dom"; +import { useDebouncedCallback } from "use-debounce"; + +import { UseDagRunServiceGetDagRunsKeyFn } from "openapi/queries"; +import { DagRunService } from "openapi/requests/services.gen"; +import type { DAGRunCollectionResponse } from "openapi/requests/types.gen"; + +import { SearchSelect } from "src/components/SearchSelect"; +import { StateBadge } from "src/components/StateBadge"; + +import type { DagRunSearchOption } from "src/utils/option"; + +import { NEWEST_FIRST, SEARCH_LIMIT, buildDagRunOption } from "./searchOptions"; + +const formatOptionLabel = (option: DagRunSearchOption) => ( + <Flex alignItems="center" gap={2} minW={0}> + <StateBadge flexShrink={0} state={option.state} /> + <Text truncate>{option.label}</Text> + </Flex> +); + +/** + * Carries the task level across the switch so the same task stays in view in the run picked. A map + * index is dropped: the same expansion is not guaranteed to exist in another run. + */ +const buildTaskPath = (groupId: string | undefined, taskId: string | undefined) => { Review Comment: On a mapped task this lands on a page that 404s. From `.../tasks/t/mapped` or `.../mapped/3` (also a task inside a mapped group) the switch goes to `.../runs/r2/tasks/t`, which renders `TaskInstance` with `map_index=-1`. An expanded task has no such row, so the page shows "No task instance found". Same from the Dag-level `/dags/d/tasks/t` page of a mapped task. It also drops the TI tab (`/xcom`, `/rendered_templates`, plugin tabs), which Grid clicks and keyboard nav keep. Could this use `buildTaskInstanceUrl({ currentPathname, dagId, runId: selected.value, taskId, isGroup, isMapped })` with no `mapIndex`? That lands on the new run's `/mapped` list and keeps the tab otherwise. `isMapped` could come down from the breadcrumb's `hasExpandedInstances`. The "drops the map index" test in `SearchDagRuns.test.tsx` currently asserts the 404 URL, so it would expect `/tasks/task_1/mapped` instead. ########## airflow-core/src/airflow/ui/src/layouts/Details/DagRunSwitcherButton.tsx: ########## @@ -0,0 +1,76 @@ +/*! + * 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 ReactNode, useState } from "react"; + +import { useTranslation } from "react-i18next"; + +import { useDagRunServiceGetDagRuns } from "openapi/queries"; + +import { CrumbSwitcher, type CrumbShape } from "src/components/Breadcrumb"; + +import { useAutoRefresh } from "src/utils"; +import type { DagRunSearchOption } from "src/utils/option"; + +import { SearchDagRuns } from "./SearchDagRuns"; +import { NEWEST_FIRST, SEARCH_LIMIT, buildDagRunOption } from "./searchOptions"; + +const NO_RUNS: Array<DagRunSearchOption> = []; + +type Props = { + readonly children: ReactNode; + readonly dagId: string; + readonly shape: CrumbShape; + readonly to: string; +}; + +/** + * The Dag run level of the breadcrumb, with a search over the Dag's runs behind its chevron. It + * stands in for both a named run and the "all runs" level, so `to` comes from the crumb itself. + * + * The runs the panel lists are loaded here rather than inside it: a query that only starts when + * the panel opens has nothing to show until it answers. Here it is already loaded by the time + * anyone opens the panel, and the page's auto-refresh keeps it that way. Unlike the grid, this + * list has to notice runs that do not exist yet, so `checkPendingRuns` scales the interval back + * once they have all finished rather than stopping. + */ +export const DagRunSwitcherButton = ({ children, dagId, shape, to }: Props) => { + const { t: translate } = useTranslation(); + const [open, setOpen] = useState(false); + const refetchInterval = useAutoRefresh({ checkPendingRuns: true, dagId }); + const { data } = useDagRunServiceGetDagRuns( + { dagId, limit: SEARCH_LIMIT, orderBy: NEWEST_FIRST }, + undefined, + { refetchInterval }, Review Comment: `useAutoRefresh({ checkPendingRuns: true })` returns `false` once nothing is queued or running (`utils/query.ts`), so this list stops refetching rather than scaling back as the docstring says. Only the internal pending-runs probe slows to 10x. A run that starts and finishes between two probe polls won't show up until something else refetches, and the last run to finish can keep its running badge. In the other direction, while any run is active every Dag page now polls `getDagRuns(limit=10)` on the refresh interval even if the panel is never opened. Would `refetchInterval: open && refetchInterval` plus a refetch when the panel opens cover both? -- 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]
