kaxil commented on code in PR #63477: URL: https://github.com/apache/airflow/pull/63477#discussion_r2927554568
########## airflow-core/src/airflow/ui/src/hooks/useHITLReviewTabs.ts: ########## @@ -0,0 +1,105 @@ +/*! + * 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 { useQuery } from "@tanstack/react-query"; + +type TabItem = { + icon: React.ReactNode; + label: string; + value: string; +}; + +const HITL_REVIEW_PLUGIN_TAB = "plugin/hitl-review"; + +export type UseHITLReviewTabsOptions = { + enabled?: boolean; + mapIndex?: number; + refetchInterval?: number | false; +}; + +export const filterHITLReviewTabs = (tabs: Array<TabItem>, hasHitlData: boolean): Array<TabItem> => + tabs.filter((tab) => tab.value !== HITL_REVIEW_PLUGIN_TAB || hasHitlData); + +const getBasePath = () => { + const baseHref = document.querySelector("head > base")?.getAttribute("href") ?? ""; + const baseUrl = new URL(baseHref, globalThis.location.origin); + + return new URL(baseUrl).pathname.replace(/\/$/u, ""); +}; + +const buildHITLReviewSessionUrl = ({ + dagId, + dagRunId, + mapIndex, + taskId, +}: { + dagId: string; + dagRunId: string; + mapIndex: number; + taskId: string; +}) => { + const searchParams = new URLSearchParams({ + dag_id: dagId, + map_index: mapIndex.toString(), + run_id: dagRunId, + task_id: taskId, + }); + + return `${getBasePath()}/hitl-review/sessions/find?${searchParams.toString()}`; +}; + +export const useHITLReviewTabs = ( + { + dagId, + dagRunId, + taskId, + }: { + dagId: string; + dagRunId: string; + taskId: string; + }, + tabs: Array<TabItem>, + options: UseHITLReviewTabsOptions = {}, +) => { + const { enabled = true, mapIndex = -1, refetchInterval } = options; + + const { data: hasHITLReviewSession = false, isLoading: isLoadingHITLReviewSession } = useQuery({ + enabled, Review Comment: The hook fires a `fetch()` on every task instance page load, even when the HITL review plugin isn't installed (no `plugin/hitl-review` tab in the array). Most Airflow deployments don't use HITL, so this is wasted traffic. You could skip the request entirely when the tab doesn't exist: ```ts const hasHitlTab = tabs.some((tab) => tab.value === HITL_REVIEW_PLUGIN_TAB); // in useQuery: enabled: enabled && hasHitlTab, ``` ########## airflow-core/src/airflow/ui/src/hooks/useHITLReviewTabs.ts: ########## @@ -0,0 +1,105 @@ +/*! + * 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 { useQuery } from "@tanstack/react-query"; + +type TabItem = { + icon: React.ReactNode; Review Comment: This `TabItem` type is identical to the one exported from `useRequiredActionTabs.ts`. Could import it from there to avoid drift: ```ts import type { TabItem } from "src/hooks/useRequiredActionTabs"; ``` ########## airflow-core/src/airflow/ui/src/hooks/useHITLReviewTabs.ts: ########## @@ -0,0 +1,105 @@ +/*! + * 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 { useQuery } from "@tanstack/react-query"; + +type TabItem = { + icon: React.ReactNode; + label: string; + value: string; +}; + +const HITL_REVIEW_PLUGIN_TAB = "plugin/hitl-review"; + +export type UseHITLReviewTabsOptions = { + enabled?: boolean; + mapIndex?: number; + refetchInterval?: number | false; +}; + +export const filterHITLReviewTabs = (tabs: Array<TabItem>, hasHitlData: boolean): Array<TabItem> => + tabs.filter((tab) => tab.value !== HITL_REVIEW_PLUGIN_TAB || hasHitlData); + +const getBasePath = () => { + const baseHref = document.querySelector("head > base")?.getAttribute("href") ?? ""; + const baseUrl = new URL(baseHref, globalThis.location.origin); + + return new URL(baseUrl).pathname.replace(/\/$/u, ""); +}; + +const buildHITLReviewSessionUrl = ({ + dagId, + dagRunId, + mapIndex, + taskId, +}: { + dagId: string; + dagRunId: string; + mapIndex: number; + taskId: string; +}) => { + const searchParams = new URLSearchParams({ + dag_id: dagId, + map_index: mapIndex.toString(), + run_id: dagRunId, + task_id: taskId, + }); + + return `${getBasePath()}/hitl-review/sessions/find?${searchParams.toString()}`; +}; + +export const useHITLReviewTabs = ( + { + dagId, + dagRunId, + taskId, + }: { + dagId: string; + dagRunId: string; + taskId: string; + }, + tabs: Array<TabItem>, + options: UseHITLReviewTabsOptions = {}, +) => { + const { enabled = true, mapIndex = -1, refetchInterval } = options; + + const { data: hasHITLReviewSession = false, isLoading: isLoadingHITLReviewSession } = useQuery({ + enabled, + queryFn: async () => { Review Comment: This uses raw `fetch()` instead of `axios`. The Airflow UI has an axios response interceptor (in `main.tsx`) that handles 401/403 by redirecting to the login page. Raw `fetch` bypasses that entirely, so if the user's session expires while on a task instance page, this request will fail silently without triggering a login redirect. Could this use `axios` or the `OpenAPI`-based client to stay consistent with the rest of the UI? ########## airflow-core/src/airflow/ui/src/hooks/useHITLReviewTabs.ts: ########## @@ -0,0 +1,105 @@ +/*! + * 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 { useQuery } from "@tanstack/react-query"; + +type TabItem = { + icon: React.ReactNode; + label: string; + value: string; +}; + +const HITL_REVIEW_PLUGIN_TAB = "plugin/hitl-review"; + +export type UseHITLReviewTabsOptions = { + enabled?: boolean; + mapIndex?: number; + refetchInterval?: number | false; +}; + +export const filterHITLReviewTabs = (tabs: Array<TabItem>, hasHitlData: boolean): Array<TabItem> => + tabs.filter((tab) => tab.value !== HITL_REVIEW_PLUGIN_TAB || hasHitlData); + +const getBasePath = () => { + const baseHref = document.querySelector("head > base")?.getAttribute("href") ?? ""; + const baseUrl = new URL(baseHref, globalThis.location.origin); Review Comment: `getBasePath()` reimplements the base URL resolution that already exists in several places (`getRedirectPath` in `src/utils/links.ts`, `OpenAPI.BASE` in `queryClient.ts`, the i18n config). Also, `new URL(baseUrl)` on line 42 wraps an already-constructed `URL` object unnecessarily. Could this use `OpenAPI.BASE` from `openapi/requests/core/OpenAPI` directly? It's already set to the trailing-slash-stripped base path. ########## airflow-core/src/airflow/ui/src/hooks/useHITLReviewTabs.ts: ########## @@ -0,0 +1,105 @@ +/*! + * 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 { useQuery } from "@tanstack/react-query"; + +type TabItem = { + icon: React.ReactNode; + label: string; + value: string; +}; + +const HITL_REVIEW_PLUGIN_TAB = "plugin/hitl-review"; + +export type UseHITLReviewTabsOptions = { + enabled?: boolean; + mapIndex?: number; + refetchInterval?: number | false; +}; + +export const filterHITLReviewTabs = (tabs: Array<TabItem>, hasHitlData: boolean): Array<TabItem> => + tabs.filter((tab) => tab.value !== HITL_REVIEW_PLUGIN_TAB || hasHitlData); + +const getBasePath = () => { + const baseHref = document.querySelector("head > base")?.getAttribute("href") ?? ""; + const baseUrl = new URL(baseHref, globalThis.location.origin); + + return new URL(baseUrl).pathname.replace(/\/$/u, ""); +}; + +const buildHITLReviewSessionUrl = ({ + dagId, + dagRunId, + mapIndex, + taskId, +}: { + dagId: string; + dagRunId: string; + mapIndex: number; + taskId: string; +}) => { + const searchParams = new URLSearchParams({ + dag_id: dagId, + map_index: mapIndex.toString(), + run_id: dagRunId, + task_id: taskId, + }); + + return `${getBasePath()}/hitl-review/sessions/find?${searchParams.toString()}`; +}; + +export const useHITLReviewTabs = ( + { + dagId, + dagRunId, + taskId, + }: { + dagId: string; + dagRunId: string; + taskId: string; + }, + tabs: Array<TabItem>, + options: UseHITLReviewTabsOptions = {}, +) => { + const { enabled = true, mapIndex = -1, refetchInterval } = options; + + const { data: hasHITLReviewSession = false, isLoading: isLoadingHITLReviewSession } = useQuery({ + enabled, + queryFn: async () => { + const response = await fetch(buildHITLReviewSessionUrl({ dagId, dagRunId, mapIndex, taskId })); + + if (response.ok) { + return true; + } + + if (response.status === 404) { Review Comment: When this throws for non-200/non-404 responses, the error is a plain `Error` with no `status` property. The global `QueryClient` retry function (in `queryClient.ts`) checks `error.status` to skip retries for 4xx errors, but since `status` is `undefined` here, it'll retry 3 times for 401/403 responses. Worth handling 401/403 explicitly (return `false`) or attaching the status code to the error. ########## airflow-core/src/airflow/ui/src/pages/TaskInstance/TaskInstance.tsx: ########## @@ -98,11 +99,18 @@ export const TaskInstance = () => { ]; } - const { tabs: displayTabs } = useRequiredActionTabs({ dagId, dagRunId: runId, taskId }, newTabs, { + const { tabs: requiredActionTabs } = useRequiredActionTabs({ dagId, dagRunId: runId, taskId }, newTabs, { autoRedirect: true, refetchInterval: isStatePending(taskInstance?.state) ? refetchInterval : false, }); Review Comment: Nit: the `// HITL review tab` comment doesn't add much since the function name already communicates that. Not blocking. -- 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]
