Uh oh!
There was an error while loading. Please reload this page.
fix(fields): carry the spec's optional location keys across a coordinate edit - #6713
Merged
os-sales merged 1 commit intoAug 28, 2026
Merged
Conversation
…ate edit
`LocationField` edits `{ lat, lng }` as one comma-separated text box and rebuilt
its emission as a fresh object from the parsed text, so the two OPTIONAL keys
`@objectstack/spec` declares alongside them — `LocationValue` is
`{ lat, lng, altitude?, accuracy? }` — were dropped the moment a user retyped
the coordinates. Nothing warned. Both keys are registered on the platform's
authorable surface, so a customer may author them even though neither repo
produces them today.
The drop predates objectui#6272: before that flip the widget emitted
`{ latitude, longitude }` and discarded the rest identically. #6272 changed only
the declared value type, so the type claimed four keys while the write path
handled two.
The carry is a key-by-key pick of exactly those two keys out of an
already-valid `LocationValue` — deliberately not a spread of the incoming
value, which would carry a stored record's retired `latitude` / `longitude`
spelling back into the emission and undo #6272's rename. A negative control
pins that, and reads the emitted object's own keys rather than `safeParse`
because `LocationValueSchema` is non-strict and accepts a polluted object.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CRJge11jso9TpXRWFt1Z49Contributor
✅ 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
|
os-sales
marked this pull request as ready for review
August 28, 2026 18:31
Uh oh!
There was an error while loading. Please reload this page.
os-sales
deleted the
claude/issue-6664-location-field-preserve-altitude-accuracy
branch
August 28, 2026 18:52
This was referenced Aug 28, 2026
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#6664
LocationFieldedits a location's coordinate pair as one comma-separated text box, andrebuilt its emission as a fresh object from the parsed text:
@objectstack/specdeclares the stored shape as{ lat, lng, altitude?, accuracy? }, so astored
altitude/accuracywas gone the moment a user retyped the coordinates. Nothingwarned; the two keys simply were not in the object handed to
onChange.Why this is worth fixing even with no producer
Triage measured both questions the card left open: no producer writes either key — not
in objectui, not in objectstack. It queued the card anyway, on a third reading: both keys
are registered on the authorable surface
(
objectstack packages/spec/authorable-surface.base.json:3438,3442), so the platform hasalready promised customers they may author them. The population that decides this is "does
a customer write it", which neither repo can measure — hence a real defect at
p2ratherthan an implementation-first key.
This predates #6272
The drop is not a regression from #6272. Before that flip the widget emitted
{ latitude, longitude }and discarded the rest identically. What #6272 changed is onlythat the declared value type is now the spec's — so the type claimed four keys while the
write path handled two. Keeping this out of that card's atomic fence was correct: the A1
ruling covered a bare two-sided rename and nothing else. This PR closes the gap that flip
made visible.
The fix
A key-by-key pick of exactly the two spec-declared optional keys, out of a value that is
already a valid
LocationValue:No spread, and no
Object.assign— deliberately. A stored record may still hold theretired
latitude/longitudespelling, and spreading the incoming value would carrythat dialect straight back into the emitted object and undo #6272's rename. The two
...occurrences left in the file are the pre-existing
...propsrest parameter and thetoDomPropsDOM pass-through; the value itself is never spread.Each optional key is taken only when it is a usable number. Measured against the spec:
z.number()rejectsNaN,Infinityand a numeric string alike (invalid_typeat[altitude]), so carrying one of those forward would make this widget emit a value theplatform's own validator refuses. Leaving it behind narrows the emission — it emits
less than it was handed, never more — rather than widening what the widget tolerates.
The finite test was extracted into a named
isFiniteNumberpredicate so the coordinatesand the two optional keys are held to the same check instead of two copies that can drift.
Tests
packages/fields/src/__tests__/LocationField.optionalKeys.test.tsx— 12 pins.Red before, green after. On the merge-base (
5967be095), with the test file presentand the widget untouched, the five positive pins fail and the rest pass:
After the fix, all 12 pass, and so do the 12 existing #6272 pins in
LocationField.specShape.test.tsx(24/24 across the two files).Negative control — green both before and after, and that is the correct reading. Four
tests build an old-dialect input carrying
latitude/longitudeand assert the emittedobject does not contain them, plus the general form: no key outside the spec's own
declared set, read off
LOCATION_SCHEMA.shaperather than copied into a literal. Thesecannot be red today, because today's code emits a freshly built
{ lat, lng }thattrivially carries no dialect. They are not vacuous — they guard the future regression
the card's fence names: someone "simplifying" the key-by-key pick into
onChange({ ...value, lat, lng }). Under that rewrite the first of them goes red, becauseits stored value carries both spellings. No positive pin can provide that guard.
One of them pins why those assertions read the emitted object's keys and not
safeParse:LocationValueSchemais a plain, non-strictz.object, so it accepts aspread-polluted object and merely strips the unknown keys from its parsed output while the
value handed to
onChangekeeps them. The schema cannot be the fence's guard.Deliberately not in scope
LocationCellRendererandObjectMapstill readlat/lngonly, so neither displays thecarried keys. Triage measured that and ruled it a feature needing its own card; it is
not written here.
Changeset
'@object-ui/fields': patch. This restores data the declared value type already promised —no API added, no accepted input retired, and the anti-spread fence keeps the emitted key
set a subset of the spec's declared four. That matches how this repo scores a change to
what a surface writes (cf.
view-overlay-write-patch-only-5233);minorhere isreserved for retiring an accepted read (cf.
view-column-io-canonical-identity-5725).Per AGENTS.md a changeset in this repo never declares
major.Verification
All at
bcfdb556e, the final commit.pnpm exec vitest run packages/fields/ packages/plugin-detail/pnpm --filter @object-ui/fields type-checktsc --noEmit && tsc -p tsconfig.test.json)npx eslint packages/fieldsnode scripts/check-lint-coverage.mjsnode scripts/check-control-bytes.mjsnode scripts/check-spec-symbol-derivation.mjsnode scripts/check-changeset-presence.mjsnode scripts/check-changeset-no-major.mjs/-fixed/-overwritenode scripts/check-vi-mock-specifiers.mjsnode scripts/check-package-self-import.mjsnode scripts/check-readme-exports.mjsnode scripts/check-published-dist-tooling.mjspnpm exec vitest run scripts/Both edited files were confirmed present in the
tsconfig.test.jsonprogram viatsc --listFiles, so the green type-check really covers the new test file rather thanskipping it.
The repo-wide
pnpm lintwas narrowed topackages/fieldslocally. That narrowingexcludes nothing:
eslint.config.jsenables no type-aware linting (noprojectorprojectService) and none of the 17 local rules ineslint-rules/reads the filesystem,so a verdict is a pure function of the single file being linted and this three-file diff
cannot move the verdict on a file it did not touch — and
check-lint-coverage.mjsindependently reports 0 outstanding errors across all 46 packages.
Generated by Claude Code