m038andClaude Opus 5
3250ad366a
Merged state recorded (user/ dd19995, outer 4450bd6, pin bumped).
The "owner_username cluster" was wrong: on merged main only DEL4 fails, with
identical site.yaml and content, so auth was never the cause. The worktree's
extra five failures came from its incomplete git-ignored user/plugins/ set.
DEL4 itself is real and stays open — deleting an entry removes it from the
DOM and from disk, but a fresh trip-page load makes the server re-emit the
card, the same invalidation bug the spec's header says was fixed once before.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
255 lines
38 KiB
Markdown
255 lines
38 KiB
Markdown
---
|
||
title: Post Form Location Override - Plan
|
||
type: feat
|
||
date: 2026-07-23
|
||
origin: docs/working/specs/2026-07-23-post-form-location-override-design.md
|
||
artifact_contract: ce-unified-plan/v1
|
||
artifact_readiness: implementation-ready
|
||
product_contract_source: legacy-requirements
|
||
execution: code
|
||
---
|
||
|
||
# Post Form Location Override - Plan
|
||
|
||
**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.
|
||
|
||
**Verified by a green run (2026-07-24).** The suite now executes end-to-end: `test-config` 22/22, `test-post` 6/6 (the `scripts/test-post.sh` shell suite — *not* the Playwright specs under `tests/ui/post/`, which is a separate set), and `location-override.spec.js` **20/20** — so the verifications below are no longer by inspection alone. Reaching that took fixing `make test-account` (the password was interpolated into an `sh -c` string, so a shell metacharacter in it killed every UI run), pinning `test-ui` to this checkout's own port, and repairing test cleanup, which had never been able to delete the root-owned entries Grav's Apache creates. See the commit `fix(test): close the test-entry leak into real trip content`.
|
||
|
||
Also landed after the review: maplibre's stylesheet is now lazy-`<link>`ed at panel-open instead of statically bundled, cutting `post-form.css` from 92,244 to 26,784 raw bytes (14,528 → 5,631 gzip) on every `/post` load, with a new spec asserting both halves of that boundary.
|
||
|
||
**Merged to `main` 2026-07-24** — `user/` at `dd19995`, outer at `4450bd6`, pin bumped. On merged `main`: `test-config` **22/22** and `tests/ui/post/` + `tests/ui/map` **69 passed / 1 failed** (DEL4 only, a pre-existing regression unrelated to this feature — see below). `user/` is still **unpushed by choice**; push `user/` first, then the outer repo.
|
||
|
||
**Notes carried forward:**
|
||
- The `user/` submodule commits remain **unpushed by choice** (git-sync would deploy to prod). Merged to `main` locally on 2026-07-24 and the pin bumped; pushing `user/` — then the outer repo, in that order — is the remaining step and is deliberately left to the user to time.
|
||
- **DEL4 is a real, pre-existing regression and the one thing still red on `main`** (`tests/ui/post/delete-flow.spec.js:44`, reproducible in isolation). Deleting an entry works: the card leaves the DOM and the folder leaves disk (both asserted and both pass). But a fresh load of the trip page makes the server re-emit the card — an image-less ghost of a page whose content is gone. That is precisely the bug the spec's own header says was already fixed once, so the invalidation has regressed. `cache-on-save` clears the page-tree cache on form *submit*; the delete path evidently does not do the equivalent. Practical impact: delete a bad post from the road, reload, and it is back. Worth its own branch.
|
||
- ~~This worktree's `user/` branch has diverged from `user/`'s `main`~~ **Done** — `user/main` merged in (`7903432`). It was ahead on both content and theme fixes; `denmark-2026 published: true` came with it, so the local testing flip is gone. The one conflict was `js/post/post-form.js`, a generated bundle, resolved by rebuilding rather than hand-merging minified output.
|
||
- ~~The `~/Projects` clone's `user/` carries two commits this clone cannot see~~ **Done** — merged in (`8a5cc52`). There is no second clone: `~/Projects` is a symlink to `~/Nextcloud/Projects`. What differs is the **submodule git dir** — a worktree gets `.git/worktrees/<name>/modules/user`, not the checkout's `.git/modules/user` — so `user/main` read `4721af6` here while the checkout's read `285ae37`, and the leg-connection map fix and U+200E strip were unreachable until a local `git fetch` between the two paths. Worth remembering: submodule commits made from the main checkout do not appear in a worktree until fetched, and a local fetch carries them without a push, so git-sync never fires.
|
||
- **Retracted: the "`owner_username` cluster" diagnosis was wrong.** The worktree showed 6 failures (AN2, DEL1–4, ES1) and they were attributed to `site.yaml` pinning `owner_username: mischa` while the suite authenticates as `testrunner`. On merged `main` only DEL4 fails, with byte-identical `site.yaml` and content — so auth was not the cause. The difference is environmental: the isolated worktree's `user/plugins/` was incomplete (missing `admin`, `markdown-notices`, `migrate-grav`, since `plugins/` is git-ignored and populated per-checkout by `make install-plugins`). Lesson: treat a worktree's UI failures as suspect until reproduced in the main checkout, because the worktree's plugin set is not guaranteed to match.
|
||
- ~~Every `make` target aborts with `.env:6: *** missing separator`~~ **Fixed by the user (2026-07-24)** — `make` now parses in the checkout. Worth keeping in mind: the env layering is intentional (`.env` global, `-include .env.$(ENV)` per-environment, `ENV` set by the generated env-suffixed remote targets like `make remote-install-prod`), but because `.env` is pulled in with `-include` it must be valid **makefile** syntax as well as valid dotenv — so a leading tab, a multi-line value, or a line without `=` takes down every target at once. Worktrees mask it, since `worktree-new` creates no `.env` and the include silently skips.
|
||
- **UG1, UG2 and LD1 under `tests/ui/post/` now pass** — they had been failing only because this branch predated `e17a5dc` ("block submit on unfinished photo uploads; un-squeeze EXIF portraits in lightbox"). Merging `user/main` in brought the upload gate and the oriented-derivative slide dims those specs assert, and all three went green with no product change. A first pass mistook them for live defects; the lesson is to check the submodule branch point before reading a red spec on a feature branch as a real bug.
|
||
|
||
## Goal Capsule
|
||
|
||
- **Objective:** Give the traveller a visual, mistake-catching way to set a journal entry's coordinates for a place other than their current GPS position — via a search-by-city lookup and a draggable map pin inside a new "More location details" disclosure on `/post` — without touching Admin2 or the API plugin.
|
||
- **Authority hierarchy:** The design doc (`docs/working/specs/2026-07-23-post-form-location-override-design.md`) is authoritative for behavior; this plan is authoritative for sequencing and file-level implementation. Repo conventions (`CLAUDE.md`) and the cited existing patterns override any incidental detail here.
|
||
- **Stop conditions:** Surface a blocker if the Open-Meteo geocoding endpoint's CORS or city-only-query behavior no longer matches what the design doc verified live, or if lazy-importing `maplibre-gl` breaks `post-form.js`'s existing ESM code-splitting build (the same risk the `heic-to` lazy import already carries safely).
|
||
- **Execution profile:** Standard frontend feature confined to one theme (templates untouched — the panel is built entirely in JS, mirroring the existing "More options" pattern): CSS, JS additions to `post-form.js` plus one new small module, and a best-effort Playwright spec. Test-after is fine for the JS/CSS units; the Playwright unit (U6) is written test-after against the finished behavior.
|
||
- **Tail ownership:** Rebuild theme assets (`make build-assets`) after U1–U5; run manual QA per the Definition of Done regardless of whether U6 can execute locally.
|
||
|
||
---
|
||
|
||
## Product Contract
|
||
|
||
### Summary
|
||
|
||
Add a closed-by-default "More location details" disclosure to the `/post` form, placed directly below the City/Country fields. It holds a "🔍 Look up coordinates" button (geocodes the City field via Open-Meteo, ranked by Country when filled), a single-marker MapLibre preview map, and the existing `lat`/`lng` text fields relocated out of their current CSS-hidden position. Four ways to set a coordinate — GPS button, search-result pick, dragging the pin, typing raw numbers — stay in sync with each other. The GPS button's placement and behavior, and the City/Country fields' auto-fill-when-blank behavior, are unchanged.
|
||
|
||
### Problem Frame
|
||
|
||
The only way to set a coordinate today is the GPS button (reads live position) or hand-typing/pasting raw decimal text into a CSS-hidden field — the latter is how an invisible Unicode bidi mark silently zeroed out a Denmark 2026 entry's coordinates before backend sanitization (`cleanCoordinate()` in `user/plugins/cache-on-save/cache-on-save.php`) was added. That backend fix stops silent corruption but does nothing for the underlying gap: there's still no visual, reliable way to set a location other than "here, right now," and no way to confirm a coordinate looks right before submitting. This plan closes that gap on the frontend only.
|
||
|
||
### Requirements
|
||
|
||
**Disclosure & field relocation**
|
||
- R1. A new "More location details" `<details>` panel exists, closed by default, positioned directly after the City/Country fields — a separate disclosure from the existing "More options" advanced-fields panel (`initDisclosure()` in `user/themes/intotheeast/js/src/post-form.js:341`).
|
||
- R2. The `lat`/`lng` fields relocate into this panel with their `name="data[lat]"`/`name="data[lng]"` attributes unchanged, so `cache-on-save.php`'s `sanitizeCoordinates()` and `post-form.js`'s `field('lat')`/`field('lng')` helper keep working unmodified. The CSS rule hiding them (`user/themes/intotheeast/css/style.css:893-895`) is removed.
|
||
- R3. The GPS button (`#get-location`) and City/Country fields keep their current position and behavior in the main flow.
|
||
|
||
**Search**
|
||
- R4. "🔍 Look up coordinates" queries Open-Meteo's geocoding endpoint (`https://geocoding-api.open-meteo.com/v1/search?name=<city>&count=10&language=en&format=json`) by the City field alone — never concatenating Country into the query string, since that returns zero results or a silently degraded match. When Country is non-blank, results are ranked client-side by a case-insensitive substring match against each result's `country` field, matches first; all results still render.
|
||
- 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.
|
||
- 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 (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**
|
||
- 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.
|
||
- R10. `maplibre-gl`'s JS is dynamically imported only when the panel is opened for the first time, mirroring the existing `heic-to` lazy-chunk pattern (`user/themes/intotheeast/js/src/post-form.js:281`) so ordinary GPS-only submits never fetch it. Its CSS is imported statically at the top of `post-form.js` and bundled unconditionally into `post-form.css`, since a dynamically-imported chunk's CSS is never linked automatically.
|
||
- R11. Four coordinate-setting paths stay mutually in sync: the GPS button (updates the pin live if the panel is already open, otherwise the pin reflects the new value whenever the panel is next opened); a search-result click; dragging the pin (`dragend` writes back to the fields, rounded to 6 decimal places, matching the GPS button's existing precision); and typing directly into the fields (on blur/debounced input, a valid in-range pair moves the pin; an unparseable or out-of-range value leaves the pin alone and visually flags the field until it parses again).
|
||
- R12. No pin is shown until one of the four paths above sets a value for the first time.
|
||
|
||
**Error handling & validation boundary**
|
||
- 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.
|
||
|
||
### Scope Boundaries
|
||
|
||
**Out of scope**
|
||
- Any change to `user/plugins/admin2/` or `user/plugins/api/`.
|
||
- Any change to how coordinates are stored (still plain `lat`/`lng` floats in frontmatter) or to the already-shipped `cleanCoordinate()` sanitization.
|
||
- Offline/self-hosted geocoding, or integrity verification (pinning, response signing) for the third-party geocoding/tile responses beyond HTTPS.
|
||
|
||
**Deferred to Follow-Up Work**
|
||
- If the pre-existing `make test-account` Makefile quoting bug still blocks running the Playwright suite locally when U6 lands, fixing that bug is separate follow-up work, not part of this plan — U6's spec file is written and committed regardless, and manual QA is the accepted completion gate in the meantime.
|
||
|
||
---
|
||
|
||
## Planning Contract
|
||
|
||
### Key Technical Decisions
|
||
|
||
- KTD1. **A new dedicated map module, not an extension of `initEntryMap`.** `js/maplibre-utils.js`'s `initEntryMap` (used by `entry-map.html.twig` on the trip/home pages) is built for multi-marker, GPX-drawing, popup-bearing read-only maps — none of which this single-draggable-pin preview needs. Add a small new sibling source module, `user/themes/intotheeast/js/src/location-map.js`, imported statically by `post-form.js` (it is not a new esbuild entry point — see KTD5). `MAP_STYLE` itself is extracted into a tiny shared constants module, `user/themes/intotheeast/js/src/map-style.js` (a single `export const MAP_STYLE = ...`, no side effects), imported by both `location-map.js` and the existing `js/maplibre-utils.js` — this removes the literal-duplication drift risk without pulling in `maplibre-utils.js`'s whole multi-marker/GPX machinery or its window-global side effect, since the new module has neither.
|
||
- KTD2. **Search: city-only query + client-side country ranking**, exactly as verified live in the design doc — concatenating Country into the query string breaks the "Paris, Texas" disambiguation case this feature exists for.
|
||
- KTD3. **Lazy-load boundary.** `location-map.js` exports a function (e.g. `getOrCreateLocationMap(container)`) that internally calls `import('maplibre-gl')` the first time it runs, keyed off the panel's first `toggle` event where `details.open === true` — never eagerly at page load. `maplibre-gl/dist/maplibre-gl.css` is a static top-of-file import in `post-form.js` (the JS/CSS split from R10) since esbuild never emits a `<link>` for a code-split CSS chunk.
|
||
- KTD4. **Two small sync helpers, not four independent write paths.** `syncPinFromFields()` (fields → pin: reads `field('lat')`/`field('lng')`, moves the pin if both parse as finite in-range numbers, else sets the mismatch flag on the offending field without touching the pin) is called from the search-result click, from the GPS button's success handler when the panel is already open, from the lat/lng fields' blur/debounced-input listeners, and from the panel's `toggle`-open handler (U4) so a pin set while the panel was closed — via GPS capture, or pre-existing coordinates in edit mode — renders correctly the first time the panel opens. The marker's `dragend` handler writes straight into the fields (rounded to 6 decimals) and clears any mismatch flag — it does not call `syncPinFromFields()` back, avoiding a feedback loop.
|
||
- KTD5. **No new esbuild entry point.** Unlike `trip-publish.js` (its own bundle), `location-map.js` is a plain ES module imported by `post-form.js`'s existing entry — esbuild inlines it into the same `--splitting` ESM build already configured in `user/themes/intotheeast/package.json`. Only `maplibre-gl` itself needs to be the lazy chunk; the coordinator code around it loads normally, mirroring how `heic-to` is dynamically imported from directly inside the always-loaded `post-form.js`.
|
||
- KTD6. **Panel construction is entirely JS-built, no template edit.** Mirrors `initDisclosure()` (line 341) and the photos `<details>` wrapper (line ~120): a new `initLocationDetails()` creates the `<details>`/`<summary>`, the search button/results-list/hint elements, and the map container via `document.createElement`, then moves the existing `lat`/`lng` `.form-field` wrappers into it — the same relocate-via-JS approach already used for "More options," so `post-form.html.twig` needs no structural change (only the CSS hide-rule removal in R2).
|
||
|
||
### High-Level Technical Design
|
||
|
||
```mermaid
|
||
flowchart TB
|
||
GPS["GPS button success\n(if panel open)"] --> SYNC["syncPinFromFields()"]
|
||
SEARCH["Search result click"] --> FIELDS["lat/lng fields"]
|
||
FIELDS --> SYNC
|
||
TYPE["Type + blur/debounce"] --> SYNC
|
||
SYNC --> PIN["Map pin"]
|
||
DRAG["Drag pin (dragend)"] --> FIELDS
|
||
SYNC -.invalid.-> FLAG["Mismatch flag on field\n(cleared once value parses)"]
|
||
```
|
||
|
||
Map lifecycle: first panel open → `import('maplibre-gl')` → create map + draggable marker, cache in module scope → subsequent opens call `.resize()` on the cached instance rather than recreating it.
|
||
|
||
### Assumptions
|
||
|
||
- No existing Playwright fixture creates a "search API returns N results" scenario; U6 mocks the Open-Meteo response via `page.route()` rather than depending on the live third-party endpoint, keeping the spec hermetic (and avoiding flakiness/rate-limits from a real geocoding call).
|
||
- The `location-details` panel defaults closed even when editing an entry that already has `lat`/`lng` set — see Open Questions.
|
||
|
||
---
|
||
|
||
## Implementation Units
|
||
|
||
### U1. CSS: unhide coordinate fields, style the new panel
|
||
|
||
- **Goal:** Remove the CSS rule hiding `lat`/`lng`, and add styling for the new disclosure, search results list, map container, and mismatch-flag state (R2, R9).
|
||
- **Requirements:** R2, R9.
|
||
- **Dependencies:** none.
|
||
- **Files:** `user/themes/intotheeast/css/style.css`.
|
||
- **Approach:** Remove the `display: none !important` rule at `style.css:893-895` targeting `input[name="data[lat]"]`/`input[name="data[lng]"]`. Add: a `.location-details` disclosure look mirroring `.more-options` (`user/themes/intotheeast/js/src/post-form.css:98`); a `.location-search-results` list; a `.location-map` container with a fixed height and `position: relative` so the marker's DOM element (sized ≥44×44px) sits correctly; a `.location-field--mismatch` state (red outline + inline note) for the type-mismatch flag; a disabled/"Searching…" look for the lookup button reusing the existing `.btn-action`/`is-loading` conventions (`style.css:976-991`).
|
||
- **Patterns to follow:** `.more-options`/`.more-options__summary` (`post-form.css:98-128`), `.btn-action`/`.form-status` (`style.css:970-1002`).
|
||
- **Test scenarios:** Test expectation: none -- pure CSS; visual correctness is verified manually and indirectly by U2–U5's behavioral tests (elements exist and are visible/hidden as expected).
|
||
- **Verification:** `lat`/`lng` inputs are visible only inside the new panel in the browser; no other page references the removed selector (confirmed during research — none found outside `style.css:894-895` and `post-form.js`'s own field reads).
|
||
|
||
### U2. JS: build the "More location details" panel shell
|
||
|
||
- **Goal:** Construct the closed-by-default disclosure (search UI, map container, relocated `lat`/`lng` fields) entirely in JS, positioned after the City/Country fields (R1, R2, R3, KTD6).
|
||
- **Requirements:** R1, R2, R3.
|
||
- **Dependencies:** U1.
|
||
- **Files:** `user/themes/intotheeast/js/src/post-form.js`.
|
||
- **Approach:** New `initLocationDetails()`, called from `boot()` after `initDisclosure()` and `initGeo()` (so the relocated fields already reflect any `initDraft()` restore, and `initGeo()`'s `field('lat')`/`field('lng')` lookups still resolve by attribute selector regardless of DOM position). No-op if `field('lat')`/`field('lng')` are absent. Create `<details class="location-details">` + `<summary>More location details</summary>`; append a search row (`#lookup-coords` button, `#location-search-results` list, `#location-search-hint` inline hint), a `#location-map` container, then move `field('lat').closest('.form-field')` and `field('lng').closest('.form-field')` into the details. Insert the details element immediately after `field('location_country').closest('.form-field')`.
|
||
- **Patterns to follow:** `initDisclosure()` (`post-form.js:341`) and the photos `<details>` wrapper (`post-form.js:~120`) for the create-via-JS + relocate-wrapper approach.
|
||
- **Test scenarios:**
|
||
- Happy path: on `/post`, "More location details" is present, closed by default, positioned immediately after the Country field, and contains the lookup button, an empty map container, and the (now-visible-only-inside-the-panel) `lat`/`lng` inputs.
|
||
- No-op guard: if `lat`/`lng` fields were ever absent from the DOM, `initLocationDetails()` does not throw.
|
||
- **Verification:** DOM inspection in-browser confirms structure and default-closed state.
|
||
|
||
### U3. JS: geocoding search + results list
|
||
|
||
- **Goal:** Implement the "🔍 Look up coordinates" button: city-only query, client-side country ranking, results list, and all error/empty states (R4–R8).
|
||
- **Requirements:** R4, R5, R6, R7, R8.
|
||
- **Dependencies:** U2.
|
||
- **Files:** `user/themes/intotheeast/js/src/post-form.js`.
|
||
- **Approach:** Click handler on `#lookup-coords`: if City and Country are both blank, show the inline hint and return (no fetch). Otherwise disable the button, show "Searching…", and `fetch` the Open-Meteo geocoding URL (KTD2). On response: empty/missing `results` → no-match hint; otherwise stable-sort by whether each result's `country` case-insensitively contains the Country field's value (matches first, original order preserved otherwise), then render each as an `<li>` containing a `<button type="button">` built via `createElement`/`.textContent` ("name, admin1, country") — keyboard-operable by default, matching the accessible-button convention already used elsewhere in this file (the photo-delete button's `aria-label`). Clicking (or activating via keyboard) a result button sets `lat`/`lng` (not City/Country) and calls `syncPinFromFields()` (U5); the list then hides until the next lookup. Network failure: catch, degrade silently (matching the existing reverse-geocode/weather pattern), re-enable the button in both the success and failure paths.
|
||
- **Patterns to follow:** `reverseGeocode()`/`initGeo()`'s fetch + status-state handling (`post-form.js:433-511`) for the request/error shape; the "no `innerHTML` anywhere in this file" convention for the results list; the existing accessible-button convention (photo-delete `<button>` with `aria-label`) for keyboard-operable dynamically-created controls.
|
||
- **Test scenarios:**
|
||
- Happy path: searching "Kyoto" (mocked response) renders a results list; clicking the first result sets `lat`/`lng` and leaves City/Country untouched.
|
||
- Disambiguation: City "Paris", Country "Texas" (mocked multi-result payload matching the design doc's real API shape) — the Texas-tagged result renders first in the list.
|
||
- No match: mocked empty-results response shows the inline no-match hint; pin/fields untouched.
|
||
- Empty inputs: clicking lookup with City and Country both blank shows the hint and triggers no network request.
|
||
- In-flight state: a deliberately delayed mocked response shows the disabled "Searching…" button state until it resolves.
|
||
- Network failure: a mocked rejected/failed request degrades silently, leaves fields untouched, and re-enables the button.
|
||
- XSS safety: a mocked result containing markup in its name field (e.g. `<img onerror=...>`) renders as literal text in the list, not executed.
|
||
- **Verification:** All scenarios above pass in the browser against mocked responses; the live-API disambiguation case (Paris/Texas) is additionally spot-checked once manually per the Definition of Done.
|
||
|
||
### U4. JS: MapLibre preview module (lazy load, draggable marker, singleton)
|
||
|
||
- **Goal:** Implement the single-marker preview map as a dedicated module: lazy-imported on first panel open, reused (not recreated) on subsequent opens, with a resize fix for the zero-size-canvas-while-closed issue (R9, R10, R12, KTD1, KTD3, KTD5).
|
||
- **Requirements:** R9, R10, R12.
|
||
- **Dependencies:** U2.
|
||
- **Files:** `user/themes/intotheeast/js/src/map-style.js` (new), `user/themes/intotheeast/js/src/location-map.js` (new), `user/themes/intotheeast/js/src/post-form.js`, `user/themes/intotheeast/js/maplibre-utils.js` (modified — import `MAP_STYLE` instead of declaring it inline; no behavior change).
|
||
- **Approach:** First, extract the existing `MAP_STYLE` literal out of `maplibre-utils.js:5` into `map-style.js` (a single `export const MAP_STYLE = ...`) and update `maplibre-utils.js` to import it instead of declaring it inline. In `location-map.js`, import the same constant and export `getOrCreateLocationMap(container, onDragEnd)`: on first call, `import('maplibre-gl')`, create a `maplibregl.Map` against `container` using the shared `MAP_STYLE` constant (KTD1), create one `maplibregl.Marker({ draggable: true, element: <a ≥44×44px sized div> })` (not yet added to the map until a pin is set), wire its `dragend` to call `onDragEnd(lngLat)`, and cache the created map/marker in module scope keyed by container so a second call reuses them. Return a handle: `{ setPin(lat, lng), hasPin(), resize() }`. `post-form.js` adds a static top-of-file `import 'maplibre-gl/dist/maplibre-gl.css';` (R10) and, in `initLocationDetails()`, listens for the panel's `toggle` event: on every open where `details.open` is true, call `getOrCreateLocationMap(...).resize()` (creating it on the first call, per the lazy-import contract) and then `syncPinFromFields()` (U5), so a pin set while the panel was closed — via GPS capture, or pre-existing coordinates in edit mode — renders on this first paint.
|
||
- **Patterns to follow:** the `heic-to` dynamic-import shape (`post-form.js:281`) for the lazy-load mechanics; `js/maplibre-utils.js:452` (`new maplibregl.Map({...})`) and `:508` (`new maplibregl.Marker(...)`) for the underlying MapLibre API shape, without importing that file (KTD1).
|
||
- **Test scenarios:**
|
||
- Happy path: opening the panel for the first time renders exactly one MapLibre canvas inside `#location-map`.
|
||
- No initial pin: with `lat`/`lng` both empty, opening the panel shows no marker.
|
||
- Reopen does not duplicate: closing and reopening the panel (repeatedly) leaves exactly one canvas element, and the canvas has non-zero width/height after the reopen (guards the zero-size-while-closed case).
|
||
- Lazy import boundary: an ordinary GPS-only submit where the panel is never opened triggers no network request for the `maplibre-gl` chunk (asserted via a page network-request listener in Playwright).
|
||
- **Verification:** Browser + Playwright network-tab assertion confirm the chunk fetches once (not per-reopen) and never fetches when the panel stays closed.
|
||
|
||
### U5. JS: four-way coordinate sync + mismatch flag
|
||
|
||
- **Goal:** Keep the GPS button, search picks, pin drag, and typed values mutually in sync in both directions, including the visual mismatch flag for unparseable typed input (R11, R13, R14, KTD4).
|
||
- **Requirements:** R11, R13, R14.
|
||
- **Dependencies:** U3, U4.
|
||
- **Files:** `user/themes/intotheeast/js/src/post-form.js`.
|
||
- **Approach:** Implement `syncPinFromFields()` (KTD4): parse `field('lat')`/`field('lng')` values; if both are finite numbers within range, call the map handle's `setPin`, clear the mismatch flag/class from both fields, and clear `aria-invalid`/`aria-describedby`; if either fails to parse or is out of range, leave the pin untouched and add the mismatch flag/class (plus an inline "not reflected on map" note, rendered in a `role="status"`/`aria-live="polite"` element mirroring the existing dynamic-feedback pattern used elsewhere in this file, e.g. `#location-status`) to the offending field(s), setting `aria-invalid="true"` and `aria-describedby` pointing at that note so screen-reader users are told the value wasn't reflected on the map. Wire callers: (a) the marker's `dragend` (from U4's `onDragEnd`) writes rounded-to-6-decimal values directly into the fields and clears the mismatch flag — it does not call `syncPinFromFields()` back; (b) the search-result click (U3) sets fields then calls `syncPinFromFields()`; (c) the existing GPS success handler (`initGeo()`, `post-form.js:464-475`) calls `syncPinFromFields()` after setting fields, but only if the location-details `<details>` is currently open; (d) `lat`/`lng` field `blur` and debounced `input` listeners call `syncPinFromFields()`; (e) the panel's `toggle`-open handler (U4) calls `syncPinFromFields()` on every open, so a pin set while the panel was closed — covering the case (c) doesn't, and edit-mode entries with pre-existing coordinates — renders correctly on first paint.
|
||
- **Patterns to follow:** the GPS button's existing `toFixed(6)` rounding (`post-form.js:465-466`) for consistency; `setStatus()`'s idle/loading/success/error class pattern (`post-form.js:397`) as a model for the mismatch-flag class toggling.
|
||
- **Test scenarios:**
|
||
- GPS-first-then-open: capture GPS coordinates, then open the panel — the pin appears at the GPS coordinates on first paint.
|
||
- GPS-while-open: open the panel first, then click the GPS button — the pin updates live without needing to reopen the panel.
|
||
- Drag updates fields: dragging the marker to a new position updates `lat`/`lng` to the rounded 6-decimal values matching the drop location (within a small tolerance).
|
||
- Type valid values: typing a valid in-range pair and blurring moves the pin and shows no mismatch flag.
|
||
- Type invalid values: typing a non-numeric or out-of-range value and blurring leaves the pin in place and shows the mismatch flag; a subsequent valid edit clears the flag and moves the pin.
|
||
- Search doesn't clobber City/Country: after a search-result click, the City/Country field values are unchanged from what the traveller typed, even if the matched place's name differs in spelling/case.
|
||
- **Verification:** All six scenarios pass in the browser; a submit with a search-selected location round-trips through the existing backend `cleanCoordinate()` and produces the expected saved `lat`/`lng`.
|
||
|
||
### U6. Playwright coverage (best-effort)
|
||
|
||
- **Goal:** Add automated coverage for the new search → pin → submit flow, accepting the known local test-harness risk (R4–R14 as observable behavior).
|
||
- **Requirements:** R4, R5, R6, R7, R8, R9, R10, R11, R12, R13.
|
||
- **Dependencies:** U1–U5.
|
||
- **Files:** `tests/ui/post/location-override.spec.js` (new).
|
||
- **Approach:** Mock the Open-Meteo geocoding endpoint via `page.route()` so the suite is hermetic and doesn't depend on the live third-party API or rate limits. Cover: panel closed by default; empty-input lookup click sends no request and shows the hint; a mocked multi-result search sets `lat`/`lng` from a clicked result without touching City/Country; the Paris/Texas ranking case (mocked payload mirroring the design doc's verified real-API shape) renders the Texas-tagged result first; dragging the marker (Playwright mouse API) updates the fields; typing invalid values shows the mismatch flag without crashing; reopening the panel a second time leaves exactly one map canvas; a full submit with a search-picked location saves the expected `lat`/`lng` in the entry's frontmatter (reuse the existing fixture/cleanup helpers from `tests/ui/post/post.spec.js`).
|
||
- **Patterns to follow:** `tests/ui/post/post-form-ux.spec.js` (R18's `#get-location`/geolocation-mocking spec, line 186) for the geolocation/location-status assertions; `tests/ui/post/post.spec.js` for entry fixture creation, submit, and on-disk frontmatter assertions.
|
||
- **Test scenarios:** the bullet list under Approach is the scenario list.
|
||
- **Verification:** `npm run test:ui -- tests/ui/post/location-override.spec.js` (from `tests/`) passes. **Known risk:** the pre-existing, unrelated `make test-account` Makefile quoting bug may still block running the Playwright suite locally when this unit lands — if so, the spec file is still committed correct-and-ready, and the manual QA checklist in the Definition of Done is the actual completion gate for this plan.
|
||
|
||
---
|
||
|
||
## Verification Contract
|
||
|
||
| Gate | Command | Applies to |
|
||
|---|---|---|
|
||
| Rebuild theme assets | `make build-assets` | U1–U5 (regenerates `js/post/*` and `css-compiled/post-form.css`) |
|
||
| New location-override spec | `npm run test:ui -- tests/ui/post/location-override.spec.js` (run from `tests/`) | U6 — may be blocked by the known `make test-account` issue; manual QA is the fallback gate |
|
||
| Full post-form suite (no regressions) | `npm run test:ui -- tests/ui/post` | U2–U5 |
|
||
| Manual QA (per spec's Testing Plan) | see Definition of Done | All units |
|
||
|
||
Run the dev stack for manual QA via the worktree's own container per the worktree dev-server convention. Do not flip any dev/prod mode flags to work around anything encountered here.
|
||
|
||
---
|
||
|
||
## Definition of Done
|
||
|
||
**Global**
|
||
- All four coordinate-setting paths (GPS, search + pick, drag, type) verified in-browser to keep fields and pin in sync in both directions; a submitted entry's frontmatter has the expected `lat`/`lng`.
|
||
- The ambiguous-search case (City "Paris", Country "Texas") verified to rank the Texas result first over France/Tennessee/Kentucky/Illinois matches; the no-match case verified separately.
|
||
- Reopening "More location details" a second time does not duplicate the map canvas, and the pin still reflects the current `lat`/`lng`.
|
||
- Typing garbage into `lat`/`lng` does not crash the map or move the pin; a submit still round-trips through the existing backend `cleanCoordinate()` validation.
|
||
- `make build-assets` has been run; `js/post/*` and `css-compiled/post-form.css` are current; no hand-edits to built files.
|
||
- No abandoned/experimental code left in the diff; this plan's Status line is updated to `✅ Complete (YYYY-MM-DD)`.
|
||
|
||
**Per unit**
|
||
- U1: `lat`/`lng` inputs are visible only inside the new panel; new panel/results/map/mismatch styles render as designed.
|
||
- U2: panel exists, closed by default, positioned after Country, contains the expected child elements.
|
||
- U3: search happy path, disambiguation, no-match, empty-input, in-flight, network-failure, and XSS-safety scenarios all pass.
|
||
- U4: exactly one map canvas persists across repeated opens; no pin shown until first coordinate set; `maplibre-gl` fetches once, and never when the panel stays closed.
|
||
- U5: all four sync directions verified, including the mismatch-flag set/clear cycle.
|
||
- U6: spec file committed and passing where the test harness allows it; if blocked by the known `make test-account` issue, manual QA stands in as the completion gate.
|
||
|
||
---
|
||
|
||
## Risks & Dependencies
|
||
|
||
- **Third-party geocoding dependency — outright failure.** Open-Meteo's geocoding endpoint (CORS, city-only-query semantics) is external and was verified live only at design time; a future outage or breaking contract change could break requests outright. Mitigation: the existing graceful no-match/network-failure degrade paths (R8) already absorb this.
|
||
- **Third-party geocoding dependency — ranking/schema drift.** A subtler failure mode: the API keeps returning HTTP 200 with a non-empty `results` array, but a field the client-side ranking depends on (e.g. `country`) is renamed, emptied, or restructured — R8's no-match/network-failure paths don't fire in this case, since neither condition is met. Mitigation: R7 already renders the full, unranked result list regardless of ranking outcome, so the traveller can still manually pick the correct entry — this failure mode degrades disambiguation convenience, not correctness.
|
||
- **Build-chain risk.** Dynamically importing `maplibre-gl` from inside `post-form.js`'s existing `--splitting` ESM build must not regress the already-working `heic-to` lazy chunk. Mitigation: verify via `make build-assets` plus a browser network-tab check that both chunks split correctly.
|
||
- **Zero-size canvas on first open.** MapLibre initializing against a `display:none` container is a known gotcha; mitigated by the explicit `.resize()` call on every panel open (R9, U4).
|
||
- **Test-harness blocker.** The pre-existing `make test-account` Makefile quoting bug may prevent U6 from running locally at all. This plan does not fix that bug; manual QA is the accepted fallback per the design doc's own Out-of-scope note.
|
||
|
||
---
|
||
|
||
## Open Questions
|
||
|
||
- **Should the panel auto-open in edit mode when `lat`/`lng` are already set?** The design doc says "closed by default" without carving out an edit-mode exception, and this plan's default (U2) is to honor that literally — closed even on edit. The existing "More options" panel auto-opens under a narrower condition (a toggle value deviating from its blueprint default) and `initEditMode()` separately force-opens it for edit generally; whether "More location details" should follow either precedent for entries that already have a location is a plausible UX gap the design doc didn't explicitly rule out. Non-blocking — defer to whichever behavior feels right when the panel is actually used in edit mode, but flag it as a candidate small follow-up if closed-by-default proves surprising in practice.
|