Uh oh!
There was an error while loading. Please reload this page.
plugin-form: route navigateOnSuccess through the host navigation seam, and say when a destination is refused (#5034) - #5558
Merged
Conversation
…ation (#5034) Points 1 and 2 of objectui#5034. Point 3 (the url contract for this key) is deliberately untouched and remains with objectui#5548. Point 1 — mount-blindness. Both call sites (`ObjectForm`'s `else if (!schema.submitHandler)` arm and `WizardForm`'s trailing `else`) travelled to an accepted destination with a bare `window.location.assign`, which resolves a rooted path against the ORIGIN root and so leaves the application under a host mounted at a sub-path. Both now route an app-relative destination through `pendingRedirect` + `useSubmitRedirectNavigation` — the seam PR #5111 landed for `submitBehavior.url`, already imported and wired into both components ~440 lines above these arms. `delayMs: 0` reuses that one mechanism rather than minting a second, and incidentally gives this arm the unmount-cancellation property objectui#5033 bought for the other one. A same-origin ABSOLUTE destination keeps browser-level navigation. `submitBehavior.url` is relative-only so the shared hook is right to hand over everything it holds; this key is not, and `HostNavigationValue.navigate` declares `to` to be "an already-resolved, application-relative path, never an absolute URL ... It is the CALLER's job to have judged the destination". Same judgement, same reused predicate, as objectui#5112 made on `thankYouPage.redirectUrl`. Point 2 — a refused destination was reported only as a generic success. The toast was byte-identical to the one a form with no `navigateOnSuccess` produces, so a mistyped destination or a record with no usable id failed with nobody told. The success toast now carries a note, and the authored template is logged for the author. The write succeeded, so this stays a success rather than becoming an error state. WHICH destinations are accepted is unchanged; `resolveSuccessNavigate` and `submitRedirectNavigation.ts` are untouched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014zHsbJoTkTZeJQ5DLbRXrE
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
|
os-sales
marked this pull request as ready for review
August 21, 2026 11:04
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.
Refs #5034
Lands points 1 and 2 of #5034. Point 3 — the url contract for this key — is deliberately not touched here and stays with the decision card #5548.
resolveSuccessNavigateandsubmitRedirectNavigation.tsare unmodified. Which values are accepted is byte-identical; this PR changes only who navigates once a value is accepted and what is said when one is refused.Point 1 — mount-blindness
Both call sites —
ObjectForm'selse if (!schema.submitHandler)arm andWizardForm's trailingelse— travelled to an accepted destination with a barewindow.location.assign(nav). A rooted path such as/apps/x/o/record/r1assigned that way resolves against the origin root, so under a host mounted at a sub-path (the framework CLI configures one for every embedded deployment; the console runs at basename/_console) an authored in-app destination left the application.The mechanism for this was already ruled and already landed — PR #5111, the optional injected navigation seam — and both components already held it: the
pendingRedirectstate anduseSubmitRedirectNavigationsit ~440 lines above each of these arms, imported and wired forsubmitBehavior.url. This arm was the one call site still bypassing it. No second seam was built; the rejected alternatives recorded insubmitRedirectNavigation.ts's module comment were not re-derived.The
delayMsjudgement call.PendingSubmitRedirectcarriesdelayMsandnavigateOnSuccesshas no delay concept. This reuses the same hook withdelayMs: 0, as the dispatch leaned toward. The seam's own comment notes an unset delay was already "go now" — a zero timer — so this is the existing semantics, not a new case. Reuse also hands this arm the property #5033 bought for the other one: unmounting cancels the wait, so a navigation cannot fire into a form the submitter has left.An arm split, required by the seam's own contract rather than by caution.
submitBehavior.urlis relative-only (objectstack#7496), so the shared hook is correct to hand the host everything it ever holds.navigateOnSuccessis not relative-only — its same-origin guard acceptshttps://own-host/record/1too — andHostNavigationValue.navigatedeclares:So this call site judges. An app-relative destination goes through the seam; a same-origin absolute keeps the browser-level
window.location.assignit has always had. Routing an absolute through a router would rewrite the author's full address into a path the host then places at a different one — an author who spelled out the whole address asked for that address.This is the same judgement #5112 made on
thankYouPage.redirectUrl, whose acceptance set has exactly this shape, and its predicateisAppRelativeDestinationis reused rather than re-derived — as the cross-reference comment on the card invited. It leaves the acceptance set untouched either way: if #5548 rules convergence and refuses same-origin absolutes at the door, that branch simply becomes unreachable and can be deleted.Point 2 — a refused destination was reported only as a generic success
Matching the card's own correction of #4989's overstatement: this was not total silence. The submitter does get
toast.success, so they are not left facing a still-filled form and are not invited to resubmit. The injury is that the toast was indistinguishable from the no-navigateOnSuccesscase — an author who mistyped the destination, or whose record carried no usable id, saw a form that looked entirely healthy and had silently stopped honouring a key they wrote.The write genuinely succeeded, so this stays a success, not an error and not a blocking panel. The success toast now carries a note, per the maintainer ruling of 2026-08-17 ("the success toast carries a note that the declared navigation was not performed — never indistinguishable from the no-key case"). The authored template goes to
console.warnfor the author.The note names no reason on purpose:
resolveSuccessNavigateanswers null for two different causes and returns no discriminant, so a reason could only be re-derived by reimplementing its internals at the call site — where it would drift, and would bake today's acceptance rule into user-visible prose while #5548 is open on exactly that rule.The note is single-sourced (
NAVIGATE_ON_SUCCESS_REFUSED_NOTE) so a wizard and a flat form cannot tell a submitter two different things about one refusal, and a test pins that they do not. It lives inWizardForm.tsxrather than in the natural homesuccessBehavior.ts, which is read-only for this card;ObjectFormalready importsWizardForm, so this is the dependency direction that exists and adds no cycle. A reviewer may prefer a small shared module — the file fence did not permit creating one, so flagging it rather than silently widening scope.Verification
All runs local, through the shared verify lock, on the exact pushed tree
156a31274. Exit codes captured before any pipe.pnpm exec vitest run packages/plugin-form/VITEST_EXIT=0pnpm --filter @object-ui/plugin-form type-checkTYPECHECK_EXIT=0pnpm lint(full repo,turbo run lint)FULL_LINT_EXIT=0— 47/47 tasks, 0 errorsnode scripts/check-control-bytes.mjs0— "OK (scanned 4604 tracked text file(s))"node scripts/check-changeset-presence.mjs0— "3 source file(s) of 1 released package(s) changed, and this change declares 1 changeset(s)"node scripts/check-lint-coverage.mjs0— "46/46 packages linted, 0 with outstanding errors"The full repo lint ran, so no narrowed-scan claim is being made.
Reverse verification — direction and counts predicted before running
Both predictions are recorded in the test file's docblock (17 cases total) and both were measured exactly:
window.location.assign(nav){ description }argument from both success toastsMutation A's red set is the 2 cases (one per component) asserting a host navigate received an app-relative destination, plus the mounted-host placement case. Its green set is named deliberately rather than counted as credit: the 2 absent-seam cases and the 2 same-origin-absolute cases should survive — they describe behaviour that was already correct. Those are the negative control; without them an implementation that also replaced the no-provider fallback, or that laundered an absolute through the host router, would pass this file just as well. The 8 point-2 cases survive because the refusal note is independent of the navigation site.
Mutation B's green set includes the 2 "no key declared" cases, which assert the absence of a note — they make the distinction the defect is about measurable rather than detecting this mutation.
Both mutation legs ran under a
trap … EXIT INT TERMrestore, each mutation was proved on disk by grep counts of the injected and removed text before the suite was read, and the tree was confirmed byte-identical toHEADafterwards. Mutation B's first anchor missed (ANCHOR MISS: expected 1 occurrence, found 0) and the script refused to run rather than reporting a green no-op; the anchor was corrected and re-run.One property asserted by construction rather than by a case
Unmount-cancellation reaches this arm because it reuses the same hook, but with
delayMs: 0the window between arming and firing is a single macrotask. A test of it would be either a race or a test of the timer rather than of this arm's wiring, so it is not claimed here; it stays pinned against a declared delay insubmitRedirect.timerLifetime.test.tsx.Note on the local test invocation
pnpm --filter @object-ui/plugin-form testis refused by a repo guard (objectui#3378) because running vitest from a package directory silently runs@object-ui/console's 22 files and reports them as green. The suite above was therefore run from the repo root, which is what CI does.🤖 Generated with Claude Code
https://claude.ai/code/session_014zHsbJoTkTZeJQ5DLbRXrE
Generated by Claude Code