Module 02 Slice 3: per-doctor document sharing

[mGasiorek998]

Closes #11

Summary

• New backend endpoints under /documents/:id/shares (GET list, PUT grant, DELETE revoke), all behind requireAuth() + requireRole(['patient']) inline (parent .use('/documents', ...) middleware does not match sub-paths in Hono).
• Three new use cases with discriminated-union failure modes; ownership is checked before any mutation, doctor existence is verified for grant/revoke.
• ts-rest contract entries with UUID-validated path params.
• Web sharing panel: per-document expandable list of all doctors with a Share/Shared toggle, wired through TanStack Query so the panel reflects live state and updates immediately after a toggle.

Test plan

• pnpm verify — typecheck + 85 tests + lint all green.
• 17 new integration tests covering AC1–AC6 (red→green pattern).
• Manual smoke on /patient/documents: log in as a seeded patient, upload a doc, expand "Manage sharing", grant + revoke a doctor, verify the toggle reflects state.

🤖 Generated with Claude Code

[claude]

Claude finished @mGasiorek998's task in 2m 52s —— View job

---

PR Review — Module 02 Slice 3 (per-doctor document sharing)

Verdict: ✅ APPROVE

Solid vertical slice. All AC from issue #11 are covered by code and tests, layering follows ARCHITECTURE_RULES.md, idempotency is implemented at the right layer (DB), and the role/ownership checks are correctly ordered to avoid leaking information about doctor existence to non-owners.

---

Spec compliance (issue #11)

• ✅ GET /documents/:id/shares — returns doctors w/ hasAccess, 404 unknown doc, 403 non-owner — covered (documents.test.ts:424-483).
• ✅ PUT /documents/:id/shares/:doctorId — grant + idempotency verified (documents.test.ts:486-512); uses ON CONFLICT DO NOTHING on document_shares — correct (document-repository.ts:97-102).
• ✅ DELETE /documents/:id/shares/:doctorId — revoke + idempotency verified (documents.test.ts:566-594).
• ✅ DOCTOR_NOT_FOUND for non-doctor target id — covered for both PUT/DELETE.
• ✅ 403 for non-owned document — covered for all three endpoints.
• ✅ 401/403 (auth + role) covered for all three endpoints.
• ✅ pnpm verify reported green by author.

Architecture

• ✅ Layering respected: routes → use cases → infrastructure. Use cases (get-sharing-state.ts, grant-access.ts, revoke-access.ts) carry the discriminated-union failure modes, routes only translate to HTTP. ✅
• ✅ Contract is the source of truth (packages/contracts/src/documents.ts); UUID-validated path params on the contract side. ✅
• ✅ Co-located tests, kebab-case files, PascalCase components, no barrel files, no any. ✅
• ⚠️ Middleware fragility (non-blocking). router.use('/documents', ...) does not match sub-paths in Hono, so each new sub-route has to repeat requireAuth(deps), requireRole(['patient'], deps) inline. The comment at documents-router.ts:87-88 flags this honestly, but a future contributor adding a new sub-route can easily forget. Consider switching to a path-prefixed middleware that matches both the bare path and sub-paths (e.g. add a second router.use('/documents/*', ...)), or factor out const patientOnly = [requireAuth(deps), requireRole(['patient'], deps)] so each sub-route just spreads it. Defer to a follow-up — not blocking this slice.

Security

• ✅ Ownership check (doc.patientId !== callerPatientId) runs before the doctor existence check in both grant-access.ts:18-28 and revoke-access.ts:18-28. This prevents a non-owner from probing whether a doctorId is valid. Correct.
• ✅ Order in getSharingState returns 404 for unknown document and 403 for non-owned, matching the explicit AC — the AC itself accepts this distinction, so the slight existence-leak is intentional.
• ✅ All three endpoints behind requireAuth() + requireRole(['patient']); tested with patient/doctor/no-JWT.
• ℹ️ Mild data-model note (non-blocking): isUserDoctor checks users.role = 'doctor', while listDoctorAccessForDocument joins via the doctors profile table. If a users row has role='doctor' but no matching doctors profile row, grantAccess would succeed but the doctor would be invisible in getSharingState. The seed pipeline creates both atomically, so this is theoretical — flagging only.

Test quality

• ✅ Tests use a real Postgres (no DB mocking) — complies with ARCHITECTURE_RULES.md.
• ✅ External-behavior focused: status codes, response shapes, DB row counts (e.g. expect(rows).toHaveLength(1) for idempotency in documents.test.ts:511).
• ✅ Two distinct patients + two distinct doctors set up for the sharing suite, so cross-tenant 403 cases are exercised against real foreign user IDs (not synthetic UUIDs).
• ✅ clearSharesFor() resets state between tests inside the suite — prevents bleed.
• ⚠️ No frontend tests for SharingPanel (no .test.tsx). The PR's manual-smoke checklist item is unchecked. The vertical-slice rule asks for "schema → API → UI → tests" delivered together; the UI lacks test coverage and hasn't been smoke-tested. Not a hard blocker for this hackathon-pace project, but worth noting.

Suggestions (non-blocking)

1. SharingPanel per-row pending state. pendingDoctorId is computed from grant.variables/revoke.variables, which only track the latest mutation. If a user clicks Doctor A then Doctor B before A's request returns, A's button stops showing the pending state even though A's request is still in flight (SharingPanel.tsx:26-30). Since both ops are idempotent and per-doctor, real-world impact is small, but optimistic updates (or useMutationState to query in-flight mutations per doctor) would cleanly solve both this and the "wait for refetch before UI reflects" delay.
2. PUT … body: c.type<undefined>() in the contract (documents.ts:63) is unusual — typical REST style uses an empty POST or PUT with an empty object. Works, but consider body: z.object({}).optional() for cleaner ts-rest typing if you ever want to add fields later.
3. Error body shape. Spec VR-003/EH-001 mention { error: "...", message: "..." }; current implementation and documentErrorSchema only return { error }. Consistent across the codebase (so not a regression introduced here), but eventually worth aligning the contract with the spec.

---

· branch afk/issue-11