Skip to content

restore theme dev analytics#7345

Draft
EvilGenius13 wants to merge 1 commit intomainfrom
jf-fix-theme-dev-analytics
Draft

restore theme dev analytics#7345
EvilGenius13 wants to merge 1 commit intomainfrom
jf-fix-theme-dev-analytics

Conversation

@EvilGenius13
Copy link
Copy Markdown
Contributor

@EvilGenius13 EvilGenius13 commented Apr 17, 2026

WHY are these changes introduced?

Pressing Ctrl+C in shopify theme dev dropped analytics for the session. The keypress handler called process.exit() synchronously, terminating the process before oclif's post-run hook (where reportAnalyticsEvent fires) could run. The finally { logAnalyticsData } block in theme-command.ts is skipped by the same hard exit.

Theme dev holds 5 long-lived handles (HTTP server, chokidar watcher, polling while(true), session setInterval, stdin raw mode). Letting the event loop drain naturally would require bespoke cleanup plumbing for each one. Matching app dev's established pattern — emit analytics inline, then hard-exit — is smaller, safer, and provably works.

WHAT is this pull request doing?

Emits the analytics event inline inside dev() before process.exit(0):

  1. Inline reportAnalyticsEvent call (with shop-hash metadata populated at the same point) before the hard exit, so telemetry lands even though the oclif post-run hook and theme-command.ts's finally block are both skipped.
  2. Module-level hasReportedAnalyticsEvent guard prevents double-emit if a late throw bubbles to errorHandler.
  3. Verified live against monorail — Ctrl+C now produces exactly one cmd_all_exit = 'ok' row within p50 lag.

Out of scope (follow-ups to file):

  • Top-level SIGINT handler at packages/cli/src/index.ts:65-70 (separate exit path for non-raw-mode TTY, still process.exit(1)s and skips postrun).
  • packages/theme/src/cli/utilities/theme-environment/html.ts redirect-overflow hard-exit.
  • Graceful close of theme dev's long-lived handles so process.exit(0) could eventually be removed entirely.

How to test your changes?

  1. Run shopify theme dev against a development store.
  2. Press Ctrl+C to exit.
  3. Verify process exits immediately (no delay).
  4. Verify exactly one cmd_all_exit = 'ok' analytics event lands in monorail for the session.

Automated coverage added:

  • New unit test in services/dev.test.ts mocks reportAnalyticsEvent, simulates Ctrl+C via resolveBackgroundJob, and asserts the event fires exactly once with {exitMode: 'ok'} and that process.exit(0) runs AFTER the emission (via invocationCallOrder).
  • Second test asserts the double-emit guard (re-invoking reportDevAnalytics fires exactly once).
  • Existing 19 tests in services/dev.test.ts still green. Full packages/theme suite: 566/566 pass.

Checklist

  • I've considered possible cross-platform impacts (Mac, Linux, Windows)
  • I've considered possible documentation changes
  • I've considered analytics changes to measure impact
  • The change is user-facing, so I've added a changelog entry with pnpm changeset add

@EvilGenius13 EvilGenius13 changed the title fix(theme): restore theme dev analytics by resolving backgroundJobPromise on exit restore theme dev analytics Apr 17, 2026
@EvilGenius13 EvilGenius13 requested a review from karreiro April 20, 2026 15:54
@EvilGenius13 EvilGenius13 force-pushed the jf-fix-theme-dev-analytics branch from 43acc70 to 1e43830 Compare April 20, 2026 17:26
@EvilGenius13 EvilGenius13 marked this pull request as ready for review April 20, 2026 17:57
@EvilGenius13 EvilGenius13 requested review from a team as code owners April 20, 2026 17:57
Copilot AI review requested due to automatic review settings April 20, 2026 17:57
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR aims to restore/ensure analytics capture for shopify theme dev sessions when users exit via Ctrl+C by switching from an immediate process.exit() to a “graceful shutdown” signal that allows the command lifecycle to complete.

Changes:

  • Changed the dev server “background job” promise to be resolvable (Promise<void>) and exposed its resolver to callers.
  • Updated the Ctrl+C keypress handler to invoke an onClose callback instead of calling process.exit() directly.
  • Added a changeset entry and a unit test asserting Ctrl+C triggers onClose.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File Description
packages/theme/src/cli/utilities/theme-environment/theme-environment.ts Makes the background-job promise resolvable and returns its resolver.
packages/theme/src/cli/services/dev.ts Wires Ctrl+C to resolve the background promise and adds an explicit process.exit(0) after the main await.
packages/theme/src/cli/services/dev.test.ts Updates handler construction and adds a Ctrl+C test asserting onClose is called.
.changeset/fix-theme-dev-analytics.md Declares a patch change and describes the user-visible behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread packages/theme/src/cli/services/dev.ts
Comment thread packages/theme/src/cli/services/dev.ts
Comment thread .changeset/fix-theme-dev-analytics.md Outdated
@EvilGenius13 EvilGenius13 marked this pull request as draft April 20, 2026 20:21
@EvilGenius13 EvilGenius13 removed the request for review from karreiro April 20, 2026 20:22
When users press Ctrl+C to exit `shopify theme dev`, the keypress handler
called `process.exit()` synchronously — skipping the oclif post-run hook
where analytics normally fire, so no event reached Monorail.

Fix by emitting reportAnalyticsEvent({exitMode: 'ok'}) inline inside
dev() before process.exit(0), and populating store_fqdn_hash metadata
at the same point (since the finally { logAnalyticsData } in
theme-command.ts is also skipped by the hard exit). A module-level
hasReportedAnalyticsEvent guard prevents double-emit if a later throw
bubbles to errorHandler.

Adds a real unit test that mocks reportAnalyticsEvent and asserts it
fires exactly once with the expected payload, with correct call order
vs process.exit(0) verified via invocationCallOrder.
@EvilGenius13 EvilGenius13 force-pushed the jf-fix-theme-dev-analytics branch from 1e43830 to 0704dc7 Compare April 22, 2026 17:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants