Refactor After the Audit, Not Before

There are two different reasons to refactor. The first is that the code looks messy — long functions, duplicated logic, a module that's grown past what its name suggests. That reason is real, but it's also a matter of taste, and taste is exactly what makes a refactor hard to defend when someone asks "why now, and why this, and not something else?" The second reason is that you have evidence: three, four, five separate bugs, filed independently, that all turn out to trace back to the same missing abstraction. That reason doesn't need defending. It comes with its own paper trail.

This repo's five biggest architectural extractions — #170 through #174, tracked under the umbrella review #175 — happened for the second reason, in a roughly ninety-minute window on 2026-08-27. They didn't come first. They came after a deliberate bug audit on 2026-08-25 and two days of fixing what it found, once the pattern in what kept breaking was undeniable. That ordering is the actual argument of this post, and the clearest case study for it is one specific seam that didn't even get one bug wrong before the refactor — it got one wrong and one right, in the same afternoon, for reasons that are still checkable in the pull request that did it.

Why the review happened when it did

#175 is explicit about its own trigger: it's an architecture review of "the dispatch layer (src/index.ts / src/ascii/index.ts) and the src/ascii/ subsystem — the area with the most recent commit churn (the non-null-assertion audit, latent-bug fixes)." Not "this subsystem looks old." Not "let's clean house." The area got reviewed because it was the area generating the most bug-fix commits, full stop — a symptom-driven trigger, not a taste-driven one.

By the time that review happened, one of those bugs already had a name and a fix: #66, closed 2026-08-26, found that the ASCII renderer measured box width in UTF-16 code units instead of terminal display columns, so a label containing CJK text (日本語テスト) produced a box that visibly overflowed its own borders in a real terminal. The fix added a real displayWidth() helper and a maxLineWidth() function that used it. That's one instance of a pattern, not yet a case for restructuring anything — but it's exactly the kind of concrete, line-numbered evidence the review then cited: five separate proposals, each pointing at a real, already-observed divergence between code paths that were supposed to do the same job.

"Visibly overflowed its own borders" is a claim worth showing rather than asserting, and a fenced code block is the one medium that can't show it — a web font draws the broken output and the fixed output identically. So here is that repro through an actual terminal on either side of the fix, recorded with asciinema against the real CLI and rasterized with agg:

flowchart TD
    A[日本語テスト] --> B[終了]

Watch the right-hand border. Before, it's a stray │ stranded outside a box with no corner to meet; after, the box is a rectangle again. That's the single-box path — the only one PR #94 touched.

Two of those five are quick to state. #170 found that src/index.ts (the SVG backend) and src/ascii/index.ts (the ASCII backend) each hand-rolled their own detectDiagramType, and the two had already drifted: one split a diagram's header on newline-or-semicolon, the other on newline only, so flowchart TD;A-->B could classify differently depending on which renderer touched it first. #172 — flagged in the review as its top recommendation — found that the ASCII layout engine's core data structure, AsciiGraph, had no owner for its grid-occupancy invariant: edge-routing.ts and edge-bundling.ts each independently defined a byte-for-byte identical requireGridCoord() null-check, because nothing enforced at the type level that a node's grid coordinate was actually assigned by the time routing code needed it. Its own issue body points at the direct cost of that gap: "the same invariant, patched in a dozen places instead of owned in one," and names the exact prior cleanup effort (issue #100's non-null-assertion audit) that had been mopping up the symptom file by file without ever fixing the cause.

The fixes that shipped for both were narrower than a rewrite. #176 pulled diagram-type detection into one shared module both backends import, closing the classification drift as a side effect. #179 introduced a Grid abstraction (createGrid, isOccupied, isFree) around the occupancy map and moved the deduplicated requireGridCoord guard into types.ts, where it's a pure predicate any module can use without pulling in the rest of grid.ts. #178, for #174, did the same for edge routing: edge-routing.ts and edge-bundling.ts had each independently computed anchor offsets and called the pathfinder directly, and only the regular-edge path had a tryDirectPath fast path before falling back to full A* search. The fix extracted one routeEdge() seam both call, so bundled edges stopped being the slow, unreviewed cousin of regular ones for no functional reason.

#171 is worth a pause because of what it deliberately left alone. The canvas module, src/ascii/canvas.ts, had no bounds-checked write primitive — every drawing module either indexed the array directly and unsafely or reimplemented its own guard, and xychart.ts had built an entire second canvas implementation from scratch rather than reuse the shared one. The fix added a single write() primitive and migrated the direct-indexing call sites to it. But xychart.ts's canvas is described in the merged PR as "partially unified rather than fully merged": its createCanvas/createRoleCanvas now delegate to the shared constructors, but createHexCanvas — a third parallel array tracking per-series color overlays for the chart — stays separate, because that's a genuinely different concern from anything the shared canvas needs to know about. Not every duplicate is accidental. Some of it is a different problem wearing similar-looking code, and the PR says so instead of forcing a merge that would have papered over that difference.

The seam that got tested the same afternoon it was built

