Scan the file list — does the change scope make sense?
Check the overall approach — is this the right solution to the problem?
Verify the change does not introduce architectural drift
Second Pass (bulk of review time)
Read each file diff top to bottom
Check every function change against the checklists above
Verify error handling at every I/O boundary
Flag anything that makes you pause — trust your instincts
Third Pass (5 minutes)
Think about what could go wrong in production
Check for missing tests on the code paths you flagged
Verify rollback safety — can this change be reverted without data loss?
Confirm documentation and changelog are updated if needed
Severity Levels
Classify every comment by severity so the author knows what blocks merge.
Level
Label
Meaning
Blocks Merge?
Critical
[CRITICAL]
Security vulnerability, data loss, or crash in production
Yes
Major
[MAJOR]
Bug, logic error, or significant performance regression
Yes
Minor
[MINOR]
Improvement that would reduce future maintenance cost
No
Nitpick
[NIT]
Style preference, naming suggestion, or trivial cleanup
No
Always prefix your review comment with the severity label. This removes ambiguity about what matters.
Giving Feedback
Principles
Be specific — Point to the exact line and explain the issue, not just "this is wrong"
Explain why — State the risk or consequence, not just the rule
Suggest a fix — Offer a concrete alternative or code snippet when possible
Ask, don't demand — Use questions for subjective points: "What do you think about...?"
Acknowledge good work — Call out clean solutions, clever optimizations, or thorough tests
Separate blocking from non-blocking — Use severity labels so the author knows what matters
Example Comments
Bad:
This is wrong. Fix it.
Good:
[MAJOR] This query interpolates user input directly into the SQL string (line 42), which is vulnerable to SQL injection. Consider using a parameterized query:
SELECT*FROM users WHERE id = $1
Bad:
Why didn't you add tests?
Good:
[MINOR] The new calculateDiscount() function has a few branching paths — could we add tests for the zero-quantity and negative-price edge cases to prevent regressions?
Bad:
I would have done this differently.
Good:
[NIT] This works well. An alternative approach could be extracting the retry logic into a shared withRetry() wrapper — but that's optional and could be a follow-up.
Review Anti-Patterns
Avoid these common traps that waste time and damage team trust:
Anti-Pattern
Description
Rubber-Stamping
Approving without reading. Creates false confidence and lets bugs through.
Bikeshedding
Spending 30 minutes debating a variable name while ignoring a race condition.
Blocking on Style
Refusing to approve over formatting that a linter should enforce automatically.
Gatekeeping
Requiring your personal preferred approach when the submitted one is correct.
Drive-by Reviews
Leaving one vague comment and disappearing. Commit to following through.
Scope Creep Reviews
Requesting unrelated refactors that should be separate PRs.
Stale Reviews
Letting PRs sit for days. Review within 24 hours or hand off to someone else.
Emotional Language
"This is terrible" or "obviously wrong." Critique the code, not the person.
NEVER Do
NEVER approve without reading every changed line — rubber-stamping is worse than no review
NEVER block a PR solely for style preferences — use a linter; humans review logic
NEVER leave feedback without a severity level — ambiguity causes wasted cycles
NEVER request changes without explaining why — "fix this" teaches nothing
NEVER review more than 400 lines in one sitting — comprehension drops sharply; break large PRs into sessions
NEVER skip the security checklist — one missed vulnerability outweighs a hundred style nits
NEVER make it personal — review the code, never the coder; assume good intent
