fix(test): use dependency injection for storage in ApiKeyStorage

- Add Storage interface and inject it via constructor
- Remove all global mocking (vi.stubGlobal) from tests
- Use simple in-memory storage mock for deterministic testing
- This eliminates environment-specific issues with jsdom in CI

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Author: msujaws

src/lib/storage/api-key-storage.test.ts

@@ -1,39 +1,32 @@
-import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'
-import { ApiKeyStorage } from './api-key-storage'
+import { describe, it, expect, beforeEach } from 'vitest'
+import { ApiKeyStorage, type Storage } from './api-key-storage'
 
 describe('ApiKeyStorage', () => {
   let storage: ApiKeyStorage
-  let localStorageMock: Record<string, string>
+  let mockStorageData: Record<string, string>
+  let mockStorage: Storage
   const testKeyMaterial = 'test-key-material-for-encryption'
 
   beforeEach(() => {
-    // Mock localStorage
-    localStorageMock = {}
-
-    vi.stubGlobal('localStorage', {
+    // Create in-memory storage mock (no global stubbing needed)
+    mockStorageData = {}
+    mockStorage = {
       // eslint-disable-next-line unicorn/no-null
-      getItem: vi.fn((key: string) => localStorageMock[key] ?? null), // null is correct for localStorage API
-      setItem: vi.fn((key: string, value: string) => {
-        localStorageMock[key] = value
-      }),
-      removeItem: vi.fn((key: string) => {
+      getItem: (key: string) => mockStorageData[key] ?? null,
+      setItem: (key: string, value: string) => {
+        mockStorageData[key] = value
+      },
+      removeItem: (key: string) => {
         // eslint-disable-next-line @typescript-eslint/no-dynamic-delete
-        delete localStorageMock[key]
-      }),
-      clear: vi.fn(() => {
-        localStorageMock = {}
-      }),
-      length: 0,
-      key: vi.fn(),
+        delete mockStorageData[key]
+      },
+    }
+
+    // Inject both key material and storage for deterministic testing
+    storage = new ApiKeyStorage({
+      keyMaterial: testKeyMaterial,
+      storage: mockStorage,
     })
-
-    // Use constructor parameter for consistent key derivation in tests
-    storage = new ApiKeyStorage(testKeyMaterial)
-  })
-
-  afterEach(() => {
-    vi.restoreAllMocks()
-    vi.unstubAllGlobals()
   })
 
   describe('saveApiKey', () => {
@@ -42,9 +35,7 @@ describe('ApiKeyStorage', () => {
 
       await storage.saveApiKey(apiKey)
 
-      // eslint-disable-next-line @typescript-eslint/unbound-method
-      expect(localStorage.setItem).toHaveBeenCalled()
-      const savedValue = localStorageMock['bugzilla_api_key']
+      const savedValue = mockStorageData['bugzilla_api_key']
       expect(savedValue).toBeDefined()
       expect(savedValue).not.toBe(apiKey) // Should be encrypted
     })
@@ -53,10 +44,10 @@ describe('ApiKeyStorage', () => {
       const apiKey = 'test-api-key-123'
 
       await storage.saveApiKey(apiKey)
-      const firstSave = localStorageMock['bugzilla_api_key']
+      const firstSave = mockStorageData['bugzilla_api_key']
 
       await storage.saveApiKey(apiKey)
-      const secondSave = localStorageMock['bugzilla_api_key']
+      const secondSave = mockStorageData['bugzilla_api_key']
 
       // Should have different encrypted values due to random IV
       expect(firstSave).not.toBe(secondSave)
@@ -65,8 +56,7 @@ describe('ApiKeyStorage', () => {
     it('should handle empty API key', async () => {
       await storage.saveApiKey('')
 
-      // eslint-disable-next-line @typescript-eslint/unbound-method
-      expect(localStorage.setItem).toHaveBeenCalled()
+      expect(mockStorageData['bugzilla_api_key']).toBeDefined()
     })
   })
 
@@ -87,7 +77,7 @@ describe('ApiKeyStorage', () => {
     })
 
     it('should return undefined when localStorage value is invalid', async () => {
-      localStorageMock['bugzilla_api_key'] = 'invalid-encrypted-data'
+      mockStorageData['bugzilla_api_key'] = 'invalid-encrypted-data'
 
       const retrieved = await storage.getApiKey()
 
@@ -96,7 +86,7 @@ describe('ApiKeyStorage', () => {
 
     it('should handle decryption errors gracefully', async () => {
       // Set malformed encrypted data
-      localStorageMock['bugzilla_api_key'] = JSON.stringify({
+      mockStorageData['bugzilla_api_key'] = JSON.stringify({
         iv: 'invalid-iv',
         encryptedData: 'invalid-data',
       })
@@ -113,9 +103,7 @@ describe('ApiKeyStorage', () => {
 
       storage.clearApiKey()
 
-      // eslint-disable-next-line @typescript-eslint/unbound-method
-      expect(localStorage.removeItem).toHaveBeenCalledWith('bugzilla_api_key')
-      expect(localStorageMock['bugzilla_api_key']).toBeUndefined()
+      expect(mockStorageData['bugzilla_api_key']).toBeUndefined()
     })
 
     it('should not throw error when no key exists', () => {
@@ -144,10 +132,10 @@ describe('ApiKeyStorage', () => {
   describe('encryption', () => {
     it('should encrypt different keys to different values', async () => {
       await storage.saveApiKey('key1')
-      const encrypted1 = localStorageMock['bugzilla_api_key']
+      const encrypted1 = mockStorageData['bugzilla_api_key']
 
       await storage.saveApiKey('key2')
-      const encrypted2 = localStorageMock['bugzilla_api_key']
+      const encrypted2 = mockStorageData['bugzilla_api_key']
 
       expect(encrypted1).not.toBe(encrypted2)
     })

src/lib/storage/api-key-storage.ts

@@ -10,16 +10,29 @@ interface EncryptedData {
   encryptedData: string // Base64 encoded encrypted data
 }
 
+/**
+ * Storage interface for dependency injection (enables testing)
+ */
+export interface Storage {
+  getItem(key: string): string | null
+  setItem(key: string, value: string): void
+  removeItem(key: string): void
+}
+
 export class ApiKeyStorage {
   private encoder = new TextEncoder()
   private decoder = new TextDecoder()
   private keyMaterialOverride?: string
+  private storage: Storage
 
   /**
-   * @param keyMaterialOverride - Optional override for key derivation (for testing only)
+   * @param options - Optional configuration for testing
+   * @param options.keyMaterial - Override for key derivation
+   * @param options.storage - Override for storage backend (defaults to localStorage)
    */
-  constructor(keyMaterialOverride?: string) {
-    this.keyMaterialOverride = keyMaterialOverride
+  constructor(options?: { keyMaterial?: string; storage?: Storage }) {
+    this.keyMaterialOverride = options?.keyMaterial
+    this.storage = options?.storage ?? localStorage
   }
 
   /**
@@ -127,14 +140,14 @@ export class ApiKeyStorage {
    */
   async saveApiKey(apiKey: string): Promise<void> {
     const encrypted = await this.encrypt(apiKey)
-    localStorage.setItem(STORAGE_KEY, JSON.stringify(encrypted))
+    this.storage.setItem(STORAGE_KEY, JSON.stringify(encrypted))
   }
 
   /**
    * Retrieve API key from localStorage (decrypted)
    */
   async getApiKey(): Promise<string | undefined> {
-    const stored = localStorage.getItem(STORAGE_KEY)
+    const stored = this.storage.getItem(STORAGE_KEY)
 
     if (!stored) {
       return undefined
@@ -153,13 +166,13 @@ export class ApiKeyStorage {
    * Remove API key from localStorage
    */
   clearApiKey(): void {
-    localStorage.removeItem(STORAGE_KEY)
+    this.storage.removeItem(STORAGE_KEY)
   }
 
   /**
    * Check if API key exists in localStorage
    */
   hasApiKey(): boolean {
-    return localStorage.getItem(STORAGE_KEY) !== null
+    return this.storage.getItem(STORAGE_KEY) !== null
   }
 }