From 3979ab160736a3a9073c91f000679f4664194880 Mon Sep 17 00:00:00 2001 From: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Date: Tue, 2 Jun 2026 06:22:25 +0800 Subject: [PATCH] fix(actions): honour the inner result envelope so failed actions don't report success Object-action / flow / api routes wrap the handler's own result, so a transport 200 with `{ success: true }` can still carry an INNER `{ success: false, error }` in `data` (e.g. "Action 'x' not found"). dispatchServerAction/dispatchFlow/ dispatchApi only checked the OUTER flag, so a failed action was reported to the user as success with no error. Add a shared interpretEnvelope() that unwraps one level and honours the inner `success`, lifting the inner `error` for display. Use it at all three dispatch sites. (Same envelope trap useTriggerFlow already handles for flow runs.) Unit-tested against the real payload; lint + typecheck clean. Co-Authored-By: Claude Opus 4.8 --- __tests__/lib/record-actions.test.ts | 45 ++++++++++++++++++ lib/record-actions.ts | 68 +++++++++++++++++++--------- 2 files changed, 92 insertions(+), 21 deletions(-) create mode 100644 __tests__/lib/record-actions.test.ts diff --git a/__tests__/lib/record-actions.test.ts b/__tests__/lib/record-actions.test.ts new file mode 100644 index 0000000..3e456ff --- /dev/null +++ b/__tests__/lib/record-actions.test.ts @@ -0,0 +1,45 @@ +/** + * Tests for interpretEnvelope — the server-response interpreter behind object + * actions / flow triggers. The key case: a transport 200 whose body wraps an + * INNER { success: false } must be reported as a failure, not a false success. + */ +import { interpretEnvelope } from "~/lib/record-actions"; + +const ok = { ok: true, status: 200 }; +const notFound = { ok: false, status: 404 }; + +describe("interpretEnvelope", () => { + it("treats a clean 200 as success and lifts the inner data payload", () => { + const r = interpretEnvelope(ok, { success: true, data: { id: "1" } }, "fallback", true); + expect(r).toEqual({ success: true, data: { id: "1" }, reload: true }); + }); + + it("fails on a non-ok HTTP status", () => { + const r = interpretEnvelope(notFound, null, "fallback err", false); + expect(r.success).toBe(false); + expect(r.error).toBe("fallback err"); + }); + + it("fails when the outer envelope says success:false", () => { + const r = interpretEnvelope(ok, { success: false, error: "denied" }, "fallback", false); + expect(r).toMatchObject({ success: false, error: "denied" }); + }); + + it("fails when the INNER data envelope says success:false (the bug)", () => { + // What the action route actually returns for an unknown handler. + const body = { success: true, data: { success: false, error: "Action 'x' not found" } }; + const r = interpretEnvelope(ok, body, "fallback", true); + expect(r.success).toBe(false); + expect(r.error).toBe("Action 'x' not found"); + }); + + it("falls back to the default error when the inner failure has no message", () => { + const r = interpretEnvelope(ok, { success: true, data: { success: false } }, "fallback", true); + expect(r.success).toBe(false); + expect(r.error).toBe("fallback"); + }); + + it("honours the reload flag on success", () => { + expect(interpretEnvelope(ok, { success: true, data: {} }, "f", false).reload).toBe(false); + }); +}); diff --git a/lib/record-actions.ts b/lib/record-actions.ts index 5edb0d1..bbd6520 100644 --- a/lib/record-actions.ts +++ b/lib/record-actions.ts @@ -98,6 +98,35 @@ async function readJson(res: Response): Promise | null> } } +/** + * Interpret a server response envelope into an {@link ActionResult}. + * + * Automation / action routes wrap the handler's own result, so a transport-level + * 200 with `{ success: true }` can still carry an INNER `{ success: false, + * error }` in `data` (e.g. "Action 'x' not found"). Checking only the outer + * flag reports those failures as success — so we unwrap one level and honour the + * inner `success` too, lifting the inner `error` for display. + */ +export function interpretEnvelope( + res: Pick, + json: Record | null, + fallbackError: string, + reload: boolean, +): ActionResult { + if (!res.ok || json?.success === false) { + return { success: false, error: (json?.error as string) ?? fallbackError }; + } + const inner = json?.data; + if (inner && typeof inner === "object" && (inner as { success?: unknown }).success === false) { + const innerObj = inner as { error?: unknown }; + return { + success: false, + error: typeof innerObj.error === "string" ? innerObj.error : fallbackError, + }; + } + return { success: true, data: inner, reload }; +} + /* ---- Per-type dispatch ------------------------------------------------- */ async function dispatchUrl( @@ -136,13 +165,12 @@ async function dispatchFlow( }, ); const json = await readJson(res); - if (!res.ok || json?.success === false) { - return { - success: false, - error: (json?.error as string) ?? `Flow "${flowName}" failed (HTTP ${res.status})`, - }; - } - return { success: true, data: json?.data, reload: action.refreshAfter !== false }; + return interpretEnvelope( + res, + json, + `Flow "${flowName}" failed (HTTP ${res.status})`, + action.refreshAfter !== false, + ); } async function dispatchApi( @@ -160,13 +188,12 @@ async function dispatchApi( body: JSON.stringify(params), }); const json = await readJson(res); - if (!res.ok || json?.success === false) { - return { - success: false, - error: (json?.error as string) ?? `Request failed (HTTP ${res.status})`, - }; - } - return { success: true, data: json?.data, reload: action.refreshAfter !== false }; + return interpretEnvelope( + res, + json, + `Request failed (HTTP ${res.status})`, + action.refreshAfter !== false, + ); } // Generic record mutation with the collected params. if (Object.keys(params).length > 0) { @@ -191,13 +218,12 @@ async function dispatchServerAction( }, ); const json = await readJson(res); - if (!res.ok || json?.success === false) { - return { - success: false, - error: (json?.error as string) ?? `Action "${target}" failed (HTTP ${res.status})`, - }; - } - return { success: true, data: json?.data, reload: action.refreshAfter === true }; + return interpretEnvelope( + res, + json, + `Action "${target}" failed (HTTP ${res.status})`, + action.refreshAfter === true, + ); } /* ---- Lifecycle --------------------------------------------------------- */