Combine Code Review

Combine Code Review

Quick Reference

Review Checklist

• [ ] All sink closures use [weak self] when self owns cancellable
• [ ] No assign(to:on:self) usage (use assign(to: &$property) or sink)
• [ ] All AnyCancellables stored in Set or property (not discarded)
• [ ] Subjects exposed as AnyPublisher via eraseToAnyPublisher()
• [ ] flatMap used correctly (not when map + switchToLatest needed)
• [ ] Error handling inside flatMap to keep main chain alive
• [ ] tryMap followed by mapError to restore error types
• [ ] receive(on: DispatchQueue.main) before UI updates
• [ ] PassthroughSubject for events, CurrentValueSubject for state
• [ ] Future wrapped in Deferred when used with retry

When to Load References

• Reviewing Subjects or publisher selection → publishers.md
• Reviewing operator chains or combining publishers → operators.md
• Reviewing subscriptions or memory issues → memory.md
• Reviewing error handling or try* operators → error-handling.md

Hard gates (before you report findings)

Complete in order. Do not skip ahead while a prior gate is open.

1. Scope — Pass: You name at least one file or type under review that imports Combine or uses APIs from the Quick Reference (e.g. AnyPublisher, @Published, PassthroughSubject). If none apply, stop with “out of scope.” 2. Subscription retention — Pass: For each sink, assign, and store(in:) in scope, you state where the AnyCancellable is retained (property, Set, task lifetime) or mark ephemeral with a one-line reason (e.g. synchronous one-shot that cannot outlive caller). If you cannot tell from the snippet, say unknown and ask for surrounding storage, do not assume safe. 3. Retain-cycle claim — Pass: Confirmed leak findings state the capture chain (e.g. self → stored cancellable → closure strongly capturing self). Label suspected cases risk / verify, not confirmed leaks. When arguing safety, cite [weak self], [unowned self], or non-capturing patterns you relied on. 4. UI / main thread — Pass: For updates to UIKit/SwiftUI from a chain, you either point to receive(on: DispatchQueue.main), @MainActor, or equivalent before the UI work, or flag missing scheduling with file:line. 5. Severity and checklist — Pass: Every high or critical item includes file:line (or exact pasted lines) and names which Review Checklist row it breaks. Lower-severity notes may omit line numbers but must still be reproducible from named files.

Review Questions

1. Are all subscriptions being retained? (Check for discarded AnyCancellables) 2. Could any sink or assign create a retain cycle with self? 3. Does flatMap need to be switchToLatest for search/autocomplete? 4. What happens when this publisher fails? (Will it kill the main chain?) 5. Are error types preserved or properly mapped after try* operators?

Combine Error Handling

Error Types in Combine

Every publisher declares Publisher<Output, Failure>. Unlike other reactive frameworks, Combine enforces error types at compile time.

The Never Type

• Failure == Never means the publisher can never fail
• Required for assign(to:on:) - must convert failable publishers first
• Created by replaceError(with:) or catch with infallible fallback

Converting Error Types

// setFailureType: Never → CustomError
Just("Hello")
    .setFailureType(to: APIError.self)

// mapError: URLError → APIError
urlSession.dataTaskPublisher(for: url)
    .mapError { .networkError($0) }

try* Operators

The try-prefixed operators allow throwing but erase error type to `Swift.Error`.

Always follow tryMap with mapError:

publisher
    .tryMap { try JSONDecoder().decode(User.self, from: $0) }
    .mapError { $0 as? APIError ?? .unknown($0) }

catch vs replaceError

// replaceError: Simple fallback value
imagePublisher
    .replaceError(with: placeholderImage)

// catch: Inspect error, provide fallback publisher
primaryAPI
    .catch { error -> AnyPublisher<Data, Never> in
        if case .notFound = error {
            return fallbackAPI.replaceError(with: Data())
        }
        return Just(Data()).eraseToAnyPublisher()
    }

Critical Anti-Patterns

1. Error Handling in Main Chain Kills Publisher

// BAD: Main chain dies after first error
searchText
    .flatMap { query in networkRequest(query) }
    .replaceError(with: [])  // Publisher dead after one error!
    .sink { results in ... }

// GOOD: Handle errors inside flatMap
searchText
    .flatMap { query in
        networkRequest(query)
            .replaceError(with: [])  // Inner publisher handles error
    }
    .sink { results in ... }  // Main chain stays alive

2. Using tryMap Without mapError

// BAD: Loses specific error type
publisher.tryMap { try decode($0) }
// Failure is now plain Error

// GOOD: Restore error type
publisher.tryMap { try decode($0) }
    .mapError { $0 as? APIError ?? .unknown($0) }

