review: a browser step for timecode-anchored feedback

[bluworl]

What

The process in SKILL.md goes from step 6 "Preview" to step 8 "Iterate", and in
between there is no step for looking at the cut and saying what is wrong with
it. Feedback comes back as "the bit about halfway through chapter four is
long", and somebody has to go find it.

This adds the missing step: a page you open from disk, scrub, and leave comments
on exact timecodes. cut, shorten, lengthen, wrong, free text, or a voice
note. A comment can also be pinned to a point inside the frame, for the notes
that are about a place rather than a moment: a logo, a caption, a hand in the
wrong position.

review.py --dump reads them back as time-ordered markdown with frame numbers,
so they can be turned into EDL changes.

A real pass over a 21-minute cut at 1.5x, which is how this was tested.

python helpers/review.py <edit>/final.mp4
python helpers/review.py --dump <edit>/review/final.review.json

- **4:16.23** `shorten` (frames 6155-6402): this run goes nowhere
- **5:34.03** `wrong` (frame 8014 at 62%,34% of the frame): logo from the old brand
- **7:02.11** `note` (frame 10126): (spoken) hold this beat, it lands better

Nothing is applied automatically. The notes stay notes: turning them into cuts
is a decision, not a transformation.

Why there is no server

The first design had a local http.server with hand-written Range support.
Measured on Chrome 153, a file:// page is already a secure context, gets
MediaRecorder and showDirectoryPicker, and seeks 150s into a 380MB local
file in 201ms. On a 1.08GB, 21-minute cut the seeks came back at 40-103ms.

Range support was the only thing the server would have added, so it is gone: no
process to stop, no port, no new dependency, and nothing left running after the
review.

Pointing at the frame

Coordinates are fractions of the picture, not of the element, so a point stays
on the same pixel when the window changes size or moves to another screen. A
click on the letterbox bars is ignored rather than turned into a coordinate that
is not on the picture. The comment panel moves to the top of the frame when the
point is low, because lower thirds are where captions and logos live.

Play and pause stay on the space bar, so pointing at something never costs a
stray pause.

Voice notes

The page records and writes the audio; it never calls ElevenLabs, because that
would mean shipping the API key inside an HTML file. --dump transcribes with
the same Scribe call transcribe.py uses, writes the text back into the notes
file, and so never pays for the same audio twice.

Browser support

Chrome and Edge save notes and voice straight to disk, through a directory
handle kept in IndexedDB, so the folder is asked for once. Elsewhere the page
says so on load and falls back to downloading the JSON; recording is disabled
with the reason shown in the button.

Implementation notes

• No new dependencies. Frame rate parsing reuses render.parse_fps and
  render.probe_source_fps; transcription reuses transcribe.call_scribe.
  parse_fps signals a bad rate with argparse.ArgumentTypeError, which is not
  a ValueError, so review.py translates it rather than leaking an argparse
  exception out of a library function.
• The page and Python agree on one rule for frames, round(t * fps), written in
  both places and checked on both sides.
• The generated page refuses to be written when its relative path back to the
  video does not resolve to the video, rather than producing a <video> that
  silently fails to load.
• Tests follow tests/test_render_fps.py: unittest, loaded with importlib,
  no network, no browser, no ffmpeg on real media.

python -m unittest discover -s tests

The page itself has no unit tests, deliberately: tests/ should stay runnable
without a browser. It was verified by driving a real Chrome against a generated
clip, including the fallbacks and a full round trip from a recorded voice note
to its transcription.

🤖 Generated with Claude Code

https://claude.ai/code/session_01827TRKdsvH5j5fakiczykP

---

Summary by cubic

The workflow previously jumped from Preview directly to Iterate; this adds Review between them so feedback lands on exact timecodes instead of "the bit about halfway through chapter four." review.py <video> writes a browser page next to the cut, and review.py --dump reads the notes back as time-ordered markdown.

• Reviewers scrub the page and leave cut, shorten, lengthen, wrong, free-text, or voice notes, optionally pinned to a point inside the frame.
• The page runs from disk with no server; Chrome and Edge save notes and voice straight into review/, other browsers fall back to a JSON download and disable voice with the reason shown.
• Voice notes are transcribed once on --dump via the existing Scribe path, and the text is written back into the notes file.
• Notes are never applied automatically; turning them into edits stays a decision.
• Frame-rate parsing and transcription reuse render.parse_fps and transcribe.call_scribe, and tests/test_review.py runs without a browser or network.
• SKILL.md now has Review as step 8 and Iterate as step 9.

