fix(mister): launch AmigaVision from active install#847
Merged
Conversation
📝 WalkthroughWalkthroughThis PR implements dynamic AmigaVision install path discovery for the Mister platform. The Amiga hook now detects the correct install by checking for boot images across configured root directories, optionally verifying game listing containment, and prioritizing paths ending with ChangesAmigaVision Install Discovery
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/platforms/mister/cores/cores_test.go (1)
34-57: ⚡ Quick winAdd branch tests for new Amiga hook paths.
This test validates the main path, but the new hook logic also introduced additional branches (e.g.,
demos.txtand alternate boot image handling). Please add focused cases so all new branches are covered.As per coding guidelines, "Write tests for all new code — see TESTING.md and pkg/testing/README.md".
Also applies to: 59-72
🤖 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/cores/cores_test.go` around lines 34 - 57, Add unit tests to cover the new branches in the Amiga hook logic: create additional test cases alongside TestHookAmiga_WritesBootFileToActiveInstall that exercise (1) the demos.txt branch (call hookAmiga with a path ending in "listings/demos.txt/<Name>") and (2) the alternate boot image handling (create installs where the boot file is not "ags_boot" or is located/named differently and assert correct copy/move behavior and absence in stale paths). Use the same helpers used in the existing test (e.g., writeAmigaVisionTestInstall) to create stale and valid installs, reuse config.NewConfig setup with IndexRoot entries, call hookAmiga(cfg, nil, <path>) for each scenario, and assert the expected boot file contents/locations and that stale paths no longer contain the boot image; add separate focused test functions (e.g., TestHookAmiga_HandlesDemosList and TestHookAmiga_AlternateBootImage) to keep branches isolated.pkg/platforms/mister/cores/hooks.go (1)
200-207: ⚡ Quick winAvoid duplicated Amiga boot-image detection logic across layers.
Line 200 and Line 209 duplicate behavior that also exists in
pkg/platforms/mister/platform.go(hasAmigaVisionImage/ preferred-path rules). Centralizing this into one shared helper reduces drift risk between scanner and hook decisions.Also applies to: 209-234
🤖 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/cores/hooks.go` around lines 200 - 207, The hasAmigaVisionBootImage function duplicates logic already present as hasAmigaVisionImage/preferred-path rules in pkg/platforms/mister/platform.go; refactor by creating a single shared helper (e.g., ExportedHasAmigaVisionImage or hasAmigaImageShared) in the mister package or a new helper file and update both hasAmigaVisionBootImage and the scanner code to call that shared helper, removing the duplicated checks around "AmigaVision.hdf"/"MegaAGS.hdf" so both scanner and hooks use the same implementation.
🤖 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 `@pkg/platforms/mister/cores/hooks.go`:
- Around line 248-254: The scanner loop that searches for gameName (using
bufio.NewScanner, scanner.Scan(), scanner.Text()) currently ignores
scanner.Err() and can silently return false on I/O errors; after the for
scanner.Scan() loop, check scanner.Err() and handle it (return the error or log
it and propagate) instead of unconditionally returning false so calling code can
distinguish "not found" from a read failure; update the function that performs
this file listing check to surface or log the scanner error and only return
false when no error occurred and the name wasn't found.
---
Nitpick comments:
In `@pkg/platforms/mister/cores/cores_test.go`:
- Around line 34-57: Add unit tests to cover the new branches in the Amiga hook
logic: create additional test cases alongside
TestHookAmiga_WritesBootFileToActiveInstall that exercise (1) the demos.txt
branch (call hookAmiga with a path ending in "listings/demos.txt/<Name>") and
(2) the alternate boot image handling (create installs where the boot file is
not "ags_boot" or is located/named differently and assert correct copy/move
behavior and absence in stale paths). Use the same helpers used in the existing
test (e.g., writeAmigaVisionTestInstall) to create stale and valid installs,
reuse config.NewConfig setup with IndexRoot entries, call hookAmiga(cfg, nil,
<path>) for each scenario, and assert the expected boot file contents/locations
and that stale paths no longer contain the boot image; add separate focused test
functions (e.g., TestHookAmiga_HandlesDemosList and
TestHookAmiga_AlternateBootImage) to keep branches isolated.
In `@pkg/platforms/mister/cores/hooks.go`:
- Around line 200-207: The hasAmigaVisionBootImage function duplicates logic
already present as hasAmigaVisionImage/preferred-path rules in
pkg/platforms/mister/platform.go; refactor by creating a single shared helper
(e.g., ExportedHasAmigaVisionImage or hasAmigaImageShared) in the mister package
or a new helper file and update both hasAmigaVisionBootImage and the scanner
code to call that shared helper, removing the duplicated checks around
"AmigaVision.hdf"/"MegaAGS.hdf" so both scanner and hooks use the same
implementation.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4501ef3d-1d05-4988-b2de-3b0e718df0f0
📒 Files selected for processing (4)
pkg/platforms/mister/cores/cores_test.gopkg/platforms/mister/cores/hooks.gopkg/platforms/mister/custom_scanners_test.gopkg/platforms/mister/platform.go
Comment on lines
+248
to
+254
| scanner := bufio.NewScanner(f) | ||
| for scanner.Scan() { | ||
| if scanner.Text() == gameName { | ||
| return true | ||
| } | ||
| } | ||
| return false |
There was a problem hiding this comment.
Handle scanner read errors in listing checks.
On Line 248, bufio.Scanner errors are ignored; this can silently return false on I/O failure and mis-select the install path.
💡 Proposed fix
scanner := bufio.NewScanner(f)
for scanner.Scan() {
if scanner.Text() == gameName {
return true
}
}
+if err := scanner.Err(); err != nil {
+ log.Warn().Err(err).Str("listing_path", listingPath).Msg("failed reading AmigaVision listing")
+}
return false🤖 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/cores/hooks.go` around lines 248 - 254, The scanner loop
that searches for gameName (using bufio.NewScanner, scanner.Scan(),
scanner.Text()) currently ignores scanner.Err() and can silently return false on
I/O errors; after the for scanner.Scan() loop, check scanner.Err() and handle it
(return the error or log it and propagate) instead of unconditionally returning
false so calling code can distinguish "not found" from a read failure; update
the function that performs this file listing check to surface or log the scanner
error and only return false when no error occurred and the name wasn't found.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Validation
Summary by CodeRabbit
New Features
Tests