marvin8/cang

Watch

Fork 0

fix: copy all clip snapshots to output_dir on ingest #93

Merged

coding-agent-marvin8 merged 0 commits from refs/pull/93/head into main

2026-06-06 00:45:20 +00:00

coding-agent-marvin8 commented

2026-06-05 18:52:15 +00:00

(Migrated from codeberg.org)

_copy_thumbnail is replaced by _persist_snapshots(clip, camera_root, output_path) which copies the full set of clip snapshots into a per-clip subdirectory (output_dir/camera/filename_stem/) preserving the relative path structure from camera_root so that _snapshot_time and _snapshot_label continue to work. The middle frame is still written as the .jpg preview thumbnail alongside the MP4.

A new collect_snapshots_from_output() function in the snapshots route looks in this directory as a fallback when the camera-root search returns nothing, slotting in before the existing single-thumbnail fallback so old clips continue to work.

Closes #92

Previously _copy_thumbnail saved only the middle snapshot alongside the MP4. With delete_after_transcode = true every other snapshot for the clip was permanently deleted, leaving the Snapshots page with at most one image. _copy_thumbnail is replaced by _persist_snapshots(clip, camera_root, output_path) which copies the full set of clip snapshots into a per-clip subdirectory (output_dir/camera/filename_stem/) preserving the relative path structure from camera_root so that _snapshot_time and _snapshot_label continue to work. The middle frame is still written as the .jpg preview thumbnail alongside the MP4. A new collect_snapshots_from_output() function in the snapshots route looks in this directory as a fallback when the camera-root search returns nothing, slotting in before the existing single-thumbnail fallback so old clips continue to work. Closes #92

coding-agent-marvin8 commented

2026-06-05 20:50:17 +00:00

(Migrated from codeberg.org)

Review responses — commit e20d058

This comment addresses all review comments on this PR.

🔴 Fixed: `_persist_snapshots` blocks the event loop (comments 16639880, 16633757)

Fixed in commit e20d058. Both _done closures now call _persist_snapshots via loop.run_in_executor(None, ...) so the blocking shutil.copy2 loop runs in a thread-pool thread rather than on the event loop. Existing test test_on_done_copies_snapshot continues to cover the full behaviour.

🔴 Fixed: OSError silently discarded (comment 16633796)

Fixed in commit e20d058. Added a module-level _logger = logging.getLogger(__name__) and changed the bare except OSError: return None to log a WARNING with exc_info=True before returning None. New test test_persist_snapshots_logs_on_oserror verifies the warning is emitted.

✅ Not actioned: symlink path-traversal (comments 16633676, 16633679, 16633682) — stale

These issues were already resolved by commit 1c0b46e ('extract _build_allowed_roots helper, resolve symlinks in path checks') which landed on main before this PR was rebased. _resolve_snapshots and clip_snapshot_image both now call .resolve() on both sides before the is_relative_to check.

✅ Not actioned: `allowed_roots` duplication (comments 16633706, 16633709, 16633763, 16633790) — stale

Already fixed by the _build_allowed_roots(cfg) helper introduced in 1c0b46e. Both route handlers now call it.

✅ Not actioned: `_safe_filename` is dead code (comment 16639865)

_safe_filename is active code, called at lines 67 and 132 of lifespan.py in both _scan_camera and the _on_file watcher handler.

✅ Not actioned: `str | None` annotation style (comment 16633703)

str | None is the correct Python 3.10+ union syntax and is preferred over Optional[str] in this codebase (Python ≥ 3.11 per project config). No change needed.

✅ Not actioned: regex recompilation (comment 16633745)

_LEADING_DIGITS_RE is already compiled once at module level (line 13 of snapshots.py). There is no per-call compilation.

✅ Not actioned: `collect_snapshots_from_output` empty-directory case (comment 16633721)

sorted(clip_dir.glob("**/*.jpg")) evaluates to [] when the directory contains no .jpg files. The case is already handled correctly.

✅ Not actioned: `mkdir` inside copy loop (comments 16639859, 16639883)

Each snapshot maps to a unique subdirectory within clip_dir (Dahua structure: <date>/<channel>/jpg/<HH>/<MM>/). The dst.parent.mkdir(parents=True, exist_ok=True) call must stay per-snapshot because different snapshots land in different directories. The exist_ok=True flag makes re-entrant calls cheap; extracting a pre-pass would add complexity without meaningfully reducing I/O.

