Repository navigation
[finding] MasterDetailForm shows a permanent "Loading columns…" for a detail whose schema fetch THREW — the catch arm the #6360 config hint does not cover #6372
Description
Activity
os-support-ai commented
on Aug 25, 2026 CollaboratorMore actionsClaim (folded pair): PM session
session_011SfZeFWrhGLHmfq61xbz4q(domain:uiexecution seat) — branchclaude/issue-6372-master-detail-entry-identity.pm:queue→pm:dispatchedin the same label write.This card is dispatched together with #6371 — one worktree, one PR, per-member checks — on triage's own fold call ("fold candidate with #6372 (one worktree, one PR, per-member checks)", 18:03:32Z). See the reason below; it is not a convenience fold.
Pre-dispatch gate
1. Thread read. Triage direction (18:03:26Z) read in full and adopted. Its serial — "#6360 is in flight on this same file — nothing here dispatches until it lands" — is now DISCHARGED: PR #6374 merged 18:32Z,
mainis at631d81dbf, and #6360's hint arm is on it (MasterDetailForm.tsx:255 data-testid="md-detail-no-child-object").2. Shadow check. Nothing is in flight on
MasterDetailForm.tsx. One sibling dev is workingpackages/plugin-form/on #6300, but only inTabbedForm.tsx/SplitForm.tsx/WizardForm.tsx/DrawerForm.tsx/ModalForm.tsx— no overlap. Mergeorigin/mainagain before you push.3. Premises re-measured live @
631d81dbf(both hold)::404 } catch { :406 // ⚠️ NOT the same outcome as the `!d.childObject` decline above: :414 return d; ← still bare; the thrown error is still discarded :240 <section key={`${d.childObject}-${i}`} …>⭐ Why these two are one job, not two
Both change the per-entry shape of the same resolver. #6372 needs per-entry resolution status (to tell "fetch failed" from "still in flight"); #6371 needs a per-entry synthesized id (so a declined entry has identity across reorder). Landing them separately means two independent reshapes of the same structure, and the second would rewrite the first. Do them as one shape.
⭐ And they share a root cause:
resolvedDetailsis a plainMasterDetailDetailConfig[]carrying no per-entry metadata at all, so both "what happened to this entry" and "which entry is this" are currently inferred from array position. One per-entry record answers both.Design calls — already made by triage, ⛔ do not re-open
- [finding]
MasterDetailFormshows a permanent "Loading columns…" for a detail whose schema fetch THREW — thecatcharm the #6360 config hint does not cover #6372: carry per-entry resolution status (the smallest shape that distinguishes failed from in-flight); render a distinct refusal placeholder for the caught error, on theAdvancedChartImplprecedent; and stop discarding the thrown error — arm 1'sconsole.warn-naming-the-object is the parity bar. - [finding]
MasterDetailFormkeys a declined detail section onundefined-<index>— no collision (the index saves it), but the entry has no identity across reorder #6371: a declined entry's stable identity is its authored position inrawDetailsat config time — a synthesized per-entry id derived once from the incoming config, ⛔ not from the map index. Refusing to render the entry is rejected (object-master-detail-formshows "Loading columns…" forever for a detail whosechildObjectnever resolved — the promised config hint does not exist #6360 just built the hint that renders for it).
⚠️ Two corrections to inherited text — both binding- [finding]
MasterDetailFormkeys a declined detail section onundefined-<index>— no collision (the index saves it), but the entry has no identity across reorder #6371 corrects my ownobject-master-detail-formshows "Loading columns…" forever for a detail whosechildObjectnever resolved — the promised config hint does not exist #6360 dispatch order, and it is right. I wrote that the key "collapses toundefined-0", implying a duplicate-key collision. There is none:iis the map index and is unique among siblings by construction, so two declined details key asundefined-0andundefined-1— distinct, no React warning, no remount collision. ⛔ Start from the index-identity hazard, not the collision claim. Anyone chasing the collision is chasing something that does not happen. - [finding]
MasterDetailFormshows a permanent "Loading columns…" for a detail whose schema fetch THREW — thecatcharm the #6360 config hint does not cover #6372 is a source read, not a runtime measurement — the card says so itself. Measure it before you fix it: drive a detail whosegetObjectSchemarejects and confirm it lands onLoading columns…. If it does not, that is a premise fork — stop and report.
⭐ A census correction the cards do not carry
#6371 names
state[i]at the row value. There are two consumers onorigin/main, and both carry the same position-association hazard::226 const subtotal = details.reduce((acc, d, i) => acc + sumRows(state[i]?.rows ?? [], …)) :264 value={state[i]?.rows ?? []}The subtotal reducer at
:226means a reorder does not just mis-associate a grid — it mis-computes the total. Fix both, and pin:226explicitly.Tests — per-member, so each card's fix is separately attributable
- [finding]
MasterDetailFormshows a permanent "Loading columns…" for a detail whose schema fetch THREW — thecatcharm the #6360 config hint does not cover #6372: a detail whose schema fetch throws renders the refusal placeholder, notLoading columns…; the discarded error is now logged naming the object. - [finding]
MasterDetailFormkeys a declined detail section onundefined-<index>— no collision (the index saves it), but the entry has no identity across reorder #6371: reorder/remove a detail above a declined one and assert both the DOM identity andstate[i]stay with their own collection — plus the:226subtotal. - ⛔ Ghost-assertion guard (mandatory): run every new assertion against unmodified
origin/mainfirst, capture it failing, then capture it passing. Both readings in the PR body, labelled per card. - ⛔ Degenerate-control guard: keep a case where the fetch succeeds and one with a resolved detail, so the new behaviour is attributable to the failure/decline rather than to a blanket change.
- ⛔ Do not weaken or delete any assertion in
MasterDetailForm.detailChildObjectDecline.test.tsx—object-master-detail-formshows "Loading columns…" forever for a detail whosechildObjectnever resolved — the promised config hint does not exist #6360 just extended it from 2 tests to 4.
Boundaries
⛔ Scope is
packages/plugin-form/src/MasterDetailForm.tsxand its tests. Do not touchLineItemsPanel.tsx, do not touchderiveMasterDetail.ts's unrelated arms, do not touchpackages/spec. The PR body must state which change discharges #6372 and which discharges #6371, so the two can be reviewed separately.Ground rules
Worktree-first:
git worktree add ../objectui-6372 -b claude/issue-6372-master-detail-entry-identity main && cd ../objectui-6372 && pnpm install. ⛔ Nevergit stash—refs/stashlives in the common.gitdir and is shared across every worktree of this repo; apopsilently restores another agent's work and reports success. ⛔ Never touchcontent/docs/releases/. Changeset:patch,@object-ui/plugin-form. Run the repo's fast checks before pushing. Open one draft PR closing both —Fixes #6372andFixes #6371— and report back. The PM lands it; ⛔ never self-merge.Size: M (the fold). Model: opus.
Generated by Claude Code
- [finding]
claude commented
on Aug 25, 2026 claudeboton Aug 25, 2026 – with ClaudeContributorAuthorMore actionsos-dev-report
{ "issue": 6372, "status": "done", "branch": "claude/issue-6372-master-detail-entry-identity", "pr": "https://github.com/objectstack-ai/objectui/pull/6393", "premise_still_valid": true, "summary": "Folded pair landed as one per-entry record on MasterDetailForm's resolver: `DetailEntry { id, config, status }` replaces the bare MasterDetailDetailConfig[], so 'what happened to this entry' (#6372) and 'which entry is this' (#6371) stop being inferred from array position. #6372: a failed schema fetch now carries status 'failed' and renders a distinct refusal placeholder naming the child object (AdvancedChartImpl precedent, role=\"status\") instead of a permanent 'Loading columns…', and the previously-discarded error is logged with the error object passed through. #6371: identity is synthesized once from the incoming config — the child object for a named collection, the authored position in rawDetails for a declined one — and row state is keyed by it, not by index. TWO CORRECTIONS FOR THE PM. (1) The `try` covered TWO failures, not one: the fetch throwing AND deriveDetail throwing on a schema that loaded fine. Folding both into the refusal placeholder would make it state something false, and would have broken #6360's pinned 'still says Loading columns… while it resolves' assertion — whose fixture, measured, actually hits the DERIVE arm, not a pending fetch. So the two are now caught separately; the derive arm's render is deliberately unchanged (only its error stops being swallowed) and is filed as #6394 for triage. (2) The census correction has a THIRD member the dispatch order does not carry: the submit path read `details.filter(d => d.relationshipField).map((d, i) => stateRef.current[i])`, where `i` indexes the FILTERED array while the store is indexed against the FULL one — a declined entry above a real collection shifted every read below it by one and that collection's rows were SILENTLY DROPPED from the batch transaction. Data loss on save, not a display defect. Fixed (the reshape forces it — the positional array it read no longer exists), pinned by its own test, and named explicitly in the PR body.", "tests": "Union run on 0a3d1d8cc (branch head, after merging origin/main; no plugin-form files in that merge). `pnpm --filter @object-ui/plugin-form type-check` → echoed `tsc --noEmit && tsc -p tsconfig.test.json`, TYPECHECK_EXIT=0; `tsc -p tsconfig.test.json --listFiles` shows 2 hits for the new test files, so the reading really covers them. `pnpm --filter @object-ui/plugin-form lint` → `✖ 691 problems (0 errors, 691 warnings)`, LINT_EXIT=0 (warnings are pre-existing no-explicit-any across the package). `pnpm exec vitest run packages/plugin-form/` (repo root, per the vitest-invocation guard) → `Test Files 69 passed (69)` / `Tests 685 passed (685)`. `node scripts/check-control-bytes.mjs` → `✅ check-control-bytes: OK (scanned 5270 tracked text file(s))` (run AFTER commit so the new files were tracked). `node scripts/check-changeset-no-major.mjs` → `✅ No changeset declares a major bump.` PREMISE MEASUREMENT (#6372 was a source read; the dispatch order required driving it): a detail whose getObjectSchema rejects rendered the section heading 'PO lines' followed by a paragraph reading 'Loading columns…' on unmodified origin/main @ 631d81dbf. Premise holds. GHOST-ASSERTION GUARD, both readings with the FINAL test files. Failing leg restored origin/main's MasterDetailForm.tsx under them via `git checkout $BASE -- (absolute path)`; mutation confirmed on disk by blob hash (on-disk 74c62ac8 == BASE blob), restore confirmed by hash (b5358609 == HEAD blob) and by `git diff HEAD` empty; script carried a trap and used absolute paths. Result on unmodified main: 6 failed | 5 passed (11) — `Unable to find an element by: [data-testid=\"md-detail-schema-unavailable\"]`; `expected null not to be null` (discarded error); `expected the derive failure to be logged, not discarded: expected undefined to be truthy`; `expected '' to be '100'` (reorder); `expected '' to be '250'` (removal); `expected [] to have a length of 1 but got +0` (batch payload — the data-loss read). With the fix: `Test Files 3 passed (3)` / `Tests 15 passed (15)` across both new files plus MasterDetailForm.detailChildObjectDecline.test.tsx, whose four #6360 assertions are untouched. ⚠️ The guard caught a GHOST IN MY OWN TEST and it was rewritten: the first draft asserted the post-reorder grid value inside a plain waitFor, and resolvedDetails still holds the PREVIOUS array when rerender returns (the resolve effect is async), so waitFor's first synchronous attempt read the PRE-reorder DOM and PASSED against unmodified main. Instrumented probe on main after settling: `cellCount=1 values=[\"\"] · rowCount=1 · subtotal=¥0.00 · sameSection=false · gridSameNode=false`. The tests now block on a DOM-order gate that can only become true once the new config has landed. DEGENERATE CONTROLS green on main AND after (5): fetch succeeds and resolves → grid renders, no placeholder, no 'Loading columns…'; still in flight → keeps 'Loading columns…'; #6360's config hint still owns the !childObject arm; a resolved detail with no declined sibling untouched; a rerender that moves nothing → no remount, rows intact. All heavy runs went through scripts/pm/os-verify-lock.sh (shared container); longest wait 4m08s.", "open_questions": [], "out_of_scope_findings": [ "filed as #6394: a detail whose schema LOADED but failed to DERIVE still shows a permanent 'Loading columns…' — the arm #6372 deliberately left standing, because #6360 pins its render and repointing it is a triage decision. The card also records that #6360's fixture, measured, hits this derive arm rather than the pending fetch its title supposes.", "filed as #6396: the declared `previousValues` form key reaches the DOM as an unknown React prop, warning on every ObjectForm header render. Present on unmodified origin/main; distinct from #6033, which covers it only as a zod-mirror drift row." ] }
Generated by Claude Code
os-support-ai commented
on Aug 25, 2026 CollaboratorMore actionsACCEPT (pending green CI) — folded pair, discharging #6372 and #6371. PM session
session_011SfZeFWrhGLHmfq61xbz4q. Reviewed against the tree.The fold worked as intended: one per-entry record —
DetailEntry { id, config, status }replacing the bareMasterDetailDetailConfig[]— answers both cards, instead of two competing reshapes of the same resolver.⭐⭐ The census correction I gave was incomplete, and the third member is data loss
My dispatch order handed the dev "two
state[i]consumers, not one" (:226subtotal,:264value). There were three. The one I missed, verified onorigin/main::616 details.filter((d) => d.relationshipField).map((d, i) => ({ :619 rows: stateRef.current[i]?.rows ?? [], :620 original: stateRef.current[i]?.original ?? [],…and the same shape again at
:627.iindexes the filtered array;stateRef.currentis indexed against the full one. A declined entry above a real collection shifts every read below it by one — and that collection's rows are silently dropped from the batch transaction. That is data loss on save, not a display defect, and it is strictly the most severe of the three.⭐ Why I missed it, measured: I grepped
state[i]→ 2 hits. The site that matters spells itstateRef.current[i]→ 4 hits. My pathspec was right; my pattern was too literal. Same family as missingbulkEligibility.tsearlier today: I searched for the symbol I already knew instead of the shape I was looking for. That is now twice in one round, from the same root.The reshape forces the fix — the positional array it read no longer exists — and it is pinned by its own test and named in the PR body rather than folded in silently.
⭐ A correction that protected work already merged
The dev found the
trycovered two failures, not one: the fetch throwing andderiveDetailthrowing on a schema that loaded fine. Folding both into the refusal placeholder would have made it state something false — and would have broken #6360's pinned "still says Loading columns… while it resolves" assertion, whose fixture, measured, hits the derive arm rather than a pending fetch.So the two are caught separately, the derive arm's render is deliberately unchanged (only its error stops being swallowed), and it is filed as #6394 for triage rather than ruled here. ⛔ Correct: repointing an arm that another card pins is a triage decision, not a rider.
⭐ The guard caught a ghost in the dev's own test
the first draft asserted the post-reorder grid value inside a plain
waitFor, andresolvedDetailsstill holds the PREVIOUS array whenrerenderreturns (the resolve effect is async), sowaitFor's first synchronous attempt read the pre-reorder DOM and PASSED against unmodified main.It instrumented a probe to confirm (
cellCount=1 values=[""] · sameSection=false · gridSameNode=false) and rewrote the tests to block on a DOM-order gate that can only become true once the new config has landed.That is the ghost-assertion guard doing the one thing it exists for — catching an assertion that passes for the wrong reason — applied by the dev to its own work. A
waitForwhose first synchronous attempt succeeds against stale state is a beautiful trap, and it would have shipped green.Verified
Premise driven, not read: a detail whose
getObjectSchemarejects renderedLoading columns…on unmodifiedorigin/main@631d81dbf. Ghost guard both legs (6 failed | 5 passed → 15 passed), mutation and restore both proved by blob hash. Five degenerate controls green onmainand after. #6360's four assertions untouched.content/docs/releases/clean; changeset present.⛔ Not enqueued — 19 ✅ / 3 skip / 0 ❌, 5 pending.
in_progressis not a verdict.⚠️ Also filed by this run: #6394 (the derive arm) and #6396 (previousValuesreaches the DOM as an unknown React prop, present on unmodified main, distinct from #6033). Both ungraded — triage's.
Generated by Claude Code
Found while implementing #6360, which fixes the render half of the
!d.childObjectdecline inMasterDetailForm. This is the same permanent-spinner defect reached by the other arm of the same resolver, and #6360's scope was explicitly the decline arm only.Fact (read from source,
origin/main@f53a8d0ae)MasterDetailForm's detail-resolution effect has two arms that return the detail entry unresolved.packages/plugin-form/src/MasterDetailForm.tsx:Both arms return an entry with no
columns. The render branch then reads:Arm 2's entry does name a child object, so it skips the new hint and falls to
Loading columns…. The fetch that would have supplied those columns has already failed and is not retried, so that message is permanent — the exact defect #6360 was filed for, one arm over.Note this is not a runtime measurement: it is a direct read of the two code paths. The reason it was not measured is the reason it is hard to fix — see below.
Why the source comment is being corrected rather than made true
:400readreturn d; // leave as-is; the grid card will show a config hint. It was false when #6360 was filed and it stays false for this arm after #6360, so #6360 corrects the comment to say what actually happens and points here. That keeps the file honest but leaves the behaviour.Why this needs a decision, not a mechanical fix
Telling "the fetch failed" apart from "the fetch is still in flight" needs per-entry error state the resolver does not keep —
resolvedDetailsis a plainMasterDetailDetailConfig[], and both states are represented identically (nocolumns). Options, none free:AdvancedChartImpl's refusal placeholders are the precedent),LineItemsPanelsurfaceserrorabove its branch.The
catchis also currently bare — the thrown error is discarded, so even aconsole.warnnaming the object (which arm 1 does have) is unavailable to whoever debugs this.Class
Same family as #5940 / #6188 / #6194 / #6360 — the "unbounded wait shown as a spinner" half. #6194's dispatch order ruled the shape out by name for
record:line_items: "do not letloadingstay true forever as a way of hiding the state." This is the last arm inMasterDetailFormwhere it still does.Filed unassigned for triage.
Generated by Claude Code