Qobuz external provider#257
Open
rubiojr wants to merge 21 commits into
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Qobuz as a configurable provider: config model, CLI/setup wizard and reset command, bundle scraping and credential persistence, OAuth session capture, internal Qobuz API client, provider implementation with stream resolution and registry, UI keybinding/wiring, tests, docs, and site updates. ChangesQobuz Streaming Provider Integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Open
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/configuration.md`:
- Line 3: The docs mention Qobuz in the setup wizard but the enumerated
"provider" valid-values list is missing "qobuz", so update the documentation to
include it; add the string "qobuz" to the provider valid-values list in the
configuration docs and also add the same entry to the mirrored provider list in
the site's generated index HTML (the site/index.html mirror of docs) so both
user-facing sources stay in sync.
In `@docs/keybindings.md`:
- Line 68: Add the new Q keybinding ("Q" -> "Open Qobuz provider") to every
quick-switch row and provider keybinding table that omits it in
docs/keybindings.md so the document is consistent (search for "quick-switch"
rows and the provider lists and insert the `Q` entry where missing), and mirror
the same addition in site/index.html so both docs/ and site/ are updated in sync
as required by the guidelines.
In `@external/qobuz/bundle.go`:
- Around line 61-92: The code in the function that fetches the Qobuz login page
and bundle (using resp/resp2, reBundleURL, bundleBaseURL and returning *bundle)
does not check HTTP status codes and may try to parse error pages; add guards
after each client.Do call to verify resp.StatusCode and resp2.StatusCode are in
the 2xx range and return a descriptive error (including the numeric status and
Status string; optionally include a small excerpt of the response body) when the
status is non-2xx so callers get actionable errors instead of misleading regex
failures.
- Around line 57-89: Several bare errors in bundle.go should be wrapped with
context: replace raw returns of err from http.NewRequestWithContext (for req and
req2), io.ReadAll(resp.Body) reading the login page (variable page) and
io.ReadAll(resp2.Body) reading bundle.js (variable body) with fmt.Errorf calls
that add context (e.g., "create login request: %w", "read login page: %w",
"create bundle request: %w", "read bundle.js: %w"). Update the error returns
around the symbols NewRequestWithContext, io.ReadAll (page) and io.ReadAll
(body) so they follow the existing pattern used for client.Do errors.
In `@external/qobuz/client_test.go`:
- Around line 26-39: The test TestTrackFileURLSignature is tautological because
it builds the same raw string twice; change it to exercise the real signing
logic by calling the production signer (e.g. invoke client.trackFileURL or the
function that computes rawSig in client.go) with the same inputs and assert the
returned signature equals a fixed expected hex string (precomputed), or
otherwise assert against a hard-coded expected hash instead of recomputing via
md5hex; ensure the test references the production signing path (rawSig or
trackFileURL) so regressions in the raw format are caught.
In `@external/qobuz/client.go`:
- Around line 62-88: The doRequest function currently returns raw errors from
http.NewRequestWithContext, c.http.Do, and io.ReadAll (around the request build,
transport call, and body read); update those returns to wrap the underlying
error with fmt.Errorf using a "qobuz:" context (e.g., fmt.Errorf("qobuz: create
request: %w", err), fmt.Errorf("qobuz: http do: %w", err), fmt.Errorf("qobuz:
read response body: %w", err)) so error handling is consistent across the
package; ensure you import fmt if not present and keep existing behavior
otherwise.
- Around line 68-72: doRequest currently sets Content-Type to text/plain when
body != "" which is incorrect for form-encoded POSTs; change the logic so that
when callers send URL-encoded form bodies (e.g., the POSTs from user/login that
send "extra=partner" and the oauth/callback that send params.Encode()), the
request uses Content-Type: application/x-www-form-urlencoded;charset=UTF-8
instead of text/plain; either update doRequest to accept a contentType parameter
or detect url-encoded payloads and set the header accordingly, and update the
callers (the user/login and oauth/callback code paths) to pass or rely on the
new content type behavior.
In `@external/qobuz/creds.go`:
- Around line 50-79: Update loadCreds and saveCreds to wrap all propagated
errors with fmt.Errorf so callers get contextualized errors: in loadCreds wrap
errors returned from CredsPath(), os.ReadFile(path), and json.Unmarshal(data,
&creds) (e.g. fmt.Errorf("loadCreds: CredsPath failed: %w", err) or similar with
appropriate context), and in saveCreds wrap errors from CredsPath(),
os.MkdirAll(filepath.Dir(path), ...), json.Marshal(creds), and
os.WriteFile(path, data, ...); also add the fmt import. Ensure each fmt.Errorf
uses "%w" to preserve the original error for unwrapping.
In `@external/qobuz/provider_test.go`:
- Around line 24-38: TestTrackArtist uses sequential assertions instead of a
table-driven approach; refactor it to iterate over a slice of test cases that
each provide a name, input apiTrack and *apiAlbum, and expected string, then run
subtests (t.Run) calling trackArtist for each case and assert equality. Use the
existing types apiTrack, apiAlbum, apiArtist and the trackArtist function names
to build cases for "performer present", "album fallback", and "no artist", and
ensure error messages include the case name for clarity.
In `@external/qobuz/provider.go`:
- Around line 84-87: At the site in provider.go where newClientSilent(ctx) is
called and you currently return playlist.ErrNeedsAuth on any error, log the
underlying err with a descriptive message (e.g., "silent auth failed") before
mapping it to playlist.ErrNeedsAuth so the real cause (rejected token,
network/API error, corrupt creds) is captured; use the package's existing logger
(or log.Printf if no logger is available) to include the err value and any
relevant context prior to returning playlist.ErrNeedsAuth.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 21979b24-2c8e-4020-aec5-fef2144b004c
📒 Files selected for processing (27)
cmd/setup.gocmd/setup_test.gocommands.goconfig.toml.exampleconfig/config.goconfig/config_test.godocs/cli.mddocs/configuration.mddocs/keybindings.mddocs/qobuz.mdexternal/qobuz/bundle.goexternal/qobuz/bundle_test.goexternal/qobuz/client.goexternal/qobuz/client_test.goexternal/qobuz/creds.goexternal/qobuz/doc.goexternal/qobuz/provider.goexternal/qobuz/provider_test.goexternal/qobuz/session.goexternal/qobuz/stream.goexternal/qobuz/stream_test.goexternal/qobuz/types.gomain.goprovider/types.gosite/index.htmlui/model/keymap.goui/model/keys.go
Comment on lines
+24
to
+38
| func TestTrackArtist(t *testing.T) { | ||
| withPerformer := apiTrack{Performer: apiArtist{Name: "Performer"}} | ||
| if got := trackArtist(withPerformer, nil); got != "Performer" { | ||
| t.Errorf("performer name: got %q want %q", got, "Performer") | ||
| } | ||
|
|
||
| album := &apiAlbum{Artist: apiArtist{Name: "AlbumArtist"}} | ||
| if got := trackArtist(apiTrack{}, album); got != "AlbumArtist" { | ||
| t.Errorf("album fallback: got %q want %q", got, "AlbumArtist") | ||
| } | ||
|
|
||
| if got := trackArtist(apiTrack{}, nil); got != "" { | ||
| t.Errorf("no artist: got %q want empty", got) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Convert TestTrackArtist to a table-driven test.
This test uses three sequential ad-hoc assertions rather than a case table.
💚 Proposed table-driven form
-func TestTrackArtist(t *testing.T) {
- withPerformer := apiTrack{Performer: apiArtist{Name: "Performer"}}
- if got := trackArtist(withPerformer, nil); got != "Performer" {
- t.Errorf("performer name: got %q want %q", got, "Performer")
- }
-
- album := &apiAlbum{Artist: apiArtist{Name: "AlbumArtist"}}
- if got := trackArtist(apiTrack{}, album); got != "AlbumArtist" {
- t.Errorf("album fallback: got %q want %q", got, "AlbumArtist")
- }
-
- if got := trackArtist(apiTrack{}, nil); got != "" {
- t.Errorf("no artist: got %q want empty", got)
- }
-}
+func TestTrackArtist(t *testing.T) {
+ album := &apiAlbum{Artist: apiArtist{Name: "AlbumArtist"}}
+ tests := []struct {
+ name string
+ track apiTrack
+ album *apiAlbum
+ want string
+ }{
+ {"performer name", apiTrack{Performer: apiArtist{Name: "Performer"}}, nil, "Performer"},
+ {"album fallback", apiTrack{}, album, "AlbumArtist"},
+ {"no artist", apiTrack{}, nil, ""},
+ }
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ if got := trackArtist(tt.track, tt.album); got != tt.want {
+ t.Errorf("trackArtist() = %q, want %q", got, tt.want)
+ }
+ })
+ }
+}As per coding guidelines: "Tests must use table-driven test patterns."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func TestTrackArtist(t *testing.T) { | |
| withPerformer := apiTrack{Performer: apiArtist{Name: "Performer"}} | |
| if got := trackArtist(withPerformer, nil); got != "Performer" { | |
| t.Errorf("performer name: got %q want %q", got, "Performer") | |
| } | |
| album := &apiAlbum{Artist: apiArtist{Name: "AlbumArtist"}} | |
| if got := trackArtist(apiTrack{}, album); got != "AlbumArtist" { | |
| t.Errorf("album fallback: got %q want %q", got, "AlbumArtist") | |
| } | |
| if got := trackArtist(apiTrack{}, nil); got != "" { | |
| t.Errorf("no artist: got %q want empty", got) | |
| } | |
| } | |
| func TestTrackArtist(t *testing.T) { | |
| album := &apiAlbum{Artist: apiArtist{Name: "AlbumArtist"}} | |
| tests := []struct { | |
| name string | |
| track apiTrack | |
| album *apiAlbum | |
| want string | |
| }{ | |
| {"performer name", apiTrack{Performer: apiArtist{Name: "Performer"}}, nil, "Performer"}, | |
| {"album fallback", apiTrack{}, album, "AlbumArtist"}, | |
| {"no artist", apiTrack{}, nil, ""}, | |
| } | |
| for _, tt := range tests { | |
| t.Run(tt.name, func(t *testing.T) { | |
| if got := trackArtist(tt.track, tt.album); got != tt.want { | |
| t.Errorf("trackArtist() = %q, want %q", got, tt.want) | |
| } | |
| }) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@external/qobuz/provider_test.go` around lines 24 - 38, TestTrackArtist uses
sequential assertions instead of a table-driven approach; refactor it to iterate
over a slice of test cases that each provide a name, input apiTrack and
*apiAlbum, and expected string, then run subtests (t.Run) calling trackArtist
for each case and assert equality. Use the existing types apiTrack, apiAlbum,
apiArtist and the trackArtist function names to build cases for "performer
present", "album fallback", and "no artist", and ensure error messages include
the case name for clarity.
Author
There was a problem hiding this comment.
This is a style preference. Tests are fine as is IMO. @bjarneo what's your take/preference?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
external/qobuz/provider.go (1)
3-13: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winWrap upstream errors with operation context in the new Qobuz paths.
The newly added error returns are bare
err, which drops call-site context and breaks the project’s Go error-handling rule.
As per coding guidelines, "Error handling must wrap errors withfmt.Errorf("context: %w", err)."🧱 Proposed fix
import ( "context" + "fmt" "slices" "strconv" "sync" "time" @@ pls, err := c.userPlaylists(ctx) if err != nil { - return nil, err + return nil, fmt.Errorf("qobuz: list user playlists: %w", err) } @@ tracks, err := c.playlistTracks(ctx, pl.ID.String()) if err != nil { - return nil, err + return nil, fmt.Errorf("qobuz: list tracks for playlist %s: %w", pl.ID.String(), err) }Also applies to: 255-264
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@external/qobuz/provider.go` around lines 3 - 13, The new Qobuz path implementations return bare err values; update each place in the Qobuz provider where you currently return err (e.g., any return patterns like "return ..., err") to wrap the upstream error with fmt.Errorf("operation description: %w", err), add "fmt" to the imports, and use a concise, descriptive operation context (for example "fetching track", "building playlist", or "calling qobuz API") so the returned error preserves call-site context across the provider code.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@external/qobuz/provider_test.go`:
- Around line 40-78: Combine the two tests into a single table-driven test that
uses subtests: create a slice of cases (name, input []apiTrack, want expected
slice) and iterate cases calling t.Run(case.name, func(t *testing.T){ ... });
inside each subtest call dedupeTracksByID(case.input) and assert length and
per-item fields as the existing checks do (use got[i].ID.String() and
got[i].Title). Replace TestDedupeTracksByID and TestDedupeTracksByIDEmpty with
one TestDedupeTracksByID that enumerates both the non-empty scenario and the
nil/empty scenario as cases and performs the same assertions in each subtest.
In `@external/qobuz/provider.go`:
- Around line 260-270: The loop in allPlaylistTracks currently fetches all
playlistTracks then dedupes and truncates, causing extra API calls; change the
loop over pls to maintain a seen map (map[string]bool keyed by track ID) and,
after calling c.playlistTracks(ctx, pl.ID.String()), iterate each returned track
and only append if not seen, marking it seen immediately; after each append
check if len(all) >= allTracksLimit and if so break out of the outer loop to
short-circuit further playlist fetches; you can then remove or skip the final
dedupeTracksByID call (or keep it as a safety no-op) and still trim all to
allTracksLimit if needed.
---
Outside diff comments:
In `@external/qobuz/provider.go`:
- Around line 3-13: The new Qobuz path implementations return bare err values;
update each place in the Qobuz provider where you currently return err (e.g.,
any return patterns like "return ..., err") to wrap the upstream error with
fmt.Errorf("operation description: %w", err), add "fmt" to the imports, and use
a concise, descriptive operation context (for example "fetching track",
"building playlist", or "calling qobuz API") so the returned error preserves
call-site context across the provider code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6b33ad95-df0c-433c-946d-a47d117390ce
📒 Files selected for processing (3)
docs/qobuz.mdexternal/qobuz/provider.goexternal/qobuz/provider_test.go
Comment on lines
+40
to
+78
| func TestDedupeTracksByID(t *testing.T) { | ||
| tracks := []apiTrack{ | ||
| {ID: "1", Title: "first"}, | ||
| {ID: "2", Title: "second"}, | ||
| {ID: "1", Title: "dup of first"}, | ||
| {ID: "3", Title: "third"}, | ||
| {ID: "2", Title: "dup of second"}, | ||
| {ID: "", Title: "no id a"}, | ||
| {ID: "", Title: "no id b"}, | ||
| } | ||
|
|
||
| got := dedupeTracksByID(tracks) | ||
|
|
||
| want := []struct { | ||
| id string | ||
| title string | ||
| }{ | ||
| {"1", "first"}, // first occurrence wins | ||
| {"2", "second"}, | ||
| {"3", "third"}, | ||
| {"", "no id a"}, // empty-ID tracks are always kept | ||
| {"", "no id b"}, | ||
| } | ||
| if len(got) != len(want) { | ||
| t.Fatalf("got %d tracks, want %d", len(got), len(want)) | ||
| } | ||
| for i, w := range want { | ||
| if got[i].ID.String() != w.id || got[i].Title != w.title { | ||
| t.Errorf("track %d = {%q, %q}, want {%q, %q}", | ||
| i, got[i].ID.String(), got[i].Title, w.id, w.title) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| func TestDedupeTracksByIDEmpty(t *testing.T) { | ||
| if got := dedupeTracksByID(nil); len(got) != 0 { | ||
| t.Errorf("dedupeTracksByID(nil) = %v, want empty", got) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Convert the new dedupe tests to a table-driven pattern with subtests.
TestDedupeTracksByID and TestDedupeTracksByIDEmpty are currently ad-hoc; combine both scenarios into a single case table with t.Run(...).
As per coding guidelines, "Tests must use table-driven test patterns. Colocate tests with sources using *_test.go files."
🧪 Proposed fix
func TestDedupeTracksByID(t *testing.T) {
- tracks := []apiTrack{
- {ID: "1", Title: "first"},
- {ID: "2", Title: "second"},
- {ID: "1", Title: "dup of first"},
- {ID: "3", Title: "third"},
- {ID: "2", Title: "dup of second"},
- {ID: "", Title: "no id a"},
- {ID: "", Title: "no id b"},
- }
-
- got := dedupeTracksByID(tracks)
-
- want := []struct {
- id string
- title string
- }{
- {"1", "first"}, // first occurrence wins
- {"2", "second"},
- {"3", "third"},
- {"", "no id a"}, // empty-ID tracks are always kept
- {"", "no id b"},
- }
- if len(got) != len(want) {
- t.Fatalf("got %d tracks, want %d", len(got), len(want))
- }
- for i, w := range want {
- if got[i].ID.String() != w.id || got[i].Title != w.title {
- t.Errorf("track %d = {%q, %q}, want {%q, %q}",
- i, got[i].ID.String(), got[i].Title, w.id, w.title)
- }
- }
-}
-
-func TestDedupeTracksByIDEmpty(t *testing.T) {
- if got := dedupeTracksByID(nil); len(got) != 0 {
- t.Errorf("dedupeTracksByID(nil) = %v, want empty", got)
- }
+ tests := []struct {
+ name string
+ in []apiTrack
+ want []struct {
+ id string
+ title string
+ }
+ }{
+ {
+ name: "dedupes repeated IDs and keeps empty IDs",
+ in: []apiTrack{
+ {ID: "1", Title: "first"},
+ {ID: "2", Title: "second"},
+ {ID: "1", Title: "dup of first"},
+ {ID: "3", Title: "third"},
+ {ID: "2", Title: "dup of second"},
+ {ID: "", Title: "no id a"},
+ {ID: "", Title: "no id b"},
+ },
+ want: []struct {
+ id string
+ title string
+ }{
+ {"1", "first"},
+ {"2", "second"},
+ {"3", "third"},
+ {"", "no id a"},
+ {"", "no id b"},
+ },
+ },
+ {
+ name: "nil input yields empty result",
+ in: nil,
+ want: nil,
+ },
+ }
+
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ got := dedupeTracksByID(tt.in)
+ if len(got) != len(tt.want) {
+ t.Fatalf("got %d tracks, want %d", len(got), len(tt.want))
+ }
+ for i, w := range tt.want {
+ if got[i].ID.String() != w.id || got[i].Title != w.title {
+ t.Errorf("track %d = {%q, %q}, want {%q, %q}",
+ i, got[i].ID.String(), got[i].Title, w.id, w.title)
+ }
+ }
+ })
+ }
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@external/qobuz/provider_test.go` around lines 40 - 78, Combine the two tests
into a single table-driven test that uses subtests: create a slice of cases
(name, input []apiTrack, want expected slice) and iterate cases calling
t.Run(case.name, func(t *testing.T){ ... }); inside each subtest call
dedupeTracksByID(case.input) and assert length and per-item fields as the
existing checks do (use got[i].ID.String() and got[i].Title). Replace
TestDedupeTracksByID and TestDedupeTracksByIDEmpty with one TestDedupeTracksByID
that enumerates both the non-empty scenario and the nil/empty scenario as cases
and performs the same assertions in each subtest.
Source: Coding guidelines
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@external/qobuz/provider_test.go`:
- Around line 85-147: TestSampleTracks currently contains multiple scenarios in
one body; convert it into a table-driven test using t.Run. Create a slice of
case structs (name, input slice creator or count, cap, expectedLenCheck function
or expectedLen value, and any per-case invariants) and include two cases at
minimum ("under cap" using mk(100), cap=500 and asserting length==100, same
elements, and shuffled order; "over cap" using mk(1000), cap=10 and asserting
length==10, all from input, and no duplicates). Inside the loop call
t.Run(tc.name, func(t *testing.T){ ... }) and reuse the existing helpers mk,
idSet, rand.New(...).Shuffle and sampleTracks; move the original assertions into
the subtest body so each case runs independently and failures are scoped to the
named case. Ensure any common setup (r := rand.New(...)) stays outside the cases
if desired or is reinitialized per-case for deterministic behavior.
In `@external/qobuz/provider.go`:
- Around line 259-267: The calls to c.userPlaylists(ctx) and
c.playlistTracks(ctx, pl.ID.String()) currently return raw errors; update the
error returns in the surrounding function (the method invoking these calls,
e.g., randomTracks) to wrap those errors using fmt.Errorf with context (for
example: return nil, fmt.Errorf("fetching user playlists: %w", err) and return
nil, fmt.Errorf("fetching tracks for playlist %s: %w", pl.ID.String(), err));
ensure fmt is imported if not already.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: cda3d025-9948-49c1-8717-8387f3ccd26a
📒 Files selected for processing (3)
docs/qobuz.mdexternal/qobuz/provider.goexternal/qobuz/provider_test.go
Comment on lines
+85
to
+147
| func TestSampleTracks(t *testing.T) { | ||
| mk := func(n int) []apiTrack { | ||
| ts := make([]apiTrack, n) | ||
| for i := range ts { | ||
| ts[i] = apiTrack{ID: json.Number(strconv.Itoa(i))} | ||
| } | ||
| return ts | ||
| } | ||
| idSet := func(ts []apiTrack) map[string]bool { | ||
| m := make(map[string]bool, len(ts)) | ||
| for _, tr := range ts { | ||
| m[tr.ID.String()] = true | ||
| } | ||
| return m | ||
| } | ||
|
|
||
| r := rand.New(rand.NewPCG(42, 1024)) | ||
|
|
||
| // Under the cap: every track is kept, but the list must still be shuffled. | ||
| // This is the case that used to be returned in playlist order unchanged. | ||
| in := mk(100) | ||
| got := sampleTracks(in, 500, r.Shuffle) | ||
| if len(got) != 100 { | ||
| t.Fatalf("under cap: len = %d, want 100", len(got)) | ||
| } | ||
| want := idSet(in) | ||
| for _, tr := range got { | ||
| if !want[tr.ID.String()] { | ||
| t.Errorf("under cap: track %s not from input", tr.ID) | ||
| } | ||
| } | ||
| sameOrder := true | ||
| for i := range got { | ||
| if got[i].ID != in[i].ID { | ||
| sameOrder = false | ||
| break | ||
| } | ||
| } | ||
| if sameOrder { | ||
| t.Error("under cap: list was not shuffled") | ||
| } | ||
|
|
||
| // Over the cap: exactly n tracks, all from the input, no duplicates. | ||
| big := mk(1000) | ||
| all := idSet(big) | ||
| for trial := 0; trial < 20; trial++ { | ||
| s := sampleTracks(big, 10, r.Shuffle) | ||
| if len(s) != 10 { | ||
| t.Fatalf("over cap: len = %d, want 10", len(s)) | ||
| } | ||
| seen := make(map[string]bool, len(s)) | ||
| for _, tr := range s { | ||
| id := tr.ID.String() | ||
| if !all[id] { | ||
| t.Fatalf("over cap: track %q not from input", id) | ||
| } | ||
| if seen[id] { | ||
| t.Fatalf("over cap: duplicate track %q", id) | ||
| } | ||
| seen[id] = true | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Convert TestSampleTracks to a table-driven test.
This function embeds multiple scenarios in one body; restructure as named cases (t.Run) to match the repository’s required test style and keep extensions easier.
As per coding guidelines, "Tests must use table-driven test patterns. Colocate tests with sources using *_test.go files."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@external/qobuz/provider_test.go` around lines 85 - 147, TestSampleTracks
currently contains multiple scenarios in one body; convert it into a
table-driven test using t.Run. Create a slice of case structs (name, input slice
creator or count, cap, expectedLenCheck function or expectedLen value, and any
per-case invariants) and include two cases at minimum ("under cap" using
mk(100), cap=500 and asserting length==100, same elements, and shuffled order;
"over cap" using mk(1000), cap=10 and asserting length==10, all from input, and
no duplicates). Inside the loop call t.Run(tc.name, func(t *testing.T){ ... })
and reuse the existing helpers mk, idSet, rand.New(...).Shuffle and
sampleTracks; move the original assertions into the subtest body so each case
runs independently and failures are scoped to the named case. Ensure any common
setup (r := rand.New(...)) stays outside the cases if desired or is
reinitialized per-case for deterministic behavior.
Source: Coding guidelines
Author
|
Except for the test code style (happy to tweak if desired), this sesms to be working fine and ready for 👀 |
Reference implementations: * https://github.com/vitiko98/qobuz-dl * https://github.com/vicrodh/qbz * https://github.com/SofusA/qobine
The old TestTrackFileURLSignature was tautological: it built the raw sig string by hand, md5'd it, then asserted md5hex of the same string matched. It never touched the production code, so a broken sig format would've sailed right through. Extract trackFileURLSig() and pin it against a precomputed hash so a change to the raw layout breaks streaming AND the test, instead of just streaming.
commands.go has accepted "qobuz" since the provider landed, but the docs valid-values list never got the memo. Add it.
fetchBundle never checked status codes, so a 5xx or a Cloudflare block from play.qobuz.com would sail through and fail later with a confusing "bundle URL not found in login page" — pointing at the regex instead of the real problem. Check the status after each request and surface it directly. Also wrap the remaining bare errors so they match the Do() calls that were already wrapped.
A Library entry next to Favorite Tracks that plays a random sample of up to 500 tracks from across all your playlists, deduped by track ID so songs in multiple playlists only show up once. Ctrl+R reshuffles. It's a synthetic playlist (playlists/random) like Favorite Tracks, so it rides the same Tracks() resolve + cache path. Capped at 500 because each track costs a track/getFileUrl call to resolve a short-lived stream URL — resolving an unbounded library would be slow, wasteful, and timeout-prone. Dedupe and shuffle are pulled into helpers so they're actually testable.
The request-build, transport, and body-read errors in doRequest were returned bare while the status and decode paths already wrapped. Wrap them too, with the endpoint, so a failure points at which call actually blew up. Per CodeRabbit.
The gather looped playlists one at a time, so a big library paid the sum of every playlist/get round-trip in series. Fan them out with a bounded semaphore (8, same as the stream-resolve pool) and merge in playlist order so dedupe stays deterministic. This deliberately still fetches every playlist: CodeRabbit's suggestion to short-circuit at 500 would break the random sampling — you'd only ever draw from the first few playlists. This cuts the latency without the bias.
The list-playlists and per-playlist fetch errors were returned bare. Wrap them per the error-handling guideline. The client layer already prefixes "qobuz: <endpoint>", so the per-playlist wrap carries the one thing it can't — which playlist ID failed — instead of a redundant restatement.
The main provider table listed Q for Qobuz but the two quick-switch rows (browse + playlist panes) still showed S/N/P/J/E/Y/C/M/L/R without it. The site already had Q in both, so the docs were the ones out of sync. Slot it in.
Qobuz implements search and album/artist browse, but the Ctrl+F search list, the provider-browser drill-in list, and the "other Ctrl+F providers" note all predated it. Add it to each. The site's Ctrl+F list already had Qobuz, so this closes the last docs/site gap for it.
ensureClient collapsed every newClientSilent error into ErrNeedsAuth, so a transient network error or corrupt creds looked identical to "not signed in" and vanished. Log it via applog.Debug first — that writes to the file log, not the TUI footer/stderr, so it's safe here (and buildTrack already uses it).
The credential load/save helpers returned bare errors, so a path-resolution, read, parse, mkdir, encode, or write failure surfaced without saying which step broke — "invalid character" for a corrupt creds file was especially opaque. Wrap each with qobuz: context. %w keeps the chain intact for the callers that already wrap (newClientSilent, the save path).
doRequest tagged its POST bodies text/plain, but user/login (extra=partner) and oauth/callback (params.Encode()) are both application/x-www-form-urlencoded. Label them correctly — it matches what the Qobuz web player sends, so it's less fingerprintable, and it won't break if Qobuz ever parses bodies strictly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Still a work in progress. Functional, but code may need further tweaks.Adds a Qobuz external provider, closing #140
Similar to other external providers, following their code style/paths.
The auth flow is the flow other open source clients out there use, which essentially involves scrapping a page via an Oauth-like flow. The provider will open a in the browser so you can log in, then cache the creds. It's subject to break if upstream changes things and not very elegant, but that's what Qobuz offers us for now. They don't have a proper Oauth flow for apps or an auth API, to my knowledge.
To easy the review, related changes have been grouped per commit.
Reference implementations checked:
Screenshots / video
How to test
cliamp setupChecklist
make checkpassesdocs/andsite/index.htmlupdated for user-facing changesSummary by CodeRabbit
New Features
Documentation
Tests