From 039871a92cb0b3d34c217ee50a642bbf343610be Mon Sep 17 00:00:00 2001 From: domverse Date: Wed, 15 Jul 2026 21:44:15 +0200 Subject: [PATCH] US01-06: Review Inventory and Duplicates in the Browser --- frontend/css/app.css | 130 +++++++++ frontend/index.html | 20 ++ frontend/js/api.js | 38 +++ frontend/js/app.js | 355 ++++++++++++++++++++++++ frontend/js/router.js | 20 ++ photo_pipeline/api/app.py | 11 +- photo_pipeline/api/routes/duplicates.py | 61 ++++ photo_pipeline/api/routes/inventory.py | 21 ++ photo_pipeline/schemas/__init__.py | 5 + photo_pipeline/schemas/duplicates.py | 14 + photo_pipeline/services/duplicates.py | 58 +++- photo_pipeline/services/inventory.py | 41 ++- pyproject.toml | 11 +- tests/e2e/test_review_ui.py | 184 ++++++++++++ tests/integration/test_review_api.py | 145 ++++++++++ 15 files changed, 1110 insertions(+), 4 deletions(-) create mode 100644 frontend/css/app.css create mode 100644 frontend/index.html create mode 100644 frontend/js/api.js create mode 100644 frontend/js/app.js create mode 100644 frontend/js/router.js create mode 100644 photo_pipeline/api/routes/duplicates.py create mode 100644 photo_pipeline/api/routes/inventory.py create mode 100644 photo_pipeline/schemas/__init__.py create mode 100644 photo_pipeline/schemas/duplicates.py create mode 100644 tests/e2e/test_review_ui.py create mode 100644 tests/integration/test_review_api.py diff --git a/frontend/css/app.css b/frontend/css/app.css new file mode 100644 index 0000000..600b4c0 --- /dev/null +++ b/frontend/css/app.css @@ -0,0 +1,130 @@ +:root { + --bg: #000; + --surface: #0e0e11; + --surface-2: #17171c; + --border: #26262e; + --text: #f4f4f6; + --muted: #9a9aa6; + --accent: #5b8cff; + --danger: #ff5b6e; + --ok: #3ddc97; + --warn: #ffcf5b; + --radius: 10px; +} + +* { box-sizing: border-box; } + +body { + margin: 0; + background: var(--bg); + color: var(--text); + font: 15px/1.5 system-ui, -apple-system, Segoe UI, Roboto, sans-serif; +} + +.app-header { + display: flex; + align-items: center; + gap: 24px; + padding: 12px 20px; + border-bottom: 1px solid var(--border); + background: var(--surface); + position: sticky; + top: 0; + z-index: 10; +} + +.brand { font-weight: 700; letter-spacing: 0.2px; } + +nav a { + color: var(--muted); + text-decoration: none; + margin-right: 16px; + padding: 6px 0; + border-bottom: 2px solid transparent; +} +nav a[aria-current="page"] { color: var(--text); border-bottom-color: var(--accent); } +nav a:hover { color: var(--text); } + +main { padding: 20px; max-width: 1200px; margin: 0 auto; } + +.toolbar { display: flex; gap: 12px; align-items: center; margin-bottom: 16px; flex-wrap: wrap; } +input, select, button { font: inherit; } +input[type="search"], select { + background: var(--surface-2); + color: var(--text); + border: 1px solid var(--border); + border-radius: 8px; + padding: 8px 10px; +} + +button { + background: var(--surface-2); + color: var(--text); + border: 1px solid var(--border); + border-radius: 8px; + padding: 8px 14px; + cursor: pointer; +} +button:hover:not(:disabled) { border-color: var(--accent); } +button:disabled { opacity: 0.4; cursor: default; } +button.primary { background: var(--accent); border-color: var(--accent); color: #06122e; font-weight: 600; } +button.danger { border-color: var(--danger); color: var(--danger); } + +table { width: 100%; border-collapse: collapse; } +th, td { text-align: left; padding: 8px 10px; border-bottom: 1px solid var(--border); } +th { color: var(--muted); font-weight: 600; font-size: 13px; } +tbody tr { outline: none; } +tbody tr[aria-selected="true"] { background: rgba(91, 140, 255, 0.16); } +tbody tr:focus { box-shadow: inset 3px 0 0 var(--accent); } + +.badge { + display: inline-block; + padding: 2px 8px; + border-radius: 999px; + font-size: 12px; + border: 1px solid var(--border); + color: var(--muted); +} +.badge.exact, .badge.pixel { color: var(--ok); border-color: var(--ok); } +.badge.perceptual { color: var(--warn); border-color: var(--warn); } +.badge.decided { color: var(--ok); border-color: var(--ok); } +.badge.reopened, .badge.dismissed { color: var(--warn); border-color: var(--warn); } + +.muted { color: var(--muted); } +.pager { display: flex; gap: 12px; align-items: center; margin-top: 16px; } + +.cluster-grid { display: grid; grid-template-columns: repeat(auto-fit, minmax(260px, 1fr)); gap: 16px; } +.member { + background: var(--surface); + border: 1px solid var(--border); + border-radius: var(--radius); + padding: 12px; +} +.member.canonical { border-color: var(--accent); } +.member img { width: 100%; height: auto; background: var(--surface-2); border-radius: 8px; display: block; transform-origin: top left; } +.member dl { margin: 10px 0 0; font-size: 13px; } +.member dt { color: var(--muted); } +.member dd { margin: 0 0 6px; word-break: break-all; } + +.decision-bar { display: flex; gap: 12px; margin: 20px 0; flex-wrap: wrap; align-items: center; } + +.alert { + padding: 12px 16px; + border-radius: var(--radius); + border: 1px solid var(--danger); + color: var(--danger); + background: rgba(255, 91, 110, 0.1); + margin-bottom: 16px; +} + +.confirm { + background: var(--surface); + border: 1px solid var(--warn); + border-radius: var(--radius); + padding: 16px; + margin: 16px 0; +} + +.card { background: var(--surface); border: 1px solid var(--border); border-radius: var(--radius); padding: 14px; } +a.link { color: var(--accent); text-decoration: none; } +a.link:hover { text-decoration: underline; } diff --git a/frontend/index.html b/frontend/index.html new file mode 100644 index 0000000..bfeb057 --- /dev/null +++ b/frontend/index.html @@ -0,0 +1,20 @@ + + + + + + Photo Pipeline — Review + + + +
+ Photo Pipeline + +
+
+ + + diff --git a/frontend/js/api.js b/frontend/js/api.js new file mode 100644 index 0000000..fa401f4 --- /dev/null +++ b/frontend/js/api.js @@ -0,0 +1,38 @@ +// Single API client: one place for fetch, JSON, and the error envelope. +const BASE = "/api/v1"; + +async function request(path, options = {}) { + const response = await fetch(BASE + path, { + headers: { "Content-Type": "application/json" }, + ...options, + }); + let body = null; + try { + body = await response.json(); + } catch (_) { + body = null; + } + if (!response.ok) { + const envelope = body && body.error ? body.error : {}; + const error = new Error(envelope.message || response.statusText); + error.status = response.status; + error.code = envelope.code || "error"; + throw error; + } + return body; +} + +export const api = { + listAssets: (params = {}) => + request("/inventory/assets?" + new URLSearchParams(params).toString()), + listClusters: (params = {}) => + request("/duplicates/clusters?" + new URLSearchParams(params).toString()), + getCluster: (id) => request(`/duplicates/clusters/${encodeURIComponent(id)}`), + decide: (id, payload) => + request(`/duplicates/clusters/${encodeURIComponent(id)}/decision`, { + method: "POST", + body: JSON.stringify(payload), + }), + thumbnailUrl: (assetId, size = 512) => + `${BASE}/assets/${encodeURIComponent(assetId)}/thumbnail?size=${size}`, +}; diff --git a/frontend/js/app.js b/frontend/js/app.js new file mode 100644 index 0000000..e70ab62 --- /dev/null +++ b/frontend/js/app.js @@ -0,0 +1,355 @@ +import { api } from "./api.js"; +import { navigate, onRouteChange, parseHash } from "./router.js"; + +const root = document.getElementById("app"); + +// Tiny DOM helper — builds nodes without innerHTML injection. +function el(tag, attrs = {}, ...children) { + const node = document.createElement(tag); + for (const [key, value] of Object.entries(attrs)) { + if (value == null || value === false) continue; + if (key === "class") node.className = value; + else if (key === "dataset") Object.assign(node.dataset, value); + else if (key.startsWith("on") && typeof value === "function") + node.addEventListener(key.slice(2).toLowerCase(), value); + else node.setAttribute(key, value); + } + for (const child of children.flat()) { + if (child == null || child === false) continue; + node.appendChild(typeof child === "string" ? document.createTextNode(child) : child); + } + return node; +} + +function show(...nodes) { + root.replaceChildren(...nodes); +} + +function setActiveNav(view) { + document.querySelectorAll("nav a[data-nav]").forEach((a) => { + if (a.dataset.nav === view) a.setAttribute("aria-current", "page"); + else a.removeAttribute("aria-current"); + }); +} + +function errorBanner(message) { + return el("div", { class: "alert", role: "alert" }, message); +} + +// ── Inventory view ────────────────────────────────────────────────────────── +async function renderInventory(params) { + setActiveNav("inventory"); + const limit = Number(params.limit || 50); + const offset = Number(params.offset || 0); + let data; + try { + data = await api.listAssets({ + limit, + offset, + availability: params.availability || "", + q: params.q || "", + }); + } catch (error) { + show(errorBanner(`Failed to load inventory: ${error.message}`)); + return; + } + + const search = el("input", { + type: "search", + placeholder: "Filter by path…", + value: params.q || "", + "aria-label": "Filter by path", + onchange: (e) => navigate("/inventory", { ...params, q: e.target.value, offset: 0 }), + }); + const availability = el( + "select", + { + "aria-label": "Availability", + onchange: (e) => + navigate("/inventory", { ...params, availability: e.target.value, offset: 0 }), + }, + el("option", { value: "" }, "All availability"), + ...["active", "archived_offline", "archived_online"].map((v) => + el("option", { value: v, selected: params.availability === v ? "selected" : false }, v) + ) + ); + + const rows = data.items.map((asset, index) => + el( + "tr", + { + "data-testid": "asset-row", + "data-asset-id": asset.id, + tabindex: "-1", + role: "row", + onclick: () => selectRow(index), + }, + el("td", {}, asset.current_path || "(no path)"), + el("td", {}, asset.availability_state), + el("td", {}, asset.missing ? "missing" : "present"), + el("td", { class: "muted" }, asset.byte_size != null ? String(asset.byte_size) : "—") + ) + ); + + const tbody = el("tbody", {}, ...rows); + let selected = -1; + function selectRow(index) { + if (index < 0 || index >= rows.length) return; + if (selected >= 0) rows[selected].setAttribute("aria-selected", "false"); + selected = index; + rows[selected].setAttribute("aria-selected", "true"); + rows[selected].focus(); + } + + tbody.addEventListener("keydown", (event) => { + if (event.key === "ArrowDown") { + event.preventDefault(); + selectRow(Math.min(selected + 1, rows.length - 1) || 0); + } else if (event.key === "ArrowUp") { + event.preventDefault(); + selectRow(Math.max(selected - 1, 0)); + } + }); + + const table = el( + "table", + { "aria-label": "Inventory" }, + el( + "thead", + {}, + el("tr", {}, el("th", {}, "Path"), el("th", {}, "Availability"), el("th", {}, "State"), el("th", {}, "Bytes")) + ), + tbody + ); + + const start = data.total === 0 ? 0 : offset + 1; + const end = Math.min(offset + limit, data.total); + const pager = el( + "div", + { class: "pager" }, + el( + "button", + { + "data-testid": "prev-page", + disabled: offset === 0 ? "disabled" : false, + onclick: () => navigate("/inventory", { ...params, offset: Math.max(offset - limit, 0) }), + }, + "Previous" + ), + el("span", { class: "muted", "data-testid": "page-info" }, `${start}–${end} of ${data.total}`), + el( + "button", + { + "data-testid": "next-page", + disabled: end >= data.total ? "disabled" : false, + onclick: () => navigate("/inventory", { ...params, offset: offset + limit }), + }, + "Next" + ) + ); + + show( + el("h1", {}, "Inventory"), + el("div", { class: "toolbar" }, search, availability), + table, + pager + ); + if (rows.length) selectRow(0); +} + +// ── Duplicate cluster list ─────────────────────────────────────────────────── +async function renderClusters(params) { + setActiveNav("duplicates"); + let data; + try { + data = await api.listClusters({ state: params.state || "", limit: 100 }); + } catch (error) { + show(errorBanner(`Failed to load clusters: ${error.message}`)); + return; + } + + const items = data.items.map((cluster) => + el( + "div", + { class: "card", "data-testid": "cluster-card" }, + el( + "div", + {}, + el("a", { class: "link", href: `#/duplicates/${cluster.id}` }, `Cluster ${cluster.id.slice(0, 8)}`), + " ", + el("span", { class: `badge ${cluster.method}` }, cluster.method), + " ", + el("span", { class: `badge ${cluster.state}` }, cluster.state) + ), + el("div", { class: "muted" }, `${cluster.members.length} members · confidence ${cluster.confidence}`) + ) + ); + + show( + el("h1", {}, "Duplicate clusters"), + data.items.length ? el("div", { class: "cluster-grid" }, ...items) : el("p", { class: "muted" }, "No clusters detected.") + ); +} + +// ── Duplicate comparison ───────────────────────────────────────────────────── +async function renderClusterDetail(id, extra = {}) { + setActiveNav("duplicates"); + let cluster; + try { + cluster = await api.getCluster(id); + } catch (error) { + show(errorBanner(`Failed to load cluster: ${error.message}`)); + return; + } + + const zoom = el("input", { + type: "range", + min: "1", + max: "3", + step: "0.1", + value: "1", + "aria-label": "Synchronized zoom", + "data-testid": "zoom", + }); + + const images = []; + const members = cluster.members.map((member) => { + const isCanonical = cluster.canonical_asset_id === member.asset_id; + const img = el("img", { + src: api.thumbnailUrl(member.asset_id, 512), + alt: member.current_path || member.asset_id, + loading: "lazy", + }); + images.push(img); + const actions = + cluster.state === "decided" || cluster.state === "dismissed" + ? [] + : [ + el( + "button", + { + "data-testid": "make-canonical", + onclick: () => decide(cluster, "canonical", member.asset_id), + }, + "Keep this as canonical" + ), + ]; + return el( + "div", + { class: `member ${isCanonical ? "canonical" : ""}`, "data-testid": "member" }, + img, + el( + "dl", + {}, + el("dt", {}, "Path"), + el("dd", {}, member.current_path || "—"), + el("dt", {}, "Role"), + el("dd", {}, member.role), + el("dt", {}, "pHash distance"), + el("dd", {}, member.distance == null ? "—" : String(member.distance)), + el("dt", {}, "Bytes"), + el("dd", {}, member.byte_size == null ? "—" : String(member.byte_size)) + ), + ...actions + ); + }); + + zoom.addEventListener("input", () => { + const scale = zoom.value; + images.forEach((img) => (img.style.transform = `scale(${scale})`)); + }); + + const decisionBar = el( + "div", + { class: "decision-bar" }, + el("span", { "data-testid": "cluster-state", class: `badge ${cluster.state}` }, `state: ${cluster.state}`), + el("span", { class: `badge ${cluster.confidence}` }, `confidence: ${cluster.confidence}`), + cluster.requires_confirmation ? el("span", { class: "muted" }, "fuzzy — confirmation required") : null, + el( + "button", + { class: "danger", "data-testid": "not-duplicate", onclick: () => decide(cluster, "not_duplicate") }, + "Not a duplicate" + ), + el("button", { "data-testid": "defer", onclick: () => decide(cluster, "deferred") }, "Defer") + ); + + const nodes = [ + el("p", {}, el("a", { class: "link", href: "#/duplicates" }, "← All clusters")), + el("h1", {}, `Compare cluster ${cluster.id.slice(0, 8)}`), + ]; + if (extra.conflict) { + nodes.push( + errorBanner("This cluster changed since you loaded it. Nothing was modified; showing the latest state.") + ); + } + nodes.push(el("div", { class: "toolbar" }, el("label", {}, "Zoom "), zoom)); + nodes.push(decisionBar); + if (extra.pending) nodes.push(confirmPanel(cluster, extra.pending)); + nodes.push(el("div", { class: "cluster-grid" }, ...members)); + show(...nodes); +} + +function confirmPanel(cluster, pending) { + return el( + "div", + { class: "confirm", role: "dialog", "aria-label": "Confirm decision", "data-testid": "confirm" }, + el("p", {}, `Confirm "${pending.decision.replace("_", " ")}" for this fuzzy cluster?`), + el( + "div", + { class: "decision-bar" }, + el( + "button", + { + class: "primary", + "data-testid": "confirm-yes", + onclick: () => applyDecision(cluster, pending.decision, pending.canonicalId), + }, + "Confirm" + ), + el( + "button", + { "data-testid": "confirm-no", onclick: () => renderClusterDetail(cluster.id) }, + "Cancel" + ) + ) + ); +} + +// A fuzzy cluster needs explicit confirmation; exact/pixel apply immediately. +function decide(cluster, decision, canonicalId) { + if (cluster.requires_confirmation) { + renderClusterDetail(cluster.id, { pending: { decision, canonicalId } }); + } else { + applyDecision(cluster, decision, canonicalId); + } +} + +async function applyDecision(cluster, decision, canonicalId) { + try { + await api.decide(cluster.id, { + decision, + expected_version: cluster.version, + canonical_asset_id: canonicalId || null, + }); + renderClusterDetail(cluster.id); + } catch (error) { + if (error.status === 409) { + renderClusterDetail(cluster.id, { conflict: true }); + } else { + show(errorBanner(`Decision failed: ${error.message}`)); + } + } +} + +// ── Router ─────────────────────────────────────────────────────────────────── +function render() { + const { path, params } = parseHash(); + const clusterMatch = path.match(/^\/duplicates\/(.+)$/); + if (path === "/inventory" || path === "/") renderInventory(params); + else if (path === "/duplicates") renderClusters(params); + else if (clusterMatch) renderClusterDetail(clusterMatch[1]); + else show(errorBanner("Unknown view")); +} + +onRouteChange(render); +render(); diff --git a/frontend/js/router.js b/frontend/js/router.js new file mode 100644 index 0000000..939128c --- /dev/null +++ b/frontend/js/router.js @@ -0,0 +1,20 @@ +// Hash router. URLs are stable and shareable: the view, its filters, and paging +// all live in the hash, so a reload restores exactly what was on screen. +export function parseHash() { + const raw = location.hash.slice(1) || "/inventory"; + const [path, queryString = ""] = raw.split("?"); + return { path, params: Object.fromEntries(new URLSearchParams(queryString)) }; +} + +export function navigate(path, params = {}) { + const clean = Object.fromEntries( + Object.entries(params).filter(([, v]) => v !== undefined && v !== null && v !== "") + ); + const query = Object.keys(clean).length ? "?" + new URLSearchParams(clean) : ""; + location.hash = path + query; +} + +export function onRouteChange(handler) { + window.addEventListener("hashchange", handler); + window.addEventListener("DOMContentLoaded", handler); +} diff --git a/photo_pipeline/api/app.py b/photo_pipeline/api/app.py index 2eb2d7b..d90ffa1 100644 --- a/photo_pipeline/api/app.py +++ b/photo_pipeline/api/app.py @@ -9,14 +9,18 @@ and exposes the versioned ``/api/v1`` surface; US01-02 ships only health. from __future__ import annotations from contextlib import asynccontextmanager +from pathlib import Path from fastapi import FastAPI +from fastapi.staticfiles import StaticFiles -from photo_pipeline.api.routes import health, thumbnails +from photo_pipeline.api.routes import duplicates, health, inventory, thumbnails from photo_pipeline.config import Config from photo_pipeline.db import create_db_engine, create_session_factory, run_migrations from photo_pipeline.logging import configure_logging +FRONTEND_DIR = Path(__file__).resolve().parents[2] / "frontend" + def create_app(config: Config | None = None) -> FastAPI: config = config or Config.from_env() @@ -38,5 +42,10 @@ def create_app(config: Config | None = None) -> FastAPI: app = FastAPI(title="Photo Pipeline", version="0.1.0", lifespan=lifespan) app.include_router(health.router, prefix="/api/v1") + app.include_router(inventory.router, prefix="/api/v1") + app.include_router(duplicates.router, prefix="/api/v1") app.include_router(thumbnails.router, prefix="/api/v1") + # Static single-page app (hash-routed). Mounted last so /api/v1 wins. + if FRONTEND_DIR.is_dir(): + app.mount("/app", StaticFiles(directory=FRONTEND_DIR, html=True), name="app") return app diff --git a/photo_pipeline/api/routes/duplicates.py b/photo_pipeline/api/routes/duplicates.py new file mode 100644 index 0000000..8f3bdfb --- /dev/null +++ b/photo_pipeline/api/routes/duplicates.py @@ -0,0 +1,61 @@ +"""Duplicate cluster review API: list, detail, and decision. + +Decisions carry an ``expected_version``; a stale version returns 409 without +mutating anything, so a browser acting on out-of-date state cannot overwrite a +newer decision. +""" + +from __future__ import annotations + +from fastapi import APIRouter, Query, Request +from fastapi.responses import JSONResponse + +from photo_pipeline.schemas import DecisionRequest +from photo_pipeline.services.duplicates import ConflictError, DuplicateError, DuplicateService + +router = APIRouter(tags=["duplicates"]) + + +def _service(request: Request) -> DuplicateService: + return DuplicateService(request.app.state.session_factory) + + +def _error(status: int, code: str, message: str) -> JSONResponse: + return JSONResponse(status_code=status, content={"error": {"code": code, "message": message}}) + + +@router.get("/duplicates/clusters") +def list_clusters( + request: Request, + state: str | None = None, + limit: int = Query(50, ge=1, le=200), + offset: int = Query(0, ge=0), +) -> dict: + return _service(request).list_clusters(state=state, limit=limit, offset=offset) + + +@router.get("/duplicates/clusters/{cluster_id}") +def get_cluster(cluster_id: str, request: Request): + detail = _service(request).get_cluster(cluster_id) + if detail is None: + return _error(404, "not_found", f"unknown cluster {cluster_id}") + return detail + + +@router.post("/duplicates/clusters/{cluster_id}/decision") +def decide(cluster_id: str, body: DecisionRequest, request: Request): + service = _service(request) + if service.get_cluster(cluster_id) is None: + return _error(404, "not_found", f"unknown cluster {cluster_id}") + try: + return service.decide( + cluster_id, + body.decision, + canonical_asset_id=body.canonical_asset_id, + expected_version=body.expected_version, + reason=body.reason, + ) + except ConflictError as error: + return _error(409, "version_conflict", str(error)) + except DuplicateError as error: + return _error(422, "invalid_decision", str(error)) diff --git a/photo_pipeline/api/routes/inventory.py b/photo_pipeline/api/routes/inventory.py new file mode 100644 index 0000000..eef71c1 --- /dev/null +++ b/photo_pipeline/api/routes/inventory.py @@ -0,0 +1,21 @@ +"""Inventory listing API for the review UI.""" + +from __future__ import annotations + +from fastapi import APIRouter, Query, Request + +from photo_pipeline.services.inventory import InventoryService + +router = APIRouter(tags=["inventory"]) + + +@router.get("/inventory/assets") +def list_assets( + request: Request, + limit: int = Query(50, ge=1, le=200), + offset: int = Query(0, ge=0), + availability: str | None = None, + q: str | None = None, +) -> dict: + service = InventoryService(request.app.state.session_factory) + return service.list_assets(limit=limit, offset=offset, availability=availability, query=q) diff --git a/photo_pipeline/schemas/__init__.py b/photo_pipeline/schemas/__init__.py new file mode 100644 index 0000000..f116276 --- /dev/null +++ b/photo_pipeline/schemas/__init__.py @@ -0,0 +1,5 @@ +"""Pydantic API request/response contracts.""" + +from photo_pipeline.schemas.duplicates import DecisionRequest + +__all__ = ["DecisionRequest"] diff --git a/photo_pipeline/schemas/duplicates.py b/photo_pipeline/schemas/duplicates.py new file mode 100644 index 0000000..46a176b --- /dev/null +++ b/photo_pipeline/schemas/duplicates.py @@ -0,0 +1,14 @@ +"""Request contracts for duplicate decisions.""" + +from __future__ import annotations + +from typing import Literal + +from pydantic import BaseModel + + +class DecisionRequest(BaseModel): + decision: Literal["canonical", "not_duplicate", "deferred"] + expected_version: int + canonical_asset_id: str | None = None + reason: str | None = None diff --git a/photo_pipeline/services/duplicates.py b/photo_pipeline/services/duplicates.py index bdb59e9..80ada60 100644 --- a/photo_pipeline/services/duplicates.py +++ b/photo_pipeline/services/duplicates.py @@ -30,7 +30,7 @@ from dataclasses import dataclass, field from datetime import datetime, timezone from enum import Enum -from sqlalchemy import select +from sqlalchemy import func, select from sqlalchemy.orm import sessionmaker from photo_pipeline.models import ( @@ -466,6 +466,62 @@ class DuplicateService: asset = session.get(Asset, cur) cur = asset.canonical_asset_id if asset else None + # ── reads for the review UI ──────────────────────────────────────────────── + def list_clusters( + self, *, state: str | None = None, limit: int = 50, offset: int = 0 + ) -> dict: + limit = max(1, min(limit, 200)) + offset = max(0, offset) + with self._session_factory() as session: + stmt = select(DuplicateCluster) + if state: + stmt = stmt.where(DuplicateCluster.state == state) + total = session.scalar(select(func.count()).select_from(stmt.subquery())) + rows = session.execute( + stmt.order_by(DuplicateCluster.created_at).limit(limit).offset(offset) + ).scalars() + items = [self._snapshot(session, c.id) for c in rows] + return {"items": items, "total": int(total or 0), "limit": limit, "offset": offset} + + def get_cluster(self, cluster_id: str) -> dict | None: + """Cluster detail enriched with per-member asset evidence for comparison.""" + with self._session_factory() as session: + cluster = session.get(DuplicateCluster, cluster_id) + if cluster is None: + return None + members = [] + for member in session.execute( + select(DuplicateMember).where(DuplicateMember.cluster_id == cluster_id) + ).scalars(): + asset = session.get(Asset, member.asset_id) + try: + evidence = json.loads(member.evidence) if member.evidence else {} + except json.JSONDecodeError: + evidence = {} + members.append( + { + "asset_id": member.asset_id, + "role": member.role, + "distance": member.distance, + "evidence": evidence, + "current_path": asset.current_path if asset else None, + "byte_size": asset.byte_size if asset else None, + "phash": asset.phash if asset else None, + } + ) + members.sort(key=lambda m: m["asset_id"]) + return { + "id": cluster.id, + "method": cluster.method, + "confidence": cluster.confidence, + "state": cluster.state, + "decision": cluster.decision, + "canonical_asset_id": cluster.canonical_asset_id, + "version": cluster.version, + "requires_confirmation": cluster.method == Method.PERCEPTUAL.value, + "members": members, + } + # ── decisions ──────────────────────────────────────────────────────────── def decide( self, diff --git a/photo_pipeline/services/inventory.py b/photo_pipeline/services/inventory.py index 9d1b5fb..07c3a48 100644 --- a/photo_pipeline/services/inventory.py +++ b/photo_pipeline/services/inventory.py @@ -30,7 +30,7 @@ from enum import Enum from pathlib import Path from typing import Iterable -from sqlalchemy import select +from sqlalchemy import func, select from sqlalchemy.orm import Session, sessionmaker from photo_pipeline import path_policy @@ -55,6 +55,21 @@ class ScanResult: asset_ids: dict[str, str] = field(default_factory=dict) # path -> asset id +def _asset_dict(asset: Asset) -> dict: + return { + "id": asset.id, + "current_path": asset.current_path, + "availability_state": asset.availability_state, + "byte_size": asset.byte_size, + "current_sha256": asset.current_sha256, + "pixel_sha256": asset.pixel_sha256, + "phash": asset.phash, + "canonical_asset_id": asset.canonical_asset_id, + "missing": asset.missing_at is not None, + "state_version": asset.state_version, + } + + class InventoryService: def __init__(self, session_factory: sessionmaker) -> None: self._session_factory = session_factory @@ -101,6 +116,30 @@ class InventoryService: result.counts = dict(Counter(result.occurrences.values())) return result + def list_assets( + self, + *, + limit: int = 50, + offset: int = 0, + availability: str | None = None, + query: str | None = None, + ) -> dict: + """Paged, filterable asset listing for the inventory UI (read-only).""" + limit = max(1, min(limit, 200)) + offset = max(0, offset) + with self._session_factory() as session: + stmt = select(Asset) + if availability: + stmt = stmt.where(Asset.availability_state == availability) + if query: + stmt = stmt.where(Asset.current_path.like(f"%{query}%")) + total = session.scalar(select(func.count()).select_from(stmt.subquery())) + rows = session.execute( + stmt.order_by(Asset.current_path).limit(limit).offset(offset) + ).scalars() + items = [_asset_dict(a) for a in rows] + return {"items": items, "total": int(total or 0), "limit": limit, "offset": offset} + def _reconcile_one( self, session: Session, diff --git a/pyproject.toml b/pyproject.toml index 7b6993e..6c63f0b 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -11,7 +11,16 @@ dependencies = [ ] [project.optional-dependencies] -test = ["pytest>=8", "httpx>=0.27"] +test = [ + "pytest>=8", + "httpx>=0.27", + "pillow>=10", + "numpy>=1.26", + "scipy>=1.11", + "playwright>=1.40", + "pytest-playwright>=0.4", +] +# Browser end-to-end tests also require: python -m playwright install chromium [tool.ruff] line-length = 100 diff --git a/tests/e2e/test_review_ui.py b/tests/e2e/test_review_ui.py new file mode 100644 index 0000000..f8e61ba --- /dev/null +++ b/tests/e2e/test_review_ui.py @@ -0,0 +1,184 @@ +"""Browser end-to-end review journeys against the real server + static frontend. + +Covers keyboard inventory review, fuzzy-decision confirmation, stale-version +conflict handling, and decision persistence across reload. The server runs as a +real child process; the browser talks to it over HTTP like production. +""" + +import shutil +import socket +import subprocess +import sys +import time +from pathlib import Path +from types import SimpleNamespace + +import httpx +import numpy as np +import pytest +from PIL import Image + +REPO = Path(__file__).resolve().parents[2] + + +def _structured(path, seed, size=(256, 192)): + path.parent.mkdir(parents=True, exist_ok=True) + rng = np.random.default_rng(seed) + w, h = size + base = np.zeros((h, w, 3), dtype=np.uint8) + for _ in range(6): + x0, y0 = int(rng.integers(0, w - 60)), int(rng.integers(0, h - 60)) + base[y0 : y0 + 60, x0 : x0 + 60] = rng.integers(0, 256, 3) + grad = np.linspace(0, 120, w, dtype=np.uint8) + base[:, :, 0] = np.clip(base[:, :, 0].astype(int) + grad[None, :], 0, 255) + Image.fromarray(base).save(path, quality=95) + return path + + +def _resized(src, dst, scale=0.5): + with Image.open(src) as image: + image.resize( + (int(image.width * scale), int(image.height * scale)), Image.LANCZOS + ).save(dst, quality=95) + + +def _free_port(): + with socket.socket() as sock: + sock.bind(("127.0.0.1", 0)) + return sock.getsockname()[1] + + +def _seed(data_dir, lib): + # In-process seed, then dispose so the server process owns the database freely. + from photo_pipeline.config import Config + from photo_pipeline.db import create_db_engine, create_session_factory, run_migrations + from photo_pipeline.services.duplicates import DuplicateService + from photo_pipeline.services.inventory import InventoryService + + a = _structured(lib / "a.jpg", 1) + shutil.copy2(a, lib / "a_copy.jpg") # exact pair + b = _structured(lib / "b.jpg", 2) + _resized(b, lib / "b_small.jpg") # perceptual pair + + config = Config.from_env( + {"PHOTO_PIPELINE_DATA_DIR": str(data_dir), "PHOTO_PIPELINE_LIBRARY_ROOTS": str(lib)} + ) + run_migrations(config.database_url) + engine = create_db_engine(config.database_url) + sf = create_session_factory(engine) + InventoryService(sf).scan(lib) + DuplicateService(sf).detect() + engine.dispose() + + +@pytest.fixture +def server(tmp_path): + data = tmp_path / "data" + data.mkdir() + lib = tmp_path / "lib" + lib.mkdir() + _seed(data, lib) + + port = _free_port() + env = { + "PATH": __import__("os").environ.get("PATH", ""), + "PHOTO_PIPELINE_DATA_DIR": str(data), + "PHOTO_PIPELINE_LIBRARY_ROOTS": str(lib), + "PHOTO_PIPELINE_HOST": "127.0.0.1", + "PHOTO_PIPELINE_PORT": str(port), + } + proc = subprocess.Popen( + [sys.executable, "-m", "photo_pipeline", "serve"], + cwd=str(REPO), + env=env, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + ) + base = f"http://127.0.0.1:{port}" + deadline = time.monotonic() + 30 + ready = False + while time.monotonic() < deadline: + if proc.poll() is not None: + out, err = proc.communicate() + pytest.fail(f"server exited: {err.decode(errors='replace')}") + try: + if httpx.get(f"{base}/api/v1/health/ready", timeout=1).status_code == 200: + ready = True + break + except httpx.HTTPError: + time.sleep(0.2) + if not ready: + proc.terminate() + pytest.fail("server never became ready") + + client = httpx.Client(base_url=base, timeout=5) + + def cluster_by_method(method): + clusters = client.get("/api/v1/duplicates/clusters").json()["items"] + return next(c for c in clusters if c["method"] == method) + + try: + yield SimpleNamespace(base=base, client=client, cluster_by_method=cluster_by_method) + finally: + client.close() + proc.terminate() + try: + proc.wait(timeout=10) + except subprocess.TimeoutExpired: + proc.kill() + + +def test_inventory_keyboard_review(page, server): + page.goto(f"{server.base}/app/#/inventory") + rows = page.get_by_test_id("asset-row") + rows.first.wait_for() + assert rows.count() == 4 + # First row auto-selected; arrow-down moves the selection. + assert rows.nth(0).get_attribute("aria-selected") == "true" + page.keyboard.press("ArrowDown") + assert rows.nth(1).get_attribute("aria-selected") == "true" + assert rows.nth(0).get_attribute("aria-selected") == "false" + + +def test_fuzzy_decision_requires_confirmation(page, server): + cluster = server.cluster_by_method("perceptual") + page.goto(f"{server.base}/app/#/duplicates/{cluster['id']}") + page.get_by_test_id("not-duplicate").click() + # A confirmation appears before anything is persisted. + page.get_by_test_id("confirm").wait_for() + assert "open" in page.get_by_test_id("cluster-state").inner_text() + page.get_by_test_id("confirm-yes").click() + page.wait_for_function( + "document.querySelector('[data-testid=cluster-state]').innerText.includes('dismissed')" + ) + + +def test_stale_version_shows_conflict(page, server): + cluster = server.cluster_by_method("perceptual") + page.goto(f"{server.base}/app/#/duplicates/{cluster['id']}") + # Open the confirmation for a fuzzy decision (this refetches the current version). + page.get_by_test_id("defer").click() + page.get_by_test_id("confirm").wait_for() + # While the dialog is open, another actor decides — bumping the version. + resp = server.client.post( + f"/api/v1/duplicates/clusters/{cluster['id']}/decision", + json={"decision": "deferred", "expected_version": cluster["version"]}, + ) + assert resp.status_code == 200 + # Confirming now carries the stale version; the server refuses it. + page.get_by_test_id("confirm-yes").click() + page.get_by_role("alert").wait_for() + assert "changed" in page.get_by_role("alert").inner_text() + + +def test_decision_persists_after_reload(page, server): + cluster = server.cluster_by_method("perceptual") + page.goto(f"{server.base}/app/#/duplicates/{cluster['id']}") + page.get_by_test_id("not-duplicate").click() + page.get_by_test_id("confirm-yes").click() + page.wait_for_function( + "document.querySelector('[data-testid=cluster-state]').innerText.includes('dismissed')" + ) + page.reload() + page.get_by_test_id("cluster-state").wait_for() + assert "dismissed" in page.get_by_test_id("cluster-state").inner_text() diff --git a/tests/integration/test_review_api.py b/tests/integration/test_review_api.py new file mode 100644 index 0000000..7ef8c44 --- /dev/null +++ b/tests/integration/test_review_api.py @@ -0,0 +1,145 @@ +"""Review API contract: inventory paging/filtering, cluster list/detail, and the +decision endpoint's success and error (404/409/422) behavior. + +Fixtures inlined to avoid shadowing the characterization suite's ``conftest``. +""" + +import shutil +from types import SimpleNamespace + +import numpy as np +import pytest +from fastapi.testclient import TestClient +from PIL import Image + +from photo_pipeline.api.app import create_app +from photo_pipeline.config import Config +from photo_pipeline.db import run_migrations +from photo_pipeline.services.duplicates import DuplicateService, Method +from photo_pipeline.services.inventory import InventoryService + + +def structured(path, seed, size=(256, 192)): + path.parent.mkdir(parents=True, exist_ok=True) + rng = np.random.default_rng(seed) + w, h = size + base = np.zeros((h, w, 3), dtype=np.uint8) + for _ in range(6): + x0, y0 = int(rng.integers(0, w - 60)), int(rng.integers(0, h - 60)) + base[y0 : y0 + 60, x0 : x0 + 60] = rng.integers(0, 256, 3) + grad = np.linspace(0, 120, w, dtype=np.uint8) + base[:, :, 0] = np.clip(base[:, :, 0].astype(int) + grad[None, :], 0, 255) + Image.fromarray(base).save(path, quality=95) + return path + + +def resized_copy(src, dst, scale=0.5): + with Image.open(src) as image: + image.resize( + (int(image.width * scale), int(image.height * scale)), Image.LANCZOS + ).save(dst, quality=95) + return dst + + +@pytest.fixture +def seeded(tmp_path): + data = tmp_path / "data" + data.mkdir() + lib = tmp_path / "lib" + lib.mkdir() + # exact pair + a perceptual pair + a = structured(lib / "a.jpg", 1) + shutil.copy2(a, lib / "a_copy.jpg") + b = structured(lib / "b.jpg", 2) + resized_copy(b, lib / "b_small.jpg") + config = Config.from_env( + {"PHOTO_PIPELINE_DATA_DIR": str(data), "PHOTO_PIPELINE_LIBRARY_ROOTS": str(lib)} + ) + run_migrations(config.database_url) + app = create_app(config) + client = TestClient(app) + client.__enter__() + InventoryService(app.state.session_factory).scan(lib) + DuplicateService(app.state.session_factory).detect() + yield SimpleNamespace(client=client, app=app) + client.__exit__(None, None, None) + + +def test_inventory_lists_and_pages(seeded): + client = seeded.client + full = client.get("/api/v1/inventory/assets").json() + assert full["total"] == 4 + page = client.get("/api/v1/inventory/assets?limit=2&offset=0").json() + assert len(page["items"]) == 2 + assert page["limit"] == 2 + page2 = client.get("/api/v1/inventory/assets?limit=2&offset=2").json() + assert page2["items"][0]["id"] != page["items"][0]["id"] + + +def test_inventory_filters_by_path(seeded): + result = seeded.client.get("/api/v1/inventory/assets?q=a_copy").json() + assert result["total"] == 1 + assert "a_copy" in result["items"][0]["current_path"] + + +def test_clusters_list_and_detail(seeded): + clusters = seeded.client.get("/api/v1/duplicates/clusters").json() + methods = {c["method"] for c in clusters["items"]} + assert {Method.EXACT.value, Method.PERCEPTUAL.value} <= methods + + perceptual = next(c for c in clusters["items"] if c["method"] == Method.PERCEPTUAL.value) + detail = seeded.client.get(f"/api/v1/duplicates/clusters/{perceptual['id']}").json() + assert detail["requires_confirmation"] is True + assert len(detail["members"]) == 2 + assert "evidence" in detail["members"][0] + + +def test_decision_success_and_persists(seeded): + clusters = seeded.client.get("/api/v1/duplicates/clusters?state=open").json() + cluster = clusters["items"][0] + resp = seeded.client.post( + f"/api/v1/duplicates/clusters/{cluster['id']}/decision", + json={"decision": "deferred", "expected_version": cluster["version"]}, + ) + assert resp.status_code == 200 + assert resp.json()["state"] == "deferred" + again = seeded.client.get(f"/api/v1/duplicates/clusters/{cluster['id']}").json() + assert again["state"] == "deferred" + + +def test_decision_stale_version_conflicts_without_mutation(seeded): + clusters = seeded.client.get("/api/v1/duplicates/clusters?state=open").json() + cluster = clusters["items"][0] + stale = seeded.client.post( + f"/api/v1/duplicates/clusters/{cluster['id']}/decision", + json={"decision": "deferred", "expected_version": cluster["version"] + 99}, + ) + assert stale.status_code == 409 + assert stale.json()["error"]["code"] == "version_conflict" + unchanged = seeded.client.get(f"/api/v1/duplicates/clusters/{cluster['id']}").json() + assert unchanged["state"] == cluster["state"] # nothing mutated + + +def test_decision_unknown_and_invalid(seeded): + assert ( + seeded.client.post( + "/api/v1/duplicates/clusters/ghost/decision", + json={"decision": "deferred", "expected_version": 1}, + ).status_code + == 404 + ) + clusters = seeded.client.get("/api/v1/duplicates/clusters?state=open").json() + cluster = clusters["items"][0] + invalid = seeded.client.post( + f"/api/v1/duplicates/clusters/{cluster['id']}/decision", + json={ + "decision": "canonical", + "expected_version": cluster["version"], + "canonical_asset_id": "not-a-member", + }, + ) + assert invalid.status_code == 422 + + +def test_cluster_detail_unknown_is_404(seeded): + assert seeded.client.get("/api/v1/duplicates/clusters/ghost").status_code == 404 -- 2.49.1