Skip to content

Commit f52a9d7

Browse files
claude[bot]claude
andauthored
fix(fields): read a 0 geolocation coordinate as a place, not as an absence (objectui#8055) (#8898)
`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. Claude-Session: https://claude.ai/code/session_01611D6ZaRaMmwTNQmSbk8MH Co-authored-by: Claude <noreply@anthropic.com>
1 parent b686ebf commit f52a9d7

3 files changed

Lines changed: 312 additions & 5 deletions

File tree

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
---
2+
'@object-ui/fields': patch
3+
---
4+
5+
`GeolocationField` renders a `0` coordinate as the place it is, and stops leaking a literal
6+
`0` into the DOM (objectui#8055).
7+
8+
Two independent defects were live in the widget's **display/read** path, and fixing either
9+
one alone left the other:
10+
11+
1. **A valid location displayed as empty.** Every presence test in the widget asked the
12+
parsed NUMBER whether it was falsy — `if (!loc.latitude || !loc.longitude)` — and `0` is
13+
falsy. So the equator (`latitude: 0`), the prime meridian (`longitude: 0`) and a perfect
14+
`accuracy: 0` all took the "no value" branch: the coordinates row showed the `EmptyValue`
15+
em dash, the accuracy row vanished, and "View on map" was withheld — including its
16+
`openInMaps` handler, which refused the same value a second time. The stored data was
17+
intact and unreachable.
18+
19+
2. **A literal `0` reached the DOM.** Three of those guards were JSX render expressions
20+
(`{location.latitude && location.longitude && (…)}`). With `latitude === 0` such an
21+
expression evaluates to `0`, and React renders the NUMBER as a text node. Measured on
22+
`main`, `{ latitude: 0, longitude: 120.1551 }` rendered `"—0"`: the em dash from defect 1,
23+
and a stray `0` beside it from defect 2.
24+
25+
Both are closed by one predicate that is nullish **and** boolean-valued — nullish so `0`
26+
counts as present, boolean so no numeric operand can ever reach the DOM through `&&`.
27+
28+
**The delta is exactly `0` and `-0`.** The predicate excludes `NaN` just as the old falsy
29+
guard did; a bare `!= null` would have started rendering `"NaN, NaN"` at a surface that has
30+
always shown the placeholder for an unreadable coordinate. `Infinity`, `null`, `undefined`, a
31+
missing half of the pair and a non-number all answer exactly as they did before.
32+
33+
**If you depended on the old rendering**, you were depending on a zero coordinate displaying
34+
as no coordinate; nothing in this repo did (the sibling `LocationField` already pins the
35+
opposite — a stored `{ lat: 0, lng: 0 }` renders `0, 0`). A `type: 'geolocation'` record
36+
holding a zero now shows its coordinates, its accuracy row and its map link.
Lines changed: 212 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,212 @@
1+
/**
2+
* ObjectUI
3+
* Copyright (c) 2024-present ObjectStack Inc.
4+
*
5+
* This source code is licensed under the MIT license found in the
6+
* LICENSE file in the root directory of this source tree.
7+
*/
8+
9+
/**
10+
* objectui#8055 — `GeolocationField`'s DISPLAY path reads a valid `0`
11+
* coordinate as "no location", and leaks the literal `0` into the DOM.
12+
*
13+
* ## TWO defects, and this file keeps them apart
14+
*
15+
* The card measured one reading and correctly separated two independent
16+
* defects inside it. They are pinned by DIFFERENT assertions here, because
17+
* fixing either one alone leaves the other live:
18+
*
19+
* 1. **Falsy used as a presence test.** `if (!loc.latitude || !loc.longitude)`
20+
* asks the parsed NUMBER whether it is falsy, and `0` is falsy — so the
21+
* equator (`latitude: 0`), the prime meridian (`longitude: 0`) and a
22+
* perfect `accuracy: 0` all took the "no value" branch. Measured on `main`:
23+
* `{ latitude: 0, longitude: 120.1551 }` rendered `"—0"` where the control
24+
* `{ latitude: 30.2741, longitude: 120.1551 }` rendered
25+
* `"30.274100, 120.155100View on map"`.
26+
* ⇒ pinned by the "renders the coordinates" assertions.
27+
*
28+
* 2. **A numeric operand in a JSX conditional.** The same guard was also
29+
* written as `{location.latitude && location.longitude && (…)}`. With
30+
* `latitude === 0` that expression evaluates to `0`, and React renders the
31+
* NUMBER as a text node — the trailing `0` of `"—0"`.
32+
* ⇒ pinned by {@link directTextNodes}, which reads exactly the place such
33+
* an operand lands: a text node that is a DIRECT child of the container
34+
* holding the guard.
35+
*
36+
* ⚠️ The two are pinned separately on purpose. `directTextNodes` stays EMPTY
37+
* when only defect 1 is live (a boolean guard renders nothing either way), and
38+
* the coordinate string is still rendered when only defect 2 is live — so
39+
* neither assertion can stand in for the other. The "View on map" assertions
40+
* answer to BOTH, because that one site carried both defects; they are the
41+
* user-facing consequence, not the separator.
42+
*
43+
* ## The two live CONTROLS this card came with
44+
*
45+
* `value={location.latitude ?? ''}` was ALREADY correct on `main` — it uses
46+
* `??`, and the box showed `"0"`. It is pinned below as a control: it proves
47+
* the probe can see this widget and read a `0` out of it, and it must not have
48+
* been "fixed" into consistency with the broken guards. The non-zero render
49+
* string is the second control and must be byte-identical to the card's
50+
* reading.
51+
*/
52+
53+
import { describe, it, expect, vi, afterEach } from 'vitest';
54+
import React from 'react';
55+
import { render, cleanup, fireEvent } from '@testing-library/react';
56+
import type { FieldMetadata } from '@object-ui/types';
57+
import { GeolocationField, type GeolocationValue } from '../widgets/GeolocationField';
58+
59+
/** U+2014, the glyph `EmptyValue` draws — asserting on it is asserting "no value". */
60+
const EM_DASH = '—';
61+
62+
const field = (name: string, type: string): FieldMetadata =>
63+
({ name, label: name, type } as unknown as FieldMetadata);
64+
65+
const noop = () => undefined;
66+
67+
/** The readonly composite collapses to one row; that row is the container. */
68+
function renderReadonly(value: GeolocationValue): HTMLElement {
69+
const { container } = render(
70+
<GeolocationField value={value} onChange={noop} field={field('geo', 'location')} readonly />,
71+
);
72+
return container.firstElementChild as HTMLElement;
73+
}
74+
75+
/** The editable composite's outer container (`space-y-3`). */
76+
function renderEditable(value: GeolocationValue): HTMLElement {
77+
const { container } = render(
78+
<GeolocationField value={value} onChange={noop} field={field('geo', 'location')} />,
79+
);
80+
return container.firstElementChild as HTMLElement;
81+
}
82+
83+
/**
84+
* The non-blank text nodes that are DIRECT children of `el`.
85+
*
86+
* ⭐ This is defect 2's oracle, and it is written as a DOM shape rather than as
87+
* a string diff on purpose: `{0 && <Button/>}` puts the number where React puts
88+
* any other child — a text node parented by the element holding the guard. A
89+
* text node nested deeper is somebody's legitimate content (the accuracy row
90+
* really does render `"Accuracy: ±"`, `"0"`, `"m"` as three nodes), so the
91+
* scan is deliberately NOT recursive.
92+
*/
93+
function directTextNodes(el: Element): string[] {
94+
return Array.from(el.childNodes)
95+
.filter((n) => n.nodeType === 3 /* Node.TEXT_NODE */)
96+
.map((n) => n.textContent ?? '')
97+
.filter((t) => t.trim() !== '');
98+
}
99+
100+
afterEach(cleanup);
101+
102+
describe('objectui#8055 defect 1 — a `0` coordinate is a PLACE, not an absence', () => {
103+
it('⭐ readonly: a zero LATITUDE renders the coordinates, not the empty placeholder', () => {
104+
const row = renderReadonly({ latitude: 0, longitude: 120.1551 });
105+
106+
// ⛔ Not `not.toContain('—')` alone: that assertion is satisfied by the
107+
// stray `0` of defect 2 and would call a still-broken widget fixed. The
108+
// positive half is `toFixed(6)`'s own product, which nothing but a real
109+
// formatted coordinate can produce.
110+
expect(row.textContent).toContain('0.000000, 120.155100');
111+
expect(row.textContent).not.toContain(EM_DASH);
112+
});
113+
114+
it('readonly: a zero LONGITUDE is the same rule, not a second one', () => {
115+
const row = renderReadonly({ latitude: 30.2741, longitude: 0 });
116+
expect(row.textContent).toContain('30.274100, 0.000000');
117+
expect(row.textContent).not.toContain(EM_DASH);
118+
});
119+
120+
it('readonly: `{ latitude: 0, longitude: 0 }` renders both zeros', () => {
121+
const row = renderReadonly({ latitude: 0, longitude: 0 });
122+
expect(row.textContent).toContain('0.000000, 0.000000');
123+
expect(row.textContent).not.toContain(EM_DASH);
124+
});
125+
126+
it('editable: an `accuracy` of 0 renders its row instead of hiding it', () => {
127+
const outer = renderEditable({ latitude: 30.2741, longitude: 120.1551, accuracy: 0 });
128+
expect(outer.textContent).toContain('Accuracy: ±0m');
129+
});
130+
});
131+
132+
describe('objectui#8055 defect 2 — no numeric operand reaches the DOM as a text node', () => {
133+
it('⭐ readonly: the coordinates row has no bare text-node children', () => {
134+
const row = renderReadonly({ latitude: 0, longitude: 120.1551 });
135+
136+
// On `main` this is `['0']` — the trailing `0` of the card's `"—0"`.
137+
expect(directTextNodes(row)).toEqual([]);
138+
});
139+
140+
it('⭐ editable: the button row has no bare text-node children', () => {
141+
const outer = renderEditable({ latitude: 0, longitude: 120.1551 });
142+
const buttonRow = outer.firstElementChild as HTMLElement;
143+
expect(directTextNodes(buttonRow)).toEqual([]);
144+
});
145+
146+
it('⭐ editable: the accuracy guard leaks nothing into the outer container', () => {
147+
const outer = renderEditable({ latitude: 30.2741, longitude: 120.1551, accuracy: 0 });
148+
expect(directTextNodes(outer)).toEqual([]);
149+
});
150+
});
151+
152+
describe('objectui#8055 — the affordance the two defects withheld', () => {
153+
it('readonly: "View on map" is offered for a zero coordinate', () => {
154+
const row = renderReadonly({ latitude: 0, longitude: 120.1551 });
155+
expect(row.querySelector('button')?.textContent).toBe('View on map');
156+
});
157+
158+
it('editable: both buttons are offered, matching the non-zero control', () => {
159+
const outer = renderEditable({ latitude: 0, longitude: 120.1551 });
160+
const labels = Array.from(outer.querySelectorAll('button')).map((b) => b.textContent);
161+
expect(labels).toEqual(['Use Current Location', 'View on map']);
162+
});
163+
164+
it('the map opens on the real zero coordinate — `openInMaps` refused it too', () => {
165+
const opened = vi.spyOn(window, 'open').mockImplementation(() => null);
166+
const row = renderReadonly({ latitude: 0, longitude: 120.1551 });
167+
168+
fireEvent.click(row.querySelector('button') as HTMLButtonElement);
169+
170+
expect(opened).toHaveBeenCalledWith('https://www.google.com/maps?q=0,120.1551', '_blank');
171+
opened.mockRestore();
172+
});
173+
});
174+
175+
describe('objectui#8055 CONTROLS — what must NOT have moved', () => {
176+
it('⭐ CONTROL: the coordinate boxes already showed `0` and still do (`?? \'\'`)', () => {
177+
// The card measured this site as ALREADY CORRECT on `main`. A control that
178+
// goes red with the subject is not a control: this one proves the probe
179+
// reads a `0` out of this widget, and that the one guard that was right was
180+
// not "fixed" into consistency with the broken ones.
181+
const outer = renderEditable({ latitude: 0, longitude: 0 });
182+
const boxes = Array.from(outer.querySelectorAll('input[type="number"]'));
183+
expect(boxes.map((b) => (b as HTMLInputElement).value)).toEqual(['0', '0']);
184+
});
185+
186+
it('⭐ CONTROL: the non-zero render string is byte-identical to the card`s reading', () => {
187+
const row = renderReadonly({ latitude: 30.2741, longitude: 120.1551 });
188+
expect(row.textContent).toBe('30.274100, 120.155100View on map');
189+
});
190+
191+
it('CONTROL: a genuinely ABSENT coordinate still renders the empty placeholder', () => {
192+
const row = renderReadonly({});
193+
expect(row.textContent).toContain(EM_DASH);
194+
expect(row.querySelector('button')).toBeNull();
195+
});
196+
197+
it('CONTROL: one absent half is still no location — a half pair invents nothing', () => {
198+
const row = renderReadonly({ latitude: 0 });
199+
expect(row.textContent).toContain(EM_DASH);
200+
expect(row.querySelector('button')).toBeNull();
201+
});
202+
203+
it('CONTROL: `NaN` stays empty — the delta of this fix is exactly `0`', () => {
204+
// The presence predicate is nullish AND NaN-excluding, deliberately: a bare
205+
// `!= null` would have started rendering `"NaN, NaN"` at a surface that has
206+
// always shown the placeholder for an unreadable coordinate. Every input
207+
// other than `0` / `-0` answers exactly as it did before this card.
208+
const row = renderReadonly({ latitude: NaN, longitude: 120.1551 });
209+
expect(row.textContent).toContain(EM_DASH);
210+
expect(row.querySelector('button')).toBeNull();
211+
});
212+
});

