From 5e5cac488354542d96248ee7ae04fa56e7295c42 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 4 Jul 2026 22:39:19 +0000 Subject: [PATCH] fix: address copilot and coderabbit review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Use named returns for IsAlreadyConverted and WriteChapterToCBZ so errs.Capture defers propagate close errors to the caller. - Add context.Context to IsAlreadyConverted and ExtractChapter for cancellation/timeout support; thread it from Optimize. - Filter non-image files (__MACOSX, Thumbs.db, .DS_Store, etc.) and only extract supported image extensions in ExtractChapter. - Validate TempDir is non-empty in ConvertChapter before creating output dir. - Separate fatal conversion errors from PageIgnoredError in the WebP converter result aggregation so real failures aren't masked. - Fix error message "failed to load chapter" → "failed to extract chapter". - Use %w instead of %v in fmt.Errorf for proper error wrapping. - Fix LoadChapter doc comment to match its actual behavior. - Extract shared parseConvertedComment/parseConvertedCommentTime helpers to avoid duplicated zip-comment parsing logic. - Convert tests to use testify assertions (assert/require) per guidelines. Co-authored-by: Belphemur <197810+Belphemur@users.noreply.github.com> --- internal/cbz/cbz_creator.go | 2 +- internal/cbz/cbz_loader.go | 123 +++++++++--- internal/cbz/cbz_loader_test.go | 277 ++++++++------------------- internal/utils/file_utils_test.go | 6 +- internal/utils/optimize.go | 34 ++-- pkg/converter/webp/webp_converter.go | 30 ++- 6 files changed, 216 insertions(+), 256 deletions(-) diff --git a/internal/cbz/cbz_creator.go b/internal/cbz/cbz_creator.go index d52c66e..2a4b6c0 100644 --- a/internal/cbz/cbz_creator.go +++ b/internal/cbz/cbz_creator.go @@ -14,7 +14,7 @@ import ( // WriteChapterToCBZ creates a CBZ file from a Chapter by streaming page files // from disk directly into the zip archive. No image data is held in memory. -func WriteChapterToCBZ(chapter *manga.Chapter, outputFilePath string) error { +func WriteChapterToCBZ(chapter *manga.Chapter, outputFilePath string) (err error) { log.Debug(). Str("chapter_file", chapter.FilePath). Str("output_path", outputFilePath). diff --git a/internal/cbz/cbz_loader.go b/internal/cbz/cbz_loader.go index a95126d..01c3af4 100644 --- a/internal/cbz/cbz_loader.go +++ b/internal/cbz/cbz_loader.go @@ -10,6 +10,7 @@ import ( "os" "path/filepath" "strings" + "time" "github.com/araddon/dateparse" "github.com/belphemur/CBZOptimizer/v2/internal/manga" @@ -18,10 +19,52 @@ import ( "github.com/rs/zerolog/log" ) +// supportedImageExtensions contains file extensions considered valid image pages. +var supportedImageExtensions = map[string]bool{ + ".jpg": true, + ".jpeg": true, + ".png": true, + ".gif": true, + ".webp": true, + ".bmp": true, + ".tiff": true, + ".tif": true, +} + +// parseConvertedComment checks if a zip comment's first line is a parseable date, +// indicating the archive was already converted. Returns true and the parsed time if so. +func parseConvertedComment(comment string) bool { + if comment == "" { + return false + } + scanner := bufio.NewScanner(strings.NewReader(comment)) + if scanner.Scan() { + _, err := dateparse.ParseAny(scanner.Text()) + return err == nil + } + return false +} + +// parseConvertedCommentTime parses the converted timestamp from a zip comment. +// Returns the time and true if the comment indicates conversion, otherwise zero time and false. +func parseConvertedCommentTime(comment string) (time.Time, bool) { + if comment == "" { + return time.Time{}, false + } + scanner := bufio.NewScanner(strings.NewReader(comment)) + if scanner.Scan() { + t, err := dateparse.ParseAny(scanner.Text()) + if err == nil { + return t, true + } + } + return time.Time{}, false +} + // IsAlreadyConverted performs a fast check to see if the archive is already // converted without extracting any image data. It reads only the zip comment // and metadata files (converted.txt) to determine conversion status. -func IsAlreadyConverted(filePath string) (bool, error) { +func IsAlreadyConverted(ctx context.Context, filePath string) (converted bool, err error) { log.Debug().Str("file_path", filePath).Msg("Checking if already converted") pathLower := strings.ToLower(filepath.Ext(filePath)) @@ -34,15 +77,9 @@ func IsAlreadyConverted(filePath string) (bool, error) { defer errs.Capture(&err, r.Close, "failed to close zip reader") // Check zip comment - if r.Comment != "" { - scanner := bufio.NewScanner(strings.NewReader(r.Comment)) - if scanner.Scan() { - _, err := dateparse.ParseAny(scanner.Text()) - if err == nil { - log.Debug().Str("file_path", filePath).Msg("Already converted (zip comment)") - return true, nil - } - } + if parseConvertedComment(r.Comment) { + log.Debug().Str("file_path", filePath).Msg("Already converted (zip comment)") + return true, nil } // Check for converted.txt inside the archive @@ -69,7 +106,6 @@ func IsAlreadyConverted(filePath string) (bool, error) { // For CBR files, we need to use the archives library to check if pathLower == ".cbr" { - ctx := context.Background() fsys, err := archives.FileSystem(ctx, filePath, nil) if err != nil { return false, fmt.Errorf("failed to open archive: %w", err) @@ -106,7 +142,7 @@ func IsAlreadyConverted(filePath string) (bool, error) { // ExtractChapter extracts an archive (CBZ/CBR) to a temp directory on disk. // Pages are streamed directly to files — no image data is held in memory. // Returns a Chapter with PageFile entries pointing to extracted files. -func ExtractChapter(filePath string) (*manga.Chapter, error) { +func ExtractChapter(ctx context.Context, filePath string) (*manga.Chapter, error) { log.Debug().Str("file_path", filePath).Msg("Extracting chapter to disk") // Create temp directory for extraction @@ -131,39 +167,45 @@ func ExtractChapter(filePath string) (*manga.Chapter, error) { if pathLower == ".cbz" { r, err := zip.OpenReader(filePath) if err == nil { - if r.Comment != "" { - scanner := bufio.NewScanner(strings.NewReader(r.Comment)) - if scanner.Scan() { - t, err := dateparse.ParseAny(scanner.Text()) - if err == nil { - chapter.IsConverted = true - chapter.ConvertedTime = t - } - } + if t, ok := parseConvertedCommentTime(r.Comment); ok { + chapter.IsConverted = true + chapter.ConvertedTime = t } _ = r.Close() } } // Extract files using the archives library (supports both CBZ and CBR) - ctx := context.Background() fsys, err := archives.FileSystem(ctx, filePath, nil) if err != nil { _ = os.RemoveAll(tempDir) return nil, fmt.Errorf("failed to open archive: %w", err) } - err = fs.WalkDir(fsys, ".", func(path string, d fs.DirEntry, err error) error { - if err != nil { - return err + err = fs.WalkDir(fsys, ".", func(path string, d fs.DirEntry, walkErr error) error { + if walkErr != nil { + return walkErr } if d.IsDir() { return nil } + // Check for context cancellation during extraction + select { + case <-ctx.Done(): + return ctx.Err() + default: + } + ext := strings.ToLower(filepath.Ext(path)) fileName := strings.ToLower(filepath.Base(path)) + // Skip OS-specific metadata files and junk + if isJunkFile(path) { + log.Debug().Str("file_path", filePath).Str("skipped", path).Msg("Skipping junk file") + return nil + } + // Handle ComicInfo.xml if ext == ".xml" && fileName == "comicinfo.xml" { file, err := fsys.Open(path) @@ -198,6 +240,12 @@ func ExtractChapter(filePath string) (*manga.Chapter, error) { return nil } + // Only extract supported image files + if !supportedImageExtensions[ext] { + log.Debug().Str("file_path", filePath).Str("skipped", path).Str("ext", ext).Msg("Skipping non-image file") + return nil + } + // Extract image file to disk file, err := fsys.Open(path) if err != nil { @@ -256,9 +304,24 @@ func ExtractChapter(filePath string) (*manga.Chapter, error) { return chapter, nil } -// LoadChapter is a convenience function that checks conversion status and -// extracts the chapter. It returns early if already converted (without -// extracting all pages). -func LoadChapter(filePath string) (*manga.Chapter, error) { - return ExtractChapter(filePath) +// isJunkFile returns true for known OS/tool metadata files that should not be +// treated as pages (e.g., __MACOSX/, Thumbs.db, .DS_Store). +func isJunkFile(path string) bool { + // __MACOSX resource fork directories + if strings.Contains(path, "__MACOSX") { + return true + } + baseLower := strings.ToLower(filepath.Base(path)) + switch baseLower { + case "thumbs.db", ".ds_store", "desktop.ini": + return true + } + return false +} + +// LoadChapter extracts the chapter from a CBZ/CBR file to disk. +// It delegates to ExtractChapter and always extracts all pages. +// Use IsAlreadyConverted for a fast conversion status check without extraction. +func LoadChapter(filePath string) (*manga.Chapter, error) { + return ExtractChapter(context.Background(), filePath) } diff --git a/internal/cbz/cbz_loader_test.go b/internal/cbz/cbz_loader_test.go index 17cf3d6..588bce2 100644 --- a/internal/cbz/cbz_loader_test.go +++ b/internal/cbz/cbz_loader_test.go @@ -2,14 +2,16 @@ package cbz import ( "archive/zip" + "context" "fmt" "os" "path/filepath" - "strings" "testing" "time" "github.com/belphemur/CBZOptimizer/v2/internal/manga" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestLoadChapter(t *testing.T) { @@ -48,29 +50,18 @@ func TestLoadChapter(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { chapter, err := LoadChapter(tc.filePath) - if err != nil { - t.Fatalf("Failed to load chapter: %v", err) - } + require.NoError(t, err) defer func() { _ = chapter.Cleanup() }() - actualPages := len(chapter.Pages) - if actualPages != tc.expectedPages { - t.Errorf("Expected %d pages, but got %d", tc.expectedPages, actualPages) - } + assert.Equal(t, tc.expectedPages, len(chapter.Pages)) - if !strings.Contains(chapter.ComicInfoXml, tc.expectedSeries) { - t.Errorf("ComicInfoXml does not contain the expected series: %s", tc.expectedSeries) - } + assert.Contains(t, chapter.ComicInfoXml, tc.expectedSeries) - if chapter.IsConverted != tc.expectedConversion { - t.Errorf("Expected chapter to be converted: %t, but got %t", tc.expectedConversion, chapter.IsConverted) - } + assert.Equal(t, tc.expectedConversion, chapter.IsConverted) // Verify pages are on disk for _, page := range chapter.Pages { - if page.FilePath == "" { - t.Errorf("Page %d has no file path", page.Index) - } + assert.NotEmpty(t, page.FilePath, "Page %d has no file path", page.Index) } }) } @@ -101,40 +92,28 @@ func TestIsAlreadyConverted(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - result, err := IsAlreadyConverted(tc.filePath) - if err != nil { - t.Fatalf("Unexpected error: %v", err) - } - if result != tc.expected { - t.Errorf("Expected %t, got %t", tc.expected, result) - } + result, err := IsAlreadyConverted(context.Background(), tc.filePath) + require.NoError(t, err) + assert.Equal(t, tc.expected, result) }) } } func TestIsAlreadyConverted_NonexistentFile(t *testing.T) { - _, err := IsAlreadyConverted("/nonexistent/file.cbz") - if err == nil { - t.Error("Expected error for nonexistent file") - } + _, err := IsAlreadyConverted(context.Background(), "/nonexistent/file.cbz") + require.Error(t, err) } func TestIsAlreadyConverted_InvalidExtension(t *testing.T) { // Create a temp file with unsupported extension tmpFile, err := os.CreateTemp("", "test-*.txt") - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) defer func() { _ = os.Remove(tmpFile.Name()) }() _ = tmpFile.Close() - result, err := IsAlreadyConverted(tmpFile.Name()) - if err != nil { - t.Fatalf("Unexpected error: %v", err) - } - if result { - t.Error("Expected false for unsupported extension") - } + result, err := IsAlreadyConverted(context.Background(), tmpFile.Name()) + require.NoError(t, err) + assert.False(t, result, "Expected false for unsupported extension") } func TestIsAlreadyConverted_CBZWithConvertedComment(t *testing.T) { @@ -143,27 +122,19 @@ func TestIsAlreadyConverted_CBZWithConvertedComment(t *testing.T) { cbzPath := filepath.Join(tmpDir, "test.cbz") f, err := os.Create(cbzPath) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) w := zip.NewWriter(f) _ = w.SetComment(time.Now().Format(time.RFC3339)) // Add a dummy file fw, err := w.Create("page.webp") - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) _, _ = fw.Write([]byte("dummy")) _ = w.Close() _ = f.Close() - result, err := IsAlreadyConverted(cbzPath) - if err != nil { - t.Fatalf("Unexpected error: %v", err) - } - if !result { - t.Error("Expected CBZ with date comment to be detected as converted") - } + result, err := IsAlreadyConverted(context.Background(), cbzPath) + require.NoError(t, err) + assert.True(t, result, "Expected CBZ with date comment to be detected as converted") } func TestIsAlreadyConverted_CBZWithNonDateComment(t *testing.T) { @@ -171,120 +142,82 @@ func TestIsAlreadyConverted_CBZWithNonDateComment(t *testing.T) { cbzPath := filepath.Join(tmpDir, "test.cbz") f, err := os.Create(cbzPath) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) w := zip.NewWriter(f) _ = w.SetComment("this is not a date") fw, err := w.Create("page.jpg") - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) _, _ = fw.Write([]byte("dummy")) _ = w.Close() _ = f.Close() - result, err := IsAlreadyConverted(cbzPath) - if err != nil { - t.Fatalf("Unexpected error: %v", err) - } - if result { - t.Error("Expected CBZ with non-date comment to NOT be detected as converted") - } + result, err := IsAlreadyConverted(context.Background(), cbzPath) + require.NoError(t, err) + assert.False(t, result, "Expected CBZ with non-date comment to NOT be detected as converted") } func TestExtractChapter_NonexistentFile(t *testing.T) { - _, err := ExtractChapter("/nonexistent/file.cbz") - if err == nil { - t.Error("Expected error for nonexistent file") - } + _, err := ExtractChapter(context.Background(), "/nonexistent/file.cbz") + require.Error(t, err) } func TestExtractChapter_PageExtensions(t *testing.T) { - chapter, err := ExtractChapter("../../testdata/Chapter 128.cbz") - if err != nil { - t.Fatalf("Failed to extract chapter: %v", err) - } + chapter, err := ExtractChapter(context.Background(), "../../testdata/Chapter 128.cbz") + require.NoError(t, err) defer func() { _ = chapter.Cleanup() }() // All pages should have valid image extensions validExts := map[string]bool{".jpg": true, ".jpeg": true, ".png": true, ".webp": true, ".gif": true} for _, page := range chapter.Pages { - if !validExts[page.Extension] { - t.Errorf("Page %d has unexpected extension: %s", page.Index, page.Extension) - } + assert.True(t, validExts[page.Extension], "Page %d has unexpected extension: %s", page.Index, page.Extension) } } func TestExtractChapter_PagesHaveSequentialIndices(t *testing.T) { - chapter, err := ExtractChapter("../../testdata/Chapter 128.cbz") - if err != nil { - t.Fatalf("Failed to extract chapter: %v", err) - } + chapter, err := ExtractChapter(context.Background(), "../../testdata/Chapter 128.cbz") + require.NoError(t, err) defer func() { _ = chapter.Cleanup() }() for i, page := range chapter.Pages { - if page.Index != uint16(i) { - t.Errorf("Expected page index %d, got %d", i, page.Index) - } + assert.Equal(t, uint16(i), page.Index) } } func TestExtractChapter_Cleanup(t *testing.T) { - chapter, err := ExtractChapter("../../testdata/Chapter 128.cbz") - if err != nil { - t.Fatalf("Failed to extract chapter: %v", err) - } + chapter, err := ExtractChapter(context.Background(), "../../testdata/Chapter 128.cbz") + require.NoError(t, err) tempDir := chapter.TempDir // Verify temp dir exists - if _, err := os.Stat(tempDir); os.IsNotExist(err) { - t.Fatal("TempDir does not exist after extraction") - } + assert.DirExists(t, tempDir) // Cleanup should remove temp dir err = chapter.Cleanup() - if err != nil { - t.Fatalf("Cleanup failed: %v", err) - } + require.NoError(t, err) - if _, err := os.Stat(tempDir); !os.IsNotExist(err) { - t.Error("TempDir should not exist after cleanup") - } + assert.NoDirExists(t, tempDir) } func TestExtractChapter_CBR(t *testing.T) { - chapter, err := ExtractChapter("../../testdata/Chapter 1.cbr") - if err != nil { - t.Fatalf("Failed to extract CBR chapter: %v", err) - } + chapter, err := ExtractChapter(context.Background(), "../../testdata/Chapter 1.cbr") + require.NoError(t, err) defer func() { _ = chapter.Cleanup() }() - if len(chapter.Pages) != 16 { - t.Errorf("Expected 16 pages, got %d", len(chapter.Pages)) - } + assert.Len(t, chapter.Pages, 16) // All page files should exist on disk for _, page := range chapter.Pages { - if _, err := os.Stat(page.FilePath); os.IsNotExist(err) { - t.Errorf("Page file does not exist on disk: %s", page.FilePath) + assert.FileExists(t, page.FilePath) } - } } func TestExtractChapter_ConvertedStatus(t *testing.T) { - chapter, err := ExtractChapter("../../testdata/Chapter 10_converted.cbz") - if err != nil { - t.Fatalf("Failed to extract chapter: %v", err) - } + chapter, err := ExtractChapter(context.Background(), "../../testdata/Chapter 10_converted.cbz") + require.NoError(t, err) defer func() { _ = chapter.Cleanup() }() - if !chapter.IsConverted { - t.Error("Expected converted chapter to have IsConverted = true") - } - if chapter.ConvertedTime.IsZero() { - t.Error("Expected non-zero ConvertedTime for converted chapter") - } + assert.True(t, chapter.IsConverted, "Expected converted chapter to have IsConverted = true") + assert.False(t, chapter.ConvertedTime.IsZero(), "Expected non-zero ConvertedTime for converted chapter") } func TestWriteChapterToCBZ_EmptyChapter(t *testing.T) { @@ -298,15 +231,11 @@ func TestWriteChapterToCBZ_EmptyChapter(t *testing.T) { } err := WriteChapterToCBZ(chapter, outputPath) - if err != nil { - t.Fatalf("Should not error on empty chapter: %v", err) - } + require.NoError(t, err) // Verify the file is a valid zip r, err := zip.OpenReader(outputPath) - if err != nil { - t.Fatalf("Failed to open output CBZ: %v", err) - } + require.NoError(t, err) _ = r.Close() } @@ -331,15 +260,11 @@ func TestWriteChapterToCBZ_WithComicInfo(t *testing.T) { chapter.SetConverted() err := WriteChapterToCBZ(chapter, outputPath) - if err != nil { - t.Fatalf("Failed to write chapter: %v", err) - } + require.NoError(t, err) // Verify ComicInfo.xml is present r, err := zip.OpenReader(outputPath) - if err != nil { - t.Fatalf("Failed to open output CBZ: %v", err) - } + require.NoError(t, err) defer func() { _ = r.Close() }() foundComicInfo := false @@ -348,14 +273,10 @@ func TestWriteChapterToCBZ_WithComicInfo(t *testing.T) { foundComicInfo = true } } - if !foundComicInfo { - t.Error("ComicInfo.xml not found in output CBZ") - } + assert.True(t, foundComicInfo, "ComicInfo.xml not found in output CBZ") // Verify zip comment has converted timestamp - if r.Comment == "" { - t.Error("Expected zip comment with conversion timestamp") - } + assert.NotEmpty(t, r.Comment, "Expected zip comment with conversion timestamp") } func TestWriteChapterToCBZ_InvalidPagePath(t *testing.T) { @@ -371,9 +292,7 @@ func TestWriteChapterToCBZ_InvalidPagePath(t *testing.T) { } err := WriteChapterToCBZ(chapter, outputPath) - if err == nil { - t.Error("Expected error when page file does not exist") - } + require.Error(t, err) } func TestIsAlreadyConverted_CBZWithConvertedTxt(t *testing.T) { @@ -382,32 +301,22 @@ func TestIsAlreadyConverted_CBZWithConvertedTxt(t *testing.T) { cbzPath := filepath.Join(tmpDir, "test.cbz") f, err := os.Create(cbzPath) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) w := zip.NewWriter(f) // Add converted.txt with a date fw, err := w.Create("converted.txt") - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) _, _ = fw.Write([]byte(time.Now().Format(time.RFC3339))) // Add a dummy page fw, err = w.Create("page.webp") - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) _, _ = fw.Write([]byte("dummy")) _ = w.Close() _ = f.Close() - result, err := IsAlreadyConverted(cbzPath) - if err != nil { - t.Fatalf("Unexpected error: %v", err) - } - if !result { - t.Error("Expected CBZ with converted.txt to be detected as converted") - } + result, err := IsAlreadyConverted(context.Background(), cbzPath) + require.NoError(t, err) + assert.True(t, result, "Expected CBZ with converted.txt to be detected as converted") } func TestIsAlreadyConverted_CBZWithInvalidConvertedTxt(t *testing.T) { @@ -416,25 +325,17 @@ func TestIsAlreadyConverted_CBZWithInvalidConvertedTxt(t *testing.T) { cbzPath := filepath.Join(tmpDir, "test.cbz") f, err := os.Create(cbzPath) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) w := zip.NewWriter(f) fw, err := w.Create("converted.txt") - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) _, _ = fw.Write([]byte("not a valid date")) _ = w.Close() _ = f.Close() - result, err := IsAlreadyConverted(cbzPath) - if err != nil { - t.Fatalf("Unexpected error: %v", err) - } - if result { - t.Error("Expected CBZ with invalid date in converted.txt to NOT be detected as converted") - } + result, err := IsAlreadyConverted(context.Background(), cbzPath) + require.NoError(t, err) + assert.False(t, result, "Expected CBZ with invalid date in converted.txt to NOT be detected as converted") } func TestExtractChapter_WithConvertedTxt(t *testing.T) { @@ -443,36 +344,24 @@ func TestExtractChapter_WithConvertedTxt(t *testing.T) { cbzPath := filepath.Join(tmpDir, "test.cbz") f, err := os.Create(cbzPath) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) w := zip.NewWriter(f) fw, err := w.Create("converted.txt") - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) convertedTime := time.Date(2024, 6, 15, 12, 0, 0, 0, time.UTC) _, _ = fw.Write([]byte(convertedTime.Format(time.RFC3339))) fw, err = w.Create("page001.jpg") - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) _, _ = fw.Write([]byte("fake image")) _ = w.Close() _ = f.Close() - chapter, err := ExtractChapter(cbzPath) - if err != nil { - t.Fatalf("Failed to extract: %v", err) - } + chapter, err := ExtractChapter(context.Background(), cbzPath) + require.NoError(t, err) defer func() { _ = chapter.Cleanup() }() - if !chapter.IsConverted { - t.Error("Expected chapter with converted.txt to be marked as converted") - } - if chapter.ConvertedTime.IsZero() { - t.Error("Expected non-zero ConvertedTime") - } + assert.True(t, chapter.IsConverted, "Expected chapter with converted.txt to be marked as converted") + assert.False(t, chapter.ConvertedTime.IsZero(), "Expected non-zero ConvertedTime") } func TestWriteChapterToCBZ_MultiplePages(t *testing.T) { @@ -501,27 +390,19 @@ func TestWriteChapterToCBZ_MultiplePages(t *testing.T) { chapter.SetConverted() err := WriteChapterToCBZ(chapter, outputPath) - if err != nil { - t.Fatalf("Failed to write: %v", err) - } + require.NoError(t, err) // Verify archive contents r, err := zip.OpenReader(outputPath) - if err != nil { - t.Fatalf("Failed to open: %v", err) - } + require.NoError(t, err) defer func() { _ = r.Close() }() // Should have 5 pages = 5 files (conversion status stored in zip comment, not as file) expectedFiles := 5 - if len(r.File) != expectedFiles { - t.Errorf("Expected %d files in archive, got %d", expectedFiles, len(r.File)) - } + assert.Len(t, r.File, expectedFiles) // Verify zip comment is set - if r.Comment == "" { - t.Error("Expected zip comment for converted chapter") - } + assert.NotEmpty(t, r.Comment, "Expected zip comment for converted chapter") } func TestWriteChapterToCBZ_InvalidOutputPath(t *testing.T) { @@ -541,7 +422,5 @@ func TestWriteChapterToCBZ_InvalidOutputPath(t *testing.T) { } err := WriteChapterToCBZ(chapter, "/nonexistent/dir/output.cbz") - if err == nil { - t.Error("Expected error for invalid output path") - } + require.Error(t, err) } diff --git a/internal/utils/file_utils_test.go b/internal/utils/file_utils_test.go index 9eac597..2c3e868 100644 --- a/internal/utils/file_utils_test.go +++ b/internal/utils/file_utils_test.go @@ -4,6 +4,8 @@ import ( "os" "path/filepath" "testing" + + "github.com/stretchr/testify/assert" ) func TestIsValidFolder(t *testing.T) { @@ -51,9 +53,7 @@ func TestIsValidFolder(t *testing.T) { t.Run(tt.name, func(t *testing.T) { path := tt.setup(t) result := IsValidFolder(path) - if result != tt.expected { - t.Errorf("IsValidFolder(%q) = %v, want %v", path, result, tt.expected) - } + assert.Equal(t, tt.expected, result) }) } } diff --git a/internal/utils/optimize.go b/internal/utils/optimize.go index 3ba6a43..b0c380d 100644 --- a/internal/utils/optimize.go +++ b/internal/utils/optimize.go @@ -41,7 +41,7 @@ func Optimize(options *OptimizeOptions) error { Msg("Optimization parameters") // Step 1: Fast conversion check before extracting (new requirement) - alreadyConverted, err := cbz.IsAlreadyConverted(options.Path) + alreadyConverted, err := cbz.IsAlreadyConverted(context.Background(), options.Path) if err != nil { log.Debug().Str("file", options.Path).Err(err).Msg("Conversion check failed, proceeding with extraction") } @@ -52,10 +52,22 @@ func Optimize(options *OptimizeOptions) error { // Step 2: Extract chapter to disk log.Debug().Str("file", options.Path).Msg("Extracting chapter") - chapter, err := cbz.ExtractChapter(options.Path) + + // Create context for extraction (use timeout if configured) + var extractCtx context.Context + if options.Timeout > 0 { + var cancel context.CancelFunc + extractCtx, cancel = context.WithTimeout(context.Background(), options.Timeout) + defer cancel() + log.Debug().Str("file", options.Path).Dur("timeout", options.Timeout).Msg("Applying timeout") + } else { + extractCtx = context.Background() + } + + chapter, err := cbz.ExtractChapter(extractCtx, options.Path) if err != nil { log.Error().Str("file", options.Path).Err(err).Msg("Failed to extract chapter") - return fmt.Errorf("failed to load chapter: %v", err) + return fmt.Errorf("failed to extract chapter: %w", err) } defer func() { if cleanupErr := chapter.Cleanup(); cleanupErr != nil { @@ -75,17 +87,7 @@ func Optimize(options *OptimizeOptions) error { Msg("Chapter extracted successfully") // Step 3: Convert pages file-to-file - var ctx context.Context - if options.Timeout > 0 { - var cancel context.CancelFunc - ctx, cancel = context.WithTimeout(context.Background(), options.Timeout) - defer cancel() - log.Debug().Str("file", options.Path).Dur("timeout", options.Timeout).Msg("Applying timeout") - } else { - ctx = context.Background() - } - - convertedChapter, err := options.ChapterConverter.ConvertChapter(ctx, chapter, options.Quality, options.Split, func(msg string, current uint32, total uint32) { + convertedChapter, err := options.ChapterConverter.ConvertChapter(extractCtx, chapter, options.Quality, options.Split, func(msg string, current uint32, total uint32) { if current%10 == 0 || current == total { log.Info().Str("file", chapter.FilePath).Uint32("current", current).Uint32("total", total).Msg("Converting") } else { @@ -98,7 +100,7 @@ func Optimize(options *OptimizeOptions) error { log.Debug().Str("file", chapter.FilePath).Err(err).Msg("Page conversion error (non-fatal)") } else { log.Error().Str("file", chapter.FilePath).Err(err).Msg("Chapter conversion failed") - return fmt.Errorf("failed to convert chapter: %v", err) + return fmt.Errorf("failed to convert chapter: %w", err) } } if convertedChapter == nil { @@ -140,7 +142,7 @@ func Optimize(options *OptimizeOptions) error { err = cbz.WriteChapterToCBZ(convertedChapter, outputPath) if err != nil { log.Error().Str("output_path", outputPath).Err(err).Msg("Failed to write converted chapter") - return fmt.Errorf("failed to write converted chapter: %v", err) + return fmt.Errorf("failed to write converted chapter: %w", err) } // If overriding a CBR file, delete the original diff --git a/pkg/converter/webp/webp_converter.go b/pkg/converter/webp/webp_converter.go index 5359de6..bf04d1a 100644 --- a/pkg/converter/webp/webp_converter.go +++ b/pkg/converter/webp/webp_converter.go @@ -74,6 +74,11 @@ func (converter *Converter) ConvertChapter(ctx context.Context, chapter *manga.C return nil, err } + // Validate TempDir is set to prevent writing to cwd + if chapter.TempDir == "" { + return nil, fmt.Errorf("chapter TempDir is empty, cannot create output directory") + } + // Create output directory for converted files outputDir := filepath.Join(chapter.TempDir, "output") if err := os.MkdirAll(outputDir, 0755); err != nil { @@ -147,25 +152,36 @@ func (converter *Converter) ConvertChapter(ctx context.Context, chapter *manga.C return nil, ctx.Err() } - // Collect results + // Collect results — separate fatal errors from page-ignored errors var convertedPages []*manga.PageFile - var errList []error + var fatalErrors []error + var ignoredErrors []error for _, result := range results { if result.err != nil { if errors.Is(result.err, context.DeadlineExceeded) || errors.Is(result.err, context.Canceled) { return nil, result.err } - errList = append(errList, result.err) + var pageIgnored *converterrors.PageIgnoredError + if errors.As(result.err, &pageIgnored) { + ignoredErrors = append(ignoredErrors, result.err) + } else { + fatalErrors = append(fatalErrors, result.err) + } } if result.pages != nil { convertedPages = append(convertedPages, result.pages...) } } + // Fatal errors take priority over ignored-page errors + if len(fatalErrors) > 0 { + return nil, errors.Join(fatalErrors...) + } + if len(convertedPages) == 0 { - if len(errList) > 0 { - return nil, errors.Join(errList...) + if len(ignoredErrors) > 0 { + return nil, errors.Join(ignoredErrors...) } return nil, fmt.Errorf("no pages were converted") } @@ -182,8 +198,8 @@ func (converter *Converter) ConvertChapter(ctx context.Context, chapter *manga.C chapter.Pages = convertedPages var aggregatedError error - if len(errList) > 0 { - aggregatedError = errors.Join(errList...) + if len(ignoredErrors) > 0 { + aggregatedError = errors.Join(ignoredErrors...) } log.Debug().