fix(actions): one source for the /actions envelope rule, and redirectUrl finally works (#2967)

objectstack#3913 follow-up. The /actions response wraps TWICE — the route's own
{success, data} inside the dispatcher's — and a failure has three shapes, only
one of which res.ok catches. That rule was hand-rolled in two places
(useConsoleActionRuntime.serverActionHandler and RecordDetailView's copy of the
same handler) and the two drifted. Four hand-rolled copies produced three
distinct bugs: a failed action reported as success (the green-toast symptom, on
the console's MAIN action path), a React #31 crash from the nested {message}
object reaching toast.error() as a child, and redirectUrl never firing.

Both handlers now call interpretActionResponse from utils/actionResponse, and a
ratchet test fails if a third hand-rolled copy appears. The ratchet also asserts
it is still scanning the two known callers, so it cannot start passing vacuously
if the files move.

redirectUrl was unreachable, not merely wrong. A script action can return
{ redirectUrl } to ask the console to open a URL; both handlers read it off
body.data — the ACTION envelope — but that level is constructed by the server
and only ever holds success/data, so body.data.redirectUrl was always undefined
and no handler could work around it. An opensInNewTab action was worse than a
no-op: it pre-opens a tab on a spinner page for popup-blocker safety, and with
no redirect to drive it to, that tab sat on the spinner forever.

ActionResult.data still carries the action envelope, unchanged — resultDialog
field paths in the wild may have adapted to that depth, so it is not silently
re-pointed here.

Author: os-zhuang

.changeset/actions-envelope-single-source.md

@@ -0,0 +1,44 @@
+---
+"@object-ui/app-shell": patch
+---
+
+fix(actions): one source for the `/actions` envelope rule, and `redirectUrl` finally works (objectstack#3913 follow-up)
+
+The `/actions` response wraps **twice** — the route's own `{success, data}`
+inside the dispatcher's — and a failure has three shapes, only one of which
+`res.ok` catches. That rule was hand-rolled in two places
+(`useConsoleActionRuntime.serverActionHandler` and `RecordDetailView`'s copy of
+the same handler), and the two drifted. Four hand-rolled copies produced three
+distinct bugs:
+
+1. **A failed action reported as success** — the copy that didn't inspect the
+   inner envelope was the console's *main* action path, so a failure fired the
+   green "completed" toast on every list and page surface (fixed in #2963).
+2. **React #31 crash** — the nested `{message, code}` object handed to
+   `toast.error()` as a React child (fixed in #2963).
+3. **`redirectUrl` never fired** — *fixed here.*
+
+Both handlers now call `interpretActionResponse` from `utils/actionResponse`,
+and a ratchet test (`actions-envelope.ratchet.test.ts`) fails if a third
+hand-rolled copy appears.
+
+## `redirectUrl` was unreachable
+
+A script action can return `{ redirectUrl: 'https://…' }` to ask the console to
+open a URL. Both handlers read it off `body.data` — the **action** envelope,
+one level too shallow:
+
+```
+{ success: true, data: { success: true, data: { redirectUrl: '…' } } }
+                 ^^^^ read here          ^^^^ actually lives here
+```
+
+`body.data` is constructed by the server and only ever holds `success` / `data`,
+so `body.data.redirectUrl` was **always** undefined — the convention could never
+fire, and no handler could work around it. An `opensInNewTab` action was worse
+than a no-op: it pre-opens a tab on a spinner page for popup-blocker safety, and
+with no redirect to drive it to, that tab sat on the spinner forever.
+
+`ActionResult.data` still carries the **action envelope**, unchanged — some
+`resultDialog` field paths in the wild may have adapted to that depth, so it is
+not silently re-pointed here.

packages/app-shell/src/actions-envelope.ratchet.test.ts

@@ -0,0 +1,95 @@
+/**
+ * ObjectUI
+ * Copyright (c) 2024-present ObjectStack Inc.
+ *
+ * objectstack#3913 ratchet — every `/api/v1/actions` caller goes through
+ * `interpretActionResponse`.
+ *
+ * The bug this guards: the `/actions` response wraps TWICE (the route's own
+ * `{success, data}` inside the dispatcher's), and a failure has three shapes
+ * only one of which `res.ok` catches. Two copies of that rule existed — the
+ * shared console runtime and RecordDetailView's — and they drifted: one learned
+ * to inspect the inner envelope, the other did not. The one that did not was
+ * the console's MAIN action path, so a failed action fired a green "completed"
+ * toast on every list and page surface while the real error was swallowed.
+ * `marketplaceApi.installPackage` had the same hole and could report a package
+ * as installed when it was not.
+ *
+ * Reading the envelope by hand is the anti-pattern, not any particular way of
+ * reading it wrong: four hand-rolled copies produced three different bugs
+ * (missed inner failure, a `{message}` object handed to `toast.error()` as a
+ * React child → React #31, and `redirectUrl` read one level too shallow so it
+ * never fired). One helper, one rule.
+ *
+ * If this fails: don't hand-roll the check. Import `interpretActionResponse`
+ * from `utils/actionResponse` — or, better, call the action through
+ * `@objectstack/client`, which folds every shape into `{ success, data?, error? }`.
+ */
+
+import { describe, it, expect } from 'vitest';
+import { readdirSync, readFileSync, statSync } from 'node:fs';
+import path from 'node:path';
+import { fileURLToPath } from 'node:url';
+
+const here = path.dirname(fileURLToPath(import.meta.url));
+const appShellSrc = here;
+
+/** The helper that owns the rule — exempt from its own guard. */
+const OWNER = path.join(appShellSrc, 'utils', 'actionResponse.ts');
+
+/** Files allowed to name the route without going through the helper. */
+const EXEMPT = new Set<string>([
+    OWNER,
+    // The cloud marketplace install posts to a *cloud* origin's action route
+    // and consumes `InstallResponse`, not `ActionResult`. It still has to
+    // honour the envelope, and does — covered by its own tests — but it is not
+    // an ActionRunner handler, so it does not use this helper.
+    path.join(appShellSrc, 'console', 'marketplace', 'marketplaceApi.ts'),
+]);
+
+function walk(dir: string, out: string[] = []): string[] {
+    for (const entry of readdirSync(dir)) {
+        if (entry === 'node_modules' || entry === 'dist' || entry === '__tests__') continue;
+        const full = path.join(dir, entry);
+        if (statSync(full).isDirectory()) walk(full, out);
+        else if (/\.(ts|tsx)$/.test(entry) && !/\.test\.tsx?$/.test(entry)) out.push(full);
+    }
+    return out;
+}
+
+/** A source file that POSTs to the action route. */
+const ROUTE = /\/api\/v1\/actions\//;
+
+/**
+ * Comments name the route freely while explaining the routing rules (e.g.
+ * ConsoleShell's note on why `modal` stays client-side). Only CODE counts.
+ */
+function stripComments(src: string): string {
+    return src
+        .replace(/\/\*[\s\S]*?\*\//g, '')
+        .replace(/(^|[^:])\/\/.*$/gm, '$1');
+}
+
+describe('objectstack#3913 ratchet — /actions callers use interpretActionResponse', () => {
+    const callers = walk(appShellSrc)
+        .filter((f) => !EXEMPT.has(f) && ROUTE.test(stripComments(readFileSync(f, 'utf8'))));
+
+    const offenders = callers
+        .filter((f) => !readFileSync(f, 'utf8').includes('interpretActionResponse'))
+        .map((f) => path.relative(appShellSrc, f));
+
+    it('no app-shell file interprets an /actions response by hand', () => {
+        expect(offenders).toEqual([]);
+    });
+
+    it('still guards something — the known callers are present', () => {
+        // A ratchet that matches nothing passes vacuously forever. Pin that the
+        // two handlers it exists for are actually being scanned.
+        expect(callers.map((f) => path.relative(appShellSrc, f))).toEqual(
+            expect.arrayContaining([
+                path.join('hooks', 'useConsoleActionRuntime.tsx'),
+                path.join('views', 'RecordDetailView.tsx'),
+            ]),
+        );
+    });
+});

packages/app-shell/src/hooks/__tests__/useConsoleActionRuntime.test.tsx

@@ -358,6 +358,32 @@ describe('useConsoleActionRuntime — authenticated handlers', () => {
     expect(res.error).toBe("Action 'log_call' on object 'global' not found");
   });
 
+  it('serverActionHandler opens a handler-returned redirectUrl (read through BOTH envelopes)', async () => {
+    // The action route wraps twice: `{success, data:{success, data: <handler>}}`.
+    // This used to read `redirectUrl` off the ACTION envelope — a level where
+    // only `success`/`data` ever live — so the convention never fired and an
+    // `opensInNewTab` action left its pre-opened tab on the spinner forever.
+    const openSpy = vi.spyOn(window, 'open').mockReturnValue({} as any);
+    authFetchSpy.mockResolvedValue({
+      ok: true,
+      status: 200,
+      json: async () => ({
+        success: true,
+        data: { success: true, data: { redirectUrl: 'https://example.test/sso' } },
+      }),
+    });
+    const { result } = renderHook(() =>
+      useConsoleActionRuntime({ dataSource: {}, objects: [], objectName: 'crm_call' }),
+    );
+
+    await act(async () => {
+      await result.current.serverActionHandler({ type: 'script', name: 'open_env' } as any);
+    });
+
+    expect(openSpy).toHaveBeenCalledWith('https://example.test/sso', '_blank');
+    openSpy.mockRestore();
+  });
+
   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.

packages/app-shell/src/hooks/useConsoleActionRuntime.tsx

@@ -49,6 +49,7 @@ import { EnvironmentEntitlementDialog, type EntitlementDialogState } from '../en
 import { entitlementDialogFromError, type EntitlementDialogSpec } from '../environment/entitlements';
 import { resolvePageVarTokens } from '../utils/resolvePageVarTokens';
 import { actionErrorDetail } from '../utils/actionErrorDetail';
+import { interpretActionResponse } from '../utils/actionResponse';
 
 const FALLBACK_USER = { id: 'current-user', name: 'Demo User', isPlatformAdmin: false };
 
@@ -587,32 +588,28 @@ export function useConsoleActionRuntime(opts: ConsoleActionRuntimeOptions): Cons
         },
       );
       const json = await res.json().catch(() => null);
-      // 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})`);
+      // Single source for the `/actions` envelope rule — shared with
+      // RecordDetailView, whose copy of this handler drifted from it and caused
+      // objectstack#3913's console symptom. See utils/actionResponse.
+      const outcome = interpretActionResponse(res, json, `Action "${targetName}"`);
+      if (!outcome.ok) {
         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
         // return). Toasting here too double-fires the error (two identical toasts).
-        return { success: false, error: errMsg };
+        return { success: false, error: outcome.error };
       }
       const shouldRefresh = action.refreshAfter !== false;
       if (shouldRefresh) refresh();
-      const data = json?.data;
-      const redirectUrl = (data && typeof data === 'object' && typeof (data as any).redirectUrl === 'string')
-        ? (data as any).redirectUrl as string
+      const data = outcome.envelope;
+      // Read `redirectUrl` off the HANDLER's return value, not the action
+      // envelope wrapping it. This used to read one level too shallow, where
+      // only `success`/`data` ever live — so an action returning
+      // `{ redirectUrl }` was silently ignored and an `opensInNewTab` action
+      // left its pre-opened tab parked on the spinner page forever.
+      const payload = outcome.payload;
+      const redirectUrl = (payload && typeof payload === 'object' && typeof (payload as any).redirectUrl === 'string')
+        ? (payload as any).redirectUrl as string
         : null;
       if (redirectUrl) {
         if (preOpenedTab) {

packages/app-shell/src/utils/__tests__/actionResponse.test.ts

@@ -0,0 +1,89 @@
+/**
+ * ObjectUI
+ * Copyright (c) 2024-present ObjectStack Inc.
+ *
+ * The `/actions` envelope rule, once (objectstack#3913). Four hand-rolled
+ * copies of this produced three distinct bugs; these cases pin all three.
+ */
+
+import { describe, it, expect } from 'vitest';
+import { interpretActionResponse, readActionPayload } from '../actionResponse';
+
+const ok = { ok: true, status: 200 };
+
+describe('interpretActionResponse — failure shapes', () => {
+    it('catches a business rejection hiding under HTTP 200 (the reported bug)', () => {
+        // `res.ok` is TRUE and the OUTER `success` is TRUE. Only the inner
+        // envelope reports the failure — miss it and the ActionRunner fires a
+        // green "completed" toast over a failed action.
+        const out = interpretActionResponse(ok, {
+            success: true,
+            data: { success: false, error: "Action 'log_call' on object '*' not found" },
+        }, 'Action "log_call"');
+
+        expect(out.ok).toBe(false);
+        expect(out.error).toBe("Action 'log_call' on object '*' not found");
+    });
+
+    it('catches a dispatch failure and resolves the nested {message} to a STRING', () => {
+        // Handing the `{message, code}` OBJECT to toast.error() as a React
+        // child crashes the page (React #31).
+        const out = interpretActionResponse({ ok: false, status: 404 }, {
+            success: false,
+            error: { message: "Action 'log_call' on object 'global' not found", code: 404 },
+        }, 'Action "log_call"');
+
+        expect(out.ok).toBe(false);
+        expect(typeof out.error).toBe('string');
+        expect(out.error).toBe("Action 'log_call' on object 'global' not found");
+    });
+
+    it('catches a transport-level success:false', () => {
+        const out = interpretActionResponse(ok, { success: false, error: 'nope' }, 'Action "x"');
+        expect(out).toMatchObject({ ok: false, error: 'nope' });
+    });
+
+    it('falls back to a labelled message when the body carries none', () => {
+        const out = interpretActionResponse({ ok: false, status: 500 }, null, 'Action "x"');
+        expect(out.ok).toBe(false);
+        expect(out.error).toBe('Action "x" failed (HTTP 500)');
+    });
+});
+
+describe('interpretActionResponse — success', () => {
+    it('reports ok and exposes both envelope levels', () => {
+        const out = interpretActionResponse(ok, {
+            success: true,
+            data: { success: true, data: { id: 'call_1' } },
+        }, 'Action "log_call"');
+
+        expect(out.ok).toBe(true);
+        // `envelope` is the ACTION envelope — the level ActionResult.data has
+        // always carried.
+        expect(out.envelope).toEqual({ success: true, data: { id: 'call_1' } });
+        // `payload` is what the handler actually returned.
+        expect(out.payload).toEqual({ id: 'call_1' });
+    });
+});
+
+describe('readActionPayload — the double-wrap trap', () => {
+    it('reaches the handler value through BOTH envelopes', () => {
+        // The bug: reading one level lands on `{success, data}`, where only
+        // `success` and `data` ever live — so an action returning
+        // `{ redirectUrl }` was silently ignored, because
+        // `body.data.redirectUrl` is never set by anything.
+        const envelope = { success: true, data: { redirectUrl: 'https://example.test/sso' } };
+
+        expect((envelope as any).redirectUrl).toBeUndefined();       // the old read
+        expect(readActionPayload(envelope)).toEqual({ redirectUrl: 'https://example.test/sso' });
+    });
+
+    it('passes a non-enveloped body through rather than inventing undefined', () => {
+        expect(readActionPayload({ id: 'x' })).toEqual({ id: 'x' });
+        expect(readActionPayload(null)).toBeNull();
+    });
+
+    it('does not mistake an array for an envelope', () => {
+        expect(readActionPayload([1, 2])).toEqual([1, 2]);
+    });
+});