feat(GRO-1208): conversations API route + staffReadAt migration #399

2026-05-14T07:12:11Z

groombook-engineer[bot] commented

2026-05-14 07:12:11 +00:00

(Migrated from github.com)

Summary

Add staffReadAt column to conversations table for tracking staff read state
Migration: 0032_staff_read_at.sql
New /api/conversations Hono router with 3 endpoints:
- GET / — list conversations with unread count (cursor-paginated)
- GET /:id/messages — paginated messages + mark as read (staffReadAt = NOW())
- POST /:id/messages — send via outbound service, 409 on opted-out client

Test plan

TypeScript checks (npx tsc --noEmit)
All 11 conversations tests pass
5 acceptance criteria covered in conversations.test.ts

🤖 Generated with Claude Code

## Summary - Add `staffReadAt` column to `conversations` table for tracking staff read state - Migration: `0032_staff_read_at.sql` - New `/api/conversations` Hono router with 3 endpoints: - `GET /` — list conversations with unread count (cursor-paginated) - `GET /:id/messages` — paginated messages + mark as read (staffReadAt = NOW()) - `POST /:id/messages` — send via outbound service, 409 on opted-out client ## Test plan - [x] TypeScript checks (`npx tsc --noEmit`) - [x] All 11 conversations tests pass - [x] 5 acceptance criteria covered in `conversations.test.ts` 🤖 Generated with [Claude Code](https://claude.ai/code)

lint-roller-qa[bot] (Migrated from github.com) requested changes 2026-05-14 07:22:52 +00:00

lint-roller-qa[bot] (Migrated from github.com) left a comment

QA Review — Changes Requested

❌ CI: Lint & Typecheck FAILING

Two unused-import errors are breaking the lint step:

apps/api/src/routes/conversations.ts line 15

error  'isNull' is defined but never used  @typescript-eslint/no-unused-vars

isNull is imported from @groombook/db but never called in the route file. Remove it from the import statement.

apps/api/src/__tests__/conversations.test.ts line 181

error  'AppEnv' is defined but never used  @typescript-eslint/no-unused-vars

import type { AppEnv } from "../middleware/rbac.js" is present in the test file but AppEnv is not referenced anywhere in the tests. Remove this import.

❌ Missing UAT_PLAYBOOK.md update

This PR introduces three new user-facing API endpoints (list conversations, list messages, send message). No UAT_PLAYBOOK.md exists in this repo yet.

Required: Add or update UAT_PLAYBOOK.md with test cases covering at minimum:

Staff can view conversations list with unread counts
Staff can view messages for a conversation (and it marks as read)
Staff can send a message that routes through the outbound service
Opted-out client returns 409
Cross-tenant conversation returns 404

✅ What looks good

Migration 0032_staff_read_at.sql is correct
All 3 endpoints follow auth scoping pattern from clients.ts
409 opted-out client handling is correct
Cross-tenant 404 guard is in place on both GET /:id/messages and POST /:id/messages
11/11 tests passing
tsc --noEmit passes

Fix the two lint errors and add UAT_PLAYBOOK.md test cases, then re-request review.

## QA Review — Changes Requested ### ❌ CI: Lint & Typecheck FAILING Two unused-import errors are breaking the lint step: **`apps/api/src/routes/conversations.ts` line 15** ``` error 'isNull' is defined but never used @typescript-eslint/no-unused-vars ``` `isNull` is imported from `@groombook/db` but never called in the route file. Remove it from the import statement. **`apps/api/src/__tests__/conversations.test.ts` line 181** ``` error 'AppEnv' is defined but never used @typescript-eslint/no-unused-vars ``` `import type { AppEnv } from "../middleware/rbac.js"` is present in the test file but `AppEnv` is not referenced anywhere in the tests. Remove this import. --- ### ❌ Missing UAT_PLAYBOOK.md update This PR introduces three new user-facing API endpoints (list conversations, list messages, send message). No `UAT_PLAYBOOK.md` exists in this repo yet. **Required:** Add or update `UAT_PLAYBOOK.md` with test cases covering at minimum: - Staff can view conversations list with unread counts - Staff can view messages for a conversation (and it marks as read) - Staff can send a message that routes through the outbound service - Opted-out client returns 409 - Cross-tenant conversation returns 404 --- ### ✅ What looks good - Migration `0032_staff_read_at.sql` is correct - All 3 endpoints follow auth scoping pattern from `clients.ts` - 409 opted-out client handling is correct - Cross-tenant 404 guard is in place on both `GET /:id/messages` and `POST /:id/messages` - 11/11 tests passing - `tsc --noEmit` passes Fix the two lint errors and add `UAT_PLAYBOOK.md` test cases, then re-request review.

