Skip to content

Conversation

@UndeffinedDev
Copy link

@UndeffinedDev UndeffinedDev commented Oct 25, 2025

This PR adds integration with the SimpMusic Lyrics provider, allowing the application to fetch and display lyrics for supported tracks directly from the SimpMusic Lyrics API.

I added it because while using the app, I noticed that some songs, for example in Spanish, do not have lyrics or synchronized lyrics.

Example of what I mean

2025-10-25.18-38-52.mp4

(btw, this is my first contribution to the project and first PR I've done in a public project)

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR integrates the SimpMusic Lyrics API as a new lyrics provider to improve lyrics coverage, particularly for songs in languages like Spanish where existing providers may not have lyrics or synchronized lyrics.

Key changes:

  • Added a new SimpMusicLyrics provider class that implements the LyricProvider interface
  • Registered the provider in the providers registry and enum
  • Implemented search logic with fallback strategies and artist matching using Jaro-Winkler similarity

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.

File Description
src/plugins/synced-lyrics/providers/SimpMusicLyrics.ts New provider implementation with search, filtering, and result selection logic
src/plugins/synced-lyrics/providers/index.ts Added SimpMusicLyrics to the ProviderNames enum
src/plugins/synced-lyrics/providers/renderer.ts Registered the new provider instance in the providers object

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +58 to +61
if (config()?.showLyricsEvenIfInexact) {
return null;
}
return null;
Copy link

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

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

The logic in lines 58-61 is redundant. The condition checks if the config has showLyricsEvenIfInexact enabled, but then returns null regardless of the result. Both branches return null, making the if statement unnecessary. This code should either be removed entirely or have different logic in the two branches.

Copilot uses AI. Check for mistakes.
Copy link
Member

Choose a reason for hiding this comment

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

good AI

const ratio = Math.max(
...permutations.map(([x, y]) => jaroWinkler(x, y)),
);
if (ratio < 0.85) continue;
Copy link

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

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

The artist comparison threshold at 0.85 is more lenient than in other providers. For example, LRCLib uses 0.9 for the same comparison. This could lead to false positive matches. Consider aligning with the stricter threshold used by other providers in this codebase for consistency.

Suggested change
if (ratio < 0.85) continue;
if (ratio < 0.9) continue;

Copilot uses AI. Check for mistakes.
Copy link
Member

Choose a reason for hiding this comment

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

good AI

Comment on lines +74 to +77
permutations.push([a.toLowerCase(), b.toLowerCase()]);
}
}

Copy link

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

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

The permutations array creates only one direction of comparisons. Looking at LRCLib's implementation (lines 86-97), it creates permutations in both directions (artistA to artistB and artistB to artistA) to ensure symmetrical matching. The current implementation may miss valid matches depending on the comparison order.

Suggested change
permutations.push([a.toLowerCase(), b.toLowerCase()]);
}
}
const aLower = a.toLowerCase();
const bLower = b.toLowerCase();
// Add both directions to ensure symmetrical matching, as in LRCLib
permutations.push([aLower, bLower], [bLower, aLower]);
}
}
if (!permutations.length) {
continue;
}

Copilot uses AI. Check for mistakes.
Comment on lines +88 to +97
filteredResults.sort(
(a, b) =>
Math.abs(a.durationSeconds - songDuration) -
Math.abs(b.durationSeconds - songDuration),
);

const maxVote = Math.max(...filteredResults.map((r) => r.vote ?? 0));

const topVoted = filteredResults.filter((r) => (r.vote ?? 0) === maxVote);

Copy link

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

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

The sorting logic only considers duration differences but ignores the vote field when selecting from the top-voted results. After filtering for maximum votes, the results are already sorted by duration (line 88-92), so taking topVoted[0] gets the best match by duration among the highest-voted songs. However, this means the initial duration-based sort on filteredResults is overwritten. Consider moving the vote-based filtering before the duration sorting to make the logic clearer.

