From 1543e155a871caed0a3f74c6c4d8c265c8890fd5 Mon Sep 17 00:00:00 2001 From: SAY-5 Date: Mon, 4 May 2026 19:22:34 -0700 Subject: [PATCH] fix(ui): guard against empty Files slice in buildDownloads Skip ROMs whose cached row has has_multiple_files=false and an empty Files slice (and log a warning) instead of indexing g.Files[0]. This panic was reachable when an incremental cache update wrote a row without a files array. Fixes #223 Signed-off-by: SAY-5 --- ui/download.go | 8 ++++ ui/download_test.go | 91 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 99 insertions(+) create mode 100644 ui/download_test.go diff --git a/ui/download.go b/ui/download.go index 230ab53..0f082e6 100644 --- a/ui/download.go +++ b/ui/download.go @@ -368,6 +368,14 @@ func (s *DownloadScreen) buildDownloads(config internal.Config, host romm.Host, downloadLocation = filepath.Join(tmpDir, fmt.Sprintf("grout_multirom_%d.zip", g.ID)) sourceURL, _ = url.JoinPath(host.URL(), "/api/roms/", strconv.Itoa(g.ID), "content", g.FsName) } else { + // Skip games with no file metadata to avoid an out-of-range panic. + // This can happen when the cached row was written without a + // `files` array (e.g. after an incremental cache update). + if len(g.Files) == 0 { + gaba.GetLogger().Warn("Skipping ROM with no file metadata; refresh the library to repopulate it", + "game", g.Name, "id", g.ID, "fs_name", g.FsName) + continue + } // Find the file to download - use selected file if specified, otherwise first file fileToDownload := g.Files[0] if selectedFileID > 0 { diff --git a/ui/download_test.go b/ui/download_test.go new file mode 100644 index 0000000..b28600f --- /dev/null +++ b/ui/download_test.go @@ -0,0 +1,91 @@ +package ui + +import ( + "os" + "testing" + + "grout/internal" + "grout/romm" +) + +func TestMain(m *testing.M) { + // cfw.GetCFW() (called from buildDownloads) requires the CFW env var to + // be set to a recognised value, otherwise it terminates the process. + if os.Getenv("CFW") == "" { + os.Setenv("CFW", "ROCKNIX") + } + os.Exit(m.Run()) +} + +// TestBuildDownloads_EmptyFiles is a regression test for issue #223: +// a cached ROM with HasMultipleFiles == false and an empty Files slice +// must not panic. The entry should be skipped instead. +func TestBuildDownloads_EmptyFiles(t *testing.T) { + s := NewDownloadScreen() + config := internal.Config{} + host := romm.Host{RootURI: "http://example.invalid"} + platform := romm.Platform{ID: 1, FSSlug: "nds", Name: "Nintendo DS"} + + games := []romm.Rom{ + { + ID: 60, + Name: "Professor Layton and the Curious Village", + FsName: "Professor Layton and the Curious Village (USA).nds", + FsNameNoExt: "Professor Layton and the Curious Village (USA)", + HasMultipleFiles: false, + Files: nil, + }, + } + + defer func() { + if r := recover(); r != nil { + t.Fatalf("buildDownloads panicked on empty Files slice: %v", r) + } + }() + + downloads, artDownloads, gamelistEntries := s.buildDownloads(config, host, platform, games, 0) + + if len(downloads) != 0 { + t.Errorf("expected 0 downloads when Files is empty, got %d", len(downloads)) + } + if len(artDownloads) != 0 { + t.Errorf("expected 0 art downloads when Files is empty, got %d", len(artDownloads)) + } + if len(gamelistEntries) != 0 { + t.Errorf("expected 0 gamelist entries when Files is empty, got %d", len(gamelistEntries)) + } +} + +// TestBuildDownloads_SingleFile_HappyPath sanity-checks that a normal +// single-file ROM still produces a download URL after the empty-Files guard. +func TestBuildDownloads_SingleFile_HappyPath(t *testing.T) { + s := NewDownloadScreen() + config := internal.Config{} + host := romm.Host{RootURI: "http://example.invalid"} + platform := romm.Platform{ID: 1, FSSlug: "nds", Name: "Nintendo DS"} + + games := []romm.Rom{ + { + ID: 42, + Name: "Test Game", + FsName: "test.nds", + FsNameNoExt: "test", + HasMultipleFiles: false, + Files: []romm.RomFile{ + {ID: 100, FileName: "test.nds"}, + }, + }, + } + + downloads, _, gamelistEntries := s.buildDownloads(config, host, platform, games, 0) + + if len(downloads) != 1 { + t.Fatalf("expected 1 download, got %d", len(downloads)) + } + if len(gamelistEntries) != 1 { + t.Fatalf("expected 1 gamelist entry, got %d", len(gamelistEntries)) + } + if downloads[0].URL == "" { + t.Error("expected non-empty download URL") + } +}