fix(fields): GeolocationField emits null for a cleared coordinate - #8056
Merged
Conversation
Emptying a latitude or longitude box emitted `{ …, latitude: undefined }`
where CurrencyField, PercentField and NumberField all emit `null` for the
identical user action, and LocationField emits `null` too. This composite
was the only widget of the class that did not.
`undefined` cannot survive serialization: JSON.stringify drops an
undefined-valued key outright, so the emission stopped saying "the user
cleared this" the moment it left memory. The pin asserts the emission is
`null` AND that it survives a JSON round-trip — the assertion the old code
actually failed.
Measured, and deliberately NOT claimed: the escalation on the card was that
the dropped key reaches a PATCH as an ABSENT key ("leave it alone") so the
old coordinate silently survives a save. Absent really is not null on the
write path — the client sends PATCH + JSON.stringify, driver-memory merges
{ ...stored, ...data } and driver-sql SETs only present keys. But the
dropped key here is nested one level BELOW the key the write path merges
on: the body still carries the composite's own key, a location value is a
single JSON column, and nothing deep-merges, so the whole value is replaced
and the cleared coordinate does not come back. No silent data loss found.
A legitimate 0 coordinate is unaffected and now pinned: the emptiness test
reads the raw input string, and '0' is not an empty string.
GeolocationValue widens latitude/longitude/accuracy to `number | null`.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QnpvbdoRisQdRAczkLwnf5
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
|
os-steve
marked this pull request as ready for review
September 6, 2026 13:42
baozhoutao
pushed a commit
that referenced
this pull request
Sep 6, 2026
Brings in batch 22 (#8063, packages/layout/README.md leaves the ledger) and #8056, which moves packages/fields source and therefore the built types this corpus is judged against. Forced build, gates, probes and tests re-run on the merged head. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FhBNJcLRZLe8M87VcUgpKr
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 #6848
Step 1 was a MEASUREMENT, and it re-prices the card
Triage wrote the escalation condition into the card so it would not be re-argued: if an absent key and an explicit
nullare treated differently on the write path, this is immediately apriority:p1silent data-loss defect (the user clears a coordinate, sees an empty box, saves, and the old coordinate survives, with no diagnostic); if they are treated identically, it drops to ap3consistency tidy.The answer has two halves, and they point in opposite directions. My verdict: NOT p1 — recommend
priority:p3.Half 1 — absent really is NOT null on the write path (the condition's literal antecedent is TRUE)
Read on the tree, entrance re-taken (triage cited
:2958;mainhas moved, it is nowpackages/data-objectstack/src/index.ts:3081):ObjectStackAdapter.update()passesdatastraight through tothis.client.data.update(...)— no normalization of any kind.@objectstack/client17.2.0data.updatesendsmethod: "PATCH",body: JSON.stringify(data).driver-memory'supdate()merges{ ...table[index], ...data, ... }— a shallow spread.driver-sql'supdate()issuesbuilder.update(payload)—SETfor the keys present in the payload.packages/objectql/src/secret-fields.ts: "To clear the stored X, write null; to leave it unchanged, omit the field."Executed end to end through that model: an absent key leaves
{"id":"r1","amount":42}unchanged, an explicitnullwrites{"id":"r1","amount":null}. Different.Half 2 — but the harm the
p1grade is DEFINED by is not reachable for this widgetThe escalation condition defines its own harm in the parenthetical: the old coordinate survives the save. It does not.
The dropped key is nested one level below the key the write path merges on.
GeolocationFieldemits a composite, so the payload is{ FIELD: { longitude: 120.1551 } }— the composite's own key is present. Alocationvalue is stored as a single JSON column (JSON_COLUMN_TYPESindriver-sql, which listscomposite/address/location/recordas objects), and nothing on the path deep-merges (the onlydeepMergeinobjectqlis the metadata registry's, not the data write path). So the whole value is replaced.Measured through the real widget, then through the real chain (
sanitizeFormData→ the client'sJSON.stringify→ the drivers' shallow merge):sanitizeFormDatawas confirmed to be a pass-through (out[key] = value) that only walks top-level keys, andObjectFormsends the whole sanitizedformDataon edit with no dirty-diff and no merge.⇒ No silent data loss exists here, and none is fixed by this PR. The antecedent is true in general; the consequent is not reachable for this widget. I am reporting both rather than collapsing them, because the two halves disagree and picking a side quietly is what the card was written to prevent.
The falsy-guard question, measured as asked
fieldValue ? … : undefinedtests the raw input string, not the parsed number.'0'is a non-empty string, so a legitimate0coordinate takes the value branch:The trap is NOT live at this emission site. It is now pinned, so a future "simplify" cannot move the guard onto the parsed number, where it would be live.
What changed
packages/fields/src/widgets/GeolocationField.tsx(emission site re-taken at:82, verbatim as cited):CurrencyField,PercentFieldandNumberFieldalready emitnullfor the identical action, andLocationFieldemitsnulltoo (onChange(null)) — this composite was the only widget of the class that did not.undefinedcannot survive serialization:JSON.stringifydrops the key outright, so the emission stopped saying "the user cleared this" the moment it left memory.GeolocationValuewidenslatitude/longitude/accuracytonumber | null.undefinedstays admissible — an untouched coordinate is genuinely absent.Tests
packages/fields/src/__tests__/GeolocationClearEmission.test.tsx(8 tests). It asserts the emission isnulland that it survivesJSON.stringify— the second half is the point, since the defect was never about which sentinel sat in memory. Also pins the0coordinate and the class-wide agreement across all four widgets.NumberInputWidgets.browserDeliverable.test.tsx— itsgeolocation/1erow pinned the defect as current product behaviour. It now readsnull, with the history kept in the comment rather than deleted, because that reading is what the divergence was reported from.pnpm --filter @object-ui/fields test→ 135 files passed, 2194 tests passed.pnpm --filter @object-ui/fields type-check→ exit 0.check:control-bytes,check:unreferenced-sources,check:published-dist,check:published-tsconfig-exclude, and the three changeset gates (check-changeset-presenceconfirms 1 changeset for 3 changed published-source files, compared against merge-base06761b351).Reverse verification (ablation)
Predicted direction before running: turns red. Fix committed first, mutation proven to land on disk by blob hash (
33979f4→6826a7f) and by anchor counts (: null1→0,: undefined0→1), with anEXIT INT TERMtrap holding the restore:Five red across both pin files. Restore proven by byte identity — on-disk blob back to
33979f42…, matchingHEAD, withgit diff HEADempty. The tests import the widget by relative source path, so nodistis involved and the mutation reaches them directly.Cross-package type check
fieldsrebuilt first, and the emittedpackages/fields/dist/widgets/GeolocationField.d.tsconfirmed to carrylatitude?: number | null— so the reading below is of a fresh build, not a cache. Then a two-leg probe through a real consumer (@object-ui/plugin-form):latitude: nullassigned toGeolocationValue→ exit 0 (rejected before this change, so the widening is live in the consumer's view).latitude: 'not-a-number'→ exit 1,error TS2322— the control leg, proving the check has teeth.GeolocationValueis referenced by name nowhere outsidepackages/fields, and the coordinate keys have no other consumer (thelatitudeField/longitudeFieldhits in@object-ui/typesare map field names, a different thing).Declared narrowing
I ran the owning package's full suite, the targeted gate families, and the consumer-direction type sweep. I did not run every one of the 20 affected packages' test suites locally — that is CI's run, and this is a declared narrowing rather than a silent one. Gate status at report time is therefore
in_progress; CI convergence is the PM's read, not mine.Note for whoever derives gates here:
scripts/pm/dispatch-gates.mjsdoes not exist in this repo, so the families above were derived by hand from.github/workflows/andpackage.json.Scope fences honoured
parseFloatin Currency/Percent vsNumberin Number/Geolocation was measured to agree on every string a real browser can deliver — latent, not live. Not widened.''is taken from that table as a value a real browser delivers.packages/data-objectstackwas read-only for the measurement; nopackages/spec; objectstack read-only.nullinstead ofundefinedmoves no schema's accept set — the objectstackLocationValueSchemais astrictObjectoverlat/lng, so{longitude: …}and{latitude: null, longitude: …}are refused identically by it. (Thatlat/lngvslatitude/longitudespelling gap is already known and ruled — see docs(fields):location.mdxteaches the deprecated{ latitude, longitude }spelling as canonical — the spec'sLocationValueis{ lat, lng }(docs half of the #6272 A1 ruling) #6660 / the bug(fields): atype: 'location'value spelled{ lat, lng }reads as0, 0inLocationFieldwhile the same value renders correctly throughLocationCellRenderer#6272 A1 ruling — so it is not filed as new.)Out-of-scope finding, filed not fixed
#8055 —
GeolocationFieldrenders a valid0coordinate as EMPTY, and leaks a literal0into the DOM. Measured while working here: readonly at{ latitude: 0, longitude: 120.1551 }(a real point on the equator) renders"—0"against a control of"30.274100, 120.155100View on map". Two distinct defects — the falsy-on-number guards informatLocation/openInMaps/ the map-button gates, and0 && xrendering a literal0through JSX. That is the widget's display path, a different code path and a different defect class from this card's emission path, so per the dispatch's instruction it is named rather than silently folded in.Generated by Claude Code