From fe6ce1058a7ceaba7f79665bc5d06ba0e813c0ff Mon Sep 17 00:00:00 2001 From: Bogdan0708 Date: Wed, 23 Sep 2026 17:06:49 +0100 Subject: [PATCH 01/24] =?UTF-8?q?chore(staging):=20record=20pilot=20projec?= =?UTF-8?q?t=20setup=20(O3=E2=80=93O6)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add the staging alias for safebite-pilot-urfs3v, the verified inventory of the legacy project, leaked key and new pilot project, and the owner ruling that lets the primary assistant perform O3–O6 via the official CLIs. Co-Authored-By: Claude Opus 5.5 (1M context) --- .firebaserc | 3 +- .../specs/2026-09-20-safebite-pwa-design.md | 10 ++-- planning/specs/firebase-inventory.md | 46 +++++++++++++++++++ 3 files changed, 54 insertions(+), 5 deletions(-) create mode 100644 planning/specs/firebase-inventory.md diff --git a/.firebaserc b/.firebaserc index 0f85de9..ec0f378 100644 --- a/.firebaserc +++ b/.firebaserc @@ -1,5 +1,6 @@ { "projects": { - "default": "demo-safebite" + "default": "demo-safebite", + "staging": "safebite-pilot-urfs3v" } } diff --git a/planning/specs/2026-09-20-safebite-pwa-design.md b/planning/specs/2026-09-20-safebite-pwa-design.md index da389e6..4c40ea3 100644 --- a/planning/specs/2026-09-20-safebite-pwa-design.md +++ b/planning/specs/2026-09-20-safebite-pwa-design.md @@ -227,14 +227,16 @@ Real-iPhone acceptance (owner + Ava) happens after staging deploy, outside CI. ### 3.1 Owner-only prerequisites (agents must never do these) +Owner ruling (2026-09-23): the primary assistant may carry out O3–O6 through the official `gcloud`/`firebase` CLIs, confirming each billing or irreversible step first. Delegated subagents still may not. Deploys and pushes remain governed by §3.2. + | ID | Action | Needed before | |----|--------|---------------| | O1 | `git pull --ff-only` to bring `a3403d1` into local `main` | Plan 1 | | O2 | Install a JDK (21+) so Firebase emulators run locally | Plan 1 verification | -| O3 | `firebase login --reauth`; then inventory `safebite-production-13ba1` read-only (owners, Firestore location, deployed rules, users, billing, API restrictions). Record findings in `planning/specs/firebase-inventory.md` | Plan 5 staging | -| O4 | Check the historical key from commit `e7c0268` in Google Cloud Console: restrict or delete it. Create a **new** server key restricted to Places API (New) for the pilot project only | Plan 3 staging | -| O5 | Create a new Firebase project for the pilot (Firestore in `europe-west2`), Blaze plan with a budget alert at £10, add alias `staging` to `.firebaserc` | Plan 5 | -| O6 | Create the two member accounts in the pilot project's Auth and their `users/` + `households/` docs via console or admin script | Plan 5 | +| O3 | ~~`firebase login --reauth`; inventory `safebite-production-13ba1` read-only~~ Done 2026-09-23: legacy project not reachable from the owner account; see `planning/specs/firebase-inventory.md` | Plan 5 staging | +| O4 | Historical key from `e7c0268`: its parent project (`776764264965`) is not held by any owner account, so it cannot be restricted (2026-09-23). **Still open:** the owner creates a **new** server key restricted to Places API (New) in the pilot project and supplies it as secret `PLACES_API_KEY` | Plan 3 staging | +| O5 | ~~Create the pilot Firebase project~~ Done 2026-09-23: `safebite-pilot-urfs3v`, Firestore `europe-west2`, Blaze, project-scoped £10 budget, alias `staging`, self sign-up disabled | Plan 5 | +| O6 | ~~Create the two member accounts and their `users/` + `households/` docs~~ Done 2026-09-23 by admin script; both sign-ins verified | Plan 5 | | O7 | ~~Decide whether `docs/` remains a GitHub Pages site~~ Done: `docs/` stays public Pages; planning docs live in `planning/` | — | | O8 | Approve this spec and Plan 1 | Any implementation | diff --git a/planning/specs/firebase-inventory.md b/planning/specs/firebase-inventory.md new file mode 100644 index 0000000..521630d --- /dev/null +++ b/planning/specs/firebase-inventory.md @@ -0,0 +1,46 @@ +# Firebase and Google Cloud inventory (O3–O6) + +Recorded 2026-09-23. On that day the owner authorised the primary assistant to carry out O3–O6 through the official `gcloud` and `firebase` CLIs, running as ``. Every result below was read back after the change was made. + +## O3 — legacy project `safebite-production-13ba1` + +Not reachable. `gcloud projects describe`, the Firebase Management API (403) and `firebase projects:list` all refuse or omit it for the owner account. It is not in the account's pending-deletion list (the last 30 days). The legacy iOS config (`SafeBite/SafeBite/GoogleService-Info.plist`) gives project number `113380788016`. The pilot does not depend on this project, so no inventory is possible or needed. If it ever reappears under another account, treat its rules (public read) and seeded data as untrusted. + +## O4 — historical key from commit `e7c0268` + +The API Keys lookup was denied (`apikeys.keys.lookup`), but the error names the key's parent as project number `776764264965`. The owner could not find that project under any account they hold. The key therefore belongs to a deleted or foreign project and cannot be restricted from here. The pilot never uses it. The replacement Places key will be created in the pilot project (owner supplies it later). Until then, `config/discovery` is absent, so discovery stays switched off. + +## O5 — pilot project + +| Item | Value | +|---|---| +| Project ID / number | `safebite-pilot-urfs3v` / `1081260388315` | +| `.firebaserc` alias | `staging` (default stays `demo-safebite`) | +| Billing | Blaze, billing account `` | +| Budget | "SafeBite pilot £10": £10/month, scoped to this project only; alerts at 50 %, 90 %, 100 % actual and 100 % forecast (the account-wide £10 budget is separate and unchanged) | +| Firestore | `(default)`, Native mode, `europe-west2`, delete protection enabled | +| Firestore rules | None released yet, so all client access is denied until `firestore.rules` is deployed | +| Auth | Firebase Auth (not Identity Platform), email + password only, **self sign-up disabled** (`client.permissions.disabledUserSignup`; a public `accounts:signUp` returns `ADMIN_ONLY_OPERATION`) | +| Authorised domains | `localhost`, `safebite-pilot-urfs3v.firebaseapp.com`, `safebite-pilot-urfs3v.web.app` | +| Web app | "SafeBite PWA", app ID `1:1081260388315:web:6696e3bc27a3170e6041fe`; its `VITE_FIREBASE_*` values come from `firebase apps:sdkconfig` and are not committed | +| APIs enabled | Firestore, Identity Toolkit, Cloud Functions, Cloud Build, Artifact Registry, Cloud Run, Eventarc, Secret Manager, Places (New), Firebase Rules, Firebase Hosting, Billing Budgets | + +The owner considered reusing `mitch-ai-services`, then chose a new project instead. That project runs an unrelated live Cloud Run service whose default service account holds `roles/editor`, so it could reach household data, and its existing spend would make a £10 SafeBite budget meaningless. + +A stray console-created project `safebyte-1` (number `1045562242738`, no billing, no data) appeared during setup; the owner decides whether to delete it. + +## O6 — member accounts + +Created with a one-off, uncommitted admin script (REST, the owner's gcloud credentials, no service-account key). It writes the same shapes as `functions/src/seed-emulator.ts`: + +- `households/home`: `name: "Home"`, `memberIds` = both UIDs, `createdAt` +- `users/`: Bogdan (``), `householdId: "home"` +- `users/`: Ava (``), `householdId: "home"` + +Both accounts signed in successfully with their generated temporary passwords, which are held only in `~/.config/safebite/pilot-accounts.txt` (mode 600) on the owner's machine. The app has no password-change or reset flow yet; add one (Plan 5 settings) or reset via the Admin API before handing Ava her password. + +## Still outstanding before staging acceptance + +- New Places key in the pilot project, stored as the functions secret `PLACES_API_KEY`, then `config/discovery` created with `enabled: true` and a daily cap. +- Deploy order (landing review): functions, then rules and hosting together. +- Real iPhone/Safari acceptance (Plan 5). From 4e0bc201a6dfa601b79f7a56a5521579758b9916 Mon Sep 17 00:00:00 2001 From: Bogdan0708 Date: Wed, 23 Sep 2026 17:35:41 +0100 Subject: [PATCH 02/24] chore(staging): record Places key and member password setup Co-Authored-By: Claude Opus 5.5 (1M context) --- planning/specs/2026-09-20-safebite-pwa-design.md | 2 +- planning/specs/firebase-inventory.md | 9 +++++---- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/planning/specs/2026-09-20-safebite-pwa-design.md b/planning/specs/2026-09-20-safebite-pwa-design.md index 4c40ea3..aac5e5f 100644 --- a/planning/specs/2026-09-20-safebite-pwa-design.md +++ b/planning/specs/2026-09-20-safebite-pwa-design.md @@ -234,7 +234,7 @@ Owner ruling (2026-09-23): the primary assistant may carry out O3–O6 through t | O1 | `git pull --ff-only` to bring `a3403d1` into local `main` | Plan 1 | | O2 | Install a JDK (21+) so Firebase emulators run locally | Plan 1 verification | | O3 | ~~`firebase login --reauth`; inventory `safebite-production-13ba1` read-only~~ Done 2026-09-23: legacy project not reachable from the owner account; see `planning/specs/firebase-inventory.md` | Plan 5 staging | -| O4 | Historical key from `e7c0268`: its parent project (`776764264965`) is not held by any owner account, so it cannot be restricted (2026-09-23). **Still open:** the owner creates a **new** server key restricted to Places API (New) in the pilot project and supplies it as secret `PLACES_API_KEY` | Plan 3 staging | +| O4 | Historical key from `e7c0268`: its parent project (`776764264965`) is not held by any owner account, so it cannot be restricted (2026-09-23). A new key restricted to Places API (New) was created in the pilot project and stored as secret `PLACES_API_KEY` (done 2026-09-23) | Plan 3 staging | | O5 | ~~Create the pilot Firebase project~~ Done 2026-09-23: `safebite-pilot-urfs3v`, Firestore `europe-west2`, Blaze, project-scoped £10 budget, alias `staging`, self sign-up disabled | Plan 5 | | O6 | ~~Create the two member accounts and their `users/` + `households/` docs~~ Done 2026-09-23 by admin script; both sign-ins verified | Plan 5 | | O7 | ~~Decide whether `docs/` remains a GitHub Pages site~~ Done: `docs/` stays public Pages; planning docs live in `planning/` | — | diff --git a/planning/specs/firebase-inventory.md b/planning/specs/firebase-inventory.md index 521630d..f9ceccd 100644 --- a/planning/specs/firebase-inventory.md +++ b/planning/specs/firebase-inventory.md @@ -8,7 +8,7 @@ Not reachable. `gcloud projects describe`, the Firebase Management API (403) and ## O4 — historical key from commit `e7c0268` -The API Keys lookup was denied (`apikeys.keys.lookup`), but the error names the key's parent as project number `776764264965`. The owner could not find that project under any account they hold. The key therefore belongs to a deleted or foreign project and cannot be restricted from here. The pilot never uses it. The replacement Places key will be created in the pilot project (owner supplies it later). Until then, `config/discovery` is absent, so discovery stays switched off. +The API Keys lookup was denied (`apikeys.keys.lookup`), but the error names the key's parent as project number `776764264965`. The owner could not find that project under any account they hold. The key therefore belongs to a deleted or foreign project and cannot be restricted from here. The pilot never uses it. The replacement key was created in the pilot project on 2026-09-23 (see O5 below). `config/discovery` is still absent, so discovery stays switched off until deploy. ## O5 — pilot project @@ -23,7 +23,8 @@ The API Keys lookup was denied (`apikeys.keys.lookup`), but the error names the | Auth | Firebase Auth (not Identity Platform), email + password only, **self sign-up disabled** (`client.permissions.disabledUserSignup`; a public `accounts:signUp` returns `ADMIN_ONLY_OPERATION`) | | Authorised domains | `localhost`, `safebite-pilot-urfs3v.firebaseapp.com`, `safebite-pilot-urfs3v.web.app` | | Web app | "SafeBite PWA", app ID `1:1081260388315:web:6696e3bc27a3170e6041fe`; its `VITE_FIREBASE_*` values come from `firebase apps:sdkconfig` and are not committed | -| APIs enabled | Firestore, Identity Toolkit, Cloud Functions, Cloud Build, Artifact Registry, Cloud Run, Eventarc, Secret Manager, Places (New), Firebase Rules, Firebase Hosting, Billing Budgets | +| Places key | API key `` ("SafeBite functions Places (server)"), API restriction `places.googleapis.com` only, no application restriction (server-side use from Cloud Functions). Piped straight into Secret Manager secret `PLACES_API_KEY` (version 1, label `firebase-managed=functions`) without being displayed; verified with one free IDs-only Text Search | +| APIs enabled | Firestore, Identity Toolkit, Cloud Functions, Cloud Build, Artifact Registry, Cloud Run, Eventarc, Secret Manager, Places (New), Firebase Rules, Firebase Hosting, Billing Budgets, API Keys | The owner considered reusing `mitch-ai-services`, then chose a new project instead. That project runs an unrelated live Cloud Run service whose default service account holds `roles/editor`, so it could reach household data, and its existing spend would make a £10 SafeBite budget meaningless. @@ -37,10 +38,10 @@ Created with a one-off, uncommitted admin script (REST, the owner's gcloud crede - `users/`: Bogdan (``), `householdId: "home"` - `users/`: Ava (``), `householdId: "home"` -Both accounts signed in successfully with their generated temporary passwords, which are held only in `~/.config/safebite/pilot-accounts.txt` (mode 600) on the owner's machine. The app has no password-change or reset flow yet; add one (Plan 5 settings) or reset via the Admin API before handing Ava her password. +Both accounts signed in successfully. The owner then replaced the generated passwords with chosen ones in `~/.config/safebite/pilot-accounts.txt` (mode 600) on the owner's machine; they were applied through the Admin API and both sign-ins re-verified. The app has no password-change flow yet. ## Still outstanding before staging acceptance -- New Places key in the pilot project, stored as the functions secret `PLACES_API_KEY`, then `config/discovery` created with `enabled: true` and a daily cap. +- Create `config/discovery` with `enabled: true` and a daily cap at deploy time. - Deploy order (landing review): functions, then rules and hosting together. - Real iPhone/Safari acceptance (Plan 5). From 4aacea9e1ff1984a162796cc354ee52137379fba Mon Sep 17 00:00:00 2001 From: Bogdan0708 Date: Thu, 24 Sep 2026 08:05:24 +0100 Subject: [PATCH 03/24] =?UTF-8?q?docs(spec):=20add=20=C2=A73.7=20Plan=204?= =?UTF-8?q?=20design=20(shortlist,=20visited,=20notes)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Record the owner rulings from the 2026-09-24 brainstorm: shortlist within records, notes on the restaurant, household-wide visited, a separate collection/{rid} state document, and change password in Settings. Align the §2.3 data model rows and the §3.3 plan table. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../specs/2026-09-20-safebite-pwa-design.md | 158 +++++++++++++++++- 1 file changed, 155 insertions(+), 3 deletions(-) diff --git a/planning/specs/2026-09-20-safebite-pwa-design.md b/planning/specs/2026-09-20-safebite-pwa-design.md index aac5e5f..6c6b3d5 100644 --- a/planning/specs/2026-09-20-safebite-pwa-design.md +++ b/planning/specs/2026-09-20-safebite-pwa-design.md @@ -157,8 +157,8 @@ All household data lives under `households/{householdId}`. | `households/{hid}` | `name`, `memberIds: string[]`, `createdAt` | admin only | | `households/{hid}/restaurants/{rid}` | `name`, `address`, `lat`, `lng`, `googlePlaceId?`, `phone?`, `website?`, `createdBy`, `createdAt`, `updatedAt`, `version` | members via rules | | `households/{hid}/restaurants/{rid}/claims/{cid}` | `kind` (`dedicatedKitchen`, `separateFryer`, `trainedStaff`, `gfMenu`, `preparationPractice`, `accreditation`), `value` (`yes`/`no`/`partial`), `detail`, `source: {type: 'restaurantStatement'|'accreditingBody'|'ownVisit'|'thirdParty', label, url?}`, `checkedAt`, `expiresAt?`, `authorUid`, `createdAt` | members via rules; accreditation kind validated by rules to require `source.type == 'accreditingBody'` and non-empty `source.url` | -| `households/{hid}/collection/{rid}` | `restaurantId`, `savedBy`, `savedAt`, `visited`, `visitedAt?`, `version` | members via rules | -| `households/{hid}/collection/{rid}/notes/{nid}` | `authorUid`, `text`, `createdAt`, `updatedAt` | author only | +| `households/{hid}/collection/{rid}` | `shortlisted`, `visited`, `visitedOn?`, `updatedBy`, `updatedByName`, `updatedAt`, `version` (see §3.7) | members via rules | +| `households/{hid}/restaurants/{rid}/notes/{nid}` | `text`, `authorUid`, `authorName`, `createdAt`, `updatedAt`, `version` (see §3.7) | author only; any member during restaurant deletion | | `config/discovery` | `enabled: boolean`, `dailySearchCap: number` | admin only (kill switch) | | `households/{hid}/usage/{yyyymmdd}` | `searches`, `details` | functions only | @@ -269,7 +269,7 @@ branch/worktree, reviewed, then merged before the next begins. | **1. Foundation and household auth** — `planning/plans/2026-09-20-safebite-pwa-01-foundation.md` | Repo hygiene; `web/` + `functions/` scaffolds; emulator-only config; new rules for `users`/`households`; `requireMember` + `whoami` callable; sign-in / not-invited / member shell; emulator seed; Playwright + CI. A member signs in and sees the shell; a non-member is refused; rules tests prove isolation | O1, O2 | | **2. Restaurant records and evidence** | **PWA app shell first** (`vite-plugin-pwa` manifest, real icon set replacing the Vite logo, `apple-touch-icon`, `apple-mobile-web-app-capable`, `theme-color`, standalone display — a plan gap found in the Plan 1 final review); then `restaurants` + `claims` model, rules with accreditation validation and version checks, private editing form, evidence display with checked/expired states, "call ahead" prompts, unit + rules + e2e tests | Plan 1 — 2a, 2a-h and 2b executed 2026-09-21 (see §3.5 and `planning/plans/2026-09-21-safebite-pwa-02b-records.md`) | | **3. Discovery through functions** | `searchDestination`, `searchNearby`, `placeDetails` callables with secret key, kill switch, caps, attribution; discover UI with all failure states; search cancellation; external directions links; "add to our records" from a result (stores place ID only) | Plan 2 — design in §3.6 (2026-09-22) | -| **4. Shared collection and notes** | `collection` + `notes` model and rules, save/unsave/visited, authored notes, optimistic concurrency with reload prompt, account-switch cache clearing, e2e | Plan 2 (Plan 3 optional) | +| **4. Shared collection and notes** | `collection` + `notes` model and rules, save/unsave/visited, authored notes, optimistic concurrency with reload prompt, account-switch cache clearing, e2e | Plan 2 (Plan 3 optional) — design in §3.7 (2026-09-24) | | **5. Privacy, offline, operations** | Opt-in offline download to IndexedDB, clear-on-signout, export callable, account-deletion callable, settings page, privacy/terms content, staging config files, cost-control checklist, real-iPhone acceptance script | Plans 1–4, O3–O6 | Plans 2–5 are written after Plan 1 is executed and reviewed, so they can name @@ -881,3 +881,155 @@ regardless (audit observation, 2026-09-22). Place Details; coordinates on records; offline copies of anything from Google; visited state and notes (Plan 4); the square-icon question (Plan 5); staging key creation and secret binding (owner action O4 — a prerequisite for staging, not for this plan); Firestore index changes. + +### 3.7 Plan 4 design — shortlist, visited state and notes (brainstormed 2026-09-24) + +Plan 2b made the Saved tab list every restaurant record, so a record is already shared by the +household. Plan 4 therefore does not add a second "saved" list. It adds a shortlist and visited +state *on top of* the records, plus authored notes. This section supersedes the `collection` and +`notes` rows of §2.3 wherever they differ. + +Owner rulings taken during the brainstorm: + +1. **Shortlist within the records.** Records remain everything the household has researched, + including places judged unsafe. "Shortlisted" means "we want to go". The Saved tab filters + *Shortlist* (default) or *All records*. Named trip lists are deferred. +2. **Notes belong to the restaurant record**, not to the shortlist entry. They survive + un-shortlisting and are removed only with the restaurant. +3. **Visited is household-wide:** one flag plus a visit date, settable and clearable by either + member, independent of the shortlist. No per-member visits, no visit log. +4. **State lives in a separate document per restaurant** (`collection/{rid}`), so toggling + shortlist or visited never bumps the restaurant's `version` and never conflicts with an edit + of its details. (Rejected: fields on the restaurant, since every toggle would conflict with edits and + reopen the reviewed Plan 2b rules; a `state` subdocument under each restaurant, which needs a + collection-group query and index to list.) +5. **Change password ships in Plan 4** (Settings), not Plan 5. + +#### Data model + +`households/{hid}/collection/{rid}`: document id = restaurant id; created on first use. + +| Field | Rule | +|---|---| +| `shortlisted` | bool | +| `visited` | bool | +| `visitedOn` | present exactly when `visited == true`; UTC-midnight timestamp (the `checkedAt` convention); `<= request.time + 1 day` | +| `updatedBy` | `== request.auth.uid` | +| `updatedByName` | `==` the caller's own `users/{uid}.displayName` (as `authorName` on claims). Members cannot read each other's `users` documents, so the name is denormalised | +| `updatedAt` | `== request.time` | +| `version` | create `== 1`; update `== resource.data.version + 1` | + +Keys are exactly these (`hasOnly`/`hasAll`, with `visitedOn` optional). Create and update require +the parent restaurant to exist with `deleting == false`. Delete is allowed only while the parent is +marked `deleting` (sweep step). Clients never delete a collection document otherwise: +un-shortlisting writes `shortlisted: false`. The §2.3 fields `restaurantId`, `savedBy` and `savedAt` are +dropped: the id names the restaurant and `updatedBy`/`updatedAt` replace the others. + +`households/{hid}/restaurants/{rid}/notes/{nid}` + +| Field | Rule | +|---|---| +| `text` | non-blank string, `<= 2000` characters (`LIMITS.note`, parity-tested) | +| `authorUid` | `== request.auth.uid` on create; immutable | +| `authorName` | `==` the caller's own `users/{uid}.displayName` on create; immutable | +| `createdAt` | `== request.time` on create; immutable | +| `updatedAt` | `== request.time` on every write | +| `version` | create `== 1`; update `== resource.data.version + 1` | + +Read: any member. Create: any member, parent exists and is not `deleting`. Update: the author only, +changing only `text`, `updatedAt`, `version`, and only while the parent is not `deleting`. +Delete: the author at any time, **or any member while the parent is `deleting`** (so the sweep can +remove the other member's notes). + +Neither document carries anything a safety label could be derived from, and neither write touches a +claim, so visiting or noting can never refresh a verification date (§2.1). + +#### Deletion protocol (extends §3.5) + +Mark `deleting` → sweep claims → sweep notes (same paged, server-read, transactional sweep) → +delete `collection/{rid}` if present → delete the restaurant. Every step is idempotent. The +existing resume path ("Finish deleting" on the Saved page, automatic resume once per mount) runs +the extended sequence. Progress text gains "Removing notes…". The deletion confirmation reads: +"Deletes the restaurant, its evidence, and both members' notes." + +#### Client + +Repository (`web/src/records/repository.ts`, still the only Firestore module for records, or a +sibling `collection.ts`/`notes.ts` if the file would pass ~400 lines): `watchCollection(hid)`, +`setShortlisted`, `setVisited(hid, rid, base, visitedOn | null)`, `watchNotes`, `addNote`, +`updateNote`, `deleteNote`, `sweepNotes`. All writes are `runTransaction`s with typed +`WriteOutcome`s. Version checks run inside the transaction (a missing collection document is +base version 0 → create). Online-only, as in §3.5. + +**Saved page.** Filter *Shortlist* | *All records*, held in component state only. Rows show name, +address and plain labels "Shortlisted" / "Visited 3 May 2026", with no safety wording in the list. +Order by name. Empty states: no records ("No restaurants yet. Add the first one.") vs an empty +shortlist ("Nothing on the shortlist. Open a record and tap Add to shortlist."). Two listeners +(restaurants, collection) joined by id in a pure, unit-tested function. One offline notice when +either is cached. Deleting rows appear under both filters. No toggles in the list. + +**Restaurant page.** +- A status block under the address: "Add to shortlist" / "On shortlist · Remove"; "Mark visited" opens + an inline date field (default today, no future dates) with Save/Cancel; when visited: "Visited + · Change date · Clear"; "Last changed by ". A version conflict shows the + current state with the standard conflict message and never auto-retries. Offline disables the + controls, as with "Add evidence". +- "Our notes", between Evidence and "Call ahead and ask", subtitled "Personal notes. They are not + evidence and don't change any checked date." Newest first. Each note shows the text, the author, + the date and "edited" when `version > 1`. Edit (inline) and Delete (in-page confirm, never + `window.confirm`) are offered only on the caller's own notes. "Add a note" is an inline textarea + with a counter. +- A failed save keeps the draft with the error. An edit conflict shows the current text beside the + draft and lets the member choose. +- The page shows one offline notice for all its listeners, which closes the Plan 2b parked double-notice item. + The parked Plan 2b wording items (resume-flow text, the form's shared outcome block) are fixed here. + +**Account switch.** `signOut` = Firebase sign-out, then `window.location.replace("/")`: a full +reload discards every listener, the Firestore memory cache and all React state. The +service-worker precache holds only the app shell. + +**Change password (Settings).** Current password, new password, confirmation. +`reauthenticateWithCredential` then `updatePassword`. The minimum is 8 characters (stricter than +Firebase's 6). The messages map the wrong current password (`auth/invalid-credential`, +`auth/wrong-password`), `auth/too-many-requests`, `auth/network-request-failed` / offline, and a +mismatched confirmation. Success: "Password changed", and the member stays signed in. No email reset +(needs templates and a trusted domain). A forgotten password is reset via the Admin API. + +#### Tests + +- **Unit:** repository collection/notes functions, including the version check re-run on + transaction retry and base-version-0 create; the records×collection join and filter; visit-date + validation; `LIMITS.note` parity with the rules; Saved page filters and empty states; restaurant + page status block, notes list (author-only controls, edited marker), note draft preserved on failure, + edit-conflict chooser; single offline notice; change-password states; sign-out reload. +- **Rules (`functions/test`, emulator):** collection create/update/version, the `visitedOn` + conditions, `updatedBy`/`updatedByName` spoofing, parent missing or `deleting`, delete only while + `deleting`; notes create/update author-only, immutable fields, `authorName` spoofing, 2000-character + limit, delete by non-author refused unless the parent is `deleting`; non-member refused throughout. +- **Browser (`web/e2e`, two members):** a shortlist change seen live by the other member; filter; + mark visited, change date, clear; notes by both members with author-only controls; deleting a + restaurant sweeps both members' notes and the collection document, including an interrupted + deletion resumed from the Saved page; simultaneous toggle conflict; change password, then sign in with the + new one; sign out as a member and sign in as the non-member with no restaurant name rendered at any point. +- **Gate:** typecheck; web unit; functions + rules; browser (retries 0); boot-guard, preview and + upgrade; guardrail greps. + +#### Decisions taken without owner input (override if wrong) + +| Decision | Reason | +|----------|--------| +| Note limit 2,000 characters; password minimum 8 | Room for a visit account; stricter than Firebase's floor | +| Filter choice not persisted | No new browser storage before Plan 5's offline design | +| No quick toggles in the list | Avoids accidental taps while scrolling | +| Sign-out does a full reload | The only reset that provably clears the Firestore memory cache and every listener | + +#### Deploy note + +The new rules must deploy with the new hosting build. The old client never touches these paths, +so deploying rules first is safe. + +#### Not in this plan + +Named trip lists; per-member visits or a visit log; email password reset; offline download, +export and account deletion (Plan 5, which must include `collection` documents and notes, and +delete the caller's notes on account deletion); list-level quick actions. From 6ad9c231594f799aee56d73421f0f8ee62b799f7 Mon Sep 17 00:00:00 2001 From: Bogdan0708 Date: Thu, 24 Sep 2026 17:19:56 +0100 Subject: [PATCH 04/24] =?UTF-8?q?docs(spec):=20amend=20=C2=A73.7=20after?= =?UTF-8?q?=20the=20Plan=204=20design=20review=20(F1=E2=80=93F3)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit F1: rules-enforced deletion completion gate (cleanupDone + collection exists check) so no client version can orphan notes or collection state; read-before-delete sweeps that converge under concurrency. F2: every tab that observed a signed-in UID resets on sign-out or UID change, driven by the auth listener rather than the sign-out button. F3: password-policy rejection, fallback error, success only after updatePassword. Also joined read states, note confirmation identity, conflict chooser semantics, and the corresponding acceptance tests. Archive the design review. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../audits/2026-09-24-plan-4-design-review.md | 64 +++++++++ .../specs/2026-09-20-safebite-pwa-design.md | 136 +++++++++++++++--- 2 files changed, 178 insertions(+), 22 deletions(-) create mode 100644 planning/audits/2026-09-24-plan-4-design-review.md diff --git a/planning/audits/2026-09-24-plan-4-design-review.md b/planning/audits/2026-09-24-plan-4-design-review.md new file mode 100644 index 0000000..c6dffd5 --- /dev/null +++ b/planning/audits/2026-09-24-plan-4-design-review.md @@ -0,0 +1,64 @@ +# Plan 4 design review — §3.7 + +Reviewed commit: `a149918116df8358abfe67289d149cd6dd3e89d6` on `worktree-pwa-04-collection`. +Reviewed worktree: `/home/godja/Dev/AvaGF/.claude/worktrees/pwa-04-collection`. + +## Verdict + +**Changes requested before the implementation plan: two P2 design findings and one P3 correction.** The shortlist/visited/notes model is coherent, but the rollout/deletion and account-switch contracts need amendment. This is a source-and-design review, not a claim that unimplemented Plan 4 behavior has been runtime-tested. + +Compared §3.7 and the §2.3/§3.3 changes with the existing rules, records repository, authentication, page lifecycle and browser helpers. Independent review passes covered database/deletion and authentication/password behavior. Current official Firebase documentation was checked for the relevant platform guarantees. No production configuration, secrets or deployed data were inspected or changed. + +## F1 — P2: old clients can bypass the expanded deletion sequence + +Spec locations: lines 949–953 and 1028–1029; collection deletion condition at 923–925. Existing code: `web/src/records/repository.ts:232–252`, `web/src/records/RestaurantsPage.tsx:34–41`, `firestore.rules:128–129`. + +The deploy note says the old client never touches the new paths, so rules-first deployment is safe. However, it still deletes their parent restaurant. The existing client finishes deletion by sweeping claims and deleting the restaurant; the current final-delete rule requires only membership and `deleting == true`. It also automatically resumes marked restaurants when the Saved page mounts. §3.7 extends the new client sequence but supplies no revised final-delete/old-client contract. + +Concrete mixed-version scenario: + +1. A Plan 4 client creates restaurant notes and `collection/{rid}`. +2. An old open client deletes that restaurant, or resumes a deletion started by the new client. +3. It sweeps claims and removes the restaurant without sweeping notes or collection state. +4. The restaurant vanishes from the list, removing the normal resume entry. The collection document cannot satisfy the proposed delete rule anymore because its parent is absent; another member's orphaned notes likewise cannot be swept through the proposed parent-deleting exception. + +Firestore does not cascade document deletion to subcollections. [Firebase deletion documentation](https://firebase.google.com/docs/firestore/manage-data/delete-data#delete_documents). The separate collection document also survives independently. This is a concrete consequence of the proposed compatibility contract, not a reproduced production incident. + +**Required amendment:** define how incomplete cleanup prevents final parent deletion across bundle versions: a completion gate in the protocol/rules that old finishers cannot skip, server-owned cleanup, or an explicitly enforced cutover that prevents old clients from deleting once Plan 4 data exists. A version marker added only at the initial mark is insufficient if the old resumer can still finish an already-marked document. Merely deploying hosting does not replace every open tab. Do not describe the rollout as unconditionally safe because old clients ignore the new paths. + +**Acceptance:** run the old deletion/resume path against the new rules and seeded notes/collection state, including an old resumer racing a new deleter. Either refuse final deletion while preserving a resumable parent or complete all cleanup. Also cover two new-client sweepers and retries after each intermediate step; missing notes/state/parent must not turn successful concurrent cleanup into a misleading permission failure. + +If the first-ever deployment will provably contain only Plan 4 clients, that can be documented as a limited rollout precondition instead; it is not a general compatibility guarantee for cached bundles or rollback. + +## F2 — P2: the sign-out reset covers only the initiating tab + +Spec locations: lines 987–989 and 1024. Existing code: `web/src/firebase.ts:23–25`, `web/src/auth/AuthProvider.tsx:67–85`. + +Firebase's default browser auth persistence synchronizes auth state between same-origin tabs. [Firebase persistence documentation](https://firebase.google.com/docs/auth/web/auth-state-persistence#expected_behavior_across_browser_tabs). + +With household data loaded in tabs A and B, the proposed `signOut()` wrapper reloads A. B receives an auth-state callback and hides/unmounts its member shell, but does not execute A's wrapper. Its module-scoped Firestore instance and memory cache remain. The design therefore does not meet its cache-clearing promise in all open documents. This finding establishes incomplete clearing, **not demonstrated unauthorized rendering**. + +**Required amendment:** define a reset in every document that observes a previously authenticated UID become null or a different UID. Coordinate that with the explicit sign-out action; initial signed-out startup and same-UID token/reauthentication events must not cause reload loops. Hiding the old UI and invalidating pending callbacks remain necessary while reset occurs. A full reload is a reasonable implementation, but is not the only possible reset mechanism. + +**Acceptance:** two pages in one browser context, both with loaded records/notes; sign out in one, assert reset and removal of previous-account state in both, then sign in as the non-member without test-driven navigation. The existing auth helper calls `page.goto('/')`, and the account-switch test also calls `page.reload()` (`web/e2e/auth.spec.ts:6,59`); using those between identities would clear memory independently and conceal a broken application reset. Separate contexts used for two household members do not exercise shared auth persistence. + +## F3 — P3: password-update rejection is missing from the error contract + +Spec location: lines 991–996. + +Eight characters is a reasonable client minimum for this design, but Firebase's server policy can impose different length or composition requirements; six is a default, not a universal policy. [Firebase password-policy documentation](https://firebase.google.com/docs/auth/web/password-auth#recommended_set_a_password_policy). No live project-policy check was performed, so this is an incomplete contract, not an observed staging failure. + +Keep the proposed reauthentication followed by `updatePassword`, but add password-policy rejection (including `auth/weak-password`) and an unknown-error fallback. Reauthentication failure must not invoke the update. Successful reauthentication followed by a rejected update must show an actionable error, retain usable controls and never report success. Treat eight characters as client validation, or separately make it an explicit owner-controlled server-policy requirement. + +## Decisions and implementation-plan clarifications + +- **`updatedByName`: accept.** Comparing it with the caller's own admin-managed user document on every state write is consistent with the existing evidence-author design. It records the name at the time of the last change; it does not require peer-user reads. +- **2,000-character notes, transient filter choice and no list toggles: accept.** These are bounded choices consistent with the pilot scope. The notes limit still needs actual boundary rules tests, not just a literal-parity check. +- **Reload on sign-out: acceptable once F2 covers all documents.** Replace the claim that it is the only provable reset with the narrower guarantee the implementation will test. +- **Joined read states:** specify behavior while either restaurant/collection listener is loading, denied or errored. An unknown collection snapshot must not be interpreted as an empty shortlist or base-version-0 record. Confirm absence from a server-backed snapshot before allowing the missing-state default; authoritative empty messages require the necessary ready snapshots, as §3.5 already requires. Keep one offline notice without suppressing errors. Add mixed-state tests for list and detail. +- **Deletion/notes concurrency:** in addition to the mixed-version case, test duplicate sweepers, author editing/deleting while a restaurant is marked, and a stale note edit/delete on another device. Explicitly retain claim-ID-style confirmation identity for notes. Define which current text a conflict chooser adopts before the next versioned write. +- **Test isolation:** extend emulator cleanup helpers to remove notes and collection documents; isolate the password-change test's user or restore its password in cleanup. Existing browser suites assume the shared fixture password. Do not manually reload to make account-switch tests pass. + +## Next step + +Amend F1/F2 and the password error contract in §3.7, carry the acceptance cases into the implementation plan, and then proceed with plan review. No implementation changes or emulator/browser gates were run for this design-only review. The report is left uncommitted; the reviewed spec is unchanged. diff --git a/planning/specs/2026-09-20-safebite-pwa-design.md b/planning/specs/2026-09-20-safebite-pwa-design.md index 6c6b3d5..5a486cf 100644 --- a/planning/specs/2026-09-20-safebite-pwa-design.md +++ b/planning/specs/2026-09-20-safebite-pwa-design.md @@ -944,13 +944,44 @@ remove the other member's notes). Neither document carries anything a safety label could be derived from, and neither write touches a claim, so visiting or noting can never refresh a verification date (§2.1). -#### Deletion protocol (extends §3.5) - -Mark `deleting` → sweep claims → sweep notes (same paged, server-read, transactional sweep) → -delete `collection/{rid}` if present → delete the restaurant. Every step is idempotent. The -existing resume path ("Finish deleting" on the Saved page, automatic resume once per mount) runs -the extended sequence. Progress text gains "Removing notes…". The deletion confirmation reads: -"Deletes the restaurant, its evidence, and both members' notes." +#### Deletion protocol (extends §3.5; amended after audit F1) + +Mark `deleting` → sweep claims → sweep notes → delete `collection/{rid}` if present → set +`cleanupDone: true` → delete the restaurant. The existing resume path ("Finish deleting" on the +Saved page, automatic resume once per mount) runs the extended sequence. Progress text gains +"Removing notes…". The deletion confirmation reads: "Deletes the restaurant, its evidence, and +both members' notes." + +**Completion gate (rules).** A Plan 3-era client finishes a deletion by sweeping claims and deleting +the restaurant; Firestore does not cascade to subcollections, so under the old delete rule it +would orphan notes and collection state. The gate makes that impossible for any client version: + +- `restaurants` gains an optional `cleanupDone` (bool; added to `validRestaurant`'s `hasOnly`). + Creation and ordinary updates must not set it (the create rule requires it absent; the ordinary + update branch requires it unchanged). +- A new update branch **marks cleanup done**: `resource.data.deleting == true`, + `request.resource.data.cleanupDone == true`, affected keys only `cleanupDone`, `version`, + `updatedAt`, version `+ 1`, `updatedAt == request.time`. +- Delete requires `deleting == true && cleanupDone == true && + !exists(.../collection/$(rid))`. + +After the `deleting` mark no new claim, note or collection document can be created (their +create rules require a parent with `deleting == false`). The client therefore sets +`cleanupDone` only after its sweeps have returned empty from the server, and the flag cannot go +stale. Rules cannot prove that a subcollection is empty. The gate relies on that ordering, and +the `exists()` check guards the one sibling document it can see. + +An old client's final delete is refused. The restaurant stays listed as "Deleting…", and any +Plan 4 client completes it (automatic resume or "Finish deleting"). The old tab shows its +existing permission message, and its update banner offers the reload. + +**Idempotent, concurrent sweeps.** Each page is read from the server. The transaction then +`tx.get`s every document on the page and deletes only those that still exist. Deleting +`collection/{rid}`, setting `cleanupDone` and deleting the restaurant also read first. An +already-missing document or an already-set flag counts as done, never as a failure. Two +sweepers, a sweeper racing an old-client resumer, and a retry after any intermediate step +therefore all converge without a misleading permission error. (The existing Plan 2b +`sweepClaims`/`removeRestaurant`, which delete blind, change to the same pattern.) #### Client @@ -984,16 +1015,57 @@ either is cached. Deleting rows appear under both filters. No toggles in the lis - The page shows one offline notice for all its listeners, which closes the Plan 2b parked double-notice item. The parked Plan 2b wording items (resume-flow text, the form's shared outcome block) are fixed here. -**Account switch.** `signOut` = Firebase sign-out, then `window.location.replace("/")`: a full -reload discards every listener, the Firestore memory cache and all React state. The +**Account switch (amended after audit F2).** Firebase synchronises auth state across same-origin +tabs, so the reset belongs in the auth listener, not in the sign-out button. `AuthProvider` keeps +`lastUid`, the last signed-in UID observed *in this document*. When `onAuthStateChanged` reports +`null` or a different UID while `lastUid` is set, the provider bumps its generation counter, +immediately renders a neutral "Signing out…" state (old UI hidden, pending callbacks void), and +calls `window.location.replace("/")`. A full reload discards every listener, the Firestore memory +cache and all React state in that tab. Every tab that had a member signed in resets itself, whichever +tab initiated the change. Explicit `signOut()` only calls Firebase sign-out, and the listener +performs the reset in the initiating tab too. No loops: a document that starts signed out has no +`lastUid`, and after the reload it starts fresh. Token refreshes do not fire +`onAuthStateChanged`, and reauthentication keeps the same UID, so neither triggers a reset. The service-worker precache holds only the app shell. -**Change password (Settings).** Current password, new password, confirmation. -`reauthenticateWithCredential` then `updatePassword`. The minimum is 8 characters (stricter than -Firebase's 6). The messages map the wrong current password (`auth/invalid-credential`, -`auth/wrong-password`), `auth/too-many-requests`, `auth/network-request-failed` / offline, and a -mismatched confirmation. Success: "Password changed", and the member stays signed in. No email reset -(needs templates and a trusted domain). A forgotten password is reset via the Admin API. +**Change password (Settings; amended after audit F3).** Current password, new password, +confirmation. Client validation: at least 8 characters, and the confirmation matches. This is a client +rule only: the pilot project has no server password policy (verified 2026-09-24), and one could +be added later. Sequence: `reauthenticateWithCredential`. If it fails, stop and **never** call +`updatePassword`. Otherwise call `updatePassword`. Messages: + +| Condition | Message | +|---|---| +| wrong current password (`auth/invalid-credential`, `auth/wrong-password`) | "That isn't your current password." | +| `auth/too-many-requests` | "Too many attempts. Wait a few minutes and try again." | +| `auth/network-request-failed` or offline | the standard offline message | +| server policy rejects the new password (`auth/weak-password`, `auth/password-does-not-meet-requirements`) | "Your new password doesn't meet this account's password rules. Choose a different one." | +| `auth/requires-recent-login` | "For security, sign out and back in, then try again." | +| anything else | "Couldn't change your password. Your old password still works." | + +"Password changed" appears only after `updatePassword` resolves. After any failure the form stays +usable and submit is re-enabled. A wrong current password clears only that field. A policy rejection +clears the two new-password fields. Offline and unknown errors keep every field. The member stays +signed in on success (same UID, so no reset). No email reset (it needs templates and a trusted domain). +A forgotten password is reset via the Admin API. + +**Read states for joined data (audit clarification).** The Saved list and the restaurant page each +combine two or more listeners. The combined view is loading until every listener has produced a +snapshot. A `denied` or `error` from any listener is shown (errors are never hidden behind the offline +notice). A missing `collection/{rid}` means "not shortlisted, not visited" (base version 0) only +when the collection snapshot is server-backed (`ready`). While it is loading, errored or only cached, +the status controls are disabled and no "Nothing on the shortlist" empty state is shown. Authoritative +empty messages need `ready` snapshots, as in §3.5. One offline notice covers all listeners that +are `offline`. + +**Notes: confirmation identity and conflicts (audit clarification).** A pending delete confirmation +is bound to the note's id and version, like the claim confirmations in Plan 2b (37cc46b). It resets if +that note changes or disappears. An edit conflict shows the current server text next to the +member's draft. Choosing "Keep mine" writes the draft with the *current* server version as its +base; choosing "Use theirs" discards the draft. Either way the next write carries the version the +chooser displayed, so a third change triggers a fresh conflict. A note edited or deleted from +another device while the restaurant is being deleted gets the ordinary `notFound`/`conflict` +outcomes. #### Tests @@ -1004,13 +1076,29 @@ mismatched confirmation. Success: "Password changed", and the member stays signe edit-conflict chooser; single offline notice; change-password states; sign-out reload. - **Rules (`functions/test`, emulator):** collection create/update/version, the `visitedOn` conditions, `updatedBy`/`updatedByName` spoofing, parent missing or `deleting`, delete only while - `deleting`; notes create/update author-only, immutable fields, `authorName` spoofing, 2000-character - limit, delete by non-author refused unless the parent is `deleting`; non-member refused throughout. + `deleting`; notes create/update author-only, immutable fields, `authorName` spoofing, text at + exactly 2,000 characters accepted and at 2,001 refused, delete by non-author refused unless the parent is `deleting`, + author edit/delete while the parent is `deleting`; completion gate: `cleanupDone` refused on + create and on ordinary updates, allowed only by the mark-done branch, restaurant delete refused + without `cleanupDone` or while `collection/{rid}` exists; non-member refused throughout. +- **Mixed-version deletion (emulator, repository level):** the Plan 3-era sequence (sweep claims, + delete restaurant, blind deletes as in the merged Plan 3 `repository.ts`) run against the new rules with seeded notes and + collection state is refused at the final delete, the parent stays marked and resumable, and the + new `finishDeleting` then completes all cleanup; an old resumer racing a new deleter; two new + sweepers concurrently; a retry after each intermediate step (claims swept, notes swept, + collection removed, `cleanupDone` set) completes without a permission outcome. - **Browser (`web/e2e`, two members):** a shortlist change seen live by the other member; filter; mark visited, change date, clear; notes by both members with author-only controls; deleting a restaurant sweeps both members' notes and the collection document, including an interrupted deletion resumed from the Saved page; simultaneous toggle conflict; change password, then sign in with the - new one; sign out as a member and sign in as the non-member with no restaurant name rendered at any point. + new one (the test uses its own user or restores the fixture password in cleanup); + **cross-tab reset:** two pages in one browser context, both showing records and notes. Sign out in + page A. Both pages reset (a marker set on `window` before the sign-out is gone afterwards) and show + the sign-in form without any test-driven `goto`/`reload`. Then sign in as the non-member in page B, + again without test navigation, and no restaurant name is ever rendered. Unit tests: no reload on + initial signed-out start, on the same UID, or on token refresh. +- **Test isolation:** the emulator clean-up helpers also remove notes and collection documents. + Account-switch tests never rely on manual reloads. - **Gate:** typecheck; web unit; functions + rules; browser (retries 0); boot-guard, preview and upgrade; guardrail greps. @@ -1021,12 +1109,16 @@ mismatched confirmation. Success: "Password changed", and the member stays signe | Note limit 2,000 characters; password minimum 8 | Room for a visit account; stricter than Firebase's floor | | Filter choice not persisted | No new browser storage before Plan 5's offline design | | No quick toggles in the list | Avoids accidental taps while scrolling | -| Sign-out does a full reload | The only reset that provably clears the Firestore memory cache and every listener | +| Account change resets each tab by a full reload | Discards the Firestore memory cache, listeners and React state in one step; the cross-tab browser test is the guarantee | -#### Deploy note +#### Deploy note (amended after audit F1) -The new rules must deploy with the new hosting build. The old client never touches these paths, -so deploying rules first is safe. +Old clients never write the new paths. The completion gate stops them finishing a deletion that +would orphan notes or collection state, so old tabs, cached bundles and a hosting rollback remain +safe after the new rules deploy. Deploy order: rules and functions first, then hosting. Staging has +never served hosting (verified 2026-09-24: live channel with no releases, site returns 404), so +the first deploy has no older clients in the wild. The gate is not relied on for that; it covers +rollbacks and later releases. #### Not in this plan From 0dafaac043a8e4ceb3271b0599ee217c47f21915 Mon Sep 17 00:00:00 2001 From: Bogdan0708 Date: Thu, 24 Sep 2026 17:47:30 +0100 Subject: [PATCH 05/24] =?UTF-8?q?docs(plan):=20Plan=204=20implementation?= =?UTF-8?q?=20plan=20=E2=80=94=20shortlist,=20visited,=20notes?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ten tasks from spec §3.7 (amended f8edfc9): rules, the deletion completion gate with mixed-version proof, the data layer, Saved page, status block, notes, per-tab reset, change password, and two browser tasks. Records one deliberate deviation (toggle-conflict browser test). Co-Authored-By: Claude Opus 5.5 (1M context) --- .../2026-09-24-safebite-pwa-04-collection.md | 3945 +++++++++++++++++ 1 file changed, 3945 insertions(+) create mode 100644 planning/plans/2026-09-24-safebite-pwa-04-collection.md diff --git a/planning/plans/2026-09-24-safebite-pwa-04-collection.md b/planning/plans/2026-09-24-safebite-pwa-04-collection.md new file mode 100644 index 0000000..51d1d4a --- /dev/null +++ b/planning/plans/2026-09-24-safebite-pwa-04-collection.md @@ -0,0 +1,3945 @@ +# SafeBite PWA — Plan 4: Shortlist, visited state and notes + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Add the household shortlist and visited state (`households/{hid}/collection/{rid}`), authored notes on restaurant records (`restaurants/{rid}/notes/{nid}`), a rules-enforced deletion completion gate that no client version can bypass, per-tab reset on any account change, and a change-password form. Everything is proven by rules, web unit and browser tests. + +**Architecture:** Same layout as Plans 1–3 (`web/`, `functions/`, root scripts, `firestore.rules`). Rules gain a top-level `callerName()`, a `collection/{rid}` match, a `notes/{nid}` match under restaurants and a `cleanupDone` completion gate on restaurant deletion. `web/src/records/repository.ts` remains the records module and owns the deletion protocol (now read-before-delete). It also exports its transaction and listener helpers to two new sibling modules, `collection.ts` (shortlist/visited) and `notes.ts`. Pure helpers (`combine.ts` for joined read states, `join.ts` for records × state) keep the pages thin. The restaurant page gains `StatusBlock.tsx` and `NotesSection.tsx`. `AuthProvider` resets its document through `resetDocument()` when a previously seen UID signs out or changes. `ChangePasswordForm.tsx` sits in Settings, backed by `auth/changePassword.ts`. + +**Tech Stack:** Vite 8.3, React 19, react-router 8, TypeScript 6 strict (web) / 5.9 (functions), Vitest 5, Playwright 1.63 (Chromium only), Firebase JS SDK 12, `@firebase/rules-unit-testing` 5, firebase-tools 15.30, Node 22. + +**Spec:** `planning/specs/2026-09-20-safebite-pwa-design.md`. **§3.7 (as amended at f8edfc9 after `planning/audits/2026-09-24-plan-4-design-review.md`) is the binding design for this plan.** It covers rulings 1–5, the data model, the deletion protocol and completion gate, the client, account switch, change password, read states, note confirmation and conflicts, tests, decisions, the deploy note and exclusions. Also binding: §2.1 non-negotiable rules (a note never grants a label or refreshes a checked date), §2.4 security model, §3.5 (read states, online-only transactional writes, deletion protocol this plan extends) and §3.2 guardrails. Previous plans: `planning/plans/2026-09-21-safebite-pwa-02b-records.md` (repository, pages, e2e helpers) and `planning/plans/2026-09-22-safebite-pwa-03-discovery.md`. + +## Global Constraints + +- Emulators only (`demo-safebite`). No `firebase deploy`, no `git push`, no billing or console changes, no access to the staging project `safebite-pilot-urfs3v`. The string `safebite-production-13ba1` must not appear in new files. +- Working directory is the worktree `/home/godja/Dev/AvaGF/.claude/worktrees/pwa-04-collection` on branch `worktree-pwa-04-collection`. Never run anything in `/home/godja/Dev/AvaGF` itself. +- Every Firestore write from `web/` goes through `runTransaction` via the repository's `write()` helper (online pre-check, typed `WriteOutcome`, never throws). No `setDoc`/`updateDoc`/`deleteDoc`/`writeBatch` in `web/src`. Never `window.confirm`/`alert`/`prompt` in `web/src`. +- No numerical safety score. Notes and collection documents carry nothing a safety label could be derived from. No write touches a claim except the existing claim functions. +- `LIMITS.note` = 2000 characters. The client password minimum is 8 characters (client rule only). Visit dates are UTC-midnight timestamps read as calendar days (`dates.ts`), never later than the device's local today on the client, `<= request.time + 1 day` in rules. +- British spelling in UI copy. The testids of existing screens stay unchanged unless a step says otherwise. +- The browser Firestore SDK keeps offline persistence off (memory cache). No new `localStorage`/`sessionStorage`/IndexedDB use in `web/src`. +- Line endings: every file this plan touches is LF. Edit with tools that preserve endings. +- Node 22; TS strict. Commit messages end with `Co-Authored-By: Claude Opus 5.5 (1M context) `. Implementers may see a different attribution reminder; use this line. +- Emulator-backed runs (`npm run emu:test`, `npm run emu:e2e`) take one to three minutes here, so give those shell commands a 10 minute timeout. +- Every task ends with `npm run typecheck` and `npm run test:unit` green. Add `npm run emu:test` when the task touched `functions/` or `firestore.rules`, and `npm run emu:e2e` when it touched `web/e2e`, rules or anything the browser suite exercises. State unit counts as "previous + N new". Do not assert absolute totals in commit messages. Baseline at `f8edfc9`: web unit 270, functions + rules 282, browser 29. + +--- + +## File map + +| Path | Responsibility | +|------|----------------| +| `firestore.rules` | `callerName()`; restaurant `cleanupDone` + `isMarkingCleanupDone()` + delete gate; `collection/{rid}`; `restaurants/{rid}/notes/{nid}` | +| `functions/test/rules.collection.test.ts` (new) | Rules tests for collection state and notes | +| `functions/test/rules.records.test.ts` | Restaurant create/update/delete cases for `cleanupDone` and the gate | +| `functions/test/rules.deletion.test.ts` (new) | Mixed-version and concurrent deletion protocol against the rules | +| `web/src/records/types.ts` | `CollectionState`, `Note`, `NoteInput` | +| `web/src/records/validation.ts` (+test) | `LIMITS.note`; `validateNoteText`; `validateVisitedOn` | +| `web/src/records/rulesParity.test.ts` | `LIMITS.note` appears in the rules | +| `web/src/test/memoryFirestore.ts` (new) | In-memory transaction/page fake shared by repository, collection and notes tests | +| `web/src/records/repository.ts` (+test) | Exports helpers; `notesCol`; read-before-delete `sweep`; `sweepNotes`; `removeCollectionState`; `markCleanupDone`; `finishDeleting` extended; `DeleteStep` gains `sweepingNotes` | +| `web/src/records/collection.ts` (+test, new) | `watchCollection`, `watchCollectionEntry`, `setShortlisted`, `setVisited` | +| `web/src/records/notes.ts` (+test, new) | `watchNotes`, `addNote`, `updateNote`, `deleteNote` | +| `web/src/records/combine.ts` (+test, new) | `isData`, `anyOffline`, `combineStates` | +| `web/src/records/join.ts` (+test, new) | `joinRecords`, `filterRows` | +| `web/src/records/messages.ts` (+test, new test) | `statusOutcomeMessage`, `deleteProgressText`, `finishOutcomeText` | +| `web/src/records/RestaurantsPage.tsx` (+test) | Filter, labels, empty states, joined read state, resume wording | +| `web/src/records/StatusBlock.tsx` (+test, new) | Shortlist and visited controls | +| `web/src/records/NotesSection.tsx` (+test, new) | Notes list, composer, edit with conflict chooser, delete with confirmation identity | +| `web/src/records/RestaurantDetailPage.tsx` (+test) | Wires StatusBlock and NotesSection; one offline notice for four listeners | +| `web/src/records/RestaurantFormPage.tsx` (+test) | Deletion confirmation text; delete outcome uses the delete verb; progress text via `deleteProgressText` | +| `web/src/auth/resetDocument.ts` (new) | `resetDocument()` → `window.location.replace("/")` | +| `web/src/auth/AuthProvider.tsx` (+test) | `lastUid`; `resetting` state; reset on null or a different UID | +| `web/src/App.tsx` | Renders the `resetting` state | +| `web/src/auth/changePassword.ts` (+test, new) | `validateNewPassword`, `changePassword`, `CHANGE_PASSWORD_MESSAGES` | +| `web/src/pages/ChangePasswordForm.tsx` (+test, new) | The Settings form | +| `web/src/pages/SettingsPage.tsx` (+test) | Renders `ChangePasswordForm` | +| `web/src/styles.css` | `.labels`, `.label`, `.filter`, `.note` | +| `web/e2e/emulator-rest.ts` | `clearRecords` also removes notes and collection documents; `seedNote`, `seedCollection`, `listNoteIds`, `collectionExists`, `updateNoteViaRest`, `sweepClaimsViaRest` | +| `web/e2e/auth-rest.ts` (new) | `setPasswordViaAdmin` for the Auth emulator | +| `web/e2e/collection.spec.ts` (new) | Shortlist, visited, notes, deletion and conflict scenarios | +| `web/e2e/records.spec.ts` | Two list assertions select *All records* (Task 4) | +| `web/e2e/auth.spec.ts` | Cross-tab reset and change-password scenarios | +| `web/playwright.config.ts` | `globalTimeout` 1,200 s; comment counts | +| `README.md` | Web section: shortlist/notes, change password, test counts | + +--- + +### Task 1: Rules — collection state and notes + +**Files:** +- Modify: `firestore.rules` +- Create: `functions/test/rules.collection.test.ts` +- Modify: `web/src/records/validation.ts` (only `LIMITS`), `web/src/records/rulesParity.test.ts` + +**Interfaces:** +- Consumes: existing rules functions `signedIn`, `isMember(hid)`, `nonBlankString(v, max)`, `utcMidnight(ts)`. +- Produces: rules accepting exactly the documents later tasks write: + - `households/{hid}/collection/{rid}`: `{ shortlisted: bool, visited: bool, visitedOn?: timestamp, updatedBy: string, updatedByName: string, updatedAt: serverTimestamp, version: int }` + - `households/{hid}/restaurants/{rid}/notes/{nid}`: `{ text: string, authorUid, authorName, createdAt, updatedAt, version }` + - `LIMITS.note = 2000` exported from `web/src/records/validation.ts`. + +- [ ] **Step 1: Write the failing rules tests** + +Create `functions/test/rules.collection.test.ts`: + +```ts +import { readFileSync } from "node:fs"; +import path from "node:path"; +import { afterAll, beforeAll, beforeEach, describe, it } from "vitest"; +import { assertFails, assertSucceeds, initializeTestEnvironment, type RulesTestEnvironment } from "@firebase/rules-unit-testing"; +import { collection, deleteDoc, doc, getDoc, getDocs, serverTimestamp, setDoc, Timestamp, updateDoc } from "firebase/firestore"; + +const PROJECT_ID = "demo-safebite"; +const RULES_PATH = path.resolve(process.cwd(), "..", "firestore.rules"); + +let env: RulesTestEnvironment; + +beforeAll(async () => { + env = await initializeTestEnvironment({ + projectId: PROJECT_ID, + firestore: { rules: readFileSync(RULES_PATH, "utf8"), host: "127.0.0.1", port: 8080 }, + }); +}); + +beforeEach(async () => { + await env.clearFirestore(); + await env.withSecurityRulesDisabled(async (ctx) => { + const db = ctx.firestore(); + await setDoc(doc(db, "households/home"), { name: "Home", memberIds: ["ava", "bogdan"], createdAt: new Date() }); + await setDoc(doc(db, "households/other"), { name: "Other", memberIds: ["stranger"], createdAt: new Date() }); + await setDoc(doc(db, "users/ava"), { householdId: "home", displayName: "Ava" }); + await setDoc(doc(db, "users/bogdan"), { householdId: "home", displayName: "Bogdan" }); + await setDoc(doc(db, "users/stranger"), { householdId: "other", displayName: "Stranger" }); + }); +}); + +afterAll(async () => { + await env.cleanup(); +}); + +const as = (uid: string) => env.authenticatedContext(uid).firestore(); +const R = "households/home/restaurants"; +const S = "households/home/collection"; +const utcDate = (d: string) => Timestamp.fromDate(new Date(`${d}T00:00:00.000Z`)); +const isoDay = (date: Date) => date.toISOString().slice(0, 10); +const daysAhead = (n: number) => isoDay(new Date(Date.now() + n * 86_400_000)); +const NAMES: Record = { ava: "Ava", bogdan: "Bogdan", stranger: "Stranger" }; + +async function seed(docPath: string, data: Record): Promise { + await env.withSecurityRulesDisabled(async (ctx) => { + await setDoc(doc(ctx.firestore(), docPath), data); + }); +} + +async function seedRestaurant(id: string, over: Record = {}): Promise { + await seed(`${R}/${id}`, { + name: "Seeded", + address: "Somewhere 1", + createdBy: "ava", + createdAt: Timestamp.fromDate(new Date("2026-09-01T10:00:00Z")), + updatedAt: Timestamp.fromDate(new Date("2026-09-01T10:00:00Z")), + version: 1, + deleting: false, + ...over, + }); +} + +/** A valid collection-state write as the client sends it (full document, server updatedAt). */ +function stateWrite(uid = "ava", over: Record = {}): Record { + const data: Record = { + shortlisted: true, + visited: false, + updatedBy: uid, + updatedByName: NAMES[uid], + updatedAt: serverTimestamp(), + version: 1, + ...over, + }; + for (const key of Object.keys(data)) if (data[key] === undefined) delete data[key]; + return data; +} + +async function seedState(rid: string, over: Record = {}): Promise { + await seed(`${S}/${rid}`, { shortlisted: true, visited: false, updatedBy: "ava", updatedByName: "Ava", updatedAt: new Date(), version: 1, ...over }); +} + +describe("collection — reads", () => { + it("members read and list; non-members and anonymous do not", async () => { + await seedRestaurant("r1"); + await seedState("r1"); + await assertSucceeds(getDoc(doc(as("ava"), `${S}/r1`))); + await assertSucceeds(getDocs(collection(as("bogdan"), S))); + await assertFails(getDoc(doc(as("stranger"), `${S}/r1`))); + await assertFails(getDocs(collection(as("stranger"), S))); + await assertFails(getDoc(doc(env.unauthenticatedContext().firestore(), `${S}/r1`))); + }); +}); + +describe("collection — create", () => { + beforeEach(async () => { + await seedRestaurant("r1"); + }); + + it.each([ + ["shortlisted only", {}], + ["visited with a date", { shortlisted: false, visited: true, visitedOn: utcDate("2026-05-03") }], + ["visited one day ahead of UTC", { visited: true, visitedOn: utcDate(daysAhead(1)) }], + ["neither flag", { shortlisted: false }], + ])("accepts %s", async (_label, over) => { + await assertSucceeds(setDoc(doc(as("ava"), `${S}/r1`), stateWrite("ava", over))); + }); + + it.each([ + ["version is not 1", { version: 2 }], + ["updatedBy is someone else", { updatedBy: "bogdan" }], + ["updatedByName is not the caller's display name", { updatedByName: "Bogdan" }], + ["client-supplied updatedAt", { updatedAt: new Date() }], + ["visited without visitedOn", { visited: true }], + ["visitedOn without visited", { visitedOn: utcDate("2026-05-03") }], + ["visitedOn not at UTC midnight", { visited: true, visitedOn: Timestamp.fromDate(new Date("2026-05-03T10:00:00Z")) }], + // three, not two: see rules.records.test.ts on request.time near midnight + ["visitedOn three days ahead", { visited: true, visitedOn: utcDate(daysAhead(3)) }], + ["shortlisted is a string", { shortlisted: "yes" }], + ["an unknown key", { rating: 5 }], + ["a legacy savedBy key", { savedBy: "ava" }], + ["missing shortlisted", { shortlisted: undefined }], + ])("rejects a create where %s", async (_label, over) => { + await assertFails(setDoc(doc(as("ava"), `${S}/r1`), stateWrite("ava", over))); + }); + + it("rejects a create for a missing restaurant or one marked deleting", async () => { + await assertFails(setDoc(doc(as("ava"), `${S}/ghost`), stateWrite("ava"))); + await seedRestaurant("r2", { deleting: true }); + await assertFails(setDoc(doc(as("ava"), `${S}/r2`), stateWrite("ava"))); + }); + + it("rejects a create by a non-member or anonymous client", async () => { + await assertFails(setDoc(doc(as("stranger"), `${S}/r1`), stateWrite("stranger"))); + await assertFails(setDoc(doc(env.unauthenticatedContext().firestore(), `${S}/r1`), stateWrite("ava"))); + }); +}); + +describe("collection — update and delete", () => { + beforeEach(async () => { + await seedRestaurant("r1"); + await seedState("r1", { version: 3 }); + }); + + it("either member writes the next version with their own identity", async () => { + await assertSucceeds(setDoc(doc(as("bogdan"), `${S}/r1`), stateWrite("bogdan", { shortlisted: false, visited: true, visitedOn: utcDate("2026-05-03"), version: 4 }))); + }); + + it.each([ + ["the version is stale", { version: 3 }], + ["the version skips ahead", { version: 5 }], + ["updatedByName is spoofed", { version: 4, updatedByName: "Ava" }], + ])("rejects an update where %s", async (_label, over) => { + await assertFails(setDoc(doc(as("bogdan"), `${S}/r1`), stateWrite("bogdan", over))); + }); + + it("rejects updates once the restaurant is marked deleting", async () => { + await seedRestaurant("r1", { deleting: true }); + await assertFails(setDoc(doc(as("ava"), `${S}/r1`), stateWrite("ava", { version: 4 }))); + }); + + it("delete is refused while the restaurant is live and allowed once it is marked deleting", async () => { + await assertFails(deleteDoc(doc(as("ava"), `${S}/r1`))); + await seedRestaurant("r1", { deleting: true }); + await assertFails(deleteDoc(doc(as("stranger"), `${S}/r1`))); + await assertSucceeds(deleteDoc(doc(as("bogdan"), `${S}/r1`))); + }); +}); + +const N = `${R}/r1/notes`; + +/** A valid note create as the client sends it. */ +function noteCreate(uid = "ava", over: Record = {}): Record { + const data: Record = { + text: "Staff knew exactly what coeliac means.", + authorUid: uid, + authorName: NAMES[uid], + createdAt: serverTimestamp(), + updatedAt: serverTimestamp(), + version: 1, + ...over, + }; + for (const key of Object.keys(data)) if (data[key] === undefined) delete data[key]; + return data; +} + +async function seedNote(id: string, uid = "ava", over: Record = {}): Promise { + await seed(`${N}/${id}`, { ...noteCreate(uid), createdAt: new Date("2026-09-01T10:00:00Z"), updatedAt: new Date("2026-09-01T10:00:00Z"), version: 2, ...over }); +} + +describe("notes — reads", () => { + it("members read and list; non-members and anonymous do not", async () => { + await seedRestaurant("r1"); + await seedNote("n1"); + await assertSucceeds(getDoc(doc(as("bogdan"), `${N}/n1`))); + await assertSucceeds(getDocs(collection(as("ava"), N))); + await assertFails(getDoc(doc(as("stranger"), `${N}/n1`))); + await assertFails(getDoc(doc(env.unauthenticatedContext().firestore(), `${N}/n1`))); + }); +}); + +describe("notes — create", () => { + beforeEach(async () => { + await seedRestaurant("r1"); + }); + + it("accepts a note from either member, including exactly 2,000 characters", async () => { + await assertSucceeds(setDoc(doc(as("ava"), `${N}/a`), noteCreate("ava"))); + await assertSucceeds(setDoc(doc(as("bogdan"), `${N}/b`), noteCreate("bogdan", { text: "x".repeat(2000) }))); + }); + + it.each([ + ["text of 2,001 characters", { text: "x".repeat(2001) }], + ["whitespace-only text", { text: " " }], + ["text not a string", { text: 42 }], + ["authorUid is someone else", { authorUid: "bogdan" }], + ["authorName is not the caller's display name", { authorName: "Bogdan" }], + ["client-supplied createdAt", { createdAt: new Date() }], + ["client-supplied updatedAt", { updatedAt: new Date() }], + ["version is not 1", { version: 2 }], + ["an unknown key", { verified: true }], + ["missing text", { text: undefined }], + ])("rejects a create where %s", async (_label, over) => { + await assertFails(setDoc(doc(as("ava"), `${N}/bad`), noteCreate("ava", over))); + }); + + it("rejects a create under a missing parent or a parent marked deleting", async () => { + await assertFails(setDoc(doc(as("ava"), `${R}/ghost/notes/bad`), noteCreate("ava"))); + await seedRestaurant("r2", { deleting: true }); + await assertFails(setDoc(doc(as("ava"), `${R}/r2/notes/bad`), noteCreate("ava"))); + }); + + it("rejects a create by a non-member or anonymous client", async () => { + await assertFails(setDoc(doc(as("stranger"), `${N}/bad`), noteCreate("stranger"))); + await assertFails(setDoc(doc(env.unauthenticatedContext().firestore(), `${N}/bad`), noteCreate("ava"))); + }); +}); + +describe("notes — update", () => { + beforeEach(async () => { + await seedRestaurant("r1"); + await seedNote("n1", "ava"); + }); + + it("the author edits the text with the next version and a server updatedAt", async () => { + await assertSucceeds(updateDoc(doc(as("ava"), `${N}/n1`), { text: "Edited", version: 3, updatedAt: serverTimestamp() })); + }); + + it.each([ + ["the other member edits", "bogdan", { text: "Edited", version: 3, updatedAt: serverTimestamp() }], + ["the version is stale", "ava", { text: "Edited", version: 2, updatedAt: serverTimestamp() }], + ["the version skips ahead", "ava", { text: "Edited", version: 4, updatedAt: serverTimestamp() }], + ["updatedAt is client-supplied", "ava", { text: "Edited", version: 3, updatedAt: new Date() }], + ["authorUid changes", "ava", { authorUid: "bogdan", version: 3, updatedAt: serverTimestamp() }], + ["authorName changes", "ava", { authorName: "Bogdan", version: 3, updatedAt: serverTimestamp() }], + ["createdAt changes", "ava", { createdAt: new Date(), version: 3, updatedAt: serverTimestamp() }], + ["text becomes too long", "ava", { text: "x".repeat(2001), version: 3, updatedAt: serverTimestamp() }], + ])("rejects an update where %s", async (_label, uid, patch) => { + await assertFails(updateDoc(doc(as(uid), `${N}/n1`), patch)); + }); + + it("rejects an author edit while the restaurant is marked deleting", async () => { + await seedRestaurant("r1", { deleting: true }); + await assertFails(updateDoc(doc(as("ava"), `${N}/n1`), { text: "Edited", version: 3, updatedAt: serverTimestamp() })); + }); +}); + +describe("notes — delete", () => { + beforeEach(async () => { + await seedRestaurant("r1"); + await seedNote("n1", "ava"); + }); + + it("the author deletes; the other member and non-members may not while the restaurant is live", async () => { + await assertFails(deleteDoc(doc(as("bogdan"), `${N}/n1`))); + await assertFails(deleteDoc(doc(as("stranger"), `${N}/n1`))); + await assertSucceeds(deleteDoc(doc(as("ava"), `${N}/n1`))); + }); + + it("once the restaurant is marked deleting any member may delete (the sweep), never a non-member", async () => { + await seedRestaurant("r1", { deleting: true }); + await assertFails(deleteDoc(doc(as("stranger"), `${N}/n1`))); + await assertSucceeds(deleteDoc(doc(as("bogdan"), `${N}/n1`))); + }); + + it("the author may still delete while the restaurant is marked deleting", async () => { + await seedRestaurant("r1", { deleting: true }); + await assertSucceeds(deleteDoc(doc(as("ava"), `${N}/n1`))); + }); +}); +``` + +- [ ] **Step 2: Run to verify the new tests fail** + +Run: `npm run emu:test` (10 min timeout) +Expected: the new `rules.collection.test.ts` cases that expect `assertSucceeds` FAIL (default deny); the existing 282 still pass. + +- [ ] **Step 3: Add the rules** + +In `firestore.rules`, directly after the `isMember(hid)` function, add: + +``` + // The caller's own users/{uid} document. Rules get() is not subject to the read rules, so no + // peer-user read is introduced. Used wherever a stored display name must be the writer's own. + function callerName() { + return get(/databases/$(database)/documents/users/$(request.auth.uid)).data.displayName; + } +``` + +Inside `match /restaurants/{rid}`, after the closing brace of `match /claims/{cid}`, add: + +``` + // Authored notes (spec §3.7). Only the author edits; the author deletes at any time, and + // any member may delete once the restaurant is marked deleting (deletion sweep). + match /notes/{nid} { + function noteParent() { + return get(/databases/$(database)/documents/households/$(hid)/restaurants/$(rid)); + } + + allow read: if isMember(hid); + + allow create: if isMember(hid) + && exists(/databases/$(database)/documents/households/$(hid)/restaurants/$(rid)) + && noteParent().data.deleting == false + && request.resource.data.keys().hasOnly(['text', 'authorUid', 'authorName', 'createdAt', 'updatedAt', 'version']) + && request.resource.data.keys().hasAll(['text', 'authorUid', 'authorName', 'createdAt', 'updatedAt', 'version']) + && nonBlankString(request.resource.data.text, 2000) + && request.resource.data.authorUid == request.auth.uid + && request.resource.data.authorName == callerName() + && request.resource.data.createdAt == request.time + && request.resource.data.updatedAt == request.time + && request.resource.data.version == 1; + + allow update: if isMember(hid) + && resource.data.authorUid == request.auth.uid + && noteParent().data.deleting == false + && request.resource.data.diff(resource.data).affectedKeys().hasOnly(['text', 'updatedAt', 'version']) + && nonBlankString(request.resource.data.text, 2000) + && request.resource.data.updatedAt == request.time + && request.resource.data.version == resource.data.version + 1; + + // Author first: `||` short-circuits, so an author can delete even if the parent is gone. + allow delete: if isMember(hid) + && (resource.data.authorUid == request.auth.uid || noteParent().data.deleting == true); + } +``` + +Inside `match /households/{hid}`, after the closing brace of `match /restaurants/{rid}`, add: + +``` + // Shortlist and visited state, one document per restaurant, id = restaurant id (spec §3.7). + match /collection/{rid} { + function stateParentPath() { + return /databases/$(database)/documents/households/$(hid)/restaurants/$(rid); + } + function liveParent() { + return exists(stateParentPath()) && get(stateParentPath()).data.deleting == false; + } + function validState(data) { + return data.keys().hasOnly(['shortlisted', 'visited', 'visitedOn', 'updatedBy', 'updatedByName', 'updatedAt', 'version']) + && data.keys().hasAll(['shortlisted', 'visited', 'updatedBy', 'updatedByName', 'updatedAt', 'version']) + && data.shortlisted is bool + && data.visited is bool + && data.visited == ('visitedOn' in data) + && (!('visitedOn' in data) + || (utcMidnight(data.visitedOn) && data.visitedOn <= request.time + duration.value(1, 'd'))) + && data.updatedBy == request.auth.uid + && data.updatedByName == callerName() + && data.updatedAt == request.time + && data.version is int; + } + + allow read: if isMember(hid); + allow create: if isMember(hid) && liveParent() && validState(request.resource.data) + && request.resource.data.version == 1; + allow update: if isMember(hid) && liveParent() && validState(request.resource.data) + && request.resource.data.version == resource.data.version + 1; + // Deletion sweep only: the restaurant must exist and be marked deleting. + allow delete: if isMember(hid) && exists(stateParentPath()) && get(stateParentPath()).data.deleting == true; + } +``` + +Check the brace nesting: `collection` sits beside `restaurants` inside `households/{hid}`; `notes` sits beside `claims` inside `restaurants/{rid}`. + +- [ ] **Step 4: Add `LIMITS.note` and its parity check** + +In `web/src/records/validation.ts` change the `LIMITS` line to: + +```ts +export const LIMITS = { name: 120, address: 300, phone: 40, website: 300, detail: 1000, sourceLabel: 200, sourceUrl: 500, googlePlaceId: 200, note: 2000 } as const; +``` + +In `web/src/records/rulesParity.test.ts`, at the end of the `"carries the same field limits as validation.ts"` test body, add: + +```ts + expect(rules).toContain(`nonBlankString(request.resource.data.text, ${LIMITS.note})`); +``` + +- [ ] **Step 5: Run everything** + +Run: `npm run emu:test` → all pass (282 + the new file's cases). +Run: `npm run typecheck && npm run test:unit` → pass (270; the parity test gains an assertion, not a test). +Run: `npm run emu:e2e` → 29 pass (rules only grew). + +- [ ] **Step 6: Commit** + +```bash +git add firestore.rules functions/test/rules.collection.test.ts web/src/records/validation.ts web/src/records/rulesParity.test.ts +git commit -m "feat(rules): shortlist/visited state and authored notes + +Co-Authored-By: Claude Opus 5.5 (1M context) " +``` + +--- +### Task 2: Deletion completion gate — rules, read-before-delete protocol, mixed-version proof + +**Files:** +- Modify: `firestore.rules` (restaurant match) +- Modify: `functions/test/rules.records.test.ts` +- Create: `functions/test/rules.deletion.test.ts` +- Create: `web/src/test/memoryFirestore.ts` +- Modify: `web/src/records/repository.ts`, `web/src/records/repository.test.ts` +- Modify: `web/src/records/messages.ts`; create `web/src/records/messages.test.ts` +- Modify: `web/src/records/RestaurantsPage.tsx`, `web/src/records/RestaurantFormPage.tsx` (progress text only) + +**Interfaces:** +- Consumes: Task 1 rules (`collection/{rid}` delete while parent `deleting`; notes delete by any member while parent `deleting`). +- Produces (all in `web/src/records/repository.ts`, used by Tasks 3–6): + - `export type DeleteStep = "marking" | "sweeping" | "sweepingNotes" | "removing"` + - `export class ConflictError extends Error {}`, `export class NotFoundError extends Error {}` + - `export function classify(err: unknown): WriteOutcome` + - `export async function write(run: (tx: Transaction) => Promise): Promise>` + - `export const LISTEN`, `export function listenerFailure(err: FirestoreError): Snapshot`, `export function toDate(value: unknown): Date` + - `export const restaurantRef(hid, rid)`, `export const notesCol(hid, rid)`, `export const collectionRef(hid, rid)` + - `export function sweepNotes(hid, rid): Promise>`, `export function removeCollectionState(hid, rid): Promise`, `export function markCleanupDone(hid, rid): Promise` + - `finishDeleting(hid, rid, onProgress?)` runs `sweeping → sweepingNotes → removing` (claims, notes, state document, `cleanupDone`, restaurant). + - `messages.ts`: `export function deleteProgressText(step: DeleteStep): string` + - `web/src/test/memoryFirestore.ts`: `type Store`, `memoryTransactions(runTransaction, store)`, `memoryPage(store, collectionPath)` + +- [ ] **Step 1: Write the failing rules tests for the gate** + +In `functions/test/rules.records.test.ts`: + +(a) In the `"rejects a create where %s"` table of `describe("restaurants — create")`, add the row: + +```ts + ["cleanupDone is set on create", { cleanupDone: false }], +``` + +(b) Replace the test `"accepts deleting a marked restaurant by either member, never by a non-member"` with: + +```ts + it("accepts deleting a marked, cleaned-up restaurant by either member, never by a non-member", async () => { + const p = await seedRestaurant("r1", { deleting: true, cleanupDone: true }); + await assertFails(deleteDoc(doc(as("stranger"), p))); + await assertSucceeds(deleteDoc(doc(as("bogdan"), p))); + }); +``` + +(c) Append a new describe block at the end of the restaurant section (before `const utcDate = …`): + +```ts +describe("restaurants — deletion completion gate (spec §3.7, audit F1)", () => { + it("rejects adding cleanupDone through an ordinary update or together with the deleting mark", async () => { + const p = await seedRestaurant("r1"); + await assertFails(updateDoc(doc(as("ava"), p), { name: "x", cleanupDone: true, version: 4, updatedAt: serverTimestamp() })); + await assertFails(updateDoc(doc(as("ava"), p), { deleting: true, cleanupDone: true, version: 4, updatedAt: serverTimestamp() })); + }); + + it("accepts marking cleanup done on a restaurant marked deleting (only cleanupDone, version, updatedAt)", async () => { + const p = await seedRestaurant("r1", { deleting: true, version: 4 }); + await assertSucceeds(updateDoc(doc(as("bogdan"), p), { cleanupDone: true, version: 5, updatedAt: serverTimestamp() })); + }); + + it.each([ + ["the restaurant is live", { deleting: false, version: 4 }, { cleanupDone: true, version: 5, updatedAt: serverTimestamp() }], + ["the version is stale", { deleting: true, version: 4 }, { cleanupDone: true, version: 4, updatedAt: serverTimestamp() }], + ["cleanupDone is false", { deleting: true, version: 4 }, { cleanupDone: false, version: 5, updatedAt: serverTimestamp() }], + ["another field changes too", { deleting: true, version: 4 }, { cleanupDone: true, name: "x", version: 5, updatedAt: serverTimestamp() }], + ["updatedAt is client-supplied", { deleting: true, version: 4 }, { cleanupDone: true, version: 5, updatedAt: new Date() }], + ["it is already done", { deleting: true, cleanupDone: true, version: 4 }, { cleanupDone: true, version: 5, updatedAt: serverTimestamp() }], + ])("rejects marking cleanup done when %s", async (_label, seeded, patch) => { + const p = await seedRestaurant("r1", seeded); + await assertFails(updateDoc(doc(as("ava"), p), patch)); + }); + + it("refuses the final delete until cleanupDone is set and the collection document is gone", async () => { + const p = await seedRestaurant("r1", { deleting: true }); + await assertFails(deleteDoc(doc(as("ava"), p))); // a Plan 3-era finisher stops here + await seedRestaurant("r1", { deleting: true, cleanupDone: true }); + await env.withSecurityRulesDisabled(async (ctx) => { + await setDoc(doc(ctx.firestore(), "households/home/collection/r1"), { shortlisted: true, visited: false, updatedBy: "ava", updatedByName: "Ava", updatedAt: new Date(), version: 1 }); + }); + await assertFails(deleteDoc(doc(as("ava"), p))); + await env.withSecurityRulesDisabled(async (ctx) => { + await deleteDoc(doc(ctx.firestore(), "households/home/collection/r1")); + }); + await assertSucceeds(deleteDoc(doc(as("ava"), p))); + }); +}); +``` + +- [ ] **Step 2: Write the failing mixed-version protocol tests** + +Create `functions/test/rules.deletion.test.ts`: + +```ts +import { readFileSync } from "node:fs"; +import path from "node:path"; +import { afterAll, beforeAll, beforeEach, describe, expect, it } from "vitest"; +import { assertFails, initializeTestEnvironment, type RulesTestEnvironment } from "@firebase/rules-unit-testing"; +import { collection, doc, getDoc, getDocs, limit, query, runTransaction, serverTimestamp, setDoc, Timestamp } from "firebase/firestore"; + +/** + * The deletion protocol against the real rules with mixed client versions (spec §3.7, audit F1). + * `oldFinish` is the merged Plan 3 client's finishDeleting (blind claim sweep, blind restaurant + * delete). `newFinish` mirrors web/src/records/repository.ts finishDeleting after Plan 4 (every + * step reads before it deletes). Keep newFinish in step with the repository. + */ +const PROJECT_ID = "demo-safebite"; +const RULES_PATH = path.resolve(process.cwd(), "..", "firestore.rules"); +const R = "households/home/restaurants"; +const S = "households/home/collection"; + +let env: RulesTestEnvironment; + +beforeAll(async () => { + env = await initializeTestEnvironment({ + projectId: PROJECT_ID, + firestore: { rules: readFileSync(RULES_PATH, "utf8"), host: "127.0.0.1", port: 8080 }, + }); +}); + +beforeEach(async () => { + await env.clearFirestore(); + await env.withSecurityRulesDisabled(async (ctx) => { + const db = ctx.firestore(); + await setDoc(doc(db, "households/home"), { name: "Home", memberIds: ["ava", "bogdan"], createdAt: new Date() }); + await setDoc(doc(db, "users/ava"), { householdId: "home", displayName: "Ava" }); + await setDoc(doc(db, "users/bogdan"), { householdId: "home", displayName: "Bogdan" }); + }); +}); + +afterAll(async () => { + await env.cleanup(); +}); + +const as = (uid: string) => env.authenticatedContext(uid).firestore(); +type Db = ReturnType; + +/** A restaurant a member has marked deleting, still holding a claim, both members' notes and state. */ +async function seedDoomed(rid: string): Promise { + await env.withSecurityRulesDisabled(async (ctx) => { + const db = ctx.firestore(); + const at = Timestamp.fromDate(new Date("2026-09-01T10:00:00Z")); + await setDoc(doc(db, `${R}/${rid}`), { name: "Doomed", address: "1 Road", createdBy: "ava", createdAt: at, updatedAt: at, version: 2, deleting: true }); + await setDoc(doc(db, `${R}/${rid}/claims/c1`), { kind: "gfMenu", value: "yes", detail: "", source: { type: "ownVisit", label: "x" }, checkedAt: Timestamp.fromDate(new Date("2026-09-01T00:00:00Z")), authorUid: "ava", authorName: "Ava", createdAt: at }); + await setDoc(doc(db, `${R}/${rid}/notes/n-ava`), { text: "Ava's note", authorUid: "ava", authorName: "Ava", createdAt: at, updatedAt: at, version: 1 }); + await setDoc(doc(db, `${R}/${rid}/notes/n-bogdan`), { text: "Bogdan's note", authorUid: "bogdan", authorName: "Bogdan", createdAt: at, updatedAt: at, version: 1 }); + await setDoc(doc(db, `${S}/${rid}`), { shortlisted: true, visited: false, updatedBy: "ava", updatedByName: "Ava", updatedAt: at, version: 1 }); + }); +} + +interface Remaining { restaurant: boolean; claims: number; notes: number; state: boolean } + +async function remaining(rid: string): Promise { + let out: Remaining = { restaurant: false, claims: 0, notes: 0, state: false }; + await env.withSecurityRulesDisabled(async (ctx) => { + const db = ctx.firestore(); + out = { + restaurant: (await getDoc(doc(db, `${R}/${rid}`))).exists(), + claims: (await getDocs(collection(db, `${R}/${rid}/claims`))).size, + notes: (await getDocs(collection(db, `${R}/${rid}/notes`))).size, + state: (await getDoc(doc(db, `${S}/${rid}`))).exists(), + }; + }); + return out; +} + +const GONE: Remaining = { restaurant: false, claims: 0, notes: 0, state: false }; + +async function oldFinish(db: Db, rid: string): Promise { + const claims = await getDocs(query(collection(db, `${R}/${rid}/claims`), limit(100))); + await runTransaction(db, async (tx) => { + for (const c of claims.docs) tx.delete(c.ref); + }); + await runTransaction(db, async (tx) => { + tx.delete(doc(db, `${R}/${rid}`)); + }); +} + +async function sweepSub(db: Db, rid: string, sub: "claims" | "notes"): Promise { + for (;;) { + const page = await getDocs(query(collection(db, `${R}/${rid}/${sub}`), limit(100))); + if (page.empty) return; + await runTransaction(db, async (tx) => { + const snaps = await Promise.all(page.docs.map((d) => tx.get(d.ref))); + for (const s of snaps) if (s.exists()) tx.delete(s.ref); + }); + } +} + +const NEW_STEPS: Array<(db: Db, rid: string) => Promise> = [ + (db, rid) => sweepSub(db, rid, "claims"), + (db, rid) => sweepSub(db, rid, "notes"), + (db, rid) => + runTransaction(db, async (tx) => { + const s = await tx.get(doc(db, `${S}/${rid}`)); + if (s.exists()) tx.delete(s.ref); + }), + (db, rid) => + runTransaction(db, async (tx) => { + const r = await tx.get(doc(db, `${R}/${rid}`)); + if (!r.exists() || r.get("cleanupDone") === true) return; + tx.update(r.ref, { cleanupDone: true, version: (r.get("version") as number) + 1, updatedAt: serverTimestamp() }); + }), + (db, rid) => + runTransaction(db, async (tx) => { + const r = await tx.get(doc(db, `${R}/${rid}`)); + if (r.exists()) tx.delete(r.ref); + }), +]; + +async function newFinish(db: Db, rid: string, stepsToRun = NEW_STEPS.length): Promise { + for (const step of NEW_STEPS.slice(0, stepsToRun)) await step(db, rid); +} + +describe("deletion protocol with mixed client versions", () => { + it("a Plan 3-era finisher is refused at the final delete and leaves a resumable parent; a Plan 4 finisher completes it", async () => { + await seedDoomed("r1"); + await assertFails(oldFinish(as("bogdan"), "r1")); + expect(await remaining("r1")).toEqual({ restaurant: true, claims: 0, notes: 2, state: true }); + await newFinish(as("ava"), "r1"); + expect(await remaining("r1")).toEqual(GONE); + }); + + it("an old resumer racing a new deleter never leaves notes or state without their restaurant", async () => { + await seedDoomed("r1"); + await Promise.allSettled([oldFinish(as("bogdan"), "r1"), newFinish(as("ava"), "r1")]); + const after = await remaining("r1"); + if (!after.restaurant) expect(after).toEqual(GONE); + await newFinish(as("ava"), "r1"); + expect(await remaining("r1")).toEqual(GONE); + }); + + it("two Plan 4 finishers at once both succeed", async () => { + await seedDoomed("r1"); + await Promise.all([newFinish(as("ava"), "r1"), newFinish(as("bogdan"), "r1")]); + expect(await remaining("r1")).toEqual(GONE); + }); + + it.each([1, 2, 3, 4])("a retry after %i completed step(s) finishes without a permission failure", async (done) => { + await seedDoomed("r1"); + await newFinish(as("ava"), "r1", done); + await newFinish(as("bogdan"), "r1"); + expect(await remaining("r1")).toEqual(GONE); + }); + + it("a finisher running after everything is already gone is a no-op, not a failure", async () => { + await seedDoomed("r1"); + await newFinish(as("ava"), "r1"); + await newFinish(as("bogdan"), "r1"); + expect(await remaining("r1")).toEqual(GONE); + }); +}); +``` + +- [ ] **Step 3: Run to verify they fail** + +Run: `npm run emu:test` +Expected: FAIL. The existing delete test now seeds `cleanupDone` (an unknown key for `validRestaurant`) but the old delete rule ignores it; the mark-done cases fail (no branch). The mixed-version "refused" case FAILS because the old rule lets `oldFinish` delete the restaurant. + +- [ ] **Step 4: Implement the gate in the rules** + +In `firestore.rules`: + +(a) In `validRestaurant(data)`, add `'cleanupDone'` to the `hasOnly` list and append a type check, so the function reads: + +``` + function validRestaurant(data) { + return data.keys().hasOnly(['name', 'address', 'phone', 'website', 'lat', 'lng', 'googlePlaceId', 'createdBy', 'createdAt', 'updatedAt', 'version', 'deleting', 'cleanupDone']) + && data.keys().hasAll(['name', 'address', 'createdBy', 'createdAt', 'updatedAt', 'version', 'deleting']) + && nonBlankString(data.name, 120) + && nonBlankString(data.address, 300) + && optionalString(data, 'phone', 40) + && optionalHttpUrl(data, 'website', 300) + && optionalString(data, 'googlePlaceId', 200) + && validCoordinates(data) + && data.createdBy is string + && data.createdAt is timestamp + && data.updatedAt is timestamp + && data.version is int + && data.deleting is bool + && (!('cleanupDone' in data) || data.cleanupDone is bool); + } +``` + +(b) After `isMarkingDeleting()`, add: + +``` + // Deletion protocol, completion gate (spec §3.7, audit F1): set only after the client's claim + // and note sweeps returned empty from the server and the collection document is gone. Nothing + // can be created under a marked restaurant, so the flag cannot go stale. + function isMarkingCleanupDone() { + return resource.data.deleting == true + && resource.data.get('cleanupDone', false) == false + && request.resource.data.get('cleanupDone', false) == true + && request.resource.data.diff(resource.data).affectedKeys().hasOnly(['cleanupDone', 'version', 'updatedAt']); + } +``` + +(c) In `match /restaurants/{rid}`, replace the `allow create`, `allow update` and `allow delete` rules with: + +``` + allow create: if isMember(hid) + && validRestaurant(request.resource.data) + && !('cleanupDone' in request.resource.data) + && request.resource.data.createdBy == request.auth.uid + && request.resource.data.version == 1 + && request.resource.data.deleting == false + && request.resource.data.createdAt == request.time + && request.resource.data.updatedAt == request.time; + + // Optimistic concurrency: exactly the next version, server-stamped. Once deleting is true + // the only permitted change is marking cleanup done (so a claim sweep cannot be undercut). + allow update: if isMember(hid) + && validRestaurant(request.resource.data) + && request.resource.data.createdBy == resource.data.createdBy + && request.resource.data.createdAt == resource.data.createdAt + && request.resource.data.updatedAt == request.time + && request.resource.data.version == resource.data.version + 1 + && ((resource.data.deleting == false + && !('cleanupDone' in request.resource.data) + && (request.resource.data.deleting == false || isMarkingDeleting())) + || isMarkingCleanupDone()); + + // Final step: only a marked restaurant whose cleanup is done and whose collection + // document is gone. A client that skips the note/state sweep cannot pass this. + allow delete: if isMember(hid) + && resource.data.deleting == true + && resource.data.get('cleanupDone', false) == true + && !exists(/databases/$(database)/documents/households/$(hid)/collection/$(rid)); +``` + +- [ ] **Step 5: Run the rules tests** + +Run: `npm run emu:test` +Expected: PASS. All earlier tests pass, plus the new gate cases and the mixed-version file (8 cases). + +- [ ] **Step 6: Add the shared in-memory Firestore fake** + +Create `web/src/test/memoryFirestore.ts`: + +```ts +import { vi, type Mock } from "vitest"; + +/** Document path → data. Paths look like "households/home/restaurants/r1". */ +export type Store = Map>; + +interface Ref { + id: string; + path: string; +} + +function snapshot(store: Store, ref: Ref) { + const data = store.get(ref.path); + return { id: ref.id, ref, exists: () => data !== undefined, data: () => (data === undefined ? undefined : { ...data }) }; +} + +/** + * Wires a mocked `runTransaction` to an in-memory store, so a sequence of repository calls sees + * its own earlier writes. Test-only; paths come from the `doc`/`collection` mocks each test file + * installs (they join segments with "/"). + */ +export function memoryTransactions(runTransaction: Mock, store: Store) { + const tx = { + get: vi.fn(async (ref: Ref) => snapshot(store, ref)), + set: vi.fn((ref: Ref, data: Record) => { + store.set(ref.path, { ...data }); + }), + update: vi.fn((ref: Ref, patch: Record) => { + store.set(ref.path, { ...store.get(ref.path), ...patch }); + }), + delete: vi.fn((ref: Ref) => { + store.delete(ref.path); + }), + }; + runTransaction.mockImplementation(async (_db: unknown, run: (t: typeof tx) => Promise) => run(tx)); + return tx; +} + +/** The documents directly under `collectionPath`, shaped like a getDocsFromServer page. */ +export function memoryPage(store: Store, collectionPath: string, pageSize = 100) { + const prefix = `${collectionPath}/`; + const docs = [...store.keys()] + .filter((p) => p.startsWith(prefix) && !p.slice(prefix.length).includes("/")) + .slice(0, pageSize) + .map((p) => { + const id = p.slice(prefix.length); + return { id, ref: { id, path: p } }; + }); + return { empty: docs.length === 0, size: docs.length, docs }; +} +``` + +- [ ] **Step 7: Write the failing repository tests** + +In `web/src/records/repository.test.ts`: + +(a) Add to the imports from `"./repository"`: `finishDeleting`, `markCleanupDone`, `removeCollectionState`, `sweepNotes`. Add `import { memoryPage, memoryTransactions, type Store } from "../test/memoryFirestore";`. + +(b) Delete the test `"deleteRestaurant runs mark → sweep → remove and reports progress; stops at the first non-ok outcome"` and add, inside `describe("deletion protocol")`: + +```ts + const RP = "households/home/restaurants/r1"; + + function doomedStore(over: Record = {}): Store { + return new Map>([ + [RP, { ...storedRestaurant, deleting: true, version: 4, ...over }], + [`${RP}/claims/c1`, { kind: "gfMenu" }], + [`${RP}/claims/c2`, { kind: "separateFryer" }], + [`${RP}/notes/n1`, { text: "Ava's", authorUid: "ava-uid" }], + [`${RP}/notes/n2`, { text: "Bogdan's", authorUid: "bogdan-uid" }], + ["households/home/collection/r1", { shortlisted: true, visited: false, version: 2 }], + ]); + } + + it("finishDeleting sweeps claims and notes, removes the state document, marks cleanupDone, then removes the restaurant", async () => { + const store = doomedStore(); + const tx = memoryTransactions(m.runTransaction, store); + m.getDocsFromServer.mockImplementation(async (q: { path: string }) => memoryPage(store, q.path)); + const steps: string[] = []; + expect(await finishDeleting("home", "r1", (s) => steps.push(s))).toEqual({ kind: "ok", value: undefined }); + expect(steps).toEqual(["sweeping", "sweepingNotes", "removing"]); + expect(store.size).toBe(0); + expect(tx.update).toHaveBeenCalledWith({ id: "r1", path: RP }, { cleanupDone: true, version: 5, updatedAt: serverTimestamp() }); + const updateOrder = tx.update.mock.invocationCallOrder[0]!; + const restaurantDelete = tx.delete.mock.calls.findIndex(([ref]) => (ref as { path: string }).path === RP); + expect(tx.delete.mock.invocationCallOrder[restaurantDelete]!).toBeGreaterThan(updateOrder); + }); + + it("sweeps skip documents another sweeper already removed instead of failing", async () => { + const store = doomedStore(); + const tx = memoryTransactions(m.runTransaction, store); + store.delete(`${RP}/notes/n2`); // removed between the page read and this transaction + m.getDocsFromServer + .mockResolvedValueOnce({ empty: false, size: 2, docs: [{ id: "n1", ref: { id: "n1", path: `${RP}/notes/n1` } }, { id: "n2", ref: { id: "n2", path: `${RP}/notes/n2` } }] }) + .mockResolvedValueOnce({ empty: true, size: 0, docs: [] }); + expect(await sweepNotes("home", "r1")).toEqual({ kind: "ok", value: 1 }); + expect(tx.delete).toHaveBeenCalledTimes(1); + }); + + it("finishing an already-removed restaurant is a no-op, not a failure", async () => { + const store: Store = new Map(); + const tx = memoryTransactions(m.runTransaction, store); + m.getDocsFromServer.mockImplementation(async (q: { path: string }) => memoryPage(store, q.path)); + expect(await finishDeleting("home", "r1")).toEqual({ kind: "ok", value: undefined }); + expect(tx.update).not.toHaveBeenCalled(); + expect(tx.delete).not.toHaveBeenCalled(); + }); + + it("markCleanupDone refuses a live restaurant and does nothing when already set", async () => { + memoryTransactions(m.runTransaction, new Map([[RP, { ...storedRestaurant }]])); + expect(await markCleanupDone("home", "r1")).toEqual({ kind: "notFound" }); + const tx = memoryTransactions(m.runTransaction, new Map([[RP, { ...storedRestaurant, deleting: true, cleanupDone: true }]])); + expect(await markCleanupDone("home", "r1")).toEqual({ kind: "ok", value: undefined }); + expect(tx.update).not.toHaveBeenCalled(); + }); + + it("removeCollectionState deletes only a document that exists", async () => { + const tx = memoryTransactions(m.runTransaction, new Map()); + expect(await removeCollectionState("home", "r1")).toEqual({ kind: "ok", value: undefined }); + expect(tx.delete).not.toHaveBeenCalled(); + }); + + it("deleteRestaurant runs mark → sweeps → remove and reports every step; stops at the first non-ok outcome", async () => { + const store = doomedStore({ deleting: false, version: 3 }); + memoryTransactions(m.runTransaction, store); + m.getDocsFromServer.mockImplementation(async (q: { path: string }) => memoryPage(store, q.path)); + const steps: string[] = []; + expect(await deleteRestaurant("home", "r1", 3, (s) => steps.push(s))).toEqual({ kind: "ok", value: undefined }); + expect(steps).toEqual(["marking", "sweeping", "sweepingNotes", "removing"]); + expect(store.size).toBe(0); + + const stopped: string[] = []; + fakeTx({ exists: true, data: { ...storedRestaurant, version: 9 } }); + expect(await deleteRestaurant("home", "r1", 3, (s) => stopped.push(s))).toEqual({ kind: "conflict" }); + expect(stopped).toEqual(["marking"]); + }); +``` + +Create `web/src/records/messages.test.ts`: + +```ts +import { describe, expect, it } from "vitest"; +import { deleteProgressText } from "./messages"; + +describe("deleteProgressText", () => { + it("names every deletion step", () => { + expect(deleteProgressText("marking")).toBe("Marking…"); + expect(deleteProgressText("sweeping")).toBe("Removing evidence…"); + expect(deleteProgressText("sweepingNotes")).toBe("Removing notes…"); + expect(deleteProgressText("removing")).toBe("Removing restaurant…"); + }); +}); +``` + +- [ ] **Step 8: Run to verify they fail** + +Run: `npm --prefix web test -- repository messages` +Expected: FAIL (`finishDeleting` does not sweep notes; `sweepNotes`, `markCleanupDone`, `removeCollectionState`, `deleteProgressText` missing). + +- [ ] **Step 9: Implement the repository changes** + +In `web/src/records/repository.ts`: + +(a) Add `type CollectionReference` to the `firebase/firestore` type imports. + +(b) Replace the lines from `export type DeleteStep …` through `const claimsCol = …` with: + +```ts +export type DeleteStep = "marking" | "sweeping" | "sweepingNotes" | "removing"; + +/** Documents deleted per transaction during a sweep (well under Firestore's per-transaction limit). */ +export const SWEEP_PAGE = 100; + +// The helpers below are exported for the sibling record modules (collection.ts, notes.ts) only. +export class ConflictError extends Error {} +export class NotFoundError extends Error {} + +const restaurantsCol = (hid: string) => collection(db, "households", hid, "restaurants"); +export const restaurantRef = (hid: string, rid: string) => doc(db, "households", hid, "restaurants", rid); +const claimsCol = (hid: string, rid: string) => collection(db, "households", hid, "restaurants", rid, "claims"); +export const notesCol = (hid: string, rid: string) => collection(db, "households", hid, "restaurants", rid, "notes"); +export const collectionRef = (hid: string, rid: string) => doc(db, "households", hid, "collection", rid); +``` + +(c) Add `export` to `function toDate`, `function listenerFailure`, `const LISTEN`, `function classify` and `async function write`. Their bodies are unchanged. + +(d) Replace `sweepClaims`, `removeRestaurant` and `finishDeleting` (everything from the `/** Deletion step 2 …` comment up to, but not including, the `/** The whole protocol …` comment) with: + +```ts +/** + * Deletion steps 2–3 (spec §3.7): server-read pages; each page's documents are re-read inside one + * transaction and only those still present are deleted, so two finishers and retries converge + * instead of one of them failing on an already-deleted document. + */ +async function sweep(col: CollectionReference): Promise> { + let deleted = 0; + for (;;) { + let page; + try { + page = await getDocsFromServer(query(col, limit(SWEEP_PAGE))); + } catch (err) { + return classify(err); + } + if (page.empty) return { kind: "ok", value: deleted }; + const refs = page.docs.map((d) => d.ref); + const outcome = await write(async (tx) => { + const snaps = await Promise.all(refs.map((ref) => tx.get(ref))); + let removed = 0; + for (const snap of snaps) { + if (snap.exists()) { + tx.delete(snap.ref); + removed += 1; + } + } + return removed; + }); + if (outcome.kind !== "ok") return outcome; + deleted += outcome.value; + } +} + +export function sweepClaims(hid: string, rid: string): Promise> { + return sweep(claimsCol(hid, rid)); +} + +export function sweepNotes(hid: string, rid: string): Promise> { + return sweep(notesCol(hid, rid)); +} + +/** Deletion step 4: the shortlist/visited document, if any. */ +export function removeCollectionState(hid: string, rid: string): Promise { + return write(async (tx) => { + const snap = await tx.get(collectionRef(hid, rid)); + if (snap.exists()) tx.delete(snap.ref); + }); +} + +/** Deletion step 5, the completion gate. Already removed or already done counts as done. */ +export function markCleanupDone(hid: string, rid: string): Promise { + return write(async (tx) => { + const snap = await tx.get(restaurantRef(hid, rid)); + if (!snap.exists()) return; + const d = snap.data() as DocumentData; + if (d.deleting !== true) throw new NotFoundError(); + if (d.cleanupDone === true) return; + tx.update(snap.ref, { cleanupDone: true, version: Number(d.version) + 1, updatedAt: serverTimestamp() }); + }); +} + +/** Deletion step 6. The rules refuse this unless the gate is satisfied. Already removed counts as done. */ +export function removeRestaurant(hid: string, rid: string): Promise { + return write(async (tx) => { + const snap = await tx.get(restaurantRef(hid, rid)); + if (snap.exists()) tx.delete(snap.ref); + }); +} + +export async function finishDeleting(hid: string, rid: string, onProgress?: (step: DeleteStep) => void): Promise { + onProgress?.("sweeping"); + const claims = await sweepClaims(hid, rid); + if (claims.kind !== "ok") return claims; + onProgress?.("sweepingNotes"); + const notes = await sweepNotes(hid, rid); + if (notes.kind !== "ok") return notes; + onProgress?.("removing"); + const state = await removeCollectionState(hid, rid); + if (state.kind !== "ok") return state; + const done = await markCleanupDone(hid, rid); + if (done.kind !== "ok") return done; + return removeRestaurant(hid, rid); +} +``` + +Update the module comment's second paragraph reference: the deletion protocol is now "spec §3.5, extended by §3.7". + +In `web/src/records/messages.ts` add: + +```ts +import type { DeleteStep } from "./repository"; + +const DELETE_PROGRESS: Record = { + marking: "Marking…", + sweeping: "Removing evidence…", + sweepingNotes: "Removing notes…", + removing: "Removing restaurant…", +}; + +export function deleteProgressText(step: DeleteStep): string { + return DELETE_PROGRESS[step]; +} +``` + +Merge the new `DeleteStep` import into the existing `import type { WriteOutcome } from "./repository";` line, making it `import type { DeleteStep, WriteOutcome } from "./repository";`. + +In `web/src/records/RestaurantsPage.tsx` and `web/src/records/RestaurantFormPage.tsx`, delete the local `STEP_TEXT` constant and the `type DeleteStep` import. Replace every `STEP_TEXT[step]` with `deleteProgressText(step)` and `STEP_TEXT.sweeping` with `deleteProgressText("sweeping")`, and import `deleteProgressText` from `"./messages"`. The form already imports `outcomeMessage` from there, so add `deleteProgressText` to that import. + +- [ ] **Step 10: Run the gates** + +Run: `npm run typecheck && npm run test:unit` → PASS (270 + 7 new: 6 repository, 1 messages; one repository test replaced). +Run: `npm run emu:test` → PASS. +Run: `npm run emu:e2e` → 29 PASS. Records scenarios 4 and 5 now finish deletion through the gate. + +- [ ] **Step 11: Commit** + +```bash +git add firestore.rules functions/test/rules.records.test.ts functions/test/rules.deletion.test.ts web/src/test/memoryFirestore.ts web/src/records/repository.ts web/src/records/repository.test.ts web/src/records/messages.ts web/src/records/messages.test.ts web/src/records/RestaurantsPage.tsx web/src/records/RestaurantFormPage.tsx +git commit -m "feat(records): deletion completion gate and convergent sweeps + +Restaurant delete now requires cleanupDone and no collection document, so +no client version can orphan notes or state; every deletion step reads +before it deletes. Mixed-version protocol proven against the rules. + +Co-Authored-By: Claude Opus 5.5 (1M context) " +``` + +--- +### Task 3: Client data layer — types, validation, `collection.ts`, `notes.ts` + +**Files:** +- Modify: `web/src/records/types.ts`, `web/src/records/validation.ts`, `web/src/records/validation.test.ts` +- Create: `web/src/records/collection.ts`, `web/src/records/collection.test.ts` +- Create: `web/src/records/notes.ts`, `web/src/records/notes.test.ts` + +**Interfaces:** +- Consumes (Task 2, `./repository`): `write`, `ConflictError`, `NotFoundError`, `LISTEN`, `listenerFailure`, `toDate`, `restaurantRef`, `notesCol`, `collectionRef`, `type Snapshot`, `type WriteOutcome`; `./dates`: `fromCalendarDate`, `toCalendarDate`, `isCalendarDate`, `compareCalendarDates`. +- Produces: + - `types.ts`: `interface CollectionState { shortlisted: boolean; visited: boolean; visitedOn?: CalendarDate; updatedBy: string; updatedByName: string; updatedAt: Date; version: number }` and `interface Note { id: string; text: string; authorUid: string; authorName: string; createdAt: Date; updatedAt: Date; version: number }` + - `validation.ts`: `validateNoteText(text: string): string | null`, `validateVisitedOn(value: string, today: CalendarDate): string | null` + - `collection.ts`: `watchCollection(hid, cb: (s: Snapshot>) => void): () => void`, `watchCollectionEntry(hid, rid, cb: (s: Snapshot) => void): () => void`, `setShortlisted(hid, rid, author: Author, baseVersion: number, shortlisted: boolean): Promise>`, `setVisited(hid, rid, author: Author, baseVersion: number, visitedOn: CalendarDate | null): Promise>` + - `notes.ts`: `watchNotes(hid, rid, cb: (s: Snapshot) => void): () => void` (newest first), `addNote(hid, rid, author: Author, text: string): Promise>`, `updateNote(hid, rid, nid, baseVersion: number, text: string): Promise>`, `deleteNote(hid, rid, nid, baseVersion: number): Promise` + +A missing collection document means base version 0. The UI treats it as "not shortlisted, not visited" only when the snapshot is `ready` (Tasks 4–5). + +- [ ] **Step 1: Write the failing validation tests** + +Append to `web/src/records/validation.test.ts` (merge `validateNoteText, validateVisitedOn` into the existing `./validation` import): + +```ts +describe("validateNoteText", () => { + it("refuses blank text and text over the limit, accepts up to 2,000 characters after trimming", () => { + expect(validateNoteText(" ")).toBe("Write something first."); + expect(validateNoteText("x".repeat(2001))).toBe("Keep this to 2000 characters."); + expect(validateNoteText(` ${"x".repeat(2000)} `)).toBeNull(); + expect(validateNoteText("Lovely staff")).toBeNull(); + }); +}); + +describe("validateVisitedOn", () => { + it("needs a real calendar date that is not after the device's today", () => { + expect(validateVisitedOn("", "2026-09-24")).toBe("Enter the date of the visit."); + expect(validateVisitedOn("2026-02-30", "2026-09-24")).toBe("Enter the date of the visit."); + expect(validateVisitedOn("2026-09-25", "2026-09-24")).toBe("The visit date can't be in the future."); + expect(validateVisitedOn("2026-09-24", "2026-09-24")).toBeNull(); + expect(validateVisitedOn("2025-05-03", "2026-09-24")).toBeNull(); + }); +}); +``` + +- [ ] **Step 2: Write the failing `collection.test.ts`** + +Create `web/src/records/collection.test.ts`: + +```ts +import { serverTimestamp, Timestamp } from "firebase/firestore"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { memoryTransactions, type Store } from "../test/memoryFirestore"; + +const m = vi.hoisted(() => ({ runTransaction: vi.fn(), onSnapshot: vi.fn() })); + +vi.mock("../firebase", () => ({ db: { fake: true } })); +vi.mock("firebase/firestore", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + runTransaction: m.runTransaction, + onSnapshot: m.onSnapshot, + getDocsFromServer: vi.fn(), + collection: (_db: unknown, ...segments: string[]) => ({ path: segments.join("/") }), + doc: (parent: unknown, ...segments: string[]) => { + const base = typeof (parent as { path?: string }).path === "string" ? (parent as { path: string }).path : ""; + const path = [base, ...segments].filter(Boolean).join("/"); + return { id: segments[segments.length - 1] ?? "", path }; + }, + query: (source: unknown) => source, + orderBy: () => undefined, + limit: () => undefined, + }; +}); + +import { setShortlisted, setVisited, watchCollection, watchCollectionEntry } from "./collection"; + +const RP = "households/home/restaurants/r1"; +const SP = "households/home/collection/r1"; +const AVA = { uid: "ava-uid", displayName: "Ava" }; +const live = { name: "Da Marco", address: "Via Roma 1", deleting: false, version: 2 }; +const stored = { shortlisted: true, visited: true, visitedOn: Timestamp.fromDate(new Date("2026-05-03T00:00:00Z")), updatedBy: "bogdan-uid", updatedByName: "Bogdan", updatedAt: Timestamp.fromDate(new Date("2026-05-03T10:00:00Z")), version: 3 }; + +beforeEach(() => { + Object.defineProperty(navigator, "onLine", { configurable: true, value: true }); +}); +afterEach(() => vi.clearAllMocks()); + +describe("setShortlisted / setVisited", () => { + it("creates version 1 from base 0 with the author's identity and a server timestamp", async () => { + const store: Store = new Map([[RP, live]]); + memoryTransactions(m.runTransaction, store); + expect(await setShortlisted("home", "r1", AVA, 0, true)).toEqual({ kind: "ok", value: 1 }); + expect(store.get(SP)).toEqual({ shortlisted: true, visited: false, updatedBy: "ava-uid", updatedByName: "Ava", updatedAt: serverTimestamp(), version: 1 }); + }); + + it("changing the shortlist keeps the visit date and bumps the version", async () => { + const store: Store = new Map>([[RP, live], [SP, stored]]); + memoryTransactions(m.runTransaction, store); + expect(await setShortlisted("home", "r1", AVA, 3, false)).toEqual({ kind: "ok", value: 4 }); + expect(store.get(SP)).toMatchObject({ shortlisted: false, visited: true, visitedOn: Timestamp.fromDate(new Date("2026-05-03T00:00:00Z")), updatedBy: "ava-uid", version: 4 }); + }); + + it("setVisited stores a UTC-midnight date; clearing drops visitedOn and keeps the shortlist", async () => { + const store: Store = new Map([[RP, live]]); + memoryTransactions(m.runTransaction, store); + expect(await setVisited("home", "r1", AVA, 0, "2026-09-20")).toEqual({ kind: "ok", value: 1 }); + expect((store.get(SP)!.visitedOn as Timestamp).toDate().toISOString()).toBe("2026-09-20T00:00:00.000Z"); + expect(store.get(SP)).toMatchObject({ shortlisted: false, visited: true }); + await setShortlisted("home", "r1", AVA, 1, true); + expect(await setVisited("home", "r1", AVA, 2, null)).toEqual({ kind: "ok", value: 3 }); + expect(store.get(SP)).not.toHaveProperty("visitedOn"); + expect(store.get(SP)).toMatchObject({ shortlisted: true, visited: false, version: 3 }); + }); + + it("reports conflict without writing when the stored version differs from the base", async () => { + const store: Store = new Map>([[RP, live], [SP, stored]]); + const tx = memoryTransactions(m.runTransaction, store); + expect(await setShortlisted("home", "r1", AVA, 2, false)).toEqual({ kind: "conflict" }); + expect(await setShortlisted("home", "r1", AVA, 0, true)).toEqual({ kind: "conflict" }); + expect(tx.set).not.toHaveBeenCalled(); + }); + + it("reports notFound for a missing restaurant or one marked deleting", async () => { + memoryTransactions(m.runTransaction, new Map()); + expect(await setShortlisted("home", "r1", AVA, 0, true)).toEqual({ kind: "notFound" }); + memoryTransactions(m.runTransaction, new Map([[RP, { ...live, deleting: true }]])); + expect(await setVisited("home", "r1", AVA, 0, "2026-09-20")).toEqual({ kind: "notFound" }); + }); + + it("reports offline before starting a transaction when the browser is offline", async () => { + Object.defineProperty(navigator, "onLine", { configurable: true, value: false }); + expect(await setShortlisted("home", "r1", AVA, 0, true)).toEqual({ kind: "offline" }); + expect(m.runTransaction).not.toHaveBeenCalled(); + }); +}); + +describe("watchers", () => { + it("watchCollection maps documents by restaurant id and reports ready/offline", () => { + const seen: unknown[] = []; + m.onSnapshot.mockImplementation((_q: unknown, _o: unknown, next: (s: unknown) => void) => { + next({ metadata: { fromCache: false }, docs: [{ id: "r1", data: () => stored }] }); + next({ metadata: { fromCache: true }, docs: [] }); + return () => {}; + }); + watchCollection("home", (s) => seen.push(s)); + expect(seen[0]).toEqual({ status: "ready", value: { r1: { shortlisted: true, visited: true, visitedOn: "2026-05-03", updatedBy: "bogdan-uid", updatedByName: "Bogdan", updatedAt: new Date("2026-05-03T10:00:00Z"), version: 3 } } }); + expect(seen[1]).toEqual({ status: "offline", value: {} }); + }); + + it("watchCollectionEntry reports null for a missing document, with the snapshot's source, and denied on permission errors", () => { + const seen: unknown[] = []; + m.onSnapshot.mockImplementation((_r: unknown, _o: unknown, next: (s: unknown) => void, fail: (e: unknown) => void) => { + next({ metadata: { fromCache: false }, exists: () => false }); + next({ metadata: { fromCache: true }, exists: () => false }); + fail(Object.assign(new Error("denied"), { code: "permission-denied" })); + return () => {}; + }); + watchCollectionEntry("home", "r1", (s) => seen.push(s)); + expect(seen).toEqual([{ status: "ready", value: null }, { status: "offline", value: null }, { status: "denied" }]); + expect(m.onSnapshot.mock.calls[0]![1]).toEqual({ includeMetadataChanges: true }); + }); +}); +``` + +- [ ] **Step 3: Write the failing `notes.test.ts`** + +Create `web/src/records/notes.test.ts`: + +```ts +import { serverTimestamp, Timestamp } from "firebase/firestore"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { memoryTransactions, type Store } from "../test/memoryFirestore"; + +const m = vi.hoisted(() => ({ runTransaction: vi.fn(), onSnapshot: vi.fn(), orderBy: vi.fn() })); + +vi.mock("../firebase", () => ({ db: { fake: true } })); +vi.mock("firebase/firestore", async (importOriginal) => { + const actual = await importOriginal(); + let generated = 0; + return { + ...actual, + runTransaction: m.runTransaction, + onSnapshot: m.onSnapshot, + getDocsFromServer: vi.fn(), + collection: (_db: unknown, ...segments: string[]) => ({ path: segments.join("/") }), + doc: (parent: unknown, ...segments: string[]) => { + const base = typeof (parent as { path?: string }).path === "string" ? (parent as { path: string }).path : ""; + const id = segments.length > 0 ? segments[segments.length - 1]! : `gen-${++generated}`; + const path = segments.length > 0 ? [base, ...segments].filter(Boolean).join("/") : `${base}/${id}`; + return { id, path }; + }, + query: (source: unknown) => source, + orderBy: m.orderBy, + limit: () => undefined, + }; +}); + +import { addNote, deleteNote, updateNote, watchNotes } from "./notes"; + +const RP = "households/home/restaurants/r1"; +const NP = `${RP}/notes/n1`; +const AVA = { uid: "ava-uid", displayName: "Ava" }; +const live = { name: "Da Marco", address: "Via Roma 1", deleting: false, version: 2 }; +const at = Timestamp.fromDate(new Date("2026-09-01T10:00:00Z")); +const note = { text: "Great staff", authorUid: "ava-uid", authorName: "Ava", createdAt: at, updatedAt: at, version: 2 }; + +beforeEach(() => { + Object.defineProperty(navigator, "onLine", { configurable: true, value: true }); +}); +afterEach(() => vi.clearAllMocks()); + +describe("addNote", () => { + it("writes version 1, the author's identity and server timestamps under a live restaurant", async () => { + const store: Store = new Map([[RP, live]]); + memoryTransactions(m.runTransaction, store); + const outcome = await addNote("home", "r1", AVA, "Staff knew about cross-contamination."); + expect(outcome.kind).toBe("ok"); + const id = (outcome as { value: string }).value; + expect(store.get(`${RP}/notes/${id}`)).toEqual({ + text: "Staff knew about cross-contamination.", + authorUid: "ava-uid", + authorName: "Ava", + createdAt: serverTimestamp(), + updatedAt: serverTimestamp(), + version: 1, + }); + }); + + it("reports notFound under a missing restaurant or one marked deleting", async () => { + memoryTransactions(m.runTransaction, new Map()); + expect((await addNote("home", "r1", AVA, "x")).kind).toBe("notFound"); + memoryTransactions(m.runTransaction, new Map([[RP, { ...live, deleting: true }]])); + expect((await addNote("home", "r1", AVA, "x")).kind).toBe("notFound"); + }); +}); + +describe("updateNote", () => { + it("writes only text, updatedAt and the next version", async () => { + const store: Store = new Map>([[RP, live], [NP, note]]); + const tx = memoryTransactions(m.runTransaction, store); + expect(await updateNote("home", "r1", "n1", 2, "Edited")).toEqual({ kind: "ok", value: 3 }); + expect(tx.update).toHaveBeenCalledWith({ id: "n1", path: NP }, { text: "Edited", updatedAt: serverTimestamp(), version: 3 }); + }); + + it("reports conflict on a stale base and notFound for a missing note or a restaurant marked deleting", async () => { + const store: Store = new Map>([[RP, live], [NP, note]]); + const tx = memoryTransactions(m.runTransaction, store); + expect(await updateNote("home", "r1", "n1", 1, "Edited")).toEqual({ kind: "conflict" }); + expect(await updateNote("home", "r1", "gone", 1, "Edited")).toEqual({ kind: "notFound" }); + store.set(RP, { ...live, deleting: true }); + expect(await updateNote("home", "r1", "n1", 2, "Edited")).toEqual({ kind: "notFound" }); + expect(tx.update).not.toHaveBeenCalled(); + }); +}); + +describe("deleteNote", () => { + it("deletes only the version the member confirmed", async () => { + const store: Store = new Map>([[RP, live], [NP, note]]); + memoryTransactions(m.runTransaction, store); + expect(await deleteNote("home", "r1", "n1", 1)).toEqual({ kind: "conflict" }); + expect(store.has(NP)).toBe(true); + expect(await deleteNote("home", "r1", "n1", 2)).toEqual({ kind: "ok", value: undefined }); + expect(store.has(NP)).toBe(false); + expect(await deleteNote("home", "r1", "n1", 2)).toEqual({ kind: "notFound" }); + }); +}); + +describe("watchNotes", () => { + it("lists newest first and reports ready/offline", () => { + const seen: unknown[] = []; + m.onSnapshot.mockImplementation((_q: unknown, _o: unknown, next: (s: unknown) => void) => { + next({ metadata: { fromCache: true }, docs: [{ id: "n1", data: () => note }] }); + return () => {}; + }); + watchNotes("home", "r1", (s) => seen.push(s)); + expect(m.orderBy).toHaveBeenCalledWith("createdAt", "desc"); + expect(seen[0]).toEqual({ status: "offline", value: [{ id: "n1", text: "Great staff", authorUid: "ava-uid", authorName: "Ava", createdAt: new Date("2026-09-01T10:00:00Z"), updatedAt: new Date("2026-09-01T10:00:00Z"), version: 2 }] }); + }); +}); +``` + +- [ ] **Step 4: Run to verify they fail** + +Run: `npm --prefix web test -- validation collection notes` +Expected: FAIL (modules and functions missing). + +- [ ] **Step 5: Implement** + +Append to `web/src/records/types.ts`: + +```ts +/** + * Read model of households/{hid}/collection/{rid} (spec §3.7): household-wide shortlist and + * visited state. A missing document means not shortlisted, not visited, version 0, but only + * when the snapshot came from the server. + */ +export interface CollectionState { + shortlisted: boolean; + visited: boolean; + visitedOn?: CalendarDate; + updatedBy: string; + updatedByName: string; + updatedAt: Date; + version: number; +} + +/** Read model of households/{hid}/restaurants/{rid}/notes/{nid}. Personal notes, never evidence. */ +export interface Note { + id: string; + text: string; + authorUid: string; + authorName: string; + createdAt: Date; + updatedAt: Date; + version: number; +} +``` + +Append to `web/src/records/validation.ts`: + +```ts +export function validateNoteText(text: string): string | null { + const trimmed = text.trim(); + if (trimmed === "") return "Write something first."; + if (trimmed.length > LIMITS.note) return tooLong(LIMITS.note); + return null; +} + +/** `today` is the device's local calendar day (useToday); the rules allow one day of slack. */ +export function validateVisitedOn(value: string, today: CalendarDate): string | null { + if (!isCalendarDate(value)) return "Enter the date of the visit."; + if (compareCalendarDates(value, today) > 0) return "The visit date can't be in the future."; + return null; +} +``` + +Create `web/src/records/collection.ts`: + +```ts +import { collection, onSnapshot, serverTimestamp, Timestamp, type DocumentData, type DocumentSnapshot } from "firebase/firestore"; +import { db } from "../firebase"; +import { fromCalendarDate, toCalendarDate } from "./dates"; +import { collectionRef, ConflictError, LISTEN, listenerFailure, NotFoundError, restaurantRef, toDate, write, type Snapshot, type WriteOutcome } from "./repository"; +import type { Author, CalendarDate, CollectionState } from "./types"; + +/** + * Shortlist and visited state (spec §3.7). One document per restaurant, id = restaurant id, + * written whole by every change so the stored shape always matches the rules. Online-only + * transactions via the repository's write() (spec §3.5). + */ + +export function toCollectionState(snap: Pick): CollectionState { + const d = snap.data() as DocumentData; + const s: CollectionState = { + shortlisted: d.shortlisted === true, + visited: d.visited === true, + updatedBy: String(d.updatedBy), + updatedByName: String(d.updatedByName), + updatedAt: toDate(d.updatedAt), + version: Number(d.version), + }; + if (d.visitedOn instanceof Timestamp) s.visitedOn = toCalendarDate(d.visitedOn); + return s; +} + +export function watchCollection(hid: string, cb: (s: Snapshot>) => void): () => void { + return onSnapshot( + collection(db, "households", hid, "collection"), + LISTEN, + (snap) => { + const value: Record = {}; + for (const d of snap.docs) value[d.id] = toCollectionState(d); + cb({ status: snap.metadata.fromCache ? "offline" : "ready", value }); + }, + (err) => cb(listenerFailure(err)), + ); +} + +export function watchCollectionEntry(hid: string, rid: string, cb: (s: Snapshot) => void): () => void { + return onSnapshot( + collectionRef(hid, rid), + LISTEN, + (snap) => cb({ status: snap.metadata.fromCache ? "offline" : "ready", value: snap.exists() ? toCollectionState(snap) : null }), + (err) => cb(listenerFailure(err)), + ); +} + +interface StatePatch { + shortlisted?: boolean; + visitedOn?: CalendarDate | null; +} + +function writeState(hid: string, rid: string, author: Author, baseVersion: number, patch: StatePatch): Promise> { + return write(async (tx) => { + const parent = await tx.get(restaurantRef(hid, rid)); + if (!parent.exists() || (parent.data() as DocumentData).deleting === true) throw new NotFoundError(); + const ref = collectionRef(hid, rid); + const snap = await tx.get(ref); + const current = snap.exists() ? toCollectionState(snap) : null; + const version = current?.version ?? 0; + if (version !== baseVersion) throw new ConflictError(); + const shortlisted = patch.shortlisted ?? current?.shortlisted ?? false; + const visitedOn = patch.visitedOn !== undefined ? patch.visitedOn : (current?.visitedOn ?? null); + const data: DocumentData = { + shortlisted, + visited: visitedOn !== null, + updatedBy: author.uid, + updatedByName: author.displayName, + updatedAt: serverTimestamp(), + version: version + 1, + }; + if (visitedOn !== null) data.visitedOn = fromCalendarDate(visitedOn); + tx.set(ref, data); + return version + 1; + }); +} + +export function setShortlisted(hid: string, rid: string, author: Author, baseVersion: number, shortlisted: boolean): Promise> { + return writeState(hid, rid, author, baseVersion, { shortlisted }); +} + +/** `null` clears the visit. */ +export function setVisited(hid: string, rid: string, author: Author, baseVersion: number, visitedOn: CalendarDate | null): Promise> { + return writeState(hid, rid, author, baseVersion, { visitedOn }); +} +``` + +Create `web/src/records/notes.ts`: + +```ts +import { doc, onSnapshot, orderBy, query, serverTimestamp, type DocumentData, type DocumentSnapshot } from "firebase/firestore"; +import { ConflictError, LISTEN, listenerFailure, notesCol, NotFoundError, restaurantRef, toDate, write, type Snapshot, type WriteOutcome } from "./repository"; +import type { Author, Note } from "./types"; + +/** + * Authored notes on a restaurant (spec §3.7). Only the author edits or deletes (rules-enforced); + * edits and deletes carry the version the member saw, so a change from another device surfaces + * as a conflict instead of being overwritten. Callers pass validated, trimmed text. + */ + +export function toNote(snap: Pick): Note { + const d = snap.data() as DocumentData; + return { + id: snap.id, + text: String(d.text), + authorUid: String(d.authorUid), + authorName: String(d.authorName), + createdAt: toDate(d.createdAt), + updatedAt: toDate(d.updatedAt), + version: Number(d.version), + }; +} + +export function watchNotes(hid: string, rid: string, cb: (s: Snapshot) => void): () => void { + return onSnapshot( + query(notesCol(hid, rid), orderBy("createdAt", "desc")), + LISTEN, + (snap) => cb({ status: snap.metadata.fromCache ? "offline" : "ready", value: snap.docs.map(toNote) }), + (err) => cb(listenerFailure(err)), + ); +} + +export function addNote(hid: string, rid: string, author: Author, text: string): Promise> { + return write(async (tx) => { + const parent = await tx.get(restaurantRef(hid, rid)); + if (!parent.exists() || (parent.data() as DocumentData).deleting === true) throw new NotFoundError(); + const ref = doc(notesCol(hid, rid)); + tx.set(ref, { text, authorUid: author.uid, authorName: author.displayName, createdAt: serverTimestamp(), updatedAt: serverTimestamp(), version: 1 }); + return ref.id; + }); +} + +export function updateNote(hid: string, rid: string, nid: string, baseVersion: number, text: string): Promise> { + return write(async (tx) => { + const parent = await tx.get(restaurantRef(hid, rid)); + const snap = await tx.get(doc(notesCol(hid, rid), nid)); + if (!parent.exists() || (parent.data() as DocumentData).deleting === true || !snap.exists()) throw new NotFoundError(); + const current = toNote(snap); + if (current.version !== baseVersion) throw new ConflictError(); + const next = baseVersion + 1; + tx.update(snap.ref, { text, updatedAt: serverTimestamp(), version: next }); + return next; + }); +} + +export function deleteNote(hid: string, rid: string, nid: string, baseVersion: number): Promise { + return write(async (tx) => { + const snap = await tx.get(doc(notesCol(hid, rid), nid)); + if (!snap.exists()) throw new NotFoundError(); + if (toNote(snap).version !== baseVersion) throw new ConflictError(); + tx.delete(snap.ref); + }); +} +``` + +- [ ] **Step 6: Run the gates** + +Run: `npm run typecheck && npm run test:unit` → PASS (previous + 2 validation + 8 collection + 6 notes). + +- [ ] **Step 7: Commit** + +```bash +git add web/src/records/types.ts web/src/records/validation.ts web/src/records/validation.test.ts web/src/records/collection.ts web/src/records/collection.test.ts web/src/records/notes.ts web/src/records/notes.test.ts +git commit -m "feat(records): shortlist/visited and notes data layer + +Co-Authored-By: Claude Opus 5.5 (1M context) " +``` + +--- +### Task 4: Saved page — joined read state, Shortlist/All filter, labels, empty states, resume wording + +**Files:** +- Create: `web/src/records/combine.ts`, `web/src/records/combine.test.ts` +- Create: `web/src/records/join.ts`, `web/src/records/join.test.ts` +- Modify: `web/src/records/messages.ts`, `web/src/records/messages.test.ts` +- Modify: `web/src/records/RestaurantsPage.tsx`, `web/src/records/RestaurantsPage.test.tsx` +- Modify: `web/src/styles.css` +- Modify: `web/e2e/records.spec.ts` (two list assertions now need *All records*) + +**Interfaces:** +- Consumes: `watchRestaurants`, `finishDeleting` (`./repository`); `watchCollection` (`./collection`, Task 3); `WatchState` (`./useWatch`); `formatCalendarDate` (`./dates`); `deleteProgressText` (`./messages`, Task 2). +- Produces: + - `combine.ts`: `isData(s: WatchState): s is Extract, { status: "ready" | "offline" }>`, `anyOffline(...states: Array>): boolean`, `combineStates(a: WatchState, b: WatchState): WatchState<[A, B]>` + - `join.ts`: `interface RecordRow { restaurant: Restaurant; state: CollectionState | null }`, `type RecordFilter = "shortlist" | "all"`, `joinRecords(restaurants, states): RecordRow[]`, `filterRows(rows, filter): RecordRow[]` + - `messages.ts`: `finishOutcomeText(kind: WriteOutcome["kind"]): string` + - Saved page testids (new): `filter-shortlist`, `filter-all` (`aria-pressed`), `label-shortlisted`, `label-visited`, `shortlist-empty`. Existing testids are kept. + +- [ ] **Step 1: Write the failing pure-helper tests** + +Create `web/src/records/combine.test.ts`: + +```ts +import { describe, expect, it } from "vitest"; +import { anyOffline, combineStates, isData } from "./combine"; + +describe("combineStates", () => { + it("is loading until both listeners have produced a snapshot", () => { + expect(combineStates({ status: "loading" }, { status: "ready", value: 1 })).toEqual({ status: "loading" }); + expect(combineStates({ status: "ready", value: 1 }, { status: "loading" })).toEqual({ status: "loading" }); + }); + + it("never hides denied or an error behind other states, denied first", () => { + expect(combineStates({ status: "error", message: "boom" }, { status: "denied" })).toEqual({ status: "denied" }); + expect(combineStates({ status: "offline", value: 1 }, { status: "error", message: "boom" })).toEqual({ status: "error", message: "boom" }); + expect(combineStates({ status: "loading" }, { status: "error", message: "boom" })).toEqual({ status: "error", message: "boom" }); + }); + + it("is offline when either side is cache-backed, ready only when both are from the server", () => { + expect(combineStates({ status: "offline", value: 1 }, { status: "ready", value: "a" })).toEqual({ status: "offline", value: [1, "a"] }); + expect(combineStates({ status: "ready", value: 1 }, { status: "ready", value: "a" })).toEqual({ status: "ready", value: [1, "a"] }); + }); +}); + +describe("isData / anyOffline", () => { + it("classify states", () => { + expect(isData({ status: "offline", value: [] })).toBe(true); + expect(isData({ status: "loading" })).toBe(false); + expect(anyOffline({ status: "ready", value: 1 }, { status: "loading" })).toBe(false); + expect(anyOffline({ status: "ready", value: 1 }, { status: "offline", value: 2 })).toBe(true); + }); +}); +``` + +Create `web/src/records/join.test.ts`: + +```ts +import { describe, expect, it } from "vitest"; +import { filterRows, joinRecords } from "./join"; +import type { CollectionState, Restaurant } from "./types"; + +const r = (id: string, deleting = false): Restaurant => ({ id, name: id, address: "x", createdBy: "ava-uid", createdAt: new Date(0), updatedAt: new Date(0), version: 1, deleting }); +const s = (shortlisted: boolean): CollectionState => ({ shortlisted, visited: false, updatedBy: "ava-uid", updatedByName: "Ava", updatedAt: new Date(0), version: 1 }); + +describe("joinRecords / filterRows", () => { + it("pairs each restaurant with its state by id, null when absent", () => { + const rows = joinRecords([r("a"), r("b")], { a: s(true) }); + expect(rows).toEqual([{ restaurant: r("a"), state: s(true) }, { restaurant: r("b"), state: null }]); + }); + + it("the shortlist keeps shortlisted rows and every deleting row; all keeps everything", () => { + const rows = joinRecords([r("a"), r("b"), r("c"), r("d", true)], { a: s(true), b: s(false) }); + expect(filterRows(rows, "shortlist").map((x) => x.restaurant.id)).toEqual(["a", "d"]); + expect(filterRows(rows, "all").map((x) => x.restaurant.id)).toEqual(["a", "b", "c", "d"]); + }); +}); +``` + +Append to `web/src/records/messages.test.ts` (merge `finishOutcomeText` into the import): + +```ts +describe("finishOutcomeText", () => { + it("tells the member what to do when resuming a deletion fails", () => { + expect(finishOutcomeText("offline")).toBe("You are offline. Connect, then tap Finish deleting."); + expect(finishOutcomeText("notFound")).toBe("Already removed."); + expect(finishOutcomeText("permission")).toContain("That change was refused."); + expect(finishOutcomeText("failed")).toBe("Could not finish deleting. Tap Finish deleting to try again."); + expect(finishOutcomeText("conflict")).toBe("Could not finish deleting. Tap Finish deleting to try again."); + }); +}); +``` + +- [ ] **Step 2: Write the failing page tests** + +In `web/src/records/RestaurantsPage.test.tsx`: + +(a) Extend the hoisted mocks and add the collection mock. Replace the `m` block, the `./repository` mock line and the `beforeEach` with: + +```ts +const m = vi.hoisted(() => ({ + watchRestaurants: vi.fn(), + watchCollection: vi.fn(), + finishDeleting: vi.fn(), + signOut: vi.fn(), +})); +vi.mock("./repository", () => ({ watchRestaurants: m.watchRestaurants, finishDeleting: m.finishDeleting })); +vi.mock("./collection", () => ({ watchCollection: m.watchCollection })); +``` + +```ts +let emit: (s: Snapshot) => void = () => {}; +let emitState: (s: Snapshot>) => void = () => {}; +beforeEach(() => { + m.watchRestaurants.mockImplementation((_hid: string, cb: (s: Snapshot) => void) => { + emit = cb; + return () => {}; + }); + // Existing tests assume an authoritative, empty collection unless a test says otherwise. + m.watchCollection.mockImplementation((_hid: string, cb: (s: Snapshot>) => void) => { + emitState = cb; + cb({ status: "ready", value: {} }); + return () => {}; + }); + m.finishDeleting.mockResolvedValue({ kind: "ok", value: undefined }); +}); +``` + +Import `CollectionState` next to `Restaurant` from `./types`. + +(b) The existing test `"shows loading, then the household's restaurants as links"` now needs the *All records* filter because nothing is shortlisted. Insert `await userEvent.click(screen.getByTestId("filter-all"));` after the `act(() => emit(…))` line and make the test `async`. In `"disables Add while offline and shows the offline notice with the cached rows"`, add the same click (making it `async`) before asserting the row. + +(c) The existing error test counts `watchRestaurants` re-subscriptions. Retry now re-subscribes both listeners, so it still gains exactly one `watchRestaurants` call. Leave it unchanged. + +(d) Append: + +```ts +const st = (over: Partial = {}): CollectionState => ({ shortlisted: true, visited: false, updatedBy: "ava-uid", updatedByName: "Ava", updatedAt: new Date(), version: 1, ...over }); + +describe("RestaurantsPage — shortlist", () => { + it("defaults to the shortlist with labels, and All records shows everything", async () => { + renderPage(); + act(() => { + emit({ status: "ready", value: [r({ id: "a", name: "Da Marco" }), r({ id: "b", name: "Zest" })] }); + emitState({ status: "ready", value: { a: st({ visited: true, visitedOn: "2026-05-03" }) } }); + }); + expect(screen.getByTestId("filter-shortlist")).toHaveAttribute("aria-pressed", "true"); + const rows = screen.getAllByTestId("restaurant-row"); + expect(rows).toHaveLength(1); + expect(rows[0]).toHaveTextContent("Da Marco"); + expect(screen.getByTestId("label-shortlisted")).toHaveTextContent("Shortlisted"); + expect(screen.getByTestId("label-visited")).toHaveTextContent("Visited 3 May 2026"); + await userEvent.click(screen.getByTestId("filter-all")); + expect(screen.getAllByTestId("restaurant-row")).toHaveLength(2); + expect(screen.getByTestId("filter-all")).toHaveAttribute("aria-pressed", "true"); + }); + + it("distinguishes no records from an empty shortlist", () => { + renderPage(); + act(() => emit({ status: "ready", value: [r({ id: "a", name: "Da Marco" })] })); + expect(screen.getByTestId("shortlist-empty")).toHaveTextContent("Nothing on the shortlist. Open a record and tap Add to shortlist."); + expect(screen.queryByTestId("restaurants-empty")).toBeNull(); + act(() => emit({ status: "ready", value: [] })); + expect(screen.getByTestId("restaurants-empty")).toBeInTheDocument(); + expect(screen.queryByTestId("shortlist-empty")).toBeNull(); + }); + + it("never shows an empty shortlist from a cached or failed collection snapshot", () => { + renderPage(); + act(() => { + emit({ status: "ready", value: [r({ id: "a", name: "Da Marco" })] }); + emitState({ status: "offline", value: {} }); + }); + expect(screen.queryByTestId("shortlist-empty")).toBeNull(); + expect(screen.getAllByTestId("read-offline")).toHaveLength(1); + act(() => emitState({ status: "error", message: "boom" })); + expect(screen.getByTestId("read-error")).toHaveTextContent("boom"); + expect(screen.queryByTestId("shortlist-empty")).toBeNull(); + }); + + it("shows one offline notice when both listeners are cache-backed", () => { + renderPage(); + act(() => { + emit({ status: "offline", value: [r({ id: "a", name: "Da Marco" })] }); + emitState({ status: "offline", value: { a: st() } }); + }); + expect(screen.getAllByTestId("read-offline")).toHaveLength(1); + expect(screen.getByTestId("restaurant-row")).toHaveTextContent("Da Marco"); + }); + + it("keeps deleting rows under the shortlist filter", () => { + renderPage(); + act(() => emit({ status: "ready", value: [r({ id: "d", name: "Doomed", deleting: true })] })); + expect(screen.getByTestId("restaurant-deleting")).toHaveTextContent("Doomed"); + }); + + it("explains a failed resume in words the member can act on", async () => { + m.finishDeleting.mockResolvedValue({ kind: "failed", message: "x" }); + renderPage(); + act(() => emit({ status: "ready", value: [r({ id: "d", name: "Doomed", deleting: true })] })); + await waitFor(() => expect(screen.getByTestId("restaurant-deleting")).toHaveTextContent("Could not finish deleting. Tap Finish deleting to try again.")); + }); +}); +``` + +- [ ] **Step 3: Run to verify they fail** + +Run: `npm --prefix web test -- combine join messages RestaurantsPage` +Expected: FAIL (modules missing; the page has no filter). + +- [ ] **Step 4: Implement the helpers** + +Create `web/src/records/combine.ts`: + +```ts +import type { WatchState } from "./useWatch"; + +/** + * Read states for views built from several listeners (spec §3.7 "Read states for joined data"). + * Precedence: denied, gone, error, loading, then offline if any side is cache-backed, else ready. + * An error is never hidden behind the offline notice. + */ +export function isData(s: WatchState): s is Extract, { status: "ready" | "offline" }> { + return s.status === "ready" || s.status === "offline"; +} + +export function anyOffline(...states: Array>): boolean { + return states.some((s) => s.status === "offline"); +} + +export function combineStates(a: WatchState, b: WatchState): WatchState<[A, B]> { + if (a.status === "denied" || b.status === "denied") return { status: "denied" }; + if (a.status === "gone" || b.status === "gone") return { status: "gone" }; + if (a.status === "error") return { status: "error", message: a.message }; + if (b.status === "error") return { status: "error", message: b.message }; + if (!isData(a) || !isData(b)) return { status: "loading" }; + return { status: a.status === "offline" || b.status === "offline" ? "offline" : "ready", value: [a.value, b.value] }; +} +``` + +Create `web/src/records/join.ts`: + +```ts +import type { CollectionState, Restaurant } from "./types"; + +export interface RecordRow { + restaurant: Restaurant; + state: CollectionState | null; +} + +export type RecordFilter = "shortlist" | "all"; + +export function joinRecords(restaurants: Restaurant[], states: Record): RecordRow[] { + return restaurants.map((restaurant) => ({ restaurant, state: states[restaurant.id] ?? null })); +} + +/** Deleting rows stay under both filters so an interrupted deletion can always be finished. */ +export function filterRows(rows: RecordRow[], filter: RecordFilter): RecordRow[] { + if (filter === "all") return rows; + return rows.filter((row) => row.restaurant.deleting || row.state?.shortlisted === true); +} +``` + +Append to `web/src/records/messages.ts`: + +```ts +/** The Saved page's "Finish deleting" outcomes (the parked Plan 2b resume wording). */ +export function finishOutcomeText(kind: WriteOutcome["kind"]): string { + switch (kind) { + case "ok": return ""; + case "offline": return "You are offline. Connect, then tap Finish deleting."; + case "notFound": return "Already removed."; + case "permission": return outcomeMessage("permission", "This restaurant"); + case "conflict": + case "failed": return "Could not finish deleting. Tap Finish deleting to try again."; + } +} +``` + +- [ ] **Step 5: Implement the page** + +Replace `web/src/records/RestaurantsPage.tsx` with: + +```tsx +import { useEffect, useRef, useState } from "react"; +import { Link } from "react-router"; +import { watchCollection } from "./collection"; +import { combineStates, isData } from "./combine"; +import { formatCalendarDate } from "./dates"; +import { filterRows, joinRecords, type RecordFilter } from "./join"; +import { deleteProgressText, finishOutcomeText } from "./messages"; +import { finishDeleting, watchRestaurants } from "./repository"; +import { ReadStateNotice } from "./ReadStateNotice"; +import type { CollectionState, Restaurant } from "./types"; +import { useMember } from "./useMember"; +import { useWatch } from "./useWatch"; + +export function RestaurantsPage() { + const { householdId } = useMember(); + const restaurants = useWatch((cb) => watchRestaurants(householdId, cb), [householdId]); + const states = useWatch>((cb) => watchCollection(householdId, cb), [householdId]); + const [filter, setFilter] = useState("shortlist"); + const [progress, setProgress] = useState>({}); + const resumed = useRef(new Set()); + const combined = combineStates(restaurants.state, states.state); + const offline = combined.status === "offline"; + const all = isData(combined) ? joinRecords(combined.value[0], combined.value[1]) : []; + const rows = filterRows(all, filter); + + function retry() { + restaurants.retry(); + states.retry(); + } + + async function finish(rid: string) { + setProgress((p) => ({ ...p, [rid]: deleteProgressText("sweeping") })); + const outcome = await finishDeleting(householdId, rid, (step) => setProgress((p) => ({ ...p, [rid]: deleteProgressText(step) }))); + if (outcome.kind !== "ok") setProgress((p) => ({ ...p, [rid]: finishOutcomeText(outcome.kind) })); + } + + // Resume interrupted deletions once per mount from the authoritative restaurant list alone, + // whatever the collection listener is doing (deletion protocol is resumable, spec §3.5/§3.7). + const rs = restaurants.state; + useEffect(() => { + if (rs.status !== "ready") return; + for (const r of rs.value) { + if (r.deleting && !resumed.current.has(r.id)) { + resumed.current.add(r.id); + void finish(r.id); + } + } + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [rs]); + + return ( +
+

