diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml new file mode 100644 index 00000000..3e6cf41c --- /dev/null +++ b/.github/workflows/ci.yml @@ -0,0 +1,32 @@ +name: CI + +on: + pull_request: + branches: + - staging + - main + +jobs: + verify: + runs-on: ubuntu-latest + steps: + - name: Checkout + uses: actions/checkout@v4 + + - name: Set up Node.js + uses: actions/setup-node@v4 + with: + node-version: '20.9.0' + cache: npm + + - name: Install dependencies + run: npm ci + + - name: Lint + run: npm run lint + + - name: Type check + run: npx tsc --noEmit + + - name: Test + run: npm test diff --git a/.gitignore b/.gitignore index f95f6938..c9990269 100644 --- a/.gitignore +++ b/.gitignore @@ -40,4 +40,4 @@ yarn-error.log* # typescript *.tsbuildinfo next-env.d.ts -core +/core diff --git a/.nvmrc b/.nvmrc new file mode 100644 index 00000000..f3f52b42 --- /dev/null +++ b/.nvmrc @@ -0,0 +1 @@ +20.9.0 diff --git a/AGENTS.md b/AGENTS.md index 368d0651..eec74298 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -92,7 +92,10 @@ Certified is a passwordless identity platform built on **AT Protocol** (atproto) | React | 19.x | | Language | TypeScript 5 (strict, `paths: { "@/*": ["./src/*"] }`) | | Styling | Tailwind CSS 3.4 (utilities only) + custom CSS in `globals.css` (BEM-like) | -| Atproto SDK | `@atproto/api` 0.19, `@atproto/oauth-client-node` 0.3, `@atproto/jwk-jose` 0.1 (`@atproto/oauth-client` 0.6 pulled in transitively) | +| Theming | `next-themes` 0.4 (light/dark via `data-theme` on ``) | +| Atproto SDK | `@atproto/api` 0.13, `@atproto/oauth-client-node` 0.3, `@atproto/jwk-jose` 0.1 (`@atproto/oauth-client` 0.6 pulled in transitively) | +| Rich text | `@tiptap/react` 3.x (+ `starter-kit`, `extension-link`, `extension-placeholder`, `pm`) | +| Maps | `leaflet` 1.9 + `react-leaflet` 5.x | | Session/State store | Upstash Redis (`@upstash/redis`) — REST-based, serverless-safe | | Server actions | None — all server work is in route handlers (`src/app/api/**`) | | Wallets | `wagmi` 2.x + `viem` 2.x + `@tanstack/react-query` (mounted only on `/settings/wallet`) | @@ -642,7 +645,7 @@ These rules are mandatory. Treat any deviation as a regression. 6. **Sanitize input twice** — client AND server (defense in depth). Use `stripInvisible`, `sanitizeEmail`, `sanitizeHandle` from `src/lib/utils/sanitize.ts`. The regex is `/[​-‏
- ⁠-­͏؜᠎]/g`. 7. **Sanitize 5xx errors.** Never echo `err.message` from upstream PDS errors when status ≥ 500 — return `"Internal server error"` (or a route-specific generic). The XRPC proxy and `/api/groups/register` both do this; copy the pattern. (4xx errors *can* echo upstream messages — those are usually validation errors a user can act on.) 8. **Repo ownership on writes** — for `createRecord`/`putRecord`/`deleteRecord`, `body.repo` must equal the session DID. Cross-repo writes are 403. -9. **Collection allowlist** — only the four `ALLOWED_WRITE_COLLECTIONS` can be written through the XRPC proxy. Add to that array consciously, not implicitly. +9. **Collection allowlist** — only the eleven `ALLOWED_WRITE_COLLECTIONS` can be written through the XRPC proxy. Add to that array consciously, not implicitly. 10. **Blob limits** — 4 MB cap (image MIME types only) on `/api/xrpc/[...method]` for `uploadBlob`; 5 MB on the group blob route. Both check `Content-Length` and the actual buffer size. Vercel has a hard ~4.5 MB body cap that constrains the XRPC proxy. 11. **Service-auth tokens are short-lived and per-LXM** — `getServiceAuthToken(agent, lxm)` issues a token bound to a single method. Don't cache or reuse. diff --git a/CHANGES.md b/CHANGES.md new file mode 100644 index 00000000..4b64bd40 --- /dev/null +++ b/CHANGES.md @@ -0,0 +1,677 @@ +# CHANGES — overnight-review + +Running log of every discretionary change made on the `overnight-review` branch during the two-phase +auto-review. Source of findings: [`REVIEW.md`](./REVIEW.md). Created from `feat/positioning-redesign` +HEAD; `main` untouched; branch left unmerged. + +**Baseline at branch creation:** vitest `291 passing`, `tsc --noEmit` `0 errors`, `npm run lint` +`0 errors / 69 warnings`. + +**Commit gate (every implemented item):** the change is committed only if, after it, the full +vitest suite is green **and** `tsc --noEmit` reports 0 errors **and** lint introduces no new errors. +For testable behavior, a failing test is written first (and confirmed red) before the fix. Existing +tests are never modified to make a change pass; new tests may be added. If the gate can't be made +green within the item's scope, the change is reverted and the item is logged with status `BLOCKED`. + +**Final result (Phase 2 complete):** 106 commits on `overnight-review`. Of the actioned items: +**98 IMPLEMENTED, 3 BLOCKED, 3 SKIPPED** (see the Log below). All 11 bugs and 6 actionable risks +landed. Final gate: vitest **474 passing**, `tsc --noEmit` **0 errors**, `npm run lint` +**0 errors / 67 warnings** (down from 69; lint now also covers root configs + scripts via `eslint .`). +No baseline test was modified except an import-line expansion in `badges.test.ts` (additions only — +no assertion weakened). `main` is untouched; branch left unmerged. + +**Blocked (reverted, left for you):** +- **quality-002** — delete dead `cert-context.ts`. Its REVIEW.md recommendation is conditional on no + in-flight branch reintroducing the Explore aggregate; 4 remote branches import this exact module, so + deletion was held. (quality-003 still fixed the module's pagination gap.) +- **quality-033** — `useBskyPosts.loadMore` race. Not reproducible: the existing `requestIdRef` bump + + cursor reset on handle change already prevent the mis-attribution. No change made. +- **quality-045** — extract a shared list-modal shell. The two surfaces diverge in user-visible + behavior (focus/select, error copy, maxLength, labels, CSS), so no *pure* extraction exists → + escalated to judgment. + +**Known follow-up surfaced (not a gate; for your decision):** `quality-010` added `tsconfig.test.json` ++ a `typecheck:test` script. Running it reveals **~21 pre-existing type errors in test files** (mostly +`src/lib/atproto/__tests__/notifications.test.ts`) that were never type-checked before. CI (`ci.yml`) +intentionally runs only the main `tsc --noEmit` (which excludes tests) so CI stays green; cleaning up +the test-file type errors is a separate effort. + +--- + +## Held — not auto-implemented + +Excluded from auto-implementation because they change external behavior, a public API/contract, a data +schema, dependency choices, or architecture — or are coupled to a held judgment item. Listed for +Holke's decision; see the matching section in `REVIEW.md`. + +- **judgment-001 … judgment-010** — all 10 judgment items (see `REVIEW.md` § "Judgment — held for your decision"). +- **risk-005** — drop client-supplied `validate:false` on own-repo writes. Changes the write-envelope + contract; verifier rated it low-confidence optional hardening. Held. +- **risk-007** — add a rate limiter to `/api/resolve-did`. Duplicate of **judgment-002**. Held under judgment-002. +- **risk-009** — give `deleteFollow` a `targetDid` param + add a DELETE handler to the group follow + route. Adds new API surface. The behavior-preserving doc note may be applied; the API change is held. +- **quality-024** — add `autoprefixer` to the PostCSS chain. Identical change to **judgment-007**. Held under judgment-007. +- **quality-042** — `useProfilePds` IIFE alignment. Underlying bug refuted; report recommends skipping. Skipped. +- **quality-004** — reconcile/delete the dead `src/config/trusted-evaluators.ts`. The clean fix + requires either deleting an existing test file (forbidden — never modify the test suite) or + reconciling a 3-vs-4 DID divergence (a behavior change about which evaluators are trusted). Both are + human calls. Held. +- **quality-056 · approot-sitemap-public-gap** — add `/welcome` + `/apps` to `sitemap.ts`/robots. The + `/welcome` half is coupled to the unresolved canonical-URL decision **judgment-005**. Held. +- **quality-056 · quality-overexport-1** — de-export pervasively over-exported internal helpers. Too + broad/vague to auto-apply safely (risk of breaking re-exports/tests); better as a focused + human-guided cleanup. Held. + +--- + +## Log + +One entry per item, in implementation order. Status is `IMPLEMENTED` or `BLOCKED`. + +### bug-001 — Context-update attachment href scheme allowlist · IMPLEMENTED +- **Why it's an improvement:** Closes a stored XSS — a federated PDS author could plant a `javascript:` uri that executed on click in the certified.app origin. +- **Change:** `resolveAttachment` now rejects a uri attachment unless `safeHttpUrl(entry.uri)` is non-null, returning the normalized http(s) value; fixes all three render sinks at the source. +- **Test:** src/lib/atproto/__tests__/context-attachment.test.ts — fails before (javascript:/data: resolved), passes after. +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings + +### bug-002 — OG/Twitter share image references a non-existent certified-hero file on 5 pages · IMPLEMENTED +- **Why it's an improvement:** The most-shared landing URL (/welcome) and four legal pages served a 404 OG image, so social unfurls rendered with no preview; now they point at the real on-disk asset. +- **Change:** Replaced the five `certified-hero-1200x630.png` strings with `certs-hero-1200x630.png` in welcome/terms/privacy/dsa/imprint, matching the file on disk plus layout.tsx and about/page.tsx. +- **Test:** src/app/__tests__/metadata-og-image.test.ts — asserts every OG/Twitter image in each page's exported metadata resolves under public/; fails before (5 red), passes after. +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings + +### bug-003 — InvalidSwap detection is dead for the group BFF write path · IMPLEMENTED +- **Why it's an improvement:** Group admins' inline cert/project edits that lose a CID-precondition race now reach the conflict-rebase + drafts-recovery machinery instead of failing generically and silently dropping unsaved edits. +- **Change:** `extractRouteError` now surfaces the atproto discriminator (`XRPCError.error`) as `code`, and the group activity/project routes echo it as `{ error, code }` — mirroring the XRPC proxy — so `writeToRepo` re-raises `InvalidSwapError`. +- **Test:** src/lib/utils/__tests__/api.test.ts (extractRouteError surfaces `code: "InvalidSwap"`) — fails before, passes after; plus src/lib/atproto/__tests__/repo-write.test.ts (group-route InvalidSwap body → InvalidSwapError). +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings + +### bug-004 — Inline-edit Save during in-flight avatar/banner upload dropped the new image · IMPLEMENTED +- **Why it's an improvement:** Saving mid-upload no longer silently re-persists the stale `base.avatar`/`base.banner` while the UI shows the new image as saved. +- **Change:** `handleSave` now tracks the in-flight avatar/banner upload promises in refs and awaits them before composing the record, then writes the freshly-resolved blob instead of the (stale) closed-over state. +- **Test:** src/hooks/__tests__/use-profile-inline-edit.test.tsx — never-resolving `uploadAvatar`: asserts `putProfile` is never called with the stale `OLD_AVATAR` and Save can't complete; fails before, passes after. (Plus a resolve-path case asserting the NEW blob is persisted.) +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings + +### bug-005 — Project location strongRef now resolves + displays · IMPLEMENTED +- **Why it's an improvement:** The project Location meta row now renders the place name instead of silently disappearing (and never leaks `[object Object]`). +- **Change:** In project-detail.tsx, resolve the `location` strongRef like the edit page (parse at:// → getRecord via authFetch → `splitLocationName(value.name)`) into a resolved label used by the meta row + `hasAnyMeta`, keeping the legacy inline-string path. +- **Test:** src/components/project/__tests__/project-detail-location.test.tsx — fails before, passes after. +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings + +### bug-006 — Audit-log result pill allowlist matches actual permitted/denied values · IMPLEMENTED +- **Why it's an improvement:** Audit result chips now get their `--permitted`/`--denied` color class instead of always falling through to the unstyled `--unknown`. +- **Change:** In org-settings.tsx, replaced the stale `["success","failure","error"]` allowlist with an exported `auditResultClassSuffix` helper mapping `AuditEntry.result` ("permitted"|"denied") to the matching class suffix (else "unknown"). +- **Test:** src/components/groups/__tests__/org-settings.test.ts — asserts permitted→permitted, denied→denied, other→unknown; fails before, passes after. +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings + +### bug-007 — /endorsements Received tab hides rejected endorsements with no way to un-reject · IMPLEMENTED +- **Why it's an improvement:** The owner's own inbox now surfaces already-rejected endorsements (with Accept/Reject controls), so a rejected award no longer vanishes with no way to un-reject it. +- **Change:** In endorsements/page.tsx, `ReceivedEndorsementsList` now calls `useReceivedEndorsements(did, { includeRejected: true })` — matching the profile owner surface; §22.21 privacy holds since foreign viewers never reach this page. +- **Test:** src/app/__tests__/endorsements-received-rejected.test.tsx — mocks the hook to honor `includeRejected` and asserts the rejected row is absent before / renders after the fix; fails before, passes after. +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings + +### bug-008 — Blob/image upload errors swallowed on cert/project forms · IMPLEMENTED +- **Why it's an improvement:** A failed image upload now surfaces an error and clears the stale preview, so a cert/project can no longer publish silently without the previewed image. +- **Change:** Wrapped each `uploadBlob` call (create, project/new, project edit, project-detail inline-edit) in try/catch that sets the page's error state and clears+revokes the dangling preview/blob. +- **Test:** src/app/__tests__/create-image-upload-error.test.tsx — fails before (unhandled rejection, no error surfaced), passes after. +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings + +### bug-009 — Banner picker shows no live preview after selecting a file · IMPLEMENTED +- **Why it's an improvement:** The banner control now previews the picked image immediately instead of showing the stale saved banner, matching AvatarUpload's behavior in the same form. +- **Change:** BannerUpload self-previews via an object URL on pick (revoked on error/unmount), falling back to currentBannerUrl; this also fixes the group edit page, which uses BannerUpload directly. +- **Test:** src/components/profile/__tests__/banner-upload-preview.test.tsx — fails before, passes after. +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings + +### bug-010 — Feed PreviewCard MapPin never renders when a date period is also present · IMPLEMENTED +- **Why it's an improvement:** A cert with both a date period and locations now shows the location pin next to its "N locations" text, instead of dropping the pin whenever any other meta item is present. +- **Change:** CertPreview now pushes a ReactNode meta entry (`<> N locations`) and PreviewCard's `meta` prop became `ReactNode[]` with `withLocationIcon` removed, mirroring ExploreListRow / CertListRow. +- **Test:** src/components/home/__tests__/cert-preview-location-icon.test.tsx — fails before, passes after. +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings + +### bug-011 — useUserActivities.loadMore appends stale-DID records after a profile switch · IMPLEMENTED +- **Why it's an improvement:** After switching profiles, an in-flight loadMore for the previous DID no longer appends that DID's records to the reset list, and overlapping cursor-boundary edges no longer produce duplicate rows. +- **Change:** Added a `generationRef` bumped on each initial load; `loadMore` captures its generation, bails when superseded, and dedups appended records by `uri` via a `seen` Set (mirrors use-explore / use-home-feed). +- **Test:** src/hooks/__tests__/use-user-activities.test.tsx — fails before, passes after. +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings + +### risk-001 — XRPC proxy echoes un-redacted 4xx upstream messages to the client · IMPLEMENTED +- **Why it's an improvement:** A 4xx from createRecord/putRecord/updateEmail/etc. can embed a JWT/DPoP/Bearer fragment in its message; the proxy now redacts it before returning to the browser, matching the canonical extractRouteError posture. +- **Change:** In `xrpcError`, the 4xx branch now returns `redactSecrets(rawMessage)` instead of the raw upstream string (route.ts:111); 5xx/empty still collapse to "Internal server error". +- **Test:** src/app/api/xrpc/[...method]/__tests__/xrpc-error.test.ts — fails before, passes after (asserts Bearer/JWT fragments are redacted in echoed 4xx; clean 4xx still echoed; 5xx still generic; discriminator preserved). +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings + +### risk-002 — groups/register logs raw atproto error on org-limit path · IMPLEMENTED +- **Why it's an improvement:** stops DPoP/Bearer tokens (on `err.cause`/`.stack`/`.message`) from leaking into logs on the org-limit check failure path. +- **Change:** replaced `console.error("Org creation limit check failed:", err)` with `logSafe("[groups/register] org-limit check failed", err)`; kept the 503. +- **Test:** src/app/api/groups/register/__tests__/org-limit-log-safe.test.ts — fails before, passes after. +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings + +### risk-004 — groups/register does not server-side sanitize handle and forwards email unvalidated · IMPLEMENTED +- **Why it's an improvement:** enforces AGENTS.md §17.6/§24.5 sanitize-at-the-boundary defense-in-depth so invisible-char/over-length handles and malformed emails can't be forwarded verbatim to the group service. +- **Change:** run `sanitizeHandle(rawHandle)` and re-check the 253 cap on the sanitized result; validate the optional email with feedback/route.ts's regex + 254-char cap, rejecting with 400 on failure; forward the sanitized handle/validated email. +- **Test:** src/app/api/groups/register/__tests__/register-sanitize.test.ts — fails before, passes after (ZWSP/leading-@ stripped; sanitized-handle 253 cap; invalid email rejected/omitted; valid email forwarded unchanged). +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings + +### risk-003 — Profile inline-edit save orphans a location record on retry · IMPLEMENTED +- **Why it's an improvement:** a first-time location add followed by a failed marker write no longer re-mints a fresh orphan location record on each Save retry; the retry overwrites the same record in place. +- **Change:** in `handleSave`, capture the rkey minted by the first `putLocationRecord` (createRecord) into a session-scoped `mintedLocationRkeyRef`, and reuse it as the `existingRkey` fallback when the marker has no persisted strongRef; cleared on edit-click / cancel / successful save. +- **Test:** src/hooks/__tests__/use-profile-inline-edit-location-retry.test.tsx — fails before, passes after (putOrgMarker rejects once; asserts the retry's putLocationRecord reuses the minted rkey and allocates no new mint). +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings + +### risk-006 — App-only surfaces crawlable (missing robots disallow) · IMPLEMENTED +- **Why it's an improvement:** Keeps authenticated/app shells out of search indexes per AGENTS §18, while leaving public /profile and /project indexable. +- **Change:** Added /home, /explore, /search, /activity, /activity/* to the robots.ts disallow array. +- **Test:** src/app/__tests__/robots-app-only-disallow.test.ts — fails before, passes after. +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings + +### risk-008 — saveWithSwap `read` callbacks use raw `fetch` instead of `authFetch` · IMPLEMENTED +- **Why it's an improvement:** the conflict re-read now routes through `authFetch`, so a 401 on the session-bearing `/api/xrpc/.../getRecord` route fires the `onUnauthorized` interceptor instead of being silently swallowed. +- **Change:** swapped raw `fetch(` → `authFetch(` in the `read` callback of both project save flows (project-detail.tsx and the project edit page); `authFetch` was already imported in both. +- **Test:** src/app/project/[did]/[rkey]/edit/__tests__/save-reread-authfetch.test.tsx — fails before, passes after (drives a 409 write → conflict re-read 401 → asserts onUnauthorized fires). +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings + +### quality-001 — UserFeed component is dead code (never imported) · IMPLEMENTED +- **Why it's an improvement:** removes ~30 lines of unreachable code that also masked bug-011's only reachable path and would have rendered broken bylines (empty `did`) if ever wired. +- **Change:** deleted src/components/feed/user-feed.tsx after confirming 0 repo-wide importers (the only mention is a prose doc comment in feed-layout.tsx, untouched). +- **Test:** refactor; no natural test — full suite green. +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings + +### quality-002 — cert-context.ts dead module (304 lines) — delete · BLOCKED +- **Reason:** escalate-to-judgment: the Recommendation's explicit precondition ("confirm no in-flight branch reintroduces the Explore aggregate first") fails — four in-flight branches (origin/feat/88-follower-events-feed, /89-feed-enhancements, /quality-pass-rebased, /wider-sidebar-and-navbar-border) reintroduce the Explore aggregate via src/hooks/use-cert-context.ts + src/components/explore/* and import fetchAllCertContext / CertContextItem from this exact module; deleting now would conflict with that in-flight work. + +### quality-003 — fetchAllCertContext pagination · IMPLEMENTED +- **Why it's an improvement:** `listAndFilter` now follows the listRecords cursor, so it sees every record on the author's PDS instead of only the first 50 — matches that live past page one are no longer silently dropped, honoring the "All"/"every" naming. +- **Change:** rewrote `listAndFilter` to walk the cursor in a `while (true)` loop (limit 100, same 400/404→empty handling), filtering + normalizing each page, mirroring `fetchTypedLists` / `listEndorsementListCollections`. +- **Test:** refactor; no natural test (module is dead today — quality-002 — and tsconfig excludes __tests__) — full suite green. +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings + +### quality-005 — Dead EndorseShortcut + inert received-grid optimistic overlay · IMPLEMENTED +- **Why it's an improvement:** removes ~140 lines of unreachable code and a false comment from the largest profile file; `EndorseShortcut` was never rendered and the optimistic overlay was permanently inert (its only setters were never wired to any child), so deleting it eliminates dead state and a misleading claim about sidebar handoff. +- **Change:** removed `EndorseShortcut`, the `optimisticAdds`/`optimisticHides` state + the de-dup `displayReceived` memo (collapsed `displayReceived` to `received.endorsements`), the unused `handleEndorsed`/`handleRevoked` callbacks, and the misleading "exported to the sidebar" comment. +- **Test:** refactor; no natural test (testable=false; pure dead-code deletion) — full suite green. +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings + +### quality-011 — Dead exports in workspace.ts · IMPLEMENTED +- **Why it's an improvement:** removes never-imported public API, including a `@deprecated` function carrying a known filter-inversion bug, shrinking the surface callers can misuse. +- **Change:** deleted `fetchOrganizationDids`, the `@deprecated fetchOrganizationDidsForSet`, and the now-unused `OrganizationDidsGraphQLResponse` type (all zero importers). +- **Test:** refactor; no natural test (testable=false; pure dead-code deletion) — full suite green. +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings + +### quality-049 — CertHeadlineByline dead — delete · IMPLEMENTED +- **Why it's an improvement:** Removes dead, unimported code that duplicates author-byline logic now living inline in ActivityDetail (CertHeadlineColumns). +- **Change:** Deleted src/components/feed/cert-headline-byline.tsx (0 importers; shared CSS classes left intact as they're used by activity-detail.tsx). +- **Test:** refactor; no natural test — full suite green. +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings + +### quality-050 — LocationCard dead code · IMPLEMENTED +- **Why it's an improvement:** Removes an unused component and fixes a comment that referenced it, reducing dead code and doc drift. +- **Change:** Deleted src/components/feed/location-card.tsx and dropped the stale `+ LocationCard` mention from the doc comment in cert-locations-map.tsx. +- **Test:** refactor; no natural test — full suite green +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings + +### quality-051 — FeedLayout stale doc comments · IMPLEMENTED +- **Why it's an improvement:** JSDoc now names the real consumers instead of phantom GlobalFeed/PersonalFeed/UserFeed components that no longer exist, so future readers aren't misled. +- **Change:** Rewrote the FeedLayout component JSDoc and the getDid prop comment to reference profile-certs and project-detail; dropped the non-existent feed-source names. +- **Test:** refactor; no natural test — full suite green +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings + +### quality-009 — Add CI workflow (ci.yml) · IMPLEMENTED +- **Why it's an improvement:** Enforces the documented lint/tsc/test baseline on every PR into staging/main, so regressions Vercel's `next build` can't catch are now gated. +- **Change:** Added `.github/workflows/ci.yml` running `npm ci` → `npm run lint` → `npx tsc --noEmit` → `npm test` on pull_request into staging and main, with Node pinned to 20.9.0 (aligns with quality-018). +- **Test:** refactor; no natural test (new CI config file, testable=false) — full suite green +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings + +### quality-010 — tsconfig excludes test files from tsc · IMPLEMENTED +- **Why it's an improvement:** The main tsc gate never type-checked the 33 `__tests__` files, so tests could call renamed/removed exports and still report "0 errors"; a dedicated test typecheck makes those errors visible. +- **Change:** Added `tsconfig.test.json` (extends base, un-excludes `src/**/__tests__/**`) and a `typecheck:test` npm script; the main `tsconfig.json` exclude is left intact so `npx tsc --noEmit` stays the 0-error production gate. Per the coordination note, the new step surfaces 12 pre-existing test-file type errors (across 6 files) that quality-009's CI should triage before gating on it. +- **Test:** refactor; no natural test (config + npm script, testable=false) — full suite green +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings + +### quality-018 — No Node version pin (engines/.nvmrc) · IMPLEMENTED +- **Why it's an improvement:** Pins the Node runtime so contributors/CI match Vercel's runtime (Next 16 requires Node >=20.9.0), avoiding "works on my machine" build discrepancies. +- **Change:** Added `"engines": { "node": ">=20.9.0" }` to package.json and a `.nvmrc` pinned to `20.9.0`. +- **Test:** refactor; no natural test (config files, testable=false) — full suite green +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings + +### quality-019 — config-5: No `typecheck` npm script · IMPLEMENTED +- **Why it's an improvement:** The `tsc --noEmit` gate no longer depends on memory; it's an invokable, CI-wireable script. +- **Change:** Added `"typecheck": "tsc --noEmit"` to package.json scripts. +- **Test:** refactor; no natural test — full suite green. +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings + +### quality-020 — lint script `--ext` no-op under flat config · IMPLEMENTED +- **Why it's an improvement:** Removes a dead/misleading flag that does nothing under ESLint 9 flat config, where file selection is governed by `files`/`ignores`. +- **Change:** Changed the `lint` script in package.json from `eslint src/ --ext .ts,.tsx` to `eslint src/`. +- **Test:** refactor; no natural test — full suite green. +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings + +### quality-021 — lint scope is `src/` only; root configs/scripts unlinted · IMPLEMENTED +- **Why it's an improvement:** Root configs (next.config.ts, scripts/*.mjs, build configs) are now linted, not just `src/`. +- **Change:** `lint` script now runs `eslint .`; flat-config `ignores` widened to `**/.next/`, `**/node_modules/`, `**/coverage/`, `.claude/` (the new anonymous-default-export warning on the config was cleared by naming the export). +- **Test:** refactor; no natural test — full suite green. +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings + +### quality-022 — audit scripts disagree on dev-server port · IMPLEMENTED +- **Why it's an improvement:** all three audit scripts now point at the same default dev-server origin, so audit-screenshots no longer silently skips every route against a default `next dev` (:3000). +- **Change:** unified the port default to `process.env.BASE || "http://localhost:3000"` — fixed audit-screenshots' :3001 default, replaced recapture-landing's three hardcoded :3000 URLs with a BASE constant, and made capture-divergence-sheet honor BASE (file:// fallback preserved when unset). +- **Test:** refactor; no natural test (scripts are dev-only Playwright tooling, testable=false) — full suite green. +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings + +### quality-023 — .gitignore `core` pattern unanchored · IMPLEMENTED +- **Why it's an improvement:** Bare `core` ignored any file/dir named `core` at any depth; a future `src/lib/core/` or `core.ts` would be silently untracked. +- **Change:** Anchored the pattern to `/core` so it only matches the root coredump file. +- **Test:** refactor; no natural test — full suite green. +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings + +### quality-025 — saveWithSwap untyped cross-shape contract · IMPLEMENTED +- **Why it's an improvement:** Constrains `TDrafts extends Partial` so a draft key that collides with the snapshot under an incompatible type is a compile error instead of being silently excluded from conflict detection and auto-rebased over a concurrent server change. +- **Change:** Tightened the `TDrafts` generic bound and removed the `as Record` casts in the conflict-detection block; documented the `read().value`-shape invariant. +- **Test:** src/lib/atproto/__tests__/save-with-swap.test.ts — type-contract case fails before (TS2578 unused `@ts-expect-error` under tsconfig.test.json), passes after; runtime conflict/rebase cases lock behavior. +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings + +### quality-026 — unify the two divergent shallowEqual implementations · IMPLEMENTED +- **Why it's an improvement:** the dirty-set step and the conflict step in the same save flow now share one array-guarded comparator, so an array-vs-object edge can never be classified inconsistently between them. +- **Change:** exported the array-guarded `shallowEqual` from `swap-drafts.ts` and imported it into `save-with-swap.ts`, deleting that file's unguarded copy. +- **Test:** src/lib/atproto/__tests__/save-with-swap.test.ts — refactor (dedup); no natural red-before test since both copies already agreed on observable output, so the added case is a regression guard asserting cross-step agreement; full suite green. +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings + +### quality-027 — resolveCanonicalEndorsementDef treats missing createdAt as earliest · IMPLEMENTED +- **Why it's an improvement:** a malformed endorsement def lacking `createdAt` can no longer win canonical and schedule the well-formed defs for background deletion; the self-heal logic now keeps the real def. +- **Change:** the canonical sort now treats a missing/empty `createdAt` as latest (sorts to the END) instead of as `""` (earliest); exported the helper so it is directly testable. +- **Test:** src/lib/atproto/__tests__/badges.test.ts — added `resolveCanonicalEndorsementDef` cases; the two malformed-def cases fail before, pass after. +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings + +### quality-028 — location.ts uses shared strict parseAtUri · IMPLEMENTED +- **Why it's an improvement:** A single, tested at:// parser is used everywhere; malformed URIs with trailing segments are now rejected consistently instead of being silently accepted. +- **Change:** Deleted the private lenient `parseAtUri` (and its `ParsedAtUri` type) in location.ts and imported the shared strict parser from `@/lib/atproto/activity-uri`. +- **Test:** src/lib/atproto/__tests__/location.test.ts — fails before, passes after (4-segment uri → `readLocationStrongRef` returns null, no fetch). +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings + +### quality-029 — parseNotificationsPage trusts indexer node shape · IMPLEMENTED +- **Why it's an improvement:** A partial indexer edge no longer surfaces as a fully-typed Notification with `count: undefined`, which fed NaN row text and a `.includes` throw in NotificationRow. +- **Change:** Extended the malformed-edge guard to also require `count` (number), `latestRecordUri`, `latestRecordCid`, and `latestAuthor`; such edges are skipped with the existing console warning. +- **Test:** src/lib/atproto/__tests__/notifications.test.ts — fails before, passes after. +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings + +### quality-030 — resolveHandle returns alsoKnownAs verbatim · IMPLEMENTED +- **Why it's an improvement:** A non-handle (e.g. `example.com/some/path` from an attacker-controllable did:web doc) no longer leaks through as a handle. +- **Change:** After stripping `at://`, resolveHandle now rejects values that are empty, lack a dot, or contain a slash/whitespace, returning null instead. +- **Test:** src/lib/atproto/__tests__/resolve-handle.test.ts — fails before, passes after. +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings + +### quality-006 — useOrgProfile module-level cache · IMPLEMENTED +- **Why it's an improvement:** The hook is mounted in 4 layout components on every authenticated page; a shared cache + in-flight map collapses 3-4 concurrent identical fan-outs (incl. an off-app plc.directory resolve) per navigation into one. +- **Change:** Added a module-level bounded cache keyed by `activeOrg.groupDid` plus an in-flight `Map` (mirroring use-org-marker / use-author-info); state is seeded from cache and refetch() stays as a cache-evicting force path. Behavior-preserving for the only consumed output (`orgAvatarUrl`). +- **Test:** refactor; no natural test (testable=false) — full suite green. +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings + +### quality-037 — useOrgProfile.refetch exposes fetchData directly · SKIPPED +- **Reason:** Already fixed by quality-006 (commit fde4b18) — `refetch` is now a no-arg `useCallback(async (): Promise => …, [groupDid])` that calls `fetchOrgProfileData(groupDid)` only; the typed return is `() => Promise` and no MouseEvent can reach getOrgProfile's AbortSignal param. Applying the literal Recommendation would drop quality-006's cache-eviction/refresh-tick logic (a regression), so no production change is warranted; a test-first guard could not go red. + +### quality-031 — useHomeFeed INVALID_CURSOR recovery controller now tracked · IMPLEMENTED +- **Why it's an improvement:** Prevents a setState-after-unmount when the hook unmounts during an INVALID_CURSOR recovery reload. +- **Change:** Store the recovery `AbortController` in a ref that the effect cleanup aborts (aborting any prior recovery first), instead of an untracked local. +- **Test:** src/hooks/__tests__/use-home-feed-invalid-cursor.test.tsx — fails before, passes after. +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings + +### quality-032 — window-focus revalidation handlers now pass an abortable signal · IMPLEMENTED +- **Why it's an improvement:** A focus event near unmount could setState on an unmounted hook because the focus fetch carried no signal, so the existing `if (signal?.aborted)` guard was dead code. +- **Change:** Each focus handler (useReceivedEndorsements, useProfileResponses, useBlueskyFollows) now owns a ref'd AbortController aborted on next focus and on effect cleanup/unmount, and passes its signal into the fetch path. +- **Test:** src/hooks/__tests__/focus-revalidate-abort.test.tsx — fails before, passes after. +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings + +### quality-033 — useBskyPosts.loadMore can append a superseded page on handle change · BLOCKED +- **Reason:** Could not reproduce the race in a faithful failing test: on every handle change the effect synchronously clears the cursor (blocking a new loadMore via the `!cursor` guard) and calls fetchPage, bumping the shared requestIdRef past any in-flight loadMore, so a stale loadMore's fetchPage returns null and never appends — both interleavings probed stay green. Per the LOW-CONFIDENCE coordination note, blocking rather than making a speculative change. + +### quality-034 — useEndorsementLists mutation callbacks read stale lists via closure · IMPLEMENTED +- **Why it's an improvement:** A concurrent refetch landing during a mutation's await is no longer clobbered by an optimistic merge built from a pre-await snapshot. +- **Change:** Added a `listsRef` mirror; mutation callbacks now read the current list from `listsRef.current` and build update/remove merges off the live state inside the functional updater, and `lists` was dropped from their deps. +- **Test:** src/hooks/__tests__/use-endorsement-lists-stale-closure.test.tsx — fails before, passes after. +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings + +### quality-035 — useExploreData loadMore passes signal:null · IMPLEMENTED +- **Why it's an improvement:** A filter change now cancels the in-flight loadMore page instead of fetching-then-discarding it, saving wasted network on this network-heavy route. +- **Change:** loadMore now passes the current generation's AbortController signal into loadPage (via a controllerRef the initial-fetch effect populates) instead of `null`; the effect cleanup's existing abort cancels it. +- **Test:** refactor; no natural test (testable: false) — full suite green. +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings + +### quality-036 — Promoted object-URL preview never revoked after save · IMPLEMENTED +- **Why it's an improvement:** The avatar/banner blob: preview promoted into the local mirror on save no longer leaks for the page lifetime; it is revoked once the canonical CDN URL catches up or on unmount. +- **Change:** Track promoted localAvatarUrl/localBannerUrl in refs via coordinated tracked setters; clear+revoke the mirror in effects keyed on the avatarUrl/bannerUrl props (refetch caught up) and revoke any held blob URL on unmount. +- **Test:** refactor; no natural test (testable: false) — full suite green. +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings + +### quality-038 — usePendingAwardsCount returns 0 not null when logged out · IMPLEMENTED +- **Why it's an improvement:** Aligns the hook with its JSDoc contract so a future consumer that distinguishes null (hide) from 0 (loaded-but-empty) won't misbehave. +- **Change:** The logged-out / no-did early return now yields `null` instead of `0`, matching the cold-cache and loading branches. +- **Test:** src/hooks/__tests__/use-pending-awards-count-logged-out.test.tsx — fails before, passes after. +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings + +### quality-039 — useBottomSheetDrag visualViewport styles not reset on cleanup · IMPLEMENTED +- **Why it's an improvement:** A reused/reopened sheet node no longer flashes a stale clamped maxHeight/bottom until the next resize event. +- **Change:** The visualViewport effect cleanup now resets `maxHeight=''` and `bottom=''` (capturing the node in a local to satisfy exhaustive-deps). +- **Test:** refactor; no natural test (testable=no) — full suite green. +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings + +### quality-040 — useBottomSheetDrag dismiss timeout can fire after unmount · IMPLEMENTED +- **Why it's an improvement:** Prevents the drag-to-dismiss onClose callback from firing 250ms after the sheet unmounts, removing a sharp edge for non-state callers. +- **Change:** Store the dismiss setTimeout id in a `dismissTimeout` ref and clearTimeout it in an unmount cleanup effect. +- **Test:** refactor; no natural test (testable=false) — full suite green. +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings + +### quality-041 — useOrgMarker.refresh stale value during concurrent fetch · IMPLEMENTED +- **Why it's an improvement:** A post-save refresh on a page with a concurrent in-flight mount can no longer re-read the pre-save record. +- **Change:** On refresh (refreshTick > 0) the effect now also `inFlight.delete(did)` alongside `cache.delete(did)`, so the stale in-flight promise can't satisfy the refetch. +- **Test:** src/hooks/__tests__/use-org-marker-refresh-inflight.test.tsx — fails before, passes after. +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings + +### quality-012 — Extract shared PersonCard + name-cache hook · IMPLEMENTED +- **Why it's an improvement:** removes a copy-pasted PersonCard and two duplicate module-scoped name caches across the profile endorsements/followers tabs, leaving one source of truth. +- **Change:** extracted the superset PersonCard into `src/components/profile/person-card.tsx` (note?/listTitle? optional) and the duplicated `useAuthorNamesMap` batch hook into `src/hooks/use-author-names-map.ts`; both profile tabs now import the shared versions (pure, behavior-preserving). +- **Test:** `src/components/profile/__tests__/person-card.test.tsx` — fails before (no module), passes after; asserts name/handle/date rows match with and without the optional note/list rows. +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings + +### quality-043 — ProjectItemRow permanent skeleton on load failure · IMPLEMENTED +- **Why it's an improvement:** a failed/404 project no longer shows an eternal grey skeleton; viewers get a terminal fallback row and owners can still remove the dangling reference. +- **Change:** gated the skeleton on `isLoading`; when `!project && !isLoading`, render a fallback `ItemRowShell` (URI rkey tail or "Project unavailable", remove button preserved for owners). +- **Test:** src/components/profile/__tests__/project-item-row.test.tsx — fails before, passes after. +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings + +### quality-044 — replace hand-rolled outside-click/Escape with useClickOutsideClose · IMPLEMENTED +- **Why it's an improvement:** Removes three duplicated mousedown/keydown effects in favor of the shared hook, gaining its onClose-ref optimization (no listener re-attach on re-render). +- **Change:** Replaced the inline outside-click + Escape effects in profile-endorsements.tsx (sort + filter dropdowns) and endorsement-lists.tsx (sort dropdown) with `useClickOutsideClose` anchored on each existing `*__sort-wrap` div. +- **Test:** refactor; no natural test — full suite green. +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings + +### quality-045 — three near-identical CreateListModal / bulk-paste modals duplicated · BLOCKED +- **Reason:** escalate-to-judgment — the two CreateListModals diverge in user-visible behavior (focus+select vs focus, silent-return vs "Title is required", maxLength 120/500 vs 256/1000, separate CSS class families) and the two paste modals have entirely different state machines + action rows (PasteProgress vs showCloseOnly/showTryAgain), so a single shared dialog/shell cannot be a pure behavior-preserving extraction; it would either change behavior on one surface or require a large flag surface that doesn't reduce complexity. + +### quality-046 — createdAt comparators never return 0 (unstable sort) · IMPLEMENTED +- **Why it's an improvement:** Same-timestamp lists no longer shuffle their relative order between renders — the createdAt sort is now stable. +- **Change:** Routed `sortLists`' created-desc / created-asc comparators in endorsement-lists.tsx through a three-way `compareString` (returns 0 on equality), matching profile-endorsements.tsx; exported `sortLists` for unit testing. +- **Test:** src/components/profile/__tests__/endorsement-lists-sort.test.ts — fails before (sortLists not exported), passes after. +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings + +### quality-047 — Raw at-URI used as the accessible label for item checkboxes · IMPLEMENTED +- **Why it's an improvement:** Screen readers no longer announce the full DID+collection+rkey at-URI for each select checkbox; they get a concise positional label instead. +- **Change:** In profile-lists.tsx the item checkbox `aria-label` now uses the 1-based row index (`Select item ${index + 1}`) rather than the raw at-URI. +- **Test:** refactor; no natural test (testable=false) — full suite green. +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings + +### quality-048 — sidebar EndorseButton snaps back to Endorse on list-append failure · IMPLEMENTED +- **Why it's an improvement:** A failed list-append no longer rolls back the already-persisted award's optimistic "Endorsed" state, removing a duplicate-endorsement nudge. +- **Change:** Extracted the reason-confirm orchestration into `runEndorseReasonConfirm`, which refetches the given set before the list-append and, on append failure, keeps optimistic=true while surfacing only the attribution error. +- **Test:** src/components/profile/__tests__/endorse-reason-confirm.test.ts — fails before, passes after. +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings + +### quality-007 — Security-critical sanitize.ts has no unit tests · IMPLEMENTED +- **Why it's an improvement:** Locks in the pinned invisible-char regex and whitespace/@/lowercase rules at the login/feedback boundary, so a future regex edit that narrows the allowlisted code-point ranges fails CI instead of silently regressing. +- **Change:** Added a co-located test file for `stripInvisible`/`sanitizeEmail`/`sanitizeHandle`; no production change. +- **Test:** src/lib/utils/__tests__/sanitize.test.ts — pure test addition (no defect to fix); asserts current correct behavior, full suite green. +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings + +### quality-014 — search-actors logs raw error via logSafe · IMPLEMENTED +- **Why it's an improvement:** Adopts the repo-wide logSafe convention so the catch-all error log is redaction-safe and won't leak if the upstream becomes authenticated. +- **Change:** Replaced `console.error("[search-actors]", err)` with `logSafe("[search-actors] upstream error", err)` and added the import. +- **Test:** refactor; no natural test — full suite green +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings + +### quality-015 — notifications upstream error log includes up to 500 chars of the response body · IMPLEMENTED +- **Why it's an improvement:** Stops writing the user's own notification data (DIDs, record URIs) into server logs — removes a PII-in-logs smell. +- **Change:** Changed the non-2xx `console.warn` to log only `upstream.status`, dropping the `responseBody.slice(0, 500)` argument. +- **Test:** refactor; no natural test (testable: no) — full suite green +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings + +### quality-016 — xrpcError clamps upstream status to valid HTTP range · IMPLEMENTED +- **Why it's an improvement:** An out-of-range upstream status (0, -1, 1000, non-integer) no longer throws a RangeError inside the terminal catch and collapses to a clean masked 500 instead of an opaque framework 500. +- **Change:** In `xrpcError` (route.ts), clamp the resolved status inline — `Number.isInteger(s) && s>=200 && s<=599 ? s : 500` — before it reaches `NextResponse.json(..., { status })`. +- **Test:** src/app/api/xrpc/[...method]/__tests__/xrpc-error.test.ts — new case "clamps out-of-range upstream statuses to 500 (quality-016)"; fails before, passes after. +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings + +### quality-017 — indexer body-size cap measures UTF-16 length not bytes · IMPLEMENTED +- **Why it's an improvement:** The documented 32KB body cap is now enforced in bytes, so a multi-byte body can no longer slip ~3x the byte limit past the post-read check. +- **Change:** Replaced `text.length > MAX_BODY_SIZE` with `Buffer.byteLength(text, "utf8") > MAX_BODY_SIZE` in the indexer route's post-read size check. +- **Test:** src/app/api/indexer/__tests__/route.test.ts — new case posts >32KB of multi-byte chars with no Content-Length and asserts 413; fails before (200), passes after. +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings + +### quality-008 — Popover content uses --z-popover token, not z-[40] · IMPLEMENTED +- **Why it's an improvement:** Restores token-driven stacking so re-tiering the z-map can't silently break every popover consumer (CLAUDE.md hard rule 5). +- **Change:** Swapped the hardcoded `z-[40]` for `z-[var(--z-popover)]` in `PopoverContent`'s className. +- **Test:** refactor; no natural test — full suite green. +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings + +### quality-013 — onboarding step-profile creates object URLs in render without revoking · IMPLEMENTED +- **Why it's an improvement:** Stops StepProfile leaking a fresh avatar+banner blob URL on every re-render (e.g. each keystroke); blob URLs are now revoked on change/unmount. +- **Change:** Moved `URL.createObjectURL` for replacement avatar/banner into `useMemo` keyed on the File and added cleanup effects that revoke the previous URL, mirroring use-profile-inline-edit / avatar-upload. +- **Test:** refactor; no natural test (testable=false) — full suite green. +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings + +### quality-052 — ActivityCard never resets imageFailed when imageUrl changes · IMPLEMENTED +- **Why it's an improvement:** A reused ActivityCard whose record/image URL mutates in place (no remount) no longer stays stuck on the placeholder after a prior load failure; the new URL is retried. +- **Change:** Added `useEffect(() => setImageFailed(false), [imageUrl])`, mirroring the existing reset-on-dep-change effect in ActivityDetail/ProjectDetail. +- **Test:** src/components/feed/__tests__/activity-card-image-reset.test.tsx — fails before, passes after. +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings + +### quality-055 — ActivityCard not memoized; loadMore re-renders all cards · IMPLEMENTED +- **Why it's an improvement:** loadMore swaps the activities array identity, so prior cards re-rendered needlessly; memoizing lets unchanged cards bail out on long lists. +- **Change:** Wrapped the default-exported ActivityCard in React.memo (props are stable per URI). +- **Test:** refactor; no natural test — full suite green. +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings + +### quality-053 — News post images use array index as React key · IMPLEMENTED +- **Why it's an improvement:** Index keys break reconciliation on reorder/filter; keying by the unique CDN URL gives each image a stable identity. +- **Change:** Keyed the sliced news image map by `img.thumb` instead of the array index in news-section.tsx. +- **Test:** refactor; no natural test (testable=false) — full suite green. +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings + +### quality-054 — expanded locations map height now viewport-relative · IMPLEMENTED +- **Why it's an improvement:** Modal map now tracks resize/rotation instead of a stale one-shot window.innerHeight read. +- **Change:** Drive `.cert-detail__map--modal` height from CSS `min(720px, 70vh)` and pass `height="100%"` to the inner Map (removes the JS innerHeight calc). +- **Test:** refactor; no natural test (testable=no) — full suite green. +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings + +### quality-056-auth-redirect-1 — safeRedirect http: gate matches module's !=="production" · IMPLEMENTED +- **Why it's an improvement:** http loopback redirects no longer fail-closed under NODE_ENV="test"; gate now matches the rest of the module (still https-only in prod). +- **Change:** `safeRedirect` http: allowance changed from `NODE_ENV === "development"` to `NODE_ENV !== "production"` (and aligned the doc comment). +- **Test:** src/lib/auth/__tests__/safe-redirect-test-env.test.tsx — fails before, passes after. +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings + +### quality-056-auth-session-fixation-1 — document why swallowed deleteSession is safe · IMPLEMENTED +- **Why it's an improvement:** Clarifies that the swallowed deleteSession failure is not a session-fixation hole, so a future reader doesn't "harden" it back into failing sign-in. +- **Change:** Added a one-line comment in callback-handler/route.ts noting createSession overwrites the cookie, so a failed deleteSession can only orphan a TTL'd Redis key. +- **Test:** refactor; no natural test (comment-only) — full suite green. +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings + +### quality-056-auth-csrf-doc-1 — CSRF missing-Origin-AND-Referer divergence from §8 · IMPLEMENTED +- **Why it's an improvement:** Records that rejecting requests missing both Origin and Referer is a deliberate fail-closed choice, so it isn't "fixed" back to the laxer Origin-only behavior AGENTS §8 describes. +- **Change:** Added a comment in csrf.ts above the `!rawOrigin` guard explaining the stricter behavior; no behavior change. +- **Test:** refactor; no natural test (comment-only) — full suite green. +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings + +### quality-056-api-trust-5 — AGENTS allowlist count: "four" → "eleven" · IMPLEMENTED +- **Why it's an improvement:** Removes doc drift — §17.9 said "the four" `ALLOWED_WRITE_COLLECTIONS` but the array in the XRPC route holds 11, so the security-rules doc no longer understates the write surface. +- **Change:** AGENTS.md §17.9 wording updated from "the four" to "the eleven"; code array untouched (`app.certified.badge.response` write-enablement confirmed intentional per the array's inline comment). §22.5 cross-reference has no count, so left as-is. +- **Test:** doc-only (testable=false); AGENTS.md isn't linted/typechecked — full suite green. +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings + +### quality-056-cert-5 — Extract duplicated countGraphemes to src/lib/utils · IMPLEMENTED +- **Why it's an improvement:** Removes a verbatim 9-line grapheme counter copied across 4 form pages — one canonical helper means a single place to fix or extend the character-counting logic. +- **Change:** Added `src/lib/utils/graphemes.ts` exporting `countGraphemes` (verbatim body) and imported it in create, project/new, project edit, and activity edit pages, deleting each local `useCallback` copy. Behavior-preserving. +- **Test:** src/lib/utils/__tests__/graphemes.test.ts — fails before (module missing), passes after; asserts ASCII / multi-code-unit emoji / ZWJ sequence / combining-mark single-grapheme counting. +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings + +### quality-056-cert-6 — Extract own-certs quick-pick fetch into useOwnCerts · IMPLEMENTED +- **Why it's an improvement:** Removes a ~55-line listRecords fetch effect copied verbatim between /project/new and the project edit page — one shared hook means the quick-pick fetch (collection, sort, best-effort error handling, abort) lives in a single place. +- **Change:** Added `src/hooks/use-own-certs.ts` exporting `useOwnCerts(did)` (reads `activeOrg` via useOrg internally, same source-DID logic), and replaced the local ownCerts state + fetch useEffect in both project pages with the hook call. Behavior-preserving. +- **Test:** refactor; no natural test (testable=false) — full suite green. +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings + +### quality-056-cert-7 — Document activity-only meta reads in project-detail · IMPLEMENTED +- **Why it's an improvement:** Prevents a future "dead read" cleanup from silently deleting startDate/endDate/contributors handling that intentionally tolerates legacy and foreign activity-shaped records. +- **Change:** Added a clarifying comment in `project-detail.tsx` above the startDate/endDate reads noting these (and contributors) are activity-only meta the project forms never write, kept to render legacy/foreign records. Comment-only, behavior-preserving. +- **Test:** doc/comment-only (testable=false); no natural test — full suite green. +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings + +### quality-056-map-3 — Drop dead theme reactivity in map tiles · IMPLEMENTED +- **Why it's an improvement:** Removes a no-op `useTheme` subscription and URL-keyed remount logic that implied theme-reactive tiles, even though the Esri satellite tile URLs are theme-invariant — eliminating a misleading comment and an unnecessary render dependency. +- **Change:** In `map.tsx`, removed the `useTheme` import + `resolvedTheme` read, renamed `ThemeReactiveTiles` to `BaseTiles`, dropped the `key={config.url}`/`key={overlay.url}` props (keys never changed), and rewrote the misleading "swaps its URL when the theme changes" comments to state the satellite raster is theme-invariant. Behavior-preserving. +- **Test:** refactor; no natural test (testable=false) — full suite green. +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings + +### quality-056-map-4 — Delete dead forwardGeocode export · IMPLEMENTED +- **Why it's an improvement:** Removes a dead single-hit export with zero importers (callers use suggestForwardGeocode), shrinking the surface and ending stale doc-drift. +- **Change:** Deleted the unused `forwardGeocode` function from src/lib/locations/geocode.ts and dropped the now-stale "legacy forwardGeocode stays available" line from the suggestForwardGeocode doc comment. +- **Test:** refactor; no natural test (testable=false) — full suite green. +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings (pre-existing baseline error in onboarding-context.tsx, untouched) + +### quality-056-map-5 — Raw hex in map polygon pathOptions + #000 in leaflet.css · IMPLEMENTED +- **Why it's an improvement:** Removes a raw-hex token violation (CLAUDE.md rule 2) from leaflet.css and documents the unavoidable JS-literal exception so it isn't "fixed" incorrectly later. +- **Change:** Added invariant `--media-letterbox` token in tokens.css and pointed `.leaflet-doc__embed` background at it; left the Leaflet `pathOptions` hex literals (Leaflet can't read CSS vars from JS) with a justifying comment. +- **Test:** refactor; no natural test (testable=false) — full suite green. +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings (pre-existing baseline error in onboarding-context.tsx, untouched) + +### quality-056-ui-primitives-2 — ProviderRedirectOverlay hardcodes zIndex:9999 · IMPLEMENTED +- **Why it's an improvement:** Removes a literal z-index (CLAUDE.md rule 5) and ties the overlay's stacking to the shared scale, so future z-index reshuffles stay coherent. +- **Change:** Replaced `zIndex: 9999` with `zIndex: "var(--z-skip-nav)"` in provider-redirect-overlay.tsx (9999 is exactly the `--z-skip-nav` token value — behavior-preserving). +- **Test:** refactor; no natural test (testable=false) — full suite green. +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings (pre-existing baseline error in onboarding-context.tsx, untouched) + +### quality-056-ui-primitives-3 — Skeleton text variant aria-hidden + honors width · IMPLEMENTED +- **Why it's an improvement:** Decorative text-skeleton lines are now hidden from assistive tech and the documented `width` prop is no longer silently ignored. +- **Change:** Added `aria-hidden` to the text-variant container, honored `width` on non-last lines (last stays 60%), and moved `...style` before the computed width so width isn't overridden. +- **Test:** src/components/ui/__tests__/skeleton.test.tsx — fails before, passes after +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings (1 pre-existing error in onboarding-context.tsx, unrelated) + +### quality-056-ui-primitives-4 — Button icon-loading spinner + default type · IMPLEMENTED +- **Why it's an improvement:** Prevents spinner/icon overlap in the 40x40 icon slot, and stops Buttons from implicitly submitting forms. +- **Change:** Hide children when `loading && size==="icon"` (spinner only); add default `type="button"` (overridable via prop). +- **Test:** src/components/ui/__tests__/button.test.tsx — fails before, passes after +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings (1 pre-existing error in onboarding-context.tsx, untouched) + +### quality-056-explore-1 — Delete dead ?attrs= filter in explore · IMPLEMENTED +- **Why it's an improvement:** Removes read+applied filter logic that no UI ever writes (the `attrs` Set is always empty), so the code carried unreachable branches and a misleading comment. +- **Change:** Deleted the `?attrs=` URL read (explore.tsx ~227-232), the `attrs` prop on `ResultsArea` (pass + param + type), and the six `attrs.has(...)` filter branches across accounts/projects/certs; left `degrees` filtering and sorting untouched. +- **Test:** refactor; no natural test (filter never active today) — full suite green +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings (1 pre-existing error in untouched onboarding-context.tsx, unchanged by this commit) + +### quality-056-explore-3 — Sort + quality popover triggers expose aria-haspopup · IMPLEMENTED +- **Why it's an improvement:** Screen readers now learn the Sort and quality-filter buttons open a popup, matching the sibling sub-prefix dropdown trigger (and home-feed menus) that already announce one. +- **Change:** Added `aria-haspopup="menu"` to the Sort trigger (explore.tsx ~671) and the quality-filter trigger (~726), the only two popover triggers in the file that lacked it. +- **Test:** src/components/explore-page/__tests__/explore-popover-haspopup.test.tsx — fails before, passes after +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings (1 pre-existing error in untouched onboarding-context.tsx, unchanged by this commit) + +### quality-056-groups-3 — Leave Group modal now uses ConfirmDialog · IMPLEMENTED +- **Why it's an improvement:** Routes the Leave-Group modal through the shared ``/`` (CLAUDE.md hard rule 7 / §22.16), gaining native `` Esc-close, scroll-lock, and focus restore instead of a hand-rolled `signin-modal__backdrop`. +- **Change:** Replaced the bespoke backdrop div in `src/app/groups/page.tsx` with `` (preserving the Leave action, destructive variant, in-flight gating, and warning copy); dropped the now-unused `LogOut`/`Button` imports. +- **Test:** src/app/groups/__tests__/leave-group-confirm-dialog.test.tsx — fails before (modal was a `div role="dialog"`, no `alertdialog`/``), passes after. +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings (1 pre-existing error in untouched safe-redirect-test-env.test.tsx, unchanged by this commit) + +### quality-056-groups-4 — Delete dead AddOrgModal + MembershipSyncModal · IMPLEMENTED +- **Why it's an improvement:** Removes two unreferenced (0-importer) components, shrinking the surface; this also resolves groups-5 since the invariant-token inline styles (`--color-primary`/`--color-mid-gray` on an app surface) vanish with the deleted AddOrgModal file. +- **Change:** Deleted `src/components/groups/add-org-modal.tsx` and `src/components/groups/membership-sync-modal.tsx` (confirmed 0 external importers via grep for AddOrgModal/MembershipSyncModal/MembershipChange). +- **Test:** refactor (dead-code deletion); no natural test — full suite green. +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings (1 pre-existing error in untouched safe-redirect-test-env.test.tsx, unchanged by this commit) + +### quality-056-groups-5 — AddOrgModal invariant-token inline styles · SKIPPED +- **Reason:** resolved by quality-056-groups-4 — file deleted + +### quality-056-auth-redirect-1 (follow-up) — clear lint error in the new safe-redirect test · IMPLEMENTED +- **Why it's an improvement:** Restores the 0-errors lint gate. The original test for this item introduced a React-Compiler lint error (`Cannot reassign variables declared outside of the component/hook`) by reassigning a module-level `captured` var inside a render component; the wave's agents misattributed it as pre-existing and committed through it (lint went 69→70 with 1 error). +- **Change:** Rewrote `src/lib/auth/__tests__/safe-redirect-test-env.test.tsx` to use `renderHook(() => useAuth(), { wrapper })` instead of a module-level-reassigning `Capture` component. Same assertions (http: redirect permitted under NODE_ENV=test, no error). +- **Test:** the test itself (unchanged assertions, still green). +- **Gate:** vitest green · tsc 0 errors · lint 0 errors / 69 warnings (regression cleared). + +### quality-056-groups-6 — add-members partial failure leaves added members staged · IMPLEMENTED +- **Why it's an improvement:** A retry after a partial add-members failure no longer re-adds members the group service already accepted. +- **Change:** `handleAddMembers` now catches per-iteration and re-stages only the failing member onward via a new exported `remainingAfterAddIndex` helper (org-settings.tsx). +- **Test:** src/components/groups/__tests__/org-settings-pending.test.ts — fails before, passes after. +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings + +### quality-056-profile-edit-2 — BannerUpload onRemove dead in edit flow · SKIPPED +- **Reason:** Take-the-DROP-path coordination note says SKIP if `onRemove` is now actually used. It is: `BannerUpload.onRemove` is wired live through the inline-edit flow — profile/[handle]/page.tsx:375 passes `onBannerRemove={handleRemoveBanner}` -> profile-overview.tsx:164 `onRemove={onBannerRemove}` -> banner-upload.tsx:138-149 renders the working "Remove" pill. Only the dedicated edit-profile route (profile-edit-form.tsx:344-348) omits it; the prop/branch is not dead on the component. Dropping it would delete the shipping in-place banner-remove feature (behavior regression), not remove dead code. + +### quality-056-profile-edit-3 — edit-form inputs link to their error text · IMPLEMENTED +- **Why it's an improvement:** screen readers now announce the validation error tied to each profile-edit field instead of leaving the error `

` orphaned from its control. +- **Change:** gave each error `

` a matching id and set `aria-describedby` on the raw display-name/pronouns/bio/website inputs and the org-URL row inputs (website points at its help text when valid); no migration to the Input/Textarea primitives. +- **Test:** src/components/profile/__tests__/profile-edit-form-aria-describedby.test.tsx — fails before, passes after. +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings + +### quality-056-profile-edit-5 — BannerUpload hasPending write-once boolean can desync · IMPLEMENTED +- **Why it's an improvement:** After the inline-edit Remove clears the banner, BannerUpload no longer keeps showing the stale picked preview or a stuck "Replace banner" label — the displayed image is now the single source of truth. +- **Change:** Dropped the write-once `hasPending` state; the button label derives from `hasImage`, and a new effect releases the local preview when the parent's `currentBannerUrl` changes (e.g. Remove → null), mirroring the inline-edit hook's quality-036 mirror-clear. bug-009 self-preview and profile-edit-2's Remove pill are preserved. +- **Test:** src/components/profile/__tests__/banner-upload-pending-desync.test.tsx — fails before, passes after. +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings + +### quality-056-profile-edit-4 — avatar overlay raw colors → tokens · IMPLEMENTED +- **Why it's an improvement:** The avatar hover overlay no longer pins raw `bg-black`/`bg-opacity-50`/`text-white` (CLAUDE.md hard rule 2), so the dark scrim and light icon now flow from theme tokens that adapt in dark mode like the sibling image-edit overlay. +- **Change:** Replaced `bg-black bg-opacity-50` with `bg-[var(--navy-overlay-70)]` (the canonical dark-scrim token used for modal/image overlays) and `text-white` with `text-[var(--color-white)]` on the Camera icon in avatar-upload.tsx; dark-translucent-overlay-with-light-icon parity preserved. +- **Test:** refactor; no natural test — full suite green. +- **Gate:** vitest green · tsc 0 errors · lint 69 warnings + +### quality-056-landing-deadcode-1 — delete dead orbiting-logos component · IMPLEMENTED +- **Why it's an improvement:** Removes 354 lines of unreferenced dead code whose embedded raw rgba/box-shadow/zIndex (token-rule violations) vanish with the file. +- **Change:** Deleted src/components/landing/orbiting-logos.tsx after confirming 0 importers (grep for OrbitingLogos / orbiting-logos). +- **Test:** refactor; no natural test — full suite green. +- **Gate:** vitest green · tsc 0 errors · lint 67 warnings + +### quality-056-landing-darkmode-1 — legal/marketing pages use theme-aware link/text tokens · IMPLEMENTED +- **Why it's an improvement:** raw `text-blue-600/-800` and `text-gray-500` are pinned light-mode colors that don't flip, so links and the "Last updated" line rendered low-contrast/wrong in dark mode (CLAUDE.md rules 2/9). +- **Change:** in about/terms/privacy/dsa/imprint, replaced link classes with `text-[var(--color-accent)] hover:text-[var(--color-accent-hover)]` and the muted line with `text-[var(--fg-muted)]`, and dropped the inert `prose-navy` class. +- **Test:** refactor; no natural test (testable=false) — full suite green. +- **Gate:** vitest green · tsc 0 errors · lint 67 warnings + +### quality-056-approot-global-error-tokens — global-error.tsx radius 6px → 2px + justifying comment · IMPLEMENTED +- **Why it's an improvement:** Brings the root error boundary's radius into visual parity with `var(--radius)` (2px) and documents why tokens are intentionally inlined there. +- **Change:** Changed both `borderRadius: "6px"` to `"2px"` in global-error.tsx and added a comment noting tokens (and the CSS-token stylesheet) are unavailable in the root error boundary, justifying the inline raw hex. +- **Test:** refactor; no natural test (testable=false; root error boundary, behavior-preserving) — full suite green +- **Gate:** vitest green · tsc 0 errors · lint 67 warnings + +### quality-056-approot-agents-dep-version-drift — AGENTS §2 stale dependency versions · IMPLEMENTED +- **Why it's an improvement:** AGENTS §2 tech-stack table now matches package.json, so agents reading it don't act on stale/missing dependency facts. +- **Change:** Corrected `@atproto/api` 0.19→0.13 and added Theming (next-themes), Rich text (tiptap) and Maps (leaflet/react-leaflet) rows to the §2 table. +- **Test:** refactor; no natural test (doc-only, AGENTS.md is not linted/typechecked) — full suite green. +- **Gate:** vitest green · tsc 0 errors · lint 67 warnings + +### quality-056-authz-repo-3 — role allowlists derived from one ORG_ROLES source · IMPLEMENTED +- **Why it's an improvement:** the members-POST {member,admin} and role-PUT {member,admin,owner} allowlists were two bare arrays in separate route files that could silently drift; both now derive from a single source of truth. +- **Change:** added `ORG_ROLES` + derived `ORG_ASSIGNABLE_ROLES` (= ORG_ROLES minus owner) and `isOrgRole`/`isAssignableRole` guards to `src/lib/groups/constants.ts`; members/route.ts uses `isAssignableRole`, role/route.ts uses `isOrgRole`. Exact sets preserved (members POST excludes owner). +- **Test:** src/lib/groups/__tests__/roles.test.ts — fails before, passes after. +- **Gate:** vitest green · tsc 0 errors · lint 67 warnings + +### quality-056-authz-repo-4 — register org-limit member walk early-exit · IMPLEMENTED +- **Why it's an improvement:** the org-creation-limit check is a per-group existence test (caller's own self-added entry), so paginating every member page of every group was wasted work that scaled with group size — an availability/perf smell against large groups. +- **Change:** in `src/app/api/groups/register/route.ts` the inner member-list walk now returns as soon as a `did === ownerDid && addedBy === ownerDid` entry is found, instead of buffering all members and `.some(...)` after exhausting every page. Fail-closed-on-CGS-error 503 and the outer batch early-exit are unchanged. +- **Test:** src/app/api/groups/register/__tests__/org-limit-member-walk.test.ts — fails before (member.list called twice), passes after (once). +- **Gate:** vitest green · tsc 0 errors · lint 67 warnings + +### quality-056-notif-row-1 — notification-row dedupes truncateDid + getInitials · IMPLEMENTED +- **Why it's an improvement:** Removes a drifted local copy of truncateDid and a hand-rolled initials helper in favor of the shared utils, ending divergence (the local copy showed only 4 trailing DID chars vs the shared 6). +- **Change:** Added a canonical `truncateDid` to `src/lib/utils/did.ts` and switched notification-row to it plus `getInitials` from `src/lib/utils/initials.ts`, deleting the local truncateDid and the inline `fallbackInitials` expression. +- **Test:** refactor; no natural test — full suite green (425 tests). +- **Gate:** vitest green · tsc 0 errors · lint 67 warnings + +### quality-056-deadcode-4 — dead exports in labeller.ts · IMPLEMENTED +- **Why it's an improvement:** removes unused vocabulary/helpers so the constants-only module reflects its actual surface; less to read and maintain. +- **Change:** deleted `pickKnownLabel`, `DEFAULT_SELECTED_FILTERS`, `FilterValue`, and `ALL_LABELS` from `src/lib/atproto/labeller.ts` (0 importers each; `ALL_LABELS` was only used by `pickKnownLabel`). Kept `LabelValue` and `LABEL_DISPLAY` (still imported). +- **Test:** refactor; no natural test — full suite green. +- **Gate:** vitest green · tsc 0 errors · lint 67 warnings + +### quality-056-deadcode-6 — delete dead exports (getBlobRefLinkFromBlob / clearRecentlyViewed / ORG_PROFILE_COLLECTION) · IMPLEMENTED +- **Why it's an improvement:** removes three unreferenced exports, shrinking the public surface and dead code. +- **Change:** deleted `getBlobRefLinkFromBlob` (types.ts), `clearRecentlyViewed` (recently-viewed.ts), and `ORG_PROFILE_COLLECTION` (groups/constants.ts) after confirming 0 importers each. +- **Test:** refactor; no natural test — full suite green. +- **Gate:** vitest green · tsc 0 errors · lint 67 warnings + +### quality-056-utils-tests-2 — did.ts validators untested · IMPLEMENTED +- **Why it's an improvement:** Locks in the behavior of `isValidDid`/`isDid` that gate ~10 security-sensitive route guards, so future edits to the DID regex/prefix check can't silently regress. +- **Change:** Added a co-located test file for `src/lib/utils/did.ts`; no production change. +- **Test:** src/lib/utils/__tests__/did.test.ts — pure test addition (no defect to fix), passes against current behavior; full suite green. +- **Gate:** vitest green · tsc 0 errors · lint 67 warnings + +### quality-056-utils-tests-3 — bounded-cache / format-date / ip / recently-viewed tests · IMPLEMENTED +- **Why it's an improvement:** Locks in the branchy/security-relevant behavior of four previously-untested utils (cache eviction/cap, UTC date formatting, trusted-IP header parsing, recently-viewed dedupe/cap) so regressions are caught. +- **Change:** Added four co-located test files; no production change. clientIp confirmed in ip.ts; clearRecentlyViewed already removed by deadcode-6, so only the remaining exports are tested. +- **Test:** src/lib/utils/__tests__/{bounded-cache,format-date,ip,recently-viewed}.test.ts — pure test addition (no defect to fix), 35 cases pass against current behavior; full suite green. +- **Gate:** vitest green · tsc 0 errors · lint 67 warnings + + diff --git a/REVIEW.md b/REVIEW.md new file mode 100644 index 00000000..e818d4f6 --- /dev/null +++ b/REVIEW.md @@ -0,0 +1,819 @@ +# Overnight Review — certified-app (branch overnight-review) + +This review consolidates an 18-module + 6-lens pass (90 raw findings), each independently verified. After merging duplicates (OG-image 404, context-update XSS, safeRedirect NODE_ENV drift, dead `forwardGeocode`) and dropping the items a verifier refuted, **86 distinct findings remain**: 11 bugs, 9 risks, 56 quality items, and 10 judgment calls held for you. The highest-signal cluster is a handful of confirmed user-facing defects: a stored-XSS sink in context-update attachments (no `safeHttpUrl` scheme gate), broken OG/Twitter share images on the landing + four legal pages, a group-write conflict path where `InvalidSwap` detection is dead (silent edit loss), avatar/banner uploads that can be silently dropped on Save, and a project `location` written as a strongRef but read as a string so it never renders. Most remaining items are consolidation/dead-code/token-compliance hygiene plus a large untested-utility surface and a fully-absent CI gate. Two refuted items were dropped after verification. Many low-stakes findings carry verifier verdict "unverified (deferred to the Phase-2 test gate)"; they are kept because they are cheap, test-gated, and low-risk to implement. + +## Counts + +| Tag | Critical | High | Medium | Low | Total | +|---|---|---|---|---|---| +| bug | 0 | 3 | 4 | 4 | 11 | +| risk | 0 | 1 | 3 | 5 | 9 | +| quality | 0 | 0 | 9 | 47 | 56 | +| judgment | 0 | 0 | 4 | 6 | 10 | +| **Total** | **0** | **4** | **20** | **62** | **86** | + +## How to read this + +Four tags, three of them auto-implemented in Phase 2 and one held for you: + +- **bug** — code that produces incorrect behavior against its own stated/implemented contract (wrong output, data loss, dead detection path, broken UI state). Auto-implemented in Phase 2. +- **risk** — defense-in-depth / latent-exposure gaps: a thing that is not provably exploited today but diverges from a safer established pattern (un-redacted error echo, missing scheme allowlist, raw-error logging, missing rate limiter). Auto-implemented in Phase 2. +- **quality** — maintainability: dead code, duplication, token/aria drift, missing tests, naming/contract mismatches. No external behavior change. Auto-implemented in Phase 2. +- **judgment** — anything that changes external behavior, a public API/contract, a data schema/lexicon, dependency choices, or architecture. **HELD for Holke to decide** — not auto-implemented. + +**Phase 2 mechanics:** each bug + risk + quality item is implemented as one test-gated commit. If implementing an item turns the test suite red and the fix can't be made green cleanly, that commit is reverted and the item is logged as blocked. Judgment items are never auto-implemented; they wait for your call below. + +--- + +## Bugs + +### bug-001 — Context-update URI attachment rendered as `` with no scheme allowlist (stored javascript: XSS) +- **Severity:** high +- **Confidence:** high (confirmed by two independent agents: `context-1`, `xss-1`) +- **Files:** src/components/context/context-updates.tsx:358, :401, :425; src/lib/atproto/context-attachment.ts:210-216 +- **Evidence:** `resolveAttachment` validates only `typeof entry.uri === "string" && entry.uri.length > 0` — no scheme check — then three render sinks emit `href={attachment.uri}` / `href={uri}` verbatim. Records come from a federated PDS via the XRPC read proxy (`fetchContextAttachments`), so a record author can set `uri: "javascript:alert(document.cookie)"`; a viewer click executes script in the certified.app origin. The codebase's canonical guard `safeHttpUrl` (src/lib/utils/safe-url.ts) is applied in every sibling renderer (leaflet-document, leaflet-iframe-node, rich-text) but never imported under src/components/context/. Mandated by AGENTS.md §17.6 / §22.11 / §12. +- **Recommendation:** Reject the uri in `resolveAttachment` unless `safeHttpUrl(entry.uri)` is non-null (fixes all three sinks at the source), or compute `const href = safeHttpUrl(uri)` per tile and render a `` when null. Import from `@/lib/utils/safe-url`. +- **Testable:** yes — unit-test `resolveAttachment({ $type: "org.hypercerts.defs#uri", uri: "javascript:alert(1)" })` returns null; a normal https uri still resolves. +- **Risk of change:** low +- **Note:** Original tag was `risk` on both reports; a verifier on `context-1` suggested `bug`/`high`. Promoted to bug because it is a concrete, reachable security defect with a tested in-repo guard available. + +### bug-002 — OG/Twitter share image references a non-existent file on /welcome + 4 legal pages +- **Severity:** high +- **Confidence:** high (confirmed by two independent agents: `landing-og-1`, `approot-og-image-404`) +- **Files:** src/app/welcome/page.tsx:20, :34; src/app/terms/page.tsx:14; src/app/privacy/page.tsx:14; src/app/dsa/page.tsx:14; src/app/imprint/page.tsx:14 (correct refs for comparison: src/app/layout.tsx:50, :61; src/app/about/page.tsx:16) +- **Evidence:** Five pages set the OG/Twitter image to `…/assets/certified-hero-1200x630.png`, but the only asset on disk is `public/assets/certs-hero-1200x630.png` (verified via `find`). Root layout and /about correctly reference `certs-hero`. No rewrite/redirect rescues the `certified-` path, so the most-shared landing URL and four legal pages serve a 404 image — unfurls render with no preview. AGENTS.md §18/§20 document the name as `certified-hero`, so the docs are stale, not authoritative. +- **Recommendation:** Change the five `certified-hero-1200x630.png` strings to `certs-hero-1200x630.png` (lowest-risk; matches the file + layout + about). Add a test asserting every metadata image path resolves to a file under `public/`. +- **Testable:** yes — assert each `images` URL in the exported `metadata` of welcome/terms/privacy/dsa/imprint resolves via `fs.existsSync` under `public/`. +- **Risk of change:** low + +### bug-003 — InvalidSwap detection is dead for the group BFF write path (group-record swap saves silently lose edits) +- **Severity:** high +- **Confidence:** high (confirmed) +- **Files:** src/lib/atproto/repo-write.ts:89-111; src/app/api/groups/[groupDid]/project/route.ts:270-273; src/app/api/groups/[groupDid]/activity/route.ts:125,174; src/lib/utils/api.ts:19-31; src/lib/atproto/save-with-swap.ts:108-114 +- **Evidence:** `writeToRepo` detects a CID-precondition failure via `data.code === "InvalidSwap" || data.error === "InvalidSwap"`. That shape is produced only by the own-DID XRPC proxy. The group routes return `{ error: message }` with NO `code` field (the discriminator is dropped by `extractRouteError`, which never reads `e.error`), and `message` is the redacted human string — never literally "InvalidSwap". So for group writes the detection never fires, `writeToRepo` throws a generic Error, and `saveWithSwap` re-throws past the conflict-rebase + drafts-recovery machinery. The user's unsaved edits are not persisted and they get a generic failure instead of the conflict banner. Reachable via `putCertRecord`/`putProjectRecord` whenever a group admin's inline edit races another writer. The in-code comment claiming the group routes "surface the discriminator in data.code" is factually false. +- **Recommendation:** Make the group BFF routes preserve the discriminator — return `{ error: message, code: err.error }` (or map InvalidSwap → HTTP 409 / a stable `code`), mirroring the XRPC proxy. Alternatively treat HTTP 409 as InvalidSwap inside `writeToRepo`. +- **Testable:** yes — in repo-write.test.ts, with `targetDid !== ownDid`, mock authFetch to resolve the group route's actual `{ error: "Record was modified" }` body and assert `writeToRepo` rejects with `InvalidSwapError`. +- **Risk of change:** medium + +### bug-004 — Inline-edit Save while an avatar/banner upload is in-flight silently drops the new image +- **Severity:** high +- **Confidence:** high (confirmed) +- **Files:** src/hooks/use-profile-inline-edit.ts:483-516, :556-576, :674-688; src/app/profile/[handle]/page.tsx:316-322; src/components/ui/edit-banner.tsx:73 +- **Evidence:** `handleAvatarFile`/`handleBannerFile` set the object-URL preview synchronously but only set `pendingAvatarBlob` after the upload resolves. `handleSave` reads `pendingAvatarBlob`; if it is still null it falls back to the OLD `base?.avatar`, so the new image is not written. The post-save mirror keys off `pendingAvatarPreviewUrl` (which IS set), promoting the preview to read-mode display — so the UI shows the new avatar as saved while the PDS has the old one. The EditBanner Save button is gated only on `isSaving`; `hasPendingAvatar/Banner` never gate Save, and no upload-in-flight state is tracked. On the next resolve-did refetch the old image reappears. +- **Recommendation:** Gate Save while an upload is pending — await the in-flight upload Promise inside `handleSave`, or pass `canSave={!(previewUrl && !blob)}` to EditBanner (EditBanner already supports `canSave`). +- **Testable:** yes — mock `uploadAvatar` as a never-resolving promise, pick a file, call `handleSave`, assert `putProfile` is not called with the stale `base.avatar` (or that Save is disabled/awaits). +- **Risk of change:** low + +### bug-005 — Project `location` is written as a strongRef object but read as a string — never displays +- **Severity:** medium *(downranked below the high bugs; high-confidence)* +- **Confidence:** high (confirmed) +- **Files:** src/components/project/project-detail.tsx:325, :1024; src/app/project/new/page.tsx:303; src/app/project/[did]/[rkey]/edit/page.tsx:489 +- **Evidence:** Both create and edit persist `location` as `{ uri, cid }`. The detail view reads it with `asString(...)`, which returns null for an object, so the Location meta row never renders and `hasAnyMeta` never counts it (even bypassing `asString`, `{location}` would print `[object Object]`). The edit page's own hydration parses the strongRef + resolves the name correctly, confirming the intended shape. +- **Recommendation:** In project-detail.tsx, resolve the strongRef like the edit page (parse at:// → getRecord → `splitLocationName(value.name)`), or at minimum guard the object shape so `[object Object]` never renders. Keep the legacy string path. +- **Testable:** yes — render `` with `value.location = { uri, cid }`, mock getRecord → `{ value: { name: "Berlin" } }`, assert a Location row appears and `[object Object]` never does. +- **Risk of change:** low + +### bug-006 — Audit-log result pill never gets its color class (result allowlist mismatches the actual values) +- **Severity:** medium +- **Confidence:** high (confirmed) +- **Files:** src/components/groups/org-settings.tsx:588, :591 +- **Evidence:** `safeResult = ["success","failure","error"].includes(entry.result) ? entry.result : "unknown"`, but `AuditEntry.result` is typed `"permitted" | "denied"` (types.ts:82) and the only CSS classes are `--permitted`/`--denied` (pages.css:587,592). Neither domain value is in the allowlist, so `safeResult` is always `"unknown"` → `org-audit__result--unknown`, a class with no CSS. Every result chip renders unstyled; the raw text masks the breakage. +- **Recommendation:** `const safeResult = ["permitted","denied"].includes(entry.result) ? entry.result : "unknown"`. Keep type + CSS in sync. +- **Testable:** yes — render OrgSettings with stubbed `{ result: "permitted" }` / `{ result: "denied" }` and assert the element carries `--permitted` / `--denied`, not `--unknown`. +- **Risk of change:** low + +### bug-007 — /endorsements Received tab hides rejected endorsements with no way to un-reject +- **Severity:** medium +- **Confidence:** high (confirmed) +- **Files:** src/app/endorsements/page.tsx:149, :134; src/hooks/use-received-endorsements.ts:373 +- **Evidence:** `ReceivedEndorsementsList` is the viewer's own management inbox (it renders Accept/Reject controls) but calls `useReceivedEndorsements(did)` without `{ includeRejected: true }`, so the hook strips every award with `responseState === "rejected"`. Already-rejected endorsements are invisible, and once an indexer re-scan joins the rejected state the row vanishes with no UI to bring it back. The profile owner surface correctly passes `includeRejected: viewerIsOwner` (per §22.21) — this page is the inconsistent owner surface. (Verifier note: the row does not vanish *immediately* on click, only after the 5-min cache refresh; the steady-state outcome is as described.) +- **Recommendation:** Pass `{ includeRejected: true }` and render rejected rows (or add the same hide/only-rejected/show-all filter the profile surface uses). +- **Testable:** yes — mock the hook to return one award with `responseState: "rejected"`; assert the row is absent today and renders after the fix. +- **Risk of change:** medium + +### bug-008 — Blob/image upload errors are swallowed — cert/project can publish without the previewed image +- **Severity:** medium +- **Confidence:** high (confirmed) +- **Files:** src/app/create/page.tsx:320; src/app/project/new/page.tsx:205; src/app/project/[did]/[rkey]/edit/page.tsx:327; src/components/project/project-detail.tsx:553 +- **Evidence:** `handleImageFile`/`handleBannerFile` `await uploadBlob(...)` with NO try/catch; `ImageEditOverlay.handleChange` uses try/finally with NO catch, so a rejection becomes an unhandled promise rejection (no global handler exists). The optimistic preview is already shown but `pendingImageBlob` stays null, and `record.image` is only attached when the blob exists — so the record publishes without the previewed image. `canSubmit` also doesn't wait on an in-flight upload, so Publish can fire before the upload resolves. +- **Recommendation:** Wrap each `uploadBlob` call in try/catch: set the page's `error` state and clear/revoke the dangling preview + blob. Optionally add an `isUploadingImage` flag to `canSubmit`. (create and project/new already render `` from `error` state.) +- **Testable:** yes — mock `uploadBlob` to reject, fire `onFile`, assert an error surfaces and the preview is cleared rather than leaving a submittable form. +- **Risk of change:** low + +### bug-009 — Banner picker in ProfileEditForm shows no live preview after selecting a file +- **Severity:** medium +- **Confidence:** high (confirmed) +- **Files:** src/components/profile/profile-edit-form.tsx:344-348; src/components/profile/banner-upload.tsx:35-99; src/app/settings/edit-profile/page.tsx:202-203; src/app/groups/[groupDid]/edit-profile/page.tsx:198-202 +- **Evidence:** `BannerUpload` renders only from `currentBannerUrl`; its doc-comment says the parent owns the object-URL preview, but neither consumer creates one — `currentBannerUrl` is passed straight from the saved record. After picking a banner the image area keeps showing the OLD banner; the only feedback is the button label flipping to "Replace banner". The avatar self-previews (AvatarUpload creates an object URL), so the two controls behave inconsistently in the same form. The save path still persists correctly — this is a UX feedback gap, not data loss. +- **Recommendation:** In ProfileEditForm create+revoke an object URL on banner pick and pass it as `currentBannerUrl` (falling back to saved), or make BannerUpload self-preview like AvatarUpload. Apply to the group edit page too. +- **Testable:** yes — fire a change on the banner input with a File and assert the rendered `` becomes a blob:/object URL. +- **Risk of change:** low + +### bug-010 — Location MapPin in feed PreviewCard never renders when a date period is also present +- **Severity:** low +- **Confidence:** high (confirmed) +- **Files:** src/components/home/home-feed.tsx:1129-1138, :966-981 +- **Evidence:** The pin gate `i === 0 && withLocationIcon && i === meta.length - 1` is only satisfiable when meta has exactly one entry. CertPreview builds `meta = [period, "N locations"]`, so when a cert has both a period and locations the pin renders nowhere; it only appears when location is the sole meta item. Cosmetic — the "N locations" text still renders. +- **Recommendation:** Push a ReactNode meta entry (`<> {n} locations`) and drop `withLocationIcon`, mirroring ExploreListRow / CertListRow which already do exactly this. +- **Testable:** yes — render PreviewCard with `meta={["Jan–Mar 2025","3 locations"]}` + `withLocationIcon` and assert a MapPin svg appears next to "3 locations". +- **Risk of change:** low + +### bug-011 — useUserActivities.loadMore appends stale-DID records after a profile switch and has no URI dedup +- **Severity:** low *(verifier downgraded from medium — latent/unreachable today)* +- **Confidence:** medium (confirmed at the function level; no reachable live caller) +- **Files:** src/hooks/use-user-activities.ts:52, :57 +- **Evidence:** `loadMore()` calls `fetchActivities(did, cursor, 20)` with no AbortSignal and no generation guard, then `setActivities((prev) => [...prev, ...data.records])` with no dedup. An in-flight loadMore for profile A can resolve after a switch to B and append A's rows to B's reset list; sibling hooks (use-explore, use-home-feed) gate on a generation token and dedup by URI. **Live impact is latent:** the only consumer that threads `loadMore` is `UserFeed`, which is dead code (see quality-001); the two mounted consumers never call `loadMore`. +- **Recommendation:** Thread an AbortController/generation token through loadMore like use-explore, bail when superseded, and dedup appended records by `uri` (mirror use-home-feed's `seen` Set). Coordinate with quality-001 (delete UserFeed) — fixing or deleting both resolves the latent path. +- **Testable:** yes — resolve page 1 for did=A, delay loadMore, switch to B, then resolve A's loadMore; assert `activities` contains only B's records and no duplicate URIs. +- **Risk of change:** low + +--- + +## Risks + +### risk-001 — XRPC proxy echoes un-redacted 4xx upstream messages to the client +- **Severity:** medium +- **Confidence:** high (confirmed) +- **Files:** src/app/api/xrpc/[...method]/route.ts:110-111, :123 +- **Evidence:** In `xrpcError`, the 4xx message returned to the client is the raw upstream string; `redactSecrets` is applied only to the server log, not the returned value (lines 388-391 GET / 579-582 POST echo it as `{ error: message }`). The canonical `extractRouteError` does the opposite — `redactSecrets(e.message)` for 4xx — and that posture is pinned by api.test.ts. atproto error messages have been observed to embed JWTs/DPoP proofs, so a 400/401/403 on createRecord/putRecord/updateEmail/resetPassword can surface secret-shaped fragments to the browser. +- **Recommendation:** `const message = status >= 500 || !rawMessage ? "Internal server error" : redactSecrets(rawMessage)`. `redactSecrets` is already imported. +- **Testable:** yes — call `xrpcError({ status: 400, message: "bad token Bearer eyJabc.def.ghi" })` and assert the returned message is redacted. +- **Risk of change:** low + +### risk-002 — groups/register logs the raw atproto error (DPoP/Bearer leak) on the org-limit check failure path +- **Severity:** medium +- **Confidence:** high (confirmed) +- **Files:** src/app/api/groups/register/route.ts:129 +- **Evidence:** `console.error("Org creation limit check failed:", err)` logs the raw error from `getServiceAuth(...)` (an authenticated OAuth/DPoP Agent call). Per log-safe.ts, the atproto SDK attaches the upstream Request (DPoP proofs + Bearer tokens) on `err.cause`, and stack traces serialize the same Request; `logSafe` exists to drop `.cause`/`.stack` and redact `.message`. Every sibling route uses logSafe/extractRouteError; this line is the outlier. +- **Recommendation:** `logSafe("[groups/register] org-limit check failed", err)` (already imported). Keep the 503. +- **Testable:** yes — pass an Error with `Bearer eyJ…` in `.message` and a `.cause` Request; assert the emitted payload is redacted and omits cause/stack. +- **Risk of change:** low + +### risk-003 — Profile inline-edit save is a non-atomic multi-write; mid-sequence failure orphans/partially persists with no rollback +- **Severity:** low *(verifier downgraded from medium — orphans are harmless records in the user's own repo, not data loss)* +- **Confidence:** medium (confirmed; "dataloss" framing overstated) +- **Files:** src/hooks/use-profile-inline-edit.ts:595, :630, :659, :697 +- **Evidence:** `handleSave` does up to three sequential PDS writes (`putProfile` → `putLocationRecord` → `putOrgMarker`) with no transaction. If `putLocationRecord` mints a new location record (first-time add, no rkey) but `putOrgMarker` then throws, the location record is orphaned and re-minted on each retry. The catch reverts in-memory mirrors, leaving a transient profile/marker divergence that self-heals on the next successful save. No data is lost. +- **Recommendation:** Reuse a stable rkey for the location record so retries overwrite rather than orphan (putLocationRecord already supports this), or mint the location record only after the referencing marker write succeeds. On partial success, surface which part failed. +- **Testable:** yes — mock putProfile resolve / putOrgMarker reject; assert no second orphan location record is created on retry. +- **Risk of change:** medium + +### risk-004 — groups/register does not server-side sanitize handle and forwards email unvalidated +- **Severity:** low +- **Confidence:** medium (confirmed) +- **Files:** src/app/api/groups/register/route.ts:31-50, :151 +- **Evidence:** The route validates only `handle.length > 253` and `ownerDid === auth.did`; it never runs `sanitizeHandle`/`stripInvisible` on handle nor validates email, and forwards `{ handle, ownerDid, email }` verbatim. AGENTS.md §17.6/§24.5 mandate sanitizing at the boundary even when the client also does; the create UI does not sanitize either. The group service is expected to validate, so this is a defense-in-depth gap. (Email path is practically unreachable today — `registerGroup`'s email arg has no caller.) +- **Recommendation:** `sanitizeHandle(handle)` (re-check the 253 cap on the result) and validate email with feedback/route.ts's regex + 254-char cap before forwarding; drop/reject on failure. +- **Testable:** yes — POST a handle with a zero-width space and assert the forwarded value is stripped; POST `email: "not-an-email"` and assert 400 or email omitted. +- **Risk of change:** medium + +### risk-005 — XRPC proxy passes createRecord/putRecord body verbatim — no allowlist on envelope fields (validate, swapCommit, rkey) +- **Severity:** low +- **Confidence:** low *(verifier judged the original framing inverted; kept as a low-confidence consistency note, not the refuted security claim)* +- **Files:** src/app/api/xrpc/[...method]/route.ts:431-505 +- **Evidence:** After enforcing `repo === did` + collection allowlist, the proxy forwards the raw client body, so a client can set `validate:false`, `swapCommit`, or an arbitrary `rkey`. **Verifier:** this is confined to the user's OWN repo (no cross-tenant break) and is the documented write boundary; the group routes' `pickAllowedFields` defends the inner record (mass-assignment), not the envelope, so the "parity" argument is inverted. The only residual is a user seeding lexicon-invalid records into their own repo that downstream readers must already tolerate. +- **Recommendation:** Optional hardening only — drop client-supplied `validate:false` so a user can't store lexicon-invalid records. Otherwise document the intentional own-repo trust (already in AGENTS.md §10). +- **Testable:** yes — POST createRecord with `validate:false` + a record missing required fields; assert the proxy strips `validate` or upstream rejects. +- **Risk of change:** medium + +### risk-006 — Authenticated/dynamic surfaces are crawlable: /home, /search, /activity/* missing from robots disallow and lack per-page noindex +- **Severity:** medium +- **Confidence:** high (confirmed) +- **Files:** src/app/robots.ts:8-9; src/app/home/page.tsx:1; src/app/search/layout.tsx:3; src/app/activity/[did]/[rkey]/page.tsx:1 +- **Evidence:** robots.ts disallows /settings,/create,/endorsements,/notifications,/groups,/oauth,/api but not /home,/explore,/search,/activity. No page sets `robots: { index:false }`. Auth is fully client-side (no middleware), so the unauthenticated `/home` shell renders a 200 sign-in CTA that Googlebot can index; `/search` even ships a canonical inviting indexation. AGENTS.md §18 mandates authenticated pages set `robots:{index:false,follow:false}` — this is the §22.12 robots/sitemap drift pitfall. Index/SEO hygiene, not private-data exposure. +- **Recommendation:** Per page, add `export const metadata = { robots: { index:false, follow:false } }` to app-only surfaces (home, activity detail, search, notifications) and/or extend robots.ts disallow with /home,/explore,/search,/activity. Keep genuinely public surfaces (/profile, /project) indexable. +- **Testable:** yes — invoke robots.ts default export and assert the disallow array includes the app-only routes; or assert each page's `metadata.robots.index === false`. +- **Risk of change:** low + +### risk-007 — resolve-did is an unauthenticated 3-fetch outbound fan-out with no rate limiter +- **Severity:** low *(see judgment-002 — a verifier reclassified this as a judgment/consistency call; retained here as the lower-severity risk framing)* +- **Confidence:** medium (confirmed technically; intentionally incremental per prior audit) +- **Files:** src/app/api/resolve-did/route.ts:168-194 +- **Evidence:** GET /api/resolve-did is unauthenticated and issues up to 3 outbound fetches (resolveHandle, getCertsProfile → resolvePdsUrl + getRecord, fetchBskyAppViewProfile) per request. Its siblings resolve-handle and search-actors are explicitly IP/DID rate-limited with egress-quota comments; this one is not. A DID-doc cache (5-min TTL) and the SSRF allowlist mitigate, and a prior audit examined this exact file and chose not to flag it — hence the judgment overlap. +- **Recommendation:** Add an IP+DID limiter mirroring search-actors (`makeLimiter("resolve-did", 60, 60)`), or document the intentional omission inline. **This duplicates judgment-002 — implement only if you (Holke) approve the broader rate-limit rollout.** +- **Testable:** no +- **Risk of change:** low + +### risk-008 — saveWithSwap `read` callbacks use raw `fetch` instead of `authFetch` +- **Severity:** low *(verifier: original risk rationale refuted; survives as a quality/consistency note — see also moved to quality)* +- **Confidence:** low (uncertain — needs human/runtime check) +- **Files:** src/components/project/project-detail.tsx:692; src/app/project/[did]/[rkey]/edit/page.tsx:535 +- **Evidence:** Both conflict-resolution `read` callbacks use raw `fetch('/api/xrpc/.../getRecord')` instead of `authFetch`, bypassing the 401 `onUnauthorized` interceptor. **Verifier:** the described session-expiry failure mode is effectively unreachable — read() only runs on the InvalidSwap conflict path (the write's authFetch already fires onUnauthorized on a real 401), and getRecord is a public-read method that won't 401 here. So this is a low-value consistency nit, not a session-expiry risk. Retained at low confidence; the verifier suggested tag `quality`. +- **Recommendation:** Swap raw `fetch` → `authFetch` (both already imported) for convention consistency. No happy-path change. +- **Testable:** yes (low value) — mock getRecord 401 during save and assert onUnauthorized fires with authFetch. +- **Risk of change:** low + +### risk-009 — deleteFollow cannot target a group repo — group-aware unfollow mis-targets the personal repo +- **Severity:** low *(verifier downgraded from bug/medium — latent, no reachable group-context caller and the group route has no DELETE handler)* +- **Confidence:** medium (confirmed observation; tag changed bug→risk) +- **Files:** src/lib/atproto/follow.ts:105, :65; src/hooks/use-social-graph-sync.ts:158 +- **Evidence:** `createFollow` is group-aware via `targetDid`, but `deleteFollow(ownDid, rkey)` has no `targetDid` and always POSTs deleteRecord with `repo=ownDid` (the proxy enforces `repo===session`), so a follow written to a group repo has no delete path. Today no group-context caller exists, the group follow route exports only POST (no DELETE), and both deleteFollow call sites are personal-context — so it's a latent API-completeness gap, not an active mis-target. +- **Recommendation:** Add `opts?: { targetDid?: string }` to `deleteFollow` mirroring `createFollow`, routing through the group BFF when set (requires adding a DELETE handler to /api/groups/[groupDid]/follow). Until then, document that group follows can't be removed via this helper. +- **Testable:** yes (after adding the param) — `deleteFollow(ownDid, rkey, { targetDid: groupDid })` hits `/api/groups//...`, not the personal proxy. +- **Risk of change:** low + +--- + +## Quality + +### quality-001 — UserFeed component is dead code (never imported) +- **Severity:** medium *(highest-value dead-code item: also masks bug-011)* +- **Confidence:** high (confirmed dead; corroborated by hooks-fetch-1's verifier) +- **Files:** src/components/feed/user-feed.tsx:16 +- **Evidence:** No repo-wide importer of UserFeed; FeedLayout's real consumers import it directly. It also feeds an empty `did` (`getDid={() => authorDid ?? ""}`) which would render broken bylines if wired. It is the only consumer of useUserActivities.loadMore (bug-011). +- **Recommendation:** Delete user-feed.tsx (resolves bug-011's reachable path), or wire it into the profile route with a real DID. +- **Testable:** no +- **Risk of change:** low + +### quality-002 — Whole module src/lib/atproto/cert-context.ts (304 lines) is dead — zero importers +- **Severity:** medium +- **Confidence:** high +- **Files:** src/lib/atproto/cert-context.ts:27, :35, :65, :271 +- **Evidence:** All four exports have zero external references; the referenced `use-cert-context.ts` hook does not exist and `fetchContextUpdates` lives in a different module. (Also the home of bug-candidate `atproto-records-2` below — fix or delete together.) +- **Recommendation:** Delete the module (confirm no in-flight branch reintroduces the Explore aggregate first). +- **Testable:** no +- **Risk of change:** low + +### quality-003 — fetchAllCertContext reads only the first 50 records per lexicon and never walks the cursor +- **Severity:** low *(originally bug/medium; the module is currently dead code — see quality-002 — so no live impact)* +- **Confidence:** medium (confirmed correctness gap, dead today) +- **Files:** src/lib/atproto/cert-context.ts:226-261, :271-304 +- **Evidence:** `listAndFilter` issues one `listRecords` with `limit:"50"` and never follows the cursor, then filters in-memory, despite the "All"/"every" naming. Sibling readers (fetchTypedLists, listEndorsementListCollections) walk the cursor. Currently no caller, so latent. +- **Recommendation:** Paginate via the cursor like the sibling readers, or rename/document the single-page cap. If quality-002 deletes the module, this resolves itself. +- **Testable:** yes — first page of 50 non-matching + cursor, second page with the match; assert the match is returned. +- **Risk of change:** low + +### quality-004 — src/config/trusted-evaluators.ts is dead in app code and duplicates the live list with a divergent DID set +- **Severity:** medium +- **Confidence:** high +- **Files:** src/config/trusted-evaluators.ts:22, :30, :33; src/lib/atproto/trusted-evaluators.ts:22 +- **Evidence:** The config `TRUSTED_EVALUATORS` is referenced only by its own test; `ALL_EVALUATOR_DIDS`/`ALL_EVALUATORS_STABLE_KEY` have zero references. The live module (used by home-feed + use-evaluator-endorsements) has 4 DIDs; the dead config has 3. The config's "authoritative source of truth" comment is false. +- **Recommendation:** Pick one source of truth: delete the config module + its test, OR have the lib import from it and reconcile the 3-vs-4 DID divergence. Removing the module requires removing its test. +- **Testable:** yes — assert the two arrays are identical if both kept (fails today). +- **Risk of change:** low + +### quality-005 — Dead code: EndorseShortcut + the received-grid optimistic-overlay state machine +- **Severity:** medium +- **Confidence:** high (confirmed dead by two agents: `profile-lists-endorse-1`, `dataloss-priv-1`) +- **Files:** src/components/profile/profile-endorsements.tsx:976-1075, :135-186, :445-456 +- **Evidence:** `EndorseShortcut` is never rendered; `handleEndorsed`/`handleRevoked` (the only setters of `optimisticAdds`/`optimisticHides`) are never passed to any child, so the optimistic overlay in `displayReceived` is permanently inert. The comment at :447-450 claiming the handlers are "exported to the sidebar" is false — the sidebar's EndorseButton has its own optimistic state. ~140 lines of dead code in the largest profile file plus a misleading comment. +- **Recommendation:** Delete the overlay machinery + EndorseShortcut and collapse `displayReceived` to `received.endorsements`; remove the misleading comment. (Or wire the handlers into the sidebar if optimistic Received-grid update is actually wanted — confirm intent.) +- **Testable:** yes (negative) — assert displayReceived never diverges from received.endorsements under current wiring. +- **Risk of change:** medium + +### quality-006 — useOrgProfile has no module-level cache and is mounted in 4 layout components +- **Severity:** medium +- **Confidence:** high (confirmed) +- **Files:** src/hooks/use-org-profile.ts:25-64; navbar.tsx:36; mobile-sidebar.tsx:35; desktop-left-rail.tsx:81; desktop-top-bar.tsx:140 +- **Evidence:** The hook fetches getOrgProfile + getOrgMetadata + resolvePdsUrl on every mount with no shared cache (unlike useOrgMarker/useProfilePds/useAuthorInfo). It is instantiated in 4 layout components that all render on every authenticated page → 3-4 concurrent identical fan-outs (one being an off-app plc.directory resolvePdsUrl) per navigation, just to derive orgAvatarUrl. +- **Recommendation:** Add a module-level cache keyed by `activeOrg.groupDid` + in-flight Map (mirror use-org-marker / use-author-info); keep refetch() as a cache-evicting force path. +- **Testable:** no +- **Risk of change:** low + +### quality-007 — Security-critical sanitize.ts has no unit tests despite AGENTS.md pinning its exact regex +- **Severity:** medium +- **Confidence:** high +- **Files:** src/lib/utils/sanitize.ts:5, :13, :17 +- **Evidence:** `stripInvisible`/`sanitizeEmail`/`sanitizeHandle` are pure, security-relevant (login/feedback boundary), and untested, while every sibling pure util has tests. A future regex edit could silently narrow the allowlisted code-point ranges undetected. +- **Recommendation:** Add sanitize.test.ts covering ZWSP/ZWJ/BOM/soft-hyphen removal, leading-@ strip, whitespace stripping for email/handle, lowercase for email, internal-whitespace preservation, and clean-string passthrough. +- **Testable:** yes +- **Risk of change:** low + +### quality-008 — Popover content hardcodes z-[40] instead of the --z-popover token +- **Severity:** medium +- **Confidence:** high +- **Files:** src/components/ui/popover.tsx:179 +- **Evidence:** `z-[40]` duplicates `--z-popover: 40` (tokens.css:159), violating CLAUDE.md hard rule 5 / AGENTS §11.5 on the canonical primitive that replaced the four ad-hoc menus. Renders identically today but breaks stacking for every consumer if the z-map is re-tiered. +- **Recommendation:** `z-[var(--z-popover)]` (or `zIndex:"var(--z-popover)"` in the inline style). +- **Testable:** no +- **Risk of change:** low + +### quality-009 — config-1: No CI workflow exists — the documented test/type/lint baseline is never enforced +- **Severity:** medium +- **Confidence:** high +- **Files:** .github/CODEOWNERS:1; package.json:5 +- **Evidence:** `.github/` contains only CODEOWNERS; no workflows. CLAUDE.md references "make CI green" and a 291-test/0-tsc baseline, but nothing runs lint/tsc/vitest on push/PR. Vercel's `next build` does not run vitest, so test regressions are unguarded. +- **Recommendation:** Add `.github/workflows/ci.yml` running `npm ci` → `npm run lint` → `npx tsc --noEmit` → `npm test` on PRs into staging/main, with a pinned Node version (quality-018). +- **Testable:** no +- **Risk of change:** low + +### quality-010 — config-2: tsconfig excludes all test files, so `tsc --noEmit` never type-checks the 17 __tests__ files +- **Severity:** medium +- **Confidence:** high +- **Files:** tsconfig.json:38-41 +- **Evidence:** `"exclude": ["node_modules","src/**/__tests__/**"]` removes test files from the program; vitest doesn't type-check either. Tests can call renamed/removed exports and still pass while "0 tsc errors" reports clean — the baseline covers production code only. +- **Recommendation:** Stop excluding `src/**/__tests__/**`, or add a `tsconfig.test.json` / `vitest run --typecheck` step wired into CI (quality-009). +- **Testable:** no +- **Risk of change:** low + +### quality-011 — Dead exported helpers in workspace.ts: fetchOrganizationDids + @deprecated fetchOrganizationDidsForSet +- **Severity:** low +- **Confidence:** high +- **Files:** src/lib/atproto/workspace.ts:77, :352 +- **Evidence:** Both have zero references; `fetchOrganizationDidsForSet` is explicitly `@deprecated` in favor of `fetchDidsByKindInSet` (which is used). The deprecated complement-based function carries its own filter-inversion-bug warning. +- **Recommendation:** Delete both plus now-unused helper types (e.g. `OrganizationDidsGraphQLResponse`). +- **Testable:** no +- **Risk of change:** low + +### quality-012 — PersonCard + fetchName/useAuthorNamesMap copy-pasted across profile-endorsements.tsx and profile-followers.tsx +- **Severity:** medium +- **Confidence:** high +- **Files:** src/components/profile/profile-endorsements.tsx:885, :1089; src/components/profile/profile-followers.tsx:548, :617 +- **Evidence:** `PersonCard` is defined locally in both files (followers is a strict subset of endorsements); the name-cache helper differs only in comments. Two module-scoped caches for the same `/api/resolve-did` resolution that `useAuthorInfo` already centralizes. AGENTS §15a refers to "the shared row," implying one component. +- **Recommendation:** Extract one `PersonCard` (superset props, `note?`/`listTitle?` optional) into person-card.tsx; replace the duplicated name-cache with the existing `useAuthorInfo` batch hook. +- **Testable:** yes — render the shared PersonCard with/without note/listTitle and assert the rows match. +- **Risk of change:** medium + +### quality-013 — onboarding step-profile creates object URLs in render body without revoking (memory leak) +- **Severity:** medium +- **Confidence:** high +- **Files:** src/components/onboarding/steps/step-profile.tsx:50, :53; src/components/onboarding/onboarding-modal.tsx:151 +- **Evidence:** `previewAvatarUrl`/`previewBannerUrl` call `URL.createObjectURL(...)` directly in the render body; StepProfile re-renders on every keystroke, so a fresh blob URL is allocated for avatar+banner each render and never revoked. The rest of the codebase pairs createObjectURL with revokeObjectURL. +- **Recommendation:** Compute the preview in `useMemo`/`useEffect` keyed on the File and revoke the previous URL on change/unmount, mirroring use-profile-inline-edit / avatar-upload. +- **Testable:** no +- **Risk of change:** low + +### quality-014 — api-misc-3: search-actors logs the raw error object instead of using logSafe +- **Severity:** low +- **Confidence:** high +- **Files:** src/app/api/search-actors/route.ts:103 +- **Evidence:** `console.error("[search-actors]", err)` vs the repo-wide logSafe convention. Here `err` is from an unauthenticated public fetch (no secret to leak), so lower-risk than risk-002 — but it would silently start leaking if swapped to an authenticated agent call. +- **Recommendation:** `logSafe("[search-actors] upstream error", err)`. +- **Testable:** no +- **Risk of change:** low + +### quality-015 — api-misc-4: notifications upstream error log includes up to 500 chars of the GraphQL response body +- **Severity:** low +- **Confidence:** medium +- **Files:** src/app/api/notifications/route.ts:228 +- **Evidence:** On non-2xx, `console.warn(..., responseBody.slice(0,500))` writes the user's own notification data (DIDs, record URIs) to logs — PII-in-logs smell, not a credential leak, and bounded by the slice. +- **Recommendation:** Log only `upstream.status`, or route through logSafe. +- **Testable:** no +- **Risk of change:** low + +### quality-016 — api-trust-2: xrpcError does not clamp the upstream status to the valid HTTP range +- **Severity:** low +- **Confidence:** medium +- **Files:** src/app/api/xrpc/[...method]/route.ts:108-109, :386-392 +- **Evidence:** `status` accepts any upstream number (0, -1, 1000) and is passed straight to `NextResponse.json(..., { status })`; an out-of-range status throws RangeError inside the terminal catch → opaque framework 500. The sibling `clampHttpStatus` exists for exactly this. +- **Recommendation:** Reuse `clampHttpStatus` (or inline `Number.isInteger(s) && s>=200 && s<=599 ? s : 500`). +- **Testable:** yes — `xrpcError({ status: 0 })` returns 500 and the handler doesn't throw. +- **Risk of change:** low + +### quality-017 — api-trust-3: indexer body-size cap measures UTF-16 string length, not byte length +- **Severity:** low +- **Confidence:** medium +- **Files:** src/app/api/indexer/route.ts:1426-1432, :1416-1422 +- **Evidence:** The post-read check uses `text.length` (UTF-16 code units) while MAX_BODY_SIZE is documented as 32KB bytes; multi-byte bodies (or a falsified Content-Length) can carry ~3-4x the byte limit. Defense-in-depth, so small practical exposure, but the "32KB" guarantee is inaccurate. +- **Recommendation:** `Buffer.byteLength(text, "utf8") > MAX_BODY_SIZE`, or read via `arrayBuffer()`. +- **Testable:** yes — POST >32KB of multi-byte chars with no Content-Length and assert 413. +- **Risk of change:** low + +### quality-018 — config-7: No Node version pin (engines/.nvmrc) despite Next 16 requiring Node >=20.9.0 +- **Severity:** low +- **Confidence:** high +- **Files:** package.json:2 +- **Evidence:** No `engines`, no `packageManager`, no `.nvmrc`. Contributors/CI can run a Node version diverging from Vercel's runtime → "works on my machine" build discrepancies. Compounds quality-009. +- **Recommendation:** Add `"engines": { "node": ">=20.9.0" }` + `.nvmrc`; surface in the CI matrix. +- **Testable:** no +- **Risk of change:** low + +### quality-019 — config-5: No `typecheck` npm script +- **Severity:** low +- **Confidence:** high +- **Files:** package.json:5 +- **Evidence:** No `typecheck` script; the documented `npx tsc --noEmit` gate depends on memory, and with no CI it never runs automatically. +- **Recommendation:** Add `"typecheck": "tsc --noEmit"` and invoke from CI. +- **Testable:** no +- **Risk of change:** low + +### quality-020 — config-3: lint script's `--ext .ts,.tsx` is a no-op under ESLint 9 flat config +- **Severity:** low +- **Confidence:** high +- **Files:** package.json:9 +- **Evidence:** Flat config governs file selection via `files`, not `--ext`; both invocations lint the same 363 files. The flag is dead/misleading legacy. +- **Recommendation:** `"lint": "eslint src/"`; scope via flat-config `files`/`ignores`. +- **Testable:** no +- **Risk of change:** low + +### quality-021 — config-4: lint scope is `src/` only — root configs and scripts/*.mjs never linted +- **Severity:** low +- **Confidence:** high +- **Files:** package.json:9; scripts/audit-screenshots.mjs:1; next.config.ts:1; eslint.config.mjs:1 +- **Evidence:** next.config.ts (CSP/redirects), eslint.config.mjs, vitest/tailwind/postcss configs, and the three audit scripts (Playwright logic) are never linted; e.g. an unused `(e)` catch in audit-screenshots.mjs:62 goes unflagged. +- **Recommendation:** `eslint .` with flat-config `ignores` for `.next/`/`node_modules/`/`coverage/`; recheck the warning baseline. +- **Testable:** no +- **Risk of change:** medium + +### quality-022 — config-8: audit scripts disagree on dev-server port (3001 vs hardcoded 3000) +- **Severity:** low +- **Confidence:** high +- **Files:** scripts/audit-screenshots.mjs:14; scripts/recapture-landing.mjs:21; scripts/capture-divergence-sheet.mjs:10 +- **Evidence:** audit-screenshots defaults BASE to :3001 (env-overridable); recapture-landing hardcodes :3000 with no override; `next dev` defaults to 3000. So audit-screenshots silently skips every route against a default dev server. +- **Recommendation:** All three read `process.env.BASE` with a single default `http://localhost:3000`. +- **Testable:** no +- **Risk of change:** low + +### quality-023 — config-9: .gitignore `core` pattern is unanchored (shadows any file/dir named core) +- **Severity:** low +- **Confidence:** high +- **Files:** .gitignore:44 +- **Evidence:** Bare `core` ignores any `core` file/dir at any depth — a future `src/lib/core/` or `core.ts` would be silently untracked. (The coredump it targets is at root and untracked — not a leak.) +- **Recommendation:** Anchor to `/core`. +- **Testable:** no +- **Risk of change:** low + +### quality-024 — config-6: Tailwind v3 PostCSS chain has no autoprefixer; vendor prefixes hand-maintained across 39+ sites +- **Severity:** low +- **Confidence:** medium +- **Files:** postcss.config.mjs:3-4; package.json:49 +- **Evidence:** PostCSS chain is only `{ tailwindcss: {} }`; autoprefixer is absent from the chain and Tailwind v3.4 doesn't run it. The codebase hand-writes `-webkit-*` prefixes in 39+ places; a new prefixed property could silently ship unprefixed in Safari. (Overlaps judgment-007 — the doc-vs-config disagreement is the judgment half.) +- **Recommendation:** Add `autoprefixer` devDep + plugin entry. Verify prefix output and no visual regression. (See judgment-007 — confirm intent first.) +- **Testable:** no +- **Risk of change:** medium + +### quality-025 — atproto-records-3: saveWithSwap relies on an untyped cross-shape contract with no guard +- **Severity:** low +- **Confidence:** medium +- **Files:** src/lib/atproto/save-with-swap.ts:92-149, :123-132 +- **Evidence:** `computeDirtyFields(mountSnapshot, drafts)` across two unrelated generics, then casts and compares `mountSnapshot[key]` vs `fresh.value[key]`, assuming all three key sets coincide. A renamed/UI-only draft key would be excluded from conflict detection and silently auto-rebase over a concurrent server change. +- **Recommendation:** Constrain `TDrafts extends Partial`, or accept explicit `dirtyKeys` from the caller; document the read().value-shape invariant. +- **Testable:** yes — drafts whose key differs from snapshot for a field the server also changed; assert conflict is detected. +- **Risk of change:** medium + +### quality-026 — atproto-records-4: two near-duplicate shallowEqual implementations with divergent semantics +- **Severity:** low +- **Confidence:** medium +- **Files:** src/lib/atproto/save-with-swap.ts:156-166; src/lib/utils/swap-drafts.ts:160-174 +- **Evidence:** Both are used in the same save flow; the swap-drafts version guards `Array.isArray(a) !== Array.isArray(b)`, save-with-swap's omits it — an array-vs-object edge could be classified inconsistently between the dirty-set step and the conflict step. +- **Recommendation:** Export one shared shallowEqual and reuse it in both. +- **Testable:** yes — `[]` vs `{}` through both paths; assert agreement. +- **Risk of change:** low + +### quality-027 — atproto-records-5: resolveCanonicalEndorsementDef treats missing createdAt as earliest, so a malformed def could win canonical +- **Severity:** low +- **Confidence:** medium +- **Files:** src/lib/atproto/badges.ts:336-348 +- **Evidence:** Ascending sort with `(a.value.createdAt ?? "") < ...` makes a def lacking createdAt sort first → chosen as canonical, scheduling well-formed defs for background deletion. Latent (default def always writes createdAt) but the self-heal logic would do the wrong thing. +- **Recommendation:** Sort missing createdAt to the end (treat as +Infinity) or filter unparseable defs before selecting canonical. +- **Testable:** yes +- **Risk of change:** low + +### quality-028 — social-2: location.ts ships a private parseAtUri that diverges from the shared strict parser +- **Severity:** low +- **Confidence:** high +- **Files:** src/lib/atproto/location.ts:448, :404; src/lib/atproto/activity-uri.ts:15 +- **Evidence:** Private `parseAtUri` uses `parts.length < 3` and drops trailing segments; the canonical shared parser uses `!== 3` and rejects extras (with a test). A malformed `at://did/coll/rkey/garbage` parses here where the shared one rejects. +- **Recommendation:** Import `parseAtUri` from activity-uri.ts and delete the private copy (already imported at the top of the read section). +- **Testable:** yes — `readLocationStrongRef` with a 4-segment uri returns null. +- **Risk of change:** low + +### quality-029 — social-3: parseNotificationsPage trusts indexer node shape after only spot-checking three fields +- **Severity:** low +- **Confidence:** medium +- **Files:** src/lib/atproto/notifications.ts:51, :7 +- **Evidence:** Validates only reason/sortAt/id then pushes the raw node as a fully-typed Notification; count/reasonSubject/latestAuthor/etc. are declared present but unchecked, so a partial edge surfaces as `count: undefined` while TS believes it's populated. +- **Recommendation:** Validate the remaining load-bearing fields, or mark the unchecked fields optional in the type. +- **Testable:** yes +- **Risk of change:** low + +### quality-030 — social-4: resolveHandle returns the alsoKnownAs value verbatim without validating it's a handle +- **Severity:** low +- **Confidence:** medium +- **Files:** src/lib/atproto/did.ts:217 +- **Evidence:** Strips `at://` from the first `alsoKnownAs` entry and returns the remainder unchanged; a did:web doc (attacker-controllable) with `at://example.com/some/path` yields `example.com/some/path` shown as a handle. Text-only, no injection, but a non-handle leaks through. +- **Recommendation:** After stripping, sanity-check it looks like a handle (non-empty, has a dot, no slash/whitespace) else return null. +- **Testable:** yes +- **Risk of change:** low + +### quality-031 — hooks-fetch-2: useHomeFeed INVALID_CURSOR recovery creates an untracked AbortController that can setState after unmount +- **Severity:** low +- **Confidence:** medium +- **Files:** src/hooks/use-home-feed.ts:354 +- **Evidence:** The catch-branch recovery `load()` uses a local controller never stored/aborted by effect cleanup; an unmount mid-flight leaves it uncancelled and setState fires after unmount. Other fetches in the hook abort via cleanup. +- **Recommendation:** Store the recovery controller in a ref the unmount cleanup aborts (or reuse the effect's controller). +- **Testable:** yes +- **Risk of change:** low + +### quality-032 — hooks-fetch-3: window-focus revalidation handlers fire fetches without an AbortController +- **Severity:** low +- **Confidence:** medium +- **Files:** src/hooks/use-received-endorsements.ts:346; src/hooks/use-profile-responses.ts:177; src/hooks/use-bluesky-follows.ts:137 +- **Evidence:** Focus listeners call the fetch path with no signal, so the `if (signal?.aborted)` guard is always false; a focus event near unmount can setState after unmount (useReceivedEndorsements/useBlueskyFollows call component setState directly). Inconsistent with §25's AbortController discipline. +- **Recommendation:** Give the focus handler its own ref'd AbortController aborted on cleanup/next focus, or guard with an `aliveRef`. +- **Testable:** yes +- **Risk of change:** low + +### quality-033 — hooks-fetch-4: useBskyPosts.loadMore can append a superseded page when the handle changes mid-fetch +- **Severity:** low +- **Confidence:** low +- **Files:** src/hooks/use-bsky-posts.ts:266, :230 +- **Evidence:** `requestIdRef` is shared across initial-load and loadMore; loadMore reads cursor/hasMore from closure and appends without re-checking the handle still matches, so a stale loadMore finishing between two initial fetches can mis-attribute. `aliveRef` covers unmount but not handle-change-while-mounted. +- **Recommendation:** Capture the handle at call time and compare before setPosts, or clear posts + bump the token atomically on handle change. +- **Testable:** yes +- **Risk of change:** low + +### quality-034 — hooks-fetch-5: useEndorsementLists mutation callbacks read possibly-stale `lists` via closure +- **Severity:** low +- **Confidence:** low +- **Files:** src/hooks/use-endorsement-lists.ts:276, :294, :332, :464 +- **Evidence:** update/delete/add callbacks close over `lists` and call `lists.find(...)` against the closure snapshot rather than the functional updater; an await before the optimistic merge means a concurrent refetch can be clobbered. No `listsRef` mitigation, and every mutation rebinds all callbacks. +- **Recommendation:** Read current list inside the functional updater (or a `listsRef`), drop `lists` from deps. +- **Testable:** yes +- **Risk of change:** medium + +### quality-035 — hooks-fetch-6: useExploreData loadMore passes signal:null, so a filter change can't cancel the in-flight page +- **Severity:** low +- **Confidence:** low +- **Files:** src/hooks/use-explore.ts:361-362 +- **Evidence:** The generation guard prevents bad state, but loadMore passes `signal: null`, so a filter change during loadMore fetches-then-discards rather than cancelling — wasted network on a network-heavy route. +- **Recommendation:** Pass an AbortSignal tied to generation into loadMore's loadPage. Low priority. +- **Testable:** no +- **Risk of change:** low + +### quality-036 — hooks-state-2: promoted object-URL preview is never revoked after save +- **Severity:** low +- **Confidence:** medium +- **Files:** src/hooks/use-profile-inline-edit.ts:674-688, :690-691 +- **Evidence:** On save the preview URL is promoted to localAvatarUrl/localBannerUrl and the preview refs nulled without `revokeObjectURL`; once the resolve-did refetch returns the CDN URL, `effectiveAvatarUrl` still resolves to localAvatarUrl (never cleared), so the blob URL leaks for the page lifetime. handleCancelEdit revokes correctly; the save path doesn't. +- **Recommendation:** Revoke the prior local blob URL on a fresh refetch/unmount and clear localAvatarUrl once the canonical prop changes. +- **Testable:** no +- **Risk of change:** low + +### quality-037 — hooks-state-4: useOrgProfile.refetch exposes fetchData directly (onClick passes the event as an AbortSignal) +- **Severity:** low +- **Confidence:** medium (confirmed; tag changed bug→quality) +- **Files:** src/hooks/use-org-profile.ts:25-58, :75 +- **Evidence:** `refetch: fetchData` whose first param is `signal?`; an `onClick={refetch}` would pass the MouseEvent as the signal. Harmless today (no caller), but fragile vs useProfile.refetch which wraps to take no args. Verifier: latent/harmless → quality, not bug. +- **Recommendation:** `const refetch = useCallback(() => fetchData(), [fetchData])`. +- **Testable:** yes +- **Risk of change:** low + +### quality-038 — hooks-state-6: usePendingAwardsCount returns 0 (not null) when logged out, contradicting its JSDoc contract +- **Severity:** low +- **Confidence:** high +- **Files:** src/hooks/use-pending-awards-count.ts:62, :30-35 +- **Evidence:** JSDoc says it returns null when logged out; implementation returns 0. Both consumers treat 0 and null as "hide," so no current defect, but a future consumer distinguishing them would misbehave. +- **Recommendation:** Return null when logged out (or update the JSDoc to say 0). Prefer null for consistency with the cold-cache/loading branches. +- **Testable:** yes +- **Risk of change:** low + +### quality-039 — hooks-state-7: useBottomSheetDrag visualViewport effect mutates inline styles but never resets them on cleanup +- **Severity:** low +- **Confidence:** medium +- **Files:** src/hooks/use-bottom-sheet-drag.ts:56-79 +- **Evidence:** The resize handler writes maxHeight/bottom; cleanup only removes the listener. Because the node is reused across open/close, a reopened sheet can briefly show a stale clamped height/offset until the next resize. +- **Recommendation:** Reset `maxHeight=''` and `bottom=''` in cleanup. +- **Testable:** no +- **Risk of change:** low + +### quality-040 — hooks-state-8: useBottomSheetDrag setTimeout(onClose, 250) can fire after unmount +- **Severity:** low +- **Confidence:** low +- **Files:** src/hooks/use-bottom-sheet-drag.ts:108-111 +- **Evidence:** The dismiss timeout id is not stored/cleared; if the component unmounts within 250ms, onClose (an arbitrary caller callback) still fires. Benign for React state but a sharp edge for non-state callbacks. +- **Recommendation:** Store the id in a ref and clearTimeout in cleanup. +- **Testable:** no +- **Risk of change:** low + +### quality-041 — hooks-state-9: useOrgMarker.refresh can return a stale value when a concurrent fetch is in-flight +- **Severity:** low +- **Confidence:** medium +- **Files:** src/hooks/use-org-marker.ts:159-187, :52-58 +- **Evidence:** refresh deletes the cache entry but not the inFlight map entry; if another mount has a fetch in flight, fetchOrgMarker returns the pre-existing promise resolving to the pre-refresh value. The editor's post-save refreshOrgMarker could observe the pre-save record on a concurrent-mount page. +- **Recommendation:** On refresh, also `inFlight.delete(did)` (or force a no-cache fetch ignoring the dedupe). +- **Testable:** yes +- **Risk of change:** low + +### quality-042 — hooks-state-5: useProfilePds in-flight dedupe note (kept as a stylistic alignment; the bug claim was refuted) +- **Severity:** low +- **Confidence:** low (the duplicate-fetch claim was refuted; only an IIFE-style alignment remains) +- **Files:** src/hooks/use-profile-pds.ts:40-57 +- **Evidence:** `inflight.get(did) ?? resolvePdsUrl(did)` eagerly evaluates resolvePdsUrl. Verifier refuted the concurrency claim (React effects flush sequentially; the lower-layer `fetchDidDocument` dedupes the network call anyway, and the hook has a single call site). No observable defect. +- **Recommendation:** Optional only — mirror fetchOrgMarker's register-before-await IIFE for readability. No behavioral benefit. *(Consider skipping in Phase 2.)* +- **Testable:** yes (low value) +- **Risk of change:** low + +### quality-043 — profile-lists-endorse-2: ProjectItemRow shows a permanent skeleton when a project fails to load +- **Severity:** low *(originally bug/low; treated as quality terminal-UI gap — verifier confirmed)* +- **Confidence:** high (confirmed) +- **Files:** src/components/profile/profile-lists.tsx:629-646; src/hooks/use-project.ts:95 +- **Evidence:** The render gate `if (!project)` returns the loading skeleton, but useProject resolves to `{project:null, isLoading:false, error}` on 404/error. The row renders the skeleton forever with no error fallback; non-owners get a permanent grey bar. Cert/account variants fall back to "Untitled cert"/"Unknown". +- **Recommendation:** Gate the skeleton on `isLoading`, render a fallback row when `!project && !isLoading` (URI tail or "Project unavailable", remove button still available to owners). +- **Testable:** yes +- **Risk of change:** low +- **Note:** Original tag bug/low; kept as quality because it is a missing terminal-UI state, behavior-preserving to fix. + +### quality-044 — profile-lists-endorse-3: duplicated outside-click + Escape dropdown handler instead of useClickOutsideClose +- **Severity:** low +- **Confidence:** high +- **Files:** src/components/profile/profile-endorsements.tsx:205-223, :228-246; src/components/profile/endorsement-lists.tsx:121-139; src/hooks/use-click-outside-close.ts:23-61 +- **Evidence:** Three hand-rolled mousedown/keydown+ref effects exist while the repo ships `useClickOutsideClose` (whose docstring calls itself the drop-in replacement). The inline copies miss the onClose-ref optimization and re-attach listeners on re-render. +- **Recommendation:** Replace each with `useClickOutsideClose` anchored on the existing `*__sort-wrap` div. +- **Testable:** no +- **Risk of change:** medium + +### quality-045 — profile-lists-endorse-4: three near-identical CreateListModal / bulk-paste modals duplicated across the two list files +- **Severity:** low +- **Confidence:** medium +- **Files:** src/components/profile/profile-lists.tsx:721-826, :854-1060; src/components/profile/endorsement-lists.tsx:702-813, :1000-1282 +- **Evidence:** Each file defines its own create/edit dialog and bulk-paste modal with parallel parse-row state machines, the same `split(/[\s,]+/)` parser, and shared `profile-lists__paste-*` classes. CLAUDE.md rule 10 warns against exactly this drift. +- **Recommendation:** Extract a shared create/edit dialog + bulk-paste shell (parameterized by resolve fn + status-label map). *(Pure-extraction is behavior-preserving; if caller signatures must change it becomes judgment.)* +- **Testable:** no +- **Risk of change:** medium + +### quality-046 — profile-lists-endorse-5: createdAt comparators never return 0, making the sort unstable for equal timestamps +- **Severity:** low +- **Confidence:** medium +- **Files:** src/components/profile/endorsement-lists.tsx:1289-1293; src/components/profile/profile-endorsements.tsx:148, :1216-1218 +- **Evidence:** `(a,b) => a.createdAt > b.createdAt ? -1 : 1` returns 1 on equality, so same-second items can shuffle between renders. A correct three-way `compareString` already exists in the same path. +- **Recommendation:** Use the three-way `compareString` (return 0 on equality) for all createdAt comparators. +- **Testable:** yes +- **Risk of change:** low + +### quality-047 — profile-lists-endorse-6: raw at-URI used as the accessible label for item checkboxes +- **Severity:** low +- **Confidence:** medium +- **Files:** src/components/profile/profile-lists.tsx:454 +- **Evidence:** `aria-label={`Select ${uri}`}` reads the full DID+collection+rkey to screen readers; the surrounding row already resolves a human title. +- **Recommendation:** Pass the resolved title (or row index) to the label. +- **Testable:** no +- **Risk of change:** low + +### quality-048 — dataloss-priv-4: sidebar EndorseButton clears optimistic state and rethrows when list-append fails +- **Severity:** low +- **Confidence:** medium +- **Files:** src/components/profile/profile-sidebar.tsx:814, :825, :833 +- **Evidence:** The award is created first; if the optional list-append throws, the catch does `setOptimistic(null)` and rethrows before `ownGiven.refetch()`, so the button snaps back to "Endorse" even though the award succeeded — nudging a duplicate endorsement. +- **Recommendation:** Run `ownGiven.refetch()` before the list-append; on append failure keep optimistic=true and surface only the list-attribution error. +- **Testable:** yes +- **Risk of change:** low + +### quality-049 — feed-deadcode-2: CertHeadlineByline is dead code (superseded by inline CertHeadlineColumns) +- **Severity:** low +- **Confidence:** high +- **Files:** src/components/feed/cert-headline-byline.tsx:31 +- **Evidence:** Zero importers; ActivityDetail uses an inline CertHeadlineColumns. Stale duplication of author-byline logic. +- **Recommendation:** Delete the file (or extract one shared byline cell). +- **Testable:** no +- **Risk of change:** low + +### quality-050 — feed-deadcode-3: LocationCard is dead code (only referenced in a comment) +- **Severity:** low +- **Confidence:** high +- **Files:** src/components/feed/location-card.tsx:30 +- **Evidence:** Only reference is a prose comment in cert-locations-map.tsx; cert detail renders all locations via the consolidated CertLocationsMap. +- **Recommendation:** Delete location-card.tsx and fix the inaccurate comment. +- **Testable:** no +- **Risk of change:** low + +### quality-051 — feed-staledoc-1: FeedLayout doc comments reference non-existent GlobalFeed/PersonalFeed +- **Severity:** low +- **Confidence:** high +- **Files:** src/components/feed/feed-layout.tsx:31; src/components/feed/user-feed.tsx:13 +- **Evidence:** JSDoc cites GlobalFeed/PersonalFeed/UserFeed; only the dead UserFeed exists. Real consumers are profile-certs.tsx and project-detail.tsx. +- **Recommendation:** Update the comments to the real consumers; drop the phantom names. (Coordinate with quality-001.) +- **Testable:** no +- **Risk of change:** low + +### quality-052 — feed-img-state-1: ActivityCard never resets imageFailed when imageUrl changes +- **Severity:** low +- **Confidence:** medium +- **Files:** src/components/feed/activity-card.tsx:27, :40 +- **Evidence:** imageFailed is only set true and never reset on imageUrl change; ActivityDetail solved the same with `useEffect(()=>setImageFailed(false),[baseImageUrl])`. Masked today by `key={record.uri}`, but breaks on instance reuse with a mutated record. +- **Recommendation:** Add `useEffect(() => setImageFailed(false), [imageUrl])`. +- **Testable:** yes +- **Risk of change:** low + +### quality-053 — feed-news-key-1: News post images use array index as React key +- **Severity:** low +- **Confidence:** medium +- **Files:** src/components/right-rail/news-section.tsx:104, :111 +- **Evidence:** `images.slice(0,4).map((img,i)=> )` — index keys, the antipattern the module's `contributorKey()` exists to avoid. Static today, breaks on reorder/filter. +- **Recommendation:** `key={img.thumb}` (unique CDN URL). +- **Testable:** no +- **Risk of change:** low + +### quality-054 — feed-map-modal-resize-1: expanded locations map height computed once from window.innerHeight +- **Severity:** low +- **Confidence:** medium +- **Files:** src/components/feed/cert-locations-map.tsx:286 +- **Evidence:** Modal map height is a one-shot `window.innerHeight*0.7` read with no resize subscription; rotating/resizing while open leaves a stale height. Minor UX. +- **Recommendation:** Drive height from a vh-based CSS container, or comment that it's intentionally fixed at open time. +- **Testable:** no +- **Risk of change:** low + +### quality-055 — feed-card-perf-1: ActivityCard not memoized; every loadMore re-renders all existing cards +- **Severity:** low +- **Confidence:** medium +- **Files:** src/components/feed/feed-layout.tsx:123; src/components/feed/activity-card.tsx:21 +- **Evidence:** loadMore replaces the activities array (new identity), so all prior plain-function ActivityCards re-render. Reconciliation cost only (author info is module-cached), but avoidable on long lists; props are stable per uri. +- **Recommendation:** Wrap ActivityCard in React.memo. +- **Testable:** no +- **Risk of change:** low + +### quality-056 — Remaining low-stakes quality items (token/aria/doc-drift cluster) +- **Severity:** low +- **Confidence:** mixed (each verified; all low-stakes, Phase-2 test-gated) +- **Files:** consolidated; see per-item below +- **Evidence:** A cluster of individually-small, behavior-preserving cleanups, each kept as its own Phase-2 commit: + - **auth-redirect-1 / sec-session-csrf-1 (merged):** `safeRedirect` gates http: on `NODE_ENV === "development"` while the module uses `!== "production"` — fail-closes http loopback under NODE_ENV="test". Fix: `!== "production"`. (auth-context.tsx:30-37) — testable yes. + - **auth-session-fixation-1:** callback-handler swallows deleteSession failure before createSession; add a one-line comment that createSession overwrites the cookie so a failed delete can only orphan a TTL'd Redis key. (callback-handler/route.ts:30-34) — testable yes. + - **auth-csrf-doc-1:** CSRF code rejects missing Origin AND Referer (stricter than AGENTS §8); keep the code, add a comment noting the deliberate divergence so it isn't "fixed" back. (csrf.ts:20-24) — testable yes. + - **api-trust-5:** AGENTS §17.9/§22.5 say "the four" ALLOWED_WRITE_COLLECTIONS but the array has 11; update the doc (and confirm `app.certified.badge.response` write-enablement is intended). (xrpc route:24-48) — doc. + - **cert-5:** `countGraphemes` duplicated verbatim across 4 pages → extract to src/lib/utils. (create:281, project/new:173, project edit:300, activity edit) — testable yes. + - **cert-6:** own-certs quick-pick fetch duplicated between /project/new and project edit → extract `useOwnCerts`. — no test. + - **cert-7:** project-detail reads activity-only meta (startDate/endDate/contributors) the project forms never write; drop or document. (project-detail.tsx:318-333) — no test. + - **map-3:** dead theme reactivity in ThemeReactiveTiles (useTheme/key-by-URL no-op because tile URLs are constant); drop the subscription + misleading comment. (map.tsx:176-208; tiles.ts:36-55) — no test. + - **map-4 / deadcode-5 (merged):** `forwardGeocode` is a dead single-hit export; delete (callers use `suggestForwardGeocode`). (geocode.ts:31) — no test. + - **map-5:** raw hex in map.tsx polygon pathOptions + `#000` in leaflet.css:137 vs tokens (rule 2). Tokenize the CSS hit; comment/justify the JS literals (Leaflet can't read CSS vars from JS). — no test. + - **ui-primitives-2:** ProviderRedirectOverlay hardcodes `zIndex: 9999` vs `--z-skip-nav`/`--z-feedback`; tokenize or pick a dedicated token. (provider-redirect-overlay.tsx:7) — no test. + - **ui-primitives-3:** Skeleton text variant omits `aria-hidden` and ignores the documented `width` prop; add aria-hidden, document/honor width, move `...style` before computed width. (skeleton.tsx:57,67) — testable yes. + - **ui-primitives-4:** Button renders spinner + icon child for `size="icon"` loading, and has no default `type`; hide children when `loading && size==="icon"`, default `type="button"`. (button.tsx:58,64,70) — testable yes. + - **explore-1:** `?attrs=` filter is read+applied but never written by any UI; wire a control or delete the read + filter blocks. (explore.tsx:228-232, 1144-1230) — no test. + - **explore-3:** Sort + quality popover triggers omit `aria-haspopup` (sub-dropdown and home-feed buttons have it). (explore.tsx:674-744) — testable yes. + - **groups-3:** Leave Group modal is hand-rolled (`signin-modal__backdrop`) instead of `` (hard rule 7 / §22.16): no focus trap/Esc/scroll-lock. Replace with ConfirmDialog (already used in org-settings). (groups/page.tsx:255-256) — testable yes. + - **groups-4:** AddOrgModal + MembershipSyncModal are dead code; delete. (add-org-modal.tsx:21; membership-sync-modal.tsx:22) — no test. + - **groups-5:** AddOrgModal inline styles use invariant `--color-primary` + landing `--color-mid-gray` on an app surface (breaks dark mode); resolves if groups-4 deletes it, else use `--fg-primary`/`--fg-muted`. — no test. + - **groups-6:** add-members loop leaves already-added members staged on partial failure; set `pendingMembers` to only the failures. (org-settings.tsx:183-190) — testable yes. + - **profile-edit-2:** BannerUpload `onRemove` is dead in the edit flow (banner non-removable); wire a `bannerRemoved` flag or drop the unused branch. (banner-upload.tsx:121-132; profile-edit-form.tsx:344-348) — testable yes. + - **profile-edit-3:** edit-form inputs fork raw ``/`