feat(theme-check-common): two checks for range setting structural validity#1202
Open
hyldmo wants to merge 1 commit into
Open
feat(theme-check-common): two checks for range setting structural validity#1202hyldmo wants to merge 1 commit into
hyldmo wants to merge 1 commit into
Conversation
Adds two checks for `range` setting structural validity, mirroring the dual-export pattern from Shopify#1185 (ValidSelectDefault): - ValidRangeDefault: errors when a range setting's default value is outside [min, max] or not aligned to the (min + n*step) grid. Covers setting defaults, presets[].settings, section default.settings, and config/settings_schema.json. - ValidRangeStepCount: errors when a range setting has more than 101 discrete steps. Covers the setting definition and config/settings_schema.json. Both surface as hard 'Invalid schema' errors during `shopify theme dev` and in the theme editor, but `shopify theme push` accepts them silently — making this a common footgun where CI passes but the local theme dev server crashes on every render. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds two checks for
rangesetting structural validity, both mirroring the dual-export pattern from #1185 (ValidSelectDefault).ValidRangeDefaultToday you can write a
rangesetting whosedefaultlies outside[min, max]or off themin + n*stepgrid, e.g.{ "type": "range", "min": 0, "max": 160, "step": 8, "default": 60 }shopify theme pushaccepts it.shopify theme devand the theme editor reject it with:which is a hard 500 on the entire local theme server, not a per-section warning.
ValidRangeStepCountSame class of bug for Shopify's 101-step ceiling:
{ "type": "range", "min": 0, "max": 200, "step": 1 }Storefront error:
Step count is
(max − min) / step + 1; the check suggests the smallest step that fits within 101.Coverage
ValidRangeDefaultValidRangeStepCountsettings[i].defaultpresets[i].settingsdefault.settingsconfig/settings_schema.jsonValidRangeStepCountis intentionally setting-definition-only — the 101-step limit applies to the range's own min/max/step, not to preset/default values (which are covered byValidRangeDefault).Severity
ERRORfor both — these are hard runtime failures that crashtheme dev, not stylistic concerns.Float handling
1e-9tolerance sostep: 0.1, default: 4.7and similar fractional ranges don't false-positive. Suggested replacement values (nearest valid step, or minimum step that fits within 101) are included in the messages.Tests
28 new tests; full
theme-check-commonsuite (974 tests / 102 files) passes.Why
This footgun comes up regularly in real-world theme repos —
shopify theme pushand the CLI'stheme-checkboth accept these schemas, but the storefront/theme-editor parser rejects them at render time, taking down the entire local theme dev server. Catching it statically would save a class of "CI green, local crashes" reports.