Crux Read real diffs, a PR description, and review comments — spot the design bug a reviewer must catch, judge a PR's reviewability, and rewrite a comment so it states severity and a fix.
Your altitude — climbing toward senior
ZeroJuniorMiddleSenior
You are at senior altitude — in orbit
◷ 14 min
Review is read, not recited. Here are the artifacts you actually face in a PR — a diff, a description, a thread of comments. For each, do what a senior reviewer does: find the defect that tooling can’t, judge whether the change is even reviewable, and turn a vague comment into one the author can act on.
Goal
Practise the reviewer’s loop on real material: separate the undecidable design flaw from the mechanical noise, read PR shape for reviewability, and write feedback that answers how important and what to do.
Snippet 1 — the retry path a reviewer must catch
# PR: "Add billing retry on transient gateway errors"def charge_customer(order_id, amount): record = db.insert_charge(order_id=order_id, amount=amount, status="pending") for attempt in range(3): resp = gateway.charge(amount) # may time out AFTER charging if resp.ok: db.update(record, status="settled") return resp time.sleep(2 ** attempt) # back off and retry db.update(record, status="failed") raise ChargeError(order_id)
Quiz
Completed
CI is green — format, lint, types, and the unit tests all pass. What is the defect review exists to catch here?
Heads-up Backoff timing is a tuning detail, not the defect. The correctness bug is that a retry after a post-charge timeout charges the customer twice — exactly the undecidable, money-path flaw a human review is for.
Heads-up Tests assert the paths the author imagined; they're green precisely because no one tested the timeout-after-success case. This is why review targets intent and the money path, not just whether the suite passes.
Heads-up That's a style/typing nit a linter or type system should own — decidable and cheap. Spending the review on it while the double-charge ships is the inverted-priorities failure the unit warns about.
Snippet 2 — the PR description
Title: Refactor checkout + add coupons + migrate orders tableFiles changed: 47 +1,812 −940Description: "Cleaned up checkout, added coupon support, and migrated theorders table to the new schema. All in one PR so it deploys atomically."
Quiz
Completed
As the assigned reviewer, what is the senior response to this PR before reading a single line of the diff?
Heads-up Attention is a fixed budget that doesn't scale with diff size — past a few hundred lines you skim regardless of effort, and the migration's real risk hides in the unread bulk. The fix is decomposition, not more willpower.
Heads-up Atomic deploy is a real concern, but it's solved by ordering/feature-flagging stacked PRs, not by one unreadable diff. 'Deploys atomically' is the rationalization that produces the rubber stamp on the highest-risk change.
Heads-up Each added reviewer still faces 1,800 lines past their own cognitive limit — you get more skims and more pickup wait, not more detection. The per-person attention limit is the constraint; diff size is the lever.
Snippet 3 — a review comment
Reviewer comment on a 40-line PR, left as one line on the file: "this is confusing, why did you even do it this way"
Quiz
Completed
Why will this comment generate friction, and what is the minimal rewrite that fixes it?
Heads-up Length isn't the problem — clarity is. A short comment that names its severity and a concrete action beats a long one that still leaves the author guessing. The fix is a label plus an actionable ask, not more words.
Heads-up Bluntness that attacks the author and omits severity creates round-trips and erodes the psychological safety that keeps people seeking early feedback. It's slower in expectation, not faster.
Heads-up If there's a genuine readability concern, raising it is correct — review is for maintainability too. The defect is how it's phrased, not that it was raised; tag it and make it actionable.
Snippet 4 — the diff that should never reach a human
- const userName=req.body.userName+ const userName = req.body.userName;- function getUser( id ){return db.find(id)}+ function getUser(id) { return db.find(id); }
Quiz
Completed
These are the only changes in a PR, and a reviewer spent ten minutes hand-checking the spacing and semicolons. What does this reveal about the pipeline?
Heads-up Manual style checking is inconsistent and a recurring per-PR tax; a formatter applies the same rules identically and instantly. Routing decidable checks through a human is the slow, error-prone version of a tool.
Heads-up Formatting changes are fine to land — the point is they should be produced and verified by a formatter in CI, not reviewed by hand. Ban the manual review of them, not the changes.
Heads-up It shows the team lacks a formatting gate, not that the author is deficient. With auto-format on save and a CI block, no human ever sees or judges this class of change.
Recap
Reading is the job: a green CI and passing tests don’t certify the money path, so the reviewer hunts the undecidable defect — here, a non-idempotent retry that double-charges. PR shape is read before the diff: a 1,812-line, three-concern change gets split or stacked because attention won’t scale to it. Comments are judged by whether they state severity and a fix and aim at the code, not the author. And a diff full of spacing fixes hand-checked by a human is a missing formatter gate — the decidable class should never reach review. Diagnose from the artifact, fix at the right layer, keep human attention on what only a human can decide.