Copilot commented on code in PR #64404: URL: https://github.com/apache/airflow/pull/64404#discussion_r3025325419
########## airflow-core/src/airflow/ui/src/pages/TaskInstance/ExtraLinks.test.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 "@testing-library/jest-dom"; +import { render, screen } from "@testing-library/react"; +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { useParams } from "react-router-dom"; + +import * as queries from "openapi/queries"; +import { Wrapper } from "src/utils/Wrapper"; + +import { ExtraLinks } from "./ExtraLinks"; + +vi.mock("openapi/queries"); +vi.mock("react-router-dom", async () => { + const actual = await vi.importActual("react-router-dom"); + + return { + ...actual, + useParams: vi.fn(), + }; +}); + +describe("ExtraLinks Component", () => { + beforeEach(() => { + vi.clearAllMocks(); + vi.mocked(useParams).mockReturnValue({ + dagId: "test-dag", + mapIndex: "-1", + runId: "test-run", + taskId: "test-task", + }); + }); + + it("renders internal links with target='_self'", async () => { + vi.mocked(queries.useTaskInstanceServiceGetExtraLinks).mockReturnValue({ + data: { + extra_links: { + "Internal Link": "/dags/test/runs/run1", + "Same Origin": "http://localhost:3000/some-path";, + }, + }, + } as any); + + // Mock window.location.origin + vi.stubGlobal("location", { origin: "http://localhost:3000"; }); Review Comment: These tests stub a global (`location`) but don’t restore it afterward. That can leak state across tests and cause order-dependent failures. Add cleanup (e.g., `afterEach(() => vi.unstubAllGlobals())` or restore the original `location` explicitly) so the global environment is reset after each test. ########## airflow-core/src/airflow/ui/src/pages/TaskInstance/ExtraLinks.tsx: ########## @@ -47,14 +47,22 @@ export const ExtraLinks = ({ refetchInterval }: ExtraLinksProps) => { <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 : ( + {Object.entries(data.extra_links).map(([key, url]) => + url ? ( <Button asChild colorPalette="brand" key={key} variant="surface"> - <a href={value} rel="noopener noreferrer" target="_blank"> + <a + href={url} + rel="noopener noreferrer" + target={ + url.startsWith("http") && !url.startsWith(window.location.origin) + ? "_blank" + : "_self" Review Comment: `startsWith(window.location.origin)` is not a safe same-origin check. A URL like `http://localhost:3000.evil.com/...` will pass the prefix test and be treated as same-origin, causing external content to open in the same tab. Prefer parsing with `new URL(url, window.location.origin)` and comparing `.origin` equality (and optionally restricting to `http:`/`https:`) to classify external vs internal reliably. ########## airflow-core/src/airflow/ui/src/pages/TaskInstance/ExtraLinks.tsx: ########## @@ -47,14 +47,22 @@ export const ExtraLinks = ({ refetchInterval }: ExtraLinksProps) => { <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 : ( + {Object.entries(data.extra_links).map(([key, url]) => + url ? ( <Button asChild colorPalette="brand" key={key} variant="surface"> - <a href={value} rel="noopener noreferrer" target="_blank"> + <a + href={url} + rel="noopener noreferrer" + target={ + url.startsWith("http") && !url.startsWith(window.location.origin) + ? "_blank" + : "_self" + } + > Review Comment: The PR description says the REST API returns structured link objects `{url, target}` and the UI should honor the configured `target`, but this UI code still assumes `extra_links` values are URL strings and derives `target` heuristically. This is a behavioral mismatch and will also break if the API response shape has changed as described. Update the UI to consume `{ url, target }` (and fall back for backward compatibility if needed), and apply the server-provided `target` rather than recomputing it. ########## airflow-core/newsfragments/64404.feature.rst: ########## @@ -0,0 +1 @@ +Extra links now determine whether to open in the same tab or a new tab based on the URL; internal links use `_self` and external links use `_blank`. Review Comment: This newsfragment documents URL-based target selection, but the PR description states the goal is a *configurable* `target` propagated through serialization/REST/UI. If the intended behavior is “configured target is honored,” the fragment should reflect that; if the intent changed to URL-based heuristics, the PR description should be updated accordingly so release notes match the actual feature. -- 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]
