Sagargupta16 Code Review
What to Look For
Review in this priority order:
1. Correctness (highest priority)
- Does the code do what the PR description claims?
- Are there off-by-one errors, null/undefined cases, race conditions?
- Does it handle errors properly?
- Are edge cases covered (empty arrays, zero, negative, very large)?
- Does it break existing behavior?
2. Security
- Is user input validated at trust boundaries?
- Are queries parameterized (no SQL injection)?
- Is output encoded (no XSS)?
- Are secrets absent from code and logs?
- Are authentication/authorization checks in place?
- Are dependencies up to date and trusted?
3. Performance
- Are there N+1 queries?
- Is pagination used for list endpoints?
- Are loops doing synchronous I/O?
- Any unnecessary re-computation or re-renders?
- Are caches invalidated correctly?
4. Maintainability
- Does the code match existing patterns in the codebase?
- Are names clear and specific?
- Is complexity proportional to the problem?
- Are comments explaining WHY (not WHAT)?
- Is there duplicated logic that should be extracted?
5. Testing
- Are there tests for the new behavior?
- Do tests actually test behavior, not implementation details?
- Are both happy path AND error cases covered?
- Do existing tests still pass?
6. Style (lowest priority, automated)
- Let linters/formatters handle this
- Don't nitpick on whitespace or import order in review comments
How to Review Systematically
1. Read the PR description -- what is this trying to do?
2. Check the test diff first -- does the tested behavior match the description?
3. Read the implementation diff
4. Run the code locally if the change is significant
5. Check CI status -- what's passing/failing?
6. Consider: how would this break in production?
7. Write comments grouped by severity
Giving Good Feedback
Categories of Comments
| Tag | Meaning | Example |
|---|---|---|
blocking: |
Must fix before merge | blocking: SQL injection in line 42 |
needs-discussion: |
Design concern worth debating | needs-discussion: should we use JWT or sessions? |
suggestion: |
Optional improvement | suggestion: this could use Array.flatMap |
nit: |
Trivial / stylistic | nit: typo in comment |
question: |
Asking to understand | question: why did we choose this approach? |
praise: |
Call out good work | praise: nice error handling here |
What Makes a Good Comment
blocking: SQL injection risk in `getUserByEmail`
The query uses string concatenation:
```
db.query(`SELECT * FROM users WHERE email = '${email}'`)
```
Use parameterized queries instead:
```
db.query('SELECT * FROM users WHERE email = $1', [email])
```
This prevents attackers from injecting SQL via crafted email values.
Good comments:
- State the problem
- Explain the impact
- Suggest a specific fix
- Link to docs or examples if helpful
Bad comments:
- "This is wrong" (no explanation)
- "I don't like this" (not actionable)
- Demanding changes without explaining why
- Restating what the code does
Self-Review Before Submitting
Before marking your own PR ready:
| Check | Why |
|---|---|
| Re-read your own diff | Catches obvious issues |
| Run the feature end-to-end | Verifies it actually works |
| Remove debug logs, commented code, TODOs | Clean up |
| Update or add tests | Behavior without tests is untested |
| Update docs if behavior changed | Future-you will forget |
| Check for secrets in diff | Last chance to catch them |
| Write a clear PR description | Reviewers need context |
PR Description Template
## Summary
One sentence: what does this PR do?
## Why
The motivation / context / ticket link.
## Changes
- Bullet list of key changes
- Include new dependencies
- Flag breaking changes
## Testing
How was this tested? Include screenshots/logs for UI/output changes.
## Notes for Reviewer
Anything non-obvious? Trade-offs made? Open questions?
Reviewing Others' PRs
Before leaving comments:
- Read the comments already left -- don't duplicate
- Check CI status -- is the author already fixing things?
- Check how far behind the base branch -- outdated PRs may need rebase first
- Assume good intent -- ask questions before assuming mistakes
Anti-Patterns
| Anti-Pattern | Problem | Do Instead |
|---|---|---|
| Rubber-stamping without reading | Bugs ship to production | Actually review the code |
| Reviewing only lines shown in diff | Miss broken callers | Check related files too |
| Nit-picking style | Wastes time, demoralizes | Let linters handle style |
| Blocking on opinion disagreements | Gridlock | Distinguish blocking from suggestion |
| Vague comments ("this is bad") | Author can't act | State problem + impact + fix |
| Approving while CI is red | Merges broken code | Wait for CI green |
| Reviewing while tired | Miss obvious bugs | Batch reviews, review fresh |