Conversation
|
@sushilbang is attempting to deploy a commit to the sz47's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughA popstate event listener is added to re-apply the current theme during browser navigation using deferred execution via a zero-delay timeout to ensure proper theme persistence across page transitions. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
js/theme/theme.js (1)
35-46: Fix memory leaks insetThemethat are exacerbated by the new popstate listener.The
setThemefunction has memory leaks that will accumulate more rapidly with the new popstate listener:
Style element leak (lines 35-46): A new
<style>element is created and appended on every call without removing previous ones. With the popstate listener, this accumulates on every navigation.Blob URL leak (lines 50-54, 153):
URL.createObjectURLcreates blob URLs that are never revoked, causing memory leaks. The previous blob URL should be revoked before creating a new one.🔎 Proposed fixes for memory leaks
Fix 1: Reuse or remove old style elements
- const styleSheet = document.createElement('style'); + let styleSheet = document.querySelector('style[data-theme-selection]'); + if (!styleSheet) { + styleSheet = document.createElement('style'); + styleSheet.setAttribute('data-theme-selection', 'true'); + document.head.appendChild(styleSheet); + } styleSheet.textContent = ` ::selection { background-color: var(--fg-accent); color: var(--bg-accent); } ::-moz-selection { background-color: var(--fg-accent); color: var(--bg-accent); } `; - document.head.appendChild(styleSheet);Fix 2: Revoke previous blob URL before creating a new one
const textColor = theme['--fg-1'] || '#000000'; const bgColor = theme['--bg-1'] || '#ffffff'; const favicon = document.querySelector('link[rel="icon"]') || document.createElement('link'); + // Revoke previous blob URL to prevent memory leak + if (favicon.href && favicon.href.startsWith('blob:')) { + URL.revokeObjectURL(favicon.href); + } favicon.rel = 'icon'; favicon.type = 'image/svg+xml'; favicon.href = createThemedFaviconSVG(textColor, bgColor); document.head.appendChild(favicon);Also applies to: 50-54
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
js/theme/theme.js
🧰 Additional context used
🧬 Code graph analysis (1)
js/theme/theme.js (1)
js/wisk.js (1)
wisk(3-3)
| window.addEventListener('popstate', () => { | ||
| setTimeout(() => wisk.theme.setTheme(wisk.theme.getTheme()), 0); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for favicon and popstate references in the codebase
echo "=== Searching for favicon and popstate references ==="
rg -n -C3 'favicon|popstate' --type=js -g '!js/theme/theme.js'Repository: sohzm/wisk
Length of output: 5652
🏁 Script executed:
#!/bin/bash
# Examine the theme.js file to understand the setTheme function and the new popstate listener
echo "=== Checking file size first ==="
wc -l js/theme/theme.js
echo -e "\n=== Examining the popstate listener (lines 155-160) ==="
sed -n '155,160p' js/theme/theme.js
echo -e "\n=== Examining the setTheme function (lines 1-65) ==="
sed -n '1,65p' js/theme/theme.js
echo -e "\n=== Searching for URL.revokeObjectURL to check blob cleanup ==="
rg -n 'revokeObjectURL' js/theme/theme.jsRepository: sohzm/wisk
Length of output: 2582
🏁 Script executed:
#!/bin/bash
# Look for any issue tracking or comments about issue #11
echo "=== Searching for issue #11 references ==="
rg -n '#11|issue.*11' --type=js --type=md
echo -e "\n=== Checking if there are git logs or comments ==="
git log --oneline --all 2>/dev/null | head -20Repository: sohzm/wisk
Length of output: 50367
This approach causes memory leaks and flicker—investigate why the favicon is lost on navigation instead.
The zero-delay setTimeout and the need to re-apply the theme on every popstate event indicate this is a workaround for the underlying issue.
Issues with the current implementation:
- Style element accumulation: Every
setThemecall appends a new<style>element todocument.headwithout cleaning up previous ones, causing memory growth on each navigation. - Blob URL leaks:
createThemedFaviconSVGcreates blob URLs viaURL.createObjectURL(line 153), but there is no correspondingURL.revokeObjectURLcall, leaking memory. - Favicon flicker: Updating the favicon's
hrefon every popstate event forces the browser to reload the favicon, causing visible flicker.
Instead of re-applying the theme on navigation, determine why the favicon is lost in the first place. Is the browser naturally handling favicon persistence? Is there a DOM mutation or style reset happening elsewhere that this listener is trying to mask?
🤖 Prompt for AI Agents
In js/theme/theme.js around lines 157 to 159, the popstate + zero-delay
setTimeout is a workaround causing style element accumulation, blob URL leaks,
and favicon flicker; remove or disable the popstate handler and instead make
setTheme idempotent by: when applying a theme, replace/cleanup any previously
injected <style> (use a stable id or data-attribute and remove the old element
before appending the new one) and when creating a themed favicon revoke any
previous blob URL via URL.revokeObjectURL before assigning a new href (store the
current blob URL on the favicon element or module state), and only update the
favicon when the theme actually changes; also investigate why navigation is
losing the favicon (e.g., head re-render or router logic) rather than
re-applying the theme on every popstate.
1 participant
solves the problem but has a flicker.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.