Skip to content

Commit 0704dc7

Browse files
committed
fix(theme): restore theme dev analytics on Ctrl+C exit
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.
1 parent 2f9bc85 commit 0704dc7

5 files changed

Lines changed: 196 additions & 14 deletions

File tree

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+
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.

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: 149 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,36 @@ 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/storefront-password-prompt.js', () => ({
52+
ensureValidPassword: vi.fn(),
53+
}))
54+
vi.mock('../utilities/theme-environment/dev-server-session.js', () => ({
55+
initializeDevServerSession: vi.fn(),
56+
}))
1957

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

125163
test('opens localhost when "t" is pressed', () => {
126164
// Given
127-
const handler = createKeypressHandler(urls, ctx)
165+
const handler = createKeypressHandler(urls, ctx, vi.fn())
128166

129167
// When
130168
handler('t', {name: 't'})
@@ -135,7 +173,7 @@ describe('createKeypressHandler', () => {
135173

136174
test('opens theme preview when "p" is pressed', () => {
137175
// Given
138-
const handler = createKeypressHandler(urls, ctx)
176+
const handler = createKeypressHandler(urls, ctx, vi.fn())
139177

140178
// When
141179
handler('p', {name: 'p'})
@@ -146,7 +184,7 @@ describe('createKeypressHandler', () => {
146184

147185
test('opens theme editor when "e" is pressed', () => {
148186
// Given
149-
const handler = createKeypressHandler(urls, ctx)
187+
const handler = createKeypressHandler(urls, ctx, vi.fn())
150188

151189
// When
152190
handler('e', {name: 'e'})
@@ -157,7 +195,7 @@ describe('createKeypressHandler', () => {
157195

158196
test('opens gift card preview when "g" is pressed', () => {
159197
// Given
160-
const handler = createKeypressHandler(urls, ctx)
198+
const handler = createKeypressHandler(urls, ctx, vi.fn())
161199

162200
// When
163201
handler('g', {name: 'g'})
@@ -169,7 +207,7 @@ describe('createKeypressHandler', () => {
169207
test('appends preview path to theme editor URL when lastRequestedPath is not "/"', () => {
170208
// Given
171209
const ctxWithPath = {lastRequestedPath: '/products/test-product'}
172-
const handler = createKeypressHandler(urls, ctxWithPath)
210+
const handler = createKeypressHandler(urls, ctxWithPath, vi.fn())
173211

174212
// When
175213
handler('e', {name: 'e'})
@@ -182,7 +220,7 @@ describe('createKeypressHandler', () => {
182220

183221
test('debounces rapid keypresses - only opens URL once during debounce window', () => {
184222
// Given
185-
const handler = createKeypressHandler(urls, ctx)
223+
const handler = createKeypressHandler(urls, ctx, vi.fn())
186224

187225
// When
188226
handler('t', {name: 't'})
@@ -197,7 +235,7 @@ describe('createKeypressHandler', () => {
197235

198236
test('allows keypresses after debounce period expires', () => {
199237
// Given
200-
const handler = createKeypressHandler(urls, ctx)
238+
const handler = createKeypressHandler(urls, ctx, vi.fn())
201239

202240
// When
203241
handler('t', {name: 't'})
@@ -220,7 +258,7 @@ describe('createKeypressHandler', () => {
220258

221259
test('debounces different keys during the same debounce window', () => {
222260
// Given
223-
const handler = createKeypressHandler(urls, ctx)
261+
const handler = createKeypressHandler(urls, ctx, vi.fn())
224262

225263
// When
226264
handler('t', {name: 't'})
@@ -232,4 +270,105 @@ describe('createKeypressHandler', () => {
232270
expect(openURL).toHaveBeenCalledTimes(1)
233271
expect(openURL).toHaveBeenCalledWith(urls.local)
234272
})
273+
274+
})
275+
276+
describe('dev() Ctrl-C analytics', () => {
277+
const mockConfig = {} as unknown as Config
278+
const adminSession = {storeFqdn: 'test.myshopify.com', token: 'x'}
279+
280+
let exitSpy: MockInstance
281+
let resolveBackgroundJob: () => void
282+
283+
const baseOptions = {
284+
adminSession,
285+
commandConfig: mockConfig,
286+
directory: '/tmp/theme',
287+
store: 'test.myshopify.com',
288+
open: false,
289+
theme,
290+
force: false,
291+
'theme-editor-sync': false,
292+
'live-reload': 'hot-reload' as const,
293+
'error-overlay': 'default' as const,
294+
noDelete: false,
295+
ignore: [],
296+
only: [],
297+
}
298+
299+
beforeEach(() => {
300+
vi.mocked(hasRequiredThemeDirectories).mockResolvedValue(true)
301+
vi.mocked(isStorefrontPasswordProtected).mockResolvedValue(false)
302+
vi.mocked(initializeDevServerSession).mockResolvedValue({
303+
storeFqdn: adminSession.storeFqdn,
304+
token: adminSession.token,
305+
} as any)
306+
vi.mocked(getAvailableTCPPort).mockResolvedValue(9292)
307+
vi.mocked(checkPortAvailability).mockResolvedValue(true)
308+
309+
const backgroundJobPromise = new Promise<void>((resolve) => {
310+
resolveBackgroundJob = resolve
311+
})
312+
vi.mocked(setupDevServer).mockReturnValue({
313+
serverStart: vi.fn().mockResolvedValue(undefined),
314+
renderDevSetupProgress: vi.fn().mockResolvedValue(undefined),
315+
backgroundJobPromise,
316+
resolveBackgroundJob: resolveBackgroundJob!,
317+
dispatchEvent: vi.fn(),
318+
} as any)
319+
320+
exitSpy = vi.spyOn(process, 'exit').mockImplementation(() => undefined as never)
321+
})
322+
323+
afterEach(() => {
324+
vi.clearAllMocks()
325+
exitSpy.mockRestore()
326+
})
327+
328+
test('reports analytics event exactly once with exitMode=ok and store_fqdn_hash metadata on Ctrl-C', async () => {
329+
// Given / When
330+
const devPromise = dev(baseOptions)
331+
332+
// Flush microtasks so the Promise.all is awaiting backgroundJobPromise.
333+
await new Promise((resolve) => setImmediate(resolve))
334+
335+
// Simulate Ctrl-C by resolving the background job.
336+
resolveBackgroundJob()
337+
await devPromise
338+
339+
// Then
340+
expect(reportAnalyticsEvent).toHaveBeenCalledTimes(1)
341+
expect(reportAnalyticsEvent).toHaveBeenCalledWith({config: mockConfig, exitMode: 'ok'})
342+
343+
expect(addPublicMetadata).toHaveBeenCalledTimes(1)
344+
const publicMetadataFn = vi.mocked(addPublicMetadata).mock.calls[0]![0] as () => Record<string, unknown>
345+
expect(publicMetadataFn()).toEqual({store_fqdn_hash: expect.any(String)})
346+
347+
expect(addSensitiveMetadata).toHaveBeenCalledTimes(1)
348+
const sensitiveMetadataFn = vi.mocked(addSensitiveMetadata).mock.calls[0]![0] as () => Record<string, unknown>
349+
expect(sensitiveMetadataFn()).toEqual({store_fqdn: adminSession.storeFqdn})
350+
351+
expect(exitSpy).toHaveBeenCalledWith(0)
352+
353+
// Order: analytics before exit.
354+
const reportOrder = vi.mocked(reportAnalyticsEvent).mock.invocationCallOrder[0]!
355+
const exitOrder = exitSpy.mock.invocationCallOrder[0]!
356+
expect(reportOrder).toBeLessThan(exitOrder)
357+
})
358+
359+
test('double-emit guard: reportDevAnalytics called twice only emits once', async () => {
360+
// Given: run dev() once to set the flag
361+
const devPromise = dev(baseOptions)
362+
await new Promise((resolve) => setImmediate(resolve))
363+
resolveBackgroundJob()
364+
await devPromise
365+
366+
expect(reportAnalyticsEvent).toHaveBeenCalledTimes(1)
367+
368+
// When: re-invoke reportDevAnalytics directly (simulates a second exit path)
369+
await reportDevAnalytics(mockConfig, adminSession as any)
370+
371+
// Then: still only one call
372+
expect(reportAnalyticsEvent).toHaveBeenCalledTimes(1)
373+
})
235374
})

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

Lines changed: 35 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
@@ -69,6 +76,8 @@ export async function dev(options: DevOptions) {
6976
})
7077
}
7178

79+
hasReportedAnalyticsEvent = false
80+
7281
if (options.listing) {
7382
await ensureListingExists(options.directory, options.listing)
7483
}
@@ -128,11 +137,14 @@ export async function dev(options: DevOptions) {
128137
},
129138
}
130139

131-
const {serverStart, renderDevSetupProgress, backgroundJobPromise} = setupDevServer(options.theme, ctx)
140+
const {serverStart, renderDevSetupProgress, backgroundJobPromise, resolveBackgroundJob} = setupDevServer(
141+
options.theme,
142+
ctx,
143+
)
132144

133145
readline.emitKeypressEvents(process.stdin)
134146

135-
const keypressHandler = createKeypressHandler(urls, ctx)
147+
const keypressHandler = createKeypressHandler(urls, ctx, resolveBackgroundJob)
136148
process.stdin.on('keypress', keypressHandler)
137149

138150
await Promise.all([
@@ -149,17 +161,36 @@ export async function dev(options: DevOptions) {
149161
}
150162
}),
151163
])
164+
165+
await reportDevAnalytics(options.commandConfig, options.adminSession)
166+
167+
process.exit(0)
168+
}
169+
170+
export async function reportDevAnalytics(config: Config, session: AdminSession): Promise<void> {
171+
if (hasReportedAnalyticsEvent) return
172+
hasReportedAnalyticsEvent = true
173+
174+
try {
175+
await addPublicMetadata(() => ({store_fqdn_hash: hashString(session.storeFqdn)}))
176+
await addSensitiveMetadata(() => ({store_fqdn: session.storeFqdn}))
177+
await reportAnalyticsEvent({config, exitMode: 'ok'})
178+
} catch (_error) {
179+
// Analytics must never block exit.
180+
}
152181
}
153182

154183
export function createKeypressHandler(
155184
urls: {local: string; giftCard: string; themeEditor: string; preview: string},
156185
ctx: {lastRequestedPath: string},
186+
onClose: () => void,
157187
) {
158188
const debouncedOpenURL = debounce(openURLSafely, 100, {leading: true, trailing: false})
159189

160190
return (_str: string, key: {ctrl?: boolean; name?: string}) => {
161191
if (key.ctrl && key.name === 'c') {
162-
process.exit()
192+
onClose()
193+
return
163194
}
164195

165196
switch (key.name) {
@@ -180,6 +211,7 @@ export function createKeypressHandler(
180211
case 'g':
181212
debouncedOpenURL(urls.giftCard, 'gift card preview')
182213
break
214+
case undefined:
183215
default:
184216
break
185217
}

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)