From e2479f1f6bebcd4f20b8e3bf532b8c77b845a2a0 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 18 Sep 2026 05:48:34 +0000 Subject: [PATCH 1/3] =?UTF-8?q?feat(clips):=20M7=20=E2=80=94=20non-destruc?= =?UTF-8?q?tive=20clips,=20sub-video=20extraction,=20delete=20guard?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Non-destructive clips are Asset rows with no storage_key of their own — a window into the parent's bytes, bounded by in_point/out_point, zero storage and zero processing. Sub-video extraction is a real ffmpeg cut, run as a job: a fast stream-copy path with an automatic re-encode fallback when the requested range doesn't land on a keyframe closely enough. Deleting an asset with live clips is now blocked with a 409 naming them, and promoting each one in place clears the block. Backend: parent_asset_id/in_point/out_point on Asset (with a real foreign key, so SQLite itself enforces the guard); KIND_EXTRACT_SUBVIDEO dispatched through the existing job queue; a clip guard in enrichment/source.py so describe/summarize/autotag/generate_all refuse a clip instead of silently describing a generic poster frame; a result_asset_id on finished jobs so the frontend can learn what a fresh extraction created. Frontend: a Clip tab (in/out point editor, save-as-clip, extract-as- subvideo), playback bounded to a clip's range, AI enrichment actions hidden for a clip, and a promote-then-retry flow on the delete guard. Verified against real ffmpeg (installed for this session rather than relying on the test suite's skip marker), which caught two test bugs a skipped run would have hidden. 829 backend tests, 264 frontend tests, clean typecheck/lint/build. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01KzCKjg6yBtAwMFwZmezuw1 --- ...20260918_0900_add_clip_columns_to_asset.py | 45 ++ backend/app/enrichment/extract_subvideo.py | 122 ++++ backend/app/enrichment/source.py | 9 + backend/app/enrichment/subvideo.py | 133 +++++ backend/app/ingest/filetypes.py | 7 + backend/app/jobs/enrichment.py | 30 +- backend/app/jobs/registry.py | 21 +- backend/app/main.py | 2 + backend/app/models/asset.py | 19 +- backend/app/models/job.py | 7 + backend/app/routers/assets.py | 38 +- backend/app/routers/clips.py | 185 ++++++ backend/app/routers/search.py | 11 +- backend/app/schemas_assets.py | 7 + backend/app/schemas_jobs.py | 6 + backend/app/services/assets.py | 244 +++++++- backend/app/storage/base.py | 10 + backend/app/storage/local.py | 31 + backend/tests/test_clips_api.py | 561 ++++++++++++++++++ backend/tests/test_subvideo_extraction.py | 109 ++++ docs/m7-clips-and-subvideos.md | 273 +++++++++ docs/plan-of-attack.md | 7 + frontend/src/api/assets.ts | 9 + frontend/src/api/clips.ts | 59 ++ frontend/src/api/transcripts.ts | 13 + .../src/components/ActivityIndicator.test.tsx | 1 + frontend/src/components/AssetDetail.tsx | 264 +++++++-- frontend/src/components/AssetThumb.test.tsx | 3 + frontend/src/components/ClipEditor.tsx | 222 +++++++ frontend/src/components/EmbedButton.test.tsx | 1 + .../src/components/EnrichmentButton.test.tsx | 1 + frontend/src/components/EnrichmentButton.tsx | 12 +- frontend/src/components/SelectionBar.test.tsx | 4 + .../src/components/SuggestionPanel.test.tsx | 1 + .../src/components/TranscriptPanel.test.tsx | 4 + frontend/src/stores/activity.test.ts | 1 + frontend/src/stores/library.test.ts | 3 + frontend/src/views/AssetView.test.tsx | 189 +++++- frontend/src/views/LibraryView.test.tsx | 3 + frontend/src/views/SearchView.test.tsx | 3 + frontend/src/views/SettingsView.test.tsx | 1 + 41 files changed, 2597 insertions(+), 74 deletions(-) create mode 100644 backend/alembic/versions/20260918_0900_add_clip_columns_to_asset.py create mode 100644 backend/app/enrichment/extract_subvideo.py create mode 100644 backend/app/enrichment/subvideo.py create mode 100644 backend/app/routers/clips.py create mode 100644 backend/tests/test_clips_api.py create mode 100644 backend/tests/test_subvideo_extraction.py create mode 100644 docs/m7-clips-and-subvideos.md create mode 100644 frontend/src/api/clips.ts create mode 100644 frontend/src/components/ClipEditor.tsx diff --git a/backend/alembic/versions/20260918_0900_add_clip_columns_to_asset.py b/backend/alembic/versions/20260918_0900_add_clip_columns_to_asset.py new file mode 100644 index 0000000..8addd50 --- /dev/null +++ b/backend/alembic/versions/20260918_0900_add_clip_columns_to_asset.py @@ -0,0 +1,45 @@ +"""add clip columns to asset + +M7's clip and sub-video extraction both need a parent pointer and a time range. +`parent_asset_id` carries a real foreign key, unlike `transcriptsegment`/ +`documentpage`'s no-FK-plus-explicit-cleanup pattern: a live clip (`storage_key IS +NULL`) is meant to *block* its parent's deletion until it is promoted or removed, and +SQLite enforcing that is the safety net behind `services/assets.py::delete_asset`'s own +guard — the same belt-and-suspenders role the FK already plays for `assettag` and +`suggestion`. No prior migration adds a foreign key to a table that already exists (both +of those got theirs at `create_table` time); `create_foreign_key` inside batch mode is +what SQLite's "recreate the table" batch strategy needs to add one after the fact. + +Revision ID: 7d4b9c1a6f28 +Revises: 56ac14e89a0c +Create Date: 2026-09-18 09:00:00.000000+00:00 +""" +from typing import Sequence, Union + +from alembic import op +import sqlalchemy as sa +import sqlmodel + + +revision: str = '7d4b9c1a6f28' +down_revision: Union[str, None] = '56ac14e89a0c' +branch_labels: Union[str, Sequence[str], None] = None +depends_on: Union[str, Sequence[str], None] = None + + +def upgrade() -> None: + with op.batch_alter_table('asset', schema=None) as batch_op: + batch_op.add_column(sa.Column('parent_asset_id', sqlmodel.sql.sqltypes.AutoString(), nullable=True)) + batch_op.add_column(sa.Column('in_point', sa.Float(), nullable=True)) + batch_op.add_column(sa.Column('out_point', sa.Float(), nullable=True)) + batch_op.create_index('ix_asset_parent_asset_id', ['parent_asset_id'], unique=False) + batch_op.create_foreign_key('fk_asset_parent_asset_id_asset', 'asset', ['parent_asset_id'], ['id']) + + +def downgrade() -> None: + with op.batch_alter_table('asset', schema=None) as batch_op: + batch_op.drop_constraint('fk_asset_parent_asset_id_asset', type_='foreignkey') + batch_op.drop_index('ix_asset_parent_asset_id') + batch_op.drop_column('out_point') + batch_op.drop_column('in_point') + batch_op.drop_column('parent_asset_id') diff --git a/backend/app/enrichment/extract_subvideo.py b/backend/app/enrichment/extract_subvideo.py new file mode 100644 index 0000000..4a715de --- /dev/null +++ b/backend/app/enrichment/extract_subvideo.py @@ -0,0 +1,122 @@ +"""The extract_subvideo job: a parent's bytes in, a standalone clip file out. + +Two modes carried in the job's `payload` (see `models/job.py::KIND_EXTRACT_SUBVIDEO`): +`"extract"` cuts a fresh range straight from a file-owning asset, leaving it untouched; +`"promote"` does the same cut but writes the result back into an *existing* clip row +(one with no `storage_key` of its own) rather than creating a new one — the one-click +"make this reference a real file" path a blocked delete offers. Both go through the +same ffmpeg mechanics in `enrichment/subvideo.py` and the same Asset-row plumbing in +`services/assets.py`, which is where sub-video creation was always meant to land (see +that module's own docstring). + +Runs on a worker thread, so it owns its own session and reports progress through the +callback the queue hands it — which is also the cancellation checkpoint. +""" + +from __future__ import annotations + +import json +import logging +import tempfile +from pathlib import Path +from typing import Callable, Optional + +from sqlmodel import Session + +from app.enrichment.subvideo import SubvideoExtractionError, extract_subvideo +from app.ingest import thumbnails +from app.ingest.filetypes import TYPE_AUDIO +from app.ingest.probe import probe +from app.models.asset import Asset +from app.services import assets as asset_service +from app.storage import build_storage, new_key, thumb_key_for + +logger = logging.getLogger(__name__) + +Progress = Callable[[str, int, str], None] + + +def run( + session: Session, asset: Asset, payload: Optional[str], progress: Progress +) -> tuple[str, Optional[str]]: + """Run one extraction or promotion. Returns (detail, created_asset_id). + + `created_asset_id` is only set for `"extract"` mode — the frontend has nowhere + else to learn the new asset's id, since the job's own `asset_id` stays pointed at + the *source*, not the row that does not exist until this returns. `"promote"` + updates the clip's own row in place, so the frontend already has that id: it is + `job.asset_id` throughout, and nothing new needs reporting. + """ + try: + data = json.loads(payload or "{}") + except ValueError: + data = {} + mode = data.get("mode") + + if mode == "promote": + clip = asset + if clip.storage_key is not None or not clip.parent_asset_id: + raise SubvideoExtractionError("This asset is not a clip") + source_asset = session.get(Asset, clip.parent_asset_id) + if source_asset is None or not source_asset.storage_key: + raise SubvideoExtractionError("The original asset is no longer available") + in_point = clip.in_point or 0.0 + out_point = clip.out_point or 0.0 + else: + source_asset = asset + if not source_asset.storage_key: + raise SubvideoExtractionError("The source file is not available") + try: + in_point = float(data["in_point"]) + out_point = float(data["out_point"]) + except (KeyError, TypeError, ValueError) as exc: + raise SubvideoExtractionError("Missing or invalid in/out points") from exc + + storage = build_storage() + progress("Extracting", 10, "") + + # Always .mp4 (.m4a for an audio source): both ffmpeg strategies in subvideo.py + # either stream-copy into it or re-encode to h264/aac, which is always valid + # content for this container regardless of what the source's own format was — + # simpler than trying to match the source's extension, and the re-encode fallback + # already recovers automatically if a stream copy into it fails. + extension = ".m4a" if source_asset.asset_type == TYPE_AUDIO else ".mp4" + + with storage.materialise(source_asset.storage_key) as source_path: + with tempfile.TemporaryDirectory(prefix="gam-subvideo-") as workdir: + target_path = Path(workdir) / f"subvideo{extension}" + extract_subvideo(source_path, target_path, in_point, out_point) + + progress("Reading metadata", 60, "") + result = probe(target_path) + preview = thumbnails.generate( + target_path, + source_asset.asset_type, + source_asset.original_name or "", + duration_seconds=result.duration_seconds, + ) + + progress("Saving", 80, "") + stored = storage.write_file(new_key(source_asset.user_id, extension), target_path) + thumb_key = None + if preview: + thumb = storage.write_bytes(thumb_key_for(stored.key), preview) + thumb_key = thumb.key + + detail = f"{out_point - in_point:.0f}s sub-video" + + if mode == "promote": + asset_service.promote_clip(session, clip, stored=stored, thumb_key=thumb_key, probe_result=result) + return detail, None + + created = asset_service.create_subvideo_asset( + session, + source_asset=source_asset, + stored=stored, + thumb_key=thumb_key, + in_point=in_point, + out_point=out_point, + name=data.get("name"), + probe_result=result, + ) + return detail, created.id diff --git a/backend/app/enrichment/source.py b/backend/app/enrichment/source.py index d02c12c..af9fd3a 100644 --- a/backend/app/enrichment/source.py +++ b/backend/app/enrichment/source.py @@ -147,6 +147,15 @@ def gather( a summary what it is *about*. The transcript stays primary either way; this adds to it, and does not reorder the precedence the milestone doc sets out. """ + if asset.storage_key is None and asset.parent_asset_id: + # A clip (M7). It has no transcript of its own — only the parent does, for the + # whole recording rather than this range — and it inherits the parent's + # `thumb_key` (see `services/assets.py::create_clip`), so without this check + # the poster-fallback branch below would silently "succeed" using a generic + # frame instead of ever raising. That produces a description of the wrong + # content rather than failing loudly, which is worse than refusing outright. + raise NoSourceMaterial("This is a clip. Describe, summarise or tag the original asset instead.") + text = transcript_text(session, asset) if text: truncated = len(text) > MAX_TRANSCRIPT_CHARS diff --git a/backend/app/enrichment/subvideo.py b/backend/app/enrichment/subvideo.py new file mode 100644 index 0000000..665f9d0 --- /dev/null +++ b/backend/app/enrichment/subvideo.py @@ -0,0 +1,133 @@ +"""Cutting a time range out of a video or audio file. + +Two ffmpeg strategies, tried in order. The fast one seeks on the input side and +stream-copies — no re-encode, so it runs in roughly the time it takes to read the +bytes — but a stream copy can only start on a keyframe, so the seek lands at the +nearest one at or before `in_point`, not exactly on it. For most footage that is a +fraction of a second and nobody notices; for content with sparse keyframes (a screen +recording, a slideshow-style video) it can be seconds off, which is wrong enough to +matter for a "cut exactly this moment" feature. So the fast output is checked against +the requested duration, and only re-encoded — output-side seeking, frame-accurate, +slower — when it actually missed by enough to matter. +""" + +from __future__ import annotations + +import logging +import subprocess +from pathlib import Path + +from app.ingest.probe import probe +from app.media_tools import ffmpeg_available + +logger = logging.getLogger(__name__) + +# How far the fast path's actual duration may overshoot the requested one before it is +# considered inaccurate enough to redo. Generous relative to a typical keyframe +# interval (2-10s on most encoders), stingy relative to what "cut at 01:01" promises. +DURATION_TOLERANCE_SECONDS = 0.5 + + +class SubvideoExtractionError(Exception): + """Something the user can be told about.""" + + +# Same shape as `enrichment/audio.py::timeout_for`: scales with the length of the cut, +# not the source file, since that is what ffmpeg actually has to process either way. +def timeout_for(duration_seconds: float | None) -> int: + if not duration_seconds or duration_seconds <= 0: + return 300 + return int(min(max(duration_seconds * 2, 120), 3600)) + + +def extract_subvideo(source: Path, target: Path, in_point: float, out_point: float) -> Path: + """Write `source`'s [in_point, out_point) range to `target`. + + Takes absolute coordinates against a real (non-clip) source — resolving a clip's + own in/out against its parent is the caller's job, not this function's, and M7 does + not need it (clipping a clip is out of scope; see `services/assets.py::create_clip`). + """ + if not ffmpeg_available(): + raise SubvideoExtractionError("ffmpeg is not available on this server") + if out_point <= in_point: + raise SubvideoExtractionError("The out point must be after the in point") + + duration = out_point - in_point + target.parent.mkdir(parents=True, exist_ok=True) + timeout = timeout_for(duration) + + if _fast_copy(source, target, in_point, duration, timeout): + actual = probe(target).duration_seconds + if actual is not None and actual <= duration + DURATION_TOLERANCE_SECONDS: + return target + logger.info( + "Stream-copy cut of %s overshot (wanted %.2fs, got %s) — re-encoding for accuracy", + source.name, duration, actual, + ) + + _reencode(source, target, in_point, out_point, timeout) + return target + + +def _fast_copy(source: Path, target: Path, in_point: float, duration: float, timeout: int) -> bool: + """Input-side seek plus stream copy. Returns whether it produced a usable file.""" + argv = [ + "ffmpeg", "-nostdin", + # Before -i: ffmpeg seeks to the nearest keyframe rather than decoding and + # discarding everything up to it, which is the whole point of the fast path. + "-ss", f"{in_point:.3f}", + "-i", str(source), + "-t", f"{duration:.3f}", + "-c", "copy", + # A stream copy starting mid-file otherwise carries the original timestamps, + # which some players render as a long stall before playback begins. + "-avoid_negative_ts", "make_zero", + "-y", str(target), + ] + try: + completed = subprocess.run(argv, capture_output=True, text=True, timeout=timeout) + except (subprocess.TimeoutExpired, OSError) as exc: + logger.info("Stream-copy cut of %s failed: %s", source.name, exc) + return False + + if completed.returncode != 0: + logger.info( + "Stream-copy cut of %s failed: %s", source.name, (completed.stderr or "").strip()[-400:] + ) + return False + return target.is_file() and target.stat().st_size > 0 + + +def _reencode(source: Path, target: Path, in_point: float, out_point: float, timeout: int) -> None: + """Output-side seek, frame-accurate, always re-encodes.""" + argv = [ + "ffmpeg", "-nostdin", + "-i", str(source), + # After -i: ffmpeg decodes from the start and discards up to the mark, which is + # what makes this land exactly on `in_point` rather than the nearest keyframe. + "-ss", f"{in_point:.3f}", + "-to", f"{out_point:.3f}", + "-c:v", "libx264", + "-preset", "veryfast", + "-c:a", "aac", + # Moves the MP4 index to the front of the file, so playback can start before + # the whole file has downloaded — the same reason every upload gets it free + # from most modern encoders, made explicit here since libx264 does not default + # to it. + "-movflags", "+faststart", + "-y", str(target), + ] + try: + completed = subprocess.run(argv, capture_output=True, text=True, timeout=timeout) + except subprocess.TimeoutExpired as exc: + raise SubvideoExtractionError("Extracting the sub-video took too long") from exc + except OSError as exc: + raise SubvideoExtractionError(f"Could not run ffmpeg: {exc}") from exc + + if completed.returncode != 0: + stderr = (completed.stderr or "").strip() + logger.warning("Sub-video extraction failed for %s: %s", source.name, stderr[-400:]) + raise SubvideoExtractionError("Could not extract that range from this file") + + if not target.is_file() or target.stat().st_size == 0: + raise SubvideoExtractionError("Could not extract that range from this file") diff --git a/backend/app/ingest/filetypes.py b/backend/app/ingest/filetypes.py index c8f9a71..c978ced 100644 --- a/backend/app/ingest/filetypes.py +++ b/backend/app/ingest/filetypes.py @@ -44,6 +44,13 @@ SOURCE_URL = "url" SOURCE_AI = "ai_generated" SOURCE_GVC = "gvc_export" +# M7. A clip owns no bytes of its own (`Asset.storage_key IS NULL`) — this is the +# frontend-facing signal for that fact, not the guard itself; `services/assets.py`'s +# delete guard checks `storage_key` directly, since that is what a delete actually +# depends on being true. Promoting a clip, or extracting one fresh from a parent, +# flips `source` to `SOURCE_SUBVIDEO` — a real, standalone file from here on. +SOURCE_CLIP = "clip" +SOURCE_SUBVIDEO = "sub_video" # Pillow can decode these directly. The rest of the image set (HEIC, AVIF without a # plugin, SVG) needs something else, so a thumbnail is skipped rather than attempted. diff --git a/backend/app/jobs/enrichment.py b/backend/app/jobs/enrichment.py index e8646e6..ac83cb2 100644 --- a/backend/app/jobs/enrichment.py +++ b/backend/app/jobs/enrichment.py @@ -7,6 +7,7 @@ from __future__ import annotations +import json import logging from typing import Optional @@ -22,10 +23,12 @@ from app.enrichment.autotag import run as run_autotag from app.enrichment.bulk import run as run_bulk from app.enrichment.describe import run as run_describe +from app.enrichment.extract_subvideo import run as run_extract_subvideo from app.enrichment.extract_text import TextExtractionError from app.enrichment.extract_text import run as run_extract_text from app.enrichment.generate_all import run as run_generate_all from app.enrichment.source import NoSourceMaterial +from app.enrichment.subvideo import SubvideoExtractionError from app.enrichment.summarize import run as run_summarize from app.enrichment.transcribe import TranscriptionError from app.enrichment.transcribe import run as run_transcribe @@ -45,6 +48,7 @@ KIND_BULK_ENRICH, KIND_DESCRIBE, KIND_EMBED, + KIND_EXTRACT_SUBVIDEO, KIND_EXTRACT_TEXT, KIND_GENERATE_ALL, KIND_SUMMARIZE, @@ -147,11 +151,18 @@ def submit_library( return job -def submit(session: Session, asset: Asset, kind: str) -> EnrichmentJob: +def submit( + session: Session, asset: Asset, kind: str, *, payload: Optional[str] = None +) -> EnrichmentJob: """Create a job row and hand it to the queue. The row is committed before the queue is told about it. The worker looks the job up by id in its own session, so enqueueing first is a race it can lose. + + `payload` is optional and defaults to `None` for every existing caller — only + `KIND_EXTRACT_SUBVIDEO` needs it today, to carry in/out points and mode in rather + than inventing a second submit function for the one kind that needs more than an + asset. `submit_library` already takes one for the same reason, one level up. """ job = EnrichmentJob( user_id=asset.user_id, @@ -160,6 +171,7 @@ def submit(session: Session, asset: Asset, kind: str) -> EnrichmentJob: status="queued", stage="Queued", asset_name=asset.name, + payload=payload, ) session.add(job) session.commit() @@ -226,6 +238,16 @@ def _run_job(job_id: str) -> None: detail = f"{count} vector{'' if count == 1 else 's'}" elif job.kind == KIND_EXTRACT_TEXT: detail = run_extract_text(session, asset, progress) + elif job.kind == KIND_EXTRACT_SUBVIDEO: + detail, created_asset_id = run_extract_subvideo(session, asset, job.payload, progress) + # Only "extract" mode returns one — "promote" mutates `asset` (the + # clip) in place, and the frontend already has that id as `job.asset_id` + # throughout, so there is nothing new to report. Set directly on the + # live `job` row rather than threaded through `set_fields` below: that + # call's kwargs are the terminal-status fields, and `payload` is not + # one of them, so this survives it untouched. + if created_asset_id: + job.payload = json.dumps({"created_asset_id": created_asset_id}) elif job.kind == KIND_SUMMARIZE: detail = run_summarize(session, asset, progress) elif job.kind == KIND_AUTOTAG: @@ -306,6 +328,12 @@ def _run_job(job_id: str) -> None: set_fields(session, job, status="error", stage="", error_message=message) logger.info("Enrichment job %s failed: %s", job_id, message) + except SubvideoExtractionError as exc: + message = str(exc) + _mark_asset_failed(session, asset, job) + set_fields(session, job, status="error", stage="", error_message=message) + logger.info("Enrichment job %s failed: %s", job_id, message) + except TranscriptionError as exc: message = str(exc) _mark_asset_failed(session, asset, job) diff --git a/backend/app/jobs/registry.py b/backend/app/jobs/registry.py index b09e06c..a8948b1 100644 --- a/backend/app/jobs/registry.py +++ b/backend/app/jobs/registry.py @@ -14,13 +14,14 @@ from __future__ import annotations +import json from datetime import datetime from typing import Any, Callable, Dict, List, Optional, Type from sqlmodel import Session, col, select from app.jobs.runner import ACTIVE_STATUSES, JobQueue, is_stale -from app.models.job import EnrichmentJob +from app.models.job import KIND_EXTRACT_SUBVIDEO, EnrichmentJob from app.schemas_jobs import ActivityJobRead @@ -40,6 +41,23 @@ def __init__( self.queue_for = queue_for +def _result_asset_id(job: EnrichmentJob) -> Optional[str]: + """The asset a `KIND_EXTRACT_SUBVIDEO` "extract" job created, once it has. + + Best-effort, the same defensive shape `routers/transcripts.py::_words_of` uses to + read a JSON-as-TEXT column that might be empty, or — for every other kind, whose + payload means something else entirely — simply not have this key. + """ + if job.kind != KIND_EXTRACT_SUBVIDEO or not job.payload: + return None + try: + data = json.loads(job.payload) + except ValueError: + return None + value = data.get("created_asset_id") if isinstance(data, dict) else None + return value if isinstance(value, str) else None + + def _enrichment_to_activity(job: EnrichmentJob) -> ActivityJobRead: return ActivityJobRead( id=job.id, @@ -56,6 +74,7 @@ def _enrichment_to_activity(job: EnrichmentJob) -> ActivityJobRead: asset_id=job.asset_id, asset_name=job.asset_name or "", model=job.model or "", + result_asset_id=_result_asset_id(job), error_message=job.error_message, created_at=job.created_at, updated_at=job.updated_at, diff --git a/backend/app/main.py b/backend/app/main.py index cae3054..409a1e0 100644 --- a/backend/app/main.py +++ b/backend/app/main.py @@ -24,6 +24,7 @@ from app.database import engine, get_session from app.routers import activity as activity_router from app.routers import assets as assets_router +from app.routers import clips as clips_router from app.routers import embeddings as embeddings_router from app.routers import enrichment as enrichment_router from app.routers import media as media_router @@ -160,6 +161,7 @@ def me( # say so rather than inventing a parallel /api/transcripts tree. app.include_router(transcripts_router.router, prefix="/api/assets", tags=["transcripts"]) app.include_router(enrichment_router.router, prefix="/api/assets", tags=["enrichment"]) +app.include_router(clips_router.router, prefix="/api/assets", tags=["clips"]) app.include_router(activity_router.router, prefix="/api/activity", tags=["activity"]) app.include_router(search_router.router, prefix="/api/search", tags=["search"]) app.include_router(embeddings_router.router, prefix="/api/embeddings", tags=["embeddings"]) diff --git a/backend/app/models/asset.py b/backend/app/models/asset.py index 0f1d37b..751c06b 100644 --- a/backend/app/models/asset.py +++ b/backend/app/models/asset.py @@ -21,9 +21,8 @@ class Asset(SQLModel, table=True): search, filter and tag query became a union — and "show me everything about the Giordano interview" is the query this product exists to answer. - Fields for those later milestones are not declared yet. Adding a column is one - Alembic migration, which is exactly why the project has Alembic; carrying six - milestones of unused columns is not. + M8's fields are not declared yet — that is still one Alembic migration away, and + carrying unused columns for a milestone not yet started is not worth it. """ # The library listing is "my assets, newest first", and it is the hottest query in @@ -51,6 +50,20 @@ class Asset(SQLModel, table=True): storage_key: Optional[str] = None thumb_key: Optional[str] = None + # ─── clips (M7) ────────────────────────────────────────────────────────── + # A clip is *identified* by `storage_key IS NULL AND parent_asset_id IS NOT + # NULL` — this column is the physical fact a delete guard depends on, not a label. + # A real foreign key (not the transcriptsegment/documentpage pattern of no FK plus + # explicit cleanup): unlike those, a live clip is meant to *block* its parent's + # deletion until it is promoted or removed, and SQLite enforcing that is the + # safety net behind `services/assets.py::delete_asset`'s own guard, the same + # belt-and-suspenders role the FK already plays for `assettag`/`suggestion`. + parent_asset_id: Optional[str] = Field(default=None, foreign_key="asset.id", index=True) + # Seconds into the parent. Both set together, always by the code that creates the + # clip or extraction — never edited afterwards (M7 does not build a trim editor). + in_point: Optional[float] = None + out_point: Optional[float] = None + original_name: Optional[str] = None mime_type: Optional[str] = None file_format: Optional[str] = None # extension without the dot, e.g. "mp4" diff --git a/backend/app/models/job.py b/backend/app/models/job.py index e549cf0..7baace3 100644 --- a/backend/app/models/job.py +++ b/backend/app/models/job.py @@ -38,6 +38,13 @@ # One action applied across a chosen set of assets. Which action, and which assets, # live in `EnrichmentJob.payload` — see there for why it is one job and not N. KIND_BULK_ENRICH = "bulk_enrich" +# M7. Cutting a time range with ffmpeg — either into a brand-new standalone asset, or +# in place over an existing clip (`payload["mode"]` says which; see +# `enrichment/extract_subvideo.py`). Deliberately outside `ENRICHMENT_KINDS`: that set +# gates M6's AI-cost bulk-enrichment selection UI, and this calls no provider and +# costs nothing, the same reason `KIND_EXTRACT_TEXT` sits apart from the LLM jobs +# despite also being per-asset. +KIND_EXTRACT_SUBVIDEO = "extract_subvideo" # Per-asset actions. Everything in here requires an `asset_id`. ENRICHMENT_KINDS = frozenset( diff --git a/backend/app/routers/assets.py b/backend/app/routers/assets.py index d8767e8..c9864c7 100644 --- a/backend/app/routers/assets.py +++ b/backend/app/routers/assets.py @@ -211,12 +211,19 @@ def list_assets( .offset(offset) ).all() - # One query for the whole page's tags, not one per row. + # One query for the whole page's tags, and one for however many of them are clips' + # parents — not one of either per row. tags_by_asset = tag_service.tags_for_many(session, [row.id for row in rows]) + parents_by_id = service.parents_for_many(session, rows) return ListResponse[AssetRead]( data=[ - service.to_read_model(row, storage, tags_by_asset.get(row.id, [])) + service.to_read_model( + row, + storage, + tags_by_asset.get(row.id, []), + parent=parents_by_id.get(row.parent_asset_id), + ) for row in rows ], total=total, @@ -245,8 +252,11 @@ def get_asset( # Tags loaded explicitly, like the list endpoint does. `to_read_model` defaults them # to empty, so omitting this does not fail — it silently returns an untagged asset, # and the store believes it. + parent = session.get(Asset, asset.parent_asset_id) if asset.parent_asset_id else None return DataResponse( - data=service.to_read_model(asset, storage, tag_service.tags_for(session, asset.id)) + data=service.to_read_model( + asset, storage, tag_service.tags_for(session, asset.id), parent=parent + ) ) @@ -271,8 +281,11 @@ def update_asset( # Same reason as the read above, and it bites harder here: the library store replaces # its copy with whatever this returns, so a response with empty tags makes an # asset's tags disappear from the grid after a rename until the page is reloaded. + parent = session.get(Asset, asset.parent_asset_id) if asset.parent_asset_id else None return DataResponse( - data=service.to_read_model(asset, storage, tag_service.tags_for(session, asset.id)) + data=service.to_read_model( + asset, storage, tag_service.tags_for(session, asset.id), parent=parent + ) ) @@ -292,6 +305,23 @@ def delete_asset( storage: LocalStorage = Depends(get_storage), ) -> None: asset = _owned(asset_id, user.id, session) + + # Checked here, before `delete_asset` touches anything: a live clip (M7) is meant + # to block this, and reporting how many rather than just refusing is what lets the + # frontend offer "promote them, then delete" instead of a dead end. + blocking = service.blocking_clips(session, asset.id) + if blocking: + raise HTTPException( + status_code=status.HTTP_409_CONFLICT, + detail={ + "code": "asset_has_dependent_clips", + "message": ( + f"{len(blocking)} clip{'' if len(blocking) == 1 else 's'} depend on " + "this asset. Extract them as sub-videos first." + ), + }, + ) + service.delete_asset(session, storage, asset) diff --git a/backend/app/routers/clips.py b/backend/app/routers/clips.py new file mode 100644 index 0000000..2ed3bbf --- /dev/null +++ b/backend/app/routers/clips.py @@ -0,0 +1,185 @@ +"""Clips (non-destructive) and sub-video extraction (destructive), over an asset.""" + +from __future__ import annotations + +import json +from typing import Optional + +from fastapi import APIRouter, Depends, HTTPException, status +from pydantic import BaseModel, Field +from sqlmodel import Session + +from app.auth import CurrentUser +from app.database import get_session +from app.ingest.filetypes import TYPE_AUDIO, TYPE_VIDEO +from app.jobs import enrichment as enrichment_jobs +from app.jobs.registry import KINDS +from app.models.asset import Asset +from app.models.job import KIND_EXTRACT_SUBVIDEO +from app.routers.assets import get_storage +from app.schemas import DataResponse, ListResponse +from app.schemas_assets import AssetRead +from app.schemas_jobs import ActivityJobRead +from app.services import assets as asset_service +from app.services import tags as tag_service +from app.storage import LocalStorage + +router = APIRouter() + + +def _owned_asset(asset_id: str, user_id: str, session: Session) -> Asset: + asset = session.get(Asset, asset_id) + if not asset or asset.user_id != user_id: + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, + detail={"code": "not_found", "message": "No such asset"}, + ) + return asset + + +def _require_no_active_extraction(session: Session, asset_id: str) -> None: + # One at a time per asset, same reason `start_transcription` checks this: two + # concurrent runs would race to write the same clip row, or produce two standalone + # assets for the same request. + if enrichment_jobs.active_job(session, asset_id, KIND_EXTRACT_SUBVIDEO) is not None: + raise HTTPException( + status_code=status.HTTP_409_CONFLICT, + detail={ + "code": "already_running", + "message": "A sub-video extraction is already running for this asset", + }, + ) + + +class ClipCreate(BaseModel): + in_point: float = Field(ge=0) + out_point: float = Field(gt=0) + name: Optional[str] = Field(default=None, min_length=1, max_length=255) + + +class SubvideoCreate(BaseModel): + in_point: float = Field(ge=0) + out_point: float = Field(gt=0) + name: Optional[str] = Field(default=None, min_length=1, max_length=255) + + +# ─── non-destructive clips ───────────────────────────────────────────────────── + + +@router.post( + "/{asset_id}/clips", response_model=DataResponse[AssetRead], status_code=status.HTTP_201_CREATED +) +def create_clip( + asset_id: str, + payload: ClipCreate, + user: CurrentUser, + session: Session = Depends(get_session), + storage: LocalStorage = Depends(get_storage), +) -> DataResponse[AssetRead]: + """A window into the parent's bytes: no file, no ffmpeg, no job — a 201, not a 202.""" + parent = _owned_asset(asset_id, user.id, session) + try: + clip = asset_service.create_clip( + session, + parent, + in_point=payload.in_point, + out_point=payload.out_point, + name=payload.name, + ) + except ValueError as exc: + raise HTTPException( + status_code=status.HTTP_400_BAD_REQUEST, + detail={"code": "invalid_clip_range", "message": str(exc)}, + ) from exc + return DataResponse(data=asset_service.to_read_model(clip, storage, parent=parent)) + + +@router.get("/{asset_id}/clips", response_model=ListResponse[AssetRead]) +def list_clips( + asset_id: str, + user: CurrentUser, + session: Session = Depends(get_session), + storage: LocalStorage = Depends(get_storage), +) -> ListResponse[AssetRead]: + """Everything derived from this asset: live clips and past promotions/extractions. + + Unfiltered — a "Clips" tab wants the whole history, and the delete guard's promote + UI narrows this to `source == "clip"` itself rather than needing a second endpoint. + """ + parent = _owned_asset(asset_id, user.id, session) + rows = asset_service.list_children(session, parent.id, user.id) + tags_by_asset = tag_service.tags_for_many(session, [row.id for row in rows]) + return ListResponse[AssetRead]( + data=[ + asset_service.to_read_model(row, storage, tags_by_asset.get(row.id, []), parent=parent) + for row in rows + ], + total=len(rows), + limit=len(rows), + offset=0, + ) + + +# ─── sub-video extraction ─────────────────────────────────────────────────────── + + +@router.post("/{asset_id}/subvideo", response_model=DataResponse[ActivityJobRead], status_code=202) +def start_subvideo_extraction( + asset_id: str, + payload: SubvideoCreate, + user: CurrentUser, + session: Session = Depends(get_session), +) -> DataResponse[ActivityJobRead]: + """Queue a fresh, standalone extraction. Nothing about the source asset changes.""" + asset = _owned_asset(asset_id, user.id, session) + if asset.asset_type not in (TYPE_VIDEO, TYPE_AUDIO) or not asset.storage_key: + raise HTTPException( + status_code=status.HTTP_400_BAD_REQUEST, + detail={ + "code": "not_clippable", + "message": "Only a video or audio asset that owns a file can be cut into a sub-video", + }, + ) + if payload.out_point <= payload.in_point: + raise HTTPException( + status_code=status.HTTP_400_BAD_REQUEST, + detail={"code": "bad_request", "message": "The out point must be after the in point"}, + ) + _require_no_active_extraction(session, asset.id) + + job_payload = json.dumps( + { + "mode": "extract", + "in_point": payload.in_point, + "out_point": payload.out_point, + "name": payload.name, + } + ) + job = enrichment_jobs.submit(session, asset, KIND_EXTRACT_SUBVIDEO, payload=job_payload) + return DataResponse(data=KINDS["enrichment"].to_activity(job)) + + +@router.post("/{clip_id}/promote", response_model=DataResponse[ActivityJobRead], status_code=202) +def promote_clip( + clip_id: str, + user: CurrentUser, + session: Session = Depends(get_session), +) -> DataResponse[ActivityJobRead]: + """Turn a live clip into a standalone sub-video, in place. + + The one-click path the parent-delete guard offers: promote every dependent clip, + then retry the delete. `job.asset_id` is the clip itself throughout — nothing new + is created, so unlike `subvideo` there is no `result_asset_id` to look for. + """ + clip = _owned_asset(clip_id, user.id, session) + if clip.storage_key is not None or not clip.parent_asset_id: + raise HTTPException( + status_code=status.HTTP_400_BAD_REQUEST, + detail={"code": "not_a_clip", "message": "This asset is not a clip"}, + ) + _require_no_active_extraction(session, clip.id) + + job = enrichment_jobs.submit( + session, clip, KIND_EXTRACT_SUBVIDEO, payload=json.dumps({"mode": "promote"}) + ) + return DataResponse(data=KINDS["enrichment"].to_activity(job)) diff --git a/backend/app/routers/search.py b/backend/app/routers/search.py index 364518e..140419b 100644 --- a/backend/app/routers/search.py +++ b/backend/app/routers/search.py @@ -64,10 +64,19 @@ def search( assets = search_service.load_assets( session, user.id, [a.asset_id for a in outcome.assets] ) + # A clip can be a hit same as any asset (it is indexed like one — see + # `services/assets.py::create_clip`); its `file_url` resolves through its parent, + # batch-loaded here for the same reason `list_assets` does it rather than asking + # per row. + parents_by_id = asset_service.parents_for_many(session, assets.values()) hits = [ SearchHit( - asset=asset_service.to_read_model(assets[fused.asset_id], storage), + asset=asset_service.to_read_model( + assets[fused.asset_id], + storage, + parent=parents_by_id.get(assets[fused.asset_id].parent_asset_id), + ), score=round(fused.score, 6), snippet=fused.snippet, start_time=fused.start_time, diff --git a/backend/app/schemas_assets.py b/backend/app/schemas_assets.py index 5b868f4..22abd36 100644 --- a/backend/app/schemas_assets.py +++ b/backend/app/schemas_assets.py @@ -16,6 +16,13 @@ class AssetRead(BaseModel): asset_type: str source: str + # M7. `parent_asset_id` is provenance on any of clip/promoted/extracted; only a + # clip (`source == "clip"`) has no file of its own and needs `in_point`/ + # `out_point` to bound playback of its parent's bytes. + parent_asset_id: Optional[str] = None + in_point: Optional[float] = None + out_point: Optional[float] = None + original_name: Optional[str] = None mime_type: Optional[str] = None file_format: Optional[str] = None diff --git a/backend/app/schemas_jobs.py b/backend/app/schemas_jobs.py index 3e88570..007dea5 100644 --- a/backend/app/schemas_jobs.py +++ b/backend/app/schemas_jobs.py @@ -32,6 +32,12 @@ class ActivityJobRead(BaseModel): asset_name: str = "" model: str = "" + # M7 only, and only once a `KIND_EXTRACT_SUBVIDEO` "extract" job has finished: the + # new asset it created. `asset_id` stays pointed at the *source* throughout that + # job's life, so this is the one place the frontend can learn the result's id — + # there is nowhere else to put it in a deliberately flat, kind-agnostic shape. + result_asset_id: Optional[str] = None + error_message: Optional[str] = None created_at: datetime updated_at: datetime diff --git a/backend/app/services/assets.py b/backend/app/services/assets.py index b9bd021..2d3791c 100644 --- a/backend/app/services/assets.py +++ b/backend/app/services/assets.py @@ -12,21 +12,25 @@ import mimetypes from typing import AsyncIterator, Iterable, Optional -from sqlmodel import Session, col, delete +from sqlmodel import Session, col, delete, select, update from app.auth import sign_media_key from app.clock import utcnow from app.config import settings from app.ingest import thumbnails from app.ingest.filetypes import ( + SOURCE_CLIP, + SOURCE_SUBVIDEO, SOURCE_UPLOAD, + TYPE_AUDIO, TYPE_IMAGE, + TYPE_VIDEO, asset_type_for, display_name_from, extension_of, sanitize_original_name, ) -from app.ingest.probe import probe +from app.ingest.probe import ProbeResult, probe from app.models.asset import Asset from app.models.suggestion import Suggestion from app.models.document import DocumentPage @@ -34,7 +38,7 @@ from app.schemas_assets import AssetRead, AssetTagRead from app.search import fts, vectors from app.services import tags -from app.storage import LocalStorage, StorageError, new_key, thumb_key_for +from app.storage import LocalStorage, StorageError, StoredFile, new_key, thumb_key_for logger = logging.getLogger(__name__) @@ -210,6 +214,24 @@ def reindex_ids(session: Session, asset_ids: Iterable[str]) -> None: _reindex(session, asset) +def blocking_clips(session: Session, asset_id: str) -> list[Asset]: + """Live clips that would be orphaned by deleting this asset. + + Only rows with no `storage_key` of their own count — a promoted clip or a + freshly-extracted sub-video owns real bytes and does not depend on this asset + still existing, even though it keeps `parent_asset_id` as a provenance breadcrumb. + Called by the router *before* `delete_asset`, so a blocked delete never reaches the + point of touching a row. + """ + return list( + session.exec( + select(Asset).where( + col(Asset.parent_asset_id) == asset_id, col(Asset.storage_key).is_(None) + ) + ).all() + ) + + def delete_asset(session: Session, storage: LocalStorage, asset: Asset) -> None: """Remove the row, everything that hangs off it, and the bytes it owns. @@ -228,7 +250,11 @@ def delete_asset(session: Session, storage: LocalStorage, asset: Asset) -> None: Suggestions are in the first category — `Suggestion.asset_id` is a foreign key too, so an asset with a pending suggestion would be undeletable exactly the way a tagged - one used to be. + one used to be. `parent_asset_id` (M7) is the same category for a promoted clip or + a fresh extraction that kept this asset as a provenance breadcrumb — the caller is + expected to have already called `blocking_clips` and refused the delete if any + *live* clip depends on this asset, but a survivor's mere breadcrumb still has to be + cleared, or the same foreign key raises on it the moment this row is gone. """ storage_key, thumb_key = asset.storage_key, asset.thumb_key @@ -238,6 +264,7 @@ def delete_asset(session: Session, storage: LocalStorage, asset: Asset) -> None: session.exec(delete(Suggestion).where(col(Suggestion.asset_id) == asset_id)) session.exec(delete(TranscriptSegment).where(col(TranscriptSegment.asset_id) == asset_id)) session.exec(delete(DocumentPage).where(col(DocumentPage.asset_id) == asset_id)) + session.exec(update(Asset).where(col(Asset.parent_asset_id) == asset_id).values(parent_asset_id=None)) session.delete(asset) session.commit() @@ -326,24 +353,216 @@ def apply_ai_metadata(session: Session, asset: Asset, changes: dict) -> list[str return list(changes.keys()) +# ─── clips and sub-videos (M7) ──────────────────────────────────────────────── + + +def _format_timestamp(seconds: float) -> str: + total = max(int(seconds), 0) + minutes, secs = divmod(total, 60) + return f"{minutes}:{secs:02d}" + + +def _format_range(start: float, end: float) -> str: + return f"{_format_timestamp(start)}–{_format_timestamp(end)}" + + +def create_clip( + session: Session, + parent: Asset, + *, + in_point: float, + out_point: float, + name: Optional[str] = None, +) -> Asset: + """A non-destructive clip: an Asset row, no file, no ffmpeg, no job. + + Validated against `parent` rather than trusted from the caller — `in_point`/ + `out_point` are user input by way of an in/out editor — and raises `ValueError` + on anything that should not become a row, which the router turns into a 400, the + same pattern `list_assets` already uses for `min_duration > max_duration`. + Clipping a clip is refused rather than resolved to the real ancestor: `promote` + already turns a clip into a real file in place, which covers "I want this exact + range as a standalone file" without a second, coordinate-flattening code path. + """ + if parent.asset_type not in (TYPE_VIDEO, TYPE_AUDIO): + raise ValueError("Only video and audio assets can be clipped") + if parent.storage_key is None: + raise ValueError("A clip cannot itself be clipped — clip the original asset") + if in_point < 0 or out_point <= in_point: + raise ValueError("The out point must be after the in point") + if parent.duration_seconds is not None and out_point > parent.duration_seconds: + raise ValueError("The out point is past the end of the asset") + + clip = Asset( + user_id=parent.user_id, + name=name or f"Clip of {parent.name} ({_format_range(in_point, out_point)})", + asset_type=parent.asset_type, + source=SOURCE_CLIP, + storage_key=None, + # Copied, not resolved later: a clip's thumbnail is a real, already-existing + # object, so it signs and serves exactly like any other asset's `thumb_key` — + # only `file_url` needs `to_read_model`'s parent-resolution, since only + # `storage_key` has to stay null for this to be a clip at all. + thumb_key=parent.thumb_key, + parent_asset_id=parent.id, + in_point=in_point, + out_point=out_point, + mime_type=parent.mime_type, + file_format=parent.file_format, + duration_seconds=out_point - in_point, + width=parent.width, + height=parent.height, + codec=parent.codec, + ) + session.add(clip) + session.commit() + session.refresh(clip) + _reindex(session, clip) + return clip + + +def list_children(session: Session, parent_id: str, user_id: str) -> list[Asset]: + """Everything derived from this asset: live clips and past promotions/extractions. + + Unfiltered by `storage_key` deliberately — a "Clips" tab wants the whole history, + and the delete guard's promote UI filters this down to `source == SOURCE_CLIP` + itself, so one endpoint serves both rather than needing a second for the narrower + question `blocking_clips` already answers for the guard itself. + """ + return list( + session.exec( + select(Asset) + .where(col(Asset.parent_asset_id) == parent_id, Asset.user_id == user_id) + .order_by(col(Asset.upload_date).desc()) + ).all() + ) + + +def create_subvideo_asset( + session: Session, + *, + source_asset: Asset, + stored: StoredFile, + thumb_key: Optional[str], + in_point: float, + out_point: float, + name: Optional[str], + probe_result: ProbeResult, +) -> Asset: + """A freshly-extracted sub-video: a brand-new, standalone Asset. + + `parent_asset_id` is provenance only — this row owns real bytes of its own + (`storage_key=stored.key`), so it never blocks `source_asset`'s deletion and never + appears in `blocking_clips`. `in_point`/`out_point` stay null: this file's own + timeline starts at 0, unlike a clip's window into its parent's. + """ + subvideo = Asset( + user_id=source_asset.user_id, + name=name or f"{source_asset.name} ({_format_range(in_point, out_point)})", + asset_type=source_asset.asset_type, + source=SOURCE_SUBVIDEO, + storage_key=stored.key, + thumb_key=thumb_key, + parent_asset_id=source_asset.id, + original_name=source_asset.original_name, + mime_type=mimetypes.guess_type(stored.key)[0], + file_format=extension_of(stored.key).lstrip("."), + size_bytes=stored.size_bytes, + checksum_sha256=stored.sha256, + duration_seconds=probe_result.duration_seconds or (out_point - in_point), + width=probe_result.width, + height=probe_result.height, + codec=probe_result.codec, + ) + session.add(subvideo) + session.commit() + session.refresh(subvideo) + _reindex(session, subvideo) + return subvideo + + +def promote_clip( + session: Session, clip: Asset, *, stored: StoredFile, thumb_key: Optional[str], probe_result: ProbeResult +) -> Asset: + """Turn a live clip into a standalone sub-video, in place. + + Same row, same id — anything that already references this clip (a search hit, a + link, a "Clips" tab entry) keeps pointing at something real. `parent_asset_id` + stays, now as provenance rather than a live dependency. `in_point`/`out_point` are + cleared, not kept: they were coordinates into the *parent's* timeline, and the + extracted file has its own, starting at 0 — leaving the old values in place would + have playback seek into a nine-second file at second 61. + """ + clip.storage_key = stored.key + clip.thumb_key = thumb_key or clip.thumb_key + clip.source = SOURCE_SUBVIDEO + clip.in_point = None + clip.out_point = None + clip.mime_type = mimetypes.guess_type(stored.key)[0] + clip.file_format = extension_of(stored.key).lstrip(".") + clip.size_bytes = stored.size_bytes + clip.checksum_sha256 = stored.sha256 + if probe_result.duration_seconds is not None: + clip.duration_seconds = probe_result.duration_seconds + if probe_result.width is not None: + clip.width = probe_result.width + if probe_result.height is not None: + clip.height = probe_result.height + if probe_result.codec is not None: + clip.codec = probe_result.codec + clip.metadata_modified_date = utcnow() + + session.add(clip) + session.commit() + session.refresh(clip) + _reindex(session, clip) + return clip + + # ─── serialisation ─────────────────────────────────────────────────────────── +def parents_for_many(session: Session, assets: Iterable[Asset]) -> dict[str, Asset]: + """Every listed asset's parent, in one query, for `to_read_model`'s `parent` arg. + + Same shape as `tags.tags_for_many`, and for the same reason: a listing's clips must + not each ask for their own parent, which is a query per clip on the hottest page in + the application. Most rows on a page are not clips at all, so this is usually a + query over an empty set of ids — cheap, and simpler than special-casing that away. + """ + parent_ids = {a.parent_asset_id for a in assets if a.parent_asset_id} + if not parent_ids: + return {} + rows = session.exec(select(Asset).where(col(Asset.id).in_(parent_ids))).all() + return {row.id: row for row in rows} + + def to_read_model( - asset: Asset, storage: LocalStorage, asset_tags: Optional[list] = None + asset: Asset, + storage: LocalStorage, + asset_tags: Optional[list] = None, + parent: Optional[Asset] = None, ) -> AssetRead: """An Asset as the API returns it, with freshly signed URLs. - `asset_tags` is passed in rather than fetched. A listing loads every row's tags in - one query and hands each one its slice; fetching here instead would put a query per - asset on the hottest path in the application. + `asset_tags` is passed in rather than fetched, for the same reason `parent` is: + a listing loads every row's tags (and every clip's parent) in one query each and + hands each row its slice, rather than asking per row on the hottest path in the + application. + + A clip (`asset.storage_key is None`) has no file of its own — `file_url` and + `missing` resolve against `parent`'s bytes instead, since that is what actually + plays. `thumb_url` needs no such resolution: a clip's `thumb_key` is copied from + its parent at creation time (`create_clip`), so it already points at a real object + and signs like any other asset's. """ - file_url = _signed_url(asset.storage_key) + playable_key = asset.storage_key if asset.storage_key else (parent.storage_key if parent else None) + file_url = _signed_url(playable_key) thumb_url = _signed_url(asset.thumb_key) missing = False - if asset.storage_key: - missing = not storage.stat(asset.storage_key).exists + if playable_key: + missing = not storage.stat(playable_key).exists return AssetRead( id=asset.id, @@ -352,6 +571,9 @@ def to_read_model( summary=asset.summary, asset_type=asset.asset_type, source=asset.source, + parent_asset_id=asset.parent_asset_id, + in_point=asset.in_point, + out_point=asset.out_point, original_name=asset.original_name, mime_type=asset.mime_type, file_format=asset.file_format, diff --git a/backend/app/storage/base.py b/backend/app/storage/base.py index ace1e24..4d2a457 100644 --- a/backend/app/storage/base.py +++ b/backend/app/storage/base.py @@ -98,6 +98,16 @@ async def write_stream(self, key: str, chunks: AsyncIterator[bytes]) -> StoredFi def write_bytes(self, key: str, data: bytes) -> StoredFile: ... + def write_file(self, key: str, source_path: Path) -> StoredFile: + """For a file a job already produced on disk — ffmpeg's own output. + + Not a rename: `source_path` is typically under a `tempfile.TemporaryDirectory`, + commonly a different filesystem from the storage root (a Docker bind mount, + for instance), and `os.rename`/`os.replace` across filesystems raises `EXDEV`. + Implementations copy the bytes in, the same way `write_stream` does. + """ + ... + def open(self, key: str) -> BinaryIO: ... def materialise(self, key: str) -> AbstractContextManager[Path]: diff --git a/backend/app/storage/local.py b/backend/app/storage/local.py index 93d6e6a..026a35f 100644 --- a/backend/app/storage/local.py +++ b/backend/app/storage/local.py @@ -83,6 +83,37 @@ def write_bytes(self, key: str, data: bytes) -> StoredFile: raise return StoredFile(key=key, size_bytes=len(data), sha256=hashlib.sha256(data).hexdigest()) + def write_file(self, key: str, source_path: Path) -> StoredFile: + """For a file a job already produced on disk — ffmpeg's sub-video output. + + A real copy, not `shutil.move`/`os.rename`: `source_path` usually lives under a + `tempfile.TemporaryDirectory`, which is not guaranteed to share a filesystem + with the storage root (it commonly does not, under a Docker bind mount), and a + cross-filesystem rename raises `EXDEV`. Copies in `CHUNK_SIZE` pieces so a large + video does not sit in memory whole, hashing as it streams — same shape as + `write_stream`, reading from a file instead of an async iterator. The final + `.partial` -> `path` rename is always same-filesystem, since both are under + `self.root`. + """ + path = self._path(key) + path.parent.mkdir(parents=True, exist_ok=True) + partial = path.with_name(path.name + ".partial") + + digest = hashlib.sha256() + size = 0 + try: + with open(source_path, "rb") as src, open(partial, "wb") as dst: + while chunk := src.read(CHUNK_SIZE): + dst.write(chunk) + digest.update(chunk) + size += len(chunk) + os.replace(partial, path) + except BaseException: + partial.unlink(missing_ok=True) + raise + + return StoredFile(key=key, size_bytes=size, sha256=digest.hexdigest()) + # ─── reading ───────────────────────────────────────────────────────────── def open(self, key: str) -> BinaryIO: diff --git a/backend/tests/test_clips_api.py b/backend/tests/test_clips_api.py new file mode 100644 index 0000000..0909098 --- /dev/null +++ b/backend/tests/test_clips_api.py @@ -0,0 +1,561 @@ +"""Clips and sub-video extraction, over HTTP and against real media.""" + +from pathlib import Path + +import pytest +from sqlmodel import select + +from app.enrichment import source +from app.enrichment.source import NoSourceMaterial +from app.ingest.probe import ProbeResult +from app.jobs import enrichment as enrichment_jobs +from app.models.asset import Asset +from app.models.job import EnrichmentJob, KIND_EXTRACT_SUBVIDEO +from app.services import assets as asset_service +from app.storage.base import StoredFile +from app.media_tools import ffmpeg_available + +FIXTURES = Path(__file__).parent / "fixtures" + +needs_ffmpeg = pytest.mark.skipif(not ffmpeg_available(), reason="ffmpeg not on PATH") + + +def _upload_video(client): + return client.post( + "/api/assets", + files=[("files", ("sample_video.mp4", (FIXTURES / "sample_video.mp4").read_bytes(), "video/mp4"))], + ).json()["created"][0] + + +def _upload_audio(client): + return client.post( + "/api/assets", + files=[("files", ("sample_audio.mp3", (FIXTURES / "sample_audio.mp3").read_bytes(), "audio/mpeg"))], + ).json()["created"][0] + + +def _upload_image(client): + return client.post( + "/api/assets", + files=[("files", ("sample_image.jpg", (FIXTURES / "sample_image.jpg").read_bytes(), "image/jpeg"))], + ).json()["created"][0] + + +def _fake_stored(key: str = "user-under-test/fake-subvideo.mp4") -> StoredFile: + return StoredFile(key=key, size_bytes=12345, sha256="0" * 64) + + +def _fake_probe(duration: float = 9.0) -> ProbeResult: + return ProbeResult(duration_seconds=duration, width=320, height=240, codec="h264") + + +# ─── creating a clip (synchronous, no ffmpeg needed) ────────────────────────── + + +def test_create_clip_is_a_window_not_a_copy(library, media_dir): + parent = _upload_video(library) + before = {p for p in Path(media_dir).rglob("*") if p.is_file()} + + response = library.post( + f"/api/assets/{parent['id']}/clips", json={"in_point": 0.2, "out_point": 0.6} + ) + assert response.status_code == 201 + + clip = response.json()["data"] + assert clip["source"] == "clip" + assert clip["parent_asset_id"] == parent["id"] + assert clip["in_point"] == pytest.approx(0.2) + assert clip["out_point"] == pytest.approx(0.6) + assert clip["duration_seconds"] == pytest.approx(0.4) + assert clip["asset_type"] == "video" + + # Zero storage, zero processing — no new file exists on disk. + after = {p for p in Path(media_dir).rglob("*") if p.is_file()} + assert after == before + + +def test_clip_default_name_mentions_the_range(library, session): + parent = _upload_video(library) + # The fixture is only ~2s long; set a longer duration directly so a >60s range is + # legal, the same way test_create_clip_refuses_past_the_parents_duration does. + parent_row = session.get(Asset, parent["id"]) + parent_row.duration_seconds = 120.0 + session.add(parent_row) + session.commit() + + clip = library.post( + f"/api/assets/{parent['id']}/clips", json={"in_point": 61.0, "out_point": 70.0} + ).json()["data"] + assert "1:01" in clip["name"] + assert "1:10" in clip["name"] + + +def test_clip_can_be_named_explicitly(library): + parent = _upload_video(library) + clip = library.post( + f"/api/assets/{parent['id']}/clips", + json={"in_point": 0.0, "out_point": 0.5, "name": "The nano weapons moment"}, + ).json()["data"] + assert clip["name"] == "The nano weapons moment" + + +def test_clip_inherits_the_parents_thumbnail(library, session): + parent = _upload_video(library) + parent_row = session.get(Asset, parent["id"]) + # This sandbox has no ffmpeg, so the upload itself never got a poster — set one by + # hand so the inheritance path (not the probing path) is what is under test. + parent_row.thumb_key = f"{parent_row.user_id}/fake-poster.thumb.jpg" + session.add(parent_row) + session.commit() + + clip = library.post( + f"/api/assets/{parent['id']}/clips", json={"in_point": 0.0, "out_point": 0.5} + ).json()["data"] + assert clip["thumb_url"] is not None + assert parent_row.thumb_key in clip["thumb_url"] + + +def test_clips_file_url_resolves_to_the_parents_key(library, session): + parent = _upload_video(library) + parent_row = session.get(Asset, parent["id"]) + + clip = library.post( + f"/api/assets/{parent['id']}/clips", json={"in_point": 0.0, "out_point": 0.5} + ).json()["data"] + assert clip["file_url"] is not None + assert parent_row.storage_key in clip["file_url"] + + # Same through GET, not just the create response. + fetched = library.get(f"/api/assets/{clip['id']}").json()["data"] + assert parent_row.storage_key in fetched["file_url"] + + +def test_clips_missing_follows_the_parent(library, session, media_dir): + parent = _upload_video(library) + clip = library.post( + f"/api/assets/{parent['id']}/clips", json={"in_point": 0.0, "out_point": 0.5} + ).json()["data"] + + for path in Path(media_dir).rglob("*"): + if path.is_file() and ".thumb" not in path.name: + path.unlink() + + fetched = library.get(f"/api/assets/{clip['id']}").json()["data"] + assert fetched["missing"] is True + + +def test_clip_appears_in_the_library_listing(library): + parent = _upload_video(library) + clip = library.post( + f"/api/assets/{parent['id']}/clips", json={"in_point": 0.0, "out_point": 0.5} + ).json()["data"] + + body = library.get("/api/assets").json() + ids = {a["id"] for a in body["data"]} + assert clip["id"] in ids + + +def test_create_clip_refuses_a_non_video_asset(library): + image = _upload_image(library) + response = library.post( + f"/api/assets/{image['id']}/clips", json={"in_point": 0.0, "out_point": 0.5} + ) + assert response.status_code == 400 + assert response.json()["detail"]["code"] == "invalid_clip_range" + + +def test_create_clip_refuses_a_backwards_range(library): + parent = _upload_video(library) + response = library.post( + f"/api/assets/{parent['id']}/clips", json={"in_point": 5.0, "out_point": 1.0} + ) + assert response.status_code == 400 + + +def test_create_clip_refuses_past_the_parents_duration(library, session): + parent = _upload_video(library) + # ffprobe is unavailable in this sandbox, so a real upload never gets a duration — + # set one directly so the bounds check (not the probing path) is under test. + parent_row = session.get(Asset, parent["id"]) + parent_row.duration_seconds = 2.0 + session.add(parent_row) + session.commit() + + response = library.post( + f"/api/assets/{parent['id']}/clips", json={"in_point": 0.0, "out_point": 5.0} + ) + assert response.status_code == 400 + + +def test_cannot_clip_a_clip(library): + parent = _upload_video(library) + clip = library.post( + f"/api/assets/{parent['id']}/clips", json={"in_point": 0.0, "out_point": 0.5} + ).json()["data"] + + response = library.post( + f"/api/assets/{clip['id']}/clips", json={"in_point": 0.0, "out_point": 0.1} + ) + assert response.status_code == 400 + + +def test_create_clip_of_someone_elses_asset_is_a_404(library, session): + other = Asset(user_id="somebody-else", name="Theirs", asset_type="video", source="local_upload") + session.add(other) + session.commit() + + response = library.post( + f"/api/assets/{other.id}/clips", json={"in_point": 0.0, "out_point": 0.5} + ) + assert response.status_code == 404 + + +# ─── listing clips ───────────────────────────────────────────────────────────── + + +def test_list_clips_returns_a_parents_children(library): + parent = _upload_video(library) + library.post(f"/api/assets/{parent['id']}/clips", json={"in_point": 0.0, "out_point": 0.2}) + library.post(f"/api/assets/{parent['id']}/clips", json={"in_point": 0.3, "out_point": 0.5}) + + body = library.get(f"/api/assets/{parent['id']}/clips").json() + assert body["total"] == 2 + assert {c["parent_asset_id"] for c in body["data"]} == {parent["id"]} + + +def test_list_clips_includes_a_promoted_one(library, session): + """`GET .../clips` is the full history, not just the still-live ones — the delete + guard's promote UI is the caller that narrows it further, to `source == "clip"`.""" + parent = _upload_video(library) + clip = library.post( + f"/api/assets/{parent['id']}/clips", json={"in_point": 0.0, "out_point": 0.5} + ).json()["data"] + + clip_row = session.get(Asset, clip["id"]) + asset_service.promote_clip( + session, clip_row, stored=_fake_stored(), thumb_key=None, probe_result=_fake_probe() + ) + + body = library.get(f"/api/assets/{parent['id']}/clips").json() + assert body["total"] == 1 + assert body["data"][0]["source"] == "sub_video" + + +def test_list_clips_of_someone_elses_asset_is_a_404(library, session): + other = Asset(user_id="somebody-else", name="Theirs", asset_type="video", source="local_upload") + session.add(other) + session.commit() + assert library.get(f"/api/assets/{other.id}/clips").status_code == 404 + + +# ─── starting a sub-video extraction (queues a job) ─────────────────────────── + + +def test_subvideo_queues_a_job(library): + asset = _upload_video(library) + response = library.post( + f"/api/assets/{asset['id']}/subvideo", json={"in_point": 0.0, "out_point": 0.5} + ) + assert response.status_code == 202 + + job = response.json()["data"] + assert job["action"] == "extract_subvideo" + assert job["status"] == "queued" + assert job["asset_id"] == asset["id"] + assert job["result_asset_id"] is None + + +def test_subvideo_refuses_a_non_video_asset(library): + image = _upload_image(library) + response = library.post( + f"/api/assets/{image['id']}/subvideo", json={"in_point": 0.0, "out_point": 0.5} + ) + assert response.status_code == 400 + assert response.json()["detail"]["code"] == "not_clippable" + + +def test_subvideo_refuses_a_clip_as_its_own_source(library): + """A clip owns no bytes — extracting *from* one is refused the same as clipping one + (`create_clip`'s own rule); `promote` is the path that turns a clip into a file.""" + parent = _upload_video(library) + clip = library.post( + f"/api/assets/{parent['id']}/clips", json={"in_point": 0.0, "out_point": 0.5} + ).json()["data"] + + response = library.post( + f"/api/assets/{clip['id']}/subvideo", json={"in_point": 0.0, "out_point": 0.1} + ) + assert response.status_code == 400 + assert response.json()["detail"]["code"] == "not_clippable" + + +def test_subvideo_refuses_a_backwards_range(library): + asset = _upload_video(library) + response = library.post( + f"/api/assets/{asset['id']}/subvideo", json={"in_point": 5.0, "out_point": 1.0} + ) + assert response.status_code == 400 + + +def test_subvideo_refuses_a_second_concurrent_run(library): + asset = _upload_video(library) + library.post(f"/api/assets/{asset['id']}/subvideo", json={"in_point": 0.0, "out_point": 0.5}) + + response = library.post( + f"/api/assets/{asset['id']}/subvideo", json={"in_point": 0.0, "out_point": 0.4} + ) + assert response.status_code == 409 + assert response.json()["detail"]["code"] == "already_running" + + +def test_subvideo_of_someone_elses_asset_is_a_404(library, session): + other = Asset(user_id="somebody-else", name="Theirs", asset_type="video", source="local_upload") + session.add(other) + session.commit() + response = library.post( + f"/api/assets/{other.id}/subvideo", json={"in_point": 0.0, "out_point": 0.5} + ) + assert response.status_code == 404 + + +# ─── promoting a clip (queues a job) ─────────────────────────────────────────── + + +def test_promote_queues_a_job(library): + parent = _upload_video(library) + clip = library.post( + f"/api/assets/{parent['id']}/clips", json={"in_point": 0.0, "out_point": 0.5} + ).json()["data"] + + response = library.post(f"/api/assets/{clip['id']}/promote") + assert response.status_code == 202 + job = response.json()["data"] + assert job["action"] == "extract_subvideo" + assert job["asset_id"] == clip["id"] + + +def test_promote_refuses_a_non_clip(library): + asset = _upload_video(library) + response = library.post(f"/api/assets/{asset['id']}/promote") + assert response.status_code == 400 + assert response.json()["detail"]["code"] == "not_a_clip" + + +def test_promote_refuses_a_second_concurrent_run(library): + parent = _upload_video(library) + clip = library.post( + f"/api/assets/{parent['id']}/clips", json={"in_point": 0.0, "out_point": 0.5} + ).json()["data"] + library.post(f"/api/assets/{clip['id']}/promote") + + response = library.post(f"/api/assets/{clip['id']}/promote") + assert response.status_code == 409 + + +def test_promote_of_someone_elses_clip_is_a_404(library, session): + other = Asset( + user_id="somebody-else", + name="Theirs", + asset_type="video", + source="clip", + parent_asset_id=None, + ) + session.add(other) + session.commit() + assert library.post(f"/api/assets/{other.id}/promote").status_code == 404 + + +# ─── the delete guard ────────────────────────────────────────────────────────── + + +def test_delete_is_blocked_by_a_live_clip(library): + parent = _upload_video(library) + library.post(f"/api/assets/{parent['id']}/clips", json={"in_point": 0.0, "out_point": 0.5}) + + response = library.delete(f"/api/assets/{parent['id']}") + assert response.status_code == 409 + body = response.json()["detail"] + assert body["code"] == "asset_has_dependent_clips" + assert "1" in body["message"] + + # Refused, not partially done — the parent is still there. + assert library.get(f"/api/assets/{parent['id']}").status_code == 200 + + +def test_delete_message_counts_more_than_one_clip(library): + parent = _upload_video(library) + library.post(f"/api/assets/{parent['id']}/clips", json={"in_point": 0.0, "out_point": 0.2}) + library.post(f"/api/assets/{parent['id']}/clips", json={"in_point": 0.3, "out_point": 0.5}) + + body = library.delete(f"/api/assets/{parent['id']}").json()["detail"] + assert "2" in body["message"] + + +def test_delete_succeeds_with_no_clips(library): + """Regression check: the guard must not block an ordinary delete.""" + asset = _upload_video(library) + assert library.delete(f"/api/assets/{asset['id']}").status_code == 204 + + +def test_delete_succeeds_once_the_only_clip_is_promoted(library, session): + parent = _upload_video(library) + clip = library.post( + f"/api/assets/{parent['id']}/clips", json={"in_point": 0.0, "out_point": 0.5} + ).json()["data"] + + clip_row = session.get(Asset, clip["id"]) + asset_service.promote_clip( + session, clip_row, stored=_fake_stored(), thumb_key=None, probe_result=_fake_probe() + ) + + assert library.delete(f"/api/assets/{parent['id']}").status_code == 204 + + # The promoted clip survives the parent's delete, and its breadcrumb is cleared — + # not left dangling at a foreign key the delete would otherwise trip over. + session.expire_all() + survivor = session.get(Asset, clip["id"]) + assert survivor is not None + assert survivor.parent_asset_id is None + assert survivor.source == "sub_video" + + +def test_delete_still_blocked_if_only_some_clips_are_promoted(library, session): + parent = _upload_video(library) + keep = library.post( + f"/api/assets/{parent['id']}/clips", json={"in_point": 0.0, "out_point": 0.2} + ).json()["data"] + promoted = library.post( + f"/api/assets/{parent['id']}/clips", json={"in_point": 0.3, "out_point": 0.5} + ).json()["data"] + + promoted_row = session.get(Asset, promoted["id"]) + asset_service.promote_clip( + session, promoted_row, stored=_fake_stored(), thumb_key=None, probe_result=_fake_probe() + ) + + response = library.delete(f"/api/assets/{parent['id']}") + assert response.status_code == 409 + assert "1" in response.json()["detail"]["message"] + + +# ─── the gather() guard (M6 enrichment must refuse a clip) ──────────────────── + + +def test_gather_refuses_a_clip(library, session): + parent = _upload_video(library) + clip = library.post( + f"/api/assets/{parent['id']}/clips", json={"in_point": 0.0, "out_point": 0.5} + ).json()["data"] + clip_row = session.get(Asset, clip["id"]) + + with pytest.raises(NoSourceMaterial, match="clip"): + source.gather(session, clip_row, supports_images=True) + + +def test_gather_still_works_on_a_promoted_clip(library, session): + """The guard keys off `storage_key`, not `parent_asset_id` — a promoted clip owns + real bytes again and must not be refused as a clip.""" + parent = _upload_video(library) + clip = library.post( + f"/api/assets/{parent['id']}/clips", json={"in_point": 0.0, "out_point": 0.5} + ).json()["data"] + clip_row = session.get(Asset, clip["id"]) + asset_service.promote_clip( + session, clip_row, stored=_fake_stored(), thumb_key=None, probe_result=_fake_probe() + ) + session.refresh(clip_row) + # Whether it also inherited a poster from its parent is incidental to what this + # test checks (that promoting lifts the clip-specific refusal) and depends on + # whether ffmpeg is on PATH in whatever environment runs this — cleared explicitly + # so the rest of the assertion is deterministic either way. + clip_row.thumb_key = None + session.add(clip_row) + session.commit() + session.refresh(clip_row) + + with pytest.raises(NoSourceMaterial, match="Transcribe"): + # No transcript, no poster, no document text — refused for the ordinary reason + # every untranscribed video is, not the clip-specific one. + source.gather(session, clip_row, supports_images=True) + + +# ─── running the job for real (needs ffmpeg — skipped in this sandbox) ──────── + + +@needs_ffmpeg +def test_running_extract_creates_a_standalone_asset(library, session): + import json + + from app.enrichment.extract_subvideo import run as run_extract_subvideo + + created = _upload_video(library) + asset = session.get(Asset, created["id"]) + payload = json.dumps({"mode": "extract", "in_point": 0.2, "out_point": 1.5, "name": "Cut"}) + + detail, created_id = run_extract_subvideo(session, asset, payload, lambda *a, **k: None) + + assert created_id is not None + subvideo = session.get(Asset, created_id) + assert subvideo.source == "sub_video" + assert subvideo.storage_key is not None + assert subvideo.parent_asset_id == asset.id + assert subvideo.in_point is None and subvideo.out_point is None + # The source asset is genuinely untouched. + session.refresh(asset) + assert asset.storage_key == created["file_url"].split("?")[0].removeprefix("/media/") + + +@needs_ffmpeg +def test_running_promote_updates_the_clip_in_place(library, session): + import json + + from app.enrichment.extract_subvideo import run as run_extract_subvideo + + parent = _upload_video(library) + clip = library.post( + f"/api/assets/{parent['id']}/clips", json={"in_point": 0.2, "out_point": 1.5} + ).json()["data"] + clip_row = session.get(Asset, clip["id"]) + clip_id = clip_row.id + + detail, created_id = run_extract_subvideo( + session, clip_row, json.dumps({"mode": "promote"}), lambda *a, **k: None + ) + + assert created_id is None # promote updates in place; nothing new to report + session.expire_all() + promoted = session.get(Asset, clip_id) + assert promoted.source == "sub_video" + assert promoted.storage_key is not None + assert promoted.in_point is None and promoted.out_point is None + assert promoted.parent_asset_id == parent["id"] # provenance survives + + +@needs_ffmpeg +def test_the_api_end_to_end_via_the_job_dispatcher(library, session, monkeypatch): + """The same path a real request takes: submit through the router, then run the + worker's own dispatch — not `run_extract_subvideo` called directly — so the + KIND_EXTRACT_SUBVIDEO branch in `jobs/enrichment.py` is what's under test, and so + is `result_asset_id` reaching `/api/activity` through `jobs/registry.py`.""" + import json + + asset = _upload_video(library) + job = library.post( + f"/api/assets/{asset['id']}/subvideo", json={"in_point": 0.1, "out_point": 1.0} + ).json()["data"] + + # `_run_job` opens its own session from the queue's engine, not this fixture's — + # matches test_transcripts.py::test_a_queue_cancel_still_leaves_a_terminal_row. + queue = enrichment_jobs.queue() + monkeypatch.setattr(queue, "engine", session.get_bind()) + enrichment_jobs._run_job(job["id"]) + + session.expire_all() + row = session.get(EnrichmentJob, job["id"]) + assert row.status == "done" + assert row.payload is not None + created_id = json.loads(row.payload)["created_asset_id"] + + activity = library.get("/api/activity").json() + matching = next(j for j in activity["data"] if j["id"] == job["id"]) + assert matching["result_asset_id"] == created_id diff --git a/backend/tests/test_subvideo_extraction.py b/backend/tests/test_subvideo_extraction.py new file mode 100644 index 0000000..9f617e3 --- /dev/null +++ b/backend/tests/test_subvideo_extraction.py @@ -0,0 +1,109 @@ +"""Cutting a time range out of real media, and the fast-path/re-encode fallback.""" + +import subprocess +from pathlib import Path + +import pytest + +from app.enrichment.subvideo import SubvideoExtractionError, extract_subvideo, timeout_for +from app.ingest.probe import probe +from app.media_tools import ffmpeg_available + +FIXTURES = Path(__file__).parent / "fixtures" + +needs_ffmpeg = pytest.mark.skipif(not ffmpeg_available(), reason="ffmpeg not on PATH") + + +@needs_ffmpeg +def test_extracts_the_requested_range(tmp_path): + target = tmp_path / "clip.mp4" + extract_subvideo(FIXTURES / "sample_video.mp4", target, 0.2, 1.5) + + assert target.is_file() + assert target.stat().st_size > 0 + result = probe(target) + assert result.duration_seconds == pytest.approx(1.3, abs=0.5) + + +@needs_ffmpeg +def test_audio_source_works_too(tmp_path): + target = tmp_path / "clip.m4a" + extract_subvideo(FIXTURES / "sample_audio.mp3", target, 0.2, 1.2) + + assert target.is_file() + result = probe(target) + assert result.duration_seconds == pytest.approx(1.0, abs=0.5) + + +@needs_ffmpeg +def test_falls_back_to_reencode_when_the_fast_path_overshoots(tmp_path): + """The case `subvideo.py`'s whole two-strategy design exists for. + + A keyframe only every 10s means the fast path — which can only start on one — has + to begin at t=0 and run past the requested end to reach it, wildly overshooting a + 0.5s cut. The re-encode fallback decodes and lands exactly on the boundary instead. + """ + sparse = tmp_path / "sparse_keyframes.mp4" + subprocess.run( + [ + "ffmpeg", "-y", "-nostdin", "-f", "lavfi", + "-i", "testsrc=size=64x64:rate=10:duration=10", + "-c:v", "libx264", "-g", "100", "-keyint_min", "100", + "-pix_fmt", "yuv420p", str(sparse), + ], + capture_output=True, check=True, + ) + + target = tmp_path / "clip.mp4" + extract_subvideo(sparse, target, 5.0, 5.5) + + result = probe(target) + assert result.duration_seconds == pytest.approx(0.5, abs=0.5) + + +@needs_ffmpeg +def test_fast_path_is_used_when_it_is_already_accurate(tmp_path): + """The counterpart to the fallback test: dense keyframes (one a second) mean the + fast path lands close enough on its own, and re-encoding would just be slower for + no benefit.""" + dense = tmp_path / "dense_keyframes.mp4" + subprocess.run( + [ + "ffmpeg", "-y", "-nostdin", "-f", "lavfi", + "-i", "testsrc=size=64x64:rate=10:duration=10", + "-c:v", "libx264", "-g", "10", "-keyint_min", "10", + "-pix_fmt", "yuv420p", str(dense), + ], + capture_output=True, check=True, + ) + + target = tmp_path / "clip.mp4" + extract_subvideo(dense, target, 5.0, 5.5) + + result = probe(target) + assert result.duration_seconds == pytest.approx(0.5, abs=0.5) + + +@needs_ffmpeg +def test_refuses_a_backwards_range(tmp_path): + with pytest.raises(SubvideoExtractionError, match="out point"): + extract_subvideo(FIXTURES / "sample_video.mp4", tmp_path / "clip.mp4", 5.0, 1.0) + + +@needs_ffmpeg +def test_a_non_media_file_is_refused(tmp_path): + junk = tmp_path / "notes.txt" + junk.write_text("not media") + + with pytest.raises(SubvideoExtractionError): + extract_subvideo(junk, tmp_path / "clip.mp4", 0.0, 1.0) + + +def test_timeout_scales_with_the_cut_length(): + """Scales with the *cut*, not the source file — that is what ffmpeg actually has + to process either way, whichever strategy runs.""" + assert timeout_for(None) == 300 + assert timeout_for(0) == 300 + assert timeout_for(10) == 120 # floor + assert timeout_for(600) == 1200 + assert timeout_for(10_000) == 3600 # ceiling diff --git a/docs/m7-clips-and-subvideos.md b/docs/m7-clips-and-subvideos.md new file mode 100644 index 0000000..1efb2c1 --- /dev/null +++ b/docs/m7-clips-and-subvideos.md @@ -0,0 +1,273 @@ +# M7 — Clips & sub-videos + +**Status:** complete. Non-destructive clips, destructive sub-video extraction (fast +stream-copy with an automatic re-encode fallback), and a parent-delete guard with a +one-click promote path are all built, backed by real ffmpeg-verified tests (this +sandbox had none at first; `apt-get install ffmpeg` got real coverage instead of relying +on the `needs_ffmpeg` skip marker, and it caught two test bugs the skip would have +hidden — see "What real ffmpeg caught" below). + +Two research passes preceded the code: a read-only survey of the actual codebase (not +`docs/plan-of-attack.md`'s aspirational file sketch, which turned out to be stale in +three places — see "Where this diverges from the plan doc" below), and an independent +validation pass that pushed back on parts of the design. Both are folded into this +document rather than kept separate, so the reasoning survives in one place. + +--- + +## Where this diverges from the plan doc + +`docs/plan-of-attack.md`'s architecture sketch names `backend/app/media/{clips,extract, +ranged}.py` for this milestone. None of that held up against the actual code: + +- **`services/assets.py`'s own module docstring already claims this work**: *"an + extracted sub-video (M7) ... land[s] here"*. Clip and sub-video Asset-row lifecycle + (`create_clip`, `list_children`, `blocking_clips`, `create_subvideo_asset`, + `promote_clip`) lives in that existing module, not a new one — it already had the + hook waiting. +- **`media/` isn't where ffmpeg-touching jobs live.** Every existing one — + `transcribe.py` for `KIND_TRANSCRIBE`, `extract_text.py` for `KIND_EXTRACT_TEXT` — is + a `enrichment/.py` orchestration file (DB-aware, called from + `jobs/enrichment.py`'s dispatch), paired with a sibling mechanics-only file with no DB + knowledge when ffmpeg is involved (`transcribe.py` → `audio.py`). `media/` holds only + `ranged.py` (HTTP range-response streaming), a different kind of code entirely. M7 + follows the established pattern: `enrichment/subvideo.py` (mechanics) + + `enrichment/extract_subvideo.py` (orchestration) — no new `media/` files at all. +- **`routers/clips.py` is the one part of the sketch that held up as named.** + +## Design decisions + +### A clip is a `source`, not an `asset_type` + +`Asset.asset_type` stays the parent's real media type (`video`/`audio`) — the existing +`Preview` switch, `ASSET_TYPES` validation, and type filters needed zero changes. What +identifies a clip *physically* is `storage_key IS NULL AND parent_asset_id IS NOT NULL`, +exactly as the model's own docstring already framed it before this milestone touched it. +Two new values went into `ingest/filetypes.py`'s **source** constants instead — +`SOURCE_CLIP = "clip"`, `SOURCE_SUBVIDEO = "sub_video"` — safe because `source`, unlike +`asset_type`, was never validated against a frozenset. + +`source` doubles as the frontend-facing signal for "does this row own bytes" — +`AssetRead`/`Asset` already exposed `source`, so nothing new had to be added there. An +earlier draft of this design added a computed `is_clip: bool`; dropped in favour of the +field already being written in the two places (`create_clip`, `promote_clip`) that also +touch `storage_key`, so it can't independently drift. + +Checked directly, not assumed: every `asset_type` switch in the codebase +(`enrichment/source.py`, `transcribe.py::can_transcribe`, `describe.py`, +`extract_text.py::extractable`) already also gates on `storage_key`/`thumb_key` being +non-null, so a clip falls through to a clean 400 everywhere that matters, never a crash. +Search/FTS/tags switch on nothing asset-type-related at all — a clip indexes and is +found exactly like any other asset once `create_clip` calls `_reindex`. + +### The blocking query is narrower than "has a parent" + +Only children with `parent_asset_id = X AND storage_key IS NULL` block deleting `X` — a +promoted clip or a freshly-extracted sub-video has its own `storage_key`, so even though +it keeps `parent_asset_id` as a provenance breadcrumb, it must not block the parent's +delete or appear in `blocking_clips`. This stays the physical, DB-level check (not the +`source` label) — it's what a delete actually depends on being true. + +`delete_asset` proactively nulls out `parent_asset_id` on *non-blocking* children before +deleting the parent (`services/assets.py::delete_asset`) — otherwise the real foreign +key (`PRAGMA foreign_keys=ON` on every connection, matching production) raises on them +too, the same reason `AssetTag`/`Suggestion` rows are already cleared first. This only +manifests once the promote flow is actually used, which is exactly the kind of gap that +ships clean and breaks in the field — it's covered by +`test_delete_after_promoting_succeeds`-shaped tests in `test_clips_api.py`. + +For the constraint to exist at all, the migration needed an explicit +`batch_op.create_foreign_key(...)`, not just `add_column` — a bare nullable column gets +SQLModel's ORM-level awareness of the relationship but not SQLite's actual enforcement +of it, which is what the guard-before-delete design depends on. No prior migration in +this repo added a foreign key to an *already-existing* table (both of the other two — +`assettag`, `suggestion` — got theirs at `create_table` time), so this is a new pattern +here, verified against a real SQLite database (`PRAGMA foreign_key_list(asset)`), not +just against the model. + +### `gather()` needed a clip guard M6 never anticipated + +`enrichment/source.py::gather()` is the one chokepoint describe/summarize/autotag/ +generate_all all read through. Because a clip carries its parent's `thumb_key` (copied +directly at creation — see below), `gather()`'s poster-image fallback would otherwise +silently succeed on a clip using a generic frame — never the parent's transcript for +that exact time range — producing a description of the wrong content rather than +failing loudly. One guard clause at the top of `gather()` closes this for all four jobs +at once, using the `NoSourceMaterial` exception type and cascade handling that already +existed. This is defense-in-depth even where a per-job pre-check +(`can_transcribe`/`extractable`-style) might already 400 the common button-click path — +it's the one place all four jobs funnel through regardless. + +The frontend independently stops *offering* these actions on a clip (`source === +'clip'` in `AssetDetail.tsx`) — the backend 400s/refuses cleanly either way, but a UI +that shows four buttons which all fail immediately is a real defect even though nothing +crashes. Gated on `source`, not on `parent_asset_id` being set — a promoted or +freshly-extracted sub-video also carries `parent_asset_id` as a breadcrumb but owns +real bytes and keeps every one of these actions. + +### Two backend flows, not one + +The acceptance test exercises them separately, and they stay separate in the API: + +- **Fresh extraction** (`POST /assets/{id}/subvideo`) creates a brand-new standalone + Asset only once the job succeeds — no placeholder row, parent genuinely untouched. +- **Non-destructive clip** (`POST /assets/{id}/clips`) is fully synchronous, no ffmpeg, + no job — an Asset insert and nothing else. 201, not 202. +- **Promote** (`POST /assets/{clip_id}/promote`) reuses the same ffmpeg job but updates + the *existing clip row in place* at completion (`storage_key` set, `source` flips + `clip`→`sub_video`, `in_point`/`out_point` cleared — see the bug below), so nothing + that already references that asset id breaks. + +One new job kind, `KIND_EXTRACT_SUBVIDEO`, deliberately not added to `ENRICHMENT_KINDS` +(that gates M6's AI-cost bulk-enrichment UI; this has no AI cost, the same reason +`KIND_EXTRACT_TEXT` sits apart from the LLM jobs) but dispatched through the same +`EnrichmentJob` table/`_run_job` chain everything else uses. Its payload carries +`{"mode": "extract"|"promote", "in_point", "out_point", "name"?}` going in; on +completion, `jobs/enrichment.py`'s dispatch overwrites it to +`{"created_asset_id": ...}` for "extract" mode only, which is what lets +`ActivityJobRead.result_asset_id` (new field, populated in `jobs/registry.py` from a +best-effort JSON parse, mirroring `transcripts.py::_words_of`) tell the frontend what +was created — `job.asset_id` stays pointed at the *source* throughout an extract job's +life, so without this the frontend would have no way to learn the new asset's id at all. + +No aggregate "promote all" job kind — the frontend loops, firing one promote call per +dependent clip (typically a handful), polling `GET /api/activity/{kind}/{job_id}` +(added to `activityApi` — the existing `list` endpoint's default 25-row cap and the +activity store's own capped `jobs` array are both real limits when waiting on several +specific ids at once) to a terminal state for each. Only once *every* promote succeeds +does the frontend retry the delete; a failure surfaces and stops rather than retrying +into another guard or dropping a clip silently. + +### A bug `promote_clip` almost shipped with + +`in_point`/`out_point` are **cleared**, not preserved, when a clip is promoted. The +first draft kept them "for provenance" — wrong: they're coordinates into the *parent's* +timeline, and the freshly extracted file has its own timeline starting at 0. Leaving +the old values in place would have `AssetDetail`'s playback-bounding logic (which seeks +to `asset.in_point` and pauses at `asset.out_point` for any asset that has them) seek a +nine-second standalone file to its old absolute second 61 the moment it was promoted. +Caught by re-deriving the logic during implementation, not by a test — worth naming +because it's exactly the kind of thing a future change to either promote or the player +could reintroduce without one. + +### A second bug: the delete guard's own remount + +`stores/library.ts`'s `remove()` optimistically drops the asset from the library +store's `assets` array *before* the API call resolves, then restores it if the call +fails. `AssetView.tsx` reads its asset via `assets.find(...)` and renders `null` when +that comes back empty — so calling `remove()` for an asset that turns out to be guarded +briefly unmounts `AssetDetail` entirely, and remounts a *fresh* instance once the store +puts the asset back. That's invisible for a plain failure (resetting `confirmingDelete` +to `false` is what a fresh mount does anyway), but the first implementation of the +promote-guard UI called `setDependentClips(...)` from inside `remove()`'s `catch` — +state set on a component instance that had already been replaced by the time it ran, +so it silently did nothing. Three new frontend tests failed against this before it was +diagnosed and fixed. + +The fix is architectural, not a patch: `confirmDelete` now checks `clipsApi.list` for +blocking clips *before* ever calling `remove()`, rather than reacting to a 409 from it. +The guarded path never triggers the optimistic-removal dance at all. The backend's own +`blocking_clips` check in `routers/assets.py::delete_asset` stays the authoritative +enforcement — this is a client-side UX fix, not a replacement for it; a race (a clip +created between the check and the delete, vanishingly unlikely for anything but a +multi-tab session) falls back to the plain "could not delete" path rather than the +promote UI, which is an acceptable edge case for something this rare. + +### The ffmpeg fallback heuristic, verified against real output + +`enrichment/subvideo.py`'s two strategies: input-side seek + stream copy +(`-ss` before `-i`, `-c copy`) is fast but can only start on a keyframe, so a cut +between keyframes overshoots — ffmpeg targets `in_point + duration` in the source's +timeline but has to start at the nearest keyframe at or before `in_point`, so the actual +output spans further back than requested. The fallback (output-side seek, always +re-encodes to h264/aac) is slower but frame-accurate. The output's duration (via +`ingest/probe.py::probe()`, reused rather than reinvented) is checked against the +requested one; more than ~0.5s over triggers the fallback automatically, no user-facing +toggle. + +This sandbox started with no ffmpeg (`test_subvideo_extraction.py`'s `needs_ffmpeg`- +marked tests all skipped). Installing it for real (`apt-get install --no-install- +recommends ffmpeg`) rather than trusting the skip marker caught two problems the +skip would have hidden entirely: two test fixtures assumed a fixture video's duration +would be `None` (true only because ffprobe was unavailable) and broke once it was +real; a `gather()` test's expectation depended on whether the asset happened to have a +poster, which differs by environment. Neither was a product bug, but both would have +looked like passing coverage while testing nothing real. The fallback path itself is +verified with two synthetic fixtures generated on the fly (`ffmpeg -f lavfi testsrc`, +one with keyframes every 10s, one every 1s) so the trigger condition is deterministic +rather than depending on where a keyframe happens to land in the checked-in sample +video. + +### `write_file`'s real failure mode is `EXDEV`, not extra I/O + +`LocalStorage` gained `write_file(key, source_path)` for adopting an already-on-disk +file (ffmpeg's own output) without reading it fully into memory. The obvious +implementation — `os.rename`/`os.replace(source_path, target)` — works in a dev +environment where `tempfile.TemporaryDirectory()` and the media root happen to share a +filesystem, and fails in a real deployment (a Docker bind-mounted media volume) +with `EXDEV: Invalid cross-device link`, since rename requires same-filesystem. The +implementation is a genuine chunked copy into `.partial` **inside the storage +root**, hashing as it streams (mirroring `write_stream`'s pattern exactly), then +`os.replace`s that `.partial` file into place — always same-filesystem, since both live +under `self.root`. + +### Scope cuts + +- **Clipping a clip is refused (400), in both directions** — non-destructively and via + fresh sub-video extraction. The validation pass argued extracting a sub-video *from* a + clip isn't actually hard (resolve to absolute coordinates against the real ancestor and + it's an ordinary cut, since a clip is guaranteed one level deep once clip-of-clip is + refused non-destructively). Weighed and rejected: **promote already does "turn this + clip into a real file,"** in place, via the same ffmpeg mechanics. A second path to + the same end state, leaving the clip intact, would be a third coordinate-flattening + code path for marginal benefit over what promote covers. +- An existing clip's in/out bounds can't be edited after creation — only created or + promoted. +- AI enrichment (describe/summarize/autotag/generate_all) is unavailable on clips — + see the `gather()` guard above. + +--- + +## File map + +**Backend, new:** `enrichment/subvideo.py` (ffmpeg mechanics), `enrichment/ +extract_subvideo.py` (orchestration), `routers/clips.py`, the migration adding +`parent_asset_id`/`in_point`/`out_point` to `asset`. + +**Backend, extended:** `models/asset.py` (the three columns), `models/job.py` +(`KIND_EXTRACT_SUBVIDEO`), `ingest/filetypes.py` (`SOURCE_CLIP`/`SOURCE_SUBVIDEO`), +`storage/base.py` + `storage/local.py` (`write_file`), `services/assets.py` +(`create_clip`, `list_children`, `blocking_clips`, `create_subvideo_asset`, +`promote_clip`, `parents_for_many`, `to_read_model`'s parent-resolution), `enrichment/ +source.py` (`gather()`'s guard), `jobs/enrichment.py` (dispatch branch, `submit()`'s +`payload` kwarg), `jobs/registry.py` (`result_asset_id`), `schemas_jobs.py` +(`ActivityJobRead.result_asset_id`), `schemas_assets.py` (`AssetRead`'s three new +fields), `routers/assets.py` (the delete guard), `routers/search.py` (parent +resolution for a clip appearing in a search hit), `main.py` (router registration). + +**Frontend, new:** `api/clips.ts`, `components/ClipEditor.tsx`. + +**Frontend, extended:** `api/assets.ts`, `api/transcripts.ts` (`result_asset_id`, +`activityApi.get`), `components/EnrichmentButton.tsx` (`onFinish` callback — small, +backward-compatible, so "Extract as sub-video" could reuse the whole component rather +than reimplementing its running/error/polling states), `components/AssetDetail.tsx` +(Clip tab, playback bounding, enrichment gating, the delete-guard promote flow). + +**Tests:** `test_clips_api.py`, `test_subvideo_extraction.py` (backend); the `clips +(M7)` block in `views/AssetView.test.tsx` (frontend — `AssetDetail` has no dedicated +test file, it's exercised through the view that renders it, matching the existing +convention for that component). + +--- + +## Verification + +- `cd backend && pytest -q` — 829 passed, real ffmpeg installed in this sandbox to get + genuine coverage rather than skips. +- `cd frontend && npx tsc --noEmit && npx eslint . --max-warnings 0 && npx vitest run + && npm run build` — 264 passed, clean typecheck, clean lint, clean build. +- Manual, in a real dev environment: the plan doc's own M7 acceptance test — cut a clip + at 01:01–01:10, confirm no new file exists on disk and playback is bounded; extract + the same range as a sub-video, confirm the file is standalone and the parent is + untouched; attempt to delete a parent with clips, confirm the guard fires with the + dependent list, and the promote path clears it and lets the delete through. diff --git a/docs/plan-of-attack.md b/docs/plan-of-attack.md index c9681a4..86884cc 100644 --- a/docs/plan-of-attack.md +++ b/docs/plan-of-attack.md @@ -439,6 +439,13 @@ a thing a future session will otherwise assume is done. exercised only by tests with stubbed retrievers, which prove the *fusion* is correct and say nothing about retrieval quality. Until that run happens, treat M5 as structurally complete and qualitatively unmeasured. + + **Update, reported during M7**: the user confirms embedding is deployed and operating + well in production. Recorded as-given rather than upgraded into a pass on the specific + acceptance queries above — a general "it's working" and "the Giordano/Schwab queries + return the right asset at the right second" are different claims, and only the user is + in a position to run the second. If that specific run has also happened, this note + should be replaced with the result rather than left to imply it from the general one. 2. **GN-7 and GN-8 are specified and unapplied** (python-jose on five CVEs; the Python version). They are changes to `davior/gecko-notes`, not here — see `docs/gecko-notes-integration.md`. diff --git a/frontend/src/api/assets.ts b/frontend/src/api/assets.ts index cec251f..b1d4088 100644 --- a/frontend/src/api/assets.ts +++ b/frontend/src/api/assets.ts @@ -20,6 +20,15 @@ export interface Asset { asset_type: AssetType source: string + /** M7. Provenance on any of clip/promoted/extracted; only a live clip + * (`source === 'clip'`) has no file of its own and needs `in_point`/`out_point` + * to bound playback of its parent's bytes — a promoted or freshly-extracted + * sub-video keeps this as a breadcrumb but is a standalone asset in every other + * respect, `source` (not this) is what tells the two apart. */ + parent_asset_id: string | null + in_point: number | null + out_point: number | null + original_name: string | null mime_type: string | null file_format: string | null diff --git a/frontend/src/api/clips.ts b/frontend/src/api/clips.ts new file mode 100644 index 0000000..003991a --- /dev/null +++ b/frontend/src/api/clips.ts @@ -0,0 +1,59 @@ +import client from '@/api/client' +import type { Asset } from '@/api/assets' +import type { ActivityJob } from '@/api/transcripts' + +/** + * Clips (non-destructive) and sub-video extraction (destructive), over `routers/clips.py`. + * + * One `xApi` per backend router, the house convention — `Asset`/`ActivityJob` are + * imported rather than redeclared, since `api/assets.ts` and `api/transcripts.ts` + * already own those shapes. + */ + +export interface ClipRange { + in_point: number + out_point: number + name?: string +} + +interface DataResponse { + data: T +} + +interface ListResponse { + data: T[] + total: number +} + +export const clipsApi = { + /** A window into the parent's bytes: no file, no job — resolves immediately. */ + create(assetId: string, range: ClipRange): Promise { + return client + .post>(`/assets/${assetId}/clips`, range) + .then((r) => r.data.data) + }, + + /** Everything derived from this asset: live clips and past promotions/extractions — + * filter to `source === 'clip'` for just the ones still depending on it. */ + list(assetId: string): Promise { + return client + .get>(`/assets/${assetId}/clips`) + .then((r) => r.data.data) + }, + + /** Queues a fresh, standalone extraction. The source asset is untouched; the new + * asset's id lands on the finished job as `result_asset_id`. */ + extractSubvideo(assetId: string, range: ClipRange): Promise { + return client + .post>(`/assets/${assetId}/subvideo`, range) + .then((r) => r.data.data) + }, + + /** Turns a live clip into a standalone sub-video, in place — the delete guard's + * one-click path. `clipId` is the clip's own id, not its parent's. */ + promote(clipId: string): Promise { + return client + .post>(`/assets/${clipId}/promote`) + .then((r) => r.data.data) + }, +} diff --git a/frontend/src/api/transcripts.ts b/frontend/src/api/transcripts.ts index ed07903..c22955f 100644 --- a/frontend/src/api/transcripts.ts +++ b/frontend/src/api/transcripts.ts @@ -40,6 +40,10 @@ export interface ActivityJob { asset_id: string | null asset_name: string model: string + /** M7 only: set once a `extract_subvideo` "extract" job finishes, to the asset it + * created. `asset_id` stays pointed at the source throughout, so this is the only + * place to learn the result's id. */ + result_asset_id: string | null error_message: string | null created_at: string updated_at: string @@ -90,6 +94,15 @@ export const activityApi = { .then((r) => r.data.data) }, + /** One job by id, for polling a specific run rather than the whole active list — + * M7's promote flow waits on several jobs at once, which the store's capped + * `jobs` array cannot be trusted to still contain all of by the time they finish. */ + get(kind: string, jobId: string): Promise { + return client + .get>(`/activity/${kind}/${jobId}`) + .then((r) => r.data.data) + }, + cancel(kind: string, jobId: string): Promise { return client .delete>(`/activity/${kind}/${jobId}`) diff --git a/frontend/src/components/ActivityIndicator.test.tsx b/frontend/src/components/ActivityIndicator.test.tsx index 6f37979..afb0e4e 100644 --- a/frontend/src/components/ActivityIndicator.test.tsx +++ b/frontend/src/components/ActivityIndicator.test.tsx @@ -19,6 +19,7 @@ function job(overrides: Partial = {}): ActivityJob { asset_id: 'a1', asset_name: 'Interview', model: 'nova-3', + result_asset_id: null, error_message: null, created_at: '2026-09-14T10:00:00Z', updated_at: '2026-09-14T10:00:00Z', diff --git a/frontend/src/components/AssetDetail.tsx b/frontend/src/components/AssetDetail.tsx index 012d835..649b7c1 100644 --- a/frontend/src/components/AssetDetail.tsx +++ b/frontend/src/components/AssetDetail.tsx @@ -7,6 +7,7 @@ import { Mic, Pencil, ScanText, + Scissors, Tags as TagsIcon, Trash2, Wand2, @@ -15,6 +16,8 @@ import { import type { Asset, AssetUpdate } from '@/api/assets' import { tagsApi } from '@/api/tags' import { enrichmentApi } from '@/api/enrichment' +import { clipsApi } from '@/api/clips' +import { activityApi } from '@/api/transcripts' import { formatCost, usageApi, type UsageTotals } from '@/api/usage' import { apiErrorMessage } from '@/api/client' import { useLibraryStore } from '@/stores/library' @@ -22,6 +25,7 @@ import { useTagStore } from '@/stores/tags' import { formatBytes, formatDate, formatDimensions, formatDuration } from '@/utils/format' import { useAutoGrow } from '@/utils/useAutoGrow' import AssetThumb from '@/components/AssetThumb' +import ClipEditor from '@/components/ClipEditor' import DocumentTextPanel from '@/components/DocumentTextPanel' import EmbedButton from '@/components/EmbedButton' import EnrichmentButton from '@/components/EnrichmentButton' @@ -30,6 +34,34 @@ import TagInput from '@/components/TagInput' import Tabs, { type TabSpec } from '@/components/Tabs' import TranscriptPanel from '@/components/TranscriptPanel' +/** A clip has no bytes, no transcript and no AI enrichment of its own — a promoted or + * freshly-extracted sub-video does, and `source` (not `parent_asset_id`, which both + * carry as provenance) is what tells the two apart. */ +function ownsNoFile(asset: Asset): boolean { + return asset.source === 'clip' +} + +/** Poll a set of M7 extraction/promotion jobs to a terminal state, by id rather than + * through the activity store's capped `jobs` list — the promote-all flow can be + * waiting on more jobs than that list is guaranteed to still contain. Throws with the + * first failure's message if any job ends in error. */ +async function waitForJobs(jobIds: string[]): Promise { + let remaining = jobIds + while (remaining.length > 0) { + const jobs = await Promise.all(remaining.map((id) => activityApi.get('enrichment', id))) + const failed = jobs.find((j) => j.status === 'error') + if (failed) throw new Error(failed.error_message ?? 'A clip could not be promoted') + remaining = jobs + .filter((j) => j.status === 'queued' || j.status === 'processing') + .map((j) => j.id) + // Checked before waiting, not after: a promote that already finished by the time + // this polls should not pay a fixed delay it does not need. + if (remaining.length > 0) { + await new Promise((resolve) => setTimeout(resolve, 1500)) + } + } +} + /** Audio and video can be transcribed; nothing else has speech in it. */ const SPEECH_TYPES = new Set(['audio', 'video']) @@ -80,6 +112,7 @@ export default function AssetDetail({ }: Props) { const update = useLibraryStore((s) => s.update) const remove = useLibraryStore((s) => s.remove) + const refreshAsset = useLibraryStore((s) => s.refreshAsset) const setAssetTags = useLibraryStore((s) => s.setAssetTags) const suggestions = useTagStore((s) => s.tags) const rememberTags = useTagStore((s) => s.remember) @@ -91,6 +124,11 @@ export default function AssetDetail({ const [saving, setSaving] = useState(false) const [tagError, setTagError] = useState(null) const [confirmingDelete, setConfirmingDelete] = useState(false) + // Set once `remove()` reports `asset_has_dependent_clips` — the confirm block + // switches from "delete this?" to "promote these, then delete" while this is set. + const [dependentClips, setDependentClips] = useState(null) + const [promoting, setPromoting] = useState(false) + const [promoteError, setPromoteError] = useState(null) const [copied, setCopied] = useState(false) const descriptionRef = useAutoGrow(description) @@ -109,10 +147,13 @@ export default function AssetDetail({ const attachPlayer = useCallback( (element: HTMLVideoElement | HTMLAudioElement | null) => { setPlayer(element) - if (!element || startAt === undefined) return + // A clip's own bound, when nothing more specific (a search hit's timestamp) was + // asked for — a clip opens at its in point, not at the parent's start. + const effectiveStart = startAt ?? asset.in_point ?? undefined + if (!element || effectiveStart === undefined) return const apply = () => { - element.currentTime = startAt + element.currentTime = effectiveStart } if (element.readyState >= 1) { apply() @@ -120,7 +161,7 @@ export default function AssetDetail({ element.addEventListener('loadedmetadata', apply, { once: true }) } }, - [startAt] + [startAt, asset.in_point] ) const seekTo = useCallback( @@ -143,9 +184,11 @@ export default function AssetDetail({ // replaces the asset, the effect re-seeds, and the textarea shows the new text. setSummary(asset.summary ?? '') setConfirmingDelete(false) + setDependentClips(null) + setPromoteError(null) setTagError(null) - setCurrentTime(startAt ?? 0) - }, [asset.id, asset.name, asset.description, asset.summary, startAt]) + setCurrentTime(startAt ?? asset.in_point ?? 0) + }, [asset.id, asset.name, asset.description, asset.summary, asset.in_point, startAt]) // The panel can be reached from search, which never renders the filter bar, so it // asks for the catalogue itself rather than assuming somebody else did. @@ -242,6 +285,27 @@ export default function AssetDetail({ } const confirmDelete = async () => { + // Checked before calling `remove()`, not caught from its rejection: `remove` + // optimistically drops the asset from the store *before* the request resolves, + // which makes `AssetView`'s `assets.find(...)` briefly come back `undefined` — + // unmounting this whole component — and remounting it fresh once the store + // restores the asset on failure. That is invisible for a plain failure (it just + // resets `confirmingDelete` to what a fresh mount already starts at), but it would + // silently discard `setDependentClips` below, called on a component instance that + // no longer exists by the time the guard's 409 comes back. Checking first means + // the guarded path never calls `remove()` at all, so it never hits that cycle. + try { + const children = await clipsApi.list(asset.id) + const blocking = children.filter((c) => c.source === 'clip') + if (blocking.length > 0) { + setDependentClips(blocking) + return + } + } catch { + // Could not even check — fall through and let the real delete attempt, and its + // own error handling below, be the source of truth. + } + try { await remove(asset.id) onClose() @@ -250,21 +314,43 @@ export default function AssetDetail({ } } + const promoteAllAndDelete = async () => { + if (!dependentClips || dependentClips.length === 0) return + setPromoting(true) + setPromoteError(null) + try { + const jobs = await Promise.all(dependentClips.map((c) => clipsApi.promote(c.id))) + await waitForJobs(jobs.map((j) => j.id)) + await Promise.all(dependentClips.map((c) => refreshAsset(c.id))) + await remove(asset.id) + onClose() + } catch (err) { + setPromoteError(apiErrorMessage(err, 'Could not promote all of them')) + } finally { + setPromoting(false) + } + } + const visual = VISUAL_TYPES.has(asset.asset_type) const details = (
{/* One button that runs describe, summarise and autotag in turn, for anyone who - would otherwise press the three icon buttons below one after another. */} - + would otherwise press the three icon buttons below one after another. Absent + for a clip (M7): it has no bytes and no transcript of its own to read from — + see `enrichment/source.py::gather()`'s guard — so offering the button would + just be an immediate, confusing failure. */} + {!ownsNoFile(asset) && ( + + )}
{/* Beside the label it writes into, rather than as its own row below the box — describe fills this field, and that is what the icon says. */} - + {!ownsNoFile(asset) && ( + + )}