Conversation
NathanTarbert
left a comment
NathanTarbert
left a comment
There was a problem hiding this comment.
This is the biggest of the five and it holds up well — the schemas are right, the parsed output is actually used, and the tests survive mutation. Three things to fix, and one cross-PR note.
As context: CI has not run on any of your five PRs. Fork contributions need a maintainer to approve the workflow runs, and that has not happened yet, so nothing has gone green through no fault of yours. I ran everything locally: build 10/10, typecheck 10/10, 778 tests passing.
Fix before merge
1. A JSON null body still returns 500. The presence check in apps/web/src/app/api/tickets/route.ts:138-143 reads body.title before safeParse runs, so a body of literal null — valid JSON — throws a TypeError and produces exactly the 500 this PR sets out to remove. A string or array body is fine; only null slips through.
Running safeParse first and deriving the "required" message from the Zod issues fixes it. The same ordering exists in the articles and broadcasts routes; those return 400, but only because both end in a blanket catch { 400 } that swallows it — which is pre-existing and worth its own issue, since it also means a real database failure reports "Invalid request body".
2. page has no ceiling. parsePagination in apps/web/src/lib/validate.ts:66-74 clamps pageSize but only floors page at 1. A very large page computes a skip past MAX_SAFE_INTEGER, which Prisma rejects at the client boundary. Number.isSafeInteger plus a ceiling closes it. The docstring says out-of-range values fall back to defaults, which is true of pageSize and not yet of page.
3. prettier --check fails on 5 of the 6 changed files. pnpm format fixes it — but see the note at the bottom, because this is partly ours.
Cross-PR, and worth sorting before you touch either branch
#271 and #272 both modify apps/web/src/app/api/broadcasts/route.ts and apps/web/src/app/api/docs/articles/route.ts, plus both of their test files. Each is clean against main on its own, so the conflict only appears once one of them lands — git merge-tree on the two branches reports content conflicts in both routes.
More usefully: both PRs implement pagination parsing separately. This one adds parsePagination() to the new lib/validate.ts; #272 inlines the same Number.parseInt + clamp logic directly in each route. Consolidating on the shared helper before either merges saves you resolving a conflict between two copies of your own code.
Smaller
The enum tests assert against hand-copied mocks rather than the real generated enums, and the same 38-line enum block now appears in three test files. Today's copies are accurate — I checked all four ticket enums and both broadcast enums against schema.prisma — so this is about drift later, not a bug now. One test importing the real enums unmocked and comparing key sets would pin it.
Also minor: GET /api/tickets still passes account and assignee ids through without the length cap the POST path applies, and tickets/[id]/route.ts still hand-rolls three enum checks that validate.ts now centralises. Both fine to leave, worth a line in the description so they do not read as oversights.
What I verified, so nobody redoes it
The validation is not decorative. No route parses and then reuses the raw body — every handler assigns parsed.data and reads through it, which is the classic zod mistake and you avoided it. A bad enum in a query string returns 400 and never reaches Prisma.
The schemas match the database exactly: same enum members, and every field with a default is optional while every nullable column is nullish. Error bodies stay { error: string } with a field name and no internal detail, so the existing UI keeps working. The length caps cannot break editing, because PATCH does not accept title or description at all.
zod as a new dependency is done right — same version spec already used elsewhere in the repo, in dependencies rather than dev, and a three-line lockfile delta with no transitive churn.
And the tests are real: loosening an enum, dropping the length caps and widening audience fails six tests across both the schema and route suites.
Not yours
The prettier failures are on files that were already unformatted on main. The format check runs over whole changed files rather than diff hunks, so editing them pulled a pre-existing backlog into scope. We need to decide how to clear that generally — it is going to hit every one of your PRs, and it is not reasonable to ask you to absorb it.
2 participants
POST /api/tickets cast body.priority/type/source straight to Prisma enums, accepted unlimited-length title/description, and passed through account/assignee IDs unchecked. GET /api/tickets cast query params (status/source/priority/type) the same way, so one bad enum value became a Prisma throw and an HTTP 500. POST /api/broadcasts passed an arbitrary audience string into a Prisma enum column, and POST /api/docs/articles accepted unbounded title/content. No zod usage existed anywhere in apps/web.
This PR adds a shared validation module (apps/web/src/lib/validate.ts, zod) with per-route schemas, defensive pagination parsing (non-numeric page/pageSize previously produced NaN and a Prisma throw), enum filters that report unknown values as 400s, and length caps. Legacy success/error shapes are preserved.
Verification (all real, run locally):