code-review-patterns

Name: code-review-patterns
Author: sabahattink

27stars9forks

How to install code-review-patterns?

sabahattink/antigravity-fullstack-hq/code-review-patterns

$npx -y skills add sabahattink/antigravity-fullstack-hq --skill code-review-patterns

Installs into the current project.

Summary

Code review checklist, what to look for, how to give feedback, PR review flow. Use when reviewing a pull request, or when preparing code for review.

Files · 1

View on GitHub

SKILL.md

1	# Code Review Patterns
2
3	## The Goal of Code Review
4
5	Code review is NOT about finding mistakes to win arguments. It is about:
6	1. Correctness — Does the code do what it's supposed to do?
7	2. Safety — Are there security or data integrity risks?
8	3. Clarity — Will the next developer understand this in 6 months?
9	4. Consistency — Does this follow the established patterns in the codebase?
10
11	## What to Look For — Priority Order
12
13	### P0 — Block (must fix before merge)
14	```
15	□ Security vulnerability (SQL injection, XSS, auth bypass, hardcoded secrets)
16	□ Data loss risk (wrong delete scope, missing transaction, no backup)
17	□ Correctness bug (wrong logic, off-by-one, race condition)
18	□ Broken tests or test coverage dropped below 80%
19	□ Deployment risk (migration without rollback, breaking API change)
20	```
21
22	### P1 — Should Fix
23	```
24	□ Missing error handling (silent catch, swallowed exception)
25	□ N+1 query problem
26	□ Function over 50 lines (extract responsibility)
27	□ File over 800 lines (extract module)
28	□ Nesting depth > 4 (use early returns)
29	□ Missing input validation on public endpoints
30	□ console.log / debug statements left in
31	□ TODO comments without tracking issue
32	```
33
34	### P2 — Consider Fixing
35	```
36	□ Naming: unclear variable/function names
37	□ Missing comments on non-obvious logic
38	□ Magic numbers (use named constants)
39	□ Duplicate code (extract utility)
40	□ Missing type annotations
41	```
42
43	### P3 — Optional / Style
44	```
45	□ Formatting inconsistency (should be caught by linter/prettier)
46	□ Minor naming preferences
47	□ Ordering of exports
48	```
49
50	## How to Write Review Comments
51
52	### Be specific and constructive
53
54	```
55	# Bad comment
56	"This is wrong."
57
58	# Good comment
59	"This will fail when `user` is null (e.g., when the token is valid but
60	the user account was deleted). Consider throwing `UnauthorizedException`
61	here rather than proceeding: `if (!user) throw new UnauthorizedException()`"
62	```
63
64	### Distinguish opinion from requirement
65
66	Use prefixes to signal severity:
67	```
68	[BLOCK] — Must fix before merge (security, correctness)
69	[SHOULD] — Strong recommendation, technical debt if skipped
70	[NIT] — Minor, only fix if easy — don't hold up the PR
71	[IDEA] — Thought worth considering, not a request
72	[QUESTION]— Genuine question, not criticism
73	```
74
75	### Examples by category
76
77	```typescript
78	// [BLOCK] SQL injection risk — user input is directly interpolated
79	// Change to: db.query('SELECT * FROM users WHERE email = $1', [email])
80	const result = await db.query(`SELECT * FROM users WHERE email = '${email}'`)
81
82	// [SHOULD] Missing await — this returns a Promise<void>, not void.
83	// The function returns before the email is sent.
84	emailService.send(user.email, 'Welcome!')
85
86	// [NIT] Could simplify with optional chaining:
87	// return user?.profile?.avatar ?? null
88	if (user && user.profile && user.profile.avatar) {
89	return user.profile.avatar
90	}
91	return null
92
93	// [IDEA] Consider using a discriminated union here instead of nullable
94	// fields — it makes the state machine explicit at compile time.
95
96	// [QUESTION] Is this intentionally public? Seems like an admin-only action.
97	@Get('all-users')
98	findAllUsers() { ... }
99	```
100
101	## PR Checklist for the Author
102
103	Before requesting review:
104	```
105	□ Self-review the diff — read your own code as if reviewing someone else's
106	□ All tests pass (npm test / pnpm test)
107	□ No lint errors (npm run lint)
108	□ Type check passes (npm run type-check)
109	□ No debug statements or commented-out code
110	□ PR description explains WHY, not just WHAT
111	□ Related issues linked
112	□ Screenshots for UI changes
113	□ Breaking changes documented
114	□ Migration scripts tested locally
115	```
116
117	## PR Description Template
118
119	```markdown
120	## What
121	[One paragraph: what does this change do?]
122
123	## Why
124	[Why is this change needed? Link to issue if applicable.]
125
126	## How
127	[Brief explanation of the approach taken.]
128
129	## Testing
130	- [ ] Unit tests added/updated
131	- [ ] Integration tests pass
132	- [ ] Tested locally against real data
133
134	## Screenshots
135	[For UI changes — before/after]
136
137	## Breaking Changes
138	[List any breaking API changes, DB schema changes, or env var changes]
139	```
140
141	## Reviewing Large PRs
142
143	For PRs with 500+ lines changed:
144
145	```
146	1. Read the PR description first — understand the intent
147	2. Start with tests — they document expected behavior
148	3. Read the new interfaces/types — they reveal the design
149	4. Read the service/business logic — the core of the change
150	5. Read the controllers/handlers — boundary validation
151	6. Check the migrations last — easy to miss side effects
152	```
153
154	## Common Code Smells to Flag
155
156	### The God Object
157	```typescript
158	// Bad: UsersService does everything
159	class UsersService {
160	createUser() {}
161	sendEmail() {}
162	generateReport() {}
163	processPayment() {}
164	updateInventory() {}
165	}
166
167	// Flag it: "This service has too many responsibilities.
168	// Consider extracting EmailService, ReportService, PaymentService."
169	```

