Skip to content

Surface invalid theme check config as an error instead of crashing#7868

Draft
PhilippeCollin wants to merge 1 commit into
mainfrom
theme-check-config-errors
Draft

Surface invalid theme check config as an error instead of crashing#7868
PhilippeCollin wants to merge 1 commit into
mainfrom
theme-check-config-errors

Conversation

@PhilippeCollin

@PhilippeCollin PhilippeCollin commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

WHY are these changes introduced?

shopify theme check crashes with an unhandled error when its configuration can't be loaded:

Error: ENOENT: no such file or directory, open './.theme-check.yml'

theme check passes the --config value (or a discovered .theme-check.yml) straight into @shopify/theme-check-node's config loader with no error handling, so any config-loading failure escapes as an unexpected crash instead of an actionable message. The same root cause covers a whole class of failures: missing file, inline config string where a path is expected, empty --config, a directory (EISDIR), and malformed YAML. These are configuration problems, not CLI bugs.

What is happening?

A new loadThemeCheckConfig helper wraps loadConfig and turns any config-loading failure into a user-facing AbortError (shown to the user, not reported as a bug). Config loading is a single, well-defined step — the first thing themeCheckRun does, and what --print/--list already call — so a failure there is by definition a config problem; no error-message parsing needed. The fs error code (ENOENT/EISDIR) only refines the message:

  • Missing file → The Theme Check configuration file couldn't be found: <path>.
  • Directory → The Theme Check configuration path is a directory, not a file: <path>.
  • Anything else → The Theme Check configuration is invalid. (with the underlying detail)

…each with a hint to pass a valid .theme-check.yml or a preset like theme-check:recommended. Later phases (globbing, parsing, the checks) aren't caught here, so genuine bugs still report.

How to test your changes?

  1. shopify theme check --config ./does-not-exist.yml → clear "couldn't be found" message (was a crash).
  2. shopify theme check --config <a directory> → clear "not a file" message.
  3. A .theme-check.yml with an invalid shape → clear "configuration is invalid" message.

Tests

  • Unit: describeThemeCheckConfigError (missing file, directory, other failure, non-Error value) and loadThemeCheckConfig (resolves on success; wraps any failure in AbortError).
  • Integration: theme check surfaces a missing/malformed config as AbortError, and an error from the check phase still propagates unchanged.

@PhilippeCollin PhilippeCollin requested a review from a team as a code owner June 19, 2026 15:06
Copilot AI review requested due to automatic review settings June 19, 2026 15:06
@PhilippeCollin PhilippeCollin requested a review from a team as a code owner June 19, 2026 15:06
@github-actions github-actions Bot added the Area: @shopify/theme @shopify/theme package issues label Jun 19, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR prevents shopify theme check from crashing when Theme Check configuration resolution/loading fails, and instead surfaces those failures as user-facing AbortErrors (so they’re treated as expected errors and not reported as unhandled bugs).

Changes:

  • Added throwIfThemeCheckConfigError to classify known config-loading failures (missing file/dir path, invalid YAML/config) and rethrow them as AbortErrors.
  • Wrapped theme check config-loading operations (--print, --list, and the main run) to apply the new config error handling while rethrowing unrelated errors unchanged.
  • Added unit + integration tests covering the new behavior, plus a changeset for a patch release.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
packages/theme/src/cli/services/check.ts Adds throwIfThemeCheckConfigError and supporting helpers to convert known config-resolution errors into AbortErrors.
packages/theme/src/cli/services/check.test.ts Adds unit tests for throwIfThemeCheckConfigError and adjusts output mocking to preserve other exports.
packages/theme/src/cli/commands/theme/check.ts Wraps config-related operations in runWithConfigErrorHandling so config failures surface as AbortErrors instead of unhandled exceptions.
packages/theme/src/cli/commands/theme/check.test.ts Adds integration tests verifying missing/malformed configs become AbortErrors and unrelated errors still propagate.
.changeset/theme-check-config-errors.md Declares a patch changeset describing the improved error surfacing for invalid Theme Check configs.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread packages/theme/src/cli/services/check.ts Outdated
`theme check` resolves the Theme Check configuration through `themeCheckRun`
(and `loadConfig` for `--print`/`--list`). theme-check-node raises a typed
`ThemeCheckConfigError` for a missing, unreadable, or malformed configuration,
so the command recognizes a configuration problem with a single `instanceof`
check and surfaces a clear AbortError (shown to the user, not reported as an
unexpected crash). Errors from later phases are rethrown unchanged, so genuine
bugs are still reported.

Assisted-By: devx/e547eed5-2a6b-4b20-972a-c6fe290bdf0c
@PhilippeCollin PhilippeCollin force-pushed the theme-check-config-errors branch from 7d2ec0e to 7d5d761 Compare June 19, 2026 22:22
@PhilippeCollin PhilippeCollin marked this pull request as draft June 19, 2026 22:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: @shopify/theme @shopify/theme package issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants