mirror of
https://github.com/navidrome/navidrome.git
synced 2026-10-08 02:17:25 +02:00
fix(podcast): address second round of CodeRabbit feedback
- Create the episode file only after a 200 response so failed downloads don't leave empty files behind - Reuse the existing StreamID as the MediaFile ID on re-download instead of inserting duplicate tracks - Link mobile podcast list rows to the channel show page - Ignore stale getPodcasts responses and clear episodes when the channel changes in PodcastShow
This commit is contained in:
parent
c6774f45e8
commit
6f49440b9c
4 changed files with 62 additions and 10 deletions
|
|
@ -348,13 +348,6 @@ func (s *podcastService) doDownload(ctx context.Context, ep *model.PodcastEpisod
|
|||
}
|
||||
|
||||
dest := filepath.Join(dir, ep.ID+"."+suffix)
|
||||
f, err := os.Create(dest)
|
||||
if err != nil {
|
||||
s.setEpisodeError(ctx, ep, err)
|
||||
return
|
||||
}
|
||||
defer f.Close()
|
||||
|
||||
if err := validateURL(ep.EnclosureURL); err != nil {
|
||||
s.setEpisodeError(ctx, ep, fmt.Errorf("invalid enclosure URL: %w", err))
|
||||
return
|
||||
|
|
@ -380,6 +373,15 @@ func (s *podcastService) doDownload(ctx context.Context, ep *model.PodcastEpisod
|
|||
return
|
||||
}
|
||||
|
||||
// Create the file only once we have a 200 response, so failed requests
|
||||
// don't leave empty files behind.
|
||||
f, err := os.Create(dest)
|
||||
if err != nil {
|
||||
s.setEpisodeError(ctx, ep, err)
|
||||
return
|
||||
}
|
||||
defer f.Close()
|
||||
|
||||
// Use Content-Length as total size when RSS feed didn't provide it
|
||||
if resp.ContentLength > 0 && ep.Size == 0 {
|
||||
ep.Size = resp.ContentLength
|
||||
|
|
@ -407,8 +409,14 @@ func (s *podcastService) doDownload(ctx context.Context, ep *model.PodcastEpisod
|
|||
now := time.Now()
|
||||
tags := model.Tags{}
|
||||
tags.Add("genre", "Podcast")
|
||||
// Reuse the existing MediaFile on re-download so Put updates it instead of
|
||||
// leaving a duplicate track behind.
|
||||
mfID := ep.StreamID
|
||||
if mfID == "" {
|
||||
mfID = id.NewRandom()
|
||||
}
|
||||
mf := &model.MediaFile{
|
||||
ID: id.NewRandom(),
|
||||
ID: mfID,
|
||||
LibraryID: libID,
|
||||
Path: relPath,
|
||||
Title: ep.Title,
|
||||
|
|
|
|||
|
|
@ -179,6 +179,21 @@ var _ = Describe("PodcastService", func() {
|
|||
Expect(found).To(BeTrue())
|
||||
})
|
||||
|
||||
It("reuses the existing StreamID's MediaFile on re-download", func() {
|
||||
_ = svc.DownloadEpisode(ctx, "ep-1")
|
||||
Eventually(func() model.PodcastStatus {
|
||||
return episodeRepo.Data["ep-1"].Status
|
||||
}, "3s").Should(Equal(model.PodcastStatusCompleted))
|
||||
first := episodeRepo.Data["ep-1"].StreamID
|
||||
Expect(first).ToNot(BeEmpty())
|
||||
|
||||
_ = svc.DownloadEpisode(ctx, "ep-1")
|
||||
Eventually(func() model.PodcastStatus {
|
||||
return episodeRepo.Data["ep-1"].Status
|
||||
}, "3s").Should(Equal(model.PodcastStatusCompleted))
|
||||
Expect(episodeRepo.Data["ep-1"].StreamID).To(Equal(first))
|
||||
})
|
||||
|
||||
It("records the file path after download", func() {
|
||||
_ = svc.DownloadEpisode(ctx, "ep-1")
|
||||
expectedPath := filepath.Join(conf.Server.DataFolder.String(), "podcasts", "ch-1", "ep-1.mp3")
|
||||
|
|
@ -236,6 +251,22 @@ var _ = Describe("PodcastService", func() {
|
|||
BeforeEach(func() {
|
||||
channelRepo.Data["ch-1"] = &model.PodcastChannel{ID: "ch-1", Title: "Test Channel"}
|
||||
})
|
||||
It("does not leave an empty file behind when the request fails", func() {
|
||||
episodeRepo.Data["ep-bad"] = &model.PodcastEpisode{
|
||||
ID: "ep-bad",
|
||||
ChannelID: "ch-1",
|
||||
EnclosureURL: "http://localhost:0/no-such.mp3",
|
||||
Suffix: "mp3",
|
||||
Status: model.PodcastStatusNew,
|
||||
}
|
||||
_ = svc.DownloadEpisode(ctx, "ep-bad")
|
||||
Eventually(func() model.PodcastStatus {
|
||||
return episodeRepo.Data["ep-bad"].Status
|
||||
}, "3s").Should(Equal(model.PodcastStatusError))
|
||||
dest := filepath.Join(conf.Server.DataFolder.String(), "podcasts", "ch-1", "ep-bad.mp3")
|
||||
_, err := os.Stat(dest)
|
||||
Expect(os.IsNotExist(err)).To(BeTrue())
|
||||
})
|
||||
It("sets status to error when download fails", func() {
|
||||
episodeRepo.Data["ep-bad"] = &model.PodcastEpisode{
|
||||
ID: "ep-bad",
|
||||
|
|
|
|||
|
|
@ -179,6 +179,7 @@ const PodcastList = ({ permissions, ...props }) => {
|
|||
>
|
||||
{isXsmall ? (
|
||||
<SimpleList
|
||||
linkType="show"
|
||||
leftAvatar={(r) => <CoverArtField record={r} />}
|
||||
primaryText={(r) => r.title}
|
||||
secondaryText={(r) => r.url}
|
||||
|
|
|
|||
|
|
@ -1,4 +1,4 @@
|
|||
import React, { useEffect, useState } from 'react'
|
||||
import React, { useEffect, useRef, useState } from 'react'
|
||||
import {
|
||||
Card,
|
||||
CardContent,
|
||||
|
|
@ -110,11 +110,17 @@ const PodcastShow = (props) => {
|
|||
const { record } = useShowController(props)
|
||||
const [episodes, setEpisodes] = useState([])
|
||||
|
||||
// Incremented on every load and on channel change; responses from a
|
||||
// superseded request are ignored so they can't overwrite the current channel.
|
||||
const requestGen = useRef(0)
|
||||
|
||||
const loadEpisodes = () => {
|
||||
if (!record?.id) return
|
||||
const gen = ++requestGen.current
|
||||
subsonic
|
||||
.getPodcasts(record.id, true)
|
||||
.then((res) => {
|
||||
if (gen !== requestGen.current) return
|
||||
const channels = res?.json?.['subsonic-response']?.podcasts?.channel || []
|
||||
const ch = channels.find((c) => c.id === record.id)
|
||||
setEpisodes(ch?.episode || [])
|
||||
|
|
@ -122,7 +128,13 @@ const PodcastShow = (props) => {
|
|||
.catch(() => {})
|
||||
}
|
||||
|
||||
useEffect(loadEpisodes, [record?.id])
|
||||
useEffect(() => {
|
||||
setEpisodes([]) // don't show the previous channel's episodes
|
||||
loadEpisodes()
|
||||
return () => {
|
||||
requestGen.current++
|
||||
}
|
||||
}, [record?.id])
|
||||
|
||||
// Stay subscribed to SSE progress while the view is mounted: a download
|
||||
// started from here may not be flagged 'downloading' yet when we refresh.
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue