Skip to content

fix: replace fmt.Sprintf URL building with url.JoinPath, log home section failures#46

Merged
koinsaari merged 1 commit into
mainfrom
fix/url-building-and-home-logging
Jun 28, 2026
Merged

fix: replace fmt.Sprintf URL building with url.JoinPath, log home section failures#46
koinsaari merged 1 commit into
mainfrom
fix/url-building-and-home-logging

Conversation

@koinsaari

@koinsaari koinsaari commented Jun 28, 2026

Copy link
Copy Markdown
Contributor

URL construction in the Jellyfin client and media mapper used fmt.Sprintf and string concatenation, which doesn't encode special characters in path segments and breaks if the base URL has a trailing slash. Home section failures were silently swallowed with no log signal. A misconfigured base URL would also produce empty StreamURL fields with no indication of why.

Summary

  • Replace fmt.Sprintf/string concatenation with url.JoinPath in all Jellyfin client methods (GetItem, GetItems, AuthenticateByName, QuickConnectInitiate, QuickConnectAuthenticate) and media mapper (image URLs, stream URLs)
  • Validate JELLYFIN_URL and PROXY_BASE_URL as absolute URLs at startup — misconfigured base URL now fails fast instead of producing empty URL fields silently
  • Add *slog.Logger to media.Service and emit slog.Warn per failed home section instead of silently discarding errors

Implementation notes

QuickConnectAuthenticate appends a query param (secret), so it goes through url.JoinPathurl.Parseq.Setq.Encode rather than a bare url.JoinPath call.

Mapper functions are pure (no error return), so URL joins use a local joinURL helper that returns "" on error. Startup validation in config.go is the real guard — a bad base URL exits before the server starts.

🤖 Generated with Claude Code

@koinsaari koinsaari added the bug Something isn't working label Jun 28, 2026
@koinsaari koinsaari self-assigned this Jun 28, 2026
@github-actions

github-actions Bot commented Jun 28, 2026

Copy link
Copy Markdown

Claude finished @koinsaari's task in 1m 30s —— View job


Code Review: PR #46

  • Read REVIEW.md and CLAUDE.md for review instructions
  • Read PR diff and all changed files
  • Check security invariants (auth import cycle, SQL, JWT)
  • Check code quality rules (URL building, discarded returns, url.Parse errors)
  • Verify no remaining fmt.Sprintf URL patterns in jellyfin client
  • Post review findings

No blocking issues. The PR correctly replaces all fmt.Sprintf/concatenation URL building in the Jellyfin client with url.JoinPath, and properly logs home section failures instead of silently discarding them.

What's good:

  • All url.JoinPath error returns are checked — no silently ignored errors.
  • QuickConnectAuthenticate correctly uses url.JoinPathurl.Parseq.Setq.Encode for the query parameter, and the url.Parse error is checked (per CLAUDE.md: "Never ignore the error from url.Parse with _").
  • The rename from url (which would shadow the newly-imported net/url package) to raw/decoded in items.go is a necessary and clean fix.
  • The slog.Warn in Home() includes both the section ID and the error — useful for debugging without leaking PII.
  • All test call sites updated consistently with slog.Default().
  • No auth import cycle violations, no SQL issues, no JWT keyfunc changes.

🟡 Nit: internal/media/mapper.go:20 and :165 still use fmt.Sprintf for Jellyfin image URLs (%s/Items/%s/Images/Primary). The inputs are server-config baseURL and Jellyfin UUIDs (not user-controlled), so this isn't a bug, but it's the same pattern this PR is fixing elsewhere. Worth a follow-up for consistency. Fix this →


@koinsaari koinsaari force-pushed the fix/url-building-and-home-logging branch from 3a3194e to 62efb1e Compare June 28, 2026 13:27
@github-actions

github-actions Bot commented Jun 28, 2026

Copy link
Copy Markdown

Claude finished @koinsaari's task in 1m 47s —— View job


Code Review: PR #46

  • Read REVIEW.md and CLAUDE.md for review instructions
  • Read PR diff and all changed files
  • Check security invariants (auth import cycle, SQL, JWT)
  • Check code quality rules (URL building, discarded returns, url.Parse errors)
  • Verify no remaining fmt.Sprintf URL patterns in jellyfin client
  • Post review findings

