diff --git a/cmd/upbrr/interactive.go b/cmd/upbrr/interactive.go index 4459bb57..7253cb36 100644 --- a/cmd/upbrr/interactive.go +++ b/cmd/upbrr/interactive.go @@ -958,7 +958,8 @@ func runCLIScreenshotHandling(ctx context.Context, coreSvc api.Core, req api.Req } finalImages := mergeScreenshotImages(nil, plan.FinalSelections) - if len(plan.SuggestedSelections) == 0 { + pending := pendingScreenshotSelections(plan) + if len(pending) == 0 { if len(finalImages) > 0 { return nil } @@ -972,7 +973,7 @@ func runCLIScreenshotHandling(ctx context.Context, coreSvc api.Core, req api.Req return nil } - result, err := coreSvc.GenerateScreenshots(ctx, req, plan.SuggestedSelections, api.ScreenshotPurposeFinal) + result, err := coreSvc.GenerateScreenshots(ctx, req, pending, api.ScreenshotPurposeFinal) if err != nil { return fmt.Errorf("upbrr: generate screenshots: %w", err) } @@ -992,6 +993,26 @@ func runCLIScreenshotHandling(ctx context.Context, coreSvc api.Core, req api.Req return nil } +// pendingScreenshotSelections drops baseline slots that already exist on disk. +// Plan reports the full baseline, so each final-capture path filters its own. +func pendingScreenshotSelections(plan api.ScreenshotPlan) []api.ScreenshotSelection { + if len(plan.ExistingScreenshots) == 0 { + return plan.SuggestedSelections + } + existing := make(map[int]struct{}, len(plan.ExistingScreenshots)) + for _, image := range plan.ExistingScreenshots { + existing[image.Index] = struct{}{} + } + pending := make([]api.ScreenshotSelection, 0, len(plan.SuggestedSelections)) + for _, selection := range plan.SuggestedSelections { + if _, ok := existing[selection.Index]; ok { + continue + } + pending = append(pending, selection) + } + return pending +} + func mergeScreenshotImages(base []api.ScreenshotImage, extra []api.ScreenshotImage) []api.ScreenshotImage { if len(extra) == 0 { return base diff --git a/cmd/upbrr/interactive_test.go b/cmd/upbrr/interactive_test.go index 901b74a4..48a82be6 100644 --- a/cmd/upbrr/interactive_test.go +++ b/cmd/upbrr/interactive_test.go @@ -11,6 +11,7 @@ import ( "maps" "os" "path/filepath" + "slices" "strings" "testing" "time" @@ -2261,3 +2262,47 @@ func (c *cliCoreForTest) SaveDescriptionOverride(_ context.Context, req api.Requ func (c *cliCoreForTest) Close() error { return nil } + +func TestPendingScreenshotSelectionsSkipsExistingSlots(t *testing.T) { + t.Parallel() + + plan := api.ScreenshotPlan{ + SuggestedSelections: []api.ScreenshotSelection{ + {Index: 0}, {Index: 1}, {Index: 2}, {Index: 3}, + }, + ExistingScreenshots: []api.ScreenshotImage{ + {Index: 1}, {Index: 2}, + }, + } + + indices := func(selections []api.ScreenshotSelection) []int { + out := make([]int, 0, len(selections)) + for _, selection := range selections { + out = append(out, selection.Index) + } + return out + } + + if got := indices(pendingScreenshotSelections(plan)); !slices.Equal(got, []int{0, 3}) { + t.Fatalf("expected only slots 0 and 3 pending, got %v", got) + } + + plan.ExistingScreenshots = nil + if got := indices(pendingScreenshotSelections(plan)); !slices.Equal(got, []int{0, 1, 2, 3}) { + t.Fatalf("expected all 4 slots pending when nothing exists, got %v", got) + } + + // Every slot captured: the CLI must find nothing to do rather than reshoot. + plan.ExistingScreenshots = []api.ScreenshotImage{{Index: 0}, {Index: 1}, {Index: 2}, {Index: 3}} + if got := pendingScreenshotSelections(plan); len(got) != 0 { + t.Fatalf("expected no pending slots when every slot is captured, got %v", indices(got)) + } + + // Plan already excludes captures that drifted off their slot, so a stale file + // never reaches ExistingScreenshots and its slot stays pending. + plan.SuggestedSelections = []api.ScreenshotSelection{{Index: 0}, {Index: 1}} + plan.ExistingScreenshots = []api.ScreenshotImage{{Index: 0}} + if got := indices(pendingScreenshotSelections(plan)); !slices.Equal(got, []int{1}) { + t.Fatalf("expected the uncaptured slot pending, got %v", got) + } +} diff --git a/gui/frontend/src/app.tsx b/gui/frontend/src/app.tsx index 563c028c..e5b199f1 100644 --- a/gui/frontend/src/app.tsx +++ b/gui/frontend/src/app.tsx @@ -88,6 +88,12 @@ import { import { hasFetchedExternalPreviewData } from "./utils/externalPreview"; import { handleExternalLinkClick } from "./utils/externalLinks"; import { normalizeJobStatus } from "./utils/jobStatus"; +import { + buildExistingSlotImages, + pendingSelections, + selectionTimestamp, + staleSlotPaths, +} from "./utils/screenshotSlots"; import { isMetadataProgressPathMatch } from "./utils/metadataProgress"; import { dupeSkipReason, isRuleSkippedResult } from "./utils/dupeCheck"; import { @@ -1797,17 +1803,42 @@ export default function App() { ); // Helper functions for screenshot management (not in the hook) - const handleDeleteExistingImage = (image: ScreenshotImage) => { + const handleDeleteExistingImage = async (image: ScreenshotImage) => { if (!image.Path) return; + if (!globalThis.confirm("Delete this image from the temp folder?")) { + return; + } const deletedPath = image.Path; + // Only prune local state once the file is actually gone, or it returns on reload. + if (!(await screenshots.deleteScreenshotFile(deletedPath))) { + return; + } screenshots.setExistingImages((prev) => prev.filter((entry) => entry.image.Path !== deletedPath), ); + screenshots.setExistingTrackerImages((prev) => + prev.filter((entry) => entry.image.Path !== deletedPath), + ); + screenshots.setPreviewImages((prev) => + prev.filter((entry) => entry.image.Path !== deletedPath), + ); if (screenshots.finalImagesRef.current.length > 0) { - screenshots.saveFinalSelections( + await screenshots.saveFinalSelections( screenshots.finalImagesRef.current.filter((entry) => entry.image.Path !== deletedPath), ); } + screenshots.setScreenshotPlan((prev) => { + if (!prev) return prev; + return { + ...prev, + ExistingScreenshots: (prev.ExistingScreenshots || []).filter( + (entry) => entry.Path !== deletedPath, + ), + ExistingTrackerScreenshots: (prev.ExistingTrackerScreenshots || []).filter( + (entry) => entry.Path !== deletedPath, + ), + }; + }); }; const mergeFinalSelections = ( @@ -1882,15 +1913,8 @@ export default function App() { return { selections: nextSelections, targetIndex: resolvedIndex }; }; - const normalizeSelectionTimestamp = (entry: ScreenshotSelection) => { - if (Number.isFinite(entry.TimestampSeconds) && entry.TimestampSeconds > 0) { - return entry.TimestampSeconds; - } - if (Number.isFinite(entry.Frame) && entry.Frame > 0) { - return entry.Frame / previewFrameRate; - } - return 0; - }; + const normalizeSelectionTimestamp = (entry: ScreenshotSelection) => + selectionTimestamp(entry, previewFrameRate); const desiredScreenCount = () => { if ( @@ -3034,21 +3058,11 @@ export default function App() { }); const candidates = autoSelections.length > 0 ? autoSelections : baseSelections; - const resolveTimestamp = (entry: ScreenshotSelection) => { - if (Number.isFinite(entry.TimestampSeconds) && entry.TimestampSeconds > 0) { - return entry.TimestampSeconds; - } - if (Number.isFinite(entry.Frame) && entry.Frame > 0) { - return entry.Frame / previewFrameRate; - } - return 0; - }; - const closest = candidates.reduce( (best, entry) => { - const currentDiff = Math.abs(resolveTimestamp(entry) - timestamp); + const currentDiff = Math.abs(normalizeSelectionTimestamp(entry) - timestamp); if (!best) return entry; - const bestDiff = Math.abs(resolveTimestamp(best) - timestamp); + const bestDiff = Math.abs(normalizeSelectionTimestamp(best) - timestamp); if (currentDiff < bestDiff) return entry; return best; }, @@ -3077,7 +3091,7 @@ export default function App() { const manualSelection = reindexed.selections.find((entry) => { const source = (entry.Source || "").toLowerCase(); if (source !== "manual") return false; - const ts = resolveTimestamp(entry); + const ts = normalizeSelectionTimestamp(entry); if (!Number.isFinite(ts) || ts <= 0) return false; return Math.abs(ts - timestamp) <= tolerance; }); @@ -3125,20 +3139,18 @@ export default function App() { } }; - const buildExistingSelectionIndexSet = () => { - const indices = new Set(); - const addImages = (images: ScreenshotImage[] | undefined) => { - if (!images || images.length === 0) return; - images.forEach((image) => { - if (Number.isFinite(image.Index)) { - indices.add(image.Index); - } - }); - }; - addImages(screenshots.screenshotPlan?.ExistingScreenshots); - addImages(screenshots.existingImages.map((entry) => entry.image)); - addImages(screenshots.finalImagesRef.current.map((entry) => entry.image)); - return indices; + // Slot -> image currently filling it. A freshly loaded plan is passed in because + // setScreenshotPlan has not flushed yet. + const existingSlotImages = (plan?: ScreenshotPlan | null) => { + const finalPaths = new Set( + screenshots.finalImagesRef.current.map((entry) => entry.image.Path).filter(Boolean), + ); + return buildExistingSlotImages( + (plan ?? screenshots.screenshotPlan)?.ExistingScreenshots, + screenshots.previewImages + .map((entry) => entry.image) + .filter((image) => image.Path && finalPaths.has(image.Path)), + ); }; const handleGenerateScreenshots = async () => { @@ -3148,16 +3160,17 @@ export default function App() { return; } let selections = screenshots.screenshotSelections; + let loadedPlan: ScreenshotPlan | null = null; if (selections.length === 0) { - const plan = await screenshots.loadScreenshotPlan(false); - selections = plan?.SuggestedSelections || []; + loadedPlan = await screenshots.loadScreenshotPlan(false); + selections = loadedPlan?.SuggestedSelections || []; } if (selections.length === 0) { screenshots.setScreenshotsError("No screenshot selections available."); return; } - const existingIndices = buildExistingSelectionIndexSet(); - const filteredSelections = selections.filter((entry) => !existingIndices.has(entry.Index)); + const existingSlots = existingSlotImages(loadedPlan); + const filteredSelections = pendingSelections(selections, existingSlots, previewFrameRate); if (filteredSelections.length === 0) { screenshots.setScreenshotsError("All requested screenshots already exist."); return; @@ -3167,9 +3180,29 @@ export default function App() { const result = await runScreenshotCapture(filteredSelections, "final"); screenshots.setFinalResult(result); const images = result.Images || []; + + const capturedPaths = new Set(images.map((image) => image.Path)); + const stalePaths = staleSlotPaths(images, existingSlots); + for (const stalePath of stalePaths) { + await screenshots.deleteScreenshotFile(stalePath); + } + const previews = await Promise.all(images.map(screenshots.readScreenshotImage)); - const merged = mergeFinalSelections(screenshots.finalImagesRef.current, previews); - await screenshots.saveFinalSelections(merged); + const kept = screenshots.finalImagesRef.current.filter( + (entry) => !stalePaths.has(entry.image.Path), + ); + await screenshots.saveFinalSelections(mergeFinalSelections(kept, previews)); + screenshots.setPreviewImages((prev) => + prev.filter((entry) => !stalePaths.has(entry.image.Path)), + ); + // Record the captures so a repeat click skips them instead of re-shooting. + screenshots.setScreenshotPlan((prev) => { + if (!prev) return prev; + const retained = (prev.ExistingScreenshots || []).filter( + (entry) => !stalePaths.has(entry.Path) && !capturedPaths.has(entry.Path), + ); + return { ...prev, ExistingScreenshots: [...retained, ...images] }; + }); } catch (err) { screenshots.setScreenshotsError(String(err)); } finally { diff --git a/gui/frontend/src/hooks/useScreenshots.ts b/gui/frontend/src/hooks/useScreenshots.ts index 757fe781..764eb64d 100644 --- a/gui/frontend/src/hooks/useScreenshots.ts +++ b/gui/frontend/src/hooks/useScreenshots.ts @@ -151,7 +151,12 @@ export const useScreenshots = ({ setExistingImages([]); setExistingTrackerImages([]); - setPreviewImages([]); + // Keep previews still held by final selections; they carry the slot Index. + setPreviewImages((prev) => + prev.filter((entry) => + finalImagesRef.current.some((final) => final.image.Path === entry.image.Path), + ), + ); setFinalResult(null); // Load existing screenshot previews @@ -439,11 +444,16 @@ export const useScreenshots = ({ [path, idOverrideState, releaseOverrideState], ); - const deleteTrackerImageFile = useCallback( - async (imagePath: string) => { + // Delete one screenshot file; true only when it is gone from disk. + const deleteScreenshotFile = useCallback( + async (imagePath: string): Promise => { const deleter = globalThis.go?.guiapp?.App?.DeleteScreenshot; - if (!deleter || !path.trim() || !imagePath) { - return; + if (!deleter) { + setScreenshotsError("Deleting screenshots is unavailable in this build."); + return false; + } + if (!path.trim() || !imagePath) { + return false; } try { await deleter( @@ -452,8 +462,10 @@ export const useScreenshots = ({ normalizeReleaseOverrides(releaseOverrideState?.overrides || {}), imagePath, ); + return true; } catch (err) { setScreenshotsError(String(err)); + return false; } }, [path, idOverrideState, releaseOverrideState], @@ -564,7 +576,12 @@ export const useScreenshots = ({ if (deleted.length === 0) return; const deletedPaths = new Set(deleted.map((image) => image.Path)); setPreviewImages((prev) => prev.filter((entry) => !deletedPaths.has(entry.image.Path))); - }, [previewImages, deleteImageSet]); + if (finalImagesRef.current.some((entry) => deletedPaths.has(entry.image.Path))) { + await saveFinalSelections( + finalImagesRef.current.filter((entry) => !deletedPaths.has(entry.image.Path)), + ); + } + }, [previewImages, deleteImageSet, saveFinalSelections]); // Delete all final images const handleDeleteAllFinalImages = useCallback(async () => { @@ -620,7 +637,7 @@ export const useScreenshots = ({ const linked = trackerLinkByURL.get(url); let removedFinal = false; if (linked?.Path) { - await deleteTrackerImageFile(linked.Path); + await deleteScreenshotFile(linked.Path); } await deleteTrackerImageURL(url); removeTrackerImageURLState(url); @@ -634,7 +651,7 @@ export const useScreenshots = ({ removeTrackerImageURLState, trackerLinkByURL, removeFinalSelectionInternal, - deleteTrackerImageFile, + deleteScreenshotFile, ], ); @@ -799,6 +816,7 @@ export const useScreenshots = ({ addFinalSelection, removeFinalSelection, reorderFinalSelections, + deleteScreenshotFile, handleDeleteAllExistingImages, handleDeleteAllTrackerImages, handleDeleteAllPreviewImages, diff --git a/gui/frontend/src/pages/screenshots/index.tsx b/gui/frontend/src/pages/screenshots/index.tsx index 65fc6faa..488a9f06 100644 --- a/gui/frontend/src/pages/screenshots/index.tsx +++ b/gui/frontend/src/pages/screenshots/index.tsx @@ -52,7 +52,7 @@ type Props = Readonly<{ handleDeleteAllExistingImages: () => void; existingTrackerImages: ScreenshotPreviewImage[]; handleDeleteAllTrackerImages: () => void; - handleDeleteExistingImage: (image: ScreenshotImage) => void; + handleDeleteExistingImage: (image: ScreenshotImage) => Promise; showFrameSelections: boolean; screenshotSelections: ScreenshotSelection[]; updateSelectionTime: (index: number, value: string) => void; diff --git a/gui/frontend/src/utils/screenshotSlots.test.ts b/gui/frontend/src/utils/screenshotSlots.test.ts new file mode 100644 index 00000000..52575309 --- /dev/null +++ b/gui/frontend/src/utils/screenshotSlots.test.ts @@ -0,0 +1,113 @@ +// Copyright (c) 2025-2026, Audionut and the autobrr contributors. +// SPDX-License-Identifier: GPL-2.0-or-later + +import { describe, expect, it } from "vitest"; + +import type { ScreenshotImage, ScreenshotSelection } from "../types"; +import { + buildExistingSlotImages, + pendingSelections, + selectionTimestamp, + slotTolerance, + staleSlotPaths, +} from "./screenshotSlots"; + +const image = (index: number, timestamp: number, path: string): ScreenshotImage => + ({ Index: index, TimestampSeconds: timestamp, Path: path }) as ScreenshotImage; + +const selection = (index: number, timestamp: number, frame = 0): ScreenshotSelection => ({ + Index: index, + TimestampSeconds: timestamp, + Frame: frame, + Source: "auto", +}); + +const FPS = 24; + +describe("slotTolerance", () => { + it("floors at half a second and widens for slow frame rates", () => { + expect(slotTolerance(24)).toBe(0.5); + expect(slotTolerance(1)).toBe(1); + expect(slotTolerance(0)).toBe(0.5); + }); +}); + +describe("selectionTimestamp", () => { + it("prefers seconds and falls back to the frame number", () => { + expect(selectionTimestamp(selection(1, 12.5), FPS)).toBe(12.5); + expect(selectionTimestamp(selection(1, 0, 48), FPS)).toBe(2); + expect(selectionTimestamp(selection(1, 0, 48), 0)).toBe(0); + }); +}); + +describe("buildExistingSlotImages", () => { + it("lets a promoted preview claim a slot the plan does not know about", () => { + const slots = buildExistingSlotImages( + [image(1, 10, "shot-1.png")], + [image(2, 20, "shot-preview-2.png")], + ); + expect(slots.get(1)?.Path).toBe("shot-1.png"); + expect(slots.get(2)?.Path).toBe("shot-preview-2.png"); + }); +}); + +describe("pendingSelections", () => { + it("skips slots whose file still matches, keeps retimed and empty ones", () => { + const slots = buildExistingSlotImages([image(1, 10, "shot-1.png"), image(2, 20, "shot-2.png")]); + const pending = pendingSelections( + [selection(1, 10.2), selection(2, 95), selection(3, 30)], + slots, + FPS, + ); + // 1 still matches (within tolerance), 2 was retimed, 3 was never shot. + expect(pending.map((entry) => entry.Index)).toEqual([2, 3]); + }); + + it("returns the full baseline when nothing exists yet", () => { + const selections = [selection(1, 10), selection(2, 20)]; + expect(pendingSelections(selections, new Map(), FPS)).toHaveLength(2); + }); + + it("skips a slot filled by a promoted preview", () => { + const slots = buildExistingSlotImages([], [image(1, 10, "shot-preview-1.png")]); + expect(pendingSelections([selection(1, 10), selection(2, 20)], slots, FPS)).toHaveLength(1); + }); + + it("compares a frame-only selection against the file timestamp", () => { + const slots = buildExistingSlotImages([image(1, 2, "shot-1.png")]); + // Frame 48 @ 24fps = 2s, which is the file it already has. + expect(pendingSelections([selection(1, 0, 48)], slots, FPS)).toHaveLength(0); + // Frame 480 @ 24fps = 20s: retimed, so it must be reshot. + expect(pendingSelections([selection(1, 0, 480)], slots, FPS)).toHaveLength(1); + }); + + it("reshoots a slot whose file drifted by more than the frame tolerance", () => { + const slots = buildExistingSlotImages([image(1, 10, "shot-1.png")]); + expect(pendingSelections([selection(1, 10.4)], slots, FPS)).toHaveLength(0); + expect(pendingSelections([selection(1, 10.6)], slots, FPS)).toHaveLength(1); + }); +}); + +describe("staleSlotPaths", () => { + it("retires the file a re-shot slot replaced, and nothing else", () => { + const slots = buildExistingSlotImages([image(1, 10, "shot-1.png"), image(2, 20, "shot-2.png")]); + const stale = staleSlotPaths([image(2, 95, "shot-2-new.png")], slots); + expect([...stale]).toEqual(["shot-2.png"]); + }); + + it("does not retire a file the capture rewrote in place", () => { + const slots = buildExistingSlotImages([image(1, 10, "shot-1.png")]); + expect(staleSlotPaths([image(1, 10, "shot-1.png")], slots).size).toBe(0); + }); + + it("retires the preview file a re-shot slot replaced", () => { + const slots = buildExistingSlotImages([], [image(1, 10, "shot-preview-1.png")]); + const stale = staleSlotPaths([image(1, 40, "shot-1-new.png")], slots); + expect([...stale]).toEqual(["shot-preview-1.png"]); + }); + + it("leaves untouched slots alone", () => { + const slots = buildExistingSlotImages([image(1, 10, "shot-1.png"), image(2, 20, "shot-2.png")]); + expect(staleSlotPaths([image(3, 30, "shot-3.png")], slots).size).toBe(0); + }); +}); diff --git a/gui/frontend/src/utils/screenshotSlots.ts b/gui/frontend/src/utils/screenshotSlots.ts new file mode 100644 index 00000000..5697bf20 --- /dev/null +++ b/gui/frontend/src/utils/screenshotSlots.ts @@ -0,0 +1,70 @@ +// Copyright (c) 2025-2026, Audionut and the autobrr contributors. +// SPDX-License-Identifier: GPL-2.0-or-later + +import type { ScreenshotImage, ScreenshotSelection } from "../types"; + +// Mirrors screenshotTimestampMatchesSelection in internal/services/screenshots. +export const slotTolerance = (frameRate: number): number => + frameRate > 0 ? Math.max(0.5, 1 / frameRate) : 0.5; + +export const selectionTimestamp = (entry: ScreenshotSelection, frameRate: number): number => { + if (Number.isFinite(entry.TimestampSeconds) && entry.TimestampSeconds > 0) { + return entry.TimestampSeconds; + } + if (Number.isFinite(entry.Frame) && entry.Frame > 0 && frameRate > 0) { + return entry.Frame / frameRate; + } + return 0; +}; + +/** + * Image currently filling each capture slot. Plan.ExistingScreenshots covers files + * on disk; promoted previews are written as -preview- files that never reach it, so + * they are passed in separately - previewImages keeps their true slot Index. + */ +export const buildExistingSlotImages = ( + existing: readonly ScreenshotImage[] | undefined, + promotedPreviews: readonly ScreenshotImage[] = [], +): Map => { + const slots = new Map(); + for (const image of [...(existing || []), ...promotedPreviews]) { + if (Number.isFinite(image.Index)) { + slots.set(image.Index, image); + } + } + return slots; +}; + +/** + * Baseline slots still needing a capture. A slot is skipped only while its file + * still matches the selection; retiming a slot must re-shoot it, not count as done. + */ +export const pendingSelections = ( + selections: readonly ScreenshotSelection[], + slots: ReadonlyMap, + frameRate: number, +): ScreenshotSelection[] => { + const tolerance = slotTolerance(frameRate); + return selections.filter((entry) => { + const existing = slots.get(entry.Index); + if (!existing) return true; + const drift = Math.abs((existing.TimestampSeconds || 0) - selectionTimestamp(entry, frameRate)); + return drift > tolerance; + }); +}; + +/** Files superseded by a capture: a re-shot slot lands on a new filename. */ +export const staleSlotPaths = ( + captured: readonly ScreenshotImage[], + slots: ReadonlyMap, +): Set => { + const capturedPaths = new Set(captured.map((image) => image.Path)); + const stale = new Set(); + for (const image of captured) { + const replaced = slots.get(image.Index)?.Path; + if (replaced && !capturedPaths.has(replaced)) { + stale.add(replaced); + } + } + return stale; +}; diff --git a/internal/services/screenshots/scenarios_test.go b/internal/services/screenshots/scenarios_test.go new file mode 100644 index 00000000..c9496cba --- /dev/null +++ b/internal/services/screenshots/scenarios_test.go @@ -0,0 +1,680 @@ +// Copyright (c) 2025-2026, Audionut and the autobrr contributors. +// SPDX-License-Identifier: GPL-2.0-or-later + +package screenshots + +import ( + "context" + "errors" + "image/color" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/autobrr/upbrr/internal/config" + internalerrors "github.com/autobrr/upbrr/internal/errors" + "github.com/autobrr/upbrr/internal/paths" + "github.com/autobrr/upbrr/pkg/api" +) + +// The screenshot lifecycle is: Plan proposes baseline slots -> Capture writes +// files named after their slot and timestamp -> the user promotes some into +// final selections -> Delete retires files. Every test below is one user-visible +// step of that lifecycle. The invariant they collectively guard is that a slot is +// "done" only while a file exists whose timestamp still matches the slot, and +// that a final selection the user picked survives a plan reload. + +// Manual frames 120/360/600 at 24fps land on slots 0/1/2 at 5s/15s/25s. +const ( + scenarioFrame0 = 120 + scenarioFrame1 = 360 + scenarioFrame2 = 600 +) + +type scenarioFixture struct { + root string + tmpDir string + base string + meta api.PreparedMetadata +} + +// newScenarioFixture builds a release whose baseline is three manual frames, so +// slot timestamps are exact and tests do not depend on the auto-spacing formula. +func newScenarioFixture(t *testing.T) (*scenarioFixture, *Service) { + t.Helper() + root := t.TempDir() + mediaInfoPath := writeScenarioMediaInfo(t, root, "", "24.000") + + meta := api.PreparedMetadata{ + SourcePath: filepath.Join(root, "movie.mkv"), + MediaInfoJSONPath: mediaInfoPath, + ScreenshotOverrides: api.ScreenshotOverrides{ + ManualFrames: []int{scenarioFrame0, scenarioFrame1, scenarioFrame2}, + }, + } + tmpDir, _, err := paths.ReleaseTempDir(root, meta, meta.SourcePath) + if err != nil { + t.Fatalf("release temp dir: %v", err) + } + if err := os.MkdirAll(tmpDir, 0o700); err != nil { + t.Fatalf("mkdir release temp: %v", err) + } + return &scenarioFixture{ + root: root, + tmpDir: tmpDir, + base: screenshotBaseName(meta), + meta: meta, + }, + NewServiceWithRepo(config.Config{}, api.NopLogger{}, root, nil, nil) +} + +func writeScenarioMediaInfo(t *testing.T, root string, duration string, frameRate string) string { + t.Helper() + tracks := `{"@type":"Video","FrameRate":"` + frameRate + `"}` + if duration != "" { + tracks = `{"@type":"General","Duration":"` + duration + `"},` + tracks + } + mediaInfoPath := filepath.Join(root, "mediainfo.json") + payload := []byte(`{"media":{"track":[` + tracks + `]}}`) + if err := os.WriteFile(mediaInfoPath, payload, 0o600); err != nil { + t.Fatalf("write mediainfo: %v", err) + } + return mediaInfoPath +} + +// writeScenarioCapture plants the file Capture would have written for a slot. +func (f *scenarioFixture) writeScenarioCapture(t *testing.T, index int, timestampSeconds float64) string { + t.Helper() + imagePath := filepath.Join(f.tmpDir, buildScreenshotFilename(f.base, index, timestampSeconds, api.ScreenshotPurposeFinal)) + if err := os.WriteFile(imagePath, []byte("png"), 0o600); err != nil { + t.Fatalf("write capture: %v", err) + } + return imagePath +} + +func (f *scenarioFixture) writeScenarioFile(t *testing.T, name string) string { + t.Helper() + imagePath := filepath.Join(f.tmpDir, name) + if err := os.WriteFile(imagePath, []byte("png"), 0o600); err != nil { + t.Fatalf("write file: %v", err) + } + return imagePath +} + +func existingPaths(images []api.ScreenshotImage) []string { + paths := make([]string, 0, len(images)) + for _, image := range images { + paths = append(paths, image.Path) + } + return paths +} + +// scenarioRepo serves stored final selections back to Plan. +type scenarioRepo struct { + api.MetadataRepository + finals []api.ScreenshotFinalSelection + screens []api.Screenshot + trackers []api.TrackerMetadata + + savedScreens []api.Screenshot + savedTrackers []api.TrackerMetadata + deletedScreen []string + deletedFinal []string +} + +func (r *scenarioRepo) ListScreenshotsByPath(context.Context, string) ([]api.Screenshot, error) { + return r.screens, nil +} + +func (r *scenarioRepo) ListFinalSelections(context.Context, string) ([]api.ScreenshotFinalSelection, error) { + return r.finals, nil +} + +func (r *scenarioRepo) SaveScreenshot(_ context.Context, shot api.Screenshot) error { + r.savedScreens = append(r.savedScreens, shot) + return nil +} + +func (r *scenarioRepo) DeleteScreenshot(_ context.Context, imagePath string) error { + r.deletedScreen = append(r.deletedScreen, imagePath) + return nil +} + +func (r *scenarioRepo) DeleteFinalSelection(_ context.Context, imagePath string) error { + r.deletedFinal = append(r.deletedFinal, imagePath) + return nil +} + +func (r *scenarioRepo) ListTrackerMetadataByPath(context.Context, string) ([]api.TrackerMetadata, error) { + return r.trackers, nil +} + +func (r *scenarioRepo) SaveTrackerMetadata(_ context.Context, record api.TrackerMetadata) error { + r.savedTrackers = append(r.savedTrackers, record) + return nil +} + +// --- Plan: baseline --------------------------------------------------------- + +func TestPlanBaselineForFreshRelease(t *testing.T) { + fixture, service := newScenarioFixture(t) + + plan, err := service.Plan(context.Background(), fixture.meta, 3) + if err != nil { + t.Fatalf("plan: %v", err) + } + if len(plan.SuggestedSelections) != 3 { + t.Fatalf("expected 3 baseline slots, got %#v", plan.SuggestedSelections) + } + for slot, selection := range plan.SuggestedSelections { + if selection.Index != slot { + t.Fatalf("expected zero-based slot indices, got %#v", plan.SuggestedSelections) + } + if selection.Source != "manual" { + t.Fatalf("expected manual source for frame overrides, got %#v", selection) + } + } + if len(plan.ExistingScreenshots) != 0 || len(plan.FinalSelections) != 0 { + t.Fatalf("fresh release must have nothing captured, got %#v / %#v", plan.ExistingScreenshots, plan.FinalSelections) + } +} + +func TestPlanAutoBaselineIgnoresCountWhenManualFramesGiven(t *testing.T) { + fixture, service := newScenarioFixture(t) + + // count=10 but the user pinned 3 frames: the pinned frames win. + plan, err := service.Plan(context.Background(), fixture.meta, 10) + if err != nil { + t.Fatalf("plan: %v", err) + } + if len(plan.SuggestedSelections) != 3 { + t.Fatalf("manual frames must override the requested count, got %#v", plan.SuggestedSelections) + } +} + +func TestPlanAddsOneSlotForDiscReleases(t *testing.T) { + root := t.TempDir() + mediaInfoPath := writeScenarioMediaInfo(t, root, "100", "25.000") + videoTS := filepath.Join(root, "VIDEO_TS") + if err := os.MkdirAll(videoTS, 0o700); err != nil { + t.Fatalf("mkdir VIDEO_TS: %v", err) + } + if err := os.WriteFile(filepath.Join(videoTS, "VTS_01_1.VOB"), []byte(strings.Repeat("c", 99)), 0o600); err != nil { + t.Fatalf("write VOB: %v", err) + } + meta := api.PreparedMetadata{SourcePath: root, DiscType: "DVD", MediaInfoJSONPath: mediaInfoPath} + service := NewService(config.Config{}, api.NopLogger{}, t.TempDir(), nil) + + plan, err := service.Plan(context.Background(), meta, 3) + if err != nil { + t.Fatalf("plan: %v", err) + } + // Disc releases reserve an extra slot for the menu capture. + if len(plan.SuggestedSelections) != 4 { + t.Fatalf("expected 3 requested + 1 disc slot, got %#v", plan.SuggestedSelections) + } +} + +func TestPlanRequiresManualFramesWithoutTiming(t *testing.T) { + root := t.TempDir() + mediaInfoPath := writeScenarioMediaInfo(t, root, "", "0") + meta := api.PreparedMetadata{ + SourcePath: filepath.Join(root, "movie.mkv"), + MediaInfoJSONPath: mediaInfoPath, + } + service := NewService(config.Config{}, api.NopLogger{}, root, nil) + + plan, err := service.Plan(context.Background(), meta, 3) + if err != nil { + t.Fatalf("plan: %v", err) + } + if !plan.RequiresManualFrames { + t.Fatalf("expected manual frames to be required without duration or frame rate, got %#v", plan) + } + if len(plan.SuggestedSelections) != 0 { + t.Fatalf("expected no baseline without timing, got %#v", plan.SuggestedSelections) + } +} + +// --- Plan: which files count as "already captured" -------------------------- + +func TestPlanReportsMatchingCaptureAsExisting(t *testing.T) { + fixture, service := newScenarioFixture(t) + existing := fixture.writeScenarioCapture(t, 1, 15) + + plan, err := service.Plan(context.Background(), fixture.meta, 3) + if err != nil { + t.Fatalf("plan: %v", err) + } + if len(plan.SuggestedSelections) != 3 { + t.Fatalf("baseline must stay whole so any slot can be reshot, got %#v", plan.SuggestedSelections) + } + if len(plan.ExistingScreenshots) != 1 || plan.ExistingScreenshots[0].Index != 1 || plan.ExistingScreenshots[0].Path != existing { + t.Fatalf("expected slot 1 reported as captured, got %#v", plan.ExistingScreenshots) + } +} + +func TestPlanIgnoresCaptureWhoseTimestampNoLongerMatchesItsSlot(t *testing.T) { + fixture, service := newScenarioFixture(t) + // Slot 1 is 15s. This file claims slot 1 but was shot at 90s: the user + // retimed the slot, so the stale file must not mark it done. + fixture.writeScenarioCapture(t, 1, 90) + + plan, err := service.Plan(context.Background(), fixture.meta, 3) + if err != nil { + t.Fatalf("plan: %v", err) + } + if len(plan.ExistingScreenshots) != 0 { + t.Fatalf("retimed slot must not count as captured, got %#v", plan.ExistingScreenshots) + } +} + +func TestPlanIgnoresCaptureOutsideTheBaseline(t *testing.T) { + fixture, service := newScenarioFixture(t) + // Left over from a run that asked for more screenshots than this one does. + fixture.writeScenarioCapture(t, 7, 300) + + plan, err := service.Plan(context.Background(), fixture.meta, 3) + if err != nil { + t.Fatalf("plan: %v", err) + } + if len(plan.ExistingScreenshots) != 0 { + t.Fatalf("orphan slot must not count as captured, got %#v", plan.ExistingScreenshots) + } +} + +func TestPlanIgnoresPreviewAndDiscMenuFilesAsCaptures(t *testing.T) { + fixture, service := newScenarioFixture(t) + fixture.writeScenarioFile(t, fixture.base+"-preview-01-ss_00015000-123456.png") + fixture.writeScenarioFile(t, fixture.base+"-dvd-menu-01-123456.png") + + plan, err := service.Plan(context.Background(), fixture.meta, 3) + if err != nil { + t.Fatalf("plan: %v", err) + } + if len(plan.ExistingScreenshots) != 0 { + t.Fatalf("preview and menu files are not slot captures, got %#v", plan.ExistingScreenshots) + } +} + +// A deleted screenshot leaves its database row behind, and the slot must still come +// back as pending so the user can regenerate it. +func TestPlanTreatsDeletedCaptureAsRegenerable(t *testing.T) { + fixture, _ := newScenarioFixture(t) + deleted := filepath.Join(fixture.tmpDir, buildScreenshotFilename(fixture.base, 1, 15, api.ScreenshotPurposeFinal)) + repo := &scenarioRepo{screens: []api.Screenshot{{SourcePath: fixture.meta.SourcePath, ImagePath: deleted, Timestamp: 15}}} + service := NewServiceWithRepo(config.Config{}, api.NopLogger{}, fixture.root, nil, repo) + + plan, err := service.Plan(context.Background(), fixture.meta, 3) + if err != nil { + t.Fatalf("plan: %v", err) + } + if len(plan.SuggestedSelections) != 3 { + t.Fatalf("deleted slot must remain in the baseline, got %#v", plan.SuggestedSelections) + } + if len(plan.ExistingScreenshots) != 0 { + t.Fatalf("a database row without a file on disk is not a capture, got %#v", plan.ExistingScreenshots) + } +} + +// --- Plan: final selections survive a reload -------------------------------- + +func TestPlanKeepsFinalSelectionsInSavedOrder(t *testing.T) { + fixture, _ := newScenarioFixture(t) + slot0 := fixture.writeScenarioCapture(t, 0, 5) + slot1 := fixture.writeScenarioCapture(t, 1, 15) + slot2 := fixture.writeScenarioCapture(t, 2, 25) + + // The user dragged slot 2's shot to the front. Order is the list position, + // not the capture slot, so a reload must not confuse the two. + repo := &scenarioRepo{finals: []api.ScreenshotFinalSelection{ + {SourcePath: fixture.meta.SourcePath, ImagePath: slot2, Order: 0}, + {SourcePath: fixture.meta.SourcePath, ImagePath: slot0, Order: 1}, + {SourcePath: fixture.meta.SourcePath, ImagePath: slot1, Order: 2}, + }} + service := NewServiceWithRepo(config.Config{}, api.NopLogger{}, fixture.root, nil, repo) + + plan, err := service.Plan(context.Background(), fixture.meta, 3) + if err != nil { + t.Fatalf("plan: %v", err) + } + if got := existingPaths(plan.FinalSelections); len(got) != 3 { + t.Fatalf("reordering final selections must not drop them, got %#v", got) + } + if plan.FinalSelections[0].Path != slot2 { + t.Fatalf("expected the reordered pick to stay first, got %#v", existingPaths(plan.FinalSelections)) + } +} + +func TestPlanKeepsDiscMenuFinalSelection(t *testing.T) { + fixture, _ := newScenarioFixture(t) + slot0 := fixture.writeScenarioCapture(t, 0, 5) + // Menu captures carry no timestamp in their filename and are not baseline + // slots, so a timestamp/slot filter must not evict them. + menu := fixture.writeScenarioFile(t, fixture.base+"-dvd-menu-01-123456.png") + + repo := &scenarioRepo{finals: []api.ScreenshotFinalSelection{ + {SourcePath: fixture.meta.SourcePath, ImagePath: menu, Order: 0, Source: api.ScreenshotSelectionSourceMenu}, + {SourcePath: fixture.meta.SourcePath, ImagePath: slot0, Order: 1}, + }} + service := NewServiceWithRepo(config.Config{}, api.NopLogger{}, fixture.root, nil, repo) + + plan, err := service.Plan(context.Background(), fixture.meta, 3) + if err != nil { + t.Fatalf("plan: %v", err) + } + if len(plan.FinalSelections) != 2 { + t.Fatalf("disc menu selection must survive a plan reload, got %#v", existingPaths(plan.FinalSelections)) + } + if plan.FinalSelections[0].Path != menu || plan.FinalSelections[0].Purpose != api.ScreenshotPurposeMenu { + t.Fatalf("expected the menu image kept and tagged, got %#v", plan.FinalSelections[0]) + } +} + +func TestPlanDropsFinalSelectionWhoseFrameLeftTheBaseline(t *testing.T) { + fixture, _ := newScenarioFixture(t) + slot0 := fixture.writeScenarioCapture(t, 0, 5) + // Shot at 90s, which is no longer any slot in the baseline. + stale := fixture.writeScenarioCapture(t, 1, 90) + + repo := &scenarioRepo{finals: []api.ScreenshotFinalSelection{ + {SourcePath: fixture.meta.SourcePath, ImagePath: slot0, Order: 0}, + {SourcePath: fixture.meta.SourcePath, ImagePath: stale, Order: 1}, + }} + service := NewServiceWithRepo(config.Config{}, api.NopLogger{}, fixture.root, nil, repo) + + plan, err := service.Plan(context.Background(), fixture.meta, 3) + if err != nil { + t.Fatalf("plan: %v", err) + } + if len(plan.FinalSelections) != 1 || plan.FinalSelections[0].Path != slot0 { + t.Fatalf("expected only the in-baseline pick kept, got %#v", existingPaths(plan.FinalSelections)) + } +} + +func TestPlanSkipsFinalSelectionOutsideTempDir(t *testing.T) { + fixture, _ := newScenarioFixture(t) + outside := filepath.Join(t.TempDir(), "elsewhere.png") + if err := os.WriteFile(outside, []byte("png"), 0o600); err != nil { + t.Fatalf("write outside image: %v", err) + } + repo := &scenarioRepo{finals: []api.ScreenshotFinalSelection{ + {SourcePath: fixture.meta.SourcePath, ImagePath: outside, Order: 0}, + }} + service := NewServiceWithRepo(config.Config{}, api.NopLogger{}, fixture.root, nil, repo) + + plan, err := service.Plan(context.Background(), fixture.meta, 3) + if err != nil { + t.Fatalf("plan: %v", err) + } + if len(plan.FinalSelections) != 0 { + t.Fatalf("a selection outside the release temp dir must be ignored, got %#v", existingPaths(plan.FinalSelections)) + } +} + +// --- Capture ---------------------------------------------------------------- + +func newCaptureService(t *testing.T, fixture *scenarioFixture, repo api.MetadataRepository) *Service { + t.Helper() + ffmpegRoot := t.TempDir() + if err := writeTestBundledFFmpeg(ffmpegRoot); err != nil { + t.Fatalf("write bundled ffmpeg: %v", err) + } + t.Chdir(ffmpegRoot) + runner := &writeOutputRunner{payload: testPNGBytes(t, color.RGBA{R: 180, G: 180, B: 180, A: 255})} + return NewServiceWithRepo(config.Config{}, api.NopLogger{}, fixture.root, runner, repo) +} + +func TestCaptureRejectsEmptySelections(t *testing.T) { + fixture, service := newScenarioFixture(t) + + _, err := service.Capture(context.Background(), fixture.meta, nil, api.ScreenshotPurposeFinal) + if !errors.Is(err, internalerrors.ErrInvalidInput) { + t.Fatalf("expected invalid input for an empty capture request, got %v", err) + } +} + +func TestCaptureReportsInvalidTimestampWithoutFailingTheBatch(t *testing.T) { + fixture, _ := newScenarioFixture(t) + service := newCaptureService(t, fixture, nil) + + result, err := service.Capture(context.Background(), fixture.meta, []api.ScreenshotSelection{ + {Index: 0, TimestampSeconds: 5}, + {Index: 1}, // no timestamp and no frame: unusable + }, api.ScreenshotPurposeFinal) + if err != nil { + t.Fatalf("capture: %v", err) + } + if len(result.Images) != 1 || result.Images[0].Index != 0 { + t.Fatalf("expected the usable selection to still be captured, got %#v", result.Images) + } + if len(result.Errors) != 1 || result.Errors[0].Index != 1 { + t.Fatalf("expected the unusable selection reported as an error, got %#v", result.Errors) + } +} + +func TestCaptureDerivesTimestampFromFrameNumber(t *testing.T) { + fixture, _ := newScenarioFixture(t) + service := newCaptureService(t, fixture, nil) + + result, err := service.Capture(context.Background(), fixture.meta, []api.ScreenshotSelection{ + {Index: 0, Frame: scenarioFrame1}, // 360 @ 24fps = 15s + }, api.ScreenshotPurposeFinal) + if err != nil { + t.Fatalf("capture: %v", err) + } + if len(result.Images) != 1 || result.Images[0].TimestampSeconds != 15 { + t.Fatalf("expected frame 360 at 24fps to resolve to 15s, got %#v", result.Images) + } + if !strings.Contains(filepath.Base(result.Images[0].Path), "ss_00015000") { + t.Fatalf("expected the timestamp encoded in the filename, got %q", result.Images[0].Path) + } +} + +func TestCapturePersistsFinalsButNotPreviews(t *testing.T) { + fixture, _ := newScenarioFixture(t) + repo := &scenarioRepo{} + service := newCaptureService(t, fixture, repo) + selections := []api.ScreenshotSelection{{Index: 0, TimestampSeconds: 5}} + + preview, err := service.Capture(context.Background(), fixture.meta, selections, api.ScreenshotPurposePreview) + if err != nil { + t.Fatalf("capture preview: %v", err) + } + if !strings.Contains(filepath.Base(preview.Images[0].Path), "-preview-") { + t.Fatalf("expected a preview filename, got %q", preview.Images[0].Path) + } + if len(repo.savedScreens) != 0 { + t.Fatalf("previews are throwaway and must not be persisted, got %#v", repo.savedScreens) + } + + if _, err := service.Capture(context.Background(), fixture.meta, selections, api.ScreenshotPurposeFinal); err != nil { + t.Fatalf("capture final: %v", err) + } + if len(repo.savedScreens) != 1 { + t.Fatalf("expected the final capture persisted, got %#v", repo.savedScreens) + } +} + +// A preview file never lands in ExistingScreenshots, so promoting one leaves its +// slot pending server-side. The clients compensate; this pins the server contract. +func TestPlanDoesNotTreatPromotedPreviewAsSlotCapture(t *testing.T) { + fixture, _ := newScenarioFixture(t) + previewPath := fixture.writeScenarioFile(t, buildScreenshotFilename(fixture.base, 1, 15, api.ScreenshotPurposePreview)) + repo := &scenarioRepo{finals: []api.ScreenshotFinalSelection{ + {SourcePath: fixture.meta.SourcePath, ImagePath: previewPath, Order: 0}, + }} + service := NewServiceWithRepo(config.Config{}, api.NopLogger{}, fixture.root, nil, repo) + + plan, err := service.Plan(context.Background(), fixture.meta, 3) + if err != nil { + t.Fatalf("plan: %v", err) + } + if len(plan.ExistingScreenshots) != 0 { + t.Fatalf("a promoted preview is not a slot capture, got %#v", plan.ExistingScreenshots) + } + if len(plan.FinalSelections) != 1 || plan.FinalSelections[0].Path != previewPath { + t.Fatalf("a promoted preview must survive as a final selection, got %#v", existingPaths(plan.FinalSelections)) + } +} + +// --- Delete ----------------------------------------------------------------- + +func TestDeleteRemovesFileAndDatabaseRows(t *testing.T) { + fixture, _ := newScenarioFixture(t) + repo := &scenarioRepo{} + service := NewServiceWithRepo(config.Config{}, api.NopLogger{}, fixture.root, nil, repo) + target := fixture.writeScenarioCapture(t, 1, 15) + + if err := service.Delete(context.Background(), fixture.meta, target); err != nil { + t.Fatalf("delete: %v", err) + } + if _, err := os.Stat(target); !os.IsNotExist(err) { + t.Fatalf("expected the file gone from disk, stat err = %v", err) + } + if len(repo.deletedScreen) != 1 || len(repo.deletedFinal) != 1 { + t.Fatalf("expected both the screenshot row and the final selection removed, got %#v / %#v", repo.deletedScreen, repo.deletedFinal) + } +} + +// The GUI reloads after deleting, so a second delete of the same path is normal. +func TestDeleteIsIdempotentForMissingFiles(t *testing.T) { + fixture, _ := newScenarioFixture(t) + repo := &scenarioRepo{} + service := NewServiceWithRepo(config.Config{}, api.NopLogger{}, fixture.root, nil, repo) + target := filepath.Join(fixture.tmpDir, buildScreenshotFilename(fixture.base, 1, 15, api.ScreenshotPurposeFinal)) + + if err := service.Delete(context.Background(), fixture.meta, target); err != nil { + t.Fatalf("deleting an already-missing file must not fail: %v", err) + } + if len(repo.deletedScreen) != 1 { + t.Fatalf("expected the database row cleaned even when the file was gone, got %#v", repo.deletedScreen) + } +} + +func TestDeleteRejectsPathsOutsideTheReleaseTempDir(t *testing.T) { + fixture, service := newScenarioFixture(t) + sibling := fixture.tmpDir + ".evil" + if err := os.MkdirAll(sibling, 0o700); err != nil { + t.Fatalf("mkdir sibling: %v", err) + } + + cases := map[string]string{ + "empty": "", + "whitespace": " ", + "traversal": filepath.Join(fixture.tmpDir, "..", "escape.png"), + "sibling prefix dir": filepath.Join(sibling, "shot.png"), + "absolute elsewhere": filepath.Join(t.TempDir(), "shot.png"), + "not an image": filepath.Join(fixture.tmpDir, "notes.txt"), + } + for name, target := range cases { + t.Run(name, func(t *testing.T) { + if err := service.Delete(context.Background(), fixture.meta, target); !errors.Is(err, internalerrors.ErrInvalidInput) { + t.Fatalf("expected invalid input, got %v", err) + } + }) + } +} + +// --- Tracker images --------------------------------------------------------- + +// trackerFixture wires a release whose tracker has one already-uploaded image, and +// returns the local path that image is mirrored to. +func trackerFixture(t *testing.T, fixture *scenarioFixture) (api.TrackerMetadata, string) { + t.Helper() + record := api.TrackerMetadata{ + SourcePath: fixture.meta.SourcePath, + Tracker: "UTP", + ImageURLs: []string{"https://images.example/foo.png"}, + } + fixture.meta.TrackerData = []api.TrackerMetadata{record} + mirrored := filepath.Join(fixture.tmpDir, "utp", "foo_01.png") + if err := os.MkdirAll(filepath.Dir(mirrored), 0o700); err != nil { + t.Fatalf("mkdir tracker dir: %v", err) + } + if err := os.WriteFile(mirrored, []byte("png"), 0o600); err != nil { + t.Fatalf("write tracker image: %v", err) + } + return record, mirrored +} + +func TestPlanLinksTrackerImagesAndAutoSelectsThem(t *testing.T) { + fixture, _ := newScenarioFixture(t) + _, mirrored := trackerFixture(t, fixture) + service := NewServiceWithRepo(config.Config{}, api.NopLogger{}, fixture.root, nil, &scenarioRepo{}) + + plan, err := service.Plan(context.Background(), fixture.meta, 3) + if err != nil { + t.Fatalf("plan: %v", err) + } + if len(plan.TrackerImageLinks) != 1 || plan.TrackerImageLinks[0].Path != mirrored { + t.Fatalf("expected the uploaded image linked to its local mirror, got %#v", plan.TrackerImageLinks) + } + // A linked image is already accounted for, so it is not offered again as a loose file. + if len(plan.ExistingTrackerScreenshots) != 0 { + t.Fatalf("a linked tracker image must not also be listed as unlinked, got %#v", existingPaths(plan.ExistingTrackerScreenshots)) + } + if len(plan.FinalSelections) != 1 || plan.FinalSelections[0].Path != mirrored { + t.Fatalf("tracker images are auto-included in final selections, got %#v", existingPaths(plan.FinalSelections)) + } +} + +func TestPlanKeepsTrackerImageInItsSavedPosition(t *testing.T) { + fixture, _ := newScenarioFixture(t) + _, mirrored := trackerFixture(t, fixture) + slot0 := fixture.writeScenarioCapture(t, 0, 5) + + // The user put the tracker image first. It has no timestamp and no slot, so a + // reload must keep it where they put it rather than re-appending it at the end. + repo := &scenarioRepo{finals: []api.ScreenshotFinalSelection{ + {SourcePath: fixture.meta.SourcePath, ImagePath: mirrored, Order: 0}, + {SourcePath: fixture.meta.SourcePath, ImagePath: slot0, Order: 1}, + }} + service := NewServiceWithRepo(config.Config{}, api.NopLogger{}, fixture.root, nil, repo) + + plan, err := service.Plan(context.Background(), fixture.meta, 3) + if err != nil { + t.Fatalf("plan: %v", err) + } + if got := existingPaths(plan.FinalSelections); len(got) != 2 || got[0] != mirrored { + t.Fatalf("expected the tracker image kept in first position, got %#v", got) + } +} + +func TestDeleteDropsTheTrackerImageURLItMirrors(t *testing.T) { + fixture, _ := newScenarioFixture(t) + record, mirrored := trackerFixture(t, fixture) + repo := &scenarioRepo{trackers: []api.TrackerMetadata{record}} + service := NewServiceWithRepo(config.Config{}, api.NopLogger{}, fixture.root, nil, repo) + + if err := service.Delete(context.Background(), fixture.meta, mirrored); err != nil { + t.Fatalf("delete: %v", err) + } + if len(repo.savedTrackers) != 1 { + t.Fatalf("expected the tracker record rewritten, got %#v", repo.savedTrackers) + } + if len(repo.savedTrackers[0].ImageURLs) != 0 { + t.Fatalf("deleting the mirrored file must drop its uploaded URL, got %#v", repo.savedTrackers[0].ImageURLs) + } +} + +// --- SaveFinalSelections ---------------------------------------------------- + +func TestSaveFinalSelectionsRejectsUntrustedPaths(t *testing.T) { + fixture, _ := newScenarioFixture(t) + service := NewServiceWithRepo(config.Config{}, api.NopLogger{}, fixture.root, nil, &scenarioRepo{}) + + cases := map[string][]api.ScreenshotImage{ + "empty path": {{Path: ""}}, + "outside tmp": {{Path: filepath.Join(t.TempDir(), "shot.png")}}, + "bad extension": {{Path: filepath.Join(fixture.tmpDir, "shot.exe")}}, + } + for name, images := range cases { + t.Run(name, func(t *testing.T) { + if err := service.SaveFinalSelections(context.Background(), fixture.meta, images); !errors.Is(err, internalerrors.ErrInvalidInput) { + t.Fatalf("expected invalid input, got %v", err) + } + }) + } +} diff --git a/internal/services/screenshots/service.go b/internal/services/screenshots/service.go index 244cac04..87b2d495 100644 --- a/internal/services/screenshots/service.go +++ b/internal/services/screenshots/service.go @@ -129,81 +129,10 @@ func (s *Service) Plan(ctx context.Context, meta api.PreparedMetadata, count int baselineSelections = buildScreenshotSelections(total, plan.DurationSeconds, plan.FrameRate, meta) } base := screenshotBaseName(meta) - baselineByIndex := screenshotSelectionsByIndex(baselineSelections) - // Load existing screenshots from database that still exist on disk. - var missingIndexTimestamps []float64 - var existingIndices map[int]struct{} - if s.repo != nil { - dbScreenshots, err := s.repo.ListScreenshotsByPath(ctx, meta.SourcePath) - if err != nil { - s.logger.Debugf("screenshots: failed to load existing screenshots: %v", err) - } else { - existingIndices = make(map[int]struct{}, len(dbScreenshots)) - kept := 0 - for _, shot := range dbScreenshots { - pathValue := strings.TrimSpace(shot.ImagePath) - if pathValue == "" || !isAllowedImageExt(pathValue) || !isPathWithinDir(tmpDir, pathValue) { - continue - } - if info, statErr := os.Stat(pathValue); statErr != nil || info.IsDir() { - continue - } - timestamp := shot.Timestamp - if timestamp <= 0 { - timestamp = parseScreenshotTimestamp(pathValue, base) - } - if index, ok := parseScreenshotIndexStrict(pathValue, base); ok { - if selection, found := baselineByIndex[index]; found && screenshotTimestampMatchesSelection(timestamp, selection, plan.FrameRate) { - existingIndices[index] = struct{}{} - } - } else { - missingIndexTimestamps = append(missingIndexTimestamps, timestamp) - } - kept++ - } - s.logger.Debugf("screenshots: found existing screenshots db=%d disk=%d", len(dbScreenshots), kept) - } - } - - if existingIndices == nil { - existingIndices = make(map[int]struct{}) - } - if len(missingIndexTimestamps) > 0 && len(baselineSelections) > 0 { - slices.Sort(missingIndexTimestamps) - for _, existingTs := range missingIndexTimestamps { - bestIndex := -1 - bestDiff := 0.0 - for _, candidate := range baselineSelections { - if _, used := existingIndices[candidate.Index]; used { - continue - } - diff := abs(candidate.TimestampSeconds - existingTs) - if bestIndex < 0 || diff < bestDiff { - bestIndex = candidate.Index - bestDiff = diff - } - } - if bestIndex >= 0 { - if selection, found := baselineByIndex[bestIndex]; found && screenshotTimestampMatchesSelection(existingTs, selection, plan.FrameRate) { - existingIndices[bestIndex] = struct{}{} - } - } - } - } - - var suggestions []api.ScreenshotSelection - if len(baselineSelections) > 0 { - suggestions = make([]api.ScreenshotSelection, 0, total) - for _, candidate := range baselineSelections { - if _, used := existingIndices[candidate.Index]; used { - continue - } - suggestions = append(suggestions, candidate) - } - } - - plan.SuggestedSelections = suggestions + // Report the full baseline and let capture paths filter plan.ExistingScreenshots + // themselves; pruning here leaves a deleted screenshot's slot unregenerable. + plan.SuggestedSelections = baselineSelections plan.ExistingScreenshots = filterScreenshotsMatchingSelections(listExistingScreens(tmpDir, base), baselineSelections, plan.FrameRate) plan.TrackerImageLinks = buildTrackerImageLinks(meta, tmpDir) @@ -211,7 +140,12 @@ func (s *Service) Plan(ctx context.Context, meta api.PreparedMetadata, count int listTrackerScreens(tmpDir, base), plan.TrackerImageLinks, ) - plan.FinalSelections = filterScreenshotsMatchingSelections(s.loadFinalSelections(ctx, meta, tmpDir), baselineSelections, plan.FrameRate) + plan.FinalSelections = filterFinalSelections( + s.loadFinalSelections(ctx, meta, tmpDir), + baselineSelections, + plan.TrackerImageLinks, + plan.FrameRate, + ) // Automatically include tracker images in final selections plan.FinalSelections = mergeTrackerImagesIntoFinalSelections(plan.FinalSelections, plan.TrackerImageLinks) @@ -570,7 +504,7 @@ func (s *Service) Delete(ctx context.Context, meta api.PreparedMetadata, imagePa if err != nil { return fmt.Errorf("screenshots: resolve temp path: %w", err) } - if absTarget != absTmp && !strings.HasPrefix(absTarget, absTmp+string(os.PathSeparator)) { + if !isPathWithinDir(tmpDir, absTarget) { return internalerrors.ErrInvalidInput } if s.logger != nil { @@ -829,6 +763,41 @@ func filterScreenshotsMatchingSelections(images []api.ScreenshotImage, selection return filtered } +// filterFinalSelections drops picks whose frame left the baseline - retiming a slot +// strands its old capture - while keeping the ones that are not baseline frames at +// all. It must not key on Index: a final selection's Index is its position in the +// user's ordering, not the slot its file was shot for, so reordering the list would +// otherwise evict every pick. Menu captures and tracker images carry no timestamp +// and belong to no slot, so they are held regardless. +func filterFinalSelections( + images []api.ScreenshotImage, + selections []api.ScreenshotSelection, + trackerLinks []api.ScreenshotLinkedImage, + frameRate float64, +) []api.ScreenshotImage { + if len(images) == 0 { + return nil + } + linked := make(map[string]struct{}, len(trackerLinks)) + for _, link := range trackerLinks { + if strings.TrimSpace(link.Path) != "" { + linked[link.Path] = struct{}{} + } + } + filtered := make([]api.ScreenshotImage, 0, len(images)) + for _, image := range images { + _, isTracker := linked[image.Path] + keep := image.Purpose == api.ScreenshotPurposeMenu || isTracker || + slices.ContainsFunc(selections, func(selection api.ScreenshotSelection) bool { + return screenshotTimestampMatchesSelection(image.TimestampSeconds, selection, frameRate) + }) + if keep { + filtered = append(filtered, image) + } + } + return filtered +} + func listExistingScreens(tmpDir, base string) []api.ScreenshotImage { pattern := filepath.Join(tmpDir, base+"-*.png") matches, err := filepath.Glob(pattern) @@ -1146,33 +1115,6 @@ func parseScreenshotIndex(path string, base string) int { return parsed } -func parseScreenshotIndexStrict(path string, base string) (int, bool) { - name := filepath.Base(path) - name = strings.TrimSuffix(name, filepath.Ext(name)) - if base != "" { - prefix := base + "-" - if !strings.HasPrefix(name, prefix) { - return 0, false - } - name = strings.TrimPrefix(name, prefix) - } - name = strings.TrimPrefix(name, "preview-") - parts := strings.SplitN(name, "-", 2) - if len(parts) == 0 || parts[0] == "" { - return 0, false - } - for _, ch := range parts[0] { - if ch < '0' || ch > '9' { - return 0, false - } - } - parsed, err := strconv.Atoi(parts[0]) - if err != nil { - return 0, false - } - return parsed, true -} - func buildScreenshotFilename(base string, index int, timestampSeconds float64, purpose api.ScreenshotPurpose) string { ms := max(int64(timestampSeconds*1000), 0) stamp := fmt.Sprintf("ss_%08d", ms)