✨ Implement iframe caching mechanism - #5
Conversation
…hedIframePortal components, update ChatItem and StreamItem to utilize caching
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds an iframe caching system (provider, hook, portal) that preserves iframe DOM containers keyed by cacheKey. Integrates caching into GridItem via a cacheKey and container ref; ChatItem/StreamItem supply keys. Wraps the watch route with IframeCacheProvider to enable cache scope. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant WatchRoute as Watch Route
participant Provider as IframeCacheProvider
participant GridItem
participant Portal as CachedIframePortal
participant Cache as Cache Map
participant DOM as Hidden Container Area
User->>WatchRoute: Navigate to /watch
WatchRoute->>Provider: Render children inside provider
GridItem->>Portal: Mount with cacheKey + targetRef
Portal->>Provider: getIframeContainer(cacheKey)
alt container missing
Provider->>DOM: create hidden container
Provider->>Cache: store container by key
end
Provider-->>Portal: return container
Portal->>GridItem: append container into targetRef (mount iframe)
User->>GridItem: Unmount or move item
Portal->>Provider: releaseIframeContainer(cacheKey)
Provider->>DOM: move container back to hidden area (preserve iframe)
Note over Provider,Cache: TTL-based purge will remove stale containers
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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 |
There was a problem hiding this comment.
Pull Request Overview
This PR implements an iframe caching mechanism to prevent iframes from reloading when grid items are moved, resized, or remounted, improving the user experience by maintaining stream and chat state.
- Adds IframeCacheProvider context to manage cached iframe containers
- Introduces CachedIframePortal component to handle iframe portalling and caching
- Updates ChatItem and StreamItem to use the new caching system with unique cache keys
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/routes/watch.tsx | Wraps the Watch component with IframeCacheProvider to enable caching |
| src/components/gridItems/StreamItem/StreamItem.tsx | Adds cache key for stream items and passes it to GridItem |
| src/components/gridItems/GridItem/GridItem.tsx | Integrates CachedIframePortal to wrap iframe with caching functionality |
| src/components/gridItems/ChatItem/ChatItem.tsx | Adds cache key for chat items and passes it to GridItem |
| src/components/IframeCache/IframeCache.tsx | Implements the complete iframe caching system with provider and portal components |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/components/IframeCache/IframeCache.tsx (1)
11-17: Fast refresh limitation acknowledged.The static analysis warning about fast refresh is valid but acceptable here. The file exports both the hook (
useIframeCache) and components (IframeCacheProvider,CachedIframePortal), which can break fast refresh in development.If this becomes problematic, consider splitting into separate files:
IframeCacheContext.tsfor context and hookIframeCacheProvider.tsxfor the provider componentCachedIframePortal.tsxfor the portal componentHowever, the current structure is reasonable given the tight coupling between these exports.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/components/IframeCache/IframeCache.tsx(1 hunks)src/components/gridItems/ChatItem/ChatItem.tsx(1 hunks)src/components/gridItems/GridItem/GridItem.tsx(4 hunks)src/components/gridItems/StreamItem/StreamItem.tsx(1 hunks)src/routes/watch.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/routes/watch.tsx (1)
src/components/IframeCache/IframeCache.tsx (1)
IframeCacheProvider(23-69)
src/components/gridItems/GridItem/GridItem.tsx (2)
src/hooks/useIframeRefresh.ts (1)
useIframeRefresh(3-37)src/components/IframeCache/IframeCache.tsx (1)
CachedIframePortal(77-94)
🪛 GitHub Check: Run Linter
src/components/IframeCache/IframeCache.tsx
[warning] 11-11:
Fast refresh only works when a file only exports components. Use a new file to share constants or functions between components
🔇 Additional comments (5)
src/components/gridItems/StreamItem/StreamItem.tsx (1)
16-16: LGTM!The cache key derivation follows a consistent naming pattern (
stream-${streamName}), matching the approach inChatItem(chat-${streamName}). The multiline formatting improves readability, and the integration with the iframe caching mechanism is correct.Also applies to: 23-30
src/routes/watch.tsx (1)
59-101: LGTM!The
IframeCacheProvideris correctly placed to wrap all grid items, enabling the iframe caching mechanism throughout the layout. The wrapper preserves all existing rendering logic and props.src/components/gridItems/ChatItem/ChatItem.tsx (1)
14-14: LGTM!The cache key derivation is consistent with
StreamItemand correctly distinguishes chat iframes from stream iframes using thechat-prefix.Also applies to: 17-17
src/components/gridItems/GridItem/GridItem.tsx (1)
36-51: Verify the necessity ofkey={iframeSrc}on the iframe.The portal integration is well-structured, but the
key={iframeSrc}prop on line 40 will cause React to unmount and remount the iframe whenever theiframeSrcchanges, even though the cached container persists. This could reduce the effectiveness of the caching mechanism.For example, in
ChatItem, toggling dark theme changes theiframeSrc, which would force a complete iframe remount and reload rather than preserving the iframe state.Consider whether the
keyprop is necessary, or if it should be based on a more stable identifier (e.g.,key={cacheKey}or removed entirely).src/components/IframeCache/IframeCache.tsx (1)
77-94: Portal implementation is correct.The
CachedIframePortalcorrectly:
- Retrieves the cached container for the given key
- Moves the container to the target ref on mount (line 84)
- Releases the container back to the hidden area on unmount (line 89)
- Creates a portal to render children into the container (line 93)
The
appendChildcall safely moves the container even if it's already attached elsewhere in the DOM.
…move stale iframe containers after 10 minutes
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/components/IframeCache/IframeCache.tsx (1)
64-73: Unsafe non-null assertion.Line 72 uses the non-null assertion operator (
!) after checkinghas(key)and setting the value. While logically safe in the current flow, this pattern was flagged in a previous review and remains unaddressed.Apply this diff for defensive programming:
const getIframeContainer = useCallback((key: string): HTMLDivElement => { lastUsedRef.current.set(key, Date.now()); if (!containerMapRef.current.has(key)) { const container = document.createElement('div'); container.style.width = '100%'; container.style.height = '100%'; containerMapRef.current.set(key, container); } - return containerMapRef.current.get(key)!; + const container = containerMapRef.current.get(key); + if (!container) { + throw new Error(`Container for key "${key}" not found in cache.`); + } + return container; }, []);
🧹 Nitpick comments (1)
src/components/IframeCache/IframeCache.tsx (1)
1-17: Fast Refresh limitation with mixed exports.The linter warns that mixing component exports with hook exports in a single file can break Fast Refresh during development. While not a functional issue, it can degrade the developer experience.
Consider splitting into separate files:
src/components/IframeCache/context.tsfor the context and hooksrc/components/IframeCache/IframeCacheProvider.tsxfor the provider componentsrc/components/IframeCache/CachedIframePortal.tsxfor the portal componentsrc/components/IframeCache/index.tsfor re-exports
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/IframeCache/IframeCache.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/components/IframeCache/IframeCache.tsx (3)
src/hooks/useIframeRefresh.ts (5)
useIframeRefresh(3-37)node(6-27)timeoutRef(28-34)node(19-24)timeoutRef(29-33)src/components/gridItems/GridItem/GridItem.tsx (3)
ref(20-46)GridItemRef(14-16)() => ({ refreshIframe })(23-23)src/components/gridItems/StreamItem/StreamItem.tsx (1)
StreamItemProps(9-25)
🪛 GitHub Check: Run Linter
src/components/IframeCache/IframeCache.tsx
[warning] 11-11:
Fast refresh only works when a file only exports components. Use a new file to share constants or functions between components
| const cleanupInterval = setInterval( | ||
| () => { | ||
| const now = Date.now(); | ||
| const CACHE_TTL = 10 * 60 * 1000; // 10 minutes | ||
|
|
||
| for (const [key, lastUsed] of lastUsedRef.current.entries()) { | ||
| if (now - lastUsed > CACHE_TTL) { | ||
| const container = containerMapRef.current.get(key); | ||
| container?.remove(); | ||
| containerMapRef.current.delete(key); | ||
| lastUsedRef.current.delete(key); | ||
| } | ||
| } | ||
| }, | ||
| 5 * 60 * 1000 | ||
| ); |
There was a problem hiding this comment.
Critical: Cleanup may remove actively used containers.
The cleanup interval removes containers based solely on the lastUsed timestamp, without checking whether the container is currently visible (mounted in a target element). If a user keeps a stream open for more than 10 minutes, its container will be removed from the map and DOM while still in use, breaking the UI.
Apply this diff to only evict containers that are in the hidden container:
const cleanupInterval = setInterval(
() => {
const now = Date.now();
const CACHE_TTL = 10 * 60 * 1000; // 10 minutes
for (const [key, lastUsed] of lastUsedRef.current.entries()) {
if (now - lastUsed > CACHE_TTL) {
const container = containerMapRef.current.get(key);
- container?.remove();
- containerMapRef.current.delete(key);
- lastUsedRef.current.delete(key);
+ // Only remove if container is in the hidden area (not actively displayed)
+ if (container && container.parentElement === hiddenContainerRef.current) {
+ container.remove();
+ containerMapRef.current.delete(key);
+ lastUsedRef.current.delete(key);
+ }
}
}
},
5 * 60 * 1000
);📝 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.
| const cleanupInterval = setInterval( | |
| () => { | |
| const now = Date.now(); | |
| const CACHE_TTL = 10 * 60 * 1000; // 10 minutes | |
| for (const [key, lastUsed] of lastUsedRef.current.entries()) { | |
| if (now - lastUsed > CACHE_TTL) { | |
| const container = containerMapRef.current.get(key); | |
| container?.remove(); | |
| containerMapRef.current.delete(key); | |
| lastUsedRef.current.delete(key); | |
| } | |
| } | |
| }, | |
| 5 * 60 * 1000 | |
| ); | |
| const cleanupInterval = setInterval( | |
| () => { | |
| const now = Date.now(); | |
| const CACHE_TTL = 10 * 60 * 1000; // 10 minutes | |
| for (const [key, lastUsed] of lastUsedRef.current.entries()) { | |
| if (now - lastUsed > CACHE_TTL) { | |
| const container = containerMapRef.current.get(key); | |
| // Only remove if container is in the hidden area (not actively displayed) | |
| if (container && container.parentElement === hiddenContainerRef.current) { | |
| container.remove(); | |
| containerMapRef.current.delete(key); | |
| lastUsedRef.current.delete(key); | |
| } | |
| } | |
| } | |
| }, | |
| 5 * 60 * 1000 | |
| ); |
🤖 Prompt for AI Agents
In src/components/IframeCache/IframeCache.tsx around lines 41 to 56, the cleanup
currently evicts containers purely by lastUsed timestamp which can remove
containers that are actively mounted; modify the eviction to also check that the
container is inside the hidden container before removing: obtain the hidden
container element (hiddenContainerRef.current) and only call container.remove()
and delete from maps when container !== undefined AND hiddenContainerRef.current
exists AND container.parentElement === hiddenContainerRef.current (i.e.,
container is currently in the hidden/offscreen pool); keep the existing TTL and
interval logic unchanged.
| export function CachedIframePortal({ cacheKey, targetRef, children }: CachedIframePortalProps) { | ||
| const { getIframeContainer, releaseIframeContainer } = useIframeCache(); | ||
| const container = getIframeContainer(cacheKey); | ||
|
|
||
| useEffect(() => { | ||
| // Move container to target when mounted | ||
| if (targetRef.current) { | ||
| targetRef.current.appendChild(container); | ||
| } | ||
|
|
||
| return () => { | ||
| // Release container when unmounted | ||
| releaseIframeContainer(cacheKey); | ||
| }; | ||
| }, [cacheKey, container, targetRef, releaseIframeContainer]); | ||
|
|
||
| return createPortal(children, container); | ||
| } |
There was a problem hiding this comment.
Critical: Side effects during render and ref tracking issues.
Multiple issues with the portal implementation:
-
Line 99:
getIframeContaineris called during render, not in an effect. This creates side effects during render (updatinglastUsedref) and violates React's rendering model. It should be moved into the effect. -
Effect dependencies: The effect depends on
targetRef, but React doesn't track changes totargetRef.current. If the ref's current value changes after mount, the container won't be re-attached to the new target. -
Null safety: If
targetRef.currentis null on mount, the container won't be appended anywhere, leaving the portal in an inconsistent state.
Apply this diff to fix these issues:
export function CachedIframePortal({ cacheKey, targetRef, children }: CachedIframePortalProps) {
const { getIframeContainer, releaseIframeContainer } = useIframeCache();
- const container = getIframeContainer(cacheKey);
+ const containerRef = useRef<HTMLDivElement | null>(null);
useEffect(() => {
+ // Get or create container inside effect
+ const container = getIframeContainer(cacheKey);
+ containerRef.current = container;
+
// Move container to target when mounted
- if (targetRef.current) {
+ const target = targetRef.current;
+ if (target) {
- targetRef.current.appendChild(container);
+ target.appendChild(container);
+ } else {
+ console.warn(`CachedIframePortal: targetRef.current is null for key "${cacheKey}"`);
}
return () => {
// Release container when unmounted
releaseIframeContainer(cacheKey);
};
}, [cacheKey, targetRef, getIframeContainer, releaseIframeContainer]);
- return createPortal(children, container);
+ return containerRef.current ? createPortal(children, containerRef.current) : null;
}📝 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.
| export function CachedIframePortal({ cacheKey, targetRef, children }: CachedIframePortalProps) { | |
| const { getIframeContainer, releaseIframeContainer } = useIframeCache(); | |
| const container = getIframeContainer(cacheKey); | |
| useEffect(() => { | |
| // Move container to target when mounted | |
| if (targetRef.current) { | |
| targetRef.current.appendChild(container); | |
| } | |
| return () => { | |
| // Release container when unmounted | |
| releaseIframeContainer(cacheKey); | |
| }; | |
| }, [cacheKey, container, targetRef, releaseIframeContainer]); | |
| return createPortal(children, container); | |
| } | |
| export function CachedIframePortal({ cacheKey, targetRef, children }: CachedIframePortalProps) { | |
| const { getIframeContainer, releaseIframeContainer } = useIframeCache(); | |
| const containerRef = useRef<HTMLDivElement | null>(null); | |
| useEffect(() => { | |
| // Get or create container inside effect | |
| const container = getIframeContainer(cacheKey); | |
| containerRef.current = container; | |
| // Move container to target when mounted | |
| const target = targetRef.current; | |
| if (target) { | |
| target.appendChild(container); | |
| } else { | |
| console.warn(`CachedIframePortal: targetRef.current is null for key "${cacheKey}"`); | |
| } | |
| return () => { | |
| // Release container when unmounted | |
| releaseIframeContainer(cacheKey); | |
| }; | |
| }, [cacheKey, targetRef, getIframeContainer, releaseIframeContainer]); | |
| return containerRef.current | |
| ? createPortal(children, containerRef.current) | |
| : null; | |
| } |
🤖 Prompt for AI Agents
In src/components/IframeCache/IframeCache.tsx around lines 97-114, move the call
to getIframeContainer out of render and into a useEffect: create a local state
(or ref) to hold the container and initialize it inside useEffect using
getIframeContainer(cacheKey), then only render the portal when that container
exists. In the same effect (or a separate effect) append the container to
targetRef.current and re-attach if targetRef.current changes by tracking the
actual DOM node (use a ref for previousTarget or a callback ref) so changes to
targetRef.current re-run the attach logic; ensure cleanup releases the container
via releaseIframeContainer(cacheKey) and removes the node from any previous
target to avoid leaks. Also handle the case where targetRef.current is null on
mount by still creating the container and waiting to append it later when
targetRef.current becomes non-null (i.e., perform append on any target change),
keeping createPortal conditional on the container being non-null.
add IframeCacheProvider and CachedIframePortal components, update ChatItem and StreamItem to utilize caching
Summary by CodeRabbit