Code Review

Code Review Skill

Good reviews catch bugs. Great reviews teach the author something.

Review Priority (What Matters Most)

1. Correctness — Does it do what it's supposed to?
2. Security — Can it be exploited?
3. Maintainability — Will the next person understand this?
4. Performance — Will it scale?
5. Style — Is it consistent? (ideally enforced by linters, not humans)

3-Pass Review

Pass 1 shortcut: Read the PR description and test names first. They reveal intent faster than code.

Comment Prefixes

Good vs Bad Comment Examples

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

Mission-Critical Review (NASA Standards)

For safety-critical projects, apply NASA/JPL Power of 10 rules during review:

Blocking Violations (Must Fix)

High Priority (Strong Recommendation)

Medium Priority (Consider)

Trigger: User mentions "mission-critical", "NASA standards", "high reliability", or "safety-critical"

Reference: .github/instructions/nasa-code-standards.instructions.md

Review Timing

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

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

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

1. Scan command registrations: vscode.commands.registerCommand('command.id', ...)
2. Scan UI references: Search HTML/views for command IDs
3. Cross-check: Commands in UI but not registered = broken; registered but unused = dead
4. 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