#173 is the most direct answer to the CJK-bug-resurfacing story from this series' Unicode post, but not in the tidy way a thesis statement wants it to be. It's a better story than that, because the people doing the refactor got a real decision right, in real time, using evidence that would go on to be independently confirmed within hours.

The problem #173 named: class-diagram.ts and er-diagram.ts both call a shared drawMultiBox primitive, but each carried a verbatim-identical copy of a classifyBoxChar helper. Separately, sequence.ts skipped drawMultiBox entirely, hand-rolling its own actor-box drawing (drawActorBox) instead. The issue asked for two things: dedupe classifyBoxChar between class and ER, and investigate why sequence diagrams were the odd one out — either fold them into the shared primitive too, or document in writing why the divergence is load-bearing.

#177, which closed it, did the first cleanly and refused to do the second. The PR description lays out exactly why, and it's worth quoting the reasoning rather than summarizing it, because the reasoning is the whole point: drawMultiBox sized its content by raw .length and left-aligned it with fixed padding. Actor labels, by contrast, were already sized through maxLineWidth — the same display-width-aware function #66 had introduced the day before — because actor names routinely contain the wide CJK text that function exists to measure correctly. Migrating drawActorBox onto drawMultiBox as it stood would have silently narrowed boxes around wide-character actor labels, in the PR's own words, "reintroducing the exact bug maxLineWidth exists to avoid." So drawActorBox stayed a hand-rolled implementation, and the divergence got a comment explaining precisely why, instead of a forced merge.

That decision turned out to be exactly right, and the confirmation arrived the same day. #182, filed hours later during review of an unrelated PR (#180, the canvas write-primitive refactor), reported that drawMultiBox itself — the very function #173 had just left sequence.ts out of — measured text with line.length and indexed it with line[i], the identical code-unit-counting bug #66 had fixed everywhere else, just not here. It's "the same bug class as #66, which fixed the single-box path... but missed this one," as #182 puts it. The PR that had closed #173 hours earlier is the reason anyone could state that so precisely: it had just documented, in a code comment, that drawMultiBox sizes by raw .length — a fact directly discoverable by whoever reviewed #180 next. The seam work didn't just avoid introducing a new bug into sequence diagrams. Its own paper trail is what made the existing bug in drawMultiBox legible enough to file within the same working session.

Side by side with the #66 capture above, the recurrence is unmistakably the same failure wearing a different diagram type. (The repro below is #182's, written in block-brace form: the issue's one-liner class A { +名前 x } uses an inline-brace syntax the class parser only learned later, and renders as an empty string at 571fb9a^ — which would have made for a very boring screenshot.)

classDiagram
  class A {
    +名前 x
  }

The border is sized for the six code units in +x: 名前, but that row draws in eight terminal columns, so it overruns by exactly the two extra columns 名前 costs. The divider and the bottom edge stop where the code thought the content ended.

The honest coda is that this still isn't where the story ends. #334, closed a week later on 2026-09-02, found CJK/wide-character misalignment in sequence diagrams anyway — but in src/sequence/renderer.ts and src/sequence/layout.ts's own participant-spacing math, a layer above drawActorBox's content sizing, which had never referenced the display-width helpers at all. drawActorBox's box contents were fine, exactly as #177's careful investigation intended. The lifeline spacing around those boxes was a separate piece of arithmetic nobody had looked at yet. Refactoring after an audit doesn't mean the pattern stops recurring the moment you name it. It means each recurrence gets easier to place, because you already know which seam it belongs to and which ones it doesn't.

What "refactor after the audit" actually buys you

None of these five extractions read like a rewrite. Each one is small enough to fit in a single PR, scoped to a specific, already-observed divergence, with a named bug or a named risk as its justification. That's the practical shape of "refactor because five bugs point at the same missing abstraction" instead of "refactor because it feels messy": the fix is exactly as big as the evidence, not bigger.

It's also not a coincidence that #175's own tracking issue already reasons in terms of small interfaces hiding real complexity behind them — its diagrams literally mark a shared Grid/routeEdge/write() seam as a "deep" node (thick, dark) against the "leak" nodes (red-outlined) representing duplicated logic with no shared boundary. That's the same distinction John Ousterhout's A Philosophy of Software Design draws between a deep module (simple interface, real work happening behind it) and a shallow one (an interface about as complicated as what it wraps). #173 is a clean instance of that idea cutting both ways: classifyBoxChar was shallow duplication with no reason to exist twice, so it got merged into one seam. drawActorBox looked like the same kind of duplication from a distance, but turned out to be a genuinely different interface — different coordinate system, different alignment semantics — and merging it would have made the resulting module shallower, not deeper, by forcing one interface to serve two incompatible contracts. Telling those two cases apart is the actual skill here, and it's not something a mechanical dedup pass can do for you.

The general principle holds up under the closest reading this post could give it: refactor after the pattern is proven, not before, and expect the audit that proved it to keep paying rent afterward — in this case, by making the next bug in the same family faster to place, and occasionally by stopping you from creating one yourself.