Saved

+

Our restaurant records.

+ +

+ { if (offline) e.preventDefault(); }} + className={offline ? "disabled-link" : undefined} + > + Add restaurant + +

+

+ + +

+ {combined.status === "ready" && all.length === 0 &&

No restaurants yet. Add the first one.

} + {combined.status === "ready" && all.length > 0 && rows.length === 0 && ( +

Nothing on the shortlist. Open a record and tap Add to shortlist.

+ )} + {rows.length > 0 && ( +
    + {rows.map(({ restaurant: r, state }) => + r.deleting ? ( +
  • + {r.name} — Deleting… +
    + {progress[r.id]} + +
    +
    • + ) : ( +
  • + + {r.name} +
    + {r.address} + {state && (state.shortlisted || state.visitedOn) && ( + + {state.shortlisted && Shortlisted} + {state.visited && state.visitedOn && Visited {formatCalendarDate(state.visitedOn)}} + + )} + +
    • + ), + )} +
+ )} +
+ ); +} +``` + +Append to `web/src/styles.css`: + +```css +.filter { display: flex; gap: 0.5rem; } +.filter button[aria-pressed="true"] { font-weight: 600; text-decoration: underline; } +.labels { display: flex; gap: 0.4rem; flex-wrap: wrap; margin-top: 0.3rem; } +.label { font-size: 0.8rem; border: 1px solid #8886; border-radius: 999px; padding: 0.05rem 0.5rem; } +``` + +- [ ] **Step 6: Update the two existing browser assertions** + +The Saved tab now opens on the shortlist, and those two scenarios' records are not shortlisted. In `web/e2e/records.spec.ts`: +- In scenario 1, immediately before `await expect(page.getByTestId("restaurant-row")).toContainText("Da Marco");`, insert `await page.getByTestId("filter-all").click();`. +- In scenario 7, immediately before `await expect(page.getByTestId("restaurant-row")).toContainText("Ava's place");`, insert the same line. + +Scenario 7's later `toHaveCount(0)` on the not-invited screen needs no change. + +- [ ] **Step 7: Run the gates** + +Run: `npm run typecheck && npm run test:unit` → PASS (previous + 4 combine + 2 join + 1 messages + 6 page). +Run: `npm run emu:e2e` → 29 PASS. + +- [ ] **Step 8: Commit** + +```bash +git add web/src/records/combine.ts web/src/records/combine.test.ts web/src/records/join.ts web/src/records/join.test.ts web/src/records/messages.ts web/src/records/messages.test.ts web/src/records/RestaurantsPage.tsx web/src/records/RestaurantsPage.test.tsx web/src/styles.css web/e2e/records.spec.ts +git commit -m "feat(saved): shortlist filter, visited labels and joined read states + +Co-Authored-By: Claude Opus 5.5 (1M context) " +``` + +--- +### Task 5: Restaurant page — shortlist and visited status block, one offline notice + +**Files:** +- Create: `web/src/records/StatusBlock.tsx`, `web/src/records/StatusBlock.test.tsx` +- Modify: `web/src/records/messages.ts`, `web/src/records/messages.test.ts` +- Modify: `web/src/records/RestaurantDetailPage.tsx`, `web/src/records/RestaurantDetailPage.test.tsx` + +**Interfaces:** +- Consumes: `setShortlisted`, `setVisited`, `watchCollectionEntry` (`./collection`, Task 3); `isData`, `anyOffline` (`./combine`, Task 4); `validateVisitedOn` (Task 3); `useToday`, `formatCalendarDate`, `outcomeMessage`, `ReadStateNotice`. +- Produces: + - `StatusBlock` props: `{ householdId: string; rid: string; author: Author; state: WatchState; disabled: boolean; onRetry: () => void }` + - testids: `status-block`, `shortlist-add`, `shortlist-state`, `shortlist-remove`, `visited-mark`, `visited-date`, `visited-save`, `visited-cancel`, `visited-error`, `visited-state`, `visited-change`, `visited-clear`, `status-changed-by`, `status-outcome` (`data-kind`) + - `messages.ts`: `statusOutcomeMessage(kind: WriteOutcome["kind"]): string` + - The detail page shows exactly one `read-offline` notice when any of its listeners is cache-backed. + +Controls are enabled only when the collection snapshot is `ready` (server-backed), the page is not offline, and no write is in flight. A missing document is "not shortlisted, not visited", base version 0. + +- [ ] **Step 1: Write the failing tests** + +Append to `web/src/records/messages.test.ts` (merge `statusOutcomeMessage` into the import): + +```ts +describe("statusOutcomeMessage", () => { + it("explains a lost race without asking for a draft reload", () => { + expect(statusOutcomeMessage("conflict")).toBe("Someone else changed this at the same moment. The current state is shown; try again if you still want the change."); + expect(statusOutcomeMessage("notFound")).toBe("This restaurant was deleted."); + expect(statusOutcomeMessage("offline")).toBe("You are offline. Connect and try again."); + }); +}); +``` + +Create `web/src/records/StatusBlock.test.tsx`: + +```tsx +import { fireEvent, render, screen, waitFor } from "@testing-library/react"; +import userEvent from "@testing-library/user-event"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import type { CollectionState } from "./types"; +import type { WatchState } from "./useWatch"; + +const m = vi.hoisted(() => ({ setShortlisted: vi.fn(), setVisited: vi.fn() })); +vi.mock("./collection", () => m); +vi.mock("./useToday", () => ({ useToday: () => "2026-09-24" })); +vi.mock("../auth/AuthProvider", () => ({ useAuth: () => ({ signOut: vi.fn() }) })); + +import { StatusBlock } from "./StatusBlock"; + +const AVA = { uid: "ava-uid", displayName: "Ava" }; +const st = (over: Partial = {}): CollectionState => ({ shortlisted: false, visited: false, updatedBy: "bogdan-uid", updatedByName: "Bogdan", updatedAt: new Date(), version: 3, ...over }); + +function renderBlock(state: WatchState, disabled = false) { + return render( {}} />); +} + +beforeEach(() => { + m.setShortlisted.mockResolvedValue({ kind: "ok", value: 1 }); + m.setVisited.mockResolvedValue({ kind: "ok", value: 1 }); +}); +afterEach(() => vi.clearAllMocks()); + +describe("StatusBlock", () => { + it("a missing document from the server means not shortlisted, not visited, base version 0", async () => { + renderBlock({ status: "ready", value: null }); + expect(screen.queryByTestId("status-changed-by")).toBeNull(); + await userEvent.click(screen.getByTestId("shortlist-add")); + expect(m.setShortlisted).toHaveBeenCalledWith("home", "r1", AVA, 0, true); + }); + + it("shows the stored state and who changed it last; Remove uses the stored version", async () => { + renderBlock({ status: "ready", value: st({ shortlisted: true, visited: true, visitedOn: "2026-05-03" }) }); + expect(screen.getByTestId("shortlist-state")).toHaveTextContent("On shortlist"); + expect(screen.getByTestId("visited-state")).toHaveTextContent("Visited 3 May 2026"); + expect(screen.getByTestId("status-changed-by")).toHaveTextContent("Last changed by Bogdan"); + await userEvent.click(screen.getByTestId("shortlist-remove")); + expect(m.setShortlisted).toHaveBeenCalledWith("home", "r1", AVA, 3, false); + }); + + it("Mark visited defaults to today, refuses a future date, and saves a past one", async () => { + renderBlock({ status: "ready", value: st() }); + await userEvent.click(screen.getByTestId("visited-mark")); + const input = screen.getByTestId("visited-date"); + expect(input).toHaveValue("2026-09-24"); + // jsdom sanitises partial date strings, so set the whole value at once. + fireEvent.change(input, { target: { value: "2026-09-30" } }); + await userEvent.click(screen.getByTestId("visited-save")); + expect(screen.getByTestId("visited-error")).toHaveTextContent("The visit date can't be in the future."); + expect(m.setVisited).not.toHaveBeenCalled(); + fireEvent.change(input, { target: { value: "2026-09-20" } }); + await userEvent.click(screen.getByTestId("visited-save")); + expect(m.setVisited).toHaveBeenCalledWith("home", "r1", AVA, 3, "2026-09-20"); + await waitFor(() => expect(screen.queryByTestId("visited-date")).toBeNull()); + }); + + it("Change date starts from the stored date; Clear sends null", async () => { + renderBlock({ status: "ready", value: st({ visited: true, visitedOn: "2026-05-03" }) }); + await userEvent.click(screen.getByTestId("visited-change")); + expect(screen.getByTestId("visited-date")).toHaveValue("2026-05-03"); + await userEvent.click(screen.getByTestId("visited-cancel")); + await userEvent.click(screen.getByTestId("visited-clear")); + expect(m.setVisited).toHaveBeenCalledWith("home", "r1", AVA, 3, null); + }); + + it("a conflict shows the standard message and never retries by itself", async () => { + m.setShortlisted.mockResolvedValue({ kind: "conflict" }); + renderBlock({ status: "ready", value: null }); + await userEvent.click(screen.getByTestId("shortlist-add")); + await waitFor(() => expect(screen.getByTestId("status-outcome")).toHaveAttribute("data-kind", "conflict")); + expect(m.setShortlisted).toHaveBeenCalledTimes(1); + }); + + it.each([ + ["cached", { status: "offline", value: null } as const, false], + ["from a page that is offline", { status: "ready", value: null } as const, true], + ])("disables every control when the state is %s", (_label, state, disabled) => { + renderBlock(state, disabled); + expect(screen.getByTestId("shortlist-add")).toBeDisabled(); + expect(screen.getByTestId("visited-mark")).toBeDisabled(); + }); + + it("shows loading and errors instead of controls until the state is known", () => { + renderBlock({ status: "loading" }); + expect(screen.queryByTestId("shortlist-add")).toBeNull(); + expect(screen.getByTestId("read-loading")).toBeInTheDocument(); + renderBlock({ status: "error", message: "boom" }); + expect(screen.getByTestId("read-error")).toHaveTextContent("boom"); + }); +}); +``` + +In `web/src/records/RestaurantDetailPage.test.tsx`: + +(a) Add the collection mock and emitter. Change the hoisted mocks to include `watchCollectionEntry: vi.fn(), setShortlisted: vi.fn(), setVisited: vi.fn()`. Keep `vi.mock("./repository", () => m)` and add `vi.mock("./collection", () => m);`. Add + +```ts +let emitState: (s: Snapshot) => void = () => {}; +``` + +to the emitters. In `beforeEach` add the following. Existing tests assume an authoritative "nothing stored" state: + +```ts + m.watchCollectionEntry.mockImplementation((_h: string, _r: string, cb: (s: Snapshot) => void) => { + emitState = cb; + cb({ status: "ready", value: null }); + return () => {}; + }); +``` + +Import `CollectionState` from `./types`. + +(b) Append: + +```ts +describe("RestaurantDetailPage — status block", () => { + it("renders the status block with the stored state", () => { + renderPage(); + act(() => { + emitRestaurant({ status: "ready", value: restaurant }); + emitClaims({ status: "ready", value: [] }); + emitState({ status: "ready", value: { shortlisted: true, visited: false, updatedBy: "bogdan-uid", updatedByName: "Bogdan", updatedAt: new Date(), version: 2 } }); + }); + expect(screen.getByTestId("shortlist-state")).toHaveTextContent("On shortlist"); + }); + + it("shows one offline notice when only the collection state is cached", () => { + renderPage(); + act(() => { + emitRestaurant({ status: "ready", value: restaurant }); + emitClaims({ status: "ready", value: [] }); + emitState({ status: "offline", value: null }); + }); + expect(screen.getAllByTestId("read-offline")).toHaveLength(1); + expect(screen.getByTestId("shortlist-add")).toBeDisabled(); + expect(screen.getByTestId("add-evidence")).toHaveAttribute("aria-disabled", "true"); + }); + + it("shows a collection error in the status block without hiding the evidence", () => { + renderPage(); + act(() => { + emitRestaurant({ status: "ready", value: restaurant }); + emitClaims({ status: "ready", value: [] }); + emitState({ status: "error", message: "boom" }); + }); + expect(screen.getByTestId("status-block")).toHaveTextContent("boom"); + expect(screen.getByTestId("evidence-gfMenu")).toBeInTheDocument(); + }); +}); +``` + +- [ ] **Step 2: Run to verify they fail** + +Run: `npm --prefix web test -- messages StatusBlock RestaurantDetailPage` +Expected: FAIL (`StatusBlock`, `statusOutcomeMessage` missing; the page has no status block). + +- [ ] **Step 3: Implement** + +Append to `web/src/records/messages.ts`: + +```ts +/** Shortlist/visited writes: there is no draft to reload, the live state is already on screen. */ +export function statusOutcomeMessage(kind: WriteOutcome["kind"]): string { + switch (kind) { + case "conflict": return "Someone else changed this at the same moment. The current state is shown; try again if you still want the change."; + case "notFound": return "This restaurant was deleted."; + default: return outcomeMessage(kind, "This change"); + } +} +``` + +Create `web/src/records/StatusBlock.tsx`: + +```tsx +import { useState } from "react"; +import { setShortlisted, setVisited } from "./collection"; +import { isData } from "./combine"; +import { formatCalendarDate } from "./dates"; +import { statusOutcomeMessage } from "./messages"; +import { ReadStateNotice } from "./ReadStateNotice"; +import type { WriteOutcome } from "./repository"; +import type { Author, CollectionState } from "./types"; +import { useToday } from "./useToday"; +import type { WatchState } from "./useWatch"; +import { validateVisitedOn } from "./validation"; + +interface Props { + householdId: string; + rid: string; + author: Author; + state: WatchState; + /** True when any listener on the page is cache-backed: writes need a connection. */ + disabled: boolean; + onRetry: () => void; +} + +/** + * Household shortlist and visited state for one restaurant (spec §3.7). Never computes or shows + * anything safety-related; visiting touches only the collection document. + */ +export function StatusBlock({ householdId, rid, author, state, disabled, onRetry }: Props) { + const today = useToday(); + const [busy, setBusy] = useState(false); + const [outcome, setOutcome] = useState(null); + const [editingDate, setEditingDate] = useState(null); + const [dateError, setDateError] = useState(null); + + if (!isData(state)) { + return ( +
+ +
+ ); + } + + const current = state.value; + // A missing or cached document is only a safe base when it came from the server. + const locked = disabled || busy || state.status !== "ready"; + const base = current?.version ?? 0; + + async function run(action: () => Promise>): Promise { + setBusy(true); + setOutcome(null); + const result = await action(); + setBusy(false); + if (result.kind !== "ok") setOutcome(result.kind); + return result.kind === "ok"; + } + + async function saveDate() { + if (editingDate === null) return; + const problem = validateVisitedOn(editingDate, today); + setDateError(problem); + if (problem) return; + if (await run(() => setVisited(householdId, rid, author, base, editingDate))) setEditingDate(null); + } + + return ( +
+

