Complete s9 + aa: NewServerWithLogger returns error; share-page renderer extracted

Two related refactors that converged on internal/api/server.go.

s9 — NewServerWithLogger no longer panics on nil deps.Config. Returns
(*Server, error) so cmd/player/main.go can log a clear message and exit
cleanly when wiring is misconfigured. All callers updated, including
api unit tests that construct a Server directly.

aa — Share-page HTML rendering moved out of handlers_share.go into a
new internal/web package. internal/web/sharepage.go owns
SharePageRenderer and the private injectShareMedia helper; the api
handler now calls s.shareRenderer.Render(...) and routes errors via
http.Error. injectShareMedia coverage was moved alongside it in
internal/web/sharepage_test.go; the duplicate tests in
handlers_more_test.go were removed (placeholder comment left so the
reference is greppable). Server gained one new field (shareRenderer
*web.SharePageRenderer) and one new constructor line; production
wiring uses deps.StaticFS.

Background: both tasks were initially worked by separate sub-agents
that stopped mid-edit when usage limits hit; their WIP was preserved
in a stash and finalized in this commit. Tests rerun from a clean
state: full Go unit suite green, 25/25 LLM e2e, 22/22 Playwright.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

11 files changed, 264 insertions, 85 deletions

diff --git a/player-server/cmd/player/main.go b/player-server/cmd/player/main.go
index 8835a1a..8b80d8d 100644
--- a/player-server/cmd/player/main.go
+++ b/player-server/cmd/player/main.go
@@ -246,7 +246,7 @@ func runWithSignal(args []string, sigCh <-chan os.Signal) error {
 	staticFS := http.Dir("web")
 	remuxer := probe.NewFFRemuxer()
 	streamer := service.NewMediaStreamer(remuxer)
-	server := api.NewServerWithLogger(api.ServerDeps{
+	server, err := api.NewServerWithLogger(api.ServerDeps{
 		Store:          store,
 		Hasher:         deps.hasher,
 		SessionManager: deps.sm,
@@ -267,6 +267,9 @@ func runWithSignal(args []string, sigCh <-chan os.Signal) error {
 		StaticFS:      staticFS,
 		MediaStreamer: streamer,
 	}, logger)
+	if err != nil {
+		return fmt.Errorf("failed to create API server: %w", err)
+	}
 
 	return runServer(server, cfg, logger, sigCh)
 }
diff --git a/player-server/internal/api/handlers_more_test.go b/player-server/internal/api/handlers_more_test.go
index 5ae71bb..4c441cd 100644
--- a/player-server/internal/api/handlers_more_test.go
+++ b/player-server/internal/api/handlers_more_test.go
@@ -1209,43 +1209,9 @@ func TestServer_SharePage(t *testing.T) {
 	})
 }
 
-func TestInjectShareMedia(t *testing.T) {
-	t.Run("injects marshaled metadata", func(t *testing.T) {
-		html, err := injectShareMedia(`<script><!--SHARE_MEDIA--></script>`, map[string]string{"stream_url": "/s/abc/stream"})
-		if err != nil {
-			t.Fatalf("expected no error, got %v", err)
-		}
-		if !strings.Contains(html, `"stream_url":"/s/abc/stream"`) {
-			t.Fatalf("expected injected JSON, got %q", html)
-		}
-	})
-
-	t.Run("omits empty thumbnail url", func(t *testing.T) {
-		html, err := injectShareMedia(`<script><!--SHARE_MEDIA--></script>`, service.GetSharedMediaResult{
-			Media:       &service.SharedMediaView{ID: 1, FileName: "share.mp3", Type: model.MediaTypeAudio},
-			HasThumb:    false,
-			StreamURL:   "/s/abc/stream",
-			DownloadURL: "/s/abc/download",
-			ThumbURL:    "",
-		})
-		if err != nil {
-			t.Fatalf("expected no error, got %v", err)
-		}
-		if strings.Contains(html, "thumb_url") {
-			t.Fatalf("expected no thumb_url in injected JSON, got %q", html)
-		}
-	})
-
-	t.Run("returns marshal error", func(t *testing.T) {
-		_, err := injectShareMedia(`<script><!--SHARE_MEDIA--></script>`, map[string]any{"bad": make(chan int)})
-		if err == nil {
-			t.Fatal("expected marshal error")
-		}
-		if !strings.Contains(err.Error(), "marshal share metadata") {
-			t.Fatalf("expected wrapped marshal error, got %v", err)
-		}
-	})
-}
+// TestInjectShareMedia coverage moved to internal/web/sharepage_test.go
+// when share-page rendering was extracted into the internal/web package
+// (task aa). The api package no longer owns the injection helper.
 
 func TestServer_ShareStream(t *testing.T) {
 	path := makeTempFile(t, "shared")
diff --git a/player-server/internal/api/handlers_playback_test.go b/player-server/internal/api/handlers_playback_test.go
index 5b14b39..7febab0 100644
--- a/player-server/internal/api/handlers_playback_test.go
+++ b/player-server/internal/api/handlers_playback_test.go
@@ -28,7 +28,9 @@ func newPlaybackTestServer(t *testing.T, store repository.Store, sm auth.Session
 		CountUsersFunc:  func(context.Context) (int, error) { return 1, nil },
 		GetUserByIDFunc: func(context.Context, int64) (*model.User, error) { return &model.User{ID: 1, IsAdmin: true}, nil },
 	}
-	return NewServer(ServerDeps{
+	// NewServer now returns (*Server, error); we pass a non-nil Config here,
+	// so a failure points to a wiring bug in the test setup.
+	srv, err := NewServer(ServerDeps{
 		Store:          store,
 		SessionManager: sm,
 		Config:         &internal.Config{},
@@ -38,6 +40,10 @@ func newPlaybackTestServer(t *testing.T, store repository.Store, sm auth.Session
 		},
 		StaticFS: fs,
 	})
+	if err != nil {
+		t.Fatalf("NewServer: %v", err)
+	}
+	return srv
 }
 
 // sessionForPlaybackTest creates a session cookie that resolves to userID 1.
diff --git a/player-server/internal/api/handlers_podcast_test.go b/player-server/internal/api/handlers_podcast_test.go
index 4836218..4138d97 100644
--- a/player-server/internal/api/handlers_podcast_test.go
+++ b/player-server/internal/api/handlers_podcast_test.go
@@ -49,7 +49,9 @@ func newPodcastTestServer(t *testing.T, store repository.Store, hasher auth.Hash
 			GetUserByIDFunc: func(context.Context, int64) (*model.User, error) { return &model.User{ID: 1, IsAdmin: true}, nil },
 		}
 	}
-	return NewServer(ServerDeps{
+	// NewServer now returns (*Server, error); tests always supply Config,
+	// so the only realistic cause of failure is a programming mistake.
+	srv, err := NewServer(ServerDeps{
 		Store:          store,
 		Hasher:         hasher,
 		SessionManager: sm,
@@ -68,6 +70,10 @@ func newPodcastTestServer(t *testing.T, store repository.Store, hasher auth.Hash
 		},
 		StaticFS: fs,
 	})
+	if err != nil {
+		t.Fatalf("NewServer: %v", err)
+	}
+	return srv
 }
 
 // setupPodcastE2E creates a full server with a real SQLite store and real services.
diff --git a/player-server/internal/api/handlers_share.go b/player-server/internal/api/handlers_share.go
index 5c7db83..50ad3ef 100644
--- a/player-server/internal/api/handlers_share.go
+++ b/player-server/internal/api/handlers_share.go
@@ -1,10 +1,7 @@
 package api
 
 import (
-	"encoding/json"
 	"errors"
-	"fmt"
-	"io"
 	"net/http"
 	"strings"
 	"time"
@@ -91,39 +88,17 @@ func (s *Server) handleSharePage(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	// Serve HTML page with media metadata injected.
-	f, err := s.staticFS.Open("share.html")
+	// Render the HTML view via the dedicated renderer. This keeps the
+	// handler focused on transport concerns (status codes, headers) and
+	// keeps templating in the internal/web package.
+	page, err := s.shareRenderer.Render(res)
 	if err != nil {
-		http.Error(w, "not found", http.StatusNotFound)
-		return
-	}
-	defer f.Close()
-	stat, err := f.Stat()
-	if err != nil {
-		http.Error(w, "not found", http.StatusNotFound)
-		return
-	}
-	var buf strings.Builder
-	if _, err := io.Copy(&buf, f); err != nil {
-		http.Error(w, "internal error", http.StatusInternalServerError)
-		return
-	}
-	html, err := injectShareMedia(buf.String(), res)
-	if err != nil {
-		s.logger.Error("marshal share page metadata", "err", err)
+		s.logger.Error("render share page", "err", err)
 		http.Error(w, "internal error", http.StatusInternalServerError)
 		return
 	}
 	w.Header().Set("Content-Type", "text/html; charset=utf-8")
-	http.ServeContent(w, r, "share.html", stat.ModTime(), strings.NewReader(html))
-}
-
-func injectShareMedia(html string, data any) (string, error) {
-	encoded, err := json.Marshal(data)
-	if err != nil {
-		return "", fmt.Errorf("marshal share metadata: %w", err)
-	}
-	return strings.Replace(html, "<!--SHARE_MEDIA-->", string(encoded), 1), nil
+	http.ServeContent(w, r, page.Name, page.ModTime, strings.NewReader(page.HTML))
 }
 
 func (s *Server) handleShareThumbnail(w http.ResponseWriter, r *http.Request) {
diff --git a/player-server/internal/api/handlers_test.go b/player-server/internal/api/handlers_test.go
index fea6861..2c662b3 100644
--- a/player-server/internal/api/handlers_test.go
+++ b/player-server/internal/api/handlers_test.go
@@ -65,7 +65,10 @@ func newTestServer(t *testing.T, store repository.Store, hasher auth.Hasher, sm
 	if len(streamer) > 0 {
 		mediaStreamer = streamer[0]
 	}
-	return NewServer(ServerDeps{
+	// NewServer now returns an error when required deps (e.g. Config) are
+	// missing. Tests always pass a non-nil Config, so a failure here indicates
+	// a programming mistake in the test setup itself.
+	srv, err := NewServer(ServerDeps{
 		Store:          store,
 		Hasher:         hasher,
 		SessionManager: sm,
@@ -84,6 +87,10 @@ func newTestServer(t *testing.T, store repository.Store, hasher auth.Hasher, sm
 		StaticFS:      fs,
 		MediaStreamer: mediaStreamer,
 	})
+	if err != nil {
+		t.Fatalf("NewServer: %v", err)
+	}
+	return srv
 }
 
 func addSessionCookie(t *testing.T, store repository.Store, sm auth.SessionManager, userID int64) *http.Cookie {
diff --git a/player-server/internal/api/integration_test.go b/player-server/internal/api/integration_test.go
index c5e7d18..2f51bd2 100644
--- a/player-server/internal/api/integration_test.go
+++ b/player-server/internal/api/integration_test.go
@@ -63,7 +63,9 @@ func newIntegrationServer(t *testing.T) *integrationEnv {
 		"share.html":     {Data: []byte("share")},
 	}
 
-	srv := NewServer(ServerDeps{
+	// NewServer now returns (*Server, error); cfg is always provided here so a
+	// failure indicates a wiring bug in the test setup.
+	srv, err := NewServer(ServerDeps{
 		Store:          store,
 		Hasher:         hasher,
 		SessionManager: sm,
@@ -82,6 +84,9 @@ func newIntegrationServer(t *testing.T) *integrationEnv {
 		},
 		StaticFS: http.FS(staticFS),
 	})
+	if err != nil {
+		t.Fatalf("NewServer: %v", err)
+	}
 
 	return &integrationEnv{srv: srv}
 }
diff --git a/player-server/internal/api/server.go b/player-server/internal/api/server.go
index 608e934..2fccf32 100644
--- a/player-server/internal/api/server.go
+++ b/player-server/internal/api/server.go
@@ -2,6 +2,7 @@ package api
 
 import (
 	"context"
+	"errors"
 	"log/slog"
 	"net/http"
 	"strconv"
@@ -12,6 +13,7 @@ import (
 	"codeberg.org/snonux/player/internal/auth"
 	"codeberg.org/snonux/player/internal/repository"
 	"codeberg.org/snonux/player/internal/service"
+	"codeberg.org/snonux/player/internal/web"
 )
 
 // Server holds HTTP handlers and dependencies.
@@ -35,6 +37,7 @@ type Server struct {
 	playbackHintSvc service.PlaybackHintsService
 	streamer        service.MediaStreamer
 	staticFS        http.FileSystem
+	shareRenderer   *web.SharePageRenderer
 	logger          *slog.Logger
 	mw              *Middleware
 }
@@ -67,14 +70,20 @@ type ServerDeps struct {
 }
 
 // NewServer creates a Server with routes.
-func NewServer(deps ServerDeps) *Server {
+// It returns an error if required dependencies (e.g. Config) are missing
+// so callers can handle invalid input gracefully instead of crashing.
+func NewServer(deps ServerDeps) (*Server, error) {
 	return NewServerWithLogger(deps, slog.Default())
 }
 
 // NewServerWithLogger creates a Server with routes and an injected logger.
-func NewServerWithLogger(deps ServerDeps, logger *slog.Logger) *Server {
+// It returns an error if deps.Config is nil; previously this case panicked,
+// but returning an error lets the caller (e.g. cmd/player/main.go) report
+// the failure cleanly and exit with a useful message rather than crashing
+// deep in the wiring code.
+func NewServerWithLogger(deps ServerDeps, logger *slog.Logger) (*Server, error) {
 	if deps.Config == nil {
-		panic("api.NewServerWithLogger: Config is nil")
+		return nil, errors.New("api.NewServerWithLogger: Config is nil")
 	}
 	if deps.StaticFS == nil {
 		deps.StaticFS = http.Dir("web")
@@ -101,12 +110,13 @@ func NewServerWithLogger(deps ServerDeps, logger *slog.Logger) *Server {
 		playbackHintSvc: deps.Services.PlaybackHints,
 		streamer:        deps.MediaStreamer,
 		staticFS:        deps.StaticFS,
+		shareRenderer:   web.NewSharePageRenderer(deps.StaticFS),
 		logger:          logger,
 		mw:              NewMiddleware(deps.Services.Auth, deps.SessionManager),
 	}
 	s.routes()
 	s.handler = withCORS(s.cfg.CORSAllowedOrigins, s.mw.BootstrapRedirect(s.mux))
-	return s
+	return s, nil
 }
 
 // ServeHTTP implements http.Handler.
diff --git a/player-server/internal/api/server_test.go b/player-server/internal/api/server_test.go
index a3e84a7..91f793d 100644
--- a/player-server/internal/api/server_test.go
+++ b/player-server/internal/api/server_test.go
@@ -5,15 +5,18 @@ import (
 	"testing"
 )
 
-func TestNewServerWithLogger_PanicsOnNilConfig(t *testing.T) {
-	defer func() {
-		if r := recover(); r == nil {
-			t.Fatal("expected panic for nil Config, got none")
-		}
-	}()
-
-	NewServerWithLogger(ServerDeps{
+// TestNewServerWithLogger_ErrorsOnNilConfig verifies that the constructor
+// returns an error (rather than panicking) when deps.Config is nil. The
+// caller in cmd/player/main.go relies on this to fail gracefully.
+func TestNewServerWithLogger_ErrorsOnNilConfig(t *testing.T) {
+	srv, err := NewServerWithLogger(ServerDeps{
 		Config:   nil,
 		StaticFS: nil,
 	}, slog.Default())
+	if err == nil {
+		t.Fatal("expected error for nil Config, got nil")
+	}
+	if srv != nil {
+		t.Fatalf("expected nil Server on error, got %v", srv)
+	}
 }
diff --git a/player-server/internal/web/sharepage.go b/player-server/internal/web/sharepage.go
new file mode 100644
index 0000000..37844d8
--- /dev/null
+++ b/player-server/internal/web/sharepage.go
@@ -0,0 +1,106 @@
+// Package web renders HTML pages for the player-server.
+//
+// The share-page renderer encapsulates the templating concern that used
+// to live inline in the api package: opening the static share.html file,
+// replacing the SHARE_MEDIA placeholder with marshaled JSON metadata,
+// and reporting the file's ModTime for cache validators.
+//
+// Keeping this logic here lets HTTP handlers stay focused on routing and
+// error translation (Separation of Concerns) and stops them reaching
+// through a file-system abstraction (Law of Demeter).
+package web
+
+import (
+	"encoding/json"
+	"fmt"
+	"io"
+	"net/http"
+	"strings"
+	"time"
+)
+
+// ShareMediaPlaceholder is the HTML comment that gets substituted with
+// the JSON-encoded share metadata inside share.html.
+const ShareMediaPlaceholder = "<!--SHARE_MEDIA-->"
+
+// shareTemplateName is the filename looked up in the static FS.
+const shareTemplateName = "share.html"
+
+// SharePageRenderer renders the public share landing page by inlining
+// share metadata into a static HTML template.
+//
+// A renderer captures the file system and the placeholder it works with
+// so callers (HTTP handlers) only need to pass the data to inject.
+type SharePageRenderer struct {
+	fs          http.FileSystem
+	template    string
+	placeholder string
+}
+
+// NewSharePageRenderer builds a renderer backed by the given file system.
+// The file system must contain share.html. The placeholder defaults to
+// ShareMediaPlaceholder.
+func NewSharePageRenderer(fs http.FileSystem) *SharePageRenderer {
+	return &SharePageRenderer{
+		fs:          fs,
+		template:    shareTemplateName,
+		placeholder: ShareMediaPlaceholder,
+	}
+}
+
+// RenderedPage carries the bytes to serve along with the source template's
+// modification time (used for HTTP cache validators in ServeContent).
+type RenderedPage struct {
+	HTML    string
+	ModTime time.Time
+	Name    string
+}
+
+// Render reads the share template from the file system, injects the
+// JSON-encoded data in place of the placeholder, and returns the result.
+//
+// The caller (an HTTP handler) is responsible for turning errors into
+// appropriate HTTP status codes; this package stays transport-agnostic.
+func (r *SharePageRenderer) Render(data any) (RenderedPage, error) {
+	if r == nil || r.fs == nil {
+		return RenderedPage{}, fmt.Errorf("share renderer not configured")
+	}
+
+	f, err := r.fs.Open(r.template)
+	if err != nil {
+		return RenderedPage{}, fmt.Errorf("open share template: %w", err)
+	}
+	defer f.Close()
+
+	stat, err := f.Stat()
+	if err != nil {
+		return RenderedPage{}, fmt.Errorf("stat share template: %w", err)
+	}
+
+	var buf strings.Builder
+	if _, err := io.Copy(&buf, f); err != nil {
+		return RenderedPage{}, fmt.Errorf("read share template: %w", err)
+	}
+
+	html, err := injectShareMedia(buf.String(), r.placeholder, data)
+	if err != nil {
+		return RenderedPage{}, err
+	}
+
+	return RenderedPage{
+		HTML:    html,
+		ModTime: stat.ModTime(),
+		Name:    r.template,
+	}, nil
+}
+
+// injectShareMedia replaces placeholder with the JSON-encoded form of
+// data, returning the new HTML. It is private to keep this package's
+// surface small: callers are expected to go through SharePageRenderer.
+func injectShareMedia(html, placeholder string, data any) (string, error) {
+	encoded, err := json.Marshal(data)
+	if err != nil {
+		return "", fmt.Errorf("marshal share metadata: %w", err)
+	}
+	return strings.Replace(html, placeholder, string(encoded), 1), nil
+}
diff --git a/player-server/internal/web/sharepage_test.go b/player-server/internal/web/sharepage_test.go
new file mode 100644
index 0000000..7a89131
--- /dev/null
+++ b/player-server/internal/web/sharepage_test.go
@@ -0,0 +1,92 @@
+package web
+
+import (
+	"errors"
+	"io"
+	"net/http"
+	"os"
+	"strings"
+	"testing"
+	"testing/fstest"
+)
+
+func TestSharePageRenderer_Render(t *testing.T) {
+	t.Run("injects marshaled metadata", func(t *testing.T) {
+		fs := fstest.MapFS{
+			"share.html": {Data: []byte(`<script><!--SHARE_MEDIA--></script>`)},
+		}
+		r := NewSharePageRenderer(http.FS(fs))
+		page, err := r.Render(map[string]string{"stream_url": "/s/abc/stream"})
+		if err != nil {
+			t.Fatalf("unexpected error: %v", err)
+		}
+		if !strings.Contains(page.HTML, `"stream_url":"/s/abc/stream"`) {
+			t.Fatalf("expected injected JSON, got %q", page.HTML)
+		}
+		if page.Name != "share.html" {
+			t.Fatalf("expected name share.html, got %q", page.Name)
+		}
+	})
+
+	t.Run("returns marshal error", func(t *testing.T) {
+		fs := fstest.MapFS{
+			"share.html": {Data: []byte(`<script><!--SHARE_MEDIA--></script>`)},
+		}
+		r := NewSharePageRenderer(http.FS(fs))
+		_, err := r.Render(map[string]any{"bad": make(chan int)})
+		if err == nil {
+			t.Fatal("expected marshal error")
+		}
+		if !strings.Contains(err.Error(), "marshal share metadata") {
+			t.Fatalf("expected wrapped marshal error, got %v", err)
+		}
+	})
+
+	t.Run("nil renderer", func(t *testing.T) {
+		var r *SharePageRenderer
+		if _, err := r.Render(nil); err == nil {
+			t.Fatal("expected error from nil renderer")
+		}
+	})
+
+	t.Run("missing template", func(t *testing.T) {
+		r := NewSharePageRenderer(http.FS(fstest.MapFS{}))
+		_, err := r.Render(nil)
+		if err == nil {
+			t.Fatal("expected open error")
+		}
+	})
+
+	t.Run("stat error propagates", func(t *testing.T) {
+		r := NewSharePageRenderer(statErrorFS{})
+		_, err := r.Render(nil)
+		if err == nil {
+			t.Fatal("expected stat error")
+		}
+	})
+}
+
+func TestInjectShareMedia(t *testing.T) {
+	html, err := injectShareMedia(`a <!--X--> b`, "<!--X-->", "v")
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if html != `a "v" b` {
+		t.Fatalf("unexpected result: %q", html)
+	}
+}
+
+// statErrorFS returns a file whose Stat() fails — used to cover the
+// rare error path in Render where the template is openable but cannot
+// be stat'd.
+type statErrorFS struct{}
+
+func (statErrorFS) Open(string) (http.File, error) { return statErrorFile{}, nil }
+
+type statErrorFile struct{}
+
+func (statErrorFile) Close() error                       { return nil }
+func (statErrorFile) Read([]byte) (int, error)           { return 0, io.EOF }
+func (statErrorFile) Seek(int64, int) (int64, error)     { return 0, nil }
+func (statErrorFile) Readdir(int) ([]os.FileInfo, error) { return nil, nil }
+func (statErrorFile) Stat() (os.FileInfo, error)         { return nil, errors.New("stat failed") }