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

.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]);
+    });
+});