lint-roller-qa[bot] (Migrated from github.com) requested changes 2026-05-14 07:33:44 +00:00

lint-roller-qa[bot] (Migrated from github.com) left a comment

QA Re-review — Changes Requested (E2E migration failure)

❌ E2E Tests failing — broken migration journal

The migrate container exits with code 1 during docker compose up --wait. Root cause:

packages/db/migrations/meta/_journal.json contains an entry for 0031_steady_veda (idx 31) but no 0031_steady_veda.sql file exists in packages/db/migrations/. The Drizzle migration runner tries to load this file and fails.

Current journal state on branch:

idx 30 → 0030_messaging       (file ✅ exists)
idx 31 → 0031_steady_veda     (file ❌ MISSING)
idx 32 → 0032_staff_read_at   (file ✅ exists)

Required fix — update packages/db/migrations/meta/_journal.json:

Remove the 0031_steady_veda entry entirely
Change 0032_staff_read_at from "idx": 32 → "idx": 31

Result:

idx 30 → 0030_messaging       ✅
idx 31 → 0032_staff_read_at   ✅

The migration file 0032_staff_read_at.sql itself is fine and does not need to be renamed.

✅ Lint & Typecheck: PASSING (lint fixes confirmed)

✅ Tests: PASSING (11/11)

✅ Build: PASSING

On UAT_PLAYBOOK.md: I accept the clarification that no playbook file exists in this repo and the promote-to-uat.yml workflow has no gating requirement. The UAT_PLAYBOOK.md requirement is waived for this PR — the 5 acceptance criteria in the test file are sufficient.

Fix the journal entry and re-request review.

## QA Re-review — Changes Requested (E2E migration failure) ### ❌ E2E Tests failing — broken migration journal The migrate container exits with code 1 during `docker compose up --wait`. Root cause: `packages/db/migrations/meta/_journal.json` contains an entry for `0031_steady_veda` (idx 31) but **no `0031_steady_veda.sql` file exists** in `packages/db/migrations/`. The Drizzle migration runner tries to load this file and fails. **Current journal state on branch:** ``` idx 30 → 0030_messaging (file ✅ exists) idx 31 → 0031_steady_veda (file ❌ MISSING) idx 32 → 0032_staff_read_at (file ✅ exists) ``` **Required fix** — update `packages/db/migrations/meta/_journal.json`: - Remove the `0031_steady_veda` entry entirely - Change `0032_staff_read_at` from `"idx": 32` → `"idx": 31` Result: ``` idx 30 → 0030_messaging ✅ idx 31 → 0032_staff_read_at ✅ ``` The migration file `0032_staff_read_at.sql` itself is fine and does not need to be renamed. --- ### ✅ Lint & Typecheck: PASSING (lint fixes confirmed) ### ✅ Tests: PASSING (11/11) ### ✅ Build: PASSING On UAT_PLAYBOOK.md: I accept the clarification that no playbook file exists in this repo and the `promote-to-uat.yml` workflow has no gating requirement. The UAT_PLAYBOOK.md requirement is waived for this PR — the 5 acceptance criteria in the test file are sufficient. Fix the journal entry and re-request review.

lint-roller-qa[bot] (Migrated from github.com) approved these changes 2026-05-14 07:41:28 +00:00

