From 9181f6390c6b261d9fb6119254e137992de2f25d Mon Sep 17 00:00:00 2001 From: adminbjkai Date: Sat, 11 Apr 2026 23:31:43 -0400 Subject: [PATCH] =?UTF-8?q?chore(review):=202026-04-11=20audit=20=E2=80=94?= =?UTF-8?q?=20doc=20drift,=20a11y=20chip=20toggle,=20playbackFilter=20edge?= =?UTF-8?q?s?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - docs/system.md: remove 'signed outbound webhooks' from 'intentionally missing' (outbound HMAC signing has been live since 2026-04-10 — ADR-003, impl plan, README, and deliver-webhook.ts all agree it's shipped) - docs/contracts.md: - auth/setup: doc 400 -> code 409 (+ explicit 201 success shape) - auth/login: add documented 429 + Retry-After branch - auth/me: doc 500 -> code 404 - videos/:id/delete: add concrete response shape and the idempotent-repeat note - components/transcript-card/TranscriptControls.tsx: speaker-filter chips now expose aria-pressed + action-framed aria-label (a11y parity with the 2026-04-10 ARIA video-rail-tab fix) - components/TranscriptCard.speaker-selection.test.tsx: +1 test locking aria-pressed toggle behavior - components/player-card/playbackFilter.test.ts: +5 edge-case tests (duration guard, clamp, out-of-order input, non-finite/degenerate, filter-off passthrough) - docs/status.md, docs/cap5_implementation_plan.md: record the new coverage - docs/review-2026-04-11.md: dated review doc for this pass - scripts/ship-review-2026-04-11.sh: helper used to land these edits safely --- README.md | 3 +- .../TranscriptCard.speaker-selection.test.tsx | 20 ++ .../player-card/playbackFilter.test.ts | 85 ++++++ .../transcript-card/TranscriptControls.tsx | 7 +- docs/cap5_implementation_plan.md | 1 + docs/contracts.md | 13 +- docs/review-2026-04-11.md | 107 ++++++++ docs/status.md | 2 + docs/system.md | 1 - scripts/ship-review-2026-04-11.sh | 252 ++++++++++++++++++ 10 files changed, 486 insertions(+), 5 deletions(-) create mode 100644 docs/review-2026-04-11.md create mode 100644 scripts/ship-review-2026-04-11.sh diff --git a/README.md b/README.md index d63d377..18bf198 100644 --- a/README.md +++ b/README.md @@ -105,4 +105,5 @@ That starts: - [docs/status.md](docs/status.md) — current gaps and next improvement areas - [docs/auth-plan.md](docs/auth-plan.md) — current auth status and constraints - [docs/review-auth-system.md](docs/review-auth-system.md) — dated auth-system code-review snapshot -- [docs/review-2026-04-10.md](docs/review-2026-04-10.md) — dated full-repo review + changelog (most recent) +- [docs/review-2026-04-11.md](docs/review-2026-04-11.md) — dated full-repo review + changelog (most recent) +- [docs/review-2026-04-10.md](docs/review-2026-04-10.md) — previous dated full-repo review + changelog diff --git a/apps/web/src/components/TranscriptCard.speaker-selection.test.tsx b/apps/web/src/components/TranscriptCard.speaker-selection.test.tsx index 9489b19..2530d3a 100644 --- a/apps/web/src/components/TranscriptCard.speaker-selection.test.tsx +++ b/apps/web/src/components/TranscriptCard.speaker-selection.test.tsx @@ -80,4 +80,24 @@ describe("TranscriptCard speaker selection", () => { expect(screen.getByText("No speakers selected.")).toBeTruthy(); expect(screen.queryByText("Guest reply.")).toBeNull(); }); + + it("exposes aria-pressed state on each speaker-filter chip and flips it on toggle", () => { + const view = renderTranscriptCard(); + + const chips = view.container.querySelectorAll("button.speaker-filter-chip"); + expect(chips.length).toBe(2); + // Both chips start selected (aria-pressed=true). + chips.forEach((chip) => { + expect(chip.getAttribute("aria-pressed")).toBe("true"); + }); + + fireEvent.click(chips[0]!); + + // After toggling Host off, the first chip flips to aria-pressed=false; the + // second chip stays pressed. aria-label also flips to match. + expect(chips[0]!.getAttribute("aria-pressed")).toBe("false"); + expect(chips[0]!.getAttribute("aria-label")).toBe("Show Host"); + expect(chips[1]!.getAttribute("aria-pressed")).toBe("true"); + expect(chips[1]!.getAttribute("aria-label")).toBe("Hide Guest"); + }); }); diff --git a/apps/web/src/components/player-card/playbackFilter.test.ts b/apps/web/src/components/player-card/playbackFilter.test.ts index 92e44f2..0211fe1 100644 --- a/apps/web/src/components/player-card/playbackFilter.test.ts +++ b/apps/web/src/components/player-card/playbackFilter.test.ts @@ -95,4 +95,89 @@ describe("playbackFilter", () => { expect(getPlaybackCorrection(0, ranges)).toBe(5); }); + + it("returns no ranges when durationSeconds is non-positive", () => { + const ranges = buildPlayableSpeakerRanges({ + durationSeconds: 0, + transcriptSegments: [ + { startSeconds: 1, endSeconds: 2, speaker: 0 }, + ], + selectedSpeakerIds: new Set([0]), + speakerFilteringActive: true, + }); + expect(ranges).toEqual([]); + }); + + it("clamps segment boundaries to [0, durationSeconds]", () => { + const ranges = buildPlayableSpeakerRanges({ + durationSeconds: 10, + transcriptSegments: [ + // Start before 0 and end after duration — should clamp, not drop. + { startSeconds: -2, endSeconds: 100, speaker: 0 }, + ], + selectedSpeakerIds: new Set([0]), + speakerFilteringActive: true, + segmentStartPaddingSeconds: 0, + segmentEndPaddingSeconds: 0, + segmentMergeGapSeconds: 0, + }); + expect(ranges).toEqual([{ startSeconds: 0, endSeconds: 10 }]); + }); + + it("sorts input segments by start time before merging", () => { + // Out-of-order input simulates a transcript that arrived with reordered + // segments (e.g. after an edit). buildPlayableSpeakerRanges must normalize. + const ranges = buildPlayableSpeakerRanges({ + durationSeconds: 30, + transcriptSegments: [ + { startSeconds: 10, endSeconds: 12, speaker: 0 }, + { startSeconds: 4, endSeconds: 6, speaker: 0 }, + { startSeconds: 6.05, endSeconds: 8, speaker: 0 }, + ], + selectedSpeakerIds: new Set([0]), + speakerFilteringActive: true, + segmentStartPaddingSeconds: 0, + segmentEndPaddingSeconds: 0, + segmentMergeGapSeconds: 0.1, + }); + expect(ranges).toEqual([ + { startSeconds: 4, endSeconds: 8 }, + { startSeconds: 10, endSeconds: 12 }, + ]); + }); + + it("drops non-finite or degenerate segments without crashing", () => { + const ranges = buildPlayableSpeakerRanges({ + durationSeconds: 30, + transcriptSegments: [ + { startSeconds: Number.NaN, endSeconds: 3, speaker: 0 }, + { startSeconds: 5, endSeconds: 5, speaker: 0 }, // zero-length collapses after padding + { startSeconds: 10, endSeconds: 12, speaker: 0 }, + ], + selectedSpeakerIds: new Set([0]), + speakerFilteringActive: true, + segmentStartPaddingSeconds: 0, + segmentEndPaddingSeconds: 0, + segmentMergeGapSeconds: 0, + }); + expect(ranges).toEqual([{ startSeconds: 10, endSeconds: 12 }]); + }); + + it("passes through every segment when speakerFilteringActive is false", () => { + const ranges = buildPlayableSpeakerRanges({ + durationSeconds: 30, + transcriptSegments: [ + { startSeconds: 0, endSeconds: 2, speaker: 0 }, + { startSeconds: 2, endSeconds: 4, speaker: 1 }, + ], + // Empty selection would normally hide everything, but filtering is off + // so everything is playable. + selectedSpeakerIds: new Set(), + speakerFilteringActive: false, + segmentStartPaddingSeconds: 0, + segmentEndPaddingSeconds: 0, + segmentMergeGapSeconds: 0, + }); + expect(ranges).toEqual([{ startSeconds: 0, endSeconds: 4 }]); + }); }); diff --git a/apps/web/src/components/transcript-card/TranscriptControls.tsx b/apps/web/src/components/transcript-card/TranscriptControls.tsx index 1967a56..aef064f 100644 --- a/apps/web/src/components/transcript-card/TranscriptControls.tsx +++ b/apps/web/src/components/transcript-card/TranscriptControls.tsx @@ -199,10 +199,15 @@ export function TranscriptControls({ )} {speakerIds.map((speaker) => { const isHidden = hiddenSpeakers.has(speaker); + const label = getSpeakerLabel(speaker); return ( ); })} diff --git a/docs/cap5_implementation_plan.md b/docs/cap5_implementation_plan.md index 35c1fe2..b2c016d 100644 --- a/docs/cap5_implementation_plan.md +++ b/docs/cap5_implementation_plan.md @@ -85,6 +85,7 @@ In progress. - tests for cleanup-artifact key collection and no-object path - tests for `claimOne()` + `reclaimExpiredLeases()` (parameters, SQL path, batch-size env) - `WORKER_CLAIM_BATCH_SIZE` is now explicitly reserved/dormant; reclaim was split out to a dedicated `WORKER_RECLAIM_BATCH_SIZE` +- edge-case unit coverage for `buildPlayableSpeakerRanges` (duration guard, boundary clamp, out-of-order inputs, non-finite/degenerate segments, filter-off passthrough) ### Remaining work 1. expand delete + retry lifecycle coverage beyond focused unit tests diff --git a/docs/contracts.md b/docs/contracts.md index a20c7e3..7d29447 100644 --- a/docs/contracts.md +++ b/docs/contracts.md @@ -51,7 +51,8 @@ Response: Behavior: - succeeds only if zero users exist in the database -- returns 400 if attempted when a user already exists +- returns 201 on success with `{ ok: true, userId }` +- returns 409 if attempted when a user already exists - hashes password with bcrypt (cost 12) ### `POST /api/auth/login` @@ -76,6 +77,7 @@ Response: Behavior: - returns 401 on invalid email or password +- returns 429 with a `Retry-After` header when throttled by login rate limiting - sets `cap5_token` as an httpOnly, Secure, SameSite=Strict cookie - token is a JWT signed with HS256, valid for 7 days by default @@ -108,7 +110,7 @@ Response: Behavior: - requires valid JWT in `Authorization: Bearer ` header or `cap5_token` cookie - returns 401 if not authenticated -- returns 500 if user record no longer exists (shouldn't happen in normal operation) +- returns 404 if the user record no longer exists (shouldn't happen in normal operation) ### Authentication headers @@ -214,11 +216,18 @@ Behavior: ### `POST /api/videos/:id/delete` Soft delete. +Response: + +```json +{ "ok": true, "videoId": "uuid", "deletedAt": "2026-04-11T00:00:00Z" } +``` + Behavior: - requires `Idempotency-Key` - sets `videos.deleted_at` - queues `cleanup_artifacts` delayed by 5 minutes +- idempotent: calling on an already-deleted video returns the original `deletedAt` without re-queueing cleanup ### `POST /api/videos/:id/retry` Retry path for transcription / AI work. diff --git a/docs/review-2026-04-11.md b/docs/review-2026-04-11.md new file mode 100644 index 0000000..e201611 --- /dev/null +++ b/docs/review-2026-04-11.md @@ -0,0 +1,107 @@ +# cap5 — full-repo review and changelog + +**Date:** 2026-04-11 +**Scope:** Follow-up audit after the 2026-04-10 review. Doc-vs-code drift, small accessibility fix on the speaker filter, expanded unit coverage around selected-speaker playback, and an inventory of on-disk cruft that still needs host-side cleanup. +**Verification:** `pnpm typecheck` + `pnpm test` all green after changes. The new tests run locally in `apps/web`. + +This file is a point-in-time record. For ongoing status, see `docs/status.md`; for sequencing, see `docs/cap5_implementation_plan.md`; for the previous review, see `docs/review-2026-04-10.md`. + +## Verdict + +The repo is still in materially good shape. No critical bugs, no security-incident-class issues. The previous review's big structural edits (Docker migrate switched to the Node runner, `WORKER_CLAIM_BATCH_SIZE` split into claim + reclaim, `formatDuration`/`formatTimestamp` dedupe, S3 endpoint defaults aligned, claim/reclaim test coverage, Web E2E ARIA-tab alignment) are all still in place and correct. The speaker-filter playback feature that landed in the last commit on `chore/review-2026-04-10` (`1729942 feat(web): ship speaker-filter playback impl that test 9 depends on`) is coherent and the code matches the docs it produced. + +What this pass found were the smaller things left behind by a busy merge week: three doc claims that disagreed with the live code, one ARIA gap on the new speaker-filter chips, and five-ish playbackFilter edge cases worth locking in before anyone else refactors that module. + +## Changes applied in this review + +### 1. `docs/system.md` — "signed outbound webhooks" removed from "Intentionally missing" + +`docs/system.md` listed "signed outbound webhooks" in the *Intentionally missing or incomplete* section at the bottom of the file. That has not been true since outbound HMAC signing shipped — ADR-003 in the same file documents the inbound/outbound HMAC shape; `apps/worker/src/lib/hmac.ts::signOutboundWebhook` is the live implementation; `apps/worker/src/handlers/deliver-webhook.ts` uses it; `README.md` lists "outbound notification webhooks … with HMAC headers" under *What works now*; and `docs/cap5_implementation_plan.md` lists signed outbound webhook delivery as Phase 1 / completed. Four places said shipped, one place said missing. The missing-one was wrong; removed. + +### 2. `docs/contracts.md` — auth status codes corrected to match `apps/web-api/src/routes/auth.ts` + +Three small corrections, all verified against the live route handler: + +- `POST /api/auth/setup` — doc said "returns 400 if attempted when a user already exists". Code returns **409** (`reply.code(409).send({ error: 'Account already exists' })` at `auth.ts:80`). Also added the missing success shape `{ ok: true, userId }` with explicit 201 status (`auth.ts:97`). +- `POST /api/auth/login` — added the 429 branch. Login has rate limiting via `InMemoryLoginRateLimiter` and returns 429 with a `Retry-After` header when throttled (`auth.ts:119`). This was live but undocumented in the contracts file, so anyone writing a client would treat 429 as unexpected. +- `GET /api/auth/me` — doc said "returns 500 if user record no longer exists". Code returns **404** (`auth.ts:195`). + +### 3. `docs/contracts.md` — `POST /api/videos/:id/delete` response shape made explicit + +The delete contract described behavior but did not show the response shape. Added the concrete `{ ok, videoId, deletedAt }` response and called out that calling delete on an already-deleted video is idempotent — it returns the original `deletedAt` without re-queueing `cleanup_artifacts`. That idempotent path is important for anyone building a retry-happy client because otherwise they'd see cleanup jobs accumulating on repeated delete calls (they don't). + +### 4. Accessibility: `aria-pressed` on speaker-filter chips + +`apps/web/src/components/transcript-card/TranscriptControls.tsx` renders the speaker-filter chips as plain `