3. assertNoFailure in Production

// BAD: Crashes app on network error
networkPublisher
    .assertNoFailure()  // Fatal error!

// GOOD: Handle expected errors
networkPublisher
    .catch { _ in Just(defaultValue) }

4. assign(to:on:) with Failable Publishers

// COMPILE ERROR: Failure must be Never
networkPublisher  // Failure: URLError
    .assign(to: \.data, on: viewModel)

// FIXED: Handle errors first
networkPublisher
    .replaceError(with: defaultData)  // Now Failure is Never
    .assign(to: \.data, on: viewModel)

5. Not Handling Errors Before Long-Lived Subscriptions

// BAD: First error kills subscription permanently
dataPublisher
    .receive(on: DispatchQueue.main)
    .assign(to: \.items, on: viewModel)  // Dead after first error

// GOOD: Error handling preserves subscription
dataPublisher
    .catch { _ in Just([]) }
    .receive(on: DispatchQueue.main)
    .assign(to: \.items, on: viewModel)

Review Questions

1. Is error type preserved through the pipeline? (Check for naked tryMap) 2. Will this publisher survive its first error? (Check where catch/replaceError is) 3. Is assertNoFailure used for expected errors? (Should only be programming errors) 4. Are error types unified at API boundaries with mapError? 5. Is the publisher infallible (Failure == Never) before assign(to:on:)?

Combine Memory Management

AnyCancellable Lifecycle

AnyCancellable is a type-erasing wrapper that automatically calls `cancel()` when deallocated.

Critical behavior: If not retained, the subscription cancels immediately. This often manifests as NSURLErrorDomain -999 errors.

// BAD: Subscription cancels immediately
func fetchData() {
    publisher.sink { data in self.data = data }
    // AnyCancellable not stored - immediately released!
}

// GOOD: Store in Set
var cancellables = Set<AnyCancellable>()

func fetchData() {
    publisher.sink { [weak self] data in
        self?.data = data
    }.store(in: &cancellables)
}

The Retain Cycle Pattern

Retain cycles occur when: 1. self owns the cancellable (via cancellables Set) 2. The cancellable owns the closure 3. The closure captures self strongly

self → cancellables → closure → self (CYCLE)

Critical Anti-Patterns

1. Strong Self in sink()

// RETAIN CYCLE
publisher.sink { value in
    self.property = value  // Strong capture
}.store(in: &cancellables)

// FIXED
publisher.sink { [weak self] value in
    self?.property = value
}.store(in: &cancellables)

2. assign(to:on:) with self

assign(to:on:) always captures its target strongly. No weak option exists.

// RETAIN CYCLE - ALWAYS
publisher
    .assign(to: \.property, on: self)
    .store(in: &cancellables)

// FIX 1: Use sink with weak self
publisher.sink { [weak self] value in
    self?.property = value
}.store(in: &cancellables)

// FIX 2: Use assign(to:) with @Published (iOS 14+)
@Published var property: Value
publisher.assign(to: &$property)  // No AnyCancellable returned

3. Not Storing the Cancellable

// BUG: Subscription dies immediately
func subscribe() {
    publisher.sink { print($0) }  // Discarded!
}

// FIXED
var cancellables = Set<AnyCancellable>()
func subscribe() {
    publisher.sink { print($0) }
        .store(in: &cancellables)
}

4. Nested Closures Missing Weak Captures

// Each closure needs its own [weak self]
publisher
    .flatMap { [weak self] value in
        self?.transform(value) ?? Empty()
    }
    .sink { [weak self] result in  // Need [weak self] again!
        self?.handle(result)
    }
    .store(in: &cancellables)

5. Long-Lived Subscriptions Without Weak Self

// MEMORY LEAK: Timer keeps self alive forever
Timer.publish(every: 1.0, on: .main, in: .common)
    .autoconnect()
    .sink { _ in
        self.updateUI()  // Strong capture
    }
    .store(in: &cancellables)

// FIXED
Timer.publish(every: 1.0, on: .main, in: .common)
    .autoconnect()
    .sink { [weak self] _ in
        self?.updateUI()
    }
    .store(in: &cancellables)

Single Cancellable Pattern

For auto-cancelling previous subscriptions (search debouncing):

private var searchCancellable: AnyCancellable?

func search(_ query: String) {
    // Previous subscription automatically cancelled
    searchCancellable = searchPublisher(query)
        .sink { [weak self] results in
            self?.results = results
        }
}

Review Questions

1. Is every sink() and assign() result stored? 2. Does sink() use [weak self] when self owns the cancellable? 3. Is assign(to:on:) used with self? (Always a leak) 4. Are there nested closures missing weak captures? 5. Are long-lived subscriptions (timers, notifications) using weak self?

