kaxil commented on code in PR #63477: URL: https://github.com/apache/airflow/pull/63477#discussion_r2934049395
########## airflow-core/src/airflow/ui/src/hooks/useHITLReviewTabs.ts: ########## @@ -0,0 +1,91 @@ +/*! + * 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"; +import axios, { type AxiosError } from "axios"; + +import { OpenAPI } from "openapi/requests/core/OpenAPI"; +import type { TabItem } from "src/hooks/useRequiredActionTabs"; + +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); Review Comment: `filterHITLReviewTabs` is exported but only called inside this file (line 89). Can be a plain `const` without the export unless you plan to use it in tests or elsewhere. ########## airflow-core/src/airflow/ui/src/hooks/useHITLReviewTabs.ts: ########## @@ -0,0 +1,91 @@ +/*! + * 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"; +import axios, { type AxiosError } from "axios"; + +import { OpenAPI } from "openapi/requests/core/OpenAPI"; +import type { TabItem } from "src/hooks/useRequiredActionTabs"; + +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); + +export const useHITLReviewTabs = ( Review Comment: `useRequiredActionTabs` redirects users away from `required_actions` when there's no HITL data. This hook removes the `plugin/hitl-review` tab from the list but doesn't redirect if the user is already on that route. If someone bookmarks or shares a direct URL to `plugin/hitl-review`, they'll land on a page with no matching tab content. Worth considering whether a redirect is needed here too. ########## airflow-core/src/airflow/ui/src/pages/TaskInstance/TaskInstance.tsx: ########## @@ -98,11 +99,17 @@ 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, }); + const { tabs: displayTabs } = useHITLReviewTabs({ dagId, dagRunId: runId, taskId }, requiredActionTabs, { + enabled: !isNaN(parsedMapIndex), Review Comment: `enabled: !isNaN(parsedMapIndex)` is always `true` here because `mapIndex` defaults to `"-1"` and `parseInt("-1", 10)` is `-1`, which passes `!isNaN`. The check only fails for genuinely non-numeric URL params, which normal routing shouldn't produce. If the intent is "always enabled", you could omit it (the hook defaults to `true`). If it's a defensive guard for bad URLs, a comment would clarify. ########## airflow-core/src/airflow/ui/src/hooks/useHITLReviewTabs.ts: ########## @@ -0,0 +1,91 @@ +/*! + * 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"; +import axios, { type AxiosError } from "axios"; + +import { OpenAPI } from "openapi/requests/core/OpenAPI"; +import type { TabItem } from "src/hooks/useRequiredActionTabs"; + +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); + +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 hasHitlTab = tabs.some((tab) => tab.value === HITL_REVIEW_PLUGIN_TAB); + + const { data: hasHITLReviewSession = false, isLoading: isLoadingHITLReviewSession } = useQuery({ + enabled: enabled && hasHitlTab, + queryFn: () => + axios + .get(`${OpenAPI.BASE}/hitl-review/sessions/find`, { + params: { + dag_id: dagId, + map_index: mapIndex, + run_id: dagRunId, + task_id: taskId, + }, + }) + .then(() => true) + .catch((error: unknown) => { + if (!axios.isAxiosError(error)) { + return Promise.reject(error); + } + + const status = error.response?.status; + + if (status === 404) { + return false; Review Comment: This status-copying block exists because the QueryClient retry function reads `error.status`, but AxiosErrors store it under `error.response?.status`. It works, but it's fragile — if someone removes these lines not understanding why, retries silently break for 4xx responses. A short comment explaining the coupling would help, or you could throw a new error with a `status` property instead of mutating the AxiosError. -- 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]
