refactor: audit, split server.go, add unit tests, and fix latent bugs

- Remove dead code: browser cdp/content_scrape strategies, writer package, printUsage, downloadAndStoreCoverCLI in main.go - Fix bugs: defer-in-loop in pocketbase deleteWhere, listAll() pagination hard cap removed, splitChapterTitle off-by-one in date extraction - Split server.go (~1700 lines) into focused handler files: handlers_audio, handlers_browse, handlers_progress, handlers_ranking, handlers_scrape - Export htmlutil.AttrVal/TextContent/ResolveURL; add storage/coverutil.go to consolidate duplicate helpers - Flatten deeply nested conditionals: voices() early-return guards, ScrapeCatalogue next-link double attr scan, chapterNumberFromKey dead strings.Cut line, splitChapterTitle double-nested unit/suffix loop - Add unit tests: htmlutil (9 funcs), novelfire ScrapeMetadata (3 cases), orchestrator Run (5 cases), storage chapterNumberFromKey/splitChapterTitle (22 cases); all pass with go build/vet/test clean
2026-03-04 22:14:23 +05:00
parent 7b48707cd9
commit fb6b364382
26 changed files with 2359 additions and 2392 deletions
--- a/scraper/internal/storage/coverutil.go
+++ b/scraper/internal/storage/coverutil.go
@@ -0,0 +1,59 @@
+package storage
+
+import (
+	"context"
+	"fmt"
+	"io"
+	"log/slog"
+	"net/http"
+	"time"
+)
+
+// DownloadAndStoreCover fetches the image at imageURL and stores it in the
+// store under key. Errors are logged but not returned — this is best-effort.
+// If the asset is already present the download is skipped.
+func DownloadAndStoreCover(store Store, log *slog.Logger, key, imageURL string) {
+	ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
+	defer cancel()
+
+	// Skip if already stored.
+	if _, _, ok, _ := store.GetBrowseAsset(ctx, key); ok {
+		return
+	}
+
+	req, err := http.NewRequestWithContext(ctx, http.MethodGet, imageURL, nil)
+	if err != nil {
+		log.Warn("cover: build request failed", "key", key, "url", imageURL, "err", err)
+		return
+	}
+	req.Header.Set("User-Agent", "Mozilla/5.0 (compatible; libnovel-scraper/1.0)")
+
+	resp, err := http.DefaultClient.Do(req)
+	if err != nil {
+		log.Warn("cover: fetch failed", "key", key, "url", imageURL, "err", fmt.Errorf("%w", err))
+		return
+	}
+	defer resp.Body.Close()
+
+	if resp.StatusCode != http.StatusOK {
+		log.Warn("cover: non-200 response", "key", key, "url", imageURL, "status", resp.StatusCode)
+		return
+	}
+
+	data, err := io.ReadAll(resp.Body)
+	if err != nil {
+		log.Warn("cover: read body failed", "key", key, "url", imageURL, "err", err)
+		return
+	}
+
+	contentType := resp.Header.Get("Content-Type")
+	if contentType == "" {
+		contentType = "image/jpeg"
+	}
+
+	if err := store.SaveBrowseAsset(ctx, key, data, contentType); err != nil {
+		log.Warn("cover: SaveBrowseAsset failed", "key", key, "err", err)
+		return
+	}
+	log.Debug("cover: stored", "key", key, "bytes", len(data))
+}
--- a/scraper/internal/storage/hybrid.go
+++ b/scraper/internal/storage/hybrid.go
@@ -193,7 +193,6 @@ func (h *HybridStore) ReindexChapters(ctx context.Context, slug string) (int, er
 // form "{slug}/vol-N/lo-hi/chapter-N.md".
 func chapterNumberFromKey(key string) int {
 	// Grab the filename portion after the last '/'.
-	_, file, _ := strings.Cut(key, "/")
 	parts := strings.Split(key, "/")
 	if len(parts) == 0 {
 		return 0
@@ -206,7 +205,6 @@ func chapterNumberFromKey(key string) int {
 	if err != nil || n <= 0 {
 		return 0
 	}
-	_ = file
 	return n
 }

@@ -443,24 +441,35 @@ func splitChapterTitle(raw string) (title, date string) {
 			raw = strings.TrimSpace(raw[idx:])
 		}
 	}
-	// Detect trailing relative date.
+	// Detect trailing relative date.  Build a flat list of all suffixes once
+	// to avoid a double-nested loop.
 	units := []string{"second", "minute", "hour", "day", "week", "month", "year"}
-	lower := strings.ToLower(raw)
+	suffixes := make([]string, 0, len(units)*2)
 	for _, u := range units {
-		for _, suffix := range []string{u + "s ago", u + " ago"} {
-			if idx := strings.LastIndex(lower, suffix); idx > 0 {
-				// Find start of date token (digit before the unit).
-				start := strings.LastIndex(raw[:idx], " ")
-				if start < 0 {
-					start = 0
-				}
-				numPart := strings.TrimSpace(raw[start:idx])
-				fields := strings.Fields(numPart)
-				if len(fields) > 0 {
-					if _, err := strconv.Atoi(fields[0]); err == nil {
-						return strings.TrimSpace(raw[:start]), strings.TrimSpace(raw[start : idx+len(suffix)])
-					}
-				}
+		suffixes = append(suffixes, u+"s ago", u+" ago")
+	}
+	lower := strings.ToLower(raw)
+	for _, suffix := range suffixes {
+		idx := strings.LastIndex(lower, suffix)
+		if idx <= 0 {
+			continue
+		}
+		// Find start of the numeric token that precedes the unit.
+		// Strip any whitespace that separates the number from the unit so
+		// that LastIndex finds the space before the digit, not the one
+		// between the digit and the unit word.
+		before := strings.TrimRight(raw[:idx], " \t")
+		start := strings.LastIndex(before, " ")
+		if start < 0 {
+			start = 0
+		} else {
+			start++ // advance past the space to point at the digit
+		}
+		numPart := strings.TrimSpace(raw[start:idx])
+		fields := strings.Fields(numPart)
+		if len(fields) > 0 {
+			if _, err := strconv.Atoi(fields[0]); err == nil {
+				return strings.TrimSpace(raw[:start]), strings.TrimSpace(raw[start : idx+len(suffix)])
 			}
 		}
 	}
--- a/scraper/internal/storage/hybrid_integration_test.go
+++ b/scraper/internal/storage/hybrid_integration_test.go
@@ -13,6 +13,7 @@ package storage
 import (
 	"context"
 	"fmt"
+	"log/slog"
 	"strings"
 	"testing"
 	"time"
@@ -48,7 +49,7 @@ func newTestHybridStore(t *testing.T) *HybridStore {
 	ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
 	defer cancel()

-	hs, err := NewHybridStore(ctx, pbCfg, minioCfg)
+	hs, err := NewHybridStore(ctx, pbCfg, minioCfg, slog.Default())
 	if err != nil {
 		t.Fatalf("NewHybridStore: %v", err)
 	}
@@ -418,7 +419,7 @@ func TestHybridStore_PresignAudio(t *testing.T) {
 	ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
 	defer cancel()

-	key := hs.AudioObjectKey(slug, 1, "af_bella", 1.0)
+	key := hs.AudioObjectKey(slug, 1, "af_bella")
 	fakeAudio := []byte("ID3\x03\x00\x00\x00\x00\x00\x00hybrid-presign-audio-test")

 	if err := hs.minio.PutAudio(ctx, key, fakeAudio); err != nil {
@@ -470,10 +471,3 @@ func TestHybridStore_AudioCache(t *testing.T) {
 }

 // ─── helpers ──────────────────────────────────────────────────────────────────
-
-func min(a, b int) int {
-	if a < b {
-		return a
-	}
-	return b
-}
--- a/scraper/internal/storage/hybrid_unit_test.go
+++ b/scraper/internal/storage/hybrid_unit_test.go
@@ -0,0 +1,77 @@
+package storage
+
+import (
+	"testing"
+)
+
+// ── chapterNumberFromKey ──────────────────────────────────────────────────────
+
+func TestChapterNumberFromKey(t *testing.T) {
+	cases := []struct {
+		key  string
+		want int
+	}{
+		// Standard four-segment key.
+		{"my-novel/vol-0/1-50/chapter-1.md", 1},
+		{"my-novel/vol-0/1-50/chapter-42.md", 42},
+		{"my-novel/vol-0/51-100/chapter-99.md", 99},
+		// Large chapter numbers.
+		{"some-novel/vol-1/1001-1050/chapter-1024.md", 1024},
+		// Nested deeper paths should still work (last segment used).
+		{"a/b/c/d/chapter-7.md", 7},
+		// Malformed / unexpected inputs — should return 0 without panicking.
+		{"chapter-notanumber.md", 0},
+		{"", 0},
+		// No .md extension — TrimSuffix is a no-op; TrimPrefix still strips
+		// "chapter-", so the number is parsed successfully.
+		{"no-md-extension/chapter-5", 5},
+		{"my-novel/vol-0/1-50/chapter-0.md", 0}, // 0 is invalid (chapters are 1-based)
+		{"my-novel/vol-0/1-50/chapter--1.md", 0},
+	}
+
+	for _, tc := range cases {
+		got := chapterNumberFromKey(tc.key)
+		if got != tc.want {
+			t.Errorf("chapterNumberFromKey(%q) = %d, want %d", tc.key, got, tc.want)
+		}
+	}
+}
+
+// ── splitChapterTitle ─────────────────────────────────────────────────────────
+
+func TestSplitChapterTitle(t *testing.T) {
+	cases := []struct {
+		raw       string
+		wantTitle string
+		wantDate  string
+	}{
+		// No date — title is returned as-is.
+		{"The Great Battle", "The Great Battle", ""},
+		// Leading numeric index is stripped.
+		{"42 The Great Battle", "The Great Battle", ""},
+		// Relative date with plural unit.
+		{"The Storm Arrives 3 days ago", "The Storm Arrives", "3 days ago"},
+		// Singular unit.
+		{"A New Hope 1 week ago", "A New Hope", "1 week ago"},
+		// Minutes and seconds.
+		{"Flash Fight 5 minutes ago", "Flash Fight", "5 minutes ago"},
+		{"Quick Strike 30 seconds ago", "Quick Strike", "30 seconds ago"},
+		// Months and years.
+		{"Old Chapter 2 months ago", "Old Chapter", "2 months ago"},
+		{"Ancient Story 1 year ago", "Ancient Story", "1 year ago"},
+		// Leading index AND trailing date.
+		{"5 The Final Chapter 2 hours ago", "The Final Chapter", "2 hours ago"},
+		// Extra whitespace.
+		{"  The Calm  ", "The Calm", ""},
+		// Empty string.
+		{"", "", ""},
+	}
+
+	for _, tc := range cases {
+		title, date := splitChapterTitle(tc.raw)
+		if title != tc.wantTitle || date != tc.wantDate {
+			t.Errorf("splitChapterTitle(%q) = (%q, %q), want (%q, %q)",
+				tc.raw, title, date, tc.wantTitle, tc.wantDate)
+		}
+	}
+}
--- a/scraper/internal/storage/integration_test.go
+++ b/scraper/internal/storage/integration_test.go
@@ -16,6 +16,7 @@ package storage
 import (
 	"context"
 	"fmt"
+	"log/slog"
 	"os"
 	"strings"
 	"testing"
@@ -66,7 +67,7 @@ func newTestPocketBaseStore(t *testing.T) *PocketBaseStore {
 		AdminEmail:    envOr("POCKETBASE_ADMIN_EMAIL", "admin@libnovel.local"),
 		AdminPassword: envOr("POCKETBASE_ADMIN_PASSWORD", "changeme123"),
 	}
-	store := NewPocketBaseStore(cfg)
+	store := NewPocketBaseStore(cfg, slog.Default())
 	ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second)
 	defer cancel()
 	if err := store.EnsureCollections(ctx); err != nil {
@@ -216,7 +217,7 @@ func TestMinioClient_PresignChapter(t *testing.T) {
 func TestMinioClient_AudioRoundTrip(t *testing.T) {
 	mc := newTestMinioClient(t)
 	slug := testSlug(t)
-	key := AudioObjectKey(slug, 1, "af_bella", 1.0)
+	key := AudioObjectKey(slug, 1, "af_bella")

 	// Use minimal fake MP3 bytes (just a recognisable prefix).
 	fakeAudio := []byte("ID3\x03\x00\x00\x00\x00\x00\x00integration-test-audio")
@@ -256,7 +257,7 @@ func TestMinioClient_AudioRoundTrip(t *testing.T) {
 func TestMinioClient_PresignAudio(t *testing.T) {
 	mc := newTestMinioClient(t)
 	slug := testSlug(t)
-	key := AudioObjectKey(slug, 1, "af_bella", 1.0)
+	key := AudioObjectKey(slug, 1, "af_bella")
 	fakeAudio := []byte("ID3\x03\x00\x00\x00\x00\x00\x00presign-audio-test")

 	ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
--- a/scraper/internal/storage/pocketbase.go
+++ b/scraper/internal/storage/pocketbase.go
@@ -163,33 +163,48 @@ func (p *pbClient) listOne(ctx context.Context, collection, filter string) (map[
 	return result.Items[0], nil
 }

-// listAll returns all records (up to 500) from a collection matching filter.
+// listAll returns all records from a collection matching filter by paginating
+// through all pages (PocketBase default page size is capped at 500).
 func (p *pbClient) listAll(ctx context.Context, collection, filter, sort string) ([]map[string]interface{}, error) {
-	q := url.Values{}
-	if filter != "" {
-		q.Set("filter", filter)
+	const perPage = 500
+	var all []map[string]interface{}
+
+	for page := 1; ; page++ {
+		q := url.Values{}
+		if filter != "" {
+			q.Set("filter", filter)
+		}
+		if sort != "" {
+			q.Set("sort", sort)
+		}
+		q.Set("perPage", fmt.Sprintf("%d", perPage))
+		q.Set("page", fmt.Sprintf("%d", page))
+		path := fmt.Sprintf("/api/collections/%s/records?%s", collection, q.Encode())
+		resp, err := p.do(ctx, http.MethodGet, path, nil)
+		if err != nil {
+			return nil, err
+		}
+		if resp.StatusCode != http.StatusOK {
+			b, _ := io.ReadAll(resp.Body)
+			resp.Body.Close()
+			return nil, fmt.Errorf("pocketbase: listAll %s: status %d: %s", collection, resp.StatusCode, b)
+		}
+		var result struct {
+			Page       int                      `json:"page"`
+			TotalPages int                      `json:"totalPages"`
+			Items      []map[string]interface{} `json:"items"`
+		}
+		if err := json.NewDecoder(resp.Body).Decode(&result); err != nil {
+			resp.Body.Close()
+			return nil, fmt.Errorf("pocketbase: listAll %s: decode: %w", collection, err)
+		}
+		resp.Body.Close()
+		all = append(all, result.Items...)
+		if page >= result.TotalPages || len(result.Items) == 0 {
+			break
+		}
 	}
-	if sort != "" {
-		q.Set("sort", sort)
-	}
-	q.Set("perPage", "500")
-	path := fmt.Sprintf("/api/collections/%s/records?%s", collection, q.Encode())
-	resp, err := p.do(ctx, http.MethodGet, path, nil)
-	if err != nil {
-		return nil, err
-	}
-	defer resp.Body.Close()
-	if resp.StatusCode != http.StatusOK {
-		b, _ := io.ReadAll(resp.Body)
-		return nil, fmt.Errorf("pocketbase: listAll %s: status %d: %s", collection, resp.StatusCode, b)
-	}
-	var result struct {
-		Items []map[string]interface{} `json:"items"`
-	}
-	if err := json.NewDecoder(resp.Body).Decode(&result); err != nil {
-		return nil, fmt.Errorf("pocketbase: listAll %s: decode: %w", collection, err)
-	}
-	return result.Items, nil
+	return all, nil
 }

 // upsert creates a record; if one matching filter already exists it updates it.
@@ -238,11 +253,12 @@ func (p *pbClient) deleteWhere(ctx context.Context, collection, filter string) e
 		if err != nil {
 			return err
 		}
-		defer resp.Body.Close()
 		if resp.StatusCode != http.StatusNoContent && resp.StatusCode != http.StatusOK {
 			b, _ := io.ReadAll(resp.Body)
+			resp.Body.Close()
 			return fmt.Errorf("pocketbase: deleteWhere %s id=%s: status %d: %s", collection, id, resp.StatusCode, b)
 		}
+		resp.Body.Close()
 	}
 	return nil
 }