Skip to content

Commit caa0ce1

Browse files
os-zhuangclaude
andcommitted
refactor(actions): navigation becomes a declared alias of 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 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<RunnableActionType, …>` 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 <noreply@anthropic.com>
1 parent 2374a49 commit caa0ce1

7 files changed

Lines changed: 417 additions & 85 deletions

File tree

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
1+
---
2+
"@object-ui/types": minor
3+
"@object-ui/core": minor
4+
---
5+
6+
refactor(actions): `navigation` becomes a named alias of the spec's `url`, sharing one navigator (#2944)
7+
8+
The last open item of #2944: `ActionRunner` dispatched a seventh action type,
9+
`navigation`, that `@objectstack/spec`'s `ActionType` does not contain. The issue
10+
asked for a decision — promote it upstream or delete the case. Neither, as stated.
11+
12+
- **Promoting it is wrong.** The spec already has `url` for "go to a location",
13+
with `openIn` for the new-tab/same-tab choice. A seventh type would put a
14+
second spec name on one operation, which is the exact failure the #2901 audit
15+
is named after: *a second definition of the vocabulary exists, and the renderer
16+
is faithful to the wrong one*.
17+
- **Deleting it is worse, because it is silent.** `{ type: 'navigation', to: … }`
18+
is authored today (`element:button` CTAs). Without the case the action falls
19+
through to `executeActionSchema`, which returns `{ success: true }` — a green
20+
toast that navigates nowhere. That is #2960's trap.
21+
22+
So it stays, but stops being dialect. `ObjectUiLocalActionType` /
23+
`OBJECTUI_LOCAL_ACTION_TYPES` in `@object-ui/types` declare it as objectui's own
24+
alias of `url` — the same treatment #2985 gave `PageVisualizationAlias` — and the
25+
runner routes both names through one navigator.
26+
27+
**The alias had already drifted, which is the point.** `executeNavigation` was
28+
quietly the weaker of the two implementations: no `${param.X}` / `${ctx.X}`
29+
interpolation, `openIn` ignored, and no `/api/…` full-page short-circuit (the
30+
redirect-dance case `url` exists to handle). An author who wrote
31+
`{ type: 'navigation', to: '/x?p=${param.p}' }` shipped the literal `${param.p}`,
32+
while the identical `url` action resolved it. Both names now behave identically;
33+
`url` in turn gains `replace` pass-through, the one modifier only the alias had.
34+
35+
Additive only. `replace` is omitted from the `NavigationHandler` options object
36+
when unset, so hosts see the option shape they already saw.
37+
38+
The new guard is structural rather than another assertion. The runner's built-in
39+
dispatch is a table typed `Record<RunnableActionType, …>` instead of a `switch`,
40+
so an `ActionType` the spec **adds** stops compiling until an executor exists for
41+
it — the Tier-2 "validates at save, renders nothing at run time" failure (#2942)
42+
becomes a build error for actions. `spec-derived-unions.test.ts` additionally
43+
asserts `navigation` is *absent* from the spec enum, so the day it is adopted
44+
upstream, the test fails and names the alias to retire.

docs/audits/2901-spec-enum-renderer-coverage.md

Lines changed: 32 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22

33
Audited against `@objectstack/spec@16.0.0-rc.0` and `objectui@cd09a7b90`.
44
**Re-validated against `17.0.0-rc.0`** — see the addendum at the end, which also
5-
carries three corrections. Read it before acting on any row below.
5+
carries four corrections. Read it before acting on any row below.
66

77
Every row below was confirmed by reading the dispatch code. Nothing was exercised at
88
runtime, so symptoms are read off the code path, not observed. Rows the auditors could
@@ -165,7 +165,7 @@ value corruption rather than disclosure.
165165

166166
| Name | Location | Call |
167167
|---|---|---|
168-
| `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. |
168+
| `navigation` action type | [ActionRunner.ts](../../packages/core/src/actions/ActionRunner.ts) | ~~Promote~~**resolved as a declared alias instead.** See the correction below. |
169169
| `combo` chart type | [normalizeChartSchema.ts:63](../../packages/plugin-charts/src/normalizeChartSchema.ts) | Drawn at `AdvancedChartImpl.tsx:819`. Promote or delete. |
170170
| `system` theme mode | ThemeProvider ×2 | Delete — the spec name is `auto`, and `react/ThemeContext.tsx` already gets it right. |
171171
| `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
358358
to exclude. It is not a permission bypass — row-scoping `$and`-composes as a
359359
separate arm and survives — but it ranks with Tier 1, not Tier 2.
360360

361+
## Correction 4 — "promote `navigation` upstream" was the wrong call
362+
363+
Direction B told the reader to promote the `navigation` action type into the spec,
364+
on the grounds that it is "a real capability the spec doesn't name". The capability
365+
is real; the framing was not. The spec already names it — `type: 'url'`, with
366+
`openIn` for the new-tab choice — so promoting `navigation` would have put a second
367+
spec name on one operation. That is the failure this audit exists to describe,
368+
committed one level up.
369+
370+
The row *also* under-read the cost of the stated alternative. Deleting the case
371+
does not surface an error: the action falls through to `executeActionSchema`, which
372+
returns `{ success: true }`, so the user gets a success toast and no navigation —
373+
#2960's trap, reached by way of a cleanup.
374+
375+
What was missing from the row is that the alias had **already drifted**, which is
376+
the ordinary outcome of two implementations of one concept. `executeNavigation` did
377+
no `${param.X}` interpolation, ignored `openIn`, and skipped the `/api/…`
378+
full-page short-circuit — so `{ type: 'navigation', to: '/x?p=${param.p}' }`
379+
shipped the literal `${param.p}` while the identical `url` action resolved it.
380+
381+
Resolved by declaring it (`ObjectUiLocalActionType`) and routing both names
382+
through one navigator — the same treatment #2985 gave the page-visualization names.
383+
Generalizing: a "renderer-local dialect" row needs a third option next to promote
384+
and delete — **alias to the spec name it duplicates** — and picking between the
385+
three starts with asking whether the spec really lacks the concept or just spells
386+
it differently.
387+
361388
## Status of the items above
362389

363390
| Item | Status |
@@ -366,7 +393,9 @@ separate arm and survives — but it ranks with Tier 1, not Tier 2.
366393
| `before`/`after`, `'not in'`, + 8 legacy spellings | Fixed — #2974 |
367394
| `DensityModeSchema` inert | Resolved upstream (removed in spec 17) |
368395
| 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.) |
369-
| Everything else | Open — #2941 (Tier 1), #2942 (Tier 2), #2943 (Tier 3), #2944 (forks), #2945 (vocabularies) |
396+
| The five shadowing forks | Fixed — #2985 (closes #2944) |
397+
| `navigation` action type | Resolved as a declared alias, not promoted — see Correction 4 |
398+
| Everything else | Open — #2941 (Tier 1), #2942 (Tier 2), #2943 (Tier 3), #2945 (vocabularies) |
370399

371400
One method note for anyone repeating this: the original run read a checkout that
372401
was behind `origin/main`, which produced a spurious "objectui resolves two spec

packages/core/src/actions/ActionRunner.ts

Lines changed: 111 additions & 78 deletions
Original file line numberDiff line numberDiff line change
@@ -10,11 +10,18 @@
1010
* @object-ui/core - Action Runner
1111
*
1212
* Executes actions defined in ActionSchema and EventHandler.
13-
* Supports all spec v2.0.1 action types: script, url, modal, flow, api.
13+
*
14+
* Dispatches every `@objectstack/spec` `ActionType` — `script`, `url`, `modal`,
15+
* `flow`, `api`, `form` — plus the one declared renderer-local alias,
16+
* `navigation` (see `ObjectUiLocalActionType`). The table that does it is typed
17+
* against that vocabulary, so this list cannot go stale the way the old comment
18+
* had (it still claimed five types after `form` shipped).
19+
*
1420
* Features: conditional execution, confirmation, toast notifications,
1521
* redirect handling, action chaining, custom handler registration.
1622
*/
1723

24+
import type { RunnableActionType } from '@object-ui/types';
1825
import { ExpressionEvaluator } from '../evaluator/ExpressionEvaluator';
1926
import { globalUndoManager, type UndoableOperation } from './UndoManager';
2027

@@ -76,7 +83,8 @@ export interface ApiConfig {
7683
* Compatible with both UIActionSchema (spec v2.0.1) and legacy crud.ts ActionSchema.
7784
*/
7885
export interface ActionDef {
79-
/** Action type identifier: 'script' | 'url' | 'modal' | 'flow' | 'api' | 'navigation' | custom */
86+
/** Action type identifier — a `RunnableActionType` (the spec's six plus the
87+
* `navigation` alias of `url`) or a custom type with a registered handler. */
8088
type?: string;
8189
/** Legacy action type field */
8290
actionType?: string;
@@ -353,6 +361,33 @@ export class ActionRunner {
353361
private paramCollectionHandler: ParamCollectionHandler | null;
354362
private resultDialogHandler: ResultDialogHandler | null;
355363

364+
/**
365+
* Built-in dispatch, one entry per runnable action type.
366+
*
367+
* A table rather than a `switch` because the type annotation is the guard:
368+
* `Record<RunnableActionType, …>` stops compiling the moment
369+
* `@objectstack/spec` adds an `ActionType` this runner has no executor for.
370+
* That is the whole Tier-2 failure class for actions — a spec name that
371+
* validates at save and then does nothing at run time (#2942) — converted
372+
* into a build error. It also rejects the reverse: an executor keyed by a
373+
* name that is neither in the spec nor a declared local alias.
374+
*
375+
* `navigation` is that declared alias (see `ObjectUiLocalActionType`); it
376+
* shares `url`'s navigator.
377+
*/
378+
private readonly builtinExecutors: Record<
379+
RunnableActionType,
380+
(action: ActionDef) => Promise<ActionResult>
381+
> = {
382+
script: (action) => this.executeScript(action),
383+
url: (action) => this.executeUrl(action),
384+
modal: (action) => this.executeModal(action),
385+
flow: (action) => this.executeFlow(action),
386+
api: (action) => this.executeAPI(action),
387+
form: (action) => this.executeForm(action),
388+
navigation: (action) => this.executeNavigation(action),
389+
};
390+
356391
constructor(context: ActionContext = {}) {
357392
this.context = withIdentityAlias(context);
358393
this.evaluator = new ExpressionEvaluator(this.context);
@@ -514,43 +549,24 @@ export class ActionRunner {
514549
return result;
515550
}
516551

517-
// Built-in action execution by type
552+
// Built-in action execution by type.
518553
let result: ActionResult;
519-
520-
switch (actionType) {
521-
case 'script':
522-
result = await this.executeScript(action);
523-
break;
524-
case 'url':
525-
result = await this.executeUrl(action);
526-
break;
527-
case 'modal':
528-
result = await this.executeModal(action);
529-
break;
530-
case 'flow':
531-
result = await this.executeFlow(action);
532-
break;
533-
case 'api':
534-
result = await this.executeAPI(action);
535-
break;
536-
case 'form':
537-
result = await this.executeForm(action);
538-
break;
539-
case 'navigation':
540-
result = await this.executeNavigation(action);
541-
break;
542-
default:
543-
// Legacy fallback: check for navigate, api, or onClick
544-
if (action.navigate) {
545-
result = await this.executeNavigation(action);
546-
} else if (action.api || action.endpoint) {
547-
result = await this.executeAPI(action);
548-
} else if (action.onClick) {
549-
await action.onClick();
550-
result = { success: true };
551-
} else {
552-
result = await this.executeActionSchema(action);
553-
}
554+
const builtin = actionType
555+
? this.builtinExecutors[actionType as RunnableActionType]
556+
: undefined;
557+
558+
if (builtin) {
559+
result = await builtin(action);
560+
} else if (action.navigate) {
561+
// Legacy fallback: check for navigate, api, or onClick
562+
result = await this.executeNavigation(action);
563+
} else if (action.api || action.endpoint) {
564+
result = await this.executeAPI(action);
565+
} else if (action.onClick) {
566+
await action.onClick();
567+
result = { success: true };
568+
} else {
569+
result = await this.executeActionSchema(action);
554570
}
555571

556572
await this.handlePostExecution(action, result);
@@ -778,13 +794,34 @@ export class ActionRunner {
778794
if (!rawUrl) {
779795
return { success: false, error: 'No URL provided for url action' };
780796
}
797+
return this.navigateTo(action, rawUrl, action);
798+
}
781799

800+
/**
801+
* The one navigator behind `type: 'url'` and its `navigation` alias.
802+
*
803+
* Both names mean "go to a location", and they had two implementations that
804+
* drifted: only this one interpolated `${param.X}`, promoted `/api/…` to the
805+
* `apiBase`, short-circuited a redirect-dance URL to a full-page load, and
806+
* honoured `openIn`. A `navigation` action was quietly the weaker of the two
807+
* (#2944 item 3). Sharing the body is what keeps the alias honest.
808+
*
809+
* `action` supplies the interpolation context (`params`) and `openIn`;
810+
* `source` supplies the navigation modifiers, which live on the nested
811+
* `action.navigate` block for the legacy shape and on the action itself
812+
* otherwise.
813+
*/
814+
private async navigateTo(
815+
action: ActionDef,
816+
rawUrl: unknown,
817+
source: ActionDef,
818+
): Promise<ActionResult> {
782819
// Apply target ${param.X} / ${ctx.X} interpolation FIRST — the
783820
// ExpressionEvaluator that follows would otherwise see unresolved
784821
// `param.provider` and either error or substitute undefined. Doing it
785822
// here also URL-encodes values (required for query-position params
786823
// like `?provider=foo+bar`).
787-
const interpolated = this.interpolateTarget(rawUrl, action);
824+
const interpolated = this.interpolateTarget(rawUrl as string, action);
788825
let url = this.evaluator.evaluate(interpolated) as string;
789826

790827
if (!this.isValidUrl(url)) {
@@ -805,7 +842,10 @@ export class ActionRunner {
805842
url = `${apiBase}${url}`;
806843
}
807844

808-
const isExternal = url.startsWith('http://') || url.startsWith('https://');
845+
// `source.external` is the `navigation` shape's explicit override; the
846+
// scheme heuristic covers every other case.
847+
const isExternal =
848+
Boolean(source.external) || url.startsWith('http://') || url.startsWith('https://');
809849
// Same-origin API endpoints (most commonly the auth provider's
810850
// `/api/v1/auth/sign-in/social` redirect dance) issue server-side
811851
// 302s that must be followed by the *browser*, not the SPA router.
@@ -823,15 +863,25 @@ export class ActionRunner {
823863
return { success: true };
824864
}
825865
// `openIn` is the first-class, declarative switch (ActionSchema.openIn);
826-
// it wins over the legacy `params.newTab` escape hatch and the
827-
// external-URL heuristic.
866+
// it wins over the legacy `navigate.newTab` / `params.newTab` escape
867+
// hatches and the external-URL heuristic.
868+
const openIn = source.openIn ?? action.openIn;
828869
const newTab =
829-
action.openIn === 'new-tab' ? true
830-
: action.openIn === 'self' ? false
831-
: (action.params?.newTab ?? isExternal);
870+
openIn === 'new-tab' ? true
871+
: openIn === 'self' ? false
872+
: (source.newTab ?? action.params?.newTab ?? isExternal);
873+
874+
// History semantics, the one thing the `navigation` shape carries that
875+
// `ActionSchema` has no field for. Omitted from the handler call when
876+
// unset so hosts see the same options object they always did.
877+
const replace = source.replace ?? action.replace;
832878

833879
if (this.navigationHandler) {
834-
this.navigationHandler(url, { external: isExternal, newTab });
880+
this.navigationHandler(url, {
881+
external: isExternal,
882+
newTab,
883+
...(replace === undefined ? {} : { replace: Boolean(replace) }),
884+
});
835885
return { success: true };
836886
}
837887

@@ -907,9 +957,6 @@ export class ActionRunner {
907957
return result;
908958
}
909959

910-
/**
911-
* Execute navigation action
912-
*/
913960
/**
914961
* `form` action — open a FormView as a routed page (`/forms/:name`, per the
915962
* spec). `target` is the FormView name. Without this case the action fell
@@ -935,37 +982,23 @@ export class ActionRunner {
935982
return { success: true, redirect: to };
936983
}
937984

985+
/**
986+
* `navigation` — objectui's alias of the spec's `url` (`ObjectUiLocalActionType`).
987+
*
988+
* Kept because `{ type: 'navigation', to: … }` is authored today
989+
* (`element:button` CTAs) and because deleting the case would not fail loudly:
990+
* the action would land in `executeActionSchema` and report success while
991+
* navigating nowhere. All it does now is resolve the alias's own spelling of
992+
* the target and hand off to the shared navigator, so the two names cannot
993+
* drift apart again. Prefer `type: 'url'` + `openIn` in new metadata.
994+
*/
938995
private async executeNavigation(action: ActionDef): Promise<ActionResult> {
939-
const nav = action.navigate || action;
940-
const to = this.evaluator.evaluate(nav.to || nav.target) as string;
941-
942-
if (!this.isValidUrl(to)) {
943-
return {
944-
success: false,
945-
error: 'Invalid URL scheme. Only http://, https://, and relative URLs are allowed.',
946-
};
947-
}
948-
949-
const isExternal = nav.external || (typeof to === 'string' && (
950-
to.startsWith('http://') || to.startsWith('https://')
951-
));
952-
953-
if (this.navigationHandler) {
954-
this.navigationHandler(to, {
955-
external: isExternal,
956-
newTab: nav.newTab ?? isExternal,
957-
replace: nav.replace,
958-
});
959-
return { success: true };
960-
}
961-
962-
if (isExternal) {
963-
window.open(to, '_blank', 'noopener,noreferrer');
964-
} else {
965-
return { success: true, redirect: to };
996+
const nav = (action.navigate || action) as ActionDef;
997+
const rawUrl = nav.to || nav.target || nav.redirect;
998+
if (!rawUrl) {
999+
return { success: false, error: 'No URL provided for navigation action' };
9661000
}
967-
968-
return { success: true };
1001+
return this.navigateTo(action, rawUrl, nav);
9691002
}
9701003

9711004
/**

0 commit comments

Comments
 (0)