From caa0ce1160a664c65c1d351b723ed03a879b0694 Mon Sep 17 00:00:00 2001 From: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Date: Thu, 30 Jul 2026 16:06:18 +0800 Subject: [PATCH] refactor(actions): `navigation` becomes a declared alias of `url`, sharing one navigator (#2944) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The last open item of #2944. `ActionRunner` dispatched a seventh action type, `navigation`, that `@objectstack/spec`'s `ActionType` does not contain. The issue asked to promote it upstream or delete the case. Neither, as stated. Promoting is wrong: the spec already names this operation — `type: 'url'`, with `openIn` for the new-tab choice — so a seventh type would put a second spec name on one operation, which is the failure this audit exists to describe, committed one level up. Deleting is worse because it is silent: `{ type: 'navigation', to: … }` is authored today (`element:button` CTAs), and without the case the action falls through to `executeActionSchema`, which returns `{ success: true }` — a green toast that navigates nowhere (#2960's trap). The alias had already drifted, which is the actual finding. `executeNavigation` was quietly the weaker of two implementations of one concept: no `${param.X}` interpolation, `openIn` ignored, no `/api/…` full-page short-circuit. So `{ type: 'navigation', to: '/x?p=${param.p}' }` shipped the literal `${param.p}` while the identical `url` action resolved it. So it stays and stops being dialect. `ObjectUiLocalActionType` / `OBJECTUI_LOCAL_ACTION_TYPES` declare it as objectui's own alias of `url` — the treatment #2985 gave the page-visualization names — and both names now route through one `navigateTo`. `url` gains `replace` pass-through, the one modifier only the alias carried; `replace` is omitted from the `NavigationHandler` options when unset, so hosts see the shape they already saw. The guard is structural rather than another assertion: built-in dispatch is a table typed `Record` instead of a `switch`, so an `ActionType` the spec ADDS stops compiling until an executor exists for it — #2942's "validates at save, renders nothing at run time" class, made a build error for actions. `spec-derived-unions.test.ts` additionally asserts `navigation` is absent from the spec enum, so the day it is adopted upstream the test fails and names the alias to retire. Also covers the `/api/` short-circuit and `apiBase` promotion, which had none — the project runs in a node env with no `window`, which is why — and corrects the audit doc's Direction B row, which still prescribed promoting. Co-Authored-By: Claude Opus 5 --- .changeset/navigation-action-alias.md | 44 ++++ .../2901-spec-enum-renderer-coverage.md | 35 +++- packages/core/src/actions/ActionRunner.ts | 189 ++++++++++-------- .../actions/__tests__/ActionRunner.test.ts | 165 ++++++++++++++- .../src/__tests__/spec-derived-unions.test.ts | 26 ++- packages/types/src/index.ts | 4 +- packages/types/src/ui-action.ts | 39 ++++ 7 files changed, 417 insertions(+), 85 deletions(-) create mode 100644 .changeset/navigation-action-alias.md diff --git a/.changeset/navigation-action-alias.md b/.changeset/navigation-action-alias.md new file mode 100644 index 0000000000..a4cea52243 --- /dev/null +++ b/.changeset/navigation-action-alias.md @@ -0,0 +1,44 @@ +--- +"@object-ui/types": minor +"@object-ui/core": minor +--- + +refactor(actions): `navigation` becomes a named alias of the spec's `url`, sharing one navigator (#2944) + +The last open item of #2944: `ActionRunner` dispatched a seventh action type, +`navigation`, that `@objectstack/spec`'s `ActionType` does not contain. The issue +asked for a decision — promote it upstream or delete the case. Neither, as stated. + +- **Promoting it is wrong.** The spec already has `url` for "go to a location", + with `openIn` for the new-tab/same-tab choice. A seventh type would put a + second spec name on one operation, which is the exact failure the #2901 audit + is named after: *a second definition of the vocabulary exists, and the renderer + is faithful to the wrong one*. +- **Deleting it is worse, because it is silent.** `{ type: 'navigation', to: … }` + is authored today (`element:button` CTAs). Without the case the action falls + through to `executeActionSchema`, which returns `{ success: true }` — a green + toast that navigates nowhere. That is #2960's trap. + +So it stays, but stops being dialect. `ObjectUiLocalActionType` / +`OBJECTUI_LOCAL_ACTION_TYPES` in `@object-ui/types` declare it as objectui's own +alias of `url` — the same treatment #2985 gave `PageVisualizationAlias` — and the +runner routes both names through one navigator. + +**The alias had already drifted, which is the point.** `executeNavigation` was +quietly the weaker of the two implementations: no `${param.X}` / `${ctx.X}` +interpolation, `openIn` ignored, and no `/api/…` full-page short-circuit (the +redirect-dance case `url` exists to handle). An author who wrote +`{ type: 'navigation', to: '/x?p=${param.p}' }` shipped the literal `${param.p}`, +while the identical `url` action resolved it. Both names now behave identically; +`url` in turn gains `replace` pass-through, the one modifier only the alias had. + +Additive only. `replace` is omitted from the `NavigationHandler` options object +when unset, so hosts see the option shape they already saw. + +The new guard is structural rather than another assertion. The runner's built-in +dispatch is a table typed `Record` instead of a `switch`, +so an `ActionType` the spec **adds** stops compiling until an executor exists for +it — the Tier-2 "validates at save, renders nothing at run time" failure (#2942) +becomes a build error for actions. `spec-derived-unions.test.ts` additionally +asserts `navigation` is *absent* from the spec enum, so the day it is adopted +upstream, the test fails and names the alias to retire. diff --git a/docs/audits/2901-spec-enum-renderer-coverage.md b/docs/audits/2901-spec-enum-renderer-coverage.md index e00118c8f1..030bd78f19 100644 --- a/docs/audits/2901-spec-enum-renderer-coverage.md +++ b/docs/audits/2901-spec-enum-renderer-coverage.md @@ -2,7 +2,7 @@ Audited against `@objectstack/spec@16.0.0-rc.0` and `objectui@cd09a7b90`. **Re-validated against `17.0.0-rc.0`** — see the addendum at the end, which also -carries three corrections. Read it before acting on any row below. +carries four corrections. Read it before acting on any row below. Every row below was confirmed by reading the dispatch code. Nothing was exercised at runtime, so symptoms are read off the code path, not observed. Rows the auditors could @@ -165,7 +165,7 @@ value corruption rather than disclosure. | Name | Location | Call | |---|---|---| -| `navigation` action type | [ActionRunner.ts:539](../../packages/core/src/actions/ActionRunner.ts) | Fully implemented (`executeNavigation`). **Promote** — it is a real capability the spec doesn't name. | +| `navigation` action type | [ActionRunner.ts](../../packages/core/src/actions/ActionRunner.ts) | ~~Promote~~ — **resolved as a declared alias instead.** See the correction below. | | `combo` chart type | [normalizeChartSchema.ts:63](../../packages/plugin-charts/src/normalizeChartSchema.ts) | Drawn at `AdvancedChartImpl.tsx:819`. Promote or delete. | | `system` theme mode | ThemeProvider ×2 | Delete — the spec name is `auto`, and `react/ThemeContext.tsx` already gets it right. | | `scale-fade` preset | [useAnimation.ts:71](../../packages/react/src/hooks/useAnimation.ts) | Delete with the hyphen/underscore fix. | @@ -358,6 +358,33 @@ On a list whose only narrowing is that filter, the user sees rows they had asked to exclude. It is not a permission bypass — row-scoping `$and`-composes as a separate arm and survives — but it ranks with Tier 1, not Tier 2. +## Correction 4 — "promote `navigation` upstream" was the wrong call + +Direction B told the reader to promote the `navigation` action type into the spec, +on the grounds that it is "a real capability the spec doesn't name". The capability +is real; the framing was not. The spec already names it — `type: 'url'`, with +`openIn` for the new-tab choice — so promoting `navigation` would have put a second +spec name on one operation. That is the failure this audit exists to describe, +committed one level up. + +The row *also* under-read the cost of the stated alternative. Deleting the case +does not surface an error: the action falls through to `executeActionSchema`, which +returns `{ success: true }`, so the user gets a success toast and no navigation — +#2960's trap, reached by way of a cleanup. + +What was missing from the row is that the alias had **already drifted**, which is +the ordinary outcome of two implementations of one concept. `executeNavigation` did +no `${param.X}` interpolation, ignored `openIn`, and skipped the `/api/…` +full-page short-circuit — so `{ type: 'navigation', to: '/x?p=${param.p}' }` +shipped the literal `${param.p}` while the identical `url` action resolved it. + +Resolved by declaring it (`ObjectUiLocalActionType`) and routing both names +through one navigator — the same treatment #2985 gave the page-visualization names. +Generalizing: a "renderer-local dialect" row needs a third option next to promote +and delete — **alias to the spec name it duplicates** — and picking between the +three starts with asking whether the spec really lacks the concept or just spells +it differently. + ## Status of the items above | Item | Status | @@ -366,7 +393,9 @@ separate arm and survives — but it ranks with Tier 1, not Tier 2. | `before`/`after`, `'not in'`, + 8 legacy spellings | Fixed — #2974 | | `DensityModeSchema` inert | Resolved upstream (removed in spec 17) | | Packages lacking the `@objectstack/spec` devDep | **6** — `plugin-list` and `data-objectstack` gained it in #2974; `plugin-charts`, `plugin-dashboard`, `plugin-report`, `components`, `mobile` and `fields` remain. (The body above says 6 because it counted a 13-package "needs a guard" list that omitted `fields`; adding `fields` and removing the two now fixed leaves 6 either way.) | -| Everything else | Open — #2941 (Tier 1), #2942 (Tier 2), #2943 (Tier 3), #2944 (forks), #2945 (vocabularies) | +| The five shadowing forks | Fixed — #2985 (closes #2944) | +| `navigation` action type | Resolved as a declared alias, not promoted — see Correction 4 | +| Everything else | Open — #2941 (Tier 1), #2942 (Tier 2), #2943 (Tier 3), #2945 (vocabularies) | One method note for anyone repeating this: the original run read a checkout that was behind `origin/main`, which produced a spurious "objectui resolves two spec diff --git a/packages/core/src/actions/ActionRunner.ts b/packages/core/src/actions/ActionRunner.ts index 205ed35a77..e5682f5191 100644 --- a/packages/core/src/actions/ActionRunner.ts +++ b/packages/core/src/actions/ActionRunner.ts @@ -10,11 +10,18 @@ * @object-ui/core - Action Runner * * Executes actions defined in ActionSchema and EventHandler. - * Supports all spec v2.0.1 action types: script, url, modal, flow, api. + * + * Dispatches every `@objectstack/spec` `ActionType` — `script`, `url`, `modal`, + * `flow`, `api`, `form` — plus the one declared renderer-local alias, + * `navigation` (see `ObjectUiLocalActionType`). The table that does it is typed + * against that vocabulary, so this list cannot go stale the way the old comment + * had (it still claimed five types after `form` shipped). + * * Features: conditional execution, confirmation, toast notifications, * redirect handling, action chaining, custom handler registration. */ +import type { RunnableActionType } from '@object-ui/types'; import { ExpressionEvaluator } from '../evaluator/ExpressionEvaluator'; import { globalUndoManager, type UndoableOperation } from './UndoManager'; @@ -76,7 +83,8 @@ export interface ApiConfig { * Compatible with both UIActionSchema (spec v2.0.1) and legacy crud.ts ActionSchema. */ export interface ActionDef { - /** Action type identifier: 'script' | 'url' | 'modal' | 'flow' | 'api' | 'navigation' | custom */ + /** Action type identifier — a `RunnableActionType` (the spec's six plus the + * `navigation` alias of `url`) or a custom type with a registered handler. */ type?: string; /** Legacy action type field */ actionType?: string; @@ -353,6 +361,33 @@ export class ActionRunner { private paramCollectionHandler: ParamCollectionHandler | null; private resultDialogHandler: ResultDialogHandler | null; + /** + * Built-in dispatch, one entry per runnable action type. + * + * A table rather than a `switch` because the type annotation is the guard: + * `Record` stops compiling the moment + * `@objectstack/spec` adds an `ActionType` this runner has no executor for. + * That is the whole Tier-2 failure class for actions — a spec name that + * validates at save and then does nothing at run time (#2942) — converted + * into a build error. It also rejects the reverse: an executor keyed by a + * name that is neither in the spec nor a declared local alias. + * + * `navigation` is that declared alias (see `ObjectUiLocalActionType`); it + * shares `url`'s navigator. + */ + private readonly builtinExecutors: Record< + RunnableActionType, + (action: ActionDef) => Promise + > = { + script: (action) => this.executeScript(action), + url: (action) => this.executeUrl(action), + modal: (action) => this.executeModal(action), + flow: (action) => this.executeFlow(action), + api: (action) => this.executeAPI(action), + form: (action) => this.executeForm(action), + navigation: (action) => this.executeNavigation(action), + }; + constructor(context: ActionContext = {}) { this.context = withIdentityAlias(context); this.evaluator = new ExpressionEvaluator(this.context); @@ -514,43 +549,24 @@ export class ActionRunner { return result; } - // Built-in action execution by type + // Built-in action execution by type. let result: ActionResult; - - switch (actionType) { - case 'script': - result = await this.executeScript(action); - break; - case 'url': - result = await this.executeUrl(action); - break; - case 'modal': - result = await this.executeModal(action); - break; - case 'flow': - result = await this.executeFlow(action); - break; - case 'api': - result = await this.executeAPI(action); - break; - case 'form': - result = await this.executeForm(action); - break; - case 'navigation': - result = await this.executeNavigation(action); - break; - default: - // Legacy fallback: check for navigate, api, or onClick - if (action.navigate) { - result = await this.executeNavigation(action); - } else if (action.api || action.endpoint) { - result = await this.executeAPI(action); - } else if (action.onClick) { - await action.onClick(); - result = { success: true }; - } else { - result = await this.executeActionSchema(action); - } + const builtin = actionType + ? this.builtinExecutors[actionType as RunnableActionType] + : undefined; + + if (builtin) { + result = await builtin(action); + } else if (action.navigate) { + // Legacy fallback: check for navigate, api, or onClick + result = await this.executeNavigation(action); + } else if (action.api || action.endpoint) { + result = await this.executeAPI(action); + } else if (action.onClick) { + await action.onClick(); + result = { success: true }; + } else { + result = await this.executeActionSchema(action); } await this.handlePostExecution(action, result); @@ -778,13 +794,34 @@ export class ActionRunner { if (!rawUrl) { return { success: false, error: 'No URL provided for url action' }; } + return this.navigateTo(action, rawUrl, action); + } + /** + * The one navigator behind `type: 'url'` and its `navigation` alias. + * + * Both names mean "go to a location", and they had two implementations that + * drifted: only this one interpolated `${param.X}`, promoted `/api/…` to the + * `apiBase`, short-circuited a redirect-dance URL to a full-page load, and + * honoured `openIn`. A `navigation` action was quietly the weaker of the two + * (#2944 item 3). Sharing the body is what keeps the alias honest. + * + * `action` supplies the interpolation context (`params`) and `openIn`; + * `source` supplies the navigation modifiers, which live on the nested + * `action.navigate` block for the legacy shape and on the action itself + * otherwise. + */ + private async navigateTo( + action: ActionDef, + rawUrl: unknown, + source: ActionDef, + ): Promise { // Apply target ${param.X} / ${ctx.X} interpolation FIRST — the // ExpressionEvaluator that follows would otherwise see unresolved // `param.provider` and either error or substitute undefined. Doing it // here also URL-encodes values (required for query-position params // like `?provider=foo+bar`). - const interpolated = this.interpolateTarget(rawUrl, action); + const interpolated = this.interpolateTarget(rawUrl as string, action); let url = this.evaluator.evaluate(interpolated) as string; if (!this.isValidUrl(url)) { @@ -805,7 +842,10 @@ export class ActionRunner { url = `${apiBase}${url}`; } - const isExternal = url.startsWith('http://') || url.startsWith('https://'); + // `source.external` is the `navigation` shape's explicit override; the + // scheme heuristic covers every other case. + const isExternal = + Boolean(source.external) || url.startsWith('http://') || url.startsWith('https://'); // Same-origin API endpoints (most commonly the auth provider's // `/api/v1/auth/sign-in/social` redirect dance) issue server-side // 302s that must be followed by the *browser*, not the SPA router. @@ -823,15 +863,25 @@ export class ActionRunner { return { success: true }; } // `openIn` is the first-class, declarative switch (ActionSchema.openIn); - // it wins over the legacy `params.newTab` escape hatch and the - // external-URL heuristic. + // it wins over the legacy `navigate.newTab` / `params.newTab` escape + // hatches and the external-URL heuristic. + const openIn = source.openIn ?? action.openIn; const newTab = - action.openIn === 'new-tab' ? true - : action.openIn === 'self' ? false - : (action.params?.newTab ?? isExternal); + openIn === 'new-tab' ? true + : openIn === 'self' ? false + : (source.newTab ?? action.params?.newTab ?? isExternal); + + // History semantics, the one thing the `navigation` shape carries that + // `ActionSchema` has no field for. Omitted from the handler call when + // unset so hosts see the same options object they always did. + const replace = source.replace ?? action.replace; if (this.navigationHandler) { - this.navigationHandler(url, { external: isExternal, newTab }); + this.navigationHandler(url, { + external: isExternal, + newTab, + ...(replace === undefined ? {} : { replace: Boolean(replace) }), + }); return { success: true }; } @@ -907,9 +957,6 @@ export class ActionRunner { return result; } - /** - * Execute navigation action - */ /** * `form` action — open a FormView as a routed page (`/forms/:name`, per the * spec). `target` is the FormView name. Without this case the action fell @@ -935,37 +982,23 @@ export class ActionRunner { return { success: true, redirect: to }; } + /** + * `navigation` — objectui's alias of the spec's `url` (`ObjectUiLocalActionType`). + * + * Kept because `{ type: 'navigation', to: … }` is authored today + * (`element:button` CTAs) and because deleting the case would not fail loudly: + * the action would land in `executeActionSchema` and report success while + * navigating nowhere. All it does now is resolve the alias's own spelling of + * the target and hand off to the shared navigator, so the two names cannot + * drift apart again. Prefer `type: 'url'` + `openIn` in new metadata. + */ private async executeNavigation(action: ActionDef): Promise { - const nav = action.navigate || action; - const to = this.evaluator.evaluate(nav.to || nav.target) as string; - - if (!this.isValidUrl(to)) { - return { - success: false, - error: 'Invalid URL scheme. Only http://, https://, and relative URLs are allowed.', - }; - } - - const isExternal = nav.external || (typeof to === 'string' && ( - to.startsWith('http://') || to.startsWith('https://') - )); - - if (this.navigationHandler) { - this.navigationHandler(to, { - external: isExternal, - newTab: nav.newTab ?? isExternal, - replace: nav.replace, - }); - return { success: true }; - } - - if (isExternal) { - window.open(to, '_blank', 'noopener,noreferrer'); - } else { - return { success: true, redirect: to }; + const nav = (action.navigate || action) as ActionDef; + const rawUrl = nav.to || nav.target || nav.redirect; + if (!rawUrl) { + return { success: false, error: 'No URL provided for navigation action' }; } - - return { success: true }; + return this.navigateTo(action, rawUrl, nav); } /** diff --git a/packages/core/src/actions/__tests__/ActionRunner.test.ts b/packages/core/src/actions/__tests__/ActionRunner.test.ts index b16ec661f1..19cff72274 100644 --- a/packages/core/src/actions/__tests__/ActionRunner.test.ts +++ b/packages/core/src/actions/__tests__/ActionRunner.test.ts @@ -6,7 +6,7 @@ * LICENSE file in the root directory of this source tree. */ -import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import { ActionRunner, executeAction, @@ -572,6 +572,169 @@ describe('ActionRunner', () => { expect(result.success).toBe(false); expect(result.error).toContain('Invalid URL'); }); + + it('accepts a flat `to` with no `navigate` block (element:button CTAs)', async () => { + const navHandler = vi.fn(); + runner.setNavigationHandler(navHandler); + const result = await runner.execute({ + type: 'navigation', + to: '/apps/cloud_control/sys_environment', + }); + expect(result.success).toBe(true); + expect(navHandler).toHaveBeenCalledWith( + '/apps/cloud_control/sys_environment', + expect.objectContaining({ external: false }), + ); + }); + + it('errors when no target is given, instead of reporting a no-op as success', async () => { + const result = await runner.execute({ type: 'navigation' }); + expect(result.success).toBe(false); + expect(result.error).toContain('No URL provided'); + }); + + // ------------------------------------------------------------------ + // `navigation` is an alias of `url` (#2944 item 3) and shares its + // navigator. Before that, it was quietly the weaker of the two: no + // `${param.X}` interpolation, `openIn` ignored, no `/api/…` full-page + // short-circuit. These pin the convergence. + // ------------------------------------------------------------------ + + it('interpolates ${param.X} in the target, as `url` always did', async () => { + const navHandler = vi.fn(); + runner.setNavigationHandler(navHandler); + await runner.execute({ + type: 'navigation', + to: '/records?owner=${param.owner}', + params: { owner: 'u 1' }, + }); + expect(navHandler).toHaveBeenCalledWith( + '/records?owner=u%201', + expect.objectContaining({ external: false }), + ); + }); + + it('honours `openIn` — the declarative switch it used to ignore', async () => { + const navHandler = vi.fn(); + runner.setNavigationHandler(navHandler); + await runner.execute({ type: 'navigation', to: '/print/a3', openIn: 'new-tab' }); + expect(navHandler).toHaveBeenCalledWith( + '/print/a3', + expect.objectContaining({ newTab: true }), + ); + }); + + it('still lets `navigate.newTab` force a new tab without `openIn`', async () => { + const navHandler = vi.fn(); + runner.setNavigationHandler(navHandler); + await runner.execute({ + type: 'navigation', + navigate: { to: '/print/a4', newTab: true }, + }); + expect(navHandler).toHaveBeenCalledWith( + '/print/a4', + expect.objectContaining({ newTab: true }), + ); + }); + + it('treats an explicit `navigate.external` as external', async () => { + const navHandler = vi.fn(); + runner.setNavigationHandler(navHandler); + await runner.execute({ + type: 'navigation', + navigate: { to: '/go/elsewhere', external: true }, + }); + expect(navHandler).toHaveBeenCalledWith( + '/go/elsewhere', + expect.objectContaining({ external: true }), + ); + }); + }); + + describe('url ⇄ navigation share one navigator (#2944 item 3)', () => { + it('`url` passes `replace` through — the one modifier only the alias carried', async () => { + const navHandler = vi.fn(); + runner.setNavigationHandler(navHandler); + await runner.execute({ type: 'url', target: '/records/1', replace: true }); + expect(navHandler).toHaveBeenCalledWith( + '/records/1', + expect.objectContaining({ replace: true }), + ); + }); + + it('omits `replace` from the handler options when unset', async () => { + const navHandler = vi.fn(); + runner.setNavigationHandler(navHandler); + await runner.execute({ type: 'navigation', to: '/records/1' }); + expect(navHandler).toHaveBeenCalledWith('/records/1', { external: false, newTab: false }); + }); + + it('both names resolve the same target the same way', async () => { + const viaUrl = vi.fn(); + const viaNav = vi.fn(); + runner.setNavigationHandler(viaUrl); + await runner.execute({ type: 'url', target: '/x?p=${param.p}', params: { p: 'a b' } }); + runner.setNavigationHandler(viaNav); + await runner.execute({ type: 'navigation', to: '/x?p=${param.p}', params: { p: 'a b' } }); + expect(viaNav.mock.calls[0]).toEqual(viaUrl.mock.calls[0]); + }); + + }); + + // The two branches the shared navigator must not lose. Both were untested + // while `executeUrl` was their only caller, and both are what the better-auth + // social-login redirect dance depends on: an `/api/…` target has to be a + // full-page load, because pushing it into the SPA router matches no route and + // lands on the home page, so the OAuth flow never starts. + describe('the /api/ full-page short-circuit', () => { + // This project runs in a node environment, so there is no `window` at all — + // which is exactly why these two branches had no coverage. Stub the minimum + // the navigator touches and put it back afterwards. + type MaybeWindow = { window?: unknown }; + let href: ReturnType; + let hadWindow: boolean; + let previousWindow: unknown; + + beforeEach(() => { + href = vi.fn(); + hadWindow = 'window' in globalThis; + previousWindow = (globalThis as MaybeWindow).window; + (globalThis as MaybeWindow).window = { + location: { + origin: 'http://localhost', + get href() { return ''; }, + set href(v: string) { href(v); }, + }, + open: vi.fn(), + }; + }); + + afterEach(() => { + if (hadWindow) (globalThis as MaybeWindow).window = previousWindow; + else delete (globalThis as MaybeWindow).window; + }); + + it('sends an /api/ target to a full-page load, not the SPA router', async () => { + const navHandler = vi.fn(); + runner.setNavigationHandler(navHandler); + + const result = await runner.execute({ + type: 'url', + target: '/api/v1/auth/sign-in/social?provider=${param.provider}', + params: { provider: 'github' }, + }); + + expect(result.success).toBe(true); + expect(href).toHaveBeenCalledWith('/api/v1/auth/sign-in/social?provider=github'); + expect(navHandler).not.toHaveBeenCalled(); + }); + + it('prefixes an /api/ target with the context apiBase (split SPA + backend)', async () => { + const split = new ActionRunner({ ...context, apiBase: 'http://localhost:3000/' }); + await split.execute({ type: 'url', target: '/api/v1/auth/sign-in/social' }); + + expect(href).toHaveBeenCalledWith('http://localhost:3000/api/v1/auth/sign-in/social'); + }); }); describe('form action type', () => { diff --git a/packages/types/src/__tests__/spec-derived-unions.test.ts b/packages/types/src/__tests__/spec-derived-unions.test.ts index 1b09930a2d..1f45bd64da 100644 --- a/packages/types/src/__tests__/spec-derived-unions.test.ts +++ b/packages/types/src/__tests__/spec-derived-unions.test.ts @@ -30,6 +30,10 @@ * The `satisfies` checks below are the real enforcement: they stop compiling if * the objectui alias stops covering the spec vocabulary. The runtime assertions * guard against the spec's own enum being emptied or renamed underneath us. + * + * The last case is the inverse: `navigation` is a name objectui runs that the + * spec does NOT have. It is asserted absent from the spec so that the day the + * spec adopts it, this file fails and names the alias to retire. */ import { describe, it, expect } from 'vitest'; import { @@ -38,9 +42,10 @@ import { ActionType as SpecActionType, PageTypeSchema as SpecPageTypeSchema, } from '@objectstack/spec/ui'; +import { OBJECTUI_LOCAL_ACTION_TYPES } from '../ui-action'; import type { ChartType } from '../data-display'; import type { ReportType } from '../reports'; -import type { ActionType } from '../ui-action'; +import type { ActionType, RunnableActionType } from '../ui-action'; import type { PageType, PageVisualizationAlias } from '../layout'; /** @@ -52,9 +57,11 @@ const _chartCovers = null as unknown as SpecChart satisfies ChartType; const _reportCovers = null as unknown as 'tabular' | 'summary' | 'matrix' | 'joined' satisfies ReportType; const _actionCovers = null as unknown as 'script' | 'url' | 'modal' | 'flow' | 'api' | 'form' satisfies ActionType; const _pageCovers = null as unknown as 'record' | 'home' | 'app' | 'utility' | 'list' satisfies PageType; -// The sanctioned local extension is still part of the union. +// The sanctioned local extensions are still part of their unions. const _vizCovers = null as unknown as PageVisualizationAlias satisfies PageType; +const _runnableCovers = null as unknown as ActionType satisfies RunnableActionType; void _chartCovers; void _reportCovers; void _actionCovers; void _pageCovers; void _vizCovers; +void _runnableCovers; /** Read a spec enum's members, failing loudly if the shape ever changes. */ const optionsOf = (schema: unknown, name: string): string[] => { @@ -78,6 +85,21 @@ describe('unions derived from a spec vocabulary stay derived (#2944)', () => { expect(optionsOf(SpecActionType, 'ActionType')).toContain('form'); }); + it('`navigation` is objectui\'s declared alias, not a spec action type', () => { + // #2944 item 3 asked for a decision: promote `navigation` upstream or delete + // the `ActionRunner` case. Neither, as stated. The spec already has `url` for + // "go to a location" (with `openIn`), so a seventh type would be a second + // spec name for one operation — the exact failure this audit is named after. + // And deleting the case is silent, not loud: the action falls into + // `executeActionSchema`, which returns success without navigating (#2960). + // + // So it stays as a NAMED local alias sharing `url`'s navigator. This test is + // the tripwire: if the spec ever does adopt `navigation`, it fails and the + // alias is the thing to retire. + expect(optionsOf(SpecActionType, 'ActionType')).not.toContain('navigation'); + expect([...OBJECTUI_LOCAL_ACTION_TYPES]).toEqual(['navigation']); + }); + it('ReportType includes `joined` — the member the fork dropped', () => { expect(optionsOf(SpecReportType, 'ReportType')).toContain('joined'); }); diff --git a/packages/types/src/index.ts b/packages/types/src/index.ts index f0d9753316..510961d2fa 100644 --- a/packages/types/src/index.ts +++ b/packages/types/src/index.ts @@ -867,6 +867,8 @@ export type { ActionLocation, ActionComponent, ActionType, + ObjectUiLocalActionType, + RunnableActionType, ActionParam, ActionSchema as UIActionSchema, ActionGroup, @@ -883,7 +885,7 @@ export type { UndoRedoState, } from './ui-action'; -export { ACTION_LOCATIONS, ActionLocationSchema } from './ui-action'; +export { ACTION_LOCATIONS, ActionLocationSchema, OBJECTUI_LOCAL_ACTION_TYPES } from './ui-action'; // ============================================================================ // ObjectStack Protocol Namespaces - Protocol Re-exports diff --git a/packages/types/src/ui-action.ts b/packages/types/src/ui-action.ts index 1cc1dae59f..716fcb371a 100644 --- a/packages/types/src/ui-action.ts +++ b/packages/types/src/ui-action.ts @@ -80,6 +80,45 @@ export type ActionComponent = */ export type ActionType = z.infer; +/** + * Renderer-local action types — names `ActionRunner` dispatches that the spec's + * `ActionType` does not contain (#2944 item 3, #2945). + * + * `navigation` is an **alias of the spec's `url`**, not a seventh kind: both + * mean "go to a location". It survives for two reasons. + * 1. `{ type: 'navigation', to: … }` is a live authored shape — `element:button` + * CTAs use it. + * 2. Dropping the case would not fail loudly. The action would fall through to + * `executeActionSchema`, which returns `{ success: true }` — a green toast + * that navigates nowhere, which is #2960's exact trap. + * + * It is declared here rather than left implicit so that it stops being dialect: + * an importer can see that `navigation` is objectui's own, and the runner routes + * it through the same navigator as `url`, so the two can no longer drift the way + * they had (the `navigation` path did no `${param.X}` interpolation, ignored + * `openIn`, and skipped the `/api/…` full-page short-circuit that `url` has). + * + * Prefer `type: 'url'` + `openIn` in new metadata. If the spec ever adopts + * `navigation`, the guard in `__tests__/spec-derived-unions.test.ts` fails and + * points at this alias to retire. + */ +export type ObjectUiLocalActionType = 'navigation'; + +export const OBJECTUI_LOCAL_ACTION_TYPES = [ + 'navigation', +] as const satisfies readonly ObjectUiLocalActionType[]; + +/** + * Every action `type` that `ActionRunner` dispatches to a built-in executor: + * the spec vocabulary plus the declared local aliases above. + * + * The runner's dispatch table is typed `Record`, so a + * value the spec ADDS stops compiling until an executor exists for it. That + * turns the Tier-2 failure — a spec name that validates and then renders + * nothing (#2942) — into a build error for actions. + */ +export type RunnableActionType = ActionType | ObjectUiLocalActionType; + /** * Field type for action parameters * Subset of field types commonly used in action parameter collection UIs.