fix(app-shell): make MetadataService.saveObject's fields argument required - #6643
Merged
Merged
Conversation
…uired `ObjectSchema.fields` is REQUIRED, and `metadata-protocol`'s `saveMetaItem` parses the whole item against that schema and throws before it persists. So a `saveObject` call that omitted the field list built a body the server was guaranteed to refuse `422 INVALID_METADATA` — every time it ran. The method cannot write a valid document without it, so the argument is now required: the guaranteed runtime failure moves to compile time and runtime accept/reject is unchanged. Nothing new is exported. The objectui#6240 anti-wipe control moves with the signature rather than being deleted as newly-unreachable — its runtime half still guards the path a JavaScript consumer can reach, and its new `@ts-expect-error` is the compile-time half, which reds if the argument ever goes back to optional. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CRJge11jso9TpXRWFt1Z49
Contributor
✅ Console Performance Budget
The eager closure is every chunk the entry reaches through static imports — what the browser fetches and parses before the app renders. The entry chunk on its own is a small fraction of it. 📦 Bundle Size Report
Size Limits
|
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 free
to 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 #6490
Implements the maintainer ruling of 2026-08-27 (verbatim 「其他接受」 — Option 1):
MetadataService.saveObject's field-list argument becomes required. Verified at commit2503a710.What changed
One production character.
existingFields?: FieldMetadataPayload[]becomesexistingFields: FieldMetadataPayload[]. Everything else in the diff is documentation, the moved pin, and call sites.Runtime behaviour is unchanged, and that is the whole justification.
ObjectSchema.fieldsis not merely typed, it is REQUIRED, andmetadata-protocol'ssaveMetaItemparses the whole item against that schema and throws before it persists. So a call that omitted the field list built a body the server was guaranteed to refuse422 INVALID_METADATA— every time it ran, against every backend. The method cannot write a valid document without the argument. Requiring it moves that guaranteed runtime failure to compile time; the only calls it breaks are calls that already failed.That measurement is not quoted from the card — it is executed in this branch. The carried-forward pin asserts
issuesOf(ObjectSchema.safeParse(body))equals['invalid_type @ fields']on the omitted-argument body, against the installed@objectstack/spec17.2.0, and it passes.The #6240 positive control moved with the signature — it is not deleted
The pin reads "omits
fieldsentirely when the caller supplied none — it does NOT write{}". Both halves of it are still live, and they measure different things:MetadataServiceis public API through the exporteduseMetadataService, so a JavaScript consumer — or a TypeScript one that casts past the types — can still arrive with no field list. What it must get is the loud, harmless 422, never the silent wipe.toObjectPayload's parameter deliberately stays optional for exactly this: it is the last line of defence and may not be deleted as newly-unreachable code.@ts-expect-error, andtsconfig.test.jsoncompiles this file, so an unused directive is itself an error. The line reds if the argument ever goes back to optional — the only direction this narrowing can regress in.Reverse-verified, predicted direction stated before the run: reverting the signature to optional must turn the type-check red on an unused directive. Observed exactly that —
error TS2578: Unused '@ts-expect-error' directive.atMetadataService.objectPayloadFieldsMap.test.ts(236,5),VERDICT command-exit 2. Mutation confirmed on disk by counting both spellings (required 1→0, optional 0→1) and by agit hash-objectdiffering from the HEAD blob; restore confirmed by the hash returning to the HEAD blob andgit diff HEADbeing empty. No rebuild was needed for either leg:MetadataService.tsis compiled from source by bothtscprojects, andtsconfig.test.json's emptypathsonly redirects cross-package specifiers to built.d.ts.Declined options, recorded so neither is taken later as a shortcut
{}default.{}parses GREEN and the PUT is an upsert, so defaulting would delete every field of the object on a save that only meant to rename it — trading a loud, harmless 422 for silent data loss.One cell newly pinned, and why it is in scope
An EMPTY list is a different statement from a missing one:
[]means "this object has no fields", writes{}, and under the upsert performs the wipe the caller asked for — the same authoritative readingsaveFieldsalready gives its own empty list. Behaviour is unchanged ([]is truthy, so it has always converted to{}); it is pinned now because the required parameter is what routes a caller with nothing to hand toward that spelling. The two readings of "no fields" must stay distinguishable, so the pin sits directly beside the anti-wipe control.Call sites
In-repo production callers measured at zero; only tests call it. Four omitted-argument call sites, all in
MetadataService.saveAdvisories.test.ts, now pass the smallest well-formed list — those suites are about the advisory channel, not the field list, and nothing else about their assertions changes.Confirmed by anchored grep with a positive control in the same query shape, repo-wide over ts/tsx/js/jsx excluding
node_modulesanddist:\.saveObject\(\s*[^,()]*\s*\)— 1 hit, and it is the deliberate@ts-expect-errorline 238 above;\.saveObject\(\s*[^,()]*\s*,— 16 hits, so the query shape is live and a zero in the first row would have meant something;Clause-② — still "no"
No new export was needed. Zero added
exportstatements in the diff (git diff | grep '^+' | grep exportis empty). The change narrows the published surface rather than widening it: nothing new is exported, nothing new is accepted, and runtime accept/reject is unchanged. The stop-and-report condition was not reached.Verification, all at commit
2503a710pnpm --filter @object-ui/app-shell type-check(tsc --noEmit && tsc -p tsconfig.test.json)VERDICT command-exit 0pnpm --workspace-concurrency=2 --filter '...@object-ui/app-shell' type-check— the downstream consumer closure, 4 of 47 projects: app-shell,@object-ui/console, and the two console examplesVERDICT command-exit 0, each printingDonepnpm exec vitest run --maxWorkers=2 packages/app-shell/src/services/Test Files 10 passed (10)·Tests 102 passed (102)✓— "omitsfieldsentirely when the caller supplied none — it does NOT write{}" and "objectui#6490 · an EMPTY list is a STATEMENT, not a missing argument — it writes{}"pnpm changeset:check✅ All workspace packages are in the changeset fixed group.·✅ No changeset declares a 'major' bump.pnpm check:control-bytes✅ check-control-bytes: OK (scanned 5488 tracked text file(s); skipped 85 binary).pnpm check:designer-field-key-paritydesigner-field-key-parity: OK(ObjectMetadataPayload 6 declared— unchanged, no new keys)pnpm check:self-import·pnpm check:phantom-deps✅ No package names itself inside its own src/.·✅ Every in-scope import is declared by the package that publishes it.The type-check is the measurement that matters here, since the whole change is a compile-time contract. Note the first downstream run was red on
Cannot find module '@object-ui/plugin-*'— unbuilt sibling plugins, not this change; it went green after building console's own dependency closure.Lint was narrowed to the affected package, declared:
eslint .insidepackages/app-shell— byte-identical to whatturbo run lintinvokes for this package. Population and count read from eslint's own output, not guessed:--format jsonreports 1006 files linted, 0 errors, 2775 warnings (the pre-existing warning baseline), and the three edited files report 0 errors with one pre-existingno-explicit-anywarning atMetadataService.ts:640, outside both of my diff hunks. Invariance for untouched files is measured rather than assumed:eslint.config.jsdeclares noparserOptions.projectand noprojectService, so type-aware linting is off and every file's verdict is computed from its own source — this diff cannot move the verdict of a file it did not touch.Bump
Scored
minor, notmajor, and this is not a gate refusing an honest declaration. AGENTS.md §版本号策略 defines the honest declaration in this repo: objectui's major is pinned to the@objectstackmajor so that "same major ⇒ compatible" holds across the two repos, and all publishable packages share onefixedgroup, so onemajorwould carry 39 packages off that line. The written convention is that objectui's own breaking changes ship asminorwith the break spelled out in the changeset body — which this one does, in words, including the sentence that previously-omitted-argument calls were already a guaranteed 422.scripts/check-changeset-no-major.mjsmechanically enforces that policy; it was not consulted to pick the score.One observation, no action taken
The still-pending
.changeset/6240-object-payload-fields-map.mdends with "saveObject(obj, existingFields)keeps itsFieldMetadataPayload[]parameter type — the array is converted inside — so no caller's call site changes." That is true of #6240's own change and stays true; it will simply sit alongside this entry in the same release. Editing another card's changeset would be scope widening, so it is left alone and flagged here instead.Generated by Claude Code