fix PR comments

src/components/common/LazyImage.vue

@@ -31,7 +31,7 @@
 
 <script setup lang="ts">
 import Skeleton from 'primevue/skeleton'
-import { computed, ref, watch } from 'vue'
+import { computed, onUnmounted, ref, watch } from 'vue'
 
 import { useIntersectionObserver } from '@/composables/useIntersectionObserver'
 import { useMediaCache } from '@/services/mediaCacheService'
@@ -41,7 +41,7 @@ const {
   alt = '',
   imageClass = '',
   imageStyle,
-  rootMargin = '50px'
+  rootMargin = '300px'
 } = defineProps<{
   src: string
   alt?: string
@@ -57,7 +57,7 @@ const isImageLoaded = ref(false)
 const hasError = ref(false)
 const cachedSrc = ref<string | undefined>(undefined)
 
-const { getCachedMedia } = useMediaCache()
+const { getCachedMedia, acquireUrl, releaseUrl } = useMediaCache()
 
 // Use intersection observer to detect when the image container comes into view
 useIntersectionObserver(
@@ -75,7 +75,6 @@ useIntersectionObserver(
 // Only start loading the image when it's in view
 const shouldLoad = computed(() => isIntersecting.value)
 
-// Watch for when we should load and handle caching
 watch(
   shouldLoad,
   async (shouldLoad) => {
@@ -85,16 +84,19 @@ watch(
         if (cachedMedia.error) {
           hasError.value = true
         } else if (cachedMedia.objectUrl) {
-          cachedSrc.value = cachedMedia.objectUrl
+          const acquiredUrl = acquireUrl(src)
+          cachedSrc.value = acquiredUrl || cachedMedia.objectUrl
         } else {
           cachedSrc.value = src
         }
       } catch (error) {
         console.warn('Failed to load cached media:', error)
-        // Fallback to original src
         cachedSrc.value = src
       }
     } else if (!shouldLoad) {
+      if (cachedSrc.value?.startsWith('blob:')) {
+        releaseUrl(src)
+      }
       // Hide image when out of view
       isImageLoaded.value = false
       cachedSrc.value = undefined
@@ -113,4 +115,10 @@ const onImageError = () => {
   hasError.value = true
   isImageLoaded.value = false
 }
+
+onUnmounted(() => {
+  if (cachedSrc.value?.startsWith('blob:')) {
+    releaseUrl(src)
+  }
+})
 </script>

src/services/mediaCacheService.ts

@@ -1,4 +1,4 @@
-import { shallowRef } from 'vue'
+import { reactive } from 'vue'
 
 export interface CachedMedia {
   src: string
@@ -16,10 +16,11 @@ export interface MediaCacheOptions {
 }
 
 class MediaCacheService {
-  public cache = shallowRef(new Map<string, CachedMedia>())
+  public cache = reactive(new Map<string, CachedMedia>())
   private readonly maxSize: number
   private readonly maxAge: number
   private cleanupInterval: number | null = null
+  private urlRefCount = new Map<string, number>()
 
   constructor(options: MediaCacheOptions = {}) {
     this.maxSize = options.maxSize ?? 100
@@ -41,49 +42,58 @@ class MediaCacheService {
 
   private cleanup() {
     const now = Date.now()
-    const cacheMap = this.cache.value
     const keysToDelete: string[] = []
 
     // Find expired entries
-    for (const [key, entry] of Array.from(cacheMap.entries())) {
+    for (const [key, entry] of Array.from(this.cache.entries())) {
       if (now - entry.lastAccessed > this.maxAge) {
-        keysToDelete.push(key)
-        // Revoke object URL to free memory
+        // Only revoke object URL if no components are using it
         if (entry.objectUrl) {
-          URL.revokeObjectURL(entry.objectUrl)
+          const refCount = this.urlRefCount.get(entry.objectUrl) || 0
+          if (refCount === 0) {
+            URL.revokeObjectURL(entry.objectUrl)
+            this.urlRefCount.delete(entry.objectUrl)
+            keysToDelete.push(key)
+          }
+          // Don't delete cache entry if URL is still in use
+        } else {
+          keysToDelete.push(key)
         }
       }
     }
 
     // Remove expired entries
-    if (keysToDelete.length > 0) {
-      const newCache = new Map(cacheMap)
-      keysToDelete.forEach((key) => newCache.delete(key))
-      this.cache.value = newCache
-    }
+    keysToDelete.forEach((key) => this.cache.delete(key))
 
-    // If still over size limit, remove oldest entries
-    if (cacheMap.size > this.maxSize) {
-      const entries = Array.from(cacheMap.entries())
+    // If still over size limit, remove oldest entries that aren't in use
+    if (this.cache.size > this.maxSize) {
+      const entries = Array.from(this.cache.entries())
       entries.sort((a, b) => a[1].lastAccessed - b[1].lastAccessed)
 
-      const toRemove = entries.slice(0, cacheMap.size - this.maxSize)
-      const newCache = new Map(cacheMap)
+      let removedCount = 0
+      const targetRemoveCount = this.cache.size - this.maxSize
+
+      for (const [key, entry] of entries) {
+        if (removedCount >= targetRemoveCount) break
 
-      toRemove.forEach(([key, entry]) => {
         if (entry.objectUrl) {
-          URL.revokeObjectURL(entry.objectUrl)
+          const refCount = this.urlRefCount.get(entry.objectUrl) || 0
+          if (refCount === 0) {
+            URL.revokeObjectURL(entry.objectUrl)
+            this.urlRefCount.delete(entry.objectUrl)
+            this.cache.delete(key)
+            removedCount++
+          }
+        } else {
+          this.cache.delete(key)
+          removedCount++
         }
-        newCache.delete(key)
-      })
-
-      this.cache.value = newCache
+      }
     }
   }
 
   async getCachedMedia(src: string): Promise<CachedMedia> {
-    const cacheMap = this.cache.value
-    let entry = cacheMap.get(src)
+    let entry = this.cache.get(src)
 
     if (entry) {
       // Update last accessed time
@@ -99,9 +109,7 @@ class MediaCacheService {
     }
 
     // Update cache with loading entry
-    const newCache = new Map(cacheMap)
-    newCache.set(src, entry)
-    this.cache.value = newCache
+    this.cache.set(src, entry)
 
     try {
       // Fetch the media
@@ -122,10 +130,7 @@ class MediaCacheService {
         lastAccessed: Date.now()
       }
 
-      const finalCache = new Map(this.cache.value)
-      finalCache.set(src, updatedEntry)
-      this.cache.value = finalCache
-
+      this.cache.set(src, updatedEntry)
       return updatedEntry
     } catch (error) {
       console.warn('Failed to cache media:', src, error)
@@ -138,44 +143,45 @@ class MediaCacheService {
         lastAccessed: Date.now()
       }
 
-      const errorCache = new Map(this.cache.value)
-      errorCache.set(src, errorEntry)
-      this.cache.value = errorCache
-
+      this.cache.set(src, errorEntry)
       return errorEntry
     }
   }
 
-  getCacheStats() {
-    const cacheMap = this.cache.value
-    return {
-      size: cacheMap.size,
-      maxSize: this.maxSize,
-      entries: Array.from(cacheMap.keys())
+  acquireUrl(src: string): string | undefined {
+    const entry = this.cache.get(src)
+    if (entry?.objectUrl) {
+      const currentCount = this.urlRefCount.get(entry.objectUrl) || 0
+      this.urlRefCount.set(entry.objectUrl, currentCount + 1)
+      return entry.objectUrl
+    }
+    return undefined
+  }
+
+  releaseUrl(src: string): void {
+    const entry = this.cache.get(src)
+    if (entry?.objectUrl) {
+      const count = (this.urlRefCount.get(entry.objectUrl) || 1) - 1
+      if (count <= 0) {
+        URL.revokeObjectURL(entry.objectUrl)
+        this.urlRefCount.delete(entry.objectUrl)
+        // Remove from cache as well
+        this.cache.delete(src)
+      } else {
+        this.urlRefCount.set(entry.objectUrl, count)
+      }
     }
   }
 
   clearCache() {
-    const cacheMap = this.cache.value
     // Revoke all object URLs
-    for (const entry of Array.from(cacheMap.values())) {
+    for (const entry of Array.from(this.cache.values())) {
       if (entry.objectUrl) {
         URL.revokeObjectURL(entry.objectUrl)
       }
     }
-    this.cache.value = new Map()
-  }
-
-  preloadMedia(urls: string[]) {
-    // Preload media in the background without blocking
-    urls.forEach((url) => {
-      if (!this.cache.value.has(url)) {
-        // Don't await - fire and forget
-        this.getCachedMedia(url).catch(() => {
-          // Ignore preload errors
-        })
-      }
-    })
+    this.cache.clear()
+    this.urlRefCount.clear()
   }
 
   destroy() {
@@ -188,7 +194,7 @@ class MediaCacheService {
 }
 
 // Global instance
-let mediaCacheInstance: MediaCacheService | null = null
+export let mediaCacheInstance: MediaCacheService | null = null
 
 export function useMediaCache(options?: MediaCacheOptions) {
   if (!mediaCacheInstance) {
@@ -197,16 +203,15 @@ export function useMediaCache(options?: MediaCacheOptions) {
 
   const getCachedMedia = (src: string) =>
     mediaCacheInstance!.getCachedMedia(src)
-  const getCacheStats = () => mediaCacheInstance!.getCacheStats()
   const clearCache = () => mediaCacheInstance!.clearCache()
-  const preloadMedia = (urls: string[]) =>
-    mediaCacheInstance!.preloadMedia(urls)
+  const acquireUrl = (src: string) => mediaCacheInstance!.acquireUrl(src)
+  const releaseUrl = (src: string) => mediaCacheInstance!.releaseUrl(src)
 
   return {
     getCachedMedia,
-    getCacheStats,
     clearCache,
-    preloadMedia,
+    acquireUrl,
+    releaseUrl,
     cache: mediaCacheInstance.cache
   }
 }

tests-ui/tests/services/mediaCacheService.test.ts

@@ -0,0 +1,35 @@
+import { describe, expect, it, vi } from 'vitest'
+
+import { useMediaCache } from '../../../src/services/mediaCacheService'
+
+// Mock fetch
+global.fetch = vi.fn()
+global.URL = {
+  createObjectURL: vi.fn(() => 'blob:mock-url'),
+  revokeObjectURL: vi.fn()
+} as any
+
+describe('mediaCacheService', () => {
+  describe('URL reference counting', () => {
+    it('should handle URL acquisition for non-existent cache entry', () => {
+      const { acquireUrl } = useMediaCache()
+
+      const url = acquireUrl('non-existent.jpg')
+      expect(url).toBeUndefined()
+    })
+
+    it('should handle URL release for non-existent cache entry', () => {
+      const { releaseUrl } = useMediaCache()
+
+      // Should not throw error
+      expect(() => releaseUrl('non-existent.jpg')).not.toThrow()
+    })
+
+    it('should provide acquireUrl and releaseUrl methods', () => {
+      const cache = useMediaCache()
+
+      expect(typeof cache.acquireUrl).toBe('function')
+      expect(typeof cache.releaseUrl).toBe('function')
+    })
+  })
+})