bbovenzi commented on code in PR #61113:
URL: https://github.com/apache/airflow/pull/61113#discussion_r2742789058
##########
airflow-core/src/airflow/ui/src/pages/TaskInstance/ExtraLinks.tsx:
##########
@@ -39,24 +39,55 @@ export const ExtraLinks = ({ refetchInterval }:
ExtraLinksProps) => {
},
undefined,
{
- refetchInterval,
+ // refetchInterval,
+ refetchInterval: refetchInterval === false ? false : (refetchInterval ??
1000), // polling every 1 second
+ refetchIntervalInBackground: true,
+ refetchOnWindowFocus: true,
},
);
- return data && Object.keys(data.extra_links).length > 0 ? (
+ // Show section if there are extra links OR if we're loading/polling
+ const hasExtraLinks = data && Object.keys(data.extra_links).length > 0;
+
+ if (!hasExtraLinks && !isLoading) {
+ return undefined;
+ }
+
+ return (
<Box py={1}>
<Heading size="sm">{translate("extraLinks")}</Heading>
<HStack gap={2} py={2}>
- {Object.entries(data.extra_links).map(([key, value], _) =>
- value === null ? undefined : (
- <Button asChild colorPalette="brand" key={key} variant="surface">
- <a href={value} rel="noopener noreferrer" target="_blank">
- {key}
- </a>
- </Button>
- ),
- )}
+ {hasExtraLinks ? (
+ <>
+ {Object.entries(data.extra_links).map(([key, value]) => {
+ // If value is empty string, it means link is being resolved -
show loading
+ if (value === "") {
+ return (
+ <Button colorPalette="brand" disabled key={key}
variant="surface">
+ <Spinner mr={2} size="sm" />
+ {key}
+ </Button>
+ );
+ }
+
+ // If value is a valid URL, render as clickable link (active
button)
+ return (
+ <Button asChild colorPalette="brand" key={key}
variant="surface">
+ <a href={value ?? ""} rel="noopener noreferrer"
target="_blank">
+ {key}
+ </a>
+ </Button>
+ );
+ })}
+ </>
+ ) : undefined}
+ {!hasExtraLinks && isLoading ? (
+ <Button colorPalette="brand" disabled variant="surface">
Review Comment:
Button has both `loading` and `loadingText` params which we should use here
instead of declaring < <Button> three different times. It also means we don't
need to manually implement a Spinner.
##########
airflow-core/src/airflow/ui/src/pages/TaskInstance/ExtraLinks.tsx:
##########
@@ -39,24 +39,55 @@ export const ExtraLinks = ({ refetchInterval }:
ExtraLinksProps) => {
},
undefined,
{
- refetchInterval,
+ // refetchInterval,
+ refetchInterval: refetchInterval === false ? false : (refetchInterval ??
1000), // polling every 1 second
Review Comment:
What are we trying to do on this line? The refetchInterval is already
`number | false` so I don't get the need for this extra logic
##########
airflow-core/src/airflow/ui/src/pages/TaskInstance/ExtraLinks.tsx:
##########
@@ -16,21 +16,21 @@
* specific language governing permissions and limitations
* under the License.
*/
-import { Box, Button, Heading, HStack } from "@chakra-ui/react";
+import { Box, Button, Heading, HStack, Spinner } from "@chakra-ui/react";
import { useTranslation } from "react-i18next";
import { useParams } from "react-router-dom";
import { useTaskInstanceServiceGetExtraLinks } from "openapi/queries";
type ExtraLinksProps = {
- readonly refetchInterval: number | false;
+ readonly refetchInterval?: number | false;
Review Comment:
Why was this made optional?
##########
airflow-core/src/airflow/ui/src/pages/TaskInstance/ExtraLinks.tsx:
##########
@@ -39,24 +39,55 @@ export const ExtraLinks = ({ refetchInterval }:
ExtraLinksProps) => {
},
undefined,
{
- refetchInterval,
+ // refetchInterval,
+ refetchInterval: refetchInterval === false ? false : (refetchInterval ??
1000), // polling every 1 second
+ refetchIntervalInBackground: true,
+ refetchOnWindowFocus: true,
Review Comment:
Can you explain why we need to change the defaults for this request in
particular?
##########
airflow-core/src/airflow/ui/src/queries/useTrigger.ts:
##########
@@ -37,29 +38,71 @@ export const useTrigger = ({ dagId, onSuccessConfirm }: {
dagId: string; onSucce
const [error, setError] = useState<unknown>(undefined);
const { t: translate } = useTranslation("components");
const navigate = useNavigate();
- const { dagId: selectedDagId } = useParams();
+ // const { dagId: selectedDagId } = useParams();
+
+ // const onSuccess = async (dagRun: TriggerDagRunResponse) => {
+ // const queryKeys = [
+ // [useDagServiceGetDagsUiKey],
+ // UseDagRunServiceGetDagRunsKeyFn({ dagId }, [{ dagId }]),
+ // UseTaskInstanceServiceGetTaskInstancesKeyFn({ dagId, dagRunId: "~" },
[{ dagId, dagRunId: "~" }]),
+ // UseGridServiceGetGridRunsKeyFn({ dagId }, [{ dagId }]),
+ // ];
+
+ // await Promise.all(queryKeys.map((key) =>
queryClient.invalidateQueries({ queryKey: key })));
+
+ // toaster.create({
+ // description: translate("triggerDag.toaster.success.description"),
+ // title: translate("triggerDag.toaster.success.title"),
+ // type: "success",
+ // });
+ // onSuccessConfirm();
+
+ // // Only redirect if we're already on the dag page
+ // if (selectedDagId === dagRun.dag_id) {
+ // navigate(`/dags/${dagRun.dag_id}/runs/${dagRun.dag_run_id}`);
+ // }
+ // };
Review Comment:
This is simply not ready for review.
--
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]
