feat: lazy loading for evidences preview with caching (faster) #2762
Conversation
WalkthroughAdds an in-memory attachment cache and lazy-loading for EvidenceFilePreview using IntersectionObserver; previews fetch and cache blob URLs, store misses, show loading/no-preview states, and revoke blob URLs on replace/remove; displayPreview now accepts an Changes
Sequence Diagram(s)sequenceDiagram
participant Comp as EvidenceFilePreview
participant Obs as IntersectionObserver
participant Cache as attachmentCache
participant API as Backend
Comp->>Obs: observe(containerElement)
Obs-->>Comp: intersection -> isVisible = true
alt cache hit
Comp->>Cache: get(cacheKey)
Cache-->>Comp: CachedAttachment (url, fileExists)
Comp->>Comp: displayPreview(att from cache)
else cache miss
Comp->>API: fetchAttachment(id, name)
API-->>Comp: blob / 404 / error
Comp->>Cache: set(cacheKey, CachedAttachment|miss)
Comp->>Comp: displayPreview(att or NoPreview/Loading)
end
rect rgb(240,240,220)
Note over Comp,Obs: component destroyed
Comp->>Obs: disconnect()
Note right of Cache: cache may revoke blob URLs on replace/remove/clear
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/lib/components/ModelTable/EvidenceFilePreview.svelte (1)
120-134: Keyboard accessibility: don’t toggle on Tab; support Enter/Space; add aria-label.Current handler toggles on Tab, which interferes with focus navigation.
- onkeydown={(event) => { - if ((display && event.key === 'Escape') || event.key === 'Tab') { - display = !display; - } - }} + onkeydown={(event) => { + if (!display && (event.key === 'Enter' || event.key === ' ')) { + display = true; + } else if (display && event.key === 'Escape') { + display = false; + } + }} + aria-label="Preview attachment"
🧹 Nitpick comments (12)
frontend/src/lib/stores/attachmentCache.ts (4)
65-68: Use hasOwnProperty for membership checks.Avoid prototype-chain hits with the in operator.
- has: (key: string): boolean => { - const cache = get({ subscribe }); - return key in cache; - } + has: (key: string): boolean => { + const cache = get({ subscribe }); + return Object.prototype.hasOwnProperty.call(cache, key); + }
19-19: Remove unused destructured set.It isn’t used and shadows your custom set API name.
- const { subscribe, set, update } = writable<AttachmentCache>({}); + const { subscribe, update } = writable<AttachmentCache>({});
8-12: Export the CachedAttachment type and reuse it in consumers.Prevents type drift with the component’s duplicate Attachment interface.
-interface CachedAttachment { +export interface CachedAttachment { type: string; url: string; fileExists: boolean; }
77-79: Strengthen cache key to avoid collisions (evidence vs. revisions).Current key omits the scope, risking collisions for identical id/name across endpoints.
-export function generateAttachmentCacheKey(id: string, attachmentName: string): string { - return `${id}-${attachmentName}`; -} +export function generateAttachmentCacheKey( + id: string, + attachmentName: string, + variant?: 'evidence' | 'revision' +): string { + const scope = variant ?? 'evidence'; + return `${scope}:${id}:${attachmentName}`; +}Please update call sites accordingly (see suggested changes in EvidenceFilePreview.svelte).
frontend/src/lib/components/ModelTable/EvidenceFilePreview.svelte (8)
27-31: Support cancellation to prevent stale updates and wasted work.Use AbortController to cancel in-flight fetches on prop changes or unmount.
const fetchAttachment = async () => { + // Cancel any previous fetch and create a new controller + abortCtl?.abort(); + abortCtl = new AbortController(); // Generate cache key cacheKey = generateAttachmentCacheKey(meta.id, meta.attachment);
21-26: Wire up AbortController lifecycle.Declare controller and abort on destroy.
- let attachment: Attachment | undefined = $state(); + let attachment: Attachment | undefined = $state(); + let abortCtl: AbortController | undefined = $state(); onDestroy(() => { // Clean up observer if (observer) { observer.disconnect(); } + abortCtl?.abort(); });Also applies to: 83-88
114-118: Fix Tailwind z-index utility.Use an arbitrary value with brackets.
-const wrapperClasses = - 'fixed w-full h-full inset-0 flex justify-center items-center backdrop-blur-sm backdrop-brightness-40 z-999'; +const wrapperClasses = + 'fixed w-full h-full inset-0 flex justify-center items-center backdrop-blur-sm backdrop-brightness-40 z-[999]';
136-141: Avoid conflicting height classes when expanded.When display is true, h-24 and h-[90%] both render; prefer a single branch-specific class.
- <img - src={att.url} - alt="attachment" - class="h-24 object-contain {display ? imageElementClasses : ''}" - /> + <img + src={att.url} + alt="attachment" + class={display ? imageElementClasses : 'h-24 object-contain'} + /> - <embed - src={att.url} - type="application/pdf" - class="h-24 object-contain {display ? embedElementClasses : ''}" - /> + <embed + src={att.url} + type="application/pdf" + class={display ? embedElementClasses : 'h-24 object-contain'} + />Also applies to: 147-150
4-7: Reuse the exported type instead of duplicating the interface.Import the type from the cache module to keep things in sync.
import { m } from '$paraglide/messages'; import { attachmentCache, generateAttachmentCacheKey } from '$lib/stores/attachmentCache'; +import type { CachedAttachment as Attachment } from '$lib/stores/attachmentCache'; @@ -interface Attachment { - type: string; - url: string; - fileExists: boolean; -}Also applies to: 15-19
29-29: Include variant in cache key to avoid cross-endpoint collisions.Pass a scope to the key generator (requires the companion change in attachmentCache.ts).
- cacheKey = generateAttachmentCacheKey(meta.id, meta.attachment); + cacheKey = generateAttachmentCacheKey( + meta.id, + meta.attachment, + meta.evidence ? 'revision' : 'evidence' + ); @@ - const key = generateAttachmentCacheKey(meta.id, meta.attachment); + const key = generateAttachmentCacheKey( + meta.id, + meta.attachment, + meta.evidence ? 'revision' : 'evidence' + );Also applies to: 93-99
59-81: Optional: preload earlier for smoother UX.Consider a larger rootMargin (e.g., 200px) to start fetch slightly earlier on fast scroll.
27-53: Optional: dedupe in-flight requests across components.Two components can request the same key before the first write completes. A simple in-flight Promise map in the cache layer would coalesce concurrent fetches.
I can sketch an in-flight map pattern if you’d like.
Also applies to: 59-81, 102-107
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/src/lib/components/ModelTable/EvidenceFilePreview.svelte(4 hunks)frontend/src/lib/stores/attachmentCache.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: build_enterprise_frontend
- GitHub Check: build_community_frontend
- GitHub Check: build (22)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: lint-community (22)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: startup-functional-test (3.12)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
frontend/src/lib/components/ModelTable/EvidenceFilePreview.svelte (3)
25-25: Remove redundant state variable.The
cacheKeystate variable is unnecessary since it's recalculated on lines 29 and 103. Cache keys should be computed inline where needed rather than stored in reactive state.Apply this diff:
-let cacheKey = $state('');Then inline the computation where used.
66-91: Consider checking initial visibility.The observer setup doesn't check if
containerElementis already in the viewport on mount. For elements that start visible, the intersection callback may not fire immediately, delaying the fetch. Consider checkingentry.intersectionRatioor usingobserver.takeRecords()after observation starts.Alternative approach:
observer = new IntersectionObserver( (entries) => { entries.forEach((entry) => { if (entry.isIntersecting && !isVisible) { isVisible = true; - // Fetch attachment when element becomes visible fetchAttachment().then((_attachment) => { attachment = _attachment; }); - // Disconnect after first load observer?.disconnect(); } }); }, { - rootMargin: '50px' // Start loading slightly before element is visible + rootMargin: '50px', + threshold: 0 } ); observer.observe(containerElement); + // Check if already visible + if (containerElement.getBoundingClientRect().top < window.innerHeight) { + isVisible = true; + fetchAttachment().then((_attachment) => { + attachment = _attachment; + }); + observer.disconnect(); + } }
100-121: Simplify coordination between run block and observer.The run block (lines 112-117) and onMount observer (lines 77-79) both call
fetchAttachment(), creating duplicate fetch paths. Consider:
- Let the observer handle all initial loads
- Use the run block only for cache lookups when meta changes
- Or remove the
isVisiblecheck and always fetch in the run block (simpler but less lazy)Current logic may skip fetching if meta changes before the element is observed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/src/lib/components/ModelTable/EvidenceFilePreview.svelte(4 hunks)frontend/src/lib/stores/attachmentCache.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/lib/stores/attachmentCache.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: build_community_frontend
- GitHub Check: build_enterprise_frontend
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: startup-docker-compose-test
- GitHub Check: Analyze (python)
🔇 Additional comments (4)
frontend/src/lib/components/ModelTable/EvidenceFilePreview.svelte (4)
4-6: LGTM!The imports correctly add lifecycle hooks and cache utilities needed for the lazy loading implementation.
93-98: LGTM!Observer cleanup is properly implemented. Note: blob URL cleanup still needed (flagged separately).
130-163: LGTM!The refactored snippet signature with the
attparameter improves reusability and clarity. The implementation correctly handles image and PDF previews.
165-181: LGTM!The template correctly implements lazy loading container binding and provides clear loading, error, and no-preview states for better UX.
| const blob = await res.blob(); | ||
| const result = { | ||
| type: blob.type, | ||
| url: URL.createObjectURL(blob), | ||
| fileExists: true | ||
| } satisfies Attachment; | ||
| attachmentCache.set(cacheKey, result); | ||
| return result; |
There was a problem hiding this comment.
Revoke blob URLs to prevent memory leaks.
Blob URLs created on line 52 are never revoked, causing memory leaks as attachments accumulate. Implement cleanup:
- Revoke URLs in
onDestroy - Revoke old URL before caching a replacement
- Consider tracking URLs for batch cleanup
Example cleanup in onDestroy:
onDestroy(() => {
if (observer) {
observer.disconnect();
}
// Revoke blob URL to free memory
if (attachment?.url) {
URL.revokeObjectURL(attachment.url);
}
});🤖 Prompt for AI Agents
In frontend/src/lib/components/ModelTable/EvidenceFilePreview.svelte around
lines 49 to 56, the created blob URL (URL.createObjectURL) is never revoked
causing memory leaks; update the component to revoke URLs: before caching a new
result, call URL.revokeObjectURL on any previous cached attachment.url for the
same cacheKey, store/tracked created URLs so they can be revoked in batch, and
add an onDestroy handler that disconnects any observer and revokes the current
attachment.url (and any tracked URLs) to free memory.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
frontend/src/lib/components/ModelTable/EvidenceFilePreview.svelte (3)
25-25: Consider removing the cacheKey state variable.The
cacheKeystate variable is set infetchAttachment(line 29) but is never read elsewhere. Therunblock generates its own key inline (line 110). This state variable appears unused and can be removed.Apply this diff:
let containerElement: HTMLElement | undefined = $state(); let isVisible = $state(false); let observer: IntersectionObserver | undefined; - let cacheKey = $state('');And remove the assignment on line 29:
const fetchAttachment = async () => { - // Generate cache key - cacheKey = generateAttachmentCacheKey(meta.id, meta.attachment); + const cacheKey = generateAttachmentCacheKey(meta.id, meta.attachment); // Check cache first
77-84: Simplify observer logic.The
!isVisiblecheck on line 77 is redundant since the observer disconnects immediately after first load (line 84). Additionally, the promise chain could be more concise.Apply this diff to simplify:
entries.forEach((entry) => { - if (entry.isIntersecting && !isVisible) { + if (entry.isIntersecting) { isVisible = true; - // Fetch attachment when element becomes visible - fetchAttachment().then((_attachment) => { - attachment = _attachment; - }); - // Disconnect after first load + fetchAttachment().then((att) => (attachment = att)); observer?.disconnect(); } });
107-128: Consider consolidating cache-check logic.The
runblock duplicates cache-checking logic that's already infetchAttachment. While the current implementation works correctly, you could simplify by always callingfetchAttachment(which handles caching internally):run(() => { if (mounted && meta.attachment) { - // Check cache first when meta changes - const key = generateAttachmentCacheKey(meta.id, meta.attachment); - if (attachmentCache.has(key)) { - const cached = attachmentCache.get(key); - if (cached) { - attachment = cached; - return; - } - } - - // If visible, fetch immediately if (isVisible) { - fetchAttachment().then((_attachment) => { - attachment = _attachment; - }); + fetchAttachment().then((att) => (attachment = att)); + } else { + // Check if already cached + const key = generateAttachmentCacheKey(meta.id, meta.attachment); + attachment = attachmentCache.get(key); } } else { attachment = undefined; } });This reduces duplication while preserving the optimization of not fetching when not visible.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/lib/components/ModelTable/EvidenceFilePreview.svelte(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: build_community_frontend
- GitHub Check: build_enterprise_frontend
- GitHub Check: Analyze (python)
🔇 Additional comments (3)
frontend/src/lib/components/ModelTable/EvidenceFilePreview.svelte (3)
96-105: LGTM! Proper cleanup with clear documentation.The cleanup logic correctly disconnects the observer, and the comment clearly explains why blob URLs are not revoked here (shared cache responsibility). This properly addresses the previous memory leak concerns.
137-170: LGTM! Good refactoring of the snippet.Parameterizing the
displayPreviewsnippet withatt: Attachmentmakes the dependency explicit and the snippet more reusable. All references are correctly updated.
172-188: LGTM! Clear loading states and error handling.The template properly handles all states: loading, preview available, no preview, and missing attachment. The container binding enables the lazy-loading functionality.
| if ((err as any)?.name === 'AbortError') { | ||
| const miss = { type: '', url: '', fileExists: false } satisfies Attachment; | ||
| return miss; |
There was a problem hiding this comment.
Inconsistent error caching on AbortError.
The AbortError path returns a miss without caching it (lines 58-60), unlike the generic error path which caches the miss (lines 62-64). This inconsistency could trigger unnecessary refetches if the user scrolls away during a fetch and then returns.
However, I notice there's no AbortController in this code. If abort functionality isn't needed, consider removing the special AbortError handling:
} catch (err) {
- if ((err as any)?.name === 'AbortError') {
- const miss = { type: '', url: '', fileExists: false } satisfies Attachment;
- return miss;
- }
const miss = { type: '', url: '', fileExists: false } satisfies Attachment;
attachmentCache.set(cacheKey, miss);
return miss;Alternatively, if you plan to add abort support later, make the caching consistent:
} catch (err) {
if ((err as any)?.name === 'AbortError') {
const miss = { type: '', url: '', fileExists: false } satisfies Attachment;
+ attachmentCache.set(cacheKey, miss);
return miss;
}
const miss = { type: '', url: '', fileExists: false } satisfies Attachment;
attachmentCache.set(cacheKey, miss);
return miss;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ((err as any)?.name === 'AbortError') { | |
| const miss = { type: '', url: '', fileExists: false } satisfies Attachment; | |
| return miss; | |
| } catch (err) { | |
| if ((err as any)?.name === 'AbortError') { | |
| const miss = { type: '', url: '', fileExists: false } satisfies Attachment; | |
| attachmentCache.set(cacheKey, miss); | |
| return miss; | |
| } | |
| const miss = { type: '', url: '', fileExists: false } satisfies Attachment; | |
| attachmentCache.set(cacheKey, miss); | |
| return miss; | |
| } |
🤖 Prompt for AI Agents
In frontend/src/lib/components/ModelTable/EvidenceFilePreview.svelte around
lines 58 to 60, the AbortError branch returns a miss without caching it while
the generic error path caches the miss; either remove the AbortError-specific
branch if abort behavior isn't used (so all errors follow the same cache path),
or update the AbortError path to mirror the generic error handling by creating
the same miss object and writing it to the cache before returning; if you intend
to support aborts later, also add an AbortController to the fetch call and wire
it into the caller so aborts are intentional and handled consistently.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
ab-smith
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
frontend/src/lib/components/ModelTable/EvidenceFilePreview.svelte (3)
57-65: Remove dead AbortError handling or make it consistent.The
AbortErrorcheck is inconsistent (doesn't cache the miss) and is dead code since noAbortControlleris passed tofetch. The network request on lines 41-43 provides nosignalparameter, soAbortErrorwill never be thrown.Apply this diff to remove the dead code path and simplify error handling:
} catch (err) { - if ((err as any)?.name === 'AbortError') { - const miss = { type: '', url: '', fileExists: false } satisfies Attachment; - return miss; - } const miss = { type: '', url: '', fileExists: false } satisfies Attachment; attachmentCache.set(cacheKey, miss); return miss; }Alternatively, if you plan to add abort support later, cache the miss consistently in the
AbortErrorpath as well.
28-37: Consider optimizing cache lookup.The code calls
has()thenget(), resulting in two cache lookups. You can simplify this by callingget()once and checking its result.Apply this diff:
// Generate cache key cacheKey = generateAttachmentCacheKey(meta.id, meta.attachment); // Check cache first - if (attachmentCache.has(cacheKey)) { - const cached = attachmentCache.get(cacheKey); - if (cached) { - return cached; - } + const cached = attachmentCache.get(cacheKey); + if (cached) { + return cached; }
107-128: Consider optimizing cache lookup pattern (same as infetchAttachment).The cache lookup on lines 110-117 uses the same
has()thenget()pattern that can be simplified to a singleget()call.Apply this diff:
if (mounted && meta.attachment) { // Check cache first when meta changes const key = generateAttachmentCacheKey(meta.id, meta.attachment); - if (attachmentCache.has(key)) { - const cached = attachmentCache.get(key); - if (cached) { - attachment = cached; - return; - } + const cached = attachmentCache.get(key); + if (cached) { + attachment = cached; + return; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/src/lib/components/ModelTable/EvidenceFilePreview.svelte(4 hunks)frontend/src/lib/stores/attachmentCache.ts(1 hunks)
🔇 Additional comments (5)
frontend/src/lib/stores/attachmentCache.ts (1)
75-82: LGTM!The singleton export and cache key generator are clean and well-structured.
frontend/src/lib/components/ModelTable/EvidenceFilePreview.svelte (4)
1-26: LGTM!The imports and state setup properly support lazy loading with IntersectionObserver and cache integration. All necessary pieces are in place.
69-94: LGTM!The IntersectionObserver setup is well-implemented with proper lifecycle management. The 50px
rootMarginprovides good UX by preloading slightly before the element enters the viewport, and disconnecting after first load prevents duplicate fetches.
96-105: LGTM!The cleanup logic is correct, and the comment clearly explains why blob URLs are not revoked here. Since the cache is shared across components, per-component revocation would break other instances using the same attachment.
137-188: LGTM!The rendering logic is well-structured. Making
displayPreviewaccept an explicitattparameter is cleaner than relying on closure, and the template properly handles all states (loading, cached, error, no preview).
| remove: (key: string) => { | ||
| update((cache) => { | ||
| if (cache[key]) { | ||
| URL.revokeObjectURL(cache[key].url); | ||
| delete cache[key]; | ||
| } | ||
| return cache; | ||
| }); | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Fix immutability: remove mutates cache in-place.
The remove method uses delete cache[key] and returns the mutated object, inconsistent with set which returns a new object via spread. This breaks the immutability pattern and may cause Svelte reactivity issues.
Apply this diff to use destructuring for immutable removal:
remove: (key: string) => {
update((cache) => {
if (cache[key]) {
URL.revokeObjectURL(cache[key].url);
- delete cache[key];
}
- return cache;
+ const { [key]: _, ...rest } = cache;
+ return rest;
});
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| remove: (key: string) => { | |
| update((cache) => { | |
| if (cache[key]) { | |
| URL.revokeObjectURL(cache[key].url); | |
| delete cache[key]; | |
| } | |
| return cache; | |
| }); | |
| }, | |
| remove: (key: string) => { | |
| update((cache) => { | |
| if (cache[key]) { | |
| URL.revokeObjectURL(cache[key].url); | |
| } | |
| const { [key]: _, ...rest } = cache; | |
| return rest; | |
| }); | |
| }, |
🤖 Prompt for AI Agents
In frontend/src/lib/stores/attachmentCache.ts around lines 45 to 53, the remove
function currently mutates the cache in-place using delete; change it to be
immutable by creating a new object without the removed key (using object
destructuring to omit the key or by shallow-cloning and deleting from the clone)
and return that new object from update; make sure to call URL.revokeObjectURL
for the removed entry's url before returning the new cache so revocation still
occurs but the original cache object is not mutated.
ab-smith
left a comment
ab-smith
left a comment
There was a problem hiding this comment.
how do we invalidate the cache and avoid memory exhaustion?
not sure about the cache keys and management neither.
Summary by CodeRabbit
New Features
Performance
Bug Fixes / Stability