fix(share): give visual feedback when downloading from a share (#5865)

* fix(share): give visual feedback when downloading a share

The share page handed the download URL to navidrome-music-player, which fell
through to downloadjs and buffered the whole ZIP into memory via XHR before
saving it. Nothing was handed to the browser until the last byte arrived, so a
large share produced a long silent window with no player feedback and no
browser download UI, inviting repeat clicks that each spawn another server-side
zip+transcode.

Use the player's customDownloader prop to trigger a synthetic anchor instead,
so the browser performs the download and reports its own progress. An anchor
rather than assigning window.location.href: the share page's service worker
registers a NavigationRoute over all navigations, which intercepts the streamed
archive and fails it into the offline fallback (observed as HTTP 503 in Chrome).

handleDownloads now loads the share before streaming so it can set
Content-Disposition and Content-Type. This also fixes error reporting: ZipShare
previously wrote to the ResponseWriter before checkShareError ran, locking the
status at 200, so expired, missing and non-downloadable shares all returned 200.
They now correctly return 410, 404 and 403.

* feat(share): acknowledge the download click in the player

The browser's download UI is the real progress indicator, but nothing in the
page itself reacted to the click, so the moment before the browser catches up
still read as unresponsive. Dim the download button and make it unclickable for
two seconds after a download starts, reusing the JSS function-value pattern the
existing single-track styling already uses.

A repeat download restarts the window instead of extending the original, and
the timer is cleared on unmount. This also blunts repeat clicking, where every
extra click costs another server-side zip and transcode.

Add SharePlayer tests covering the download mechanism and this state machine.
The dimming itself is verified in a browser rather than jsdom: JSS function
values are not evaluated there, so the rule is never emitted and a CSS
assertion would pass or fail for the wrong reason.

Signed-off-by: Deluan <deluan@navidrome.org>

* test(share): assert render counts in SharePlayer feedback tests

The two acknowledgement tests compared the props object across renders, which
React may reuse, so they passed without proving anything and then failed once
the surrounding assertions changed. Count renders instead, and let the pending
timer run out rather than advancing exactly to its deadline, which does not
cross it.

The repeat-download test now also asserts that no render happens at the
original deadline, proving the timer was replaced rather than merely that one
eventually fired.

* fix(share): count one visit per share download

The preflight share load added in this branch made every download record two
visits: handleDownloads called Share.Load, and ZipShare then loaded the share
again internally. Share.Load increments and persists VisitCount, so the counter
advanced twice per download and the repository work was duplicated.

Pass the already-loaded share into ZipShare instead of its id. handleDownloads
is its only production caller, and it now has the share in hand for the
Content-Disposition header anyway.

The archiver test asserts Load is not called, so the double-load cannot come
back unnoticed. Verified against a running server: the counter now advances by
one per download.

* test(share): derive feedback-window timings from the constant

The acknowledgement tests hardcoded clock advances tuned to a 2000ms window.
Raising DOWNLOAD_FEEDBACK_MS to 5000 left them advancing 1500ms and 1001ms,
which no longer reach the deadline they are meant to cross, so the repeat-
download test passed without proving the timer had been replaced.

Export the constant and derive the advances from it, and let the pending timer
run out in the unmount test rather than advancing a fixed amount. Changing the
duration can no longer silently strand a test short of its deadline.

---------

Signed-off-by: Deluan <deluan@navidrome.org>

core/archiver.go

@@ -21,7 +21,7 @@ import (
 type Archiver interface {
 	ZipAlbum(ctx context.Context, id string, format string, bitrate int, w io.Writer) error
 	ZipArtist(ctx context.Context, id string, format string, bitrate int, w io.Writer) error
-	ZipShare(ctx context.Context, id string, w io.Writer) error
+	ZipShare(ctx context.Context, s *model.Share, w io.Writer) error
 	ZipPlaylist(ctx context.Context, id string, format string, bitrate int, w io.Writer) error
 }
 
@@ -100,16 +100,14 @@ func (a *archiver) albumFilename(mf model.MediaFile, format string, isMultiDisc
 	return fmt.Sprintf("%s/%s", str.SanitizeFilename(mf.Album), file)
 }
 
-func (a *archiver) ZipShare(ctx context.Context, id string, out io.Writer) error {
-	s, err := a.shares.Load(ctx, id)
-	if err != nil {
-		return err
-	}
+// ZipShare takes an already-loaded share: Share.Load records a visit, so
+// loading it again here would count every download twice.
+func (a *archiver) ZipShare(ctx context.Context, s *model.Share, out io.Writer) error {
 	if !s.Downloadable {
 		return model.ErrNotAuthorized
 	}
 	log.Debug(ctx, "Zipping share", "name", s.ID, "format", s.Format, "bitrate", s.MaxBitRate, "numTracks", len(s.Tracks))
-	return a.zipMediaFiles(ctx, id, s.ID, s.Format, s.MaxBitRate, out, s.Tracks, false)
+	return a.zipMediaFiles(ctx, s.ID, s.ID, s.Format, s.MaxBitRate, out, s.Tracks, false)
 }
 
 func (a *archiver) ZipPlaylist(ctx context.Context, id string, format string, bitrate int, out io.Writer) error {

core/archiver_test.go

@@ -130,13 +130,16 @@ var _ = Describe("Archiver", func() {
 				Tracks:       mfs,
 			}
 
-			sh.On("Load", mock.Anything, "1").Return(share, nil)
 			ms.On("NewStream", mock.Anything, mock.Anything, stream.Request{Format: "mp3", BitRate: 128}).Return(io.NopCloser(strings.NewReader("test")), nil).Times(2)
 
 			out := new(bytes.Buffer)
-			err := arch.ZipShare(context.Background(), "1", out)
+			err := arch.ZipShare(context.Background(), share, out)
 			Expect(err).To(BeNil())
 
+			// Share.Load records a visit; re-loading here would double-count
+			// every download.
+			sh.AssertNotCalled(GinkgoT(), "Load", mock.Anything, mock.Anything)
+
 			zr, err := zip.NewReader(bytes.NewReader(out.Bytes()), int64(out.Len()))
 			Expect(err).To(BeNil())
 

server/public/handle_downloads.go

@@ -1,18 +1,41 @@
 package public
 
 import (
+	"cmp"
+	"fmt"
 	"net/http"
+	"strings"
 
+	"github.com/navidrome/navidrome/model"
 	"github.com/navidrome/navidrome/utils/req"
+	"github.com/navidrome/navidrome/utils/str"
 )
 
 func (pub *Router) handleDownloads(w http.ResponseWriter, r *http.Request) {
+	ctx := r.Context()
 	id, err := req.Params(r).String(":id")
 	if err != nil {
 		http.Error(w, err.Error(), http.StatusBadRequest)
 		return
 	}
 
-	err = pub.archiver.ZipShare(r.Context(), id, w)
-	checkShareError(r.Context(), w, err, id)
+	// Load the share before streaming: once ZipShare writes its first byte the
+	// status is locked at 200, so errors could no longer be reported.
+	s, err := pub.share.Load(ctx, id)
+	if err != nil {
+		checkShareError(ctx, w, err, id)
+		return
+	}
+	if !s.Downloadable {
+		checkShareError(ctx, w, model.ErrNotAuthorized, id)
+		return
+	}
+
+	name := str.SanitizeFilename(cmp.Or(s.Description, s.ID))
+	name = strings.ReplaceAll(name, ",", "_")
+	w.Header().Set("Content-Disposition", fmt.Sprintf("attachment; filename=%q", name+".zip"))
+	w.Header().Set("Content-Type", "application/zip")
+
+	err = pub.archiver.ZipShare(ctx, s, w)
+	checkShareError(ctx, w, err, id)
 }

server/public/handle_downloads_test.go

@@ -0,0 +1,134 @@
+package public
+
+import (
+	"context"
+	"errors"
+	"io"
+	"net/http"
+	"net/http/httptest"
+	"time"
+
+	"github.com/navidrome/navidrome/core"
+	"github.com/navidrome/navidrome/model"
+	"github.com/navidrome/navidrome/tests"
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+)
+
+type mockArchiver struct {
+	called bool
+	err    error
+}
+
+func (m *mockArchiver) ZipAlbum(context.Context, string, string, int, io.Writer) error {
+	return nil
+}
+
+func (m *mockArchiver) ZipArtist(context.Context, string, string, int, io.Writer) error {
+	return nil
+}
+
+func (m *mockArchiver) ZipPlaylist(context.Context, string, string, int, io.Writer) error {
+	return nil
+}
+
+func (m *mockArchiver) ZipShare(_ context.Context, _ *model.Share, w io.Writer) error {
+	m.called = true
+	if m.err != nil {
+		return m.err
+	}
+	_, _ = w.Write([]byte("zip-contents"))
+	return nil
+}
+
+var _ = Describe("handleDownloads", func() {
+	var ds *tests.MockDataStore
+	var shareRepo *tests.MockShareRepo
+	var archiver *mockArchiver
+	var pub *Router
+
+	BeforeEach(func() {
+		ds = &tests.MockDataStore{}
+		shareRepo = &tests.MockShareRepo{}
+		ds.MockedShare = shareRepo
+		archiver = &mockArchiver{}
+		pub = &Router{ds: ds, archiver: archiver, share: core.NewShare(ds)}
+	})
+
+	shareIs := func(s *model.Share) {
+		shareRepo.ID = s.ID
+		shareRepo.Entity = s
+	}
+
+	makeRequest := func(id string) *httptest.ResponseRecorder {
+		r := httptest.NewRequest("GET", "/public/d/"+id+"?%3Aid="+id, nil)
+		w := httptest.NewRecorder()
+		pub.handleDownloads(w, r)
+		return w
+	}
+
+	It("sets a Content-Disposition filename from the share description", func() {
+		shareIs(&model.Share{ID: "abc123", Description: "My Mixtape", Downloadable: true})
+
+		w := makeRequest("abc123")
+
+		Expect(w.Code).To(Equal(http.StatusOK))
+		Expect(w.Header().Get("Content-Disposition")).To(Equal(`attachment; filename="My Mixtape.zip"`))
+		Expect(w.Header().Get("Content-Type")).To(Equal("application/zip"))
+		Expect(archiver.called).To(BeTrue())
+		Expect(w.Body.String()).To(Equal("zip-contents"))
+	})
+
+	It("falls back to the share ID when there is no description", func() {
+		shareIs(&model.Share{ID: "abc123", Downloadable: true})
+
+		w := makeRequest("abc123")
+
+		Expect(w.Header().Get("Content-Disposition")).To(Equal(`attachment; filename="abc123.zip"`))
+	})
+
+	It("sanitizes characters that are unsafe in a filename", func() {
+		shareIs(&model.Share{ID: "abc123", Description: `AC/DC: Live, 1979`, Downloadable: true})
+
+		w := makeRequest("abc123")
+
+		Expect(w.Header().Get("Content-Disposition")).To(Equal(`attachment; filename="AC_DC_ Live_ 1979.zip"`))
+	})
+
+	It("returns 403 without invoking the archiver when the share is not downloadable", func() {
+		shareIs(&model.Share{ID: "abc123", Description: "No Download", Downloadable: false})
+
+		w := makeRequest("abc123")
+
+		Expect(w.Code).To(Equal(http.StatusForbidden))
+		Expect(archiver.called).To(BeFalse())
+		Expect(w.Header().Get("Content-Disposition")).To(BeEmpty())
+	})
+
+	It("returns 404 when the share does not exist", func() {
+		shareIs(&model.Share{ID: "other", Downloadable: true})
+
+		w := makeRequest("missing")
+
+		Expect(w.Code).To(Equal(http.StatusNotFound))
+		Expect(archiver.called).To(BeFalse())
+	})
+
+	It("returns 410 when the share has expired", func() {
+		shareIs(&model.Share{ID: "abc123", Downloadable: true, ExpiresAt: new(time.Now().Add(-time.Hour))})
+
+		w := makeRequest("abc123")
+
+		Expect(w.Code).To(Equal(http.StatusGone))
+		Expect(archiver.called).To(BeFalse())
+	})
+
+	It("returns 500 when the share lookup fails", func() {
+		shareRepo.Error = errors.New("db error")
+
+		w := makeRequest("abc123")
+
+		Expect(w.Code).To(Equal(http.StatusInternalServerError))
+		Expect(archiver.called).To(BeFalse())
+	})
+})

server/subsonic/e2e/e2e_suite_test.go

@@ -330,7 +330,7 @@ func (n noopArchiver) ZipArtist(context.Context, string, string, int, io.Writer)
 	return model.ErrNotFound
 }
 
-func (n noopArchiver) ZipShare(context.Context, string, io.Writer) error {
+func (n noopArchiver) ZipShare(context.Context, *model.Share, io.Writer) error {
 	return model.ErrNotFound
 }
 

ui/src/share/SharePlayer.jsx

@@ -1,15 +1,24 @@
 import ReactJkMusicPlayer from 'navidrome-music-player'
+import { useCallback, useEffect, useRef, useState } from 'react'
 import config, { shareInfo } from '../config'
 import { shareCoverUrl, shareDownloadUrl, shareStreamUrl } from '../utils'
 
 import { makeStyles } from '@material-ui/core/styles'
 
+// How long the download button stays inert after a click. The browser needs a
+// moment to show its own download UI; until then the page looks unresponsive.
+export const DOWNLOAD_FEEDBACK_MS = 5000
+
 const useStyle = makeStyles({
   player: {
     '& .group .next-audio': {
       pointerEvents: (props) => props.single && 'none',
       opacity: (props) => props.single && 0.65,
     },
+    '& .group.audio-download': {
+      pointerEvents: (props) => props.downloading && 'none',
+      opacity: (props) => props.downloading && 0.65,
+    },
     '@media (min-width: 768px)': {
       '& .react-jinke-music-player-mobile > div': {
         width: 768,
@@ -23,7 +32,14 @@ const useStyle = makeStyles({
 })
 
 const SharePlayer = () => {
-  const classes = useStyle({ single: shareInfo?.tracks.length === 1 })
+  const [downloading, setDownloading] = useState(false)
+  const timer = useRef(null)
+  const classes = useStyle({
+    single: shareInfo?.tracks.length === 1,
+    downloading,
+  })
+
+  useEffect(() => () => clearTimeout(timer.current), [])
 
   const list = shareInfo?.tracks.map((s) => {
     return {
@@ -34,11 +50,23 @@ const SharePlayer = () => {
       duration: s.duration,
     }
   })
-  const onBeforeAudioDownload = () => {
-    return Promise.resolve({
-      src: shareDownloadUrl(shareInfo?.id),
-    })
-  }
+  // An anchor, not a navigation: the service worker's NavigationRoute would
+  // intercept the streamed archive and fail it.
+  const customDownloader = useCallback(() => {
+    const link = document.createElement('a')
+    link.href = shareDownloadUrl(shareInfo?.id)
+    link.download = ''
+    document.body.appendChild(link)
+    link.click()
+    document.body.removeChild(link)
+
+    setDownloading(true)
+    clearTimeout(timer.current)
+    timer.current = setTimeout(
+      () => setDownloading(false),
+      DOWNLOAD_FEEDBACK_MS,
+    )
+  }, [])
   const options = {
     audioLists: list,
     mode: 'full',
@@ -59,7 +87,7 @@ const SharePlayer = () => {
     <ReactJkMusicPlayer
       {...options}
       className={classes.player}
-      onBeforeAudioDownload={onBeforeAudioDownload}
+      customDownloader={customDownloader}
     />
   )
 }

ui/src/share/SharePlayer.test.jsx

@@ -0,0 +1,139 @@
+import { render, act } from '@testing-library/react'
+import SharePlayer, { DOWNLOAD_FEEDBACK_MS } from './SharePlayer'
+
+let playerProps
+let renderCount
+
+vi.mock('navidrome-music-player', () => ({
+  default: (props) => {
+    playerProps = props
+    renderCount++
+    return <div data-testid="player" />
+  },
+}))
+
+vi.mock('../config', () => ({
+  default: { enableDownloads: true },
+  shareInfo: {
+    id: 'share-1',
+    downloadable: true,
+    tracks: [{ id: 't1', title: 'One', artist: 'A', duration: 100 }],
+  },
+}))
+
+vi.mock('../utils', () => ({
+  shareDownloadUrl: (id) => `/share/d/${id}`,
+  shareStreamUrl: (id) => `/share/s/${id}`,
+  shareCoverUrl: (id) => `/share/img/${id}`,
+}))
+
+describe('SharePlayer', () => {
+  let clickSpy
+
+  beforeEach(() => {
+    vi.useFakeTimers()
+    playerProps = null
+    renderCount = 0
+    // Downloading for real would navigate the jsdom window.
+    clickSpy = vi
+      .spyOn(HTMLAnchorElement.prototype, 'click')
+      .mockImplementation(() => {})
+  })
+
+  afterEach(() => {
+    vi.runOnlyPendingTimers()
+    vi.useRealTimers()
+    vi.restoreAllMocks()
+  })
+
+  it('downloads via an anchor so the service worker does not intercept it', () => {
+    render(<SharePlayer />)
+
+    let anchor
+    clickSpy.mockImplementation(function () {
+      anchor = { href: this.href, download: this.download }
+    })
+
+    act(() => {
+      playerProps.customDownloader()
+    })
+
+    expect(clickSpy).toHaveBeenCalledTimes(1)
+    expect(anchor.href).toContain('/share/d/share-1')
+    // Empty, so the server's Content-Disposition filename wins.
+    expect(anchor.download).toBe('')
+  })
+
+  it('removes the anchor from the document after clicking', () => {
+    render(<SharePlayer />)
+
+    act(() => {
+      playerProps.customDownloader()
+    })
+
+    expect(document.querySelectorAll('a[download]')).toHaveLength(0)
+  })
+
+  // The inert styling itself is driven by JSS function values, which jsdom does
+  // not evaluate; it is verified in a browser. What is checked here is the
+  // state machine feeding it -- that the component re-renders on download and
+  // again when the window closes.
+  it('re-renders when the feedback window opens and closes', () => {
+    render(<SharePlayer />)
+
+    const beforeDownload = renderCount
+    act(() => {
+      playerProps.customDownloader()
+    })
+    expect(renderCount).toBeGreaterThan(beforeDownload)
+
+    const beforeExpiry = renderCount
+    act(() => {
+      vi.runAllTimers()
+    })
+    expect(renderCount).toBeGreaterThan(beforeExpiry)
+  })
+
+  it('restarts the feedback window on a repeat download', () => {
+    render(<SharePlayer />)
+
+    act(() => {
+      playerProps.customDownloader()
+    })
+    const elapsedBeforeRepeat = Math.floor(DOWNLOAD_FEEDBACK_MS / 2)
+    act(() => {
+      vi.advanceTimersByTime(elapsedBeforeRepeat)
+    })
+    act(() => {
+      playerProps.customDownloader()
+    })
+
+    // Past the first timer's deadline, which it would have fired at had the
+    // repeat download not replaced it.
+    const beforeOriginalDeadline = renderCount
+    act(() => {
+      vi.advanceTimersByTime(DOWNLOAD_FEEDBACK_MS - elapsedBeforeRepeat + 1)
+    })
+    expect(renderCount).toBe(beforeOriginalDeadline)
+
+    act(() => {
+      vi.runAllTimers()
+    })
+    expect(renderCount).toBeGreaterThan(beforeOriginalDeadline)
+  })
+
+  it('does not update state after unmount', () => {
+    const { unmount } = render(<SharePlayer />)
+    const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {})
+
+    act(() => {
+      playerProps.customDownloader()
+    })
+    unmount()
+    act(() => {
+      vi.runAllTimers()
+    })
+
+    expect(errorSpy).not.toHaveBeenCalled()
+  })
+})