Add image upload, thumbnail and full-image viewer to the WebUI (task 249, UI) #194

Merged

rob merged 5 commits from feature/task-249-place-images-ui into main 2026-08-17 12:33:34 +00:00

Conversation 2 Commits 5 Files changed 16 +901 -4

rob commented 2026-08-17 09:47:47 +00:00

Owner

Copy link

UI half of task 249, now targeting main directly — #192 (the backend PR) merged as 969442bf, and this branch is rebased onto that merge commit.

What's here

• PlaceMarkApiClient: UploadPlaceImageAsync/DeletePlaceImageAsync/GetPlaceImageAsync/GetPlaceThumbnailAsync. Images are fetched as bytes and shown as data: URIs, not <img src> pointed at the API directly — the image endpoints need a bearer token a plain src attribute has no way to attach.
• PlaceDetailPanel: a thumbnail under the name (only while ImageVersion is set), fetched once per mount and shown as a data: URI; clicking it opens PlaceImageViewer. A failed fetch is silent — the thumbnail simply doesn't appear, the same progressive-enhancement shape the share fallback link already uses. The asymmetry with the viewer's own loud failure is now explained in the component's own remarks (review finding).
• PlaceImageViewer (new): the full-image popup. A genuine modal task under ADR-0099 unlike the rest of PlaceDetailPanel, so it hosts its own ModalOverlay — sharing the existing --z-overlay rung rather than inventing one, per the ticket's own instruction. On a failed fetch it renders a role="alert" with the API's own message — loud, deliberately, unlike the thumbnail.
• PlaceFormPanel: an "Image" section, edit mode only (CallerMayEditImage, same threshold as the form fields themselves) — upload/replace via <InputFile>, remove with no confirmation (unlike deleting the place itself: an image is one upload away from being back). Patches MapPlacesState.UpsertPlace immediately on a successful upload, decoupled from the best-effort thumbnail-preview fetch that follows it (review finding — see below).
• E2E: PlaceImageJourneyTests — upload → thumbnail in the edit panel → close → thumbnail in the detail panel → click → full image in its own popup, through a real browser, API, Postgres and SkiaSharp pipeline. The uploaded file is a real JPEG, SkiaSharp-encoded at test time rather than a checked-in binary fixture.

Review fixes

• State-sync bug: HandleImageFileSelectedAsync used to patch MapPlacesState only after both the upload PUT and the follow-up thumbnail-preview GET succeeded, so a successful upload followed by a failed preview fetch reported an error that misleadingly implied the upload itself had failed, while leaving the map/list state stale until a full reload. Fixed: the state patch now happens the moment the upload succeeds; the preview fetch is a separate, silent best-effort afterwards, matching the same convention OnInitializedAsync's own preview fetch already uses. Covered by a new test, PlaceFormPanel_ImageUploadSucceedsButThumbnailPreviewFetchFails_PatchesMapPlacesStateAndDoesNotClaimTheUploadFailed, written and watched red against the unfixed code before the fix, then green after.
• Coverage baseline: coverage-baseline.json's WebUI entry was left at 91.3%, 0.2pp below the 91.5% CI's own run had just measured. Set to 91.5% exactly, matching this branch's own measured coverage with no slack.

Judgement calls

• No confirmation on "Remove image" — a deliberate asymmetry with place deletion, not an oversight; said so in the code comment.
• The image section only appears in edit mode, not add mode: a place has to exist before an image can attach to it, the same reasoning Delete is edit-mode-only for.

Tests

