refactor: consolidate version parsing and discover backup apps dynamically#3709
refactor: consolidate version parsing and discover backup apps dynamically#3709usvimal wants to merge 1 commit intospicetify:mainfrom
Conversation
…cally - Add utils.SpotifyVersion type with ParseSpotifyVersion() and AtLeast() to eliminate duplicated version parsing in preprocess.go and apply.go - Add utils/constants.go with app name constants (AppXPUI, AppLogin) - Make backup.Extract() discover .spa files via filepath.Glob instead of hardcoding ["xpui", "login"], so new Spotify apps are handled automatically without code changes
📝 WalkthroughWalkthroughIntroduces centralized Spotify version parsing utilities and refactors version parsing across multiple modules to use the new utility. Additionally, implements dynamic SPA file discovery in backup extraction, replacing hardcoded file lists. New constants define known application identifiers in a shared utilities package. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
since this is ai slop like other prs - I'm rejecting it. maybe if you would have done this manually since this is literally not a huge task I might have considered merging it |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
> [!CAUTION]
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)src/backup/backup.go (1)
18-32:⚠️ Potential issue | 🟡 MinorSilent success when no
.spafiles are foundIf
backupPathis empty, missing, or contains no.spafiles, Glob ignores file system errors such as I/O errors reading directories and returnsnil, nil, sospaFilesis an empty slice. Theforloop body never executes, yetspinner.Success("Extracted backup")fires unconditionally — the user sees a successful extraction even though nothing happened. This can mask a corrupt or incomplete backup.🐛 Proposed fix
func Extract(backupPath, extractPath string) { spinner, _ := utils.Spinner.Start("Extracting backup") spaFiles, _ := filepath.Glob(filepath.Join(backupPath, "*.spa")) + if len(spaFiles) == 0 { + spinner.Fail("No .spa files found in backup") + utils.Fatal(fmt.Errorf("backup at %s contains no .spa files", backupPath)) + } for _, spaFile := range spaFiles { appName := strings.TrimSuffix(filepath.Base(spaFile), ".spa") appExtractToFolder := filepath.Join(extractPath, appName) err := utils.Unzip(spaFile, appExtractToFolder) if err != nil { spinner.Fail("Failed to extract backup") utils.Fatal(err) } } spinner.Success("Extracted backup") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backup/backup.go` around lines 18 - 32, In Extract, handle the filepath.Glob error and the empty-result case so we don't report success when nothing was extracted: after calling filepath.Glob(filepath.Join(backupPath, "*.spa")) check the returned error and if non-nil call spinner.Fail(...) and utils.Fatal(err); then if len(spaFiles) == 0 call spinner.Fail("No .spa files found in backup") (or similar) and exit/fatalize instead of proceeding to spinner.Success; reference the Extract function, spaFiles, spinner and backupPath when making these checks.
🧹 Nitpick comments (3)
src/utils/version.go (2)
33-41:AtLeastis never called; callers retain semantically incorrect version comparisons
AtLeastis introduced to fix a real correctness gap, butapply.go(Line 55) andpreprocess.go(Lines 224, 231, 238) were not migrated to use it. The raw chained checks — e.g.,spotifyMajor >= 1 && spotifyMinor >= 2 && spotifyPatch >= 57— are wrong for any Spotify release that bumps minor past 2 (e.g.,1.3.0: minor3 >= 2✓ but patch0 >= 57✗ → whole condition false, even though1.3.0 > 1.2.57).AtLeasthandles this correctly; please migrate the callers.♻️ Example migration for apply.go Line 55 and preprocess.go Lines 224, 231, 238
In
src/apply/apply.go:- if spotifyMajor >= 1 && spotifyMinor >= 2 && spotifyPatch >= 57 { + if spotifyVer.AtLeast(1, 2, 57) {In
src/preprocess/preprocess.go:- if spotifyMajor >= 1 && spotifyMinor >= 2 && spotifyPatch >= 57 { + if spotifyVer.AtLeast(1, 2, 57) {- if spotifyMajor >= 1 && spotifyMinor >= 2 && (spotifyPatch >= 28 && spotifyPatch <= 57) { + if spotifyVer.AtLeast(1, 2, 28) && !spotifyVer.AtLeast(1, 2, 58) {- if spotifyMajor >= 1 && spotifyMinor >= 2 && spotifyPatch < 78 { + if !spotifyVer.AtLeast(1, 2, 78) {Once
spotifyMajor,spotifyMinor,spotifyPatchare no longer referenced, the unpacking assignments on the lines immediately followingParseSpotifyVersioncalls can also be removed, leaving justspotifyVer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/version.go` around lines 33 - 41, Replace the incorrect chained numeric comparisons with the SpotifyVersion.AtLeast method: after calling ParseSpotifyVersion keep the SpotifyVersion value (e.g., spotifyVer) and change conditions like "spotifyMajor >= 1 && spotifyMinor >= 2 && spotifyPatch >= 57" to "spotifyVer.AtLeast(1,2,57)"; update all callers (apply.go and preprocess.go branches referenced in the review) to use spotifyVer.AtLeast(...) and remove any now-unused unpacking of spotifyMajor/spotifyMinor/spotifyPatch returned by ParseSpotifyVersion so only spotifyVer is kept.
20-22:len(parts) > 0guard is always true
strings.Splitnever returns an empty slice — for any input (including"") it returns at least[""]. The check on Line 20 is therefore always true and can be removed.♻️ Simplified ParseSpotifyVersion
func ParseSpotifyVersion(ver string) SpotifyVersion { ver = strings.TrimPrefix(ver, "v") parts := strings.Split(ver, ".") v := SpotifyVersion{} - if len(parts) > 0 { - v.Major, _ = strconv.Atoi(parts[0]) - } + v.Major, _ = strconv.Atoi(parts[0]) if len(parts) > 1 { v.Minor, _ = strconv.Atoi(parts[1]) } if len(parts) > 2 { v.Patch, _ = strconv.Atoi(parts[2]) } return v }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/version.go` around lines 20 - 22, The `len(parts) > 0` guard in ParseSpotifyVersion is redundant because strings.Split always returns at least [""]; remove the if statement and directly assign v.Major using parts[0] (i.e., v.Major, _ = strconv.Atoi(parts[0])), keeping the existing error-ignoring behavior so behavior is unchanged; reference symbols: ParseSpotifyVersion, parts, v.Major, strconv.Atoi.src/utils/constants.go (1)
10-11: Remove unused constants and consider immutable API design for registry data
KnownApps,AppXPUI, andAppLoginare not referenced anywhere in the codebase and should be removed as dead code.Additionally, if a registry of known app identifiers is needed in the future, avoid exposing mutable exported slices. Instead, return a copy via a function or keep the slice unexported with a getter function to prevent accidental or malicious modification of global state.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/constants.go` around lines 10 - 11, Remove the dead exported symbols AppXPUI, AppLogin, and KnownApps from constants.go (they are unused); if you need a registry later, make the slice unexported (e.g., knownApps) and provide a getter function (e.g., func KnownApps() []string) that returns a copy of the slice to preserve immutability and prevent external mutation, ensuring all references use the getter instead of the raw slice.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/backup/backup.go`:
- Around line 18-32: In Extract, handle the filepath.Glob error and the
empty-result case so we don't report success when nothing was extracted: after
calling filepath.Glob(filepath.Join(backupPath, "*.spa")) check the returned
error and if non-nil call spinner.Fail(...) and utils.Fatal(err); then if
len(spaFiles) == 0 call spinner.Fail("No .spa files found in backup") (or
similar) and exit/fatalize instead of proceeding to spinner.Success; reference
the Extract function, spaFiles, spinner and backupPath when making these checks.
---
Nitpick comments:
In `@src/utils/constants.go`:
- Around line 10-11: Remove the dead exported symbols AppXPUI, AppLogin, and
KnownApps from constants.go (they are unused); if you need a registry later,
make the slice unexported (e.g., knownApps) and provide a getter function (e.g.,
func KnownApps() []string) that returns a copy of the slice to preserve
immutability and prevent external mutation, ensuring all references use the
getter instead of the raw slice.
In `@src/utils/version.go`:
- Around line 33-41: Replace the incorrect chained numeric comparisons with the
SpotifyVersion.AtLeast method: after calling ParseSpotifyVersion keep the
SpotifyVersion value (e.g., spotifyVer) and change conditions like "spotifyMajor
>= 1 && spotifyMinor >= 2 && spotifyPatch >= 57" to
"spotifyVer.AtLeast(1,2,57)"; update all callers (apply.go and preprocess.go
branches referenced in the review) to use spotifyVer.AtLeast(...) and remove any
now-unused unpacking of spotifyMajor/spotifyMinor/spotifyPatch returned by
ParseSpotifyVersion so only spotifyVer is kept.
- Around line 20-22: The `len(parts) > 0` guard in ParseSpotifyVersion is
redundant because strings.Split always returns at least [""]; remove the if
statement and directly assign v.Major using parts[0] (i.e., v.Major, _ =
strconv.Atoi(parts[0])), keeping the existing error-ignoring behavior so
behavior is unchanged; reference symbols: ParseSpotifyVersion, parts, v.Major,
strconv.Atoi.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/apply/apply.gosrc/backup/backup.gosrc/preprocess/preprocess.gosrc/utils/constants.gosrc/utils/version.go
2 participants
Summary
utils.SpotifyVersiontype withParseSpotifyVersion()andAtLeast()method to eliminate duplicated version parsing logic that was copy-pasted in bothpreprocess.goandapply.go.utils/constants.gowith app name constants (AppXPUI,AppLogin) andKnownAppsslice.backup.Extract()discover.spafiles dynamically viafilepath.Glob("*.spa")instead of hardcoding["xpui", "login"]. This means new Spotify apps are automatically extracted without code changes.Files Changed
src/utils/version.go— NewSpotifyVersiontypesrc/utils/constants.go— New app name constantssrc/apply/apply.go— UseParseSpotifyVersion, remove unusedstrconvsrc/preprocess/preprocess.go— UseParseSpotifyVersionsrc/backup/backup.go— Dynamic.spadiscovery viafilepath.GlobTest plan
spicetify applycorrectly applies version-gated patchesspicetify backup+spicetify restoreround-trips correctlyxpui.spaandlogin.spaSummary by CodeRabbit