Fix review findings on bulk delete, share editing, selection

- expiry editor commits on blur/Enter with explicit clear button — no more
  per-keystroke PATCHes transiently expiring live links
- shift-range walks the filtered view, anchors reset on clear, updaters
  kept pure (StrictMode-safe)
- bulk delete surfaces errors, 404s on zero deletions, and shares one
  delete+cleanup path with single and album delete (s3::photo_prefix is
  the only prefix source)
- expiry end-of-day convention extracted; share rows update from the
  PATCH response instead of reloading the list
- Gallery container always renders, restoring the simple observer effect

README.md

@@ -5,6 +5,23 @@ share them with clients via private (optionally password-protected) links,
 collect accept/reject votes, ratings and tags, and let clients download
 originals.
 
+## Screenshots
+
+The album view — drag-and-drop upload, justified gallery, and each client's
+feedback (votes, stars, tags) overlaid on the thumbnails:
+
+![Album view with upload zone and feedback overlays](docs/screenshots/album-admin.jpg)
+
+What clients see on a share link — vote on favorites, filter by verdict,
+download selects or the whole album as a ZIP:
+
+![Client gallery with verdict filters](docs/screenshots/share-client.jpg)
+
+The lightbox — accept/reject, star rating, tags, and keyboard-driven culling
+(`P`/`X`/`U`, `1`–`5`):
+
+![Lightbox with voting and rating controls](docs/screenshots/lightbox.jpg)
+
 ## Architecture
 
 ```

frontend/src/components/Gallery.jsx

@@ -1,4 +1,4 @@
-import { useCallback, useRef, useState } from 'react'
+import { useEffect, useRef, useState } from 'react'
 import { imgUrl } from '../api'
 
 // True justified layout: pack photos greedily into rows at their real aspect
@@ -32,22 +32,17 @@ function layoutRows(photos, containerWidth, targetHeight, gap) {
 }
 
 export default function Gallery({ photos, onOpen, overlay, selected, onToggleSelect }) {
+  const containerRef = useRef(null)
   const [width, setWidth] = useState(0)
-  const observerRef = useRef(null)
 
-  // Callback ref instead of mount effect: the container div unmounts whenever
-  // the photo list is empty (e.g. a filter with no matches), so the observer
-  // must re-attach to each new element — a once-per-mount effect would leave
-  // the re-rendered gallery unobserved at width 0, rendering nothing.
-  const containerRef = useCallback((el) => {
-    observerRef.current?.disconnect()
-    observerRef.current = null
-    if (!el) return
-    observerRef.current = new ResizeObserver((entries) => setWidth(entries[0].contentRect.width))
-    observerRef.current.observe(el)
+  useEffect(() => {
+    const observer = new ResizeObserver((entries) => setWidth(entries[0].contentRect.width))
+    observer.observe(containerRef.current)
+    return () => observer.disconnect()
   }, [])
 
-  if (photos.length === 0) return null
+  // The container renders even with zero photos — unmounting it would detach
+  // the observer and leave a later non-empty render stuck at width 0.
   const gap = 6
   const targetHeight = width < 700 ? 170 : 240
   const rows = width > 0 ? layoutRows(photos, width, targetHeight, gap) : []

frontend/src/pages/AlbumPage.jsx

@@ -18,6 +18,67 @@ function fmtEta(seconds) {
 
 const UPLOAD_CONCURRENCY = 3
 
+// Expiry convention, in one place for the create form and the row editor:
+// end of the chosen day in the photographer's local timezone — date-only
+// strings would parse as UTC midnight and expire a day early.
+const endOfDayIso = (day) => (day ? new Date(`${day}T23:59:59`).toISOString() : null)
+
+// ISO timestamp -> local yyyy-mm-dd for date inputs.
+const localDate = (iso) => {
+  const d = new Date(iso)
+  return `${d.getFullYear()}-${String(d.getMonth() + 1).padStart(2, '0')}-${String(d.getDate()).padStart(2, '0')}`
+}
+
+// Staged expiry editor: commits on blur/Enter, never per keystroke — typing
+// a year fires change events with bogus intermediate dates (year 0002) that
+// must not hit the live link. The ✕ clears explicitly; Safari's date input
+// has no native clear control.
+function ExpiryDate({ value, onCommit }) {
+  const current = value ? localDate(value) : ''
+  const [draft, setDraft] = useState(current)
+  useEffect(() => setDraft(current), [current])
+
+  const commit = () => {
+    if (draft === current) return
+    // A half-typed year (e.g. 0002) can survive until blur; don't persist it.
+    if (draft && draft.slice(0, 4) < '2000') {
+      setDraft(current)
+      return
+    }
+    onCommit(endOfDayIso(draft))
+  }
+
+  return (
+    <label className="row field-label" title="Expiry date — empty means the link never expires">
+      <span className="muted">expires</span>
+      <input
+        type="date"
+        value={draft}
+        onChange={(e) => setDraft(e.target.value)}
+        onBlur={commit}
+        onKeyDown={(e) => {
+          if (e.key === 'Enter') e.target.blur()
+        }}
+      />
+      {draft ? (
+        <button
+          type="button"
+          className="btn btn-ghost"
+          title="Remove expiry — link never expires"
+          onClick={() => {
+            setDraft('')
+            onCommit(null)
+          }}
+        >
+          ✕
+        </button>
+      ) : (
+        <span className="muted">never</span>
+      )}
+    </label>
+  )
+}
+
 function UploadZone({ albumId, onUploaded }) {
   const [queue, setQueue] = useState([])
   const [dragging, setDragging] = useState(false)
@@ -198,11 +259,7 @@ function SharesPanel({ albumId }) {
           label: form.label,
           password: form.password || null,
           allow_download: form.allow_download,
-          // End of the chosen day in the photographer's local timezone —
-          // date-only strings would parse as UTC midnight and expire a day early.
-          expires_at: form.expires_at
-            ? new Date(`${form.expires_at}T23:59:59`).toISOString()
-            : null,
+          expires_at: endOfDayIso(form.expires_at),
         },
       })
       setForm({ label: '', password: '', allow_download: true, expires_at: '' })
@@ -221,18 +278,14 @@ function SharesPanel({ albumId }) {
 
   const update = async (shareId, patch) => {
     try {
-      await api(`/api/shares/${shareId}`, { method: 'PATCH', body: patch })
+      const updated = await api(`/api/shares/${shareId}`, { method: 'PATCH', body: patch })
+      setShares((list) => list.map((s) => (s.id === shareId ? updated : s)))
       setError(null)
     } catch (e) {
       setError(e.message)
+      // Re-sync the controlled inputs with what the server actually has.
+      load()
     }
-    load()
-  }
-
-  // ISO timestamp -> local yyyy-mm-dd for the date input.
-  const localDate = (iso) => {
-    const d = new Date(iso)
-    return `${d.getFullYear()}-${String(d.getMonth() + 1).padStart(2, '0')}-${String(d.getDate()).padStart(2, '0')}`
   }
 
   return (
@@ -262,21 +315,7 @@ function SharesPanel({ albumId }) {
               />
               downloads
             </label>
-            <label className="row field-label" title="Expiry date — empty means the link never expires">
-              <span className="muted">expires</span>
-              <input
-                type="date"
-                value={s.expires_at ? localDate(s.expires_at) : ''}
-                onChange={(e) =>
-                  update(s.id, {
-                    // End of the chosen day in local time, like the create form.
-                    expires_at: e.target.value
-                      ? new Date(`${e.target.value}T23:59:59`).toISOString()
-                      : null,
-                  })
-                }
-              />
-            </label>
+            <ExpiryDate value={s.expires_at} onCommit={(iso) => update(s.id, { expires_at: iso })} />
             {s.locked && (
               <button
                 className="btn"
@@ -414,8 +453,12 @@ export default function AlbumPage() {
   const removeSelected = async () => {
     if (!confirm(`Delete ${selected.size} selected photo${selected.size === 1 ? '' : 's'}? This cannot be undone.`))
       return
-    await api('/api/photos/delete', { method: 'POST', body: { ids: [...selected] } })
-    clear()
+    try {
+      await api('/api/photos/delete', { method: 'POST', body: { ids: [...selected] } })
+      clear()
+    } catch (e) {
+      alert(`Delete failed: ${e.message}`)
+    }
     load()
   }
 

frontend/src/pages/SharePage.jsx

@@ -193,7 +193,11 @@ export default function SharePage() {
           photos={visible}
           onOpen={lightbox.openAt}
           selected={view.allow_download ? selected : undefined}
-          onToggleSelect={view.allow_download ? toggle : undefined}
+          // Shift-ranges must walk the filtered view, not the full album —
+          // otherwise hidden photos get swept into the selection.
+          onToggleSelect={
+            view.allow_download ? (id, shift) => toggle(id, shift, visible) : undefined
+          }
           overlay={(p) =>
             p.my_verdict || p.my_rating || p.my_tags.length > 0 ? (
               <div className="g-overlay">

frontend/src/useSelection.js

@@ -18,30 +18,42 @@ export default function useSelection(photos) {
   }, [photos])
 
   // Shift-toggle selects the whole range from the previously toggled photo
-  // (both directions), so contiguous runs don't need per-photo clicks.
-  const toggle = (photoId, shift = false) =>
+  // (both directions), so contiguous runs don't need per-photo clicks. The
+  // range walks `list` — the photo order the user is actually looking at —
+  // which callers with a filtered view must pass explicitly, so hidden
+  // photos are never swept into the selection. Anchor updates and range
+  // computation stay out of the setSelected updater: React may re-invoke
+  // updaters (StrictMode does), so they must be pure.
+  const toggle = (photoId, shift = false, list = photos) => {
+    const anchor = lastToggled.current
+    lastToggled.current = photoId
+    if (shift && anchor) {
+      const a = list.findIndex((p) => p.id === anchor)
+      const b = list.findIndex((p) => p.id === photoId)
+      if (a >= 0 && b >= 0) {
+        const range = list.slice(Math.min(a, b), Math.max(a, b) + 1).map((p) => p.id)
+        setSelected((prev) => new Set([...prev, ...range]))
+        return
+      }
+    }
     setSelected((prev) => {
       const next = new Set(prev)
-      if (shift && lastToggled.current) {
-        const a = photos.findIndex((p) => p.id === lastToggled.current)
-        const b = photos.findIndex((p) => p.id === photoId)
-        if (a >= 0 && b >= 0) {
-          for (const p of photos.slice(Math.min(a, b), Math.max(a, b) + 1)) next.add(p.id)
-          lastToggled.current = photoId
-          return next
-        }
-      }
       if (next.has(photoId)) next.delete(photoId)
       else next.add(photoId)
-      lastToggled.current = photoId
       return next
     })
+  }
 
   // Adds `list` (the caller's currently visible photos) to the selection —
   // additive, so selecting all of one filtered view keeps picks from another.
   const selectAll = (list) =>
     setSelected((prev) => new Set([...prev, ...list.map((p) => p.id)]))
-  const clear = () => setSelected(new Set())
+  const clear = () => {
+    // Reset the anchor too — a shift-click after Clear must start fresh, not
+    // extend a range from a photo selected before the wipe.
+    lastToggled.current = null
+    setSelected(new Set())
+  }
   const selectedBytes = photos.reduce((sum, p) => sum + (selected.has(p.id) ? p.size_bytes : 0), 0)
   const totalBytes = photos.reduce((sum, p) => sum + p.size_bytes, 0)
 

src/routes/albums.rs

@@ -7,7 +7,7 @@ use serde::{Deserialize, Serialize};
 use uuid::Uuid;
 
 use crate::error::{ApiError, ApiResult};
-use crate::models::{Album, JobKind, Photo, PhotoStatus};
+use crate::models::{Album, Photo, PhotoStatus};
 use crate::state::AppState;
 
 #[derive(Serialize, sqlx::FromRow)]
@@ -232,17 +232,13 @@ pub async fn delete(
     if locked.is_none() {
         return Err(ApiError::not_found());
     }
-    // Delete photos and enqueue their S3 cleanup atomically, in one statement.
-    sqlx::query(
-        "with deleted as (delete from photos where album_id = $1 returning id)
-         insert into jobs (kind, payload)
-         select $2, jsonb_build_object('prefix', 'photos/' || id || '/')
-         from deleted",
-    )
-    .bind(album_id)
-    .bind(JobKind::DeleteS3Prefix.as_str())
-    .execute(&mut *tx)
-    .await?;
+    // Delete photos through the shared path so the S3 cleanup convention
+    // lives in one place; the album lock above keeps this set complete.
+    let photo_ids: Vec<Uuid> = sqlx::query_scalar("select id from photos where album_id = $1")
+        .bind(album_id)
+        .fetch_all(&mut *tx)
+        .await?;
+    super::photos::delete_with_cleanup(&mut tx, &photo_ids).await?;
     sqlx::query("delete from albums where id = $1")
         .bind(album_id)
         .execute(&mut *tx)

src/routes/photos.rs

@@ -189,24 +189,42 @@ pub async fn by_hash(
     photo.map(Json).ok_or_else(ApiError::not_found)
 }
 
+/// The one deletion path: delete the given photos and enqueue their S3
+/// cleanup jobs in the same transaction. Prefixes come from s3::photo_prefix
+/// so the key layout has a single source of truth. Returns how many photos
+/// were actually deleted.
+pub(super) async fn delete_with_cleanup(
+    tx: &mut sqlx::PgConnection,
+    ids: &[Uuid],
+) -> Result<u64, sqlx::Error> {
+    let deleted: Vec<Uuid> = sqlx::query_scalar("delete from photos where id = any($1) returning id")
+        .bind(ids)
+        .fetch_all(&mut *tx)
+        .await?;
+    if deleted.is_empty() {
+        return Ok(0);
+    }
+    let prefixes: Vec<String> = deleted.iter().map(|id| s3::photo_prefix(*id)).collect();
+    sqlx::query(
+        "insert into jobs (kind, payload)
+         select $1, jsonb_build_object('prefix', p)
+         from unnest($2::text[]) as p",
+    )
+    .bind(JobKind::DeleteS3Prefix.as_str())
+    .bind(&prefixes)
+    .execute(&mut *tx)
+    .await?;
+    Ok(deleted.len() as u64)
+}
+
 pub async fn delete(
     State(state): State<AppState>,
     Path(photo_id): Path<Uuid>,
 ) -> ApiResult<Json<serde_json::Value>> {
     let mut tx = state.db.begin().await?;
-    let deleted = sqlx::query("delete from photos where id = $1")
-        .bind(photo_id)
-        .execute(&mut *tx)
-        .await?;
-    if deleted.rows_affected() == 0 {
+    if delete_with_cleanup(&mut tx, &[photo_id]).await? == 0 {
         return Err(ApiError::not_found());
     }
-    jobs::enqueue(
-        &mut *tx,
-        JobKind::DeleteS3Prefix,
-        serde_json::json!({ "prefix": s3::photo_prefix(photo_id) }),
-    )
-    .await?;
     tx.commit().await?;
     Ok(Json(serde_json::json!({ "ok": true })))
 }
@@ -216,8 +234,6 @@ pub struct DeleteManyBody {
     ids: Vec<Uuid>,
 }
 
-/// Bulk delete: photos vanish and their S3 cleanup jobs are enqueued in one
-/// statement, same as album deletion.
 pub async fn delete_many(
     State(state): State<AppState>,
     Json(body): Json<DeleteManyBody>,
@@ -226,18 +242,14 @@ pub async fn delete_many(
         return Err(ApiError::bad_request("ids must not be empty"));
     }
     let mut tx = state.db.begin().await?;
-    let jobs = sqlx::query(
-        "with deleted as (delete from photos where id = any($1) returning id)
-         insert into jobs (kind, payload)
-         select $2, jsonb_build_object('prefix', 'photos/' || id || '/')
-         from deleted",
-    )
-    .bind(&body.ids)
-    .bind(JobKind::DeleteS3Prefix.as_str())
-    .execute(&mut *tx)
-    .await?;
+    let deleted = delete_with_cleanup(&mut tx, &body.ids).await?;
+    // Consistent with the single-photo route: deleting nothing is an error,
+    // not a silent success (e.g. a stale tab re-deleting already-gone photos).
+    if deleted == 0 {
+        return Err(ApiError::not_found());
+    }
     tx.commit().await?;
-    Ok(Json(serde_json::json!({ "ok": true, "deleted": jobs.rows_affected() })))
+    Ok(Json(serde_json::json!({ "ok": true, "deleted": deleted })))
 }
 
 pub async fn reprocess(