Repository navigation
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Default error mapping prevents all six migrated requests from reaching the intended missing-scope handling.
1 open finding
What changed in this PR
Routes theme mutations through the shared theme request wrapper to improve missing-scope errors and prepare for Trust Battery handling in #8562.
Changes:
- Migrates six mutation requests to
requestThemeAdminDoc. - Adds missing-scope tests for publishing and deletion.
- Adds a patch changeset.
| File | Description |
|---|---|
| packages/cli-kit/src/public/node/themes/api.ts | Routes six mutations through the shared wrapper. |
| packages/cli-kit/src/public/node/themes/api.test.ts | Tests publishing and deletion scope errors. |
| .changeset/theme-mutations-missing-scope.md | Documents the intended error-message improvement. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| const batch = filenames.slice(i, i + batchSize) | ||
| // eslint-disable-next-line no-await-in-loop | ||
| const {themeFilesDelete} = await adminRequestDoc({ | ||
| const {themeFilesDelete} = await requestThemeAdminDoc({ |
catlee
left a comment
There was a problem hiding this comment.
Looks good. Two non-blocking notes inline.
Generated with Pi.
Theme mutations keep the default Admin error handling. It changes the ClientError into a GraphQLClientError, so the missing-scope check did not see the ACCESS_DENIED error. The check now reads the GraphQL errors from both error types. The tests use the mapped error that theme mutations get.
extractGraphQLErrors returns the GraphQL errors of a ClientError or of the GraphQLClientError that errorHandler maps it to. The theme access scope check uses it instead of its own copy.
1a3a3cd to
309f40b
Compare
- The `write_themes` next step now names `theme publish`, `theme delete`, `theme rename` and `theme duplicate` as well as `theme push` and `theme dev`, because those commands now reach this message too. - Add the missing-scope test for `themeUpdate`, `deleteThemeAssets`, `themeDuplicate` and `bulkUploadThemeAssets`, so that all six theme mutations that use `requestThemeAdminDoc` are covered.
Differences in type declarationsWe detected differences in the type declarations generated by Typescript for this branch compared to the baseline ('main' branch). Please, review them to ensure they are backward-compatible. Here are some important things to keep in mind:
New type declarationsWe found no new type declarations in this PR Existing type declarationspackages/cli-kit/dist/private/node/api/graphql.d.ts@@ -1,4 +1,4 @@
-import { Variables } from 'graphql-request';
+import { ClientError, Variables } from 'graphql-request';
export declare function debugLogRequestInfo(api: string, query: string, url: string, variables?: Variables, headers?: Record<string, string>): void;
export declare function sanitizeVariables(variables: Variables): string;
/**
@@ -11,4 +11,5 @@ export declare function sanitizeVariables(variables: Variables): string;
* the full JSON representation if no messages can be extracted.
*/
export declare function extractGraphQLErrorMessages(errors: any[] | undefined): string | undefined;
-export declare function errorHandler(api: string): (error: unknown, requestId?: string) => unknown;
\ No newline at end of file
+export declare function errorHandler(api: string): (error: unknown, requestId?: string) => unknown;
+export declare function extractGraphQLErrors(error: unknown): NonNullable<ClientError['response']['errors']>;
\ No newline at end of file
|

WHY are these changes introduced?
Theme reads (
theme pull,theme list,theme info, ...) already go throughrequestThemeAdminDoc, which turns anACCESS_DENIEDGraphQL error into a friendly message that names the missing scope. Theme mutations still calledadminRequestDocdirectly, so the same error surfaced as a raw GraphQL failure.This also creates the single wrapper that #8562 needs for Trust Battery challenge handling.
WHAT is this pull request doing?
themePublishfromadminRequestDoctorequestThemeAdminDocfor the missing-scope message that theme reads already show.themeDeletefromadminRequestDoctorequestThemeAdminDocfor the missing-scope message that theme reads already show.themeUpdatefromadminRequestDoctorequestThemeAdminDocfor the missing-scope message that theme reads already show.themeDuplicatefromadminRequestDoctorequestThemeAdminDocfor the missing-scope message that theme reads already show.deleteThemeAssets(ThemeFilesDelete) fromadminRequestDoctorequestThemeAdminDocfor the missing-scope message that theme reads already show.uploadFiles(ThemeFilesUpsert) fromadminRequestDoctorequestThemeAdminDocfor the missing-scope message that theme reads already show.abortIfMissingThemeAccessScopefromClientErroronly toClientErrororGraphQLClientError, because theme mutations keep the default Admin error handling, which maps theClientErrorto aGraphQLClientErrorbefore the check runs. Without this, the six mutations above never show the missing-scope message.themePublishandthemeDeletethat assert thewrite_themesmissing-scope abort. They reject with the output of the realerrorHandler('Admin'), which is what a mutation gets. The existing read tests keep the rawClientError, because reads passhandleErrors: false.extractGraphQLErrorstoprivate/node/api/graphql.ts, next toerrorHandler, for one reader of the GraphQL errors before and after mapping. The scope check uses it, and Handle Trust Battery challenges in theme commands #8562's challenge check uses it too. A test proves thaterrorHandlerkeeps the errors that both checks read.fetchTheme,metafieldDefinitionsByOwnerType, andpasswordProtectedstay onadminRequestDoc: their queries read no theme files, so no theme decision runs for them on Core's side.Checklist
patchfor bug fixes ·minorfor new features ·majorfor breaking changes) and added a changeset withpnpm changeset add