feat: add --customise wizard, YouTube playlist provider, volume persi…#265
Open
HanshalBobate wants to merge 1 commit into
Open
feat: add --customise wizard, YouTube playlist provider, volume persi…#265HanshalBobate wants to merge 1 commit into
HanshalBobate wants to merge 1 commit into
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 an interactive TUI to persist user customisations, a YouTube-playlist provider, CLI flags to run/clear customisations, startup wiring to apply customisations to providers, improved yt-dlp stderr parsing, and small UI persistence/behavior tweaks. ChangesCustomization System and Provider Configuration
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
config/customisations.go (1)
36-95: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftMove customisation load/save ownership to the mandated config files.
This file now owns configuration loading/saving logic, but repository rules reserve those responsibilities for
config/config.go(loading) andconfig/saver.go(saving).As per coding guidelines, "Configuration loading and merging must be handled in
config/config.gofor base loading,config/flags.gofor CLI overrides, andconfig/saver.gofor saving."🤖 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 `@config/customisations.go` around lines 36 - 95, The LoadCustomisations, SaveCustomisations and ClearCustomisations functions in config/customisations.go implement configuration loading/saving but repository rules require loading to be centralized in config/config.go and saving in config/saver.go; move the file's ownership accordingly by removing or delegating these functions here and instead: (1) implement the loading and merging of Customisations inside the existing load flow in the function(s) in config/config.go (call or migrate the logic from LoadCustomisations, keep behavior such as creating ProviderNames map and handling fs.ErrNotExist), (2) implement persistence (the json MarshalIndent + file write and directory creation) in the save path in config/saver.go (migrate logic from SaveCustomisations and ClearCustomisations, preserving permissions and ErrNotExist handling), and (3) leave config/customisations.go with only type definitions or thin helpers if needed, updating all callers to use the new config/config.go and config/saver.go entry points (ensure symbols LoadCustomisations/SaveCustomisations/ClearCustomisations are removed or replaced by the new centralized functions).Source: Coding guidelines
resolve/resolve.go (1)
535-539: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winPreserve underlying yt-dlp error when stderr is cleaned.
When
msg != "", both paths currently return string-only errors and drop the underlyingerr. Keep cleaned stderr text, but still wrap%w.Suggested fix
msg := cleanYTDLError(stderr.String()) if msg != "" { - return nil, fmt.Errorf("yt-dlp: %s", msg) + return nil, fmt.Errorf("yt-dlp: %s: %w", msg, err) } @@ msg := cleanYTDLError(stderr.String()) if msg != "" { - return "", fmt.Errorf("yt-dlp: %s", msg) + return "", fmt.Errorf("yt-dlp: %s: %w", msg, err) }As per coding guidelines, "Error handling must wrap errors with
fmt.Errorf(\"context: %w\", err)."Also applies to: 603-608
🤖 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 `@resolve/resolve.go` around lines 535 - 539, The cleaned stderr path currently returns a string-only error and drops the original error; change the return in the branch that handles msg != "" to wrap the underlying err with fmt.Errorf so the cleaned stderr is included but the original error is preserved (e.g. return nil, fmt.Errorf("yt-dlp: %s: %w", msg, err)); make the same change for the analogous yt-dlp error-handling block later in the file (the block that also uses cleanYTDLError, msg, stderr and err).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 `@cmd/customise.go`:
- Line 29: Remove the user-facing fmt.Println("Customisations saved
successfully!") from cmd/customise.go and instead return a nil error or explicit
success status from the enclosing function so the caller (main/run flow) can
decide how to present success messages; locate the fmt.Println call and replace
it with a return/propagation of success (e.g., return nil) and ensure any
callers handle and print the success message in main or run.
- Around line 27-31: The CLI currently swallows save failures: in
cmd/customise.go update the Customise() return path so when the model assertion
yields a *customiseModel and either cm.saved is false or save() set cm.err you
return that error instead of nil. Specifically, in the Customise() function(s)
that perform "cm, ok := m.(*customiseModel)" and check "if ok && cm.saved {
fmt.Println(...)" change the logic to return cm.err when ok is true but cm.saved
is false (and return the underlying m.err/cm.err as applicable); also handle the
case where the type assertion fails by returning a sensible error. Ensure you
reference the customiseModel.saved, customiseModel.err (or m.err) and the save()
result so failures propagate to the caller rather than always returning nil.
- Around line 16-19: The code in cmd/customise.go silently replaces c after any
error from config.LoadCustomisations(), risking overwriting a corrupt file on
save; change the logic in the LoadCustomisations handling so that only a "file
not found" case initializes c = config.Customisations{ProviderNames:
make(map[string]string)}, while all other errors are returned/propagated (or
cause a visible failure) instead of being masked — update the branch around
config.LoadCustomisations() that assigns c to ensure it checks the error type
(e.g., file-not-exist) and otherwise returns or logs the error rather than
resetting c.
In `@commands.go`:
- Line 66: Replace the direct return of cmd.Customise() with error capture and
contextual wrapping: call err := cmd.Customise(); if err != nil { return
fmt.Errorf("failed to customise command %v: %w", cmd, err) } and otherwise
return nil (or the successful value as appropriate). This wraps the error per
guidelines and provides the failing command context (use cmd's identifier if a
specific accessor like Name() exists).
- Around line 58-67: The runtime should enforce that --clear-all is only valid
together with --customise: add a pre-check in the command handler (before using
c.Bool("customise") to branch) that if c.Bool("clear-all") &&
!c.Bool("customise") then return an error (e.g. fmt.Errorf("--clear-all requires
--customise")). This change touches the same logic that calls
config.ClearCustomisations() and cmd.Customise() so the existing branch that
clears customisations when both flags are set can remain unchanged; the
pre-check simply prevents --clear-all from being ignored when --customise is not
provided.
In `@config/customisations.go`:
- Around line 41-44: The code currently swallows errors from
customisationsPath() by returning (c, nil); change the error handling so the
function propagates the actual error instead of masking it — replace the silent
return with returning the error (e.g., return c, err or otherwise return the
error per this function's signature) so failures from customisationsPath() are
surfaced; update the return at the customisationsPath() call site (the block
using variable c) accordingly.
- Around line 65-94: SaveCustomisations and ClearCustomisations currently return
raw errors from helper calls which loses context; update these functions to wrap
each returned error with fmt.Errorf("...: %w", err) providing operation context
(e.g., in SaveCustomisations wrap errors from customisationsPath with "get
customisations path", from os.MkdirAll with "create customisations directory",
from json.MarshalIndent with "marshal customisations", and from os.WriteFile
with "write customisations file"), and in ClearCustomisations wrap errors from
customisationsPath with "get customisations path" and from os.Remove with
"remove customisations file" (preserve the existing special-case for
fs.ErrNotExist); add fmt to imports if missing.
In `@external/radio/provider.go`:
- Around line 52-61: The Provider constructor currently leaves p.stations empty
on first-run (when appdir.Dir() fails or loadStations returns an error), causing
Playlists() to be empty; modify the Provider initialization (the New/constructor
in external/radio/provider.go that builds the &Provider{}) so that when no
radios.toml is found or appdir.Dir() errors you explicitly seed p.stations with
the built-in default station list (e.g., call or assign the existing
defaultStations/defaultSeed function/variable) while still initializing
p.favorites = &Favorites{byURL: make(map[string]struct{})}; ensure the code path
that handles loadStations(err != nil) also assigns the defaults so first-run
users get the default Radio experience.
In `@external/ytplaylist/provider.go`:
- Around line 29-35: The Tracks method currently ignores the id parameter;
update Provider.Tracks to validate the incoming id (e.g., return an error if id
== "0" or id == "" such as fmt.Errorf("invalid playlist id: %q", id)) and then
pass that validated id into resolve.ResolveYTDLBatch instead of always using
p.url (i.e., call resolve.ResolveYTDLBatch(id, 0, 0)); keep the existing error
wrapping for the ResolveYTDLBatch call and return the resolved tracks on
success.
In `@main.go`:
- Line 66: Replace the ignored error from calling config.LoadCustomisations() by
capturing and handling it: change the call that currently does cust, _ :=
config.LoadCustomisations() to cust, err := config.LoadCustomisations(), then if
err != nil handle it appropriately (e.g., log the error with context and exit
startup or return the error from main) so malformed or unreadable customisations
are not silently ignored; ensure subsequent code still uses the cust variable
when no error occurred.
In `@README.md`:
- Around line 3-4: Update all README links that point to the upstream repo so
they target this fork: replace occurrences of "bjarneo/cliamp" with
"HanshalBobate/cliamp" for the Homebrew formula/tap, raw install script URL,
GitHub releases/downloads links, and git clone URLs; ensure any badge, curl/wget
install line, and example brew tap/install command reflect the fork's owner so
users install this fork's customised build rather than upstream.
In `@ui/model/keys.go`:
- Line 596: Replace the silent ignore of m.configSaver.Save for the "volume" key
with explicit error-handling and a brief rationale comment: call
m.configSaver.Save("volume", fmt.Sprintf("%.2f", m.player.Volume())) and if it
returns an error, log it at debug level (e.g., use m.logger.Debugf or
equivalent) with a short message like "failed to save volume" so failures appear
in logs without spamming the user; also add a one-line comment above the change
explaining that volume saves are frequent so errors are only logged at debug
level to avoid UX noise and to keep behavior consistent with other settings
(repeat/shuffle/visualizer) which report errors to users.
- Around line 594-602: Refactor the volume adjustment cases to avoid calling
m.player.Volume() twice: after calling m.player.SetVolume(...), read the clamped
volume once into a local (e.g., newVol := m.player.Volume()), use newVol when
calling m.configSaver.Save("volume", fmt.Sprintf("%.2f", newVol)), and then call
m.notifyPlayback(); apply this change in both the "+" / "=" and "-" cases so
SetVolume, Volume(), Save and notifyPlayback use the cached post-adjustment
value.
---
Outside diff comments:
In `@config/customisations.go`:
- Around line 36-95: The LoadCustomisations, SaveCustomisations and
ClearCustomisations functions in config/customisations.go implement
configuration loading/saving but repository rules require loading to be
centralized in config/config.go and saving in config/saver.go; move the file's
ownership accordingly by removing or delegating these functions here and
instead: (1) implement the loading and merging of Customisations inside the
existing load flow in the function(s) in config/config.go (call or migrate the
logic from LoadCustomisations, keep behavior such as creating ProviderNames map
and handling fs.ErrNotExist), (2) implement persistence (the json MarshalIndent
+ file write and directory creation) in the save path in config/saver.go
(migrate logic from SaveCustomisations and ClearCustomisations, preserving
permissions and ErrNotExist handling), and (3) leave config/customisations.go
with only type definitions or thin helpers if needed, updating all callers to
use the new config/config.go and config/saver.go entry points (ensure symbols
LoadCustomisations/SaveCustomisations/ClearCustomisations are removed or
replaced by the new centralized functions).
In `@resolve/resolve.go`:
- Around line 535-539: The cleaned stderr path currently returns a string-only
error and drops the original error; change the return in the branch that handles
msg != "" to wrap the underlying err with fmt.Errorf so the cleaned stderr is
included but the original error is preserved (e.g. return nil,
fmt.Errorf("yt-dlp: %s: %w", msg, err)); make the same change for the analogous
yt-dlp error-handling block later in the file (the block that also uses
cleanYTDLError, msg, stderr and err).
🪄 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: ed46c04d-d921-46ec-8d6a-3424333b9cd9
📒 Files selected for processing (11)
README.mdcmd/customise.gocommands.goconfig/customisations.goexternal/radio/provider.goexternal/ytplaylist/provider.gomain.goresolve/resolve.goui/model/keys.goui/model/update.goui/model/view.go
Comment on lines
+3
to
+4
| > **This is a personal fork by [HanshalBobate](https://github.com/HanshalBobate).** It stays in sync with the [upstream repo (bjarneo/cliamp)](https://github.com/bjarneo/cliamp) and adds the customisation features described in the [My Customisations](#my-customisations) section below. | ||
|
|
There was a problem hiding this comment.
Update the install/build links to match the fork.
Now that the README says this is HanshalBobate’s fork, the brew, raw install script, releases, and clone URLs below should stop pointing at bjarneo/cliamp; otherwise readers will install upstream and miss the customisation features documented here.
🤖 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 `@README.md` around lines 3 - 4, Update all README links that point to the
upstream repo so they target this fork: replace occurrences of "bjarneo/cliamp"
with "HanshalBobate/cliamp" for the Homebrew formula/tap, raw install script
URL, GitHub releases/downloads links, and git clone URLs; ensure any badge,
curl/wget install line, and example brew tap/install command reflect the fork's
owner so users install this fork's customised build rather than upstream.
Comment on lines
594
to
602
| case "+", "=": | ||
| m.player.SetVolume(m.player.Volume() + 1) | ||
| _ = m.configSaver.Save("volume", fmt.Sprintf("%.2f", m.player.Volume())) | ||
| m.notifyPlayback() | ||
|
|
||
| case "-": | ||
| m.player.SetVolume(m.player.Volume() - 1) | ||
| _ = m.configSaver.Save("volume", fmt.Sprintf("%.2f", m.player.Volume())) | ||
| m.notifyPlayback() |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider refactoring to reduce redundant Volume() reads.
The current pattern calls m.player.Volume() twice per volume adjustment: once to compute the new value (line 595/600) and again to save it (line 596/601). While this correctly captures any clamping behavior from SetVolume, it's slightly inefficient.
Consider caching the post-adjustment volume:
case "+", "=":
- m.player.SetVolume(m.player.Volume() + 1)
- _ = m.configSaver.Save("volume", fmt.Sprintf("%.2f", m.player.Volume()))
+ newVol := m.player.Volume() + 1
+ m.player.SetVolume(newVol)
+ // Save the clamped volume returned by the player, not our computed value,
+ // to handle volume limits correctly.
+ _ = m.configSaver.Save("volume", fmt.Sprintf("%.2f", m.player.Volume()))
m.notifyPlayback()This reduces redundant computation in the first SetVolume call while still capturing the clamped value from the player.
🤖 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 `@ui/model/keys.go` around lines 594 - 602, Refactor the volume adjustment
cases to avoid calling m.player.Volume() twice: after calling
m.player.SetVolume(...), read the clamped volume once into a local (e.g., newVol
:= m.player.Volume()), use newVol when calling m.configSaver.Save("volume",
fmt.Sprintf("%.2f", newVol)), and then call m.notifyPlayback(); apply this
change in both the "+" / "=" and "-" cases so SetVolume, Volume(), Save and
notifyPlayback use the cached post-adjustment value.
HanshalBobate
force-pushed
the
main
branch
2 times, most recently
from
1b276c3 to
ea2684e
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@config/customisations.go`:
- Around line 29-33: In customisationsPath, wrap the error returned from
appdir.Dir() with context before returning instead of returning the raw err;
change the early return to something like return "", fmt.Errorf("determine app
dir: %w", err) (use fmt.Errorf with "%w") so callers get contextualized errors;
update imports if necessary to include fmt.
In `@external/radio/provider.go`:
- Line 31: Tests in external/radio/provider_test.go still reference the removed
constants builtinName and builtinURL; update the tests to reflect the new
"default built-in radio is disabled" behavior by removing those constant
references and changing assertions that expected the built-in entry to instead
verify it is absent (e.g., assert the radio list does not contain
builtinName/builtinURL strings or assert the default built-in radio count is
zero), or simply remove the lines that reference builtinName and builtinURL;
adjust helper variables in the test file as needed so no undefined symbols
remain.
In `@ui/model/update.go`:
- Around line 726-730: The handlers for playback.SetVolumeMsg and ipc.VolumeMsg
call m.configSaver.Save and ignore its error but lack the explanatory comment
used elsewhere; update the message handlers around the m.player.SetVolume /
m.configSaver.Save calls to add a brief comment (matching the rationale in
keys.go lines 596–598) explaining why the Save error is intentionally suppressed
(non-fatal, best-effort persistence) so future readers understand the decision
and the pattern is consistent across handlers.
🪄 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: a5f5285e-f5e2-41c1-bcb8-7ce85010550b
📒 Files selected for processing (11)
README.mdcmd/customise.gocommands.goconfig/customisations.goexternal/radio/provider.goexternal/ytplaylist/provider.gomain.goresolve/resolve.goui/model/keys.goui/model/update.goui/model/view.go
|
|
||
| const builtinName = "cliamp radio" | ||
| const builtinURL = "https://radio.cliamp.stream/streams.m3u" | ||
| // Default built-in radio is disabled. |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify radio provider tests compile after removing built-in constants.
cd external/radio && go test -c -o /dev/null 2>&1 | head -20Repository: bjarneo/cliamp
Length of output: 514
🏁 Script executed:
#!/bin/bash
# Verify radio provider tests compile and run (or fail) after removing built-in constants.
cd external/radio && go test ./... 2>&1 | head -50Repository: bjarneo/cliamp
Length of output: 352
Fix broken tests: removed builtinName/builtinURL constants still referenced.
external/radio/provider_test.go references builtinName and builtinURL (lines 28–29, 67–68), but they no longer exist, causing go test to fail to build. Update the tests to match the new “default built-in radio is disabled” behavior (or remove those references).
🤖 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/radio/provider.go` at line 31, Tests in
external/radio/provider_test.go still reference the removed constants
builtinName and builtinURL; update the tests to reflect the new "default
built-in radio is disabled" behavior by removing those constant references and
changing assertions that expected the built-in entry to instead verify it is
absent (e.g., assert the radio list does not contain builtinName/builtinURL
strings or assert the default built-in radio count is zero), or simply remove
the lines that reference builtinName and builtinURL; adjust helper variables in
the test file as needed so no undefined symbols remain.
Source: Linters/SAST tools
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
ui/model/update.go (1)
726-730: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd error suppression rationale comment for consistency.
Volume persistence at Lines 728 and 784 suppresses save errors (same as
keys.goLines 596-599), but lacks the explanatory comment. For maintainability and consistency with the key-handler pattern, add a brief comment:case playback.SetVolumeMsg: m.player.SetVolume(msg.VolumeDB) + // Errors suppressed to avoid disrupting playback control flow. _ = m.configSaver.Save("volume", fmt.Sprintf("%.2f", msg.VolumeDB)) m.notifyAll() return m, nilApply the same pattern at Line 784 for
ipc.VolumeMsg.🤖 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 `@ui/model/update.go` around lines 726 - 730, At the two places where volume persistence errors are intentionally ignored—inside the playback.SetVolumeMsg handler (where m.configSaver.Save("volume", fmt.Sprintf("%.2f", msg.VolumeDB)) assigns to _ ) and inside the ipc.VolumeMsg handler—add the same brief explanatory comment used in the key-handler pattern (e.g., "// intentionally ignore save error: best-effort persistence for UX, not fatal") immediately above the assignment to _ so maintainers understand the suppression rationale and consistency is preserved.config/customisations.go (1)
29-35:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winWrap
appdir.Dir()error with context.Line 32 returns the raw error from
appdir.Dir(), inconsistent with error wrapping at lines 44, 52, 56, 69, 73, 78, 82, 91, and 98. As per coding guidelines, errors must be wrapped with context.Suggested fix
func customisationsPath() (string, error) { dir, err := appdir.Dir() if err != nil { - return "", err + return "", fmt.Errorf("customisations: resolve app directory: %w", err) } return filepath.Join(dir, "user_customisations.config"), nil }🤖 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 `@config/customisations.go` around lines 29 - 35, The function customisationsPath should wrap the error returned by appdir.Dir() with contextual text like the other functions; change the error return in customisationsPath (which calls appdir.Dir()) to return fmt.Errorf("getting app dir: %w", err) (or use your project's preferred error-wrapping helper) instead of returning err directly so callers receive a wrapped error with context.Source: Coding guidelines
cmd/customise.go (1)
34-36: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winMove user-facing success message to main/run flow.
Line 35 prints
Customisations saved successfully!in thecmdlayer. As per coding guidelines, user-facing messages should be surfaced frommain.go/run(...)only. Return a success indicator and let the caller incommands.gohandle the output.🤖 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 `@cmd/customise.go` around lines 34 - 36, The code currently prints a user-facing message directly where it checks cm.saved using fmt.Println; remove that direct print and instead return a success indicator from the function (e.g., change the function signature to return (bool, error) or at minimum bool) so the caller can decide how to present output; specifically, eliminate the fmt.Println("Customisations saved successfully!") line, return true when cm.saved is set (or return nil error / true as per your chosen signature), and update the caller to inspect the returned success flag and emit the user-visible message.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 `@cmd/customise.go`:
- Line 64: The code currently discards the error from config.Load() (cfg, _ :=
config.Load()), which can mask theme-loading failures and cause a silent
fallback to themes[0]; update this call to handle the returned error from
config.Load() (e.g., cfg, err := config.Load()), and either log the error with
context (using the existing logger) or return/propagate it from the surrounding
command handler so the wizard can surface the failure instead of silently
continuing; ensure references to cfg and the fallback to themes[0] remain valid
after the change and adjust control flow in the customise command to abort or
prompt the user if the config load fails.
- Around line 206-209: The loop that iterates paired extra fields (for i := 3; i
< len(m.fields); i += 2) relies on an undocumented invariant from
newCustomiseModel and can panic when i+1 is out of range; update the loop to be
defensive by changing the condition to for i := 3; i+1 < len(m.fields); i += 2
and/or add a short comment referencing newCustomiseModel's pairing contract so
future changes don't break m.fields indexing (ensure you modify the loop that
reads name := strings.TrimSpace(m.fields[i].val) and url :=
strings.TrimSpace(m.fields[i+1].val)).
In `@commands.go`:
- Around line 58-73: The Action handler in commands.go prints the success
message directly; remove the fmt.Println("All customisations have been cleared
and returned to defaults.") and instead return a sentinel value so main/run can
present the message. Define and return an exported sentinel error (e.g. var
ErrCustomisationsCleared = errors.New("customisations cleared")) from this code
path where config.ClearCustomisations() succeeds (replace the print+nil with
return ErrCustomisationsCleared), leaving the other returns unchanged; main.run
can detect ErrCustomisationsCleared and print the user-facing message.
---
Duplicate comments:
In `@cmd/customise.go`:
- Around line 34-36: The code currently prints a user-facing message directly
where it checks cm.saved using fmt.Println; remove that direct print and instead
return a success indicator from the function (e.g., change the function
signature to return (bool, error) or at minimum bool) so the caller can decide
how to present output; specifically, eliminate the fmt.Println("Customisations
saved successfully!") line, return true when cm.saved is set (or return nil
error / true as per your chosen signature), and update the caller to inspect the
returned success flag and emit the user-visible message.
In `@config/customisations.go`:
- Around line 29-35: The function customisationsPath should wrap the error
returned by appdir.Dir() with contextual text like the other functions; change
the error return in customisationsPath (which calls appdir.Dir()) to return
fmt.Errorf("getting app dir: %w", err) (or use your project's preferred
error-wrapping helper) instead of returning err directly so callers receive a
wrapped error with context.
In `@ui/model/update.go`:
- Around line 726-730: At the two places where volume persistence errors are
intentionally ignored—inside the playback.SetVolumeMsg handler (where
m.configSaver.Save("volume", fmt.Sprintf("%.2f", msg.VolumeDB)) assigns to _ )
and inside the ipc.VolumeMsg handler—add the same brief explanatory comment used
in the key-handler pattern (e.g., "// intentionally ignore save error:
best-effort persistence for UX, not fatal") immediately above the assignment to
_ so maintainers understand the suppression rationale and consistency is
preserved.
🪄 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: f02de456-6170-4dd5-963f-b04ce01e55b7
📒 Files selected for processing (10)
cmd/customise.gocommands.goconfig/customisations.goexternal/radio/provider.goexternal/ytplaylist/provider.gomain.goresolve/resolve.goui/model/keys.goui/model/update.goui/model/view.go
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
ui/model/update.go (1)
783-787: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winSame clamping issue as
playback.SetVolumeMsghandler above.See the previous comment for
playback.SetVolumeMsgat lines 726-731. This handler should also read backm.player.Volume()afterSetVolumeto save the clamped value.🤖 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 `@ui/model/update.go` around lines 783 - 787, The VolumeMsg handler currently saves the original msg.DB instead of the potentially clamped value; after calling m.player.SetVolume(msg.DB) read back the effective value via m.player.Volume() and pass that formatted/clamped value to m.configSaver.Save("volume", ...), keeping the existing best-effort ignore of the save error and still calling m.notifyAll(); update the save call to use fmt.Sprintf("%.2f", m.player.Volume()) so persisted state matches the actual player state.
🤖 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 `@main.go`:
- Around line 89-96: The loop that registers providers from cust.ExtraPlaylists
must skip entries with empty URLs: before calling ytplaylist.New(ep.URL) in the
for loop (the block that builds model.ProviderEntry with Key/Name/Provider), add
a guard that checks if ep.URL == "" and continue (optionally log or warn), so no
provider is created for empty URLs; leave name fallback logic as-is and only
create/append the model.ProviderEntry when ep.URL is non-empty.
- Around line 82-87: The code is saving user-provided playlist URLs
(cust.RadioReplacement and ep_url_* values) without validating they are
well-formed YouTube playlist URLs, causing failures later in ytplaylist/yt-dlp;
add upfront validation: implement a helper like
validateYouTubePlaylistURL(string) bool (or error) that checks for a YouTube
playlist pattern (e.g., host contains youtube.com or youtu.be and contains a
playlist id via "list=" or a valid playlist path), call that helper from the
customise wizard (cmd/customise.go) before assigning/saving
cust.RadioReplacement and ep_url_* (reject/return error to user if invalid), and
also call it from config.SaveCustomisations to validate any incoming save
payload and return an error instead of writing malformed URLs to JSON; update
any tests or error messages accordingly so
ytplaylist.New/resolve.ResolveYTDLBatch only receives validated URLs.
In `@ui/model/update.go`:
- Around line 726-731: In the playback.SetVolumeMsg case you save msg.VolumeDB
directly which can diverge if m.player.SetVolume clamps the value; after calling
m.player.SetVolume(msg.VolumeDB) read the actual clamped value via
m.player.Volume() and persist that (use m.configSaver.Save("volume",
fmt.Sprintf("%.2f", m.player.Volume()))), then notify; apply the same change in
the ipc.VolumeMsg handling so both code paths save the player's read-back volume
rather than the incoming msg value.
---
Duplicate comments:
In `@ui/model/update.go`:
- Around line 783-787: The VolumeMsg handler currently saves the original msg.DB
instead of the potentially clamped value; after calling
m.player.SetVolume(msg.DB) read back the effective value via m.player.Volume()
and pass that formatted/clamped value to m.configSaver.Save("volume", ...),
keeping the existing best-effort ignore of the save error and still calling
m.notifyAll(); update the save call to use fmt.Sprintf("%.2f",
m.player.Volume()) so persisted state matches the actual player state.
🪄 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: a4d8d984-a4ac-4e33-8b89-c51208122a8d
📒 Files selected for processing (10)
cmd/customise.gocommands.goconfig/customisations.goexternal/radio/provider.goexternal/ytplaylist/provider.gomain.goresolve/resolve.goui/model/keys.goui/model/update.goui/model/view.go
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/radio/provider.go`:
- Around line 239-240: Tests fail because catalog code still performs real HTTP
calls; modify the catalog-loading code to allow injecting a test HTTP client so
tests can use a mock or httptest server. Specifically, add an http.Client field
(e.g., httpClient) to the Provider struct (or the concrete type used by
loadCatalog/fetch functions), ensure functions that call the Radio Browser API
(e.g., loadCatalog, fetchRadioBrowser, searchStations, or any method that
performs http.Get/Do) use that httpClient instead of
http.Get/http.DefaultClient, default the field to http.DefaultClient in Provider
creation (e.g., NewProvider) for production, and update catalog tests
(TestSearchStationsSuccess, TestSearchStationsDefaultLimit,
TestTopStationsOffset, TestTopStationsOffsetClampsNegatives) to supply either an
httptest.Server or a mock http.Client to return deterministic responses; no
other behavioral changes are required.
In `@external/ytplaylist/provider.go`:
- Around line 14-17: The New constructor currently accepts any string; modify
New(url string) *Provider to validate the input and return nil for invalid URLs:
trim whitespace (use strings.TrimSpace) and if empty return nil, and also check
for expected YouTube playlist patterns (e.g., strings.Contains(url,
"youtube.com") || strings.Contains(url, "youtu.be") || strings.Contains(url,
"list=")) and return nil if none match; update imports to include "strings" and
keep Provider struct the same; callers should check for nil provider (no other
files need changes here).
In `@main.go`:
- Around line 89-99: The loop that builds providers from cust.ExtraPlaylists
should defensively validate ep.URL before creating a ytplaylist provider: trim
whitespace from ep.URL, skip if the trimmed value is empty, and parse it with
net/url.Parse (or url.ParseRequestURI) to ensure it is a well-formed URL with an
http or https scheme; use the trimmed URL when calling ytplaylist.New and append
only when validation succeeds (optionally log or continue on invalid URLs).
Update the code around cust.ExtraPlaylists, ep.URL, and the ProviderEntry
construction so malformed or whitespace-only URLs are ignored and only
validated, trimmed URLs are passed into ytplaylist.New.
- Around line 82-87: cust.RadioReplacement is used unchecked and may be
whitespace or a malformed YouTube playlist URL before being passed to
ytplaylist.New; update the conditional around the provider setup to first
strings.TrimSpace(cust.RadioReplacement) into a local variable, check it's
non-empty, parse it with net/url.Parse and validate it looks like a YouTube
playlist URL (host contains "youtube.com" or "youtu.be" and query contains a
non-empty "list" param, or other allowed pattern); if validation passes call
ytplaylist.New(trimmed) and append that ProviderEntry, otherwise fall back to
creating radio.New() and append that ProviderEntry. Ensure you reference
cust.RadioReplacement, ytplaylist.New, radio.New and model.ProviderEntry in the
changed block so malformed/whitespace inputs never reach ytplaylist.New.
In `@resolve/resolve.go`:
- Around line 458-467: The code may wrap a nil error when falling back from
client.GetPlaylist to resolveYTDL: if the native playlist call succeeded but
returned zero videos (pl.Videos empty) and resolveYTDL also returns zero tracks
(errYTDL == nil), the final fmt.Errorf("youtube playlist resolve: %w", err) will
wrap nil; to fix, ensure the original err is set to a meaningful non-nil
sentinel before wrapping — e.g., when detecting an empty playlist from the
native client (where you currently have err == nil but len(pl.Videos) == 0)
assign err = fmt.Errorf("empty playlist") (or a descriptive error), then keep
the existing logic that prefers returning errYTDL when non-nil and otherwise
wraps err; reference symbols: resolveYTDL, errYTDL, err, and the native playlist
result (pl / pl.Videos) so the sentinel assignment happens right before the
fallback/return block shown.
🪄 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: 920da2f4-479c-4b1b-be5f-4234dc2b9d0f
📒 Files selected for processing (10)
cmd/customise.gocommands.goconfig/customisations.goexternal/radio/provider.goexternal/ytplaylist/provider.gomain.goresolve/resolve.goui/model/keys.goui/model/update.goui/model/view.go
Comment on lines
+14
to
+17
| // New creates a new YouTube playlist provider. | ||
| func New(url string) *Provider { | ||
| return &Provider{url: url} | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Validate URL parameter in constructor.
New() accepts any string without validation. If an empty, whitespace-only, or malformed URL is passed (e.g., from a hand-edited config file), the provider will be created but fail later when Tracks() calls resolve.ResolveYTDLBatch. Validating up front improves error clarity and prevents broken providers from being registered.
🛡️ Suggested validation
func New(url string) *Provider {
+ if strings.TrimSpace(url) == "" {
+ return nil
+ }
return &Provider{url: url}
}Add an import for strings if not already present. Callers should check for nil return.
Alternatively, validate that the URL contains expected YouTube playlist patterns (youtube.com, youtu.be, list=) to catch malformed URLs early.
📝 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
| // New creates a new YouTube playlist provider. | |
| func New(url string) *Provider { | |
| return &Provider{url: url} | |
| } | |
| // New creates a new YouTube playlist provider. | |
| func New(url string) (*Provider, error) { | |
| if strings.TrimSpace(url) == "" { | |
| return nil, fmt.Errorf("invalid url: empty or whitespace-only") | |
| } | |
| return &Provider{url: url}, nil | |
| } |
🤖 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/ytplaylist/provider.go` around lines 14 - 17, The New constructor
currently accepts any string; modify New(url string) *Provider to validate the
input and return nil for invalid URLs: trim whitespace (use strings.TrimSpace)
and if empty return nil, and also check for expected YouTube playlist patterns
(e.g., strings.Contains(url, "youtube.com") || strings.Contains(url, "youtu.be")
|| strings.Contains(url, "list=")) and return nil if none match; update imports
to include "strings" and keep Provider struct the same; callers should check for
nil provider (no other files need changes here).
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@main.go`:
- Around line 245-246: The code appends untrimmed cust.RadioReplacement into
resolved.Pending causing whitespace-only URLs; fix by trimming before the check:
compute radioUrl := strings.TrimSpace(cust.RadioReplacement) (or reuse the
existing radioUrl declared in broader scope) and change the condition to if
radioUrl != "" { resolved.Pending = append(resolved.Pending, radioUrl) } so only
non-empty, trimmed values are added; ensure any remaining uses of
cust.RadioReplacement use the trimmed radioUrl.
🪄 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: 0570d002-5b0a-4aeb-b311-9f9f90eb466a
📒 Files selected for processing (10)
cmd/customise.gocommands.goconfig/customisations.goexternal/radio/provider.goexternal/ytplaylist/provider.gomain.goresolve/resolve.goui/model/keys.goui/model/update.goui/model/view.go
…stence, and UX improvements
Author
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.
feat: add
--customisewizard, YouTube playlist provider, volume persistence, and UX improvementsSummary
This PR adds a non-breaking personalisation layer on top of cliamp that lets users configure the player's appearance and providers without touching any config files directly. All features gracefully fall back to upstream defaults when unconfigured.
Features added
1. Interactive customisation wizard (
cliamp --customise)A Bubbletea TUI wizard accessible via the new
--customiseglobal flag. Walks the user through every available option and writes the result to~/.config/cliamp/user_customisations.config(JSON). No file exists → player behaves identically to the upstream defaults.Also adds
--customise --clear-allto wipe all customisations in one command and return to factory defaults.2. YouTube playlist as a provider (
external/ytplaylist)A new
playlist.Providerimplementation that wraps a YouTube playlist URL and scrapes its tracks on demand viayt-dlp. Users can:3. Rename provider pills
Any source name shown in the SRC pill (e.g.
Radio,Local) can be replaced with an arbitrary alphanumeric string via the wizard. Stored inuser_customisations.configunderprovider_names.4. Volume persistence
Volume is now saved to
config.tomlon every change — via+/=/-keypresses, theSetVolumemessage, or IPC. The saved value is picked up automatically on the next launch, so the player always opens at the last-used volume.5. Automatic single-playlist loading
When a provider exposes exactly one playlist (e.g. a custom YouTube playlist provider), the track list is loaded immediately without requiring the user to press Enter on the playlist selection screen.
6. SRC pill always visible
The source-selection pill is now rendered even when only one provider is configured. Previously it was hidden when
len(providers) <= 1, which made the UI look broken after hiding the Local provider.7. Hide Local provider toggle
A
hide_local: true/falseflag inuser_customisations.config(configurable via the wizard) that removes the Local file provider from the SRC pill entirely.8. Improved
yt-dlperror reportingExtracted a
cleanYTDLErrorhelper that strips multi-lineyt-dlpwarning output and returns only the most relevantERROR:line. Prevents multi-line stderr from breaking the TUI layout.New files
cmd/customise.go--customisewizardconfig/customisations.gouser_customisations.configexternal/ytplaylist/provider.goplaylist.ProviderimplementationModified files
commands.go--customiseand--clear-allglobal flagsmain.gouser_customisations.config; Local provider conditionally includedresolve/resolve.gocleanYTDLErrorhelper; improved playlist fallback error propagationexternal/radio/provider.goui/model/update.goSetVolumeMsg/ipc.VolumeMsgui/model/keys.go+/=/-key pressui/model/view.golen(providers) >= 1(was> 1)Screenshots / video
cliamp --customisewizardSRC pill with a custom YouTube playlist provider
How to test
go build -o cliamp ../cliamp --customiseand step through the wizard:My Radio)trueand save./cliamp— verify the SRC pill shows your custom name and the Local provider is gone+/-then quit and relaunch — verify volume is restored./cliamp --customise --clear-all— verify all customisations are removed and the player returns to upstream defaultsuser_customisations.configpresent, the player is indistinguishable from the upstream buildChecklist
make checkpassesdocs/andsite/index.htmlupdated for user-facing changesuser_customisations.configadded to.gitignore(personal config, not tracked)user_customisations.configis absentSummary by CodeRabbit
New Features
Improvements