mirror of
https://github.com/navidrome/navidrome.git
synced 2026-10-15 22:07:08 +02:00
fix(ui): explain temporary lyrics cooldowns
This commit is contained in:
parent
5fc73c22b3
commit
c875ca4704
11 changed files with 164 additions and 6 deletions
|
|
@ -87,6 +87,7 @@ func songsByArtistTitleWithLyricsFirst(artist, title string) model.QueryOptions
|
|||
}
|
||||
|
||||
func (l *lyricsService) getLyricsForCandidates(ctx context.Context, mediaFiles []*model.MediaFile) (model.LyricList, error) {
|
||||
var pluginErr error
|
||||
for pattern := range strings.SplitSeq(conf.Server.LyricsPriority, ",") {
|
||||
pattern = strings.TrimSpace(pattern)
|
||||
if pattern == "" {
|
||||
|
|
@ -101,6 +102,9 @@ func (l *lyricsService) getLyricsForCandidates(ctx context.Context, mediaFiles [
|
|||
lyricsList, err := l.getLyricsFromSource(ctx, mf, pattern)
|
||||
if err != nil {
|
||||
log.Error(ctx, "error getting lyrics", "source", pattern, err)
|
||||
if pattern != "embedded" && !strings.HasPrefix(pattern, ".") {
|
||||
pluginErr = err
|
||||
}
|
||||
continue
|
||||
}
|
||||
|
||||
|
|
@ -110,7 +114,7 @@ func (l *lyricsService) getLyricsForCandidates(ctx context.Context, mediaFiles [
|
|||
}
|
||||
}
|
||||
|
||||
return nil, nil
|
||||
return nil, pluginErr
|
||||
}
|
||||
|
||||
func (l *lyricsService) getLyricsFromSource(ctx context.Context, mf *model.MediaFile, pattern string) (model.LyricList, error) {
|
||||
|
|
|
|||
|
|
@ -365,6 +365,18 @@ var _ = Describe("Lyrics", func() {
|
|||
Expect(err).To(BeNil())
|
||||
Expect(list).To(Equal(embeddedLyrics)) // falls through to embedded
|
||||
})
|
||||
|
||||
It("should return a plugin error when no later source has lyrics", func() {
|
||||
conf.Server.LyricsPriority = "test-lyrics-plugin,embedded"
|
||||
mf.Lyrics = ""
|
||||
mockLoader.err = fmt.Errorf("remote lyrics cooldown active for 30s")
|
||||
svc := lyrics.NewLyrics(nil, mockLoader)
|
||||
|
||||
list, err := svc.GetLyrics(ctx, &mf)
|
||||
|
||||
Expect(list).To(BeEmpty())
|
||||
Expect(err).To(MatchError("remote lyrics cooldown active for 30s"))
|
||||
})
|
||||
})
|
||||
|
||||
var _ = Describe("GetLyricsByArtistTitle", func() {
|
||||
|
|
|
|||
|
|
@ -417,6 +417,7 @@ const LyricsPanel = ({
|
|||
inline = false,
|
||||
loading = false,
|
||||
error = null,
|
||||
errorMessage = null,
|
||||
labels = {},
|
||||
}) => {
|
||||
const classes = useStyles()
|
||||
|
|
@ -675,7 +676,7 @@ const LyricsPanel = ({
|
|||
const message = loading
|
||||
? labels.loading || 'Loading lyrics'
|
||||
: error
|
||||
? labels.unavailable || 'Lyrics unavailable'
|
||||
? errorMessage || labels.unavailable || 'Lyrics unavailable'
|
||||
: labels.empty || 'No lyrics available'
|
||||
return (
|
||||
<div
|
||||
|
|
|
|||
|
|
@ -944,6 +944,18 @@ describe('<LyricsPanel />', () => {
|
|||
)
|
||||
})
|
||||
|
||||
it('explains a temporary lyrics cooldown', () => {
|
||||
renderPanel({
|
||||
mainLyric: null,
|
||||
error: new Error('rate limited'),
|
||||
errorMessage: 'Remote lyrics are rate-limited. Retrying in 30 seconds.',
|
||||
})
|
||||
|
||||
expect(screen.getByTestId('lyrics-empty-state')).toHaveTextContent(
|
||||
'Remote lyrics are rate-limited. Retrying in 30 seconds.',
|
||||
)
|
||||
})
|
||||
|
||||
it.each(['light', 'dark'])(
|
||||
'keeps future tokens readable and translations subordinate in the %s theme',
|
||||
(type) => {
|
||||
|
|
|
|||
|
|
@ -102,6 +102,7 @@ const LyricsSidebar = ({
|
|||
audioInstance,
|
||||
loading = false,
|
||||
error = null,
|
||||
errorMessage = null,
|
||||
labels = {},
|
||||
returnFocusRef,
|
||||
}) => {
|
||||
|
|
@ -291,6 +292,7 @@ const LyricsSidebar = ({
|
|||
audioInstance={audioInstance}
|
||||
loading={loading}
|
||||
error={error}
|
||||
errorMessage={errorMessage}
|
||||
labels={labels}
|
||||
/>
|
||||
</div>
|
||||
|
|
|
|||
|
|
@ -10,6 +10,7 @@ export const emptyLyricLayers = Object.freeze({
|
|||
|
||||
const MAX_LYRIC_CACHE_ENTRIES = 75
|
||||
const NEGATIVE_CACHE_TTL_MS = 30_000
|
||||
const COOLDOWN_PATTERN = /(?:cooldown active for|retry after)\s+(\d+)s/gi
|
||||
|
||||
const cache = new Map()
|
||||
const inFlight = new Map()
|
||||
|
|
@ -23,6 +24,15 @@ const normalizeLyricLayers = (layers) => ({
|
|||
const readStructuredLyrics = (response) =>
|
||||
response?.json?.['subsonic-response']?.lyricsList?.structuredLyrics || []
|
||||
|
||||
const retryDelayFromError = (error) => {
|
||||
const serverMessage =
|
||||
error?.body?.['subsonic-response']?.error?.message || error?.message || ''
|
||||
const matches = [...String(serverMessage).matchAll(COOLDOWN_PATTERN)]
|
||||
if (matches.length === 0) return null
|
||||
const seconds = Math.max(...matches.map((match) => Number(match[1])))
|
||||
return Number.isFinite(seconds) && seconds > 0 ? seconds * 1000 : null
|
||||
}
|
||||
|
||||
const buildCacheKey = ({ trackId, preferredLanguage, updatedAt }) =>
|
||||
[trackId || '', updatedAt || '', preferredLanguage || ''].join('\u0000')
|
||||
|
||||
|
|
@ -117,8 +127,10 @@ const useEnhancedLyrics = ({
|
|||
layers: emptyLyricLayers,
|
||||
loading: false,
|
||||
error: null,
|
||||
retryAt: null,
|
||||
}))
|
||||
const [requestGeneration, setRequestGeneration] = useState(0)
|
||||
const [, setCountdownTick] = useState(0)
|
||||
|
||||
const retry = useCallback(() => {
|
||||
cache.delete(cacheKey)
|
||||
|
|
@ -132,13 +144,20 @@ const useEnhancedLyrics = ({
|
|||
layers: emptyLyricLayers,
|
||||
loading: false,
|
||||
error: null,
|
||||
retryAt: null,
|
||||
})
|
||||
return undefined
|
||||
}
|
||||
|
||||
const cached = readCachedLyrics(cacheKey)
|
||||
if (cached) {
|
||||
setState({ cacheKey, layers: cached, loading: false, error: null })
|
||||
setState({
|
||||
cacheKey,
|
||||
layers: cached,
|
||||
loading: false,
|
||||
error: null,
|
||||
retryAt: null,
|
||||
})
|
||||
return undefined
|
||||
}
|
||||
|
||||
|
|
@ -153,12 +172,19 @@ const useEnhancedLyrics = ({
|
|||
layers: emptyLyricLayers,
|
||||
loading: true,
|
||||
error: null,
|
||||
retryAt: null,
|
||||
})
|
||||
|
||||
request.promise
|
||||
.then((layers) => {
|
||||
if (!active) return
|
||||
setState({ cacheKey, layers, loading: false, error: null })
|
||||
setState({
|
||||
cacheKey,
|
||||
layers,
|
||||
loading: false,
|
||||
error: null,
|
||||
retryAt: null,
|
||||
})
|
||||
})
|
||||
.catch((error) => {
|
||||
if (!active || error?.name === 'AbortError') return
|
||||
|
|
@ -168,6 +194,10 @@ const useEnhancedLyrics = ({
|
|||
layers: emptyLyricLayers,
|
||||
loading: false,
|
||||
error,
|
||||
retryAt: (() => {
|
||||
const delay = retryDelayFromError(error)
|
||||
return delay == null ? null : Date.now() + delay
|
||||
})(),
|
||||
})
|
||||
})
|
||||
|
||||
|
|
@ -184,11 +214,34 @@ const useEnhancedLyrics = ({
|
|||
trackId,
|
||||
])
|
||||
|
||||
useEffect(() => {
|
||||
if (
|
||||
state.cacheKey !== cacheKey ||
|
||||
state.retryAt == null ||
|
||||
disabled ||
|
||||
!requested
|
||||
) {
|
||||
return undefined
|
||||
}
|
||||
|
||||
const retryDelay = Math.max(0, state.retryAt - Date.now())
|
||||
const retryTimer = window.setTimeout(retry, retryDelay)
|
||||
const countdownTimer = window.setInterval(
|
||||
() => setCountdownTick((current) => current + 1),
|
||||
1_000,
|
||||
)
|
||||
return () => {
|
||||
window.clearTimeout(retryTimer)
|
||||
window.clearInterval(countdownTimer)
|
||||
}
|
||||
}, [cacheKey, disabled, requested, retry, state.cacheKey, state.retryAt])
|
||||
|
||||
if (state.cacheKey !== cacheKey) {
|
||||
return {
|
||||
layers: emptyLyricLayers,
|
||||
loading: Boolean(trackId && !disabled && requested),
|
||||
error: null,
|
||||
retryAfterSeconds: null,
|
||||
retry,
|
||||
}
|
||||
}
|
||||
|
|
@ -196,6 +249,10 @@ const useEnhancedLyrics = ({
|
|||
layers: state.layers,
|
||||
loading: state.loading,
|
||||
error: state.error,
|
||||
retryAfterSeconds:
|
||||
state.retryAt == null
|
||||
? null
|
||||
: Math.max(1, Math.ceil((state.retryAt - Date.now()) / 1_000)),
|
||||
retry,
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -54,6 +54,7 @@ describe('useEnhancedLyrics', () => {
|
|||
})
|
||||
|
||||
afterEach(() => {
|
||||
vi.useRealTimers()
|
||||
localStorage.clear()
|
||||
clearEnhancedLyricsCache()
|
||||
})
|
||||
|
|
@ -130,6 +131,33 @@ describe('useEnhancedLyrics', () => {
|
|||
expect(subsonic.getLyricsBySongId).toHaveBeenCalledTimes(2)
|
||||
})
|
||||
|
||||
it('reports a cooldown countdown and retries the current song when it expires', async () => {
|
||||
vi.useFakeTimers()
|
||||
vi.setSystemTime(new Date('2026-08-23T06:00:00Z'))
|
||||
const cooldownError = new Error('better lyrics API cooldown active for 2s')
|
||||
subsonic.getLyricsBySongId
|
||||
.mockRejectedValueOnce(cooldownError)
|
||||
.mockResolvedValueOnce(responseFor('Recovered after cooldown'))
|
||||
|
||||
const { result } = renderLyrics({ trackId: 'rate-limited-song' })
|
||||
await act(async () => {
|
||||
await Promise.resolve()
|
||||
})
|
||||
|
||||
expect(result.current.error).toBe(cooldownError)
|
||||
expect(result.current.retryAfterSeconds).toBe(2)
|
||||
|
||||
await act(async () => {
|
||||
vi.advanceTimersByTime(2_000)
|
||||
await Promise.resolve()
|
||||
})
|
||||
|
||||
expect(subsonic.getLyricsBySongId).toHaveBeenCalledTimes(2)
|
||||
expect(result.current.layers.main?.line[0].value).toBe(
|
||||
'Recovered after cooldown',
|
||||
)
|
||||
})
|
||||
|
||||
it('keeps cached lyrics separate by preferred language', async () => {
|
||||
subsonic.getLyricsBySongId
|
||||
.mockResolvedValueOnce(responseFor('English lyrics', 'en'))
|
||||
|
|
|
|||
|
|
@ -27,6 +27,7 @@ const usePlayerLyrics = ({
|
|||
layers: lyricLayers,
|
||||
loading: lyricsLoading,
|
||||
error: lyricsError,
|
||||
retryAfterSeconds: lyricsRetryAfterSeconds,
|
||||
retry: retryLyrics,
|
||||
} = useEnhancedLyrics({
|
||||
trackId,
|
||||
|
|
@ -56,6 +57,12 @@ const usePlayerLyrics = ({
|
|||
toggle: translate('player.toggleLyricText'),
|
||||
loading: translate('player.lyricsLoadingText'),
|
||||
unavailable: translate('player.lyricsUnavailableText'),
|
||||
rateLimited:
|
||||
lyricsRetryAfterSeconds == null
|
||||
? null
|
||||
: translate('player.lyricsRateLimitedText', {
|
||||
smart_count: lyricsRetryAfterSeconds,
|
||||
}),
|
||||
empty: translate('player.emptyLyricText'),
|
||||
resize: translate('player.resizeLyricsSidebarText'),
|
||||
showTranslation: translate('player.showLyricsTranslationText'),
|
||||
|
|
@ -63,7 +70,7 @@ const usePlayerLyrics = ({
|
|||
showPronunciation: translate('player.showLyricsPronunciationText'),
|
||||
hidePronunciation: translate('player.hideLyricsPronunciationText'),
|
||||
}),
|
||||
[translate],
|
||||
[lyricsRetryAfterSeconds, translate],
|
||||
)
|
||||
|
||||
const toggleLyrics = useCallback(() => {
|
||||
|
|
@ -118,6 +125,7 @@ const usePlayerLyrics = ({
|
|||
audioInstance,
|
||||
loading: lyricsLoading,
|
||||
error: lyricsError,
|
||||
errorMessage: labels.rateLimited,
|
||||
labels,
|
||||
obscuredByQueue,
|
||||
returnFocusRef: lyricsToggleRef,
|
||||
|
|
@ -160,6 +168,7 @@ const usePlayerLyrics = ({
|
|||
audioInstance={audioInstance}
|
||||
loading={lyricsLoading}
|
||||
error={lyricsError}
|
||||
errorMessage={labels.rateLimited}
|
||||
labels={labels}
|
||||
inline
|
||||
/>
|
||||
|
|
|
|||
|
|
@ -660,6 +660,7 @@
|
|||
"lyricsTitleText": "Lyrics",
|
||||
"lyricsLoadingText": "Loading lyrics",
|
||||
"lyricsUnavailableText": "Lyrics unavailable",
|
||||
"lyricsRateLimitedText": "Remote lyrics are rate-limited. Retrying in %{smart_count} second. |||| Remote lyrics are rate-limited. Retrying in %{smart_count} seconds.",
|
||||
"resizeLyricsSidebarText": "Resize lyrics sidebar",
|
||||
"showLyricsTranslationText": "Show translation",
|
||||
"hideLyricsTranslationText": "Hide translation",
|
||||
|
|
|
|||
|
|
@ -131,11 +131,24 @@ const getTopSongs = (artist, count = 50) => {
|
|||
return httpClient(url('getTopSongs', null, { artist, count }))
|
||||
}
|
||||
|
||||
const requireSuccessfulLyricsResponse = (response) => {
|
||||
const subsonicResponse = response?.json?.['subsonic-response']
|
||||
if (subsonicResponse?.status !== 'failed') return response
|
||||
|
||||
const error = new Error(
|
||||
subsonicResponse.error?.message || 'Lyrics request failed',
|
||||
)
|
||||
error.body = response.json
|
||||
error.code = subsonicResponse.error?.code
|
||||
throw error
|
||||
}
|
||||
|
||||
const getLyricsBySongId = (id, requestOptions) => {
|
||||
const requestUrl = url('getLyricsBySongId', id, { enhanced: true })
|
||||
return requestOptions
|
||||
const request = requestOptions
|
||||
? httpClient(requestUrl, requestOptions)
|
||||
: httpClient(requestUrl)
|
||||
return request.then(requireSuccessfulLyricsResponse)
|
||||
}
|
||||
|
||||
const streamUrl = (id, options) => {
|
||||
|
|
|
|||
|
|
@ -207,6 +207,25 @@ describe('getLyricsBySongId', () => {
|
|||
expect(requestedUrl).toContain('id=song-123')
|
||||
expect(requestedUrl).toContain('enhanced=true')
|
||||
})
|
||||
|
||||
it('rejects a failed Subsonic lyrics response with its server message', async () => {
|
||||
httpClient.mockResolvedValueOnce({
|
||||
json: {
|
||||
'subsonic-response': {
|
||||
status: 'failed',
|
||||
error: {
|
||||
code: 0,
|
||||
message:
|
||||
'Internal Server Error: better lyrics API cooldown active for 30s',
|
||||
},
|
||||
},
|
||||
},
|
||||
})
|
||||
|
||||
await expect(subsonic.getLyricsBySongId('song-123')).rejects.toThrow(
|
||||
'better lyrics API cooldown active for 30s',
|
||||
)
|
||||
})
|
||||
})
|
||||
|
||||
describe('getDiscCoverArtUrl', () => {
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue