The code review that should have been a conversation

TL;DR#

A pull request accumulated 41 comments across six review rounds over nine days, and the author rewrote large parts of it twice. Every individual comment was reasonable. The outcome was an approved design that neither the author nor the reviewer would have chosen from scratch.

The comment count was the signal, and we read the comments instead. Past roughly ten substantive comments, a review has stopped being a review — it is a design negotiation conducted in the worst available medium: asynchronous, line-anchored, and with one party holding merge rights.

The fix is a threshold. When a review crosses it, stop commenting and have the conversation.

The case this post is about#

A 900-line pull request introducing a sync engine, reviewed the normal way: the author opened it, the reviewer went through it line by line.

PR #3120  "Add offline sync engine"
  +912 −34, 14 files
  review rounds: 6
  comments: 41  (28 substantive, 13 nits)
  days open: 9
  force-pushes: 11

The reviewer was senior, careful, and right about most things. The author was experienced and responsive. Nobody behaved badly, which is what makes this worth writing about.

The symptom#

The architecture changed twice, inside the review. Round two asked for a different conflict resolution strategy; round four asked for the queue to be pluggable:

round 1  naming, error handling, 9 comments
round 2  "should conflicts be last-write-wins or merge?"  → author rewrites the resolver
round 3  tests for the new resolver, 6 comments
round 4  "the queue should be an interface so we can swap it"  → author adds an abstraction
round 5  "actually the interface has only one impl, is it needed?"  ← contradicts round 4
round 6  nits, approved

Round five contradicting round four is the clearest symptom: the reviewer had changed their own mind after seeing the consequence, which is a normal part of design and an expensive part of review.

The result satisfied nobody. Post-merge, the author said they would have built it differently; the reviewer said they had approved it because it was "good enough and it had been nine days".

The nine days were not nine days of work. They were mostly latency:

author active time:    ~11 hours
reviewer active time:   ~3 hours
elapsed:                 9 days

Fourteen hours of work spread over nine days, because each round waited on the other person's availability.

And a third engineer's concern never surfaced. They had built something similar the previous year and would have said so in a meeting. They were not on the review.

Why the obvious fix failed#

"Review faster." The latency was real and it was not the cause. A six-round design negotiation compressed into two days is still a design negotiation in a comment thread.

"Write smaller pull requests." Generally right, and this one was genuinely one change — the sync engine does not decompose into four independently mergeable pieces without inventing seams nobody wanted. "Make it smaller" is not always available.

"Agree the design in a doc first." This is the fix, and the version we tried next failed: a two-page design doc that nobody engaged with until the pull request existed, at which point the same 41 comments happened. A doc that is not actually read is a delay, not a design process.

"Let the author decide; reviewers only block on correctness." Tempting, and it discards the reviewer's genuine knowledge. The round-two question about conflict resolution was important — the problem was the medium, not the question.

The actual mechanism#

Code review is good at one class of question and bad at another, and the two are easy to confuse because they arrive through the same interface.

Review is good at local, verifiable questions. Is this null-safe? Is this error swallowed? Does this name match the domain? Is there a test? These are anchored to a line, have a clear answer, and converge in one round.

Review is bad at design questions. Should conflicts merge or last-write-wins? Should the queue be pluggable? These are not local, have trade-offs rather than answers, and require the participants to think together — which a comment thread actively prevents, for three reasons:

It is asynchronous with high latency. Each exchange costs hours or a day. Design discussions need rapid back-and-forth to converge.

It is anchored to lines. A comment attaches to code that exists, so the conversation is biased toward modifying what is written rather than considering what is not. Nobody proposes "what if we did not need a queue at all" on line 212.

It is asymmetric. The reviewer holds merge rights, and the author has sunk cost in working code. That shapes the outcome toward "modify until acceptable" rather than "choose the best design" — which is exactly the result we got.

The useful consequence is that the two classes are distinguishable from a thread's shape, without reading it — so the escalation does not have to depend on somebody noticing in the moment, which is precisely what nobody did for nine days:

DESIGN_MARKERS = (
    "should we", "why not", "alternative", "trade-off", "tradeoff",
    "what if", "instead of", "in general", "long term", "i'd argue",
)

def triage(thread, now):
    """One review thread → keep | escalate. Called when a new comment lands."""
    depth = len(thread.comments)
    authors = {c.author for c in thread.comments}
    age_hours = (now - thread.comments[0].created_at).total_seconds() / 3600

    # Depth is the strongest signal and it is cheap. A local question converges in one
    # round; anything past three exchanges is not converging through this interface.
    if depth >= 4 and len(authors) >= 2:
        return "escalate", f"{depth} comments over {age_hours:.0f}h without converging"

    latest = thread.comments[-1].body.lower()
    if any(marker in latest for marker in DESIGN_MARKERS):
        # Deliberately requires depth >= 2. A single "why not X?" is often answered in
        # one line, and escalating those teaches people to avoid the phrasing.
        if depth >= 2:
            return "escalate", f"design language in a {depth}-comment thread"

    # A thread spanning more than a working day has already cost more in latency than
    # the call would have, regardless of what it is about.
    if age_hours > 24 and depth >= 3:
        return "escalate", f"open {age_hours:.0f}h across {depth} comments"

    return "keep", ""

