Confirm before theme pull overwrites or deletes local files

.changeset/theme-pull-skip-git-check.md

@@ -0,0 +1,5 @@
+---
+'@shopify/theme': minor
+---
+
+`shopify theme pull` now lists the local files it would overwrite or delete and asks for confirmation before changing them, replacing the check for uncommitted Git changes. Use `--force` to skip the confirmation.

packages/cli/oclif.manifest.json

@@ -12288,7 +12288,7 @@
         "force": {
           "allowNo": false,
           "char": "f",
-          "description": "Proceed without confirmation, if current directory does not seem to be theme directory.",
+          "description": "Proceed without confirmation, if current directory does not seem to be theme directory or local files would be overwritten.",
           "env": "SHOPIFY_FLAG_FORCE",
           "hidden": true,
           "name": "force",

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

@@ -50,7 +50,8 @@ If no theme is specified, then you're prompted to select the theme to pull from
     force: Flags.boolean({
       hidden: true,
       char: 'f',
-      description: 'Proceed without confirmation, if current directory does not seem to be theme directory.',
+      description:
+        'Proceed without confirmation, if current directory does not seem to be theme directory or local files would be overwritten.',
       env: 'SHOPIFY_FLAG_FORCE',
     }),
   }

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

@@ -5,14 +5,13 @@ import {ensureThemeStore} from '../utilities/theme-store.js'
 import {DevelopmentThemeManager} from '../utilities/development-theme-manager.js'
 import {hasRequiredThemeDirectories, mountThemeFileSystem} from '../utilities/theme-fs.js'
 import {fakeThemeFileSystem} from '../utilities/theme-fs/theme-fs-mock-factory.js'
-import {downloadTheme} from '../utilities/theme-downloader.js'
-import {themeComponent, ensureDirectoryConfirmed} from '../utilities/theme-ui.js'
+import {downloadTheme, localFileChanges} from '../utilities/theme-downloader.js'
+import {themeComponent, ensureDirectoryConfirmed, ensureLocalFileChangesConfirmed} from '../utilities/theme-ui.js'
 
 import {mkTmpDir, rmdir} from '@shopify/cli-kit/node/fs'
 import {buildTheme} from '@shopify/cli-kit/node/themes/factories'
 import {ensureAuthenticatedThemes} from '@shopify/cli-kit/node/session'
 import {fetchChecksums} from '@shopify/cli-kit/node/themes/api'
-import {insideGitDirectory, isClean} from '@shopify/cli-kit/node/git'
 import {test, describe, expect, vi, beforeEach} from 'vitest'
 import {dirname, joinPath} from '@shopify/cli-kit/node/path'
 import {renderSuccess} from '@shopify/cli-kit/node/ui'
@@ -28,7 +27,6 @@ vi.mock('@shopify/cli-kit/node/context/local')
 vi.mock('@shopify/cli-kit/node/session')
 vi.mock('@shopify/cli-kit/node/themes/api')
 vi.mock('@shopify/cli-kit/node/ui')
-vi.mock('@shopify/cli-kit/node/git')
 
 const adminSession = {token: '', storeFqdn: ''}
 const path = '/my-theme'
@@ -57,6 +55,7 @@ describe('pull', () => {
     vi.mocked(mountThemeFileSystem).mockReturnValue(localThemeFileSystem)
     vi.mocked(fetchChecksums).mockResolvedValue([])
     vi.mocked(themeComponent).mockReturnValue([])
+    vi.mocked(ensureLocalFileChangesConfirmed).mockResolvedValue(true)
     findDevelopmentThemeSpy.mockClear()
     fetchDevelopmentThemeSpy.mockClear()
   })
@@ -99,40 +98,49 @@ describe('pull', () => {
     )
   })
 
