Repository navigation
Conversation
Assisted-By: devx/9c08eef3-3e18-4b5f-8ed8-bef380040c5d
There was a problem hiding this comment.
Pull request overview
Improves command analytics coverage for store create dev and store delete so that expected failures emitted as exact JSON (and exiting early) still get reported, and lifecycle commands gain store attribution metadata.
Changes:
- Added synchronous analytics requirement and explicit
reportAnalyticsEvent(...)reporting for--json+AbortErrorpaths before printing JSON and exiting. - Recorded store attribution metadata for
store delete(unvalidated from flag, upgraded to validated when shop lookup yields an ID) and forstore create dev(validated once a domain is returned). - Updated unit tests to cover the new attribution + expected-error analytics behavior and expanded mocking to support multiple lookup responses.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/store/src/cli/services/store/delete/dev.ts | Records validated store attribution when shop lookup succeeds (including shop ID when available). |
| packages/store/src/cli/services/store/delete/dev.test.ts | Adds coverage for attribution recording across lookup scenarios. |
| packages/store/src/cli/services/store/create/dev.ts | Records validated store attribution as soon as the created store domain is available. |
| packages/store/src/cli/services/store/create/dev.test.ts | Verifies attribution is recorded before later polling failures and not recorded for early mutation failures. |
| packages/store/src/cli/commands/store/delete.ts | Forces synchronous analytics and explicitly reports expected JSON aborts; records initial unvalidated store metadata. |
| packages/store/src/cli/commands/store/delete.test.ts | Validates synchronous analytics, JSON abort reporting, and attribution recording in command-level tests. |
| packages/store/src/cli/commands/store/create/dev.ts | Forces synchronous analytics and explicitly reports expected JSON aborts; expands try/catch to include org selection and prompts. |
| packages/store/src/cli/commands/store/create/dev.test.ts | Validates synchronous analytics and JSON abort reporting including org-selection aborts. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Assisted-By: devx/65d0e15e-19bf-4af6-8cb3-53772dd9d107
gonzaloriestra
left a comment
There was a problem hiding this comment.
If we wait for #8405 (I can prioritize that), this PR would be much simpler. We could:
- Remove
requiresSyncAnalytics - Remove manual
reportAnalyticsEvent() - Remove manual JSON serialization
- Remove
process.exit(1) - Let AbortError propagate
What do you think?
|
@gonzaloriestra yes, let's do that! I don't want to introduce changes that will need immediate adjustment. |
|
This PR seems inactive. If it's still relevant, please add a comment saying so. Otherwise, take no action. |
WHY are these changes introduced?
Context: https://github.com/shop/issues-develop/issues/22725
store create devandstore deleteprint exact JSON for expected errors and exit directly. This path skipped the async command analytics report, so JSON expected failures were invisible inapp_cli3_command. Command analytics also had no store attribution for these lifecycle commands.WHAT is this pull request doing?
Existing command analytics now covers JSON expected failures and gains known store attribution. No new events, no schema changes, and no telemetry-only network requests.
requiresSyncAnalyticsand await expected-error reporting for their JSONAbortErrorpaths before the exact JSON output and direct exit.store deleterecords the parsed store FQDN as unvalidated; the existing accessible-shop lookup upgrades it to validated with the shop ID when available.store create devrecords the returned domain as validated before polling.How to test your changes?
No behavior or output change is intended. Run
store create dev --jsonorstore delete --jsonwith an expected failure (for example, an inaccessible store) and confirm the JSON error output and exit code are unchanged.Smoke test: Compared the PR head with the base using a non-interactive
store delete --jsonexpected failure and disabled analytics delivery. Both returned byte-identical JSON and exit code 1. Only the PR head built the awaitedexpected_errorpayload before exit, including the unvalidated store domain and matching hash. No analytics event was sent and no real store was created or deleted. Thestore create deverror path was verified through focused command tests because a live create would mutate state.Checklist
patchfor bug fixes ·minorfor new features ·majorfor breaking changes) and added a changeset withpnpm changeset addNo changeset: internal telemetry only, no user-visible behavior or output change.