Skip to content

Commit c6a114d

Browse files
committed
fix(theme): restore theme dev analytics on Ctrl+C exit
When users pressed Ctrl+C to exit `shopify theme dev`, the keypress handler called `process.exit()` synchronously, bypassing the oclif post-run hook where analytics normally fire. As a result, no event reached Monorail for clean Ctrl+C exits. 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 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`. The test also covers the once-per-exit invariant and the double-emit guard inline.
1 parent 1b709f5 commit c6a114d

5 files changed

Lines changed: 177 additions & 14 deletions

File tree

.changeset/fluffy-mangos-brake.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'@shopify/theme': patch
3+
---
4+
5+
[internal] Fix `theme dev` analytics being dropped on Ctrl+C.

packages/theme/src/cli/commands/theme/dev.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -166,6 +166,7 @@ You can run this command only in a directory that matches the [default Shopify t
166166

167167
await dev({
168168
adminSession,
169+
commandConfig: this.config,
169170
directory: flags.path,
170171
store: flags.store,
171172
password: flags.password,

packages/theme/src/cli/services/dev.test.ts

Lines changed: 131 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,17 @@
1-
import {openURLSafely, renderLinks, createKeypressHandler} from './dev.js'
2-
import {describe, expect, test, vi, beforeEach, afterEach} from 'vitest'
1+
import {dev, openURLSafely, renderLinks, createKeypressHandler, reportDevAnalytics} from './dev.js'
2+
import {setupDevServer} from '../utilities/theme-environment/theme-environment.js'
3+
import {hasRequiredThemeDirectories} from '../utilities/theme-fs.js'
4+
import {isStorefrontPasswordProtected} from '../utilities/theme-environment/storefront-session.js'
5+
import {initializeDevServerSession} from '../utilities/theme-environment/dev-server-session.js'
6+
import {describe, expect, test, vi, beforeEach, afterEach, type MockInstance} from 'vitest'
37
import {buildTheme} from '@shopify/cli-kit/node/themes/factories'
48
import {DEVELOPMENT_THEME_ROLE} from '@shopify/cli-kit/node/themes/utils'
59
import {renderSuccess, renderWarning} from '@shopify/cli-kit/node/ui'
610
import {openURL} from '@shopify/cli-kit/node/system'
11+
import {reportAnalyticsEvent} from '@shopify/cli-kit/node/analytics'
12+
import {addPublicMetadata, addSensitiveMetadata} from '@shopify/cli-kit/node/metadata'
13+
import {getAvailableTCPPort, checkPortAvailability} from '@shopify/cli-kit/node/tcp'
14+
import {Config} from '@oclif/core'
715

816
vi.mock('@shopify/cli-kit/node/ui')
917
vi.mock('@shopify/cli-kit/node/colors', () => ({
@@ -16,6 +24,33 @@ vi.mock('@shopify/cli-kit/node/colors', () => ({
1624
vi.mock('@shopify/cli-kit/node/system', () => ({
1725
openURL: vi.fn(),
1826
}))
27+
vi.mock('@shopify/cli-kit/node/analytics', () => ({
28+
reportAnalyticsEvent: vi.fn(),
29+
}))
30+
vi.mock('@shopify/cli-kit/node/metadata', () => ({
31+
addPublicMetadata: vi.fn(),
32+
addSensitiveMetadata: vi.fn(),
33+
}))
34+
vi.mock('@shopify/cli-kit/node/tcp', () => ({
35+
getAvailableTCPPort: vi.fn(),
36+
checkPortAvailability: vi.fn(),
37+
}))
38+
vi.mock('../utilities/theme-environment/theme-environment.js', () => ({
39+
setupDevServer: vi.fn(),
40+
}))
41+
vi.mock('../utilities/theme-fs.js', () => ({
42+
hasRequiredThemeDirectories: vi.fn(),
43+
mountThemeFileSystem: vi.fn().mockReturnValue({}),
44+
}))
45+
vi.mock('../utilities/theme-fs-empty.js', () => ({
46+
emptyThemeExtFileSystem: vi.fn().mockReturnValue({}),
47+
}))
48+
vi.mock('../utilities/theme-environment/storefront-session.js', () => ({
49+
isStorefrontPasswordProtected: vi.fn(),
50+
}))
51+
vi.mock('../utilities/theme-environment/dev-server-session.js', () => ({
52+
initializeDevServerSession: vi.fn(),
53+
}))
1954

2055
const store = 'my-store.myshopify.com'
2156
const theme = buildTheme({id: 123, name: 'My Theme', role: DEVELOPMENT_THEME_ROLE})!
@@ -124,7 +159,7 @@ describe('createKeypressHandler', () => {
124159

125160
test('opens localhost when "t" is pressed', () => {
126161
// Given
127-
const handler = createKeypressHandler(urls, ctx)
162+
const handler = createKeypressHandler(urls, ctx, vi.fn())
128163

129164
// When
130165
handler('t', {name: 't'})
@@ -135,7 +170,7 @@ describe('createKeypressHandler', () => {
135170

136171
test('opens theme preview when "p" is pressed', () => {
137172
// Given
138-
const handler = createKeypressHandler(urls, ctx)
173+
const handler = createKeypressHandler(urls, ctx, vi.fn())
139174

140175
// When
141176
handler('p', {name: 'p'})
@@ -146,7 +181,7 @@ describe('createKeypressHandler', () => {
146181

147182
test('opens theme editor when "e" is pressed', () => {
148183
// Given
149-
const handler = createKeypressHandler(urls, ctx)
184+
const handler = createKeypressHandler(urls, ctx, vi.fn())
150185

151186
// When
152187
handler('e', {name: 'e'})
@@ -157,7 +192,7 @@ describe('createKeypressHandler', () => {
157192

158193
test('opens gift card preview when "g" is pressed', () => {
159194
// Given
160-
const handler = createKeypressHandler(urls, ctx)
195+
const handler = createKeypressHandler(urls, ctx, vi.fn())
161196

162197
// When
163198
handler('g', {name: 'g'})
@@ -169,7 +204,7 @@ describe('createKeypressHandler', () => {
169204
test('appends preview path to theme editor URL when lastRequestedPath is not "/"', () => {
170205
// Given
171206
const ctxWithPath = {lastRequestedPath: '/products/test-product'}
172-
const handler = createKeypressHandler(urls, ctxWithPath)
207+
const handler = createKeypressHandler(urls, ctxWithPath, vi.fn())
173208

174209
// When
175210
handler('e', {name: 'e'})
@@ -182,7 +217,7 @@ describe('createKeypressHandler', () => {
182217

183218
test('debounces rapid keypresses - only opens URL once during debounce window', () => {
184219
// Given
185-
const handler = createKeypressHandler(urls, ctx)
220+
const handler = createKeypressHandler(urls, ctx, vi.fn())
186221

187222
// When
188223
handler('t', {name: 't'})
@@ -197,7 +232,7 @@ describe('createKeypressHandler', () => {
197232

198233
test('allows keypresses after debounce period expires', () => {
199234
// Given
200-
const handler = createKeypressHandler(urls, ctx)
235+
const handler = createKeypressHandler(urls, ctx, vi.fn())
201236

202237
// When
203238
handler('t', {name: 't'})
@@ -220,7 +255,7 @@ describe('createKeypressHandler', () => {
220255

221256
test('debounces different keys during the same debounce window', () => {
222257
// Given
223-
const handler = createKeypressHandler(urls, ctx)
258+
const handler = createKeypressHandler(urls, ctx, vi.fn())
224259

225260
// When
226261
handler('t', {name: 't'})
@@ -232,4 +267,90 @@ describe('createKeypressHandler', () => {
232267
expect(openURL).toHaveBeenCalledTimes(1)
233268
expect(openURL).toHaveBeenCalledWith(urls.local)
234269
})
270+
271+
})
272+
273+
describe('dev() Ctrl-C analytics', () => {
274+
const mockConfig = {} as unknown as Config
275+
const adminSession = {storeFqdn: 'test.myshopify.com', token: 'x'}
276+
277+
let exitSpy: MockInstance
278+
let resolveBackgroundJob: () => void
279+
280+
const baseOptions = {
281+
adminSession,
282+
commandConfig: mockConfig,
283+
directory: '/tmp/theme',
284+
store: 'test.myshopify.com',
285+
open: false,
286+
theme,
287+
force: false,
288+
'theme-editor-sync': false,
289+
'live-reload': 'hot-reload' as const,
290+
'error-overlay': 'default' as const,
291+
noDelete: false,
292+
ignore: [],
293+
only: [],
294+
}
295+
296+
beforeEach(() => {
297+
vi.mocked(hasRequiredThemeDirectories).mockResolvedValue(true)
298+
vi.mocked(isStorefrontPasswordProtected).mockResolvedValue(false)
299+
vi.mocked(initializeDevServerSession).mockResolvedValue({
300+
storeFqdn: adminSession.storeFqdn,
301+
token: adminSession.token,
302+
} as any)
303+
vi.mocked(getAvailableTCPPort).mockResolvedValue(9292)
304+
vi.mocked(checkPortAvailability).mockResolvedValue(true)
305+
306+
const backgroundJobPromise = new Promise<void>((resolve) => {
307+
resolveBackgroundJob = resolve
308+
})
309+
vi.mocked(setupDevServer).mockReturnValue({
310+
serverStart: vi.fn().mockResolvedValue(undefined),
311+
renderDevSetupProgress: vi.fn().mockResolvedValue(undefined),
312+
backgroundJobPromise,
313+
resolveBackgroundJob: resolveBackgroundJob!,
314+
dispatchEvent: vi.fn(),
315+
} as any)
316+
317+
exitSpy = vi.spyOn(process, 'exit').mockImplementation(() => undefined as never)
318+
})
319+
320+
afterEach(() => {
321+
vi.clearAllMocks()
322+
exitSpy.mockRestore()
323+
})
324+
325+
test('Ctrl-C path: reports analytics once, even if reportDevAnalytics is invoked again', async () => {
326+
const devPromise = dev(baseOptions)
327+
328+
// Flush microtasks so the Promise.all is awaiting backgroundJobPromise.
329+
await new Promise((resolve) => setImmediate(resolve))
330+
331+
// Simulate Ctrl-C by resolving the background job.
332+
resolveBackgroundJob()
333+
await devPromise
334+
335+
expect(reportAnalyticsEvent).toHaveBeenCalledTimes(1)
336+
expect(reportAnalyticsEvent).toHaveBeenCalledWith({config: mockConfig, exitMode: 'ok'})
337+
338+
expect(addPublicMetadata).toHaveBeenCalledTimes(1)
339+
const publicMetadataFn = vi.mocked(addPublicMetadata).mock.calls[0]![0] as () => Record<string, unknown>
340+
expect(publicMetadataFn()).toEqual({store_fqdn_hash: expect.any(String)})
341+
342+
expect(addSensitiveMetadata).toHaveBeenCalledTimes(1)
343+
const sensitiveMetadataFn = vi.mocked(addSensitiveMetadata).mock.calls[0]![0] as () => Record<string, unknown>
344+
expect(sensitiveMetadataFn()).toEqual({store_fqdn: adminSession.storeFqdn})
345+
346+
expect(exitSpy).toHaveBeenCalledWith(0)
347+
348+
const reportOrder = vi.mocked(reportAnalyticsEvent).mock.invocationCallOrder[0]!
349+
const exitOrder = exitSpy.mock.invocationCallOrder[0]!
350+
expect(reportOrder).toBeLessThan(exitOrder)
351+
352+
await reportDevAnalytics(mockConfig, adminSession as any)
353+
354+
expect(reportAnalyticsEvent).toHaveBeenCalledTimes(1)
355+
})
235356
})

packages/theme/src/cli/services/dev.ts

Lines changed: 34 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -14,15 +14,22 @@ import {checkPortAvailability, getAvailableTCPPort} from '@shopify/cli-kit/node/
1414
import {AbortError} from '@shopify/cli-kit/node/error'
1515
import {openURL} from '@shopify/cli-kit/node/system'
1616
import {debounce} from '@shopify/cli-kit/common/function'
17+
import {reportAnalyticsEvent} from '@shopify/cli-kit/node/analytics'
18+
import {addPublicMetadata, addSensitiveMetadata} from '@shopify/cli-kit/node/metadata'
19+
import {hashString} from '@shopify/cli-kit/node/crypto'
1720
import chalk from '@shopify/cli-kit/node/colors'
21+
import {Config} from '@oclif/core'
1822

1923
import readline from 'readline'
2024

2125
const DEFAULT_HOST = '127.0.0.1'
2226
const DEFAULT_PORT = '9292'
2327

28+
let hasReportedAnalyticsEvent = false
29+
2430
interface DevOptions {
2531
adminSession: AdminSession
32+
commandConfig: Config
2633
directory: string
2734
store: string
2835
password?: string
@@ -128,11 +135,14 @@ export async function dev(options: DevOptions) {
128135
},
129136
}
130137

131-
const {serverStart, renderDevSetupProgress, backgroundJobPromise} = setupDevServer(options.theme, ctx)
138+
const {serverStart, renderDevSetupProgress, backgroundJobPromise, resolveBackgroundJob} = setupDevServer(
139+
options.theme,
140+
ctx,
141+
)
132142

133143
readline.emitKeypressEvents(process.stdin)
134144

135-
const keypressHandler = createKeypressHandler(urls, ctx)
145+
const keypressHandler = createKeypressHandler(urls, ctx, resolveBackgroundJob)
136146
process.stdin.on('keypress', keypressHandler)
137147

138148
await Promise.all([
@@ -149,17 +159,37 @@ export async function dev(options: DevOptions) {
149159
}
150160
}),
151161
])
162+
163+
await reportDevAnalytics(options.commandConfig, options.adminSession)
164+
165+
process.exit(0)
166+
}
167+
168+
export async function reportDevAnalytics(config: Config, session: AdminSession): Promise<void> {
169+
if (hasReportedAnalyticsEvent) return
170+
hasReportedAnalyticsEvent = true
171+
172+
try {
173+
await addPublicMetadata(() => ({store_fqdn_hash: hashString(session.storeFqdn)}))
174+
await addSensitiveMetadata(() => ({store_fqdn: session.storeFqdn}))
175+
await reportAnalyticsEvent({config, exitMode: 'ok'})
176+
// eslint-disable-next-line no-catch-all/no-catch-all
177+
} catch (_error) {
178+
// Analytics must never block exit.
179+
}
152180
}
153181

154182
export function createKeypressHandler(
155183
urls: {local: string; giftCard: string; themeEditor: string; preview: string},
156184
ctx: {lastRequestedPath: string},
185+
onClose: () => void,
157186
) {
158187
const debouncedOpenURL = debounce(openURLSafely, 100, {leading: true, trailing: false})
159188

160189
return (_str: string, key: {ctrl?: boolean; name?: string}) => {
161190
if (key.ctrl && key.name === 'c') {
162-
process.exit()
191+
onClose()
192+
return
163193
}
164194

165195
switch (key.name) {
@@ -182,6 +212,7 @@ export function createKeypressHandler(
182212
case 'g':
183213
debouncedOpenURL(urls.giftCard, 'gift card preview')
184214
break
215+
case undefined:
185216
default:
186217
break
187218
}

packages/theme/src/cli/utilities/theme-environment/theme-environment.ts

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,11 @@ import type {Checksum, Theme} from '@shopify/cli-kit/node/themes/types'
1818
import type {DevServerContext} from './types.js'
1919

2020
export function setupDevServer(theme: Theme, ctx: DevServerContext) {
21-
const {promise: backgroundJobPromise, reject: rejectBackgroundJob} = promiseWithResolvers<never>()
21+
const {
22+
promise: backgroundJobPromise,
23+
resolve: resolveBackgroundJob,
24+
reject: rejectBackgroundJob,
25+
} = promiseWithResolvers<void>()
2226

2327
const watcherPromise = setupInMemoryTemplateWatcher(theme, ctx)
2428
const envSetup = ensureThemeEnvironmentSetup(theme, ctx, rejectBackgroundJob)
@@ -33,6 +37,7 @@ export function setupDevServer(theme: Theme, ctx: DevServerContext) {
3337
dispatchEvent: server.dispatch,
3438
renderDevSetupProgress: envSetup.renderProgress,
3539
backgroundJobPromise,
40+
resolveBackgroundJob,
3641
}
3742
}
3843

0 commit comments

Comments
 (0)