diff --git a/packages/app-shell/src/flow-envelope.ratchet.test.ts b/packages/app-shell/src/flow-envelope.ratchet.test.ts new file mode 100644 index 000000000..8200f4005 --- /dev/null +++ b/packages/app-shell/src/flow-envelope.ratchet.test.ts @@ -0,0 +1,92 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * #2958 ratchet — every flow `trigger` / `resume` caller goes through + * `interpretFlowResponse`. + * + * The sibling of `actions-envelope.ratchet.test.ts`, for the same reason. The + * rule for reading an `AutomationResult` lived in THREE hand-rolled copies — + * the flow handlers in `useConsoleActionRuntime` and `RecordDetailView`, and + * `FlowRunner`'s resume — and only the resume copy was complete. The two launch + * copies validated the transport envelope and then treated everything else as + * terminal success, so a run that failed on its first node was reported as a + * completed one: no dialog, a green "completed successfully" toast, a refresh, + * and the real error swallowed. One copy also passed the error value through + * raw, where a nested `{code, message}` reaches `toast.error()` as a React child + * and crashes the page (React #31). + * + * If this fails: don't hand-roll the check. Import `interpretFlowResponse` from + * `utils/flowResponse` — it classifies transport failure / flow failure / + * screen pause / terminal success, and guarantees a string 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', 'flowResponse.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; +} + +/** + * Comments name routes freely while explaining them. Only CODE counts. + */ +function stripComments(src: string): string { + return src + .replace(/\/\*[\s\S]*?\*\//g, '') + .replace(/(^|[^:])\/\/.*$/gm, '$1'); +} + +/** + * A file that LAUNCHES or RESUMES a flow run — i.e. one that receives an + * `AutomationResult` to interpret. + * + * Deliberately narrower than "names the automation route": most automation + * callers READ the registry (`/connectors`, `/actions`, `/_status`, `/runs` for + * the observability panel) and get back a list, not a run result. Those have no + * envelope rule to get wrong. + */ +function runsAFlow(src: string): boolean { + const code = stripComments(src); + if (!/\/api\/v1\/automation\//.test(code)) return false; + return /\/trigger[`'"]/.test(code) || /\/resume[`'"]/.test(code); +} + +describe('#2958 ratchet — flow trigger/resume callers use interpretFlowResponse', () => { + const callers = walk(appShellSrc) + .filter((f) => f !== OWNER && runsAFlow(readFileSync(f, 'utf8'))); + + const offenders = callers + .filter((f) => !readFileSync(f, 'utf8').includes('interpretFlowResponse')) + .map((f) => path.relative(appShellSrc, f)); + + it('no app-shell file interprets a flow run result by hand', () => { + expect(offenders).toEqual([]); + }); + + it('still guards something — the three known callers are present', () => { + // A ratchet that matches nothing passes vacuously forever. Pin that the + // copies 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'), + path.join('views', 'FlowRunner.tsx'), + ]), + ); + }); +}); diff --git a/packages/app-shell/src/hooks/__tests__/useConsoleActionRuntime.test.tsx b/packages/app-shell/src/hooks/__tests__/useConsoleActionRuntime.test.tsx index 62a0e2e11..1c8cfdca0 100644 --- a/packages/app-shell/src/hooks/__tests__/useConsoleActionRuntime.test.tsx +++ b/packages/app-shell/src/hooks/__tests__/useConsoleActionRuntime.test.tsx @@ -662,6 +662,159 @@ describe('flowHandler — list_toolbar selection fallback', () => { }); }); +// #2958 — a business failure comes back HTTP 200 with the failure on the INNER +// envelope (`data.success === false`), and a failed flow launch carries neither +// `status` nor `screen`. Both used to land in the terminal-success return: the +// user saw a green "completed successfully" toast, the view refreshed, and the +// real error was swallowed. These pin the failure paths AND that the success +// paths they sit next to still work. +describe('#2958 — a failure reported under HTTP 200 is a failure, not success', () => { + it('flowHandler reports a failed launch (no status, no screen) instead of terminal success', async () => { + // The reported repro: a screen flow whose first CRUD node fails. Outer + // envelope says success; only `data.success` shows the truth. + authFetchSpy.mockResolvedValue({ + ok: true, + json: async () => ({ + success: true, + data: { success: false, error: "Node 'apply' failed: Update requires an ID" }, + }), + }); + const refreshSpy = vi.fn(); + const { result } = renderHook(() => + useConsoleActionRuntime({ dataSource: {}, objects: [], objectName: 'inv', onRefresh: refreshSpy }), + ); + + let res: any; + await act(async () => { + res = await result.current.flowHandler({ type: 'flow', name: 'convert_lead', target: 'convert_lead' } as any); + }); + + expect(res.success).toBe(false); + expect(res.error).toBe("Node 'apply' failed: Update requires an ID"); + // A failed run changed nothing — refreshing implies it did. + expect(refreshSpy).not.toHaveBeenCalled(); + // The ActionRunner's post-execution hook owns the error toast; the handler + // must not fire a second one. + expect((toast as any).error).not.toHaveBeenCalled(); + }); + + it('flowHandler prefers the flow-declared errorMessage over the raw engine error', async () => { + authFetchSpy.mockResolvedValue({ + ok: true, + json: async () => ({ + success: true, + data: { + success: false, + status: 'failed', + error: "Node 'apply' failed: constraint violation on lead.status", + errorMessage: 'This lead has already been converted.', + }, + }), + }); + const { result } = renderHook(() => useConsoleActionRuntime({ dataSource: {}, objects: [] })); + + let res: any; + await act(async () => { + res = await result.current.flowHandler({ type: 'flow', name: 'convert_lead', target: 'convert_lead' } as any); + }); + + expect(res).toMatchObject({ success: false, error: 'This lead has already been converted.' }); + }); + + it('flowHandler still OPENS the wizard on a screen pause (success path intact)', async () => { + // The engine stamps `success: true` alongside `status: 'paused'`, which is + // why classifying failure first cannot swallow a wizard. `silent: true` is + // the marker that the action only opened the runner. + authFetchSpy.mockResolvedValue({ + ok: true, + json: async () => ({ + success: true, + data: { + success: true, + status: 'paused', + runId: 'run-7', + screen: { nodeId: 'collect', title: 'New Assignee', fields: [] }, + }, + }), + }); + const refreshSpy = vi.fn(); + const { result } = renderHook(() => + useConsoleActionRuntime({ dataSource: {}, objects: [], onRefresh: refreshSpy }), + ); + + let res: any; + await act(async () => { + res = await result.current.flowHandler({ type: 'flow', name: 'f', target: 'f' } as any); + }); + + expect(res).toMatchObject({ success: true, silent: true }); + // The run hasn't completed — the runner refreshes on completion, not now. + expect(refreshSpy).not.toHaveBeenCalled(); + }); + + it('flowHandler coerces a nested {code, message} error to a string (React #31)', async () => { + // Handing the object through as `error` reaches `toast.error()` as a React + // child and crashes the page. + authFetchSpy.mockResolvedValue({ + ok: false, + status: 403, + json: async () => ({ + success: false, + error: { message: 'Run is parked on a service-owned node', code: 'PERMISSION_DENIED' }, + }), + }); + const { result } = renderHook(() => useConsoleActionRuntime({ dataSource: {}, objects: [] })); + + let res: any; + await act(async () => { + res = await result.current.flowHandler({ type: 'flow', name: 'f', target: 'f' } as any); + }); + + expect(typeof res.error).toBe('string'); + expect(res.error).toBe('Run is parked on a service-owned node'); + }); + + it('apiHandler reports an HTTP-200 success:false body instead of refreshing on it', async () => { + // The `log_call`-style repro on the api transport: 200, `success: false`. + authFetchSpy.mockResolvedValue({ + ok: true, + json: async () => ({ success: false, error: "Action 'log_call' on object '*' not found" }), + }); + const refreshSpy = vi.fn(); + const { result } = renderHook(() => + useConsoleActionRuntime({ dataSource: {}, objects: [], onRefresh: refreshSpy }), + ); + + let res: any; + await act(async () => { + res = await result.current.apiHandler({ type: 'api', name: 'log_call', target: '/api/v1/x' } as any); + }); + + expect(res.success).toBe(false); + expect(res.error).toBe("Action 'log_call' on object '*' not found"); + expect(refreshSpy).not.toHaveBeenCalled(); + }); + + it('apiHandler leaves a payload that merely CONTAINS a success key alone', async () => { + // Only the envelope's own `success: false` is a failure. A handler value + // that happens to carry `success` is data, and a `success: true` envelope + // is of course fine — neither may be misread as a rejection. + authFetchSpy.mockResolvedValue({ + ok: true, + json: async () => ({ success: true, data: { success: false, rows: 0, note: 'partial' } }), + }); + const { result } = renderHook(() => useConsoleActionRuntime({ dataSource: {}, objects: [] })); + + let res: any; + await act(async () => { + res = await result.current.apiHandler({ type: 'api', name: 'x', target: '/api/v1/x' } as any); + }); + + expect(res.success).toBe(true); + expect(res.data).toEqual({ success: false, rows: 0, note: 'partial' }); + }); +}); + describe('serverActionHandler — list_toolbar selection fallback', () => { it('uses the single selected row from the runner context as recordId', async () => { authFetchSpy.mockResolvedValue({ ok: true, json: async () => ({ success: true, data: {} }) }); diff --git a/packages/app-shell/src/hooks/useConsoleActionRuntime.tsx b/packages/app-shell/src/hooks/useConsoleActionRuntime.tsx index 3888862aa..961669c99 100644 --- a/packages/app-shell/src/hooks/useConsoleActionRuntime.tsx +++ b/packages/app-shell/src/hooks/useConsoleActionRuntime.tsx @@ -43,13 +43,14 @@ import { useActionModal } from './useActionModal'; import { ActionConfirmDialog, type ConfirmDialogState } from '../views/ActionConfirmDialog'; import { ActionParamDialog, type ParamDialogState } from '../views/ActionParamDialog'; import { ActionResultDialog, type ResultDialogState } from '../views/ActionResultDialog'; -import { FlowRunner, type ScreenFlowState } from '../views/FlowRunner'; +import { FlowRunner, type ScreenFlowState, type ScreenSpec } from '../views/FlowRunner'; 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'; import { interpretActionResponse } from '../utils/actionResponse'; +import { interpretFlowResponse } from '../utils/flowResponse'; const FALLBACK_USER = { id: 'current-user', name: 'Demo User', isPlatformAdmin: false }; @@ -350,6 +351,16 @@ export function useConsoleActionRuntime(opts: ConsoleActionRuntimeOptions): Cons return { success: false, error: errorDetail(body, `HTTP ${res.status}`) }; } const json = await res.json().catch(() => ({})); + // A business rejection can arrive as HTTP 200 with `success: false` + // (objectstack#3913). `res.ok` alone misses it, so the call reported + // success, toasted green and refreshed while the error was swallowed + // (#2958). Classified BEFORE the refresh below — a rejected call + // changed nothing to re-fetch. + if (json && typeof json === 'object' && (json as { success?: unknown }).success === false) { + // `name` is not guaranteed on an api action (it can be target-only), + // so fall back to the endpoint rather than interpolating `undefined`. + return { success: false, error: errorDetail(json, `Action "${action.name || targetStr}" failed`) }; + } if (action.refreshAfter !== false) refresh(); // Unwrap the ObjectStack `{ success, data }` envelope so `result.data` // is the inner payload — the contract every `result.data` consumer @@ -470,21 +481,28 @@ export function useConsoleActionRuntime(opts: ConsoleActionRuntimeOptions): Cons }, ); const json = await res.json().catch(() => null); - if (!res.ok || (json && json.success === false)) { - return { success: false, error: errorDetail(json, `Flow "${flowName}" failed (HTTP ${res.status})`) }; + // Single source for the flow-response rule — shared with + // RecordDetailView's copy of this handler and with FlowRunner's resume. + // A launch that FAILED (HTTP 200, `data.success === false`, no `status` + // and no `screen`) used to be indistinguishable from a completed run and + // fell into the terminal-success return below: no dialog, a green toast, + // and a refresh (#2958). See utils/flowResponse. + const outcome = interpretFlowResponse(res, json, `Flow "${flowName}"`); + if (outcome.kind === 'failed') { + // The ActionRunner's post-execution hook surfaces `error` as a toast. + return { success: false, error: outcome.error }; } // Screen-flow runtime: paused at a `screen` node awaiting input — open // the FlowRunner to render the form + resume. Refresh happens on complete. - const data = json?.data ?? {}; - if (data.status === 'paused' && data.screen) { - setScreenFlow({ flowName, runId: data.runId, screen: data.screen }); + if (outcome.kind === 'paused') { + setScreenFlow({ flowName, runId: outcome.runId ?? '', screen: outcome.screen }); // The action only OPENED the wizard — it hasn't completed. Suppress the // action-level success toast; the flow-runner owns completion messaging. return { success: true, silent: true }; } const shouldRefresh = action.refreshAfter !== false; if (shouldRefresh) refresh(); - return { success: true, data: json?.data, reload: shouldRefresh }; + return { success: true, data: outcome.data, reload: shouldRefresh }; } catch (error) { return { success: false, error: (error as Error).message }; } diff --git a/packages/app-shell/src/utils/__tests__/flowResponse.test.ts b/packages/app-shell/src/utils/__tests__/flowResponse.test.ts new file mode 100644 index 000000000..caf350bf1 --- /dev/null +++ b/packages/app-shell/src/utils/__tests__/flowResponse.test.ts @@ -0,0 +1,159 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * The flow trigger/resume response rule, once (#2958). Three hand-rolled copies + * existed and only one was complete; these cases pin the shapes the two + * incomplete ones got wrong. + */ + +import { describe, it, expect } from 'vitest'; +import { interpretFlowResponse } from '../flowResponse'; + +const ok = { ok: true, status: 200 }; + +describe('interpretFlowResponse — the reported bug: failure read as terminal success', () => { + it('catches a flow failure hiding under HTTP 200 with an outer success:true', () => { + // THE bug: no `status`, no `screen`, outer `success: true`. The launch + // handlers checked only the outer envelope, so this fell through to + // their terminal-success return — green toast, no dialog, refresh. + const out = interpretFlowResponse(ok, { + success: true, + data: { success: false, error: "Node 'apply' failed: Update requires an ID" }, + }, 'Flow "convert_lead"'); + + expect(out.kind).toBe('failed'); + expect(out).toMatchObject({ error: "Node 'apply' failed: Update requires an ID" }); + }); + + it('catches an explicit status:failed even when `success` is absent', () => { + const out = interpretFlowResponse(ok, { + success: true, + data: { status: 'failed', error: 'boom' }, + }, 'Flow "x"'); + expect(out).toMatchObject({ kind: 'failed', error: 'boom' }); + }); + + it('prefers the flow-declared friendly errorMessage over the raw error', () => { + // `flow.errorMessage` exists precisely so the user sees something other + // than an engine stack message. + const out = interpretFlowResponse(ok, { + success: true, + data: { + success: false, + error: "Node 'apply' failed: constraint violation on lead.status", + errorMessage: 'This lead has already been converted.', + }, + }, 'Flow "convert_lead"'); + expect(out).toMatchObject({ kind: 'failed', error: 'This lead has already been converted.' }); + }); + + it('marks a flow failure NON-retryable — the suspension is already consumed', () => { + const out = interpretFlowResponse(ok, { + success: true, data: { success: false, error: 'boom' }, + }, 'Resume'); + expect(out).toMatchObject({ kind: 'failed', retryable: false }); + }); +}); + +describe('interpretFlowResponse — transport failures stay retryable', () => { + it('catches a non-ok status', () => { + const out = interpretFlowResponse({ ok: false, status: 502 }, { + success: false, error: 'gateway timeout', + }, 'Resume'); + expect(out).toMatchObject({ kind: 'failed', error: 'gateway timeout', retryable: true }); + }); + + it('catches a transport-level success:false under HTTP 200', () => { + const out = interpretFlowResponse(ok, { success: false, error: 'no such flow' }, 'Flow "x"'); + expect(out).toMatchObject({ kind: 'failed', error: 'no such flow', retryable: true }); + }); + + it('falls back to a labelled message when the body carries none', () => { + const out = interpretFlowResponse({ ok: false, status: 500 }, null, 'Resume'); + expect(out).toMatchObject({ kind: 'failed', error: 'Resume failed (HTTP 500)' }); + }); +}); + +describe('interpretFlowResponse — error is ALWAYS a string (React #31)', () => { + it('resolves the nested {code, message} object to a string', () => { + // Handing this object to `toast.error()` puts an object where React + // expects a child and crashes the page. + const out = interpretFlowResponse({ ok: false, status: 403 }, { + success: false, + error: { message: 'Run is parked on a service-owned node', code: 'PERMISSION_DENIED' }, + }, 'Resume'); + expect(typeof (out as { error: string }).error).toBe('string'); + expect(out).toMatchObject({ error: 'Run is parked on a service-owned node' }); + }); + + it('resolves a nested error object on the INNER envelope too', () => { + const out = interpretFlowResponse(ok, { + success: true, + data: { success: false, error: { message: 'constraint violation', code: 400 } }, + }, 'Flow "x"'); + expect(typeof (out as { error: string }).error).toBe('string'); + expect(out).toMatchObject({ error: 'constraint violation' }); + }); + + it('never returns a non-string error for an object-shaped errorMessage', () => { + // `errorMessage` is declared as a string, but the body is untyped at + // runtime — a non-string must not be passed through as the toast child. + const out = interpretFlowResponse(ok, { + success: true, + data: { success: false, errorMessage: { nope: true }, error: 'the real message' }, + }, 'Flow "x"'); + expect(out).toMatchObject({ error: 'the real message' }); + }); +}); + +describe('interpretFlowResponse — paused is not a failure', () => { + it('reports a paused screen with its runId and screen', () => { + const screen = { nodeId: 'confirm', title: 'Confirm', fields: [] }; + const out = interpretFlowResponse(ok, { + success: true, + data: { success: true, status: 'paused', runId: 'run-7', screen }, + }, 'Flow "convert_lead"'); + + expect(out).toMatchObject({ kind: 'paused', runId: 'run-7', screen }); + }); + + it('does NOT treat a pause as terminal — a screen-less pause falls through to done', () => { + // `status: 'paused'` with no `screen` is a non-screen suspension (an + // approval / wait node). There is nothing for the runner to render, so + // it must not be reported as a screen pause. + const out = interpretFlowResponse(ok, { + success: true, data: { success: true, status: 'paused', runId: 'run-8' }, + }, 'Flow "x"'); + expect(out.kind).toBe('done'); + }); +}); + +describe('interpretFlowResponse — terminal success', () => { + it('exposes the AutomationResult as `data`, unchanged', () => { + const out = interpretFlowResponse(ok, { + success: true, + data: { success: true, status: 'completed', output: { id: 'opp_1' }, durationMs: 12 }, + }, 'Flow "convert_lead"'); + + expect(out).toMatchObject({ + kind: 'done', + data: { success: true, status: 'completed', output: { id: 'opp_1' }, durationMs: 12 }, + }); + }); + + it('surfaces the flow-declared successMessage', () => { + const out = interpretFlowResponse(ok, { + success: true, data: { success: true, successMessage: 'Lead converted.' }, + }, 'Flow "x"'); + expect(out).toMatchObject({ kind: 'done', successMessage: 'Lead converted.' }); + }); + + it('leaves `data` undefined when the body carried none — not an invented {}', () => { + // `ActionResult.data` has always exposed the raw `body.data`; the launch + // handlers returned `json?.data` and consumers may test for absence. + const out = interpretFlowResponse(ok, { success: true }, 'Flow "x"'); + expect(out).toMatchObject({ kind: 'done' }); + expect((out as { data?: unknown }).data).toBeUndefined(); + }); +}); diff --git a/packages/app-shell/src/utils/flowResponse.ts b/packages/app-shell/src/utils/flowResponse.ts new file mode 100644 index 000000000..4954146b6 --- /dev/null +++ b/packages/app-shell/src/utils/flowResponse.ts @@ -0,0 +1,138 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * The ONE place the console interprets a flow `trigger` / `resume` response. + * + * This is the `/actions` story again (see {@link ./actionResponse}), on the + * automation route: the rule lived in THREE hand-rolled copies — the flow + * handlers in `useConsoleActionRuntime` and `RecordDetailView`, plus + * `FlowRunner`'s resume — and only the third one was complete. The two launch + * copies checked the transport envelope and then treated *everything else* as + * terminal success, so a flow that failed on its first node was + * indistinguishable from one that ran to completion: no dialog, a green + * "completed successfully" toast, and a refresh (#2958). + * + * ## The response wraps once, and failure has three shapes + * + * ``` + * { ← transport envelope + * success: true, + * data: { ← AutomationResult (spec: contracts/automation-service) + * success: boolean, ← required; FALSE when the run failed + * status?: 'completed' | 'paused' | 'failed', + * runId?, screen?, ← set when paused at a `screen` node + * error?, errorMessage?, successMessage? + * } + * } + * ``` + * + * - **transport failure** — `!res.ok`, or the outer `success: false`. The run + * may never have started; nothing was consumed. + * - **flow failure** — the run started and failed. HTTP **200**, outer + * `success: true`, and `data.success === false` (or `status: 'failed'`). + * `res.ok` is TRUE, so only the inner envelope shows it. This is the one the + * launch handlers missed. + * - **paused** — suspended at a `screen` node awaiting input. NOT a failure: + * the engine always stamps `success: true` alongside `status: 'paused'` + * (service-automation `engine.ts`), which is why failure can be classified + * before pausing without swallowing a wizard. + * + * Callers get one more thing out of routing through here: `error` is always a + * STRING. Handing a `{code, message}` object to `toast.error()` puts an object + * where React expects a child and crashes the page (React #31) — the same trap + * `actionErrorDetail` exists for. + */ + +import { actionErrorDetail } from './actionErrorDetail'; + +/** + * The `AutomationResult` fields the console reads. Deliberately loose: this is + * a parsed HTTP body, not a value the type system has vouched for. + */ +export interface FlowRunResult { + success?: boolean; + status?: 'completed' | 'paused' | 'failed' | string; + runId?: string; + screen?: unknown; + error?: unknown; + errorMessage?: unknown; + successMessage?: unknown; + [key: string]: unknown; +} + +/** + * `S` is the caller's screen type (`ScreenSpec`, which lives in the views + * layer). Generic rather than imported so this util stays a leaf. + */ +export type FlowResponseOutcome = + | { + kind: 'failed'; + /** Always a string — safe to hand to `toast.error()`. */ + error: string; + /** + * The request failed at the transport, so the run was NOT consumed — + * a retry of the same run is meaningful. False for a flow failure: + * the engine consumed the suspension before running downstream nodes + * (resume-once), so retrying only reaches "No suspended run", and a + * runner should close rather than leave a dead form open. + */ + retryable: boolean; + } + | { kind: 'paused'; runId?: string; screen: S; data: FlowRunResult } + | { kind: 'done'; data: FlowRunResult | undefined; successMessage?: string }; + +/** + * A flow declares a friendly `errorMessage`; prefer it over the raw `error`, + * then fall back to the label. `actionErrorDetail` guarantees a string. + */ +function flowFailureMessage(data: FlowRunResult, fallback: string): string { + const friendly = data.errorMessage; + if (typeof friendly === 'string' && friendly.length > 0) return friendly; + return actionErrorDetail(data, fallback); +} + +/** + * Classify a `POST /api/v1/automation/{flow}/trigger` or + * `.../runs/{runId}/resume` response. + * + * `label` names the flow for fallback messages (e.g. `Flow "convert_lead"`). + * `json` is the parsed body, or `null` when it could not be parsed. + */ +export function interpretFlowResponse( + res: { ok: boolean; status: number }, + json: any, + label: string, +): FlowResponseOutcome { + if (!res.ok || (json && json.success === false)) { + return { + kind: 'failed', + error: actionErrorDetail(json, `${label} failed (HTTP ${res.status})`), + retryable: true, + }; + } + + const data: FlowRunResult = (json?.data ?? {}) as FlowRunResult; + + // Checked BEFORE `paused` on purpose — see the header note on why a paused + // run can never land here. + if (data.success === false || data.status === 'failed') { + return { + kind: 'failed', + error: flowFailureMessage(data, `${label} failed`), + retryable: false, + }; + } + + if (data.status === 'paused' && data.screen) { + return { kind: 'paused', runId: data.runId, screen: data.screen as S, data }; + } + + // Terminal success. `data` is the raw `json?.data` — `undefined` when the + // body carried none, which is what `ActionResult.data` has always exposed. + return { + kind: 'done', + data: json?.data as FlowRunResult | undefined, + successMessage: typeof data.successMessage === 'string' ? data.successMessage : undefined, + }; +} diff --git a/packages/app-shell/src/views/FlowRunner.tsx b/packages/app-shell/src/views/FlowRunner.tsx index d4cce8ed1..cd5c0019e 100644 --- a/packages/app-shell/src/views/FlowRunner.tsx +++ b/packages/app-shell/src/views/FlowRunner.tsx @@ -26,6 +26,7 @@ import { } from '@object-ui/components'; import { toast } from 'sonner'; import { ScreenView, isObjectFormScreen, initialScreenValues, visibleScreenFields, type ScreenSpec } from './ScreenView'; +import { interpretFlowResponse } from '../utils/flowResponse'; export type { ScreenSpec, ScreenFieldSpec } from './ScreenView'; @@ -88,33 +89,32 @@ export function FlowRunner({ state, authFetch, baseUrl, onClose, onComplete, dat { method: 'POST', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify({ inputs }) }, ); const json = await res.json().catch(() => null); - if (!res.ok || json?.success === false) { - // Transport / envelope failure — possibly transient (network, 5xx), so - // keep the dialog open and let the user retry the same run. - toast.error(json?.error || `Resume failed (HTTP ${res.status})`); + // Shared with the two flow-LAUNCH handlers (useConsoleActionRuntime, + // RecordDetailView) so the resume path and the launch path can never again + // disagree about what a failure looks like — they did, and the launch + // copies reported failures as success (#2958). `outcome.error` is always a + // STRING: handing the nested `{code, message}` object to `toast.error()` + // renders nothing at best and crashes the page as a React child (React + // #31). See utils/flowResponse. + const outcome = interpretFlowResponse(res, json, 'Resume'); + if (outcome.kind === 'failed') { + toast.error(outcome.error); + // A transport / envelope failure may be transient (network, 5xx) and did + // not consume the suspension — keep the dialog open so the user can retry + // the same run. A flow failure is TERMINAL: the engine consumes the + // suspension before running downstream nodes (resume-once), so a retry + // would only hit "No suspended run". Close instead of leaving a dead form. + if (!outcome.retryable) onClose(); return; } - const data = json?.data ?? {}; - // The HTTP envelope is `{ success:true, data: AutomationResult }`; a flow - // that errored downstream surfaces as `data.success === false`. That is - // TERMINAL: the engine consumes the suspension before running downstream - // nodes (resume-once), so this run can never be resumed again — a retry - // would only hit "No suspended run". Close the runner instead of leaving - // a dead form open. - if (data.success === false || data.status === 'failed') { - // Prefer the flow's friendly `errorMessage`; fall back to the raw error. - toast.error(data.errorMessage || data.error || 'The flow failed to complete.'); - onClose(); - return; - } - if (data.status === 'paused' && data.screen) { - setScreen(data.screen); - setRunId(data.runId || runId); - setValues(initialScreenValues(data.screen)); + if (outcome.kind === 'paused') { + setScreen(outcome.screen); + setRunId(outcome.runId || runId); + setValues(initialScreenValues(outcome.screen)); toast.success('Saved — next step'); } else { // Terminal success — show the flow's declared completion message. - toast.success(data.successMessage || 'Done'); + toast.success(outcome.successMessage || 'Done'); onComplete(); } }; diff --git a/packages/app-shell/src/views/RecordDetailView.tsx b/packages/app-shell/src/views/RecordDetailView.tsx index f62b7dd18..98b71ce95 100644 --- a/packages/app-shell/src/views/RecordDetailView.tsx +++ b/packages/app-shell/src/views/RecordDetailView.tsx @@ -28,13 +28,14 @@ import { hasExplicitDiscussion } from '../utils/pageSchemaIntrospect'; import { ActionConfirmDialog, type ConfirmDialogState } from './ActionConfirmDialog'; import { ActionParamDialog, type ParamDialogState } from './ActionParamDialog'; import { ActionResultDialog, type ResultDialogState } from './ActionResultDialog'; -import { FlowRunner, type ScreenFlowState } from './FlowRunner'; +import { FlowRunner, type ScreenFlowState, type ScreenSpec } from './FlowRunner'; 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 { decisionOutputDefs, decisionOutputParams, foldDecisionOutputs } from '../utils/decisionOutputParams'; import { interpretActionResponse } from '../utils/actionResponse'; +import { interpretFlowResponse } from '../utils/flowResponse'; import { useRecordBreadcrumbTitle } from '../context/NavigationContext'; import type { FeedItem } from '@object-ui/types'; import type { ActionDef, ActionParamDef } from '@object-ui/core'; @@ -747,15 +748,21 @@ export function RecordDetailView({ dataSource, objects, onEdit, objectNameOverri }, ); const json = await res.json().catch(() => null); - if (!res.ok || (json && json.success === false)) { - const errMsg = json?.error || `Flow "${flowName}" failed (HTTP ${res.status})`; - return { success: false, error: errMsg }; + // Single source for the flow-response rule — shared with + // useConsoleActionRuntime's copy of this handler and FlowRunner's resume. + // This copy checked only the transport envelope and then treated + // everything else as terminal success, so a run that failed on its first + // node fired a green toast (#2958); it also passed `json.error` through + // raw, and the nested `{code, message}` shape reaches `toast.error()` as + // a React child and crashes the page (React #31). See utils/flowResponse. + const outcome = interpretFlowResponse(res, json, `Flow "${flowName}"`); + if (outcome.kind === 'failed') { + return { success: false, error: outcome.error }; } // Screen-flow runtime: the run paused at a `screen` node awaiting input — // open the FlowRunner to render the form + resume (refresh on completion). - const data = json?.data ?? {}; - if (data.status === 'paused' && data.screen) { - setScreenFlow({ flowName, runId: data.runId, screen: data.screen }); + if (outcome.kind === 'paused') { + setScreenFlow({ flowName, runId: outcome.runId ?? '', screen: outcome.screen }); // The action only OPENED the wizard — it hasn't completed. Suppress the // action-level success toast; the flow-runner owns completion messaging. return { success: true, silent: true }; @@ -764,7 +771,7 @@ export function RecordDetailView({ dataSource, objects, onEdit, objectNameOverri if (shouldRefresh) { notifyRecordChanged(); } - return { success: true, data: json?.data, reload: shouldRefresh }; + return { success: true, data: outcome.data, reload: shouldRefresh }; } catch (error) { return { success: false, error: (error as Error).message }; }