+ {current?.shortlisted ? ( + <> + On shortlist + + + ) : ( + + )} +

+

+ {editingDate === null && current?.visited && current.visitedOn && ( + <> + Visited {formatCalendarDate(current.visitedOn)} + + + + )} + {editingDate === null && !current?.visited && ( + + )} + {editingDate !== null && ( + <> + + + + {dateError && {dateError}} + + )} +

+ {current &&

Last changed by {current.updatedByName}

} + {outcome &&

{statusOutcomeMessage(outcome)}

} +
+ ); +} +``` + +In `web/src/records/RestaurantDetailPage.tsx`: + +(a) Add imports: + +```ts +import { watchCollectionEntry } from "./collection"; +import { anyOffline, isData } from "./combine"; +import { StatusBlock } from "./StatusBlock"; +``` + +and change the types import to include `CollectionState`. + +(b) In `RestaurantDetailPage`, take `uid` and `displayName` from `useMember()` (`const { householdId, uid, displayName } = useMember();`). After `claimsWatch` add: + +```ts + const stateWatch = useWatch((cb) => watchCollectionEntry(householdId, rid!, cb), [householdId, rid]); +``` + +(c) Replace the lines from `const restaurant = rs.value;` through `const claimsReady = …;` (keeping `onDeleteClaim`) with: + +```ts + const restaurant = rs.value; + const ss = stateWatch.state; + // One notice for every cache-backed listener on the page (spec §3.7); writes need the server. + const offline = anyOffline(rs, cs, ss); + const claimsReady = isData(cs); + const claims = claimsReady ? cs.value : []; + const summary = summariseEvidence(claims, today); +``` + +Keep `async function onDeleteClaim` unchanged. Remove the now-duplicated earlier `offline`, `claims` and `claimsReady` declarations. + +(d) In the JSX, replace the first `` with: + +```tsx + {offline && } +``` + +and replace `{(!claimsReady || (cs.status === "offline" && rs.status !== "offline")) && }` with: + +```tsx + {!claimsReady && } +``` + +(e) Directly after the `

