From 039a02456d15707513f707e1b3e83b1b4c3974b2 Mon Sep 17 00:00:00 2001 From: "posthog[bot]" <206114724+posthog[bot]@users.noreply.github.com> Date: Thu, 23 Jul 2026 20:28:02 +0000 Subject: [PATCH] fix(task-detail): guard against undefined task in TaskDetail render useRefreshedTask could return undefined when another observer subscribed to the same query key without initialData and created the cache entry first while the fetch was in flight, causing React Query to drop this hook's initialData. That undefined flowed into getTaskRepository, which dereferenced task.repository unguarded and threw inside React's render loop, crashing the whole TaskDetail view. Restore the pre-regression guarantee that useTaskData always has a defined task by returning `data ?? initialTask`, and make getTaskRepository null-safe on its argument as defense in depth. Generated-By: PostHog Code Task-Id: 400e2a89-4ede-447f-8ce9-97f61889cd19 --- packages/shared/src/repository.test.ts | 32 +++++++++++++++++ packages/shared/src/repository.ts | 8 ++--- .../hooks/useRefreshedTask.test.tsx | 35 ++++++++++++++++++- .../task-detail/hooks/useRefreshedTask.ts | 5 ++- 4 files changed, 74 insertions(+), 6 deletions(-) create mode 100644 packages/shared/src/repository.test.ts diff --git a/packages/shared/src/repository.test.ts b/packages/shared/src/repository.test.ts new file mode 100644 index 0000000000..b964b9ae57 --- /dev/null +++ b/packages/shared/src/repository.test.ts @@ -0,0 +1,32 @@ +import { describe, expect, it } from "vitest"; +import { getTaskRepository, parseRepository } from "./repository"; + +describe("getTaskRepository", () => { + it.each([ + { name: "undefined task", task: undefined, expected: null }, + { name: "null task", task: null, expected: null }, + { name: "missing repository", task: {}, expected: null }, + { name: "null repository", task: { repository: null }, expected: null }, + { + name: "populated repository", + task: { repository: "posthog/code" }, + expected: "posthog/code", + }, + ])("returns $expected for $name", ({ task, expected }) => { + expect(getTaskRepository(task)).toBe(expected); + }); +}); + +describe("parseRepository", () => { + it("splits an org/repo string", () => { + expect(parseRepository("posthog/code")).toEqual({ + organization: "posthog", + repoName: "code", + }); + }); + + it("returns null for malformed input", () => { + expect(parseRepository("code")).toBeNull(); + expect(parseRepository("a/b/c")).toBeNull(); + }); +}); diff --git a/packages/shared/src/repository.ts b/packages/shared/src/repository.ts index 2902588698..c7b70b5d5f 100644 --- a/packages/shared/src/repository.ts +++ b/packages/shared/src/repository.ts @@ -10,8 +10,8 @@ export const parseRepository = ( return { organization: result[0], repoName: result[1] }; }; -export function getTaskRepository(task: { - repository?: string | null; -}): string | null { - return task.repository ?? null; +export function getTaskRepository( + task: { repository?: string | null } | null | undefined, +): string | null { + return task?.repository ?? null; } diff --git a/packages/ui/src/features/task-detail/hooks/useRefreshedTask.test.tsx b/packages/ui/src/features/task-detail/hooks/useRefreshedTask.test.tsx index 9d11d1c35a..166e7f50a5 100644 --- a/packages/ui/src/features/task-detail/hooks/useRefreshedTask.test.tsx +++ b/packages/ui/src/features/task-detail/hooks/useRefreshedTask.test.tsx @@ -5,12 +5,24 @@ import type { PropsWithChildren } from "react"; import { beforeEach, describe, expect, it, vi } from "vitest"; import { useRefreshedTask } from "./useRefreshedTask"; -const mocks = vi.hoisted(() => ({ getTask: vi.fn() })); +const mocks = vi.hoisted(() => ({ + getTask: vi.fn(), + useQuery: vi.fn(), + actualUseQuery: undefined as unknown, +})); vi.mock("@posthog/ui/features/auth/authClientImperative", () => ({ getAuthenticatedClient: vi.fn(async () => ({ getTask: mocks.getTask })), })); +vi.mock("@tanstack/react-query", async (importOriginal) => { + const actual = await importOriginal(); + // Keep a handle on the real useQuery so beforeEach can restore delegation + // after a test overrides the mock's return value. + mocks.actualUseQuery = actual.useQuery; + return { ...actual, useQuery: mocks.useQuery }; +}); + function task(runId: string, status: "failed" | "in_progress"): Task { return { id: "task-123", @@ -30,6 +42,27 @@ function task(runId: string, status: "failed" | "in_progress"): Task { describe("useRefreshedTask", () => { beforeEach(() => { mocks.getTask.mockReset(); + mocks.useQuery.mockReset(); + // Delegate to the real useQuery by default; a test can override the return + // value to simulate a specific cache state. + mocks.useQuery.mockImplementation( + mocks.actualUseQuery as typeof import("@tanstack/react-query").useQuery, + ); + }); + + it("falls back to initialTask when the query yields undefined data", () => { + // A separate observer subscribing to the same query key without initialData + // can create the cache entry first while the fetch is in flight, so React + // Query hands this hook `data: undefined` despite the initialData option. + // The hook must never surface undefined (it flows into getTaskRepository). + mocks.useQuery.mockReturnValue({ data: undefined }); + const initialTask = task("run-parent", "in_progress"); + + const { result } = renderHook(() => + useRefreshedTask("task-123", initialTask), + ); + + expect(result.current).toBe(initialTask); }); it("replaces a cached failed run with the authoritative resumed run", async () => { diff --git a/packages/ui/src/features/task-detail/hooks/useRefreshedTask.ts b/packages/ui/src/features/task-detail/hooks/useRefreshedTask.ts index e2f6be477d..f8ddc9a4b5 100644 --- a/packages/ui/src/features/task-detail/hooks/useRefreshedTask.ts +++ b/packages/ui/src/features/task-detail/hooks/useRefreshedTask.ts @@ -9,5 +9,8 @@ export function useRefreshedTask(taskId: string, initialTask: Task): Task { refetchOnMount: "always", }); - return data; + // Guard against `data` being undefined: another observer subscribing to the + // same query key without `initialData` can create the cache entry first while + // the fetch is in flight, causing React Query to drop this hook's initialData. + return data ?? initialTask; }