improved spotify api queries for better results#26
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the search logic to improve artist matching by focusing on the primary artist and implementing a fallback query mechanism. It also introduces an extractImage helper to handle image selection based on size and artist name. Several issues were identified: a potential runtime error when accessing images from items that lack them, redundant variable assignments and string replacements, and a regression where the search method is hardcoded to 'album', which prevents retrieving artist profile images and breaks compatibility with the existing logic.
| const images = match.images | ||
|
|
||
| let smallest = images[0] |
There was a problem hiding this comment.
Accessing images[0] without checking if the array exists or is empty can lead to a runtime error when trying to access .url later (e.g., at line 149 or 159). Adding a safety check ensures the function handles items without images gracefully.
const images = match.images || []
if ( images.length === 0 ) {
return null
}
let smallest = images[0]|
|
||
| // Default options | ||
| let query = artist.replace( '&', 'and' ) | ||
| let query = artist.replace( '&', ';' ) |
| let method = 'artist' | ||
| // Use only primary artist for better matching | ||
| const primaryArtist = artist.split( /[;,&]/ )[0].trim() | ||
| let primaryQuery = primaryArtist.replace( '&', ';' ) |
There was a problem hiding this comment.
| const primaryArtist = artist.split( /[;,&]/ )[0].trim() | ||
| let primaryQuery = primaryArtist.replace( '&', ';' ) | ||
|
|
||
| let method = 'album' |
There was a problem hiding this comment.
The search method is now hardcoded to 'album', whereas it previously defaulted to 'artist' when no album was specified. This change prevents the library from retrieving artist profile images. Furthermore, the extractImage helper (line 123) relies on the artists property, which exists on album objects but not on artist objects, making this logic tightly coupled to the 'album' search type. If this regression is intentional, it should be documented as a breaking change.
lacymorrow
left a comment
There was a problem hiding this comment.
Thanks for the PR! The intent (better matching via field-qualified queries + fallback) is good, but there are a few issues to address before this can land:
Blocking
-
Regression: artist-only lookup is broken. When
opts.albumisnull,methodis now hardcoded to'album'(it used to default to'artist'). That removes the ability to fetch artist profile images, and the newextractImagereadsitem.artists— a field that doesn't exist onartistsearch results — so the artist code path can't work at all. Please restore themethod = 'artist'default and branchextractImageaccordingly (artist results don't have anartistsarray; match againstitem.name). -
Crash risk in
extractImage.match.imagescan be missing or empty for some Spotify items.images[0]→.urlwill throw. Guard with something like:const images = (match && match.images) || [] if (images.length === 0) return null
Should fix
-
'&' → ';'substitution is suspicious. The original code used'and', which is meaningful to Spotify's tokenizer;;isn't, andString.prototype.replacewith a string only replaces the first occurrence. If the goal is to normalize multi-artist strings, usereplaceAll('&', 'and')(or a regex) and document why. -
Redundant
let query = artist.replace('&', ';')at the top of the function — overwritten in every branch below. Replace withlet query. -
Redundant
.replace('&', ';')onprimaryArtist—primaryArtistis derived by splitting on&, so it can't contain one. Just useprimaryArtist. -
Trailing newline removed at EOF — please restore.
Nit
limit=10is reasonable, but consider documenting the matching strategy in a comment so the next reader understands whyextractImagelooks for an exact (case-insensitive) artist-name match before falling back toitems[0].
Happy to re-review once these are addressed.
- Restore method='artist' default when opts.album is null - Branch extractImage for artist results (match item.name, not item.artists) - Guard match.images: fallback to [] and return null if empty - Remove redundant initial query assignment (overwritten in every branch) - Remove redundant primaryQuery variable (primaryArtist already & -free via split) - Restore trailing newline at EOF - Add comment documenting extractImage matching strategy
|
All the review feedback is addressed in bc2f766. Blocking fixes:
Should-fix items:
Nit:
|
No description provided.