Uh oh!
There was an error while loading. Please reload this page.
fix(console): /forms/:name honours ?recordId= — load the record, prefill it, PATCH it (#4278) - #4293
Merged
Merged
Conversation
…4278) `ActionRunner.executeForm` forwards the record an action was fired from as `/forms/:name?recordId=<id>`, but the internal form route never read that param. It consumed only the `prefill_` ones, so the route rendered EMPTY inputs and its submit was an unconditional `POST /api/v1/data/:object` — an insert. An "edit this record" action therefore opened a blank form and, on Submit, created a second record while leaving the original untouched. In the showcase: open a Task, click Log Time, submit, and a NEW Task appears. `?recordId=` now selects the whole read/write pair: `GET /data/:object/:id` to prefill, `PATCH /data/:object/:id` to save, landing back on the record it updated. The verb is measured, not chosen — `PATCH /:object/:id -> updateData` is the data plugin's declared route, it is what packages/rest registers, and openapi-builtin-paths.ts records that the server "answers PATCH and 405s the PUT". Every other update client in this workspace spells the same pair. A recordId the route cannot honour fails closed (ruling point 2): 403/404, a payload whose object contradicts the form's target, and a present-but-blank `?recordId=` each render the error state. None degrades to create mode — a blank form whose submit inserts a duplicate is this bug's exact harm, so silently falling back into it would re-arm the defect. A recordId belonging to another object is not found under the form's own object and takes the same refusal path. Prefill precedence is per FIELD: explicit `prefill_` params win for the fields they name, the stored record fills the rest, and a stored null or empty string beats a create-time `defaultValue` so opening an edit form never silently proposes a change the user did not make. The destination after an edit is the id from the URL — the row we PATCHed — not one read back off the response. `UpdateDataResponse` was measured and declares the same `{ object, id, record }` triple as create, so `readCreatedRecordId` would fit; it is deliberately not pointed at one, because the edit path already holds that fact and a second reader could only ever disagree by way of a server bug this renderer would then follow. Create mode and the public `/f/:slug` path are untouched, both pinned as controls: an anonymous visitor controls the URL, so honouring `recordId` there would turn a public form into an arbitrary-record reader and writer. In app-shell only the URL-param registry's prose changed, recording that `recordId` has a second reader on a route that can never match the same URL as the record drawer's (ruling point 3 — measured, not assumed; no new constant, since the name is already reserved). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017Qqyix2QcnpUC9XeYVDzx3
The latest updates on your projects. Learn more about Vercel for GitHub. |
Contributor
✅ Console Performance Budget
📦 Bundle Size Report
Size Limits
|
yinlianghui
marked this pull request as ready for review
August 11, 2026 11:49
Uh oh!
There was an error while loading. Please reload this page.
This was referenced Aug 11, 2026
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#4278
Builds directly on #4109 / PR #4279 (
resolveSubmitBehavior,readCreatedRecordId,InternalFormRoute), whose squash90e792e11is this branch's base.The defect
ActionRunner.executeFormforwards the record an action was fired from as/forms/:name?recordId=..., but the internal form route never read that param — it consumed only theprefill_ones. So the route rendered empty inputs, andsubmitInternalwas an unconditionalPOST /api/v1/data/:object, i.e. an INSERT. An "edit this record" action opened a blank form and, on Submit, created a second record while leaving the original untouched. In the showcase app: open any Task, click Log Time, fill it in, Submit — a new Task appears.What this does
?recordId=now selects the whole read/write pair, per PM ruling point 1 (implement the read side on this route; do not reroute to the app-shell record-form surface, do not touchActionRunner):GET /api/v1/data/:object/:idto load, inputs prefilled with the stored values;PATCH /api/v1/data/:object/:idto save;The update convention was measured, not chosen
PATCH /:object/:id→updateDatais the data plugin's declared route (@objectstack/spec,api/plugin-rest-api.zod.ts), it is whatpackages/rest's server registers, andpackages/rest/src/openapi-builtin-paths.tsrecords that the server "answersPATCHand 405s thePUT" — a published document that saidPUTwas itself filed as a defect (objectstack#5588). Every other update client in this workspace spells the same pair:ApiDataSource.update(packages/core), the console's own API-discovery and Integrations pages, and app-shell'sObjectApiPanel. The body is the bare field patch, matching create.The read side is
GetDataResponse = { object, id, record }, also spec-declared. The{ success, data }transport envelope is absorbed by one extracted helper that both the create and the read path share, mirroring@objectstack/client.unwrapResponserather than spelling a second rule.Fail closed on a bad recordId (ruling point 2)
A 403/404, a payload whose
objectcontradicts the form's target, and a present-but-blank?recordId=each render the form's error state. None degrades to create mode: a blank form whose submit inserts a duplicate is this bug's exact harm, so silently falling back into it would re-arm the defect. ArecordIdbelonging to a different object is not found under the form's own object, so it 404s onto the same refusal path.Honest reachability note on the object-mismatch guard: the deployed server echoes the path object (
getDatareturnsobject: request.object) and we build that path from the view's target, so a livepackages/restdeployment cannot produce a mismatching payload. That guard is a contract assertion at the consumer, pinned with a hand-built body — not a live defect being patched. The reachable half of "wrong object" is the 404, and it is pinned as such.Prefill precedence, pinned
Explicit
prefill_params win for the fields they name; the stored record fills the rest; a field's create-timedefaultValueis lowest. Precedence is per field, never wholesale — a producer forwarding both?recordId=and?prefill_x=is expressing intent ("edit THIS record, with THIS field pre-changed"), and the narrower instruction is the more specific one. A storednullor empty string counts as a real value and beats a default, so opening an edit form never silently proposes a change the user did not make.Where an edit lands
The destination is the id from the URL — the row we just
PATCHed — not one read back off the response.UpdateDataResponsewas measured and declares the identical{ object, id, record, droppedFields? }triple as create, soreadCreatedRecordIdwould fit byte for byte; it is deliberately not pointed at one, because the edit path already holds that fact and a second reader could only ever disagree by way of a server bug this renderer would then follow. The create path has no other source, which is exactly why it must read the response. Pinned with an update response naming a different id.recordId vs the reserved drawer param (ruling point 3)
Measured rather than assumed.
recordIdis already reserved inpackages/app-shell/src/urlParams.tsasRECORD_DRAWER_PARAM, so no new registration was warranted — the name is registered, and adding a second constant for the same string would be two spellings of one contract. What was missing is that the registry described only one reader.The two readers can never see one URL: React Router renders exactly one leaf per location;
ObjectView(the onlyRECORD_DRAWER_PARAMcall site outside the registry) is mounted under/apps/*;/forms/:nameis a top-level route renderingDefaultHomeLayout+ a form and no list. Disjoint subtrees. And the meaning is the same either way — "the record this surface is about". So the app-shell change here is prose only: the registry table now names both readers and the disjointness. The console spells the literal (app-shell exports only its package root, which does not re-export./urlParams— the same unreachabilitycreatedRecordPath.tsdocuments), and a test pins the two spellings equal so they cannot drift in silence.Tests, and the reverse verification
New:
apps/console/src/components/FormPage.recordId.test.tsx(13 rendered cases) plus unit coverage for the new pure helpers inFormPage.test.ts.With
FormPage.tsxreverted to its post-#4279 state, the predicted directions were written down first and then measured — 11 red, 2 green, exactly as predicted:Received:blank, the defect verbatim)/forms/...?recordId=⇒ refusesprefill_overrides the record per field (x2)/f/:slugignores?recordId=The two controls are green on purpose and are not change-detectors. The create-path control is the whole point of the card — this must not move create mode a byte. The public-path control guards a direction nothing in this change can turn red today:
/f/:slugis anonymous and visitor-controlled, so ifrecordIdever leaked onto it that would be a data-exposure bug (an arbitrary-record reader and writer), which is worth a standing pin rather than a comment.Verification
pnpm exec vitest run apps/console/— 38 files, 421 tests passedpnpm exec vitest run packages/app-shell/— 344 files, 3286 passed (1 pre-existing skip)type-checkon@object-ui/console+@object-ui/app-shell(bothtsccommands each) — clean, after building the dependency closure...@object-ui/app-shell(i.e. the packages that consume it): 4 projects, all greeneslinton the changed files: 0 errors. Warning delta vs. the pre-fix baseline is +2react-refresh/only-export-components, from the newly exported pure helpers — the same pattern this file already uses for every one of its exported helperscheck-changeset-presence.mjsandcheck-changeset-no-major.mjsboth green (patch, both packages)Out of scope
Filed #4292 while measuring this:
?recordId=carries no object name, so the id is resolved against the FormView's target object. When an action's target view edits a different object than the record it was fired from and ids collide across objects (per-table integer keys), the route silently reads and now writes the wrong record. The producer half lives inpackages/core(ActionRunner), which this card's ruling put out of scope and which #4046 holds. Pre-#4278 the worst case was a duplicate insert; now it is a targeted update, which is why it is worth naming.Generated by Claude Code