fix(actions): a failed server action no longer reports as success (green toast)

.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.

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;

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' }),

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

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;
+}

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