themes:migrate: integrate with the new migrations endpoint contract#372
Conversation
Persist `partials/*` template entries to disk, render the new `migration_report` response section as a tight per-template list with green ticks and bold cyan identifiers, surface 5xx failures as the server's `general_error` suffixed with a link to file a GitHub issue, and restructure the command to mirror `themes:publish.ts` (lib stays log-free, command owns try/catch and `CliUx.ux.action`).
There was a problem hiding this comment.
Pull request overview
This PR updates themes:migrate to use the new help center theme migrations endpoint contract, enabling migrations to API version 3 with support for partials, prefixed assets, and a per-template migration report, while keeping the library layer log-free and moving UX/error handling into the command.
Changes:
- Refactors
lib/migrateto return amigration_reportand shifts CLI progress/error UX intothemes:migrate. - Adds formatting + tests for rendering a concise per-template migration report (
migrationReportToString). - Introduces centralized API error handling for migrations (
handleThemeMigrationApiError) and expands functional coverage (partials + 5xx behavior).
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/zcli-themes/tests/functional/migrate.test.ts | Updates functional tests for v3 migration, partial output, report printing, and 5xx messaging. |
| packages/zcli-themes/src/types.ts | Adds migration response/report/error-body types for the new endpoint contract. |
| packages/zcli-themes/src/lib/rewriteTemplates.test.ts | Adds coverage for writing partials under templates/partials/. |
| packages/zcli-themes/src/lib/migrationReportToString.ts | New helper to render a sorted, per-template migration report string. |
| packages/zcli-themes/src/lib/migrationReportToString.test.ts | Unit tests for report rendering and ordering rules. |
| packages/zcli-themes/src/lib/migrate.ts | Refactors migrate to call the endpoint, rewrite files, and return migration_report (no CLI UX). |
| packages/zcli-themes/src/lib/migrate.test.ts | Updates migrate unit tests for returned report + simplified error propagation. |
| packages/zcli-themes/src/lib/handleThemeMigrationApiError.ts | New migration-specific API error handler (template errors, general_error, 5xx issue link). |
| packages/zcli-themes/src/lib/handleThemeMigrationApiError.test.ts | Unit tests validating migration error routing and messaging. |
| packages/zcli-themes/src/commands/themes/migrate.ts | Adds action spinner, success report output, and delegates failures to the new error handler. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
BrunoBFerreira
left a comment
BrunoBFerreira
left a comment
There was a problem hiding this comment.
Left two minor comments.
Thank you for this 🙏
| this.log(migrationReportToString(report)) | ||
| } catch (e) { | ||
| CliUx.ux.action.stop(chalk.bold.red('!')) | ||
| handleThemeMigrationApiError(e as AxiosError, themePath) |
There was a problem hiding this comment.
Should we add some tests for these changes?
There was a problem hiding this comment.
We do have both functional tests within tests/functional/migrate.test.ts and unit tests within src/lib/handleThemeMigrationApiError.test.ts.
Or do you mean a specific error scenario I might have missed?
There was a problem hiding this comment.
Hmm I might have misread those. If it already covers the exception handling and CliUx usage then we are fine.
Introduce a dedicated `TemplateErrors` type (identifier-keyed) so the
migrations endpoint contract is represented honestly, instead of
piggy-backing on the path-keyed `ValidationErrors`. Drop the dead
`throw new Error('unreachable')` from `handleThemeMigrationApiError`
and let the function infer its return type — `error()` from
`@oclif/core` already throws unconditionally on the default path.
BrunoBFerreira
left a comment
BrunoBFerreira
left a comment
There was a problem hiding this comment.
Left a couple of questions, but nothing major so I am approving.
Thank you for the work as always 🙇
| this.log(migrationReportToString(report)) | ||
| } catch (e) { | ||
| CliUx.ux.action.stop(chalk.bold.red('!')) | ||
| handleThemeMigrationApiError(e as AxiosError, themePath) |
There was a problem hiding this comment.
Hmm I might have misread those. If it already covers the exception handling and CliUx usage then we are fine.
Description
Wires
themes:migrateto the reworked help_center migrations endpoint so v1/v2 themes can be migrated to api_version 3 with partials, prefix assets, and a per-template migration report. The command's UX matches sibling theme commands (lib stays log-free, command owns try/catch andCliUx.ux.action), prints a tight per-template report on success, and surfaces backend failures as the server'sgeneral_errorfollowed by a link to file a GitHub issue.