From 60a057f1712a11fc297c84309c11f7ac0efb08a3 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 29 Jul 2026 10:49:09 +0000 Subject: [PATCH] fix(actions): a failed server action no longer reports as success (green toast) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit objectstack#3913. `useConsoleActionRuntime.serverActionHandler` — the console's main action path (list toolbars, row actions, page actions) — decided success from `res.ok` and the OUTER envelope only. A server older than objectstack#3913 reports a handler failure as HTTP 200 with the failure nested one level down: {"success":true,"data":{"success":false,"error":"Action '…' not found"}} Both guards pass, so the action was reported as completed: the ActionRunner fired its green "completed" toast, the list refreshed, and the real error was swallowed. RecordDetailView's copy of the handler already inspected the inner envelope; the shared runtime now does too, as does marketplaceApi.installPackage, which had the identical hole and could report a package as installed when it was not. Current servers answer a failed action with a real HTTP status, which `!res.ok` catches first — the inner-envelope check is what keeps the console honest against a runtime that has not been upgraded yet. Also: with objectstack#3913 the failure body is {success:false, error:{message, code}}. RecordDetailView read `json?.error` raw and would have handed that OBJECT to toast.error() as a React child, crashing the page (React #31) — the exact failure the runtime's `errorDetail` helper existed to prevent. That helper is now a shared util (utils/actionErrorDetail) and both call sites use it. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011AvZj6cLX7APd7roh2eK4F --- .changeset/action-failure-inner-envelope.md | 39 ++++++++++ .../src/console/marketplace/marketplaceApi.ts | 15 +++- .../useConsoleActionRuntime.test.tsx | 73 +++++++++++++++++++ .../src/hooks/useConsoleActionRuntime.tsx | 35 +++++---- .../app-shell/src/utils/actionErrorDetail.ts | 23 ++++++ .../app-shell/src/views/RecordDetailView.tsx | 20 +++-- 6 files changed, 182 insertions(+), 23 deletions(-) create mode 100644 .changeset/action-failure-inner-envelope.md create mode 100644 packages/app-shell/src/utils/actionErrorDetail.ts diff --git a/.changeset/action-failure-inner-envelope.md b/.changeset/action-failure-inner-envelope.md new file mode 100644 index 0000000000..f695206a66 --- /dev/null +++ b/.changeset/action-failure-inner-envelope.md @@ -0,0 +1,39 @@ +--- +"@object-ui/app-shell": patch +--- + +fix(actions): a failed server action no longer reports as success (green toast) — objectstack#3913 + +`useConsoleActionRuntime.serverActionHandler` — the console's **main** action +path (list toolbars, row actions, page actions) — decided success from +`res.ok` and the OUTER envelope only: + +```ts +if (!res.ok || (json && json.success === false)) { /* failure */ } +``` + +A server older than objectstack#3913 reports a handler failure as HTTP **200** +with the failure nested one level down: + +```json +{"success":true,"data":{"success":false,"error":"Action 'log_call' on object '*' not found"}} +``` + +Both guards pass, so the action was reported as completed: the ActionRunner +fired its green "completed" toast, the list refreshed, and the real error was +swallowed. `RecordDetailView`'s copy of the same handler already inspected the +inner envelope; the shared runtime now does too, and the marketplace install +call (`marketplaceApi.installPackage`), which had the identical hole and could +report a package as installed when it was not. + +Current servers answer a failed action with a real HTTP status, which `!res.ok` +catches first — the inner-envelope check is what keeps the console honest +against a runtime that has not been upgraded yet. + +**Also fixed:** with objectstack#3913 the failure body is +`{success: false, error: {message, code}}`. `RecordDetailView` read `json?.error` +raw and would have handed that **object** to `toast.error()` as a React child, +crashing the page (React #31) — the exact failure the console runtime's +`errorDetail` helper existed to prevent. That helper is now a shared util +(`utils/actionErrorDetail`) and both call sites go through it, so a nested +`{message}` always resolves to a string. diff --git a/packages/app-shell/src/console/marketplace/marketplaceApi.ts b/packages/app-shell/src/console/marketplace/marketplaceApi.ts index fb35b6bfd2..4acc9ac60e 100644 --- a/packages/app-shell/src/console/marketplace/marketplaceApi.ts +++ b/packages/app-shell/src/console/marketplace/marketplaceApi.ts @@ -248,9 +248,20 @@ export async function installPackage(input: { }); let payload: any = null; try { payload = await res.json(); } catch { /* empty */ } - if (!res.ok) { + // A server older than objectstack#3913 reports a failed install action as + // HTTP 200 `{success: true, data: {success: false, error}}` — checking only + // `res.ok` reported a package as installed when it was not. Current servers + // answer with a real status, which `!res.ok` catches first. + const inner = payload?.data; + const innerFailed = !!inner && typeof inner === 'object' && inner.success === false; + if (!res.ok || innerFailed) { const code = payload?.code ?? payload?.error?.code ?? `HTTP_${res.status}`; - const message = payload?.error ?? payload?.message ?? res.statusText; + // `error` is a nested `{code, message}` object on the current wire and a + // plain string on the older one — read the message out of both before + // falling back, or a real explanation degrades into a bare status code. + const message = innerFailed + ? (inner.error ?? res.statusText) + : (payload?.error?.message ?? payload?.error ?? payload?.message ?? res.statusText); const err = new Error(typeof message === 'string' ? message : `${code}`); (err as any).code = code; (err as any).status = res.status; diff --git a/packages/app-shell/src/hooks/__tests__/useConsoleActionRuntime.test.tsx b/packages/app-shell/src/hooks/__tests__/useConsoleActionRuntime.test.tsx index 41bd18fb14..47f1737d54 100644 --- a/packages/app-shell/src/hooks/__tests__/useConsoleActionRuntime.test.tsx +++ b/packages/app-shell/src/hooks/__tests__/useConsoleActionRuntime.test.tsx @@ -305,6 +305,79 @@ describe('useConsoleActionRuntime — authenticated handlers', () => { expect((toast as any).error).not.toHaveBeenCalled(); }); + it('serverActionHandler treats an INNER success:false as a failure (objectstack#3913 — no green toast on a failed action)', async () => { + // A server older than objectstack#3913 wraps a handler failure as HTTP 200 + // `{success: true, data: {success: false, error}}`. Reading only `res.ok` + // and the OUTER `success` reported that as a completed action and fired the + // green "completed" toast while swallowing the real error — the reported + // bug. The console must inspect the inner envelope for those servers. + authFetchSpy.mockResolvedValue({ + ok: true, + status: 200, + json: async () => ({ + success: true, + data: { success: false, error: "Action 'log_call' on object '*' not found" }, + }), + }); + const { result } = renderHook(() => + useConsoleActionRuntime({ dataSource: {}, objects: [], objectName: 'crm_call' }), + ); + + let res: any; + await act(async () => { + res = await result.current.serverActionHandler({ type: 'script', name: 'log_call' } as any); + }); + + expect(res).toEqual({ success: false, error: "Action 'log_call' on object '*' not found" }); + expect((toast as any).error).not.toHaveBeenCalled(); + }); + + it('serverActionHandler resolves a nested {error:{message}} to a STRING (objectstack#3913 wire)', async () => { + // Current servers answer a failed action with a real status and the nested + // envelope. Passing that object through as `ActionResult.error` reaches + // `toast.error()` as a React child and crashes the page (React #31). + authFetchSpy.mockResolvedValue({ + ok: false, + status: 404, + json: async () => ({ + success: false, + error: { message: "Action 'log_call' on object 'global' not found", code: 404 }, + }), + }); + const { result } = renderHook(() => + useConsoleActionRuntime({ dataSource: {}, objects: [] }), + ); + + let res: any; + await act(async () => { + res = await result.current.serverActionHandler({ type: 'script', name: 'log_call' } as any); + }); + + expect(res.success).toBe(false); + expect(typeof res.error).toBe('string'); + expect(res.error).toBe("Action 'log_call' on object 'global' not found"); + }); + + it('serverActionHandler still reports success when the inner envelope says so', async () => { + // The success wire is unchanged by objectstack#3913 — guard against the + // inner-envelope check turning a good action into a failure. + authFetchSpy.mockResolvedValue({ + ok: true, + status: 200, + json: async () => ({ success: true, data: { success: true, data: { id: 'call_1' } } }), + }); + const { result } = renderHook(() => + useConsoleActionRuntime({ dataSource: {}, objects: [], objectName: 'crm_call' }), + ); + + let res: any; + await act(async () => { + res = await result.current.serverActionHandler({ type: 'script', name: 'log_call' } as any); + }); + + expect(res).toMatchObject({ success: true }); + }); + it('exposes ActionProvider props with the api/flow/script/modal handlers wired', () => { const { result } = renderHook(() => useConsoleActionRuntime({ dataSource: {}, objects: [], objectName: 'inv' }), diff --git a/packages/app-shell/src/hooks/useConsoleActionRuntime.tsx b/packages/app-shell/src/hooks/useConsoleActionRuntime.tsx index b82c52e9a4..8bdd907b10 100644 --- a/packages/app-shell/src/hooks/useConsoleActionRuntime.tsx +++ b/packages/app-shell/src/hooks/useConsoleActionRuntime.tsx @@ -48,6 +48,7 @@ import { resolveActionParams } from '../utils/resolveActionParams'; import { EnvironmentEntitlementDialog, type EntitlementDialogState } from '../environment/EnvironmentEntitlementDialog'; import { entitlementDialogFromError, type EntitlementDialogSpec } from '../environment/entitlements'; import { resolvePageVarTokens } from '../utils/resolvePageVarTokens'; +import { actionErrorDetail } from '../utils/actionErrorDetail'; const FALLBACK_USER = { id: 'current-user', name: 'Demo User', isPlatformAdmin: false }; @@ -68,20 +69,11 @@ function isRecordScoped(action: ActionDef): boolean { } /** - * Extract a human-readable message from an error response body. The - * ObjectStack envelope nests it as `{ error: { code, message } }` — passing - * that object through as `ActionResult.error` reaches `toast.error()` as a - * React child and crashes the page (React #31). Always resolve to a string. + * Extract a human-readable message from an error response body — shared with + * `RecordDetailView`, which runs the same `/actions` request and needs the same + * React-#31 guard. See `../utils/actionErrorDetail`. */ -function errorDetail(body: unknown, fallback: string): string { - const b = body as { error?: unknown; message?: unknown } | null; - const err = b?.error; - if (typeof err === 'string' && err.length > 0) return err; - const nested = (err as { message?: unknown } | null)?.message; - if (typeof nested === 'string' && nested.length > 0) return nested; - if (typeof b?.message === 'string' && b.message.length > 0) return b.message; - return fallback; -} +const errorDetail = actionErrorDetail; export interface ConsoleActionRuntimeOptions { /** Adapter for generic CRUD / execute calls. */ @@ -595,8 +587,21 @@ export function useConsoleActionRuntime(opts: ConsoleActionRuntimeOptions): Cons }, ); const json = await res.json().catch(() => null); - if (!res.ok || (json && json.success === false)) { - const errMsg = errorDetail(json, `Action "${targetName}" failed (HTTP ${res.status})`); + // A server older than objectstack#3913 reports a script action that THREW + // as HTTP 200 `{success: true, data: {success: false, error}}` — transport + // success wrapping a business failure. Reading only `res.ok` and the OUTER + // `success` mistook that for a completed action and fired the green + // "completed" toast while swallowing the real error. Inspect the INNER + // envelope too (RecordDetailView's copy of this handler already did). + // Current servers answer with a real status, so `!res.ok` catches it + // first; this branch is what keeps the console honest against a server + // that has not been upgraded yet. + const inner = json?.data; + const innerFailed = !!inner && typeof inner === 'object' && (inner as any).success === false; + if (!res.ok || (json && json.success === false) || innerFailed) { + const errMsg = innerFailed + ? errorDetail(inner, `Action "${targetName}" failed`) + : errorDetail(json, `Action "${targetName}" failed (HTTP ${res.status})`); if (preOpenedTab) { try { preOpenedTab.close(); } catch { /* ignore */ } } // Don't toast here — the ActionRunner's post-execution hook surfaces // `error` as a toast (see apiHandler/flowHandler, which likewise only diff --git a/packages/app-shell/src/utils/actionErrorDetail.ts b/packages/app-shell/src/utils/actionErrorDetail.ts new file mode 100644 index 0000000000..544dbc4723 --- /dev/null +++ b/packages/app-shell/src/utils/actionErrorDetail.ts @@ -0,0 +1,23 @@ +/** + * Resolve a human-readable message out of an ObjectStack error payload. + * + * Always returns a STRING. The envelope nests the message as + * `{ error: { code, message } }`, and passing that object through as + * `ActionResult.error` reaches `toast.error()` as a React child and crashes the + * page (React #31). That shape used to be rare on the `/actions` route — + * failures came back as HTTP 200 with a plain-string `error` — but since + * objectstack#3913 a failed action answers with a real status and the nested + * object, so every `/actions` caller has to go through this. + * + * Handles, in order: `{error: 'msg'}`, `{error: {message: 'msg'}}`, + * `{message: 'msg'}`, else the caller's fallback. + */ +export function actionErrorDetail(body: unknown, fallback: string): string { + const b = body as { error?: unknown; message?: unknown } | null; + const err = b?.error; + if (typeof err === 'string' && err.length > 0) return err; + const nested = (err as { message?: unknown } | null)?.message; + if (typeof nested === 'string' && nested.length > 0) return nested; + if (typeof b?.message === 'string' && b.message.length > 0) return b.message; + return fallback; +} diff --git a/packages/app-shell/src/views/RecordDetailView.tsx b/packages/app-shell/src/views/RecordDetailView.tsx index 4f21a44096..bd47243a61 100644 --- a/packages/app-shell/src/views/RecordDetailView.tsx +++ b/packages/app-shell/src/views/RecordDetailView.tsx @@ -33,6 +33,7 @@ import { RelatedRecordActionsBridge } from './RelatedRecordActionsBridge'; import { withPageTabsUrlSync } from '../utils/pageTabsUrlSync'; import { RECORD_DETAIL_TAB_PARAM, RECORD_TRAIL_PARAM, decodeRecordTrail, buildRecordTrailHref } from '../urlParams'; import { resolveActionParams } from '../utils/resolveActionParams'; +import { actionErrorDetail } from '../utils/actionErrorDetail'; import { useRecordBreadcrumbTitle } from '../context/NavigationContext'; import type { FeedItem } from '@object-ui/types'; import type { ActionDef, ActionParamDef } from '@object-ui/core'; @@ -759,15 +760,22 @@ export function RecordDetailView({ dataSource, objects, onEdit, objectNameOverri ); const json = await res.json().catch(() => null); // The action route wraps the handler's return value in a {success, data} - // envelope. A script action that THROWS is reported as - // `data: { success: false, error }` while the OUTER success stays true, - // so we must inspect the inner envelope too — otherwise a failed action - // is mistaken for success and fires the green "completed" toast while the - // real error is swallowed. + // envelope. A server older than objectstack#3913 reports a script action + // that THROWS as `data: { success: false, error }` while the OUTER success + // stays true, so we must inspect the inner envelope too — otherwise a + // failed action is mistaken for success and fires the green "completed" + // toast while the real error is swallowed. Current servers answer with a + // real status, which `!res.ok` catches first. const inner = json?.data; const innerFailed = inner && typeof inner === 'object' && inner.success === false; if (!res.ok || (json && json.success === false) || innerFailed) { - const errMsg = (innerFailed && inner.error) || json?.error || `Action "${targetName}" failed (HTTP ${res.status})`; + // Always resolve to a STRING. Since objectstack#3913 a failed action + // answers with the nested `{error: {code, message}}` envelope — handing + // that object to `toast.error()` as a React child crashes the page + // (React #31), which the raw `json?.error` read here used to do. + const errMsg = innerFailed + ? actionErrorDetail(inner, `Action "${targetName}" failed`) + : actionErrorDetail(json, `Action "${targetName}" failed (HTTP ${res.status})`); if (preOpenedTab) { try { preOpenedTab.close(); } catch { /* ignore */ } } // Don't toast here. This handler always runs through the ActionRunner // (registered as the `script` handler on the ActionProvider below, which