feat(doctor): add diagnostic command for install and config issues#3884
Closed
omnizs38 wants to merge 1 commit into
Closed
feat(doctor): add diagnostic command for install and config issues#3884omnizs38 wants to merge 1 commit into
omnizs38 wants to merge 1 commit into
Conversation
Adds "spicetify doctor" to check Spotify install/prefs paths, backup and mod state, theme asset availability, and configured extensions and custom apps, reporting anything broken instead of requiring manual troubleshooting.
📝 WalkthroughWalkthroughAdds a new "doctor" CLI command wired into the main dispatcher and help text, implementing diagnostics for Spotify installation, backups, theme assets, and extensions/custom apps. Refactors theme folder resolution in cmd.go into a reusable helper function used by the new diagnostics. ChangesDoctor Diagnostic Command
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant Main as spicetify.go
participant Doctor as cmd.Doctor()
participant FS as Filesystem
participant SpotifyStatus as spotifystatus/backupstatus
User->>Main: run "spicetify doctor"
Main->>Doctor: Doctor()
Doctor->>FS: check Apps folder / prefs file exist
Doctor->>SpotifyStatus: check Apps state / backup freshness
Doctor->>FS: resolve theme folder (findThemeFolder)
Doctor->>FS: check theme asset files (checkThemeAsset)
Doctor->>FS: resolve extension/custom app paths
Doctor->>User: print pass/fail summary
alt problems found
Doctor->>Main: os.Exit(1)
end
Compact metadata:
Poem: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/cmd/doctor.go (1)
17-119: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider splitting
Doctor()into smaller per-topic checks.The function mixes install/prefs/apps/backup/theme/extension checks in one ~100-line body. Given this is likely to grow with more diagnostics over time, extracting helpers (e.g.
checkInstallation,checkBackup,checkTheme,checkExtensions) sharing thepass/failclosures would improve readability without changing behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cmd/doctor.go` around lines 17 - 119, `Doctor()` is doing too many unrelated checks in one long body, so split it into focused helpers while keeping the same `pass`/`fail` behavior. Extract the install/prefs/apps logic, backup validation, theme validation, and extension/custom app validation into separate functions such as `checkInstallation`, `checkBackup`, `checkTheme`, and `checkExtensions`, and have `Doctor()` orchestrate them. Preserve the existing `problems` counting and shared closures so behavior stays unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/cmd/doctor.go`:
- Around line 64-75: The backup check in doctor should not call backupstatus.Get
unconditionally because it can terminate the process with log.Fatal when the
backup folder is missing. Update the doctor command logic around the
backupSection.Key("version") / backupstatus.Get path to first verify the backup
directory exists (or otherwise handle the missing-folder case) and then emit the
existing “No backup found” diagnostic instead of letting backupstatus.Get abort
the program. Keep the fix localized to the doctor backup check and the
backupstatus.Get call site.
- Around line 51-76: The backup diagnostics in doctor are unnecessarily gated by
the Apps-folder check, so `backupstatus.Get` never runs when `appsErr` is
non-nil. Update the logic in `doctor.go` so the backup validation tied to
`backupSection`, `prefsPathCfg`, and `backupFolder` runs independently of the
`spotifystatus.Get`/Apps-folder branch, while still keeping the `prefsErr ==
nil` guard if needed. Preserve the existing `fail`/`pass` messages and
structure, but decouple the backup check from the `appsErr == nil` condition.
- Around line 29-45: The Doctor checks in `main` currently treat empty
`spotify_path` and `prefs_path` as valid paths because `filepath.Join` and
`os.Stat` can resolve them relative to the current directory. Update the
`spotifyPathCfg` and `prefsPathCfg` handling to explicitly detect unset/empty
config values before calling `os.Stat`, and report them as missing/unconfigured
instead of “found.” Keep the fix localized to the existing Spotify and prefs
validation flow in `doctor.go`, using the current `pass`/`fail` messages and
`settingSection.Key(...).String()` values.
---
Nitpick comments:
In `@src/cmd/doctor.go`:
- Around line 17-119: `Doctor()` is doing too many unrelated checks in one long
body, so split it into focused helpers while keeping the same `pass`/`fail`
behavior. Extract the install/prefs/apps logic, backup validation, theme
validation, and extension/custom app validation into separate functions such as
`checkInstallation`, `checkBackup`, `checkTheme`, and `checkExtensions`, and
have `Doctor()` orchestrate them. Preserve the existing `problems` counting and
shared closures so behavior stays unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 77e3bf8e-19c5-4e07-a3b0-7e9c1b9503e1
📒 Files selected for processing (3)
spicetify.gosrc/cmd/cmd.gosrc/cmd/doctor.go
Comment on lines
+29
to
+45
| spotifyPathCfg := utils.ReplaceEnvVarsInString(settingSection.Key("spotify_path").String()) | ||
| prefsPathCfg := utils.ReplaceEnvVarsInString(settingSection.Key("prefs_path").String()) | ||
|
|
||
| spotifyAppsPath := filepath.Join(spotifyPathCfg, "Apps") | ||
| _, appsErr := os.Stat(spotifyAppsPath) | ||
| if appsErr == nil { | ||
| pass("Spotify installation found at \"" + spotifyPathCfg + "\"") | ||
| } else { | ||
| fail("Spotify installation not found at \"" + spotifyPathCfg + "\". Run \"spicetify config spotify_path <path>\" to fix it, or reinstall Spotify") | ||
| } | ||
|
|
||
| _, prefsErr := os.Stat(prefsPathCfg) | ||
| if prefsErr == nil { | ||
| pass("Spotify \"prefs\" file found at \"" + prefsPathCfg + "\"") | ||
| } else { | ||
| fail("Spotify \"prefs\" file not found at \"" + prefsPathCfg + "\". Launch Spotify at least once, then run \"spicetify config prefs_path <path>\" to fix it") | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Empty spotify_path/prefs_path config can produce a misleading "found" result.
If spotify_path or prefs_path is unset (empty string), filepath.Join("", "Apps") yields the relative path "Apps", and os.Stat will resolve it against the current working directory rather than reporting the config as unset. This could make Doctor falsely report the install/prefs as "found" when the config simply hasn't been set, undermining its diagnostic accuracy.
🩺 Guard against empty config values
+ if len(spotifyPathCfg) == 0 {
+ fail("\"spotify_path\" is not configured. Run \"spicetify config spotify_path <path>\"")
+ } else {
+ spotifyAppsPath := filepath.Join(spotifyPathCfg, "Apps")
+ _, appsErr := os.Stat(spotifyAppsPath)
+ ...
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/cmd/doctor.go` around lines 29 - 45, The Doctor checks in `main`
currently treat empty `spotify_path` and `prefs_path` as valid paths because
`filepath.Join` and `os.Stat` can resolve them relative to the current
directory. Update the `spotifyPathCfg` and `prefsPathCfg` handling to explicitly
detect unset/empty config values before calling `os.Stat`, and report them as
missing/unconfigured instead of “found.” Keep the fix localized to the existing
Spotify and prefs validation flow in `doctor.go`, using the current
`pass`/`fail` messages and `settingSection.Key(...).String()` values.
Comment on lines
+51
to
+76
| if appsErr == nil { | ||
| spotStat := spotifystatus.Get(spotifyAppsPath) | ||
| switch { | ||
| case spotStat.IsInvalid(): | ||
| fail("Spotify \"Apps\" folder is empty or invalid. Reinstall Spotify") | ||
| case spotStat.IsMixed(): | ||
| fail("Spotify has a mix of modified and stock files, likely from a recent Spotify update. Run \"spicetify backup apply\" to fix it") | ||
| case spotStat.IsStock(): | ||
| pass("Spotify is in stock (unmodified) state") | ||
| case spotStat.IsApplied(): | ||
| pass("Spotify has spicetify customizations applied") | ||
| } | ||
|
|
||
| if prefsErr == nil { | ||
| backupVersion := backupSection.Key("version").MustString("") | ||
| backStat := backupstatus.Get(prefsPathCfg, backupFolder, backupVersion) | ||
| switch { | ||
| case backStat.IsEmpty(): | ||
| fail("No backup found. Run \"spicetify backup\"") | ||
| case backStat.IsOutdated(): | ||
| fail("Backup version doesn't match installed Spotify version. Run \"spicetify backup apply\"") | ||
| default: | ||
| pass("Backup is up to date with installed Spotify version") | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Backup diagnostics unnecessarily coupled to Apps-folder detection.
The backup check (lines 64-75) only runs when both appsErr == nil and prefsErr == nil, but backupstatus.Get only depends on prefsPathCfg and backupFolder, not on the Apps folder. If spotify_path is misconfigured while prefs_path/backup are actually fine, Doctor will report the install-path problem but silently skip telling the user whether their backup is missing or outdated — reducing the diagnostic value precisely when the user needs more information to fix things.
🩺 Decouple the backup check from the Apps-folder check
- if appsErr == nil {
- spotStat := spotifystatus.Get(spotifyAppsPath)
- switch {
- case spotStat.IsInvalid():
- fail("Spotify \"Apps\" folder is empty or invalid. Reinstall Spotify")
- case spotStat.IsMixed():
- fail("Spotify has a mix of modified and stock files, likely from a recent Spotify update. Run \"spicetify backup apply\" to fix it")
- case spotStat.IsStock():
- pass("Spotify is in stock (unmodified) state")
- case spotStat.IsApplied():
- pass("Spotify has spicetify customizations applied")
- }
-
- if prefsErr == nil {
- backupVersion := backupSection.Key("version").MustString("")
- backStat := backupstatus.Get(prefsPathCfg, backupFolder, backupVersion)
- ...
- }
- }
+ if appsErr == nil {
+ spotStat := spotifystatus.Get(spotifyAppsPath)
+ switch {
+ case spotStat.IsInvalid():
+ fail("Spotify \"Apps\" folder is empty or invalid. Reinstall Spotify")
+ case spotStat.IsMixed():
+ fail("Spotify has a mix of modified and stock files, likely from a recent Spotify update. Run \"spicetify backup apply\" to fix it")
+ case spotStat.IsStock():
+ pass("Spotify is in stock (unmodified) state")
+ case spotStat.IsApplied():
+ pass("Spotify has spicetify customizations applied")
+ }
+ }
+
+ if prefsErr == nil {
+ backupVersion := backupSection.Key("version").MustString("")
+ // backup check here, independent of appsErr
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if appsErr == nil { | |
| spotStat := spotifystatus.Get(spotifyAppsPath) | |
| switch { | |
| case spotStat.IsInvalid(): | |
| fail("Spotify \"Apps\" folder is empty or invalid. Reinstall Spotify") | |
| case spotStat.IsMixed(): | |
| fail("Spotify has a mix of modified and stock files, likely from a recent Spotify update. Run \"spicetify backup apply\" to fix it") | |
| case spotStat.IsStock(): | |
| pass("Spotify is in stock (unmodified) state") | |
| case spotStat.IsApplied(): | |
| pass("Spotify has spicetify customizations applied") | |
| } | |
| if prefsErr == nil { | |
| backupVersion := backupSection.Key("version").MustString("") | |
| backStat := backupstatus.Get(prefsPathCfg, backupFolder, backupVersion) | |
| switch { | |
| case backStat.IsEmpty(): | |
| fail("No backup found. Run \"spicetify backup\"") | |
| case backStat.IsOutdated(): | |
| fail("Backup version doesn't match installed Spotify version. Run \"spicetify backup apply\"") | |
| default: | |
| pass("Backup is up to date with installed Spotify version") | |
| } | |
| } | |
| } | |
| if appsErr == nil { | |
| spotStat := spotifystatus.Get(spotifyAppsPath) | |
| switch { | |
| case spotStat.IsInvalid(): | |
| fail("Spotify \"Apps\" folder is empty or invalid. Reinstall Spotify") | |
| case spotStat.IsMixed(): | |
| fail("Spotify has a mix of modified and stock files, likely from a recent Spotify update. Run \"spicetify backup apply\" to fix it") | |
| case spotStat.IsStock(): | |
| pass("Spotify is in stock (unmodified) state") | |
| case spotStat.IsApplied(): | |
| pass("Spotify has spicetify customizations applied") | |
| } | |
| } | |
| if prefsErr == nil { | |
| backupVersion := backupSection.Key("version").MustString("") | |
| backStat := backupstatus.Get(prefsPathCfg, backupFolder, backupVersion) | |
| switch { | |
| case backStat.IsEmpty(): | |
| fail("No backup found. Run \"spicetify backup\"") | |
| case backStat.IsOutdated(): | |
| fail("Backup version doesn't match installed Spotify version. Run \"spicetify backup apply\"") | |
| default: | |
| pass("Backup is up to date with installed Spotify version") | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/cmd/doctor.go` around lines 51 - 76, The backup diagnostics in doctor are
unnecessarily gated by the Apps-folder check, so `backupstatus.Get` never runs
when `appsErr` is non-nil. Update the logic in `doctor.go` so the backup
validation tied to `backupSection`, `prefsPathCfg`, and `backupFolder` runs
independently of the `spotifystatus.Get`/Apps-folder branch, while still keeping
the `prefsErr == nil` guard if needed. Preserve the existing `fail`/`pass`
messages and structure, but decouple the backup check from the `appsErr == nil`
condition.
Comment on lines
+64
to
+75
| if prefsErr == nil { | ||
| backupVersion := backupSection.Key("version").MustString("") | ||
| backStat := backupstatus.Get(prefsPathCfg, backupFolder, backupVersion) | ||
| switch { | ||
| case backStat.IsEmpty(): | ||
| fail("No backup found. Run \"spicetify backup\"") | ||
| case backStat.IsOutdated(): | ||
| fail("Backup version doesn't match installed Spotify version. Run \"spicetify backup apply\"") | ||
| default: | ||
| pass("Backup is up to date with installed Spotify version") | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Backup check can crash the whole doctor command via log.Fatal.
backupstatus.Get (src/status/backup/backup.go) does os.ReadDir(backupPath) and calls log.Fatal(err) on failure. Since log.Fatal is log.Print + os.Exit(1) with no recovery, if backupFolder doesn't exist yet (very plausible on a first doctor run before the user has ever run spicetify backup), this call aborts the entire program with a raw OS error instead of Doctor's intended graceful "no backup found" diagnostic. This defeats the purpose of a diagnostic tool that's supposed to report problems, not crash on them.
🩺 Guard against a missing backup folder before calling backupstatus.Get
if prefsErr == nil {
backupVersion := backupSection.Key("version").MustString("")
- backStat := backupstatus.Get(prefsPathCfg, backupFolder, backupVersion)
- switch {
- case backStat.IsEmpty():
- fail("No backup found. Run \"spicetify backup\"")
- case backStat.IsOutdated():
- fail("Backup version doesn't match installed Spotify version. Run \"spicetify backup apply\"")
- default:
- pass("Backup is up to date with installed Spotify version")
- }
+ if _, err := os.Stat(backupFolder); err != nil {
+ fail("No backup found. Run \"spicetify backup\"")
+ } else {
+ backStat := backupstatus.Get(prefsPathCfg, backupFolder, backupVersion)
+ switch {
+ case backStat.IsEmpty():
+ fail("No backup found. Run \"spicetify backup\"")
+ case backStat.IsOutdated():
+ fail("Backup version doesn't match installed Spotify version. Run \"spicetify backup apply\"")
+ default:
+ pass("Backup is up to date with installed Spotify version")
+ }
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if prefsErr == nil { | |
| backupVersion := backupSection.Key("version").MustString("") | |
| backStat := backupstatus.Get(prefsPathCfg, backupFolder, backupVersion) | |
| switch { | |
| case backStat.IsEmpty(): | |
| fail("No backup found. Run \"spicetify backup\"") | |
| case backStat.IsOutdated(): | |
| fail("Backup version doesn't match installed Spotify version. Run \"spicetify backup apply\"") | |
| default: | |
| pass("Backup is up to date with installed Spotify version") | |
| } | |
| } | |
| if prefsErr == nil { | |
| backupVersion := backupSection.Key("version").MustString("") | |
| if _, err := os.Stat(backupFolder); err != nil { | |
| fail("No backup found. Run \"spicetify backup\"") | |
| } else { | |
| backStat := backupstatus.Get(prefsPathCfg, backupFolder, backupVersion) | |
| switch { | |
| case backStat.IsEmpty(): | |
| fail("No backup found. Run \"spicetify backup\"") | |
| case backStat.IsOutdated(): | |
| fail("Backup version doesn't match installed Spotify version. Run \"spicetify backup apply\"") | |
| default: | |
| pass("Backup is up to date with installed Spotify version") | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/cmd/doctor.go` around lines 64 - 75, The backup check in doctor should
not call backupstatus.Get unconditionally because it can terminate the process
with log.Fatal when the backup folder is missing. Update the doctor command
logic around the backupSection.Key("version") / backupstatus.Get path to first
verify the backup directory exists (or otherwise handle the missing-folder case)
and then emit the existing “No backup found” diagnostic instead of letting
backupstatus.Get abort the program. Keep the fix localized to the doctor backup
check and the backupstatus.Get call site.
Member
|
None of these are helpful to us, when we help someone, nor to the user actually. All of these appear when you run commands so like eh? Decided not to merge it. Thanks for contribution though! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds "spicetify doctor" to check Spotify install/prefs paths, backup and mod state, theme asset availability, and configured extensions and custom apps, reporting anything broken instead of requiring manual troubleshooting.
Summary by CodeRabbit
New Features
doctorcommand to run diagnostics on your Spotify setup.Bug Fixes