No blocking issues. The PR correctly replaces all fmt.Sprintf/concatenation URL building in the Jellyfin client with url.JoinPath, and properly logs home section failures instead of silently discarding them.

What's good:

  • All url.JoinPath error returns in the client package are checked — no silently ignored errors.
  • QuickConnectAuthenticate correctly uses url.JoinPathurl.Parseq.Setq.Encode for the query parameter, and the url.Parse error is checked (per CLAUDE.md: "Never ignore the error from url.Parse with _").
  • The rename from urlraw/decoded in items.go avoids shadowing the newly-imported net/url package — necessary and clean.
  • The slog.Warn in Home() includes both the section ID and the error — useful for debugging without leaking PII.
  • All test call sites updated consistently with slog.Default().
  • No auth import cycle violations, no SQL issues, no JWT keyfunc changes.

🟡 Nit: joinURL in internal/media/mapper.go:10-16 silently returns "" on error. The inputs are controlled (config base URLs + Jellyfin UUIDs), and a malformed base URL would fail at the client layer first, so this isn't a bug in practice. But given the PR's own goal of making failures visible, a slog.Warn inside the helper would be consistent — otherwise a misconfigured proxyBaseURL produces empty StreamURL fields with no log signal. Fix this →


…e URLs at startup

fmt.Sprintf and string concatenation for URL construction doesn't encode
special characters in path segments. url.JoinPath handles this correctly
and also tolerates trailing slashes on the base URL.

- Replace fmt.Sprintf/concat with url.JoinPath in jellyfin client (GetItem,
  GetItems, AuthenticateByName, QuickConnectInitiate, QuickConnectAuthenticate)
  and media mapper (image URLs, stream URLs)
- Validate JELLYFIN_URL and PROXY_BASE_URL as absolute URLs at startup so a
  misconfigured base URL fails fast rather than producing empty URL fields
- Add logger to media.Service and log slog.Warn per failed home section instead
  of silently dropping it, so degraded home responses are observable

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@koinsaari koinsaari force-pushed the fix/url-building-and-home-logging branch from 62efb1e to 83e37d8 Compare June 28, 2026 13:46
@github-actions

github-actions Bot commented Jun 28, 2026

Copy link
Copy Markdown

Claude finished @koinsaari's task in 1m 50s —— View job


Code Review: PR #46

  • Read REVIEW.md and CLAUDE.md for review instructions
  • Read PR diff and all changed files
  • Check security invariants (auth import cycle, SQL, JWT)
  • Check code quality rules (URL building, discarded returns, url.Parse errors)
  • Verify no remaining fmt.Sprintf URL patterns in jellyfin client
  • Post review findings

No blocking issues. The PR correctly replaces all fmt.Sprintf/concatenation URL building in the Jellyfin client with url.JoinPath, adds startup validation for base URLs, and logs home section failures instead of silently discarding them.

What's good:

  • All url.JoinPath error returns in the client package are checked — no silently ignored errors.
  • QuickConnectAuthenticate correctly uses url.JoinPathurl.Parseq.Setq.Encode for the query parameter, and the url.Parse error is checked (per CLAUDE.md: "Never ignore the error from url.Parse with _").
  • The rename from urlraw/decoded in items.go avoids shadowing the newly-imported net/url package — necessary and clean.
  • The slog.Warn in Home() includes both the section ID and the error — useful for debugging without leaking PII.
  • validateURL in config rejects relative paths, scheme-less hostnames, and opaque URIs like jellyfin:8096 — good test coverage for all four cases.
  • No auth import cycle violations (internal/auth does not import internal/clients/jellyfin). No SQL changes. No JWT keyfunc changes.
  • All test call sites updated consistently with slog.Default().

🟡 Nit: joinURL in internal/media/mapper.go:10-16 silently returns "" on error. In practice url.JoinPath won't fail here since the base URLs are validated at startup and the path segments are Jellyfin UUIDs, so this isn't a bug. But given this PR's stated goal of making failures visible, a slog.Warn inside the helper (or returning an error) would be consistent — otherwise a misconfigured proxyBaseURL that somehow passes validation produces empty StreamURL/Poster fields with no log signal. Fix this →


@koinsaari koinsaari merged commit bb1f403 into main Jun 28, 2026
8 checks passed
@koinsaari koinsaari deleted the fix/url-building-and-home-logging branch June 28, 2026 13:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant