Skip to content

Refactor trashbin.js for readability and safer artist skipping #3660

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
marmik-7 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Refactor trashbin.js for readability and safer artist skipping #3660

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 15 additions & 18 deletions Extensions/trashbin.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@

function settingsContent() {
// Options
header = document.createElement("h2");
const header = document.createElement("h2");
header.innerText = "Options";
content.appendChild(header);
Comment on lines +57 to 59
Copy link

@coderabbitai coderabbitai bot Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Critical: const declaration breaks subsequent reassignment.

Declaring header as const at line 57 causes a runtime error at line 70, where header is reassigned. This is confirmed by the pipeline failure: "Can't assign header because it's a constant."

🔎 Apply this fix to use `let` instead of `const`: 
-		const header = document.createElement("h2");
+		let header = document.createElement("h2");
📝 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.

Suggested change
const header = document.createElement("h2");
header.innerText = "Options";
content.appendChild(header);
let header = document.createElement("h2");
header.innerText = "Options";
content.appendChild(header);
🤖 Prompt for AI Agents 
In Extensions/trashbin.js around lines 57 to 59, the variable header is declared
with const but later reassigned (around line 70), causing a runtime "can't
assign header because it's a constant" error; change the declaration from const
to let so the variable can be reassigned, and verify there are no other const
references or shadowing that would conflict with the updated mutable binding.


Expand Down Expand Up @@ -152,7 +152,8 @@
try {
const value = JSON.parse(Spicetify.LocalStorage.get(item));
return value ?? defaultValue;
} catch {
} catch (e) {
console.error(`Failed to parse ${item} from LocalStorage`, e);
return defaultValue;
}
}
Expand Down Expand Up @@ -261,17 +262,15 @@
return;
}

let uriIndex = 0;
let artistUri = data.item.metadata.artist_uri;

while (artistUri) {
if (trashArtistList[artistUri]) {
Spicetify.Player.next();
return;
}

uriIndex++;
artistUri = data.item.metadata[`artist_uri:${uriIndex}`];
const artistUris = [data.item.metadata.artist_uri];
let i = 1;
while (data.item.metadata[`artist_uri:${i}`]) {
artistUris.push(data.item.metadata[`artist_uri:${i}`]);
i++;
}

if (artistUris.some(uri => trashArtistList[uri])) {
Spicetify.Player.next();
}
Comment on lines +265 to 274
Copy link

@coderabbitai coderabbitai bot Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Handle undefined artist_uri and fix formatting.

Two concerns:

  1. Potential undefined in array: If data.item.metadata.artist_uri is undefined, line 265 initializes the array with [undefined], which could cause unexpected behavior in the .some() check.

  2. Formatting issue: The pipeline reports formatting differences at lines 266-275.

🔎 Suggested fix to handle undefined and address formatting: 
-		const artistUris = [data.item.metadata.artist_uri];
-		let i = 1;
-		while (data.item.metadata[`artist_uri:${i}`]) {
-		    artistUris.push(data.item.metadata[`artist_uri:${i}`]);
-		    i++;
-		}
-		
-		if (artistUris.some(uri => trashArtistList[uri])) {
-		    Spicetify.Player.next();
-		}
+		const artistUris = [];
+		if (data.item.metadata.artist_uri) {
+			artistUris.push(data.item.metadata.artist_uri);
+		}
+		let i = 1;
+		while (data.item.metadata[`artist_uri:${i}`]) {
+			artistUris.push(data.item.metadata[`artist_uri:${i}`]);
+			i++;
+		}
+
+		if (artistUris.some((uri) => trashArtistList[uri])) {
+			Spicetify.Player.next();
+		}
📝 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.

Suggested change
const artistUris = [data.item.metadata.artist_uri];
let i = 1;
while (data.item.metadata[`artist_uri:${i}`]) {
artistUris.push(data.item.metadata[`artist_uri:${i}`]);
i++;
}
if (artistUris.some(uri => trashArtistList[uri])) {
Spicetify.Player.next();
}
const artistUris = [];
if (data.item.metadata.artist_uri) {
artistUris.push(data.item.metadata.artist_uri);
}
let i = 1;
while (data.item.metadata[`artist_uri:${i}`]) {
artistUris.push(data.item.metadata[`artist_uri:${i}`]);
i++;
}
if (artistUris.some((uri) => trashArtistList[uri])) {
Spicetify.Player.next();
}
🤖 Prompt for AI Agents 
In Extensions/trashbin.js around lines 265 to 274, the code initializes
artistUris with data.item.metadata.artist_uri which can be undefined, causing
[undefined] and incorrect .some() results and the block also has reported
formatting issues; change logic to start with an empty array, push
data.item.metadata.artist_uri only if it is truthy (or use optional chaining),
loop and push subsequent artist_uri:i entries only when they exist, and ensure
consistent indentation/spacing for lines 266–275 to match project formatter
(e.g., single tab/2 spaces and blank line placement).

}

Expand Down Expand Up @@ -342,13 +341,11 @@
const uri = uris[0];
const uriObj = Spicetify.URI.fromString(uri);
if (uriObj.type === Spicetify.URI.Type.TRACK) {
this.name = trashSongList[uri] ? UNTHROW_TEXT : THROW_TEXT;
return true;
return { name: trashSongList[uri] ? UNTHROW_TEXT : THROW_TEXT };
Copy link

@coderabbitai coderabbitai bot Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Find and examine trashbin.js to see lines 344 and 348
find . -name "trashbin.js" -type f | head -5

Repository: spicetify/cli

Length of output: 81

🏁 Script executed:

#!/bin/bash
# Read trashbin.js around lines 344 and 348
sed -n '330,360p' ./Extensions/trashbin.js | cat -n

Repository: spicetify/cli

Length of output: 1143

Fix shouldAddContextMenu to return a boolean, not an object with a name property.

Lines 344 and 348 return { name: ... } instead of a boolean value. The Spicetify API defines ShouldAddCallback as (uris: string[], uids?: string[], contextUri?: string) => boolean, which means the function must return true or false to determine whether the menu item should display. To dynamically set the menu item's label based on whether an item is in the trash, update the cntxMenu.name property directly instead of returning an object from the callback.

Also applies to: 348-348

🤖 Prompt for AI Agents 
In Extensions/trashbin.js around lines 344 and 348, the shouldAddContextMenu
callback is returning an object like { name: ... } but the Spicetify
ShouldAddCallback must return a boolean; change the callback to return
true/false (e.g., return !!trashSongList[uri]) and set the menu label by
mutating cntxMenu.name directly before returning, so the function only returns a
boolean and the dynamic label is applied via cntxMenu.name.

}

if (uriObj.type === Spicetify.URI.Type.ARTIST) {
this.name = trashArtistList[uri] ? UNTHROW_TEXT : THROW_TEXT;
return true;
return { name: trashArtistList[uri] ? UNTHROW_TEXT : THROW_TEXT };
}

return false;
Expand Down Expand Up @@ -391,7 +388,7 @@
});

const writable = await handle.createWritable();
await writable.write(JSON.stringify(data));
await writable.write(JSON.stringify(data, null, 2));
await writable.close();

Spicetify.showNotification("Backup saved succesfully.");
Expand Down
Loading