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 |
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
Synapses
See synapses.json for connections.
1---2name: code-review-833description: Systematic code review for correctness, security, and growth — not just style enforcement4---5
6# Code Review Skill
7
8> Good reviews catch bugs. Great reviews teach the author something.
9
10## Review Priority (What Matters Most)
11
121. **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)
17
18## 3-Pass Review
19
20| 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 |
25
26**Pass 1 shortcut**: Read the PR description and test names first. They reveal intent faster than code.
27
28## Comment Prefixes
29
30| 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 |
37
38### Good vs Bad Comment Examples
39
40| 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." |
46
47## Review Checklist
48
49### Security
50- [ ] No secrets, tokens, or API keys in code
51- [ ] User input validated/sanitized before use
52- [ ] Auth checks on protected endpoints
53- [ ] No SQL/command injection vectors
54- [ ] Sensitive data not logged
55
56### Logic
57- [ ] Edge cases handled (empty input, null, boundary values)
58- [ ] Error paths return meaningful messages
59- [ ] Async operations have timeout/cancellation
60- [ ] State changes are atomic (no partial updates)
61- [ ] All new branches have test coverage
62
63### Quality
64- [ ] Tests cover the *changed behavior*, not just the changed lines
65- [ ] No debug code (console.log, TODO-hacks)
66- [ ] Public API changes documented
67- [ ] Backward compatibility considered
68
69### Architecture
70- [ ] Change is in the right layer (not business logic in the controller)
71- [ ] New dependencies justified
72- [ ] No unnecessary coupling introduced
73
74## Anti-Patterns
75
76| 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 |
84
85## Review Timing
86
87| PR Size | Expected Review Time | If Larger |
88| ------- | -------------------- | --------- |
89| < 100 lines | < 30 min | — |
90| 100-400 lines | 30-60 min | Ideal size |
91| 400+ lines | 60+ min | Ask author to split |
92| 1000+ lines | Don't | Refuse; request breakdown |
93
94---
95
96## Extension Audit Methodology (VS Code Extensions)
97
98**When**: Before release, after major refactoring, or on quality concerns
99
100**Scope**: Multi-dimensional code quality analysis beyond standard code review
101
102### 5-Dimension Audit Framework
103
104| Dimension | Focus | Tools/Methods | Output |
105| --------- | ----- | ------------- | ------ |
106| **Debug & Logging** | Console statements, debug code | `grep -r "console\\.log\|console\\.debug"` | Categorize: legitimate vs removable |
107| **Dead Code** | Unused imports, orphaned files, broken refs | TypeScript compilation + manual scan | List dead commands, UI, dependencies |
108| **Performance** | Blocking I/O, sync operations, bottlenecks | `grep -r "Sync\(" src/`, profiling | Async refactoring candidates |
109| **Menu Validation** | All commands/buttons work | Manual testing + error logs | Broken commands, missing handlers |
110| **Dependencies** | Unused packages, leftover references | package.json vs import analysis | Removable dependencies |
111
112### Audit Report Template
113
114```markdown
115## Executive Summary
116- Console statements: X remaining (Y legitimate, Z removable)
117- Dead code: [commands/UI/dependencies list]
118- Performance: [blocking operations count]
119- Menu validation: [working/broken ratio]
120
121## Recommendations
1221. [Category]: [Issue] → [Action] (Priority: Critical/High/Medium)
1232. [Category]: [Issue] → [Action] (Priority: Critical/High/Medium)
124```
125
126### Console Statement Categorization
127
128| Category | Keep? | Examples |
129| -------- | ----- | -------- |
130| **Enterprise compliance** | ✅ | Audit logs, security events, GDPR actions |
131| **User feedback** | ✅ | TTS status, long-running ops, critical errors |
132| **Debug noise** | ❌ | Setup verbosity, migration logs, info messages |
133| **Development artifacts** | ❌ | "Entering function X", temporary debugging |
134
135### Performance Red Flags
136
137- **Synchronous file I/O** in UI thread: `fs.readFileSync`, `fs.existsSync`, `fs.readdirSync`
138 - **Fix**: Convert to `fs-extra` async: `await fs.readFile`, `await fs.pathExists`, `await fs.readdir`
139- **Blocking operations** in activation: Heavy computation before extension ready
140 - **Fix**: Defer to background, show loading state, or lazy-load
141- **Serial operations** that could be parallel: Sequential awaits for independent tasks
142 - **Fix**: `Promise.all([op1(), op2(), op3()])`
143
144### Dead Code Detection Pattern
145
1461. **Scan command registrations**: `vscode.commands.registerCommand('command.id', ...)`
1472. **Scan UI references**: Search HTML/views for command IDs
1483. **Cross-check**: Commands in UI but not registered = broken; registered but unused = dead
1494. **Verify disposables**: Removed commands should have disposable cleanup too
150
151### Post-Audit Verification
152
153- [ ] TypeScript compiles: `npm run compile` → exit 0
154- [ ] No orphaned imports: All imports resolve
155- [ ] Version aligned: package.json, CHANGELOG, copilot-instructions match
156- [ ] Smoke test: Extension activates, 3 random commands work
157
158**Pattern applies to**: VS Code extensions, Electron apps, Node.js services with UI
159
160## Synapses
161
162See [synapses.json](synapses.json) for connections.
majiayu000