Improve missing theme scope error message#7652
Conversation
010ba78 to
fc8c6f2
Compare
There was a problem hiding this comment.
Pull request overview
This PR improves theme API error handling by converting missing theme access scope GraphQL failures into actionable CLI abort errors.
Changes:
- Detects
ACCESS_DENIEDGraphQL errors withrequiredAccessinfetchThemes. - Replaces the raw GraphQL client error with a friendlier
AbortErrorand next-step guidance. - Adds test coverage and a patch changeset for the user-facing fix.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
packages/cli-kit/src/public/node/themes/api.ts |
Adds missing theme scope detection and friendly error generation. |
packages/cli-kit/src/public/node/themes/api.test.ts |
Covers the new friendly error path for missing read_themes. |
.changeset/friendly-theme-scope-error.md |
Records the patch release note for @shopify/cli-kit. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
fc8c6f2 to
41b800b
Compare
graygilmore
left a comment
There was a problem hiding this comment.
Love this! Couple of small things.
| 'If you authenticated with a custom app Admin API access token, open the custom app in your Shopify admin,', | ||
| 'add the required theme access scopes, reinstall the app, and use the new access token.', |
There was a problem hiding this comment.
With the announcement about custom apps we might want to change the language here.
There was a problem hiding this comment.
There are quite a few conditionals in this new logic that aren't covered by the test. We don't need to add all (e.g. don't add a test asserting that the message doesn't appear for other exceptions) but some extra coverage would be good.
| variables: {after}, | ||
| responseOptions: {handleErrors: false}, | ||
| preferredBehaviour: THEME_API_NETWORK_BEHAVIOUR, | ||
| }).catch((error: unknown) => { |
There was a problem hiding this comment.
Should we apply this same logic to findDevelopmentThemeByName? Are there any other theme fetches that we could drop this into?
| const requiredAccess = accessDeniedError?.extensions?.requiredAccess | ||
| if (typeof requiredAccess !== 'string') return undefined | ||
|
|
||
| return requiredAccess.replace(/\.$/, '') |
There was a problem hiding this comment.
I worry that there's some fragility/assumptions here where we're relying on the shape of the translation from the API but I don't have a good alternative suggestion.
| variables: {after}, | ||
| responseOptions: {handleErrors: false}, | ||
| preferredBehaviour: THEME_API_NETWORK_BEHAVIOUR, | ||
| }).catch((error: unknown) => { |
There was a problem hiding this comment.
(BOT COMMENT)
🎨 Code Style: Every other adminRequestDoc call in this file uses a bare await; the .catch() chained directly onto the awaited promise is the only instance of that pattern. A try/catch block reads more naturally for the intent (intercept an error, optionally translate it, otherwise rethrow) and doesn't rely on the reader verifying that abortIfMissingThemeAccessScope either throws or returns void.
Suggestion: Consider restructuring as:
let response
try {
response = await adminRequestDoc({
query: GetThemes,
session,
variables: {after},
responseOptions: {handleErrors: false},
preferredBehaviour: THEME_API_NETWORK_BEHAVIOUR,
})
} catch (error) {
abortIfMissingThemeAccessScope(error)
throw error
}
WHY are these changes introduced?
No public issue.
When theme commands use an account or token that cannot access Online Store themes, the Admin GraphQL API returns an
ACCESS_DENIEDerror for thethemesfield. The CLI currently surfaces the raw GraphQLClientErrorpayload, which makes the actionable fix hard to find.The high-volume observed case is a user-supplied theme password/token path, not a CLI-generated token path. Theme commands accept this value through
--password,SHOPIFY_CLI_THEME_TOKEN, or apasswordentry inshopify.theme.toml. If that value is a custom app Admin API access token, the custom app owner/admin controls its scopes in Shopify Admin. When the custom app is missingread_themes, Shopify correctly denies thethemesfield.If no password/token is supplied, the CLI uses the normal Shopify account OAuth flow and requests theme access. In that path, this error generally means the authenticated staff or collaborator account cannot access Online Store themes, so the fix is account permission/reauth rather than a missing custom app scope.
So the underlying failure is usually user/admin configuration. The CLI bug is that we present it like an unexpected GraphQL failure instead of an expected, actionable permission error.
WHAT is this pull request doing?
fetchThemesnow detects Admin GraphQLACCESS_DENIEDerrors that include a required access scope and rethrows them as a friendlyAbortError.Before, users saw a raw response/request JSON blob like:
After, users see a direct explanation and next step:
How to test your changes?
pnpm --filter @shopify/cli-kit vitest run src/public/node/themes/api.test.tsPost-release steps
No post-release steps.
Checklist
patchfor bug fixes ·minorfor new features ·majorfor breaking changes) and added a changeset withpnpm changeset addFixes shop/issues#32995