Swift Code Review

Instructions

Step 1: Read All Provided Files

Read every file mentioned or pasted before writing any comments. Do not review code you have not fully read.

If reviewing a PR, ask for:

• The feature or bug being fixed
• The files changed
• Any context about constraints or trade-offs

Step 2: Apply the Review Checklist

Architecture (MVVM)

• Business logic is in ViewModel or Service — not in View/ViewController body
• External dependencies (network, storage, time) are injected via protocol, not hardcoded
• No URLSession.shared, UserDefaults.standard, or Date() called directly from Views
• No global singletons used without injection support
• Thin View layer: no conditional business logic, no data fetching in onAppear without ViewModel delegation

Swift Quality

• No force unwraps (!) in production paths
• Errors propagated via throws or Result — not silently swallowed
• let preferred over var where value does not change
• No magic numbers or hardcoded strings — use named constants or localizable strings
• One primary type per file

Concurrency

• async/await used for all new async code (not completion handlers)
• @MainActor applied to ViewModel and any type that updates UI state
• No DispatchQueue.main.async in ViewModel or Service layers
• No sleep() anywhere
• Sendable conformance considered for types crossing actor boundaries
• Task objects are stored and cancelled if the parent scope is deallocated

Testing

• New business logic has corresponding tests
• New dependencies are injectable (protocol-backed)
• Existing tests still compile and pass
• No test changes that weaken coverage of critical paths

Naming

• Types: UpperCamelCase
• Functions / variables: lowerCamelCase
• Booleans: assertion form (isLoading, hasError, canSubmit, isPremium)
• Avoid abbreviations unless universally understood (url, id, api)
• Test methods: test_when_<Condition>_should_<ExpectedOutcome>()
• Protocol names: noun or adjective, not Protocol suffix (prefer UserRepository over UserRepositoryProtocol where unambiguous, but Protocol suffix is acceptable for clarity)

SwiftUI Specific

• @StateObject / @State used in the View that owns the instance
• @ObservedObject / @Binding used for passed-in instances
• No @EnvironmentObject used for types that should be injected explicitly
• #Preview compiles and shows correct content
• Accessibility labels on interactive elements that lack visible text

UIKit Specific

• ViewControllers are thin: no network calls, no business logic
• IBOutlet and IBAction limited to UI wiring only
• Memory management: no retain cycles in closures (use [weak self])
• No layout code hardcoded in viewDidLoad that belongs in a dedicated setup method

Step 3: Format the Review

Group by severity. Be specific. Include file name and line number or code excerpt.

## Code Review: [File or Feature Name]

### Critical — Must Fix
These block approval. Fix before merge.

- **[Issue title]** in `FileName.swift`
  ```swift
  // Problematic code

Why: [Clear explanation of the problem and risk] Fix:

// Suggested fix

Suggested — Should Fix

Strong recommendations that improve correctness, testability, or maintainability.

• [Issue] — [file] — [brief explanation and fix direction]

Minor — Nice to Have

Style, preference, or low-risk improvements. Non-blocking.

• [Note]

Approved

What is done well — acknowledge good patterns explicitly.

• [What works well and why]

### Step 4: Final Verdict

End with one of:
- **Approved** — ready to merge as-is
- **Approved with suggestions** — can merge, but consider the Suggested items
- **Changes requested** — Critical items must be resolved before merge

## Examples

### Example 1: ViewModel review

User says: "Review my LoginViewModel"

Actions:
1. Read `LoginViewModel.swift`
2. Read `LoginViewModelTests.swift` (if it exists)
3. Apply checklist
4. Note: is it testable? Are dependencies injected? Is async correct?
5. Output structured feedback

### Example 2: PR review across multiple files

User says: "Review my PR for the checkout feature — here are the changed files"

Actions:
1. Read all changed files
2. Understand the feature end-to-end
3. Apply checklist to each file
4. Identify cross-file issues (e.g., a service that's not injectable, a missing mock)
5. Output grouped review by file, then overall verdict

### Example 3: Quick sanity check

User says: "Is this code correct?"

Actions:
1. Read the code
2. Focus checklist on correctness (async, force unwraps, error handling)
3. Brief response: "This looks correct, but note X" or list Critical issues

## Troubleshooting

**Too many issues to list**: Focus on Critical and top Suggested. List remaining Minor issues as a bullet group: "Additional minor style notes: [list]". Do not overwhelm with exhaustive nit-picking.

**Reviewer disagreement on style**: If the project has a style guide (`CLAUDE.md` or linting config), defer to it. Otherwise, note it as Minor and avoid blocking on pure preference.

**Legacy code with known issues**: When reviewing a small change in a large legacy file, focus the review scope on the changed lines. Do not audit the entire file — note "broader refactoring needed in this file" as a separate future work item.