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`

// 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.