Scenarios: code review from both chairs

Code review is where tone gets tested most, because someone is judging your work in writing and everyone can read the thread. This chapter is a dense bank of real exchanges shown from both chairs: you as the author taking notes, and you as the reviewer or the person who found the problem. The move is always the same, soften the person and sharpen the substance, so every reply here carries a fact, a next step, or a reason, not just politeness. Skim for the situation you are in and steal the line.

Someone criticizes your code in review

• Reviewer flags a magic number. Q: "Why 86400 here?" A: "Fair, that is a day in seconds. Pulling it into SECONDS_PER_DAY now."
• Naming nitpick you agree with. Q: "data2 does not tell me anything." A: "Agreed, renaming to normalizedRows. Pushing in a sec."
• Reviewer thinks your function is too long. Q: "This handler is 140 lines, hard to follow." A: "Yeah. I will split the validation and the S3 write into their own functions and re-request."
• Blunt one-liner on your PR. Q: "This will N+1 the DB." A: "Good catch, it does. Batching the lookup into one WHERE id IN query, fix incoming."
• Reviewer questions a whole approach. Q: "Why polling instead of the SNS webhook we already have?" A: "Honestly I did not know the webhook existed. Let me look, that is probably cleaner."
• Style comment you find petty but cheap. Q: "We use early returns here, not nested ifs." A: "Sure, no strong feeling. Flattened it."
• Reviewer catches a missing test. Q: "No test for the empty-list case?" A: "You are right, added one and it caught a real bug in the reducer. Thanks."
• Harsh phrasing, valid point. Q: "This is completely wrong, timezones will break it." A: "Tone aside, the point stands. I am storing local time instead of UTC. Fixing to store UTC and convert at the edge."
• Reviewer asks for a comment. Q: "This regex is unreadable, can you explain it?" A: "Adding a comment with an example match. It strips the v prefix off semver tags."
• Senior reviewer pushes back on a pattern. Q: "We are trying to move away from singletons like this." A: "Did not know that was the direction. Happy to inject it instead, is there an example PR I can copy?"
• Reviewer flags a swallowed error. Q: "You are catching and ignoring the exception here." A: "That was lazy of me. Logging it and re-raising so the retry layer can see it."
• Nit on a huge PR, you are tired. Q: "Trailing whitespace on line 212." A: "Got it, and I will turn on format-on-save so this stops happening. Thanks for the patience."
• Reviewer says your abstraction is premature. Q: "This interface has one implementation, why?" A: "Fair, I was future-proofing for a case that may never come. Inlining it, we can extract later if we need to."
• You disagree but it is not the hill. Q: "Prefer map over the for loop here." A: "No real preference, switched. Substance is the same."
• Reviewer points at a security smell. Q: "You are logging the full request including the auth header." A: "That is bad, thank you. Redacting the header before it hits the logger."

You think the review feedback is wrong

