Add horizontal (16:9) and square clip formats#36
Conversation
Introduce a FormatSpec single source of truth (backend/services/formats.py) and thread an output format through the render pipeline so a detected moment can render vertical (9:16), horizontal (16:9), or square (1:1). Everything defaults to vertical, so existing behavior is unchanged. - Parameterize crop_to_vertical with target_dims; duration constants become aliases of the vertical FormatSpec (kept as module names for importers). - Fork the render on spec.reframe: reframe formats keep face-tracked cropping; horizontal uses a new fit_to_frame scale+pad letterbox that skips face analysis. Duration cap uses spec.dur_max; web HTTP caps are format-keyed. - Thread format across the MCP tools, web server, CLI (--format), and clip history (dedup treats a missing format as vertical, so a horizontal re-clip of a vertical range is not a false duplicate). Captions still use vertical geometry on horizontal clips; per-format caption profiles are a follow-up.
📝 WalkthroughWalkthroughThis PR introduces multi-format clip output support (vertical, horizontal, square) across the stack. A new ChangesMulti-format clip rendering
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Server as server.ts / web-server.ts
participant History as ClipsHistory
participant Executor as Python create_clip
participant Formats as get_format/FORMATS
Client->>Server: create_clip(format)
Server->>History: findDuplicate(..., format)
History-->>Server: duplicate check result
Server->>Executor: execute create_clip(format)
Executor->>Formats: get_format(format)
Formats-->>Executor: FormatSpec(dims, dur_max, reframe)
Executor-->>Server: clip result(format)
Server->>History: record(entry with format)
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related PRs
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/cli.py (1)
77-86: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDon't include
formatin the selection signature.
formatis a render-time-only setting — it doesn't change which clips get selected, per the PR's "moment remains format-neutral" design. Including it here busts the cached-suggestions session whenever a user re-runs with a different--format, forcing an unnecessary (and possibly nondeterministic) re-suggestion pass, which directly undermines the "reuse the same detected moments across formats" goal this PR is built around.🐛 Proposed fix
return "|".join(str(x) for x in ( bool(config.get("ai_select", True)), config.get("min_clip_duration", MIN_CLIP_DURATION), config.get("max_clip_duration", MAX_CLIP_DURATION), - config.get("format", "vertical"), ))🤖 Prompt for 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. In `@backend/cli.py` around lines 77 - 86, The selection cache key in _selection_signature should not include the render-only format value, since it does not affect which clips are selected. Update the signature to depend only on selection-affecting inputs in _selection_signature (ai_select, min_clip_duration, max_clip_duration), so rerunning with a different --format reuses the same detected moments instead of forcing a new suggestion pass.
🧹 Nitpick comments (3)
plans/horizontal-clips.md (1)
7-13: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a language tag to the fenced block.
The bare fence triggers markdownlint MD040.
textis enough here and keeps the doc lint-clean.Suggested tweak
-``` +```text🤖 Prompt for 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. In `@plans/horizontal-clips.md` around lines 7 - 13, The fenced markdown block is missing a language tag, which triggers markdownlint MD040. Update the fenced snippet in the horizontal clips plan to use a text fence, keeping the existing content unchanged so the documentation stays lint-clean.Source: Linters/SAST tools
backend/cli.py (1)
3282-3282: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive
--formatchoices from theFORMATSregistry instead of hardcoding.The PR's stated intent is a single source of truth for format definitions (
backend/services/formats.py). Hardcoding the choices list here means it can silently drift if a format is added/removed from the registry later.♻️ Proposed refactor
+ from services.formats import FORMATS + ... - proc.add_argument("--format", choices=["vertical", "horizontal", "square"], help="Output aspect ratio (default: vertical)") + proc.add_argument("--format", choices=list(FORMATS.keys()), help="Output aspect ratio (default: vertical)")🤖 Prompt for 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. In `@backend/cli.py` at line 3282, Update the argument definition in cli argument setup so `--format` no longer hardcodes the allowed values; instead, derive the `choices` from the `FORMATS` registry in backend/services/formats.py. Use the same source of truth referenced by the format-related codepath so the CLI stays in sync if formats are added or removed, and locate the change near `proc.add_argument` for `--format`.src/ui/web-server.ts (1)
673-676: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated hardcoded duration caps (180/300) across two endpoints.
The
format === "horizontal" ? 300 : 180ternary is repeated verbatim for single-clip and batch validation. Extract into a shared constant to avoid drift, and please confirm these values match the format-aware duration limits defined inbackend/services/formats.py(single source of truth per PR objective).Suggested refactor
+const FORMAT_MAX_DURATION: Record<string, number> = { + horizontal: 300, + vertical: 180, + square: 180, +}; + // ...single-clip check: - const maxDur = format === "horizontal" ? 300 : 180; + const maxDur = FORMAT_MAX_DURATION[format] ?? 180; // ...batch check: - const maxDur = c.format === "horizontal" ? 300 : 180; + const maxDur = FORMAT_MAX_DURATION[c.format] ?? 180;Also applies to: 825-828
🤖 Prompt for 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. In `@src/ui/web-server.ts` around lines 673 - 676, The duration cap logic is duplicated in the single-clip and batch validation paths, so extract the `format === "horizontal" ? 300 : 180` rule into a shared constant or helper used by both checks in `web-server.ts`. Update the validation in the relevant handler(s) to reference that shared source, and verify the values stay aligned with the format-aware limits defined in `backend/services/formats.py` so there is one authoritative source for these caps.
🤖 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 `@backend/presets.py`:
- Around line 20-27: The clip suggestion flow is still using vertical-only
duration aliases, so horizontal and square formats can be clamped to the wrong
bounds. Update `handle_suggest_clips()` and `backend/services/claude_suggest.py`
to accept and use the selected format from `FORMATS` instead of importing
`MIN_CLIP_DURATION`, `MAX_CLIP_DURATION`, `TARGET_CLIP_DURATION_MIN`, and
`TARGET_CLIP_DURATION_MAX` directly. Thread the chosen format through both the
prompt limit construction and the runtime filtering logic so duration bounds
always come from the active format.
In `@backend/services/formats.py`:
- Around line 66-67: The get_format helper is silently falling back to
DEFAULT_FORMAT for unknown format strings, which masks typos like unsupported
values. Update get_format in formats.py to explicitly validate the requested
name against FORMATS and raise an error for any non-null, unknown format instead
of using FORMATS.get(..., FORMATS[DEFAULT_FORMAT]). Keep the default only when
name is None, and make sure callers through get_format receive a clear failure
for invalid input.
In `@src/handlers/batch-clips.handler.ts`:
- Line 156: The batch clip payload is not forwarding a top-level fallback
format, so explicit clip batches can default to the backend’s vertical value
instead of the session/user format. Update the batch-clips flow around the
input.format || settings.format || "vertical" handling so both the
/api/batch-clips request and the sync executor include a top-level format
fallback when input.clips is present, ensuring clips without their own format
inherit the intended value.
In `@src/models/index.ts`:
- Line 74: The new Format union is defined in src/models/index.ts but the
related format fields still use string, so update every format?: property in
ClipResult, UIState.settings, CreateClipInput, BatchClipSpec, BatchClipsInput,
BatchClipsResult.results[], and ClipHistoryEntry to use Format instead. Keep the
change consistent across the model types in this file so all callers that thread
format through the CLI, server, and history layers get the same type safety.
In `@src/ui/web-server.ts`:
- Around line 2623-2630: The /api/ui-state handler in uiState.settings
assignment is persisting settings.format directly without validation, unlike
/api/create-clip. Update the settings update path to validate
body.settings.format against the same allowed values before writing it into
uiState.settings.format, and reject or ignore invalid values so only supported
formats can later be used by /api/mcp/export and the batch-clips flow.
- Around line 825-828: The batch clip path in web-server.ts is missing the same
format validation that /api/create-clip already performs, so invalid c.format
values can slip through and be forwarded to the render pipeline. Add an explicit
enum/allowed-value check in the /api/batch-clips per-clip validation flow before
using c.format in the maxDur logic, and return a clear 400 error for bad values;
reuse the existing create-clip format validation behavior/pattern so both
endpoints enforce the same rules.
---
Outside diff comments:
In `@backend/cli.py`:
- Around line 77-86: The selection cache key in _selection_signature should not
include the render-only format value, since it does not affect which clips are
selected. Update the signature to depend only on selection-affecting inputs in
_selection_signature (ai_select, min_clip_duration, max_clip_duration), so
rerunning with a different --format reuses the same detected moments instead of
forcing a new suggestion pass.
---
Nitpick comments:
In `@backend/cli.py`:
- Line 3282: Update the argument definition in cli argument setup so `--format`
no longer hardcodes the allowed values; instead, derive the `choices` from the
`FORMATS` registry in backend/services/formats.py. Use the same source of truth
referenced by the format-related codepath so the CLI stays in sync if formats
are added or removed, and locate the change near `proc.add_argument` for
`--format`.
In `@plans/horizontal-clips.md`:
- Around line 7-13: The fenced markdown block is missing a language tag, which
triggers markdownlint MD040. Update the fenced snippet in the horizontal clips
plan to use a text fence, keeping the existing content unchanged so the
documentation stays lint-clean.
In `@src/ui/web-server.ts`:
- Around line 673-676: The duration cap logic is duplicated in the single-clip
and batch validation paths, so extract the `format === "horizontal" ? 300 : 180`
rule into a shared constant or helper used by both checks in `web-server.ts`.
Update the validation in the relevant handler(s) to reference that shared
source, and verify the values stay aligned with the format-aware limits defined
in `backend/services/formats.py` so there is one authoritative source for these
caps.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: b1a135ed-d53c-4895-9c50-3149e0ecd587
📒 Files selected for processing (13)
backend/cli.pybackend/main.pybackend/presets.pybackend/services/clip_generator.pybackend/services/formats.pybackend/services/video_processor.pyplans/horizontal-clips.mdsrc/handlers/batch-clips.handler.tssrc/handlers/create-clip.handler.tssrc/models/index.tssrc/server.tssrc/services/clips-history.tssrc/ui/web-server.ts
| # Back-compat aliases for the vertical format's durations. Source of truth is | ||
| # FORMATS["vertical"]; kept as module-level names because cli.py, | ||
| # clip_generator.py and claude_suggest.py import them directly. | ||
| _VERTICAL = FORMATS["vertical"] | ||
| MIN_CLIP_DURATION = _VERTICAL.dur_min | ||
| MAX_CLIP_DURATION = _VERTICAL.dur_max | ||
| TARGET_CLIP_DURATION_MIN = _VERTICAL.target_min | ||
| TARGET_CLIP_DURATION_MAX = _VERTICAL.target_max |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether claude_suggest.py uses format-aware bounds or the fixed vertical aliases
fd claude_suggest.py backend
rg -n 'MIN_CLIP_DURATION|MAX_CLIP_DURATION|TARGET_CLIP_DURATION|get_format|FORMATS' backend/services/claude_suggest.py 2>/dev/null backend/claude_suggest.py 2>/dev/nullRepository: nmbrthirteen/podcli
Length of output: 1598
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,120p' backend/presets.py
printf '\n--- claude_suggest imports and duration logic ---\n'
sed -n '1,120p' backend/services/claude_suggest.py
printf '\n--- format-aware searches ---\n'
rg -n 'get_format\(|format\b|MIN_CLIP_DURATION|MAX_CLIP_DURATION|TARGET_CLIP_DURATION' backend/services/claude_suggest.py backend | sed -n '1,220p'Repository: nmbrthirteen/podcli
Length of output: 19096
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- claude_suggest function signatures ---'
rg -n '^def |format\s*[:=]' backend/services/claude_suggest.py | sed -n '1,120p'
printf '\n%s\n' '--- surrounding caller/context in cli.py ---'
sed -n '760,860p' backend/cli.py
printf '\n%s\n' '--- more callers if any ---'
rg -n 'claude_suggest|suggest.*clip|ai_select|more_suggestions_multiplier' backend/cli.py backend/main.py backend/services | sed -n '1,220p'Repository: nmbrthirteen/podcli
Length of output: 10124
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '390,760p' backend/services/claude_suggest.py
printf '\n--- main.py suggest_clips handler ---\n'
sed -n '340,430p' backend/main.pyRepository: nmbrthirteen/podcli
Length of output: 17619
Thread clip-suggestion bounds through the selected format. backend/services/claude_suggest.py still imports these vertical-only aliases and uses them for both prompt limits and runtime filtering, while handle_suggest_clips() passes no format. Horizontal/square runs can still be clamped to vertical duration caps.
🤖 Prompt for 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.
In `@backend/presets.py` around lines 20 - 27, The clip suggestion flow is still
using vertical-only duration aliases, so horizontal and square formats can be
clamped to the wrong bounds. Update `handle_suggest_clips()` and
`backend/services/claude_suggest.py` to accept and use the selected format from
`FORMATS` instead of importing `MIN_CLIP_DURATION`, `MAX_CLIP_DURATION`,
`TARGET_CLIP_DURATION_MIN`, and `TARGET_CLIP_DURATION_MAX` directly. Thread the
chosen format through both the prompt limit construction and the runtime
filtering logic so duration bounds always come from the active format.
| def get_format(name: str | None) -> FormatSpec: | ||
| return FORMATS.get(name or DEFAULT_FORMAT, FORMATS[DEFAULT_FORMAT]) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Invalid format strings silently fall back to vertical instead of erroring.
get_format uses FORMATS.get(name or DEFAULT_FORMAT, FORMATS[DEFAULT_FORMAT]), so a typo or unsupported format value (e.g., "horzontal") silently resolves to vertical rather than surfacing an error. Since this is the single source of truth consumed by the CLI, MCP handlers, and web server (per PR objectives), a mistyped/unsupported format would render silently wrong output with no diagnostic.
🛡️ Proposed fix to raise on unknown format
def get_format(name: str | None) -> FormatSpec:
- return FORMATS.get(name or DEFAULT_FORMAT, FORMATS[DEFAULT_FORMAT])
+ key = name or DEFAULT_FORMAT
+ try:
+ return FORMATS[key]
+ except KeyError:
+ raise ValueError(f"Unknown format '{name}'. Valid formats: {', '.join(FORMATS)}")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def get_format(name: str | None) -> FormatSpec: | |
| return FORMATS.get(name or DEFAULT_FORMAT, FORMATS[DEFAULT_FORMAT]) | |
| def get_format(name: str | None) -> FormatSpec: | |
| key = name or DEFAULT_FORMAT | |
| try: | |
| return FORMATS[key] | |
| except KeyError: | |
| raise ValueError(f"Unknown format '{name}'. Valid formats: {', '.join(FORMATS)}") |
🤖 Prompt for 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.
In `@backend/services/formats.py` around lines 66 - 67, The get_format helper is
silently falling back to DEFAULT_FORMAT for unknown format strings, which masks
typos like unsupported values. Update get_format in formats.py to explicitly
validate the requested name against FORMATS and raise an error for any non-null,
unknown format instead of using FORMATS.get(..., FORMATS[DEFAULT_FORMAT]). Keep
the default only when name is None, and make sure callers through get_format
receive a clear failure for invalid input.
| title: s.title || `clip_${num}`, | ||
| caption_style: s.suggested_caption_style || settings.captionStyle || "hormozi", | ||
| crop_strategy: settings.cropStrategy || "speaker", | ||
| format: input.format || settings.format || "vertical", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C3 "format" src/ui/web-server.ts | rg -n -A3 -B3 "batch-clips"Repository: nmbrthirteen/podcli
Length of output: 157
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the handler and backend files first.
git ls-files src/handlers/batch-clips.handler.ts backend/main.py src/ui/web-server.ts
# Show the relevant sections with line numbers.
sed -n '130,280p' src/handlers/batch-clips.handler.ts
printf '\n--- backend/main.py ---\n'
sed -n '1,260p' backend/main.py
printf '\n--- src/ui/web-server.ts ---\n'
sed -n '1,260p' src/ui/web-server.tsRepository: nmbrthirteen/podcli
Length of output: 23522
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the batch-clips route and inspect the request body it forwards.
rg -n "batch-clips|batch_clips|format" src/ui/web-server.ts
printf '\n--- relevant section ---\n'
sed -n '780,940p' src/ui/web-server.tsRepository: nmbrthirteen/podcli
Length of output: 5724
Forward format for explicit clip batches
input.clips currently bypasses the input.format || settings.format || "vertical" default, and neither /api/batch-clips nor the sync executor call forwards a top-level format. That makes clips without their own format fall back to "vertical" in the backend, even when the session/user format is different.
🐛 Proposed fix
} else if (input.clips) {
- clips = input.clips;
+ clips = input.clips.map((c) => ({
+ ...c,
+ format: c.format || input.format || settings.format || "vertical",
+ }));
} else {Also pass a top-level fallback in both backend calls:
const result = await executor.execute<BatchClipsResult>("batch_clips", {
video_path: videoPath,
clips,
transcript_words: transcriptWords,
+ format: input.format || settings.format || "vertical",
clean_fillers: input.clean_fillers !== false,🤖 Prompt for 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.
In `@src/handlers/batch-clips.handler.ts` at line 156, The batch clip payload is
not forwarding a top-level fallback format, so explicit clip batches can default
to the backend’s vertical value instead of the session/user format. Update the
batch-clips flow around the input.format || settings.format || "vertical"
handling so both the /api/batch-clips request and the sync executor include a
top-level format fallback when input.clips is present, ensuring clips without
their own format inherit the intended value.
|
|
||
| export type CaptionStyle = "branded" | "hormozi" | "karaoke" | "subtle"; | ||
| export type CropStrategy = "center" | "face" | "speaker"; | ||
| export type Format = "vertical" | "horizontal" | "square"; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use the new Format type instead of string for all format fields.
The Format union is defined but never applied — every format?: field in this file (ClipResult, UIState.settings, CreateClipInput, BatchClipSpec, BatchClipsInput, BatchClipsResult.results[], ClipHistoryEntry) still uses string. This loses the type-safety benefit across the whole stack (CLI handlers, server, clip history) that this PR threads format through.
♻️ Proposed refactor
- format?: string;
+ format?: Format;Apply consistently to each of the fields listed above.
Also applies to: 90-90, 121-121, 137-137, 152-152, 165-165, 188-188, 237-237
🤖 Prompt for 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.
In `@src/models/index.ts` at line 74, The new Format union is defined in
src/models/index.ts but the related format fields still use string, so update
every format?: property in ClipResult, UIState.settings, CreateClipInput,
BatchClipSpec, BatchClipsInput, BatchClipsResult.results[], and ClipHistoryEntry
to use Format instead. Keep the change consistent across the model types in this
file so all callers that thread format through the CLI, server, and history
layers get the same type safety.
| const maxDur = c.format === "horizontal" ? 300 : 180; | ||
| if (dur > maxDur) { | ||
| res.status(400).json({ | ||
| error: `Clip ${i + 1}: too long (${Math.round(dur)}s). Max 180s.`, | ||
| error: `Clip ${i + 1}: too long (${Math.round(dur)}s). Max ${maxDur}s.`, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
/api/batch-clips doesn't validate c.format against the allowed enum.
/api/create-clip explicitly rejects invalid format values (lines 702-708), but the batch endpoint's per-clip loop only uses c.format for a duration-cap comparison — an invalid/typo'd value silently falls into the default 180s bucket and is forwarded to the render pipeline instead of failing fast with a clear error.
Suggested fix
+ const validFormats = ["vertical", "horizontal", "square"];
+ if (c.format && !validFormats.includes(c.format)) {
+ res.status(400).json({
+ error: `Clip ${i + 1}: invalid format. Use: ${validFormats.join(", ")}`,
+ });
+ return;
+ }
const maxDur = c.format === "horizontal" ? 300 : 180;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const maxDur = c.format === "horizontal" ? 300 : 180; | |
| if (dur > maxDur) { | |
| res.status(400).json({ | |
| error: `Clip ${i + 1}: too long (${Math.round(dur)}s). Max 180s.`, | |
| error: `Clip ${i + 1}: too long (${Math.round(dur)}s). Max ${maxDur}s.`, | |
| const validFormats = ["vertical", "horizontal", "square"]; | |
| if (c.format && !validFormats.includes(c.format)) { | |
| res.status(400).json({ | |
| error: `Clip ${i + 1}: invalid format. Use: ${validFormats.join(", ")}`, | |
| }); | |
| return; | |
| } | |
| const maxDur = c.format === "horizontal" ? 300 : 180; | |
| if (dur > maxDur) { | |
| res.status(400).json({ | |
| error: `Clip ${i + 1}: too long (${Math.round(dur)}s). Max ${maxDur}s.`, |
🤖 Prompt for 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.
In `@src/ui/web-server.ts` around lines 825 - 828, The batch clip path in
web-server.ts is missing the same format validation that /api/create-clip
already performs, so invalid c.format values can slip through and be forwarded
to the render pipeline. Add an explicit enum/allowed-value check in the
/api/batch-clips per-clip validation flow before using c.format in the maxDur
logic, and return a clear 400 error for bad values; reuse the existing
create-clip format validation behavior/pattern so both endpoints enforce the
same rules.
| uiState.settings.captionStyle = body.settings.captionStyle; | ||
| if (body.settings.cropStrategy !== undefined) | ||
| uiState.settings.cropStrategy = body.settings.cropStrategy; | ||
| if (body.settings.format !== undefined) | ||
| uiState.settings.format = body.settings.format; | ||
| if (body.settings.logoPath !== undefined) | ||
| uiState.settings.logoPath = body.settings.logoPath; | ||
| if (body.settings.outroPath !== undefined) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
/api/ui-state persists settings.format without validating it.
Unlike /api/create-clip, an arbitrary string can be written into persisted settings.format here, which later becomes the fallback default in /api/mcp/export and (per the sibling comment) the unvalidated /api/batch-clips.
Suggested fix
- if (body.settings.format !== undefined)
+ if (
+ body.settings.format !== undefined &&
+ ["vertical", "horizontal", "square"].includes(body.settings.format)
+ )
uiState.settings.format = body.settings.format;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| uiState.settings.captionStyle = body.settings.captionStyle; | |
| if (body.settings.cropStrategy !== undefined) | |
| uiState.settings.cropStrategy = body.settings.cropStrategy; | |
| if (body.settings.format !== undefined) | |
| uiState.settings.format = body.settings.format; | |
| if (body.settings.logoPath !== undefined) | |
| uiState.settings.logoPath = body.settings.logoPath; | |
| if (body.settings.outroPath !== undefined) | |
| uiState.settings.captionStyle = body.settings.captionStyle; | |
| if (body.settings.cropStrategy !== undefined) | |
| uiState.settings.cropStrategy = body.settings.cropStrategy; | |
| if (body.settings.format !== undefined) | |
| uiState.settings.format = body.settings.format; | |
| if (body.settings.logoPath !== undefined) | |
| uiState.settings.logoPath = body.settings.logoPath; | |
| if (body.settings.outroPath !== undefined) |
🤖 Prompt for 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.
In `@src/ui/web-server.ts` around lines 2623 - 2630, The /api/ui-state handler in
uiState.settings assignment is persisting settings.format directly without
validation, unlike /api/create-clip. Update the settings update path to validate
body.settings.format against the same allowed values before writing it into
uiState.settings.format, and reject or ignore invalid values so only supported
formats can later be used by /api/mcp/export and the batch-clips flow.
1 participant
What
Makes podcli render a detected moment to more than one aspect ratio. Adds horizontal (16:9) and square (1:1) output alongside the existing vertical (9:16), so the same moment can become a Shorts clip and a YouTube in-feed / X / LinkedIn clip.
Format is a render-time property: a moment stays format-neutral; you pick the format at render. Everything defaults to
vertical, so existing behavior is byte-identical until a caller opts into another format.How
backend/services/formats.py(FormatSpec+FORMATS) replaces scattered1080x1920hardcodes. Duration constants become aliases of the vertical spec (kept as module-level names so existing importers don't break).generate_clipresolvesspec = get_format(format)once and forks onspec.reframe: reframe formats (vertical, square) keep the face-tracked crop; horizontal uses a newfit_to_framescale+pad letterbox that skips face analysis entirely.spec.dur_max; the web HTTP caps are format-keyed (horizontal→300s, others keep 180s).formatflows through the MCP tools (server.ts), the web server (incl. thestyledClipsexport whitelist), the CLI (--format), and clip history. Dedup treats a missing format as vertical, so a horizontal re-clip of an already-vertical range is not a false duplicate.Verification
horizontal→ 1920x1080 (viafit-letterbox, no face analysis) andvertical→ 1080x1920 (unchanged).tsc --noEmitclean.Deferred (follow-up)
Captions still use vertical geometry on horizontal clips (the caption components use absolute pixels tuned for 1080-tall, not relative sizing). Per-format caption profiles (
lower_third) across Remotion + the ASS fallback are the next phase. The video is dimension-correct; only caption placement is not yet format-aware.Full plan:
plans/horizontal-clips.md.Summary by CodeRabbit
New Features
Bug Fixes