Repository navigation
fix(fields): read a 0 geolocation coordinate as a place, not as an absence (objectui#8055) - #8898
Merged
Merged
Conversation
…absence (objectui#8055)
`GeolocationField`'s display path carried two independent defects, and fixing
either one alone left the other live.
1. Presence was tested by asking the parsed NUMBER whether it was falsy
(`if (!loc.latitude || !loc.longitude)`), and `0` is falsy. The equator
(`latitude: 0`), the prime meridian (`longitude: 0`) and a perfect
`accuracy: 0` all took the "no value" branch: the `EmptyValue` em dash
instead of the coordinates, no accuracy row, and no "View on map" — whose
`openInMaps` handler refused the same value a second time.
2. Three of those guards were JSX render expressions
(`{location.latitude && location.longitude && (…)}`). With `latitude === 0`
that evaluates to `0`, and React renders the NUMBER as a text node. Measured
on `main`, `{ latitude: 0, longitude: 120.1551 }` rendered `"—0"`: the dash
from defect 1 and a stray `0` from defect 2.
Both close on one predicate that is nullish AND boolean-valued — nullish so `0`
counts as present, boolean so no numeric operand can reach the DOM through
`&&`. The five sites were found by sweeping the file against the class
criterion, not by taking the card's list: every "has a value" test whose
operand may legitimately be `0`, and every JSX conditional with a numeric
operand.
The delta is exactly `0` and `-0`. `NaN` stays excluded, as the old falsy guard
had it — a bare `!= null` would have begun rendering `"NaN, NaN"` at a surface
that has always shown the placeholder for an unreadable coordinate.
`value={location.latitude ?? ''}` was already correct and is untouched; it is
pinned below as a control, along with the byte-identical non-zero render.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01611D6ZaRaMmwTNQmSbk8MH
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 #8055
GeolocationField's display/read path carried two independent defects. Both are fixedhere; the card was right that fixing either alone leaves the other live, and the ablation
below proves the two apart by making each leg red on a different test.
The class sweep — 23 conditional sites, not the card's 5
The card's "Affected sites" list is a snapshot of one measurement, so the file was swept
against the class criterion instead: every "has a value" test whose operand may
legitimately be
0must use nullish semantics, and no JSX conditional may carry a numericoperand. All 23 conditional/coercion sites in the file, with verdicts:
Class hits — fixed (5). The operand is a coordinate or an accuracy, all of which are
legitimately
0, and three of them are JSX render expressions:formatLocationif (!loc.latitude || !loc.longitude) return ''openInMapsif (!location.latitude || !location.longitude) return{location.latitude && location.longitude && (…)}{location.latitude && location.longitude && (…)}{location.accuracy && (…)}Already correct — deliberately untouched (2).
value={location.latitude ?? ''}and itslongitude twin
value={location.longitude ?? ''}. Both use??, both already showed"0".The card named the first; the second is its twin and is equally correct. Neither was "fixed"
into consistency with the broken guards — they are the live controls, see below.
Not in the class (16), each for a stated reason.
value || {}(the operand is thecomposite object, never legitimately
0);fieldValue ? Number(fieldValue) : null(theoperand is the raw input string, and
'0'is truthy — this is objectui#6848's pinnedemission site and stays as it is);
if (!navigator.geolocation)(an API object); the map URLtemplate (an interpolation, not a test);
if (readonly)and theisLoadingternary(booleans);
{formatted ? … : …}(a ternary on a string, whose''is the intendedsentinel — a ternary always renders exactly one branch, so no operand can leak);
disabled={readonly || …}×3 andclassName={… ? … : …}×2 (attribute values, not children,and never numeric);
aria-invalid={!!error || !!…}×2 (explicitly coerced); and the two newpredicates themselves.
The fix
One predicate that is nullish and boolean-valued, because the two defects need both
properties and neither implies the other:
0counts as present — that is defect 1. A boolean-valued falsy guard wouldstill call the equator "no location".
&&can never hand React a number to render as a text node — that isdefect 2. A nullish-but-numeric guard would still leak the
0.The delta is exactly
0and-0. The predicate excludesNaN, as the old falsy guarddid. A bare
n != null— the card's wording taken literally — would have started rendering"NaN, NaN"at a surface that has always shown the placeholder for an unreadable coordinate:a second display defect wearing the first one's fix.
Infinity,null,undefined, amissing half of the pair and a non-number all answer exactly as they did before. Pinned as a
control.
Before / after
The new pins run against
main's version of the file reproduce the card's reading verbatim:Against
main: 10 failed | 5 passed (15). The 5 that pass are exactly the 5 controls —a control that goes red with the subject is not a control. After the fix: 15 passed (15).
Ablation — two legs, expectations written before running
Each leg mutated the committed tree, proved the mutation was on disk (marker counts plus a
git hash-objectcomparison against the HEAD blob, with an empty hash treated as failure),ran the pins, then restored and proved the restore by state — blob back to the HEAD value
and
git diff HEADempty — never by an exit code. Both legs restore under atrap … EXIT INT TERMusinggit checkout HEAD -- ABSOLUTE_PATH. No build step is on thepath: the pins import the widget by a same-package relative source specifier, so
dist/never participates.
!!n(still boolean, so defect 2 stays fixed)0{location.latitude && location.longitude && (…)}(presence still nullish)0returnsThe legs are red on different tests, which is the point:
tests. Under leg 2 they stay green, because
formatLocationis still nullish.readonly: the coordinates row has no bare text-node children,which failed with
expected [ '0' ] to deeply equal []— the card's stray0, caught asthe DOM shape it actually is. Under leg 1 it stays green, because a boolean guard renders
nothing either way.
than sloppy — that single site carried both defects, so it is the user-facing consequence
of either, and it is deliberately not the separator.
Defect 2's oracle is written as a DOM shape, not as a string diff: it reads the non-blank
text nodes that are direct children of the element holding the guard, which is exactly
where React puts a rendered numeric operand. It is deliberately not recursive — the accuracy
row legitimately renders
"Accuracy: ±","0","m"as three nodes one level down.Per the card's warning about assertions a
0can fool: the positive half assertstoFixed(6)'s own product ("0.000000, 120.155100"), which a stray0cannot satisfy;not.toContain(EM_DASH)is never used alone.The two things the card did not measure
Does the sibling
LocationFieldcarry the same shape? Measured: no — and ⛔ it was nottouched. Its presence test is
isFiniteNumber(typeof n === 'number' && Number.isFinite(n)), which admits0; its readonly render is{coordinateText(value) || EmptyValue}, whose operand is a string ("0, 0"for a storedzero, truthy), so no numeric operand exists to leak; its one other guard's operand is
string | null. It already answers this card's class criterion, by a documented ruling —objectui#6272 option A1, whose docblock reasons about
0explicitly — and a landed pinalready asserts the opposite of this defect:
LocationField.specShape.test.tsx, "carries azero coordinate as the real place it is, not as a default", asserting
textContentis'0, 0'. Re-run green. No follow-up card is warranted.Does any consumer depend on the current empty rendering? Measured: no. No test, snapshot
or doc paragraph pins "a
0geolocation coordinate displays as empty". Every zero-coordinatefixture in the tree belongs to
LocationField/ObjectMapand pins the opposite. The onecross-package renderer of this widget is
plugin-detail'sInlineFieldInput; its emptinessauthority tests use the non-zero
{ latitude: 30.2741, longitude: 120.1551 }fixture and areunaffected. Both suites re-run green (see below). No pin was changed, and none needed to be.
Verification
pnpm --filter '@object-ui/fields^...' run build(dependency closure)pnpm --filter @object-ui/fields run type-check(tsc --noEmitandtsconfig.test.json, so the new pins are typechecked)pnpm --filter @object-ui/fields run testpnpm --filter @object-ui/plugin-detail run test(the consumer)pnpm --filter @object-ui/fields run lintfieldparam)packages/fields/src/__tests__/GeolocationZeroCoordinate-8055.test.tsxMeasured at
3db5b0ee2. Every heavy run went through the container's shared verify lock;each is quoted from the wrapper's own
VERDICT command-exitline, never a bare$?.A first
type-checkreturned exit 2 — PREREQUISITE NOT MET (workspace unbuilt,Cannot find module '@object-ui/components'). That is NOT MEASURED, not red; it is recordedhere only so the green above is not mistaken for a re-run of nothing. It was re-run after the
closure build.
Published surface: byte-identical.
packages/fields/dist/index.d.tswas built fromorigin/main's version of the widget and from this branch's and diffed: identical. Thetwo predicates and the narrowing type are module-private (no
export), soexport *inindex.tsxdoes not re-export them.Line-citation census (objectui#8875). This change inserts ~59 lines near the top of
GeolocationField.tsx, moving every line below.scripts/cross-file-line-citation-census.mjsreports zero citations naming that file, in either direction, and a five-syntax grep
confirms nothing in the tree addresses it by line. The two new files add no
path:linecitation to the population. No class-(A) repair was needed, and nothing class-(B) was
touched. Re-checked against the final commit.
验收备注
EmptyValueplaceholder path is unchanged and still correct for a genuinely absentcoordinate, for a half pair (one coordinate present, one absent — a half pair must not
invent the other half) and for
NaN. All three are pinned as controls.openInMapswas fixed as part of the class sweep, not as a separate ask: it is the samefalsy presence test one line from
formatLocation, and it refused the same valid value asecond time. Pinned by asserting the URL
window.openreceives.the card left were both measured and both came back negative (above); neither produces a
follow-up card. Successor for the
LocationFieldhalf: none — nothing to carry.LocationField, the emission path (handleFieldChange, whichobjectui#6848 measured and pinned as already correct), and the two
?? ''input bindings.Generated by Claude Code