diff --git a/.changeset/5221-onsuccess-navigation.md b/.changeset/5221-onsuccess-navigation.md new file mode 100644 index 000000000..709dc4aa8 --- /dev/null +++ b/.changeset/5221-onsuccess-navigation.md @@ -0,0 +1,38 @@ +--- +'@object-ui/core': minor +'@object-ui/app-shell': minor +--- + +Console half of `ActionSchema.onSuccess` post-success navigation. + +`@objectstack/spec` declares `onSuccess` as a closed strict object +`{ navigate: string, openIn: 'self' | 'newTab' }`, refine-scoped to `type: 'api'` and +`type: 'script'` — the two action types whose success event carries a server response. +Nothing in this renderer read it, so an action declaring the hop navigated nowhere: the +block fell into `ActionRunner`'s older `ActionDef.onSuccess` chained-callback channel, +was dispatched as an action, and failed inside `executeNavigation` with "No URL provided +for navigation action" — a red toast and no jump. The motivating report is a clone action +that leaves the user sitting on the record they cloned from. + +`ActionRunner.handlePostExecution` now performs the declared hop through +`navigationHandler` — the same SPA seam every other navigator in that file uses, which +the console wires to react-router's `navigate`, so `openIn: 'self'` is a real in-place +route hop rather than a full-page load. `interpolateTarget` gains a `${result.*}` scope +alongside `${param.*}` and `${ctx.*}`, resolved against the handler's own return value +(the level `readActionPayload` reads, one below the action envelope) and supplied only by +this call site, so a target interpolated before its request still has no `result` to +name. `openIn` is read as the one member that changes the branch and no default is +written here — the spec materialises `.default('self')`, so parse output always carries a +resolved member — and the two `openIn` spellings stay apart: this reads +`onSuccess.openIn` (`'self' | 'newTab'`), never the top-level `type: 'url'` switch +(`'self' | 'new-tab'`), each of which spec refuses in the other's position. + +The console's server-action wrapper gains the matching handler-return half: a handler may +now return `openIn: 'self'` next to its `redirectUrl` to ask for the same-tab jump, while +a `redirectUrl` **without** `openIn` keeps its shipped new-tab behaviour unchanged. When +an action declares an `onSuccess` block, the wrapper defers to the runner and only tidies +its pre-opened tab, so one navigation happens rather than two. + +The pre-existing `ActionDef.onSuccess` chained-callback channel is unchanged. It is told +apart by the spec's own declaration — a non-array object whose `navigate` is a string — +and keeps running for every other shape. diff --git a/packages/app-shell/src/hooks/useConsoleActionRuntime.tsx b/packages/app-shell/src/hooks/useConsoleActionRuntime.tsx index 007767d5e..171a9787c 100644 --- a/packages/app-shell/src/hooks/useConsoleActionRuntime.tsx +++ b/packages/app-shell/src/hooks/useConsoleActionRuntime.tsx @@ -596,14 +596,18 @@ export function useConsoleActionRuntime(opts: ConsoleActionRuntimeOptions): Cons // memoized once) while the config thunks read the latest object scope and // refresh callback — the factory's in-flight guard only spans invocations of // the same instance. - const serverActionEnvRef = useRef({ objApiName, refresh, t }); - serverActionEnvRef.current = { objApiName, refresh, t }; + const serverActionEnvRef = useRef({ objApiName, refresh, t, navigate }); + serverActionEnvRef.current = { objApiName, refresh, t, navigate }; const serverActionHandler = useMemo( () => createConsoleServerActionHandler({ fetch: authFetch, baseUrl: () => import.meta.env.VITE_SERVER_URL || '', resolveObject: () => serverActionEnvRef.current.objApiName, onRefresh: () => serverActionEnvRef.current.refresh(), + // SPA route hop for a handler-returned `openIn: 'self'` — react-router's + // own `navigate`, read through the env ref like every other live value + // here so the handler instance stays stable. + navigate: (url: string) => serverActionEnvRef.current.navigate(url), // Read through the env ref so the spinner-tab / popup-blocked copy // follows a language switch without invalidating the handler instance // (objectui#3321). diff --git a/packages/app-shell/src/utils/__tests__/consoleServerAction.test.tsx b/packages/app-shell/src/utils/__tests__/consoleServerAction.test.tsx index bd8e0fe87..cf4d4f184 100644 --- a/packages/app-shell/src/utils/__tests__/consoleServerAction.test.tsx +++ b/packages/app-shell/src/utils/__tests__/consoleServerAction.test.tsx @@ -279,3 +279,142 @@ describe('failure paths close the pre-opened tab', () => { expect(tab.close).toHaveBeenCalledTimes(1); }); }); + +describe("handler-returned openIn: 'self' (objectui#5221)", () => { + /** A handler response carrying `redirectUrl` (+ optional siblings). */ + function redirecting(extra: Record = {}) { + return okFetch({ + success: true, + data: { success: true, data: { redirectUrl: '/app/crm/contacts/rec_42', ...extra } }, + }) as any; + } + + it("hops the SPA router in place, and does NOT open a tab", async () => { + const navigate = vi.fn(); + const openSpy = vi.spyOn(window, 'open').mockReturnValue(makeTab() as any); + const { handler } = makeHandler({ fetch: redirecting({ openIn: 'self' }), navigate }); + + await handler({ type: 'script', name: 'clone_and_jump' } as any); + + expect(navigate).toHaveBeenCalledTimes(1); + expect(navigate).toHaveBeenCalledWith('/app/crm/contacts/rec_42'); + // Discriminating: the shipped behavior for this same response WITHOUT + // `openIn` is `window.open`. If the branch were ignored, this would fire. + expect(openSpy).not.toHaveBeenCalled(); + }); + + it('keeps the shipped new-tab behavior when the handler omits openIn', async () => { + // The positive control for the assertion above: same fetch body, same + // harness, one key removed — proving `window.open` is reachable here and + // the `not.toHaveBeenCalled()` above is a real measurement. + const navigate = vi.fn(); + const openSpy = vi.spyOn(window, 'open').mockReturnValue(makeTab() as any); + const { handler } = makeHandler({ fetch: redirecting(), navigate }); + + await handler({ type: 'script', name: 'clone_and_jump' } as any); + + expect(openSpy).toHaveBeenCalledWith('/app/crm/contacts/rec_42', '_blank'); + expect(navigate).not.toHaveBeenCalled(); + }); + + it("closes the optimistically pre-opened tab when the handler asks to stay put", async () => { + const tab = makeTab(); + vi.spyOn(window, 'open').mockReturnValue(tab as any); + const navigate = vi.fn(); + const { handler } = makeHandler({ fetch: redirecting({ openIn: 'self' }), navigate }); + + await handler({ + type: 'script', name: 'clone_and_jump', opensInNewTab: true, params: { recordId: 'e1' }, + } as any); + + expect(tab.close).toHaveBeenCalledTimes(1); + expect(navigate).toHaveBeenCalledWith('/app/crm/contacts/rec_42'); + }); + + it("does not accept the type:'url' kebab spelling as a same-tab request", async () => { + // `'new-tab'` is the TOP-LEVEL `openIn` key's spelling for `type:'url'` + // actions; spec refuses the crossover in each direction. `'new-tab'` here + // is simply not `'self'`, so the shipped new-tab path stands — the + // renderer never becomes looser than the contract authors are validated + // against. + const navigate = vi.fn(); + const openSpy = vi.spyOn(window, 'open').mockReturnValue(makeTab() as any); + const { handler } = makeHandler({ fetch: redirecting({ openIn: 'new-tab' }), navigate }); + + await handler({ type: 'script', name: 'clone_and_jump' } as any); + + expect(navigate).not.toHaveBeenCalled(); + expect(openSpy).toHaveBeenCalledWith('/app/crm/contacts/rec_42', '_blank'); + }); + + it('falls back to a full-page navigation for an absolute destination', async () => { + // No SPA route can express an off-origin URL. Same tab either way, so the + // handler's stated intent is honoured, never inverted into a new tab. + const navigate = vi.fn(); + const { handler } = makeHandler({ + fetch: okFetch({ + success: true, + data: { success: true, data: { redirectUrl: 'https://example.test/landing', openIn: 'self' } }, + }) as any, + navigate, + }); + const hrefs: string[] = []; + Object.defineProperty(window, 'location', { + configurable: true, + value: { get href() { return ''; }, set href(v: string) { hrefs.push(v); } }, + }); + + await handler({ type: 'script', name: 'clone_and_jump' } as any); + + expect(navigate).not.toHaveBeenCalled(); + expect(hrefs).toEqual(['https://example.test/landing']); + }); +}); + +describe('a declared onSuccess block defers to the runner (objectui#5221)', () => { + it('performs no navigation of its own, and tidies the pre-opened tab', async () => { + const tab = makeTab(); + const openSpy = vi.spyOn(window, 'open').mockReturnValue(tab as any); + const navigate = vi.fn(); + const { handler } = makeHandler({ + fetch: okFetch({ + success: true, + data: { success: true, data: { redirectUrl: '/app/crm/contacts/rec_42' } }, + }) as any, + navigate, + }); + + const res = await handler({ + type: 'script', name: 'clone_and_jump', opensInNewTab: true, params: { recordId: 'e1' }, + // The DECLARED hop — `ActionRunner.navigateOnSuccess` performs this one. + onSuccess: { navigate: '/app/crm/contacts/${result.id}', openIn: 'self' }, + } as any); + + expect(res.success).toBe(true); + expect(navigate).not.toHaveBeenCalled(); + // The pre-open is the only `window.open` — no second navigation from here. + expect(openSpy).toHaveBeenCalledTimes(1); + expect(openSpy).toHaveBeenCalledWith('about:blank', '_blank'); + expect(tab.close).toHaveBeenCalledTimes(1); + }); + + it('a legacy chained-callback onSuccess is NOT mistaken for a declared hop', async () => { + // `{ type: 'notify' }` is the runner's older `ActionDef` callback channel, + // not the spec block. The redirectUrl convention must still run. + const openSpy = vi.spyOn(window, 'open').mockReturnValue(makeTab() as any); + const navigate = vi.fn(); + const { handler } = makeHandler({ + fetch: okFetch({ + success: true, + data: { success: true, data: { redirectUrl: '/app/crm/contacts/rec_42' } }, + }) as any, + navigate, + }); + + await handler({ + type: 'script', name: 'clone_and_jump', onSuccess: { type: 'notify' }, + } as any); + + expect(openSpy).toHaveBeenCalledWith('/app/crm/contacts/rec_42', '_blank'); + }); +}); diff --git a/packages/app-shell/src/utils/consoleServerAction.ts b/packages/app-shell/src/utils/consoleServerAction.ts index d437900d2..bffa6d396 100644 --- a/packages/app-shell/src/utils/consoleServerAction.ts +++ b/packages/app-shell/src/utils/consoleServerAction.ts @@ -20,7 +20,12 @@ * skipping the POST; * - the **`redirectUrl` convention**: a handler returning `{ redirectUrl }` * asks the UI to open it — into the pre-opened tab when there is one, else a - * lazily opened one with a popup-blocked toast fallback. + * lazily opened one with a popup-blocked toast fallback. A handler may add + * `openIn: 'self'` to ask for the same-tab jump instead (objectui#5221); + * WITHOUT it the shipped new-tab behavior stands, so nothing flips silently. + * - **deferring to a declared `ActionSchema.onSuccess` block**: when the action + * itself declares the post-success hop, the runner performs it and this + * wrapper only tidies the pre-opened tab — one navigation, not two. * * This file exists because `useConsoleActionRuntime` and `RecordDetailView` * each carried a near-verbatim copy of all of the above around their @@ -40,6 +45,7 @@ import { toast } from 'sonner'; import { createServerActionHandler, readActionPayload, + readOnSuccessNavigation, resolveServerActionRecordId, type ActionContext, type ActionDef, @@ -82,6 +88,16 @@ export interface ConsoleServerActionOptions { * (objectui#3321). */ t?: ConsoleServerActionTranslate; + /** + * SPA route hop, for a handler that returned `openIn: 'self'` alongside its + * `redirectUrl`. The console passes react-router's `navigate` — the router + * this app already uses; nothing new is introduced here. + * + * Optional: without it an explicit `'self'` still lands, via the full-page + * navigation this file already falls back to. That is a slower same-tab + * arrival, never a new tab, so the handler's stated intent is never inverted. + */ + navigate?: (url: string) => void; } /** Minimal HTML escape for locale strings interpolated into the spinner document. */ @@ -208,8 +224,43 @@ export function createConsoleServerActionHandler(opts: ConsoleServerActionOption const redirectUrl = (payload && typeof payload === 'object' && typeof (payload as { redirectUrl?: unknown }).redirectUrl === 'string') ? (payload as { redirectUrl: string }).redirectUrl : null; + // ── Precedence: a DECLARED `onSuccess` block beats the handler-return + // convention ───────────────────────────────────────────────────── + // Both can be present on one `type: 'script'` action, and they mean the + // same thing — "go here afterwards". Performing both would fire two + // navigations, the second racing a page that is already unloading. The + // author's declaration is the surface `@objectstack/spec` validates and + // the only one visible in metadata, so it wins; the runner performs that + // hop after this handler returns (`ActionRunner.navigateOnSuccess`), and + // all this branch owes is the pre-opened tab. + // + // ⚠️ The spec rules each surface's own default but does NOT rule this + // precedence — objectui#5221 escalates it. If the maintainer rules the + // other way, this is the line that changes. + if (readOnSuccessNavigation(action.onSuccess)) { + closeTab(preOpenedTab); + return result; + } if (redirectUrl) { - openInTab(preOpenedTab, redirectUrl, t); + // `{ redirectUrl }` WITHOUT `openIn` keeps its shipped new-tab + // behavior — no silent flip. A handler may opt into the same-tab jump + // by returning `openIn: 'self'` explicitly, spelled as the ruled enum + // member (never the `type:'url'` key's `'new-tab'` kebab). + const opensInSelf = payload && typeof payload === 'object' + && (payload as { openIn?: unknown }).openIn === 'self'; + if (opensInSelf) { + // The tab we optimistically pre-opened is now unwanted: the handler + // asked to stay put. + closeTab(preOpenedTab); + if (opts.navigate && redirectUrl.startsWith('/')) { + opts.navigate(redirectUrl); + } else { + // Absolute/external destination — no SPA route can express it. + window.location.href = redirectUrl; + } + } else { + openInTab(preOpenedTab, redirectUrl, t); + } } else { // Handler didn't return a redirectUrl — close the empty tab we // optimistically pre-opened so the user isn't left with about:blank. diff --git a/packages/app-shell/src/views/RecordDetailView.tsx b/packages/app-shell/src/views/RecordDetailView.tsx index 5ca8c1781..da129368b 100644 --- a/packages/app-shell/src/views/RecordDetailView.tsx +++ b/packages/app-shell/src/views/RecordDetailView.tsx @@ -879,8 +879,8 @@ export function RecordDetailView({ dataSource, objects, onEdit, objectNameOverri // `action.recordId`; header/more actions carry none and use this page's id. // The env ref keeps the handler instance stable across renders (authFetch is // memoized once) while the thunks read the live record/object. - const serverActionEnvRef = useRef({ objectName, pureRecordId, notifyRecordChanged, t }); - serverActionEnvRef.current = { objectName, pureRecordId, notifyRecordChanged, t }; + const serverActionEnvRef = useRef({ objectName, pureRecordId, notifyRecordChanged, t, navigate }); + serverActionEnvRef.current = { objectName, pureRecordId, notifyRecordChanged, t, navigate }; const serverActionHandler = useMemo( () => createConsoleServerActionHandler({ fetch: authFetch, @@ -890,6 +890,9 @@ export function RecordDetailView({ dataSource, objects, onEdit, objectNameOverri recordId: (action as { recordId?: unknown }).recordId ?? serverActionEnvRef.current.pureRecordId ?? undefined, }), onRefresh: () => serverActionEnvRef.current.notifyRecordChanged(), + // SPA route hop for a handler-returned `openIn: 'self'` — react-router's + // own `navigate`, matching the shared console runtime. + navigate: (url: string) => serverActionEnvRef.current.navigate(url), // Read through the env ref so the spinner-tab / popup-blocked copy // follows a language switch without invalidating the handler instance // (objectui#3321). diff --git a/packages/core/src/actions/ActionRunner.ts b/packages/core/src/actions/ActionRunner.ts index b59ccd582..f1d69af0a 100644 --- a/packages/core/src/actions/ActionRunner.ts +++ b/packages/core/src/actions/ActionRunner.ts @@ -27,6 +27,7 @@ import { ExpressionEvaluator } from '../evaluator/ExpressionEvaluator.js'; import { hasDeclaredPredicate } from '../evaluator/declaredPredicate.js'; import { globalUndoManager, type UndoableOperation } from './UndoManager.js'; import { warnOnDeprecatedObjectParams, warnOnUnknownActionKeys } from './actionKeys.js'; +import { readActionPayload } from './actionResponse.js'; export interface ActionResult { success: boolean; @@ -1216,10 +1217,35 @@ export class ActionRunner { if (chainResult.reload) result.reload = true; } - // Execute onSuccess/onFailure callbacks + // ── ActionSchema.onSuccess — post-success navigation ──────────────────── + // + // objectui#5221, the console half of objectstack#9566/#9474. The spec + // declares `onSuccess` as a CLOSED STRICT object + // `{ navigate: string, openIn: 'self' | 'newTab' }`, refine-scoped to + // `type: 'api'` and `type: 'script'` — the two types whose success event + // carries a server response for `${result.*}` to read. + // + // This runner's OWN `ActionDef.onSuccess` predates that key and means + // something else entirely: `ActionDef | ActionDef[]`, chained callbacks. + // The two are told apart by the spec's own declaration — a non-array object + // whose `navigate` is a STRING is the spec block and nothing else can be: + // `navigate` on a callback ActionDef is the deprecated nested navigation + // ENVELOPE (`executeNavigation` reads `navigate.to`), so a string there has + // never been runnable. This is a NARROWING to the declared contract, not a + // lenient fallback: a shape the spec refuses gets no new reading here. + // + // Before this branch existed, the ruled shape fell into the callback path, + // dispatched `{ navigate: '' }` as an action, and failed inside + // `executeNavigation` with "No URL provided for navigation action" — the + // author got a red toast and no hop. if (result.success && action.onSuccess) { - const callbacks = Array.isArray(action.onSuccess) ? action.onSuccess : [action.onSuccess]; - await this.executeChain(callbacks, 'sequential'); + const navigation = readOnSuccessNavigation(action.onSuccess); + if (navigation) { + this.navigateOnSuccess(navigation, action, result); + } else { + const callbacks = Array.isArray(action.onSuccess) ? action.onSuccess : [action.onSuccess]; + await this.executeChain(callbacks, 'sequential'); + } } if (!result.success && action.onFailure) { const callbacks = Array.isArray(action.onFailure) ? action.onFailure : [action.onFailure]; @@ -1227,6 +1253,68 @@ export class ActionRunner { } } + /** + * Perform the `ActionSchema.onSuccess` hop for an action that just succeeded. + * + * **The router is the app's own.** `navigationHandler` is the seam every + * navigator in this file already goes through, and the console wires it to + * react-router's `navigate` — so `openIn: 'self'` is a real SPA route hop, + * in place, immune to popup blocking. No `window.location` / `window.open` + * mechanism is introduced here; the no-handler fallback is the same + * `result.redirect` hand-off `navigateTo` already uses. + * + * **No default is written here.** `openIn` is a materialised + * `.default('self')` in the spec, so parse output always carries a resolved + * member; this reads the ONE member that changes the branch and leaves the + * other implicit. A `?? 'self'` would be a second source of truth for a + * default the producer already resolved. + * + * **The two `openIn` spellings never cross.** This reads + * `onSuccess.openIn` (`'self' | 'newTab'`) and nothing else — never the + * top-level `type: 'url'` switch, which spells its new-tab member + * `'new-tab'`. Spec refuses each crossover spelling with a keyed error; a + * renderer that accepted either would be a second, looser contract than the + * one authors are validated against. + */ + private navigateOnSuccess( + block: OnSuccessNavigation, + action: ActionDef, + result: ActionResult, + ): void { + // `${result.*}` reads the HANDLER's return value, one level below the + // action envelope — the same level `readActionPayload` hands the + // `redirectUrl` convention. Reading `result.data` raw would see the + // `{ success, data }` envelope on pre-objectstack#3962 servers, which is + // the "one level too shallow" bug (#2904) in a new place. + const url = this.interpolateTarget(block.navigate, action, readActionPayload(result.data)); + + // `navigate` is author-supplied metadata and lands in a URL position, so + // it goes through the same scheme guard as every other navigator here. + if (!this.isValidUrl(url)) { + console.warn( + '[ActionRunner] onSuccess.navigate resolved to a URL this runner refuses to open ' + + '(only http://, https:// and relative URLs are allowed) — no navigation performed.', + { action: action.name, navigate: block.navigate, resolved: url }, + ); + return; + } + + const newTab = block.openIn === 'newTab'; + // Same external-URL convention `navigateTo` hands the handler; `newTab` + // stays the declared choice alone, never `?? isExternal`. + const external = url.startsWith('http://') || url.startsWith('https://'); + + if (this.navigationHandler) { + this.navigationHandler(url, { external, newTab }); + return; + } + if (newTab) { + window.open(url, '_blank', 'noopener,noreferrer'); + } else { + result.redirect = url; + } + } + /** * Execute script action — evaluates client-side expression via ExpressionEvaluator. * Supports ${} template expressions referencing data, record, user context. @@ -1777,7 +1865,7 @@ export class ActionRunner { * param. Consumers can extend by stuffing extra keys under * `context.ctx = {...}` before calling `runner.execute()`. */ - private interpolateTarget(target: string, action: ActionDef): string { + private interpolateTarget(target: string, action: ActionDef, resultScope?: unknown): string { if (typeof target !== 'string' || target.indexOf('${') === -1) return target; // ── The `${param.X}` scope is an INTERNAL runtime value bag, not an // authoring surface (objectui#4097 ruling B, 2026-08-11) ────────────── @@ -1813,10 +1901,20 @@ export class ActionRunner { const params = (action.params && typeof action.params === 'object' && !Array.isArray(action.params)) ? (action.params as Record) : {}; - const ctx = this.buildInterpolationContext(); - return target.replace(/\$\{(param|ctx)\.([\w.]+)\}/g, (_match, scope: string, path: string) => { - const root = scope === 'param' ? params : ctx; - const value = readPath(root, path); + // The scope MAP defines the vocabulary — the pattern is built from its + // keys, so a scope that is not in play is not a scope this call recognizes. + // + // `result` (the handler's own return value) is passed by exactly one caller: + // the `ActionSchema.onSuccess` post-success hop, where a response exists. + // Every other caller — `executeUrl`, `executeAPI` — interpolates a target + // BEFORE the request, so there is no result to read; admitting `${result.*}` + // there would widen the authorable surface with nothing behind it and + // silently blank the token instead of leaving the author's mistake visible. + const scopes: Record = { param: params, ctx: this.buildInterpolationContext() }; + if (resultScope !== undefined) scopes.result = resultScope; + const pattern = new RegExp(`\\$\\{(${Object.keys(scopes).join('|')})\\.([\\w.]+)\\}`, 'g'); + return target.replace(pattern, (_match, scope: string, path: string) => { + const value = readPath(scopes[scope], path); if (value == null) return ''; return encodeURIComponent(String(value)); }); @@ -1893,6 +1991,31 @@ export async function executeAction( return await runner.execute(action); } +/** The `ActionSchema.onSuccess` block, as the pinned spec declares it. */ +export interface OnSuccessNavigation { + navigate: string; + /** `'self' | 'newTab'` — read as data, so an off-contract value cannot widen the branch. */ + openIn?: unknown; +} + +/** + * Is this `onSuccess` the SPEC's navigation block, or this runner's older + * chained-callback channel (`ActionDef | ActionDef[]`)? + * + * The test IS the spec's declaration: a non-array object carrying a STRING + * `navigate`. Nothing else can produce that shape — the spec object is strict + * with `navigate: z.string()` required, and on a callback `ActionDef`, + * `navigate` is the deprecated nested navigation ENVELOPE that + * `executeNavigation` reads `to`/`target`/`redirect` off, so a bare string + * there has never been runnable. + */ +export function readOnSuccessNavigation(value: unknown): OnSuccessNavigation | null { + if (!value || typeof value !== 'object' || Array.isArray(value)) return null; + const navigate = (value as { navigate?: unknown }).navigate; + if (typeof navigate !== 'string' || navigate === '') return null; + return value as OnSuccessNavigation; +} + /** * Dot-path read used by target interpolation. Plain reduce — kept inline * to avoid pulling lodash for a 3-line helper. diff --git a/packages/core/src/actions/__tests__/ActionRunner.onSuccessNavigation.test.ts b/packages/core/src/actions/__tests__/ActionRunner.onSuccessNavigation.test.ts new file mode 100644 index 000000000..e62d47e15 --- /dev/null +++ b/packages/core/src/actions/__tests__/ActionRunner.onSuccessNavigation.test.ts @@ -0,0 +1,275 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +/** + * `ActionSchema.onSuccess` — post-success navigation, the console half + * (objectui#5221; spec half objectstack#9566/#9474). + * + * The pinned `@objectstack/spec` declares the key as a CLOSED STRICT object + * `{ navigate: string, openIn: 'self' | 'newTab' }`, refine-scoped to + * `type: 'api'` and `type: 'script'` — the two types that have a success event + * carrying a server response for `${result.*}` to read. `openIn` is a + * materialised `.default('self')`, so parse output ALWAYS carries a resolved + * member and this runner writes no default of its own. + * + * Three facts under test, and they are different facts: + * + * 1. **The hop happens, through the app's own router.** `navigationHandler` is + * the SPA seam every other navigator in this file already uses (the console + * wires it to react-router's `navigate`). A post-success hop that reached + * for `window.location` would leave the SPA, and `openIn: 'self'` exists + * precisely to be popup-blocker immune. + * 2. **`${result.*}` resolves against the HANDLER's return value**, one level + * below the action envelope — the same level `readActionPayload` hands the + * `redirectUrl` convention. Reading the envelope instead is the objectui#2904 + * "one level too shallow" bug in a new place. + * 3. **The two `openIn` spellings never cross.** `onSuccess.openIn` is + * `'self' | 'newTab'`; the top-level `type: 'url'` switch is + * `'self' | 'new-tab'`. Spec refuses each crossover with a keyed error, so + * this runner must not quietly accept one for the other. + */ + +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { ActionRunner } from '../ActionRunner'; + +type NavCall = [string, { external?: boolean; newTab?: boolean; replace?: boolean } | undefined]; + +/** + * A runner with an `api` handler standing in for the console's `apiHandler`, + * and a navigation spy standing in for react-router. Returns the spy so every + * assertion reads the REAL argument list, never a boolean the test computed. + */ +function makeRunner(payload: unknown = { id: 'rec_42' }) { + const nav = vi.fn(); + const runner = new ActionRunner({}); + runner.setNavigationHandler(nav as never); + runner.setToastHandler(vi.fn() as never); + runner.registerHandler('api', async () => ({ success: true, data: payload })); + runner.registerHandler('script', async () => ({ success: true, data: payload })); + const calls = () => nav.mock.calls as unknown as NavCall[]; + return { runner, nav, calls }; +} + +beforeEach(() => { + vi.restoreAllMocks(); +}); + +describe('ActionSchema.onSuccess — the SPA route hop', () => { + it('lands on the ${result.*}-interpolated route, in the same tab', async () => { + const { runner, calls } = makeRunner({ id: 'rec_42' }); + + const result = await runner.execute({ + type: 'api', + name: 'clone_record', + target: '/api/v1/records/clone', + // The customer shape (titanwind-ehr#1172): clone, then jump to the CLONE. + onSuccess: { navigate: '/app/crm/contacts/${result.id}', openIn: 'self' }, + } as never); + + expect(result.success).toBe(true); + expect(calls()).toHaveLength(1); + const [url, options] = calls()[0]; + expect(url).toBe('/app/crm/contacts/rec_42'); + expect(options?.newTab).toBe(false); + }); + + it("openIn: 'newTab' takes the other branch — same route, new tab", async () => { + const { runner, calls } = makeRunner({ id: 'rec_42' }); + + await runner.execute({ + type: 'api', + name: 'clone_record', + target: '/api/v1/records/clone', + onSuccess: { navigate: '/app/crm/contacts/${result.id}', openIn: 'newTab' }, + } as never); + + expect(calls()).toHaveLength(1); + const [url, options] = calls()[0]; + // Discriminating against the 'self' case above on the SAME route: only the + // tab choice may differ, so a branch that ignored `openIn` fails here. + expect(url).toBe('/app/crm/contacts/rec_42'); + expect(options?.newTab).toBe(true); + }); + + it("runs for type: 'script' too — the other type the refine admits", async () => { + const { runner, calls } = makeRunner({ id: 'rec_9' }); + + await runner.execute({ + type: 'script', + name: 'provision', + target: 'provision', + onSuccess: { navigate: '/app/ops/jobs/${result.id}', openIn: 'self' }, + } as never); + + expect(calls()).toHaveLength(1); + expect(calls()[0][0]).toBe('/app/ops/jobs/rec_9'); + }); + + it('reads ${result.*} from the handler payload, not the action envelope', async () => { + // The pre-#3962 legacy envelope: `{ success, data }` wrapping the handler's + // own value. `${result.id}` must see `rec_inner`, never the envelope. + const { runner, calls } = makeRunner({ success: true, data: { id: 'rec_inner' } }); + + await runner.execute({ + type: 'api', + name: 'clone_record', + target: '/api/v1/records/clone', + onSuccess: { navigate: '/app/crm/contacts/${result.id}', openIn: 'self' }, + } as never); + + expect(calls()[0][0]).toBe('/app/crm/contacts/rec_inner'); + }); + + it('interpolates ${param.*} and ${ctx.*} in the same template', async () => { + const nav = vi.fn(); + const runner = new ActionRunner({ ctx: { tenant: 'acme' } } as never); + runner.setNavigationHandler(nav as never); + runner.setToastHandler(vi.fn() as never); + runner.registerHandler('api', async () => ({ success: true, data: { id: 'rec_7' } })); + + await runner.execute({ + type: 'api', + name: 'clone_record', + target: '/api/v1/records/clone', + params: { view: 'compact' }, + onSuccess: { + navigate: '/app/${ctx.tenant}/contacts/${result.id}?view=${param.view}', + openIn: 'self', + }, + } as never); + + expect((nav.mock.calls as unknown as NavCall[])[0][0]) + .toBe('/app/acme/contacts/rec_7?view=compact'); + }); + + it('percent-encodes every interpolated value (spec: renderers MUST encode)', async () => { + const { runner, calls } = makeRunner({ id: 'a/b c' }); + + await runner.execute({ + type: 'api', + name: 'clone_record', + target: '/api/v1/records/clone', + onSuccess: { navigate: '/app/crm/contacts/${result.id}', openIn: 'self' }, + } as never); + + expect(calls()[0][0]).toBe('/app/crm/contacts/a%2Fb%20c'); + }); +}); + +describe('ActionSchema.onSuccess — controls', () => { + it('an action with NO onSuccess navigates nowhere (positive control included)', async () => { + const { runner, calls } = makeRunner({ id: 'rec_42' }); + + await runner.execute({ + type: 'api', name: 'plain', target: '/api/v1/records/touch', + } as never); + expect(calls()).toHaveLength(0); + + // POSITIVE CONTROL — the same runner, same harness, one key added. Without + // this the assertion above passes just as well when the harness is dead + // (no dispatch, no handler, a spy nobody could ever call). + await runner.execute({ + type: 'api', name: 'hops', target: '/api/v1/records/touch', + onSuccess: { navigate: '/app/x/${result.id}', openIn: 'self' }, + } as never); + expect(calls()).toHaveLength(1); + expect(calls()[0][0]).toBe('/app/x/rec_42'); + }); + + it('does not navigate when the action FAILED — onSuccess is post-SUCCESS', async () => { + const nav = vi.fn(); + const runner = new ActionRunner({}); + runner.setNavigationHandler(nav as never); + runner.setToastHandler(vi.fn() as never); + runner.registerHandler('api', async () => ({ success: false, error: 'nope' })); + + await runner.execute({ + type: 'api', name: 'clone_record', target: '/api/v1/records/clone', + onSuccess: { navigate: '/app/crm/contacts/${result.id}', openIn: 'self' }, + } as never); + + expect(nav).not.toHaveBeenCalled(); + }); + + it('an absent scope member interpolates to empty — the impl contract, measured', async () => { + // NOT invented: `interpolateTarget` has always substituted `''` for a + // nullish path (`if (value == null) return ''`). `${result.*}` joins that + // rule rather than inventing a second one, so a template naming a member + // the response did not carry produces a shortened route, not a literal + // `${result.missing}` in the URL bar. + const { runner, calls } = makeRunner({ id: 'rec_42' }); + + await runner.execute({ + type: 'api', name: 'clone_record', target: '/api/v1/records/clone', + onSuccess: { navigate: '/app/crm/contacts/${result.missing}', openIn: 'self' }, + } as never); + + expect(calls()[0][0]).toBe('/app/crm/contacts/'); + }); + + it('refuses a javascript: destination (author metadata reaches this URL)', async () => { + const { runner, nav } = makeRunner({ id: 'rec_42' }); + + await runner.execute({ + type: 'api', name: 'evil', target: '/api/v1/records/clone', + onSuccess: { navigate: 'javascript:alert(1)', openIn: 'self' }, + } as never); + + expect(nav).not.toHaveBeenCalled(); + }); +}); + +describe('ActionSchema.onSuccess — the two openIn spellings stay apart', () => { + it("does not accept the type:'url' kebab spelling as a new-tab request", async () => { + // Spec refuses `onSuccess.openIn: 'new-tab'` at parse with a keyed error. + // If this runner treated it as new-tab anyway, the renderer would become a + // second, more lenient contract than the one authors are validated against. + const { runner, calls } = makeRunner({ id: 'rec_42' }); + + await runner.execute({ + type: 'api', name: 'clone_record', target: '/api/v1/records/clone', + onSuccess: { navigate: '/app/crm/contacts/${result.id}', openIn: 'new-tab' }, + } as never); + + expect(calls()).toHaveLength(1); + expect(calls()[0][1]?.newTab).toBe(false); + }); + + it("a top-level openIn: 'new-tab' does not steer the onSuccess hop", async () => { + const { runner, calls } = makeRunner({ id: 'rec_42' }); + + await runner.execute({ + type: 'api', name: 'clone_record', target: '/api/v1/records/clone', + openIn: 'new-tab', + onSuccess: { navigate: '/app/crm/contacts/${result.id}', openIn: 'self' }, + } as never); + + expect(calls()[0][1]?.newTab).toBe(false); + }); +}); + +describe('ActionSchema.onSuccess — the legacy chained-callback channel is untouched', () => { + it('still runs an ActionDef callback, and does not treat it as navigation', async () => { + // `ActionDef.onSuccess?: ActionDef | ActionDef[]` predates the spec key and + // is a RUNTIME channel: `@objectstack/spec` strict-refuses `{ type: … }` + // inside `onSuccess`, so no validated metadata can reach it. Retiring it is + // its own card; this pins that implementing the spec key did not silently + // take it away. + const { runner, nav } = makeRunner({ id: 'rec_42' }); + const cb = vi.fn(async () => ({ success: true })); + runner.registerHandler('notify', cb as never); + + await runner.execute({ + type: 'api', name: 'clone_record', target: '/api/v1/records/clone', + onSuccess: { type: 'notify', name: 'ping' }, + } as never); + + expect(cb).toHaveBeenCalledTimes(1); + expect(nav).not.toHaveBeenCalled(); + }); +});