Theme code review: what to look for

"Theme code review" means two quite different jobs and it helps to say which one you are doing.

Reviewing a change — a pull request, a feature, a fix. The question is whether this should merge. It is bounded, it has an author who can answer questions, and most of the value is in the first fifteen minutes.

Reviewing an inherited theme — you have been handed a codebase four agencies have touched and asked whether it is salvageable. The question is what it will cost to work in. That is an audit, it produces a document rather than comments, and it is answered by reading the shape of the theme rather than any particular file.

Most of what follows applies to both, but the ordering is written for the first. If you are auditing, start at the schema and settings section — how a theme is configured tells you more about its maintainability than any individual template.

Theme Check is Shopify’s linter for Liquid and JSON in themes and theme app extensions. The documentation describes it as detecting errors and enforcing Liquid best practices, covering syntax errors, missing templates, unused variables and snippets, unknown and deprecated tags, performance issues and style errors. It runs on demand, in CI, or through a language server in the editor.

If it is not running in CI, that is the first review comment and it is worth more than anything else you could write that day. Every check it performs is a check a person would otherwise do badly and resentfully. Human review time is expensive and finite, and spending it on an unused snippet is a waste of the only thing in this process that can actually think.

The secondary benefit is cultural. A review where the machine has already said the boring things is a review where the human comments are all substantive, which is the difference between a process people value and one they route around.

Before reading the diff line by line, work out its blast radius. The size of a change in lines has almost no relationship to its risk in a theme.

A new section file that no template currently renders is close to zero risk: it cannot affect anything until someone adds it. A three-line edit to a snippet rendered inside the header is on every page of the store, including checkout-adjacent flows, including the templates nobody has looked at since 2022. The second one deserves ten times the attention and usually gets less, because it is shorter.

The practical version: for each changed file, grep the theme for where it is rendered. A snippet included in eleven places needs the reviewer to ask what happens in the other ten contexts, and specifically whether the object it now expects is always present. product.featured_image inside a snippet that also gets rendered from a blog template is the classic failure, and it will not show up in local testing unless someone thinks to look.

Ask the same question of schema changes. Removing or renaming a setting in a section schema affects every page where a merchandiser has already configured that section, and the effect lands at publish time rather than in review.

Liquid renders server-side, so its cost lands directly in time-to-first-byte, which means it is invisible in most front-end profiling and very visible to a customer. In our experience essentially all of it comes from three patterns.

Unpaginated iteration. A for loop over a collection’s products without pagination is a review comment every time. The paginate tag supports up to 50 items per page, and that ceiling exists for a reason. The version that hurts most is the one that works fine on the collection used in development and meets the 900-product collection in production.

Nested loops. Iterating products and then iterating each product’s variants, metafields or media multiplies quickly, and the multiplication is invisible in the diff because each loop looks reasonable on its own. When you see a second for inside the first, work out the worst-case product of the two bounds and ask whether that number is acceptable.

Lookups repeated inside iteration. Anything that resolves an object — a product by handle, a collection, a metafield on a related object — costs more inside a loop than the surrounding markup does, and it is usually hoistable. If the value does not change between iterations, assign it once before the loop.

The reviewer’s question is not "is this fast" but "what is the largest input this will ever see, and has anyone tried it". A collection with five products in the dev store proves nothing.

Reviewers who came from application development tend to skim section schemas as configuration boilerplate. It is the most consequential part of a theme change, because it decides whether the next request needs a developer at all.

Three questions per schema block. Is anything hardcoded that should be a setting? A heading, a link, an image, a piece of copy baked into Liquid becomes a ticket every time marketing wants it changed. Is anything a setting that should not be? Exposing structural choices — grid column counts, arbitrary padding values, colour pickers on every element — hands merchandisers enough rope to make the site inconsistent, and the resulting pages become the reason for the next redesign. Can a bad value break the page? A setting that renders into markup without a fallback will meet an empty string eventually.

The platform limits are worth knowing before designing a pattern: Shopify documents that JSON templates can render up to 25 sections, that each section can have up to 50 blocks, and that a theme can contain up to 1,000 JSON templates. Most stores never approach these, but a page-building approach that treats every element as a block can find the 50 quite quickly, and a per-product template strategy needs the 1,000 in mind.

The useful framing for the author: name the person who will use this setting, and describe what they will do with it on a Tuesday. If the answer is vague, the setting is speculative and probably should not exist.

Four checks, in descending order of how often they find something.

How much JavaScript does this add, and is it deferred? Anything blocking parse needs a reason. A feature that ships a library to do something CSS can do is worth a conversation, and that conversation goes better in review than after launch.

Does it survive the section being re-rendered? The theme editor re-renders sections without a full page load. Code that initialises on DOMContentLoaded only will appear broken to the merchandiser configuring it, which generates a bug report that is really a missing event listener.

Does the core function work without JavaScript, or degrade honestly? Not everything can, but navigation, add-to-cart and form submission generally should, and a review is the right moment to ask. The failure mode to hunt is the one that renders a control which silently does nothing when its script fails to load.

Images. Are dimensions requested explicitly rather than served at full size, is there a width and height attribute so the layout does not shift, and does the above-the-fold image load eagerly while everything below it is lazy? Image handling is the highest-leverage performance work in almost every theme and the easiest thing to get wrong in a hurry.

The failure mode of theme review is not missing a bug. It is producing so many comments that the author triages them, fixes the easy ones, and merges. Forty comments and three of them mattered is a worse outcome than three comments, because nobody knows which three.

So: do not comment on formatting a linter could enforce — configure the linter instead. Do not rewrite working code in your own idiom. Do not ask for an abstraction because a pattern appears twice; ask on the third. Do not relitigate a decision made in a previous review unless something has changed.

And separate blocking from non-blocking explicitly. We mark comments as one or the other, and hold ourselves to a small number of blockers: correctness, security, accessibility, performance with a plausible production input, and anything that cannot be changed later without a migration. Everything else is a note the author may take or leave. A review whose blockers are few and unambiguous gets acted on; one that treats every observation equally gets skimmed.

Review is the cheapest quality mechanism available and the easiest to perform theatrically. A team that reviews everything and blocks on nothing has a ritual rather than a process, and a team that blocks on everything gets routed around the first time something is urgent.

The version that lasts is narrow and enforced: a linter in CI, a short list of things that genuinely block, and a reviewer who reads for blast radius before they read for style. That is also the difference between a theme that is still workable in three years and one that becomes the reason for a rebuild — which is why we treat review standards as part of the deliverable when we do [theme customisation work](/services/build/shopify-theme-customization), not as something a client team is left to invent afterwards.

If you have inherited a theme with no review process and no linter, add the linter first, agree the blocking list second, and do not attempt to retrofit standards onto existing code. Fix what you touch. Retroactive cleanup projects are rarely finished and never funded twice.

• Shopify theme customisation
• Liquid performance: the loops that cost you a second
• Theme CI: testing a Shopify theme before it ships