Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 44 additions & 0 deletions .changeset/navigation-action-alias.md
Original file line number Diff line number Diff line change
@@ -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<RunnableActionType, …>` 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.
35 changes: 32 additions & 3 deletions docs/audits/2901-spec-enum-renderer-coverage.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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. |
Expand Down Expand Up @@ -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 |
Expand All @@ -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
Expand Down
189 changes: 111 additions & 78 deletions packages/core/src/actions/ActionRunner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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<RunnableActionType, …>` 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<ActionResult>
> = {
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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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<ActionResult> {
// 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)) {
Expand All @@ -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.
Expand All @@ -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 };
}

Expand Down Expand Up @@ -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
Expand All @@ -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<ActionResult> {
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);
}

/**
Expand Down
Loading
Loading