Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion packages/core/src/task-detail/taskCreationEffects.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import type { TaskCreationInput, TaskCreationOutput } from "@posthog/shared";
import type { TaskRun } from "@posthog/shared/domain-types";
import type { Task, TaskRun } from "@posthog/shared/domain-types";

/**
* Host-side reactions to a successful task-creation: optimistic workspace
Expand All @@ -11,4 +11,11 @@ export interface TaskCreationEffects {
onWorkspaceCreated(output: TaskCreationOutput): void;
onCreateSuccess(output: TaskCreationOutput, input?: TaskCreationInput): void;
onRunResumed(taskId: string, run: TaskRun): void;
/**
* The saga surfaced the task to the UI (onTaskReady) but a later step
* failed and rolled the task back (best-effort server-side delete), while
* caches seeded at ready-time still hold it. Undo those seeds and refetch —
* if the delete itself failed, the refetch restores the surviving task.
*/
onCreateRolledBack(task: Task): void;
}
105 changes: 105 additions & 0 deletions packages/core/src/task-detail/taskService.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,111 @@ describe("TaskService.resumeCloudPiRun", () => {
});
});

describe("TaskService.createTask rollback after surfacing", () => {
// Repo-less local ("channels chat box") creation: the saga creates the task
// row, provisions a scratch dir, fires onTaskReady, then connects the agent
// session — so a connect failure rolls back a task the UI already surfaced.
function makeHarness({ connectToTask }: { connectToTask: () => void }) {
const task = { id: "task-1", title: "T", description: "do the thing" };
const api = {
createTask: vi.fn(async () => task),
deleteTask: vi.fn(async () => undefined),
};
const host = {
getAuthenticatedClient: vi.fn(async () => api),
detectRepo: vi.fn(async () => null),
getTaskDirectory: vi.fn(async () => null),
ensureScratchDir: vi.fn(async () => "/scratch/task-1"),
track: vi.fn(),
} as unknown as ITaskCreationHost;
const sessionService = {
markTaskCreationInFlight: vi.fn(),
connectToTask: vi.fn(connectToTask),
disconnectFromTask: vi.fn(),
} as unknown as SessionService;
const effects = {
onWorkspaceCreated: vi.fn(),
onCreateSuccess: vi.fn(),
onCreateRolledBack: vi.fn(),
};
const service = new TaskService(
host,
sessionService,
effects as unknown as TaskCreationEffects,
{} as PiRunner,
rootLogger,
);
return { service, api, effects, task };
}

const input = {
content: "do the thing",
workspaceMode: "local" as const,
allowNoRepo: true,
};

it("fires onCreateRolledBack when a step fails after onTaskReady", async () => {
const { service, api, effects, task } = makeHarness({
connectToTask: () => {
throw new Error("agent server down");
},
});
const onTaskReady = vi.fn();

const result = await service.createTask(input, onTaskReady);

expect(result.success).toBe(false);
if (result.success) throw new Error("expected agent_session failure");
expect(result.failedStep).toBe("agent_session");
// The UI got the task before the failure, and the saga deleted it — the
// effect is what lets hosts pull it back out of their caches.
expect(onTaskReady).toHaveBeenCalledOnce();
expect(api.deleteTask).toHaveBeenCalledWith(task.id);
expect(effects.onCreateRolledBack).toHaveBeenCalledExactlyOnceWith(task);
});

it("does not fire onCreateRolledBack when creation succeeds", async () => {
const { service, effects } = makeHarness({ connectToTask: () => {} });

const result = await service.createTask(input, vi.fn());

expect(result.success).toBe(true);
expect(effects.onCreateRolledBack).not.toHaveBeenCalled();
});

it("does not fire onCreateRolledBack when creation fails before surfacing", async () => {
const effects = {
onWorkspaceCreated: vi.fn(),
onCreateSuccess: vi.fn(),
onCreateRolledBack: vi.fn(),
};
const host = {
getAuthenticatedClient: vi.fn(async () => ({
createTask: vi.fn().mockRejectedValue(new Error("api down")),
deleteTask: vi.fn(),
})),
detectRepo: vi.fn(async () => null),
track: vi.fn(),
} as unknown as ITaskCreationHost;
const service = new TaskService(
host,
{ markTaskCreationInFlight: vi.fn() } as unknown as SessionService,
effects as unknown as TaskCreationEffects,
{} as PiRunner,
rootLogger,
);
const onTaskReady = vi.fn();

const result = await service.createTask(input, onTaskReady);

expect(result.success).toBe(false);
if (result.success) throw new Error("expected task_creation failure");
expect(result.failedStep).toBe("task_creation");
expect(onTaskReady).not.toHaveBeenCalled();
expect(effects.onCreateRolledBack).not.toHaveBeenCalled();
});
});

