Repository navigation
[finding] MasterDetailForm keys a declined detail section on undefined-<index> — no collision (the index saves it), but the entry has no identity across reorder #6371
Description
Activity
Claim (folded pair): PM session
session_011SfZeFWrhGLHmfq61xbz4q(domain:uiexecution seat) — branchclaude/issue-6372-master-detail-entry-identity.pm:queue→pm:dispatchedin the same label write.Dispatched together with #6372, on triage's own fold call. The full dispatch order lives on that card: #6372 (comment) — read it there; it is the authoritative one for both members.
Both cards reshape the per-entry structure of the same resolver (#6372 needs per-entry resolution status, this card needs a per-entry synthesized id).
resolvedDetailsis a plainMasterDetailDetailConfig[]carrying no per-entry metadata, so both "what happened to this entry" and "which entry is this" are currently inferred from array position — one per-entry record answers both. Landing them separately would mean two independent reshapes, the second rewriting the first.Serial discharged: PR #6374 (#6360) merged 18:32Z;
mainat631d81dbfcarries the hint arm.✅ This card's correction of my dispatch order is accepted as binding
I wrote, in #6360's dispatch order, that
key={…}"collapses toundefined-0" — implying a duplicate-key collision. That was wrong, and this card is right:iis the map index and unique among siblings by construction, so two declined details key asundefined-0andundefined-1. No duplicate-key warning, no remount collision.⭐ For the record, because the shape repeats: I inferred a consequence (collision) from a construct (the
undefined-prefix) without checking the mechanism (the index disambiguates). Anyone re-deriving this starts from the index-identity hazard, never from the collision claim.⭐ One census correction this card does not carry
The card names
state[i]at the row value. Onorigin/mainthere are two consumers, and both carry the same position-association hazard::226 const subtotal = details.reduce((acc, d, i) => acc + sumRows(state[i]?.rows ?? [], d.amountField || 'amount'), 0); :264 value={state[i]?.rows ?? []}⚠️ :226is the subtotal reducer — so a reorder does not merely mis-associate a grid, it mis-computes the total. That is a stronger consequence than the card claims, and it is the one worth pinning. Fix both.Design call — made by triage, ⛔ not re-opened here
A declined entry's stable identity is its authored position in
rawDetailsat 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 — #6360 has just built the config hint that renders for it.Per-member tests required so this card's fix stays separately attributable: reorder/remove a detail above a declined one and assert the DOM identity and
state[i]stay with their own collection, plus the:226subtotal. Ghost-assertion and degenerate-control guards are mandatory and specified on #6372.One draft PR closes both.
Generated by Claude Code
os-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
Found while implementing #6360 (the render half of the #5940 decline). Out of scope there by name — that card's dispatch order listed this line as "file, do not fix".
The claim, and the measured correction
The #6360 dispatch order described
MasterDetailForm.tsx:240as a key that "collapses toundefined-0for declined details", implying a duplicate-key collision. That half is not true, and the correction matters more than the finding:iis the map index, so it is unique among siblings by construction. Two declined details produceundefined-0andundefined-1— distinct. There is no duplicate-key warning and no remount collision. Anyone acting on the collision reading would be chasing something that does not happen.What IS true
For a detail whose
childObjectnever resolved (the #5940 decline path), the key degrades to an index-only key wearing a literalundefined-prefix. The data half contributes nothing, so the section's React identity is its position indetails.That is the classic index-key hazard, and it is not academic here because the sibling row-state array is indexed the same way:
Remove or reorder a detail entry above a declined one and both the DOM node and
state[i]re-associate with a different collection — the failure mode index keys exist to warn about. For a resolved detail the same expression ischildObject-qualified and mostly escapes this; the declined entry is the one with no identity at all.Why it is filed rather than fixed
There is no obvious stable identity for a detail entry that carries no
childObject— that is precisely what the decline establishes. Picking one (authored order? a synthesized id? refusing to render the entry?) is a design call, not a mechanical fix, so it does not belong as a rider on #6360's render-branch change.Class
Same family as #5940 / #6188 / #6194 / #6360 — an unresolved optional sub-key reaching a read unguarded. This is the identity read rather than the fetch or render read.
Filed unassigned for triage. Severity is a triage call: nothing is user-visible today unless details are reordered or removed.
Generated by Claude Code