• Reviewer wants a change that breaks a contract. Q: "Just return null if the user is missing." A: "I would push back on that one. Callers expect a thrown NotFound so they can 404. Returning null would silently 200. Keep the throw?"
• You have context the reviewer lacks. Q: "Why not just cache this in memory?" A: "We run 6 pods, so an in-memory cache goes stale per pod. That is why it is in Redis. Should I add a comment saying so?"
• Reviewer misread the diff. Q: "You deleted the retry logic." A: "It only moved: it is now in withRetry on line 88, wrapping the call. Same behavior, want me to note that in the description?"
• Feedback based on an old requirement. Q: "Spec says cap it at 50 results." A: "That was the old spec. Product bumped it to 200 in the ticket last Tuesday. Linking the comment so we are aligned."
• Reviewer wants gold-plating. Q: "Should handle the case where the file is 10GB." A: "Real, but these uploads are capped at 25MB at the gateway. I would rather not add code for an input that cannot arrive. Open to being wrong."
• Performance claim you can measure. Q: "This sort will be slow." A: "I benchmarked it: 4ms on the p99 payload size. I think this is fine, but happy to look again if you have a bigger input in mind."
• Reviewer prefers a pattern that hurts here. Q: "Use the repository pattern like everywhere else." A: "Normally yes. This is a one-off migration script that runs once and gets deleted, so I kept it flat on purpose. Fine to leave?"
• You suspect the reviewer is guessing. Q: "Pretty sure this leaks a connection." A: "It should not: the using block closes it on every path including the throw. Can you point at the line where you see it staying open?"
• Disagreement worth a quick call. Q: "I really think this belongs in the shared lib." A: "We are going back and forth in text. Two-minute huddle? Easier to draw the dependency direction than type it."
• Reviewer is right on style, wrong on logic. Q: "Rename it and also flip this condition." A: "Renaming, yes. But flipping the condition would let unverified users through, that guard is intentional. Keeping the logic, changing the name."
• You defer after checking. Q: "I think the lock ordering here can deadlock." A: "I did not believe it at first, but you are right, two callers can grab them in opposite order. Reordering to always take accounts first. Good eye."
• Standing your ground politely. Q: "This should be a config flag." A: "I hear it, but every flag is a branch we test forever. This has never needed to change in 3 years. I would keep it a constant unless a real second value is coming."

Don't be confused: disputing feedback is not refusing it. "I would push back" opens a conversation and invites the reviewer to correct you; "no" closes one. Bring the reason and the escape hatch ("open to being wrong", "keep the throw?") so the other person can move without losing face. For the deeper version of holding a position, see Chapter 26.

You found a serious bug in someone else's code

• Off-by-one in their loop. Q: "Line 40 looks like it drops the last row, off-by-one?" A: "Ugh, yes, good catch. < len should be <= len. Fixing now."
• You are the reviewer, spotting a race. Q: "Two requests can both pass this check before either writes, right? Looks like a TOCTOU race." A: "Oh no, yeah. I will wrap it in a transaction with SELECT ... FOR UPDATE."
• Money bug, stay calm. Q: "This rounds with floats, so cents will drift. Can we use integer cents or Decimal?" A: "Agh, on billing code too. Switching to minor units. Thank you for catching it before prod."
• Reviewer finds a missing auth check. Q: "This endpoint does not check that the order belongs to the caller, any logged-in user can read any order." A: "That is serious. Adding the ownership check and a test. Should we flag it to security?"
• You find a silent data-loss path. Q: "If the write to eu-west fails, we still ack the message and drop it. Intended?" A: "Definitely not. It should nack and go to the DLQ. Fixing the ack ordering."
• Reviewer catches an unbounded query. Q: "This SELECT * with no limit will pull the whole table once it grows. Paginate?" A: "Yeah, that will fall over at scale. Adding keyset pagination."
• Framing severity without alarm. Q: "One blocker before I approve: the token never expires, so a leaked one is valid forever. Everything else is nits." A: "Right, setting a 15-minute TTL and a refresh path. Appreciate you separating that from the small stuff."
• You spot a swapped argument. Q: "transfer(to, from) here, but the signature is (from, to). Money is going the wrong way." A: "Wow. Yes. That would have been a very bad Monday. Swapping and adding a named-argument call."
• Reviewer finds a regression they can prove. Q: "This breaks the existing null-name case, I ran the old test against your branch and it fails. Want the repro?" A: "Please send it. I will add it to the suite so it does not slip again."
• Gently, on a junior's PR. Q: "Looking good. One real issue: the password is compared with == instead of a constant-time check, which leaks timing. Want to pair on the fix?" A: "I did not know that was a thing, yes please."
• Their fix does not fully fix it. Q: "I think the bug is still here: you guard the read but not the write path on line 120." A: "You are right, I only patched half of it. Covering the write path too."
• Serious but you might be wrong. Q: "This might be nothing, but does the retry re-send the payment if the timeout fires after the charge succeeds? Could double-charge." A: "Not nothing at all. It can. Adding an idempotency key. Really glad you asked."