describe("TaskService.createTask validation", () => {
it("rejects an input with neither content nor a taskDescription", async () => {
const result = await makeService().createTask({
Expand Down
15 changes: 14 additions & 1 deletion packages/core/src/task-detail/taskService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -150,14 +150,21 @@ export class TaskService {
}
}

// onTaskReady fires mid-saga, before the remaining steps run — so a later
// failure rolls back (deletes) a task the UI has already been handed and
// surfaced into feeds and caches. Track it so hosts can undo that.
let surfacedTask: Task | undefined;
const creator = new TaskCreationSaga(
{
posthogClient,
host: this.host,
sessionService: this.sessionService,
piRunner: this.piRunner,
track: (event, props) => this.host.track(event, props),
onTaskReady,
onTaskReady: (output) => {
surfacedTask = output.task;
onTaskReady?.(output);
},
},
this.log,
);
Expand All @@ -166,6 +173,12 @@ export class TaskService {
if (result.success) {
this.effects.onWorkspaceCreated(result.data);
this.effects.onCreateSuccess(result.data, input);
} else if (surfacedTask) {
this.log.info("Task creation rolled back after task was surfaced", {
taskId: surfacedTask.id,
failedStep: result.failedStep,
});
this.effects.onCreateRolledBack(surfacedTask);
}

return result;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -333,9 +333,12 @@ export const ChannelHomeComposer = forwardRef<

const created = await handleSubmit(content);
if (!created) {
// Creation failed — onTaskCreated never fired, so this id is still
// queued. Pull its row and give the full structured prompt (chips and
// attachments, not just flattened text) back so the user can retry.
// Creation failed. If it failed before the task surfaced, this id is
// still queued — pull its "Starting…" row (no-ops otherwise: a failure
// after onTaskCreated fired already retired the id, and the spliced
// card is rolled back centrally via onCreateRolledBack). Give the full
// structured prompt (chips and attachments, not just flattened text)
// back so the user can retry.
pendingIdsRef.current = pendingIdsRef.current.filter((p) => p !== id);
onPendingEnd(id);
editor.insertEditorContent(content);
Expand Down
53 changes: 48 additions & 5 deletions packages/ui/src/features/canvas/components/WebsiteNewTask.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,14 @@ if (typeof globalThis.ResizeObserver === "undefined") {
} as unknown as typeof ResizeObserver;
}

const { track, useFolderInstructions, taskInputProps } = vi.hoisted(() => ({
track: vi.fn(),
useFolderInstructions: vi.fn(),
taskInputProps: vi.fn(),
}));
const { track, useFolderInstructions, taskInputProps, router, openTaskInput } =
vi.hoisted(() => ({
track: vi.fn(),
useFolderInstructions: vi.fn(),
taskInputProps: vi.fn(),
router: { state: { location: { pathname: "/website/chan-1/new" } } },
openTaskInput: vi.fn(),
}));

// TaskInput is a huge hook-heavy component; stub it down to just the surface
// this test cares about — a button that fires onContextChipClick when wired.
Expand Down Expand Up @@ -59,8 +62,10 @@ vi.mock("@posthog/ui/shell/analytics", () => ({ track }));
vi.mock("@tanstack/react-query", () => ({
useQueryClient: () => ({ setQueryData: vi.fn() }),
}));
vi.mock("@posthog/ui/router/useOpenTask", () => ({ openTaskInput }));
vi.mock("@tanstack/react-router", () => ({
useNavigate: () => vi.fn(),
useRouter: () => router,
// The view reads the matched route so it can pass new-task prefill through.
useRouterState: ({
select,
Expand Down Expand Up @@ -147,3 +152,41 @@ describe("WebsiteNewTask context panel", () => {
expect(screen.getByRole("button", { name: "context-chip" })).toBeDisabled();
});
});

// onTaskCreated navigates onto the task before creation fully settles; when a
// later step fails the task is rolled back and that page is a dead end.
describe("WebsiteNewTask creation rollback", () => {
beforeEach(() => {
useFolderInstructions.mockReturnValue({ data: undefined });
taskInputProps.mockReset();
openTaskInput.mockReset();
});

function failCreation() {
const props = taskInputProps.mock.lastCall?.[0] as {
onTaskCreationFailed: (task: { id: string }, promptText: string) => void;
};
props.onTaskCreationFailed({ id: "task-9" }, "ship the thing");
}

it("returns the user to the composer when parked on the dead task", () => {
renderNewTask();
router.state.location.pathname = "/website/chan-1/tasks/task-9";

failCreation();

expect(openTaskInput).toHaveBeenCalledExactlyOnceWith({
channelId: "chan-1",
initialPrompt: "ship the thing",
});
});

it("leaves navigation alone when the user already moved elsewhere", () => {
renderNewTask();
router.state.location.pathname = "/website/chan-2";

failCreation();

expect(openTaskInput).not.toHaveBeenCalled();
});
});
17 changes: 16 additions & 1 deletion packages/ui/src/features/canvas/components/WebsiteNewTask.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,11 @@ import { useSetHeaderContent } from "@posthog/ui/hooks/useSetHeaderContent";
import { ResizableSidebar } from "@posthog/ui/primitives/ResizableSidebar";
import { toast } from "@posthog/ui/primitives/toast";
import { useAppView } from "@posthog/ui/router/useAppView";
import { openTaskInput } from "@posthog/ui/router/useOpenTask";
import { track } from "@posthog/ui/shell/analytics";
import { Flex } from "@radix-ui/themes";
import { useQueryClient } from "@tanstack/react-query";
import { useNavigate } from "@tanstack/react-router";
import { useNavigate, useRouter } from "@tanstack/react-router";
import { useCallback, useMemo, useState } from "react";

// A channel's "New task" view. Reuses /code's TaskInput, but routes the created
Expand All @@ -27,6 +28,7 @@ import { useCallback, useMemo, useState } from "react";
export function WebsiteNewTask({ channelId }: { channelId: string }) {
const spacesLayout = useChannelsLayout();
const navigate = useNavigate();
const router = useRouter();
const view = useAppView();
const queryClient = useQueryClient();
const { fileTask } = useChannelTaskMutations();
Expand Down Expand Up @@ -109,11 +111,24 @@ export function WebsiteNewTask({ channelId }: { channelId: string }) {
[channelId, fileTask, navigate, queryClient],
);

// onTaskCreated above navigates onto the task optimistically; if creation
// then fails and rolls the task back, that page is a dead end. Return the
// user to this composer with their prompt — unless they've already moved on.
const onTaskCreationFailed = useCallback(
(task: Task, promptText: string) => {
if (router.state.location.pathname.endsWith(`/tasks/${task.id}`)) {
openTaskInput({ channelId, initialPrompt: promptText });
}
},
[router, channelId],
);

return (
<Flex className="h-full min-w-0 flex-1">
<div className="min-w-0 flex-1">
<TaskInput
onTaskCreated={onTaskCreated}
onTaskCreationFailed={onTaskCreationFailed}
channelContext={channelContext}
channelName={channelName}
channelId={backendChannel?.id}
Expand Down
89 changes: 89 additions & 0 deletions packages/ui/src/features/navigation/taskBinderImpl.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
import type { Task } from "@posthog/shared/domain-types";
import { beforeEach, describe, expect, it, vi } from "vitest";

vi.mock("@posthog/di/container", () => ({
resolveService: vi.fn(),
}));

import { resolveService } from "@posthog/di/container";
import { HOST_TRPC_CLIENT } from "@posthog/host-router/client";
import { IMPERATIVE_QUERY_CLIENT } from "../../shell/queryClient";
import { navigationTaskBinder } from "./taskBinderImpl";

function makeHost(overrides?: {
workspaces?: Record<string, unknown>;
folders?: unknown[];
}) {
return {
workspace: {
getAll: { query: vi.fn(async () => overrides?.workspaces ?? {}) },
create: { mutate: vi.fn(async () => undefined) },
ensureScratchDir: {
mutate: vi.fn(async () => ({ path: "/scratch/t1" })),
},
},
folders: {
getFolders: { query: vi.fn(async () => overrides?.folders ?? []) },
getRepositoryByRemoteUrl: { query: vi.fn(async () => null) },
addFolder: { mutate: vi.fn(async () => undefined) },
},
};
}

const queryClientFake = { invalidateQueries: vi.fn(async () => undefined) };

function wire(host: ReturnType<typeof makeHost>) {
vi.mocked(resolveService).mockImplementation((token) => {
if (token === HOST_TRPC_CLIENT) return host;
if (token === IMPERATIVE_QUERY_CLIENT) return queryClientFake;
// Anything else (e.g. the shell logger's HOST_LOGGER) is unbound in this
// harness; throwing matches an unbound container and the logger no-ops.
throw new Error(`unbound token in test: ${String(token)}`);
});
}

describe("navigationTaskBinder.ensureWorkspaceForTask", () => {
beforeEach(() => {
vi.clearAllMocks();
});

it("provisions a scratch dir for a repo-less local task with no workspace", async () => {
const host = makeHost();
wire(host);
const task = { id: "t1", repository: null } as unknown as Task;

await navigationTaskBinder.ensureWorkspaceForTask(task);

expect(host.workspace.ensureScratchDir.mutate).toHaveBeenCalledWith({
taskId: "t1",
});
expect(host.workspace.create.mutate).not.toHaveBeenCalled();
expect(queryClientFake.invalidateQueries).toHaveBeenCalled();
});

it("leaves an existing scratch workspace alone", async () => {
const host = makeHost({
workspaces: { t1: { taskId: "t1", isScratch: true } },
});
wire(host);
const task = { id: "t1", repository: null } as unknown as Task;

await navigationTaskBinder.ensureWorkspaceForTask(task);

expect(host.workspace.ensureScratchDir.mutate).not.toHaveBeenCalled();
});

it("does not provision scratch for a repo task with no resolvable directory", async () => {
const host = makeHost();
wire(host);
const task = {
id: "t1",
repository: "posthog/code",
} as unknown as Task;

await navigationTaskBinder.ensureWorkspaceForTask(task);

expect(host.workspace.ensureScratchDir.mutate).not.toHaveBeenCalled();
expect(host.workspace.create.mutate).not.toHaveBeenCalled();
});
});
Loading
Loading