Preserve remote theme deletions when local cleanup fails

Author: gonzaloriestra

packages/theme/src/cli/commands/theme/delete.test.ts

@@ -1,6 +1,7 @@
 import Delete from './delete.js'
 import {themeDeleteJsonOutputSchema} from '../../services/delete/types.js'
 import {findThemes} from '../../utilities/theme-selector.js'
+import * as localStorage from '../../services/local-storage.js'
 import {Config} from '@oclif/core'
 import {ensureAuthenticatedThemes} from '@shopify/cli-kit/node/session'
 import {themeDelete} from '@shopify/cli-kit/node/themes/api'
@@ -10,6 +11,7 @@ import {inTemporaryDirectory, writeFile} from '@shopify/cli-kit/node/fs'
 import {joinPath} from '@shopify/cli-kit/node/path'
 import {outputWarn} from '@shopify/cli-kit/node/output'
 import {addSensitiveMetadata} from '@shopify/cli-kit/node/metadata'
+import {AbortError} from '@shopify/cli-kit/node/error'
 import {describe, expect, test, vi} from 'vitest'
 
 vi.mock('@shopify/cli-kit/node/session')
@@ -106,6 +108,30 @@ describe('theme delete JSON output', () => {
     })
   })
 
+  test('keeps successful JSON output when development theme cleanup fails', async () => {
+    vi.mocked(findThemes).mockResolvedValue([{...theme, role: 'development'}])
+    const cleanup = vi.spyOn(localStorage, 'removeDevelopmentTheme').mockImplementation(() => {
+      throw new AbortError('Local storage is not writable')
+    })
+    const exitCode = process.exitCode
+    try {
+      await withCapturedStandardStreams(async ({stdout, stderr}) => {
+        await run(['--store', store, '--theme', '1', '--force', '--json'])
+
+        expect(JSON.parse(stdout())).toMatchObject({status: 'success', changed: true, themes: [{id: '1'}]})
+        expect(JSON.parse(stderr())).toMatchObject({
+          type: 'diagnostic',
+          level: 'warning',
+          message: expect.stringContaining('Local storage is not writable'),
+        })
+        expect(process.exitCode).toBe(exitCode)
+      })
+    } finally {
+      cleanup.mockRestore()
+      restoreExitCode(exitCode)
+    }
+  })
+
   test.each([false, true])('preserves partial deletions and rejects total failure (partial: %s)', async (partial) => {
     const originalExitCode = process.exitCode
     vi.mocked(findThemes).mockResolvedValue(partial ? [theme, {...theme, id: 2}] : [theme])

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

@@ -1,13 +1,17 @@
 import {themesDelete as executeDelete} from './delete.js'
 import {renderThemeDeleteResult} from './delete/result.js'
+import {removeDevelopmentTheme} from './local-storage.js'
 import {findOrSelectTheme, findThemes} from '../utilities/theme-selector.js'
 import {themeDelete} from '@shopify/cli-kit/node/themes/api'
 import {Theme} from '@shopify/cli-kit/node/themes/types'
 import {test, describe, expect, vi} from 'vitest'
 import {renderConfirmationPrompt, renderSuccess} from '@shopify/cli-kit/node/ui'
+import {AbortError} from '@shopify/cli-kit/node/error'
+import {mockAndCaptureOutput} from '@shopify/cli-kit/node/testing/output'
 
 vi.mock('@shopify/cli-kit/node/ui')
 vi.mock('@shopify/cli-kit/node/themes/api')
+vi.mock('./local-storage.js')
 vi.mock('../utilities/theme-selector.js')
 vi.mock('../utilities/development-theme-manager.js', () => {
   const DevelopmentThemeManager = vi.fn()
@@ -167,6 +171,36 @@ test('does not prompt again for multiple environments', async () => {
   expect(themeDelete).toHaveBeenCalledWith(1, session)
 })
 
+test.each([false, true])('preserves remote deletions when local cleanup fails (multiple: %s)', async (multiple) => {
+  const output = mockAndCaptureOutput()
+  const developmentTheme = {...theme1, role: 'development'}
+  const themes = multiple ? [developmentTheme, theme2] : [developmentTheme]
+  vi.mocked(findThemes).mockResolvedValue(themes)
+  vi.mocked(removeDevelopmentTheme).mockImplementation(() => {
+    throw new AbortError('Local storage is not writable')
+  })
+
+  const result = await executeDelete(session, {...options, themes: ['1', '2'], force: true})
+
+  expect(result).toEqual({
+    status: 'success',
+    themes: themes.map((theme) => ({...theme, shop: session.storeFqdn})),
+  })
+  expect(output.warn()).toContain(
+    'Theme #1 was deleted, but its local development theme reference could not be removed',
+  )
+  expect(output.warn()).toContain('Local storage is not writable')
+})
+
+test('does not remove local state when remote deletion fails', async () => {
+  vi.mocked(findThemes).mockResolvedValue([{...theme1, role: 'development'}])
+  vi.mocked(themeDelete).mockRejectedValue(new Error('Deletion failed'))
+
+  await expect(executeDelete(session, {...options, themes: ['1'], force: true})).rejects.toThrow('Deletion failed')
+
+  expect(removeDevelopmentTheme).not.toHaveBeenCalled()
+})
+
 test('preserves the environment label in terminal output', () => {
   renderThemeDeleteResult({status: 'success', themes: [{...theme1, shop: session.storeFqdn}]}, 'text', {
     store: session.storeFqdn,

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

@@ -15,6 +15,7 @@ import {pluralize} from '@shopify/cli-kit/common/string'
 import {Theme} from '@shopify/cli-kit/node/themes/types'
 import {isDevelopmentTheme} from '@shopify/cli-kit/node/themes/utils'
 import {AbortError} from '@shopify/cli-kit/node/error'
+import {outputWarn} from '@shopify/cli-kit/node/output'
 
 interface DeleteOptions {
   selectTheme: boolean
@@ -44,11 +45,22 @@ export async function themesDelete(
   const deletions = await Promise.allSettled(
     themes.map(async (theme) => {
       await themeDelete(theme.id, adminSession)
-      if (isDevelopmentTheme(theme)) removeDevelopmentTheme()
       return {...theme, shop: store}
     }),
   )
   const deletedThemes = deletions.flatMap((deletion) => (deletion.status === 'fulfilled' ? [deletion.value] : []))
+  for (const theme of deletedThemes) {
+    if (!isDevelopmentTheme(theme)) continue
+    try {
+      removeDevelopmentTheme()
+      // eslint-disable-next-line no-catch-all/no-catch-all -- Local cleanup cannot undo a completed remote deletion.
+    } catch (error) {
+      const message = error instanceof Error ? error.message : String(error)
+      outputWarn(
+        `Theme #${theme.id} was deleted, but its local development theme reference could not be removed: ${message}`,
+      )
+    }
+  }
   const errors = deletions.flatMap((deletion, index) => {
     if (deletion.status === 'fulfilled') return []
     const error: unknown = deletion.reason