Suggested change
filteredResults.sort(
(a, b) =>
Math.abs(a.durationSeconds - songDuration) -
Math.abs(b.durationSeconds - songDuration),
);
const maxVote = Math.max(...filteredResults.map((r) => r.vote ?? 0));
const topVoted = filteredResults.filter((r) => (r.vote ?? 0) === maxVote);
const maxVote = Math.max(...filteredResults.map((r) => r.vote ?? 0));
const topVoted = filteredResults.filter((r) => (r.vote ?? 0) === maxVote);
topVoted.sort(
(a, b) =>
Math.abs(a.durationSeconds - songDuration) -
Math.abs(b.durationSeconds - songDuration),
);

Copilot uses AI. Check for mistakes.
let response = await fetch(url);

if (!response.ok) {
throw new Error(`HTTP error (${response.statusText})`);
Copy link

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

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

The error message format is inconsistent with other providers in the codebase. LRCLib uses the format 'bad HTTPStatus(statusText)' while this uses 'HTTP error (statusText)'. For consistency across the codebase, consider using the same error message format as other providers.

Copilot uses AI. Check for mistakes.

response = await fetch(url);
if (!response.ok) {
throw new Error(`HTTP error (${response.statusText})`);
Copy link

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

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

The error message format is inconsistent with other providers in the codebase. LRCLib uses the format 'bad HTTPStatus(statusText)' while this uses 'HTTP error (statusText)'. For consistency across the codebase, consider using the same error message format as other providers.

Copilot uses AI. Check for mistakes.

response = await fetch(url);
if (!response.ok) {
throw new Error(`HTTP error (${response.statusText})`);
Copy link

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

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

The error message format is inconsistent with other providers in the codebase. LRCLib uses the format 'bad HTTPStatus(statusText)' while this uses 'HTTP error (statusText)'. For consistency across the codebase, consider using the same error message format as other providers.

Copilot uses AI. Check for mistakes.
Copy link
Member

@ArjixWasTaken ArjixWasTaken left a comment

Choose a reason for hiding this comment

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

some nitpicking, but otherwise this looks good

please follow the DRY principles and deduplicate your code by making a re-usable function where needed

let data: SimpMusicSong[] = [];

let query = new URLSearchParams({ q: `${title} ${artist}` });
let url = `${this.baseUrl}/search?${query.toString()}`;
Copy link
Member

Choose a reason for hiding this comment

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

Redundant .toString

Suggested change
let url = `${this.baseUrl}/search?${query.toString()}`;
let url = `${this.baseUrl}/search?${query}`;

throw new Error(`HTTP error (${response.statusText})`);
}

let json = (await response.json()) as SimpMusicResponse;
Copy link
Member

Choose a reason for hiding this comment

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

Please create a zod schema and parse the data.


if (!data.length) {
query = new URLSearchParams({ q: title });
url = `${this.baseUrl}/search?${query.toString()}`;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
url = `${this.baseUrl}/search?${query.toString()}`;
url = `${this.baseUrl}/search?${query}`;

throw new Error(`HTTP error (${response.statusText})`);
}

json = (await response.json()) as SimpMusicResponse;
Copy link
Member

Choose a reason for hiding this comment

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

Zod schema x2


if (!data.length && alternativeTitle) {
query = new URLSearchParams({ q: alternativeTitle });
url = `${this.baseUrl}/search?${query.toString()}`;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
url = `${this.baseUrl}/search?${query.toString()}`;
url = `${this.baseUrl}/search?${query}`;

throw new Error(`HTTP error (${response.statusText})`);
}

json = (await response.json()) as SimpMusicResponse;
Copy link
Member

Choose a reason for hiding this comment

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

Zod schema x3

Comment on lines +125 to +141
type SimpMusicResponse = {
type: string;
data: SimpMusicSong[];
success?: boolean;
};

type SimpMusicSong = {
id: string;
videoId?: string;
songTitle: string;
artistName: string;
albumName?: string;
durationSeconds: number;
plainLyric?: string;
syncedLyrics?: string;
vote?: number;
};
Copy link
Member

Choose a reason for hiding this comment

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

instead of types, please make zod schemas 🙏

@ArjixWasTaken
Copy link
Member

Looks like UndeffinedDev has gone offline, maybe on vacation, so I'll be picking up this PR.

@ArjixWasTaken ArjixWasTaken self-assigned this Dec 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants