feat(mister): add per-profile RetroAchievements config#1128
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughMiSTer profile data now includes ChangesMiSTer profile data
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant profileDataManager
participant prepareFileItem
participant copyProfileFile
participant MountLedger
profileDataManager->>prepareFileItem: prepare retroachievements.cfg
prepareFileItem->>copyProfileFile: copy live config into profile pool
copyProfileFile-->>profileDataManager: profile config path
profileDataManager->>MountLedger: record profile item ownership
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
pkg/platforms/mister/profiledata.go (1)
106-111: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFile-kind path handling is RetroAchievements-specific, not actually generic.
profileItemTargetandprepareFileItemboth hardcoderetroAchievementsConfigFilerather than deriving the filename fromitem(e.g.item.id). The newkindfield models a generic "file vs directory" dispatch, but the file branch only works correctly for exactly one item. If a secondprofileDataItemKindFileentry is ever added toprofileDataItems,profileItemTargetand the pool path inprepareFileItemwould both silently resolve to the RetroAchievements path instead of the new item's own file.Consider adding a
filename(or similar) field toprofileDataItemSpecand using it in both places instead of the literal constant.♻️ Sketch of a more generic dispatch
type profileDataItemSpec struct { id string label string kind profileDataItemKind + // filename is the config filename for kind == profileDataItemKindFile. + filename string }func profileItemTarget(root string, item profileDataItemSpec) string { if item.kind == profileDataItemKindFile { - return filepath.Join(misterconfig.SDRootDir, retroAchievementsConfigFile) + return filepath.Join(misterconfig.SDRootDir, item.filename) } return filepath.Join(root, item.id) }- plan.pool = filepath.Join(profileDir, retroAchievementsConfigFile) + plan.pool = filepath.Join(profileDir, item.filename)Also applies to: 263-299
🤖 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 `@pkg/platforms/mister/profiledata.go` around lines 106 - 111, Make file-item path resolution generic by adding a filename field to profileDataItemSpec and populating it for each profileDataItemKindFile entry. Update profileItemTarget and prepareFileItem to use that item-specific filename instead of retroAchievementsConfigFile, while preserving directory-item handling.pkg/platforms/mister/profiledata_backup.go (1)
177-183: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the backup-eligible item list from
profileDataItemsinstead of a hardcoded slice.This is now one more hardcoded
[]string{profileDataItemSaves, profileDataItemSavestates}literal (alongside three already inbackupPlan). The new typedprofileDataItemSpec/kindmodel exists precisely to avoid this kind of duplication — filteringprofileDataItemsbykind == profileDataItemKindDirhere would keep this list in sync automatically if a future directory-kind item is added.🤖 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 `@pkg/platforms/mister/profiledata_backup.go` around lines 177 - 183, The backup loop in the profile-data backup method should derive eligible items from profileDataItems rather than the hardcoded saves/savestates slice. Filter profileDataItems for entries whose kind is profileDataItemKindDir, then process those items while preserving the existing lookup, locking, and error behavior.pkg/platforms/mister/profiledata_test.go (1)
468-487: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInconsistent path construction: some literals in this block bypass
filepath.Join.Lines 474, 476, and 478 hardcode POSIX-style path strings (
"/media/usb0/saves","/zaparoo/profiles/"+kidA().ID+"/saves","/media/fat/saves"), while the rest of this same updated block (468, 482, 484-486, 487) correctly usesfilepath.Join.As per coding guidelines, "Use
filepath.Joinfor path construction everywhere, including test files — never hardcode POSIX-style paths like "/roms/snes/game.sfc" as string literals."♻️ Suggested fix
- saves := mountsAt(m.mounts, "/media/usb0/saves") + saves := mountsAt(m.mounts, filepath.Join(mediaRootPath, "usb0", "saves")) require.Len(t, saves, 1) - assert.Equal(t, "/zaparoo/profiles/"+kidA().ID+"/saves", saves[0].Root) + assert.Equal(t, "/"+filepath.Join("zaparoo", "profiles", kidA().ID, "saves"), saves[0].Root) assert.Equal(t, "/dev/sda1", saves[0].Source) - assert.Empty(t, mountsAt(m.mounts, "/media/fat/saves")) + assert.Empty(t, mountsAt(m.mounts, filepath.Join(misterconfig.SDRootDir, "saves")))🤖 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 `@pkg/platforms/mister/profiledata_test.go` around lines 468 - 487, Update the path assertions in the profile application test around mountsAt to construct all paths with filepath.Join: use the existing mediaRootPath and relevant storage/profile constants for the USB saves, profile saves, and FAT saves paths. Preserve the current expected mount roots and sources while removing the hardcoded POSIX-style path literals.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/platforms/mister/profiledata_backup.go`:
- Around line 177-183: The backup loop in the profile-data backup method should
derive eligible items from profileDataItems rather than the hardcoded
saves/savestates slice. Filter profileDataItems for entries whose kind is
profileDataItemKindDir, then process those items while preserving the existing
lookup, locking, and error behavior.
In `@pkg/platforms/mister/profiledata_test.go`:
- Around line 468-487: Update the path assertions in the profile application
test around mountsAt to construct all paths with filepath.Join: use the existing
mediaRootPath and relevant storage/profile constants for the USB saves, profile
saves, and FAT saves paths. Preserve the current expected mount roots and
sources while removing the hardcoded POSIX-style path literals.
In `@pkg/platforms/mister/profiledata.go`:
- Around line 106-111: Make file-item path resolution generic by adding a
filename field to profileDataItemSpec and populating it for each
profileDataItemKindFile entry. Update profileItemTarget and prepareFileItem to
use that item-specific filename instead of retroAchievementsConfigFile, while
preserving directory-item handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 61c4f3db-a97c-4bf4-9a78-0e3b75432591
📒 Files selected for processing (6)
docs/api/methods.mdpkg/platforms/mister/backup_test.gopkg/platforms/mister/profiledata.gopkg/platforms/mister/profiledata_backup.gopkg/platforms/mister/profiledata_test.gopkg/service/backup/mister_integration_test.go
1 participant
Summary
retroachievements.cfgfiles excluded from local and cloud backupsCloses #1102
Summary by CodeRabbit
New Features
Bug Fixes
Documentation