feat: add RetroArch launchers for Linux and SteamOS#1066
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 (7)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughThis PR adds ZapOS support with a shared RetroArch launcher framework, runtime launcher availability metadata, coordinated blocking-process lifecycle handling, Linux and SteamOS integration, build and documentation wiring, and lint and test cleanup. ChangesZapOS and RetroArch platform integration
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant LauncherCache
participant Launcher
participant DoLaunch
participant LinuxBase
participant RetroArch
LauncherCache->>Launcher: Evaluate Availability
LauncherCache->>DoLaunch: Select available launcher
DoLaunch->>RetroArch: Launch blocking process
DoLaunch->>LinuxBase: Track process
LinuxBase->>RetroArch: Wait or custom Kill
LinuxBase->>DoLaunch: Clear matching active media
Possibly related PRs
🚥 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.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
pkg/platforms/launch.go (1)
137-155: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAvailability/nil-launch checks run after the currently active launcher is already stopped.
StopActiveLauncher(StopForPreemption)executes before theLauncher.Launch == niland newLauncher.Availabilitychecks. If the target launcher is unavailable (e.g., missing RetroArch core/executable), the current media is preempted/stopped anyway, and the launch then fails, leaving nothing active. SinceAvailabilityexists specifically as a pre-flight check, it should be validated before any side effects (stopping the currently running launcher) occur.🐛 Proposed fix: validate before preempting
- // Stop any currently running launcher before starting new one - // This ensures tracked processes (like videos) are stopped even when - // FireAndForget launches (like MGL files) start. UNLESS the new launcher - // uses a running instance (e.g., Kodi), in which case the platform's - // shouldKeepRunningInstance logic will handle stopping if needed. - if slot == mediaslot.Primary && params.Launcher.UsesRunningInstance == "" { - if stopErr := params.Platform.StopActiveLauncher(StopForPreemption); stopErr != nil { - log.Debug().Err(stopErr).Msg("no active launcher to stop or error stopping") - } - } - if params.Launcher.Launch == nil { return fmt.Errorf("launcher %q has no launch function configured", params.Launcher.ID) } if params.Launcher.Availability != nil { if err := params.Launcher.Availability(params.Config); err != nil { return fmt.Errorf("launcher %q is unavailable: %w", params.Launcher.ID, err) } } + + // Stop any currently running launcher before starting new one + // This ensures tracked processes (like videos) are stopped even when + // FireAndForget launches (like MGL files) start. UNLESS the new launcher + // uses a running instance (e.g., Kodi), in which case the platform's + // shouldKeepRunningInstance logic will handle stopping if needed. + if slot == mediaslot.Primary && params.Launcher.UsesRunningInstance == "" { + if stopErr := params.Platform.StopActiveLauncher(StopForPreemption); stopErr != nil { + log.Debug().Err(stopErr).Msg("no active launcher to stop or error stopping") + } + }🤖 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/launch.go` around lines 137 - 155, Move the Launcher.Launch nil check and Launcher.Availability validation before the StopActiveLauncher(StopForPreemption) call in the launch flow. Ensure unavailable or misconfigured launchers return their existing errors without stopping the active launcher, while preserving the current preemption behavior for validated launchers.pkg/helpers/launcher_cache.go (1)
49-68: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
extralaunchers never get theirAvailabilityevaluated.The availability-evaluation loop only runs over
all(lines 53-63) beforeextrais merged in (lines 64-68). Anyextralauncher with a non-nilAvailabilityfunc will keep the zero-valueAvailable=false/AvailabilityReason=""whenrebuildFromSliceruns, since that function only auto-populates availability for launchers whoseAvailabilityisnil. This silently excludes genuinely-availableextralaunchers fromGetAvailableLaunchersBySystem(now used by the mediascanner for folder discovery) and reports them as unavailable via the API with no reason.🐛 Proposed fix: evaluate availability after merging `extra`
func (lc *LauncherCache) Initialize(pl platforms.Platform, cfg *config.Instance, extra ...platforms.Launcher) { launchableSystems, launchableMedia := launchables.Available(cfg, pl) all := pl.Launchers(cfg) all = append(all, launchables.LaunchersFor(launchableSystems, launchableMedia)...) + for i := range extra { + if !launcherInSlice(all, extra[i].ID) { + all = append(all, extra[i]) + } + } for i := range all { all[i].Available = true all[i].AvailabilityReason = "" if all[i].Availability == nil { continue } if err := all[i].Availability(cfg); err != nil { all[i].Available = false all[i].AvailabilityReason = err.Error() } } - for i := range extra { - if !launcherInSlice(all, extra[i].ID) { - all = append(all, extra[i]) - } - } lc.rebuildFromSlice(all)As per coding guidelines, "Write tests for all new code — see TESTING.md and pkg/testing/README.md"; please add a test covering an
extralauncher with a non-nilAvailabilityto prevent regressions.🤖 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/helpers/launcher_cache.go` around lines 49 - 68, Move the availability-evaluation loop in LauncherCache.Initialize to run after extra launchers are merged into all, so added launchers with non-nil Availability are evaluated and receive accurate Available and AvailabilityReason values before rebuilding the cache. Add a regression test covering an extra launcher with a non-nil Availability function, verifying available and unavailable outcomes are handled correctly.Source: Coding guidelines
pkg/database/mediadb/media_user_data.go (1)
27-29: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winDon't drop
rows.Close()errors here. Both query helpers now ignore close failures, so an early return can hide them entirely. Keep the warning or merge the close error into the returned error.🤖 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/database/mediadb/media_user_data.go` around lines 27 - 29, The query helper functions in media_user_data.go currently ignore errors from rows.Close(), including on early returns. Update both helpers to preserve the existing warning or merge the close error into the returned error, ensuring rows.Close() is always checked before returning.pkg/platforms/shared/linuxbase/base.go (1)
122-142: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winKilled tracked processes need a wait path.
pkg/platforms/shared/linuxbase/base.go:122-142kills the previous process and immediately drops the only reference to it.pkg/platforms/shared/steam/steamtracker/integration.gocan call this path directly, so a live tracked process can be replaced without ever being waited on. Reap the superseded process before overwritingtrackedProcess.🤖 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/shared/linuxbase/base.go` around lines 122 - 142, SetTrackedProcess must wait for a superseded tracked process after killing it and before replacing trackedProcess. Update the existing-process branch to retain the old process, kill it, and wait for its completion (handling/logging wait errors) before assigning the new process; preserve the existing locking and state-reset behavior.
🧹 Nitpick comments (9)
pkg/zapscript/launch.go (1)
224-245: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winReorder availability check after the cheaper path match.
Availability(env.Cfg)runs for every launcher before the cheaphelpers.PathIsLauncherfilter. SinceAvailabilityimplementations can perform filesystem/dependency checks, most of that work is wasted on launchers that wouldn't have matched the path anyway.♻️ Proposed reordering
for i := range launchers { - if launchers[i].Availability != nil && launchers[i].Availability(env.Cfg) != nil { - continue - } if !helpers.PathIsLauncher(env.Cfg, pl, &launchers[i], path) { continue } + if launchers[i].Availability != nil && launchers[i].Availability(env.Cfg) != nil { + continue + } score := launcherInferenceScore(&launchers[i])🤖 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/zapscript/launch.go` around lines 224 - 245, Reorder the checks in inferLauncherForPath so helpers.PathIsLauncher runs before the potentially expensive Availability callback; only evaluate launchers[i].Availability(env.Cfg) after the path matches, preserving the existing skip and scoring behavior.pkg/platforms/steamos/emudeck.go (2)
194-205: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPath-validation logic duplicated (and slightly diverged) between EmuDeck and RetroDECK.
emuDeckPathTestandretrodeck.go's inlineTestclosure implement nearly identical traversal/extension checks, but with a subtle inconsistency: this function rejects.txtcase-insensitively (strings.EqualFold(ext, ".txt")), while RetroDECK's check (pkg/platforms/steamos/retrodeck.go:135) only rejects the exact-case".txt". On case-sensitive filesystems, a.TXTfile would incorrectly pass RetroDECK's test but fail EmuDeck's. Consider extracting a single shared helper (e.g. inpkg/platforms/shared/esdeorpkg/platforms/shared/launchers) for both traversal-guard + extension checks to avoid drift.🤖 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/steamos/emudeck.go` around lines 194 - 205, The path-validation logic is duplicated and differs in case handling between emuDeckPathTest and RetroDECK’s inline Test closure. Extract a shared helper for traversal protection and extension validation, update both callers to use it, and ensure .txt rejection is case-insensitive via strings.EqualFold.
168-192: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate gamescope-focus wiring instead of reusing
withGamescopeFocus.
standaloneEmuDeckLauncher'sLaunch/Killreimplement the exact same "callgamescope.ManageFocus/RevertFocus" pattern already centralized inwithGamescopeFocus(retroarch.go). Building a bare launcher and callingwithGamescopeFocus(&launcher)here (as done for the RetroArch path increateEmuDeckLauncher) would remove this duplication and keep the gamescope-wrapping logic in one place.♻️ Proposed refactor
func standaloneEmuDeckLauncher( systemFolder string, systemInfo esde.SystemInfo, emulator EmulatorConfig, ) platforms.Launcher { - return platforms.Launcher{ + launcher := platforms.Launcher{ ID: "EmuDeck" + systemInfo.GetLauncherID(), SystemID: systemInfo.SystemID, Lifecycle: platforms.LifecycleTracked, Launch: func(_ *config.Instance, path string, _ *platforms.LaunchOptions) (*os.Process, error) { - proc, err := launchStandaloneEmulator(context.Background(), emulator, path, systemFolder) - if err != nil { - return nil, err - } - if proc != nil { - go gamescope.ManageFocus(proc) - } - return proc, nil + return launchStandaloneEmulator(context.Background(), emulator, path, systemFolder) }, - Kill: func(_ *config.Instance) error { - gamescope.RevertFocus() - return nil - }, } + withGamescopeFocus(&launcher) + return launcher }🤖 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/steamos/emudeck.go` around lines 168 - 192, Refactor standaloneEmuDeckLauncher to build the launcher without inline gamescope focus handling, then return it wrapped with the existing withGamescopeFocus helper. Remove the direct gamescope.ManageFocus call from Launch and gamescope.RevertFocus call from Kill, matching the pattern used by createEmuDeckLauncher and keeping focus management centralized.pkg/platforms/linux/retroarch.go (2)
19-23: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
retroArchNetworkAddrconstant duplicated across packages.The same literal
"127.0.0.1:55355"is redefined here and inpkg/platforms/zapos/platform.go(line 24), and appears to also be redefined in thesteamospackage (referenced unqualified inretroarch_test.go). Consider hoisting this default intopkg/platforms/shared/retroarchas an exported constant to avoid drift across the three platforms.🤖 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/linux/retroarch.go` around lines 19 - 23, The RetroArch network address is duplicated across platform packages. Add an exported shared constant in pkg/platforms/shared/retroarch, then replace retroArchNetworkAddr in the Linux and Zapos implementations and the unqualified reference in the SteamOS tests with that shared constant, removing redundant local definitions.
47-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo filesystem injection point for
ensureRetroArchNetworkConfig, unlike SteamOS.This always writes via the real OS filesystem (
nilfs passed toEnsureNetworkCommandConfig), with no way to inject a test double. Compare topkg/platforms/steamos/platform.go, which added anfs afero.Fsfield andfileSystem()helper specifically soTestPlatformStartPreWritesRetroArchConfigcan verify the write withafero.NewMemMapFs(). The Linux platform has no equivalent test for itsStartPre/ensureRetroArchNetworkConfigwrite path.
As per coding guidelines,**/*.go: "Use afero for filesystem operations in testable code" and "Write tests for all new code."♻️ Mirror the SteamOS injectable-fs pattern
-func ensureRetroArchNetworkConfig() error { - if err := sharedretroarch.EnsureNetworkCommandConfig(nil, defaultRetroArchAppendConfigPath()); err != nil { +func ensureRetroArchNetworkConfig(fs afero.Fs) error { + if err := sharedretroarch.EnsureNetworkCommandConfig(fs, defaultRetroArchAppendConfigPath()); err != nil { return fmt.Errorf("write RetroArch network config: %w", err) } return nil }(and add an
fs afero.Fsfield + accessor toPlatform, wired throughStartPre, plus a memfs-based test.)🤖 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/linux/retroarch.go` around lines 47 - 52, Add an injectable afero.Fs field and fileSystem() accessor to the Linux Platform, mirroring the SteamOS implementation, and pass it from StartPre into ensureRetroArchNetworkConfig and sharedretroarch.EnsureNetworkCommandConfig instead of nil. Add a memfs-based test covering the StartPre RetroArch network-config write path, including the expected file contents.Source: Coding guidelines
pkg/platforms/shared/retroarch/coremap.go (2)
265-268: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
alternateCoreLaunchtype is declared mid-file, after several functions.As per coding guidelines,
Define Go types and consts near the top of the file, before functions and methods.alternateCoreLaunchis defined at line 265, well afterCoreDefinitions,CoreLaunches,scanSpecForSystem,CoreLaunchForFolder,CorePolicyForFolder, andcoreLaunchForDef. Consider moving it (andalternateCoreLaunches) up nearCoreDef/other type declarations.🤖 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/shared/retroarch/coremap.go` around lines 265 - 268, Move the alternateCoreLaunch and alternateCoreLaunches type declarations from their mid-file location to the top-level type declaration section near CoreDef, CoreDefinitions, and CoreLaunches, before functions such as scanSpecForSystem and CoreLaunchForFolder, without changing their definitions or behavior.Source: Coding guidelines
205-212: 🎯 Functional Correctness | 🔵 Trivial | ⚖️ Poor tradeoff
scanSpecForSystemis ambiguous for systems with multipleESFolderentries.Several
coreDefinitionsshare the sameSystemIDwith differentESFolders (e.g.,systemdefs.SystemC64at lines 51 and 53;systemdefs.SystemMSXat lines 93–94).scanSpecForSystemreturns the first definition whoseSystemIDmatches, so if analternateCoreLaunchis ever added for one of these systems, it would silently inherit the folders/extensions of the wrong definition. This doesn't currently trigger a bug (the only entries inalternateCoreLaunchestargetSystemSNES/SystemNES, each with a singleESFolder), but it's a latent trap for future additions.Consider having
alternateCoreLaunchcarry an explicitESFolder(like the primary definitions do) soscanSpecForSystemcan match unambiguously instead of falling back toSystemID-only lookup.🤖 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/shared/retroarch/coremap.go` around lines 205 - 212, Make alternate core lookups unambiguous by adding an explicit ESFolder field to the alternateCoreLaunch representation and populating it for existing entries. Update scanSpecForSystem and related alternate-core selection logic to match both SystemID and ESFolder, preserving the primary definition’s folder/extensions instead of selecting the first SystemID match.pkg/platforms/shared/retroarch/launcher.go (1)
137-156: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winRelative
Exec[0]bypasses installed-binary validation.
validateLaunchonlyStats the executable when it's an absolute path (Line 148). For a relative command (e.g., aflatpakinvocation), the function skips this check entirely and relies solely onopts.Preflight. If a caller configures a relativeExecwithout aPreflightfunction,Availability()/Launch()will report the launcher as available even when the binary can't actually be resolved onPATH, only failing later atcmd.Start().Consider falling back to
exec.LookPath(executable)when the path is relative andPreflightis nil, so availability reporting stays accurate for allExecconfigurations.♻️ Suggested fallback check
executable := opts.Exec[0] if filepath.IsAbs(executable) { info, err := fs.Stat(executable) if err != nil { return fmt.Errorf("retroarch is not installed at %s: %w", executable, err) } if info.IsDir() || info.Mode().Perm()&0o111 == 0 { return fmt.Errorf("retroarch executable is not executable: %s", executable) } + } else if opts.Preflight == nil { + if _, err := exec.LookPath(executable); err != nil { + return fmt.Errorf("retroarch is not installed on PATH: %s: %w", executable, err) + } }Please confirm whether the SteamOS/Linux RetroArch wiring (not in this batch) always supplies a
PreflightwhenExec[0]is relative.🤖 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/shared/retroarch/launcher.go` around lines 137 - 156, validateLaunch currently validates only absolute Exec[0] paths, allowing unresolved relative commands when Preflight is nil. Add a relative-path fallback using exec.LookPath(executable) when no Preflight validation is configured, returning a descriptive wrapped error if lookup fails; preserve existing absolute-path checks and use the result consistently in Availability/Launch.pkg/platforms/shared/retroarch/networkconfig.go (1)
33-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a direct unit test for this package.
EnsureNetworkCommandConfigis only exercised indirectly today, via a SteamOS platform test. As per coding guidelines,**/*.gofiles should have tests co-located perTESTING.md/pkg/testing/README.md; consider adding a smallnetworkconfig_test.gohere (e.g., usingafero.NewMemMapFs) covering the nil-fsfallback and directory-creation-failure path.🤖 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/shared/retroarch/networkconfig.go` around lines 33 - 44, Add a co-located networkconfig_test.go with direct unit tests for EnsureNetworkCommandConfig, using afero.NewMemMapFs to verify successful operation and the nil-fs fallback, plus a filesystem setup that forces directory creation to fail and asserts the returned error. Cover the generated file contents and permissions where supported, and use descriptive test names referencing EnsureNetworkCommandConfig.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.
Inline comments:
In `@pkg/platforms/launch.go`:
- Around line 259-289: The fallback path in waitForBlockingProcess
unconditionally clears active media and can overwrite newer media for untracked
processes. Add stale-process protection before calling setActiveMedia(nil),
using the process identity or equivalent lifecycle state to confirm the
completed process still owns the active media; otherwise skip the clear and log
that it was stale, matching the TrackedProcessMediaClearer behavior.
In `@pkg/platforms/shared/retroarch/options_test.go`:
- Around line 85-96: Add a regression test covering empty execution commands in
TestBuildCommand_OmitsEmptyOptions or a separate test: call BuildCommand with
Options{Exec: nil}, then assert spec.Name is empty and verify the expected
arguments and environment. Use buildCommandWithCore/BuildCommand behavior to
ensure the empty Exec case remains covered.
In `@pkg/platforms/shared/retroarch/options.go`:
- Around line 115-145: Update buildCommandWithCore to detect an empty opts.Exec
before appending RetroArch arguments and return a CommandSpec with an empty Name
while preserving the environment, so Launch and Availability can report the
intended “retroarch executable is not configured” error. Do not rely on the
existing len(argv) == 0 check, since the core and media arguments are always
appended.
In `@pkg/platforms/steamos/emudeck.go`:
- Around line 168-192: Set the standalone launcher's Availability in
standaloneEmuDeckLauncher based on whether emulator.FlatpakID is installed,
using the existing launchers.IsFlatpakInstalled check (or the corresponding
availability/preflight convention used by sharedretroarch.NewLauncher). Ensure
unavailable Flatpak-backed emulators are reported as unavailable by cache, API,
and scanner before launch, while preserving the existing Launch and Kill
behavior.
---
Outside diff comments:
In `@pkg/database/mediadb/media_user_data.go`:
- Around line 27-29: The query helper functions in media_user_data.go currently
ignore errors from rows.Close(), including on early returns. Update both helpers
to preserve the existing warning or merge the close error into the returned
error, ensuring rows.Close() is always checked before returning.
In `@pkg/helpers/launcher_cache.go`:
- Around line 49-68: Move the availability-evaluation loop in
LauncherCache.Initialize to run after extra launchers are merged into all, so
added launchers with non-nil Availability are evaluated and receive accurate
Available and AvailabilityReason values before rebuilding the cache. Add a
regression test covering an extra launcher with a non-nil Availability function,
verifying available and unavailable outcomes are handled correctly.
In `@pkg/platforms/launch.go`:
- Around line 137-155: Move the Launcher.Launch nil check and
Launcher.Availability validation before the
StopActiveLauncher(StopForPreemption) call in the launch flow. Ensure
unavailable or misconfigured launchers return their existing errors without
stopping the active launcher, while preserving the current preemption behavior
for validated launchers.
In `@pkg/platforms/shared/linuxbase/base.go`:
- Around line 122-142: SetTrackedProcess must wait for a superseded tracked
process after killing it and before replacing trackedProcess. Update the
existing-process branch to retain the old process, kill it, and wait for its
completion (handling/logging wait errors) before assigning the new process;
preserve the existing locking and state-reset behavior.
---
Nitpick comments:
In `@pkg/platforms/linux/retroarch.go`:
- Around line 19-23: The RetroArch network address is duplicated across platform
packages. Add an exported shared constant in pkg/platforms/shared/retroarch,
then replace retroArchNetworkAddr in the Linux and Zapos implementations and the
unqualified reference in the SteamOS tests with that shared constant, removing
redundant local definitions.
- Around line 47-52: Add an injectable afero.Fs field and fileSystem() accessor
to the Linux Platform, mirroring the SteamOS implementation, and pass it from
StartPre into ensureRetroArchNetworkConfig and
sharedretroarch.EnsureNetworkCommandConfig instead of nil. Add a memfs-based
test covering the StartPre RetroArch network-config write path, including the
expected file contents.
In `@pkg/platforms/shared/retroarch/coremap.go`:
- Around line 265-268: Move the alternateCoreLaunch and alternateCoreLaunches
type declarations from their mid-file location to the top-level type declaration
section near CoreDef, CoreDefinitions, and CoreLaunches, before functions such
as scanSpecForSystem and CoreLaunchForFolder, without changing their definitions
or behavior.
- Around line 205-212: Make alternate core lookups unambiguous by adding an
explicit ESFolder field to the alternateCoreLaunch representation and populating
it for existing entries. Update scanSpecForSystem and related alternate-core
selection logic to match both SystemID and ESFolder, preserving the primary
definition’s folder/extensions instead of selecting the first SystemID match.
In `@pkg/platforms/shared/retroarch/launcher.go`:
- Around line 137-156: validateLaunch currently validates only absolute Exec[0]
paths, allowing unresolved relative commands when Preflight is nil. Add a
relative-path fallback using exec.LookPath(executable) when no Preflight
validation is configured, returning a descriptive wrapped error if lookup fails;
preserve existing absolute-path checks and use the result consistently in
Availability/Launch.
In `@pkg/platforms/shared/retroarch/networkconfig.go`:
- Around line 33-44: Add a co-located networkconfig_test.go with direct unit
tests for EnsureNetworkCommandConfig, using afero.NewMemMapFs to verify
successful operation and the nil-fs fallback, plus a filesystem setup that
forces directory creation to fail and asserts the returned error. Cover the
generated file contents and permissions where supported, and use descriptive
test names referencing EnsureNetworkCommandConfig.
In `@pkg/platforms/steamos/emudeck.go`:
- Around line 194-205: The path-validation logic is duplicated and differs in
case handling between emuDeckPathTest and RetroDECK’s inline Test closure.
Extract a shared helper for traversal protection and extension validation,
update both callers to use it, and ensure .txt rejection is case-insensitive via
strings.EqualFold.
- Around line 168-192: Refactor standaloneEmuDeckLauncher to build the launcher
without inline gamescope focus handling, then return it wrapped with the
existing withGamescopeFocus helper. Remove the direct gamescope.ManageFocus call
from Launch and gamescope.RevertFocus call from Kill, matching the pattern used
by createEmuDeckLauncher and keeping focus management centralized.
In `@pkg/zapscript/launch.go`:
- Around line 224-245: Reorder the checks in inferLauncherForPath so
helpers.PathIsLauncher runs before the potentially expensive Availability
callback; only evaluate launchers[i].Availability(env.Cfg) after the path
matches, preserving the existing skip and scoring behavior.
🪄 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 Plus
Run ID: 9403c1f9-06e3-47a0-afd2-347bd4daec6d
📒 Files selected for processing (52)
.github/workflows/build.yml.github/workflows/lint-and-test.ymlTaskfile.dist.ymlcmd/zapos/main.gogo.modpkg/api/methods/launchers.gopkg/api/methods/media_response_helpers.gopkg/api/methods/media_scrape.gopkg/api/models/responses.gopkg/api/ws_dispatcher.gopkg/api/ws_dispatcher_test.gopkg/database/mediadb/media_user_data.gopkg/database/mediascanner/mediascanner.gopkg/helpers/bgpriority/bgpriority_linux.gopkg/helpers/launcher_cache.gopkg/platforms/ids/ids.gopkg/platforms/launch.gopkg/platforms/launch_test.gopkg/platforms/linux/platform.gopkg/platforms/linux/platform_test.gopkg/platforms/linux/retroarch.gopkg/platforms/mister/tracker/tracker.gopkg/platforms/platforms.gopkg/platforms/shared/linuxbase/base.gopkg/platforms/shared/linuxbase/base_test.gopkg/platforms/shared/retroarch/controls.gopkg/platforms/shared/retroarch/controls_test.gopkg/platforms/shared/retroarch/coremap.gopkg/platforms/shared/retroarch/coremap_test.gopkg/platforms/shared/retroarch/launcher.gopkg/platforms/shared/retroarch/launcher_test.gopkg/platforms/shared/retroarch/networkconfig.gopkg/platforms/shared/retroarch/options.gopkg/platforms/shared/retroarch/options_test.gopkg/platforms/steamos/emudeck.gopkg/platforms/steamos/launchers_test.gopkg/platforms/steamos/platform.gopkg/platforms/steamos/retroarch.gopkg/platforms/steamos/retroarch_test.gopkg/platforms/steamos/retrodeck.gopkg/platforms/zapos/platform.gopkg/platforms/zapos/platform_test.gopkg/readers/opticaldrive/opticaldrive_test.gopkg/service/daemon/daemon.gopkg/service/service_test.gopkg/zapscript/control_test.gopkg/zapscript/launch.gopkg/zapscript/media_context.gopkg/zapscript/zaplinks_test.goscripts/tasks/utils/makezip/main.goscripts/tasks/utils/makezip/main_test.goscripts/tasks/zapos.yml
|
|
||
| func waitForBlockingProcess( | ||
| platform Platform, | ||
| proc *os.Process, | ||
| setActiveMedia func(*models.ActiveMedia), | ||
| path string, | ||
| ) { | ||
| var waitErr error | ||
| if waiter, ok := platform.(TrackedProcessWaiter); ok { | ||
| waitErr = waiter.WaitTrackedProcess(proc) | ||
| } else { | ||
| _, waitErr = proc.Wait() | ||
| } | ||
| if waitErr != nil { | ||
| log.Debug().Err(waitErr).Msgf("blocking process wait error for: %s", path) | ||
| } else { | ||
| log.Debug().Msgf("blocking process completed for: %s", path) | ||
| } | ||
|
|
||
| if clearer, ok := platform.(TrackedProcessMediaClearer); ok { | ||
| if clearer.ClearTrackedProcessMedia(proc) { | ||
| log.Debug().Msgf("cleared active media after blocking process ended: %s", path) | ||
| } else { | ||
| log.Debug().Msgf("skipped stale active-media clear after blocking process ended: %s", path) | ||
| } | ||
| return | ||
| } | ||
|
|
||
| setActiveMedia(nil) | ||
| log.Debug().Msgf("cleared active media after blocking process ended: %s", path) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find Platform implementations using LifecycleBlocking
rg -n 'LifecycleBlocking' --type=go -g '!*_test.go'
# Find implementers of the new optional interfaces
ast-grep run --pattern 'func ($_ *$_) WaitTrackedProcess($_ *os.Process) error { $$$ }' --lang go
ast-grep run --pattern 'func ($_ *$_) ClearTrackedProcessMedia($_ *os.Process) bool { $$$ }' --lang goRepository: ZaparooProject/zaparoo-core
Length of output: 165
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate relevant files and symbols
git ls-files | rg '^(pkg/platforms/|.*\.go$)' >/tmp/all-go-files.txt
printf '\n== lifecycle blocking refs ==\n'
rg -n 'LifecycleBlocking|TrackedProcessWaiter|TrackedProcessMediaClearer|waitForBlockingProcess' pkg/platforms --type=go -g '!*_test.go' || true
printf '\n== platform implementations in pkg/platforms ==\n'
rg -n 'type .*Platform|struct .*Platform|func .*Lifecycle|func .*Launch' pkg/platforms --type=go -g '!*_test.go' || true
printf '\n== all LifecycleBlocking uses repo-wide ==\n'
rg -n 'LifecycleBlocking' --type=go -g '!*_test.go' . || true
printf '\n== optional interface implementers repo-wide ==\n'
rg -n 'WaitTrackedProcess|ClearTrackedProcessMedia' --type=go -g '!*_test.go' . || trueRepository: ZaparooProject/zaparoo-core
Length of output: 21207
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# If lifecycle refs exist, show surrounding context from candidate files.
for f in $(rg -l 'LifecycleBlocking|TrackedProcessWaiter|TrackedProcessMediaClearer' pkg/platforms --type=go -g '!*_test.go' || true); do
echo "=== $f ==="
sed -n '1,260p' "$f" | cat -n | sed -n '1,260p'
doneRepository: ZaparooProject/zaparoo-core
Length of output: 39901
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read only the relevant platform files that mention LifecycleBlocking.
for f in $(rg -l 'LifecycleBlocking' pkg/platforms --type=go -g '!*_test.go' || true); do
echo "### $f"
sed -n '1,260p' "$f"
doneRepository: ZaparooProject/zaparoo-core
Length of output: 33221
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files mentioning blocking process handling =="
rg -n 'blocking process|ClearTrackedProcessMedia|WaitTrackedProcess|setActiveMedia\(nil\)' pkg/platforms --type=go -g '!*_test.go' || true
echo
echo "== all platform launchers / implementations with lifecycle keywords =="
rg -n 'Lifecycle|Blocking|TrackedProcess|ActiveMedia|Launch\(' pkg/platforms --type=go -g '!*_test.go' || trueRepository: ZaparooProject/zaparoo-core
Length of output: 29876
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== shared/retroarch.NewLauncher call sites ==\n'
rg -n 'shared/retroarch|retroarch\.NewLauncher|NewLauncher\(opts .*retroarch|NewLaunchers\(opts .*retroarch' pkg --type=go -g '!*_test.go' || true
printf '\n== windows platform methods that might satisfy optional interfaces ==\n'
rg -n 'func \(.*Platform.*\) (WaitTrackedProcess|ClearTrackedProcessMedia|SetTrackedProcess|StopActiveLauncher|LaunchMedia)' pkg/platforms/windows --type=go -g '!*_test.go' || true
printf '\n== blocking launchers in windows package ==\n'
sed -n '330,405p' pkg/platforms/windows/platform.go | cat -n
printf '\n== interface definitions and implementations candidates ==\n'
sed -n '335,365p' pkg/platforms/platforms.go | cat -n
sed -n '130,210p' pkg/platforms/shared/linuxbase/base.go | cat -nRepository: ZaparooProject/zaparoo-core
Length of output: 9409
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== search for Linux/SteamOS/ZapOS cohort mentions with shared/retroarch ==\n'
rg -n 'shared/retroarch' pkg/platforms/{linux,steamos,bazzite,zapos,windows,mac,mister,replayos,retropie,recalbox,libreelec,batocera,chimeraos,shared} --type=go -g '!*_test.go' || true
printf '\n== any other LifecycleBlocking launchers besides RetroArch and Windows ==\n'
rg -n 'LifecycleBlocking' pkg --type=go -g '!*_test.go' || trueRepository: ZaparooProject/zaparoo-core
Length of output: 1495
Guard the fallback clear in waitForBlockingProcess. pkg/platforms/windows/platform.go still has a LifecycleBlocking launcher (GenericExecutable) without TrackedProcessMediaClearer, so the unconditional setActiveMedia(nil) can clear newer active media after an old process exits.
🤖 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/launch.go` around lines 259 - 289, The fallback path in
waitForBlockingProcess unconditionally clears active media and can overwrite
newer media for untracked processes. Add stale-process protection before calling
setActiveMedia(nil), using the process identity or equivalent lifecycle state to
confirm the completed process still owns the active media; otherwise skip the
clear and log that it was stale, matching the TrackedProcessMediaClearer
behavior.
1 participant
Summary
Validated with
task lint-fixandtask test.Summary by CodeRabbit
availableplusavailabilityReasonwhen applicable.