fix(review): strict coordinate parse, close the gate bypass, drop the stale pin
Code-review fixes to the location-override panel. parseFloat is a prefix parser, so '48abc', '48,85' (comma-decimal paste) and '35.0116S' (hemisphere suffix, silently flipped to the wrong side of the equator) all passed the isFinite + range check, cleared the mismatch flag and were POSTed verbatim — the same silent-corruption class this feature exists to end. Replaced with a whole-value decimal check, and the valid branch now normalises to the 6dp the GPS handler and onDragEnd already write, so the pin and the submitted value cannot disagree. The submit gate keyed on the .location-field--mismatch class, which was only ever set by syncFields() on blur / debounced input / toggle-open-success / GPS — never at init. Three paths therefore reached the server unflagged: a draft restored by initDraft() (which runs before initLocationDetails() and assigns .value directly, firing no events), an edit-mode async prefill, and an open panel whose maplibre chunk failed to load. syncFields() now runs once at the end of initLocationDetails(), in the toggle handler's catch arm, and after the edit prefill writes lat/lng. Blanking both fields left the marker behind, presenting a stale coordinate as if it were still the entry's — added clearPin() to the map handle and call it. Removed hasPin(), which had no caller. Corrected two comments that named the wrong stylesheet and claimed .field-invalid reuse the code does not do. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
File diff suppressed because one or more lines are too long
@@ -26,7 +26,7 @@ var cached = null; // { container, promise }
|
||||
|
||||
function buildPinElement() {
|
||||
var el = document.createElement('div');
|
||||
el.className = 'location-pin'; // visual styling lives in style.css
|
||||
el.className = 'location-pin'; // visual styling lives in post-form.css
|
||||
return el;
|
||||
}
|
||||
|
||||
@@ -58,7 +58,9 @@ export function getOrCreateLocationMap(container, onDragEnd) {
|
||||
if (!pinSet) { marker.addTo(map); pinSet = true; }
|
||||
map.panTo([lng, lat]);
|
||||
},
|
||||
hasPin: function () { return pinSet; },
|
||||
clearPin: function () {
|
||||
if (pinSet) { marker.remove(); pinSet = false; }
|
||||
},
|
||||
resize: function () { map.resize(); }
|
||||
};
|
||||
}).catch(function (err) {
|
||||
|
||||
@@ -522,9 +522,12 @@
|
||||
|
||||
/* "More location details" disclosure — search + map preview for setting an
|
||||
entry's coordinates without live GPS. Mirrors .more-options's disclosure
|
||||
look (above); the lat/lng fields (relocated here by JS) and the lookup
|
||||
button reuse style.css's existing .btn-action/.form-status/.field-invalid
|
||||
conventions unmodified. */
|
||||
look (above); the lookup button reuses style.css's existing .btn-action /
|
||||
.form-status conventions unmodified. The relocated lat/lng fields do NOT
|
||||
reuse .field-invalid — the panel adds its own .location-field--mismatch /
|
||||
.location-field-note pair below, because the mismatch state is advisory and
|
||||
carries live aria-invalid / aria-describedby wiring that showError() does
|
||||
not. Worth consolidating with .field-invalid if that ever gains the same. */
|
||||
.location-details {
|
||||
margin-bottom: var(--space-5);
|
||||
border: 1px solid var(--color-border);
|
||||
|
||||
@@ -607,12 +607,29 @@ function initLocationDetails() {
|
||||
// ── Map + sync (U4, U5, R9-R14, KTD1/KTD3/KTD4) ──
|
||||
var mapHandle = null;
|
||||
|
||||
// parseFloat is a PREFIX parser and must not be used here: '48abc' → 48,
|
||||
// '48,85' → 48 (comma-decimal paste), '35.0116S' → +35.0116 (hemisphere
|
||||
// suffix silently flipped to the wrong side of the equator). All three pass
|
||||
// an isFinite + range check, clear the flag, and post the original string —
|
||||
// the exact silent-corruption class this feature exists to end. Require the
|
||||
// whole value to be a plain decimal instead. Not bare Number(): Number('')
|
||||
// is 0, which would make a blank pair "valid" and pin it at Null Island.
|
||||
function parseCoord(v) {
|
||||
var s = String(v).trim();
|
||||
if (!s || !/^[+-]?(\d+(\.\d*)?|\.\d+)$/.test(s)) return NaN;
|
||||
return Number(s);
|
||||
}
|
||||
|
||||
function syncFields() {
|
||||
var lat = parseFloat(latEl.value);
|
||||
var lng = parseFloat(lngEl.value);
|
||||
var lat = parseCoord(latEl.value);
|
||||
var lng = parseCoord(lngEl.value);
|
||||
var valid = isFinite(lat) && isFinite(lng) &&
|
||||
lat >= -90 && lat <= 90 && lng >= -180 && lng <= 180;
|
||||
if (valid) {
|
||||
// Normalise to the same precision the GPS handler and onDragEnd
|
||||
// write, so what the pin shows is exactly what gets submitted.
|
||||
latEl.value = lat.toFixed(6);
|
||||
lngEl.value = lng.toFixed(6);
|
||||
clearMismatch();
|
||||
if (mapHandle) mapHandle.setPin(lat, lng);
|
||||
} else if (latEl.value.trim() || lngEl.value.trim()) {
|
||||
@@ -621,8 +638,10 @@ function initLocationDetails() {
|
||||
setMismatch();
|
||||
} else {
|
||||
// Both blanked back out after being flagged — nothing left to submit,
|
||||
// so the flag no longer applies.
|
||||
// so the flag no longer applies. Drop the pin too: leaving it behind
|
||||
// would present a stale coordinate as if it were still the entry's.
|
||||
clearMismatch();
|
||||
if (mapHandle) mapHandle.clearPin();
|
||||
}
|
||||
}
|
||||
syncPinFromFields = syncFields; // expose for initGeo()'s GPS handler
|
||||
@@ -643,6 +662,10 @@ function initLocationDetails() {
|
||||
syncFields();
|
||||
}).catch(function () {
|
||||
setHint('Map preview unavailable — you can still enter coordinates directly.');
|
||||
// Still validate: syncFields tolerates a null mapHandle, and without
|
||||
// this an offline traveller could open the panel and submit an
|
||||
// invalid coordinate that was never flagged.
|
||||
syncFields();
|
||||
});
|
||||
});
|
||||
|
||||
@@ -731,6 +754,14 @@ function initLocationDetails() {
|
||||
lookupBtn.textContent = LOOKUP_LABEL;
|
||||
});
|
||||
});
|
||||
|
||||
// Validate whatever is already in the fields at boot. initDraft() restores
|
||||
// data[lat]/data[lng] from localStorage before this runs, and edit-mode
|
||||
// prefills them asynchronously — both by direct .value assignment, which
|
||||
// fires no events. Without this the submit gate in initValidation (which
|
||||
// keys on .location-field--mismatch) sees no flag and lets a restored
|
||||
// invalid coordinate straight through.
|
||||
syncFields();
|
||||
}
|
||||
|
||||
/* ── Blocking required-field validation (U5, R19) ─────────────
|
||||
@@ -1418,6 +1449,11 @@ function initEditMode() {
|
||||
editSetContent(d.content);
|
||||
editSetText('lat', h.lat);
|
||||
editSetText('lng', h.lng);
|
||||
// Direct .value writes fire no events, so flag/pin state would stay
|
||||
// stale — and an entry stored with an out-of-range coordinate would
|
||||
// never be flagged. initLocationDetails() has already replaced the
|
||||
// no-op by the time this async prefill resolves.
|
||||
syncPinFromFields();
|
||||
editSetText('location_city', h.location_city);
|
||||
editSetText('location_country', h.location_country);
|
||||
editSetText('weather_desc', h.weather_desc);
|
||||
|
||||
Reference in New Issue
Block a user