177 lines
4.3 KiB
Markdown
177 lines
4.3 KiB
Markdown
## PR 리뷰
|
|
|
|
Pull Request 의 체계적 리뷰로 코드 품질과 아키텍처 건전성을 확보합니다.
|
|
|
|
### 사용법
|
|
|
|
```bash
|
|
# PR 을 포괄적으로 리뷰
|
|
gh pr view 123 --comments
|
|
"이 PR 을 체계적으로 리뷰해서 코드 품질, 보안, 아키텍처 관점으로 피드백하세요"
|
|
|
|
# 보안 특화 리뷰
|
|
gh pr diff 123
|
|
"보안 리스크와 취약점에 초점을 맞춰서 리뷰하세요"
|
|
|
|
# 아키텍처 관점의 리뷰
|
|
gh pr checkout 123 && find . -name "*.js" | head -10
|
|
"레이어 분리, 의존관계, SOLID 원칙 관점으로 아키텍처를 평가하세요"
|
|
```
|
|
|
|
### 기본 예시
|
|
|
|
```bash
|
|
# 코드 품질의 정량적 평가
|
|
find . -name "*.js" -exec wc -l {} + | sort -rn | head -5
|
|
"코드의 복잡도, 함수 크기, 중복도를 평가해서 개선점을 지적하세요"
|
|
|
|
# 보안 취약점 체크
|
|
grep -r "password\|secret\|token" . --include="*.js" | head -10
|
|
"기밀정보 누출, 하드코딩, 인증 우회 리스크를 체크하세요"
|
|
|
|
# 아키텍처 위반 탐지
|
|
grep -r "import.*from.*\.\./\.\." . --include="*.js"
|
|
"레이어 위반, 순환 의존, 결합도 문제를 평가하세요"
|
|
```
|
|
|
|
### 코멘트 분류 체계
|
|
|
|
```text
|
|
🔴 critical.must: 치명적 문제
|
|
├─ 보안 취약점
|
|
├─ 데이터 정합성 문제
|
|
└─ 시스템 장애 리스크
|
|
|
|
🟡 high.imo: 높은 우선도 개선
|
|
├─ 기능 불량 리스크
|
|
├─ 성능 문제
|
|
└─ 유지보수성 대폭 저하
|
|
|
|
🟢 medium.imo: 중간 우선도 개선
|
|
├─ 가독성 향상
|
|
├─ 코드 구조 개선
|
|
└─ 테스트 품질 향상
|
|
|
|
🟢 low.nits: 경미한 지적
|
|
├─ 스타일 통일
|
|
├─ 오타 수정
|
|
└─ 주석 추가
|
|
|
|
🔵 info.q: 질문·정보 제공
|
|
├─ 구현 의도 확인
|
|
├─ 설계 판단 배경
|
|
└─ 베스트 프랙티스 공유
|
|
```
|
|
|
|
### 리뷰 관점
|
|
|
|
#### 1. 코드 정확성
|
|
|
|
- **로직 에러**: 경계값, Null 체크, 예외 처리
|
|
- **데이터 정합성**: 타입 안전성, 밸리데이션
|
|
- **에러 핸들링**: 망라성, 적절한 처리
|
|
|
|
#### 2. 보안
|
|
|
|
- **인증·인가**: 적절한 체크, 권한 관리
|
|
- **입력 검증**: SQL 인젝션, XSS 대책
|
|
- **기밀정보**: 로그 출력 금지, 암호화
|
|
|
|
#### 3. 성능
|
|
|
|
- **알고리즘**: 시간 계산 복잡도, 메모리 효율성
|
|
- **데이터베이스**: N+1 쿼리, 인덱스 최적화
|
|
- **리소스**: 메모리 누수, 캐시 활용
|
|
|
|
#### 4. 아키텍처
|
|
|
|
- **레이어 분리**: 의존 방향, 적절한 분리
|
|
- **결합도**: 느슨한 결합, 인터페이스 활용
|
|
- **SOLID 원칙**: 단일 책임, 개방 폐쇄, 의존성 역전
|
|
|
|
### 리뷰 플로
|
|
|
|
1. **사전 확인**: PR 정보, 변경 차분, 관련 Issue
|
|
2. **체계적 체크**: 보안 → 정확성 → 성능 → 아키텍처
|
|
3. **건설적 피드백**: 구체적인 개선안과 코드 예시
|
|
4. **팔로우업**: 수정 확인, CI 상태, 최종 승인
|
|
|
|
### 효과적인 코멘트 예시
|
|
|
|
#### 보안 문제 템플릿
|
|
|
|
**포맷**:
|
|
|
|
```text
|
|
**critical.must.** [문제점 요약]
|
|
|
|
[구체적인 위험 설명]
|
|
|
|
권장 해결방안:
|
|
[해결 방법]
|
|
```
|
|
|
|
**예시**:
|
|
|
|
```text
|
|
**critical.must.** 패스워드가 평문으로 저장되고 있습니다
|
|
|
|
보안 리스크를 방지하기 위해 해싱화가 필수입니다.
|
|
|
|
권장 해결방안:
|
|
const bcrypt = require('bcrypt');
|
|
const hashedPassword = await bcrypt.hash(password, 12);
|
|
```
|
|
|
|
#### 성능 개선 템플릿
|
|
|
|
**포맷**:
|
|
|
|
```text
|
|
**high.imo.** [성능 문제점]
|
|
|
|
[문제 상황 설명]
|
|
|
|
개선안:
|
|
[최적화 방법]
|
|
```
|
|
|
|
**예시**:
|
|
|
|
```text
|
|
**high.imo.** N+1 쿼리 문제가 발생합니다
|
|
|
|
쿼리 수를 대폭 줄일 수 있습니다.
|
|
|
|
개선안:
|
|
// Eager Loading 사용
|
|
const users = await User.findAll({ include: [Post] });
|
|
```
|
|
|
|
#### 아키텍처 위반 템플릿
|
|
|
|
**포맷**:
|
|
|
|
```text
|
|
**high.must.** [아키텍처 문제점]
|
|
|
|
[위반 내용 설명]
|
|
[개선 방향 제시]
|
|
```
|
|
|
|
**예시**:
|
|
|
|
```text
|
|
**high.must.** 레이어 위반이 발생하고 있습니다
|
|
|
|
도메인 레이어가 인프라 레이어에 직접 의존하고 있습니다.
|
|
의존성 역전 원칙으로 인터페이스를 도입하세요.
|
|
```
|
|
|
|
### 주의사항
|
|
|
|
- **건설적 톤**: 공격적이 아닌 협조적인 커뮤니케이션
|
|
- **구체적 제안**: 문제 지적뿐만 아니라 해결안 제시
|
|
- **우선순위**: Critical → High → Medium → Low 순으로 대응
|
|
- **지속적 개선**: 리뷰 결과를 지식베이스화
|