Last updated on

Nine Copies of the Same Try/Catch


Part 2 of 4 in the series Architecture Review: Blog Surface Deepening Opportunities. Full series ↓

Second in a series about running an automated architecture-review skill (Matt Pocock's improve-codebase-architecture, covered in the first post) against this blog's own codebase.

The single most-edited file in this entire codebase, by a wide margin, is the collection that defines a blog post. Every time a post's category changes, or a tag is added, or a post is deleted, a handful of cached pages need to be told they're stale: the post's own page, the blog index, the category and tag archives it belongs to, and, if the post is part of a series, every sibling page that shows its title in a banner.

Four collections needed a version of this: posts, categories, tags, and the series banner itself. Each one wired up its own Payload lifecycle hooks, and each one had copied the same two shapes to do it:

  1. A query-then-refresh function: look up whatever rows reference the changed id, then tell Next.js to revalidate the path for each one.
  2. A try { ... } catch (error) { console.error(...) } wrapper around the whole thing, because a cache-refresh failure should never be allowed to fail the actual database save it's attached to.

Six near-identical copies of the first shape, and roughly nine near-identical copies of the second, spread across four files.

Flowchart showing the four collections (Posts, Categories, Tags, Series) each wrapped in its own try/catch calling a dedicated revalidate function — seven distinct functions across nine near-identical try/catch blocks, highlighted in red as duplicated.

Collapsing that down was the most "Strong"-rated, lowest-risk candidate in the whole report: mechanical, and it directly reduces what the next similar collection (there's already more than one candidate) would otherwise have to copy a fifth time. Two shared helpers replaced all of it: a generic query-and-revalidate function, and a logging wrapper around the try/catch.

Flowchart showing the same four collections after the refactor, all routing through two shared helpers: withRevalidationLogging for the try/catch wrapper, and revalidateArchivesByIds for the query-and-revalidate logic.

The part that shouldn't have been thin

The interesting part of this one wasn't the collapsing. It was what the collapsing accidentally revealed.

Payload's hook signatures are awkward to unit-test directly; they expect a realistic request context that a plain test double can't easily fake. The existing convention in this codebase for that problem is a coverage-exclusion comment around the thin wiring, on the theory that the decision logic lives elsewhere, already tested, and the wiring itself is just plumbing.

When I collapsed the try/catch wrappers, I wrapped the new, smaller wiring in exactly that convention. It seemed reasonable: the actual revalidation logic was already covered by direct tests, and what was left really did look like plumbing.

It wasn't, quite. A second adversarial review pass on the pull request pointed out that the excluded region also covered the one piece of real logic still sitting in the hook: the check for whether a post's published visibility had actually changed. Posts on this blog can be saved as drafts, and a plain draft-to-draft edit should never touch the public cache. Only a save that lands on "published," or one that just got unpublished, should. That two-line check was the entire reason a draft save doesn't leak to the live site, and it had ended up inside the excluded region along with the genuinely-thin plumbing around it. Nothing was testing it, and because it was excluded from coverage, nothing was even reporting that nothing was testing it.

The risk was concrete: a later edit that narrowed the check, say dropping the "just unpublished" case, would silently stop clearing the cache when a post goes back to draft, and the coverage gate would stay green the whole time, because the lines it should have flagged were never in the report to begin with.

I pulled the check out into its own small function, gave it a direct test covering all five cases (draft-to-draft, draft-to-published, published-to-draft, and the two first-save variants), and moved the coverage exclusion so it only covers the wiring that's actually just wiring.

That finding also prompted a wider look at whether the "thin adapter, needs a real fixture" reasoning held up anywhere else it had been used in this codebase. Narrowing each hook's parameter type down to the small slice of Payload it actually reads turned out to make all of it directly testable with plain fake objects, no realistic fixture required. All nine copies of that try/catch, and every other hook of the same shape, ended up with real tests and zero coverage exclusions.

Next: the front end's query layer, and a duplicate pattern that could have quietly served draft content to the public.

Comments

No comments yet. Be the first to comment.

Leave a comment