Refactor trashbin.js for readability and safer artist skipping #3660
Conversation
- Fixed variable redeclarations (e.g., 'header', 'artistUri') to avoid shadowing. - Improved artist skipping logic to iterate safely over multiple artists without skipping unrelated tracks. - Added proper error handling in LocalStorage parsing. - Updated JSON export to include indentation for better readability. - Adjusted context menu return values to correctly return an object instead of modifying `this`.
WalkthroughA single-file refactoring of the trashbin extension introducing improved error handling, restructured artist URI skip logic, updated context menu object returns, and enhanced JSON export formatting for readability. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Extensions/trashbin.js(5 hunks)
🧰 Additional context used
🪛 GitHub Actions: Code quality
Extensions/trashbin.js
[error] 70-71: Can't assign header because it's a constant. Unsafe fix: Replace const with let if you assign it to a new value.
[error] 266-275: File content differs from formatting output.
🔇 Additional comments (2)
Extensions/trashbin.js (2)
155-157: LGTM! Improved error handling.The error handling now properly captures and logs parsing errors with context, making debugging easier.
391-391: LGTM! Improved JSON readability.Adding indentation makes the exported backup file more human-readable and easier to manually inspect or edit.
| const header = document.createElement("h2"); | ||
| header.innerText = "Options"; | ||
| content.appendChild(header); |
There was a problem hiding this comment.
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.
| 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.
| 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(); | ||
| } |
There was a problem hiding this comment.
Handle undefined artist_uri and fix formatting.
Two concerns:
-
Potential undefined in array: If
data.item.metadata.artist_uriis undefined, line 265 initializes the array with[undefined], which could cause unexpected behavior in the.some()check. -
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.
| 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).
| 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 }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find and examine trashbin.js to see lines 344 and 348
find . -name "trashbin.js" -type f | head -5Repository: 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 -nRepository: 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.
1 participant
this.Summary by CodeRabbit
Release Notes
Bug Fixes
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.