Uh oh!
There was an error while loading. Please reload this page.
fix(app-shell): seed record_header recordIdParam from the URL record id during the page-record load window - #5182
Merged
Conversation
…ng the record load window The record header does not wait for the page record. `isLoading` — the only state the <SkeletonDetail> early return reads — is flipped false in a route-keyed microtask, while `pageRecord` arrives one `dataSource.findOne` round-trip later. For that whole window the real header renders with every `record_header` action live and clickable. Inside it, the `type:'api'` handler's `recordIdParam` seeding had no source at all: its chain was row -> literal `recordId` override -> `pageRecord`, and it stopped there, never consulting `pureRecordId` — the id the page is addressed by, read off the URL and present on the first render. Handed a `null` seed, `resolveRecordIdParamSeed` abstains by design (no value AND no error, because "no row context" is not that guard's business), so the caller injected nothing, raised nothing, and dispatched anyway. The request went out naming no record and the backend answered `missing_record_id`. Seed a minimal row from `pureRecordId` as a last resort, which is the same fallback the other three dispatch paths in this file already use. Row and override still outrank it, and the retarget guard is unchanged, so a child-object action is never handed the parent page's id. An action keyed on a non-default `recordIdField` gets a row without that key and takes the helper's named refusal — refusing beats under-specifying. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AMHbqfPiETJHA95r6WzZWS
…cordId fix Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AMHbqfPiETJHA95r6WzZWS
Contributor
✅ Console Performance Budget
📦 Bundle Size Report
Size Limits
|
os-steve
marked this pull request as ready for review
August 18, 2026 10:01
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#5176
Diagnosis (this was the deliverable, ahead of the patch)
The card's framing — "the header has two DOM variants, and only one of them injects" — does not survive contact with the source, but the reporter's measurement does. There is no second dispatch path and no second injection site. There is one path, and its seed value depends on when you click.
Why a window exists at all. The header is not gated on the record load.
isLoading— the only state the SkeletonDetail early return reads — is flipped false in a route-keyedqueueMicrotask(RecordDetailView.tsx, the effect keyed on[objectName, recordId]).pageRecordlands onedataSource.findOneround-trip later, in a separate effect with its ownpageRecordStatus. Between those two moments the real header renders, with everyrecord_headeraction live and clickable, andpageRecordstillnull. The window is not a rare hydration edge — it is the full duration of the record fetch, which is exactly why a user samples it about 1 click in 10.Why the request went out empty. In the
type:'api'handler therecordIdParamseeding chain was: stashed row -> literalrecordIdoverride ->pageRecord. It stopped there. It never consultedpureRecordId, the id the page is addressed by, which is read straight off the URL (recordIdOverride ?? params.recordId) and is therefore present on the very first render. Mid-load the chain resolves tonull, andresolveRecordIdParamSeeddeliberately abstains on a null row — returning neither a value nor an error, because "no row context at all" is documented as not that guard's business. So the call site injected nothing, raised nothing, and dispatched anyway. The body that went out was literally{}; the backend answeredmissing_record_id.The reporter's DOM correlation is real but is a co-symptom, not the cause. The same unresolved
pageRecordthat starves the seed also withholds the title and highlights chrome, so the header genuinely renders a different shape in exactly the failing window. Correlation 1:1, causation neither direction — one condition produces both.The paths disagreed, and this one was the odd one out. The three sibling dispatch paths in the same file all already fall back to
pureRecordId: the legacydataSource.updatedefault branch, the flow trigger, and the server-action bridge. Only the api path did not.Which limb of the triage ruling, and why
The ruling allowed either "both variants inject identically" or "disable the action while the record id is unavailable". The measurement picks the first limb, and rules the second one out on the facts: the record id is not unavailable. It is in the URL, synchronously, before the first paint. Only the record body is pending. Disabling every header action for the whole duration of every record fetch would be a real UX regression imposed to work around an id the platform already holds. So this makes the paths agree instead.
The second limb still does the work the first cannot cover, and is pinned as such: an action naming a non-default
recordIdFieldcannot be served by the URL (which carries the id and nothing else), so it now takes the helper's named refusal rather than emitting an under-specified request. Both halves of the acceptance floor are therefore satisfied, each where it actually applies.Change
One seeding site in
packages/app-shell/src/views/RecordDetailView.tsx. A minimal row synthesized frompureRecordIdbecomes the last resort in the chain:recordIdoverride still outrank it (pinned);recordIdFieldgets a row without that key and takes the named refusal (pinned).No
as anywas added or broadened — the two type annotations in the diff are the ones already on the lines they replace.Tests
New sibling
RecordDetailView.headerRecordIdLoadWindow.test.tsx. The 1-in-10 timing report is made deterministic by holding the window rather than sampling it:findOnereturns a promise the test releases on demand, and the harness captures the ActionProvider handlers built while the record is still in flight. That discriminator is the point of the new file — the existing pinRecordDetailView.recordIdParamSeed.test.tsxwaits for the capture whose context carries the loaded record, which by construction sits after the failure window, which is why it never caught this.Red on unpatched
main(3 of 7 failing,expected {} to deeply equal { planId: 'rec-plan-1' }— the empty body is the defect itself), green after. The first test in the file pins the premise the card rests on: the header really does render live actions while the record is loading.No existing test was weakened, skipped, or quarantined; the 6 pre-existing pins in the sibling file pass unchanged.
Verified at
f08cfa8ec:vitest run packages/app-shell/src/views/— 272 files, 2680 passed, 1 skippedvitest runon the new file +RecordDetailView.recordIdParamSeed.test.tsx+packages/core/src/actions/__tests__/recordIdParam.test.ts— 3 files, 23 passedturbo run type-check --filter @object-ui/app-shell— 30 tasks successful (app-shell runstsc --noEmitplustsc -p tsconfig.test.json, so the new test compiles too)turbo run lint --filter @object-ui/app-shell— 0 errors (2412 pre-existingno-explicit-anywarnings, unchanged)check-control-bytes,check-lint-coverage,check-type-check-coverage,check-changeset-presence,check-changeset-no-major,check-changeset-fixed— all passcheck:action-forward-parity,check:phantom-deps,check:self-import,check:spec-symbols,check:i18n-keys,check:i18n-drift— all passChangeset:
@object-ui/app-shellpatch.Scope
Stayed inside the dispatched file surface.
DeclaredActionsBar.tsxwas not touched — the second dispatch path the dispatch allowed for does not live there, because there is no second path.One adjacent observation, deliberately left alone: if
pureRecordIdwere itself absent the chain would still end in the helper's silent abstention. That is unreachable on this route (the record route cannot resolve without it) and closing it would mean changing behaviour for a case I could not reproduce, so it is reported rather than patched.Generated by Claude Code