diff --git a/.changeset/actions-envelope-single-source.md b/.changeset/actions-envelope-single-source.md new file mode 100644 index 0000000000..152cf2ae9d --- /dev/null +++ b/.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. diff --git a/packages/app-shell/src/actions-envelope.ratchet.test.ts b/packages/app-shell/src/actions-envelope.ratchet.test.ts new file mode 100644 index 0000000000..0bfda44ac1 --- /dev/null +++ b/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([ + 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'), + ]), + ); + }); +}); diff --git a/packages/app-shell/src/hooks/__tests__/useConsoleActionRuntime.test.tsx b/packages/app-shell/src/hooks/__tests__/useConsoleActionRuntime.test.tsx index 47f1737d54..f6a1740f60 100644 --- a/packages/app-shell/src/hooks/__tests__/useConsoleActionRuntime.test.tsx +++ b/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: }}`. + // 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. diff --git a/packages/app-shell/src/hooks/useConsoleActionRuntime.tsx b/packages/app-shell/src/hooks/useConsoleActionRuntime.tsx index 8bdd907b10..43499737fd 100644 --- a/packages/app-shell/src/hooks/useConsoleActionRuntime.tsx +++ b/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) { diff --git a/packages/app-shell/src/utils/__tests__/actionResponse.test.ts b/packages/app-shell/src/utils/__tests__/actionResponse.test.ts new file mode 100644 index 0000000000..cc4a405e20 --- /dev/null +++ b/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]); + }); +}); diff --git a/packages/app-shell/src/utils/actionResponse.ts b/packages/app-shell/src/utils/actionResponse.ts new file mode 100644 index 0000000000..54a072e1a3 --- /dev/null +++ b/packages/app-shell/src/utils/actionResponse.ts @@ -0,0 +1,102 @@ +/** + * The ONE place the console interprets a `POST /api/v1/actions/...` response. + * + * This existed twice — `useConsoleActionRuntime.serverActionHandler` and + * `RecordDetailView`'s copy of the same handler — and the two drifted, which is + * the whole reason objectstack#3913 was filed against the console: one copy + * learned to inspect the inner envelope and the other did not, so a failed + * action fired a green "completed" toast on every list/page surface. Two copies + * of a subtle envelope rule is a bug generator; this is the rule, once. + * + * ## The envelope is DOUBLE, and that is the trap + * + * ``` + * { ← transport envelope + * success: true, + * data: { ← action envelope + * success: true, + * data: { … } ← what the handler actually returned + * } + * } + * ``` + * + * Unlike `/api/v1/`, which wraps ONCE, `/actions` wraps twice — + * the route's own `{success, data}` sits inside the dispatcher's. Reading one + * level (the natural instinct, and what the other handlers correctly do) lands + * on the action envelope, not the payload. + * + * ## Failure has three shapes, and `res.ok` alone catches only one + * + * - **dispatch failure** — 404 no such action, 403 denied, 400 wrong action + * type, 503 unavailable, 500 the handler crashed. `res.ok` is false. + * - **business rejection** — the handler ran and said no. HTTP **200** with + * `data.success === false`. `res.ok` is TRUE; only the inner envelope shows + * it. This is the one that got missed. + * - **transport-level `success: false`** — the outer envelope reporting a + * failure directly. + */ + +import { actionErrorDetail } from './actionErrorDetail'; + +export interface ActionResponseOutcome { + ok: boolean; + /** Human-readable failure text. Always a string when `ok` is false. */ + error?: string; + /** + * The ACTION ENVELOPE (`body.data`) — `{ success, data }`. Kept as-is + * because `ActionResult.data` has always carried this level and + * `resultDialog` field paths in the wild may compensate for it; see the + * note in {@link readActionPayload}. + */ + envelope?: unknown; + /** What the handler actually returned (`body.data.data`). */ + payload?: unknown; +} + +/** + * The handler's own return value — one level deeper than the action envelope. + * + * `body.data` is `{ success, data }`; the handler's value is `body.data.data`. + * Reading the shallower level is why an action returning `{ redirectUrl }` was + * silently ignored: `body.data.redirectUrl` is never set by anything, because + * that level is constructed by the server and only ever holds `success`/`data`. + */ +export function readActionPayload(envelope: unknown): unknown { + if (envelope && typeof envelope === 'object' && !Array.isArray(envelope) && 'success' in envelope) { + return (envelope as { data?: unknown }).data; + } + // A server that did not double-wrap (or a stubbed response in a test): + // treat what we have as the payload rather than inventing an undefined. + return envelope; +} + +/** + * Classify an `/actions` response. `label` names the action for the fallback + * message (e.g. `Action "log_call" failed`). + * + * `json` is the parsed body, or `null` when it could not be parsed. + */ +export function interpretActionResponse( + res: { ok: boolean; status: number }, + json: any, + label: string, +): ActionResponseOutcome { + const envelope = json?.data; + const innerFailed = !!envelope && typeof envelope === 'object' && (envelope as any).success === false; + + if (!res.ok || (json && json.success === false) || innerFailed) { + // A business rejection carries its message on the INNER envelope; a + // dispatch failure carries it on the outer one (as a nested + // `{message, code}` object, which `actionErrorDetail` resolves to a + // string — handing that object to `toast.error()` as a React child + // crashes the page, React #31). + return { + ok: false, + error: innerFailed + ? actionErrorDetail(envelope, `${label} failed`) + : actionErrorDetail(json, `${label} failed (HTTP ${res.status})`), + }; + } + + return { ok: true, envelope, payload: readActionPayload(envelope) }; +} diff --git a/packages/app-shell/src/views/RecordDetailView.tsx b/packages/app-shell/src/views/RecordDetailView.tsx index bd47243a61..6d9ba7b79b 100644 --- a/packages/app-shell/src/views/RecordDetailView.tsx +++ b/packages/app-shell/src/views/RecordDetailView.tsx @@ -33,7 +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 { interpretActionResponse } from '../utils/actionResponse'; import { useRecordBreadcrumbTitle } from '../context/NavigationContext'; import type { FeedItem } from '@object-ui/types'; import type { ActionDef, ActionParamDef } from '@object-ui/core'; @@ -759,34 +759,23 @@ 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 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) { - // 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})`); + // Single source for the `/actions` envelope rule — shared with + // useConsoleActionRuntime, from which this copy drifted (it learned to + // inspect the inner envelope; the shared runtime had not, which is + // 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. This handler always runs through the ActionRunner // (registered as the `script` handler on the ActionProvider below, which // wires `onToast`), whose post-execution hook surfaces the returned // `error` as one toast. Toasting again double-fired the message // (e.g. RECORD_LOCKED appeared twice). Mirrors useConsoleActionRuntime. - return { success: false, error: errMsg }; + return { success: false, error: outcome.error }; } const shouldRefresh = action.refreshAfter !== false; if (shouldRefresh) notifyRecordChanged(); - const result = json?.data; + const result = outcome.envelope; // ── redirectUrl convention ──────────────────────────────────────── // A script-action handler can return `{ redirectUrl: 'https://…' }` // to ask the UI to open the URL. If the action declared @@ -794,8 +783,13 @@ export function RecordDetailView({ dataSource, objects, onEdit, objectNameOverri // (popup-blocker-safe). Otherwise we open lazily and, if blocked, // fall back to navigating the current tab so the user always gets // to the destination. - if (result && typeof result === 'object' && typeof (result as any).redirectUrl === 'string') { - const redirectUrl = (result as any).redirectUrl as string; + // + // Read 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 the convention never fired at all. + const payload = outcome.payload; + if (payload && typeof payload === 'object' && typeof (payload as any).redirectUrl === 'string') { + const redirectUrl = (payload as any).redirectUrl as string; if (preOpenedTab) { try { preOpenedTab.location.href = redirectUrl; } catch { try { preOpenedTab.close(); } catch { /* ignore */ }