…

` with the phone/website/maps/edit links, before `

Evidence

`, insert: + +```tsx + +``` + +- [ ] **Step 4: Run the gates** + +Run: `npm run typecheck && npm run test:unit` → PASS (previous + 1 messages + 8 StatusBlock (7 tests, one `it.each` with two rows) + 3 detail page). The existing detail-page offline-notice matrix (6 rows) still passes: exactly one `read-offline`. + +- [ ] **Step 5: Commit** + +```bash +git add web/src/records/StatusBlock.tsx web/src/records/StatusBlock.test.tsx web/src/records/messages.ts web/src/records/messages.test.ts web/src/records/RestaurantDetailPage.tsx web/src/records/RestaurantDetailPage.test.tsx +git commit -m "feat(records): shortlist and visited controls on the restaurant page + +Co-Authored-By: Claude Opus 5.5 (1M context) " +``` + +--- +### Task 6: Notes on the restaurant page; deletion wording in the form + +**Files:** +- Create: `web/src/records/NotesSection.tsx`, `web/src/records/NotesSection.test.tsx` +- Modify: `web/src/records/RestaurantDetailPage.tsx`, `web/src/records/RestaurantDetailPage.test.tsx` +- Modify: `web/src/records/RestaurantFormPage.tsx`, `web/src/records/RestaurantFormPage.test.tsx` +- Modify: `web/src/styles.css` + +**Interfaces:** +- Consumes: `watchNotes`, `addNote`, `updateNote`, `deleteNote` (`./notes`, Task 3); `validateNoteText`, `LIMITS` (Task 3/1); `isData`, `anyOffline` (Task 4); `outcomeMessage`. +- Produces: + - `NotesSection` props: `{ householdId: string; rid: string; author: Author; state: WatchState; disabled: boolean; onRetry: () => void }` + - testids: `notes-section`, `notes-empty`, `note-add-text`, `note-add-counter`, `note-add-save`, `note-add-error`, `note-add-outcome`; per note `note-{id}` (`data-version`), `note-edited-{id}`, `note-edit-{id}`, `note-edit-text-{id}`, `note-save-{id}`, `note-cancel-{id}`, `note-error-{id}`, `note-conflict-{id}`, `note-conflict-current-{id}`, `note-keep-mine-{id}`, `note-use-theirs-{id}`, `note-delete-{id}`, `note-delete-confirm-{id}`, `note-delete-cancel-{id}`, `note-outcome-{id}` (`data-kind`) + - Form: the delete confirmation reads "Deletes the restaurant, its evidence, and both members' notes." A failed delete uses the delete verb. + +Rules for the UI (spec §3.7): +- Edit and Delete appear only on the caller's own notes. +- A pending delete confirmation is bound to the note's id and version. It resets when that note changes. +- A failed add or save keeps the draft. +- An edit conflict shows the current server text beside the draft. "Keep mine" writes with the version shown; "Use theirs" drops the draft. + +- [ ] **Step 1: Write the failing `NotesSection` tests** + +Create `web/src/records/NotesSection.test.tsx`: + +```tsx +import { act, render, screen, waitFor } from "@testing-library/react"; +import userEvent from "@testing-library/user-event"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import type { Note } from "./types"; +import type { WatchState } from "./useWatch"; + +const m = vi.hoisted(() => ({ addNote: vi.fn(), updateNote: vi.fn(), deleteNote: vi.fn() })); +vi.mock("./notes", () => m); +vi.mock("../auth/AuthProvider", () => ({ useAuth: () => ({ signOut: vi.fn() }) })); + +import { NotesSection } from "./NotesSection"; + +const AVA = { uid: "ava-uid", displayName: "Ava" }; +const note = (over: Partial & Pick): Note => ({ text: "Staff were careful", authorUid: "ava-uid", authorName: "Ava", createdAt: new Date("2026-09-01T10:00:00Z"), updatedAt: new Date("2026-09-01T10:00:00Z"), version: 1, ...over }); + +function renderSection(state: WatchState, disabled = false) { + const view = render( {}} />); + return { + ...view, + update: (next: WatchState) => view.rerender( {}} />), + }; +} + +beforeEach(() => { + m.addNote.mockResolvedValue({ kind: "ok", value: "new" }); + m.updateNote.mockResolvedValue({ kind: "ok", value: 2 }); + m.deleteNote.mockResolvedValue({ kind: "ok", value: undefined }); +}); +afterEach(() => vi.clearAllMocks()); + +describe("NotesSection", () => { + it("says notes are not evidence, lists notes with authors, and offers controls only on the member's own", () => { + renderSection({ status: "ready", value: [note({ id: "a" }), note({ id: "b", authorUid: "bogdan-uid", authorName: "Bogdan", version: 2 })] }); + expect(screen.getByTestId("notes-section")).toHaveTextContent("Personal notes. They are not evidence and don't change any checked date."); + expect(screen.getByTestId("note-b")).toHaveTextContent("Bogdan"); + expect(screen.getByTestId("note-edited-b")).toBeInTheDocument(); + expect(screen.queryByTestId("note-edited-a")).toBeNull(); + expect(screen.getByTestId("note-edit-a")).toBeInTheDocument(); + expect(screen.queryByTestId("note-edit-b")).toBeNull(); + expect(screen.queryByTestId("note-delete-b")).toBeNull(); + }); + + it("shows the empty state only from the server", () => { + const view = renderSection({ status: "offline", value: [] }); + expect(screen.queryByTestId("notes-empty")).toBeNull(); + view.update({ status: "ready", value: [] }); + expect(screen.getByTestId("notes-empty")).toHaveTextContent("No notes yet."); + }); + + it("adds a trimmed note, refuses blank text, and keeps the draft when saving fails", async () => { + renderSection({ status: "ready", value: [] }); + await userEvent.click(screen.getByTestId("note-add-save")); + expect(screen.getByTestId("note-add-error")).toHaveTextContent("Write something first."); + expect(m.addNote).not.toHaveBeenCalled(); + await userEvent.type(screen.getByTestId("note-add-text"), " Asked about the fryer "); + expect(screen.getByTestId("note-add-counter")).toHaveTextContent("21 / 2000"); + m.addNote.mockResolvedValueOnce({ kind: "offline" }); + await userEvent.click(screen.getByTestId("note-add-save")); + await waitFor(() => expect(screen.getByTestId("note-add-outcome")).toHaveAttribute("data-kind", "offline")); + expect(screen.getByTestId("note-add-text")).toHaveValue(" Asked about the fryer "); + await userEvent.click(screen.getByTestId("note-add-save")); + expect(m.addNote).toHaveBeenLastCalledWith("home", "r1", AVA, "Asked about the fryer"); + await waitFor(() => expect(screen.getByTestId("note-add-text")).toHaveValue("")); + }); + + it("edits with the version the member started from", async () => { + renderSection({ status: "ready", value: [note({ id: "a", version: 4 })] }); + await userEvent.click(screen.getByTestId("note-edit-a")); + const box = screen.getByTestId("note-edit-text-a"); + await userEvent.clear(box); + await userEvent.type(box, "Went back, still careful"); + await userEvent.click(screen.getByTestId("note-save-a")); + expect(m.updateNote).toHaveBeenCalledWith("home", "r1", "a", 4, "Went back, still careful"); + await waitFor(() => expect(screen.queryByTestId("note-edit-text-a")).toBeNull()); + }); + + it("an edit conflict shows the current text beside the draft; Keep mine writes with the version shown", async () => { + const view = renderSection({ status: "ready", value: [note({ id: "a", version: 1 })] }); + await userEvent.click(screen.getByTestId("note-edit-a")); + await userEvent.clear(screen.getByTestId("note-edit-text-a")); + await userEvent.type(screen.getByTestId("note-edit-text-a"), "My draft"); + view.update({ status: "ready", value: [note({ id: "a", version: 2, text: "Changed on the phone" })] }); + m.updateNote.mockResolvedValueOnce({ kind: "conflict" }); + await userEvent.click(screen.getByTestId("note-save-a")); + await waitFor(() => expect(screen.getByTestId("note-conflict-a")).toBeInTheDocument()); + expect(screen.getByTestId("note-conflict-current-a")).toHaveTextContent("Changed on the phone"); + expect(screen.getByTestId("note-edit-text-a")).toHaveValue("My draft"); + await userEvent.click(screen.getByTestId("note-keep-mine-a")); + expect(m.updateNote).toHaveBeenLastCalledWith("home", "r1", "a", 2, "My draft"); + }); + + it("Use theirs drops the draft", async () => { + const view = renderSection({ status: "ready", value: [note({ id: "a" })] }); + await userEvent.click(screen.getByTestId("note-edit-a")); + view.update({ status: "ready", value: [note({ id: "a", version: 2, text: "Theirs" })] }); + m.updateNote.mockResolvedValueOnce({ kind: "conflict" }); + await userEvent.click(screen.getByTestId("note-save-a")); + await userEvent.click(await screen.findByTestId("note-use-theirs-a")); + expect(screen.queryByTestId("note-edit-text-a")).toBeNull(); + expect(screen.getByTestId("note-a")).toHaveTextContent("Theirs"); + }); + + it("delete needs a confirmation bound to the version shown, and deletes that version", async () => { + const view = renderSection({ status: "ready", value: [note({ id: "a", version: 1 })] }); + await userEvent.click(screen.getByTestId("note-delete-a")); + expect(m.deleteNote).not.toHaveBeenCalled(); + act(() => view.update({ status: "ready", value: [note({ id: "a", version: 2, text: "Edited elsewhere" })] })); + expect(screen.queryByTestId("note-delete-confirm-a")).toBeNull(); + await userEvent.click(screen.getByTestId("note-delete-a")); + await userEvent.click(screen.getByTestId("note-delete-confirm-a")); + expect(m.deleteNote).toHaveBeenCalledWith("home", "r1", "a", 2); + }); + + it("disables adding, editing and deleting while the page is offline", () => { + renderSection({ status: "offline", value: [note({ id: "a" })] }, true); + expect(screen.getByTestId("note-add-save")).toBeDisabled(); + expect(screen.getByTestId("note-edit-a")).toBeDisabled(); + expect(screen.getByTestId("note-delete-a")).toBeDisabled(); + }); + + it("shows loading and errors from the notes listener", () => { + renderSection({ status: "error", message: "boom" }); + expect(screen.getByTestId("read-error")).toHaveTextContent("boom"); + }); +}); +``` + +- [ ] **Step 2: Write the failing page and form tests** + +In `web/src/records/RestaurantDetailPage.test.tsx`: add `watchNotes: vi.fn(), addNote: vi.fn(), updateNote: vi.fn(), deleteNote: vi.fn()` to the hoisted `m`, add `vi.mock("./notes", () => m);`, and add an emitter plus a `beforeEach` default (an authoritative empty list): + +```ts +let emitNotes: (s: Snapshot) => void = () => {}; +``` + +```ts + m.watchNotes.mockImplementation((_h: string, _r: string, cb: (s: Snapshot) => void) => { + emitNotes = cb; + cb({ status: "ready", value: [] }); + return () => {}; + }); +``` + +Import `Note` from `./types`. Append: + +```ts +describe("RestaurantDetailPage — notes", () => { + it("places notes between the evidence and the call-ahead prompts", () => { + renderPage(); + act(() => { + emitRestaurant({ status: "ready", value: restaurant }); + emitClaims({ status: "ready", value: [] }); + emitNotes({ status: "ready", value: [{ id: "n1", text: "Asked twice, confident answers", authorUid: "bogdan-uid", authorName: "Bogdan", createdAt: new Date(), updatedAt: new Date(), version: 1 }] }); + }); + const notes = screen.getByTestId("notes-section"); + expect(notes).toHaveTextContent("Asked twice, confident answers"); + const evidence = screen.getByTestId("evidence-gfMenu"); + const callAhead = screen.getByTestId("call-ahead"); + expect(evidence.compareDocumentPosition(notes) & Node.DOCUMENT_POSITION_FOLLOWING).toBeTruthy(); + expect(notes.compareDocumentPosition(callAhead) & Node.DOCUMENT_POSITION_FOLLOWING).toBeTruthy(); + }); + + it("counts cached notes in the single offline notice", () => { + renderPage(); + act(() => { + emitRestaurant({ status: "ready", value: restaurant }); + emitClaims({ status: "ready", value: [] }); + emitNotes({ status: "offline", value: [] }); + }); + expect(screen.getAllByTestId("read-offline")).toHaveLength(1); + expect(screen.getByTestId("note-add-save")).toBeDisabled(); + }); +}); +``` + +In `web/src/records/RestaurantFormPage.test.tsx`, append inside the existing top-level `describe` that holds the delete tests: + +```ts + it("names notes in the delete confirmation and reports a failed delete with the delete verb", async () => { + m.deleteRestaurant.mockResolvedValue({ kind: "failed", message: "x" }); + renderAt("/restaurants/r1/edit"); + act(() => emit({ status: "ready", value: stored })); + await userEvent.click(screen.getByTestId("delete-restaurant")); + expect(screen.getByTestId("delete-question")).toHaveTextContent("Deletes the restaurant, its evidence, and both members' notes."); + await userEvent.click(screen.getByTestId("delete-confirm")); + await waitFor(() => expect(screen.getByTestId("save-outcome")).toHaveTextContent("Could not delete this restaurant. Try again.")); + }); +``` + +- [ ] **Step 3: Run to verify they fail** + +Run: `npm --prefix web test -- NotesSection RestaurantDetailPage RestaurantFormPage` +Expected: FAIL. + +- [ ] **Step 4: Implement `NotesSection`** + +Create `web/src/records/NotesSection.tsx`: + +```tsx +import { useState } from "react"; +import { isData } from "./combine"; +import { outcomeMessage } from "./messages"; +import { addNote, deleteNote, updateNote } from "./notes"; +import { ReadStateNotice } from "./ReadStateNotice"; +import type { WriteOutcome } from "./repository"; +import type { Author, Note } from "./types"; +import type { WatchState } from "./useWatch"; +import { LIMITS, validateNoteText } from "./validation"; + +interface Props { + householdId: string; + rid: string; + author: Author; + state: WatchState; + /** True when any listener on the page is cache-backed: writes need a connection. */ + disabled: boolean; + onRetry: () => void; +} + +const DATE = new Intl.DateTimeFormat("en-GB", { day: "numeric", month: "short", year: "numeric" }); + +function noteMessage(kind: WriteOutcome["kind"], adding: boolean): string { + if (kind === "notFound") return adding ? "This restaurant was deleted." : "This note was deleted."; + if (kind === "conflict") return "This note changed on another device."; + return outcomeMessage(kind, "This note"); +} + +/** Personal notes (spec §3.7). Never evidence: no kinds, no dates that feed evidence status. */ +export function NotesSection({ householdId, rid, author, state, disabled, onRetry }: Props) { + return ( +
+

Our notes

+

Personal notes. They are not evidence and don't change any checked date.

+ {!isData(state) && } + + {state.status === "ready" && state.value.length === 0 &&

No notes yet.

} + {isData(state) && + state.value.map((n) => ( + + ))} +
+ ); +} + +function NoteComposer({ householdId, rid, author, disabled }: { householdId: string; rid: string; author: Author; disabled: boolean }) { + const [text, setText] = useState(""); + const [error, setError] = useState(null); + const [outcome, setOutcome] = useState(null); + const [busy, setBusy] = useState(false); + + async function save() { + const problem = validateNoteText(text); + setError(problem); + setOutcome(null); + if (problem) return; + setBusy(true); + const result = await addNote(householdId, rid, author, text.trim()); + setBusy(false); + if (result.kind === "ok") { + setText(""); + return; + } + setOutcome(result.kind); // the draft stays in the box + } + + return ( +
+