Code Review Skill
Good reviews catch bugs. Great reviews teach the author something.
Review Priority (What Matters Most)
- Correctness — Does it do what it's supposed to?
- Security — Can it be exploited?
- Maintainability — Will the next person understand this?
- Performance — Will it scale?
- Style — Is it consistent? (ideally enforced by linters, not humans)
3-Pass Review
| Pass |
Focus |
What You're Looking For |
Time |
| 1. Orientation |
Big picture |
Does the approach make sense? Is the scope right? Over-engineered? |
2-3 min |
| 2. Logic |
Deep read |
Edge cases, null handling, error paths, concurrency, off-by-one |
10-15 min |
| 3. Polish |
Surface |
Naming, duplication, test coverage, docs |
3-5 min |
Pass 1 shortcut: Read the PR description and test names first. They reveal intent faster than code.
Comment Prefixes
| Prefix |
Meaning |
Author Response |
[blocking] |
Must fix before merge |
Fix it |
[suggestion] |
Better approach exists |
Consider it, explain if declining |
[question] |
I don't understand |
Clarify (in code, not just in reply) |
[nit] |
Trivial style issue |
Fix if easy, skip if not |
[praise] |
This is well done |
Appreciate it |
Good vs Bad Comment Examples
| Bad |
Why |
Good |
| "This is confusing" |
Vague, unhelpful |
"[suggestion] This nested ternary is hard to follow. Consider extracting to a named function like isEligibleForDiscount()." |
| "Fix this" |
No context |
"[blocking] This accepts user input without sanitization. Use escapeHtml() before rendering." |
| "Why?" |
Sounds hostile |
"[question] What's the motivation for the custom sort here vs Array.sort()? Is there a performance concern?" |
| "LGTM" (on 500-line PR) |
Rubber stamp |
"Pass 1: Approach looks right. Pass 2 comments below. Pass 3: naming is clean." |
Review Checklist
Security
- No secrets, tokens, or API keys in code
- User input validated/sanitized before use
- Auth checks on protected endpoints
- No SQL/command injection vectors
- Sensitive data not logged
Logic
- Edge cases handled (empty input, null, boundary values)
- Error paths return meaningful messages
- Async operations have timeout/cancellation
- State changes are atomic (no partial updates)
- All new branches have test coverage
Quality
- Tests cover the changed behavior, not just the changed lines
- No debug code (console.log, TODO-hacks)
- Public API changes documented
- Backward compatibility considered
Architecture
- Change is in the right layer (not business logic in the controller)
- New dependencies justified
- No unnecessary coupling introduced
Anti-Patterns
| Anti-Pattern |
What Happens |
Instead |
| Rubber-stamp |
Bugs ship |
Actually read Pass 1-3 |
| Bikeshedding |
Hours on naming, ignore logic bugs |
Spend 80% on Pass 2 |
| Gatekeeping |
Reviewees dread PRs |
Teach, don't block |
| Week-long queue |
PRs go stale, conflicts pile up |
Review within 4 hours, merge within 24 |
| Style wars |
Team friction |
Automate style (ESLint, Prettier, etc.) |
| Everything-is-blocking |
Author overwhelmed |
Use prefix system honestly |
Review Timing
| PR Size |
Expected Review Time |
If Larger |
| < 100 lines |
< 30 min |
— |
| 100-400 lines |
30-60 min |
Ideal size |
| 400+ lines |
60+ min |
Ask author to split |
| 1000+ lines |
Don't |
Refuse; request breakdown |
Extension Audit Methodology (VS Code Extensions)
When: Before release, after major refactoring, or on quality concerns
Scope: Multi-dimensional code quality analysis beyond standard code review
5-Dimension Audit Framework
| Dimension |
Focus |
Tools/Methods |
Output |
| Debug & Logging |
Console statements, debug code |
grep -r "console\\.log|console\\.debug" |
Categorize: legitimate vs removable |
| Dead Code |
Unused imports, orphaned files, broken refs |
TypeScript compilation + manual scan |
List dead commands, UI, dependencies |
| Performance |
Blocking I/O, sync operations, bottlenecks |
grep -r "Sync\(" src/, profiling |
Async refactoring candidates |
| Menu Validation |
All commands/buttons work |
Manual testing + error logs |
Broken commands, missing handlers |
| Dependencies |
Unused packages, leftover references |
package.json vs import analysis |
Removable dependencies |
Audit Report Template
## Executive Summary
- Console statements: X remaining (Y legitimate, Z removable)
- Dead code: [commands/UI/dependencies list]
- Performance: [blocking operations count]
- Menu validation: [working/broken ratio]
## Recommendations
1. [Category]: [Issue] → [Action] (Priority: Critical/High/Medium)
2. [Category]: [Issue] → [Action] (Priority: Critical/High/Medium)
Console Statement Categorization
| Category |
Keep? |
Examples |
| Enterprise compliance |
✅ |
Audit logs, security events, GDPR actions |
| User feedback |
✅ |
TTS status, long-running ops, critical errors |
| Debug noise |
❌ |
Setup verbosity, migration logs, info messages |
| Development artifacts |
❌ |
"Entering function X", temporary debugging |
Performance Red Flags
- Synchronous file I/O in UI thread:
fs.readFileSync, fs.existsSync, fs.readdirSync
- Fix: Convert to
fs-extra async: await fs.readFile, await fs.pathExists, await fs.readdir
- Blocking operations in activation: Heavy computation before extension ready
- Fix: Defer to background, show loading state, or lazy-load
- Serial operations that could be parallel: Sequential awaits for independent tasks
- Fix:
Promise.all([op1(), op2(), op3()])
Dead Code Detection Pattern
- Scan command registrations:
vscode.commands.registerCommand('command.id', ...)
- Scan UI references: Search HTML/views for command IDs
- Cross-check: Commands in UI but not registered = broken; registered but unused = dead
- Verify disposables: Removed commands should have disposable cleanup too
Post-Audit Verification
- TypeScript compiles:
npm run compile → exit 0
- No orphaned imports: All imports resolve
- Version aligned: package.json, CHANGELOG, copilot-instructions match
- Smoke test: Extension activates, 3 random commands work
Pattern applies to: VS Code extensions, Electron apps, Node.js services with UI
Synapses
See synapses.json for connections.