-  test('should ask for confirmation if the current directory is a Git directory and is not clean', async () => {
+  test('should not ask for confirmation if --force flag is provided', async () => {
+    // Given
+    const theme = buildTheme({id: 1, name: 'Theme', role: 'development'})!
+    vi.mocked(findOrSelectTheme).mockResolvedValue(theme)
+    vi.mocked(fetchDevelopmentThemeSpy).mockResolvedValue(undefined)
+
+    // When
+    await pull({...defaultFlags, theme: theme.id.toString(), force: true})
+
+    // Then
+    expect(vi.mocked(findOrSelectTheme)).toHaveBeenCalledOnce()
+    expect(vi.mocked(hasRequiredThemeDirectories)).not.toHaveBeenCalled()
+    expect(vi.mocked(ensureDirectoryConfirmed)).not.toHaveBeenCalled()
+  })
+
+  test('should ask for confirmation before changing local files', async () => {
     // Given
     const theme = buildTheme({id: 1, name: 'Theme', role: 'development'})!
-    vi.mocked(insideGitDirectory).mockResolvedValue(true)
-    vi.mocked(isClean).mockResolvedValue(false)
-    vi.mocked(ensureDirectoryConfirmed).mockResolvedValue(false)
+    const changes = {overwritten: ['templates/index.json'], deleted: ['assets/old.css']}
+    vi.mocked(findOrSelectTheme).mockResolvedValue(theme)
+    vi.mocked(localFileChanges).mockReturnValue(changes)
 
     // When
     await pull({...defaultFlags, theme: theme.id.toString()})
 
     // Then
-    expect(vi.mocked(ensureDirectoryConfirmed)).toHaveBeenCalledWith(
-      false,
-      'The current Git directory has uncommitted changes.',
-      undefined,
-      undefined,
-    )
+    expect(localFileChanges).toHaveBeenCalledWith([], localThemeFileSystem, expect.objectContaining({nodelete: false}))
+    expect(ensureLocalFileChangesConfirmed).toHaveBeenCalledWith(changes, false, undefined, undefined)
+    expect(downloadTheme).toHaveBeenCalled()
   })
 
-  test('should not ask for confirmation if --force flag is provided', async () => {
+  test('should not change local files when the confirmation is declined', async () => {
     // Given
     const theme = buildTheme({id: 1, name: 'Theme', role: 'development'})!
     vi.mocked(findOrSelectTheme).mockResolvedValue(theme)
-    vi.mocked(fetchDevelopmentThemeSpy).mockResolvedValue(undefined)
+    vi.mocked(ensureLocalFileChangesConfirmed).mockResolvedValue(false)
 
     // When
-    await pull({...defaultFlags, theme: theme.id.toString(), force: true})
+    await pull({...defaultFlags, theme: theme.id.toString()})
 
     // Then
-    expect(vi.mocked(findOrSelectTheme)).toHaveBeenCalledOnce()
-    expect(vi.mocked(hasRequiredThemeDirectories)).not.toHaveBeenCalled()
-    expect(vi.mocked(insideGitDirectory)).not.toHaveBeenCalled()
-    expect(vi.mocked(isClean)).not.toHaveBeenCalled()
-    expect(vi.mocked(ensureDirectoryConfirmed)).not.toHaveBeenCalled()
+    expect(downloadTheme).not.toHaveBeenCalled()
+    expect(renderSuccess).not.toHaveBeenCalled()
   })
 
   test('should render success message with theme preview and editor links', async () => {

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

@@ -1,6 +1,6 @@
-import {downloadTheme} from '../utilities/theme-downloader.js'
+import {downloadTheme, localFileChanges} from '../utilities/theme-downloader.js'
 import {hasRequiredThemeDirectories, mountThemeFileSystem} from '../utilities/theme-fs.js'
-import {ensureDirectoryConfirmed, themeComponent} from '../utilities/theme-ui.js'
+import {ensureDirectoryConfirmed, ensureLocalFileChangesConfirmed, themeComponent} from '../utilities/theme-ui.js'
 import {rejectGeneratedStaticAssets} from '../utilities/asset-checksum.js'
 import {ensureThemeStore} from '../utilities/theme-store.js'
 import {DevelopmentThemeManager} from '../utilities/development-theme-manager.js'
@@ -12,7 +12,6 @@ import {fetchChecksums} from '@shopify/cli-kit/node/themes/api'
 import {renderSuccess} from '@shopify/cli-kit/node/ui'
 import {glob} from '@shopify/cli-kit/node/fs'
 import {cwd} from '@shopify/cli-kit/node/path'
-import {insideGitDirectory, isClean} from '@shopify/cli-kit/node/git'
 import {recordTiming} from '@shopify/cli-kit/node/analytics'
 import {themeEditorUrl, themePreviewUrl} from '@shopify/cli-kit/node/themes/urls'
 import {Writable} from 'stream'
@@ -74,7 +73,7 @@ export interface PullFlags {
   ignore?: string[]
 
   /**
-   * Proceed without confirmation, if current directory does not seem to be theme directory.
+   * Proceed without confirmation, if current directory does not seem to be theme directory or local files would be overwritten.
    */
   force?: boolean
 
@@ -166,6 +165,11 @@ async function executePull(
   const themeChecksums = rejectGeneratedStaticAssets(remoteChecksums)
   recordTiming('theme-service:pull:file-system')
 
+  const changes = localFileChanges(themeChecksums, themeFileSystem, options)
+  if (!(await ensureLocalFileChangesConfirmed(changes, options.force, options.environment, options.multiEnvironment))) {
+    return
+  }
+
   await downloadTheme(theme, session, themeChecksums, themeFileSystem, options, context)
 
   const header = options.environment ? `Environment: ${options.environment}` : ''
@@ -233,23 +237,5 @@ async function validateDirectory(path: string, force: boolean, environment?: str
     return false
   }
 
-  /**
-   * If users are not forcing the 'pull' command, and the current directory is a
-   * Git directory and it is not clean, we ask for confirmation before proceeding.
-   */
-  const dirtyDirectory = (await insideGitDirectory(path)) && !(await isClean(path))
-
-  if (
-    dirtyDirectory &&
-    !(await ensureDirectoryConfirmed(
-      force,
-      'The current Git directory has uncommitted changes.',
-      environment,
-      multiEnvironment,
-    ))
-  ) {
-    return false
-  }
-
   return true
 }

packages/theme/src/cli/utilities/theme-downloader.test.ts

@@ -1,5 +1,5 @@
 import {fakeThemeFileSystem} from './theme-fs/theme-fs-mock-factory.js'
-import {downloadTheme} from './theme-downloader.js'
+import {downloadTheme, localFileChanges} from './theme-downloader.js'
 import {fetchThemeAssets} from '@shopify/cli-kit/node/themes/api'
 import {Checksum, ThemeAsset} from '@shopify/cli-kit/node/themes/types'
 import {test, describe, expect, vi} from 'vitest'
@@ -108,4 +108,51 @@ describe('theme-downloader', () => {
       expect(spy).toHaveBeenCalledWith(fileToDownload)
     })
   })
+
+  describe('localFileChanges', () => {
+    const root = 'tmp'
+    const files = new Map<string, ThemeAsset>([
+      ['assets/unchanged.css', {key: 'assets/unchanged.css', checksum: '1', value: 'content'}],
+      ['assets/changed.css', {key: 'assets/changed.css', checksum: '2', value: 'content'}],
+      ['assets/local-only.css', {key: 'assets/local-only.css', checksum: '3', value: 'content'}],
+    ])
+    const remote = [
+      {key: 'assets/unchanged.css', checksum: '1'},
+      {key: 'assets/changed.css', checksum: 'changed'},
+      {key: 'assets/remote-only.css', checksum: '4'},
+    ]
+
+    test('returns the local files that would be overwritten or deleted', () => {
+      // Given
+      const fileSystem = fakeThemeFileSystem(root, files)
+
+      // When
+      const changes = localFileChanges(remote, fileSystem, {nodelete: false})
+
+      // Then
+      expect(changes).toEqual({overwritten: ['assets/changed.css'], deleted: ['assets/local-only.css']})
+    })
+
+    test('does not return deleted files when nodelete is set', () => {
+      // Given
+      const fileSystem = fakeThemeFileSystem(root, files)
+
+      // When
+      const changes = localFileChanges(remote, fileSystem, {nodelete: true})
+
+      // Then
+      expect(changes).toEqual({overwritten: ['assets/changed.css'], deleted: []})
+    })
+
+    test('does not return files excluded by filters', () => {
+      // Given
+      const fileSystem = fakeThemeFileSystem(root, files, {filters: {ignore: ['assets/changed.css']}})
+
+      // When
+      const changes = localFileChanges(remote, fileSystem, {nodelete: false})
+
+      // Then
+      expect(changes).toEqual({overwritten: [], deleted: ['assets/local-only.css']})
+    })
+  })
 })

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

@@ -33,15 +33,46 @@ export async function downloadTheme(
   }
 }
 
-function buildDeleteTasks(remoteChecksums: Checksum[], themeFileSystem: ThemeFileSystem, options: DownloadOptions) {
+export interface LocalFileChanges {
+  overwritten: string[]
+  deleted: string[]
+}
+
+/**
+ * Returns the existing local files that downloading the remote theme would overwrite or delete.
+ */
+export function localFileChanges(
+  remoteChecksums: Checksum[],
+  themeFileSystem: ThemeFileSystem,
+  options: DownloadOptions,
+): LocalFileChanges {
+  return {
+    overwritten: filesToDownload(remoteChecksums, themeFileSystem).filter((key) => themeFileSystem.files.has(key)),
+    deleted: filesToDelete(remoteChecksums, themeFileSystem, options),
+  }
+}
+
+function filesToDelete(remoteChecksums: Checksum[], themeFileSystem: ThemeFileSystem, options: DownloadOptions) {
   if (options.nodelete) return []
 
   const remoteKeys = new Set(remoteChecksums.map((checksum) => checksum.key))
 
   const localKeys = themeFileSystem.applyIgnoreFilters([...themeFileSystem.files.values()]).map(({key}) => key)
-  const localFilesToBeDeleted = localKeys.filter((key) => !remoteKeys.has(key))
+  return localKeys.filter((key) => !remoteKeys.has(key))
+}
+
+function filesToDownload(remoteChecksums: Checksum[], themeFileSystem: ThemeFileSystem) {
+  return (
+    themeFileSystem
+      .applyIgnoreFilters(remoteChecksums)
+      // Filter out files we already have
+      .filter((checksum) => themeFileSystem.files.get(checksum.key)?.checksum !== checksum.checksum)
+      .map((checksum) => checksum.key)
+  )
+}
 
-  return localFilesToBeDeleted.map((key) => {
+function buildDeleteTasks(remoteChecksums: Checksum[], themeFileSystem: ThemeFileSystem, options: DownloadOptions) {
+  return filesToDelete(remoteChecksums, themeFileSystem, options).map((key) => {
     return {
       title: `Cleaning your local directory (removing ${key})`,
       task: async () => themeFileSystem.delete(key),
@@ -55,21 +86,7 @@ function buildDownloadTasks(
   themeFileSystem: ThemeFileSystem,
   session: AdminSession,
 ): Task[] {
-  let checksums = themeFileSystem.applyIgnoreFilters(remoteChecksums)
-
-  // Filter out files we already have
-  checksums = checksums.filter((checksum) => {
-    const remoteChecksumValue = checksum.checksum
-    const localAsset = themeFileSystem.files.get(checksum.key)
-
-    if (localAsset?.checksum === remoteChecksumValue) {
-      return false
-    } else {
-      return true
-    }
-  })
-
-  const filenames = checksums.map((checksum) => checksum.key)
+  const filenames = filesToDownload(remoteChecksums, themeFileSystem)
 
   const getProgress = (params: {current: number; total: number}) =>
     params.total === 0 ? `[100%]` : `[${Math.round((params.current / params.total) * 100)}%]`