Run over the pull request that prompted all this, replaying its threads in order:

thread  topic                               verdict    reason
#1      null check on `remoteId`            keep
#2      conflict resolution: merge vs LWW    escalate   design language in a 2-comment thread
#3      naming: `SyncJob` vs `SyncTask`      keep
#4      should the queue be pluggable?       escalate   5 comments over 31h without converging
#5      missing test for the retry path      keep

Two escalations out of five threads, and they are the two that accounted for 34 of the 41 comments and five of the six rounds. Thread #2 would have been caught on its second comment — roughly a day and a half before anyone suggested a call.

Past about ten substantive comments, the thing happening is design, and a line-anchored asynchronous thread with an approval gate is the worst medium available for it. The count is the signal; reading the comments to decide is reading the wrong data.

The fix#

A threshold with a named action, agreed by the team in advance so invoking it is routine rather than an escalation.

The rule, written into the team's review guide:

## Review escalation

If a pull request reaches EITHER
  - 10 substantive comments (excluding nits and praise), or
  - a third review round,
then the reviewer STOPS commenting and books 30 minutes with the author.

This is not a failure by either person. It is the review working — it found
a design question, which a thread cannot resolve.

In the call: screen-share the code, agree the approach, write the decision in
the PR description. Then review the implementation of the agreed design.

Four things make it work:

It is a count, not a judgement. Nobody has to decide whether a disagreement is "big enough" to escalate, which is the decision people avoid making. Ten comments is observable.

"Not a failure" is stated explicitly. Without that, invoking it reads as either the author being bad at design or the reviewer being obstructive. Naming it as the expected outcome of a review finding something real is what makes people use it.

The decision is written back into the pull request, so the design rationale lives with the code and the review that follows is about implementation:

## Design decision (agreed 2026-10-27, author + reviewer + one other)
Conflicts: last-write-wins, with a per-field timestamp. Merge semantics
considered and rejected — no field in the current model has a sensible merge,
and adding one later is additive. See the call notes in ADR-033.
Queue: concrete class. No second implementation is planned; extract an
interface when one exists.

And the call invites anyone with relevant history. This is what would have surfaced the third engineer's prior work.

One more change that mattered as much: separate the two kinds of comment visibly, so the count means something:

nit: prefer `items` over `itemList`                    ← non-blocking, author's call
question: is this null-safe when the cache is cold?    ← needs an answer
design: should this be pluggable?                      ← triggers the threshold

A design: prefix makes the escalation condition self-evident, and it also lets a reviewer flag a design thought without implying it blocks the merge.

How we proved it#

That the threshold fires on the right pull requests. Applied retrospectively to six months of history:

PRs reviewed                              214
PRs reaching 10+ substantive comments      17  (8%)
of those, PRs where the ARCHITECTURE changed during review   14
of the other 197, PRs where architecture changed during review  2

Eight percent of pull requests, and the threshold captures 14 of the 16 cases where a design actually changed mid-review. That is the evidence that the count correlates with the thing we care about.

That it reduces elapsed time, on the 23 pull requests that crossed it in the following four months:

                          before (17 historical)   after (23)
median review rounds              5                   2
median days open                  7.5                 2.0
median comments                  31                  12

Two rounds and two days, because the design round happened in a 30-minute call instead of three asynchronous exchanges.

And the outcome quality, which is the harder claim:

post-merge survey, "would you have built it this way from scratch?"
  author says yes:    before 6/17   after 20/23
  reviewer says yes:  before 9/17   after 21/23

Self-reported and therefore soft — but the before numbers are strikingly low, and they match what both people said about #3120 unprompted.

One honest failure of the rule. On three of the 23, the call happened and the design question was not resolvable in 30 minutes — it needed a prototype. In those cases the right move was to close the pull request, spike both options for a day, and reopen. We added that explicitly:

If 30 minutes does not resolve it, the answer is a timeboxed spike, not a
longer call and not more comments.

One limit worth stating: the comment-count threshold is tuned to this team's norms. A team that leaves many short comments would need a higher number, and a team that reviews in person already would not need the rule. The transferable part is having a threshold with a named action, not the number ten.

What this generalizes to#

Match the medium to the kind of question. Local and verifiable: asynchronous text is ideal. Trade-offs and alternatives: synchronous, with the ability to sketch. Most process friction is a question being asked in a medium that cannot answer it.

The volume of a discussion is data about the discussion. Comment count, thread depth, message count in a channel, length of a meeting. When you find yourself deep in the content trying to decide what to do, step back and read the shape instead.

Make the escalation routine and blameless, or nobody uses it. A rule that requires someone to judge a colleague's design as inadequate will not be invoked. A counter that triggers a calendar invite will be.

Write the decision where the code lives. The 30-minute call produces the most valuable artifact in the whole process — the rationale, including the option rejected. In a thread it is scattered across 41 comments; in the pull request description it is findable in a year.

Related#

• An estimate is a distribution, and you reported the mode — another signal hiding inside a process
• The test that broke on every refactor was testing the wrong thing — what review keeps asking about
• A stale doc does more damage than a missing one — where decisions should live

Keep reading#

This is part of 100 practices of senior engineers — the habits that separate five years of experience from one year five times. Follow MobileByteSensei for the rest.