Repository navigation
finding(app-shell): saveFields rebuilds every field entry from the designer model, so per-FIELD server keys are dropped on every field save #6488
Description
Activity
Dispatched by the
domain:uiseat. The hard serial triage set is now satisfied: #6480 landed (PR #6502, merged 07:29Z), sosaveFieldsis free.Premise re-verified on the merged ref before dispatching — it holds. #6502 changed the same function, and an upstream merge is the likeliest thing to absorb a downstream card, so this was measured rather than assumed. On today's
main,saveFieldsstill ends withfields: toFieldsMap(fields.map(toFieldPayload))— every field entry is still rebuilt wholesale from the designer model. #6502 wrapped the object-level spread instripReadDecorations; that is orthogonal to the field-level rebuild and absorbed nothing.Triage's requirement is checkable and you must meet it explicitly: "Each PR must land with the other's pins still green." #6480 landed
MetadataService.readDecorationStrip.test.tsbeside your edit site. Your change must leave it green, and your report should say so by name — the two edits sit on neighbouring expressions in one function, which is exactly where a careless fix quietly undoes its neighbour.Copy the form that already exists rather than inventing one.
MetadataFieldsPage.handleFieldsChangesolves this exact problem one writer over:nextFields[f.name] = fromDesignerField(f, prevFields[f.name]), wherefromDesignerFieldspreadscarryOver(prev). AndsaveFieldsalready fetches the same document for the object-level spread, so the previous per-field entries are in hand at the drop site — there is nothing extra to fetch. If you find yourself adding a request, stop and re-read.⚠️ The mirror hazard, and the thing most likely to turn this fix into a worse bug. You are adding carry-over, so ask the question in the opposite direction: can carry-over resurrect a value the author deliberately cleared? If a designer removes a field property, a naive{...prev, ...next}puts it back, and the user's deletion silently fails to persist. That is the same class of silent data loss this card is about, pointing the other way. Carry over only keys the designer does not model — which is what the sibling'scarryOver(prev)is for. Measure this, do not reason about it: clear a modelled property in the designer shape and assert it is absent from the PUT body.⛔ Do not fold with #6480. Triage checked all five gates and rejected the fold — opposite directions on neighbouring expressions, distinct pins, and this card's own text says not to. #6480 preserved keys the schema refuses; you are preserving keys that should survive.
Reverse verification, both directions. Predict RED and observe it: a server-sent per-field key the designer does not model (
expression,precision,scale,system,sortable, or a plugin-registered one) must be absent from the PUT body before your fix and present after. Then the mirror assertion above. Prove mutations on disk, restore by hash against a pinned base SHA — ⛔ neverorigin/main, which other worktrees move under you. State plainly whether any packagedist/sits between your mutation and your assertion.Worth knowing why this is urgent now: the loss is not new but it was unreachable — while
fieldswent out as an array the whole body was refused422before persistence, so nothing dropped ever reached storage. #6240 made the body parse. From that fix onward, the drop lands.Fence and hygiene.
- Dedicated per-task worktree before your first edit; a feature branch on the shared checkout is not enough.
- ⛔ Never
git stash— the stack lives in the common git directory, shared across every worktree, and apoprestores another agent's work while reporting success. - ⛔ Do not touch
content/docs/releases/. - Claim first: assign yourself, post a claim comment with your session ID and branch.
- Run tests narrowly — the shared validation lock is a cross-agent resource and a wide run has been measured holding it nearly eighteen minutes.
#6489 is queued behind you on this same seam. Report your exact file face so it can be released.
Generated by Claude Code
os-dev-report
{ "issue": 6488, "status": "done", "branch": "claude/issue-6488-carry-over-field-keys", "pr": "https://github.com/objectstack-ai/objectui/pull/6518", "premise_still_valid": true, "summary": "Premise re-checked on merged main (16aa04486, which is PR #6502 / objectui#6480): saveFields still ended with `fields: toFieldsMap(fields.map(toFieldPayload))`, so the field-level rebuild was intact and #6502's stripReadDecorations absorbed nothing. toFieldPayload now takes the previous SERVER entry and spreads carryOver(prev) under the modelled keys, copying MetadataFieldsPage.fromDesignerField rather than inventing a form; the previous entries come from the document saveFields already fetches, so no request was added (pinned: 1 GET, 1 PUT). Modelled keys are still written unconditionally, so a cleared property arrives as an explicit undefined that overrides the carried value and is dropped by JSON.stringify - the mirror hazard, measured on the request bytes rather than reasoned about. carryOver excludes the five keys FieldSchema refuses BY NAME (indexed, referenceTo, formula, isSystem, sortOrder): a stored document can still carry them and echoing one back is a 422 that blocks every later save with no UI path to clear it - without that strip the fix would have been a regression, which ablation leg B demonstrates. File face: packages/app-shell/src/services/MetadataService.ts + MetadataService.fieldKeyCarryOver.test.ts + .changeset/6488-carry-over-field-keys.md. NOTHING under packages/plugin-designer/ is touched, so objectui#6489 is free to proceed on MetadataFieldsPage.tsx. One deviation to flag: the standing dev contract says not to touch the assignee, while this card's dispatch and objectui hygiene both said to claim first - the dispatch is per-card and more recent, so the issue was self-assigned and this comment is the claim record.", "tests": "All runs on final commit 7e5f5ac50, working tree clean, every heavy run through scripts/pm/os-verify-lock.sh. (1) `pnpm exec vitest run --maxWorkers=2 packages/app-shell/src/services/` -> `Test Files 10 passed (10)`, `Tests 101 passed (101)`. TRIAGE'S REQUIREMENT, BY NAME: MetadataService.readDecorationStrip.test.ts (objectui#6480's pin) is GREEN, and so is MetadataService.objectPayloadFieldsMap.test.ts (objectui#6240's). (2) `pnpm --filter @object-ui/app-shell run type-check` EXIT=0; NOT the not-measured case - `tsc -p tsconfig.test.json --listFiles` shows 1 hit each for MetadataService.ts and MetadataService.fieldKeyCarryOver.test.ts out of 4450 files (the package's tsconfig.json excludes *.test.ts, so this was checked rather than assumed). (3) Gates: check:designer-field-key-parity -> `designer-field-key-parity: OK` with FieldMetadataPayload still `15 declared [wire] vs FieldSchema`, no index signature, ledger unmoved; check:control-bytes -> `check-control-bytes: OK (scanned 5382 tracked text file(s))`; check:self-import, check:esm-specifiers, check:vi-mock-specifiers all EXIT=0; check-changeset-presence -> `2 source file(s) of 1 released package(s) changed, and this change declares 1 changeset(s)`; `pnpm --filter @object-ui/app-shell run lint` EXIT=0, 0 errors, no warning from the new file. Exit codes captured before any pipe (redirect-then-capture), verdicts quoted from the gates' own lines. (4) REVERSE VERIFICATION, both directions, three legs, each restored against PINNED sha 844011795 (never origin/main) - restore proven by `git hash-object` == 958eda1174aa0d6dd49a4d561010e99888d06a80 with `git diff HEAD` empty, and every leg carried a `trap ... EXIT INT TERM` with absolute paths. Mutations proven ON DISK by counting the target text before/after, not by an editor exit code: leg A '...carryOver(prev),' before=1 after=0; leg B RETIRED filter before=1 after=0 with injected marker=1; leg C marker=1 plus conditional-merge close=1. NO PACKAGE dist/ SITS BETWEEN MUTATION AND ASSERTION - the subject is imported relatively (./MetadataService) and every workspace import in these suites is aliased to src/ by the root vitest.config.mts (@object-ui/data-objectstack -> packages/data-objectstack/src, @object-ui/types -> packages/types/src); the only prebuilt artifact in the loop is @objectstack/spec, the unmutated oracle, so no rebuild was required between legs and none was skipped. Predictions were written before running. Leg A (remove the carry-over): predicted survival RED / mirror GREEN, observed 6 RED / 21 GREEN - survival, retired-key and neighbour-carry cases red as predicted, plus one mirror case whose closing positive control (expression survived in the same body) is exactly what leg A removes; the clearing assertions themselves stayed green there, as `clears them one at a time` and `clears a relationship target` staying green confirms. Leg B (verbatim carry-over, strip removed): predicted only retired-key cases RED, observed exactly 3 RED / 24 GREEN. Leg C (the naive {...prev, ...defined(next)} merge - THE MIRROR): predicted mirror RED / survival GREEN, observed exactly the 3 mirror cases RED / 24 GREEN, which is what makes the clearing assertions non-vacuous. readDecorationStrip.test.ts stayed GREEN in ALL THREE legs - the independence of the two neighbouring edits as a measurement, not a claim. (5) Instrument measurements behind the fix, against installed @objectstack/spec 17.2.0: FieldSchema ACCEPTS expression, precision, scale, system, sortable (with a green baseline control) and REFUSES indexed/referenceTo/formula/isSystem/sortOrder with unrecognized_keys; it also refuses a plugin-registered key (x_plugin_thing), which is why the carry-over is NOT filtered through the client's schema. Upstream, decorateMetadataItem (objectstack packages/metadata-protocol/src/metadata-diagnostics.ts) attaches _diagnostics to the ITEM only, never to a nested field entry, so the object-level strip stays sufficient and no per-field decoration strip was added (pinned, so it reds if that ever changes). CI has NOT been waited on - draft PR opened, gate status is whatever CI reports next.", "open_questions": [], "out_of_scope_findings": [ "filed as #6519: `object-fields-io` (app-shell metadata-admin previews) strips only `indexed` on read while FieldSchema refuses five designer-written keys by name, so a draft carrying formula/referenceTo/isSystem/sortOrder round-trips verbatim back out to the same 422 objectui#4644 closed for `indexed` in that file - unassigned, no labels; it also carries the three-divergent-lists question, which spans MetadataFieldsPage.tsx and so is objectui#6489-adjacent" ] }
Generated by Claude Code
Found while implementing objectui#6240 (the object payload's
fieldsarray-to-map conversion). Filed, not fixed — a different level from the container shape that card is about, and a different level from objectui#6480.What it is
MetadataService.saveFieldspreserves unknown keys of the fetched OBJECT document by spreading it:That spread is object-level only. The
fieldsvalue is rebuilt entirely fromDesignerFieldDefinitions viatoFieldPayload, so every key the SERVER sent inside a field and the designer does not model is dropped:expression(a formula authored in metadata-admin),precision/scale,system,sortable, and anything a plugin registered.Why now
The loss is not new, but it was unreachable. While
fieldswent out as an ARRAY the whole body was refused422 INVALID_METADATA(invalid_type @ fields) before persistence, so nothingsaveFieldsdropped ever reached storage. objectui#6240 makes the body parse, so from that fix onward the drop lands.The correct form is already in the repo
The sibling writer solves exactly this.
MetadataFieldsPage.handleFieldsChangecarries the previous server entry into the new one:saveFieldsalready fetches the same document (client.meta.getItem('object', objectName)) for the object-level spread, so the per-field previous entries are in hand at the same moment — there is nothing extra to fetch.Not objectui#6480
objectui#6480 is the OBJECT-level spread: read decorations (
_diagnostics,_draft) that the served document carries andObjectSchemarefuses by name. This is the FIELD level and the opposite direction — keys that should survive and do not. The two edit neighbouring expressions and should not be folded together.Refs: objectui#6240 (the conversion that makes this reachable) · objectui#6480 (the object-level half)
Generated by Claude Code