refactor(actions): navigation becomes a declared alias of url, sharing one navigator (#2944) - #2994
Merged
Merged
Conversation
…aring 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>
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Contributor
✅ Console Performance Budget
📦 Bundle Size Report
Size Limits
|
This was referenced Jul 30, 2026
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The last open item of #2944 (item 3), and the first entry on #2945's additions-only list.
ActionRunnerdispatched a seventh action type,navigation, that@objectstack/spec'sActionTypedoes not contain. The issue asked for adecision: promote it upstream, or delete the case. Neither, as stated.
Why not promote
The spec already names this operation.
type: 'url'is "go to a location", withopenIn: 'self' | 'new-tab'for the tab choice. Addingnavigationupstreamwould put a second spec name on one operation — which is the failure this audit
exists to describe, committed one level up:
Why not delete
Deleting does not surface an error.
{ type: 'navigation', to: … }is authoredtoday —
element:buttonCTAs use it — and without the case the action fallsthrough to
executeActionSchema, which returns{ success: true }. The user getsa green success toast and no navigation. That is #2960's trap, reached by way of a
cleanup.
The actual finding: it had already drifted
Two implementations of one concept, and
executeNavigationwas quietly theweaker one:
urlnavigation(before)${param.X}/${ctx.X}interpolationopenInhonoured/api/…→ full-page loadapiBasepromotionreplace(history)So
{ type: 'navigation', to: '/x?p=${param.p}' }shipped the literal${param.p}, while the identicalurlaction resolved it.What this does
ObjectUiLocalActionType/OBJECTUI_LOCAL_ACTION_TYPESdeclarenavigationasobjectui's own alias of
url— the treatment #2985 gave the page-visualizationnames — and both names now route through one
navigateTo.urlgainsreplacepass-through, the one modifier only the alias carried.
Additive.
replaceis omitted from theNavigationHandleroptions object whenunset, so hosts see the shape they already saw.
The guard is structural, not another assertion
Built-in dispatch is now a table typed
Record<RunnableActionType, …>instead ofa
switch. AnActionTypethe spec adds stops compiling until an executorexists for it — #2942's "validates at save, renders nothing at run time" class,
converted into a build error for actions. Verified it bites by deleting one entry:
spec-derived-unions.test.tsadditionally assertsnavigationis absent fromthe spec enum — the inverse tripwire, so the day it is adopted upstream the test
fails and names the alias to retire.
Coverage for two branches that had none
The
/api/full-page short-circuit andapiBasepromotion were untested whileexecuteUrlwas their only caller. They are what the better-auth social-loginredirect dance depends on — pushing
/api/…into the SPA router matches no routeand lands on the home page, so the OAuth flow never starts — and they now have
two callers. (They had no coverage because this project runs in a node
environment with no
windowat all; the new tests stub the minimum and restoreit.)
Also
Corrects the audit doc's Direction B row, which still told readers to promote
navigation, and adds Correction 4 explaining the general point: a"renderer-local dialect" row needs a third option next to promote and delete —
alias to the spec name it duplicates — and choosing starts with asking
whether the spec really lacks the concept or just spells it differently.
Verification
packages/core+packages/typeseslintTwo test files (
basic-renderers,page-variables) hit abeforeAlltimeouton
await import('../renderers')during the full run — the machine was loaded andthe run took 1050s against a normal ~285s. Both pass standalone in 11s (33/33),
with that import taking 2.7s rather than >30s. Not a logic failure.
🤖 Generated with Claude Code