From bcfdb556eab9f44b7fc6273bf5f9669a4ebf2187 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 28 Aug 2026 18:09:59 +0000 Subject: [PATCH] fix(fields): carry the spec's optional location keys across a coordinate edit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 Claude-Session: https://claude.ai/code/session_01CRJge11jso9TpXRWFt1Z49 --- .../6664-location-optional-keys-carry.md | 27 ++ .../LocationField.optionalKeys.test.tsx | 231 ++++++++++++++++++ packages/fields/src/widgets/LocationField.tsx | 51 +++- 3 files changed, 306 insertions(+), 3 deletions(-) create mode 100644 .changeset/6664-location-optional-keys-carry.md create mode 100644 packages/fields/src/__tests__/LocationField.optionalKeys.test.tsx diff --git a/.changeset/6664-location-optional-keys-carry.md b/.changeset/6664-location-optional-keys-carry.md new file mode 100644 index 0000000000..eb84536095 --- /dev/null +++ b/.changeset/6664-location-optional-keys-carry.md @@ -0,0 +1,27 @@ +--- +'@object-ui/fields': patch +--- + +`LocationField` no longer discards a location's `altitude` / `accuracy` when the user +retypes the coordinate pair (objectui#6664). + +The widget edits the pair as one comma-separated text box and rebuilt its emission as a +fresh `{ lat, lng }` from the parsed text, so the two OPTIONAL keys `@objectstack/spec` +declares alongside them — `LocationValue` is `{ lat, lng, altitude?, accuracy? }` — were +gone the moment anyone edited the coordinates. Nothing warned; they simply were not in +the object handed to `onChange`. Both keys are registered on the platform's authorable +surface (`authorable-surface.base.json`), so a customer may author them even though the +platform itself produces neither today — measured in both repos. + +The drop **predates** objectui#6272: before that flip the widget emitted +`{ latitude, longitude }` and discarded the rest identically. What #6272 changed is only +that the *declared* value type is now the spec's, so the type claimed four keys while the +write path handled two. This closes that gap; it is not a regression #6272 introduced. + +The carry is a key-by-key pick of exactly those two keys out of a value that is already a +valid `LocationValue` — deliberately **not** a spread of the incoming value, which would +carry a stored record's retired `latitude` / `longitude` spelling straight back into the +emitted object and undo #6272's rename. A negative control pins that. Each key is taken +only when it is a usable number, because the spec's `z.number()` rejects `NaN`, `Infinity` +and a numeric string alike; leaving such a value behind narrows the emission rather than +widening what the widget accepts. diff --git a/packages/fields/src/__tests__/LocationField.optionalKeys.test.tsx b/packages/fields/src/__tests__/LocationField.optionalKeys.test.tsx new file mode 100644 index 0000000000..08140637d3 --- /dev/null +++ b/packages/fields/src/__tests__/LocationField.optionalKeys.test.tsx @@ -0,0 +1,231 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +/** + * objectui#6664 — editing the coordinate pair must not silently drop the + * spec's two optional keys. + * + * `@objectstack/spec` declares the stored shape of a `type: 'location'` value + * as `{ lat, lng, altitude?, accuracy? }`. `LocationField` edits the pair as + * one comma-separated text box and built its emission as a fresh `{ lat, lng }` + * from the parsed text, so a stored `altitude` / `accuracy` vanished the moment + * a user retyped the coordinates. Nothing warned; the two keys simply were not + * in the emitted object. + * + * 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 (to the spec's), so the type claimed four keys + * while the write path handled two. That mismatch is what made this visible; it + * is not a regression #6272 introduced, and keeping it out of that card's + * atomic fence (a bare two-sided rename, maintainer ruling 2026-08-28 「6272 + * A1」) was the right call. + * + * Triage measured that the platform has NO producer for either key — not in + * objectui, not in objectstack — and queued the card anyway, because both keys + * are registered on the authorable surface (`objectstack + * packages/spec/authorable-surface.base.json:3438,3442`). The platform has + * already promised customers they may author them, so the population that + * decides this is "does a customer write it", which neither repo can measure. + * + * ⛔ THE FENCE. The carry is a key-by-key pick of the two spec-declared + * optional keys, taken from a value that is ALREADY a valid `LocationValue` — + * never a wholesale spread of the incoming value, never `Object.assign`. A + * spread would carry a deprecated `latitude` / `longitude` key straight back + * into the emitted object and undo #6272's rename. + * + * ⚠️ And the spec schema cannot be that guard. `valueSchemaFor({ type: + * 'location' })` is a plain, NON-STRICT `z.object` — measured, and pinned in + * the last test below: it ACCEPTS `{ lat, lng, latitude, longitude }` and + * merely strips the two unknown keys from its parsed output, while the object + * handed to `onChange` still carries them. So every anti-dialect assertion here + * reads the EMITTED object's own keys rather than `safeParse`. + */ + +import { describe, it, expect, vi } from 'vitest'; +import React from 'react'; +import { render, screen, fireEvent, cleanup } from '@testing-library/react'; +import '@testing-library/jest-dom'; +import { valueSchemaFor } from '@objectstack/spec/data'; + +import { LocationField, type LocationValue } from '../widgets/LocationField'; + +const LOCATION_SCHEMA = valueSchemaFor({ type: 'location' } as any)!; + +/** + * The four keys the spec itself declares, read off the schema rather than + * copied into a literal here: the anti-spread assertions below are only worth + * anything if "outside the spec" means what the spec currently says. + */ +const SPEC_KEYS: readonly string[] = Object.keys((LOCATION_SCHEMA as any).shape); + +const field = { name: 'site', label: 'Site', type: 'location' } as any; + +/** Hangzhou, in the spec spelling, carrying both optional keys. */ +const STORED: LocationValue = { lat: 30.2741, lng: 120.1551, altitude: 5, accuracy: 12 }; +/** Shanghai — what the user retypes into the box. */ +const TYPED = '31.2304, 121.4737'; +const TYPED_PAIR = { lat: 31.2304, lng: 121.4737 }; + +/** Render with `value` stored, retype the coordinates, return what was emitted. */ +function emitAfterEdit(value: unknown, text: string = TYPED): any { + cleanup(); + const onChange = vi.fn(); + render( + , + ); + fireEvent.change(screen.getByRole('textbox'), { target: { value: text } }); + expect(onChange).toHaveBeenCalledTimes(1); + return onChange.mock.calls[0][0]; +} + +describe('LocationField carries the spec optional keys across a coordinate edit (objectui#6664)', () => { + it('keeps altitude and accuracy when the user retypes the pair', () => { + // The defect itself. This emitted `{ lat, lng }` alone before the fix, so a + // customer-authored altitude/accuracy was gone after a single edit. + expect(emitAfterEdit(STORED)).toEqual({ ...TYPED_PAIR, altitude: 5, accuracy: 12 }); + }); + + it('keeps whichever of the two the record actually carries', () => { + const altOnly = emitAfterEdit({ lat: 30.2741, lng: 120.1551, altitude: 5 }); + expect(altOnly).toEqual({ ...TYPED_PAIR, altitude: 5 }); + expect(altOnly).not.toHaveProperty('accuracy'); + + const accOnly = emitAfterEdit({ lat: 30.2741, lng: 120.1551, accuracy: 12 }); + expect(accOnly).toEqual({ ...TYPED_PAIR, accuracy: 12 }); + expect(accOnly).not.toHaveProperty('altitude'); + }); + + it('keeps a zero altitude, which is a real elevation and not an absent key', () => { + // Sea level. The carry must be keyed on "the record has a usable number", + // never on truthiness — `altitude: 0` is exactly the value a falsy test + // would silently discard. + expect(emitAfterEdit({ lat: 30.2741, lng: 120.1551, altitude: 0, accuracy: 0 })) + .toEqual({ ...TYPED_PAIR, altitude: 0, accuracy: 0 }); + }); + + it('emits a value the platform validator still accepts', () => { + // The load-bearing half is the spec's own validator, not a copy of it: the + // carry must not turn this widget into a producer of invalid values. + const parsed = LOCATION_SCHEMA.safeParse(emitAfterEdit(STORED)); + expect(parsed.success).toBe(true); + expect(parsed.data).toEqual({ ...TYPED_PAIR, altitude: 5, accuracy: 12 }); + }); + + it('does not let one unusable optional key cost the other one', () => { + // Measured against the spec schema: `z.number()` rejects NaN, Infinity and + // a numeric STRING alike — all three report `invalid_type` at `[altitude]`. + // So an unusable altitude cannot be carried without making the whole + // emission spec-invalid, and it is dropped by the same finite-number test + // the widget already applies to `lat`/`lng`. That is a NARROWING — it emits + // less, never more — and it must not take a perfectly good `accuracy` with + // it. + for (const altitude of [NaN, Infinity, '5' as unknown as number]) { + const emitted = emitAfterEdit({ lat: 30.2741, lng: 120.1551, altitude, accuracy: 12 }); + expect(emitted).not.toHaveProperty('altitude'); + expect(emitted).toEqual({ ...TYPED_PAIR, accuracy: 12 }); + expect(LOCATION_SCHEMA.safeParse(emitted).success).toBe(true); + } + }); +}); + +describe('LocationField invents optional keys it was never given (objectui#6664)', () => { + it('adds nothing to a record that carries neither key', () => { + expect(Object.keys(emitAfterEdit({ lat: 30.2741, lng: 120.1551 })).sort()) + .toEqual(['lat', 'lng']); + }); + + it('adds nothing when there is no previously stored value', () => { + expect(Object.keys(emitAfterEdit(null)).sort()).toEqual(['lat', 'lng']); + }); + + it('still emits null when the box is cleared, carrying nothing forward', () => { + // Clearing the box means "unset", and an unset location has no altitude to + // preserve. The carry must not resurrect the old value as a partial object. + const onChange = vi.fn(); + render(); + fireEvent.change(screen.getByRole('textbox'), { target: { value: ' ' } }); + expect(onChange).toHaveBeenCalledWith(null); + }); +}); + +/** + * ⚠️ NEGATIVE CONTROL — green BEFORE this fix as well as after, and that is the + * correct reading rather than a vacuous one. Today's code emits a freshly built + * `{ lat, lng }`, which trivially carries no dialect, so there is no red for + * these to show. They exist to guard the FUTURE regression the fence names: + * someone "simplifying" the key-by-key pick into `onChange({ ...value, lat, + * lng })`. Under that rewrite the first test below goes red, because its stored + * value carries BOTH spellings — which is exactly the guard the fence asked + * for, and exactly what no positive pin can provide. + */ +describe('NEGATIVE CONTROL — the carry is a key-by-key pick, never a spread (objectui#6664 fence)', () => { + it('never emits the deprecated latitude/longitude, even from a value carrying both spellings', () => { + // The sharp control: this value IS a valid `LocationValue` (`lat`/`lng` are + // finite), so the carry path really runs on it — and it must still leave + // the retired spelling behind. A spread would emit all six keys and undo + // #6272. + const emitted = emitAfterEdit({ + lat: 30.2741, + lng: 120.1551, + latitude: 30.2741, + longitude: 120.1551, + altitude: 5, + accuracy: 12, + }); + expect(emitted).not.toHaveProperty('latitude'); + expect(emitted).not.toHaveProperty('longitude'); + }); + + it('emits no key the spec does not declare, whatever the record carries', () => { + // The general form of the assertion above, derived from the spec's own + // declared key set: a spread of the incoming value would leak whatever + // extra keys a record happens to hold, of which the retired pair is only + // the most damaging example. + const emitted = emitAfterEdit({ + lat: 30.2741, + lng: 120.1551, + latitude: 30.2741, + longitude: 120.1551, + provider: 'gps', + altitude: 5, + }); + expect(SPEC_KEYS).toEqual(['lat', 'lng', 'altitude', 'accuracy']); + expect(Object.keys(emitted).filter((k) => !SPEC_KEYS.includes(k))).toEqual([]); + }); + + it('carries nothing at all out of a value the spec rejects outright', () => { + // A record in the retired `{ latitude, longitude }` spelling reads as unset + // here (ruled A1, pinned in LocationField.specShape.test.tsx). Its optional + // keys are not salvaged either — harvesting them would be reading the + // dialect through a side door, which the A1 ruling closed. + expect(LOCATION_SCHEMA.safeParse({ latitude: 30.2741, longitude: 120.1551 }).success).toBe(false); + const emitted = emitAfterEdit({ + latitude: 30.2741, + longitude: 120.1551, + altitude: 5, + accuracy: 12, + }); + expect(Object.keys(emitted).sort()).toEqual(['lat', 'lng']); + expect(emitted).toEqual(TYPED_PAIR); + }); + + it('pins WHY those assertions read keys and not safeParse: the schema is not strict', () => { + // Measured, and the reason the fence needs a guard of its own: the spec's + // `LocationValue` is a plain `z.object`, so an emission polluted by a + // spread still PASSES validation — zod merely strips the unknown keys from + // its parsed OUTPUT, while the object handed to `onChange` keeps them. + const polluted = { lat: 31.2304, lng: 121.4737, latitude: 30.2741, longitude: 120.1551 }; + const parsed = LOCATION_SCHEMA.safeParse(polluted); + expect(parsed.success).toBe(true); + expect(Object.keys(parsed.data as object).sort()).toEqual(['lat', 'lng']); + // The object itself is where the dialect survives — hence the key-level + // assertions above. If the spec ever turns strict, this test is the note + // saying the guard could then be delegated to it. + expect(polluted).toHaveProperty('latitude'); + }); +}); diff --git a/packages/fields/src/widgets/LocationField.tsx b/packages/fields/src/widgets/LocationField.tsx index a1c38fbb65..aac3b3cfe7 100644 --- a/packages/fields/src/widgets/LocationField.tsx +++ b/packages/fields/src/widgets/LocationField.tsx @@ -41,8 +41,51 @@ export type { LocationValue } from '@objectstack/spec/data'; function isLocationValue(value: unknown): value is LocationValue { if (!value || typeof value !== 'object' || Array.isArray(value)) return false; const { lat, lng } = value as Record; - return typeof lat === 'number' && Number.isFinite(lat) - && typeof lng === 'number' && Number.isFinite(lng); + return isFiniteNumber(lat) && isFiniteNumber(lng); +} + +/** + * A usable numeric component of a location: a real number, never `NaN` or an + * infinity. Named so the coordinates and the two optional keys below are held + * to the SAME test rather than to two copies of it that can drift apart. + */ +function isFiniteNumber(n: unknown): n is number { + return typeof n === 'number' && Number.isFinite(n); +} + +/** + * Build the value emitted for a freshly typed coordinate pair, carrying the + * spec's two OPTIONAL keys across the edit (objectui#6664). + * + * The box edits `lat`/`lng` only, but a stored location may also carry + * `altitude` and `accuracy` — both declared by `LocationValueSchema`, and both + * registered on the platform's authorable surface, so a customer may write + * them. Rebuilding the emission as a bare `{ lat, lng }` dropped them silently + * the moment anyone retyped the coordinates, with nothing to warn them. + * + * ⛔ The carry is a KEY-BY-KEY pick out of an already-valid `LocationValue`, + * and deliberately NOT a spread of `previous` (nor `Object.assign`): a stored + * record may still hold the retired `{ latitude, longitude }` spelling, and + * spreading it would carry that dialect straight back into the emitted object + * and undo objectui#6272's rename. The spec schema cannot be that guard — + * `LocationValueSchema` is a plain, NON-STRICT `z.object`, so it ACCEPTS a + * polluted object and merely strips the unknown keys from its own parsed + * OUTPUT, while the value handed to `onChange` keeps them. Both facts are + * pinned in `__tests__/LocationField.optionalKeys.test.tsx`. + * + * Each optional key is taken only when it is a usable number. Measured against + * the spec: `z.number()` rejects `NaN`, `Infinity` and a numeric string alike + * (`invalid_type` at `[altitude]`), so carrying one of those forward would make + * this widget emit a value the platform's own validator refuses. Leaving it + * behind is a NARROWING — this emits less than it was handed, never more — not + * a tolerant fallback of the kind AGENTS.md #0.1 bans. + */ +function carryOptionalKeys(lat: number, lng: number, previous: unknown): LocationValue { + const emitted: LocationValue = { lat, lng }; + if (!isLocationValue(previous)) return emitted; + if (isFiniteNumber(previous.altitude)) emitted.altitude = previous.altitude; + if (isFiniteNumber(previous.accuracy)) emitted.accuracy = previous.accuracy; + return emitted; } /** @@ -82,7 +125,9 @@ export function LocationField({ value, onChange, field, readonly, error, ...prop const lat = parseFloat(parts[0]); const lng = parseFloat(parts[1]); if (!isNaN(lat) && !isNaN(lng)) { - onChange({ lat, lng }); + // The typed pair replaces `lat`/`lng`; `altitude`/`accuracy` survive + // the edit (objectui#6664). Key-by-key, never a spread — see above. + onChange(carryOptionalKeys(lat, lng, value)); } // If invalid, don't update the value }