Uh oh!
There was an error while loading. Please reload this page.
feat(core,app-shell): read ActionSchema.onSuccess for post-success navigation - #5933
Merged
Merged
Conversation
…vigation
`@objectstack/spec` declares `onSuccess` as a closed strict object
`{ navigate: string, openIn: 'self' | 'newTab' }`, refine-scoped to
`type: 'api'` and `type: 'script'`. Nothing in this renderer read it, so the
declared hop never happened: 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.
`handlePostExecution` now performs the hop through `navigationHandler`, the
SPA seam the console wires to react-router's `navigate`. `interpolateTarget`
gains a `${result.*}` scope, resolved against the handler's own return value
via `readActionPayload` and supplied only by this call site. No `openIn`
default is written here — spec materialises `.default('self')` — and the two
`openIn` spellings are kept apart.
The console server-action wrapper gains the handler-return half: an explicit
`openIn: 'self'` alongside `redirectUrl` takes the same-tab route hop, while a
`redirectUrl` without it keeps its shipped new-tab behaviour. A declared
`onSuccess` block defers to the runner so one navigation happens, not two.
Part of #5221
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EuPCi56cnGyykygi3z9w4m
This was referenced Aug 24, 2026
Contributor
✅ Console Performance Budget
The eager closure is every chunk the entry reaches through static imports — what the browser fetches and parses before the app renders. The entry chunk on its own is a small fraction of it. 📦 Bundle Size Report
Size Limits
|
This was referenced Aug 24, 2026
os-zhuang
marked this pull request as ready for review
August 24, 2026 02:50
Uh oh!
There was an error while loading. Please reload this page.
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Fixes#5221
Console half of
ActionSchema.onSuccesspost-success navigation — the SPA route hop, theapi/scriptexecution path, and the${result.*}interpolation scope.Pin measurement chain
The card asserts both blockers satisfied and the spec half merged. That is a claim about
another repo's
main, so it was measured here, on this branch's base, before any code waswritten.
shared checkout
/home/user/objectui, whose HEAD was an older commit; the claim comment onthe issue quotes that reading and declares a divergence window. Re-measured against this
branch's base (
0fce2ef81), there is no divergence window — the resolved pin is theregistry's
latest.1 — exactly one resolved version, nothing below it. From
pnpm-lock.yamlat the branch base:All 31 importers resolve to one version; no entry sits below it.
2 — that version is published, and is
latest.So the pin does not lag: nothing to declare.
3 — the installed package's own declaration carries the ruled shape. From
node_modules/@objectstack/spec/dist/action.zod-*.d.ts:Closed strict object ·
navigate: string·openInaZodDefaultover the closed enum.The refine is in the same artifact:
4 — a runtime parse against the installed pin, because a declaration is not behaviour:
PIN-1 is the load-bearing one: parse output always carries
openInresolved, which is whyno default is written on the console side.
What was actually broken
Not "the key is unread".
ActionRunnerhas its own, olderActionDef.onSuccessmeaning —ActionDef | ActionDef[], chained callbacks — and the ruled block fell into it: it wasdispatched as an action,
executeNavigationreadnavigate.tooff a string, and the authorgot a red toast reading "No URL provided for navigation action" and no hop. That is the
invisible-failure class, with an error message pointing away from the cause.
The two are told apart by the spec's own declaration — a non-array object whose
navigateis a string. Nothing else can produce that shape: the spec object is strict with
navigate: z.string()required, and on a callbackActionDefnavigateis the deprecatednested navigation envelope that
executeNavigationreadsto/target/redirectoff, soa bare string there has never been runnable. This is a narrowing to the declared
contract, not a lenient fallback: a shape the spec refuses gains no new reading.
What changed
ActionRunner.handlePostExecutionperforms the declared hop throughnavigationHandler— the SPA seam every other navigator in that file already uses, whichthe console wires to react-router's
navigate. No navigation mechanism is introduced.This one seam covers both types the refine admits:
api(the console'sapiHandler),script(the console server-action wrapper), and the runner's ownexecuteAPIfallback.ActionRunner.interpolateTargetgains a${result.*}scope beside${param.*}and${ctx.*}. The scope map defines the vocabulary and the pattern is built from itskeys, so
resultexists only where a result exists — a target interpolated before itsrequest still has no
resultto name, rather than silently blanking the token.${result.*}resolves against the handler's own return value viareadActionPayload,one level below the action envelope — the level the
redirectUrlconvention already reads.openIn: read as the one member that changes the branch. No?? 'self'— that wouldbe a second source of truth for a default the producer already resolved. The two spellings
are kept apart: this reads
onSuccess.openIn('self' | 'newTab') and never the top-leveltype: 'url'switch ('self' | 'new-tab').consoleServerAction.tsgains the handler-return half: a handler may returnopenIn: 'self'beside itsredirectUrlto ask for the same-tab jump, while aredirectUrlwithoutopenInkeeps its shipped new-tab behaviour — no silent flip.When the action declares an
onSuccessblock, the wrapper defers to the runner and onlytidies its pre-opened tab, so one navigation happens rather than two.
Reachability — please read before merging
The runner half is live, but not from every surface. Measured on this branch:
onSuccessreaches the runnerDeclaredActionsBar(record header)ObjectGrid.onActionDef(row actions)RelatedRecordActionsBridgeuseNavActionDispatchaction:button/action:icon/action:group/action:menuThose four are the subject of #5493, which is where their wiring belongs — deliberately not
ridden in here. The customer report this card cites (clone-then-jump from a record header)
lands on the record-header surface, which is live with this change.
KNOWN_GAPSreason text inscripts/check-action-forward-parity.mjsboth state that the runner "has honoured it allalong", citing the chained-callback lines. That premise was false for the ruled shape — the
callback path failed on it, as above — and this PR is what makes it true. A comment
recording that measurement has been left on that card so the next reader does not act on the
stale wording.
check:action-forward-paritystill exits 0 here; the ratchet is undisturbed.Verification
All legs below ran at
59f63aab9, after the final commit, on a treegit statusreportsclean.
Cross-package resolution — no stale
dist/in the vitest legs. The root vitest configaliases the package to source, so these tests never read build output:
For the type-check leg, which does resolve through package exports, the new export was
confirmed to have reached
dist:The arms, and what each is discriminated against. Reverse verification ablated one
mechanism at a time; each mutation was confirmed on disk by grepping the injected and the
removed text (never an editor exit code), each direction was predicted before running, and
each leg was restored with
git diff HEAD --statempty. Every script carriedtrap '<restore>' EXIT INT TERM.${result.*}scopeopenIn(force same tab)'self'arm greenonSuccessdeferralopenInspellings crossjavascript:armopenIn: 'self'branchEvery assertion in both new suites therefore has a demonstrated failure mode; there are no
legs left that pass regardless of the subject. The two "X did not happen" arms that worried
me most —
javascript:refused, and no-navigation-on-failure — are E and G. The"no
onSuccessnavigates nowhere" arm carries its positive control inside the same test(same runner, same harness, one key added), because that assertion passes just as well
against a dead harness.
anchor, the bare
if (!this.isValidUrl(url)) {, matches twice inActionRunner.ts, sothe write was refused and the suite read green — a green that meant "nothing was
ablated", not "the guard is load-bearing". The on-disk confirmation caught it
(
injected-text count = 0); E was re-run against a unique anchor and is the row above.Commands, with exit codes captured before any pipe.
Both script names are echoed above, so neither run was a zero-match silent pass.
Gates — each quoted by its own verdict line, not by a shell status.
check:eager-closurerefuses to report without a console build (it calls an absent report abroken gauge, not a passing budget), so
apps/consolewas built first and the line above isa real verdict rather than a skipped one.
The i18n gates were run even though this change adds no user-facing string — the one string
added is a developer
console.warn:Lint — no narrowing to declare; the repo-wide scan was run.
eslint . --no-inline-configover the population eslint's own config selects:
Every file this PR touches carries 0 errors. The 89 are pre-existing and sit outside the
46 linted packages —
check-lint-coveragereports0 with outstanding errors (0 total)across those. The added test code follows the surrounding files' existing
as anyfixtureconvention, which lints as a warning;
.github/workflows/lint.ymlstates--max-warningsisdeliberately unset, so warnings are not a gate.
Not in scope
navigateOnSuccessis mount-blind and says nothing when its destination is refused — the key has no ruling and its own contract question is still open #5034 isplugin-form'snavigateOnSuccess— a different key with different call sites,and untouched here. The two shapes are deliberately kept apart.
ActionRunner's legacyActionDef.onSuccesschained-callback channel. Measuredwhile working: it is unreachable from validated metadata (the spec strict-refuses
{ type: … }insideonSuccess) and has zero producers in this repo outsideActionRunner's own two unit tests. Removing an exported runtime contract is its owncard; filed separately and left running here.
onSuccessblock and a handler-returnedredirectUrlwhen an action carries both. The spec rules each surface's own default but not this.
Shipping something coherent required picking one, so the declared block wins and the choice
is marked at the line that implements it. Escalated in the dev report for a ruling.
Generated by Claude Code