Someone found a serious bug in yours

• Own it fast, no defensiveness. Q: "Your change is dropping every event with a null user_id, that is 12% of traffic." A: "That is on me. Reverting now, then I fix it forward with the null handled. Sorry for the noise."
• Bug found in prod, from your PR. Q: "The p99 latency doubled after your deploy at 14:00." A: "Looking now. Almost certainly my new N+1. Rolling back this minute, root cause to follow in the channel."
• Reviewer catches it pre-merge, be grateful. Q: "This would have deleted the parent row and orphaned the children." A: "Thank you, genuinely. Adding the cascade and a test that would have caught it. Good save."
• Embarrassed but stay useful. Q: "You left a console.log that dumps the whole user object." A: "Embarrassing, removing it. Also adding a lint rule so I stop shipping these."
• You doubt the bug is yours. Q: "Your service is returning 500s." A: "On it. Quick check: the 500s start at 14:02 and my deploy landed 14:00, so likely mine. Rolling back to confirm."
• The fix will take a while, say so. Q: "The export is producing corrupt CSVs since your merge." A: "Confirmed, it is the quoting change I made. Real fix is about an hour. Disabling the export button behind a flag now so no one hits it meanwhile."
• Blameless but you caused it. Q: "Prod is down and it points at your migration." A: "Yes, my migration locked the table. Killing the query and rolling it back. Once we are green I will write it up so we add a lock timeout to the runbook."
• Reviewer finds you copied a bug. Q: "You copied the same off-by-one from the old function." A: "Ha, inherited the bug. Fixing both the copy and the original since I am in here."
• Defensiveness creeping in, catch it. Q: "This clearly was not tested." A: "Fair hit. It was not tested for the empty case. Adding the test and the fix, and I will run the full suite before re-requesting."
• You disagree it is a bug, check anyway. Q: "I think your cache never invalidates." A: "Let me verify before I argue. ... You are right, the TTL is set but the write path never busts it. Fixing."
• Found after you logged off. Q: "Your feature is throwing for EU users, saw it after you left." A: "Thanks for flagging, I am back on. It is the locale parser. Hotfix going up now, and I will backfill the failed requests."
• Closing the loop well. Q: "Did the timezone bug get fixed?" A: "Yes: fix merged in #4821, backfilled the 340 affected rows, added a regression test. Writing a two-line note for the retro."

Someone changed your code without discussing it

• They rewrote your function in their PR. Q: "I refactored your parser while I was in there." A: "Appreciate the help. Can you walk me through why? I had it that way to handle the escaped-quote case, want to make sure we kept that."
• A force-push clobbered your commits. Q: "I rebased and force-pushed your branch." A: "Heads up, that dropped my last two commits from this morning. I have them locally, re-pushing. Can we avoid force-push on shared branches?"
• Silent change landed on main. Q: "Why did the retry count go from 3 to 10?" A: "That was not me. Git blame says it changed in #4790 with no comment. Pinging the author before we assume it was intentional."
• They edited your file to unblock themselves. Q: "Had to tweak your config to get my thing working." A: "Totally fine to unblock yourself. One ask: drop a line in the PR on what you changed so I am not surprised when tests move. What did you touch?"
• Reasonable change, wrong process. Q: "I fixed a typo in your comment and merged." A: "No issue with the change itself. For anything past a typo, a quick ping first saves us both a surprise. This one is fine."
• Their change reintroduced your old bug. Q: "The double-fire is back." A: "The idempotency guard I added got removed in a refactor last week. Restoring it and adding a test that fails loudly if it goes again."
• You changed theirs, own the process. Q: "I went ahead and changed your validation to match the new schema." A: "I should have pinged you first, sorry. Here is exactly what I changed and why, tell me if I broke an assumption you had."
• Ask before you accuse. Q: "Someone deleted the feature flag I was using." A: "Before I assume the worst: was it deprecated in the cleanup ticket? Checking the PR history, then I will ask the author what the plan was."
• Set a boundary for next time. Q: "I'll just push directly to your branch to speed things up." A: "Rather you did not push to my branch mid-review. Open a PR against it and I will merge in a minute, keeps the history clean for both of us."