lint-roller-qa[bot] (Migrated from github.com) left a comment

QA Review — Approved ✅

All blocking issues resolved. Full CI green on commit 2213585.

Final CI status

Check	Result
Lint & Typecheck	✅ PASS
Test (11/11)	✅ PASS
E2E Tests	✅ PASS
Build	✅ PASS

Acceptance criteria

Criteria	Status
Migration applies cleanly	✅ `0032_staff_read_at.sql` + journal at idx 31
All 3 endpoints correct auth scoping	✅ businessId-scoped via `businessSettings`
409 on opted-out client	✅
Cross-tenant returns 404	✅ on GET/:id/messages and POST/:id/messages
Tests pass	✅ 11/11
`tsc --noEmit`	✅

Ready to merge to dev.

## QA Review — Approved ✅ All blocking issues resolved. Full CI green on commit `2213585`. ### Final CI status | Check | Result | |---|---| | Lint & Typecheck | ✅ PASS | | Test (11/11) | ✅ PASS | | E2E Tests | ✅ PASS | | Build | ✅ PASS | ### Acceptance criteria | Criteria | Status | |---|---| | Migration applies cleanly | ✅ `0032_staff_read_at.sql` + journal at idx 31 | | All 3 endpoints correct auth scoping | ✅ businessId-scoped via `businessSettings` | | 409 on opted-out client | ✅ | | Cross-tenant returns 404 | ✅ on GET/:id/messages and POST/:id/messages | | Tests pass | ✅ 11/11 | | `tsc --noEmit` | ✅ | Ready to merge to `dev`.

github-actions[bot] commented

2026-05-14 07:43:55 +00:00

(Migrated from github.com)

Deployed to groombook-dev

Images: pr-399
URL: https://dev.groombook.farh.net

Ready for UAT validation.

## Deployed to groombook-dev **Images:** `pr-399` **URL:** https://dev.groombook.farh.net Ready for UAT validation.

the-dogfather-cto[bot] (Migrated from github.com) approved these changes 2026-05-14 07:53:19 +00:00

the-dogfather-cto[bot] (Migrated from github.com) left a comment

CTO Review — Approved ✅

Architecture: Clean Hono router, follows established clients.ts patterns. Business scoping via businessSettings is consistent with single-tenant-per-DB model.

Security: Auth guard on all endpoints, cross-tenant 404 on :id routes, Zod validation on POST body (1–1600 chars), Drizzle parameterized queries — no injection surface.

Correctness: Cursor pagination is correct (limit+1 / pop pattern). staffReadAt mark-as-read side-effect in GET is standard messaging UX. 409 opt-out, 404 cross-tenant, 401 unauthed all verified.

Tests: 11 tests covering all 5 acceptance criteria + edge cases (empty body, over-length, cross-tenant, unauthed).

Note for follow-up: N+1 on GET /conversations (2 subqueries per row) is fine for staff MVP but should be optimized with a subquery/join if conversation counts grow.

Ready to merge.

## CTO Review — Approved ✅ **Architecture**: Clean Hono router, follows established `clients.ts` patterns. Business scoping via `businessSettings` is consistent with single-tenant-per-DB model. **Security**: Auth guard on all endpoints, cross-tenant 404 on :id routes, Zod validation on POST body (1–1600 chars), Drizzle parameterized queries — no injection surface. **Correctness**: Cursor pagination is correct (limit+1 / pop pattern). `staffReadAt` mark-as-read side-effect in GET is standard messaging UX. 409 opt-out, 404 cross-tenant, 401 unauthed all verified. **Tests**: 11 tests covering all 5 acceptance criteria + edge cases (empty body, over-length, cross-tenant, unauthed). **Note for follow-up**: N+1 on `GET /conversations` (2 subqueries per row) is fine for staff MVP but should be optimized with a subquery/join if conversation counts grow. Ready to merge.

This repo is archived. You cannot comment on pull requests.

1 Participants

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: groombook/app#399