docs(skills): define cinematic landing capture#1768
Conversation
|
Claude finished @zeval's task in 2m 41s —— View job Review
FindingsSuggestion (recommended, doesn't block)
Summary
Verdict: Ready with suggestions The three-file change is internally consistent — |
📝 WalkthroughWalkthroughUpdates product-video capture contracts and QA guidance for editorial landing films, including focused loops, delivery sizing, theater-width readability, motion review, browser-artifact rejection, and deterministic crop matching. It also adds a truthfulness constraint for capture-only mock executor labels. ChangesProduct video capture
Product demo seeding
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
| Filename | Overview |
|---|---|
| .agents/skills/product-demo-seeding/SKILL.md | Adds one rule requiring logical labels (e.g. mock-agent) for capture-only executors rather than exposing temporary host paths; straightforward and consistent with existing guidelines. |
| .agents/skills/product-video-capture/SKILL.md | Step 10 gains the theater-width review requirement for landing films; loop-frame bullet tightened to require three-frame match (first, settled penultimate, final) for both editorial landing and docs clips — consistent with updated reference docs. |
| .agents/skills/product-video-capture/references/camera-encoding.md | Core of the PR: adds conditional editorial landing profile, minimal JSONC config example, landing composition guidelines, narrative camera reject criteria, and extends loopFrame focused eligibility to landing — all internally consistent with the companion guard that requires test recognition before use. |
| .agents/skills/product-video-capture/references/capture-pipeline.md | Adds --test-type Chrome flag and a frame-complete perimeter inspection requirement for the automation banner; rejection conditions and raw-rejection list updated consistently. |
| .agents/skills/product-video-capture/references/qa-checklist.md | Timeline audit, content audit, and promotion checklist all updated to match the new three-frame loop requirement, theater-width gate, and banner inspection rules; formFactor: landing preflight added at the correct preflight gate. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Start: landing film requested] --> B{Check out landing repo}
B --> C{Run focused encoder tests\nformFactor landing recognised?}
C -- No --> D[STOP: older checkout\nlacks landing profile]
C -- Yes --> E[Capture: 3840x2400 raw\n--test-type --disable-infobars]
E --> F[Decode every raw frame\nFrame-indexed contact sheets]
F --> G{Any automation banner\nin perimeter?}
G -- Yes --> H[Reject take & recapture]
G -- No --> I[Design camera at 2x\nmatched 3-frame loop]
I --> J{First = settled penultimate\n= final keyframe?}
J -- No --> K[Revise camera keyframes]
J -- Yes --> L[Encode: 1920x1200\nVP9 CRF 26 / H.264 CRF 18]
L --> M[Theater-width audit\n760px, 850px, 950px]
M --> N{Text & controls readable\nat all three widths?}
N -- No --> O[Reject delivery]
N -- Yes --> P[Promote to production\nWebM, MP4, WebP]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[Start: landing film requested] --> B{Check out landing repo}
B --> C{Run focused encoder tests\nformFactor landing recognised?}
C -- No --> D[STOP: older checkout\nlacks landing profile]
C -- Yes --> E[Capture: 3840x2400 raw\n--test-type --disable-infobars]
E --> F[Decode every raw frame\nFrame-indexed contact sheets]
F --> G{Any automation banner\nin perimeter?}
G -- Yes --> H[Reject take & recapture]
G -- No --> I[Design camera at 2x\nmatched 3-frame loop]
I --> J{First = settled penultimate\n= final keyframe?}
J -- No --> K[Revise camera keyframes]
J -- Yes --> L[Encode: 1920x1200\nVP9 CRF 26 / H.264 CRF 18]
L --> M[Theater-width audit\n760px, 850px, 950px]
M --> N{Text & controls readable\nat all three widths?}
N -- No --> O[Reject delivery]
N -- Yes --> P[Promote to production\nWebM, MP4, WebP]
Reviews (7): Last reviewed commit: "docs(skills): require frame-complete art..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.agents/skills/product-video-capture/references/camera-encoding.md:
- Line 38: Update the focused-start eligibility guidance in the camera-encoding
reference to match the shared acceptance contract: allow focused framing when it
improves readability or removes irrelevant chrome or fixture-only detail, while
preserving identifying context. Align the corresponding condition across
SKILL.md and the QA checklist so all three references use the same eligibility
rule.
In @.agents/skills/product-video-capture/references/qa-checklist.md:
- Around line 49-51: Update the focused-loop checklist item for camera bounds
and loop frames to explicitly verify that the first, settled penultimate, and
final camera frames match exactly, consistent with the camera contract in
camera-encoding.md. Preserve the existing centered 1x requirement for default
clips and the calm reset behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8093a98a-7e6f-4a62-b491-0657f53e9e8b
📒 Files selected for processing (3)
.agents/skills/product-video-capture/SKILL.md.agents/skills/product-video-capture/references/camera-encoding.md.agents/skills/product-video-capture/references/qa-checklist.md
|
Claude finished @zeval's task in 2m 43s —— View job Review
Prior findings addressed in this commit (bc8a5ba)
FindingsSuggestion (recommended, doesn't block)
Summary
Verdict: Ready with suggestions The three-file set is now internally consistent on the VP9/H.264 quality parameters, focused-loop eligibility, and the penultimate-frame requirement in QA and provenance. The two remaining suggestions are small precision gaps rather than blockers: a three-word consistency fix in SKILL.md and a heading clarification in camera-encoding.md. |
|
Claude finished @zeval's task in 2m 37s —— View job Claude finished @zeval's task —— Review
Prior findings addressed (all three commits)
FindingsSuggestion (recommended, doesn't block)
Summary
Verdict: Ready with suggestion The three-file set is now internally consistent across all three review rounds: VP9/H.264 quality parameters are concrete, focused-loop eligibility matches in SKILL.md / camera-encoding.md / qa-checklist.md, the penultimate-frame requirement is in both the contract and the QA/provenance checklist, and the example heading is unambiguous. The one remaining suggestion is a missing landing JSON stub — all the information is in the doc, but a single template would remove the synthesis step for agents. |
|
Claude finished @zeval's task in 2m 32s —— View job Review
Prior findings — all addressed
FindingsSuggestion (recommended, doesn't block)
Summary
Verdict: Ready with suggestion The three-file change is fully internally consistent at this head: VP9/H.264 quality parameters are concrete, focused-loop eligibility is identical across SKILL.md / camera-encoding.md / qa-checklist.md, the settled penultimate frame requirement appears in both the encoder contract and the QA/provenance checklist, the example heading is unambiguous, and both guard notes (camera-encoding.md and qa-checklist.md §5) warn against encoding before the companion landing encoder PR lands. The one remaining suggestion — a minimal landing JSON stub — has now been raised across three rounds; the information exists but requires cross-template synthesis that a stub would eliminate. |
|
Claude finished @zeval's task in 2m 19s —— View job Review
Prior findings — all resolved at head
|
| Finding | Round | Status |
|---|---|---|
| "higher text-preserving encode quality" undefined | 1 | ✅ Now VP9 CRF 26, H.264 CRF 18 |
formFactor: "landing" without companion PR |
1 | ✅ Guard rail in camera-encoding.md + qa-checklist.md §5 |
| Focused-loop eligibility inconsistency across files | 1–2 | ✅ All three files aligned |
| QA checklist missing settled-penultimate check | 2 | ✅ §3 and §6 both require first, settled penultimate, and final |
| SKILL.md acceptance gate said "start/end crop" | 2 | ✅ Now "exactly matching first, settled penultimate, and final focused frames" |
| Example config heading ambiguous after landing prose | 2 | ✅ Now "Example general-desktop config shape" |
| No landing JSON stub | 3–4 | ✅ Minimal editorial-landing config added with correct outputWidth: 1920, outputHeight: 1200, formFactor: "landing", loopFrame: "focused", and matched keyframes |
Findings
No blockers. No remaining suggestions.
Cross-file consistency check passes on every invariant:
- "first, settled penultimate, and final" appears identically in
SKILL.md:66,camera-encoding.md:26,camera-encoding.md:53,qa-checklist.md:49, andqa-checklist.md:106 loopFrame: "focused"is restricted todocsorlandingin prose and in both JSON examples- Guard requiring companion PR test confirmation present in both
camera-encoding.mdandqa-checklist.md §5 - Landing stub dimensions (3840x2400 → 1920x1200, zoom 2) are consistent with the encoder contract
Summary
| Severity | Count |
|---|---|
| Blocker | 0 |
| Suggestion | 0 |
Verdict: Ready to merge
The three-file change is complete and internally consistent after five commits. All substantive findings from four review rounds have been addressed. The landing editorial profile is fully specified with concrete encoder parameters, a matched-loop contract, a complete JSON stub, theater-width readability requirements, and guard notes preventing premature use before the companion encoder PR lands.
|
Claude finished @zeval's task in 1m 34s —— View job Review
New commit since last review —
|
| File | Change |
|---|---|
product-demo-seeding/SKILL.md |
+1 bullet: mock executor command labels must not expose absolute host paths |
capture-pipeline.md |
Added --test-type Chrome flag; added automation-banner inspection note; added "automation banner" to raw rejection criteria |
qa-checklist.md |
Added "browser automation banner" to content audit list |
Technical correctness: --test-type is the right flag to suppress Chrome's automation infobar when --disable-infobars alone isn't sufficient on all Chrome for Testing builds. The inspection note at capture-pipeline.md:40 explains why both flags are listed together.
Cross-file consistency: "automation banner" now appears consistently in the capture-pipeline rejection criteria, the QA checklist content audit, and the browser shape inspection step — all aligned.
Scope: The new mock executor label rule in product-demo-seeding/SKILL.md fits the "keep host identity out of visible frames" pattern already present in that file's Truthfulness Contract section.
Prior findings — all resolved at 1a68e47 (confirmed unchanged)
| Finding | Status |
|---|---|
| "higher text-preserving encode quality" undefined | ✅ VP9 CRF 26, H.264 CRF 18 |
formFactor: "landing" forward ref without companion PR |
✅ Guard note in camera-encoding.md + qa-checklist.md §5 |
| Focused-loop eligibility inconsistency | ✅ All three files aligned |
| QA checklist missing settled-penultimate check | ✅ §3 and §6 both require first, settled penultimate, and final |
| SKILL.md acceptance gate said "start/end crop" | ✅ Now "exactly matching first, settled penultimate, and final focused frames" |
| Example config heading ambiguous | ✅ Now "Example general-desktop config shape" |
| No landing JSON stub | ✅ Minimal editorial-landing config added |
Findings
No blockers. No new suggestions.
Summary
| Severity | Count |
|---|---|
| Blocker | 0 |
| Suggestion | 0 |
Verdict: Ready to merge
The three-file set is fully internally consistent after six commits. The new commit adds focused capture-hygiene rules (automation banner detection, mock executor path suppression) that fit cleanly within the existing guidance structure.
|
Claude finished @zeval's task in 1m 34s —— View job Review
New commit since last review —
|
| Before | After | |
|---|---|---|
| Contract bullet | editorial landing delivery 1920x1200... (unconditional) |
when the checked-out landing tests recognize \formFactor: "landing"`, editorial delivery 1920x1200...` (conditional) |
This converts the landing contract bullet from an unconditional present-tense claim to a test-gated conditional. It is the last precision fix requested across the review thread and directly resolves the greptile observation that the bullet "reads as a present fact while the implementing companion PR is not yet merged."
Cross-file consistency at 0697945:
| Invariant | Status |
|---|---|
formFactor: "landing" guard in contract bullet |
✅ Now conditional on test recognition |
formFactor: "landing" preflight note in camera-encoding.md |
✅ Unchanged — already present |
formFactor: "landing" guard in qa-checklist.md §5 |
✅ Unchanged — already present |
| "first, settled penultimate, and final" consistent across all five references | ✅ |
| VP9 CRF 26 / H.264 CRF 18 concrete in contract | ✅ |
| Landing config stub with correct 3840x2400 → 1920x1200 / zoom 2 | ✅ |
| Theater-width audit at 760/850/950 CSS px in SKILL.md and QA | ✅ |
| Automation banner rejection in both capture-pipeline and QA content audit | ✅ |
| Mock executor label rule in product-demo-seeding | ✅ |
Prior findings — all resolved
| Finding | Round | Status |
|---|---|---|
| "higher text-preserving encode quality" undefined | 1 | ✅ VP9 CRF 26, H.264 CRF 18 |
formFactor: "landing" stated as present fact before companion PR |
1 / Greptile | ✅ Now conditional on test recognition |
| Focused-loop eligibility inconsistency across files | 1–2 / CodeRabbit | ✅ All files aligned |
| QA checklist missing settled-penultimate check | 2 / CodeRabbit | ✅ §3 and §6 both require first, settled penultimate, and final |
| SKILL.md acceptance gate said "start/end crop" | 2 | ✅ Now "exactly matching first, settled penultimate, and final focused frames" |
| Example config heading ambiguous after landing prose | 2 | ✅ Now "Example general-desktop config shape" |
| No landing JSON stub | 3–4 | ✅ Minimal editorial-landing config added |
Findings
No blockers. No new suggestions.
Summary
| Severity | Count |
|---|---|
| Blocker | 0 |
| Suggestion | 0 |
Verdict: Ready to merge
Seven commits have fully resolved every finding from the review thread. The three-file set (plus capture-pipeline.md and product-demo-seeding/SKILL.md) is internally consistent on the landing encoder contract, focused-loop eligibility, theater-width readability gates, automation banner detection, and mock executor label hygiene.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.agents/skills/product-video-capture/references/camera-encoding.md:
- Line 40: Update the focused-frame wording in the guidance sentence to use “a
single focused frame matched across the loop,” while preserving the surrounding
conditions and meaning.
In @.agents/skills/product-video-capture/references/capture-pipeline.md:
- Line 40: The browser-artifact validation must guarantee frame-complete
coverage rather than relying on first, middle, and final samples. Update
.agents/skills/product-video-capture/references/capture-pipeline.md at line 40
and .agents/skills/product-video-capture/references/qa-checklist.md at line 65
to require scanning every frame, or document a sampling cadence that guarantees
coverage, and reject any take containing a browser-managed banner.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9b600db8-eb7b-4aa1-bbab-220f12cd9cc0
📒 Files selected for processing (5)
.agents/skills/product-demo-seeding/SKILL.md.agents/skills/product-video-capture/SKILL.md.agents/skills/product-video-capture/references/camera-encoding.md.agents/skills/product-video-capture/references/capture-pipeline.md.agents/skills/product-video-capture/references/qa-checklist.md
🚧 Files skipped from review as they are similar to previous changes (1)
- .agents/skills/product-video-capture/SKILL.md
|
Claude finished @zeval's task in 2m 1s —— View job Claude finished @josé Almeida's task Review
New commit since last review —
|
| File | Change |
|---|---|
camera-encoding.md:40 |
"one matched focused frame" → "a single focused frame matched across the loop" (LanguageTool grammar fix from CodeRabbit) |
capture-pipeline.md:38-40 |
Requires decoding every raw frame into frame-indexed contact sheets; explicitly warns that first/middle/final samples cannot exclude a transient automation banner |
qa-checklist.md:66 |
Adds frame-complete inspection rule: "Fixed-fraction samples and normal-speed playback are supplementary, not frame-complete evidence" |
Technical correctness: The frame-complete requirement is now consistent across both capture and QA. The wording fix matches CodeRabbit's suggestion exactly.
Prior findings — all resolved
| Finding | Round | Status |
|---|---|---|
| "higher text-preserving encode quality" undefined | 1 | ✅ VP9 CRF 26, H.264 CRF 18 |
formFactor: "landing" stated as present fact before companion PR |
1 / Greptile | ✅ Contract bullet conditional; guard notes in camera-encoding.md and qa-checklist.md §5 |
| Focused-loop eligibility inconsistency across files | 1–2 / CodeRabbit | ✅ All five files aligned |
| QA checklist missing settled-penultimate check | 2 / CodeRabbit | ✅ §3 and §6 both require first, settled penultimate, and final |
| SKILL.md acceptance gate said "start/end crop" | 2 | ✅ Now "exactly matching first, settled penultimate, and final focused frames" |
| Example config heading ambiguous after landing prose | 2 | ✅ Now "Example general-desktop config shape" |
| No landing JSON stub | 3–4 | ✅ Minimal editorial-landing config with correct 3840x2400 → 1920x1200 / zoom 2 |
| "matched focused frame" grammar | 5 / CodeRabbit | ✅ Now "a single focused frame matched across the loop" |
| Frame-complete browser-artifact validation missing | 5 / CodeRabbit | ✅ Both capture-pipeline.md and qa-checklist.md now require all-frame inspection |
Findings
No blockers. No new suggestions.
Cross-file consistency check passes on all invariants at 347f35a:
- "first, settled penultimate, and final" appears identically in
SKILL.md:65,camera-encoding.md:26,camera-encoding.md:53,qa-checklist.md:49, andqa-checklist.md:106 loopFrame: "focused"restricted todocsorlandingin prose and in both JSON examples- Landing delivery bullet is conditional on
formFactor: "landing"test recognition; guard notes appear in bothcamera-encoding.mdandqa-checklist.md §5 - Landing stub dimensions (3840x2400 → 1920x1200, zoom 2) match the encoder contract
- Theater-width audit at 760/850/950 CSS px referenced consistently in
SKILL.md,camera-encoding.md, andqa-checklist.md - Automation banner rejection is now consistent across
capture-pipeline.mdrejection criteria, the QA content audit, and the new frame-complete inspection requirement
Summary
| Severity | Count |
|---|---|
| Blocker | 0 |
| Suggestion | 0 |
Verdict: Ready to merge
Eight commits have fully resolved every finding from five review rounds. The two final CodeRabbit findings (grammar and frame-complete validation) are cleanly addressed. The five-file change is internally consistent on every invariant.
1 participant
Defines a measurable editorial camera profile and capture gate so landing films preserve the product action at their real embed size instead of showing unreadable full-workspace chrome.
Summary
Validation
python3 .github/scripts/lint-harness-files.py .agents/skills/product-demo-seeding .agents/skills/product-video-capturepython3 /home/zeval/.agents/skills/skill-creator/scripts/quick_validate.py .agents/skills/product-video-capturepython3 /home/zeval/.agents/skills/skill-creator/scripts/quick_validate.py .agents/skills/product-demo-seedingCompanion
Checklist
Preview Environment
347f35a