fix(popupLyrics): fix lyrics not syncing when minimized#3692
fix(popupLyrics): fix lyrics not syncing when minimized#3692Nordara wants to merge 2 commits intospicetify:mainfrom
Conversation
📝 WalkthroughWalkthroughThe tick function in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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
🤖 Fix all issues with AI agents
In `@Extensions/popupLyrics.js`:
- Around line 820-822: Indentation at the setTimeout call in the else branch
(the lines containing "timeout = setTimeout(tick, 16.66, options);") uses spaces
and is inconsistent with the file's tab-based formatting; update the indentation
for those lines to use tabs (matching surrounding blocks) so the else block
aligns with the enclosing function/branch (reference symbols: timeout,
setTimeout, tick, options) to satisfy the formatter/CI.
- Around line 818-822: The hidden-document branch schedules ticks both via the
worker setInterval (worker -> postMessage -> tick) and via a setTimeout fallback
inside tick, causing potential duplicate near-simultaneous invocations; either
remove the setTimeout fallback call in tick (cleanup the else branch that calls
setTimeout) if the worker interval is now reliable, or if intentional keep both
then add a clear explanatory comment above this block referencing the worker's
setInterval and the "Fix lyrics not syncing when minimized" commit so future
readers understand why both mechanisms are needed; update the code in
popupLyrics.js around the requestAnimationFrame/else branch and the tick
function accordingly.
Extensions/popupLyrics.js
Outdated
| if (!document.hidden) { | ||
| requestAnimationFrame(() => tick(options)); | ||
| } | ||
| } else { | ||
| timeout = setTimeout(tick, 16.66, options); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check worker initialization and message handler
echo "=== Worker initialization and setup (lines 8-27) ==="
sed -n '8,27p' Extensions/popupLyrics.js
echo ""
echo "=== Worker message handler (around lines 40-44) ==="
sed -n '35,50p' Extensions/popupLyrics.js
echo ""
echo "=== Visibility change handler (around lines 818-822) ==="
sed -n '810,830p' Extensions/popupLyrics.js
echo ""
echo "=== Timeout clearing logic (around line 790) ==="
sed -n '785,795p' Extensions/popupLyrics.js
echo ""
echo "=== Check if worker is ever stopped or terminated ==="
rg -n 'worker\.(terminate|stop)' Extensions/popupLyrics.jsRepository: spicetify/cli
Length of output: 2181
🏁 Script executed:
#!/bin/bash
# Get complete visibility change handler
echo "=== Complete visibility change handler ==="
sed -n '52,75p' Extensions/popupLyrics.js
echo ""
echo "=== Check if worker is ever stopped (broader search) ==="
rg -n 'popup-lyric-stop-update\|workerIsRunning\|worker\.post' Extensions/popupLyrics.js | head -20Repository: spicetify/cli
Length of output: 677
🏁 Script executed:
#!/bin/bash
# Let me verify the exact sequence - check if setTimeout is scheduled BEFORE or AFTER worker starts
echo "=== Check exact line numbers for visibility handler and worker start ==="
rg -n 'document.addEventListener.*visibilitychange' Extensions/popupLyrics.js
echo ""
echo "=== Lines around worker creation to see timing ==="
sed -n '40,60p' Extensions/popupLyrics.js
echo ""
echo "=== Verify tick function signature and when timeouts are set ==="
rg -n 'function tick\|async function tick' Extensions/popupLyrics.jsRepository: spicetify/cli
Length of output: 774
🏁 Script executed:
#!/bin/bash
# Check git log or comments around this code to understand intent
echo "=== Check for comments near the new setTimeout code ==="
sed -n '800,825p' Extensions/popupLyrics.js
echo ""
echo "=== Look for any PR/issue context or TODOs ==="
rg -n 'TODO\|FIXME\|NOTE\|XXX' Extensions/popupLyrics.js | grep -i -E 'worker|timeout|hidden|tick'Repository: spicetify/cli
Length of output: 857
🏁 Script executed:
#!/bin/bash
# Check if there's a recent change or history
echo "=== Check recent commits mentioning this file ==="
git log --oneline -20 -- Extensions/popupLyrics.js 2>/dev/null | head -10
echo ""
echo "=== Verify the exact flow: can setTimeout AND worker.onmessage both fire at same interval? ==="
echo "Worker posts every 16.66ms via setInterval"
echo "setTimeout also schedules next tick at 16.66ms"
echo ""
echo "When document is hidden:"
echo "1. Worker's setInterval fires → postMessage → worker.onmessage calls tick()"
echo "2. Inside tick(): if hidden, schedules setTimeout(tick, 16.66)"
echo "3. At START of next tick(): clearTimeout clears pending timeout"
echo ""
echo "=== Check if there's any guard preventing worker AND setTimeout from both running ==="
sed -n '40,60p' Extensions/popupLyrics.jsRepository: spicetify/cli
Length of output: 1122
Clarify the intent of dual tick scheduling mechanisms when document is hidden.
The code uses two timing mechanisms when hidden: the Web Worker (setInterval at 16.66ms) posts messages that trigger tick(), and tick() itself schedules a setTimeout fallback at 16.66ms. While clearTimeout() at the start of tick() prevents timeout accumulation, both mechanisms can still queue tick invocations in close succession.
The commit "Fix lyrics not syncing when minimized" suggests this redundancy was intentional to address reliability issues. If so, consider documenting why both are necessary. If the worker alone is now sufficient, the setTimeout fallback can be removed to simplify the code.
🧰 Tools
🪛 GitHub Actions: Code quality
[error] 819-820: Biome: File content differs from formatting output. Run 'biome format' to fix code style issues in this file.
🤖 Prompt for AI Agents
In `@Extensions/popupLyrics.js` around lines 818 - 822, The hidden-document branch
schedules ticks both via the worker setInterval (worker -> postMessage -> tick)
and via a setTimeout fallback inside tick, causing potential duplicate
near-simultaneous invocations; either remove the setTimeout fallback call in
tick (cleanup the else branch that calls setTimeout) if the worker interval is
now reliable, or if intentional keep both then add a clear explanatory comment
above this block referencing the worker's setInterval and the "Fix lyrics not
syncing when minimized" commit so future readers understand why both mechanisms
are needed; update the code in popupLyrics.js around the
requestAnimationFrame/else branch and the tick function accordingly.
|
|
2 participants
use setTimeout fallback when document is hidden to keep tick running
Summary by CodeRabbit