Merge main into feat/docs-reconcile; defer to main on two overlapping fixes
main advanced 13 commits while this documentation audit ran — the location-override work was merged into the outer repo — and it independently fixed two of the audit's findings. Without this merge the branch would have REVERTED them, which is the worst outcome for a cleanup pass because it arrives disguised as an improvement. Both conflicts resolved in main's favour; main's wording was better informed: - CLAUDE.md, the single-map-path rule: main's carve-out (829325c) states the exception as its own top-level bullet, names MAP_STYLE as the one shared thing, and spells out both prohibitions ("do not fold it into initEntryMap", "do not add a third path"). Taken verbatim over the version drafted here. - 2026-07-23-post-form-location-override.md: main (a517331) had already set the status to Complete, with far richer detail — the multi-agent review findings, the green-run numbers, the DEL4 regression still open, and the merge SHAs. Taken in full; the audit's claim that the status "lagged" was dropped, since it was true only of this branch's older branch point. Submodule pin: main bumped user/ to dd19995 and this merge preserves that. The audit's own no-gitlink-commit discipline applies to bumping the pin as a side effect of routine work, not to discarding a bump main already made. main touched none of the other nine corrected documents, so the remaining 18 findings stand unchanged. Audit notes corrected to match reality rather than left overstated: - superseded-decisions.md R13 now dates the carve-out to 2026-07-24 (829325c) rather than implying this pass introduced it. - The reconciliation spec gains an "audit baseline moved twice" section: the submodule pin lagged real HEAD, and then the base branch advanced mid-audit. - The compounded learning's section 6 is rewritten from "audit the current state" to "re-check the baseline before publishing, not only before starting", with the two habits that actually follow: merge the base branch in before opening the PR and read conflicts as findings, and when the incoming version is better, take it wholesale. An audit has no special authority over the work it audits. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -11,7 +11,22 @@ execution: code
|
||||
|
||||
# Post Form Location Override - Plan
|
||||
|
||||
**Status:** ✅ Complete (2026-07-24) — merged to `user/` `main` as `dd19995` ("Merge feat/post-location-override into main"). Delivered `js/src/location-map.js` (the lazy-imported single-draggable-marker pin editor) and `js/src/map-style.js` (`MAP_STYLE`, now the single source of the basemap URL for both map paths), plus the "More location details" disclosure with city/country search-by-lookup in `js/src/post-form.js`. The status line lagged the merge and was corrected during the 2026-07-25 documentation reconciliation; the second map engine is now recorded as the one sanctioned exception to the single-map-path rule in `CLAUDE.md` and in [`../../reference/superseded-decisions.md`](../../reference/superseded-decisions.md) → R13.
|
||||
**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
|
||||
|
||||
@@ -45,7 +60,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.
|
||||
- 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 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**
|
||||
- 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 +69,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.
|
||||
|
||||
**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.
|
||||
|
||||
### Scope Boundaries
|
||||
|
||||
@@ -64,8 +64,8 @@ Backend sanitization has already been added (`user/plugins/cache-on-save/cache-o
|
||||
### Error handling
|
||||
|
||||
- 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.
|
||||
- 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.
|
||||
- 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: 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).
|
||||
|
||||
## Out of scope / explicitly deferred
|
||||
|
||||
@@ -102,8 +102,22 @@ Claims were checked against, not assumed from:
|
||||
| `entry-actions` routes | `user/plugins/entry-actions/entry-actions.php:63-73` |
|
||||
| `make` targets + env guard | `Makefile` (`guard-env:41-43`, `make-env-target:45-46`) |
|
||||
| `travel-memories` removal | `git log -- services/` → `a80b0a9`; `docker compose build` failure |
|
||||
| Plan status vs reality | `user/` HEAD `dd19995` |
|
||||
|
||||
The `user/` submodule was moved off the outer repo's pin to its real HEAD (`dd19995`) before
|
||||
auditing, because the pin lagged and would have produced findings against a state that is no longer
|
||||
current.
|
||||
## The audit baseline moved twice
|
||||
|
||||
Both times, auditing the convenient state rather than the real one would have produced wrong findings.
|
||||
|
||||
**The submodule pin lagged.** A fresh worktree checks out the `user/` commit the outer repo pins, not
|
||||
`user/`'s real HEAD. The pin predated the merged location-override work, so auditing it would have
|
||||
reported a feature as unbuilt and missed two new source files. `user/` was moved to its real HEAD
|
||||
(`dd19995`) before auditing, and the gitlink deliberately not committed.
|
||||
|
||||
**The outer `main` advanced 13 commits mid-audit.** The location-override branch was merged into the
|
||||
outer repo while this pass was running, which independently fixed two of the findings — the
|
||||
single-map-path carve-out (`829325c`) and the plan's `Status:` line (`a517331`). Merging `main` in
|
||||
before opening the PR was what surfaced that; without it this branch would have **reverted** both.
|
||||
`main`'s wording was better than the replacement drafted here and was kept in full. `main` touched none
|
||||
of the other nine corrected documents, so the remaining findings stand unchanged.
|
||||
|
||||
The general rule: **re-check the baseline before publishing, not only before starting.** A long audit
|
||||
races the work it is auditing.
|
||||
|
||||
Reference in New Issue
Block a user