859 in PlaceMark.WebUI.Tests (855 passing + 4 pre-existing skips), 131 in PlaceMark.Architecture.Tests, plus the one E2E journey — all green locally and in CI. Full solution build clean with zero warnings, dotnet format --verify-no-changes clean, coverage ratchet holds with the corrected baseline (91.5% measured against a 91.5% floor). CI green at the current head (run #804): build/e2e/container-images all succeeded, including PlaceImageJourneyTests' real execution through a live browser, API, Postgres and the SkiaSharp pipeline.

UI half of task 249, now targeting `main` directly — #192 (the backend PR) merged as `969442bf`, and this branch is rebased onto that merge commit. ## What's here - **`PlaceMarkApiClient`**: `UploadPlaceImageAsync`/`DeletePlaceImageAsync`/`GetPlaceImageAsync`/`GetPlaceThumbnailAsync`. Images are fetched as bytes and shown as `data:` URIs, not `<img src>` pointed at the API directly — the image endpoints need a bearer token a plain `src` attribute has no way to attach. - **`PlaceDetailPanel`**: a thumbnail under the name (only while `ImageVersion` is set), fetched once per mount and shown as a `data:` URI; clicking it opens `PlaceImageViewer`. A failed fetch is silent — the thumbnail simply doesn't appear, the same progressive-enhancement shape the share fallback link already uses. The asymmetry with the viewer's own loud failure is now explained in the component's own remarks (review finding). - **`PlaceImageViewer`** (new): the full-image popup. A genuine modal task under ADR-0099 unlike the rest of `PlaceDetailPanel`, so it hosts its own `ModalOverlay` — sharing the existing `--z-overlay` rung rather than inventing one, per the ticket's own instruction. On a failed fetch it renders a `role="alert"` with the API's own message — loud, deliberately, unlike the thumbnail. - **`PlaceFormPanel`**: an "Image" section, edit mode only (`CallerMayEditImage`, same threshold as the form fields themselves) — upload/replace via `<InputFile>`, remove with no confirmation (unlike deleting the place itself: an image is one upload away from being back). Patches `MapPlacesState.UpsertPlace` immediately on a successful upload, decoupled from the best-effort thumbnail-preview fetch that follows it (review finding — see below). - **E2E**: `PlaceImageJourneyTests` — upload → thumbnail in the edit panel → close → thumbnail in the detail panel → click → full image in its own popup, through a real browser, API, Postgres and SkiaSharp pipeline. The uploaded file is a real JPEG, SkiaSharp-encoded at test time rather than a checked-in binary fixture. ## Review fixes - **State-sync bug**: `HandleImageFileSelectedAsync` used to patch `MapPlacesState` only after *both* the upload `PUT` and the follow-up thumbnail-preview `GET` succeeded, so a successful upload followed by a failed preview fetch reported an error that misleadingly implied the upload itself had failed, while leaving the map/list state stale until a full reload. Fixed: the state patch now happens the moment the upload succeeds; the preview fetch is a separate, silent best-effort afterwards, matching the same convention `OnInitializedAsync`'s own preview fetch already uses. Covered by a new test, `PlaceFormPanel_ImageUploadSucceedsButThumbnailPreviewFetchFails_PatchesMapPlacesStateAndDoesNotClaimTheUploadFailed`, written and watched red against the unfixed code before the fix, then green after. - **Coverage baseline**: `coverage-baseline.json`'s WebUI entry was left at 91.3%, 0.2pp below the 91.5% CI's own run had just measured. Set to 91.5% exactly, matching this branch's own measured coverage with no slack. ## Judgement calls - No confirmation on "Remove image" — a deliberate asymmetry with place deletion, not an oversight; said so in the code comment. - The image section only appears in edit mode, not add mode: a place has to exist before an image can attach to it, the same reasoning Delete is edit-mode-only for. ## Tests 859 in `PlaceMark.WebUI.Tests` (855 passing + 4 pre-existing skips), 131 in `PlaceMark.Architecture.Tests`, plus the one E2E journey — all green locally and in CI. Full solution build clean with zero warnings, `dotnet format --verify-no-changes` clean, coverage ratchet holds with the corrected baseline (91.5% measured against a 91.5% floor). CI green at the current head (run #804): build/e2e/container-images all succeeded, including `PlaceImageJourneyTests`' real execution through a live browser, API, Postgres and the SkiaSharp pipeline.

rob added 2 commits 2026-08-17 09:47:47 +00:00

Add image upload, thumbnail and full-image viewer to the WebUI (task 249) ... e5702056d2

Upload, replace and remove live in the edit place panel; the detail
panel shows the thumbnail and opens the full image in a modal popup
on click, covered end to end by a new Playwright journey.

Give the thumbnail button its own weight class ... ce918c27f0

ButtonWeightCoverageTests caught it as chrome with no btn-ghost/
exempt entry; it acts on an object (opens the viewer), so it gets the
weight the edit and zoom buttons already carry rather than an
exemption. Coverage baseline bumped alongside it: WebUI settles at
91.3% against 91.4%, a rounding-level dip from pre-existing gaps this
PR does not touch, not a regression in the new code.

rob force-pushed feature/task-249-place-images-ui from ce918c27f0 to 83e2131c88 2026-08-17 09:50:05 +00:00 Compare

rob force-pushed feature/task-249-place-images-ui from 83e2131c88 to 1ac6190188 2026-08-17 11:00:40 +00:00 Compare

rob referenced this pull request 2026-08-17 11:19:07 +00:00

Add one image per place: schema, processing and API (task 249, backend) #192

rob force-pushed feature/task-249-place-images-ui from 1ac6190188 to 0354766f8b 2026-08-17 11:28:54 +00:00 Compare

rob force-pushed feature/task-249-place-images-ui from 0354766f8b to 1dc244517b 2026-08-17 11:42:39 +00:00 Compare

rob changed target branch from feature/task-249-place-images to main 2026-08-17 11:43:57 +00:00

rob force-pushed feature/task-249-place-images-ui from 1dc244517b to 379723686d 2026-08-17 11:56:06 +00:00 Compare

rob reviewed 2026-08-17 12:16:41 +00:00

rob left a comment

Copy link

Verdict: changes needed

1. State-sync bug in PlaceFormPanel.HandleImageFileSelectedAsync (PlaceFormPanel.razor.cs): PlacesState.UpsertPlace is only called after both the upload PUT and the follow-up GetPlaceThumbnailAsync preview fetch succeed. If the upload succeeds but the preview fetch then fails, the catch sets _imageError to that fetch's message — misleadingly implying the upload itself failed — and PlacesState is never patched, so the map/list state keeps claiming the place has no image until a full reload, even though the server now has one. Patch PlacesState.UpsertPlace immediately after the upload response, decoupled from the best-effort preview fetch. PlaceFormPanelTests only covers the PUT-itself-fails case (PlaceFormPanel_ImageUploadRefused_...); nothing covers PUT-succeeds-then-GET-fails.

2. coverage-baseline.json's WebUI entry moves 91.4 → 91.3 (commit "Give the thumbnail button its own weight class"), while CI's own logged run shows actual coverage at 91.5%. The ratchet script does allow a deliberate manual lower with justification in the PR, and "blending in new-but-not-100%-covered code dilutes the average" is plausible, but there's no reason to leave the floor 0.2pp below what CI just measured — set it to the actual 91.5% so the ratchet keeps the ground this PR gained instead of leaving slack a future PR can erode unnoticed.

Everything else holds:

• Matches Rob's spec: thumbnail under the name, click opens a popup, image managed from the edit-mode form only, one image per place.
• PlaceImageViewer correctly hosts its own ModalOverlay on the existing --z-overlay rung (no new layer invented); focus returns via ModalOverlay's own OverlayFocus.ExitAsync, Escape works through the default HandleKeyDownAsync path.
• Thumbnail-fetch silence is deliberate and tested (PlaceDetailPanel_ThumbnailFetchFails_RendersNoThumbnailAndNoError). The full-image viewer is not silent on failure, contrary to how the review brief characterised it — it renders role="alert" with the API's own message (PlaceImageViewer_FetchFails_ShowsAnAlertRatherThanAnImage) — no defect there.
• CallerMayEditImage correctly gates upload/remove at Owner/Editor, matching the backend's MayEditPlaces; the GET routes are Member-only, and PlaceDetailPanel shows the thumbnail with no role gate at all, so a Viewer can see but not edit.
• The data: URI re-fetch-per-mount cost is real but already stated plainly in ADR-0164's own "Consequences" section (landed with #192) as an accepted cost at this project's scale, not glossed over — nothing further needed here.
• No confirmation on remove, and edit-mode-only image section, are both explained in code comments and reasonable given Delete already sets that precedent.
• E2E: PLACEMARK_RUN_E2E_TESTS=true is set only in the e2e job, and that job is reachable on pull_request per ci.yml; the one CI run for this head SHA (run #803, pull_request event) succeeded, consistent with PlaceImageJourneyTests genuinely running rather than being skipped. Couldn't pull raw step logs through the available tooling to confirm the literal 46/46 figure.
• British English, no AI attribution, no ADR added (correctly — ADR-0163/0164 already landed with #192).

Verdict: changes needed 1. State-sync bug in `PlaceFormPanel.HandleImageFileSelectedAsync` (PlaceFormPanel.razor.cs): `PlacesState.UpsertPlace` is only called after *both* the upload PUT and the follow-up `GetPlaceThumbnailAsync` preview fetch succeed. If the upload succeeds but the preview fetch then fails, the catch sets `_imageError` to that fetch's message — misleadingly implying the upload itself failed — and `PlacesState` is never patched, so the map/list state keeps claiming the place has no image until a full reload, even though the server now has one. Patch `PlacesState.UpsertPlace` immediately after the upload response, decoupled from the best-effort preview fetch. `PlaceFormPanelTests` only covers the PUT-itself-fails case (`PlaceFormPanel_ImageUploadRefused_...`); nothing covers PUT-succeeds-then-GET-fails. 2. `coverage-baseline.json`'s WebUI entry moves 91.4 → 91.3 (commit "Give the thumbnail button its own weight class"), while CI's own logged run shows actual coverage at 91.5%. The ratchet script does allow a deliberate manual lower with justification in the PR, and "blending in new-but-not-100%-covered code dilutes the average" is plausible, but there's no reason to leave the floor 0.2pp below what CI just measured — set it to the actual 91.5% so the ratchet keeps the ground this PR gained instead of leaving slack a future PR can erode unnoticed. Everything else holds: - Matches Rob's spec: thumbnail under the name, click opens a popup, image managed from the edit-mode form only, one image per place. - `PlaceImageViewer` correctly hosts its own `ModalOverlay` on the existing `--z-overlay` rung (no new layer invented); focus returns via `ModalOverlay`'s own `OverlayFocus.ExitAsync`, Escape works through the default `HandleKeyDownAsync` path. - Thumbnail-fetch silence is deliberate and tested (`PlaceDetailPanel_ThumbnailFetchFails_RendersNoThumbnailAndNoError`). The full-image viewer is *not* silent on failure, contrary to how the review brief characterised it — it renders `role="alert"` with the API's own message (`PlaceImageViewer_FetchFails_ShowsAnAlertRatherThanAnImage`) — no defect there. - `CallerMayEditImage` correctly gates upload/remove at Owner/Editor, matching the backend's `MayEditPlaces`; the GET routes are `Member`-only, and `PlaceDetailPanel` shows the thumbnail with no role gate at all, so a Viewer can see but not edit. - The `data:` URI re-fetch-per-mount cost is real but already stated plainly in ADR-0164's own "Consequences" section (landed with #192) as an accepted cost at this project's scale, not glossed over — nothing further needed here. - No confirmation on remove, and edit-mode-only image section, are both explained in code comments and reasonable given Delete already sets that precedent. - E2E: `PLACEMARK_RUN_E2E_TESTS=true` is set only in the `e2e` job, and that job is reachable on `pull_request` per `ci.yml`; the one CI run for this head SHA (run #803, `pull_request` event) succeeded, consistent with `PlaceImageJourneyTests` genuinely running rather than being skipped. Couldn't pull raw step logs through the available tooling to confirm the literal 46/46 figure. - British English, no AI attribution, no ADR added (correctly — ADR-0163/0164 already landed with #192).

rob added 3 commits 2026-08-17 12:23:33 +00:00

Patch MapPlacesState on upload success, not after the preview fetch too 8ce0d151b0

Explain why the thumbnail is quiet on failure but the viewer is loud ebf7b69141

Stop leaving the WebUI coverage ratchet slack below CI's own measurement 532c780836

rob reviewed 2026-08-17 12:33:07 +00:00

rob left a comment

Copy link

Verdict: mergeable

Both findings from the previous round are fixed, confirmed against the diff at 532c780:

• HandleImageFileSelectedAsync now calls PlacesState.UpsertPlace immediately after the upload PUT succeeds, before the thumbnail-preview GET even starts; the preview fetch has its own nested try/catch that's silent on ApiException, matching OnInitializedAsync's existing convention. Only one call site patches state, unconditional on the preview outcome — decoupling is complete, and nothing that was a useful error before is now swallowed (the preview fetch was never the place a real error would need reporting).
• The new test discriminates both halves of the original bug: it asserts ImageVersion on the patched MapPlacesState place and asserts no role="alert" in the image section and no trace of the preview's error text in the markup. That combination is exactly what the unfixed code would have failed — old code skipped the state patch and set _imageError to the preview's message.
• The asymmetry note in PlaceDetailPanel's remarks is accurate: quiet thumbnail because it's incidental to a panel showing plenty else, loud viewer because opening it is the reader's whole reason for being there and an unexplained empty popup would be worse.

Everything else from the first review stands unchanged.

Verdict: mergeable Both findings from the previous round are fixed, confirmed against the diff at `532c780`: - `HandleImageFileSelectedAsync` now calls `PlacesState.UpsertPlace` immediately after the upload PUT succeeds, before the thumbnail-preview GET even starts; the preview fetch has its own nested try/catch that's silent on `ApiException`, matching `OnInitializedAsync`'s existing convention. Only one call site patches state, unconditional on the preview outcome — decoupling is complete, and nothing that was a useful error before is now swallowed (the preview fetch was never the place a real error would need reporting). - The new test discriminates both halves of the original bug: it asserts `ImageVersion` on the patched `MapPlacesState` place *and* asserts no `role="alert"` in the image section and no trace of the preview's error text in the markup. That combination is exactly what the unfixed code would have failed — old code skipped the state patch and set `_imageError` to the preview's message. - The asymmetry note in `PlaceDetailPanel`'s remarks is accurate: quiet thumbnail because it's incidental to a panel showing plenty else, loud viewer because opening it is the reader's whole reason for being there and an unexplained empty popup would be worse. Everything else from the first review stands unchanged.

rob merged commit 6571c77056 into main 2026-08-17 12:33:34 +00:00

rob deleted branch feature/task-249-place-images-ui 2026-08-17 12:33:34 +00:00

rob referenced this pull request from a commit 2026-08-17 12:33:35 +00:00

Add image upload, thumbnail and full-image viewer to the WebUI (#194)

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

rob/PlaceMark!194

No description provided.