Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughConverts loopyLoop from a simple loop helper into a marker tool with persistent per-track start/end markers and skip zones, robust bar reattachment and redraw, document-level context-menu and toolbar triggers, and onprogress-driven playback control (seek/next/skip) with persisted state per track. Changes
Sequence DiagramsequenceDiagram
actor User
participant UI as "Progress Bar / Menu"
participant State as "Marker State Manager"
participant Storage as "LocalStorage"
participant Player as "Playback Engine"
User->>UI: Right-click bar or marker (open menu)
UI->>UI: Determine target (start/end/skip/none)
User->>UI: Select action (set/move/remove/define skip)
UI->>State: update markers for current trackURI
State->>Storage: saveState(trackURI, {start,end,skipZones})
Storage-->>State: ack
Note over Player,State: On progress events
Player->>State: report position, trackURI
alt inside skip zone
State-->>Player: seek to zone.end (throttled)
else pos < start
State-->>Player: seek(start)
else pos >= end
State-->>Player: next()
else prev-double-press near start
Player->>Player: back()
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related PRs
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
🧹 Nitpick comments (4)
Extensions/loopyLoop.js (4)
120-124: Consider localStorage cleanup for rarely-used entries.Each song URI creates a new localStorage entry (
loopyLoop:${uri}). Over time, this could accumulate significant storage for users who set markers on many songs. Consider adding a "Clear all saved markers" option or implementing LRU eviction for entries older than a certain threshold.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Extensions/loopyLoop.js` around lines 120 - 124, The saveState function stores per-track data under keys like `loopyLoop:${uri}` without cleanup; add a cleanup mechanism and UI entry: update saveState to include a timestamp (e.g., savedAt) in the stored object, implement a new function (e.g., clearOldMarkers or clearAllSavedMarkers) that scans localStorage keys with prefix `loopyLoop:` and either (a) removes entries older than a configurable threshold (LRU by savedAt) or (b) exposes a one-click "Clear all saved markers" action in your extension menu to delete all `loopyLoop:` keys, and wire that function into the extension's settings/menu so users can run it.
282-290: Potential loop vs. advance-to-next ambiguity with Spotify's repeat mode.When
endis reached, this code always callsnext()and setsseekStartPendingUrifor repeat-mode handling. However, if the user has Spotify's repeat-one enabled, callingnext()triggers Spotify's own restart, and then the pending seek fires. This dual behavior (plugin + native repeat) might confuse users expecting pure native repeat behavior.The PR description mentions "ability for the user to use Spotify's repeat instead of the plugin to loop" — ensure this interaction is documented or consider disabling the end-marker enforcement when native repeat-one is detected.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Extensions/loopyLoop.js` around lines 282 - 290, The end-marker enforcement always calls Spicetify.Player.next() and sets seekStartPendingUri when percent >= end, which conflicts with Spotify's native repeat-one; update the handler that contains end, percent, lastNextCall, seekStartPendingUri and the call to Spicetify.Player.next() to first detect native repeat-one (via the player repeat API or state, e.g., Spicetify.Player.getRepeatMode() or equivalent) and if repeat-one is active either skip calling next() and skip setting seekStartPendingUri or respect a user setting (useNativeRepeat) to let native repeat handle the loop; ensure the same 2s debounce (lastNextCall) remains applied when you do call next().
365-371: Consider notifying user when skip zone limit is reached.The 10-zone limit silently prevents adding more zones. Users may be confused when their action appears to do nothing.
💡 Suggested notification
const s = Math.min(pendingSkipStart, mouseOnBarPercent); const e = Math.max(pendingSkipStart, mouseOnBarPercent); - if (e > s && skipZones.length < 10) { + if (e <= s) { + Spicetify.showNotification("Skip zone must have non-zero length"); + } else if (skipZones.length >= 10) { + Spicetify.showNotification("Maximum 10 skip zones reached"); + } else { skipZones.push({ start: s, end: e }); saveState(); drawSkipMarkers(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Extensions/loopyLoop.js` around lines 365 - 371, The code silently ignores adding a new skip zone when skipZones.length >= 10; update the block that handles pendingSkipStart/mouseOnBarPercent to notify the user when the 10-zone limit is reached: keep the existing logic that computes s/e and adds the zone and calls saveState() and drawSkipMarkers() when skipZones.length < 10, but add an else branch that calls a UI notification helper (e.g., notifyUser or showToast—create one if none exists) to inform the user that the maximum of 10 skip zones has been reached; reference pendingSkipStart, mouseOnBarPercent, skipZones, saveState(), and drawSkipMarkers() when making the change.
249-270: Prev-button detection heuristic may trigger on manual seeks.The detection relies on observing a jump from >3s to near-zero, which is how Spotify's prev button behaves. However, if a user manually scrubs from mid-song to the beginning, this will also trigger the "first prev press" behavior, potentially causing unexpected seeks to the start marker.
Consider adding a flag or using Spicetify platform events if available to distinguish user seeks from prev-button presses.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Extensions/loopyLoop.js` around lines 249 - 270, The prev-button heuristic (using prevProgressPercent, percent, prevPressedAt) can misfire on manual scrubs; modify the logic to ignore the heuristic when a recent user-initiated seek was detected by adding a manualSeek flag (or subscription) that is set when the player emits a seek/user-seek event and cleared after a short timeout (~1–2s). In practice: add a listener for Spicetify.Player seek/user-seek events to set manualSeek = true and clear it with setTimeout, then wrap the existing prev-detection if-block with a check like if (!manualSeek) { ... } so Spicetify.Player.seek(start) / Spicetify.Player.back() and prevPressedAt/prevProgressPercent logic only run when the seek was not a manual scrub.
🤖 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 213-218: The code accesses skipZones[activeZoneIndex] when
activeMarkerType is "zoneStart" or "zoneEnd" without validating activeZoneIndex;
add a defensive bounds check before either branch (e.g., ensure
Number.isInteger(activeZoneIndex) and activeZoneIndex >= 0 and activeZoneIndex <
skipZones.length) and early-return or fallback if invalid to avoid runtime
errors, then call drawSkipMarkers() only when a valid zone was updated;
reference activeMarkerType, activeZoneIndex, skipZones, and drawSkipMarkers when
locating where to add this check.
- Around line 292-303: Skip-zone throttle (lastSkipSeek) can prevent immediate
re-seeking for short zones or when jumping into a different zone; reduce the
1000ms throttle and track the last-skipped zone so different zones can be
re-skipped immediately. Locate the skip loop using skipZones, lastSkipSeek,
Spicetify.Player.seek, percent and event.timeStamp; change the throttle to a
smaller value (e.g., 100–250ms) and add a lastSkippedZone identifier (index or
start/end) that is compared inside the loop so if the current zone !=
lastSkippedZone you allow an immediate seek and update lastSkippedZone and
lastSkipSeek, otherwise enforce the throttle to avoid rapid repeated seeks.
Ensure lastSkippedZone is cleared/updated when percent leaves all skip zones so
future entries re-evaluate properly.
---
Nitpick comments:
In `@Extensions/loopyLoop.js`:
- Around line 120-124: The saveState function stores per-track data under keys
like `loopyLoop:${uri}` without cleanup; add a cleanup mechanism and UI entry:
update saveState to include a timestamp (e.g., savedAt) in the stored object,
implement a new function (e.g., clearOldMarkers or clearAllSavedMarkers) that
scans localStorage keys with prefix `loopyLoop:` and either (a) removes entries
older than a configurable threshold (LRU by savedAt) or (b) exposes a one-click
"Clear all saved markers" action in your extension menu to delete all
`loopyLoop:` keys, and wire that function into the extension's settings/menu so
users can run it.
- Around line 282-290: The end-marker enforcement always calls
Spicetify.Player.next() and sets seekStartPendingUri when percent >= end, which
conflicts with Spotify's native repeat-one; update the handler that contains
end, percent, lastNextCall, seekStartPendingUri and the call to
Spicetify.Player.next() to first detect native repeat-one (via the player repeat
API or state, e.g., Spicetify.Player.getRepeatMode() or equivalent) and if
repeat-one is active either skip calling next() and skip setting
seekStartPendingUri or respect a user setting (useNativeRepeat) to let native
repeat handle the loop; ensure the same 2s debounce (lastNextCall) remains
applied when you do call next().
- Around line 365-371: The code silently ignores adding a new skip zone when
skipZones.length >= 10; update the block that handles
pendingSkipStart/mouseOnBarPercent to notify the user when the 10-zone limit is
reached: keep the existing logic that computes s/e and adds the zone and calls
saveState() and drawSkipMarkers() when skipZones.length < 10, but add an else
branch that calls a UI notification helper (e.g., notifyUser or showToast—create
one if none exists) to inform the user that the maximum of 10 skip zones has
been reached; reference pendingSkipStart, mouseOnBarPercent, skipZones,
saveState(), and drawSkipMarkers() when making the change.
- Around line 249-270: The prev-button heuristic (using prevProgressPercent,
percent, prevPressedAt) can misfire on manual scrubs; modify the logic to ignore
the heuristic when a recent user-initiated seek was detected by adding a
manualSeek flag (or subscription) that is set when the player emits a
seek/user-seek event and cleared after a short timeout (~1–2s). In practice: add
a listener for Spicetify.Player seek/user-seek events to set manualSeek = true
and clear it with setTimeout, then wrap the existing prev-detection if-block
with a check like if (!manualSeek) { ... } so Spicetify.Player.seek(start) /
Spicetify.Player.back() and prevPressedAt/prevProgressPercent logic only run
when the seek was not a manual scrub.
🪄 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: 622293c7-7d7b-4708-971a-12e9cdcb04e2
📒 Files selected for processing (1)
Extensions/loopyLoop.js
…tification, formatting
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Extensions/loopyLoop.js (1)
536-543: Consider usinggetBar()helper and explicit radix forparseInt.Minor improvements:
- Line 536:
parseIntwithout radix - while it defaults to 10 for decimal strings, explicit radix is clearer- Lines 538-543: Duplicates the DOM query logic that
getBar()already provides♻️ Suggested cleanup
- const zIdx = parseInt(target.getAttribute("data-zone-index")); - const side = target.getAttribute("data-zone-side") === "end" ? "zoneEnd" : "zoneStart"; - const smContainer = document.querySelector(".playback-progressbar-container"); - const smBar = smContainer?.querySelector('input[type="range"]')?.closest("label")?.nextElementSibling; - if (smBar) { + const zIdx = parseInt(target.getAttribute("data-zone-index"), 10); + const side = target.getAttribute("data-zone-side") === "end" ? "zoneEnd" : "zoneStart"; + const smBar = getBar(); + if (smBar) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Extensions/loopyLoop.js` around lines 536 - 543, Change parseInt(target.getAttribute("data-zone-index")) to include an explicit radix (e.g., parseInt(..., 10)) and replace the duplicated DOM query for the small progress bar with the existing getBar() helper: call getBar() to obtain the bar element, use its getBoundingClientRect() to compute mouseOnBarPercent, and keep the fallback logic the same; reference variables/functions: target, data-zone-index, getBar(), mouseOnBarPercent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Extensions/loopyLoop.js`:
- Around line 536-543: Change parseInt(target.getAttribute("data-zone-index"))
to include an explicit radix (e.g., parseInt(..., 10)) and replace the
duplicated DOM query for the small progress bar with the existing getBar()
helper: call getBar() to obtain the bar element, use its getBoundingClientRect()
to compute mouseOnBarPercent, and keep the fallback logic the same; reference
variables/functions: target, data-zone-index, getBar(), mouseOnBarPercent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7d811351-d72c-42e7-bbd6-aad9a7d117a7
📒 Files selected for processing (1)
Extensions/loopyLoop.js
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Extensions/loopyLoop.js (1)
127-141:⚠️ Potential issue | 🟠 MajorSanitize persisted marker/zone data before using it.
loadState()trusts storage payloads directly (Line 138–Line 140). Corrupted or stale values (e.g., non-numeric, out of[0,1], reversed ranges) can lead to invalid seeks and unstable marker positions.🔧 Proposed hardening
function loadState() { const uri = Spicetify.Player.data?.item?.uri; start = null; end = null; skipZones = []; pendingSkipStart = null; if (!uri) return; try { const saved = Spicetify.LocalStorage.get(`loopyLoop:${uri}`); if (saved) { const data = JSON.parse(saved); - start = data.start ?? null; - end = data.end ?? null; - skipZones = Array.isArray(data.skipZones) ? data.skipZones : []; + const clamp01 = (v) => { + const n = Number(v); + return Number.isFinite(n) ? Math.max(0, Math.min(1, n)) : null; + }; + start = clamp01(data.start); + end = clamp01(data.end); + if (start !== null && end !== null && start >= end) { + start = null; + end = null; + } + skipZones = Array.isArray(data.skipZones) + ? data.skipZones + .map((z) => ({ start: clamp01(z?.start), end: clamp01(z?.end) })) + .filter((z) => z.start !== null && z.end !== null && z.end > z.start) + .slice(0, 10) + : []; } } catch (_) {} }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Extensions/loopyLoop.js` around lines 127 - 141, loadState() currently trusts JSON from Spicetify.LocalStorage.get and assigns start, end, skipZones and pendingSkipStart directly; validate and sanitize those values after JSON.parse: ensure start and end are numbers within [0,1] (or set to null), enforce start <= end (swap or nullify if reversed), validate pendingSkipStart is a number in [0,1] (or null), and ensure skipZones is an array of objects with numeric start/end in [0,1] and start < end (filter out or clamp invalid entries). Apply these checks where loadState assigns start/end/skipZones/pendingSkipStart so corrupted or stale storage cannot produce invalid seeks or marker ranges.
🤖 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 475-479: scheduleMoveHide currently overwrites moveHideTimer
allowing a previous timeout to still fire and race; modify scheduleMoveHide to
clear any existing timer before creating a new one (use clearTimeout on
moveHideTimer) so only the latest scheduled hide for moveSubmenu will run, and
then assign the new timer to moveHideTimer; reference the scheduleMoveHide
function and the moveHideTimer variable when making the change.
---
Duplicate comments:
In `@Extensions/loopyLoop.js`:
- Around line 127-141: loadState() currently trusts JSON from
Spicetify.LocalStorage.get and assigns start, end, skipZones and
pendingSkipStart directly; validate and sanitize those values after JSON.parse:
ensure start and end are numbers within [0,1] (or set to null), enforce start <=
end (swap or nullify if reversed), validate pendingSkipStart is a number in
[0,1] (or null), and ensure skipZones is an array of objects with numeric
start/end in [0,1] and start < end (filter out or clamp invalid entries). Apply
these checks where loadState assigns start/end/skipZones/pendingSkipStart so
corrupted or stale storage cannot produce invalid seeks or marker ranges.
🪄 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: 854dd35c-4433-4d61-b4c0-4ac569fbb0be
📒 Files selected for processing (1)
Extensions/loopyLoop.js
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
Extensions/loopyLoop.js (2)
536-544:⚠️ Potential issue | 🟡 MinorValidate parsed skip-zone index before activating marker context.
parseIntcan yieldNaN; passing that through leavesactiveZoneIndexinvalid and can later crash zone move/remove flows.🛠️ Suggested fix
- const zIdx = parseInt(target.getAttribute("data-zone-index"), 10); + const zIdx = Number.parseInt(target.getAttribute("data-zone-index"), 10); + if (!Number.isInteger(zIdx) || zIdx < 0 || zIdx >= skipZones.length) return; const side = target.getAttribute("data-zone-side") === "end" ? "zoneEnd" : "zoneStart";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Extensions/loopyLoop.js` around lines 536 - 544, The parsed zone index zIdx (from parseInt on target.getAttribute("data-zone-index")) must be validated before calling setupActiveMarker and openContextMenu; update the handler to check that zIdx is a finite integer (e.g., !Number.isNaN(zIdx) and Number.isFinite(zIdx)) and optionally within expected bounds, and if invalid simply bail (return) or skip calling setupActiveMarker(side, zIdx) and openContextMenu(event.clientX, event.clientY) to avoid setting an invalid activeZoneIndex and crashing later flows.
475-479:⚠️ Potential issue | 🟡 MinorCancel stale hide timers before scheduling a new submenu hide.
Older pending timers can still fire and hide the submenu after hover re-entry.
🛠️ Suggested fix
function scheduleMoveHide() { + cancelMoveHide(); moveHideTimer = setTimeout(() => { moveSubmenu.hidden = true; + moveHideTimer = null; }, 150); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Extensions/loopyLoop.js` around lines 475 - 479, scheduleMoveHide currently starts a new timeout without cancelling any existing one, allowing stale timers to hide moveSubmenu after hover re-entry; update scheduleMoveHide to clear the previous timer (clearTimeout(moveHideTimer)) before assigning a new setTimeout and optionally reset moveHideTimer to null when completing, referencing the scheduleMoveHide function and the moveHideTimer and moveSubmenu identifiers so the existing pending timeout is cancelled before scheduling a new hide.
🧹 Nitpick comments (1)
Extensions/loopyLoop.js (1)
135-141: Sanitize persisted marker payload before applying it.Loaded
start/end/skipZonesare used as trusted numbers; malformed LocalStorage can produce invalid comparisons/seeks/marker positions.♻️ Suggested hardening
if (saved) { const data = JSON.parse(saved); - start = data.start ?? null; - end = data.end ?? null; - skipZones = Array.isArray(data.skipZones) ? data.skipZones : []; + const norm = (v) => (Number.isFinite(v) ? Math.max(0, Math.min(1, v)) : null); + start = norm(data.start); + end = norm(data.end); + if (start !== null && end !== null && start >= end) { + start = null; + end = null; + } + skipZones = Array.isArray(data.skipZones) + ? data.skipZones + .map((z) => ({ start: norm(z?.start), end: norm(z?.end) })) + .filter((z) => z.start !== null && z.end !== null && z.start < z.end) + : []; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Extensions/loopyLoop.js` around lines 135 - 141, Persisted marker payload from Spicetify.LocalStorage.get(`loopyLoop:${uri}`) must be validated before assigning to start, end, and skipZones; after JSON.parse(saved) ensure start and end are either numbers or null (coerce via Number and set to null if NaN), clamp to valid ranges if needed, and only accept skipZones when Array.isArray(data.skipZones) then map/filter entries to objects with numeric start/end (dropping or sanitizing invalid entries) before assigning to skipZones; update the block that reads saved/data so it never trusts raw values and always assigns sanitized numeric values or empty defaults.
🤖 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 251-303: The onprogress listener uses event.timeStamp in
Spicetify.Player.addEventListener("onprogress", ...) but the callback's event
may be undefined; update all references to event.timeStamp (inside the handler
around the prevPressedAt, lastStartEnforce, lastNextCall, navigatingBack checks)
to use a safe fallback like const now = event?.timeStamp ?? performance.now()
and then use now instead of event.timeStamp so the handler never throws when
event is missing.
---
Duplicate comments:
In `@Extensions/loopyLoop.js`:
- Around line 536-544: The parsed zone index zIdx (from parseInt on
target.getAttribute("data-zone-index")) must be validated before calling
setupActiveMarker and openContextMenu; update the handler to check that zIdx is
a finite integer (e.g., !Number.isNaN(zIdx) and Number.isFinite(zIdx)) and
optionally within expected bounds, and if invalid simply bail (return) or skip
calling setupActiveMarker(side, zIdx) and openContextMenu(event.clientX,
event.clientY) to avoid setting an invalid activeZoneIndex and crashing later
flows.
- Around line 475-479: scheduleMoveHide currently starts a new timeout without
cancelling any existing one, allowing stale timers to hide moveSubmenu after
hover re-entry; update scheduleMoveHide to clear the previous timer
(clearTimeout(moveHideTimer)) before assigning a new setTimeout and optionally
reset moveHideTimer to null when completing, referencing the scheduleMoveHide
function and the moveHideTimer and moveSubmenu identifiers so the existing
pending timeout is cancelled before scheduling a new hide.
---
Nitpick comments:
In `@Extensions/loopyLoop.js`:
- Around line 135-141: Persisted marker payload from
Spicetify.LocalStorage.get(`loopyLoop:${uri}`) must be validated before
assigning to start, end, and skipZones; after JSON.parse(saved) ensure start and
end are either numbers or null (coerce via Number and set to null if NaN), clamp
to valid ranges if needed, and only accept skipZones when
Array.isArray(data.skipZones) then map/filter entries to objects with numeric
start/end (dropping or sanitizing invalid entries) before assigning to
skipZones; update the block that reads saved/data so it never trusts raw values
and always assigns sanitized numeric values or empty defaults.
🪄 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: 4edb13d1-58af-4cbe-8a1b-f851be83d3cd
📒 Files selected for processing (1)
Extensions/loopyLoop.js
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 155-158: openContextMenu currently opens the main contextMenu but
doesn't hide the independent fixed-position move submenu (`#loopy-move-submenu`),
causing orphaned submenus to remain; before calling openMenu(contextMenu, x, y)
modify openContextMenu to explicitly close or hide the move submenu (e.g.,
remove its "open" class or set it hidden) so any previously-opened
`#loopy-move-submenu` is closed prior to reopening the main context menu.
- Around line 587-592: Compute and validate the progress percent before using it
in the toolbar click handler: call Spicetify.Player.getProgressPercent(), check
Number.isFinite(value) and that the current track/duration exists (e.g.,
Spicetify.Player.data?.duration > 0), and only assign mouseOnBarPercent and call
setupActiveMarker/openContextMenu when the percent is valid; otherwise set
mouseOnBarPercent to a safe default (e.g., 0 or null) and skip or adapt
setupActiveMarker so it doesn't store/render NaN/Infinity. Reference toolbarBtn,
Spicetify.Player.getProgressPercent, mouseOnBarPercent, and setupActiveMarker
when making the change.
- Around line 127-142: The loadState() function currently trusts raw JSON from
Spicetify.LocalStorage.get and may assign invalid values to start, end, and
skipZones; update loadState to validate parsed data after JSON.parse: ensure
start and end are numbers (Number.isFinite) and within valid bounds (e.g., >=0
and <=1 or within the track duration model you use), otherwise set them to null;
ensure skipZones is an array and filter its entries to objects with finite
numeric start and end where start < end and both within valid range, discarding
or normalizing any malformed entries; keep the existing try/catch but add these
checks before assigning to the module-level start, end, and skipZones so
downstream code (seek(), rendering markers) never receives NaN or out-of-range
values.
🪄 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: df04c90e-0452-4cf3-8a0d-5f1c1170cdbc
📒 Files selected for processing (1)
Extensions/loopyLoop.js
Comment on lines
+155
to
+158
| function openContextMenu(x, y) { | ||
| skipStartBtn.querySelector("button").textContent = pendingSkipStart !== null ? "Cancel skip start" : "Set section skip start"; | ||
| openMenu(contextMenu, x, y); | ||
| } |
There was a problem hiding this comment.
Close the move submenu before reopening the main menu.
#loopy-move-submenu is fixed-position and independent from contextMenu. If the user right-clicks another marker while Move ▶ is open, the old submenu stays behind at the previous coordinates.
🧹 Suggested fix
function openContextMenu(x, y) {
+ cancelMoveHide();
+ moveSubmenu.hidden = true;
skipStartBtn.querySelector("button").textContent = pendingSkipStart !== null ? "Cancel skip start" : "Set section skip start";
openMenu(contextMenu, x, y);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Extensions/loopyLoop.js` around lines 155 - 158, openContextMenu currently
opens the main contextMenu but doesn't hide the independent fixed-position move
submenu (`#loopy-move-submenu`), causing orphaned submenus to remain; before
calling openMenu(contextMenu, x, y) modify openContextMenu to explicitly close
or hide the move submenu (e.g., remove its "open" class or set it hidden) so any
previously-opened `#loopy-move-submenu` is closed prior to reopening the main
context menu.
rxri
approved these changes
Apr 6, 2026
Member
|
It still would be nice for this extension to use React in the future |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Complete rehaul of loopyLoop. Now persists across sessions for a song, user can repeat the song with Spotify if they want to loop instead of the plugin doing it, added ability to skip sections, new button in toolbar, can adjust section boundaries and start/end boundaries by right clicking. If Spotify is playing from phone but you have spicetify open on your computer it will follow the []{} rules set by the extension
Summary by CodeRabbit
New Features
Improvements