docs(review): correct R8/R13 and the plan status; bump user pin to the review fixes
R8 and R13 both described behaviour that changed in review, and R13 rested on a server-side cleanCoordinate() that had never been committed. Both now describe what actually ships, with the revision called out inline rather than silently rewritten. The plan's Status keeps ✅ Complete but now records what the review changed and the two things still open before merge. Bumps the `user` gitlink to e873a9c (the review fixes). The submodule is deliberately NOT pushed: git-sync would propagate it to production. So this pin still references a commit that exists only locally — push `user/` and re-point before this branch merges. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -11,7 +11,7 @@ execution: code
|
|||||||
|
|
||||||
# Post Form Location Override - Plan
|
# Post Form Location Override - Plan
|
||||||
|
|
||||||
**Status:** ✅ Complete (2026-07-24)
|
**Status:** ✅ Complete (2026-07-24) — U1–U6 shipped, then hardened by a multi-agent code review the same day. The review found the design's stated server-side safety net (`cleanCoordinate()`) had never been committed, so it landed here; replaced a prefix-parsing coordinate check that accepted `48abc` / `48,85` / `35.0116S` (hemisphere silently flipped); closed three paths that bypassed the submit gate (draft restore, edit-mode prefill, map-load failure) because the gate read a CSS class no code set at init; added pin removal on blanked fields; made the geocode failure visible; and rewrote the U5 guard spec, which asserted only instantly-passing conditions and so could not fail. R8 and R13 above are revised accordingly. **Still open before merge:** the `user/` submodule commit is unpushed by choice (git-sync would deploy to prod), so the pin must be pushed and re-pointed at merge time; and the Playwright suite has never executed end-to-end (`make test-account` is broken by a shell-special value in `.env`), so every verification below is by inspection, not by a green run.
|
||||||
|
|
||||||
## Goal Capsule
|
## Goal Capsule
|
||||||
|
|
||||||
@@ -45,7 +45,7 @@ The only way to set a coordinate today is the GPS button (reads live position) o
|
|||||||
- R5. Lookup is explicit-click only. While in flight, the button shows a disabled "Searching…" state that always re-enables on response, no-match, or network failure.
|
- R5. Lookup is explicit-click only. While in flight, the button shows a disabled "Searching…" state that always re-enables on response, no-match, or network failure.
|
||||||
- R6. Clicking with both City and Country empty is treated as a no-match: an inline hint asks for a city or country first, and no request is sent.
|
- R6. Clicking with both City and Country empty is treated as a no-match: an inline hint asks for a city or country first, and no request is sent.
|
||||||
- R7. Multiple matches render as a clickable list (place name, admin region, country), built via `document.createElement` + `.textContent` (no `innerHTML`), matching every other dynamic-content construction already in `post-form.js`. Clicking an entry sets `lat`/`lng` and the pin only — it never writes back to City/Country. The list hides again until the next lookup.
|
- R7. Multiple matches render as a clickable list (place name, admin region, country), built via `document.createElement` + `.textContent` (no `innerHTML`), matching every other dynamic-content construction already in `post-form.js`. Clicking an entry sets `lat`/`lng` and the pin only — it never writes back to City/Country. The list hides again until the next lookup.
|
||||||
- R8. No matches renders an inline hint suggesting a country or manual pin drag; a network failure degrades silently (fields untouched), consistent with the existing reverse-geocode/weather error handling in `post-form.js`.
|
- R8. No matches renders an inline hint suggesting a country or manual pin drag; a network failure (or a non-2xx response) leaves the fields untouched and renders a *distinct* inline hint naming the connection as the problem. **Revised in code review 2026-07-24** from "degrades silently" — silence was indistinguishable from a broken button, and the two failure modes need different messages.
|
||||||
|
|
||||||
**Map preview & sync**
|
**Map preview & sync**
|
||||||
- R9. A single MapLibre GL map with one draggable marker (≥44×44px touch target) renders in the panel, reusing the site's existing style URL (`MAP_STYLE`, extracted to a shared `user/themes/intotheeast/js/src/map-style.js` module per KTD1). The map instance is created once, on the panel's first open, held in module scope, and reused (with an explicit `.resize()` call) on every subsequent open — the container sits under `display:none` while closed, so the first paint would otherwise get a zero-size canvas.
|
- R9. A single MapLibre GL map with one draggable marker (≥44×44px touch target) renders in the panel, reusing the site's existing style URL (`MAP_STYLE`, extracted to a shared `user/themes/intotheeast/js/src/map-style.js` module per KTD1). The map instance is created once, on the panel's first open, held in module scope, and reused (with an explicit `.resize()` call) on every subsequent open — the container sits under `display:none` while closed, so the first paint would otherwise get a zero-size canvas.
|
||||||
@@ -54,7 +54,7 @@ The only way to set a coordinate today is the GPS button (reads live position) o
|
|||||||
- R12. No pin is shown until one of the four paths above sets a value for the first time.
|
- R12. No pin is shown until one of the four paths above sets a value for the first time.
|
||||||
|
|
||||||
**Error handling & validation boundary**
|
**Error handling & validation boundary**
|
||||||
- R13. Invalid manual `lat`/`lng` text is never client-blocked — the visual mismatch flag (R11) is the only feedback. Final enforcement stays server-side in `cleanCoordinate()`, which already throws on a non-blank, still-invalid value after cleaning.
|
- R13. Invalid manual `lat`/`lng` text raises the visual mismatch flag (R11), **and** an unresolved flag blocks submit. **Revised in code review 2026-07-24** from "never client-blocked". The original wording deferred all enforcement to a server-side `cleanCoordinate()` described as already shipped — it was not committed anywhere, so no layer validated coordinates. It now ships in `cache-on-save.php` (both the `/post` form and the Admin2/API save paths) and the client gate stays, giving real defence in depth. The client parse is intentionally stricter than the server's `is_numeric` (whole-value decimals only, so `48,85` / `35.0116S` / `48abc` are rejected rather than prefix-parsed).
|
||||||
- R14. Geolocation permission denial keeps its existing, unmodified `#location-status` error behavior.
|
- R14. Geolocation permission denial keeps its existing, unmodified `#location-status` error behavior.
|
||||||
|
|
||||||
### Scope Boundaries
|
### Scope Boundaries
|
||||||
|
|||||||
@@ -64,8 +64,8 @@ Backend sanitization has already been added (`user/plugins/cache-on-save/cache-o
|
|||||||
### Error handling
|
### Error handling
|
||||||
|
|
||||||
- No search results: inline message under the search box, map/pin untouched.
|
- No search results: inline message under the search box, map/pin untouched.
|
||||||
- Search network failure: silent-ish degrade (consistent with existing weather/reverse-geocode error handling in `post-form.js`), fields untouched.
|
- Search network failure: fields untouched, and an inline hint says the lookup service could not be reached (distinct from the no-results message, which means the service answered). **Revised in code review 2026-07-24** — this originally said "silent-ish degrade", which in practice left the DOM byte-identical to the pre-click state, so a traveller on flaky mobile data could not tell a failed lookup from a broken button. A non-2xx response is also now treated as a failure rather than parsed as an empty result set.
|
||||||
- Invalid manual `lat`/`lng` text: no client-side hard block (the map preview and eventual server-side `cleanCoordinate()` are the safety nets); this UI's whole point is to make that failure mode rare in practice, not to duplicate the backend validator client-side.
|
- Invalid manual `lat`/`lng` text: the visual mismatch flag is the primary feedback, **and** an unresolved flag blocks submit. **Revised in code review 2026-07-24** — this originally said "no client-side hard block", on the stated grounds that server-side `cleanCoordinate()` was already the safety net. It was not: `cleanCoordinate()` had never been committed, so nothing validated coordinates anywhere. It now ships (`cache-on-save.php`, both the `/post` and Admin2 paths), so the two are genuine defence in depth rather than one imaginary net. Client-side parsing is deliberately *stricter* than the server's `is_numeric` (whole-value decimals only), which is the safe direction for a mismatch.
|
||||||
- Geolocation permission denied: unchanged existing behavior (`#location-status` error message).
|
- Geolocation permission denied: unchanged existing behavior (`#location-status` error message).
|
||||||
|
|
||||||
## Out of scope / explicitly deferred
|
## Out of scope / explicitly deferred
|
||||||
|
|||||||
+1
-1
Submodule user updated: 13c76b29a8...e873a9cb23
Reference in New Issue
Block a user