diff --git a/packages/theme/src/cli/utilities/theme-fs.test.ts b/packages/theme/src/cli/utilities/theme-fs.test.ts index ac981cbf5c..265170f8ee 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 3f4781f419..92120063ce 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, diff --git a/packages/theme/src/cli/utilities/theme-uploader.test.ts b/packages/theme/src/cli/utilities/theme-uploader.test.ts index 41f92a3525..ef6b376972 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 2b56cb0435..8f60017e11 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(() => {})