^{Written for commit f869249. Summary will update on new commits.}

[cubic-dev-ai]

11 issues found across 4 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="helpers/review.py">

<violation number="1" location="helpers/review.py:54">
P2: At an exact half-frame, Python `round` uses ties-to-even while the browser uses `Math.round`, so Python-side frame mapping and dumped timecodes can disagree with the page. Use the JavaScript-equivalent nonnegative rounding in both `time_to_frame` and `format_timecode`.</violation>

<violation number="2" location="helpers/review.py:149">
P2: When Scribe returns an empty transcript, every later `--dump` re-uploads the same voice note because the empty result is treated as missing. Test for `voice_text is None` instead of falsiness.</violation>

<violation number="3" location="helpers/review.py:160">
P1: When `--dump` processes an untrusted review JSON, a `voice` path can escape the review folder and be uploaded to Scribe. Reject resolved paths outside `notes_path.parent` before extracting audio.</violation>

<violation number="4" location="helpers/review.py:169">
P2: A single failed voice note aborts the whole `--dump` run. `call_scribe` raises `RuntimeError` on any non-200 response and `extract_audio` raises on ffmpeg failure; neither is caught inside `transcribe_pending`, so the exception propagates through `dump_notes`/`main`, the dump prints a traceback, notes already transcribed in this run are never written back (the paid transcription is thrown away), and every remaining pending note stays untranscribed until the user re-runs and pays again. This is inconsistent with the function's own graceful paths for a missing file or missing API key.</violation>

<violation number="5" location="helpers/review.py:174">
P1: If the review page remains open while transcription runs, autosaved notes added during that time are lost when this stale snapshot replaces the file. Re-read and merge the file, or coordinate the write with the page before saving transcription results.</violation>

<violation number="6" location="helpers/review.py:267">
P2: When invoked from the repository root as documented, the suggested read-back command uses `python review.py` and fails because the script is under `helpers/`. Print a resolved helper path and shell-quote both it and the notes path.</violation>
</file>

<file name="helpers/review.html">

<violation number="1" location="helpers/review.html:8">
P2: When the page is opened offline, the review UI loses its Tailwind styling because the CDN resources are unavailable, violating the local/offline workflow. Vendor the CSS and fonts locally, and remove the runtime CDN dependency.</violation>

<violation number="2" location="helpers/review.html:62">
P1: When the video aspect ratio differs from the stage, `contentBox()` and the displayed picture use different geometry, so frame-area comments land at the wrong coordinates. Add `object-contain` to the video element.</violation>

<violation number="3" location="helpers/review.html:159">
P1: When two projects contain videos with the same stem, the second review reuses the first review's browser state and disk handle, risking cross-project note contamination and writes. Key localStorage and IndexedDB entries by a unique page/video identity, not just `params.stem`.</violation>

<violation number="4" location="helpers/review.html:527">
P2: When Save or Cancel stops an active recording, the asynchronous `onstop` callback can attach that abandoned recording to the next note saved without recording. Invalidate the recorder session before stopping and ignore stale `onstop` callbacks.</violation>

<violation number="5" location="helpers/review.html:698">
P2: When the disk review is newer but has fewer or the same number of notes, the count-only conflict rule keeps stale localStorage data and can resurrect deletions or overwrite edits. Compare an explicit revision/timestamp or merge notes by identity instead of choosing by count.</violation>
</file>

<sub>Tip: instead of fixing issues one by one fix them all with cubic

Re-trigger cubic</sub>

[cubic-dev-ai]

P1: If the review page remains open while transcription runs, autosaved notes added during that time are lost when this stale snapshot replaces the file. Re-read and merge the file, or coordinate the write with the page before saving transcription results.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At helpers/review.py, line 174:

<comment>If the review page remains open while transcription runs, autosaved notes added during that time are lost when this stale snapshot replaces the file. Re-read and merge the file, or coordinate the write with the page before saving transcription results.</comment>

<file context>
@@ -0,0 +1,273 @@
+        done += 1
+
+    if done:
+        notes_path.write_text(
+            json.dumps(data, indent=2, ensure_ascii=False), encoding="utf-8"
+        )
</file context>

[cubic-dev-ai]

P1: When --dump processes an untrusted review JSON, a voice path can escape the review folder and be uploaded to Scribe. Reject resolved paths outside notes_path.parent before extracting audio.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At helpers/review.py, line 160:

<comment>When `--dump` processes an untrusted review JSON, a `voice` path can escape the review folder and be uploaded to Scribe. Reject resolved paths outside `notes_path.parent` before extracting audio.</comment>

<file context>
@@ -0,0 +1,273 @@
+
+    done = 0
+    for note in pending:
+        audio = (notes_path.parent / note["voice"]).resolve()
+        if not audio.exists():
+            print(f"voice file missing, skipped: {note['voice']}", file=sys.stderr)
</file context>

[cubic-dev-ai]

P1: When two projects contain videos with the same stem, the second review reuses the first review's browser state and disk handle, risking cross-project note contamination and writes. Key localStorage and IndexedDB entries by a unique page/video identity, not just params.stem.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At helpers/review.html, line 159:

<comment>When two projects contain videos with the same stem, the second review reuses the first review's browser state and disk handle, risking cross-project note contamination and writes. Key localStorage and IndexedDB entries by a unique page/video identity, not just `params.stem`.</comment>

<file context>
@@ -0,0 +1,955 @@
+    wrong: "#ff6b35", note: "#a8a29e",
+  };
+  const SPEEDS = [0.5, 0.75, 1, 1.25, 1.5, 1.75, 2];
+  const NOTES_KEY = "video-use-review:" + (params.stem || "review");
+  const SPEED_KEY = "video-use-review-speed";
+
</file context>

[cubic-dev-ai]

P1: When the video aspect ratio differs from the stage, contentBox() and the displayed picture use different geometry, so frame-area comments land at the wrong coordinates. Add object-contain to the video element.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At helpers/review.html, line 62:

<comment>When the video aspect ratio differs from the stage, `contentBox()` and the displayed picture use different geometry, so frame-area comments land at the wrong coordinates. Add `object-contain` to the video element.</comment>

<file context>
@@ -0,0 +1,955 @@
+    <!-- Stage -->
+    <main class="flex-1 min-w-0 flex flex-col">
+      <div id="stage" class="relative flex-1 min-h-0 bg-black cursor-crosshair">
+        <video id="video" class="absolute inset-0 w-full h-full" preload="metadata"></video>
+        <div id="pins" class="absolute pointer-events-none"></div>
+
</file context>

Suggested change

	<video id="video" class="absolute inset-0 w-full h-full" preload="metadata"></video>
	<video id="video" class="absolute inset-0 w-full h-full object-contain" preload="metadata"></video>

[cubic-dev-ai]

P2: When Scribe returns an empty transcript, every later --dump re-uploads the same voice note because the empty result is treated as missing. Test for voice_text is None instead of falsiness.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At helpers/review.py, line 149:

<comment>When Scribe returns an empty transcript, every later `--dump` re-uploads the same voice note because the empty result is treated as missing. Test for `voice_text is None` instead of falsiness.</comment>

<file context>
@@ -0,0 +1,273 @@
+    The result is written back into the notes file so a second --dump does not
+    pay for the same audio twice.
+    """
+    pending = [n for n in data["notes"] if n.get("voice") and not n.get("voice_text")]
+    if not pending:
+        return 0
</file context>

Suggested change

	pending = [n for n in data["notes"] if n.get("voice") and not n.get("voice_text")]
	pending = [n for n in data["notes"] if n.get("voice") and n.get("voice_text") is None]

[cubic-dev-ai]

P2: At an exact half-frame, Python round uses ties-to-even while the browser uses Math.round, so Python-side frame mapping and dumped timecodes can disagree with the page. Use the JavaScript-equivalent nonnegative rounding in both time_to_frame and format_timecode.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At helpers/review.py, line 54:

<comment>At an exact half-frame, Python `round` uses ties-to-even while the browser uses `Math.round`, so Python-side frame mapping and dumped timecodes can disagree with the page. Use the JavaScript-equivalent nonnegative rounding in both `time_to_frame` and `format_timecode`.</comment>

<file context>
@@ -0,0 +1,273 @@
+def time_to_frame(seconds: float, fps: float) -> int:
+    """Which frame an instant falls on. A note that says 'cut' has to become a
+    cut later, and that needs a frame, not a float."""
+    return max(0, round(seconds * fps))
+
+
</file context>

[cubic-dev-ai]

P2: When the disk review is newer but has fewer or the same number of notes, the count-only conflict rule keeps stale localStorage data and can resurrect deletions or overwrite edits. Compare an explicit revision/timestamp or merge notes by identity instead of choosing by count.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At helpers/review.html, line 698:

<comment>When the disk review is newer but has fewer or the same number of notes, the count-only conflict rule keeps stale localStorage data and can resurrect deletions or overwrite edits. Compare an explicit revision/timestamp or merge notes by identity instead of choosing by count.</comment>

<file context>
@@ -0,0 +1,955 @@
+        say("The folder holds " + data.notes.length + " notes and this browser " +
+            notes.length + ". Keeping whichever has more.");
+      }
+      if (data.notes.length > notes.length) {
+        notes = data.notes;
+        try { localStorage.setItem(NOTES_KEY, JSON.stringify(notes)); } catch (err) { /* private window */ }
</file context>

[cubic-dev-ai]

P2: When Save or Cancel stops an active recording, the asynchronous onstop callback can attach that abandoned recording to the next note saved without recording. Invalidate the recorder session before stopping and ignore stale onstop callbacks.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At helpers/review.html, line 527:

<comment>When Save or Cancel stops an active recording, the asynchronous `onstop` callback can attach that abandoned recording to the next note saved without recording. Invalidate the recorder session before stopping and ignore stale `onstop` callbacks.</comment>

<file context>
@@ -0,0 +1,955 @@
+  }
+
+  function closeEditor() {
+    if (recorder && recorder.state === "recording") recorder.stop();
+    draftVoice = null;
+    recState("");
</file context>

[cubic-dev-ai]

P2: When the page is opened offline, the review UI loses its Tailwind styling because the CDN resources are unavailable, violating the local/offline workflow. Vendor the CSS and fonts locally, and remove the runtime CDN dependency.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At helpers/review.html, line 8:

<comment>When the page is opened offline, the review UI loses its Tailwind styling because the CDN resources are unavailable, violating the local/offline workflow. Vendor the CSS and fonts locally, and remove the runtime CDN dependency.</comment>

<file context>
@@ -0,0 +1,955 @@
+<meta name="viewport" content="width=device-width, initial-scale=1.0">
+<title>Review</title>
+<script id="review-params" type="application/json">{}</script>
+<script src="https://cdn.tailwindcss.com"></script>
+<link href="https://fonts.googleapis.com/css2?family=Inter:wght@400;500;600;700;800;900&family=JetBrains+Mono:wght@400;500;600;700&display=swap" rel="stylesheet">
+<script>
</file context>

[cubic-dev-ai]

P2: A single failed voice note aborts the whole --dump run. call_scribe raises RuntimeError on any non-200 response and extract_audio raises on ffmpeg failure; neither is caught inside transcribe_pending, so the exception propagates through dump_notes/main, the dump prints a traceback, notes already transcribed in this run are never written back (the paid transcription is thrown away), and every remaining pending note stays untranscribed until the user re-runs and pays again. This is inconsistent with the function's own graceful paths for a missing file or missing API key.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At helpers/review.py, line 169:

<comment>A single failed voice note aborts the whole `--dump` run. `call_scribe` raises `RuntimeError` on any non-200 response and `extract_audio` raises on ffmpeg failure; neither is caught inside `transcribe_pending`, so the exception propagates through `dump_notes`/`main`, the dump prints a traceback, notes already transcribed in this run are never written back (the paid transcription is thrown away), and every remaining pending note stays untranscribed until the user re-runs and pays again. This is inconsistent with the function's own graceful paths for a missing file or missing API key.</comment>

<file context>
@@ -0,0 +1,273 @@
+            # The same extraction the rest of the skill uses: Scribe gets 16 kHz
+            # mono wav whatever the browser recorded.
+            extract_audio(audio, wav)
+            payload = call_scribe(wav, api_key)
+        note["voice_text"] = (payload.get("text") or "").strip()
+        done += 1
</file context>