diff --git a/.changeset/fix-theme-dev-analytics.md b/.changeset/fix-theme-dev-analytics.md new file mode 100644 index 00000000000..d244465fdd2 --- /dev/null +++ b/.changeset/fix-theme-dev-analytics.md @@ -0,0 +1,5 @@ +--- +'@shopify/theme': patch +--- + +Fix `theme dev` analytics being dropped on Ctrl+C. The command called `process.exit()` synchronously inside the keypress handler, which skipped the oclif post-run hook where analytics normally fire. The fix emits the event inline before exit. diff --git a/packages/theme/src/cli/commands/theme/dev.ts b/packages/theme/src/cli/commands/theme/dev.ts index 99eb6c25b7f..c749ed07b60 100644 --- a/packages/theme/src/cli/commands/theme/dev.ts +++ b/packages/theme/src/cli/commands/theme/dev.ts @@ -166,6 +166,7 @@ You can run this command only in a directory that matches the [default Shopify t await dev({ adminSession, + commandConfig: this.config, directory: flags.path, store: flags.store, password: flags.password, diff --git a/packages/theme/src/cli/services/dev.test.ts b/packages/theme/src/cli/services/dev.test.ts index f0ebf6f3e90..ff154c29bb2 100644 --- a/packages/theme/src/cli/services/dev.test.ts +++ b/packages/theme/src/cli/services/dev.test.ts @@ -1,9 +1,17 @@ -import {openURLSafely, renderLinks, createKeypressHandler} from './dev.js' -import {describe, expect, test, vi, beforeEach, afterEach} from 'vitest' +import {dev, openURLSafely, renderLinks, createKeypressHandler, reportDevAnalytics} from './dev.js' +import {setupDevServer} from '../utilities/theme-environment/theme-environment.js' +import {hasRequiredThemeDirectories} from '../utilities/theme-fs.js' +import {isStorefrontPasswordProtected} from '../utilities/theme-environment/storefront-session.js' +import {initializeDevServerSession} from '../utilities/theme-environment/dev-server-session.js' +import {describe, expect, test, vi, beforeEach, afterEach, type MockInstance} from 'vitest' import {buildTheme} from '@shopify/cli-kit/node/themes/factories' import {DEVELOPMENT_THEME_ROLE} from '@shopify/cli-kit/node/themes/utils' import {renderSuccess, renderWarning} from '@shopify/cli-kit/node/ui' import {openURL} from '@shopify/cli-kit/node/system' +import {reportAnalyticsEvent} from '@shopify/cli-kit/node/analytics' +import {addPublicMetadata, addSensitiveMetadata} from '@shopify/cli-kit/node/metadata' +import {getAvailableTCPPort, checkPortAvailability} from '@shopify/cli-kit/node/tcp' +import {Config} from '@oclif/core' vi.mock('@shopify/cli-kit/node/ui') vi.mock('@shopify/cli-kit/node/colors', () => ({ @@ -16,6 +24,36 @@ vi.mock('@shopify/cli-kit/node/colors', () => ({ vi.mock('@shopify/cli-kit/node/system', () => ({ openURL: vi.fn(), })) +vi.mock('@shopify/cli-kit/node/analytics', () => ({ + reportAnalyticsEvent: vi.fn(), +})) +vi.mock('@shopify/cli-kit/node/metadata', () => ({ + addPublicMetadata: vi.fn(), + addSensitiveMetadata: vi.fn(), +})) +vi.mock('@shopify/cli-kit/node/tcp', () => ({ + getAvailableTCPPort: vi.fn(), + checkPortAvailability: vi.fn(), +})) +vi.mock('../utilities/theme-environment/theme-environment.js', () => ({ + setupDevServer: vi.fn(), +})) +vi.mock('../utilities/theme-fs.js', () => ({ + hasRequiredThemeDirectories: vi.fn(), + mountThemeFileSystem: vi.fn().mockReturnValue({}), +})) +vi.mock('../utilities/theme-fs-empty.js', () => ({ + emptyThemeExtFileSystem: vi.fn().mockReturnValue({}), +})) +vi.mock('../utilities/theme-environment/storefront-session.js', () => ({ + isStorefrontPasswordProtected: vi.fn(), +})) +vi.mock('../utilities/theme-environment/storefront-password-prompt.js', () => ({ + ensureValidPassword: vi.fn(), +})) +vi.mock('../utilities/theme-environment/dev-server-session.js', () => ({ + initializeDevServerSession: vi.fn(), +})) const store = 'my-store.myshopify.com' const theme = buildTheme({id: 123, name: 'My Theme', role: DEVELOPMENT_THEME_ROLE})! @@ -124,7 +162,7 @@ describe('createKeypressHandler', () => { test('opens localhost when "t" is pressed', () => { // Given - const handler = createKeypressHandler(urls, ctx) + const handler = createKeypressHandler(urls, ctx, vi.fn()) // When handler('t', {name: 't'}) @@ -135,7 +173,7 @@ describe('createKeypressHandler', () => { test('opens theme preview when "p" is pressed', () => { // Given - const handler = createKeypressHandler(urls, ctx) + const handler = createKeypressHandler(urls, ctx, vi.fn()) // When handler('p', {name: 'p'}) @@ -146,7 +184,7 @@ describe('createKeypressHandler', () => { test('opens theme editor when "e" is pressed', () => { // Given - const handler = createKeypressHandler(urls, ctx) + const handler = createKeypressHandler(urls, ctx, vi.fn()) // When handler('e', {name: 'e'}) @@ -157,7 +195,7 @@ describe('createKeypressHandler', () => { test('opens gift card preview when "g" is pressed', () => { // Given - const handler = createKeypressHandler(urls, ctx) + const handler = createKeypressHandler(urls, ctx, vi.fn()) // When handler('g', {name: 'g'}) @@ -169,7 +207,7 @@ describe('createKeypressHandler', () => { test('appends preview path to theme editor URL when lastRequestedPath is not "/"', () => { // Given const ctxWithPath = {lastRequestedPath: '/products/test-product'} - const handler = createKeypressHandler(urls, ctxWithPath) + const handler = createKeypressHandler(urls, ctxWithPath, vi.fn()) // When handler('e', {name: 'e'}) @@ -182,7 +220,7 @@ describe('createKeypressHandler', () => { test('debounces rapid keypresses - only opens URL once during debounce window', () => { // Given - const handler = createKeypressHandler(urls, ctx) + const handler = createKeypressHandler(urls, ctx, vi.fn()) // When handler('t', {name: 't'}) @@ -197,7 +235,7 @@ describe('createKeypressHandler', () => { test('allows keypresses after debounce period expires', () => { // Given - const handler = createKeypressHandler(urls, ctx) + const handler = createKeypressHandler(urls, ctx, vi.fn()) // When handler('t', {name: 't'}) @@ -220,7 +258,7 @@ describe('createKeypressHandler', () => { test('debounces different keys during the same debounce window', () => { // Given - const handler = createKeypressHandler(urls, ctx) + const handler = createKeypressHandler(urls, ctx, vi.fn()) // When handler('t', {name: 't'}) @@ -232,4 +270,105 @@ describe('createKeypressHandler', () => { expect(openURL).toHaveBeenCalledTimes(1) expect(openURL).toHaveBeenCalledWith(urls.local) }) + +}) + +describe('dev() Ctrl-C analytics', () => { + const mockConfig = {} as unknown as Config + const adminSession = {storeFqdn: 'test.myshopify.com', token: 'x'} + + let exitSpy: MockInstance + let resolveBackgroundJob: () => void + + const baseOptions = { + adminSession, + commandConfig: mockConfig, + directory: '/tmp/theme', + store: 'test.myshopify.com', + open: false, + theme, + force: false, + 'theme-editor-sync': false, + 'live-reload': 'hot-reload' as const, + 'error-overlay': 'default' as const, + noDelete: false, + ignore: [], + only: [], + } + + beforeEach(() => { + vi.mocked(hasRequiredThemeDirectories).mockResolvedValue(true) + vi.mocked(isStorefrontPasswordProtected).mockResolvedValue(false) + vi.mocked(initializeDevServerSession).mockResolvedValue({ + storeFqdn: adminSession.storeFqdn, + token: adminSession.token, + } as any) + vi.mocked(getAvailableTCPPort).mockResolvedValue(9292) + vi.mocked(checkPortAvailability).mockResolvedValue(true) + + const backgroundJobPromise = new Promise((resolve) => { + resolveBackgroundJob = resolve + }) + vi.mocked(setupDevServer).mockReturnValue({ + serverStart: vi.fn().mockResolvedValue(undefined), + renderDevSetupProgress: vi.fn().mockResolvedValue(undefined), + backgroundJobPromise, + resolveBackgroundJob: resolveBackgroundJob!, + dispatchEvent: vi.fn(), + } as any) + + exitSpy = vi.spyOn(process, 'exit').mockImplementation(() => undefined as never) + }) + + afterEach(() => { + vi.clearAllMocks() + exitSpy.mockRestore() + }) + + test('reports analytics event exactly once with exitMode=ok and store_fqdn_hash metadata on Ctrl-C', async () => { + // Given / When + const devPromise = dev(baseOptions) + + // Flush microtasks so the Promise.all is awaiting backgroundJobPromise. + await new Promise((resolve) => setImmediate(resolve)) + + // Simulate Ctrl-C by resolving the background job. + resolveBackgroundJob() + await devPromise + + // Then + expect(reportAnalyticsEvent).toHaveBeenCalledTimes(1) + expect(reportAnalyticsEvent).toHaveBeenCalledWith({config: mockConfig, exitMode: 'ok'}) + + expect(addPublicMetadata).toHaveBeenCalledTimes(1) + const publicMetadataFn = vi.mocked(addPublicMetadata).mock.calls[0]![0] as () => Record + expect(publicMetadataFn()).toEqual({store_fqdn_hash: expect.any(String)}) + + expect(addSensitiveMetadata).toHaveBeenCalledTimes(1) + const sensitiveMetadataFn = vi.mocked(addSensitiveMetadata).mock.calls[0]![0] as () => Record + expect(sensitiveMetadataFn()).toEqual({store_fqdn: adminSession.storeFqdn}) + + expect(exitSpy).toHaveBeenCalledWith(0) + + // Order: analytics before exit. + const reportOrder = vi.mocked(reportAnalyticsEvent).mock.invocationCallOrder[0]! + const exitOrder = exitSpy.mock.invocationCallOrder[0]! + expect(reportOrder).toBeLessThan(exitOrder) + }) + + test('double-emit guard: reportDevAnalytics called twice only emits once', async () => { + // Given: run dev() once to set the flag + const devPromise = dev(baseOptions) + await new Promise((resolve) => setImmediate(resolve)) + resolveBackgroundJob() + await devPromise + + expect(reportAnalyticsEvent).toHaveBeenCalledTimes(1) + + // When: re-invoke reportDevAnalytics directly (simulates a second exit path) + await reportDevAnalytics(mockConfig, adminSession as any) + + // Then: still only one call + expect(reportAnalyticsEvent).toHaveBeenCalledTimes(1) + }) }) diff --git a/packages/theme/src/cli/services/dev.ts b/packages/theme/src/cli/services/dev.ts index 47ad2a174a7..076f0f646f9 100644 --- a/packages/theme/src/cli/services/dev.ts +++ b/packages/theme/src/cli/services/dev.ts @@ -14,15 +14,22 @@ import {checkPortAvailability, getAvailableTCPPort} from '@shopify/cli-kit/node/ import {AbortError} from '@shopify/cli-kit/node/error' import {openURL} from '@shopify/cli-kit/node/system' import {debounce} from '@shopify/cli-kit/common/function' +import {reportAnalyticsEvent} from '@shopify/cli-kit/node/analytics' +import {addPublicMetadata, addSensitiveMetadata} from '@shopify/cli-kit/node/metadata' +import {hashString} from '@shopify/cli-kit/node/crypto' import chalk from '@shopify/cli-kit/node/colors' +import {Config} from '@oclif/core' import readline from 'readline' const DEFAULT_HOST = '127.0.0.1' const DEFAULT_PORT = '9292' +let hasReportedAnalyticsEvent = false + interface DevOptions { adminSession: AdminSession + commandConfig: Config directory: string store: string password?: string @@ -69,6 +76,8 @@ export async function dev(options: DevOptions) { }) } + hasReportedAnalyticsEvent = false + if (options.listing) { await ensureListingExists(options.directory, options.listing) } @@ -128,11 +137,14 @@ export async function dev(options: DevOptions) { }, } - const {serverStart, renderDevSetupProgress, backgroundJobPromise} = setupDevServer(options.theme, ctx) + const {serverStart, renderDevSetupProgress, backgroundJobPromise, resolveBackgroundJob} = setupDevServer( + options.theme, + ctx, + ) readline.emitKeypressEvents(process.stdin) - const keypressHandler = createKeypressHandler(urls, ctx) + const keypressHandler = createKeypressHandler(urls, ctx, resolveBackgroundJob) process.stdin.on('keypress', keypressHandler) await Promise.all([ @@ -149,17 +161,36 @@ export async function dev(options: DevOptions) { } }), ]) + + await reportDevAnalytics(options.commandConfig, options.adminSession) + + process.exit(0) +} + +export async function reportDevAnalytics(config: Config, session: AdminSession): Promise { + if (hasReportedAnalyticsEvent) return + hasReportedAnalyticsEvent = true + + try { + await addPublicMetadata(() => ({store_fqdn_hash: hashString(session.storeFqdn)})) + await addSensitiveMetadata(() => ({store_fqdn: session.storeFqdn})) + await reportAnalyticsEvent({config, exitMode: 'ok'}) + } catch (_error) { + // Analytics must never block exit. + } } export function createKeypressHandler( urls: {local: string; giftCard: string; themeEditor: string; preview: string}, ctx: {lastRequestedPath: string}, + onClose: () => void, ) { const debouncedOpenURL = debounce(openURLSafely, 100, {leading: true, trailing: false}) return (_str: string, key: {ctrl?: boolean; name?: string}) => { if (key.ctrl && key.name === 'c') { - process.exit() + onClose() + return } switch (key.name) { @@ -180,6 +211,7 @@ export function createKeypressHandler( case 'g': debouncedOpenURL(urls.giftCard, 'gift card preview') break + case undefined: default: break } diff --git a/packages/theme/src/cli/utilities/theme-environment/theme-environment.ts b/packages/theme/src/cli/utilities/theme-environment/theme-environment.ts index 2660bd1acb4..0426a527f2b 100644 --- a/packages/theme/src/cli/utilities/theme-environment/theme-environment.ts +++ b/packages/theme/src/cli/utilities/theme-environment/theme-environment.ts @@ -18,7 +18,11 @@ import type {Checksum, Theme} from '@shopify/cli-kit/node/themes/types' import type {DevServerContext} from './types.js' export function setupDevServer(theme: Theme, ctx: DevServerContext) { - const {promise: backgroundJobPromise, reject: rejectBackgroundJob} = promiseWithResolvers() + const { + promise: backgroundJobPromise, + resolve: resolveBackgroundJob, + reject: rejectBackgroundJob, + } = promiseWithResolvers() const watcherPromise = setupInMemoryTemplateWatcher(theme, ctx) const envSetup = ensureThemeEnvironmentSetup(theme, ctx, rejectBackgroundJob) @@ -33,6 +37,7 @@ export function setupDevServer(theme: Theme, ctx: DevServerContext) { dispatchEvent: server.dispatch, renderDevSetupProgress: envSetup.renderProgress, backgroundJobPromise, + resolveBackgroundJob, } }