From b984d56e49f948cb3f4b55fab5c09d9da24a4d2c Mon Sep 17 00:00:00 2001 From: Camiel van Schoonhoven Date: Fri, 4 Sep 2026 17:10:37 -0700 Subject: [PATCH 1/3] fix: Artifact Preview blocked by CORS --- .../ArtifactPreviewContent.tsx | 8 +- .../ArtifactVisualizer/ArtifactVisualizer.tsx | 1 + .../ParquetVisualizer.test.tsx | 86 +++++++++++++++---- .../ArtifactVisualizer/ParquetVisualizer.tsx | 4 +- .../IOCell/ArtifactVisualizer/parquetUtils.ts | 73 ++++++++++------ src/routes/PipelineRun/ArtifactPreview.tsx | 1 + .../components/ArtifactComparisonDialog.tsx | 1 + 7 files changed, 128 insertions(+), 46 deletions(-) diff --git a/src/components/shared/ReactFlow/FlowCanvas/TaskNode/TaskOverview/IOSection/IOCell/ArtifactVisualizer/ArtifactPreviewContent.tsx b/src/components/shared/ReactFlow/FlowCanvas/TaskNode/TaskOverview/IOSection/IOCell/ArtifactVisualizer/ArtifactPreviewContent.tsx index 89b4bbf7af..12e066fca0 100644 --- a/src/components/shared/ReactFlow/FlowCanvas/TaskNode/TaskOverview/IOSection/IOCell/ArtifactVisualizer/ArtifactPreviewContent.tsx +++ b/src/components/shared/ReactFlow/FlowCanvas/TaskNode/TaskOverview/IOSection/IOCell/ArtifactVisualizer/ArtifactPreviewContent.tsx @@ -63,6 +63,7 @@ interface PreviewContentProps { type: string; name: string; isFullscreen: boolean; + totalSize?: number; } export const PreviewContent = ({ @@ -70,6 +71,7 @@ export const PreviewContent = ({ type, name, isFullscreen, + totalSize, }: PreviewContentProps) => { const { backendUrl } = useBackend(); @@ -106,7 +108,11 @@ export const PreviewContent = ({ ); case "apacheparquet": return ( - + ); case "jsonobject": case "jsonarray": diff --git a/src/components/shared/ReactFlow/FlowCanvas/TaskNode/TaskOverview/IOSection/IOCell/ArtifactVisualizer/ArtifactVisualizer.tsx b/src/components/shared/ReactFlow/FlowCanvas/TaskNode/TaskOverview/IOSection/IOCell/ArtifactVisualizer/ArtifactVisualizer.tsx index 0a7a6d84b8..cdeb354667 100644 --- a/src/components/shared/ReactFlow/FlowCanvas/TaskNode/TaskOverview/IOSection/IOCell/ArtifactVisualizer/ArtifactVisualizer.tsx +++ b/src/components/shared/ReactFlow/FlowCanvas/TaskNode/TaskOverview/IOSection/IOCell/ArtifactVisualizer/ArtifactVisualizer.tsx @@ -197,6 +197,7 @@ const ArtifactVisualizer = ({ artifactId={artifact.id} type={normalizedType} isFullscreen={isFullscreen} + totalSize={artifactData?.total_size} /> )} diff --git a/src/components/shared/ReactFlow/FlowCanvas/TaskNode/TaskOverview/IOSection/IOCell/ArtifactVisualizer/ParquetVisualizer.test.tsx b/src/components/shared/ReactFlow/FlowCanvas/TaskNode/TaskOverview/IOSection/IOCell/ArtifactVisualizer/ParquetVisualizer.test.tsx index 16b53b5e70..b0d0e62c04 100644 --- a/src/components/shared/ReactFlow/FlowCanvas/TaskNode/TaskOverview/IOSection/IOCell/ArtifactVisualizer/ParquetVisualizer.test.tsx +++ b/src/components/shared/ReactFlow/FlowCanvas/TaskNode/TaskOverview/IOSection/IOCell/ArtifactVisualizer/ParquetVisualizer.test.tsx @@ -6,7 +6,6 @@ import { ErrorBoundary } from "react-error-boundary"; import { beforeEach, describe, expect, it, vi } from "vitest"; import useToastNotification from "@/hooks/useToastNotification"; -import { ArtifactFetchError } from "@/services/executionService"; import { PARQUET_PREVIEW_ROWS } from "./parquetUtils"; import ParquetVisualizer from "./ParquetVisualizer"; @@ -22,7 +21,6 @@ vi.mock("hyparquet", () => ({ parquetMetadata: vi.fn(), parquetMetadataAsync: vi.fn(), asyncBufferFromUrl: vi.fn(), - byteLengthFromUrl: vi.fn(), cachedAsyncBuffer: vi.fn((buffer) => buffer), toJson: vi.fn((value) => value), })); @@ -94,7 +92,6 @@ const { parquetMetadata, parquetMetadataAsync, asyncBufferFromUrl, - byteLengthFromUrl, } = await import("hyparquet"); const { downloadStringAsFile } = await import("@/utils/URL"); @@ -176,14 +173,72 @@ beforeEach(() => { vi.mocked(parquetMetadataAsync).mockReset(); vi.mocked(parquetReadObjects).mockReset(); vi.mocked(downloadStringAsFile).mockReset(); - vi.mocked(byteLengthFromUrl).mockResolvedValue(1024); + mockRangeProbe(1024); vi.mocked(asyncBufferFromUrl).mockResolvedValue({ byteLength: 1024, slice: vi.fn(), } as unknown as Awaited>); }); +/** A `Range: bytes=0-0` probe answered with the total size, as object stores do. */ +const mockRangeProbe = (totalSize: number) => + vi.spyOn(globalThis, "fetch").mockResolvedValue({ + ok: true, + status: 206, + headers: new Headers({ "Content-Range": `bytes 0-0/${totalSize}` }), + } as unknown as Response); + describe("ParquetVisualizer", () => { + it("opens the file at the reported size without any size request", async () => { + const fetchSpy = mockRangeProbe(1024); + mockDataset(SCHEMA, 2); + + renderWithSuspense( + , + ); + + await waitFor(() => + expect(screen.getByTestId("table-visualizer")).toBeInTheDocument(), + ); + + expect(vi.mocked(asyncBufferFromUrl)).toHaveBeenCalledWith( + expect.objectContaining({ byteLength: 9446073 }), + ); + expect(fetchSpy).not.toHaveBeenCalled(); + }); + + it("probes the size with a ranged GET, never a HEAD, when none is reported", async () => { + const fetchSpy = mockRangeProbe(9446073); + mockDataset(SCHEMA, 2); + + renderWithSuspense( + , + ); + + await waitFor(() => + expect(screen.getByTestId("table-visualizer")).toBeInTheDocument(), + ); + + expect(vi.mocked(asyncBufferFromUrl)).toHaveBeenCalledWith( + expect.objectContaining({ byteLength: 9446073 }), + ); + expect(fetchSpy).toHaveBeenCalledWith( + "https://storage.example.com/unsized.parquet", + expect.objectContaining({ headers: { Range: "bytes=0-0" } }), + ); + const methods = fetchSpy.mock.calls.map(([, init]) => + (init?.method ?? "GET").toUpperCase(), + ); + expect(methods).not.toContain("HEAD"); + }); + it("reads via range requests, parses, and renders TableVisualizer with stats", async () => { mockDataset(SCHEMA, 2); @@ -381,6 +436,10 @@ describe("ParquetVisualizer", () => { expect(screen.queryByRole("button", { name: "Load max" })).toBeNull(); }); + /** + * A host that ignores `Range` and returns the whole object: the size probe + * sees 200 with no `Content-Range`, which is the no-range-support signal. + */ const mockFullDownload = (contentLength: string | null) => vi.spyOn(globalThis, "fetch").mockResolvedValue({ ok: true, @@ -390,10 +449,6 @@ describe("ParquetVisualizer", () => { } as unknown as Response); it("falls back to a full download when range requests are unavailable", async () => { - // Simulate a CORS/network failure on the range path (not an HTTP error). - vi.mocked(byteLengthFromUrl).mockRejectedValue( - new TypeError("Failed to fetch"), - ); mockFullDownload(String(1024)); vi.mocked(parquetMetadata).mockReturnValue({ schema: SCHEMA, @@ -418,9 +473,6 @@ describe("ParquetVisualizer", () => { }); it("errors instead of downloading a huge file when range requests are unavailable", async () => { - vi.mocked(byteLengthFromUrl).mockRejectedValue( - new TypeError("Failed to fetch"), - ); mockFullDownload(String(200 * 1024 * 1024)); renderWithSuspense( @@ -437,10 +489,6 @@ describe("ParquetVisualizer", () => { }); it("errors instead of downloading when Content-Length is absent", async () => { - vi.mocked(byteLengthFromUrl).mockRejectedValue( - new TypeError("Failed to fetch"), - ); - mockFullDownload(null); renderWithSuspense( @@ -514,9 +562,11 @@ describe("ParquetVisualizer", () => { }); it("surfaces artifact failures to the error boundary", async () => { - vi.mocked(byteLengthFromUrl).mockRejectedValue( - new ArtifactFetchError(500, "Server Error", "Failed to fetch artifact."), - ); + vi.spyOn(globalThis, "fetch").mockResolvedValue({ + ok: false, + status: 500, + statusText: "Server Error", + } as unknown as Response); renderWithSuspense( { const { data: base } = useSuspenseQuery({ queryKey: ["artifact-parquet", signedUrl], queryFn: async () => { - const opened = await openParquet(signedUrl); + const opened = await openParquet(signedUrl, byteLength); const columnCount = countColumns(opened.metadata); const { columns, rows } = await readParquetPreview( opened, diff --git a/src/components/shared/ReactFlow/FlowCanvas/TaskNode/TaskOverview/IOSection/IOCell/ArtifactVisualizer/parquetUtils.ts b/src/components/shared/ReactFlow/FlowCanvas/TaskNode/TaskOverview/IOSection/IOCell/ArtifactVisualizer/parquetUtils.ts index 869e45ebc8..1fabc32190 100644 --- a/src/components/shared/ReactFlow/FlowCanvas/TaskNode/TaskOverview/IOSection/IOCell/ArtifactVisualizer/parquetUtils.ts +++ b/src/components/shared/ReactFlow/FlowCanvas/TaskNode/TaskOverview/IOSection/IOCell/ArtifactVisualizer/parquetUtils.ts @@ -1,7 +1,6 @@ import { type AsyncBuffer, asyncBufferFromUrl, - byteLengthFromUrl, cachedAsyncBuffer, type FileMetaData, parquetMetadata, @@ -33,17 +32,18 @@ export interface OpenedParquet { metadata: FileMetaData; } -export async function openParquet(signedUrl: string): Promise { +export async function openParquet( + signedUrl: string, + byteLength?: number, +): Promise { try { - const byteLength = await byteLengthFromUrl( - signedUrl, - undefined, - fetchArtifactForHyparquet, - ); const source = cachedAsyncBuffer( await asyncBufferFromUrl({ url: signedUrl, - byteLength, + byteLength: + typeof byteLength === "number" && byteLength > 0 + ? byteLength + : await byteLengthFromRangedGet(signedUrl), fetch: fetchArtifactForHyparquet, }), ); @@ -51,26 +51,47 @@ export async function openParquet(signedUrl: string): Promise { return { source, metadata }; } catch (error) { if (error instanceof ArtifactFetchError) throw error; + return openByFullDownload(signedUrl); + } +} + +/** + * Signed object-store URLs reject HEAD (the method is part of the signature), + * and a bucket's CORS policy need not allow it either, so a blocked HEAD + * surfaces as an opaque network error rather than a 403 the caller could fall + * back from. A ranged GET is signed, CORS-safelisted, and reports the total + * size in `Content-Range`. + */ +async function byteLengthFromRangedGet(url: string): Promise { + const controller = new AbortController(); + const response = await fetchArtifactOrThrow(url, { + headers: { Range: "bytes=0-0" }, + signal: controller.signal, + }); + const totalSize = response.headers + .get("Content-Range") + ?.match(/\/(\d+)$/)?.[1]; + controller.abort(); - const response = await fetchArtifactOrThrow(signedUrl); - const contentLengthHeader = response.headers.get("Content-Length"); - const contentLength = contentLengthHeader - ? Number(contentLengthHeader) - : NaN; - if ( - !Number.isFinite(contentLength) || - contentLength > MAX_VISUALIZABLE_SIZE_BYTES - ) { - throw new ArtifactFetchError( - 413, - "Payload Too Large", - "This parquet file is too large to preview without range-request support.", - ); - } - - const source = await response.arrayBuffer(); - return { source, metadata: parquetMetadata(source) }; + if (response.status !== 206 || !totalSize) { + throw new Error("Artifact host does not support range requests."); } + return Number(totalSize); +} + +async function openByFullDownload(signedUrl: string): Promise { + const response = await fetchArtifactOrThrow(signedUrl); + const contentLength = Number(response.headers.get("Content-Length")); + if (!contentLength || contentLength > MAX_VISUALIZABLE_SIZE_BYTES) { + throw new ArtifactFetchError( + 413, + "Payload Too Large", + "This parquet file is too large to preview without range-request support.", + ); + } + + const source = await response.arrayBuffer(); + return { source, metadata: parquetMetadata(source) }; } export async function readParquetRows( diff --git a/src/routes/PipelineRun/ArtifactPreview.tsx b/src/routes/PipelineRun/ArtifactPreview.tsx index 3ca9712195..a9f9678107 100644 --- a/src/routes/PipelineRun/ArtifactPreview.tsx +++ b/src/routes/PipelineRun/ArtifactPreview.tsx @@ -137,6 +137,7 @@ const ArtifactPreviewBody = ({ artifactId={artifactId} type={normalizedType} isFullscreen={true} + totalSize={artifactData?.total_size} /> diff --git a/src/routes/v2/pages/CompareView/components/ArtifactComparisonDialog.tsx b/src/routes/v2/pages/CompareView/components/ArtifactComparisonDialog.tsx index a17b60afae..0526a5d6f1 100644 --- a/src/routes/v2/pages/CompareView/components/ArtifactComparisonDialog.tsx +++ b/src/routes/v2/pages/CompareView/components/ArtifactComparisonDialog.tsx @@ -89,6 +89,7 @@ function ComparisonPane({ artifactId={artifact.id} type={normalizedType} isFullscreen + totalSize={artifact.artifact_data?.total_size} /> )} From 5ca8bce6333d725b33504029ef2447e0272218e6 Mon Sep 17 00:00:00 2001 From: Camiel van Schoonhoven Date: Thu, 10 Sep 2026 15:45:04 -0700 Subject: [PATCH 2/3] test: guard the parquet size wiring; drop the dead HEAD fallback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The size fix was only covered at ParquetVisualizer, where the byte length is handed in directly. Removing `totalSize` from all three call sites — a full revert of the fix — left the suite green, so nothing held the seam the bug actually lived in. Assert the reported size reaches the visualizer from each call site: the task preview dialog, the shareable artifact page, and run comparison. `fetchArtifactForHyparquet` existed only to pass a 403 HEAD response back to hyparquet's own range fallback. Nothing issues a HEAD now, which left it byte-identical to `fetchArtifactOrThrow`; drop it. Also correct the range-probe comment: `Range` needs no preflight, but reading `Content-Range` back requires the bucket to expose it, so the probe is not unconditionally CORS-clean the way the comment claimed. Co-Authored-By: Claude Opus 5 (1M context) --- .../ArtifactVisualizer.test.tsx | 39 ++++++++- .../ParquetVisualizer.test.tsx | 16 ++-- .../IOCell/ArtifactVisualizer/parquetUtils.ts | 14 ++-- .../ArtifactVisualizer/useArtifactFetch.tsx | 16 ---- .../PipelineRun/ArtifactPreview.test.tsx | 4 + .../ArtifactComparisonDialog.test.tsx | 82 +++++++++++++++++++ 6 files changed, 135 insertions(+), 36 deletions(-) create mode 100644 src/routes/v2/pages/CompareView/components/ArtifactComparisonDialog.test.tsx diff --git a/src/components/shared/ReactFlow/FlowCanvas/TaskNode/TaskOverview/IOSection/IOCell/ArtifactVisualizer/ArtifactVisualizer.test.tsx b/src/components/shared/ReactFlow/FlowCanvas/TaskNode/TaskOverview/IOSection/IOCell/ArtifactVisualizer/ArtifactVisualizer.test.tsx index a0f5ba180c..b0513ff37f 100644 --- a/src/components/shared/ReactFlow/FlowCanvas/TaskNode/TaskOverview/IOSection/IOCell/ArtifactVisualizer/ArtifactVisualizer.test.tsx +++ b/src/components/shared/ReactFlow/FlowCanvas/TaskNode/TaskOverview/IOSection/IOCell/ArtifactVisualizer/ArtifactVisualizer.test.tsx @@ -108,8 +108,18 @@ vi.mock("./UrlVisualizer", () => ({ })); vi.mock("./ParquetVisualizer", () => ({ - default: ({ signedUrl }: { signedUrl: string }) => ( -
+ default: ({ + signedUrl, + byteLength, + }: { + signedUrl: string; + byteLength?: number; + }) => ( +
), })); @@ -353,7 +363,7 @@ describe("ArtifactVisualizer", () => { }); }); - it("renders ParquetVisualizer for apacheparquet type", async () => { + it("renders ParquetVisualizer for apacheparquet type with the reported size", async () => { renderWithQuery( { await userEvent.click(screen.getByText("Preview")); await waitFor(() => { - expect(screen.getByTestId("parquet-visualizer")).toBeInTheDocument(); + expect(screen.getByTestId("parquet-visualizer")).toHaveAttribute( + "data-byte-length", + "1024", + ); + }); + }); + + it("renders ParquetVisualizer without a size when the artifact reports none", async () => { + renderWithQuery( + , + ); + + await userEvent.click(screen.getByText("Preview")); + + await waitFor(() => { + expect(screen.getByTestId("parquet-visualizer")).not.toHaveAttribute( + "data-byte-length", + ); }); }); }); diff --git a/src/components/shared/ReactFlow/FlowCanvas/TaskNode/TaskOverview/IOSection/IOCell/ArtifactVisualizer/ParquetVisualizer.test.tsx b/src/components/shared/ReactFlow/FlowCanvas/TaskNode/TaskOverview/IOSection/IOCell/ArtifactVisualizer/ParquetVisualizer.test.tsx index b0d0e62c04..2f690b8102 100644 --- a/src/components/shared/ReactFlow/FlowCanvas/TaskNode/TaskOverview/IOSection/IOCell/ArtifactVisualizer/ParquetVisualizer.test.tsx +++ b/src/components/shared/ReactFlow/FlowCanvas/TaskNode/TaskOverview/IOSection/IOCell/ArtifactVisualizer/ParquetVisualizer.test.tsx @@ -163,6 +163,14 @@ const mockWideDataset = (columnCount: number, numRows: number) => { ); }; +/** A `Range: bytes=0-0` probe answered with the total size, as object stores do. */ +const mockRangeProbe = (totalSize: number) => + vi.spyOn(globalThis, "fetch").mockResolvedValue({ + ok: true, + status: 206, + headers: new Headers({ "Content-Range": `bytes 0-0/${totalSize}` }), + } as unknown as Response); + beforeEach(() => { queryClient.clear(); lastDownloadFull = undefined; @@ -180,14 +188,6 @@ beforeEach(() => { } as unknown as Awaited>); }); -/** A `Range: bytes=0-0` probe answered with the total size, as object stores do. */ -const mockRangeProbe = (totalSize: number) => - vi.spyOn(globalThis, "fetch").mockResolvedValue({ - ok: true, - status: 206, - headers: new Headers({ "Content-Range": `bytes 0-0/${totalSize}` }), - } as unknown as Response); - describe("ParquetVisualizer", () => { it("opens the file at the reported size without any size request", async () => { const fetchSpy = mockRangeProbe(1024); diff --git a/src/components/shared/ReactFlow/FlowCanvas/TaskNode/TaskOverview/IOSection/IOCell/ArtifactVisualizer/parquetUtils.ts b/src/components/shared/ReactFlow/FlowCanvas/TaskNode/TaskOverview/IOSection/IOCell/ArtifactVisualizer/parquetUtils.ts index 1fabc32190..5efa2ad88d 100644 --- a/src/components/shared/ReactFlow/FlowCanvas/TaskNode/TaskOverview/IOSection/IOCell/ArtifactVisualizer/parquetUtils.ts +++ b/src/components/shared/ReactFlow/FlowCanvas/TaskNode/TaskOverview/IOSection/IOCell/ArtifactVisualizer/parquetUtils.ts @@ -12,10 +12,7 @@ import { import { ArtifactFetchError } from "@/services/executionService"; -import { - fetchArtifactForHyparquet, - fetchArtifactOrThrow, -} from "./useArtifactFetch"; +import { fetchArtifactOrThrow } from "./useArtifactFetch"; import { type ArtifactCell, type ArtifactColumn, @@ -44,7 +41,7 @@ export async function openParquet( typeof byteLength === "number" && byteLength > 0 ? byteLength : await byteLengthFromRangedGet(signedUrl), - fetch: fetchArtifactForHyparquet, + fetch: fetchArtifactOrThrow, }), ); const metadata = await parquetMetadataAsync(source); @@ -56,11 +53,12 @@ export async function openParquet( } /** - * Signed object-store URLs reject HEAD (the method is part of the signature), + * Signed object-store URLs reject HEAD — the method is part of the signature — * and a bucket's CORS policy need not allow it either, so a blocked HEAD * surfaces as an opaque network error rather than a 403 the caller could fall - * back from. A ranged GET is signed, CORS-safelisted, and reports the total - * size in `Content-Range`. + * back from. A ranged GET is signed and needs no preflight, but reading the + * total size back requires the bucket to expose `Content-Range`; where it does + * not, this throws and the caller downloads the whole object instead. */ async function byteLengthFromRangedGet(url: string): Promise { const controller = new AbortController(); diff --git a/src/components/shared/ReactFlow/FlowCanvas/TaskNode/TaskOverview/IOSection/IOCell/ArtifactVisualizer/useArtifactFetch.tsx b/src/components/shared/ReactFlow/FlowCanvas/TaskNode/TaskOverview/IOSection/IOCell/ArtifactVisualizer/useArtifactFetch.tsx index b23b226f49..c9e0728ba1 100644 --- a/src/components/shared/ReactFlow/FlowCanvas/TaskNode/TaskOverview/IOSection/IOCell/ArtifactVisualizer/useArtifactFetch.tsx +++ b/src/components/shared/ReactFlow/FlowCanvas/TaskNode/TaskOverview/IOSection/IOCell/ArtifactVisualizer/useArtifactFetch.tsx @@ -15,22 +15,6 @@ export const fetchArtifactOrThrow: typeof fetch = async (input, init) => { return response; }; -export const fetchArtifactForHyparquet: typeof fetch = async (input, init) => { - const response = await fetch(input, init); - if (response.ok) return response; - - const method = (init?.method ?? "GET").toUpperCase(); - if (method === "HEAD" && response.status === 403) { - return response; - } - - throw new ArtifactFetchError( - response.status, - response.statusText, - "Failed to fetch artifact.", - ); -}; - export function useArtifactFetch( queryKey: string, signedUrl: string, diff --git a/src/routes/PipelineRun/ArtifactPreview.test.tsx b/src/routes/PipelineRun/ArtifactPreview.test.tsx index 0cbeeb478b..8338f94d8a 100644 --- a/src/routes/PipelineRun/ArtifactPreview.test.tsx +++ b/src/routes/PipelineRun/ArtifactPreview.test.tsx @@ -58,11 +58,13 @@ vi.mock( artifactId, name, isFullscreen, + totalSize, }: { type: string; artifactId: string; name: string; isFullscreen: boolean; + totalSize?: number; }) => (
), }; @@ -114,6 +117,7 @@ describe("ArtifactPreviewPage", () => { expect(preview).toHaveAttribute("data-artifact-id", "abc"); expect(preview).toHaveAttribute("data-name", "output"); expect(preview).toHaveAttribute("data-fullscreen", "true"); + expect(preview).toHaveAttribute("data-total-size", "100"); }); }); diff --git a/src/routes/v2/pages/CompareView/components/ArtifactComparisonDialog.test.tsx b/src/routes/v2/pages/CompareView/components/ArtifactComparisonDialog.test.tsx new file mode 100644 index 0000000000..2e046cbe33 --- /dev/null +++ b/src/routes/v2/pages/CompareView/components/ArtifactComparisonDialog.test.tsx @@ -0,0 +1,82 @@ +import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; +import { render, screen } from "@testing-library/react"; +import { describe, expect, it, vi } from "vitest"; + +import type { ArtifactNodeResponse } from "@/api/types.gen"; + +import { ArtifactComparisonDialog } from "./ArtifactComparisonDialog"; + +vi.mock( + "@/components/shared/ReactFlow/FlowCanvas/TaskNode/TaskOverview/IOSection/IOCell/ArtifactVisualizer/ArtifactPreviewContent", + () => ({ + PreviewContent: ({ + artifactId, + totalSize, + }: { + artifactId: string; + totalSize?: number; + }) => ( +
+ ), + PreviewSkeleton: () => null, + }), +); + +const queryClient = new QueryClient({ + defaultOptions: { queries: { retry: false } }, +}); + +const artifact = ( + id: string, + artifactData?: ArtifactNodeResponse["artifact_data"], +): ArtifactNodeResponse => ({ id, artifact_data: artifactData }); + +const renderDialog = ( + artifactA: ArtifactNodeResponse, + artifactB: ArtifactNodeResponse, +) => + render( + + {}} + name="table" + labelA="run-a" + labelB="run-b" + artifactA={artifactA} + artifactB={artifactB} + typeA="Apache Parquet" + typeB="Apache Parquet" + /> + , + ); + +describe("ArtifactComparisonDialog", () => { + it("passes each side's reported size to its preview", () => { + renderDialog( + artifact("a1", { total_size: 1024, is_dir: false }), + artifact("b1", { total_size: 9446073, is_dir: false }), + ); + + const [paneA, paneB] = screen.getAllByTestId("preview-content"); + expect(paneA).toHaveAttribute("data-artifact-id", "a1"); + expect(paneA).toHaveAttribute("data-total-size", "1024"); + expect(paneB).toHaveAttribute("data-artifact-id", "b1"); + expect(paneB).toHaveAttribute("data-total-size", "9446073"); + }); + + it("omits the size when a side reports no artifact data", () => { + renderDialog( + artifact("a1"), + artifact("b1", { total_size: 1024, is_dir: false }), + ); + + const [paneA, paneB] = screen.getAllByTestId("preview-content"); + expect(paneA).not.toHaveAttribute("data-total-size"); + expect(paneB).toHaveAttribute("data-total-size", "1024"); + }); +}); From 281e35abd35f26cbda1be1a28700aeec99586c69 Mon Sep 17 00:00:00 2001 From: Camiel van Schoonhoven Date: Thu, 10 Sep 2026 17:13:14 -0700 Subject: [PATCH 3/3] refactor: address review on the parquet size path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four review findings on #2704: Key the parquet query on the byte length it reads. The size was a `queryFn` input but absent from the key, so the entry claimed to be a function of the signed URL alone. No live bug — size and URL derive from the same artifact, and both paths parse the same file — but the entry would have gone stale silently if that ever stopped holding. Drop the causal clause from the too-large message. With a size supplied, any failed open falls through to the whole-file download, which for a large artifact raised "too large to preview without range-request support" even though range requests had worked and the open was what failed. The message no longer asserts a cause it cannot know. Carry one name for the size from `PreviewContent` inward. It arrived as `totalSize` and was handed on as `byteLength` a few lines later; the translation from the API's `total_size` now happens once, at each call site. Clear the query cache between tests in the comparison-dialog suite, as the two sibling suites over the same tree already do. Co-Authored-By: Claude Opus 5 (1M context) --- .../ArtifactPreviewContent.tsx | 6 +++--- .../ArtifactVisualizer/ArtifactVisualizer.tsx | 2 +- .../ArtifactVisualizer/ParquetVisualizer.tsx | 2 +- .../IOCell/ArtifactVisualizer/parquetUtils.ts | 2 +- .../PipelineRun/ArtifactPreview.test.tsx | 8 ++++---- src/routes/PipelineRun/ArtifactPreview.tsx | 2 +- .../ArtifactComparisonDialog.test.tsx | 20 +++++++++++-------- .../components/ArtifactComparisonDialog.tsx | 4 +++- 8 files changed, 26 insertions(+), 20 deletions(-) diff --git a/src/components/shared/ReactFlow/FlowCanvas/TaskNode/TaskOverview/IOSection/IOCell/ArtifactVisualizer/ArtifactPreviewContent.tsx b/src/components/shared/ReactFlow/FlowCanvas/TaskNode/TaskOverview/IOSection/IOCell/ArtifactVisualizer/ArtifactPreviewContent.tsx index 12e066fca0..265ef64b62 100644 --- a/src/components/shared/ReactFlow/FlowCanvas/TaskNode/TaskOverview/IOSection/IOCell/ArtifactVisualizer/ArtifactPreviewContent.tsx +++ b/src/components/shared/ReactFlow/FlowCanvas/TaskNode/TaskOverview/IOSection/IOCell/ArtifactVisualizer/ArtifactPreviewContent.tsx @@ -63,7 +63,7 @@ interface PreviewContentProps { type: string; name: string; isFullscreen: boolean; - totalSize?: number; + byteLength?: number; } export const PreviewContent = ({ @@ -71,7 +71,7 @@ export const PreviewContent = ({ type, name, isFullscreen, - totalSize, + byteLength, }: PreviewContentProps) => { const { backendUrl } = useBackend(); @@ -111,7 +111,7 @@ export const PreviewContent = ({ ); case "jsonobject": diff --git a/src/components/shared/ReactFlow/FlowCanvas/TaskNode/TaskOverview/IOSection/IOCell/ArtifactVisualizer/ArtifactVisualizer.tsx b/src/components/shared/ReactFlow/FlowCanvas/TaskNode/TaskOverview/IOSection/IOCell/ArtifactVisualizer/ArtifactVisualizer.tsx index cdeb354667..59c13a2db4 100644 --- a/src/components/shared/ReactFlow/FlowCanvas/TaskNode/TaskOverview/IOSection/IOCell/ArtifactVisualizer/ArtifactVisualizer.tsx +++ b/src/components/shared/ReactFlow/FlowCanvas/TaskNode/TaskOverview/IOSection/IOCell/ArtifactVisualizer/ArtifactVisualizer.tsx @@ -197,7 +197,7 @@ const ArtifactVisualizer = ({ artifactId={artifact.id} type={normalizedType} isFullscreen={isFullscreen} - totalSize={artifactData?.total_size} + byteLength={artifactData?.total_size} /> )} diff --git a/src/components/shared/ReactFlow/FlowCanvas/TaskNode/TaskOverview/IOSection/IOCell/ArtifactVisualizer/ParquetVisualizer.tsx b/src/components/shared/ReactFlow/FlowCanvas/TaskNode/TaskOverview/IOSection/IOCell/ArtifactVisualizer/ParquetVisualizer.tsx index 8809385150..27f2e16862 100644 --- a/src/components/shared/ReactFlow/FlowCanvas/TaskNode/TaskOverview/IOSection/IOCell/ArtifactVisualizer/ParquetVisualizer.tsx +++ b/src/components/shared/ReactFlow/FlowCanvas/TaskNode/TaskOverview/IOSection/IOCell/ArtifactVisualizer/ParquetVisualizer.tsx @@ -45,7 +45,7 @@ const ParquetVisualizer = ({ byteLength, }: ParquetVisualizerProps) => { const { data: base } = useSuspenseQuery({ - queryKey: ["artifact-parquet", signedUrl], + queryKey: ["artifact-parquet", signedUrl, byteLength], queryFn: async () => { const opened = await openParquet(signedUrl, byteLength); const columnCount = countColumns(opened.metadata); diff --git a/src/components/shared/ReactFlow/FlowCanvas/TaskNode/TaskOverview/IOSection/IOCell/ArtifactVisualizer/parquetUtils.ts b/src/components/shared/ReactFlow/FlowCanvas/TaskNode/TaskOverview/IOSection/IOCell/ArtifactVisualizer/parquetUtils.ts index 5efa2ad88d..152292b3ef 100644 --- a/src/components/shared/ReactFlow/FlowCanvas/TaskNode/TaskOverview/IOSection/IOCell/ArtifactVisualizer/parquetUtils.ts +++ b/src/components/shared/ReactFlow/FlowCanvas/TaskNode/TaskOverview/IOSection/IOCell/ArtifactVisualizer/parquetUtils.ts @@ -84,7 +84,7 @@ async function openByFullDownload(signedUrl: string): Promise { throw new ArtifactFetchError( 413, "Payload Too Large", - "This parquet file is too large to preview without range-request support.", + "This parquet file is too large to preview.", ); } diff --git a/src/routes/PipelineRun/ArtifactPreview.test.tsx b/src/routes/PipelineRun/ArtifactPreview.test.tsx index 8338f94d8a..765e0ee2c3 100644 --- a/src/routes/PipelineRun/ArtifactPreview.test.tsx +++ b/src/routes/PipelineRun/ArtifactPreview.test.tsx @@ -58,13 +58,13 @@ vi.mock( artifactId, name, isFullscreen, - totalSize, + byteLength, }: { type: string; artifactId: string; name: string; isFullscreen: boolean; - totalSize?: number; + byteLength?: number; }) => (
), }; @@ -117,7 +117,7 @@ describe("ArtifactPreviewPage", () => { expect(preview).toHaveAttribute("data-artifact-id", "abc"); expect(preview).toHaveAttribute("data-name", "output"); expect(preview).toHaveAttribute("data-fullscreen", "true"); - expect(preview).toHaveAttribute("data-total-size", "100"); + expect(preview).toHaveAttribute("data-byte-length", "100"); }); }); diff --git a/src/routes/PipelineRun/ArtifactPreview.tsx b/src/routes/PipelineRun/ArtifactPreview.tsx index a9f9678107..f3501a1f57 100644 --- a/src/routes/PipelineRun/ArtifactPreview.tsx +++ b/src/routes/PipelineRun/ArtifactPreview.tsx @@ -137,7 +137,7 @@ const ArtifactPreviewBody = ({ artifactId={artifactId} type={normalizedType} isFullscreen={true} - totalSize={artifactData?.total_size} + byteLength={artifactData?.total_size} />
diff --git a/src/routes/v2/pages/CompareView/components/ArtifactComparisonDialog.test.tsx b/src/routes/v2/pages/CompareView/components/ArtifactComparisonDialog.test.tsx index 2e046cbe33..2787889762 100644 --- a/src/routes/v2/pages/CompareView/components/ArtifactComparisonDialog.test.tsx +++ b/src/routes/v2/pages/CompareView/components/ArtifactComparisonDialog.test.tsx @@ -1,6 +1,6 @@ import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; import { render, screen } from "@testing-library/react"; -import { describe, expect, it, vi } from "vitest"; +import { beforeEach, describe, expect, it, vi } from "vitest"; import type { ArtifactNodeResponse } from "@/api/types.gen"; @@ -11,15 +11,15 @@ vi.mock( () => ({ PreviewContent: ({ artifactId, - totalSize, + byteLength, }: { artifactId: string; - totalSize?: number; + byteLength?: number; }) => (
), PreviewSkeleton: () => null, @@ -30,6 +30,10 @@ const queryClient = new QueryClient({ defaultOptions: { queries: { retry: false } }, }); +beforeEach(() => { + queryClient.clear(); +}); + const artifact = ( id: string, artifactData?: ArtifactNodeResponse["artifact_data"], @@ -64,9 +68,9 @@ describe("ArtifactComparisonDialog", () => { const [paneA, paneB] = screen.getAllByTestId("preview-content"); expect(paneA).toHaveAttribute("data-artifact-id", "a1"); - expect(paneA).toHaveAttribute("data-total-size", "1024"); + expect(paneA).toHaveAttribute("data-byte-length", "1024"); expect(paneB).toHaveAttribute("data-artifact-id", "b1"); - expect(paneB).toHaveAttribute("data-total-size", "9446073"); + expect(paneB).toHaveAttribute("data-byte-length", "9446073"); }); it("omits the size when a side reports no artifact data", () => { @@ -76,7 +80,7 @@ describe("ArtifactComparisonDialog", () => { ); const [paneA, paneB] = screen.getAllByTestId("preview-content"); - expect(paneA).not.toHaveAttribute("data-total-size"); - expect(paneB).toHaveAttribute("data-total-size", "1024"); + expect(paneA).not.toHaveAttribute("data-byte-length"); + expect(paneB).toHaveAttribute("data-byte-length", "1024"); }); }); diff --git a/src/routes/v2/pages/CompareView/components/ArtifactComparisonDialog.tsx b/src/routes/v2/pages/CompareView/components/ArtifactComparisonDialog.tsx index 0526a5d6f1..177b408312 100644 --- a/src/routes/v2/pages/CompareView/components/ArtifactComparisonDialog.tsx +++ b/src/routes/v2/pages/CompareView/components/ArtifactComparisonDialog.tsx @@ -59,6 +59,8 @@ function ComparisonPane({ normalizeRawType(type ?? undefined), ); + const byteLength = artifact?.artifact_data?.total_size; + return ( )}