docs(plans): tier 2 papercuts — #22/#18/#9/#4/#34/#31/#32/#37

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-06-04 17:14:39 +02:00
parent 8442afbf02
commit fbb7a297a6
1 changed files with 217 additions and 0 deletions
@@ -0,0 +1,217 @@
+# Tier 2 Papercuts Implementation Plan
+
+> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax.
+
+**Goal:** Clear a batch of small, well-specified correctness/observability/UX fixes from the issue tracker (#22, #18, #9, #4, #34, #31, #32, #37) — no new features.
+
+**Architecture:** Independent small fixes grouped by area into four tasks: backend API behaviour (#22, #18), backend cleanup (#9, #4), frontend states/a11y (#34, #31, #32, #37), then verification.
+
+**Tech Stack:** Rust (axum, sqlx, tracing), React + TS, TanStack Query, react-i18next, Vitest + RTL + MSW.
+
+**Conventions (every task):** nightly `cargo +nightly fmt`; `cargo clippy`. Frontend: no `any`/`eslint-disable`/`@ts-ignore`; en/sv i18n parity; no codename "biggus"/"dickus". Test infra via compose: `DATABASE_URL=postgres://postgres:postgres@localhost:5442/cms_dev` (this machine's override port), `MEILI_URL=http://localhost:7700`, `MEILI_MASTER_KEY=masterKey`. cargo from repo root; web from `web/`.
+
+---
+
+## Task 1: Backend API — 404 for missing vocabulary (#22) + log public 500s (#18)
+
+**Files:** Modify `crates/api/src/admin_vocab.rs`, `crates/api/src/public.rs`; Test in the existing `crates/api/tests/admin_catalog.rs` (vocab/authority harness).
+
+### #22 — `add_term` returns 404 when the vocabulary doesn't exist
+Today `db::vocab::add_term(...)` maps every error to 500; a well-formed `{id}` for a missing vocabulary triggers a foreign-key violation (SQLSTATE 23503) that should be **404**.
+
+- [ ] **Step 1: Failing test** — add to `crates/api/tests/admin_catalog.rs` (mirror its existing seed-editor/login/oneshot harness). Read the file first to reuse its helpers:
+```rust
+#[sqlx::test(migrations = "../db/migrations")]
+async fn add_term_to_missing_vocabulary_is_404(pool: PgPool) {
+    // (use this file's existing migrate_sessions + seed editor + login helpers)
+    let app = /* build_app with state */;
+    let cookie = /* login as editor */;
+    let resp = app
+        .oneshot(
+            Request::builder()
+                .method("POST")
+                .uri("/api/admin/vocabularies/00000000-0000-0000-0000-000000000000/terms")
+                .header(header::COOKIE, &cookie)
+                .header(header::CONTENT_TYPE, "application/json")
+                .body(Body::from(r#"{"labels":[{"lang":"en","label":"X"}]}"#))
+                .unwrap(),
+        )
+        .await
+        .unwrap();
+    assert_eq!(resp.status(), StatusCode::NOT_FOUND);
+}
+```
+(Match the exact helper names/signatures already in `admin_catalog.rs`. If that file doesn't have a login helper, copy the pattern from `crates/api/tests/admin_fields.rs`.)
+
+- [ ] **Step 2: Run → fails** (currently 500): `cargo test -p api --test admin_catalog add_term_to_missing_vocabulary`.
+
+- [ ] **Step 3: Fix** — in `crates/api/src/admin_vocab.rs` `add_term`, replace:
+```rust
+    let term_id = db::vocab::add_term(&mut tx, &new)
+        .await
+        .map_err(|_| StatusCode::INTERNAL_SERVER_ERROR)?;
+```
+with:
+```rust
+    let term_id = db::vocab::add_term(&mut tx, &new).await.map_err(|err| {
+        // A well-formed id for a missing vocabulary hits the FK constraint (23503).
+        if err.as_database_error().and_then(|e| e.code()).as_deref() == Some("23503") {
+            StatusCode::NOT_FOUND
+        } else {
+            tracing::error!(?err, "adding term");
+            StatusCode::INTERNAL_SERVER_ERROR
+        }
+    })?;
+```
+
+- [ ] **Step 4: Run → passes**, and confirm adding a term to an existing vocab still returns 201 (existing tests cover this).
+
+### #18 — log the discarded `sqlx::Error` on public 500 paths
+`crates/api/src/public.rs` discards errors via `.map_err(|_| StatusCode::INTERNAL_SERVER_ERROR)` (lines ~74, ~78) and `Err(_) => StatusCode::INTERNAL_SERVER_ERROR.into_response()` (line ~109). `tracing` is already a dependency of the `api` crate — just log.
+
+- [ ] **Step 5:** In `list_objects`, change both `.map_err(|_| StatusCode::INTERNAL_SERVER_ERROR)?` to:
+```rust
+        .map_err(|err| {
+            tracing::error!(?err, "listing public objects");
+            StatusCode::INTERNAL_SERVER_ERROR
+        })?;
+```
+(use a message specific to each call site — e.g. "listing public objects" and "counting public objects" — match what each query does).
+
+- [ ] **Step 6:** In `get_object`, change the `Err(_) => StatusCode::INTERNAL_SERVER_ERROR.into_response()` arm to bind and log the error:
+```rust
+        Err(err) => {
+            tracing::error!(?err, "fetching public object");
+            StatusCode::INTERNAL_SERVER_ERROR.into_response()
+        }
+```
+
+- [ ] **Step 7: Verify** — `cargo +nightly fmt`, `cargo clippy -p api --all-targets`, `cargo test -p api`. Commit:
+```bash
+git add crates/api
+git commit -m "fix(api): 404 when adding a term to a missing vocabulary (#22); log public 500s (#18)"
+```
+
+---
+
+## Task 2: Backend cleanup — enum/CHECK cross-refs (#9) + dead clone & test handle (#4)
+
+**Files:** Modify `crates/domain/src/authority.rs`, `crates/domain/src/audit.rs`, `crates/server/src/lib.rs`, `crates/server/tests/serve.rs`.
+
+> Do **not** edit any file under `crates/db/migrations/` — `sqlx::migrate!()` checksums applied migrations, so editing them (even a comment) breaks existing databases. The cross-reference comments go in the Rust enums only.
+
+- [ ] **Step 1: #9 — cross-reference comments.**
+  - In `crates/domain/src/authority.rs`, above `pub enum AuthorityKind`, add:
+    ```rust
+    /// Allowed kinds. NOTE: kept in sync by hand with the
+    /// `CHECK (kind IN ('person','organisation','place'))` constraint in
+    /// `crates/db/migrations/0002_vocabularies_authorities.sql` — update both together.
+    ```
+  - In `crates/domain/src/audit.rs`, above `pub enum AuditAction`, add an equivalent comment pointing at the `action` CHECK in `crates/db/migrations/0001_*.sql` (open the migration to name the exact file + values).
+
+- [ ] **Step 2: #4 — remove the dead clone.** In `crates/server/src/lib.rs` `run`, the `AppState` is built with `app_name: config.app_name.clone()`. Since `config.app_name` is a `String` and the only later use of `config` is the disjoint field `config.bind_addr`, change it to a move:
+```rust
+        app_name: config.app_name,
+```
+Confirm it still compiles (partial move of one field; `&config.bind_addr` afterward is fine).
+
+- [ ] **Step 3: #4 — smoke-test handle.** Open `crates/server/tests/serve.rs`. The spawned `serve(...)` task's `.unwrap()` swallows server errors as a task panic, surfacing as a confusing client error. Capture the `JoinHandle` and, after the assertions, either abort it cleanly or check it didn't error — make a server-start failure surface as a clear test failure rather than a `reqwest` error. Read the file and apply the minimal change that propagates/surfaces the server error (e.g. keep the handle, assert it hasn't finished-with-error, or `handle.abort()` at the end). Keep the test green.
+
+- [ ] **Step 4: Verify** — `cargo +nightly fmt`, `cargo clippy --workspace --all-targets`, `cargo test -p server -p domain`. Commit:
+```bash
+git add crates/domain crates/server
+git commit -m "chore: cross-ref enum/CHECK constraints (#9); drop dead clone + harden smoke test (#4)"
+```
+
+---
+
+## Task 3: Frontend — search 503 (#34), list error states (#31), a11y + dead keys (#32), authority-kind test (#37)
+
+**Files:** Modify `web/src/api/queries.ts`, `web/src/search/search-panel.tsx`, `web/src/vocab/vocabulary-terms.tsx`, `web/src/authorities/authorities-page.tsx`, `web/src/i18n/{en,sv}.json`, `web/src/fields/fields.test.tsx`; Tests in `web/src/search/search.test.tsx`, plus the vocab/authorities test files.
+
+### #34 — distinguish search 503 ("unavailable") from a generic error
+- [ ] **Step 1:** In `web/src/api/queries.ts`, add a tiny typed error and have `useSearch` throw it with the HTTP status (so the UI can branch without `any`). Near the top:
+```ts
+export class HttpError extends Error {
+  constructor(public readonly status: number) {
+    super(`HTTP ${status}`);
+    this.name = "HttpError";
+  }
+}
+```
+In `useSearch`'s `queryFn`, replace `if (error || !data) throw new Error("search failed");` with:
+```ts
+      if (error || !data) throw new HttpError(response.status);
+```
+(`response` is already destructured from `api.GET`; if not, add it.)
+
+- [ ] **Step 2: i18n** — add `search.unavailable` to BOTH `en.json` and `sv.json` (parity):
+  - en: `"unavailable": "Search is not available on this server"`
+  - sv: `"unavailable": "Sök är inte tillgängligt på den här servern"`
+
+- [ ] **Step 3:** In `web/src/search/search-panel.tsx`, where `search.isError` renders `t("search.loadError")`, branch on a 503:
+```tsx
+        {hasQuery && search.isError && (
+          <p className="p-4 text-sm text-red-600">
+            {search.error instanceof HttpError && search.error.status === 503
+              ? t("search.unavailable")
+              : t("search.loadError")}
+          </p>
+        )}
+```
+Import `HttpError` from `../api/queries`.
+
+- [ ] **Step 4: Tests** — in `web/src/search/search.test.tsx`, add: a `503` response → renders `search.unavailable`; a `500` response → renders `search.loadError`. (Use `server.use(http.get("/api/admin/search", () => new HttpResponse(null, { status: 503 })))` etc., then type a query and assert the text.)
+
+### #31 — loading/error states on the terms + authorities lists
+- [ ] **Step 5:** In `web/src/vocab/vocabulary-terms.tsx`, the terms list uses `useTerms(id)` but renders empty/data only. Add `isLoading` (skeleton or `…`) and `isError` (`t("vocab.loadError")`) branches before the empty/data render, mirroring `vocabulary-list.tsx`'s state ladder.
+- [ ] **Step 6:** In `web/src/authorities/authorities-page.tsx`, the list uses `useAuthorities(kind)`; add an `isError` branch rendering `t("authorities.loadError")` (currently a dead key — this uses it) and a loading branch. Keep the existing empty/data render.
+- [ ] **Step 7: Tests** — add an error-state test to the vocab and authorities test files: MSW returns 500 for the terms / authorities GET → the respective `loadError` text appears. (Override the default handler with `server.use(...)`.)
+
+### #32 — ARIA tab semantics + remove dead i18n keys
+- [ ] **Step 8:** In `web/src/authorities/authorities-page.tsx`, the kind tabs are `NavLink`s. Add tab semantics: wrap them in a container with `role="tablist"`, give each `role="tab"` and `aria-selected={isActive}` (the `NavLink` className callback already exposes `isActive` — use the render-prop form to set `aria-selected`). Keep the existing styling.
+- [ ] **Step 9:** Remove the unused keys `vocab.title` and `authorities.title` from BOTH `en.json` and `sv.json` (grep first: `grep -rn "vocab.title\|authorities.title\|\.title" web/src` — confirm only the i18n definitions match; nothing references them).
+
+### #37 — frontend authority-kind reveal test
+- [ ] **Step 10:** In `web/src/fields/fields.test.tsx`, add a test mirroring the existing Term test: type a key + EN label, `selectOptions(type, "authority")`, assert the authority-kind `<select>` (label `/authority kind/i`) appears, `selectOptions` it to `"person"`, submit, and assert the POST body's `authority_kind === "person"` (use a `server.use` POST handler that captures the body, like the Term test does).
+
+- [ ] **Step 11: Verify** — `cd web && pnpm test && pnpm typecheck && pnpm lint && pnpm build && pnpm check:size`. All green; bundle ≤150 KB. Commit:
+```bash
+cd /Users/olsson/Laboratory/biggus-dickus
+git add web
+git commit -m "fix(web): search 503 vs error (#34); terms/authorities list error states (#31); authority-tab a11y + dead keys (#32); authority-kind test (#37)"
+```
+
+---
+
+## Task 4: Verification
+
+- [ ] **Step 1: i18n parity** —
+```bash
+cd web
+node -e "const a=require('./src/i18n/en.json'),b=require('./src/i18n/sv.json');const k=o=>Object.entries(o).flatMap(([K,v])=>typeof v==='object'?k(v).map(s=>K+'.'+s):[K]);const ka=k(a).sort(),kb=k(b).sort();console.log(JSON.stringify(ka)===JSON.stringify(kb)?'PARITY OK':'MISMATCH '+JSON.stringify({onlyEn:ka.filter(x=>!kb.includes(x)),onlySv:kb.filter(x=>!ka.includes(x))}))"
+```
+Expected `PARITY OK`.
+
+- [ ] **Step 2: Frontend** — `pnpm typecheck && pnpm lint && pnpm test && pnpm build && pnpm check:size` (report bundle gz).
+
+- [ ] **Step 3: Backend** —
+```bash
+cd /Users/olsson/Laboratory/biggus-dickus
+DATABASE_URL=postgres://postgres:postgres@localhost:5442/cms_dev \
+  MEILI_URL=http://localhost:7700 MEILI_MASTER_KEY=masterKey \
+  cargo test -p api -p domain -p server
+cargo clippy --workspace --all-targets
+cargo +nightly fmt --check
+```
+All pass; clippy + fmt clean.
+
+- [ ] **Step 4:** No codename: `git grep -in 'biggus\|dickus' -- crates web/src` → no matches.
+
+---
+
+## Self-Review (completed)
+- **Spec coverage:** #22 (404), #18 (log 500s) → Task 1; #9 (Rust cross-ref comments), #4 (clone + smoke test) → Task 2; #34, #31, #32, #37 → Task 3; parity + suites → Task 4. ✓
+- **Scope adjustments baked in:** #8 already closed (thiserror is used); #37 backend-403 omitted (no non-EditCatalogue role exists); #9 Rust-side only (migration checksums). ✓
+- **Placeholder scan:** none — code is concrete; the "match the existing harness" notes are verification instructions against named files.
+- **Type consistency:** `HttpError` defined in queries.ts and imported in search-panel; the 23503/FK pattern matches the field-def handler; `authorities.loadError` (existing key) now consumed; `search.unavailable` added at parity.