[G1-Sync] Manual knowledge update
This commit is contained in:
Antigravity Agent
@@ -0,0 +1,319 @@
|
||||
---
|
||||
id: productivity-code-review
|
||||
title: Code Review — Checklist / 좋은 review / 빠른 turnaround
|
||||
category: Coding
|
||||
status: draft
|
||||
source_trust_level: B
|
||||
verification_status: conceptual
|
||||
created_at: 2026-05-09
|
||||
updated_at: 2026-05-09
|
||||
tags: [productivity, code-review, vibe-coding]
|
||||
tech_stack: { language: "Process", applicable_to: ["Engineering"] }
|
||||
applied_in: []
|
||||
aliases: [code review, PR review, must-fix, suggestion, nit, conventional comments]
|
||||
---
|
||||
|
||||
# Code Review
|
||||
|
||||
> 좋은 review = 학습 + 품질. **Severity 라벨 (must-fix / suggestion / nit) + 구체적 + 빠른 turnaround**. Conventional comments + 자동 lint + 최소 인간 review.
|
||||
|
||||
## 📖 핵심 개념
|
||||
- Severity: 변경 강제 vs 권장.
|
||||
- Specific: 줄 인용 + 예시 코드.
|
||||
- Fast: 1-2일 안 review.
|
||||
- Automation: lint / test / type 자동.
|
||||
|
||||
## 💻 코드 패턴
|
||||
|
||||
### Severity labels
|
||||
```
|
||||
must-fix: Block merge. Bug / 보안 / 명확 잘못.
|
||||
should-fix: 강 권장. Reviewer 가 확신.
|
||||
suggestion: Better way 제안.
|
||||
nit: 취향. Author 가 무시 OK.
|
||||
question: 이해 / 의도 명확화.
|
||||
praise: 좋은 코드 — 강화.
|
||||
```
|
||||
|
||||
### Conventional Comments
|
||||
```
|
||||
must-fix: This will throw NPE when user is null.
|
||||
suggestion: Consider extracting this to a helper.
|
||||
question: Is this intentional? Looks like a typo.
|
||||
nit: Maybe rename `x` to `index` for clarity.
|
||||
praise: Nice use of `early return` here.
|
||||
```
|
||||
|
||||
→ https://conventionalcomments.org
|
||||
|
||||
### Inline 예
|
||||
```diff
|
||||
+ const total = items.reduce((sum, i) => sum + i.price, 0);
|
||||
```
|
||||
|
||||
> **must-fix**: `i.price` 가 string 으로 올 수 있음 (API 가 decimal string 반환).
|
||||
> ```ts
|
||||
> const total = items.reduce((sum, i) => sum + Number(i.price), 0);
|
||||
> ```
|
||||
|
||||
### Checklist (general)
|
||||
```
|
||||
□ Tests pass (CI)
|
||||
□ Type / lint clean
|
||||
□ Test coverage 추가됨 (새 logic)
|
||||
□ Security 검토 (사용자 input, auth, secrets)
|
||||
□ Performance (N+1, big O)
|
||||
□ Error handling (네트워크 / DB / null)
|
||||
□ Logging (의미 있는 + PII 제외)
|
||||
□ Migration safe (idempotent, rollback)
|
||||
□ Feature flag 적절
|
||||
□ Docs / changelog 갱신
|
||||
```
|
||||
|
||||
### Domain-specific
|
||||
```
|
||||
Frontend:
|
||||
□ A11y (label, keyboard)
|
||||
□ Loading / error state
|
||||
□ Bundle size 영향
|
||||
□ React rendering (memo)
|
||||
□ i18n
|
||||
□ Mobile responsive
|
||||
|
||||
Backend:
|
||||
□ Idempotency
|
||||
□ Rate limit / auth
|
||||
□ DB index
|
||||
□ Transaction boundary
|
||||
□ Backwards-compatible API
|
||||
|
||||
DB migration:
|
||||
□ Concurrent / online
|
||||
□ Rollback plan
|
||||
□ Big table 영향
|
||||
□ Data backfill
|
||||
□ Lock 시간
|
||||
```
|
||||
|
||||
### Self-review (PR 올리기 전)
|
||||
```
|
||||
1. PR 의 diff 본인 review
|
||||
2. Description = "what + why"
|
||||
3. Screenshot / video (UI 변경)
|
||||
4. Test plan
|
||||
5. Linked issue / spec
|
||||
6. Migration / rollback note
|
||||
```
|
||||
|
||||
→ 본인 review 가 reviewer 시간 절약.
|
||||
|
||||
### Good PR description
|
||||
```markdown
|
||||
## What
|
||||
- Add `/api/users/:id/orders` endpoint
|
||||
- Return paginated list (cursor-based)
|
||||
|
||||
## Why
|
||||
- Customer dashboard needs order history (issue #123)
|
||||
|
||||
## How
|
||||
- Added `OrderRepository.findByUser(userId, cursor)`
|
||||
- Reused existing `Pagination` helper
|
||||
- Tests in `orders.test.ts`
|
||||
|
||||
## Screenshots
|
||||
[before/after]
|
||||
|
||||
## Migration
|
||||
None.
|
||||
|
||||
## Test plan
|
||||
- [ ] Unit tests pass
|
||||
- [ ] Manual: visit `/api/users/u1/orders?limit=20`
|
||||
- [ ] Manual: pagination cycle
|
||||
```
|
||||
|
||||
### Bad PR
|
||||
```
|
||||
- "fix"
|
||||
- 1000+ lines
|
||||
- Multiple unrelated changes
|
||||
- No description
|
||||
- No tests
|
||||
```
|
||||
|
||||
→ Reject + ask to split.
|
||||
|
||||
### PR size guideline
|
||||
```
|
||||
< 50 lines: < 30 min review
|
||||
50-200: 1 hour
|
||||
200-500: half day — split 권장
|
||||
500+: 분할 strongly
|
||||
```
|
||||
|
||||
→ 작은 PR = 빠른 review + bug 적음.
|
||||
|
||||
### Review 속도
|
||||
```
|
||||
Day 1: 첫 review (24h goal)
|
||||
Day 2-3: Iteration
|
||||
Day 3-5: Merge
|
||||
|
||||
→ Stale PR = context loss.
|
||||
```
|
||||
|
||||
### CODEOWNERS
|
||||
```
|
||||
# .github/CODEOWNERS
|
||||
/api/payments/ @payments-team
|
||||
/lib/security/ @security-team
|
||||
*.tf @platform-team
|
||||
```
|
||||
|
||||
→ 자동 review 요청.
|
||||
|
||||
### Review 도구
|
||||
```
|
||||
GitHub PR / GitLab MR / Bitbucket
|
||||
Reviewable.io
|
||||
Graphite — stack of small PR
|
||||
```
|
||||
|
||||
### Stack PR (Graphite, etc)
|
||||
```
|
||||
큰 feature = 여러 작은 PR (의존):
|
||||
1. PR #1: Add interface (small)
|
||||
2. PR #2: Add impl (depends on #1)
|
||||
3. PR #3: Wire up (depends on #2)
|
||||
|
||||
→ 각 PR 독립 review. 빠름.
|
||||
```
|
||||
|
||||
### Auto review (LLM)
|
||||
```
|
||||
GitHub Copilot / Cursor / Greptile / CodeRabbit / Coderabbit:
|
||||
- 자동 comment
|
||||
- Style / common mistake
|
||||
- 학습 데이터로 quality
|
||||
|
||||
→ 인간 보다 먼저 보고 작은 issue 처리.
|
||||
⚠️ 본질 / 디자인 review = 인간.
|
||||
```
|
||||
|
||||
### Pair programming vs PR
|
||||
```
|
||||
Pair: real-time, 학습.
|
||||
PR: 비동기, 기록.
|
||||
|
||||
→ Hybrid: 큰 / 위험 = pair. 일반 = PR.
|
||||
```
|
||||
|
||||
### Praise (감사)
|
||||
```
|
||||
"Nice refactor — much cleaner."
|
||||
"Good catch on the edge case."
|
||||
"Love the test coverage."
|
||||
|
||||
→ 강화 + 팀 좋은 분위기.
|
||||
```
|
||||
|
||||
### Disagreement
|
||||
```
|
||||
1. 이해 시도 — "Help me understand why ..."
|
||||
2. Trade-off 명시 — "I'd prefer X because Y, but I see your point about Z."
|
||||
3. Author 결정 우선 (own the code).
|
||||
4. Critical 만 escalate.
|
||||
```
|
||||
|
||||
→ Personal 안 됨.
|
||||
|
||||
### Review during oncall
|
||||
```
|
||||
On-call 중 = limited review time.
|
||||
긴급 만 — 큰 review skip OK.
|
||||
비-on-call 가 review.
|
||||
```
|
||||
|
||||
### Review SLO
|
||||
```
|
||||
Goal: 80% PR 가 24h 안 첫 review.
|
||||
큰 PR (500+): 다음 day OK.
|
||||
|
||||
→ Stale PR alarm (3+ days no review).
|
||||
```
|
||||
|
||||
### LGTM 의미
|
||||
```
|
||||
"Looks Good To Merge" — 너의 책임으로 merge OK.
|
||||
모든 거 검증 X. Approve = trust.
|
||||
```
|
||||
|
||||
### 모든 comment thread close 후 merge
|
||||
```
|
||||
Approve + 모든 comment resolved → merge.
|
||||
"resolved" 안 하면 author 가 잊음.
|
||||
```
|
||||
|
||||
### Comment 답
|
||||
```
|
||||
- "Done" — 변경했음.
|
||||
- "Will do in follow-up #issue" — 별 PR 로.
|
||||
- "Disagree because ..." — 이유.
|
||||
- "Won't fix" — 의도적.
|
||||
```
|
||||
|
||||
### Post-merge review (감사)
|
||||
```
|
||||
이미 merge — but 학습.
|
||||
- "이 부분 다음에 더 작은 PR 가 좋을 듯"
|
||||
- "Naming 가 더 좋을 수 있음"
|
||||
|
||||
→ Author 가 이미 ship — 가르침 / 학습 만.
|
||||
```
|
||||
|
||||
### Review 가 비싸 = 적게 review
|
||||
```
|
||||
Junior PR = 자주 review (학습).
|
||||
Senior 의 작은 PR = 빠른 LGTM OK.
|
||||
큰 / 디자인 = 깊이 review.
|
||||
```
|
||||
|
||||
### Test PR before merge
|
||||
```
|
||||
- 직접 checkout + run
|
||||
- 또는 preview environment (Vercel)
|
||||
- Edge case manual test
|
||||
|
||||
→ "merge → bug" 보다 5분 더 worth.
|
||||
```
|
||||
|
||||
## 🤔 의사결정 기준
|
||||
| 작업 | review depth |
|
||||
|---|---|
|
||||
| Hotfix urgent | Quick review (1h) |
|
||||
| Junior 의 새 feature | Deep review + teach |
|
||||
| Senior 의 작은 refactor | Quick LGTM |
|
||||
| 큰 architecture | 면대면 + design doc |
|
||||
| Production migration | 추가 reviewer + runbook |
|
||||
|
||||
## ❌ 안티패턴
|
||||
- **Severity 무 — 모든 게 must-fix**: nit 도 block.
|
||||
- **Description 빈약**: reviewer 추측.
|
||||
- **큰 PR (1000+)**: 깊이 review 못 함.
|
||||
- **Stale PR (1 week+)**: context loss.
|
||||
- **Personal attack**: 코드 ≠ author.
|
||||
- **Bikeshed (사소)**: must-fix vs nit 구분.
|
||||
- **Ship 전 self-review 없음**: reviewer 시간 낭비.
|
||||
- **Test 없는 PR + LGTM**: bug merge.
|
||||
|
||||
## 🤖 LLM 활용 힌트
|
||||
- Conventional comments + severity.
|
||||
- Self-review 가 first.
|
||||
- Auto LLM review = 기본 catch.
|
||||
- 인간 = 디자인 / 의도.
|
||||
|
||||
## 🔗 관련 문서
|
||||
- [[Productivity_PR_Template]]
|
||||
- [[Productivity_Postmortem]]
|
||||
- [[DevSec_Pre_Commit_Security]]
|
||||
Reference in New Issue
Block a user