SfSkills Domains Docs GitHub

Home / devops / code-review-checklist-salesforce

code-review-checklist-salesforce

Structured Salesforce code review for Apex, triggers, async, and tests before merge or deployment — governor limits, bulk-safe triggers, CRUD/FLS and sharing posture, meaningful tests, and naming consistency. NOT for AppExchange security-review-only deep dives — use security/secure-coding-review-checklist.

Open the skill folder on GitHub

Code Review Checklist Salesforce

This skill activates when a team needs a repeatable, platform-grounded review of Salesforce custom code before merging or promoting a release. It complements CI by giving human reviewers (or an agent) a single pass through the categories that most often cause production regressions or deployment failures: bulk safety and governor consumption, data access enforcement, test quality, and consistency with Salesforce naming and structure guidance.

Before Starting

Gather this context before working on anything in this domain:

Core Concepts

Governor limits and bulk safety

Salesforce enforces per-transaction limits on SOQL, DML, heap, CPU, and callouts. Code that issues queries or DML inside a loop over trigger records scales linearly with batch size and fails at 200. The review goal is constant work per transaction: collect ids, query once, map results, update collections. Async jobs get their own limit scope; still avoid unbounded queries inside loops because heap and CPU compound.

CRUD, FLS, and sharing

Reading or writing data without respecting the running user’s permissions is both a security defect and a deployment risk. Prefer WITH USER_MODE on inline SOQL, and Security.stripInaccessible(AccessType, records).getRecords() when returning dynamic query rows to callers — user mode throws and fails the whole DML, while stripInaccessible drops the inaccessible fields and continues, so the choice is about whether silent partial success is acceptable.

WITH SECURITY_ENFORCED is not an option a reviewer accepts. Per the Apex versioned-behavior-changes page, "With API version 67.0 and later, you cannot use the WITH SECURITY_ENFORCED clause in SOQL SELECT queries in Apex code." On a 67.0+ class its presence is a compile failure, not a style note; at 57.0–66.0 it is tech debt with a named migration to WITH USER_MODE. Treating it as evidence of a secure query is a defect in the review, at every version.

The controlling fact is the apiVersion in the class's own .cls-meta.xml, not the org's release — a Summer '26 org runs a class pinned to 58.0 with the older behaviour. The canonical per-version table is Apex security idiom by API version; cite it rather than restating it here.

Tests as contract, not decoration

Org-wide coverage gates exist, but reviewers should insist on assertions that prove behavior, @testSetup for shared data, and negative paths (expected exceptions, bulk scenarios). Tests that only instantiate classes to raise line coverage hide regressions until production.

Naming and structure

Consistent class and method names reduce onboarding cost and align with the Apex Developer Guide naming guidance. Triggers should follow the Apex Developer Guide naming conventions (e.g., ObjectNameTrigger, handler class ObjectNameTriggerHandler).

Architectural compliance

The Apex Developer Guide trigger best-practices guidance calls for a single trigger per object and zero business logic in the trigger body. Logic in the body is untestable in isolation and creates merge conflicts when multiple developers change the same trigger file. A compliant pattern routes every entry point through a handler class that owns the logic and can be instantiated independently in tests. Reviewers flag any logic-bearing trigger body (branching, SOQL, DML, service calls) as a blocking architectural issue.

Common Patterns

Bulkified trigger handler

When to use: Any before or after trigger that touches related records.

How it works: Loop Trigger.new once to collect keys; run one SOQL per object type; build maps; second loop applies updates to collections; single DML per type outside inner queries.

Why not the alternative: Querying inside for (SObject row : Trigger.new) produces N+1 SOQL and fails under load.

USER_MODE for user-facing reads

When to use: Inline SOQL in services called from Lightning or Experience Cloud where the running user’s FLS must apply.

How it works: Append WITH USER_MODE to the SOQL; invalid field access throws instead of leaking data.

Why not the alternative: Manual describe checks drift when fields are added; dynamic SOQL without stripping is easy to get wrong.

Decision Guidance

SituationRecommended ApproachReason
Trigger loads related recordsCollect ids, one query, map-driven updatesKeeps SOQL count flat vs batch size
Dynamic SOQL from controlled inputsBind variables and String.escapeSingleQuotes where binds are impossibleReduces injection and accidental full-table scans
DML on mixed SObject listsDatabase.insert(records, false) only when partial success is a product requirementAll-or-nothing default is usually safer for data integrity
Test needs shared expensive data@testSetup once per classCuts CPU and avoids order-dependent tests
Method only used in system batchDocument without sharing rationale in class headerReviewers flag implicit sharing without comment
  1. Map entry points and data flows from the diff; note trigger context variables used (Trigger.newMap, etc.) and the transaction context (synchronous trigger, async job, REST controller).
  2. Walk the governor and bulk section: SOQL/DML/callouts per loop, collection sizes, queries against large objects; all synchronous trigger paths must tolerate 200 records without per-row queries or DML.
  3. Verify CRUD/FLS and sharing: explicit with sharing / without sharing / inherited sharing on every class; SOQL access mode (WITH USER_MODE, or Security.stripInaccessible on returned rows) for user-facing reads. Flag any WITH SECURITY_ENFORCED — removed at API 67.0 — rather than accepting it as enforcement.
  4. Read test classes: assert messages, bulk test methods (200 rows), negative/error paths, System.runAs for permission-sensitive code; target 90%+ meaningful line coverage, not 75% minimum.
  5. Scan naming against Apex Developer Guide conventions (ObjectNameTrigger, handler suffix, test class suffix _Test or Test); flag dead code and System.debug statements left on production paths.
  6. Verify architectural compliance: one trigger per object, no business logic in the trigger body (no SOQL, DML, branching, or service calls inline); logic must live in a dedicated handler class.
  7. Record blocking vs advisory items in the PR or release template and link to official limit or testing docs when teaching the author.

Review Checklist

Run through these before marking work in this area complete:

Salesforce-Specific Gotchas

Non-obvious platform behaviors that cause real production problems:

  1. Trigger row batch size — Triggers fire with up to 200 records; code that worked in a five-record developer test fails in full batches. Always review with 200-row tests.
  2. System context in triggers — Apex triggers do not use the running user’s sharing by default for the trigger’s own class unless you use sharing keywords or user-mode SOQL. Data exposure reviews must include this.
  3. Governor scope in test vs production — Starting/stopping tests and async testing can mask limit issues; validate hot paths in synchronous integration-style tests where limits match production transaction scope.

Output Artifacts

ArtifactDescription
PR / release notesChecklist results with blocking vs advisory labels
Linked doc citationsPointer to Apex limits, testing, or trigger best-practice pages when educating authors

Package files