Code review for Swift teams: what to standardize and what to stop arguing about

Code review on Swift teams can be excellent.

It can also become a weekly meeting disguised as GitHub comments.

One engineer objects to guard placement. Another wants a protocol because protocols feel adult. Someone asks whether the view model should be called State, Model, ViewState, or ThingWeWillRenameLater. Meanwhile the pull request changes entitlement handling, persistence, and a background sync path with all the calm of a raccoon in a server room.

That is the failure mode: teams spend review energy where the risk is low because it is easy to comment there.

A useful review process does not try to make every engineer think identically. That would be boring, fragile, and only slightly less annoying than the alternative.

The goal is narrower:

Standardize the decisions where inconsistency creates bugs, slows maintenance, or makes ownership unclear.

Everything else should have defaults, tools, or silence.

1. Standardize correctness checks first

Code review is not primarily a taste tribunal.

It is a risk filter.

The first review pass should ask whether the change is correct under real conditions:

• What happens offline?
• What happens on retry?
• What happens when the task is cancelled?
• What happens after app restart?
• What happens when the user taps twice?
• What happens when the server returns a valid surprise?
• What happens when this runs on an older OS version you still support?

That sounds basic because it is. Many reviews still skip it and go straight to naming.

For Swift and iOS code, correctness often hides in edges:

1. lifecycle transitions
2. async cancellation
3. actor isolation
4. main-thread UI updates
5. persistence migrations
6. subscription and entitlement refresh
7. background execution limits
8. cache invalidation

Make these review topics explicit.

A pull request touching sync should not pass review because the happy path reads nicely. A pull request touching purchases should not pass because the button label is tasteful. A pull request introducing an async task should answer who owns it, what cancels it, and where errors go.

If the team has limited review energy, spend it there.

Semicolon debates can wait until Swift adds semicolons back as a prank.

2. Standardize concurrency boundaries

Swift concurrency is good. It is also extremely good at making vague ownership look modern.

A review comment like “maybe use an actor here” is not a standard. It is a weather report.

Teams need shared rules for concurrency boundaries:

• which types are @MainActor
• where unstructured Task {} is allowed
• how cancellation is propagated
• how long-running work reports progress
• which services own shared mutable state
• how non-Sendable dependencies are isolated
• whether callbacks are wrapped at the edge or allowed inside features

Without these rules, every review becomes local philosophy.

One reviewer accepts Task.detached because it fixes the compiler warning. Another blocks it because it looks suspicious. Both may be right in different contexts. That is exactly why the context needs to be written down.

A practical standard:

• UI-facing models are @MainActor unless there is a measured reason not to be.
• Services that own mutable shared state use actors or a deliberately serialized queue.
• Feature code does not create fire-and-forget tasks without a cancellation story.
• Detached tasks require a comment explaining why they must ignore caller priority and cancellation.
• Async APIs return typed results or throw meaningful errors; they do not quietly update global state as a side hobby.

The point is not to make concurrency beautiful.

The point is to make it reviewable.

3. Standardize state shapes, not every implementation detail

SwiftUI makes state mistakes visible in the most irritating way possible: the UI sort of works until it does not.

Code review should care deeply about state shape.

It should care less about whether a helper method could be one line shorter.

For features, standardize the big questions:

1. What is the source of truth?
2. What is derived state?
3. What is persisted?
4. What survives process restart?
5. What is loading, empty, stale, failed, or partially synced?
6. Which state change is allowed to trigger side effects?

If a screen has isLoading, hasError, items, selectedItem, showRetry, and didAppearOnce loosely scattered through an observable object, review should push back.

Not because enums are fashionable.

Because impossible states become possible, and production users are very good at finding them.

A better standard is to model user-visible states explicitly:

enum OrdersViewState {
    case loading
    case empty
    case showing([Order], freshness: DataFreshness)
    case failed(OrdersError, recovery: RecoveryAction)
}

This is not mandatory enum cosplay. Sometimes separate properties are fine.

The standard should be: can the reviewer understand which states are possible, which are impossible, and which transitions are intentional?

If not, the code is not reviewable yet.

4. Standardize testing expectations by risk

“Please add tests” is one of the laziest useful comments in software.

It is useful because tests matter.

It is lazy because it does not say what kind of confidence is missing.

Swift teams should standardize testing expectations by risk level:

• Pure logic gets unit tests.
• Date, locale, currency, and formatting logic gets explicit edge cases.
• Networking code gets decoding, error mapping, retry, and cancellation coverage.
• Persistence code gets migration and corruption-path tests where practical.
• SwiftUI screens get snapshot or view-state tests only where the output matters.
• Critical flows get a small number of stable UI tests, not a simulator opera.

Do not make reviewers negotiate this from scratch on every pull request.

A tiny copy change does not need a ceremonial test. A pricing-rule change does. A migration needs more than optimism. A feature flag that changes onboarding needs a rollback story.

The useful review question is:

What failure would make this change embarrassing in production, and where is that failure caught?

If the answer is “manual QA will notice,” the team has accidentally hired hope as infrastructure.

Hope has poor retention.

5. Standardize dependency rules

Dependencies deserve review pressure because they outlive the pull request.

A Swift package added for convenience can become part of the app’s build graph, binary size, license obligations, update cadence, and security surface. That is a lot of baggage for a wrapper around three functions.

Standardize the dependency checklist:

1. Why do we need this instead of a small local implementation?
2. Who owns updates?
3. What is the license?
4. Does it compile source or ship binary artifacts?
5. Does it affect app launch, build time, or binary size?
6. Is it isolated behind one module or imported everywhere?
7. What is the removal plan if it becomes abandoned?

The important rule: dependencies should enter through boundaries.

If a charting library is needed by one analytics screen, keep it in that feature. If an HTTP helper is only used by networking, keep it there. If a package leaks into Core, it is now everyone’s roommate.

Review should be suspicious of dependencies that solve small problems globally.

That is how convenience becomes architecture by accident.

6. Standardize naming where it carries meaning

Some naming debates are worth having.

Most are not.

Names matter when they communicate ownership, lifecycle, domain meaning, or side effects. Names matter less when they are merely different flavors of acceptable.

Worth standardizing:

• Client, Service, Store, Repository, and Manager meanings
• suffixes for observable state containers
• naming for request/response DTOs versus domain models
• command methods versus query methods
• async methods that mutate versus fetch
• error types exposed across module boundaries

Not worth relitigating every week:

• whether a private helper is called makeHeader or headerView
• whether a test variable is sut or subject if the codebase already picked one
• whether a local closure could be one noun shorter
• whether ViewModel is spiritually impure in a feature that already uses it consistently

Pick conventions where names encode architecture.

Automate or ignore the rest.

Review comments should not sound like a thesaurus with commit rights.

7. Stop arguing about formatting by hand

Formatting comments are a smell.

Not because formatting does not matter. It does. Consistent formatting removes noise.

But humans should not be the formatting engine unless the team enjoys paying senior engineers to be slow regexes.

Use tools for:

• indentation
• import ordering where possible
• trailing whitespace
• file endings
• obvious lint rules
• unused code warnings
• maximum line length if the team truly cares

Then make the tool the villain.

A reviewer should be able to say “the formatter will handle this” and move on with their finite lifespan.

For Swift, that may mean SwiftFormat, SwiftLint, Xcode settings, or a smaller custom rule set. The exact stack matters less than the policy:

1. if a rule is objective, automate it
2. if it is subjective but recurring, decide once
3. if it is harmless preference, stop commenting

Manual formatting review is not craftsmanship.

It is a queueing problem with opinions.

8. Standardize pull request shape

A code review standard is useless if pull requests arrive as archaeological layers.

Large mixed PRs force reviewers to review badly. They contain UI tweaks, API client changes, schema migrations, analytics events, and “small refactor” commits that somehow touch ninety files.

Then everyone pretends review happened because two approvals appeared before lunch.

Standardize PR shape:

• one primary intent per PR
• separate mechanical refactors from behavior changes
• include screenshots or recordings for UI changes
• include migration notes for persistence changes
• describe production risk explicitly
• call out intentionally deferred work
• link feature flags, rollout plans, and cleanup tasks

The PR description should answer the questions reviewers would otherwise have to reverse-engineer:

1. What changed?
2. Why now?
3. What is risky?
4. How was it tested?
5. What should reviewers focus on?

This is not bureaucracy. It is compression.

A good PR description saves five reviewers from rediscovering the same context badly.

9. Stop reviewing personal style as architecture

Every team has one or two arguments that feel important because they are familiar.

They are usually not important.

Examples:

• “I prefer extensions at the bottom.”
• “I would name this differently.”
• “Could this be a protocol?”
• “Can we make this more generic?”
• “Maybe extract this into a helper.”

Sometimes these comments are right.

Often they are just personal taste wearing an engineering badge.

The reviewer should ask:

Does this change reduce risk, improve clarity, preserve architecture, or make future work cheaper?

If not, it is probably optional. Mark it as a nit, or better, do not leave it.

Nits are not evil. Unlabeled nits are.

A codebase can tolerate different acceptable shapes. It cannot tolerate reviewers treating every acceptable shape as a defect.

10. Make the review rubric boring

The best review culture is not dramatic.

It is predictable.

A Swift team should have a short rubric that reviewers actually use:

• correctness under failure and lifecycle edges
• concurrency ownership and cancellation
• state shape and impossible states
• test coverage matched to risk
• dependency boundaries
• persistence and migration safety
• observability for production diagnosis
• accessibility and localization where UI is affected
• API shape across module boundaries
• rollout and cleanup plan when flags are involved

That list is not glamorous. Good.

Glamour is how teams end up with very elegant bugs.

Put the rubric in the repo. Link it from the PR template. Use it in onboarding. Update it when the team learns something painful.

Then stop arguing about the same things every Thursday.

11. The useful standard: fewer comments, better comments

The goal of standardizing review is not more rules.

It is fewer, sharper comments.

A good review comment explains the risk:

This starts an unstructured task from a view method, but nothing cancels it when the view disappears. If the user navigates away during the request, the task can still update state for a stale screen. Please move ownership to the view model and cancel on deinit or task replacement.

That is better than:

Maybe avoid Task {} here?

One is engineering. The other is a shrug with syntax highlighting.

Standardize the high-risk decisions. Automate the mechanical ones. Label the subjective ones honestly. Let harmless differences survive.

That is how code review becomes useful again: less theater, more signal.

The pull request gets better. The reviewer spends less energy. The team stops treating every diff like a constitutional convention.

A civilized outcome, despite the tooling.