Most code review comments on a Salesforce PR are really about formatting. Bracket placement, an unused variable, a missing test class. None of that is wrong to flag, but it’s not the job. The job is catching the thing that passes every static check and still takes the org down at 50,000 records, three releases from now.
Reading for what scales, not what compiles
A trigger that queries inside a for loop will pass code coverage. It’ll pass a linter. It might even pass a demo with ten test records. It fails the first time someone runs a data load, and by then the person who wrote it has usually moved to a different project. Governor limits don’t show up in a diff. You have to know what the code will do at volume, which means picturing the bulk case before you read a single line of the actual implementation.
I ask four things before I look at logic: what triggers this, how many records can hit it in one transaction, what happens if it runs twice, and whether it depends on a specific order of operations somewhere else in the org. Most defects I catch in review live in the gap between those questions and what the developer assumed while writing the happy path.
The bulkification checklist nobody writes down
SOQL and DML inside loops are the obvious ones, and most experienced Apex developers catch those themselves now. The subtler failures are recursive trigger calls, callouts fired per record instead of batched, and collection logic that looks bulk-safe but still does an expensive comparison somewhere in the middle. None of that fails a unit test with 200 records. It fails a Tuesday morning data load with 40,000.
Test coverage tells you almost nothing
Seventy-five percent is a compliance number, not a quality signal. A test class can hit every line and still assert nothing beyond “no exception was thrown.” When I review test code, I’m checking for negative cases: a null field, a duplicate external ID, a user without the right permission set, or a picklist value that no longer exists. If every test in the class uses the same clean data setup, the coverage number is decorative.
The better question in review isn’t “does this have tests.” It’s “would this test fail if the logic were wrong.” Write the bug in your head, then check whether any assertion in the class would catch it. Half the time it wouldn’t, and that’s the actual finding, not the coverage percentage sitting at 82%.
Where governance debt actually gets created
Every org I’ve worked in that’s been customised for years has the same pattern behind it. Nobody made one bad decision. A hundred small, reasonable-looking decisions got approved individually, and the org ended up with several ways to do the same thing: a validation rule here, a flow there, an Apex trigger doing the same check somewhere else. Each PR looked fine on its own.
Catching that in review means asking a question most reviewers skip: does this already exist somewhere else in the org? Before approving a new flow that enforces a business rule, I check whether that rule is already sitting in a validation rule, a different flow, or a trigger nobody’s touched in two years. It takes an extra five minutes. It’s the difference between one source of truth and three that quietly disagree with each other by the time someone debugs a production issue.
Naming and structure aren’t nitpicks
A trigger handler named after the object instead of the intent. A flow with no description. An Apex class mixing unrelated responsibilities because it was faster to bolt on than to split out. These get waved through constantly because none of them break anything today. What they break is the next person’s ability to safely change the org without reading every line first, from scratch, with no context. I’ll push back on structure even when the logic is correct, because the logic being correct once isn’t the bar. The bar is someone else understanding it without asking the original author, who by then might not even be on the project anymore.
Security review isn’t a separate step
Field-level security, sharing model behaviour, CRUD and FLS enforcement on SOQL: these tend to get treated as a checklist item for a security audit months later, if at all. They should be part of every review, not a phase that happens afterward. Is this Apex class running without sharing when it should respect it? Does this LWC expose a method that skips permission checks a profile would otherwise enforce? Catching that in review costs one comment. Catching it in production costs an incident report and a client conversation nobody wants to have.
What I actually leave as comments
Not “nice work,” and not a list of style preferences dressed up as requirements. I leave the specific scenario that breaks it: this fails when the account has no owner, this recursion loop triggers if the same record updates twice in one transaction, this assumes a single currency and the org has four. Specific, reproducible, tied to a real condition that exists in this org. A developer can act on that immediately. “Please refactor for clarity” gives them nothing to act on and just adds a review cycle for its own sake.
Good review also says yes fast. If a PR does what it needs to do, handles bulk correctly, respects sharing, and the tests would actually catch a regression, approve it. Slowing down good work just to leave a comment teaches people to dread review instead of using it.
The habit that makes the biggest difference
This isn’t about being harsher on people’s code. Good review protects the org the code will run in six months, not the sandbox it happened to be written in. The developer who wrote it is usually thinking about today’s ticket. The reviewer’s job is thinking about the org at scale, under load, with the security model intact, and readable by whoever inherits it next year. That gap is the whole difference between a review that catches typos and one that actually protects the platform.