From 2a3fcea2f6cfc27285a99d57c1969eadce539806 Mon Sep 17 00:00:00 2001 From: Gray Gilmore Date: Mon, 16 Mar 2026 15:47:30 -0700 Subject: [PATCH 1/2] fix: use chokidar raw events as fallback for suppressed file changes chokidar's _handleFile listener compares mtimeMs and suppresses change events when mtime hasn't changed. Build tools that preserve timestamps (e.g. via touch --reference) cause silent misses on Linux/Windows. chokidar emits a raw event unconditionally, even when the mtime check suppresses the change event. This adds a raw event listener as a fallback: when a raw event fires, we start a short 100ms timeout. If chokidar's normal change event fires within that window, we cancel the timeout (normal path handled it). If the timeout expires, we trigger the handler ourselves. The existing handleFileUpdate already computes MD5 checksums and only syncs when content actually changes, so even if both paths fire, the checksum comparison prevents redundant uploads. Co-Authored-By: Claude Opus 4.6 --- .../theme/src/cli/utilities/theme-fs.test.ts | 87 ++++++++++++++++++- packages/theme/src/cli/utilities/theme-fs.ts | 52 ++++++++++- 2 files changed, 137 insertions(+), 2 deletions(-) diff --git a/packages/theme/src/cli/utilities/theme-fs.test.ts b/packages/theme/src/cli/utilities/theme-fs.test.ts index ac981cbf5ce..265170f8eed 100644 --- a/packages/theme/src/cli/utilities/theme-fs.test.ts +++ b/packages/theme/src/cli/utilities/theme-fs.test.ts @@ -12,7 +12,7 @@ import {getPatternsFromShopifyIgnore, applyIgnoreFilters} from './asset-ignore.j import {triggerBrowserFullReload} from './theme-environment/hot-reload/server.js' import {removeFile, writeFile} from '@shopify/cli-kit/node/fs' import * as fsKit from '@shopify/cli-kit/node/fs' -import {test, describe, expect, vi, beforeEach} from 'vitest' +import {test, describe, expect, vi, beforeEach, afterEach} from 'vitest' import chokidar from 'chokidar' import {bulkUploadThemeAssets, deleteThemeAssets, fetchThemeAssets} from '@shopify/cli-kit/node/themes/api' import {renderError} from '@shopify/cli-kit/node/ui' @@ -949,6 +949,91 @@ describe('theme-fs', () => { }) }) + describe('raw event fallback for suppressed changes', () => { + const themeId = '123' + const adminSession = {token: 'token'} as AdminSession + const root = joinPath(locationOfThisFile, 'fixtures/theme') + + beforeEach(() => { + const mockWatcher = new EventEmitter() + vi.spyOn(chokidar, 'watch').mockImplementation((_) => { + return mockWatcher as any + }) + }) + + afterEach(() => { + vi.useRealTimers() + }) + + test('raw event triggers change handler when chokidar suppresses change', async () => { + const themeFileSystem = mountThemeFileSystem(root) + await themeFileSystem.ready() + await themeFileSystem.startWatcher(themeId, adminSession) + + // Enable fake timers after setup so dynamic imports and I/O complete normally + vi.useFakeTimers() + + const watcher = chokidar.watch('') as EventEmitter + const filePath = joinPath(root, 'assets', 'base.css') + + watcher.emit('raw', 'change', 'base.css', {watchedPath: filePath}) + + // Advance past RAW_CHANGE_DEBOUNCE_IN_MS (100ms) + FILE_EVENT_DEBOUNCE_TIME_IN_MS (250ms) + await vi.advanceTimersByTimeAsync(400) + + expect(themeFileSystem.files.get('assets/base.css')).toBeDefined() + }) + + test('raw event is cancelled when chokidar change event fires', async () => { + const themeFileSystem = mountThemeFileSystem(root) + await themeFileSystem.ready() + await themeFileSystem.startWatcher(themeId, adminSession) + + // Enable fake timers after setup so dynamic imports and I/O complete normally + vi.useFakeTimers() + + const watcher = chokidar.watch('') as EventEmitter + const filePath = joinPath(root, 'assets', 'base.css') + + const initialChecksum = themeFileSystem.files.get('assets/base.css')?.checksum + + // Emit raw first, then the normal change event before debounce expires + watcher.emit('raw', 'change', 'base.css', {watchedPath: filePath}) + watcher.emit('change', filePath) + + // Advance past all debounce windows + await vi.advanceTimersByTimeAsync(400) + + // The file should have been processed exactly once (via the change event path) + // Verify by checking the file was read and checksum updated + const asset = themeFileSystem.files.get('assets/base.css') + expect(asset).toBeDefined() + expect(asset!.checksum).toBe(initialChecksum) + }) + + test('duplicate raw events for the same path are deduplicated', async () => { + const themeFileSystem = mountThemeFileSystem(root) + await themeFileSystem.ready() + await themeFileSystem.startWatcher(themeId, adminSession) + + // Enable fake timers after setup so dynamic imports and I/O complete normally + vi.useFakeTimers() + + const watcher = chokidar.watch('') as EventEmitter + const filePath = joinPath(root, 'assets', 'base.css') + + // Simulate two raw events (file watcher + directory watcher) + watcher.emit('raw', 'change', 'base.css', {watchedPath: filePath}) + watcher.emit('raw', 'change', 'base.css', {watchedPath: joinPath(root, 'assets')}) + + // Advance past all debounce windows + await vi.advanceTimersByTimeAsync(400) + + // The file should still be in the filesystem (processed successfully) + expect(themeFileSystem.files.get('assets/base.css')).toBeDefined() + }) + }) + function fsEntry({key, checksum}: Checksum): [string, ThemeAsset] { return [ key, diff --git a/packages/theme/src/cli/utilities/theme-fs.ts b/packages/theme/src/cli/utilities/theme-fs.ts index 3f4781f4198..92120063ce3 100644 --- a/packages/theme/src/cli/utilities/theme-fs.ts +++ b/packages/theme/src/cli/utilities/theme-fs.ts @@ -26,6 +26,7 @@ import type { } from '@shopify/cli-kit/node/themes/types' const FILE_EVENT_DEBOUNCE_TIME_IN_MS = 250 +const RAW_CHANGE_DEBOUNCE_IN_MS = 100 const THEME_DIRECTORY_PATTERNS = [ 'assets/**/*.*', @@ -343,14 +344,63 @@ export function mountThemeFileSystem(root: string, options?: ThemeFileSystemOpti pendingEvents.set(eventKey, timeout) } + // Track pending raw-event timeouts so the normal 'change' path can cancel them + const pendingRawChanges = new Map>() + watcher .on('add', queueFsEvent.bind(null, 'add')) - .on('change', queueFsEvent.bind(null, 'change')) + .on('change', (filePath: string) => { + const pending = pendingRawChanges.get(filePath) + if (pending !== undefined) { + clearTimeout(pending) + pendingRawChanges.delete(filePath) + } + queueFsEvent('change', filePath) + }) .on('unlink', queueFsEvent.bind(null, 'unlink')) + .on('raw', (rawEvent: string, evPath: string, details: {watchedPath?: string}) => { + if (rawEvent !== 'change' && rawEvent !== 'modified') return + if (!evPath) return + + const fullPath = resolveRawChangePath(rawEvent, evPath, details) + if (!fullPath) return + + const existingTimeout = pendingRawChanges.get(fullPath) + if (existingTimeout !== undefined) { + clearTimeout(existingTimeout) + } + + pendingRawChanges.set( + fullPath, + setTimeout(() => { + pendingRawChanges.delete(fullPath) + queueFsEvent('change', fullPath) + }, RAW_CHANGE_DEBOUNCE_IN_MS), + ) + }) }, } } +function resolveRawChangePath( + rawEvent: string, + evPath: string, + details: {watchedPath?: string}, +): string | undefined { + // FSEvents (macOS): evPath is already the full absolute path + if (rawEvent === 'modified') return evPath + + // fs.watch (Linux/Windows): resolve from watchedPath + evPath + const watchedPath = details?.watchedPath + if (!watchedPath) return undefined + + // File watcher: watchedPath is the full file path, evPath is the basename + if (watchedPath.endsWith(evPath)) return watchedPath + + // Directory watcher: watchedPath is the directory, evPath is the filename + return joinPath(watchedPath, evPath) +} + export function handleSyncUpdate( unsyncedFileKeys: Set, uploadErrors: Map, From 9034f513e7108c4b8aab4b354a2443a3bdb4642b Mon Sep 17 00:00:00 2001 From: Gray Gilmore Date: Wed, 18 Mar 2026 14:30:24 -0700 Subject: [PATCH 2/2] fix: only remove successfully-uploaded files from unsyncedFileKeys Failed uploads (e.g. THROTTLED) were unconditionally removed from unsyncedFileKeys in the .then() callback of uploadBatch, regardless of whether the upload actually succeeded. This allowed the polling loop in theme-polling.ts to see these unprotected files, detect a checksum mismatch, and overwrite local files with remote versions -- defeating the 'keep local' reconciliation choice. The fix checks uploadResults.get(file.key)?.success === true before removing from unsyncedFileKeys and incrementing the progress counter. Files that fail all retries remain in unsyncedFileKeys, preventing the polling loop from overwriting them. Co-Authored-By: Claude Opus 4.6 --- .../src/cli/utilities/theme-uploader.test.ts | 104 ++++++++++++++++++ .../theme/src/cli/utilities/theme-uploader.ts | 8 +- 2 files changed, 110 insertions(+), 2 deletions(-) diff --git a/packages/theme/src/cli/utilities/theme-uploader.test.ts b/packages/theme/src/cli/utilities/theme-uploader.test.ts index 41f92a35254..ef6b3769729 100644 --- a/packages/theme/src/cli/utilities/theme-uploader.test.ts +++ b/packages/theme/src/cli/utilities/theme-uploader.test.ts @@ -560,6 +560,110 @@ describe('theme-uploader', () => { ) }) + test('should not remove failed uploads from unsyncedFileKeys', async () => { + // Given + const remoteChecksums = [ + ...MINIMUM_THEME_ASSETS.map((asset) => ({key: asset.key, checksum: '0'})), + {key: 'assets/success.liquid', checksum: 'remote-checksum-a'}, + {key: 'assets/failure.liquid', checksum: 'remote-checksum-b'}, + ] + const themeFileSystem = fakeThemeFileSystem( + 'tmp', + new Map([ + ['assets/success.liquid', {checksum: 'local-checksum-a', key: 'assets/success.liquid'}], + ['assets/failure.liquid', {checksum: 'local-checksum-b', key: 'assets/failure.liquid'}], + ]), + ) + + vi.mocked(bulkUploadThemeAssets) + .mockResolvedValueOnce([ + { + key: 'assets/success.liquid', + success: true, + errors: {}, + operation: Operation.Upload, + asset: {key: 'assets/success.liquid', checksum: 'local-checksum-a'}, + }, + { + key: 'assets/failure.liquid', + success: false, + errors: {asset: ['THROTTLED']}, + operation: Operation.Upload, + asset: {key: 'assets/failure.liquid', checksum: 'local-checksum-b'}, + }, + ]) + .mockResolvedValue([ + { + key: 'assets/failure.liquid', + success: false, + errors: {asset: ['THROTTLED']}, + operation: Operation.Upload, + asset: {key: 'assets/failure.liquid', checksum: 'local-checksum-b'}, + }, + ]) + + // When + const {renderThemeSyncProgress} = uploadTheme( + remoteTheme, + adminSession, + remoteChecksums, + themeFileSystem, + uploadOptions, + ) + await renderThemeSyncProgress() + + // Then + expect(themeFileSystem.unsyncedFileKeys.has('assets/failure.liquid')).toBe(true) + expect(themeFileSystem.unsyncedFileKeys.has('assets/success.liquid')).toBe(false) + }) + + test('should remove file from unsyncedFileKeys when retry succeeds', async () => { + // Given + const remoteChecksums = [ + ...MINIMUM_THEME_ASSETS.map((asset) => ({key: asset.key, checksum: '0'})), + {key: 'assets/flaky.liquid', checksum: 'remote-checksum'}, + ] + const themeFileSystem = fakeThemeFileSystem( + 'tmp', + new Map([['assets/flaky.liquid', {checksum: 'local-checksum', key: 'assets/flaky.liquid'}]]), + ) + + vi.mocked(bulkUploadThemeAssets) + // First attempt: failure + .mockResolvedValueOnce([ + { + key: 'assets/flaky.liquid', + success: false, + errors: {asset: ['THROTTLED']}, + operation: Operation.Upload, + asset: {key: 'assets/flaky.liquid', checksum: 'local-checksum'}, + }, + ]) + // Retry: success + .mockResolvedValueOnce([ + { + key: 'assets/flaky.liquid', + success: true, + errors: {}, + operation: Operation.Upload, + asset: {key: 'assets/flaky.liquid', checksum: 'local-checksum'}, + }, + ]) + + // When + const {renderThemeSyncProgress} = uploadTheme( + remoteTheme, + adminSession, + remoteChecksums, + themeFileSystem, + uploadOptions, + ) + await renderThemeSyncProgress() + + // Then + expect(themeFileSystem.unsyncedFileKeys.has('assets/flaky.liquid')).toBe(false) + }) + test('should not delete or upload files specified by the --ignore flag', async () => { // Given const remote = [ diff --git a/packages/theme/src/cli/utilities/theme-uploader.ts b/packages/theme/src/cli/utilities/theme-uploader.ts index 2b56cb04357..8f60017e118 100644 --- a/packages/theme/src/cli/utilities/theme-uploader.ts +++ b/packages/theme/src/cli/utilities/theme-uploader.ts @@ -270,8 +270,12 @@ function buildUploadJob( return Promise.all( createBatches(fileType).map((batch) => uploadBatch(batch, themeFileSystem, session, theme.id, uploadResults).then(() => { - progress.current += batch.length - batch.forEach((file) => themeFileSystem.unsyncedFileKeys.delete(file.key)) + batch.forEach((file) => { + if (uploadResults.get(file.key)?.success === true) { + progress.current += 1 + themeFileSystem.unsyncedFileKeys.delete(file.key) + } + }) }), ), ).then(() => {})