Someone requests major changes late

• Approved, then a big ask lands. Q: "Actually, can we make this generic for all providers, not just Stripe?" A: "That is a real expansion, roughly two more days. I would ship the Stripe version now since it is approved and tested, and open a follow-up for the generic one. Work for you?"
• Late scope from the manager. Q: "Before this merges, add audit logging to every call." A: "Doable, but it is its own piece of work with its own review. Bolting it on here delays the fix people are waiting on. Split it into a fast-follow?"
• Reviewer wants a rewrite Friday at 5. Q: "I would restructure this whole module." A: "I hear the case. Given it is Friday and this blocks QA, can we merge as-is and I take the restructure first thing Monday? Opening the ticket now so it does not get lost."
• Late change that is genuinely important. Q: "This needs rate limiting before it goes out." A: "You are right, that is not optional for a public endpoint. That changes the timeline: adding it and re-requesting tomorrow rather than today. Flagging the slip to standup."
• Distinguish blocker from wish. Q: "A few thoughts: rename these, and also I think it should be event-driven." A: "Happy to rename now. The event-driven redesign is a bigger call. Is that a blocker for you, or a future direction? Want to size it right."
• You are the reviewer asking late, own it. Q: "Sorry, I know this is late, but the error format needs to match the API standard." A: "No problem, fair one. Changing the shape now, re-requesting in 20."
• Pushing back on a moving target. Q: "One more thing, can you also handle refunds here?" A: "That is the third addition on this PR. I would rather land what we agreed and take refunds as a clean follow-up, otherwise this never merges. Fair?"
• Late ask with a real deadline behind it. Q: "Legal now needs a consent checkbox before launch." A: "If legal needs it, it goes in. That pushes merge to tomorrow AM. I will let QA know the branch is not ready tonight."
• Timing the conversation up. Q: "I think the data model is wrong but it is a big change." A: "If the model is actually wrong, better to know now than after launch. Fifteen minutes tomorrow so I understand it, then I re-plan. See Chapter 11 if we need to move the date."

Work you finished is no longer needed

• Feature cut after you shipped the PR. Q: "We decided not to do the referral feature after all." A: "No worries, it happens. Want me to close the PR, or park it behind a disabled flag in case it comes back? Your call."
• Manager softens the news. Q: "Sorry, priorities shifted and your migration is on hold." A: "All good, I would rather stop now than build more of the wrong thing. I will document where I left it so picking it up later is cheap."
• Sunk cost, handle it well. Q: "That whole module we can just delete now." A: "A little painful after three days, but if it is dead weight it should go. Deleting it. The load-test harness I wrote is reusable, keeping that piece."
• Duplicate work discovered. Q: "Heads up, another team already built this last quarter." A: "Ah, wish we had known. Let me look at theirs, if it fits I will drop mine and adopt it rather than maintain two."
• You deliver the news. Q: "The endpoint you built for us got descoped, I am sorry to be the one saying it." A: "Ok, thanks for telling me straight. Is any of it reusable elsewhere, or should I just close it out?"
• Salvage the learning. Q: "We are dropping the caching layer you spent the sprint on." A: "Understood. Before I close it, the benchmark showed the real bottleneck is the serializer, not the DB. That finding is worth keeping even if the code is not."
• Graceful non-defensiveness. Q: "Turns out we do not need the CSV export anymore." A: "Fine by me. Closing the PR and the ticket. If it resurfaces, the branch is on the repo, just re-request."
• Reframing without bitterness. Q: "The client pulled the requirement, so your work is shelved." A: "These things move, no drama. It is at a clean stopping point and documented, so nothing is wasted if it comes back. On to the next one."

👉 Review is one arena where tone is on the record; the next is the meeting, where it vanishes into the air and you have to steer it live. On to Chapter 48.