‎packages/fields/src/widgets/GeolocationField.tsx‎

Lines changed: 64 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,65 @@ export interface GeolocationValue {
2121
accuracy?: number | null;
2222
}
2323

24+
/**
25+
* A `GeolocationValue` whose two coordinates are both readable numbers — what
26+
* {@link hasCoordinates} narrows to, so `formatLocation` can call `.toFixed`
27+
* without a second null check of its own.
28+
*/
29+
type LocatedGeolocation = GeolocationValue & { latitude: number; longitude: number };
30+
31+
/**
32+
* Is this numeric slot FILLED? (objectui#8055)
33+
*
34+
* ⛔ Deliberately NOT `!n`. Every guard in this widget used to ask the parsed
35+
* NUMBER whether it was falsy, and `0` is falsy — so `latitude: 0` (the
36+
* equator), `longitude: 0` (the prime meridian) and `accuracy: 0` were all read
37+
* as "no value". A record holding a real place displayed as the `EmptyValue`
38+
* em dash and its "View on map" affordance was withheld from it; the data was
39+
* intact and unreachable. Presence is a NULLISH question, never a falsy one.
40+
*
41+
* ⚠️ The delta this predicate deliberately keeps to EXACTLY `0` and `-0`:
42+
* `NaN` answers `false` here just as it answered falsy before. A bare
43+
* `n != null` would have been the card's wording taken literally, and it would
44+
* have started rendering `"NaN, NaN"` at a surface that has always shown the
45+
* empty placeholder for an unreadable coordinate — a second display defect
46+
* wearing the first one's fix. `Infinity` was admitted by the old guard and is
47+
* still admitted, for the same reason in the other direction: it is not this
48+
* card's value. Every input other than `0` / `-0` answers exactly as it did
49+
* before.
50+
*
51+
* ⭐ It returns a BOOLEAN, and that is the SECOND defect's fix, not a style
52+
* choice — see {@link hasCoordinates}.
53+
*/
54+
const isPresentNumber = (n: number | null | undefined): n is number =>
55+
typeof n === 'number' && !Number.isNaN(n);
56+
57+
/**
58+
* Does this value carry a coordinate PAIR? Both halves, both readable.
59+
*
60+
* ⭐ Boolean-valued on purpose, because three of its call sites are JSX render
61+
* expressions. The guard used to be written inline as
62+
* `{location.latitude && location.longitude && (…)}`, and with
63+
* `location.latitude === 0` that expression EVALUATES TO `0` — which React
64+
* renders as a text node. The user saw a stray `0` next to the em dash
65+
* (`"—0"`), the `&&`-with-a-numeric-operand hazard.
66+
*
67+
* ⛔ That is a genuinely different defect from the falsy presence test above,
68+
* and the fix is genuinely different too: making the presence test nullish
69+
* would still have left a numeric operand in front of `&&`, and making the
70+
* operand boolean would still have called the equator "no location". A
71+
* predicate that is nullish AND boolean is what closes both, and the class
72+
* criterion it answers to is: in this file no "has a value" test whose operand
73+
* may legitimately be `0` uses falsy semantics, and no JSX conditional carries
74+
* a numeric operand.
75+
*
76+
* ⛔ Not a tolerant fallback (AGENTS.md #0.1): it accepts nothing this widget
77+
* did not already accept in its `value` contract, it only stops MISREADING a
78+
* value that contract has always admitted.
79+
*/
80+
const hasCoordinates = (loc: GeolocationValue): loc is LocatedGeolocation =>
81+
isPresentNumber(loc.latitude) && isPresentNumber(loc.longitude);
82+
2483
/**
2584
* Geolocation field widget - provides a location picker with coordinates
2685
* Supports manual entry and browser geolocation API
@@ -150,12 +209,12 @@ export function GeolocationField({ value, onChange, field, readonly, error, ...p
150209
};
151210

152211
const formatLocation = (loc: GeolocationValue): string => {
153-
if (!loc.latitude || !loc.longitude) return '';
212+
if (!hasCoordinates(loc)) return '';
154213
return `${loc.latitude.toFixed(6)}, ${loc.longitude.toFixed(6)}`;
155214
};
156215

157216
const openInMaps = () => {
158-
if (!location.latitude || !location.longitude) return;
217+
if (!hasCoordinates(location)) return;
159218
const url = `https://www.google.com/maps?q=${location.latitude},${location.longitude}`;
160219
window.open(url, '_blank');
161220
};
@@ -171,7 +230,7 @@ export function GeolocationField({ value, onChange, field, readonly, error, ...p
171230
<div {...readonlyHostGroupProps} className="flex items-center gap-2">
172231
<MapPin className="w-4 h-4 text-muted-foreground" />
173232
{formatted ? <span className="text-sm">{formatted}</span> : <EmptyValue />}
174-
{location.latitude && location.longitude && (
233+
{hasCoordinates(location) && (
175234
<Button
176235
type="button"
177236
variant="link"
@@ -207,7 +266,7 @@ export function GeolocationField({ value, onChange, field, readonly, error, ...p
207266
<Crosshair className="w-4 h-4 mr-2" />
208267
{isLoading ? 'Getting location...' : 'Use Current Location'}
209268
</Button>
210-
{location.latitude && location.longitude && (
269+
{hasCoordinates(location) && (
211270
<Button
212271
type="button"
213272
variant="link"
@@ -265,7 +324,7 @@ export function GeolocationField({ value, onChange, field, readonly, error, ...p
265324
</div>
266325
</div>
267326

268-
{location.accuracy && (
327+
{isPresentNumber(location.accuracy) && (
269328
<p className="text-xs text-muted-foreground">
270329
Accuracy: ±{location.accuracy.toFixed(0)}m
271330
</p>

0 commit comments

Comments
 (0)