People who liked this also viewed

6 picks

Type

code-review-patterns

27stars9forks

View on GitHub

How to install code-review-patterns?

sabahattink/antigravity-fullstack-hq/code-review-patterns

$npx -y skills add sabahattink/antigravity-fullstack-hq --skill code-review-patterns

Installs into the current project.

Summary

Code review checklist, what to look for, how to give feedback, PR review flow. Use when reviewing a pull request, or when preparing code for review.

Files · 1

View on GitHub

SKILL.md

1	# Code Review Patterns
2
3	## The Goal of Code Review
4
5	Code review is NOT about finding mistakes to win arguments. It is about:
6	1. Correctness — Does the code do what it's supposed to do?
7	2. Safety — Are there security or data integrity risks?
8	3. Clarity — Will the next developer understand this in 6 months?
9	4. Consistency — Does this follow the established patterns in the codebase?
10
11	## What to Look For — Priority Order
12
13	### P0 — Block (must fix before merge)
14	```
15	□ Security vulnerability (SQL injection, XSS, auth bypass, hardcoded secrets)
16	□ Data loss risk (wrong delete scope, missing transaction, no backup)
17	□ Correctness bug (wrong logic, off-by-one, race condition)
18	□ Broken tests or test coverage dropped below 80%
19	□ Deployment risk (migration without rollback, breaking API change)
20	```
21
22	### P1 — Should Fix
23	```
24	□ Missing error handling (silent catch, swallowed exception)
25	□ N+1 query problem
26	□ Function over 50 lines (extract responsibility)
27	□ File over 800 lines (extract module)
28	□ Nesting depth > 4 (use early returns)
29	□ Missing input validation on public endpoints
30	□ console.log / debug statements left in
31	□ TODO comments without tracking issue
32	```
33
34	### P2 — Consider Fixing
35	```
36	□ Naming: unclear variable/function names
37	□ Missing comments on non-obvious logic
38	□ Magic numbers (use named constants)
39	□ Duplicate code (extract utility)
40	□ Missing type annotations
41	```
42
43	### P3 — Optional / Style
44	```
45	□ Formatting inconsistency (should be caught by linter/prettier)
46	□ Minor naming preferences
47	□ Ordering of exports
48	```
49
50	## How to Write Review Comments
51
52	### Be specific and constructive
53
54	```
55	# Bad comment
56	"This is wrong."
57
58	# Good comment
59	"This will fail when `user` is null (e.g., when the token is valid but
60	the user account was deleted). Consider throwing `UnauthorizedException`
61	here rather than proceeding: `if (!user) throw new UnauthorizedException()`"
62	```
63
64	### Distinguish opinion from requirement
65
66	Use prefixes to signal severity:
67	```
68	[BLOCK] — Must fix before merge (security, correctness)
69	[SHOULD] — Strong recommendation, technical debt if skipped
70	[NIT] — Minor, only fix if easy — don't hold up the PR
71	[IDEA] — Thought worth considering, not a request
72	[QUESTION]— Genuine question, not criticism
73	```
74
75	### Examples by category
76
77	```typescript
78	// [BLOCK] SQL injection risk — user input is directly interpolated
79	// Change to: db.query('SELECT * FROM users WHERE email = $1', [email])
80	const result = await db.query(`SELECT * FROM users WHERE email = '${email}'`)
81
82	// [SHOULD] Missing await — this returns a Promise<void>, not void.
83	// The function returns before the email is sent.
84	emailService.send(user.email, 'Welcome!')
85
86	// [NIT] Could simplify with optional chaining:
87	// return user?.profile?.avatar ?? null
88	if (user && user.profile && user.profile.avatar) {
89	return user.profile.avatar
90	}
91	return null
92
93	// [IDEA] Consider using a discriminated union here instead of nullable
94	// fields — it makes the state machine explicit at compile time.
95
96	// [QUESTION] Is this intentionally public? Seems like an admin-only action.
97	@Get('all-users')
98	findAllUsers() { ... }
99	```
100
101	## PR Checklist for the Author
102
103	Before requesting review:
104	```
105	□ Self-review the diff — read your own code as if reviewing someone else's
106	□ All tests pass (npm test / pnpm test)
107	□ No lint errors (npm run lint)
108	□ Type check passes (npm run type-check)
109	□ No debug statements or commented-out code
110	□ PR description explains WHY, not just WHAT
111	□ Related issues linked
112	□ Screenshots for UI changes
113	□ Breaking changes documented
114	□ Migration scripts tested locally
115	```
116
117	## PR Description Template
118
119	```markdown
120	## What
121	[One paragraph: what does this change do?]
122
123	## Why
124	[Why is this change needed? Link to issue if applicable.]
125
126	## How
127	[Brief explanation of the approach taken.]
128
129	## Testing
130	- [ ] Unit tests added/updated
131	- [ ] Integration tests pass
132	- [ ] Tested locally against real data
133
134	## Screenshots
135	[For UI changes — before/after]
136
137	## Breaking Changes
138	[List any breaking API changes, DB schema changes, or env var changes]
139	```
140
141	## Reviewing Large PRs
142
143	For PRs with 500+ lines changed:
144
145	```
146	1. Read the PR description first — understand the intent
147	2. Start with tests — they document expected behavior
148	3. Read the new interfaces/types — they reveal the design
149	4. Read the service/business logic — the core of the change
150	5. Read the controllers/handlers — boundary validation
151	6. Check the migrations last — easy to miss side effects
152	```
153
154	## Common Code Smells to Flag
155
156	### The God Object
157	```typescript
158	// Bad: UsersService does everything
159	class UsersService {
160	createUser() {}
161	sendEmail() {}
162	generateReport() {}
163	processPayment() {}
164	updateInventory() {}
165	}
166
167	// Flag it: "This service has too many responsibilities.
168	// Consider extracting EmailService, ReportService, PaymentService."
169	```

People who liked this also viewed

6 picks

Type