Conversation
NathanTarbert
left a comment
There was a problem hiding this comment.
This is the cleanest of the five. The two things most likely to go wrong in a pagination change were both handled.
Context, since it explains the red checks: CI has not run on any of your five PRs — fork contributions need a maintainer to approve the workflow runs and nobody has yet. I ran it locally: build 10/10, typecheck 10/10, 755 tests passing, lint clean.
What you got right, and it is the hard part
The response shape did not change. Both routes still return { broadcasts, total } and { articles, total }; total just became a real count() instead of array.length, with page/pageSize added alongside. I traced all three callers and every one was updated, and nothing outside apps/web touches these routes.
The convention matches the endpoint that already paginates — same param names, same envelope, same shared DEFAULT_PAGE_SIZE/MAX_PAGE_SIZE, same Promise.all([findMany, count]) shape as /api/tickets. /api/sync/events uses a different convention; following tickets rather than that was the right call.
Your param parsing is actually better than the route you copied. /api/tickets does parseInt(searchParams.get('pageSize') || String(DEFAULT)), so a non-numeric value yields NaN and propagates into take. Your Number.isFinite guard avoids that. Worth porting back separately.
The month math is timezone-correct and arithmetically equivalent. Both new clauses use the existing monthWindow() helper that already feeds the other cards, so the whole page agrees, and it is still findMany plus an in-memory reduction rather than a groupBy — so no rows get dropped by a null field.
Worth addressing
The docs route has no clamp test. Removing the MAX_PAGE_SIZE clamp from apps/web/src/app/api/docs/articles/route.ts leaves all 17 tests green. The broadcasts route has the matching case; docs only got the page/pageSize test. Given that clamp is the only thing between the endpoint and an unbounded pageSize, it is worth pinning — near-copy of the broadcasts one.
Silent truncation at 100 rows. The callers pin pageSize=100 with no pagination UI, so row 101 is unreachable and the total you now return correctly is read by nobody. It is most visible on the docs page, which filters by status client-side over what it received — so with more than 100 articles the "AI Drafts" and "Published" sections show wrong counts rather than just fewer rows. The category sidebar gets its count from a separate unpaginated query, so the two can visibly disagree.
Either is fine as an interim step, but it should be honest: smallest is rendering "Showing 100 of {total}" from the total already in the response; better is pushing the status filter server-side so each section paginates on its own. Happy to take your preference — and fine to defer to a follow-up as long as it is tracked, since the perf win here should not wait on UI work.
One semantic change worth a line in the description. Resolved tickets are filtered by createdAt within the month, not by when they were resolved — so a ticket opened in January and closed in March counts toward January's average resolution time, and one opened in December and closed in January appears in no month at all. That is consistent with totalTickets, which is also createdAt-scoped, so I am not asking you to change it. But "avg resolution time for June" now means tickets created in June, and the description frames this as a pure perf fix.
Out of scope, noting it
None of the new queries are index-backed — Ticket has no index on createdAt, Broadcast has none at all, DocArticle none on the fields it now orders by. Every new query is still strictly better than what it replaced, so this is not a defect in the PR. But "these endpoints now scale" is not fully true without them. Good follow-up issue.
Cross-PR
#271 and #272 both modify the same two routes and their test files. Each is clean against main alone, so the conflict only shows up once one of them lands — and both PRs implement pagination parsing separately, this one inline per route and #271 via a shared parsePagination() in lib/validate.ts. Consolidating on the shared helper before either merges saves resolving a conflict between two copies of the same logic.
Not yours
prettier --check fails on all nine changed files, and all nine already fail on main. The check runs over whole changed files rather than diff hunks, so editing them pulled a pre-existing backlog into scope. Reformatting them here would bury a 142-line change under hundreds of unrelated lines, so we need to sort that out on our side rather than asking you to.
GET /api/broadcasts and GET /api/docs/articles returned the whole table with no take/skip, while /api/tickets and /api/sync/events already paginate. GET /api/dashboard/stats loaded the entire ticket table plus messages into memory on every view (two findMany calls with no where/take), and its averages ignored the month picker beside them.
This PR:
Verification (all real, run locally):