✅ Not actioned: fallback logic duplicated in two routes (comment 16639862)

The two-line pattern appears in clip_snapshot_album and clip_snapshot_image. Extracting a helper would require threading cfg and clip through another function for negligible gain. Left as-is per the project's guideline against premature abstractions.

✅ Not actioned: test path construction duplication (comment 16639868)

The snapshot directory paths are constructed in two distinct tests with different assertions. Extracting a fixture would obscure what each test is actually setting up.

✅ Not actioned: recursive glob performance (comments 16639874, 16633760)

collect_snapshots_from_output is a fallback path called at most once per UI request, and only when camera-root snapshots are absent. The **/*.jpg glob is correct and readable; micro-optimising a cold path is not warranted.

✅ Not actioned: `sorted()` materialises full list (comment 16639877)

The sorted list is required for stable integer indexing (snapshots[n]). A lazy generator cannot be indexed. No change possible.

✅ Not actioned: `_done` closure duplication (comments 16633712, 16633715)

The two _done closures capture different outer-scope bindings (cam.root vs _cam.root, clip vs match). Extracting a shared factory would require passing all five captured variables as arguments, adding indirection without reducing lines.

✅ Not actioned: per-request scan caching (comments 16633751, 16633754)

Caching scan results across HTTP requests is a separate architectural concern unrelated to this PR. Left for a dedicated refactor if needed.

✅ Not actioned: SRP violation in `_persist_snapshots` (comment 16633781)

The function performs one logical operation: persist a clip's snapshot assets. Copying all frames and deriving the thumbnail are inseparable steps of that single responsibility.

✅ Not actioned: fallback chain fragility (comment 16633793)

The three-step chain (collect_snapshots_for_clip → collect_snapshots_from_output → _resolve_snapshots) has clear, linear semantics. Each step handles a distinct case.

✅ Not actioned: `_resolve_snapshots` naming (comment 16633784)

_resolve_snapshots pre-dates this PR. Renaming it is out of scope.

✅ Not actioned: directory-structure coupling (comment 16633787)

collect_snapshots_from_output and _persist_snapshots must agree on the directory layout — that is intentional and documented in collect_snapshots_from_output's docstring.

✅ Not actioned: `_snapshot_label` called per-frame in template (comment 16633766)

Pre-existing code outside the scope of this PR.

✅ Not actioned: `allowed_roots` per-request cost (comment 16633748)

_build_allowed_roots is a single comprehension over a small in-memory list. The cost is negligible and no caching is needed.

**Review responses — commit e20d058** This comment addresses all review comments on this PR. --- ### 🔴 Fixed: `_persist_snapshots` blocks the event loop (comments 16639880, 16633757) **Fixed in commit e20d058.** Both `_done` closures now call `_persist_snapshots` via `loop.run_in_executor(None, ...)` so the blocking `shutil.copy2` loop runs in a thread-pool thread rather than on the event loop. Existing test `test_on_done_copies_snapshot` continues to cover the full behaviour. --- ### 🔴 Fixed: OSError silently discarded (comment 16633796) **Fixed in commit e20d058.** Added a module-level `_logger = logging.getLogger(__name__)` and changed the bare `except OSError: return None` to log a `WARNING` with `exc_info=True` before returning `None`. New test `test_persist_snapshots_logs_on_oserror` verifies the warning is emitted. --- ### ✅ Not actioned: symlink path-traversal (comments 16633676, 16633679, 16633682) — stale These issues were already resolved by commit `1c0b46e` ('extract _build_allowed_roots helper, resolve symlinks in path checks') which landed on `main` before this PR was rebased. `_resolve_snapshots` and `clip_snapshot_image` both now call `.resolve()` on both sides before the `is_relative_to` check. --- ### ✅ Not actioned: `allowed_roots` duplication (comments 16633706, 16633709, 16633763, 16633790) — stale Already fixed by the `_build_allowed_roots(cfg)` helper introduced in `1c0b46e`. Both route handlers now call it. --- ### ✅ Not actioned: `_safe_filename` is dead code (comment 16639865) `_safe_filename` is active code, called at lines 67 and 132 of `lifespan.py` in both `_scan_camera` and the `_on_file` watcher handler. --- ### ✅ Not actioned: `str | None` annotation style (comment 16633703) `str | None` is the correct Python 3.10+ union syntax and is preferred over `Optional[str]` in this codebase (Python ≥ 3.11 per project config). No change needed. --- ### ✅ Not actioned: regex recompilation (comment 16633745) `_LEADING_DIGITS_RE` is already compiled once at module level (line 13 of `snapshots.py`). There is no per-call compilation. --- ### ✅ Not actioned: `collect_snapshots_from_output` empty-directory case (comment 16633721) `sorted(clip_dir.glob("**/*.jpg"))` evaluates to `[]` when the directory contains no `.jpg` files. The case is already handled correctly. --- ### ✅ Not actioned: `mkdir` inside copy loop (comments 16639859, 16639883) Each snapshot maps to a unique subdirectory within `clip_dir` (Dahua structure: `<date>/<channel>/jpg/<HH>/<MM>/`). The `dst.parent.mkdir(parents=True, exist_ok=True)` call must stay per-snapshot because different snapshots land in different directories. The `exist_ok=True` flag makes re-entrant calls cheap; extracting a pre-pass would add complexity without meaningfully reducing I/O. --- ### ✅ Not actioned: fallback logic duplicated in two routes (comment 16639862) The two-line pattern appears in `clip_snapshot_album` and `clip_snapshot_image`. Extracting a helper would require threading `cfg` and `clip` through another function for negligible gain. Left as-is per the project's guideline against premature abstractions. --- ### ✅ Not actioned: test path construction duplication (comment 16639868) The snapshot directory paths are constructed in two distinct tests with different assertions. Extracting a fixture would obscure what each test is actually setting up. --- ### ✅ Not actioned: recursive glob performance (comments 16639874, 16633760) `collect_snapshots_from_output` is a fallback path called at most once per UI request, and only when camera-root snapshots are absent. The `**/*.jpg` glob is correct and readable; micro-optimising a cold path is not warranted. --- ### ✅ Not actioned: `sorted()` materialises full list (comment 16639877) The sorted list is required for stable integer indexing (`snapshots[n]`). A lazy generator cannot be indexed. No change possible. --- ### ✅ Not actioned: `_done` closure duplication (comments 16633712, 16633715) The two `_done` closures capture different outer-scope bindings (`cam.root` vs `_cam.root`, `clip` vs `match`). Extracting a shared factory would require passing all five captured variables as arguments, adding indirection without reducing lines. --- ### ✅ Not actioned: per-request scan caching (comments 16633751, 16633754) Caching scan results across HTTP requests is a separate architectural concern unrelated to this PR. Left for a dedicated refactor if needed. --- ### ✅ Not actioned: SRP violation in `_persist_snapshots` (comment 16633781) The function performs one logical operation: persist a clip's snapshot assets. Copying all frames and deriving the thumbnail are inseparable steps of that single responsibility. --- ### ✅ Not actioned: fallback chain fragility (comment 16633793) The three-step chain (`collect_snapshots_for_clip` → `collect_snapshots_from_output` → `_resolve_snapshots`) has clear, linear semantics. Each step handles a distinct case. --- ### ✅ Not actioned: `_resolve_snapshots` naming (comment 16633784) `_resolve_snapshots` pre-dates this PR. Renaming it is out of scope. --- ### ✅ Not actioned: directory-structure coupling (comment 16633787) `collect_snapshots_from_output` and `_persist_snapshots` must agree on the directory layout — that is intentional and documented in `collect_snapshots_from_output`'s docstring. --- ### ✅ Not actioned: `_snapshot_label` called per-frame in template (comment 16633766) Pre-existing code outside the scope of this PR. --- ### ✅ Not actioned: `allowed_roots` per-request cost (comment 16633748) `_build_allowed_roots` is a single comprehension over a small in-memory list. The cost is negligible and no caching is needed.

No reviewers

No labels

No milestone

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

marvin8/cang!93

No description provided.

Rows
Columns

fix: copy all clip snapshots to output_dir on ingest #93

🔴 Fixed: _persist_snapshots blocks the event loop (comments 16639880, 16633757)

🔴 Fixed: OSError silently discarded (comment 16633796)

✅ Not actioned: symlink path-traversal (comments 16633676, 16633679, 16633682) — stale

✅ Not actioned: allowed_roots duplication (comments 16633706, 16633709, 16633763, 16633790) — stale

✅ Not actioned: _safe_filename is dead code (comment 16639865)

✅ Not actioned: str | None annotation style (comment 16633703)