From 01c3e72c8f2512f45183c524cd9a49f3d84ed7fd Mon Sep 17 00:00:00 2001 From: Mischa Date: Fri, 24 Jul 2026 19:38:39 +0200 Subject: [PATCH 01/12] test(post): add location-override Playwright coverage Mocks the Open-Meteo geocoding endpoint via page.route() so the suite is hermetic. Covers the panel's closed-by-default state, search happy path, Paris/Texas disambiguation ranking, no-match/network-failure/in-flight states, XSS-safe rendering, map canvas singleton behavior, drag sync, the mismatch flag, the maplibre-gl lazy-load boundary, and a full submit round-tripping lat/lng into the entry's frontmatter. --- tests/ui/post/location-override.spec.js | 304 ++++++++++++++++++++++++ 1 file changed, 304 insertions(+) create mode 100644 tests/ui/post/location-override.spec.js diff --git a/tests/ui/post/location-override.spec.js b/tests/ui/post/location-override.spec.js new file mode 100644 index 0000000..25667cb --- /dev/null +++ b/tests/ui/post/location-override.spec.js @@ -0,0 +1,304 @@ +// @ts-check +// Tests: post form "More location details" — search-by-city lookup + draggable +// map pin preview for setting an entry's coordinates without live GPS. +// Covers R4-R14. The Open-Meteo geocoding endpoint is mocked via page.route() +// so this suite is hermetic (no live third-party call, no rate-limit flakiness). +const { test, expect } = require('@playwright/test'); +const path = require('path'); +const { fillEditor, waitForPhotoUpload, cleanupEntry, findEntry, readEntryMd, TEST_PHOTO } = require('../helpers'); + +const GEOCODE_URL = '**/geocoding-api.open-meteo.com/v1/search**'; + +const created = []; +test.afterAll(() => { created.forEach(cleanupEntry); }); + +// Real-API-shaped fixtures (verified live against geocoding-api.open-meteo.com). +const KYOTO_RESULTS = { + results: [ + { name: 'Kyoto', latitude: 35.0116, longitude: 135.7681, admin1: 'Kyoto Prefecture', country: 'Japan' } + ] +}; + +// Mirrors the design doc's verified live Paris query: Île-de-France (France) +// first from the API, then five US states — Texas among them, in admin1 (the +// API's `country` field is "United States" for all of the US matches, so the +// ranking must also check admin1 to disambiguate on a US state name). +const PARIS_RESULTS = { + results: [ + { name: 'Paris', latitude: 48.85341, longitude: 2.3488, admin1: 'Île-de-France Region', country: 'France' }, + { name: 'Paris', latitude: 33.66094, longitude: -95.55551, admin1: 'Texas', country: 'United States' }, + { name: 'Paris', latitude: 36.302, longitude: -88.32671, admin1: 'Tennessee', country: 'United States' }, + { name: 'Paris', latitude: 38.2098, longitude: -84.2529, admin1: 'Kentucky', country: 'United States' }, + { name: 'Paris', latitude: 39.6112, longitude: -87.6961, admin1: 'Illinois', country: 'United States' } + ] +}; + +function mockGeocode(page, body) { + return page.route(GEOCODE_URL, (route) => route.fulfill({ + status: 200, + contentType: 'application/json', + body: JSON.stringify(body) + })); +} + +async function openLocationDetails(page) { + await page.locator('.location-details__summary').click(); + await expect(page.locator('.location-details')).toHaveJSProperty('open', true); +} + +// ── Panel closed by default (R1) ──────────────────────────────────────────── +test('More location details is closed by default and holds the relocated lat/lng fields', async ({ page }) => { + await page.goto('/post'); + const details = page.locator('.location-details'); + await expect(details).toBeAttached(); + await expect(details).toHaveJSProperty('open', false); + await expect(page.locator('.location-details input[name="data[lat]"]')).toBeAttached(); + await expect(page.locator('.location-details input[name="data[lng]"]')).toBeAttached(); +}); + +// ── R6: empty City + Country sends no request ─────────────────────────────── +test('R6: clicking lookup with City and Country both empty sends no request', async ({ page }) => { + await page.goto('/post'); + let requested = false; + await page.route(GEOCODE_URL, (route) => { requested = true; route.abort(); }); + await openLocationDetails(page); + await page.click('#lookup-coords'); + await expect(page.locator('#location-search-hint')).toContainText(/city or country/i); + expect(requested).toBe(false); +}); + +// ── R7: a search result sets lat/lng only, never City/Country ────────────── +test('R7: clicking a search result sets lat/lng and leaves City/Country untouched', async ({ page }) => { + await page.goto('/post'); + await mockGeocode(page, KYOTO_RESULTS); + await page.fill('input[name="data[location_city]"]', 'Kyoto'); + await openLocationDetails(page); + await page.click('#lookup-coords'); + + const results = page.locator('.location-search-results li button'); + await expect(results).toHaveCount(1); + await results.first().click(); + + await expect(page.locator('input[name="data[lat]"]')).toHaveValue('35.011600'); + await expect(page.locator('input[name="data[lng]"]')).toHaveValue('135.768100'); + await expect(page.locator('input[name="data[location_city]"]')).toHaveValue('Kyoto'); + await expect(page.locator('input[name="data[location_country]"]')).toHaveValue(''); + // R7: the list hides again until the next lookup. + await expect(page.locator('.location-search-results li')).toHaveCount(0); +}); + +// ── R4/KTD2: Paris/Texas disambiguation ranks the Texas match first ───────── +test('disambiguation: City "Paris" + Country "Texas" ranks the Texas match first', async ({ page }) => { + await page.goto('/post'); + let requestedUrl = null; + await page.route(GEOCODE_URL, (route) => { + requestedUrl = route.request().url(); + route.fulfill({ status: 200, contentType: 'application/json', body: JSON.stringify(PARIS_RESULTS) }); + }); + await page.fill('input[name="data[location_city]"]', 'Paris'); + await page.fill('input[name="data[location_country]"]', 'Texas'); + await openLocationDetails(page); + await page.click('#lookup-coords'); + + const results = page.locator('.location-search-results li button'); + await expect(results).toHaveCount(5); + await expect(results.first()).toContainText('Texas'); + + // R4: Country is never concatenated into the query string. + expect(requestedUrl).toContain('name=Paris'); + expect(requestedUrl).not.toContain('Texas'); +}); + +// ── R8: no matches shows the inline hint, fields untouched ───────────────── +test('R8: no matches shows the no-match hint and leaves fields untouched', async ({ page }) => { + await page.goto('/post'); + await mockGeocode(page, { results: [] }); + await page.fill('input[name="data[location_city]"]', 'Nowheresville'); + await openLocationDetails(page); + await page.click('#lookup-coords'); + + await expect(page.locator('#location-search-hint')).toContainText(/no matches/i); + await expect(page.locator('input[name="data[lat]"]')).toHaveValue(''); + await expect(page.locator('input[name="data[lng]"]')).toHaveValue(''); +}); + +// ── R5: in-flight state shows "Searching…" and always re-enables ─────────── +test('R5: the lookup button shows a disabled "Searching…" state while in flight', async ({ page }) => { + await page.goto('/post'); + await page.route(GEOCODE_URL, async (route) => { + await new Promise((r) => setTimeout(r, 400)); + route.fulfill({ status: 200, contentType: 'application/json', body: JSON.stringify(KYOTO_RESULTS) }); + }); + await page.fill('input[name="data[location_city]"]', 'Kyoto'); + await openLocationDetails(page); + await page.click('#lookup-coords'); + + const btn = page.locator('#lookup-coords'); + await expect(btn).toBeDisabled(); + await expect(btn).toHaveText('Searching…'); + await expect(btn).toBeEnabled({ timeout: 5_000 }); + await expect(btn).toContainText('Look up coordinates'); +}); + +// ── R8: a network failure degrades silently and re-enables the button ────── +test('a network failure degrades silently, leaves fields untouched, and re-enables the button', async ({ page }) => { + await page.goto('/post'); + await page.route(GEOCODE_URL, (route) => route.abort('failed')); + await page.fill('input[name="data[location_city]"]', 'Kyoto'); + await openLocationDetails(page); + await page.click('#lookup-coords'); + + await expect(page.locator('#lookup-coords')).toBeEnabled(); + await expect(page.locator('input[name="data[lat]"]')).toHaveValue(''); + await expect(page.locator('input[name="data[lng]"]')).toHaveValue(''); +}); + +// ── XSS safety: an API-sourced name containing markup renders as literal text ── +test('a result name containing markup renders as literal text, not executed', async ({ page }) => { + await page.goto('/post'); + await mockGeocode(page, { + results: [{ name: '', latitude: 1, longitude: 2, country: 'Nowhere' }] + }); + await page.fill('input[name="data[location_city]"]', 'Test'); + await openLocationDetails(page); + await page.click('#lookup-coords'); + + const btn = page.locator('.location-search-results li button').first(); + await expect(btn).toContainText(''); + expect(await btn.evaluate((el) => el.querySelector('img'))).toBeNull(); + expect(await page.evaluate(() => window.__xss)).toBeUndefined(); +}); + +// ── U4: map renders exactly one canvas, no pin until a coordinate is set ─── +test('opening the panel renders exactly one map canvas with no initial pin', async ({ page }) => { + await page.goto('/post'); + await openLocationDetails(page); + await expect(page.locator('#location-map canvas.maplibregl-canvas')).toHaveCount(1, { timeout: 10_000 }); + await expect(page.locator('#location-map .maplibregl-marker')).toHaveCount(0); +}); + +// ── U4: reopening does not duplicate the canvas; resize keeps it non-zero ── +test('reopening the panel a second time leaves exactly one canvas with non-zero size', async ({ page }) => { + await page.goto('/post'); + await openLocationDetails(page); + await page.locator('.location-details__summary').click(); // close + await expect(page.locator('.location-details')).toHaveJSProperty('open', false); + await openLocationDetails(page); // reopen + + const canvases = page.locator('#location-map canvas.maplibregl-canvas'); + await expect(canvases).toHaveCount(1, { timeout: 10_000 }); + const box = await canvases.first().boundingBox(); + expect(box && box.width).toBeGreaterThan(0); + expect(box && box.height).toBeGreaterThan(0); +}); + +// ── R11: a search pick shows a pin on the map ─────────────────────────────── +test('a search-result pick renders a pin on the map', async ({ page }) => { + await page.goto('/post'); + await mockGeocode(page, KYOTO_RESULTS); + await page.fill('input[name="data[location_city]"]', 'Kyoto'); + await openLocationDetails(page); + await page.click('#lookup-coords'); + await page.locator('.location-search-results li button').first().click(); + + await expect(page.locator('#location-map .maplibregl-marker')).toHaveCount(1, { timeout: 10_000 }); +}); + +// ── R11: dragging the marker updates lat/lng (rounded to 6dp) ────────────── +test('dragging the pin updates lat/lng to the drop location', async ({ page }) => { + await page.goto('/post'); + await mockGeocode(page, KYOTO_RESULTS); + await page.fill('input[name="data[location_city]"]', 'Kyoto'); + await openLocationDetails(page); + await page.click('#lookup-coords'); + await page.locator('.location-search-results li button').first().click(); + + const marker = page.locator('#location-map .maplibregl-marker'); + await expect(marker).toHaveCount(1, { timeout: 10_000 }); + const before = await page.locator('input[name="data[lat]"]').inputValue(); + + // setPin()'s map.panTo() animates the marker into view — wait for it to + // settle so the bounding box grabbed below matches where the marker will + // actually be when the mouse events land. + await page.waitForTimeout(800); + const box = await marker.boundingBox(); + if (!box) throw new Error('marker has no bounding box'); + const startX = box.x + box.width / 2; + const startY = box.y + box.height / 2; + await page.mouse.move(startX, startY); + await page.mouse.down(); + await page.mouse.move(startX + 40, startY + 30, { steps: 5 }); + await page.mouse.up(); + + await expect(async () => { + const after = await page.locator('input[name="data[lat]"]').inputValue(); + expect(after).not.toBe(before); + expect(after).toMatch(/^-?\d+\.\d{6}$/); + }).toPass({ timeout: 5_000 }); +}); + +// ── R11/R13: typing an invalid value flags the field without crashing ────── +test('typing an invalid lat value shows the mismatch flag and clears once fixed', async ({ page }) => { + await page.goto('/post'); + await openLocationDetails(page); + + const latEl = page.locator('input[name="data[lat]"]'); + const lngEl = page.locator('input[name="data[lng]"]'); + await latEl.fill('not-a-number'); + await lngEl.fill('135.7681'); + await lngEl.blur(); + + await expect(latEl).toHaveClass(/location-field--mismatch/); + await expect(latEl).toHaveAttribute('aria-invalid', 'true'); + await expect(page.locator('#location-map .maplibregl-marker')).toHaveCount(0); + + await latEl.fill('35.0116'); + await latEl.blur(); + await expect(latEl).not.toHaveClass(/location-field--mismatch/); + await expect(page.locator('#location-map .maplibregl-marker')).toHaveCount(1); +}); + +// ── U4: lazy-load boundary — an ordinary GPS-only submit never fetches maplibre-gl ── +test('an ordinary submit without opening the panel never fetches the maplibre-gl chunk', async ({ page }) => { + const chunkRequests = []; + page.on('request', (req) => { + if (/maplibre-gl/.test(req.url())) chunkRequests.push(req.url()); + }); + + const tag = `loc-nomap-${Date.now()}`; + await page.goto('/post'); + await page.fill('input[name="data[title]"]', `UI Test ${tag}`); + await fillEditor(page, 'Location-override lazy-load guard. Safe to delete.'); + await page.locator('input.filepond--browser').setInputFiles(TEST_PHOTO); + await waitForPhotoUpload(page); + await page.locator('.btn-post').evaluate((el) => el.click()); + await expect(page.locator('.notices')).toContainText('Entry posted successfully!', { timeout: 15_000 }); + created.push(tag); + + expect(chunkRequests, 'maplibre-gl must not be fetched when the panel is never opened').toHaveLength(0); +}); + +// ── Full submit: a search-picked location round-trips into the frontmatter ── +test('a full submit with a search-picked location saves the expected lat/lng', async ({ page }) => { + const tag = `loc-submit-${Date.now()}`; + await page.goto('/post'); + await mockGeocode(page, KYOTO_RESULTS); + await page.fill('input[name="data[title]"]', `UI Test ${tag}`); + await fillEditor(page, 'Location-override submit test. Safe to delete.'); + await page.fill('input[name="data[location_city]"]', 'Kyoto'); + await openLocationDetails(page); + await page.click('#lookup-coords'); + await page.locator('.location-search-results li button').first().click(); + + await page.locator('input.filepond--browser').setInputFiles(TEST_PHOTO); + await waitForPhotoUpload(page); + await page.locator('.btn-post').evaluate((el) => el.click()); + await expect(page.locator('.notices')).toContainText('Entry posted successfully!', { timeout: 15_000 }); + created.push(tag); + + const entryDir = findEntry(tag); + expect(entryDir, 'Entry folder should exist on disk').toBeTruthy(); + const md = readEntryMd(entryDir); + expect(md).toContain('35.0116'); + expect(md).toContain('135.7681'); +}); From a517331d1bc6c51628e61d9d548c1e511f836662 Mon Sep 17 00:00:00 2001 From: Mischa Date: Fri, 24 Jul 2026 20:03:19 +0200 Subject: [PATCH 02/12] fix(post): cover the code-review fixes; mark location-override plan complete Adds Playwright coverage for the four cross-reviewer-confirmed bugs fixed in the user/ submodule (map-load race on rapid reopen, mismatch flag clearing on blank, and submit blocked on unresolved mismatch), and bumps the user/ pointer to the commit with those fixes. Co-Authored-By: Claude Sonnet 5 --- .../2026-07-23-post-form-location-override.md | 2 +- tests/ui/post/location-override.spec.js | 59 +++++++++++++++++++ user | 2 +- 3 files changed, 61 insertions(+), 2 deletions(-) diff --git a/docs/working/plans/2026-07-23-post-form-location-override.md b/docs/working/plans/2026-07-23-post-form-location-override.md index b94f506..f4edd81 100644 --- a/docs/working/plans/2026-07-23-post-form-location-override.md +++ b/docs/working/plans/2026-07-23-post-form-location-override.md @@ -11,7 +11,7 @@ execution: code # Post Form Location Override - Plan -**Status:** 📋 Not started +**Status:** ✅ Complete (2026-07-24) ## Goal Capsule diff --git a/tests/ui/post/location-override.spec.js b/tests/ui/post/location-override.spec.js index 25667cb..cd49055 100644 --- a/tests/ui/post/location-override.spec.js +++ b/tests/ui/post/location-override.spec.js @@ -258,6 +258,65 @@ test('typing an invalid lat value shows the mismatch flag and clears once fixed' await expect(page.locator('#location-map .maplibregl-marker')).toHaveCount(1); }); +// ── U4: rapid close/reopen while the maplibre-gl chunk is still in flight must +// not build two Map instances against the same container (code-review fix) ── +test('rapid close/reopen before the maplibre-gl chunk resolves still leaves exactly one canvas', async ({ page }) => { + await page.route('**/*maplibre-gl*.js', async (route) => { + await new Promise((resolve) => setTimeout(resolve, 500)); + await route.continue(); + }); + await page.goto('/post'); + + // Open, then immediately close and reopen — both toggles land while the + // delayed chunk request above is still pending. + await page.locator('.location-details__summary').click(); + await page.locator('.location-details__summary').click(); + await page.locator('.location-details__summary').click(); + await expect(page.locator('.location-details')).toHaveJSProperty('open', true); + + await expect(page.locator('#location-map canvas.maplibregl-canvas')).toHaveCount(1, { timeout: 10_000 }); +}); + +// ── U5: blanking both fields after a mismatch was flagged clears the flag ── +test('blanking both lat/lng fields after a mismatch clears the flag', async ({ page }) => { + await page.goto('/post'); + await openLocationDetails(page); + + const latEl = page.locator('input[name="data[lat]"]'); + const lngEl = page.locator('input[name="data[lng]"]'); + await latEl.fill('not-a-number'); + await lngEl.blur(); + await expect(latEl).toHaveClass(/location-field--mismatch/); + + await latEl.fill(''); + await lngEl.fill(''); + await lngEl.blur(); + await expect(latEl).not.toHaveClass(/location-field--mismatch/); + await expect(lngEl).not.toHaveClass(/location-field--mismatch/); +}); + +// ── U5: a flagged, unresolved lat/lng must block submit (code-review fix) ── +test('submitting with an unresolved lat/lng mismatch is blocked', async ({ page }) => { + const tag = `loc-mismatch-${Date.now()}`; + await page.goto('/post'); + await page.fill('input[name="data[title]"]', `UI Test ${tag}`); + await fillEditor(page, 'Location-override mismatch-blocks-submit guard. Safe to delete.'); + await page.locator('input.filepond--browser').setInputFiles(TEST_PHOTO); + await waitForPhotoUpload(page); + + await openLocationDetails(page); + const latEl = page.locator('input[name="data[lat]"]'); + const lngEl = page.locator('input[name="data[lng]"]'); + await latEl.fill('999'); + await lngEl.fill('999'); + await lngEl.blur(); + await expect(latEl).toHaveClass(/location-field--mismatch/); + + await page.locator('.btn-post').evaluate((el) => el.click()); + await expect(page.locator('.notices')).toHaveCount(0); + await expect(page).toHaveURL(/\/post/); +}); + // ── U4: lazy-load boundary — an ordinary GPS-only submit never fetches maplibre-gl ── test('an ordinary submit without opening the panel never fetches the maplibre-gl chunk', async ({ page }) => { const chunkRequests = []; diff --git a/user b/user index 02fa4e9..13c76b2 160000 --- a/user +++ b/user @@ -1 +1 @@ -Subproject commit 02fa4e94a735e1bca7603f74f989eed66ba778dd +Subproject commit 13c76b29a840e8082427e2e29c7cdaeb1ef895b6 From 829325c9c7e79fe5c59c0f475b99d4fc615807d2 Mon Sep 17 00:00:00 2001 From: Mischa Date: Fri, 24 Jul 2026 21:34:17 +0200 Subject: [PATCH 03/12] fix(review): make the mismatch-blocks-submit test able to fail; carve out the map doctrine MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The U5 guard asserted only `.notices` toHaveCount(0) and toHaveURL(/\/post/). Both pass instantly, and both also hold for a *successful* submit — the form posts to /post and only renders its notice after the round trip, and the click is dispatched via evaluate(el => el.click()), which skips Playwright's navigation-aware waiting. So the one test standing between a bad coordinate and the server could not fail. It now proves the negative on disk via findEntry() after a settling interval, registers the tag for cleanup before the click, and asserts the flag and value survived. CLAUDE.md's "one map path" section stated flatly that a single map code path exists, which location-map.js now contradicts. Recorded it as the one sanctioned exception (an editor, not a display map; lazy-imported; shares only MAP_STYLE) rather than leaving the doctrine wrong. The `user` gitlink is deliberately NOT bumped here: its pin already points at an unpushed submodule commit, which must be pushed and re-pointed before this branch merges. Co-Authored-By: Claude Opus 5 --- CLAUDE.md | 4 +++- tests/ui/post/location-override.spec.js | 16 ++++++++++++++-- 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index ac2389b..76f9e70 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -42,7 +42,9 @@ The site is structured around Trip entities. Key facts: ### One map path: `MapUtils.initEntryMap` + the `entry-map` partial -There is a **single** map code path on the site. The engine is `MapUtils.initEntryMap(opts)` in `js/src/maplibre-utils.js` (bundled into `js/map.js` via `make build-assets` — never hand-edit `js/map.js`). It builds the MapLibre map, places markers/popups, fits bounds, draws the GPX journey, and wires the fullscreen toggle. +There is a **single** map code path for every **display** map on the site. The engine is `MapUtils.initEntryMap(opts)` in `js/src/maplibre-utils.js` (bundled into `js/map.js` via `make build-assets` — never hand-edit `js/map.js`). It builds the MapLibre map, places markers/popups, fits bounds, draws the GPX journey, and wires the fullscreen toggle. + +**One sanctioned exception — the post form's pin editor.** `js/src/location-map.js` (`getOrCreateLocationMap()`) is a deliberately separate, minimal engine for the `/post` form's "More location details" panel: one *draggable* marker, no popups, no GPX, no bounds-fitting, and `maplibre-gl` **lazy-imported** so a GPS-only submit never fetches it. It is an editor, not a display map, so it shares none of `initEntryMap`'s concerns. The two share exactly one thing — the style URL, extracted into `js/src/map-style.js` (`MAP_STYLE`) and imported by both, so the basemap can never drift between them. Do not fold it into `initEntryMap`, and do not add a *third* path: a new display map uses `initEntryMap` + the `entry-map` partial. The map **markup + invocation** is shared via one partial: diff --git a/tests/ui/post/location-override.spec.js b/tests/ui/post/location-override.spec.js index cd49055..e37ca4e 100644 --- a/tests/ui/post/location-override.spec.js +++ b/tests/ui/post/location-override.spec.js @@ -312,9 +312,21 @@ test('submitting with an unresolved lat/lng mismatch is blocked', async ({ page await lngEl.blur(); await expect(latEl).toHaveClass(/location-field--mismatch/); + // Register for cleanup BEFORE the click: if the gate ever regresses, the + // entry lands on disk and the afterAll hook must still see the tag. + created.push(tag); await page.locator('.btn-post').evaluate((el) => el.click()); - await expect(page.locator('.notices')).toHaveCount(0); - await expect(page).toHaveURL(/\/post/); + + // `.notices` toHaveCount(0) and toHaveURL(/\/post/) both pass instantly and + // both also hold for a SUCCESSFUL submit (the form posts to /post and only + // renders its notice after the round trip), so neither can distinguish a + // working gate from a regressed one. Prove the negative on disk instead, + // after giving a regressed submit time to actually write. + await page.waitForTimeout(2000); + expect(findEntry(tag), 'a flagged coordinate must never reach the server').toBeFalsy(); + // And prove the block was the gate's doing: still flagged, value untouched. + await expect(latEl).toHaveClass(/location-field--mismatch/); + await expect(latEl).toHaveValue('999'); }); // ── U4: lazy-load boundary — an ordinary GPS-only submit never fetches maplibre-gl ── From 5edaf3ee1e5015f190276536bc4a429ac81c226d Mon Sep 17 00:00:00 2001 From: Mischa Date: Fri, 24 Jul 2026 21:49:02 +0200 Subject: [PATCH 04/12] docs(review): correct R8/R13 and the plan status; bump user pin to the review fixes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../working/plans/2026-07-23-post-form-location-override.md | 6 +++--- .../specs/2026-07-23-post-form-location-override-design.md | 4 ++-- user | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/docs/working/plans/2026-07-23-post-form-location-override.md b/docs/working/plans/2026-07-23-post-form-location-override.md index f4edd81..dc88d67 100644 --- a/docs/working/plans/2026-07-23-post-form-location-override.md +++ b/docs/working/plans/2026-07-23-post-form-location-override.md @@ -11,7 +11,7 @@ execution: code # 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 @@ -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. - 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 +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. **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 diff --git a/docs/working/specs/2026-07-23-post-form-location-override-design.md b/docs/working/specs/2026-07-23-post-form-location-override-design.md index 7e90a9d..36d8fcf 100644 --- a/docs/working/specs/2026-07-23-post-form-location-override-design.md +++ b/docs/working/specs/2026-07-23-post-form-location-override-design.md @@ -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 diff --git a/user b/user index 13c76b2..e873a9c 160000 --- a/user +++ b/user @@ -1 +1 @@ -Subproject commit 13c76b29a840e8082427e2e29c7cdaeb1ef895b6 +Subproject commit e873a9cb2341008c3d0fbe481e79000cac729c9d From 1f4e2aeba5a5d227dfe7b33bc5197d4d27d2679a Mon Sep 17 00:00:00 2001 From: Mischa Date: Fri, 24 Jul 2026 22:33:46 +0200 Subject: [PATCH 05/12] fix(test): close the test-entry leak into real trip content MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A ui-test entry had survived into the active trip's dailies. Three independent failures had to line up for that, and all three were real: 1. cleanupEntry() used host-side fs.rmSync. Grav's Apache workers run as root, so every entry the form creates is root-owned and recursive removal needs write permission on that directory — which the host user lacks. Cleanup had never worked for form-created entries; it just threw inside a path nothing checked. It now falls back to `docker exec … rm -rf` in the container that actually serves USER_DIR. 2. globalTeardown's dailies sweep keyed off a `parent:` in post-form.md — a key deliberately removed (the write target comes from site.yaml active_trip, and CLAUDE.md forbids re-adding a static parent). The regex could never match, so dailiesDir was always null and the sweep silently did nothing. It now reuses helpers' own resolution instead of keeping a divergent copy. 3. Nothing pinned the suite to this checkout's server. playwright.config.js defaults to :8081, so a worktree run hit the MAIN checkout — entries created in one content tree while the specs asserted and cleaned up in another. test-ui now passes GRAV_BASE_URL from GRAV_PORT, and globalSetup hard-fails when the server's bind mount disagrees with the tree the specs read. Also fixed, found on the way to a green run: - test-account interpolated the password into an `sh -c` string, so a password containing a shell metacharacter was re-parsed by the container's shell (`sh: 2: : not found`, no account, every UI run dead). It now travels via `docker exec -e`, making the recipe indifferent to its contents. - `make start` in a worktree always failed: travel-memories declares `env_file: .env` and worktree-new creates none. It degrades to start-grav there — a worktree with no server is what sent runs to :8081 in the first place. - test-form-config asserted a hero_image field that 8cf1145 deliberately removed; it had been failing ever since. Verified: config 22/22, post 6/6, location-override 20/20, and a full UI run now leaves zero ui-test entries behind. The remaining UI failures are pre-existing on main — site.yaml pins owner_username to a real account while the suite logs in as testrunner, so owner-only controls never render for it. Only trip-publish.spec.js patches that; delete-flow, edit-mode and anon-view do not. Left for a separate branch. --- Makefile | 36 +++++++++++- scripts/test-form-config.sh | 5 +- tests/global-setup.js | 57 ++++++++++++++++++ tests/global-teardown.js | 77 ++++++++++--------------- tests/ui/helpers.js | 61 +++++++++++++++++++- tests/ui/post/location-override.spec.js | 31 +++++++++- user | 2 +- 7 files changed, 216 insertions(+), 53 deletions(-) diff --git a/Makefile b/Makefile index 9998605..6153a22 100644 --- a/Makefile +++ b/Makefile @@ -54,9 +54,15 @@ $(foreach t,$(REMOTE_TARGETS),$(foreach e,$(ENVS),$(eval $(call make-env-target, GRAV_TEST_USER ?= testrunner GRAV_TEST_PASS ?= Testpass1234 +# The password is handed to the container through `docker exec -e` (the bare +# form, which forwards the already-exported variable) rather than interpolated +# into the `sh -c` string. Interpolating it meant any shell-special character in +# GRAV_TEST_PASS was re-parsed by the container's shell — a `.env` password +# containing one produced `sh: 2: : not found` and no test account. +# The recipe is now indifferent to the password's contents. test-account: - @docker exec $(GRAV_CONTAINER) sh -c 'test -f /var/www/html/user/accounts/$(GRAV_TEST_USER).yaml \ - || php bin/plugin login new-user -u $(GRAV_TEST_USER) -p "$(GRAV_TEST_PASS)" \ + @docker exec -e GRAV_TEST_PASS $(GRAV_CONTAINER) sh -c 'test -f /var/www/html/user/accounts/$(GRAV_TEST_USER).yaml \ + || php bin/plugin login new-user -u $(GRAV_TEST_USER) -p "$$GRAV_TEST_PASS" \ -e $(GRAV_TEST_USER)@example.test -N "Test Runner" -P b --admin-type both -s enabled -n' test-config: @@ -65,6 +71,13 @@ test-config: test-post: test-account @bash scripts/test-post.sh +# Pinned to THIS checkout's port, not playwright.config.js's :8081 default. In a +# worktree that default silently pointed the suite at the main checkout's server, +# so entries were created in main's user/ while the specs asserted and cleaned up +# in the worktree's — leaving ui-test entries behind in real trip content. +# tests/global-setup.js now also hard-fails on that mismatch. +GRAV_BASE_URL ?= http://localhost:$(GRAV_PORT) + test-ui: test-account @npx playwright test @@ -98,8 +111,18 @@ build-assets: -w /app node:20-alpine \ sh -c "npm install && npm run build" +# In a worktree this degrades to start-grav. The travel-memories service declares +# `env_file: .env`, and worktree-new does not create a .env, so a plain +# `docker compose up -d` there dies with "env file ... not found" — leaving the +# worktree with no server at all, which is how test runs ended up silently +# targeting the main checkout. start: - docker compose up -d + @if [ -f .worktree-env ]; then \ + echo "→ worktree: starting the grav service only (travel-memories needs a .env, which worktrees have none)"; \ + docker compose up -d grav; \ + else \ + docker compose up -d; \ + fi # Grav service only — used by `make worktree-new` (a worktree rarely needs the # travel-memories service, and this keeps its footprint minimal). @@ -185,6 +208,13 @@ demo-load: # Load every fixture trip under docs/demo/trips/ into the pages tree. # Source uses dailies/ + 04.stories/; dailies/ maps to 01.dailies/ on copy. # All copies are `|| true` so a fixture absent from an older user/ is skipped. + # + # ⚠️ A fixture whose folder name matches a REAL trip's slug is copied straight + # over that live page — docs/demo/trips/italy-2025/ collides with the real + # italy-2025 trip on purpose (the fixture supplies its GPX + dailies). So any + # field the fixture's trip.md omits gets silently deleted from real content on + # every test run: it had been dropping the trip's tagline that way. Keep a + # colliding fixture's trip.md byte-identical to the live page. docker exec $(GRAV_CONTAINER) bash -c 'for src in /var/www/html/user/docs/demo/trips/*/; do \ slug=$$(basename "$$src"); dst=/var/www/html/user/pages/01.trips/$$slug; \ mkdir -p "$$dst/01.dailies" "$$dst/04.stories"; \ diff --git a/scripts/test-form-config.sh b/scripts/test-form-config.sh index c8badfd..18490f0 100755 --- a/scripts/test-form-config.sh +++ b/scripts/test-form-config.sh @@ -59,7 +59,10 @@ check_grep "location_country field present" "name: location_country" check_grep "weather_desc field present" "name: weather_desc" check_grep "weather_temp_c field present" "name: weather_temp_c" check_grep "transport_mode field present" "name: transport_mode" -check_grep "hero_image field present" "name: hero_image" +# No hero_image assertion: the field was deliberately dropped in 8cf1145 — +# entries render their hero from the first photo, so an explicit filename was +# redundant (see the comment at that spot in post-form.md). This check outlived +# the field and had been failing ever since. check_grep "force_connect field present" "name: force_connect" check_grep "featured field present" "name: featured" diff --git a/tests/global-setup.js b/tests/global-setup.js index ec598c4..b9c6864 100644 --- a/tests/global-setup.js +++ b/tests/global-setup.js @@ -2,6 +2,58 @@ const fs = require('fs'); const path = require('path'); const { execSync } = require('child_process'); +/** + * Fail fast if the server under test does not serve the `user/` tree the specs + * read from disk. + * + * This mismatch is silent and destructive. Every post spec submits through the + * live form (the write target is derived server-side from site.yaml + * `active_trip`, so there is no per-request override), then asserts and cleans up + * on disk via helpers' USER_DIR. Run the specs from a worktree whose own + * container is down and baseURL falls back to localhost:8081 — the MAIN + * checkout — so entries get created in one content tree while cleanup deletes + * from another. The entries are then left behind in real trip content, which is + * exactly what happened on 2026-07-24. + * + * Docker is the only thing that knows the mapping, so this is best-effort: if we + * cannot determine it we warn and continue rather than blocking non-Docker runs. + * But when we CAN determine it and it disagrees, that is always a bug. + */ +function assertServerServesUserDir(baseURL, userDir) { + const port = new URL(baseURL).port || '80'; + let mountedUserDir; + try { + const container = execSync("docker ps --format '{{.Names}}\t{{.Ports}}'", { encoding: 'utf-8' }) + .split('\n').filter(Boolean) + .find(l => l.includes(`:${port}->`)); + if (!container) { + console.warn(`[setup] no running container publishes port ${port} — is the dev server up? (make start)`); + return; + } + const name = container.split('\t')[0]; + mountedUserDir = execSync( + `docker inspect ${name} --format '{{range .Mounts}}{{if eq .Destination "/var/www/html/user"}}{{.Source}}{{end}}{{end}}'`, + { encoding: 'utf-8', stdio: ['pipe', 'pipe', 'ignore'] } + ).trim(); + if (!mountedUserDir) return; // no bind mount to compare against + } catch (_) { + return; // docker unavailable — nothing to check + } + + const served = fs.realpathSync(mountedUserDir); + const asserted = fs.realpathSync(userDir); + if (served !== asserted) { + throw new Error( + `Test target mismatch — refusing to run.\n` + + ` baseURL ${baseURL} is served from: ${served}\n` + + ` but the specs read/clean up: ${asserted}\n` + + `Entries would be created in one tree and cleanup would miss them, leaving\n` + + `test entries behind in real content. Start this checkout's own server\n` + + `(make start) and point the run at it, e.g. GRAV_BASE_URL=http://localhost:.` + ); + } +} + module.exports = async function globalSetup() { const envFile = path.join(__dirname, '../.env'); if (fs.existsSync(envFile)) { @@ -23,4 +75,9 @@ module.exports = async function globalSetup() { // Ensure demo content is loaded (italy-2026-demo trip + stories + GPX files) execSync('make demo-load', { cwd: path.join(__dirname, '..'), stdio: 'inherit' }); + + // Required last: helpers.js resolves USER_DIR at require time, and the .env + // load above can supply GRAV_USER_DIR. + const { USER_DIR } = require('./ui/helpers'); + assertServerServesUserDir(process.env.GRAV_BASE_URL || 'http://localhost:8081', USER_DIR); }; diff --git a/tests/global-teardown.js b/tests/global-teardown.js index 9441321..ca21f85 100644 --- a/tests/global-teardown.js +++ b/tests/global-teardown.js @@ -1,57 +1,44 @@ const fs = require('fs'); const path = require('path'); -const { execSync } = require('child_process'); -function resolveUserDir() { - if (process.env.GRAV_USER_DIR) return process.env.GRAV_USER_DIR; - try { - const raw = execSync( - "docker inspect intotheeast_grav --format '{{range .Mounts}}{{if eq .Destination \"/var/www/html/user\"}}{{.Source}}{{end}}{{end}}'", - { encoding: 'utf-8', stdio: ['pipe', 'pipe', 'ignore'] } - ).trim(); - if (raw) return raw; - } catch (_) {} - return path.join(__dirname, '../user'); -} +// Reuse the specs' own resolution rather than reimplementing it. The previous +// version of this file derived the dailies directory from a `parent:` key in +// pages/02.post/post-form.md — a key that was deliberately removed (the write +// target is injected server-side from site.yaml `active_trip`, and CLAUDE.md +// forbids re-adding a static parent). The regex therefore never matched, +// dailiesDir was always null, and the dailies sweep below silently did nothing. +// That is how ui-test entries survived into the active trip's content. +// removeEntryDir handles the root-owned case by deleting through the container — +// see its comment. Plain fs.rmSync cannot remove what Grav's Apache wrote. +const { USER_DIR, TRACKER_DIR, removeEntryDir } = require('./ui/helpers'); function sweepUiTestEntries(dir) { - if (!fs.existsSync(dir)) return 0; - const entries = fs.readdirSync(dir).filter(e => e.includes('ui-test')); - entries.forEach(e => fs.rmSync(path.join(dir, e), { recursive: true, force: true })); - return entries.length; + if (!dir || !fs.existsSync(dir)) return 0; + const found = fs.readdirSync(dir).filter(e => e.includes('ui-test')); + let removed = 0; + found.forEach(e => { + const target = path.join(dir, e); + try { + removeEntryDir(target); + removed++; + } catch (err) { + // Loud, not silent — a swallowed failure here is exactly what let a + // ui-test entry survive into the active trip's content. + console.error(`[teardown] COULD NOT REMOVE ${target}: ${err.message}`); + } + }); + return removed; } module.exports = async function globalTeardown() { - const userDir = resolveUserDir(); - - // Read active trip slug from post-form.md - const postFormPath = path.join(userDir, 'pages/02.post/post-form.md'); - let dailiesDir = null; - if (fs.existsSync(postFormPath)) { - const content = fs.readFileSync(postFormPath, 'utf-8'); - const m = content.match(/parent:\s*['"]?\/trips\/([^/'"]+)\/dailies/); - if (m) { - const tripSlug = m[1]; - const tripsBase = path.join(userDir, 'pages/01.trips'); - const tripFolder = fs.readdirSync(tripsBase).find( - f => f === tripSlug || f.endsWith('.' + tripSlug) || f.includes(tripSlug) - ); - if (tripFolder) { - const dailiesBase = path.join(tripsBase, tripFolder); - const dailiesFolder = fs.readdirSync(dailiesBase).find( - f => f === 'dailies' || f === '01.dailies' || f.endsWith('.dailies') - ); - if (dailiesFolder) dailiesDir = path.join(dailiesBase, dailiesFolder); - } - } - } - - // Sweep both the post inbox and the active trip's dailies - const postInbox = path.join(userDir, 'pages/02.post'); - const n1 = sweepUiTestEntries(postInbox); - const n2 = dailiesDir ? sweepUiTestEntries(dailiesDir) : 0; + // Sweep both the post inbox and the active trip's dailies. + const n1 = sweepUiTestEntries(path.join(USER_DIR, 'pages/02.post')); + const n2 = sweepUiTestEntries(TRACKER_DIR); if (n1 + n2 > 0) { - console.log(`[teardown] removed ${n1} ui-test entries from 02.post, ${n2} from dailies`); + console.log( + `[teardown] removed ${n1} ui-test entries from 02.post, ` + + `${n2} from ${path.relative(USER_DIR, TRACKER_DIR)}` + ); } }; diff --git a/tests/ui/helpers.js b/tests/ui/helpers.js index a825606..c11ec09 100644 --- a/tests/ui/helpers.js +++ b/tests/ui/helpers.js @@ -170,6 +170,60 @@ async function createPhotoEntry(page, tag, { content, publish = true, created } 'Entry posted successfully!', { timeout: 15_000 }); } +/** + * Resolve the Grav container that serves USER_DIR, so cleanup can delete as root. + * Prefers GRAV_CONTAINER (set by .worktree-env / .env), else matches on the bind + * mount so a worktree never picks the main checkout's container. + */ +function resolveGravContainer() { + if (process.env.GRAV_CONTAINER) return process.env.GRAV_CONTAINER; + try { + const want = fs.realpathSync(USER_DIR); + const names = execSync("docker ps --format '{{.Names}}'", { encoding: 'utf-8', stdio: ['pipe', 'pipe', 'ignore'] }) + .split('\n').filter(Boolean); + return names.find((n) => { + const src = execSync( + `docker inspect ${n} --format '{{range .Mounts}}{{if eq .Destination "/var/www/html/user"}}{{.Source}}{{end}}{{end}}'`, + { encoding: 'utf-8', stdio: ['pipe', 'pipe', 'ignore'] } + ).trim(); + return src && fs.realpathSync(src) === want; + }) || null; + } catch (_) { + return null; + } +} + +/** + * Delete an entry directory, falling back to the container when the host cannot. + * + * Grav's Apache workers run as root, so every entry the form creates is + * root-owned. Removing one recursively needs write permission on that directory, + * which the host user does not have — so a plain fs.rmSync throws EACCES and the + * entry survives. That is how a ui-test entry ended up committed-adjacent in the + * active trip's content on 2026-07-24: cleanup had never actually worked for + * form-created entries, it just failed inside a path nothing checked. + * + * `docker exec … rm -rf` runs as root in the container, which can remove them. + */ +function removeEntryDir(dir) { + try { + fs.rmSync(dir, { recursive: true }); + return true; + } catch (err) { + if (err.code !== 'EACCES' && err.code !== 'EPERM') throw err; + } + const container = resolveGravContainer(); + if (!container) { + throw new Error( + `Cannot remove ${dir}: it is root-owned (written by Grav in the container) and no ` + + `matching container was found to delete it as root. Set GRAV_CONTAINER or remove it manually.` + ); + } + execSync(`docker exec ${container} rm -rf '/var/www/html/user/${path.relative(USER_DIR, dir)}'`, + { stdio: ['pipe', 'pipe', 'pipe'] }); + return true; +} + /** * Find a tracker entry folder by a unique slug fragment, then delete it. */ @@ -179,7 +233,7 @@ function cleanupEntry(slugFragment) { const entries = fs.readdirSync(TRACKER_DIR); const match = entries.find(e => e.includes(slugFragment)); if (match) { - fs.rmSync(path.join(TRACKER_DIR, match), { recursive: true }); + removeEntryDir(path.join(TRACKER_DIR, match)); } } @@ -202,4 +256,7 @@ function readEntryMd(entryDir) { return fs.readFileSync(path.join(entryDir, name), 'utf-8'); } -module.exports = { fillEditor, waitForPhotoUpload, postEntry, createPhotoEntry, cleanupEntry, findEntry, readEntryMd, TEST_PHOTO, TRACKER_DIR, ACTIVE_TRIP_URL }; +// USER_DIR is exported so global-setup/global-teardown resolve the same tree the +// specs assert against, instead of keeping their own (previously divergent) copy +// of this logic. +module.exports = { fillEditor, waitForPhotoUpload, postEntry, createPhotoEntry, cleanupEntry, removeEntryDir, findEntry, readEntryMd, TEST_PHOTO, USER_DIR, TRACKER_DIR, ACTIVE_TRIP_URL }; diff --git a/tests/ui/post/location-override.spec.js b/tests/ui/post/location-override.spec.js index e37ca4e..fefc65c 100644 --- a/tests/ui/post/location-override.spec.js +++ b/tests/ui/post/location-override.spec.js @@ -330,6 +330,10 @@ test('submitting with an unresolved lat/lng mismatch is blocked', async ({ page }); // ── U4: lazy-load boundary — an ordinary GPS-only submit never fetches maplibre-gl ── +// The URL pattern deliberately covers BOTH halves of the lazy boundary: the JS +// chunk (js/post/maplibre-gl-*.js) and the stylesheet +// (css-compiled/maplibre-gl.css, ed by location-map.js at panel-open — +// see its ensureMaplibreCss). Neither may be requested when the panel stays shut. test('an ordinary submit without opening the panel never fetches the maplibre-gl chunk', async ({ page }) => { const chunkRequests = []; page.on('request', (req) => { @@ -346,7 +350,32 @@ test('an ordinary submit without opening the panel never fetches the maplibre-gl await expect(page.locator('.notices')).toContainText('Entry posted successfully!', { timeout: 15_000 }); created.push(tag); - expect(chunkRequests, 'maplibre-gl must not be fetched when the panel is never opened').toHaveLength(0); + expect(chunkRequests, 'neither the maplibre-gl chunk nor its stylesheet may be fetched when the panel is never opened').toHaveLength(0); +}); + +// ── The other half of that boundary: opening the panel DOES apply the vendor CSS ── +// Without this, the guard above could keep passing while the stylesheet silently +// stopped loading at all (a broken href, a missed build step), leaving the map +// unstyled with nothing to catch it. Asserts the exists AND parsed — +// link.sheet is null until the browser has actually applied it. +test('opening the panel lazily links maplibre\'s stylesheet and applies it', async ({ page }) => { + await page.goto('/post'); + + const hrefBefore = await page.evaluate(() => Array.from(document.styleSheets) + .map((s) => s.href || '').filter((h) => /maplibre-gl\.css/.test(h))); + expect(hrefBefore, 'the vendor stylesheet must not be present before the panel opens').toHaveLength(0); + + await openLocationDetails(page); + await expect(page.locator('#location-map canvas.maplibregl-canvas')).toHaveCount(1, { timeout: 10_000 }); + + await expect.poll( + () => page.evaluate(() => { + const link = Array.from(document.querySelectorAll('link[rel="stylesheet"]')) + .find((l) => /maplibre-gl\.css/.test(l.href)); + return link ? link.sheet !== null : false; + }), + { message: 'maplibre\'s stylesheet must be linked and applied once the panel opens', timeout: 10_000 } + ).toBe(true); }); // ── Full submit: a search-picked location round-trips into the frontmatter ── diff --git a/user b/user index e873a9c..2b91aa3 160000 --- a/user +++ b/user @@ -1 +1 @@ -Subproject commit e873a9cb2341008c3d0fbe481e79000cac729c9d +Subproject commit 2b91aa30c33a183e1ef24987feb045045616fd3c From f9ab3b1561ca197d808eb0c3b7540d47eb16d854 Mon Sep 17 00:00:00 2001 From: Mischa Date: Fri, 24 Jul 2026 22:34:20 +0200 Subject: [PATCH 06/12] docs(working): record the green run, the lazy-link, and what's left before merge --- .../plans/2026-07-23-post-form-location-override.md | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/docs/working/plans/2026-07-23-post-form-location-override.md b/docs/working/plans/2026-07-23-post-form-location-override.md index dc88d67..8ee8fae 100644 --- a/docs/working/plans/2026-07-23-post-form-location-override.md +++ b/docs/working/plans/2026-07-23-post-form-location-override.md @@ -11,7 +11,17 @@ 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. **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. +**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, 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-``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. + +**Still open before merge:** +- The `user/` submodule commits are unpushed by choice (git-sync would deploy to prod), so the pin must be pushed and re-pointed at merge time. +- This worktree's `user/` branch has diverged from `user/`'s `main`, which is *ahead* on content — notably `denmark-2026` is `published: false` here but `true` on main. That 404s the active trip and cascades through the post specs, so the worktree carries an uncommitted local `published: true` for testing. Bring `user/` up to `main` before merging rather than committing that flip. +- Remaining UI failures are pre-existing on `main`, not from this branch: `site.yaml` pins `owner_username` to a real account while the suite authenticates as `testrunner`, so owner-only controls never render for it. Only `trip-publish.spec.js` patches that; `delete-flow`, `edit-mode` and `anon-view` do not. Separate branch. +- `make` is entirely broken in the **main** checkout: `-include .env` parses `.env` as makefile syntax and line 6 aborts with `*** missing separator`. Needs a value on that line fixed (or the loading approach changed) — not readable from here by policy. ## Goal Capsule From 63985428458fb736a740115e682df02766221142 Mon Sep 17 00:00:00 2001 From: Mischa Date: Fri, 24 Jul 2026 23:28:18 +0200 Subject: [PATCH 07/12] test(post): resolve USER_DIR via helpers; record why UG1/UG2/LD1 are red MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit lightbox-dims.spec.js hardcoded ../../../user, so a run against a checkout detached from the served tree planted its fixture in a different user/ than Grav renders and LD1 failed as an opaque "card never appeared" timeout. Take USER_DIR from helpers instead, which honours GRAV_USER_DIR. The three specs in this folder that fail do so for real, pre-existing reasons, and both files' headers implied otherwise: - UG1/UG2 specify a submit gate that is not implemented. post-form.js's only create-form guard is `converting > 0` (pre-FilePond HEIC conversion); it never inspects FilePond item state at submit time, and .photo-convert-status is created lazily by photoStatusEl() only from the HEIC paths — so for a plain JPEG the element never exists and both expectations fail as "element(s) not found". UG2 is the one that matters: a failed upload keeping its thumbnail is unguarded silent data loss. - LD1's header described its root cause in the past tense, reading as fixed. entry-journal.html.twig:48-49 still emits {{ img.width }} / {{ img.height }}, so EXIF-rotated photos still declare pre-rotation dims and PhotoSwipe still squeezes them. Left failing rather than skipped, per retries:0 — a red test here is a real defect, and hiding these would lose both. Co-Authored-By: Claude Opus 5 --- tests/ui/post/lightbox-dims.spec.js | 22 +++++++++++++++++----- tests/ui/post/upload-gate.spec.js | 14 ++++++++++++++ 2 files changed, 31 insertions(+), 5 deletions(-) diff --git a/tests/ui/post/lightbox-dims.spec.js b/tests/ui/post/lightbox-dims.spec.js index 7371ab6..0efc762 100644 --- a/tests/ui/post/lightbox-dims.spec.js +++ b/tests/ui/post/lightbox-dims.spec.js @@ -3,11 +3,19 @@ // browser actually renders for the linked image (BUG 2026-07-09: portrait // iPhone JPEGs squeezed to landscape in the fullscreen lightbox). // -// Root cause: entry-journal.html.twig fed `img.width`/`img.height` (raw +// Root cause: entry-journal.html.twig feeds `img.width`/`img.height` (raw // getimagesize() of the ORIGINAL file — EXIF orientation ignored) into -// data-pswp-*, while the slide href pointed at that original, which browsers -// display EXIF-rotated. For a stored-landscape portrait photo the attrs said -// landscape while the pixels rendered portrait → PhotoSwipe squeezed them. +// data-pswp-*, while the slide href points at that original, which browsers +// display EXIF-rotated. For a stored-landscape portrait photo the attrs say +// landscape while the pixels render portrait → PhotoSwipe squeezes them. +// +// ⚠️ THIS SPEC CURRENTLY FAILS — the root cause above is still live. +// entry-journal.html.twig:48-49 remains `{{ img.width }}` / `{{ img.height }}`, +// so the fixture (stored 800x600, EXIF Orientation=6) reports 800 while the +// browser renders 600. Fixing it means sourcing the dimensions from a medium +// Grav has already oriented rather than the raw original — which cannot be +// verified locally, since the dev container has no php-exif and so never +// applies auto_fix_orientation. Left failing so the squeeze stays visible. // // The invariant tested here is environment-proof: whatever file the slide // links to, its browser-rendered natural size must equal the data-pswp-* @@ -24,10 +32,14 @@ const { test, expect } = require('@playwright/test'); const path = require('path'); const fs = require('fs'); const { execSync } = require('child_process'); +// USER_DIR comes from helpers so GRAV_USER_DIR is honoured — without it a run +// against a checkout detached from the served tree plants the fixture in a +// different user/ than Grav renders, and LD1 fails as an opaque "card never +// appeared" timeout. +const { USER_DIR } = require('../helpers'); // Stored 800x600 with EXIF Orientation=6: browsers render it 600x800 portrait. const EXIF_PORTRAIT = path.join(__dirname, '../../fixtures/test-photo-exif-portrait.jpg'); -const USER_DIR = path.join(__dirname, '../../../user'); const DEMO_DAILIES = path.join(USER_DIR, 'pages/01.trips/italy-2026-demo/01.dailies'); const DEMO_TRIP_URL = '/trips/italy-2026-demo'; diff --git a/tests/ui/post/upload-gate.spec.js b/tests/ui/post/upload-gate.spec.js index 69ab3ec..2b5e553 100644 --- a/tests/ui/post/upload-gate.spec.js +++ b/tests/ui/post/upload-gate.spec.js @@ -12,6 +12,20 @@ // silent-data-loss path. // post-form.js owns the complete gate (theme code; the form plugin is // GPM-managed and not patchable in-repo). +// +// ⚠️ BOTH CASES CURRENTLY FAIL — the gate they specify is NOT implemented. +// post-form.js's only create-form submit guard is `converting > 0` (the +// pre-FilePond HEIC conversion, "Hang on — a photo is still converting."). It +// never inspects FilePond's item state at submit time. `.photo-convert-status` +// is created lazily by photoStatusEl(), which only runs from setStatus() on the +// HEIC paths — so for a plain JPEG the element never exists and both +// expectations below fail as "element(s) not found", not as a wrong message. +// refreshCollapse() does read data-filepond-item-state, but only to word the +// ("Uploading N photos…"); it gates nothing. +// These are therefore red specs describing intended behaviour. UG2 is the one +// that matters: a failed upload keeping its thumbnail is a silent-data-loss +// path with no guard. Left failing rather than skipped so the gap stays visible +// — see the plan's open items. const { test, expect } = require('@playwright/test'); const { fillEditor, findEntry, cleanupEntry, TEST_PHOTO } = require('../helpers'); From b02f27f559ad3c04db888b177e02bb8fe2dedd7b Mon Sep 17 00:00:00 2001 From: Mischa Date: Fri, 24 Jul 2026 23:28:49 +0200 Subject: [PATCH 08/12] docs(working): record three real defects behind the red post specs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Also corrects the green-run line: "test-post 6/6" is the scripts/test-post.sh shell suite, not the Playwright specs under tests/ui/post/ — conflating the two made the Playwright post specs look covered when they were never run. Co-Authored-By: Claude Opus 5 --- .../working/plans/2026-07-23-post-form-location-override.md | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/docs/working/plans/2026-07-23-post-form-location-override.md b/docs/working/plans/2026-07-23-post-form-location-override.md index 8ee8fae..6f5b0d2 100644 --- a/docs/working/plans/2026-07-23-post-form-location-override.md +++ b/docs/working/plans/2026-07-23-post-form-location-override.md @@ -13,7 +13,7 @@ execution: code **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, 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`. +**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-``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. @@ -22,6 +22,10 @@ Also landed after the review: maplibre's stylesheet is now lazy-``ed at pa - This worktree's `user/` branch has diverged from `user/`'s `main`, which is *ahead* on content — notably `denmark-2026` is `published: false` here but `true` on main. That 404s the active trip and cascades through the post specs, so the worktree carries an uncommitted local `published: true` for testing. Bring `user/` up to `main` before merging rather than committing that flip. - Remaining UI failures are pre-existing on `main`, not from this branch: `site.yaml` pins `owner_username` to a real account while the suite authenticates as `testrunner`, so owner-only controls never render for it. Only `trip-publish.spec.js` patches that; `delete-flow`, `edit-mode` and `anon-view` do not. Separate branch. - `make` is entirely broken in the **main** checkout: `-include .env` parses `.env` as makefile syntax and line 6 aborts with `*** missing separator`. Needs a value on that line fixed (or the loading approach changed) — not readable from here by policy. +- **Three specs under `tests/ui/post/` fail on real, pre-existing defects** (found while reviewing main's uncommitted work; headers now record each, left red rather than skipped): + - **UG2 — unguarded silent data loss, the one worth fixing first.** A photo whose upload fails keeps its thumbnail, satisfies the ≥1-photo validation, and the entry posts without the photo. `post-form.js`'s only create-form submit guard is `converting > 0` (pre-FilePond HEIC conversion); FilePond item state is never consulted at submit time. Directly relevant to posting from the road on a weak connection. + - **UG1** — same missing gate, in-flight rather than failed. Both fail as `.photo-convert-status` "element(s) not found", because `photoStatusEl()` only ever runs from the HEIC paths. + - **LD1** — EXIF-rotated photos still squeeze in the lightbox. `partials/entry-journal.html.twig:48-49` emits `{{ img.width }}`/`{{ img.height }}` (raw `getimagesize()`, orientation ignored) while the slide links the original, which browsers render rotated. Needs dimensions from a medium Grav has already oriented; **not verifiable locally** — the dev container has no php-exif, so `auto_fix_orientation` never applies. ## Goal Capsule From d57041d316f6dc159b5e15fa481356c27f62b7f1 Mon Sep 17 00:00:00 2001 From: Mischa Date: Fri, 24 Jul 2026 23:40:48 +0200 Subject: [PATCH 09/12] =?UTF-8?q?test(post):=20retract=20the=20"these=20sp?= =?UTF-8?q?ecs=20are=20red"=20notes=20=E2=80=94=20the=20merge=20fixed=20th?= =?UTF-8?q?em?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The warnings added in 6398542 were wrong. UG1, UG2 and LD1 were failing because this branch predated e17a5dc, not because the behaviour they assert was missing: merging user/main brought the FilePond upload gate and the oriented-derivative slide dims, and all three pass with no product change. Headers now point at e17a5dc for both mechanisms. Also corrects the plan's .env note — the env layering is intentional (.env global, .env. per environment via the generated remote-*- targets); the actual fault is just that `-include .env` additionally requires makefile-valid syntax and line 6 is not, which breaks make in both non-worktree clones. Co-Authored-By: Claude Opus 5 --- .../2026-07-23-post-form-location-override.md | 10 ++++------ tests/ui/post/lightbox-dims.spec.js | 18 +++++++----------- tests/ui/post/upload-gate.spec.js | 18 +++++------------- 3 files changed, 16 insertions(+), 30 deletions(-) diff --git a/docs/working/plans/2026-07-23-post-form-location-override.md b/docs/working/plans/2026-07-23-post-form-location-override.md index 6f5b0d2..c4eb902 100644 --- a/docs/working/plans/2026-07-23-post-form-location-override.md +++ b/docs/working/plans/2026-07-23-post-form-location-override.md @@ -19,13 +19,11 @@ Also landed after the review: maplibre's stylesheet is now lazy-``ed at pa **Still open before merge:** - The `user/` submodule commits are unpushed by choice (git-sync would deploy to prod), so the pin must be pushed and re-pointed at merge time. -- This worktree's `user/` branch has diverged from `user/`'s `main`, which is *ahead* on content — notably `denmark-2026` is `published: false` here but `true` on main. That 404s the active trip and cascades through the post specs, so the worktree carries an uncommitted local `published: true` for testing. Bring `user/` up to `main` before merging rather than committing that flip. +- ~~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 (the leg-connection map fix and the U+200E coordinate strip) — separate clone, not a worktree. They need to reach `user/main` before the pin is bumped. - Remaining UI failures are pre-existing on `main`, not from this branch: `site.yaml` pins `owner_username` to a real account while the suite authenticates as `testrunner`, so owner-only controls never render for it. Only `trip-publish.spec.js` patches that; `delete-flow`, `edit-mode` and `anon-view` do not. Separate branch. -- `make` is entirely broken in the **main** checkout: `-include .env` parses `.env` as makefile syntax and line 6 aborts with `*** missing separator`. Needs a value on that line fixed (or the loading approach changed) — not readable from here by policy. -- **Three specs under `tests/ui/post/` fail on real, pre-existing defects** (found while reviewing main's uncommitted work; headers now record each, left red rather than skipped): - - **UG2 — unguarded silent data loss, the one worth fixing first.** A photo whose upload fails keeps its thumbnail, satisfies the ≥1-photo validation, and the entry posts without the photo. `post-form.js`'s only create-form submit guard is `converting > 0` (pre-FilePond HEIC conversion); FilePond item state is never consulted at submit time. Directly relevant to posting from the road on a weak connection. - - **UG1** — same missing gate, in-flight rather than failed. Both fail as `.photo-convert-status` "element(s) not found", because `photoStatusEl()` only ever runs from the HEIC paths. - - **LD1** — EXIF-rotated photos still squeeze in the lightbox. `partials/entry-journal.html.twig:48-49` emits `{{ img.width }}`/`{{ img.height }}` (raw `getimagesize()`, orientation ignored) while the slide links the original, which browsers render rotated. Needs dimensions from a medium Grav has already oriented; **not verifiable locally** — the dev container has no php-exif, so `auto_fix_orientation` never applies. +- Every `make` target aborts with `.env:6: *** missing separator` in **both** non-worktree clones (`~/Projects` and `~/Nextcloud/Projects`; reproduced with `make test-config` in each). Worktrees are unaffected only because `worktree-new` creates no `.env`, so the `-include` silently skips — which is why all the testing above ran. The env layering itself is correct and intended: `.env` global, `-include .env.$(ENV)` per-environment, `ENV` set automatically by the generated env-suffixed remote targets (`make remote-install-prod`). The fragility is narrower — because `.env` is pulled in with `-include`, it must be valid **makefile** syntax as well as valid dotenv, and line 6 currently is not. Typical causes: a leading tab (make reads it as a recipe line), a value spanning multiple lines, or a line without `=`. +- **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 diff --git a/tests/ui/post/lightbox-dims.spec.js b/tests/ui/post/lightbox-dims.spec.js index 0efc762..e6864dc 100644 --- a/tests/ui/post/lightbox-dims.spec.js +++ b/tests/ui/post/lightbox-dims.spec.js @@ -3,19 +3,15 @@ // browser actually renders for the linked image (BUG 2026-07-09: portrait // iPhone JPEGs squeezed to landscape in the fullscreen lightbox). // -// Root cause: entry-journal.html.twig feeds `img.width`/`img.height` (raw +// Root cause: entry-journal.html.twig fed `img.width`/`img.height` (raw // getimagesize() of the ORIGINAL file — EXIF orientation ignored) into -// data-pswp-*, while the slide href points at that original, which browsers -// display EXIF-rotated. For a stored-landscape portrait photo the attrs say -// landscape while the pixels render portrait → PhotoSwipe squeezes them. +// data-pswp-*, while the slide href pointed at that original, which browsers +// display EXIF-rotated. For a stored-landscape portrait photo the attrs said +// landscape while the pixels rendered portrait → PhotoSwipe squeezed them. // -// ⚠️ THIS SPEC CURRENTLY FAILS — the root cause above is still live. -// entry-journal.html.twig:48-49 remains `{{ img.width }}` / `{{ img.height }}`, -// so the fixture (stored 800x600, EXIF Orientation=6) reports 800 while the -// browser renders 600. Fixing it means sourcing the dimensions from a medium -// Grav has already oriented rather than the raw original — which cannot be -// verified locally, since the dev container has no php-exif and so never -// applies auto_fix_orientation. Left failing so the squeeze stays visible. +// Fixed in e17a5dc: slides now link a 2000px fit-within derivative and measure +// THAT file, and derivatives are re-encoded upright, so the attrs and the +// rendered pixels agree. // // The invariant tested here is environment-proof: whatever file the slide // links to, its browser-rendered natural size must equal the data-pswp-* diff --git a/tests/ui/post/upload-gate.spec.js b/tests/ui/post/upload-gate.spec.js index 2b5e553..d873205 100644 --- a/tests/ui/post/upload-gate.spec.js +++ b/tests/ui/post/upload-gate.spec.js @@ -13,19 +13,11 @@ // post-form.js owns the complete gate (theme code; the form plugin is // GPM-managed and not patchable in-repo). // -// ⚠️ BOTH CASES CURRENTLY FAIL — the gate they specify is NOT implemented. -// post-form.js's only create-form submit guard is `converting > 0` (the -// pre-FilePond HEIC conversion, "Hang on — a photo is still converting."). It -// never inspects FilePond's item state at submit time. `.photo-convert-status` -// is created lazily by photoStatusEl(), which only runs from setStatus() on the -// HEIC paths — so for a plain JPEG the element never exists and both -// expectations below fail as "element(s) not found", not as a wrong message. -// refreshCollapse() does read data-filepond-item-state, but only to word the -// ("Uploading N photos…"); it gates nothing. -// These are therefore red specs describing intended behaviour. UG2 is the one -// that matters: a failed upload keeping its thumbnail is a silent-data-loss -// path with no guard. Left failing rather than skipped so the gap stays visible -// — see the plan's open items. +// The gate lives in e17a5dc: submit is blocked unless EVERY FilePond item is +// processing-complete, with distinct messages for the failed and still-uploading +// cases. Both assert on .photo-convert-status, which post-form.js's setStatus() +// creates via photoStatusEl() — so a passing expectation here proves the THEME +// gate fired, not the form plugin's, whose own guard only raises alert(). const { test, expect } = require('@playwright/test'); const { fillEditor, findEntry, cleanupEntry, TEST_PHOTO } = require('../helpers'); From 28bbd418684e3c5a9157e6cc9251ce7187774e4d Mon Sep 17 00:00:00 2001 From: Mischa Date: Fri, 24 Jul 2026 23:47:37 +0200 Subject: [PATCH 10/12] docs(working): record the submodule-git-dir cause and the .env fix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ~/Projects is a symlink to ~/Nextcloud/Projects — one directory, not two clones. The differing user/main refs came from the worktree having its own submodule git dir (.git/worktrees//modules/user), which is worth knowing: submodule commits made from the main checkout stay invisible in a worktree until fetched, and a local fetch moves them without a push. Co-Authored-By: Claude Opus 5 --- .../plans/2026-07-23-post-form-location-override.md | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/docs/working/plans/2026-07-23-post-form-location-override.md b/docs/working/plans/2026-07-23-post-form-location-override.md index c4eb902..3469195 100644 --- a/docs/working/plans/2026-07-23-post-form-location-override.md +++ b/docs/working/plans/2026-07-23-post-form-location-override.md @@ -18,11 +18,12 @@ execution: code Also landed after the review: maplibre's stylesheet is now lazy-``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. **Still open before merge:** -- The `user/` submodule commits are unpushed by choice (git-sync would deploy to prod), so the pin must be pushed and re-pointed at merge time. +- 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. +- Only one class of failure is left in `tests/ui/post/` + `tests/ui/map`: **64 passed, 6 failed**, all the `owner_username` cluster below. Nothing in this feature's scope is red. - ~~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 (the leg-connection map fix and the U+200E coordinate strip) — separate clone, not a worktree. They need to reach `user/main` before the pin is bumped. +- ~~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//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. - Remaining UI failures are pre-existing on `main`, not from this branch: `site.yaml` pins `owner_username` to a real account while the suite authenticates as `testrunner`, so owner-only controls never render for it. Only `trip-publish.spec.js` patches that; `delete-flow`, `edit-mode` and `anon-view` do not. Separate branch. -- Every `make` target aborts with `.env:6: *** missing separator` in **both** non-worktree clones (`~/Projects` and `~/Nextcloud/Projects`; reproduced with `make test-config` in each). Worktrees are unaffected only because `worktree-new` creates no `.env`, so the `-include` silently skips — which is why all the testing above ran. The env layering itself is correct and intended: `.env` global, `-include .env.$(ENV)` per-environment, `ENV` set automatically by the generated env-suffixed remote targets (`make remote-install-prod`). The fragility is narrower — because `.env` is pulled in with `-include`, it must be valid **makefile** syntax as well as valid dotenv, and line 6 currently is not. Typical causes: a leading tab (make reads it as a recipe line), a value spanning multiple lines, or a line without `=`. +- ~~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 From 3250ad366a729fb7a66fe3676fcd81d2d725649e Mon Sep 17 00:00:00 2001 From: Mischa Date: Fri, 24 Jul 2026 23:53:24 +0200 Subject: [PATCH 11/12] docs(working): close the plan; retract the owner_username diagnosis MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../plans/2026-07-23-post-form-location-override.md | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/docs/working/plans/2026-07-23-post-form-location-override.md b/docs/working/plans/2026-07-23-post-form-location-override.md index 3469195..6b1c63b 100644 --- a/docs/working/plans/2026-07-23-post-form-location-override.md +++ b/docs/working/plans/2026-07-23-post-form-location-override.md @@ -17,12 +17,14 @@ execution: code Also landed after the review: maplibre's stylesheet is now lazy-``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. -**Still open before merge:** +**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. -- Only one class of failure is left in `tests/ui/post/` + `tests/ui/map`: **64 passed, 6 failed**, all the `owner_username` cluster below. Nothing in this feature's scope is red. +- **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//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. -- Remaining UI failures are pre-existing on `main`, not from this branch: `site.yaml` pins `owner_username` to a real account while the suite authenticates as `testrunner`, so owner-only controls never render for it. Only `trip-publish.spec.js` patches that; `delete-flow`, `edit-mode` and `anon-view` do not. Separate branch. +- **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. From cfe070efec56e977b2a906697662f0c65305cde9 Mon Sep 17 00:00:00 2001 From: Mischa Date: Fri, 24 Jul 2026 23:54:43 +0200 Subject: [PATCH 12/12] fix(make): worktree-rm no longer unregisters user/ for the main checkout MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `submodule deinit` is needed before `worktree remove` (a populated user/ blocks it), but worktrees share .git/config — so deiniting inside the worktree stripped submodule.user.url globally. After any `make worktree-rm` the main checkout's `git submodule status` reported `-` (not initialised) while user/ sat there fully intact, and a later `submodule update` would have had no URL to work from. Re-register with an idempotent `submodule init` after the removal. Found by tearing down the post-location-override worktree. Co-Authored-By: Claude Opus 5 --- Makefile | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/Makefile b/Makefile index 6153a22..0b51158 100644 --- a/Makefile +++ b/Makefile @@ -200,6 +200,12 @@ worktree-rm: guard-name -git -C "$(WT_DIR)" submodule deinit -f user git worktree remove --force "$(WT_DIR)" git worktree prune + # The deinit above is required (a populated user/ blocks `worktree remove`), + # but worktrees SHARE .git/config — so it also strips submodule.user.url for + # the MAIN checkout, leaving `git submodule status` there showing `-` (not + # initialised) even though user/ is intact. Re-register it; init is + # idempotent and touches config only, never the working tree. + git submodule init @echo "Removed $(WT_DIR). If feat/$(NAME) is merged, drop it: git branch -d feat/$(NAME)" # ── Demo content ──────────────────────────────────────────────────────────────