feat(core,app-shell): read ActionSchema.onSuccess for post-success navigation

.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.

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).

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<string, unknown> = {}) {
+ 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');
+ });
+});

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.

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).