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
Logic
Quality
Architecture
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 |
Mission-Critical Review (NASA Standards)
For safety-critical projects, apply NASA/JPL Power of 10 rules during review:
Blocking Violations (Must Fix)
| Rule |
Check For |
Detection |
| R1 |
Recursive function without maxDepth parameter |
grep -rn "function.*\(" | xargs grep -l "walk|traverse|recurse" |
| R2 |
while loop without iteration counter |
Manual review of all while statements |
| R3 |
Unbounded array growth |
push() in loops without size checks |
High Priority (Strong Recommendation)
| Rule |
Check For |
Detection |
| R4 |
Function > 60 lines |
Line count per function |
| R5 |
Missing entry assertions |
Public functions without precondition checks |
| R8 |
Nesting > 4 levels |
Visual inspection of indentation |
Medium Priority (Consider)
| Rule |
Check For |
Detection |
| R6 |
Variable declared far from use |
Manual review |
| R7 |
Unchecked return values |
grep for ignored returns |
| R9 |
Deep property access without ?. |
obj.prop.prop.prop chains |
| R10 |
Compiler warnings |
Build output |
Trigger: User mentions "mission-critical", "NASA standards", "high reliability", or "safety-critical"
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
Pattern applies to: VS Code extensions, Electron apps, Node.js services with UI
1---2name: code-review3description: Systematic code review for correctness, security, and growth — not just style enforcement4---56# Code Review Skill78> Good reviews catch bugs. Great reviews teach the author something.910## Review Priority (What Matters Most)11121. **Correctness** — Does it do what it's supposed to?132. **Security** — Can it be exploited?143. **Maintainability** — Will the next person understand this?154. **Performance** — Will it scale?165. **Style** — Is it consistent? (ideally enforced by linters, not humans)1718## 3-Pass Review1920| Pass | Focus | What You're Looking For | Time |21| ---- | ----- | ----------------------- | ---- |22| 1. Orientation | Big picture | Does the approach make sense? Is the scope right? Over-engineered? | 2-3 min |23| 2. Logic | Deep read | Edge cases, null handling, error paths, concurrency, off-by-one | 10-15 min |24| 3. Polish | Surface | Naming, duplication, test coverage, docs | 3-5 min |2526**Pass 1 shortcut**: Read the PR description and test names first. They reveal intent faster than code.2728## Comment Prefixes2930| Prefix | Meaning | Author Response |31| ------ | ------- | --------------- |32| `[blocking]` | Must fix before merge | Fix it |33| `[suggestion]` | Better approach exists | Consider it, explain if declining |34| `[question]` | I don't understand | Clarify (in code, not just in reply) |35| `[nit]` | Trivial style issue | Fix if easy, skip if not |36| `[praise]` | This is well done | Appreciate it |3738### Good vs Bad Comment Examples3940| Bad | Why | Good |41| --- | --- | ---- |42| "This is confusing" | Vague, unhelpful | "[suggestion] This nested ternary is hard to follow. Consider extracting to a named function like `isEligibleForDiscount()`." |43| "Fix this" | No context | "[blocking] This accepts user input without sanitization. Use `escapeHtml()` before rendering." |44| "Why?" | Sounds hostile | "[question] What's the motivation for the custom sort here vs `Array.sort()`? Is there a performance concern?" |45| "LGTM" (on 500-line PR) | Rubber stamp | "Pass 1: Approach looks right. Pass 2 comments below. Pass 3: naming is clean." |4647## Review Checklist4849### Security50- [ ] No secrets, tokens, or API keys in code51- [ ] User input validated/sanitized before use52- [ ] Auth checks on protected endpoints53- [ ] No SQL/command injection vectors54- [ ] Sensitive data not logged5556### Logic57- [ ] Edge cases handled (empty input, null, boundary values)58- [ ] Error paths return meaningful messages59- [ ] Async operations have timeout/cancellation60- [ ] State changes are atomic (no partial updates)61- [ ] All new branches have test coverage6263### Quality64- [ ] Tests cover the *changed behavior*, not just the changed lines65- [ ] No debug code (console.log, TODO-hacks)66- [ ] Public API changes documented67- [ ] Backward compatibility considered6869### Architecture70- [ ] Change is in the right layer (not business logic in the controller)71- [ ] New dependencies justified72- [ ] No unnecessary coupling introduced7374## Anti-Patterns7576| Anti-Pattern | What Happens | Instead |77| ------------ | ------------ | ------- |78| Rubber-stamp | Bugs ship | Actually read Pass 1-3 |79| Bikeshedding | Hours on naming, ignore logic bugs | Spend 80% on Pass 2 |80| Gatekeeping | Reviewees dread PRs | Teach, don't block |81| Week-long queue | PRs go stale, conflicts pile up | Review within 4 hours, merge within 24 |82| Style wars | Team friction | Automate style (ESLint, Prettier, etc.) |83| Everything-is-blocking | Author overwhelmed | Use prefix system honestly |8485## Mission-Critical Review (NASA Standards)8687For safety-critical projects, apply NASA/JPL Power of 10 rules during review:8889### Blocking Violations (Must Fix)9091| Rule | Check For | Detection |92| ---- | --------- | --------- |93| **R1** | Recursive function without `maxDepth` parameter | `grep -rn "function.*\(" \| xargs grep -l "walk\|traverse\|recurse"` |94| **R2** | `while` loop without iteration counter | Manual review of all `while` statements |95| **R3** | Unbounded array growth | `push()` in loops without size checks |9697### High Priority (Strong Recommendation)9899| Rule | Check For | Detection |100| ---- | --------- | --------- |101| **R4** | Function > 60 lines | Line count per function |102| **R5** | Missing entry assertions | Public functions without precondition checks |103| **R8** | Nesting > 4 levels | Visual inspection of indentation |104105### Medium Priority (Consider)106107| Rule | Check For | Detection |108| ---- | --------- | --------- |109| **R6** | Variable declared far from use | Manual review |110| **R7** | Unchecked return values | `grep` for ignored returns |111| **R9** | Deep property access without `?.` | `obj.prop.prop.prop` chains |112| **R10** | Compiler warnings | Build output |113114**Trigger**: User mentions "mission-critical", "NASA standards", "high reliability", or "safety-critical"115116## Review Timing117118| PR Size | Expected Review Time | If Larger |119| ------- | -------------------- | --------- |120| < 100 lines | < 30 min | — |121| 100-400 lines | 30-60 min | Ideal size |122| 400+ lines | 60+ min | Ask author to split |123| 1000+ lines | Don't | Refuse; request breakdown |124125---126127## Extension Audit Methodology (VS Code Extensions)128129**When**: Before release, after major refactoring, or on quality concerns130131**Scope**: Multi-dimensional code quality analysis beyond standard code review132133### 5-Dimension Audit Framework134135| Dimension | Focus | Tools/Methods | Output |136| --------- | ----- | ------------- | ------ |137| **Debug & Logging** | Console statements, debug code | `grep -r "console\\.log\|console\\.debug"` | Categorize: legitimate vs removable |138| **Dead Code** | Unused imports, orphaned files, broken refs | TypeScript compilation + manual scan | List dead commands, UI, dependencies |139| **Performance** | Blocking I/O, sync operations, bottlenecks | `grep -r "Sync\(" src/`, profiling | Async refactoring candidates |140| **Menu Validation** | All commands/buttons work | Manual testing + error logs | Broken commands, missing handlers |141| **Dependencies** | Unused packages, leftover references | package.json vs import analysis | Removable dependencies |142143### Audit Report Template144145```markdown146## Executive Summary147- Console statements: X remaining (Y legitimate, Z removable)148- Dead code: [commands/UI/dependencies list]149- Performance: [blocking operations count]150- Menu validation: [working/broken ratio]151152## Recommendations1531. [Category]: [Issue] → [Action] (Priority: Critical/High/Medium)1542. [Category]: [Issue] → [Action] (Priority: Critical/High/Medium)155```156157### Console Statement Categorization158159| Category | Keep? | Examples |160| -------- | ----- | -------- |161| **Enterprise compliance** | ✅ | Audit logs, security events, GDPR actions |162| **User feedback** | ✅ | TTS status, long-running ops, critical errors |163| **Debug noise** | ❌ | Setup verbosity, migration logs, info messages |164| **Development artifacts** | ❌ | "Entering function X", temporary debugging |165166### Performance Red Flags167168- **Synchronous file I/O** in UI thread: `fs.readFileSync`, `fs.existsSync`, `fs.readdirSync`169 - **Fix**: Convert to `fs-extra` async: `await fs.readFile`, `await fs.pathExists`, `await fs.readdir`170- **Blocking operations** in activation: Heavy computation before extension ready171 - **Fix**: Defer to background, show loading state, or lazy-load172- **Serial operations** that could be parallel: Sequential awaits for independent tasks173 - **Fix**: `Promise.all([op1(), op2(), op3()])`174175### Dead Code Detection Pattern1761771. **Scan command registrations**: `vscode.commands.registerCommand('command.id', ...)`1782. **Scan UI references**: Search HTML/views for command IDs1793. **Cross-check**: Commands in UI but not registered = broken; registered but unused = dead1804. **Verify disposables**: Removed commands should have disposable cleanup too181182### Post-Audit Verification183184- [ ] TypeScript compiles: `npm run compile` → exit 0185- [ ] No orphaned imports: All imports resolve186- [ ] Version aligned: package.json, CHANGELOG, copilot-instructions match187- [ ] Smoke test: Extension activates, 3 random commands work188189**Pattern applies to**: VS Code extensions, Electron apps, Node.js services with UI
fabioc-aloha