Combine Operators

Key Operators by Category

Transforming

Combining

Timing

map vs flatMap vs switchToLatest

// map: Simple transformation
publisher.map { $0.uppercased() }

// flatMap: Transformation produces a publisher
publisher.flatMap { url in
    URLSession.shared.dataTaskPublisher(for: url)
}

// switchToLatest: Cancel previous (search/autocomplete)
searchText
    .map { query in searchAPI(query) }
    .switchToLatest()  // Cancels previous request

combineLatest vs merge vs zip

// merge: Combine same-type streams
let allTaps = buttonA.merge(with: buttonB)

// combineLatest: React to any change (form validation)
Publishers.CombineLatest(emailValid, passwordValid)
    .map { $0 && $1 }

// zip: Wait for both (parallel requests)
Publishers.Zip(fetchUser, fetchPreferences)

Critical Anti-Patterns

1. flatMap Instead of switchToLatest for Search

// BAD: All requests execute, results arrive out of order
searchText.flatMap { query in search(query) }

// GOOD: Cancel previous requests
searchText
    .map { query in search(query) }
    .switchToLatest()

2. Wrong Threading Operator

// BAD: subscribe(on:) doesn't affect where values received
URLSession.shared.dataTaskPublisher(for: url)
    .subscribe(on: DispatchQueue.main)  // WRONG!
    .sink { /* NOT on main thread */ }

// GOOD: Use receive(on:) for downstream
URLSession.shared.dataTaskPublisher(for: url)
    .receive(on: DispatchQueue.main)
    .sink { /* On main thread */ }

3. combineLatest with Publisher That Never Emits

// BUG: Won't emit until ALL publishers emit at least once
Publishers.CombineLatest(requiredField, optionalAction)
// If optionalAction never fires, stream never starts

4. Using tryMap Without mapError

// BAD: Erases error type to plain Error
publisher.tryMap { try decode($0) }

// GOOD: Restore specific error type
publisher.tryMap { try decode($0) }
    .mapError { $0 as? APIError ?? .unknown($0) }

Review Questions

1. Is flatMap appropriate, or should it be map + switchToLatest? 2. Are combineLatest publishers guaranteed to emit at least once? 3. Is receive(on:) used before UI updates (not subscribe(on:))? 4. Are try* operators followed by mapError for type safety? 5. Could debounce or throttle reduce unnecessary work?

Combine Publishers

Built-in Publishers

Subject Types

PassthroughSubject - Use for Events

let buttonTaps = PassthroughSubject<Void, Never>()
buttonTaps.send(())  // Subscribers only notified if already subscribed

• No initial value required
• No .value property
• New subscribers receive only future values
• Best for: button taps, user actions, transient events

CurrentValueSubject - Use for State

let loadingState = CurrentValueSubject<LoadingState, Never>(.idle)
print(loadingState.value)  // Can query current state

• Initial value required
• .value property for direct access
• New subscribers receive current value immediately
• Best for: settings, loading state, toggles

Critical Anti-Patterns

1. Exposing Subjects Publicly

// BAD: External code can call loginSubject.send(...)
class AuthManager {
    let loginSubject = PassthroughSubject<User, Error>()
}

// GOOD: Expose as read-only publisher
class AuthManager {
    private let loginSubject = PassthroughSubject<User, Error>()
    var loginPublisher: AnyPublisher<User, Error> {
        loginSubject.eraseToAnyPublisher()
    }
}

2. Using Just for Arrays When Sequence Intended

// BAD: Emits entire array as one value
Just([1, 2, 3]).sink { print($0) }  // prints: [1, 2, 3]

// GOOD: Emits each element
[1, 2, 3].publisher.sink { print($0) }  // prints: 1, 2, 3

3. Future Without Deferred for Retry

// BAD: Retry reuses cached failure
Future { promise in networkCall(completion: promise) }
    .retry(3)  // Same cached result retried!

// GOOD: Wrap in Deferred
Deferred {
    Future { promise in networkCall(completion: promise) }
}.retry(3)  // New Future created each retry

4. Wrong Subject Type for Use Case

// BAD: PassthroughSubject for state (late subscribers miss value)
let isLoggedIn = PassthroughSubject<Bool, Never>()

// GOOD: CurrentValueSubject for state
let isLoggedIn = CurrentValueSubject<Bool, Never>(false)

Review Questions

1. Are Subjects exposed publicly or converted to AnyPublisher? 2. Is the correct Subject type used (events vs state)? 3. Is Future used with retry without Deferred wrapper? 4. Are built-in publishers preferred over custom implementations? 5. Is .value access needed? (Requires CurrentValueSubject)