Skip to content

fix fetch reliability, memory leaks, and click-seek bug - #13

Merged
binimum merged 1 commit into
binimum:mainfrom
Cyberfilo:fix/improvements
Apr 10, 2026
Merged

binimum merged 1 commit into
binimum:mainfrom
Cyberfilo:fix/improvements

Conversation

@Cyberfilo

Copy link
Copy Markdown
Contributor
  • Add fetch timeouts (8s) to all lyrics requests to prevent indefinite hangs from CORS or unresponsive servers (fixes [BUG] Lyrics load for very long, and are of not word by word #12)
  • Add AbortController to cancel stale fetches on rapid song changes
  • Increase server retry count from 2 to 3 for KPOE and Tidal
  • Fix handleLineClick selector using wrong timestamp unit (ms * 1000)
  • Fix memory leaks: clear all timeouts in disconnectedCallback
  • Fix event listener leak: use stored bound references for proper removal
  • Use stored bound ref for animateProgress to avoid allocation per frame
  • Remove duplicate eslint-disable comments
  • Replace any types with generics in GoogleService romanize methods
  • Add fetch timeouts to GoogleService translate/romanize requests

- Add fetch timeouts (8s) to all lyrics requests to prevent indefinite
  hangs from CORS or unresponsive servers (fixes binimum#12)
- Add AbortController to cancel stale fetches on rapid song changes
- Increase server retry count from 2 to 3 for KPOE and Tidal
- Fix handleLineClick selector using wrong timestamp unit (ms * 1000)
- Fix memory leaks: clear all timeouts in disconnectedCallback
- Fix event listener leak: use stored bound references for proper removal
- Use stored bound ref for animateProgress to avoid allocation per frame
- Remove duplicate eslint-disable comments
- Replace any types with generics in GoogleService romanize methods
- Add fetch timeouts to GoogleService translate/romanize requests
@binimum

binimum commented Apr 10, 2026 •

Copy link
Copy Markdown
Owner

Thanks for all the patches! If you'll let me test it and get a quick second review I'll get it merged asap.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 improves lyrics loading reliability and UI behavior in AmLyrics by adding request timeouts, reducing leaks, and fixing a click-to-seek/scroll targeting issue, plus tightening typing and adding timeouts in GoogleService’s Google Translate requests.

Changes:

  • Add fetch timeouts across lyrics-provider requests and Google Translate/romanize calls.
  • Introduce cancellation plumbing and cleanup to reduce leaks (timeouts, event listeners, animation frame binding).
  • Adjust provider fallback breadth (try 3 servers) and fix click-to-seek line lookup to match millisecond timestamps.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
src/AmLyrics.ts Adds fetch timeouts, attempts stale-fetch cancellation, fixes click-to-seek selector, and cleans up timeouts/listeners/RAF usage.
src/GoogleService.ts Adds fetch timeouts for translate/romanize calls and replaces any with a generic, typed romanization shape.
Comments suppressed due to low confidence (1)

src/AmLyrics.ts:1663

  • fetchLyrics aborts the previous controller, but most awaited work below (provider fetches) doesn't use controller.signal, and state updates (e.g., setting availableSources/lyrics) aren't guarded after each await. As a result, a superseded request can still complete and overwrite UI state even though its controller was aborted.

Recommend propagating controller.signal into all fetch/provider helpers and checking controller.signal.aborted before committing results to component state.

  private async fetchLyrics() {
    // Cancel any in-flight fetch to prevent stale results from racing
    this.fetchAbortController?.abort();
    const controller = new AbortController();
    this.fetchAbortController = controller;

    this.isLoading = true;
    this.lyrics = undefined;
    this.lyricsSource = null;
    this.availableSources = [];
    this.currentSourceIndex = 0;
    this.isFetchingAlternatives = false;
    this.hasFetchedAllProviders = false;
    try {
      const resolvedMetadata = await this.resolveSongMetadata();
      // If a newer fetch was triggered while we awaited, bail out
      if (controller.signal.aborted) return;

      const isMusicIdOnlyRequest =
        Boolean(this.musicId) &&
        !this.songTitle &&
        !this.songArtist &&
        !this.query;

      const collectedSources: { lines: LyricsLine[]; source: string }[] = [];

      if (resolvedMetadata?.metadata && !isMusicIdOnlyRequest) {
        const title = resolvedMetadata.metadata.title?.trim() || '';
        const artist = resolvedMetadata.metadata.artist?.trim() || '';

        const youLyResults = await AmLyrics.fetchLyricsFromYouLyPlus(
          title,
          artist,
          resolvedMetadata.metadata,
        );

        if (youLyResults && youLyResults.length > 0) {
          collectedSources.push(...youLyResults);
        }
      }

      if (collectedSources.length === 0 && resolvedMetadata?.metadata) {
        const tidalResult = await AmLyrics.fetchLyricsFromTidal(
          resolvedMetadata.metadata,
          resolvedMetadata.catalogIsrc,
        );
        if (tidalResult && tidalResult.lines.length > 0) {
          collectedSources.push({
            lines: tidalResult.lines,
            source: 'Tidal',
          });
        }
      }

      // Fallback: LRCLIB
      if (collectedSources.length === 0 && resolvedMetadata?.metadata) {
        const lrclibResult = await AmLyrics.fetchLyricsFromLrclib(
          resolvedMetadata.metadata,
        );
        if (lrclibResult && lrclibResult.lines.length > 0) {
          collectedSources.push({
            lines: lrclibResult.lines,
            source: 'LRCLIB',
          });
        }
      }

      if (collectedSources.length === 0 && resolvedMetadata?.metadata) {
        const geniusResult = await AmLyrics.fetchLyricsFromGenius(
          resolvedMetadata.metadata,
        );
        if (geniusResult && geniusResult.lines.length > 0) {
          collectedSources.push({
            lines: geniusResult.lines,
            source: 'Genius',
          });
        }
      }

      this.hasFetchedAllProviders =
        collectedSources.length === 0 ||
        collectedSources.some(
          s =>
            s.source === 'LRCLIB' ||
            s.source === 'Tidal' ||
            s.source === 'Genius',
        );

      if (collectedSources.length > 0) {
        this.availableSources = AmLyrics.mergeAndSortSources(collectedSources);

        this.currentSourceIndex = 0;
        this.lyrics = this.availableSources[0].lines;
        this.lyricsSource = this.availableSources[0].source;
        await this.onLyricsLoaded();
        return;

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

Comment thread src/AmLyrics.ts Resolved
@binimum
binimum merged commit 53d0236 into binimum:main Apr 10, 2026
6 checks passed
@Cyberfilo
Cyberfilo deleted the fix/improvements branch April 10, 2026 09:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Lyrics load for very long, and are of not word by word

3 participants