feat: logging improvements and refactoring #3493
Conversation
src/cmd/devtools.go
Outdated
| @@ -68,5 +68,5 @@ func SetDevTools() { | |||
|
|
|||
| file.WriteAt([]byte{50}, firstPatchLocation) | |||
| file.WriteAt([]byte{50}, secondPatchLocation) | |||
| utils.PrintSuccess("Enabled DevTools!") | |||
| utils.PrintSuccess("Enable DevTools") | |||
There was a problem hiding this comment.
? xD
| utils.PrintSuccess("Enable DevTools") | |
| utils.PrintSuccess("Enabled DevTools!") |
src/preprocess/preprocess.go
Outdated
| utils.PrintInfo("If above warning says 'could not find start marker', you can safely ignore that error if you're on Spotify 1.2.63 or lower. However, if you're on 1.2.64 or higher, please report this issue") | ||
| if spotifyMajor >= 1 && spotifyMinor >= 2 && spotifyPatch >= 64 { | ||
| utils.PrintWarning(fmt.Sprintf("Could not process %s: %v", binFilePath, err)) | ||
| utils.PrintWarning("Please report this!") |
spicetify.go
Outdated
| @@ -115,16 +114,15 @@ func init() { | |||
| } | |||
|
|
|||
| if isAdmin.Check(bypassAdminCheck) { | |||
| utils.PrintError("Spicetify should not be run with administrator/root privileges") | |||
| utils.PrintError("Running as admin can cause Spotify to show a black/blank window after applying spicetify") | |||
| utils.PrintError("Spicetify should NOT be ran with administrator/root privileges") | |||
There was a problem hiding this comment.
English modals - verb is in bare infinitive
| utils.PrintError("Spicetify should NOT be ran with administrator/root privileges") | |
| utils.PrintError("Spicetify should NOT be run with administrator or root privileges") |
spicetify.go
Outdated
| } | ||
|
|
||
| return | ||
| } else { | ||
| cmd.CheckUpdate(version) | ||
| } | ||
|
|
||
| cmd.SpotifyKill() |
There was a problem hiding this comment.
why do you kill spotify. do not do that ever
WalkthroughReplace many console prints with pterm prefix printers and spinners; add SpotifyKill/SpotifyStart/SpotifyRestart and rename SetDevTools → EnableDevTools; change Backup signature to accept a second bool; remove legacy Tracker/TernaryBool utilities; simplify preprocess patch APIs and adjust user-facing messages. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant CLI as spicetify (CLI)
participant Restart as cmd.SpotifyRestart
participant Dev as cmd.EnableDevTools
participant Kill as cmd.SpotifyKill
participant Start as cmd.SpotifyStart
participant OS as OS/Process
U->>CLI: run with restart/debug flags
CLI->>Restart: SpotifyRestart(flags...)
rect rgba(200,240,255,0.12)
note over Restart: Restart orchestration
Restart->>Dev: if always_enable_devtools
Dev-->>Restart: enabled/skip
Restart->>Kill: SpotifyKill()
Kill-->>Restart: killed/confirmed
Restart->>Start: SpotifyStart(flags...)
Start->>OS: launch Spotify process
OS-->>Start: started
end
Restart-->>CLI: return
CLI-->>U: exit
sequenceDiagram
autonumber
participant U as User
participant CLI as spicetify (CLI)
participant Spinner as utils.Spinner
participant Backup as cmd.Backup
participant Apply as cmd.Apply
U->>CLI: spicetify auto
CLI->>Spinner: start "Auto"
CLI->>Backup: Backup(version, true)
alt backup succeeded and apply enabled
Backup-->>CLI: backup complete
CLI->>Apply: Apply
Apply-->>CLI: apply complete
else backup only or failed
Backup-->>CLI: backup complete / error
end
CLI->>Spinner: success / stop
CLI-->>U: done
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (10)
go.mod (1)
3-3: Fix invalidgodirective: use major.minor only; optionally addtoolchainfor the patch versionThe
godirective cannot include a patch version. If you want to pin the toolchain, use thetoolchaindirective separately.Apply this diff:
-go 1.24.2 +go 1.24 +toolchain go1.24.2src/cmd/backup.go (2)
43-50: Inconsistent error handling: use spinner.Fail() and utils.Fatal instead of log.FatalThese two fatal paths bypass the spinner and the new Fatal() helper.
- if err := backup.Start(appPath, backupFolder); err != nil { - log.Fatal(err) - } + if err := backup.Start(appPath, backupFolder); err != nil { + spinner.Fail() + utils.Fatal(err) + } @@ - appList, err := os.ReadDir(backupFolder) - if err != nil { - log.Fatal(err) - } + appList, err := os.ReadDir(backupFolder) + if err != nil { + spinner.Fail() + utils.Fatal(err) + }You can also drop the log import after this change.
63-81: “Preprocessing completed” prints even when preprocessing didn’t runSuccess is printed unconditionally, even when spotifyBasePath is empty and preprocessing is skipped. Wrap the entire preprocessing step in a spinner and only mark success when it actually runs.
- utils.PrintBold("Preprocessing") - - spotifyBasePath := spotifyPath - if spotifyBasePath == "" { - utils.PrintError("Spotify installation path not found. Cannot preprocess V8 snapshots") - } else { - preprocess.Start( - spicetifyVersion, - spotifyBasePath, - rawFolder, - preprocess.Flag{ - DisableSentry: preprocSection.Key("disable_sentry").MustBool(false), - DisableLogging: preprocSection.Key("disable_ui_logging").MustBool(false), - RemoveRTL: preprocSection.Key("remove_rtl_rule").MustBool(false), - ExposeAPIs: preprocSection.Key("expose_apis").MustBool(false), - SpotifyVer: utils.GetSpotifyVersion(prefsPath)}, - ) - } - utils.PrintSuccess("Preprocessing completed") + preSpinner, _ := utils.Spinner.Start("Preprocessing") + spotifyBasePath := spotifyPath + if spotifyBasePath == "" { + preSpinner.Fail() + utils.PrintError("Spotify installation path not found. Cannot preprocess V8 snapshots") + } else { + preprocess.Start( + spicetifyVersion, + spotifyBasePath, + rawFolder, + preprocess.Flag{ + DisableSentry: preprocSection.Key("disable_sentry").MustBool(false), + DisableLogging: preprocSection.Key("disable_ui_logging").MustBool(false), + RemoveRTL: preprocSection.Key("remove_rtl_rule").MustBool(false), + ExposeAPIs: preprocSection.Key("expose_apis").MustBool(false), + SpotifyVer: utils.GetSpotifyVersion(prefsPath), + }, + ) + preSpinner.Success() + utils.PrintSuccess("Preprocessing completed") + }src/cmd/apply.go (1)
63-67: Check error when copying spicetifyWrapper.jsCopyFile returns an error which is currently ignored.
- if preprocSection.Key("expose_apis").MustBool(false) { - utils.CopyFile( - filepath.Join(utils.GetJsHelperDir(), "spicetifyWrapper.js"), - filepath.Join(appDestPath, "xpui", "helper")) - } + if preprocSection.Key("expose_apis").MustBool(false) { + if err := utils.CopyFile( + filepath.Join(utils.GetJsHelperDir(), "spicetifyWrapper.js"), + filepath.Join(appDestPath, "xpui", "helper")); err != nil { + utils.PrintError(err.Error()) + } + }spicetify.go (2)
53-61: Fix potential out-of-bounds access when parsing flagsAccessing v[1] before validating length can panic for inputs like "-" or "-". Reorder the checks so length is validated first.
Apply this diff:
- if len(v) > 0 && v[0] == '-' { - if v[1] != '-' && len(v) > 2 { + if len(v) > 0 && v[0] == '-' { + // If it's a group of short flags like -abc, expand them. + // Only read v[1] after ensuring length > 2 to avoid OOB panics. + if len(v) > 2 && v[1] != '-' { for _, char := range v[1:] { flags = append(flags, "-"+string(char)) } } else { flags = append(flags, v) }
111-114: Do not set os.Stdout to nil — can cause panicsSetting os.Stdout = nil can crash if any code prints to stdout (fmt, pterm defaults, etc). Prefer disabling output via the logging/printing layer.
Apply this diff to avoid panics and properly silence pterm output:
import ( "errors" "io" "log" "os" "os/exec" "path/filepath" "runtime" "slices" "strings" "sync" colorable "github.com/mattn/go-colorable" + "github.com/pterm/pterm" "github.com/spicetify/cli/src/cmd" spotifystatus "github.com/spicetify/cli/src/status/spotify" "github.com/spicetify/cli/src/utils" "github.com/spicetify/cli/src/utils/isAdmin" ) @@ if quiet { log.SetOutput(io.Discard) - os.Stdout = nil + // Silence pterm-based printers as well. + pterm.DisableOutput() }If you prefer not to add the pterm import here, at minimum remove the os.Stdout assignment to avoid nil dereferences. I can wire quiet-mode into utils.Print* globally if you want.
src/preprocess/preprocess.go (4)
57-70: Close HTTP response body and validate status code when reading remote CSS mapThe response body isn’t closed, and non-200 responses aren’t handled, which can leak descriptors and mislead error handling.
Apply this diff:
func readRemoteCssMap(tag string, cssTranslationMap *map[string]string) error { var cssMapURL string = "https://raw.githubusercontent.com/spicetify/cli/" + tag + "/css-map.json" cssMapResp, err := http.Get(cssMapURL) if err != nil { return err } else { + defer cssMapResp.Body.Close() + if cssMapResp.StatusCode != http.StatusOK { + return fmt.Errorf("unexpected status fetching CSS map: %s", cssMapResp.Status) + } err := json.NewDecoder(cssMapResp.Body).Decode(cssTranslationMap) if err != nil { utils.PrintWarning("Remote CSS map JSON malformed.") return err } } return nil }
1039-1046: Bug: splitVersion silently swallows conversion errorsReturning nil error on Atoi failure makes callers treat invalid versions as 0.0.0, leading to wrong tag selection.
Apply this diff:
for i := 0; i < 3; i++ { conv, err := strconv.Atoi(vSplit[i]) if err != nil { - return [3]int{}, nil + return [3]int{}, err } vInts[i] = conv }
1073-1082: Close GitHub releases response bodyAvoid leaking the HTTP response body.
Apply this diff:
res, err := http.Get("https://api.github.com/repos/spicetify/cli/releases") if err != nil { return "", err } - body, err := io.ReadAll(res.Body) + defer res.Body.Close() + body, err := io.ReadAll(res.Body)
17-19: Clean up stale pterm v0.12.81 entries from go.sumgo.mod correctly pins
github.com/pterm/ptermto v0.12.80, but your go.sum still contains leftover v0.12.81 entries:
- github.com/pterm/pterm v0.12.81
- github.com/pterm/pterm v0.12.81/go.mod
Please run:
go mod tidyand commit the updated go.sum to remove those obsolete versions.
🧹 Nitpick comments (16)
src/utils/config.go (1)
37-37: Behavior change: defaultsidebar_configset to "0" (disabled)This flips the default. Confirm downstream docs/prompts reflect this new default to avoid user confusion.
src/cmd/devtools.go (1)
71-71: Success message grammarConsider past tense for user feedback consistency.
- utils.PrintSuccess("Enable DevTools") + utils.PrintSuccess("Enabled DevTools!")Additionally, there is a latent robustness issue if the markers are not found and for unhandled write errors. Consider adding guards and error checks. Example (outside this hunk, for reference):
// Guard against missing markers and handle write errors. if firstLocation < 0 || secondLocation < 0 { utils.PrintError(`Could not locate "app-developer" markers in offline.bnk`) os.Exit(1) } if _, err := file.WriteAt([]byte{50}, firstPatchLocation); err != nil { log.Fatal(err) } if _, err := file.WriteAt([]byte{50}, secondPatchLocation); err != nil { log.Fatal(err) }src/cmd/color.go (2)
93-103: Avoid implicit key creation; use GetKey to read without side effectsUsing
colorSection.Key(k)may implicitly create empty keys depending on ini lib behavior. PreferGetKeyto avoid mutating the in-memory config during display.- for _, k := range utils.BaseColorOrder { - colorString := colorSection.Key(k).String() - - if len(colorString) == 0 { - colorString = utils.BaseColorList[k] - k += " (*)" - } - - color := utils.ParseColor(colorString) - data = append(data, []string{utils.Bold(k), colorPreview(color), color.Hex(), color.RGB()}) - } + for _, name := range utils.BaseColorOrder { + displayName := name + colorString := "" + if key, err := colorSection.GetKey(name); err == nil { + colorString = key.String() + } + if len(colorString) == 0 { + colorString = utils.BaseColorList[name] + displayName += " (*)" + } + color := utils.ParseColor(colorString) + data = append(data, []string{utils.Bold(displayName), colorPreview(color), color.Hex(), color.RGB()}) + }
116-119: Use unified printing helper instead of log for the footnoteFor consistency with the new pterm-based UI, prefer utils.PrintInfo (or Note) over raw log.Println.
- pterm.DefaultTable.WithHasHeader().WithData(data).Render() - - log.Println("(*): Default color is used") + _ = pterm.DefaultTable.WithHasHeader().WithData(data).Render() + utils.PrintInfo("(*): Default color is used")src/backup/backup.go (1)
17-33: Guard spinner usage to avoid panics if Start fails (pterm spinner bug workaround).Given the known spinner-related issue and the ignored error from Start, calling Fail/Success on a nil spinner can panic. Make spinner calls nil-safe and capture Start error.
Apply this diff:
- spinner, _ := utils.Spinner.Start("Extract backup") + spinner, spErr := utils.Spinner.Start("Extract backup") + if spErr != nil { + // Fallback for non-interactive environments or spinner init failures + spinner = nil + } @@ - spinner.Fail() + if spinner != nil { + spinner.Fail() + } utils.Fatal(err) @@ - spinner.Success() + if spinner != nil { + spinner.Success() + }Optional: Consider marking success only if at least one .spa was actually extracted to avoid a potentially misleading success tick when both apps are absent. I can provide a small counter-based tweak if desired.
src/cmd/cmd.go (2)
248-253: Avoid duplicating “[Y/n]” or “[y/N]” on repeated invalid input.Mutating info on every call causes the hint suffix to grow when recursion happens. Build a prompt variable instead.
Apply this diff:
- if defaultAnswer { - info += " [Y/n]: " - } else { - info += " [y/N]: " - } - reader := bufio.NewReader(os.Stdin) - fmt.Print(info) + prompt := info + if defaultAnswer { + prompt += " [Y/n]: " + } else { + prompt += " [y/N]: " + } + reader := bufio.NewReader(os.Stdin) + fmt.Print(prompt)Also applies to: 255-256
283-286: Normalize version strings when comparing update availability.Tags may include a leading “v” while your version may not. Normalize both sides before comparison to prevent false “update available” messages.
Apply this diff:
- if latestTag != version { - utils.PrintInfo("New version available: v" + latestTag + " (currently on: v" + version + ")") + lt := strings.TrimPrefix(latestTag, "v") + v := strings.TrimPrefix(version, "v") + if lt != v { + utils.PrintInfo("New version available: v" + lt + " (currently on: v" + v + ")") utils.PrintInfo(`Run "spicetify update" or use a package manager to update spicetify`) }Would you like me to add a small unit-style check to ensure normalization behaves as expected for both “vX.Y.Z” and “X.Y.Z” inputs?
src/cmd/watch.go (2)
219-222: DevTools/restart flow refactor — minor nit: potential double-enabling DevTools.EnableDevTools() is called here unconditionally, and SpotifyRestart() will also call EnableDevTools() if always_enable_devtools is true (restart.go). It’s idempotent, but slightly redundant. Not urgent.
222-224: Busy-wait loop can burn CPU; sleep briefly while waiting.Insert a small sleep in the loop that waits for the debugger to come up.
Apply this diff:
- for len(utils.GetDebuggerPath()) == 0 { - // Wait until debugger is up - } + for len(utils.GetDebuggerPath()) == 0 { + // Wait until debugger is up + time.Sleep(200 * time.Millisecond) + }src/utils/path-utils.go (1)
15-24: Make spinner usage nil-safe across migrations; capture Start errors.Start errors are ignored; calling Fail/Success on a nil spinner could panic (relevant given the pterm spinner bug). Guard these calls and capture Start errors.
Apply this diff:
- spinner, _ := Spinner.Start("Migrate spicetify config folder") + spinner, spErr := Spinner.Start("Migrate spicetify config folder") + if spErr != nil { + spinner = nil + } destination := GetSpicetifyFolder() err := Copy(source, destination, true, nil) if err != nil { - spinner.Fail() + if spinner != nil { + spinner.Fail() + } Fatal(err) } os.RemoveAll(source) - spinner.Success() + if spinner != nil { + spinner.Success() + }- spinner, _ := Spinner.Start(`Migrate spicetify backup folder`) + spinner, spErr := Spinner.Start(`Migrate spicetify backup folder`) + if spErr != nil { + spinner = nil + } err := Copy(backupPath, newBackupPath, true, nil) if err != nil { - spinner.Fail() + if spinner != nil { + spinner.Fail() + } Fatal(err) } os.RemoveAll(backupPath) - spinner.Success() + if spinner != nil { + spinner.Success() + }- spinner, _ := Spinner.Start(`Migrate spicetify extracted folder`) + spinner, spErr := Spinner.Start(`Migrate spicetify extracted folder`) + if spErr != nil { + spinner = nil + } err := Copy(extractedPath, newExtractedPath, true, nil) if err != nil { - spinner.Fail() + if spinner != nil { + spinner.Fail() + } Fatal(err) } os.RemoveAll(extractedPath) - spinner.Success() + if spinner != nil { + spinner.Success() + }Optional: consider checking and logging errors from os.RemoveAll(...) with a warning before showing success. I can add that if you want tighter UX semantics.
Also applies to: 45-52, 66-74
src/cmd/restart.go (2)
35-38: Avoid appending empty launch flagsStrings("|") can return a slice with empty entries. Filter them out to avoid passing empty args to the process.
- launchFlag := settingSection.Key("spotify_launch_flags").Strings("|") - if len(launchFlag) > 0 { - flags = append(flags, launchFlag...) - } + launchFlag := settingSection.Key("spotify_launch_flags").Strings("|") + for _, f := range launchFlag { + if s := strings.TrimSpace(f); s != "" { + flags = append(flags, s) + } + }Note: requires the strings import (added in the previous comment).
14-18: Windows process detection is brittle; consider simplifying to unconditional taskkillRelying on the localized “No tasks are running” string is fragile. Since you ignore errors, you can invoke taskkill unconditionally.
- isRunning := exec.Command("tasklist", "/FI", "ImageName eq spotify.exe") - result, _ := isRunning.Output() - if !bytes.Contains(result, []byte("No tasks are running")) { - exec.Command("taskkill", "/F", "/IM", "spotify.exe").Run() - } + _ = exec.Command("taskkill", "/F", "/IM", "spotify.exe").Run()This removes the dependency on tasklist output and locales.
src/utils/print.go (1)
48-59: Preserve pterm defaults when customizing SpinnerBuilding Spinner via a raw struct literal may miss important defaults (e.g., RemoveWhenDone). Prefer deriving from pterm.DefaultSpinner and overriding needed fields.
- Spinner = pterm.SpinnerPrinter{ - Sequence: []string{"-", "\\", "|", "/"}, - Style: &pterm.ThemeDefault.SpinnerStyle, - Delay: time.Millisecond * 200, - TimerStyle: &pterm.ThemeDefault.TimerStyle, - MessageStyle: &pterm.ThemeDefault.SpinnerTextStyle, - InfoPrinter: &Info, - SuccessPrinter: &Success, - FailPrinter: &Error, - WarningPrinter: &Warning, - Writer: os.Stderr, - } + Spinner = *pterm.DefaultSpinner. + WithSequence("-", "\\", "|", "/"). + WithStyle(&pterm.ThemeDefault.SpinnerStyle). + WithDelay(time.Millisecond * 200). + WithTimerStyle(&pterm.ThemeDefault.TimerStyle). + WithMessageStyle(&pterm.ThemeDefault.SpinnerTextStyle). + WithInfoPrinter(&Info). + WithSuccessPrinter(&Success). + WithFailPrinter(&Error). + WithWarningPrinter(&Warning). + WithWriter(os.Stderr)This keeps sane defaults while applying your theme and sequence.
src/cmd/update.go (1)
98-99: Success message: ensure single “v” prefixWith the normalization above, this prints the correct version regardless of whether the tag is prefixed.
- utils.PrintSuccess("Successfully updated Spicetify to v" + tagName) + utils.PrintSuccess("Successfully updated Spicetify to v" + strings.TrimPrefix(tagName, "v"))spicetify.go (2)
116-121: Tightened admin warning copy: minor grammar nit“should NOT be ran” should be “should NOT be run”.
Apply this diff:
- utils.PrintError("Spicetify should NOT be ran with administrator/root privileges") + utils.PrintError("Spicetify should NOT be run with administrator/root privileges")
307-309: Avoid unconditionally killing Spotify for all chainable commandsKilling the client preemptively for every chainable command can be surprising (e.g., for “clear”). Gate this to commands that actually need Spotify stopped. “restart” already kills within SpotifyRestart().
Apply this diff:
- cmd.SpotifyKill() + // Only kill Spotify for commands that modify or relaunch it. + if slices.Contains(commands, "backup") || + slices.Contains(commands, "apply") || + slices.Contains(commands, "refresh") || + slices.Contains(commands, "restore") || + slices.Contains(commands, "enable-devtools") || + slices.Contains(commands, "restart") || + slices.Contains(commands, "auto") { + cmd.SpotifyKill() + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (21)
go.mod(1 hunks)spicetify.go(10 hunks)src/backup/backup.go(2 hunks)src/cmd/apply.go(9 hunks)src/cmd/auto.go(1 hunks)src/cmd/backup.go(4 hunks)src/cmd/cmd.go(2 hunks)src/cmd/color.go(2 hunks)src/cmd/config.go(1 hunks)src/cmd/devtools.go(2 hunks)src/cmd/patch.go(0 hunks)src/cmd/restart.go(1 hunks)src/cmd/update.go(2 hunks)src/cmd/watch.go(1 hunks)src/preprocess/preprocess.go(9 hunks)src/utils/config.go(1 hunks)src/utils/path-utils.go(3 hunks)src/utils/print.go(2 hunks)src/utils/show-dir.go(0 hunks)src/utils/ternary-bool.go(0 hunks)src/utils/tracker.go(0 hunks)
💤 Files with no reviewable changes (4)
- src/utils/show-dir.go
- src/utils/ternary-bool.go
- src/utils/tracker.go
- src/cmd/patch.go
🧰 Additional context used
🧬 Code Graph Analysis (13)
src/cmd/watch.go (4)
src/cmd/devtools.go (1)
EnableDevTools(15-72)src/cmd/restart.go (1)
SpotifyRestart(58-66)src/utils/print.go (3)
PrintInfo(98-100)PrintError(88-90)PrintSuccess(93-95)src/utils/watcher.go (2)
GetDebuggerPath(96-119)SendReload(122-138)
src/cmd/cmd.go (1)
src/utils/print.go (1)
PrintInfo(98-100)
src/utils/path-utils.go (2)
src/utils/print.go (3)
Spinner(48-59)Fatal(103-106)Success(18-23)src/utils/utils.go (1)
Copy(84-138)
src/cmd/devtools.go (1)
src/utils/print.go (1)
PrintSuccess(93-95)
src/backup/backup.go (1)
src/utils/print.go (3)
Spinner(48-59)Fatal(103-106)Success(18-23)
src/cmd/backup.go (5)
src/utils/print.go (7)
PrintInfo(98-100)PrintWarning(83-85)Spinner(48-59)Success(18-23)PrintBold(73-75)PrintSuccess(93-95)Fatal(103-106)src/cmd/cmd.go (1)
ReadAnswer(243-267)src/status/backup/backup.go (1)
Get(32-61)src/backup/backup.go (2)
Start(11-13)Extract(16-34)src/utils/utils.go (1)
Copy(84-138)
src/cmd/update.go (1)
src/utils/print.go (5)
PrintSuccess(93-95)PrintInfo(98-100)Spinner(48-59)Fatal(103-106)Success(18-23)
src/cmd/auto.go (1)
src/cmd/backup.go (1)
Backup(17-96)
src/cmd/restart.go (1)
src/cmd/devtools.go (1)
EnableDevTools(15-72)
src/preprocess/preprocess.go (2)
src/utils/print.go (7)
Spinner(48-59)Warning(24-29)PrintWarning(83-85)Error(30-35)PrintInfo(98-100)Success(18-23)Bold(63-65)src/utils/utils.go (1)
ReplaceOnce(175-188)
src/cmd/apply.go (4)
src/utils/print.go (8)
Spinner(48-59)Fatal(103-106)Success(18-23)Warning(24-29)PrintWarning(83-85)Info(12-17)PrintError(88-90)Error(30-35)src/preprocess/preprocess.go (1)
Start(88-277)src/utils/utils.go (3)
Copy(84-138)CopyFile(141-163)CreateJunction(317-328)src/apply/apply.go (1)
UserAsset(115-121)
spicetify.go (7)
src/cmd/cmd.go (2)
GetConfigPath(199-201)InitSetting(117-196)src/utils/print.go (5)
PrintError(88-90)PrintInfo(98-100)PrintNote(78-80)Fatal(103-106)Bold(63-65)src/cmd/backup.go (1)
Backup(17-96)src/cmd/apply.go (2)
CheckStates(183-211)Apply(17-95)src/cmd/restart.go (2)
SpotifyRestart(58-66)SpotifyKill(11-32)src/cmd/devtools.go (1)
EnableDevTools(15-72)src/cmd/auto.go (1)
Auto(12-36)
src/cmd/color.go (2)
src/utils/color.go (4)
BaseColorOrder(40-59)BaseColorList(18-37)ParseColor(76-124)Color(67-71)src/utils/print.go (3)
Bold(63-65)PrintSuccess(93-95)PrintInfo(98-100)
🔇 Additional comments (21)
go.mod (1)
8-8: Downgrade to pterm v0.12.80: API usage verifiedI’ve confirmed that in v0.12.80 the following remain available and unchanged, so your table and spinner code will continue to compile and behave as expected:
• SpinnerPrinter (used in src/utils/print.go at lines ~48–50)
• TableData and DefaultTable.WithHasHeader().WithData(...).Render() (in src/cmd/color.go at lines ~90–116)No further changes are needed.
src/cmd/config.go (1)
207-209: Tighter toggle validation looks goodRestricting to "0"/"1" is clear and consistent with config semantics and messaging.
src/cmd/devtools.go (1)
14-16: EnableDevTools rename verified – noSetDevToolsreferences found
LGTM – the ripgrep check forSetDevTools(returned no matches, so there are no lingering call sites to update.src/cmd/color.go (1)
8-8: pterm import for table rendering looks correctAligned with the pterm downgrade in go.mod and the new table-based DisplayColors.
src/cmd/auto.go (1)
18-18: Backup call updated for silent mode — looks good.The call site matches the new signature Backup(spicetifyVersion string, silent bool), and the intent (quiet within auto) is clear.
src/cmd/update.go (1)
14-24: Verified GitHub tag prefix and asset naming
The latest release tag isv2.41.0and assets follow thespicetify-<version>-<os>-<arch>.<ext>pattern (e.g.spicetify-2.41.0-darwin-amd64.tar.gz,spicetify-2.41.0-windows-x64.zip). No code changes needed.spicetify.go (10)
68-75: Short alias for bypass-admin and config path logging look good
- Accepting both “-b” and “--bypass-admin” is consistent and backwards compatible.
- Logging config path via log.Println aligns with the new logging approach.
86-90: Version printing migrated to logger — LGTMUsing log.Println keeps output consistent with the rest of the CLI.
124-129: Flag cleanup for alias is correctRemoving either “--bypass-admin” or “-b” once encountered prevents false positives downstream. Breaking after removal avoids range/slice index pitfalls.
290-300: Auto-fix flow after update is solid
- Clear, actionable message to the user.
- Runs Backup(version, true) then CheckStates/InitSetting/Apply.
- Respects noRestart flag before Restart.
312-313: Backup now takes apply flag — LGTMPassing slices.Contains(commands, "apply") makes chaining “backup apply” work without separate branching.
337-341: API renames wired correctly
- SetDevTools → EnableDevTools
- SpotifyRestart() used consistently
346-348: Unknown command now fails via utils.Fatal — LGTMThis standardizes error handling and exit codes.
350-352: Post-command restart condition reads wellProperly avoids double-restart when “restart” is explicitly requested and respects noRestart.
363-490: Help text updates are clear and consistent with behavior
- Expanded explanations for refresh/watch flags.
- New upgrade|update command documented.
- Added -b/--bypass-admin flag.
- Copy is concise and aligns with the refactors.
515-530: HelpConfig wording tweaks — LGTM
- Clearer “spotify_launch_flags” description and link.
- “check_spicetify_update” copy fits current behavior.
src/preprocess/preprocess.go (5)
147-152: V8 snapshot error handling threshold looks reasonableDifferentiating behavior at >= 1.2.64 versus older versions is a pragmatic UX improvement. Messaging is concise.
208-221: Removal of patch reporter and simplified calls — LGTMDropping the report callback cleans up the API surface; calls to exposeAPIs_* and additionalPatches() are updated consistently.
Also applies to: 227-228
805-824: additionalPatches: signature and return path simplification — LGTMNo reporter, just apply the patches and return. Straightforward.
826-949: exposeAPIs_main: consolidated patches and notification helper — LGTMThe showNotification hook and other mappings are applied cleanly; no external reporter coupling anymore.
951-1024: exposeAPIs_vendor: vendor mappings refactored cleanlySignature trimmed; URI and other helpers exposed consistently. Looks good.
spicetify.go
Outdated
| @@ -68,10 +67,10 @@ func init() { | |||
|
|
|||
| for _, v := range flags { | |||
| switch v { | |||
| case "--bypass-admin": | |||
| case "-b", "--bypass-admin": | |||
There was a problem hiding this comment.
| case "-b", "--bypass-admin": | |
| case "--bypass-admin": |
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 (7)
src/utils/watcher.go (2)
56-66: Fix potential panic in WalkDir callback: handle the error arg before using info.When WalkDir supplies a non-nil err, info may be nil. Calling info.IsDir() without checking err can panic. Report the walk error and skip the entry.
- filepath.WalkDir(root, func(filePath string, info os.DirEntry, _ error) error { - if info.IsDir() { + filepath.WalkDir(root, func(filePath string, info os.DirEntry, err error) error { + if err != nil { + // Report the walk error and continue walking. + callbackEach(filePath, err) + return nil + } + if info == nil { + return nil + } + if info.IsDir() { return nil } curr, err := os.ReadFile(filePath) if err != nil { callbackEach(filePath, err) return nil }
97-105: Close response body and validate HTTP status to avoid resource leaks and spurious parsing.
res.Bodyis not closed and the status code isn’t checked. Add a defer close and bail on non-200 responses.res, err := http.Get("http://localhost:9222/json/list") if err != nil { return "" } + defer res.Body.Close() + if res.StatusCode != http.StatusOK { + return "" + } body, err := io.ReadAll(res.Body) if err != nil { return "" }src/preprocess/preprocess.go (3)
57-70: Close response body and check HTTP status when fetching CSS map
http.Getresponse body isn’t closed and status isn’t validated. Even though this executes once per run, leaking FDs is a correctness bug; also treat non-200 as error.Suggested fix (outside changed lines):
func readRemoteCssMap(tag string, cssTranslationMap *map[string]string) error { url := "https://raw.githubusercontent.com/spicetify/cli/" + tag + "/css-map.json" resp, err := http.Get(url) if err != nil { return err } defer resp.Body.Close() if resp.StatusCode != http.StatusOK { return fmt.Errorf("unexpected status fetching CSS map: %s", resp.Status) } if err := json.NewDecoder(resp.Body).Decode(cssTranslationMap); err != nil { utils.PrintWarning("Remote CSS map JSON malformed.") return err } return nil }
1070-1083: Close response body and validate HTTP status for GitHub releases request
res.Bodyis never closed and the status code isn’t checked. On some networks, GitHub can return 403/rate-limit without a body.Suggested fix (outside changed lines):
req, _ := http.NewRequest("GET", "https://api.github.com/repos/spicetify/cli/releases", nil) req.Header.Set("User-Agent", "spicetify-cli") // helps avoid 403 from GitHub API res, err := http.DefaultClient.Do(req) if err != nil { return "", err } defer res.Body.Close() if res.StatusCode != http.StatusOK { return "", fmt.Errorf("GitHub releases API returned %s", res.Status) } body, err := io.ReadAll(res.Body) if err != nil { return "", err }
279-293: filepath.Walk callback can panic on error pathsIn StartCSS, the Walk callback uses
info.Name()without guardingerr/info. Whenerr != nil,infocan be nil andinfo.Name()will panic.Suggested fix (outside changed lines):
filepath.Walk(appPath, func(path string, info os.FileInfo, err error) error { if err != nil || info == nil { return nil } if info.IsDir() { return nil } // temp so text won't be black ._. if strings.HasPrefix(info.Name(), "pip-mini-player") && strings.HasSuffix(info.Name(), ".css") { return nil } if filepath.Ext(info.Name()) == ".css" { utils.ModifyFile(path, func(content string) string { return colorVariableReplace(content) }) } return nil })spicetify.go (2)
56-64: Fix potential panic when parsing a lone “-” flag
v[1]is accessed without checkinglen(v) > 1. A single hyphen argument (“-”) would panic. Reorder the length checks.- if len(v) > 0 && v[0] == '-' { - if v[1] != '-' && len(v) > 2 { + if len(v) > 0 && v[0] == '-' { + if len(v) > 2 && v[1] != '-' { for _, char := range v[1:] { flags = append(flags, "-"+string(char)) } } else { flags = append(flags, v) }
69-110:-balias for--bypass-adminnot wired upYou remove
-blater (Lines 126-131) but don’t setbypassAdminCheckwhen-bis provided. Users supplying-bwill still be blocked by the admin check. Add the alias here.- case "--bypass-admin": + case "-b", "--bypass-admin": bypassAdminCheck = trueAlso update the help text to advertise
-b(see separate suggestion below).
♻️ Duplicate comments (3)
src/preprocess/preprocess.go (2)
93-106: Spinner lifecycle: end exactly once and guard Start() failuresRight now the spinner can call Warning() and then always Success(), which yields conflicting states. Also, Start()’s error is ignored — if the spinner can’t start (common on some terminals or due to the referenced pterm spinner bug), dereferencing it will panic.
Refactor to:
- Only call Warning() if the remote fetch fails, else Success().
- Guard spinner method calls behind a nil check if Start() failed.
Proposed diff:
- fetchSpinner, _ := utils.Spinner.Start("Fetch remote CSS map") + fetchSpinner, sErr := utils.Spinner.Start("Fetch remote CSS map") + if sErr != nil { + fetchSpinner = nil + utils.PrintWarning("Spinner unavailable, continuing without it.") + } tag, err := FetchLatestTagMatchingOrMain(version) if err != nil { - fetchSpinner.Warning() utils.PrintWarning(err.Error()) tag = version } - if readRemoteCssMap(tag, &cssTranslationMap) != nil { - fetchSpinner.Warning() - utils.PrintInfo("Using local CSS map instead") - readLocalCssMap(&cssTranslationMap) - } - fetchSpinner.Success() + if err := readRemoteCssMap(tag, &cssTranslationMap); err != nil { + if fetchSpinner != nil { + fetchSpinner.Warning() + } + utils.PrintInfo("Using local CSS map instead") + readLocalCssMap(&cssTranslationMap) + } else { + if fetchSpinner != nil { + fetchSpinner.Success() + } + }
180-185: Progressbar: check Start() error and guard Increment() to avoid panicsIf Start() fails,
barmay be nil; calling Increment() will panic. Guard creation and usage.Proposed diff:
- bar, _ := pterm.DefaultProgressbar. + bar, barErr := pterm.DefaultProgressbar. WithTotal(totalFiles). WithTitle("Patching files"). WithTitleStyle(pterm.NewStyle(pterm.Bold)). WithShowCount(true). Start() + if barErr != nil { + bar = nil + utils.PrintWarning("Progressbar unavailable, continuing without it.") + } @@ - bar.Increment() + if bar != nil { + bar.Increment() + }Also applies to: 272-272
spicetify.go (1)
118-123: Admin warning text: improved and consistentThe revised copy is clearer and follows the previous review suggestion.
🧹 Nitpick comments (13)
src/utils/watcher.go (3)
127-135: Optional: wrap errors with context for better diagnosability.Adding context (what failed, which URL) makes logs clearer and aligns with the PR’s logging work.
For example:
- On dial failure: “websocket dial failed: %w”
- On write failure: “reload command write failed: %w”
This would require importing fmt and returning fmt.Errorf-wrapped errors.
21-47: Optional: add cancellation support to watchers and consider fsnotify for scalability.Both Watch and WatchRecursive spin forever with sleeps. Consider:
- Accepting a context.Context to allow graceful shutdown.
- Using fsnotify to avoid full-tree rescans on every tick for large trees.
I can draft a minimal context-aware version or an fsnotify-based variant if useful.
Also applies to: 53-82
121-138: No nil-pointer risk; add explicit empty-URL error in SendReloadAll existing callers pass a non-nil pointer (e.g.
&debuggerURL), so guarding againstnilisn’t needed today. However, it’s still valuable to fail fast with a clear error when URL resolution yields an empty string.Apply this lighter refactor in src/utils/watcher.go:
• Add the
errorsimport
• Replace the initial length check with explicit URL resolution and an early error if it’s empty@@ -import ( +import ( + "errors" "bytes" "encoding/json" "io" @@ "net/http" "os" "path/filepath" "strings" "time" "golang.org/x/net/websocket" ) @@ func SendReload(debuggerURL *string) error { - if len(*debuggerURL) == 0 { - *debuggerURL = GetDebuggerPath() - } - - socket, err := websocket.Dial(*debuggerURL, "", "http://localhost/") + // Resolve debugger URL, error early if missing + url := *debuggerURL + if url == "" { + url = GetDebuggerPath() + *debuggerURL = url + } + if url == "" { + return errors.New("devtools websocket URL not found") + } + + socket, err := websocket.Dial(url, "", "http://localhost/") if err != nil { return err } defer socket.Close()src/preprocess/preprocess.go (1)
802-821: Function signature refactor: OK; minor typo in patch label
- The new
additionalPatches(input string) stringand return site are correct.- Nit: the label “GraphQL definitons (>=1.2.31)” has a typo (“definitons”). Consider correcting for consistency in logs.
src/cmd/color.go (5)
92-102: Minor: avoid mutating loop variable for display labelUsing a separate
namevariable keeps the loop variable immutable and improves readability.Apply this diff:
for _, k := range utils.BaseColorOrder { - colorString := colorSection.Key(k).String() + colorString := colorSection.Key(k).String() + name := k @@ - if len(colorString) == 0 { - colorString = utils.BaseColorList[k] - k += " (*)" - } + if len(colorString) == 0 { + colorString = utils.BaseColorList[k] + name = k + " (*)" + } @@ - data = append(data, []string{utils.Bold(k), colorPreview(color), color.Hex(), color.RGB()}) + data = append(data, []string{utils.Bold(name), colorPreview(color), color.Hex(), color.RGB()})
104-113: Deterministic ordering for non-base colors
colorSection.Keys()order isn’t guaranteed. Sorting the “extra” keys yields stable output and nicer diffs.Apply this diff:
-for _, v := range colorSection.Keys() { - k := v.Name() - - if len(utils.BaseColorList[k]) != 0 { - continue - } - - color := utils.ParseColor(v.String()) - data = append(data, []string{utils.Bold(k), colorPreview(color), color.Hex(), color.RGB()}) -} +extras := make([]string, 0, len(colorSection.Keys())) +extraVals := map[string]string{} +for _, v := range colorSection.Keys() { + k := v.Name() + if _, ok := utils.BaseColorList[k]; ok { + continue + } + extras = append(extras, k) + extraVals[k] = v.String() +} +sort.Strings(extras) +for _, k := range extras { + color := utils.ParseColor(extraVals[k]) + data = append(data, []string{utils.Bold(k), colorPreview(color), color.Hex(), color.RGB()}) +}Add missing import:
// add to imports if you adopt the change import ( "sort" )
107-109: Idiomatic map membership checkUse the comma-ok idiom instead of measuring string length from the map read.
Apply this diff:
- if len(utils.BaseColorList[k]) != 0 { + if _, ok := utils.BaseColorList[k]; ok { continue }
115-115: Handle table render errors
Render()returns an error. Log it to avoid silent failures.Apply this diff:
- pterm.DefaultTable.WithHasHeader().WithData(data).Render() + if err := pterm.DefaultTable.WithHasHeader().WithData(data).Render(); err != nil { + utils.PrintError("Failed to render color table: " + err.Error()) + }
125-127: Optional: guard truecolor preview for terminals without 24-bit supportRaw 24-bit ANSI escapes look great, but some environments don’t support them. Consider gating on COLORTERM to avoid printing control sequences in those terminals. If unsupported, leave the cell blank or use a simple symbol.
Apply this diff:
-func colorPreview(color utils.Color) string { - return "\x1B[48;2;" + color.TerminalRGB() + "m \033[0m" -} +func colorPreview(color utils.Color) string { + rgb := color.TerminalRGB() + if os.Getenv("COLORTERM") == "truecolor" || os.Getenv("COLORTERM") == "24bit" { + return "\x1B[48;2;" + rgb + "m \033[0m" + } + // Fallback: leave empty if truecolor is unavailable + return "" +}Add missing import:
// add to imports if you adopt the change import ( "os" )src/cmd/backup.go (3)
14-16: Document the newsilentbehavior in the function commentPublic API changed to include
silent. The doc comment doesn’t mention howsilentaffects behavior. Clarify for future callers.-// Backup stores original apps packages, extracts them and preprocesses -// extracted apps' assets +// Backup stores original app packages, extracts them and preprocesses extracted assets. +// If silent is true, the final readiness message is suppressed (useful when chaining with "apply"). func Backup(spicetifyVersion string, silent bool) {
28-28: Minor phrasing polish (optional)“There is an available backup” reads a bit awkwardly. Consider “A backup is already available” for readability.
64-82: Consider using a spinner for “Preprocessing” for consistencyOther steps (Backup/Extract/Clear/Restore) use spinners. Preprocessing currently uses PrintBold/PrintSuccess. For a unified UX, wrap preprocessing in a spinner too. If
preprocess.Startdoes not return an error, at least mark success after it completes and fail only on detectable preconditions.Example:
- utils.PrintBold("Preprocessing") + spinnerPre, _ := utils.Spinner.Start("Preprocessing") spotifyBasePath := spotifyPath if spotifyBasePath == "" - { - utils.PrintError("Spotify installation path not found. Cannot preprocess V8 snapshots") - } else { + { + // We can't preprocess; surface a spinner failure for consistency. + spinnerPre.Fail() + utils.PrintError("Spotify installation path not found. Cannot preprocess V8 snapshots") + } else { preprocess.Start( spicetifyVersion, spotifyBasePath, rawFolder, preprocess.Flag{ DisableSentry: preprocSection.Key("disable_sentry").MustBool(false), DisableLogging: preprocSection.Key("disable_ui_logging").MustBool(false), RemoveRTL: preprocSection.Key("remove_rtl_rule").MustBool(false), ExposeAPIs: preprocSection.Key("expose_apis").MustBool(false), SpotifyVer: utils.GetSpotifyVersion(prefsPath)}, ) - utils.PrintSuccess("Preprocessing completed") + spinnerPre.Success() }spicetify.go (1)
484-485: Advertise the-balias in help after wiring it upOnce
-bis supported in flag parsing, reflect it in the help text.--bypass-admin Bypass admin check. NOT RECOMMENDED +-b, --bypass-admin Bypass admin check. NOT RECOMMENDED
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
spicetify.go(12 hunks)src/cmd/backup.go(3 hunks)src/cmd/cmd.go(2 hunks)src/cmd/color.go(2 hunks)src/cmd/devtools.go(1 hunks)src/preprocess/preprocess.go(10 hunks)src/utils/watcher.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/cmd/cmd.go
- src/cmd/devtools.go
🧰 Additional context used
🧬 Code Graph Analysis (4)
src/preprocess/preprocess.go (2)
src/utils/print.go (7)
Spinner(48-59)Warning(24-29)PrintWarning(83-85)Error(30-35)PrintInfo(98-100)Success(18-23)Bold(63-65)src/utils/utils.go (1)
ReplaceOnce(175-188)
spicetify.go (8)
src/cmd/cmd.go (2)
GetConfigPath(199-201)InitSetting(117-196)src/utils/isAdmin/unix.go (1)
Check(8-13)src/utils/isAdmin/windows.go (1)
Check(10-31)src/utils/print.go (5)
PrintError(88-90)PrintInfo(98-100)PrintNote(78-80)Fatal(103-106)Bold(63-65)src/cmd/backup.go (1)
Backup(16-97)src/cmd/restart.go (1)
SpotifyRestart(58-66)src/cmd/devtools.go (1)
EnableDevTools(15-72)src/cmd/auto.go (1)
Auto(12-36)
src/cmd/color.go (2)
src/utils/color.go (4)
BaseColorOrder(40-59)BaseColorList(18-37)ParseColor(76-124)Color(67-71)src/utils/print.go (4)
Bold(63-65)PrintNote(78-80)PrintSuccess(93-95)PrintInfo(98-100)
src/cmd/backup.go (6)
src/utils/print.go (7)
PrintInfo(98-100)PrintWarning(83-85)Spinner(48-59)Fatal(103-106)Success(18-23)PrintBold(73-75)PrintSuccess(93-95)src/cmd/cmd.go (1)
ReadAnswer(243-268)src/status/backup/backup.go (1)
Get(32-61)src/status/spotify/spotify.go (1)
Get(35-62)src/backup/backup.go (2)
Start(11-13)Extract(16-34)src/utils/utils.go (2)
Copy(84-138)GetSpotifyVersion(256-269)
🔇 Additional comments (21)
src/utils/watcher.go (1)
129-129: Good: errors are now propagated instead of silently ignored.Returning the underlying dial/write errors makes callers observant and aligns with the PR’s logging/error-handling improvements.
Also applies to: 134-134
src/preprocess/preprocess.go (6)
148-150: Clarified V8 snapshot guidance looks goodSplitting the guidance into two targeted info messages improves UX.
205-217: API refactor adoption looks consistentCalls to exposeAPIs_main/vendor were updated to the new signatures (no reporter). Patch ordering for xpui-modules/xpui-snapshot/xpui.js and vendor~xpui.js remains sensible.
224-224: Consolidated additional patches: good moveSwitch to
additionalPatches(content)aligns with the reporter removal and keeps the patching pipeline tidy.
823-947: exposeAPIs_main refactor: LGTM
- Signature without reporter is handled.
- Renaming patch label to “showNotification” is fine and clearer.
- ContextMenuV2 and other exposure patches remain scoped as intended.
948-1021: exposeAPIs_vendor refactor: LGTM
- Signature update and return site updated consistently.
- URI handling for pre/post 1.2.4 flows still covered.
93-93: pterm version pin confirmedgo.mod includes
github.com/pterm/pterm v0.12.80, matching the intended downgrade. No further changes needed.src/cmd/color.go (3)
84-118: Nice UX upgrade: table-based color overview is clear and conciseThe refactor to pterm’s table with Name/Preview/Hex/RGB reads well and aligns with the new printing approach. The “(*)” default marker is a good touch.
120-123: Updated guidance to “spicetify refresh” is correctThis aligns with the current workflow. Good change.
7-7: pterm v0.12.80 Table API compatibility confirmedVerified that in v0.12.80:
DefaultTable.WithHasHeader()(no-arg) exists (variadic defaulting to true)WithData(pterm.TableData)is chainableTableDatais defined astype TableData [][]stringNo changes needed here.
src/cmd/backup.go (5)
18-21: Microsoft Store warning rewording looks goodClearer guidance and updated terminology. The new prompt “Continue backing up anyway?” is concise.
40-60: Spinner error handling is correct and consistentGood pattern: start spinner, mark Fail on error paths before exiting, and Success on positive paths. This improves UX and keeps output consistent.
94-96: Good: suppress final message when chaining with applyUsing
silentto suppress the final “ready to apply” note avoids redundant output when “apply” runs next. Nice touch.
141-153: Restore flow: spinner usage is solidConsistent start/fail/success pattern. Reads well and improves observability.
16-16: All Backup call sites use the new two-argument signature
No remaining single-argumentBackup(...)calls were found across the codebase.spicetify.go (6)
16-16: Importing pterm is appropriate for quiet-mode controlUsing
pterm.DisableOutput()when-q/--quietis set is the right way to silence pterm printers/spinners.
112-116: Quiet mode: pterm output disabledSetting
log.SetOutput(io.Discard)andpterm.DisableOutput()is sufficient to silence output. Good cohesion with the new pterm-based printers.
126-131: Follow-through: after adding “-b”, trimming it from flags is fineOnce the alias is recognized in the switch, removing it from
flagskeeps downstream logic clean.
294-301: Update path: good UX and safe sequencing
- Helpful note when up-to-date.
Backup(version, true)suppresses redundant “ready” message before Apply.- Conditional restart respects
--no-restart.
313-314: Passingsilentbased on presence of “apply” is a nice touchPrevents duplicate messaging when chaining
backupandapply.
341-342: Restart API usage looks goodCalls to
cmd.SpotifyRestart()centralize restart logic and keep main clean.Also applies to: 351-353
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
src/preprocess/preprocess.go (2)
93-106: Spinner may end twice; end it exactly once based on outcomeIf FetchLatestTagMatchingOrMain fails but readRemoteCssMap later succeeds, the spinner first calls Warning() and then Success(), producing conflicting statuses. End the spinner exactly once after attempting the remote fetch; keep the printed warning, but don’t end the spinner until the remote read result is known.
- fetchSpinner, _ := utils.Spinner.Start("Fetch remote CSS map") - tag, err := FetchLatestTagMatchingOrMain(version) - if err != nil { - fetchSpinner.Warning() - utils.PrintWarning(err.Error()) - tag = version - } - if readRemoteCssMap(tag, &cssTranslationMap) != nil { - fetchSpinner.Warning() - utils.PrintInfo("Using local CSS map instead") - readLocalCssMap(&cssTranslationMap) - } else { - fetchSpinner.Success() - } + fetchSpinner, _ := utils.Spinner.Start("Fetch remote CSS map") + tag, err := FetchLatestTagMatchingOrMain(version) + if err != nil { + // Keep spinner running; we still try the provided version. + utils.PrintWarning(err.Error()) + tag = version + } + if err := readRemoteCssMap(tag, &cssTranslationMap); err != nil { + fetchSpinner.Warning() + utils.PrintInfo("Using local CSS map instead") + readLocalCssMap(&cssTranslationMap) + } else { + fetchSpinner.Success() + }
149-151: Good severity split between Warning and InfoInitial failure is a warning and the follow-up guidance is informational. This aligns with the earlier request to keep the warning as-is.
src/cmd/backup.go (1)
113-131: Handle Mkdir errors (use MkdirAll) so failures don’t silently passThis repeats a prior review: Mkdir calls ignore errors, and failures would still show spinner.Success at Line 135. Use MkdirAll and fail the spinner on error to avoid misleading success.
func clearBackup() { spinner, _ := utils.Spinner.Start("Clear current backup") if err := os.RemoveAll(backupFolder); err != nil { spinner.Fail() utils.Fatal(err) } - os.Mkdir(backupFolder, 0700) + if err := os.MkdirAll(backupFolder, 0700); err != nil { + spinner.Fail() + utils.Fatal(err) + } if err := os.RemoveAll(rawFolder); err != nil { spinner.Fail() utils.Fatal(err) } - os.Mkdir(rawFolder, 0700) + if err := os.MkdirAll(rawFolder, 0700); err != nil { + spinner.Fail() + utils.Fatal(err) + } if err := os.RemoveAll(themedFolder); err != nil { spinner.Fail() utils.Fatal(err) } - os.Mkdir(themedFolder, 0700) + if err := os.MkdirAll(themedFolder, 0700); err != nil { + spinner.Fail() + utils.Fatal(err) + }
🧹 Nitpick comments (5)
src/preprocess/preprocess.go (3)
951-956: Duplicate “Spicetify.URI” patching path; remove one to avoid redundant workYou inject Spicetify.URI once via a direct utils.Replace and again via vendorPatches with the same regex. While the replacement is idempotent, it’s redundant work and complicates maintenance. Keep a single approach (recommend the vendorPatches entry for consistency).
- // URI - utils.Replace( - &input, - `,(\w+)\.prototype\.toAppType`, - func(submatches ...string) string { - return fmt.Sprintf(`,(globalThis.Spicetify.URI=%s)%s`, submatches[1], submatches[0]) - })Also applies to: 959-965
59-69: Close HTTP response body and check status codereadRemoteCssMap doesn’t Close() the response body and doesn’t validate HTTP status. This can leak connections and attempt to decode non-200 responses.
- cssMapResp, err := http.Get(cssMapURL) + cssMapResp, err := http.Get(cssMapURL) if err != nil { return err } else { - err := json.NewDecoder(cssMapResp.Body).Decode(cssTranslationMap) + defer cssMapResp.Body.Close() + if cssMapResp.StatusCode < 200 || cssMapResp.StatusCode >= 300 { + return fmt.Errorf("fetch css-map.json: unexpected status %d", cssMapResp.StatusCode) + } + err := json.NewDecoder(cssMapResp.Body).Decode(cssTranslationMap) if err != nil { utils.PrintWarning("Remote CSS map JSON malformed.") return err } }
1071-1079: Close GitHub API response bodyFetchLatestTagMatchingVersion reads res.Body without closing it. Close the body after reading to prevent leaks.
- res, err := http.Get("https://api.github.com/repos/spicetify/cli/releases") + res, err := http.Get("https://api.github.com/repos/spicetify/cli/releases") if err != nil { return "", err } - body, err := io.ReadAll(res.Body) + defer res.Body.Close() + body, err := io.ReadAll(res.Body) if err != nil { return "", err }src/cmd/backup.go (2)
14-16: Typo in comment: “surpressed” → “suppressed”Minor nit in the exported doc comment.
-// If silent is true, the final readiness message is surpressed (useful when chaining with "apply") +// If silent is true, the final readiness message is suppressed (useful when chaining with "apply")
91-96: Consider checking cfg.Write() errorscfg.Write() can fail (permissions, disk issues). Checking the returned error would make failures explicit in logs.
- cfg.Write() + if err := cfg.Write(); err != nil { + utils.Fatal(err) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
spicetify.go(12 hunks)src/cmd/backup.go(3 hunks)src/preprocess/preprocess.go(10 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
src/preprocess/preprocess.go (2)
src/utils/print.go (6)
Spinner(48-59)Warning(24-29)PrintWarning(83-85)Error(30-35)PrintInfo(98-100)Success(18-23)src/utils/utils.go (1)
ReplaceOnce(175-188)
spicetify.go (8)
src/cmd/cmd.go (2)
GetConfigPath(199-201)InitSetting(117-196)src/utils/isAdmin/unix.go (1)
Check(8-13)src/utils/isAdmin/windows.go (1)
Check(10-31)src/utils/print.go (5)
PrintError(88-90)PrintInfo(98-100)PrintNote(78-80)Fatal(103-106)Bold(63-65)src/cmd/backup.go (1)
Backup(16-97)src/cmd/restart.go (1)
SpotifyRestart(58-66)src/cmd/devtools.go (1)
EnableDevTools(15-72)src/cmd/auto.go (1)
Auto(12-36)
src/cmd/backup.go (6)
src/utils/print.go (7)
PrintInfo(98-100)PrintWarning(83-85)Spinner(48-59)Fatal(103-106)Success(18-23)PrintBold(73-75)PrintSuccess(93-95)src/cmd/cmd.go (1)
ReadAnswer(243-268)src/status/backup/backup.go (1)
Get(32-61)src/backup/backup.go (2)
Start(11-13)Extract(16-34)src/preprocess/preprocess.go (2)
Start(88-275)StartCSS(279-294)src/utils/utils.go (2)
Copy(84-138)GetSpotifyVersion(256-269)
🔇 Additional comments (23)
src/preprocess/preprocess.go (4)
206-218: API refactor LGTM: simplified patch calls without reporterDropping the reporter parameter and centralizing logging is consistent with the PR’s logging changes. Call sites for exposeAPIs_main/vendor and additionalPatches are updated correctly.
Also applies to: 225-225
813-817: Nit: Patch label typo fix LGTM“GraphQL definitions (>=1.2.31)” label is corrected and matches intent. No functional changes; regex and replacement look correct.
851-858: Patch rename and injection for showNotification LGTMRenaming to “showNotification” improves clarity. The injected API preserves existing behavior while exposing a simple Spicetify-facing method.
1039-1039: Good: propagate version parse errorsReturning the strconv.Atoi error avoids masking invalid inputs and simplifies upstream handling.
src/cmd/backup.go (8)
18-21: Microsoft Store warning + prompt flow looks goodClear user guidance and a sensible default (don’t proceed unless confirmed). In quiet mode, the prompt is auto-answered via ReadAnswer’s quiet path, which keeps CI/pipelines non-interactive.
40-45: Good: spinner failure on backup errorError handling is consistent with the new spinner-based UX.
48-51: Good: directory read errors are surfaced via spinner.Fail + FatalAligned with the new logging pattern.
55-60: Good: success/fail distinction for empty backup directoryClear exit when no .spa files were backed up.
64-64: LGTM: explicit “Preprocessing” phase markerNice UX touch to separate phases.
81-81: LGTM: completion notice for preprocess phaseMatches the spinner/prefix-based output overhaul.
94-96: Conditional final message based on silent is appropriatePrevents extra noise when chaining backup → apply.
141-153: Restore spinner/error handling looks consistentFailure paths mark the spinner as failed; success marks it as completed. Clean.
spicetify.go (11)
16-16: pterm import for output controlImport is used for DisableOutput in quiet mode. Good.
56-56: Flag parsing tweak is fineTreats any leading “-” as flag; multi-short-flag expansion retained. No regressions apparent.
74-75: Config path printing uses log, consistent with other outputsLooks good.
89-90: Version printing via log.Println is fineMatches the updated logging approach.
118-123: Admin-privilege messaging improved (addresses earlier feedback)The phrasing is clearer and matches the prior review suggestion.
294-301: Automatic restart path: confirm restart strategy is acceptableThis invokes cmd.SpotifyRestart() post-update when backupable. Given prior concerns about force-killing Spotify, please double-check SpotifyRestart’s implementation (SpotifyKill/Start) to ensure a graceful approach where possible.
If helpful, I can review src/cmd/restart.go to validate kill/start behavior against OS-specific best practices. Let me know.
313-314: Passing silent=true when “apply” is chained is a nice touchReduces redundant messaging in chained flows.
339-341: Renamed/centralized APIs look goodSwitch to EnableDevTools() and SpotifyRestart() improves consistency and discoverability.
347-349: Unified fatal for unknown commands is clearConcise guidance to -h is helpful.
351-353: Conditional auto-restart at end aligns with flagsRespects --no-restart and avoids duplicate restarts if “restart” is explicitly issued.
364-391: Help text updates read wellWording/formatting improvements and flag descriptions are clear. No issues spotted.
Also applies to: 470-485, 521-531, 464-465, 468-483
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/preprocess/preprocess.go (1)
1079-1084: Do not break on malformed tag; continue scanning releasesBreaking out of the loop on an unexpected tag format can cause you to miss later valid releases. Prefer continue.
- if len(ver) != 3 { - break + if len(ver) != 3 { + continue
♻️ Duplicate comments (4)
src/preprocess/preprocess.go (3)
88-101: Spinner lifecycle: end the spinner exactly once (avoid Warning() then Success())On a tag fetch error (Line 91) you call Warning(), but if reading the remote CSS map later succeeds you then call Success() (Lines 99-101). This produces conflicting spinner states.
Apply this diff to only finalize the spinner once based on remote fetch success:
- fetchSpinner, _ := utils.Spinner.Start("Fetch remote CSS map") - tag, err := FetchLatestTagMatchingOrMain(version) - if err != nil { - fetchSpinner.Warning() - utils.PrintWarning(err.Error()) - tag = version - } - if readRemoteCssMap(tag, &cssTranslationMap) != nil { - fetchSpinner.Warning() - utils.PrintInfo("Using local CSS map instead") - readLocalCssMap(&cssTranslationMap) - } else { - fetchSpinner.Success() - } + fetchSpinner, _ := utils.Spinner.Start("Fetch remote CSS map") + tag, err := FetchLatestTagMatchingOrMain(version) + if err != nil { + // Keep spinner pending; log warning but don't end it yet. + utils.PrintWarning(err.Error()) + tag = version + } + if err := readRemoteCssMap(tag, &cssTranslationMap); err != nil { + fetchSpinner.Warning() + utils.PrintInfo("Using local CSS map instead") + readLocalCssMap(&cssTranslationMap) + } else { + fetchSpinner.Success() + }Additionally, ensure readRemoteCssMap closes the response body and checks the HTTP status:
func readRemoteCssMap(tag string, cssTranslationMap *map[string]string) error { cssMapURL := "https://raw.githubusercontent.com/spicetify/cli/" + tag + "/css-map.json" resp, err := http.Get(cssMapURL) if err != nil { return err } defer resp.Body.Close() if resp.StatusCode != http.StatusOK { _, _ = io.Copy(io.Discard, resp.Body) return fmt.Errorf("fetch CSS map failed: %s", resp.Status) } if err := json.NewDecoder(resp.Body).Decode(cssTranslationMap); err != nil { utils.PrintWarning("Remote CSS map JSON malformed.") return err } return nil }
144-146: Message clarity: added context around V8 snapshot warning looks goodThe additional hints help users decide whether to ignore the warning. Keeping the original warning line intact aligns with prior feedback.
176-181: Guard progressbar Start() and Increment() to avoid panics on unsupported terminalsStart() error is ignored and Increment() is unconditional. If Start() fails, bar may be nil and Increment() will panic.
- bar, _ := pterm.DefaultProgressbar. + bar, barErr := pterm.DefaultProgressbar. WithTotal(totalFiles). WithTitle("Patching files"). WithTitleStyle(pterm.NewStyle(pterm.Bold)). WithShowCount(true). Start() + if barErr != nil { + bar = nil + utils.PrintWarning("Progressbar unavailable, continuing without it.") + } else { + defer bar.Stop() + } @@ - bar.Increment() + if bar != nil { + bar.Increment() + }Also applies to: 268-268
src/cmd/backup.go (1)
118-131: Check and handleMkdirerrors (useMkdirAll)Directory creation failures are currently silent; spinner still reports success even if
Mkdirfails (permissions, race, disk). PreferMkdirAlland handle errors.- os.Mkdir(backupFolder, 0700) + if err := os.MkdirAll(backupFolder, 0700); err != nil { + spinner.Fail() + utils.Fatal(err) + } - os.Mkdir(rawFolder, 0700) + if err := os.MkdirAll(rawFolder, 0700); err != nil { + spinner.Fail() + utils.Fatal(err) + } - os.Mkdir(themedFolder, 0700) + if err := os.MkdirAll(themedFolder, 0700); err != nil { + spinner.Fail() + utils.Fatal(err) + }Optional DRY-up: iterate over a slice of folders and apply remove+create in a loop.
🧹 Nitpick comments (4)
src/preprocess/preprocess.go (2)
1026-1026: splitVersion: propagate Atoi errors — and guard empty stringsPropagating Atoi errors is correct. For completeness, also guard against empty or malformed inputs to avoid slicing panics.
Suggested update near the top of splitVersion (outside the selected range):
func splitVersion(version string) ([3]int, error) { vstring := version if len(vstring) == 0 { return [3]int{}, errors.New("invalid version string") } if vstring[0] == 'v' { vstring = vstring[1:] } // ... rest unchanged }
1058-1066: GitHub releases fetch: check HTTP status before reading bodyA non-200 response should be handled explicitly to avoid JSON errors on HTML error pages.
res, err := http.Get("https://api.github.com/repos/spicetify/cli/releases") if err != nil { return "", err } - defer res.Body.Close() + defer res.Body.Close() + if res.StatusCode != http.StatusOK { + _, _ = io.Copy(io.Discard, res.Body) + return "", fmt.Errorf("GitHub API returned %s", res.Status) + } body, err := io.ReadAll(res.Body) if err != nil { return "", err }src/cmd/backup.go (2)
47-60: Gate success on presence of .spa files, not just “any files”
len(appList) > 0can produce false positives (e.g., leftover non-.spa files). Align this check withstatus/backup.Get, which counts files ending with “.spa”.Apply this diff to count “.spa” entries:
appList, err := os.ReadDir(backupFolder) if err != nil { spinner.Fail() utils.Fatal(err) } -totalApp := len(appList) -if totalApp > 0 { +spaCount := 0 +for _, f := range appList { + if !f.IsDir() && strings.HasSuffix(f.Name(), ".spa") { + spaCount++ + } +} +if spaCount > 0 { spinner.Success() } else { spinner.Fail() utils.PrintInfo("Reinstall Spotify and try again") os.Exit(1) }Also add this import in the import block at the top of the file:
import "strings"
64-64: Optional: make “Preprocessing” consistent with spinnersEvery other long-running step uses a spinner. Consider replacing
PrintBold("Preprocessing")and the finalPrintSuccess("Preprocessing completed")with a spinner start/success to keep the UX uniform.I can draft a small refactor if you want to go this route.
Also applies to: 81-81
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/cmd/backup.go(3 hunks)src/preprocess/preprocess.go(12 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/preprocess/preprocess.go (2)
src/utils/print.go (7)
Spinner(48-59)Warning(24-29)PrintWarning(83-85)Error(30-35)PrintInfo(98-100)Success(18-23)Bold(63-65)src/utils/utils.go (1)
ReplaceOnce(175-188)
src/cmd/backup.go (6)
src/utils/print.go (7)
PrintInfo(98-100)PrintWarning(83-85)Spinner(48-59)Fatal(103-106)Success(18-23)PrintBold(73-75)PrintSuccess(93-95)src/cmd/cmd.go (1)
ReadAnswer(243-268)src/status/backup/backup.go (1)
Get(32-61)src/backup/backup.go (2)
Start(11-13)Extract(16-34)src/utils/utils.go (2)
Copy(84-138)GetSpotifyVersion(256-269)src/cmd/apply.go (1)
CheckStates(183-211)
🔇 Additional comments (12)
src/preprocess/preprocess.go (8)
41-50: Good extraction: centralized patch application reduces duplicationapplyPatches consolidates Replace/ReplaceOnce logic cleanly and improves readability.
201-213: Expose APIs flow: call sites updated consistently with new helper signaturesSwitching to exposeAPIs_main/vendor without reporter args matches the refactored internals and keeps responsibilities local to preprocess.
220-221: Nice consolidation: additionalPatches() keeps JS patch concerns localizedRouting through additionalPatches streamlines the per-file JS patch pipeline.
798-817: GraphQL patches extracted into additionalPatches(): good separation of concernsEncapsulating the GraphQL definitions handling here reduces noise in Start() and keeps patch logic cohesive.
808-813: Verify the >=1.2.31 GraphQL regex against multiple buildsThe new pattern looks plausible but brittle. Please sanity-check against a few known xpui builds to avoid over/under-matching.
I can help craft a small validation harness to run this regex against captured snippets from recent xpui bundles, if you can provide samples.
819-845: Context menu provider injection: robust fallback approach looks goodThe multi-strategy extraction of menu/trigger/target (old and new shapes) with a sensible default keeps this patch resilient across versions.
848-853: showNotification integration improvedSwitching to feedbackType with ERROR/NOTICE mapping is clearer and matches the new label semantics.
944-1010: Vendor exposure set is coherent and version-awareGood coverage for URI, styled-components, Tippy, Flipper, and Snackbars, with the 1.2.4+ URI fallback logic gated by presence checks. This helps avoid duplicate injections.
src/cmd/backup.go (4)
14-16: Nice API and UX tweak:silentflag plus conditional final messageSignature change and the conditional success message are coherent with the spinner-first UX and chaining with apply.
Also applies to: 94-96
40-45: Good spinner-based error handling for backup startStarting a spinner and failing it before
utils.Fatal(err)keeps the UI consistent and prevents “phantom” running spinners.
18-21: Confirm quiet-mode default for MS Store prompt
ReadAnswer("Continue backing up anyway?", false, true)means that in quiet mode the prompt is auto-answered “yes” and the backup proceeds. Verify this is intentional given the partial support warning.If the intended behavior is to fail closed in quiet mode, flip the third argument to
false.
141-153: Restore flow spinner and error paths look solidClean fail paths and success signaling are consistent with the new logging approach.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/preprocess/preprocess.go (1)
52-65: Close response body and handle non-200 HTTP status in readRemoteCssMapThe response body isn’t closed and HTTP status isn’t checked; both are important for resource safety and clearer failure handling.
Apply this diff:
func readRemoteCssMap(tag string, cssTranslationMap *map[string]string) error { var cssMapURL string = "https://raw.githubusercontent.com/spicetify/cli/" + tag + "/css-map.json" cssMapResp, err := http.Get(cssMapURL) if err != nil { return err } else { + defer cssMapResp.Body.Close() + if cssMapResp.StatusCode != http.StatusOK { + return fmt.Errorf("failed to fetch remote CSS map: %s returned %d", cssMapURL, cssMapResp.StatusCode) + } err := json.NewDecoder(cssMapResp.Body).Decode(cssTranslationMap) if err != nil { utils.PrintWarning("Remote CSS map JSON malformed.") return err } } return nil }
♻️ Duplicate comments (3)
src/preprocess/preprocess.go (3)
88-101: Spinner lifecycle: avoid calling Warning() and then Success() on the same spinnerIf FetchLatestTagMatchingOrMain fails but readRemoteCssMap succeeds, the spinner will show Warning() and then Success(), producing conflicting statuses. End the spinner exactly once based on the final outcome of reading the remote CSS map.
Apply this diff:
- fetchSpinner, _ := utils.Spinner.Start("Fetch remote CSS map") - tag, err := FetchLatestTagMatchingOrMain(version) - if err != nil { - fetchSpinner.Warning() - utils.PrintWarning(err.Error()) - tag = version - } - if readRemoteCssMap(tag, &cssTranslationMap) != nil { - fetchSpinner.Warning() - utils.PrintInfo("Using local CSS map instead") - readLocalCssMap(&cssTranslationMap) - } else { - fetchSpinner.Success() - } + fetchSpinner, _ := utils.Spinner.Start("Fetch remote CSS map") + tag, err := FetchLatestTagMatchingOrMain(version) + if err != nil { + // Still attempt to fetch using the provided version. + utils.PrintWarning(err.Error()) + tag = version + } + if err := readRemoteCssMap(tag, &cssTranslationMap); err != nil { + fetchSpinner.Warning() + utils.PrintInfo("Using local CSS map instead") + readLocalCssMap(&cssTranslationMap) + } else { + fetchSpinner.Success() + }
144-146: Keep the warning; added contextual info — LGTMThe additional Info lines preserve the warning and improve guidance for users.
176-181: Guard pterm progressbar Start() and nil bar to avoid panicsStart()’s error is ignored and Increment() is unconditional. If Start() fails (e.g., unsupported terminal), bar may be nil and Increment() will panic. Guard creation and subsequent use.
Apply this diff:
- bar, _ := pterm.DefaultProgressbar. + bar, barErr := pterm.DefaultProgressbar. WithTotal(totalFiles). WithTitle("Patching files"). WithTitleStyle(pterm.NewStyle(pterm.Bold)). WithShowCount(true). Start() + if barErr != nil { + bar = nil + utils.PrintWarning("Progressbar unavailable, continuing without it.") + } @@ - bar.Increment() + if bar != nil { + bar.Increment() + }Also applies to: 268-269
🧹 Nitpick comments (1)
src/preprocess/preprocess.go (1)
242-244: Fix invalid CSS calc and stray semicolon in injected CSS block“+ var(..)” sits outside calc() and there’s a stray “};”. This likely produces invalid CSS.
Apply this diff:
- .main-gridContainer-fixedWidth{grid-template-columns: repeat(auto-fill, var(--column-width));width: calc((var(--column-count) - 1) * var(--grid-gap)) + var(--column-count) * var(--column-width));}.main-cardImage-imageWrapper{background-color: var(--card-color, #333);border-radius: 6px;-webkit-box-shadow: 0 8px 24px rgba(0, 0, 0, .5);box-shadow: 0 8px 24px rgba(0, 0, 0, .5);padding-bottom: 100%;position: relative;width:100%;}.main-cardImage-image,.main-card-imagePlaceholder{height: 100%;left: 0;position: absolute;top: 0;width: 100%};.main-content-view{height:100%;} + .main-gridContainer-fixedWidth{grid-template-columns: repeat(auto-fill, var(--column-width));width: calc((var(--column-count) - 1) * var(--grid-gap) + var(--column-count) * var(--column-width));}.main-cardImage-imageWrapper{background-color: var(--card-color, #333);border-radius: 6px;-webkit-box-shadow: 0 8px 24px rgba(0, 0, 0, .5);box-shadow: 0 8px 24px rgba(0, 0, 0, .5);padding-bottom: 100%;position: relative;width:100%;}.main-cardImage-image,.main-card-imagePlaceholder{height: 100%;left: 0;position: absolute;top: 0;width: 100%;}.main-content-view{height:100%;}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/preprocess/preprocess.go(12 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/preprocess/preprocess.go (2)
src/utils/print.go (7)
Spinner(48-59)Warning(24-29)PrintWarning(83-85)Error(30-35)PrintInfo(98-100)Success(18-23)Bold(63-65)src/utils/utils.go (1)
ReplaceOnce(175-188)
🔇 Additional comments (8)
src/preprocess/preprocess.go (8)
41-50: Simplified applyPatches signature — LGTMDropping the reporter param cleans up the API. Call sites look consistent with the new signature.
204-213: Expose APIs call sites updated — LGTMThe split between exposeAPIs_main/vendor and the conditional vendor injection for >= 1.2.57 reads correct. Keeping additionalPatches within the ExposeAPIs branch is consistent with intent.
Also applies to: 207-210, 221-221
798-817: additionalPatches refactor — LGTMFunction signature change and consolidated return via applyPatches are clean. The GraphQL patches are scoped and self-contained.
808-813: GraphQL label and pattern — LGTMLabel corrected, pattern looks tight. No issues spotted.
819-846: exposeAPIs_main changes — LGTMContext menu provider injection and the showNotification exposure look consistent with previous behavior, renamed to reflect usage.
Also applies to: 848-853, 941-942
944-952: exposeAPIs_vendor changes — LGTMEarly Spicetify.URI assignment via Replace plus vendor patches applied through applyPatches is a good cleanup.
Also applies to: 953-989, 1015-1016
1033-1033: splitVersion: return error on Atoi failure — LGTMGood improvement. Callers already propagate the error.
1070-1070: HTTP response body is properly closed — LGTMThe added defer prevents a descriptor leak in FetchLatestTagMatchingVersion.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/utils/config.go (2)
66-74: Default config creation ignores Write() error and prints success regardless.If SaveTo fails (e.g., permission issues), we still print success. Handle and report the error explicitly.
Apply:
- defaultConfig.Write() - PrintSuccess("Default config-xpui.ini generated") + if err := defaultConfig.Write(); err != nil { + PrintWarning(fmt.Sprintf("Failed to save default config: %s", err.Error())) + } else { + PrintSuccess("Default config-xpui.ini generated") + }Note: add
fmtto imports for this block.
91-94: “Config is updated” printed before save; SaveTo error is ignored.Print after a successful save and surface the failure to the user.
Apply:
- PrintSuccess("Config is updated") - cfg.SaveTo(configPath) + if err := cfg.SaveTo(configPath); err != nil { + PrintWarning(fmt.Sprintf("Failed to update config: %s", err.Error())) + } else { + PrintSuccess("Config is updated") + }Note: requires
fmtimport if not present in this file.
♻️ Duplicate comments (4)
src/cmd/backup.go (3)
116-146: Nice: handled Mkdir errors; consider MkdirAll for resilienceGreat fix handling Mkdir errors—this addresses earlier feedback. For extra resilience (missing parents, races), MkdirAll is generally safer in CLI paths.
- if err := os.Mkdir(backupFolder, 0700); err != nil { + if err := os.MkdirAll(backupFolder, 0700); err != nil { spinner.Fail() utils.Fatal(err) } ... - if err := os.Mkdir(rawFolder, 0700); err != nil { + if err := os.MkdirAll(rawFolder, 0700); err != nil { spinner.Fail() utils.Fatal(err) } ... - if err := os.Mkdir(themedFolder, 0700); err != nil { + if err := os.MkdirAll(themedFolder, 0700); err != nil { spinner.Fail() utils.Fatal(err) }Also applies to: 122-125, 132-135, 142-145
149-153: Spinner reports success even when cfg.Write fails during clear; fail and exit on errorThis reproduces the earlier issue: the spinner succeeds unconditionally even if the config write fails. Mark the spinner as failed and abort to avoid a false-positive success.
- if err := cfg.Write(); err != nil { - utils.PrintWarning(fmt.Sprintf("Failed to save config: %s", err.Error())) - } - spinner.Success() + if err := cfg.Write(); err != nil { + spinner.Fail() + utils.Fatal(fmt.Errorf("failed to save config: %w", err)) + } + spinner.Success()
94-99: Do not continue (or print success) if config write failsA failed cfg.Write() leaves backup metadata inconsistent. Emitting only a warning and then printing “Everything is ready…” is misleading and can break subsequent flows relying on the recorded version.
- if err := cfg.Write(); err != nil { - utils.PrintWarning(fmt.Sprintf("Failed to save config: %s", err.Error())) - } - if !silent { - utils.PrintSuccess("Everything is ready, you can start applying!") - } + if err := cfg.Write(); err != nil { + utils.Fatal(fmt.Errorf("failed to save config: %w", err)) + } + if !silent { + utils.PrintSuccess("Everything is ready, you can start applying!") + }src/cmd/update.go (1)
27-47: Fix doubled “v” in download path and wrong asset filenames; align asset naming.The URL uses
"/download/v"+tagNameand the asset embedstagNameverbatim. IftagNamealready starts with"v", this yields…/download/vvX…and asset names likespicetify-vX-…which don’t exist.Apply:
@@ - utils.PrintInfo("Latest release: " + tagName) - var assetURL string = "https://github.com/spicetify/cli/releases/download/v" + tagName + "/spicetify-" + tagName + "-" + runtime.GOOS + "-" - var location string = os.TempDir() + "/spicetify-" + tagName + utils.PrintInfo("Latest release: " + tagName) + tag := tagName // keep tag verbatim for the /download/<tag>/ segment + semver := strings.TrimPrefix(tagName, "v") // assets are named without leading "v" + var assetURL string = "https://github.com/spicetify/cli/releases/download/" + tag + "/spicetify-" + semver + "-" + runtime.GOOS + "-" + var location string = os.TempDir() + "/spicetify-" + semver @@ - if runtime.GOOS == "windows" { + if runtime.GOOS == "windows" { assetURL += ".zip" location += ".zip" } else { assetURL += ".tar.gz" location += ".tar.gz" }And add the missing import:
import ( "fmt" "io" "net/http" "os" "os/exec" "path/filepath" "runtime" + "strings"To confirm the expected asset naming for the latest tag, run:
#!/bin/bash tag="$(curl -sS https://api.github.com/repos/spicetify/cli/releases/latest | jq -r .tag_name)" echo "Latest tag: $tag" curl -sS https://api.github.com/repos/spicetify/cli/releases/tags/"$tag" | jq -r '.assets[].name,.assets[].browser_download_url'
🧹 Nitpick comments (13)
src/cmd/backup.go (4)
19-23: Treat Microsoft Store notice as a warning, not merely infoGiven this is a degraded-support path, consider elevating severity to a warning to match user expectations.
- utils.PrintInfo(`You are using the Microsoft Store version of Spotify, which is only partly supported. + utils.PrintWarning(`You are using the Microsoft Store version of Spotify, which is only partly supported. Don't use the Microsoft Store version with Spicetify unless you absolutely CANNOT install Spotify from its installer. Modded Spotify cannot be launched using original Shortcut/Start menu tile. To correctly launch modified Spotify, make a desktop shortcut that executes "spicetify auto". After that, you can change its icon, pin it to the start menu or put it in the startup folder.`)
65-83: Consider a spinner for preprocessing to stay consistent with the new logging UXYou currently PrintBold + PrintSuccess; using a spinner mirrors the rest of the file and can clearly show failure when spotifyBasePath is missing.
- utils.PrintBold("Preprocessing") + preprocSpinner, _ := utils.Spinner.Start("Preprocessing") spotifyBasePath := spotifyPath if spotifyBasePath == "" { - utils.PrintError("Spotify installation path not found. Cannot preprocess V8 snapshots") + preprocSpinner.Fail() + utils.PrintError("Spotify installation path not found. Cannot preprocess V8 snapshots") } else { preprocess.Start( spicetifyVersion, spotifyBasePath, rawFolder, preprocess.Flag{ DisableSentry: preprocSection.Key("disable_sentry").MustBool(false), DisableLogging: preprocSection.Key("disable_ui_logging").MustBool(false), RemoveRTL: preprocSection.Key("remove_rtl_rule").MustBool(false), ExposeAPIs: preprocSection.Key("expose_apis").MustBool(false), SpotifyVer: utils.GetSpotifyVersion(prefsPath)}, ) - utils.PrintSuccess("Preprocessing completed") + preprocSpinner.Success() }
85-88: Optional: add a spinner around asset copy for consistencyThis step can take noticeable time; a spinner keeps feedback uniform with the rest of the command.
- err = utils.Copy(rawFolder, themedFolder, true, []string{".html", ".js", ".css"}) - if err != nil { - utils.Fatal(err) - } + copySpinner, _ := utils.Spinner.Start("Copy preprocessed assets") + if err := utils.Copy(rawFolder, themedFolder, true, []string{".html", ".js", ".css"}); err != nil { + copySpinner.Fail() + utils.Fatal(err) + } + copySpinner.Success()
56-61: Message severity nit: consider Error over Info after a failed backupAfter spinner.Fail() and aborting, “Reinstall Spotify and try again” reads more like an error path than informational.
- spinner.Fail() - utils.PrintInfo("Reinstall Spotify and try again") + spinner.Fail() + utils.PrintError("Reinstall Spotify and try again")src/utils/config.go (4)
37-37: Confirm intent: default sidebar_config flipped to "0" (disabled).This changes out-of-the-box behavior. If intentional, please add a release note and confirm no features rely on "1" as the implicit default.
Would you like me to draft a short changelog entry highlighting this behavioral change?
107-115: Potential panic when defaults are missing for a new section.
configLayout[name]assumes defaults exist; if not, this panics. Defend against unknown sections.Apply:
- sec, err := c.content.GetSection(name) + sec, err := c.content.GetSection(name) if err != nil { sec, _ = c.content.NewSection(name) - for keyName, defaultValue := range configLayout[name] { - sec.NewKey(keyName, defaultValue) - } + if keyList, ok := configLayout[name]; ok { + for keyName, defaultValue := range keyList { + sec.NewKey(keyName, defaultValue) + } + } }
130-140: Global mutation of configLayout embeds environment-specific paths into defaults.
getDefaultConfigwrites detected paths back into the globalconfigLayout. On subsequent calls, “defaults” become machine-specific. Consider returning a fresh copy constructed from constants instead of mutating the shared map.If you want, I can refactor
configLayoutinto immutable defaults + a builder that overlays detections onto a local map before writing.
192-197: Leftover debug print.Replace
println("Err")with a proper log or remove it.Apply:
- println("Err") + utils.PrintWarning("Unknown color scheme '" + schemeName + "', falling back to first section")src/cmd/cmd.go (1)
252-260: Prompt formatting is clearer; handle input robustly and avoid recursion.Accept common inputs (“yes”/“no”, case/space-insensitive) and loop instead of recursion.
Apply:
- prompt := info - if defaultAnswer { - prompt += " [Y/n]: " - } else { - prompt += " [y/N]: " - } - reader := bufio.NewReader(os.Stdin) - fmt.Print(prompt) - text, _ := reader.ReadString('\n') - text = strings.Replace(text, "\r", "", 1) - text = strings.Replace(text, "\n", "", 1) - if len(text) == 0 { - return defaultAnswer - } else if text == "y" || text == "Y" { - return true - } else if text == "n" || text == "N" { - return false - } - return ReadAnswer(info, defaultAnswer, quietModeAnswer) + prompt := info + if defaultAnswer { + prompt += " [Y/n]: " + } else { + prompt += " [y/N]: " + } + reader := bufio.NewReader(os.Stdin) + for { + fmt.Print(prompt) + text, _ := reader.ReadString('\n') + text = strings.TrimSpace(strings.ToLower(text)) + switch text { + case "": + return defaultAnswer + case "y", "yes": + return true + case "n", "no": + return false + } + // re-prompt + }src/cmd/update.go (4)
23-25: Up-to-date path: message is fine — ensure version comparison uses the same “v” convention as tags.If currentVersion lacks the “v” prefix while tagName includes it, equality fails. Consider normalizing both sides once (e.g., strip optional leading “v” for comparison) to avoid false negatives.
Would you like a small helper like
normalizeSemver(v string) stringand use it both here and in CheckUpdate?
105-106: Avoid “vv…” in success message.
tagNametypically already includes “v”. Don’t add another.Apply:
- utils.PrintSuccess("Successfully updated Spicetify to v" + tagName) + utils.PrintSuccess("Successfully updated Spicetify to " + tagName)
92-104: Clean up downloaded archive after extraction.Remove the temporary archive to avoid accumulating large files in temp.
Apply:
utils.CheckExistAndDelete(exeOld) + // best-effort cleanup of downloaded archive + _ = os.Remove(location)
58-66: Optional: add HTTP timeout.Use an http.Client with a sane timeout to avoid hanging indefinitely on slow networks.
Example:
- resp2, err := http.Get(assetURL) + client := &http.Client{ Timeout: 60 * time.Second } + resp2, err := client.Get(assetURL)Note: add
timeto imports.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
src/cmd/apply.go(9 hunks)src/cmd/backup.go(4 hunks)src/cmd/cmd.go(4 hunks)src/cmd/restart.go(1 hunks)src/cmd/update.go(3 hunks)src/utils/config.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/cmd/restart.go
- src/cmd/apply.go
🧰 Additional context used
🧬 Code Graph Analysis (3)
src/cmd/backup.go (4)
src/utils/print.go (8)
PrintInfo(98-100)PrintWarning(83-85)Spinner(48-59)Fatal(103-106)Success(18-23)PrintBold(73-75)PrintSuccess(93-95)Error(30-35)src/cmd/cmd.go (1)
ReadAnswer(247-272)src/backup/backup.go (2)
Start(11-13)Extract(16-34)src/utils/utils.go (2)
Copy(84-138)GetSpotifyVersion(256-269)
src/cmd/cmd.go (1)
src/utils/print.go (2)
PrintWarning(83-85)PrintInfo(98-100)
src/cmd/update.go (1)
src/utils/print.go (5)
PrintSuccess(93-95)PrintInfo(98-100)Spinner(48-59)Fatal(103-106)Success(18-23)
🔇 Additional comments (6)
src/cmd/backup.go (3)
29-39: Backup-state gate and messaging look consistentThe flow “backup exists → check IsBackupable → clear or instruct to restore” is sound and messages are clear.
41-61: Good spinner/error handling around the backup phaseStarting “Backup app files”, failing the spinner on errors, and only marking success when files are present is a nice improvement in UX and correctness.
Also applies to: 48-52, 56-61
15-17: Verified Backup call sites updated – no further action neededI ran a repository-wide search for
Backup(invocations and confirmed that all calls now pass bothspicetifyVersionand the newsilentboolean. The only matches without commas were references to the internalclearBackup()helper, not outdatedBackupcalls.– spicetify.go:295 –
cmd.Backup(version, true)
– spicetify.go:313 –cmd.Backup(version, slices.Contains(commands, "apply"))
– src/cmd/auto.go:18 –Backup(spicetifyVersion, true)No single-argument
Backup(...)calls remain.src/cmd/cmd.go (2)
79-81: Nice: guarded config write with warnings.This prevents hard exits on non-critical save failures during auto-detect. Looks good.
98-100: Ditto: prefs_path write guard looks good.Consistent with the spotify_path write path.
src/cmd/update.go (1)
63-68: Good: response body is closed and HTTP status validated.Nice improvement; this prevents FD leaks and surfaces 4xx/5xx promptly.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/utils/config.go (1)
94-97: Handle and gate the rewrite “success” on SaveTo’s error.When updating an existing config, you announce success before attempting
SaveTo, and ignore its error. This can also mislead users if the write fails.- if needRewrite { - PrintSuccess("Config is updated") - cfg.SaveTo(configPath) - } + if needRewrite { + if err := cfg.SaveTo(configPath); err != nil { + PrintWarning(fmt.Sprintf("Failed to update config at %s: %v", configPath, err)) + } else { + PrintSuccess("Config is updated") + } + }
♻️ Duplicate comments (1)
src/utils/config.go (1)
52-55: Interface change toWrite() errorlooks good; verify all implementers and call sites.Good move to propagate errors. Please re-run a quick sweep to ensure no call site still drops the returned error. (Earlier review flagged two spots; this PR appears to fix them.)
#!/bin/bash # Verify: all Write() calls check returned error; and only utils.config implements it. echo "=== Calls to .Write() and their error handling context ===" rg -nP --type=go -C2 '\.\s*Write\s*\(' echo -e "\n=== Implementations of Write on type config ===" rg -nP --type=go -C2 'func\s*\(\s*c\s+config\s*\)\s*Write\(\)\s*error'
🧹 Nitpick comments (3)
src/utils/config.go (2)
4-4: Minor formatting nit: prefer %v over err.Error(), and be consistent.Use
%vwith the error value directly; it’s idiomatic and avoids callingError()manually. Applies to both default write warning and the rewrite suggestion above.- PrintWarning(fmt.Sprintf("Failed to save config: %s", err.Error())) + PrintWarning(fmt.Sprintf("Failed to save config: %v", err))Also applies to: 72-74, 94-97
38-41: Default change:AdditionalOptions.sidebar_confignow "0". Confirm intent and update docs.This flips the default behavior. If users rely on the previous default ("1"), this is a behavior change. Please confirm the rationale and ensure release notes/docs reflect it. Consider a migration note for existing configs that don’t explicitly set this key.
src/cmd/config.go (1)
34-36: Good: now handlingcfg.Write()errors. Small formatting nit.Solid improvement. For consistency and idiomatic Go, prefer
%vwith the error instead of callingerr.Error().- if err := cfg.Write(); err != nil { - utils.PrintWarning(fmt.Sprintf("Failed to save config: %s", err.Error())) - } + if err := cfg.Write(); err != nil { + utils.PrintWarning(fmt.Sprintf("Failed to save config: %v", err)) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/cmd/config.go(2 hunks)src/utils/config.go(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-20T23:57:23.778Z
Learnt from: rxri
PR: spicetify/cli#3493
File: src/utils/config.go:103-105
Timestamp: 2025-08-20T23:57:23.778Z
Learning: In the Spicetify CLI codebase, the spicetify config folder is guaranteed to exist before any config operations because it's created during package initialization via the GetSpicetifyFolder() function which uses a defer statement to call CheckExistAndCreate(). Therefore, adding MkdirAll to the config Write() method is unnecessary.
Applied to files:
src/utils/config.go
🧬 Code graph analysis (2)
src/cmd/config.go (1)
src/utils/print.go (1)
PrintWarning(83-85)
src/utils/config.go (1)
src/utils/print.go (1)
PrintWarning(83-85)
🔇 Additional comments (2)
src/utils/config.go (1)
106-108: Write method return type change is correct and minimal.Returning the underlying
SaveToerror is appropriate. No need to pre-create directories here—per team learning, the Spicetify config folder is guaranteed to exist viaGetSpicetifyFolder()’s deferredCheckExistAndCreate().src/cmd/config.go (1)
209-212: Breaking change: “-1” toggle support removed
Scanning the repository (including src/cmd/config.go and README.md) found no other references to the old “-1” convention. However, removing “-1” outright may break downstream scripts or user workflows.Please review and take one of the following actions:
- Verify downstream usage
• Ensure no automation or user scripts still rely on “-1” to invert toggles.- Update documentation & release notes
• Add a deprecation notice for “-1” in your CHANGELOG, README, and any CLI docs.- (Optional) Re-enable “-1” with a warning
This preserves the old behavior while warning users to migrate.diff --git a/src/cmd/config.go b/src/cmd/config.go index abcdef0..1234567 100644 --- a/src/cmd/config.go +++ b/src/cmd/config.go @@ -208,7 +208,7 @@ func loadConfigField(field string, value string) { if value != "0" && value != "1" { - unchangeWarning(field, `"`+value+`" is not valid value. Only "0" or "1" are valid.`) + unchangeWarning(field, `"`+value+`" is not valid value. Only "0", "1", or "-1" (deprecated) are valid.`) return }Location to update:
- src/cmd/config.go lines 208–212
src/utils/config.go
Outdated
| if err := defaultConfig.Write(); err != nil { | ||
| PrintWarning(fmt.Sprintf("Failed to save config: %s", err.Error())) | ||
| } | ||
| PrintSuccess("Default config-xpui.ini generated") | ||
| return defaultConfig |
There was a problem hiding this comment.
Fix success message on default write failure.
If defaultConfig.Write() fails, the code still prints “Default config-xpui.ini generated”, which is misleading. Only emit success when the write succeeds; otherwise keep the warning. Also include the target path to aid debugging.
- if err := defaultConfig.Write(); err != nil {
- PrintWarning(fmt.Sprintf("Failed to save config: %s", err.Error()))
- }
- PrintSuccess("Default config-xpui.ini generated")
+ if err := defaultConfig.Write(); err != nil {
+ PrintWarning(fmt.Sprintf("Failed to save config to %s: %v", configPath, err))
+ } else {
+ PrintSuccess("Default config-xpui.ini generated")
+ }📝 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.
| if err := defaultConfig.Write(); err != nil { | |
| PrintWarning(fmt.Sprintf("Failed to save config: %s", err.Error())) | |
| } | |
| PrintSuccess("Default config-xpui.ini generated") | |
| return defaultConfig | |
| if err := defaultConfig.Write(); err != nil { | |
| PrintWarning(fmt.Sprintf("Failed to save config to %s: %v", configPath, err)) | |
| } else { | |
| PrintSuccess("Default config-xpui.ini generated") | |
| } | |
| return defaultConfig |
🤖 Prompt for AI Agents
In src/utils/config.go around lines 72 to 76, the code prints a success message
even when defaultConfig.Write() fails; change the flow so that after calling
defaultConfig.Write() you check the returned error and if non-nil call
PrintWarning with a message that includes both the error and the target file
path, otherwise call PrintSuccess and include the target file path; ensure the
success message is only emitted on a nil error and preserve returning
defaultConfig.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/apply/apply.go (1)
277-280: Print this guidance once, include versions in the error, and fix “GitHub” capitalizationinsertCustomApp is invoked for both xpui-snapshot.js and xpui.js in AdditionalOptions, so this block can print twice. Wrap it in a sync.Once and include concrete Spicetify/Spotify versions to make bug reports actionable. Also capitalize “GitHub.” Optionally tailor the update hint based on flags.CheckSpicetifyUpdate.
Apply this diff within the shown lines:
- utils.PrintError("Spotify version mismatch with Spicetify. Please report it on our github repository.") - utils.PrintInfo("Spicetify might have been updated for this version already. Please run `spicetify update` to check for a new version.") - utils.PrintInfo("If one isn't available yet, please wait for an update to be released or downgrade Spotify to a supported version.") - return content + warnSpotifyMismatchOnce.Do(func() { + utils.PrintError(fmt.Sprintf( + "Spotify version mismatch with Spicetify (Spicetify %s, Spotify %s). Please report it on our GitHub repository.", + flags.SpicetifyVer, flags.SpotifyVer, + )) + if flags.CheckSpicetifyUpdate { + utils.PrintInfo("No Spicetify update found for this Spotify version yet.") + } else { + utils.PrintInfo("Spicetify might already support this version. Run `spicetify update` to check for a new version.") + } + utils.PrintInfo("If one isn't available yet, please wait for an update to be released or downgrade Spotify to a supported version.") + }) + return contentAdd these supporting changes outside the range:
// imports: add import "sync" // package scope: add var warnSpotifyMismatchOnce sync.Once
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/apply/apply.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/apply/apply.go (1)
src/utils/print.go (1)
PrintInfo(98-100)
src/cmd/restart.go
Outdated
| } | ||
|
|
||
| func SpotifyRestart(flags ...string) { | ||
| enableDevtools := settingSection.Key("always_enable_devtools").MustBool(false) |
There was a problem hiding this comment.
This should also work in SpotifyStart... Some commands start spotify for a reason and we want to enable devtools then too
|
|
||
| case "auto": | ||
| cmd.Auto(version) | ||
| cmd.EvalSpotifyRestart(true) |
|
|
||
| case "enable-devtools": | ||
| cmd.SetDevTools() | ||
| cmd.EvalSpotifyRestart(true) | ||
| cmd.EnableDevTools() |
| @@ -317,7 +319,6 @@ func main() { | |||
| cmd.CheckStates() | |||
| cmd.InitSetting() | |||
| cmd.Apply(version) | |||
| restartSpotify() | |||
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cmd/apply.go (1)
282-394: Spinner shows Success even if multiple writes fail; check os.WriteFile errors and adjust modeInside RefreshApps:
- Several os.WriteFile calls ignore errors; the spinner ends with Success regardless. This can ship incomplete/broken apps while claiming success.
- File mode 0700 is too restrictive and sets the execute bit for JSON/JS/CSS.
Fix by tracking failures and reporting them via the spinner. Also switch to 0644 for file writes.
Minimum changes within the changed lines:
- spinner, _ := utils.Spinner.Start("Refresh custom apps") + spinner, _ := utils.Spinner.Start("Refresh custom apps") + hadError := false + write := func(path string, data []byte) { + if err := os.WriteFile(path, data, 0644); err != nil { + utils.PrintError(err.Error()) + hadError = true + } + } @@ - spinner.Success() + if hadError { + spinner.Warning() + } else { + spinner.Success() + }Then replace the specific writes (outside this hunk) to use the helper:
// Lines 309–313 write(filepath.Join(appDestPath, "xpui", appName+".json"), manifestFileContent) // Lines 377–381 write(filepath.Join(appDestPath, "xpui", appName+".js"), []byte(jsTemplate)) // Lines 387–391 write(filepath.Join(appDestPath, "xpui", appName+".css"), cssFileContent)This keeps UX honest: Success only when everything succeeds, Warning on partial failures.
♻️ Duplicate comments (2)
src/cmd/apply.go (2)
151-164: Addressed prior feedback: write error is handled before SuccessGood fix: write errors for spicetify-config.json now mark the spinner Warning and log the error, avoiding false positives.
220-230: CopyFile error handling on theme.js is correct and user-friendlyNow marks spinner Fail and prints the error; avoids silent theming breakage. Nice.
🧹 Nitpick comments (9)
src/cmd/apply.go (5)
45-59: Deduplicate "copy folder with spinner" patternThe overwrite paths for themed/raw assets duplicate the same Start/Copy/Fail/Success flow. A tiny helper keeps this consistent and easier to maintain.
Apply this diff within the shown range:
- spinner, _ := utils.Spinner.Start("Overwrite themed assets") - if err := utils.Copy(themedFolder, appDestPath, true, nil); err != nil { - spinner.Fail() - utils.Fatal(err) - } - spinner.Success() + if err := copyFolderWithSpinner("Overwrite themed assets", themedFolder, appDestPath); err != nil { + utils.Fatal(err) + } @@ - spinner, _ := utils.Spinner.Start("Overwrite raw assets") - if err := utils.Copy(rawFolder, appDestPath, true, nil); err != nil { - spinner.Fail() - utils.Fatal(err) - } - spinner.Success() + if err := copyFolderWithSpinner("Overwrite raw assets", rawFolder, appDestPath); err != nil { + utils.Fatal(err) + }Add this helper in the same file (nearby private funcs):
func copyFolderWithSpinner(label, src, dst string) error { spinner, _ := utils.Spinner.Start(label) if err := utils.Copy(src, dst, true, nil); err != nil { spinner.Fail() return err } spinner.Success() return nil }
156-159: Use file mode 0644 for JSON instead of 07000700 sets execute bits and restricts read access unnecessarily for a JSON file. Prefer 0644 to be readable and non-executable across platforms.
- configJsonBytes, 0700); err != nil { + configJsonBytes, 0644); err != nil {
151-155: Include the marshal error in the warning for easier diagnosisThe warning “Cannot convert colors.ini to JSON” hides the actual error. Append
err.Error()to aid debugging.- utils.PrintWarning("Cannot convert colors.ini to JSON") + utils.PrintWarning("Cannot convert colors.ini to JSON: " + err.Error())
175-185: Reflect partial failures when refreshing extensions
pushExtensionslogs per-file errors but the top-level spinner always Success if there were any items. Consider tracking whether any copy failed and set Warning on partial failure (Success only when all succeeded).Sketch:
// Option A: make pushExtensions return (okCount, errCount int) ok, failed := pushExtensions("", list...) if failed > 0 && ok > 0 { spinner.Warning() } else if failed > 0 { spinner.Fail() } else { spinner.Success() }If changing signatures is undesirable, add an error counter in
pushExtensionsand expose it via a package-level or injected collector for this call site.
72-86: Spinner outcome mismatch for AdditionalOptionsA quick inspection confirms that
AdditionalOptions(insrc/apply/apply.go) and all of its modifier functions have no return values, so any internal errors can’t be detected by the caller. As a result, the spinner insrc/cmd/apply.gois unconditionally marked as a success, even if something goes wrong under the hood.You have two paths forward:
• Rename the spinner prompt in src/cmd/apply.go (around line 73) to something like
"Running additional modifications"
This wording implies an attempt rather than a guaranteed success.
• (Optional refactor) ChangeAdditionalOptionsand itsjsModifiers(in src/apply/apply.go) to return anerror, propagate that error back to the caller, and then callspinner.Success()orspinner.Fail()based on the result.src/cmd/cmd.go (4)
81-83: Make the warning more actionable by including the config path and using %v for the error.Including the path helps users fix permission/path issues faster;
%vavoids redundantError()calls.Apply:
- if err := cfg.Write(); err != nil { - utils.PrintWarning(fmt.Sprintf("Failed to save config: %s", err.Error())) - } + if err := cfg.Write(); err != nil { + utils.PrintWarning(fmt.Sprintf("Failed to save config (%s): %v", cfg.GetPath(), err)) + }
100-102: Same here: surface the config file path in the warning.Consistency with the earlier block (Line 81) and more helpful diagnostics.
Apply:
- if err := cfg.Write(); err != nil { - utils.PrintWarning(fmt.Sprintf("Failed to save config: %s", err.Error())) - } + if err := cfg.Write(); err != nil { + utils.PrintWarning(fmt.Sprintf("Failed to save config (%s): %v", cfg.GetPath(), err)) + }
81-83: Optional: avoid double writes when both paths are auto-detected.If both spotify_path and prefs_path are fixed in one run, you’ll write the file twice. You could set both keys, then persist once (or factor a tiny helper like persistConfigOrWarn(cfg) and call it once at the end).
Also applies to: 100-102
254-260: Prompt construction is clearer; consider aligning UX and input handling.
- The explicit “[Y/n]” vs “[y/N]” suffix is good. Using plain
fmt.Printalso avoids a prefixed newline, which fits interactive prompts.- Optional: to be more robust on CRLF/LF variations, trim newlines with
strings.TrimRight(text, "\r\n")instead of two Replace calls (Lines 264–265).- Optional: if you want full consistency with the PR’s pterm direction, consider pterm’s interactive confirm widget for TTYs, falling back to this prompt in non-TTY/quiet modes.
Also applies to: 262-262
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
CONTRIBUTING.md(1 hunks)spicetify.go(13 hunks)src/cmd/apply.go(9 hunks)src/cmd/cmd.go(4 hunks)src/cmd/patch.go(4 hunks)src/utils/config.go(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- CONTRIBUTING.md
🚧 Files skipped from review as they are similar to previous changes (3)
- src/cmd/patch.go
- src/utils/config.go
- spicetify.go
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-20T23:46:36.105Z
Learnt from: rxri
PR: spicetify/cli#3493
File: src/cmd/cmd.go:288-291
Timestamp: 2025-08-20T23:46:36.105Z
Learning: The FetchLatestTag() function in src/utils/vcs.go strips the "v" prefix from GitHub release tags using TagName[1:], so when code concatenates "v" + latestTag, it properly reconstructs the version format without creating double "v" prefixes.
Applied to files:
src/cmd/cmd.go
📚 Learning: 2025-08-20T23:57:23.806Z
Learnt from: rxri
PR: spicetify/cli#3493
File: src/utils/config.go:103-105
Timestamp: 2025-08-20T23:57:23.806Z
Learning: In the Spicetify CLI codebase, the spicetify config folder is guaranteed to exist before any config operations because it's created during package initialization via the GetSpicetifyFolder() function which uses a defer statement to call CheckExistAndCreate(). Therefore, adding MkdirAll to the config Write() method is unnecessary.
Applied to files:
src/cmd/apply.go
🔇 Additional comments (4)
src/cmd/apply.go (3)
32-42: Spinner + fatal around raw asset copy: solid error signalingFailure path marks the spinner as failed before exiting; success path is explicit. This avoids misleading UX. LGTM.
402-412: Symlink creation: clear spinner gating and error pathMarks Fail on error, returns early, and Success on completion. Looks good.
168-171: apply.UserAsset does not return an error; unconditional spinner.Success is appropriateI’ve verified that UserAsset is declared as
func UserAsset(appsFolderPath, themeFolder string) { // … }and does not return an error value, so there’s no failure path to surface to the spinner. No changes needed here.
src/cmd/cmd.go (1)
290-292: No need to normalize “v” prefixes—version is already unprefixed.The GitHub Actions build uses
run: echo "TAG=${GITHUB_REF#refs/*/v}" >> $GITHUB_ENV … go build -ldflags "-X main.version=${{ env.TAG }}"to inject
versionwithout the “v” prefix, andFetchLatestTag()likewise returns an unprefixed tag (per team learning). As a result,latestTagandversionare compared on the same footing, and thevis only added in the printed message. You can safely ignore the suggested trimming—no changes are needed here.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
spicetify.go (1)
288-290: Bug: re-invocation passes all commands as a single argument; split args to preserve semanticsUsing strings.Join collapses multiple commands into one token, breaking matching (e.g., “update apply” becomes a single unmatched command). Pass arguments as separate items; optionally include flags to fully restore the original invocation.
- cmd = exec.Command(ex, strings.Join(commands[:], " ")) + // Re-run original arguments after update. Preserve flags and commands. + args := append([]string{}, flags...) + args = append(args, commands...) + cmd = exec.Command(ex, args...)
♻️ Duplicate comments (2)
spicetify.go (2)
118-123: Clarity improvement to admin/root warning is goodThe updated wording is clear and action-oriented. Thanks for aligning with the earlier guidance.
112-116: Do not set os.Stdout = nil; this can panic on any fmt.Print; redirect to os.DevNull instead*Setting Stdout to nil can cause panics in fmt.Print* inside this process or dependencies. Redirect to os.DevNull and keep pterm output disabled.
Apply this diff:
if quiet { log.SetOutput(io.Discard) - os.Stdout = nil - pterm.DisableOutput() + // Avoid panics from fmt.Print* by redirecting to /dev/null + if f, err := os.OpenFile(os.DevNull, os.O_WRONLY, 0); err == nil { + os.Stdout = f + } + // Optionally also silence stderr: + // if f2, err := os.OpenFile(os.DevNull, os.O_WRONLY, 0); err == nil { os.Stderr = f2 } + pterm.DisableOutput() }
🧹 Nitpick comments (9)
src/cmd/restart.go (6)
12-33: Kill detection is locale-fragile and heavier than needed; prefer direct, graceful termination with best-effort fallbackThe current pre-checks rely on parsing command output (e.g., “No tasks are running”), which is locale-dependent and brittle. You can simplify and make this more reliable by:
- Attempting a graceful quit first (macOS via AppleScript).
- Then unconditionally issuing the kill command and ignoring “process not found” errors.
This also avoids the extra processes (tasklist/pgrep/ps|grep) and works across locales.
Apply this diff to streamline the logic:
func SpotifyKill() { switch runtime.GOOS { case "windows": - isRunning := exec.Command("tasklist", "/FI", "ImageName eq spotify.exe") - result, _ := isRunning.Output() - if !bytes.Contains(result, []byte("No tasks are running")) { - exec.Command("taskkill", "/F", "/IM", "spotify.exe").Run() - } + // Best-effort: kill if running; ignore error when process not found. + _ = exec.Command("taskkill", "/F", "/IM", "spotify.exe").Run() case "linux": - isRunning := exec.Command("pgrep", "spotify") - _, err := isRunning.Output() - if err == nil { - exec.Command("pkill", "spotify").Run() - } + // pkill returns non-zero if no process matched; safe to ignore. + _ = exec.Command("pkill", "spotify").Run() case "darwin": - isRunning := exec.Command("sh", "-c", "ps aux | grep 'Spotify' | grep -v grep") - _, err := isRunning.CombinedOutput() - if err == nil { - exec.Command("pkill", "Spotify").Run() - } + // Try graceful quit first, then fallback. + _ = exec.Command("osascript", "-e", `tell application "Spotify" to quit`).Run() + _ = exec.Command("pkill", "-x", "Spotify").Run() } }
35-39: Calling EnableDevTools() here can terminate the whole process; consider making it non-fatal or deferringEnableDevTools() exits the program if offline.bnk is not found. Having a “start” helper terminate the CLI is surprising and can break flows that only want to start Spotify. Consider making EnableDevTools return an error instead of os.Exit, logging the error here, and still proceeding to start Spotify, or only enabling when explicitly requested.
Would you like me to draft a non-fatal variant of EnableDevTools and the corresponding call-site change here?
41-45: Filter out empty spotify_launch_flags entries to avoid passing “” as an argumentStrings("|") can produce empty elements (e.g., trailing delimiter), which then get appended as empty args.
- launchFlag := settingSection.Key("spotify_launch_flags").Strings("|") - if len(launchFlag) > 0 { - flags = append(flags, launchFlag...) - } + rawFlags := settingSection.Key("spotify_launch_flags").Strings("|") + for _, f := range rawFlags { + if trimmed := strings.TrimSpace(f); trimmed != "" { + flags = append(flags, trimmed) + } + }
46-58: Surface start errors to aid troubleshooting (currently ignored)All exec.Command(...).Start() results are ignored. If the launch fails (path missing, permissions, etc.), users get no signal. At minimum, log failures; ideally return an error.
case "windows": if isAppX { ps, _ := exec.LookPath("powershell.exe") exe := filepath.Join(os.Getenv("LOCALAPPDATA"), "Microsoft", "WindowsApps", "Spotify.exe") cmd := `& "` + exe + `" --app-directory="` + appDestPath + `"` if len(flags) > 0 { cmd += " " + strings.Join(flags, " ") } - exec.Command(ps, "-NoProfile", "-NonInteractive", "-Command", cmd).Start() + if err := exec.Command(ps, "-NoProfile", "-NonInteractive", "-Command", cmd).Start(); err != nil { + utils.PrintError("Failed to start Spotify (AppX): " + err.Error()) + } } else { - exec.Command(filepath.Join(spotifyPath, "spotify.exe"), flags...).Start() + if err := exec.Command(filepath.Join(spotifyPath, "spotify.exe"), flags...).Start(); err != nil { + utils.PrintError("Failed to start Spotify: " + err.Error()) + } }Note: This uses utils.PrintError for consistency; add
github.com/spicetify/cli/src/utilsto imports if not already present.
60-64: macOS: consider quoting/robustness and reporting errorsopen -a path with --args is correct. Two small refinements:
- Use -n to ensure the app is launched (useful right after a kill).
- Log a failure if the launch fails.
- flags = append([]string{"-a", "/Applications/Spotify.app", "--args"}, flags...) - exec.Command("open", flags...).Start() + flags = append([]string{"-n", "-a", "/Applications/Spotify.app", "--args"}, flags...) + if err := exec.Command("open", flags...).Start(); err != nil { + utils.PrintError("Failed to start Spotify (macOS): " + err.Error()) + }
67-70: Optional: small backoff to avoid race between kill and start on some systemsOn a few systems/process trees, immediate restart after kill can race with cleanup. A short sleep or readiness check can help.
func SpotifyRestart(flags ...string) { SpotifyKill() + // Small backoff to avoid race with process teardown. + time.Sleep(300 * time.Millisecond) SpotifyStart(flags...) }Add to imports:
-import ( +import ( ... + "time" )spicetify.go (3)
269-302: Update flow looks good; one note on restart conditionNice touch on auto-fixing when backupable post-update. With the “enable-devtools” case now explicitly applying and starting (see prior comment), consider simplifying the final auto-start guard accordingly to avoid edge cases.
469-486: Help text: minor polishHyphenation/capitalization is consistent; consider ending “--bypass-admin” with a period for consistency with other descriptions.
522-526: Doc link for CLI flags is great; consider validating values when launchingSince you now accept pipe-delimited flags from config, you may want to warn on obviously malformed tokens (spaces without quoting, or empty items). See earlier suggestion in restart.go for filtering.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
spicetify.go(13 hunks)src/cmd/restart.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/cmd/restart.go (1)
src/cmd/devtools.go (1)
EnableDevTools(15-72)
spicetify.go (6)
src/cmd/cmd.go (1)
GetConfigPath(205-207)src/utils/isAdmin/unix.go (1)
Check(8-13)src/utils/isAdmin/windows.go (1)
Check(10-31)src/utils/print.go (5)
PrintError(88-90)PrintInfo(98-100)PrintNote(78-80)Fatal(103-106)Bold(63-65)src/cmd/backup.go (1)
Backup(17-100)src/cmd/restart.go (2)
SpotifyRestart(67-70)SpotifyStart(35-65)
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
spicetify.go (1)
54-67: Fix potential index-out-of-range panic when parsing flagsAccessing v[1] before checking length will panic for inputs like "-" or "--". Reorder the condition to check the length first. Optionally, treat a standalone "-" as invalid or positional input instead of a flag.
Apply this minimal, safe fix:
- if len(v) > 0 && v[0] == '-' { - if v[1] != '-' && len(v) > 2 { + if len(v) > 0 && v[0] == '-' { + if len(v) > 2 && v[1] != '-' { for _, char := range v[1:] { flags = append(flags, "-"+string(char)) } } else { flags = append(flags, v) } } else { commands = append(commands, v) }Optional robustness (treat a single "-" explicitly):
- } else { - flags = append(flags, v) - } + } else if v == "-" { + // Ignore or treat as positional; choose the behavior that fits your UX. + // utils.PrintWarning("Ignoring standalone '-' argument") + commands = append(commands, v) + } else { + flags = append(flags, v) + }
♻️ Duplicate comments (3)
spicetify.go (3)
292-301: Auto-fix flow restarts Spotify: confirm UX (may kill an active session)This path unconditionally calls SpotifyRestart(), which kills and relaunches Spotify. That can abruptly interrupt active playback or work. Consider only restarting if Spotify is currently running; otherwise, prefer SpotifyStart(). Alternatively, prompt or gate behind a flag.
Possible pattern:
// Pseudocode: if running -> restart; else -> start if isRunning := /* spotifystatus or a helper to detect Spotify process */; isRunning { if !noRestart { cmd.SpotifyRestart() } } else if !noRestart { cmd.SpotifyStart() }Would you like me to draft a small helper in status/spotify to detect the running process cross-platform?
338-339: enable-devtools now calls EnableDevTools(): LGTM (addresses prior concern)This ensures the command actually enables DevTools regardless of the config toggle; final auto-start will launch Spotify unless --no-restart is set.
112-116: Don’t set os.Stdout = nil; redirect to os.DevNull to avoid panics in dependenciesSetting os.Stdout to nil risks panics from any fmt.Print* or third-party writes. Redirect to os.DevNull; keep pterm output disabled.
Apply this change:
if quiet { log.SetOutput(io.Discard) - os.Stdout = nil - pterm.DisableOutput() + // Avoid panics from fmt.Print* by discarding stdout/stderr safely. + if f, err := os.OpenFile(os.DevNull, os.O_WRONLY|os.O_APPEND, 0); err == nil { + os.Stdout = f + os.Stderr = f + } + // Suppress pterm output as well. + pterm.DisableOutput() }
🧹 Nitpick comments (5)
spicetify.go (5)
341-342: Restart maps to SpotifyRestart(): verify kill semantics are desiredFunctionally correct, but see earlier note about avoiding unnecessary kills. If you want “soft start” behavior when Spotify isn’t running, consider switching to SpotifyStart() based on runtime status.
347-349: Tiny copy edit in unknown-command errorAdd the article “a”.
-Run "spicetify -h" for list of valid commands.)) +Run "spicetify -h" for a list of valid commands.))
352-354: Auto-start only when it makes sense (avoid starting after no-op chains like “backup” alone)Starting Spotify after any chain can be surprising. Gate auto-start to commands that modify/apply state or explicitly need a launch.
- if !noRestart && !slices.Contains(commands, "restart") { - cmd.SpotifyStart() - } + // Start only if the chain included operations that benefit from launching Spotify. + shouldStart := slices.Contains(commands, "apply") || + slices.Contains(commands, "refresh") || + slices.Contains(commands, "enable-devtools") + if !noRestart && !slices.Contains(commands, "restart") && shouldStart { + cmd.SpotifyStart() + }
391-392: Nit: help text grammar for spotify-updatesMinor clarity and capitalization fix.
-spotify-updates Blocks Spotify updates by patching spotify executable. - Accepts "block" or "unblock" as parameter. +spotify-updates Blocks Spotify updates by patching the Spotify executable. + Accepts "block" or "unblock" as the parameter.
571-573: Help “Patch” section could be clearerCurrent line is terse. Suggest a slightly more descriptive phrasing.
-Allows you to apply custom patches to Spotify.`) +Apply custom patches to Spotify. See the documentation for patch usage and examples.`)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
spicetify.go(13 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
spicetify.go (7)
src/cmd/cmd.go (1)
GetConfigPath(205-207)src/utils/isAdmin/unix.go (1)
Check(8-13)src/utils/isAdmin/windows.go (1)
Check(10-31)src/utils/print.go (5)
PrintError(88-90)PrintInfo(98-100)PrintNote(78-80)Fatal(103-106)Bold(63-65)src/cmd/backup.go (1)
Backup(17-100)src/cmd/restart.go (2)
SpotifyRestart(67-70)SpotifyStart(35-65)src/cmd/devtools.go (1)
EnableDevTools(15-72)
🔇 Additional comments (4)
spicetify.go (4)
74-76: --config now prints via log.Println: LGTMConsistent with the zero-prefix logger and colorable stdout. No issues.
88-91: --version prints raw version via log.Println: LGTMSimple and consistent with other outputs.
118-124: Admin/root guard messaging improvement: LGTMClearer, user-friendly, and correctly uses bare infinitive (“should NOT be run”). Good callout to --bypass-admin.
313-314: Backup silent flag derived from presence of "apply": LGTMMatches Backup(spicetifyVersion string, silent bool) semantics; avoids redundant “ready to apply” messaging when applying in the same chain.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
spicetify.go (1)
114-115: Setting os.Stdout to nil is dangerous and can cause runtime panicsAny
fmt.Print*calls or direct writes toos.Stdoutwill panic when it's set tonil. The relevant code snippets show there arefmt.Print*calls in the codebase (e.g.,src/utils/scanner.go:17andsrc/cmd/cmd.go:256).
🧹 Nitpick comments (1)
spicetify.go (1)
119-123: Grammar correction: "NOT" should be lowercaseThe warning message uses incorrect grammar. Modal verbs like "should" are followed by the bare infinitive form of the verb.
- utils.PrintError("Spicetify should NOT be run with administrator or root privileges") + utils.PrintError("Spicetify should not be run with administrator or root privileges")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
spicetify.go(13 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
spicetify.go (8)
src/cmd/cmd.go (2)
GetConfigPath(205-207)InitSetting(123-202)src/utils/isAdmin/unix.go (1)
Check(8-13)src/utils/isAdmin/windows.go (1)
Check(10-31)src/utils/print.go (5)
PrintError(88-90)PrintInfo(98-100)PrintNote(78-80)Fatal(103-106)Bold(63-65)src/cmd/backup.go (1)
Backup(17-100)src/cmd/restart.go (2)
SpotifyRestart(67-70)SpotifyStart(35-65)src/cmd/devtools.go (1)
EnableDevTools(15-72)src/cmd/auto.go (1)
Auto(12-36)
🔇 Additional comments (8)
spicetify.go (8)
295-295: Verify the updated Backup function signatureThe
cmd.Backupfunction is now called with two parameters:versionand a booleantrue. This aligns with the AI summary indicating that the Backup signature was updated to include anapplyparameter.
313-313: Consistent usage of updated Backup function signatureThe backup command correctly uses
slices.Contains(commands, "apply")to determine the second boolean parameter for the updatedcmd.Backupfunction signature.
338-338: Function renamed from SetDevTools to EnableDevToolsThe function call has been updated from
cmd.SetDevTools()tocmd.EnableDevTools()as indicated in the AI summary. This is consistent with the related changes insrc/cmd/devtools.go.
300-301: Replacement of inline restart logic with dedicated functionsThe code now uses
cmd.SpotifyRestart()instead of inline restart logic, which improves modularity and consistency. This aligns with the new public API methods mentioned in the AI summary.Also applies to: 341-341
352-354: Improved final startup logic with dedicated SpotifyStart functionThe final startup logic now uses
cmd.SpotifyStart()instead of inline logic, which is cleaner and more consistent with the other restart-related changes throughout the codebase.
74-74: Consistent migration from fmt.Println to log.PrintlnThe config path and version output have been migrated from
fmt.Printlntolog.Println, which is consistent with the removal of thefmtimport and the logging improvements mentioned in the PR objectives.Also applies to: 89-89
347-349: Improved error handling with utils.FatalThe error handling for unknown commands now uses
utils.Fatal(errors.New(...))instead of inline error handling, which provides consistent error reporting throughout the application.
16-16: Addition of pterm import for logging improvementsThe
ptermimport has been added to support the logging improvements mentioned in the PR objectives, replacing previous console output methods with pterm's prefixes and spinners.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
spicetify.go (1)
285-289: Bug: exec.Command receives a single joined argument; pass args variadically and avoid package shadowingstrings.Join(commands, " ") makes the entire command string a single argv element (e.g., "apply refresh" becomes one arg). Also, the local variable name “cmd” shadows the imported cmd package, reducing readability.
- cmd := exec.Command(ex, cmds...) - utils.CmdScanner(cmd) - - cmd = exec.Command(ex, strings.Join(commands[:], " ")) - utils.CmdScanner(cmd) + child := exec.Command(ex, cmds...) + utils.CmdScanner(child) + + // Pass commands as separate arguments + child = exec.Command(ex, commands...) + utils.CmdScanner(child)
♻️ Duplicate comments (4)
spicetify.go (4)
352-354: Avoid unconditional SpotifyRestart; only restart when a mutating command ranAlways calling SpotifyRestart can kill a running Spotify instance even after non-mutating commands (e.g., backup). Restrict restart to cases where it’s actually required (apply/refresh/restore/enable-devtools).
-if !noRestart && !slices.Contains(commands, "restart") { - cmd.SpotifyRestart() -} +if !noRestart && !slices.Contains(commands, "restart") { + // Restart only if state changed and reload is needed + if slices.Contains(commands, "apply") || + slices.Contains(commands, "refresh") || + slices.Contains(commands, "restore") || + slices.Contains(commands, "enable-devtools") { + cmd.SpotifyRestart() + } +}
119-123: Admin/root warning wording: good fix and clearer guidanceThe updated phrasing complies with the earlier suggestion (bare infinitive + strong warning) and adds actionable next steps. Looks good.
337-339: enable-devtools: correct to call EnableDevTools() explicitlyThis addresses the previous concern where starting Spotify alone wouldn’t enable DevTools unless the config toggle was set. Good change.
112-116: Quiet mode: don’t nil out Stdout; redirect to os.DevNull (and Stderr) to avoid panics and fully silence outputSetting os.Stdout = nil risks panics anywhere a fmt.Print* (or any library writing to Stdout) is called. Redirect to os.DevNull instead and keep pterm output disabled. Also redirect Stderr for true “no output”.
if quiet { log.SetOutput(io.Discard) - os.Stdout = nil - pterm.DisableOutput() + // Avoid panics from fmt.Print* by redirecting to /dev/null + if f, err := os.OpenFile(os.DevNull, os.O_WRONLY, 0); err == nil { + os.Stdout = f + os.Stderr = f + } + // Silence pterm printers too + pterm.DisableOutput() }
🧹 Nitpick comments (1)
spicetify.go (1)
56-63: Flag parsing edge cases: consider supporting “--” terminator and negative numeric argsTreating any token starting with “-” as a flag breaks cases like negative numeric values (if ever needed in config edits) and lacks a standard “--” to stop flag parsing. This is optional but improves UX and parity with common CLIs.
Would you like me to draft a minimal change that introduces an
afterDoubleDashtoggle and preserves current short-flag expansion?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
spicetify.go(13 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
spicetify.go (7)
src/cmd/cmd.go (1)
GetConfigPath(205-207)src/utils/isAdmin/unix.go (1)
Check(8-13)src/utils/isAdmin/windows.go (1)
Check(10-31)src/utils/print.go (5)
PrintError(88-90)PrintInfo(98-100)PrintNote(78-80)Fatal(103-106)Bold(63-65)src/cmd/backup.go (1)
Backup(17-100)src/cmd/restart.go (1)
SpotifyRestart(67-70)src/cmd/devtools.go (1)
EnableDevTools(15-72)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
spicetify.go (1)
114-116: Potential runtime panics when setting os.Stdout = nilThis is identical to a previous review concern. Setting
os.Stdout = nilcan causefmt.Print*calls to panic, which is unsafe since other packages may write to stdout.
🧹 Nitpick comments (1)
spicetify.go (1)
365-365: Minor wording improvement for consistencyThe help text uses "Start backup" which is less consistent with other command descriptions that use imperative mood.
Consider using imperative mood for consistency:
-backup Start backup and preprocessing of app files. +backup Back up and preprocess app files.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
spicetify.go(14 hunks)src/preprocess/preprocess.go(13 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/preprocess/preprocess.go
🧰 Additional context used
🧬 Code graph analysis (1)
spicetify.go (7)
src/cmd/cmd.go (1)
GetConfigPath(205-207)src/utils/isAdmin/windows.go (1)
Check(10-31)src/utils/isAdmin/unix.go (1)
Check(8-13)src/utils/print.go (5)
PrintError(88-90)PrintInfo(98-100)PrintNote(78-80)Fatal(103-106)Bold(63-65)src/cmd/backup.go (1)
Backup(17-100)src/cmd/restart.go (1)
SpotifyRestart(67-70)src/cmd/devtools.go (1)
EnableDevTools(15-72)
🔇 Additional comments (24)
spicetify.go (24)
16-16: LGTM!The pterm import is correctly added to support the new spinner and prefix functionality for improved logging.
56-63: LGTM!The simplified flag parsing logic correctly removes the special handling for
-1and treats all tokens starting with-as flags. The short-flag expansion for multi-character flags (e.g.,-abc→-a,-b,-c) is preserved.
74-74: LGTM!Using
log.Printlninstead offmt.Printlnis consistent with the logging approach throughout the codebase.
89-89: LGTM!Using
log.Printlnfor version output maintains consistency with other output statements.
119-123: LGTM!The admin privilege warning messages are well-structured with clear emphasis on "NOT" and appropriate risk communication. The addition of the bypass flag information is helpful.
294-302: LGTM!The update flow correctly uses the new API methods:
cmd.Backup(version, true)with the updated signaturecmd.SpotifyRestart()for the restart logic- Proper conditional restart handling with
noRestart
313-313: LGTM!The backup command correctly uses the new signature with
slices.Contains(commands, "apply")to determine the apply parameter.
338-338: LGTM!Using
cmd.EnableDevTools()directly ensures devtools are enabled regardless of the config setting, which is the expected behavior for the explicit command.
341-341: LGTM!Using
cmd.SpotifyRestart()centralizes the restart logic and improves maintainability.
347-349: LGTM!Using
utils.Fatal()for unknown commands provides consistent error handling and messaging.
352-354: LGTM!The auto-restart logic correctly excludes cases where restart was explicitly called and respects the
noRestartflag.
370-370: LGTM!The updated help text clearly explains the flag usage with the refresh command.
377-377: LGTM!The detailed keyboard shortcut information for devtools is helpful for users.
384-384: LGTM!The clearer explanation of the
-lflag functionality improves user understanding.
390-391: LGTM!The improved help text uses proper capitalization and imperative mood as previously discussed.
393-393: LGTM!The updated description better explains the path command functionality.
406-406: LGTM!The clearer heading for available flags and options improves readability.
463-463: LGTM!The improved description for config-dir command is clearer.
465-465: LGTM!The updated description covers both upgrade and update commands appropriately.
468-494: LGTM!The flag descriptions are well-structured with clear explanations of:
- Usage contexts (e.g., "Use with 'watch' or 'path'")
- Behavioral effects (e.g., "auto-reload Spotify when changes are made")
- Important warnings (e.g., "--bypass-admin" marked as "NOT RECOMMENDED")
517-517: LGTM!The corrected description for
inject_theme_jsis more accurate.
522-525: LGTM!The updated description for
spotify_launch_flagsprovides clearer guidance on format and includes a helpful reference link.
531-531: LGTM!The improved description for
check_spicetify_updateis more accurate.
569-572: LGTM!The addition of the "Patch" section with appropriate description completes the configuration documentation.
Hi! This PR includes some logging improvements and refactors, mainly:
colorto use pterm's tableRemoved some unused code
Changed how restarting of Spotify is handled
Due to a bug in pterm related to spinners (pterm/pterm#731), it had to be downgraded to 0.12.80.
Summary by CodeRabbit
New Features
Enhancements
Removed
Chores