refactor: pr feedback

app/client/modules/backups/components/create-schedule-form/advanced-section.tsx

@@ -1,6 +1,6 @@
 import { FormControl, FormDescription, FormField, FormItem, FormLabel, FormMessage } from "~/client/components/ui/form";
 import { Textarea } from "~/client/components/ui/textarea";
-import { useWatch, type UseFormReturn } from "react-hook-form";
+import { type UseFormReturn } from "react-hook-form";
 import type { InternalFormValues } from "./types";
 import { Input } from "~/client/components/ui/input";
 
@@ -9,10 +9,6 @@ type AdvancedSectionProps = {
 };
 
 export const AdvancedSection = ({ form }: AdvancedSectionProps) => {
-	const values = useWatch({ control: form.control });
-	const preBackupWebhookBody = values.preBackupWebhookBody?.trim();
-	const postBackupWebhookBody = values.postBackupWebhookBody?.trim();
-
 	return (
 		<>
 			<FormField

app/client/modules/backups/components/create-schedule-form/types.ts

@@ -29,10 +29,10 @@ export const internalFormSchema = z.object({
 	keepYearly: z.number().optional(),
 	oneFileSystem: z.boolean().optional(),
 	customResticParamsText: z.string().optional(),
-	preBackupWebhookUrl: z.union([z.string().url(), z.literal("")]).optional(),
+	preBackupWebhookUrl: z.union([z.url(), z.literal("")]).optional(),
 	preBackupWebhookHeaders: webhookHeadersSchema,
 	preBackupWebhookBody: z.string().optional(),
-	postBackupWebhookUrl: z.union([z.string().url(), z.literal("")]).optional(),
+	postBackupWebhookUrl: z.union([z.url(), z.literal("")]).optional(),
 	postBackupWebhookHeaders: webhookHeadersSchema,
 	postBackupWebhookBody: z.string().optional(),
 	maxRetries: z.number().min(0).max(32).optional(),

apps/agent/src/commands/backup-run.test.ts

@@ -1,5 +1,7 @@
-import { afterEach, expect, test, vi } from "vitest";
 import { Effect } from "effect";
+import { HttpResponse, http } from "msw";
+import { setupServer } from "msw/node";
+import { afterAll, afterEach, beforeAll, expect, test, vi } from "vitest";
 import waitForExpect from "wait-for-expect";
 import { fromPartial } from "@total-typescript/shoehorn";
 import { parseAgentMessage, type BackupCancelPayload, type BackupRunPayload } from "@zerobyte/contracts/agent-protocol";
@@ -8,6 +10,12 @@ import { handleBackupCancelCommand } from "./backup-cancel";
 import { handleBackupRunCommand } from "./backup-run";
 import type { ControllerCommandContext, RunningJob } from "../context";
 
+const server = setupServer();
+
+beforeAll(() => {
+	server.listen({ onUnhandledRequest: "error" });
+});
+
 const createDeferred = <T>() => {
 	let resolve!: (value: T) => void;
 	const promise = new Promise<T>((resolvePromise) => {
@@ -19,7 +27,11 @@ const createDeferred = <T>() => {
 
 afterEach(() => {
 	vi.restoreAllMocks();
-	vi.unstubAllGlobals();
+	server.resetHandlers();
+});
+
+afterAll(() => {
+	server.close();
 });
 
 const createRunPayload = (overrides: Partial<BackupRunPayload> = {}) =>
@@ -82,12 +94,16 @@ const runBackupCommand = async (payload: BackupRunPayload) => {
 test("runs pre and post backup webhooks around restic", async () => {
 	const events: string[] = [];
 
-	vi.stubGlobal(
+	server.use(
-		"fetch",
+		http.post("http://localhost:8080/pre", async ({ request }) => {
-		vi.fn(async (_url: URL, init: RequestInit) => {
+			const body = (await request.json()) as { event: string };
-			const body = JSON.parse(String(init.body)) as { event: string };
 			events.push(body.event);
-			return new Response(null, { status: 204 });
+			return new HttpResponse(null, { status: 204 });
+		}),
+		http.post("http://localhost:8080/post", async ({ request }) => {
+			const body = (await request.json()) as { event: string };
+			events.push(body.event);
+			return new HttpResponse(null, { status: 204 });
 		}),
 	);
 
@@ -117,15 +133,22 @@ test("runs pre and post backup webhooks around restic", async () => {
 test("sends configured webhook headers and body without changing them", async () => {
 	const requests: Array<{ url: string; headers: Headers; body: string }> = [];
 
-	vi.stubGlobal(
+	server.use(
-		"fetch",
+		http.post("http://localhost:8080/pre", async ({ request }) => {
-		vi.fn(async (url: URL, init: RequestInit) => {
 			requests.push({
-				url: url.toString(),
+				url: request.url,
-				headers: new Headers(init.headers),
+				headers: request.headers,
-				body: String(init.body),
+				body: await request.text(),
 			});
-			return new Response(null, { status: 204 });
+			return new HttpResponse(null, { status: 204 });
+		}),
+		http.post("http://localhost:8080/post", async ({ request }) => {
+			requests.push({
+				url: request.url,
+				headers: request.headers,
+				body: await request.text(),
+			});
+			return new HttpResponse(null, { status: 204 });
 		}),
 	);
 
@@ -164,9 +187,10 @@ test("sends configured webhook headers and body without changing them", async ()
 
 test("fails without running restic when the pre-backup webhook fails", async () => {
 	const backupMock = vi.fn();
-	vi.stubGlobal(
+	server.use(
-		"fetch",
+		http.post("http://localhost:8080/pre", () => {
-		vi.fn(async () => new Response("stop failed", { status: 500 })),
+			return new HttpResponse("stop failed", { status: 500 });
+		}),
 	);
 	vi.spyOn(resticServer, "createRestic").mockReturnValue(
 		fromPartial({
@@ -192,9 +216,10 @@ test("fails without running restic when the pre-backup webhook fails", async ()
 });
 
 test("reports a post-backup webhook failure as completed warning details", async () => {
-	vi.stubGlobal(
+	server.use(
-		"fetch",
+		http.post("http://localhost:8080/post", () => {
-		vi.fn(async () => new Response("start failed", { status: 500 })),
+			return new HttpResponse("start failed", { status: 500 });
+		}),
 	);
 	vi.spyOn(resticServer, "createRestic").mockReturnValue(
 		fromPartial({

apps/agent/src/commands/backup-run.ts

@@ -69,8 +69,8 @@ export const handleBackupRunCommand = (context: ControllerCommandContext, payloa
 					runBackup: () => {
 						return restic
 							.backup(payload.repositoryConfig, payload.sourcePath, {
-								organizationId: payload.organizationId,
 								...payload.options,
+								organizationId: payload.organizationId,
 								signal: abortController.signal,
 								onProgress: (progress) => {
 									void Runtime.runPromise(
@@ -115,7 +115,7 @@ export const handleBackupRunCommand = (context: ControllerCommandContext, payloa
 							createAgentMessage("backup.failed", {
 								jobId: payload.jobId,
 								scheduleId: payload.scheduleId,
-								error: backupResult.error,
+								error: toMessage(backupResult.error),
 								errorDetails: backupResult.error,
 							}),
 						);

packages/core/src/backup-hooks/__tests__/hooks.test.ts

@@ -310,6 +310,41 @@ test("includes post-backup webhook failure details after cancellation", async ()
 	}
 });
 
+test("includes post-backup webhook failure details after completed cancellation", async () => {
+	const abortController = new AbortController();
+	let postBody: { status?: string; error?: string } | undefined;
+
+	server.use(
+		http.post("http://localhost:8080/post", async ({ request }) => {
+			postBody = (await request.json()) as { status?: string; error?: string };
+			return new HttpResponse("cleanup failed", { status: 500 });
+		}),
+	);
+
+	const result = await Effect.runPromise(
+		runBackupWithWebhooks({
+			metadata,
+			webhooks: {
+				pre: null,
+				post: { url: "http://localhost:8080/post" },
+			},
+			signal: abortController.signal,
+			runBackup: () =>
+				Effect.sync(() => {
+					abortController.abort(new Error("Backup was cancelled"));
+					return { status: "completed" as const, exitCode: 0, result: null, warningDetails: null };
+				}),
+		}),
+	);
+
+	expect(postBody).toMatchObject({ status: "cancelled", error: "Backup was cancelled" });
+	expect(result.status).toBe("cancelled");
+	if (result.status === "cancelled") {
+		expect(result.message).toContain("Backup was cancelled");
+		expect(result.message).toContain("Post-backup webhook returned HTTP 500: cleanup failed");
+	}
+});
+
 test("cancels before the pre-backup webhook without running the backup", async () => {
 	const abortController = new AbortController();
 	let backupRan = false;

packages/core/src/backup-hooks/index.ts

@@ -108,6 +108,8 @@ const getCompletedStatus = (exitCode: number, signal: AbortSignal): BackupWebhoo
 	return exitCode === 0 ? "success" : "warning";
 };
 
+const formatWebhookPhase = (phase: BackupWebhookPhase) => `${phase === "pre" ? "Pre" : "Post"}-backup`;
+
 const createAbortController = (timeoutMs: number, signal?: AbortSignal) => {
 	const abortController = new AbortController();
 	const timeout = setTimeout(() => {
@@ -167,6 +169,7 @@ export const runBackupWebhook = async (
 	try {
 		const parsedConfig = backupWebhookConfigSchema.parse(config);
 		const url = new URL(parsedConfig.url);
+		const phase = formatWebhookPhase(context.phase);
 
 		const response = await fetch(url, {
 			...createRequestInit(parsedConfig, context),
@@ -176,17 +179,26 @@ export const runBackupWebhook = async (
 		if (!response.ok) {
 			const responseText = await response.text().catch(() => "");
 			const details = responseText.trim().slice(0, 500);
-			throw new Error(`${context.phase} webhook returned HTTP ${response.status}${details ? `: ${details}` : ""}`);
+			throw new BackupWebhookError({
+				cause: new Error(`${phase} webhook returned HTTP ${response.status}`),
+				message: `${phase} webhook returned HTTP ${response.status}${details ? `: ${details}` : ""}`,
+			});
 		}
 	} catch (error) {
+		if (error instanceof BackupWebhookError) {
+			throw error;
+		}
+
+		const phase = formatWebhookPhase(context.phase);
+
 		if (controller.signal.aborted && controller.signal.reason instanceof Error) {
 			throw new BackupWebhookError({
 				cause: controller.signal.reason,
-				message: `${context.phase} webhook failed: ${controller.signal.reason.message}`,
+				message: `${phase} webhook failed: ${controller.signal.reason.message}`,
 			});
 		}
 
-		throw new BackupWebhookError({ cause: error, message: `${context.phase} webhook failed: ${toMessage(error)}` });
+		throw new BackupWebhookError({ cause: error, message: `${phase} webhook failed: ${toMessage(error)}` });
 	} finally {
 		controller.cleanup();
 	}
@@ -246,15 +258,18 @@ export const runBackupWithWebhooks = <TResult, R = never>({
 
 		if (backupResult.status === "completed") {
 			const hookStatus = getCompletedStatus(backupResult.exitCode, signal);
+			const hookError = signal.aborted ? formatErrorMessage(signal.reason) : (backupResult.warningDetails ?? undefined);
 			const postHookError = yield* runConfiguredWebhook(
 				webhooks.post,
-				createWebhookContext(metadata, "post", hookStatus, backupResult.warningDetails ?? undefined),
+				createWebhookContext(metadata, "post", hookStatus, hookError),
 				formatErrorDetails,
-				signal,
 			);
 
 			if (signal.aborted) {
-				return { status: "cancelled", message: formatErrorMessage(signal.reason) };
+				return {
+					status: "cancelled",
+					message: appendDetails(formatErrorMessage(signal.reason), postHookError) || undefined,
+				};
 			}
 
 			return {
@@ -270,7 +285,6 @@ export const runBackupWithWebhooks = <TResult, R = never>({
 			webhooks.post,
 			createWebhookContext(metadata, "post", signal.aborted ? "cancelled" : "error", errorDetails),
 			formatErrorDetails,
-			signal,
 		);
 
 		if (signal.aborted) {