code-review

Code Review

Code review after implementation. Review section by section across four dimensions, give options and suggestions for each issue, and proceed to the next section only after confirmation.

Purpose: Review code that has already been written (architecture, quality, testing, performance), not a plan review. For plan-level review, use /plan-review。

---

Step 0: Determine review scope + choose depth

Determine review scope

Determine based on ARGUMENTS:

1. File path → read the specified file
2. PR number(e.g., #123）→ gh pr diff 123 get changes
3. diff → git diff + git diff --staged get all uncommitted changes
4. Blank → default behavior same as diff
5. Branch name → git diff main...<branch> get branch diff

After reading the changes, summarize the scope of changes (which files were changed, roughly what was done), and confirm that the review target is correct.

Read context

The full contentof changed files (not just the diff hunks) → read all of it. Also read key imported or depended-on files; only after understanding the context can you give meaningful suggestions.

Choose review depth

Use AskUserQuestion to ask:

• Deep review: up to 4 issues per section, suitable for important features or PRs
• Quick review: up to 1 issue per section, suitable for small changes or hotfixes

---

Step 1-4: Four-section review

Execute in order. After each section is completed, use AskUserQuestion to confirm decisions, then proceed to the next section.

Section 1: Architecture

Issues at the system design level.

• Are component boundaries reasonable? Is there cross-layer access or mixed responsibilities?
• Are dependency directions correct? Are there circular dependencies?
• Is the data flow pattern clear? Is there implicit state passing?
• Security considerations: authentication, authorization, input validation, API boundaries

Section 2: Code quality

Issues with the code itself.

• DRY violations—actively flag duplicated logic (even if duplicated only twice)
• Error handling: Are there unhandled error paths? Do catch blocks swallow errors?
• Edge cases: null values, empty arrays, concurrency, timeouts, large amounts of data
• Naming and readability: Do variable names express intent? Is the logic too obscure?
• Overengineering: unnecessary abstraction, unused flexibility, premature optimization

Section 3: Testing

Test coverage and quality.

• Is there newly added or modified logic missing corresponding tests?
• Do tests verify behavior rather than implementation details?
• Edge case coverage: invalid input, null values, extreme values, concurrency
• Failure paths: network errors, insufficient permissions, missing resources, timeouts
• Are tests maintainable? Is there excessive mocking or brittle assertions?

Section 4: Performance

Performance and resource usage.

• N+1 queries: database/API calls inside loops
• Unnecessary computation or repeated work
• Memory: large objects, unreleased resources, unbounded collections
• Caching opportunities: repeated expensive operations
• Algorithmic complexity: is there O(n²) that can be reduced to O(n)?

---

Issue presentation format

Each issue must include:

1. Problem description — specifically point out file:line or file:SymbolName
2. Options — 2-3 options (including "do nothing"), each labeled with:
   • Implementation cost (low/medium/high)
   • Risk (likelihood of introducing bugs)
   • Scope of impact (how many files changed? affects other features?)
   • Maintenance burden (long-term impact)
3. Recommendation — the recommended option and the reason

Output example

### 1. 品質：驗證邏輯重複

**問題**：`src/handlers/create.ts:23` 和 `src/handlers/update.ts:31`
有幾乎相同的 email 驗證邏輯（正則 + 長度檢查 + domain 白名單）。

**選項**：
- **A) 抽出 validateEmail() 共用函式**（建議）
  成本：低 | 風險：低 | 影響：2 個檔案 | 維護：減少重複
- **B) 不處理**
  成本：零 | 風險：中（改一處忘改另一處）| 影響：無 | 維護：兩份要同步
- **C) 用 zod schema 統一驗證**
  成本：中 | 風險：低 | 影響：需加 zod 依賴 | 維護：宣告式更清晰

**建議 A**：最低成本消除重複，不引入新依賴。

AskUserQuestion format

At the end of each section, use AskUserQuestion:

• Option labels indicate issue number + option letter
• Put the recommended option first
• Deep mode example:1A + 2B + 3A + 4A
• Quick mode uses option letters directly

---

Step 5: Review summary

After the four review sections are complete, output:

## 程式碼審查摘要

| # | 維度 | 議題 | 決策 |
|---|------|------|------|
| 1 | 架構 | UserService 跨層耦合 | A: 透過 OrderService |
| 2 | 品質 | 驗證邏輯重複 | A: 抽出共用函式 |
| 3 | 測試 | 缺少錯誤路徑測試 | A: 補 3 個測試 |
| 4 | 效能 | N+1 查詢 | A: 改用 batch query |

### 待修項目
- [ ] `src/services/user.ts:45` — 改用 OrderService 間接存取
- [ ] `src/handlers/` — 抽出 validateEmail() 到 src/utils/validation.ts
- [ ] `src/handlers/create.test.ts` — 補充 3 個錯誤路徑測試
- [ ] `src/repositories/order.ts:67` — N+1 改為 batch query

If "do nothing" is selected for all issues, directly output LGTM and explain the reason.

---

Review principles

• DRY matters — actively flag duplication, even if it is duplicated only twice
• Testing first — better to test too much than too little
• Engineering moderation — do not over-abstract, and do not be too hacky
• Edge cases — better to think of too many than to miss any
• Explicit over clever — explicit > clever
• Review only changes — do not comment on code that was not changed (unless the change exposes an existing problem)

---

Now Execute

The user's review request is below ARGUMENTS:. Start from Step 0.