Conversation
Summary of Changes
Problems and fixes
Outdated DOM selectors
Added multiple fallback selectors for modern Spotify
addEventListener("onprogress") removed
Replaced with setInterval polling every 200ms
Spicetify.Player.seek(percent) wrong input
Now uses seek(start * duration) (milliseconds)
Context menu styling missing
Added self-contained CSS (no longer relies on Spotify's main-contextMenu-* classes)
position: absolute on context menu
Changed to position: fixed with left/top for reliable positioning
Bar not relatively positioned
Explicitly set bar.style.position = "relative"
It looks good and is futureproof, won't break with an update.
📝 WalkthroughWalkthroughThe loopyLoop extension was refactored to use multi-selector fallback strategies for DOM discovery, replaced deprecated Spicetify event listeners with polling logic, reworked context menu rendering with plain DOM elements, and enhanced CSS styling for loop markers and menu items. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Extensions/loopyLoop.js`:
- Around line 111-129: The current fixed 1000ms debounce (debounceUntil) after
calling Spicetify.Player.seek causes short loops to miss boundary checks;
replace the fixed timeout with a seek-completion guard: when you call
Spicetify.Player.seek(seekTo) set a flag (e.g., isSeeking or record
lastSeekTarget) and in the setInterval loop ignore checks only while the player
is still reaching that seek target, then clear the flag once
Spicetify.Player.getProgress()/percent is within a small epsilon of the seek
target (or when a single interval tick passes), so use that seek-completion
condition instead of debounceUntil = now + 1000; reference debounceUntil,
setInterval, start, end, percent, progress, duration and Spicetify.Player.seek
in your change.
- Around line 149-153: When setting the loop start in the startBtn handler,
clamp start so it can never equal 1 (e.g., start = Math.min(mouseOnBarPercent,
0.9999)) and then handle the ordering: if end === null set end = 0.99, otherwise
if start >= end treat it as the reorder case by swapping start and end instead
of forcing end to 0.99; this prevents start == 1 from causing a seek to duration
and ensures start < end. Reference: startBtn/createMenuItem handler, variables
start, end, and mouseOnBarPercent (and the seek behavior invoked elsewhere).
- Around line 23-29: The initialization guard in LoopyLoop currently only checks
for Spicetify.Player.origin but not Spicetify.Player.origin._state, which allows
the interval to start and later dereference origin._state in
Spicetify.Player.getDuration/getProgress, causing null derefs; update the
startup condition inside LoopyLoop to also require
Spicetify.Player.origin._state (e.g., check Spicetify?.Player?.origin?._state)
before proceeding, preserve the existing document.readyState-based backoff
(setTimeout with 300ms/1000ms) when the state is missing, and only start the
interval/initialization when origin._state is truthy so subsequent calls to
getDuration/getProgress are safe.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f74c6ca0-7ea2-4225-8012-f6a8c7ceffe4
📒 Files selected for processing (1)
Extensions/loopyLoop.js
| if (!(bar && progressContainer && Spicetify?.Player?.origin && Spicetify?.Player?.getDuration)) { | ||
| if (document.readyState !== "complete") { | ||
| setTimeout(LoopyLoop, 300); | ||
| return; | ||
| } | ||
| setTimeout(LoopyLoop, 1000); | ||
| return; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "spicetifyWrapper.js" | head -5Repository: spicetify/cli
Length of output: 87
🏁 Script executed:
find . -type f -name "loopyLoop.js" | head -5Repository: spicetify/cli
Length of output: 82
🏁 Script executed:
find . -type f -name "*.js" | xargs grep -l "getProgress\|getDuration" | head -10Repository: spicetify/cli
Length of output: 261
🏁 Script executed:
sed -n '1,50p' ./jsHelper/spicetifyWrapper.jsRepository: spicetify/cli
Length of output: 1754
🏁 Script executed:
sed -n '20,40p' ./jsHelper/spicetifyWrapper.jsRepository: spicetify/cli
Length of output: 883
🏁 Script executed:
sed -n '1440,1480p' ./jsHelper/spicetifyWrapper.jsRepository: spicetify/cli
Length of output: 1207
🏁 Script executed:
sed -n '1,80p' ./Extensions/loopyLoop.jsRepository: spicetify/cli
Length of output: 2593
🏁 Script executed:
sed -n '30,150p' ./Extensions/loopyLoop.js | head -100Repository: spicetify/cli
Length of output: 2821
🏁 Script executed:
grep -n "getDuration\|getProgress" ./Extensions/loopyLoop.jsRepository: spicetify/cli
Length of output: 303
Guard on origin._state before starting the loop machinery.
Line 23 only waits for Spicetify.Player.origin, but getDuration() and getProgress() called at lines 117–118 directly dereference origin._state.duration, origin._state.isPaused, and origin._state.timestamp without checks. The wrapper itself at lines 1450–1475 explicitly gates on origin._state before initializing related APIs. If origin exists but _state is not yet populated, the initialization passes and the interval executes, leaving a null-deref window.
Suggested fix
- if (!(bar && progressContainer && Spicetify?.Player?.origin && Spicetify?.Player?.getDuration)) {
+ if (!(bar && progressContainer && Spicetify?.Player?.origin?._state && Spicetify?.Player?.getDuration)) {📝 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 (!(bar && progressContainer && Spicetify?.Player?.origin && Spicetify?.Player?.getDuration)) { | |
| if (document.readyState !== "complete") { | |
| setTimeout(LoopyLoop, 300); | |
| return; | |
| } | |
| setTimeout(LoopyLoop, 1000); | |
| return; | |
| if (!(bar && progressContainer && Spicetify?.Player?.origin?._state && Spicetify?.Player?.getDuration)) { | |
| if (document.readyState !== "complete") { | |
| setTimeout(LoopyLoop, 300); | |
| return; | |
| } | |
| setTimeout(LoopyLoop, 1000); | |
| return; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Extensions/loopyLoop.js` around lines 23 - 29, The initialization guard in
LoopyLoop currently only checks for Spicetify.Player.origin but not
Spicetify.Player.origin._state, which allows the interval to start and later
dereference origin._state in Spicetify.Player.getDuration/getProgress, causing
null derefs; update the startup condition inside LoopyLoop to also require
Spicetify.Player.origin._state (e.g., check Spicetify?.Player?.origin?._state)
before proceeding, preserve the existing document.readyState-based backoff
(setTimeout with 300ms/1000ms) when the state is missing, and only start the
interval/initialization when origin._state is truthy so subsequent calls to
getDuration/getProgress are safe.
| let debounceUntil = 0; | ||
| setInterval(() => { | ||
| if (start != null && end != null && Spicetify.Player.isPlaying()) { | ||
| const now = Date.now(); | ||
| if (now < debounceUntil) return; | ||
|
|
||
| const progress = Spicetify.Player.getProgress(); // ms | ||
| const duration = Spicetify.Player.getDuration(); // ms | ||
| if (!duration) return; | ||
|
|
||
| const percent = progress / duration; | ||
|
|
||
| if (percent > end || percent < start - 0.01) { | ||
| debounceUntil = now + 1000; | ||
| const seekTo = Math.round(start * duration); | ||
| Spicetify.Player.seek(seekTo); | ||
| } | ||
| } | ||
| }, 200); |
There was a problem hiding this comment.
The fixed 1s debounce breaks short loops.
Line 124 suppresses boundary checks for a full second after every seek. Any practice loop shorter than that will only wrap once, then playback runs past end unchecked on the next pass. Please replace the fixed 1000 ms mute window with a one-tick or seek-completion guard.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Extensions/loopyLoop.js` around lines 111 - 129, The current fixed 1000ms
debounce (debounceUntil) after calling Spicetify.Player.seek causes short loops
to miss boundary checks; replace the fixed timeout with a seek-completion guard:
when you call Spicetify.Player.seek(seekTo) set a flag (e.g., isSeeking or
record lastSeekTarget) and in the setInterval loop ignore checks only while the
player is still reaching that seek target, then clear the flag once
Spicetify.Player.getProgress()/percent is within a small epsilon of the seek
target (or when a single interval tick passes), so use that seek-completion
condition instead of debounceUntil = now + 1000; reference debounceUntil,
setInterval, start, end, percent, progress, duration and Spicetify.Player.seek
in your change.
| const startBtn = createMenuItem("Set loop start", () => { | ||
| start = mouseOnBarPercent; | ||
| if (end === null || start > end) { | ||
| end = 0.99; | ||
| } |
There was a problem hiding this comment.
Keep start < end at the 100% boundary.
Line 150 can store start = 1, and Line 152 then sets end = 0.99, leaving an inverted loop window. From there Line 125 seeks to duration instead of a valid loop start. Clamp the start point below 1 and treat start >= end as the reorder case.
Suggested fix
const startBtn = createMenuItem("Set loop start", () => {
- start = mouseOnBarPercent;
- if (end === null || start > end) {
+ start = Math.min(mouseOnBarPercent, 0.98);
+ if (end === null || start >= end) {
end = 0.99;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Extensions/loopyLoop.js` around lines 149 - 153, When setting the loop start
in the startBtn handler, clamp start so it can never equal 1 (e.g., start =
Math.min(mouseOnBarPercent, 0.9999)) and then handle the ordering: if end ===
null set end = 0.99, otherwise if start >= end treat it as the reorder case by
swapping start and end instead of forcing end to 0.99; this prevents start == 1
from causing a seek to duration and ensures start < end. Reference:
startBtn/createMenuItem handler, variables start, end, and mouseOnBarPercent
(and the seek behavior invoked elsewhere).
|
Bruh #3771 |
I didn't see it. 😂 |
3 participants
Summary of Changes
Problems and fixes
Outdated DOM selectors
Added multiple fallback selectors for modern Spotify
addEventListener("onprogress") removed
Replaced with setInterval polling every 200ms
Spicetify.Player.seek(percent) wrong input
Now uses seek(start * duration) (milliseconds)
Context menu styling missing
Added self-contained CSS (no longer relies on Spotify's main-contextMenu-* classes)
position: absolute on context menu
Changed to position: fixed with left/top for reliable positioning
Bar not relatively positioned
Explicitly set bar.style.position = "relative"
It looks good and is futureproof, won't break with an update.
Summary by CodeRabbit
Bug Fixes
Style