feat: expand Studio podcast workflow - #139
Conversation
|
Warning Review limit reached
Next review available in: 50 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis change adds configurable caption and logo layouts, full-episode caption exports, transcript utilities, and local silence removal. The backend, Remotion renderer, web server, persistence model, Studio workspace, styling, and related tests are updated. ChangesStudio rendering and silence-removal workflows
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant EpisodeWorkspace
participant web-server
participant silence_removal
participant FFmpeg
EpisodeWorkspace->>web-server: Request silence analysis
web-server->>silence_removal: Analyze video and transcript
silence_removal-->>EpisodeWorkspace: Return silence plan
EpisodeWorkspace->>web-server: Submit approved keep ranges
web-server->>silence_removal: Render compact episode
silence_removal->>FFmpeg: Render retained segments
FFmpeg-->>EpisodeWorkspace: Provide compact output and progress
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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: 14
🧹 Nitpick comments (9)
src/ui/client/EpisodeWorkspace.jsx (3)
2039-2042: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
role="region"for the transcript container.
role="document"marks an entire document context. This element is a scrollable panel inside the application. A focusable panel with an accessible name should userole="region". Thearia-labelalready supplies the name.♻️ Proposed fix
- <div className="transcript-document" role="document" tabIndex={0} + <div className="transcript-document" role="region" tabIndex={0}🤖 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/client/EpisodeWorkspace.jsx` around lines 2039 - 2042, Update the transcript container div in the EpisodeWorkspace JSX to use role="region" instead of role="document", preserving its existing tabIndex and aria-label.
477-479: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueThe preview does not clamp
captionFontScale, but the renderer does.
Root.tsxclamps the scale to0.6–1.6. Lines 479 and 553 apply the raw value. The slider is bounded to 60–160, but line 1155 accepts any numeric value from the SSEstateevent. An out-of-range value then makes the preview disagree with the exported video. Clamp the scale in one shared helper.♻️ Proposed clamp
+ const clampScale = (value) => Math.max(60, Math.min(160, Number(value) || 100));Then use
clampScale(captionFontScale)in both preview components.🤖 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/client/EpisodeWorkspace.jsx` around lines 477 - 479, Clamp captionFontScale consistently with the renderer by introducing one shared clampScale helper using the 0.6–1.6 bounds, then apply it wherever the preview components calculate caption font size, including the paths around the existing preview styles at lines 479 and 553. Ensure SSE-provided values are normalized before rendering so preview and exported video match.
132-149: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffAdd roving
tabIndexand arrow-key handling to the radiogroup.The group uses
role="radiogroup"with sixrole="radio"buttons. A native radio group exposes one tab stop and moves the selection with the arrow keys. This implementation exposes six tab stops and no arrow-key handling. Keyboard users can still select every option, so the control remains usable, but it does not match the announced pattern.🤖 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/client/EpisodeWorkspace.jsx` around lines 132 - 149, Update LogoPositionPicker so the radiogroup has one roving tab stop: assign tabIndex={0} to the selected option and tabIndex={-1} to the others, while preserving disabled behavior. Add keyboard handling for ArrowLeft/ArrowUp and ArrowRight/ArrowDown to move selection to the previous or next LOGO_POSITIONS entry, wrapping at the boundaries and preventing default scrolling.remotion/src/components/SubtleCaptions.tsx (1)
77-77: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
nowrapapplies to the first caption span only.Line 77 sets
whiteSpace: "nowrap"on the first span. The second span at lines 83–93 keeps the default. WhensingleLineis truetext2is always empty, so the difference is not visible today. The single-line text can still overflow theleft: 60 * s/right: 60 * sband. The same pattern exists in the other caption components.🤖 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 `@remotion/src/components/SubtleCaptions.tsx` at line 77, Update the caption span rendering in SubtleCaptions so the singleLine whiteSpace behavior applies to both caption spans, including the second span containing text2, rather than only the first span. Preserve the existing conditional behavior and apply the same change to the equivalent caption components that use this pattern.src/ui/web-server.ts (2)
1637-1638: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
writeFileSyncblocks the event loop for a full-episode transcript.A full-episode word list is large.
JSON.stringifypluswriteFileSyncon the request thread stalls every other HTTP request and every SSE broadcast while it runs. The handler is alreadyasync.♻️ Proposed fix
- writeFileSync(wordsPath, JSON.stringify({ words: transcript_words }), "utf-8"); + await writeFile(wordsPath, JSON.stringify({ words: transcript_words }), "utf-8");Import
writeFilefromnode:fs/promisesalongside the existingunlinkimport.🤖 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 1637 - 1638, In the async handler containing the full-episode transcript write, replace the synchronous writeFileSync call with the promise-based writeFile API and await it. Import writeFile from node:fs/promises alongside unlink, while preserving the existing wordsPath and serialized transcript content.
1299-1304: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the caption and logo position validation into shared constants.
The same two arrays and the same
Math.max(60, Math.min(160, ...))clamp now appear five times in this file: lines 212-214, 1168-1174, 1299-1304, 1613-1618, and 3841-3846. The backend repeats the clamp inbackend/services/caption_renderer.py. One divergent edit will make the API accept a value that a later layer rejects.Define
CAPTION_POSITIONS,LOGO_POSITIONS, and anormalizeCaptionFontScale()helper once, then reuse them at all five sites.🤖 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 1299 - 1304, Define shared CAPTION_POSITIONS and LOGO_POSITIONS constants plus a normalizeCaptionFontScale() helper in the web-server module, then replace the duplicated arrays and font-scale clamp at all five validation sites, including the flow around the shown caption/logo validation. Ensure each site preserves the existing accepted positions and clamps values to 60–160 with the current default behavior.backend/services/silence_removal.py (1)
254-278: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the dead mutations in the first pass of the planner.
Lines 259-260 and lines 264-265 mutate
keep_segments. Lines 269-276 then rebuildkeep_segmentsfrom scratch as the complement ofremoved_rangesand discard those mutations. The first loop is only needed to produceremoved_ranges.The current shape suggests that the mutations affect the result, which makes the cut planner harder to verify. Drop them and keep the complement rebuild as the single source of truth.
♻️ Proposed fix
removed_ranges: list[dict] = [] cursor = 0.0 for segment in keep_segments: if segment["start"] - cursor >= min_silence_seconds: removed_ranges.append({"start": cursor, "end": segment["start"]}) - elif cursor < segment["start"]: - segment["start"] = cursor cursor = segment["end"] if duration - cursor >= min_silence_seconds: removed_ranges.append({"start": cursor, "end": duration}) - elif cursor < duration: - keep_segments[-1]["end"] = duration🤖 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/silence_removal.py` around lines 254 - 278, Remove the `segment["start"] = cursor` and `keep_segments[-1]["end"] = duration` mutations from the first pass over `keep_segments`; retain that loop only for constructing `removed_ranges`. Keep the subsequent complement rebuild as the sole source of truth for the final `keep_segments` result.tests/test_silence_removal.py (1)
40-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case for words that fall entirely inside a removed range.
_map_rangereturnsNonewhen a word has no overlap with any keep segment, andremap_timed_itemsthen drops it. That path is the one most likely to corrupt caption alignment after a cut, and no test covers it.💚 Proposed test
def test_remap_transcript_drops_words_inside_cuts(): transcript = { "words": [ {"word": "kept", "start": 0.6, "end": 1.0}, {"word": "cut", "start": 2.5, "end": 3.0}, ], "segments": [], } remapped = remap_transcript( transcript, [{"start": 0.5, "end": 2.0}, {"start": 4.5, "end": 6.0}], ) assert [w["word"] for w in remapped["words"]] == ["kept"]🤖 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 `@tests/test_silence_removal.py` around lines 40 - 56, Add a test alongside test_remap_transcript_closes_removed_gaps that passes a word entirely within a removed interval to remap_transcript, then assert the resulting words retain only the kept word and drop the fully cut word. Use the existing keep segments and empty segments structure from the proposed scenario to cover the _map_range/remap_timed_items path.remotion/render.mjs (1)
154-156: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winGuard
captionFontScaleagainstNaN.
Number(opts["caption-font-scale"] || 100)returnsNaNfor any non-numeric argument.NaNthen flows into the composition props and can produce an invalid font size instead of an error. Clamp the value to the same 60–160 range thatrender_captionsuses inbackend/services/caption_renderer.py.♻️ Proposed fix
+ const rawFontScale = Number(opts["caption-font-scale"]); + const captionFontScale = Number.isFinite(rawFontScale) + ? Math.max(60, Math.min(160, rawFontScale)) + : 100; const inputProps = { videoSrc, words, styleName, logoSrc, faceY, durationInFrames, fps, captionPosition: opts["caption-position"] || "auto", - captionFontScale: Number(opts["caption-font-scale"] || 100), + captionFontScale, logoPosition: opts["logo-position"] || "top-left", };🤖 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 `@remotion/render.mjs` around lines 154 - 156, Update the captionFontScale assignment in the render options to reject non-numeric values and clamp valid values to the 60–160 range used by render_captions. Ensure NaN cannot enter the composition props, while preserving the existing default of 100 when the option is absent or invalid.
🤖 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/services/silence_removal.py`:
- Line 436: Update the final move operation near the partial-output handling to
use shutil.move instead of os.replace, allowing the completed file to move
across filesystems. Preserve the existing source partial path, output_path
destination, and cleanup behavior.
In `@remotion/render-full-episode.mjs`:
- Around line 31-41: Update the run function’s spawnSync options to enforce a
timeout for ffmpeg/ffprobe commands, and treat a timed-out result as a failure
alongside nonzero exit status. Ensure the thrown error includes the command
context and available stderr/stdout details, while preserving normal stdout
handling for successful completion.
In `@remotion/render.mjs`:
- Around line 154-156: The caption font scale parsing in render.mjs and
render-full-episode.mjs accepts NaN and values outside the backend’s supported
range. Add one shared helper for the caption font scale that parses the CLI
value, defaults non-finite values to 100, and clamps finite values to 60–160;
import and use it at remotion/render.mjs:154-156 and
remotion/render-full-episode.mjs:64-64, replacing the existing Number
expressions in both sites.
In `@remotion/src/components/BrandedCaptions.tsx`:
- Around line 156-161: Update the caption margin calculation in BrandedCaptions
so lower captions receive additional bottom offset when logoPosition starts with
"bottom-". Preserve the existing margin behavior for top logos and other caption
positions, and ensure the offset prevents the caption block from overlapping the
bottom logo.
- Around line 76-77: Update the single-line rendering in the BrandedCaptions
component to prevent caption text from exceeding the safe inset between the
fixed left/right offsets, while preserving nowrap behavior for word grouping.
Apply a rendered-width constraint or equivalent scaling/clamping for the full
captionFontScale range, using the existing caption container dimensions and
style logic.
In `@src/models/index.ts`:
- Around line 129-131: Update the shared UIState.settings interface in the model
definition to include captionPosition, captionFontScale, logoPosition, and
onboardingDismissed, matching the corresponding settings declared and persisted
by the web-server UIState. Preserve the existing silence fields and use the same
types as the server-side declaration.
In `@src/ui/client/CopyButton.tsx`:
- Line 79: Update handleCopy around copyText so failures from both clipboard
mechanisms set a visible or announced error status, while successful copies
clear any existing error status. Preserve the current copied-state behavior and
ensure the error state is cleared after a successful copy.
In `@src/ui/client/EpisodeWorkspace.jsx`:
- Around line 1286-1302: Clear stale fullEpisodeResult in all three workspace
reset paths: add setFullEpisodeResult(null) to clearEpisode, the changedVideo
branch of loadPreset, and the “Start over” handler at
src/ui/client/EpisodeWorkspace.jsx lines 1286-1302, 858-873, and 2660
respectively.
- Around line 2150-2157: Update the silence timeline rendering in the map around
silencePlan.removed_ranges to reuse a single guarded source-duration value,
consistent with the existing silencePlan.source_duration || 0 handling. Use that
guarded value for both the range.start and range.end - range.start percentage
calculations, preventing unguarded division when source_duration is missing or
zero.
- Around line 1109-1135: Align the hydration target built in the state-event
handler with the values committed by the corresponding setters so the
synchronization signature can match. Update the cropStrategy and
silenceThreshold, silenceMinPause, and silencePadding handling to use the same
presence checks and fallback values as the setter logic, preserving valid falsy
server values such as 0.
- Around line 718-721: The logo preview resolver and exported logo_path
currently disagree for unregistered filesystem paths. Update the logo handling
around resolveAssetName and logoPreviewUrl so preview generation and logo_path
use the same asset validation/resolution rule, or clear/reset non-registered
logo values before export; preserve registered asset previews and
backend-compatible values.
- Around line 1533-1547: Update the silenceRenderStream completion handler in
the useEffect to set an appropriate error when rendered output_path, transcript,
or pendingSilenceOriginalRef.current is missing, before clearing the pending
state and job ID. Preserve the existing success updates when all required values
are present.
In `@src/ui/public/css/styles.css`:
- Line 749: Update the background declaration at the affected styles.css rule to
use the configured lowercase spelling of the current-color keyword, while
leaving the adjacent text color declaration unchanged.
In `@src/ui/web-server.ts`:
- Around line 1465-1468: The three new handlers bypass the source allowlist by
validating video_path only with existsSync. In src/ui/web-server.ts at lines
1465-1468, 1521-1524, and 1601-1604, extract an assertAllowedSource(video_path)
helper using the existing allowedSourcePaths/registerSourcePath controls, and
call it before creating analysis jobs, rendering or writing manifests, and
spawning the renderer.
---
Nitpick comments:
In `@backend/services/silence_removal.py`:
- Around line 254-278: Remove the `segment["start"] = cursor` and
`keep_segments[-1]["end"] = duration` mutations from the first pass over
`keep_segments`; retain that loop only for constructing `removed_ranges`. Keep
the subsequent complement rebuild as the sole source of truth for the final
`keep_segments` result.
In `@remotion/render.mjs`:
- Around line 154-156: Update the captionFontScale assignment in the render
options to reject non-numeric values and clamp valid values to the 60–160 range
used by render_captions. Ensure NaN cannot enter the composition props, while
preserving the existing default of 100 when the option is absent or invalid.
In `@remotion/src/components/SubtleCaptions.tsx`:
- Line 77: Update the caption span rendering in SubtleCaptions so the singleLine
whiteSpace behavior applies to both caption spans, including the second span
containing text2, rather than only the first span. Preserve the existing
conditional behavior and apply the same change to the equivalent caption
components that use this pattern.
In `@src/ui/client/EpisodeWorkspace.jsx`:
- Around line 2039-2042: Update the transcript container div in the
EpisodeWorkspace JSX to use role="region" instead of role="document", preserving
its existing tabIndex and aria-label.
- Around line 477-479: Clamp captionFontScale consistently with the renderer by
introducing one shared clampScale helper using the 0.6–1.6 bounds, then apply it
wherever the preview components calculate caption font size, including the paths
around the existing preview styles at lines 479 and 553. Ensure SSE-provided
values are normalized before rendering so preview and exported video match.
- Around line 132-149: Update LogoPositionPicker so the radiogroup has one
roving tab stop: assign tabIndex={0} to the selected option and tabIndex={-1} to
the others, while preserving disabled behavior. Add keyboard handling for
ArrowLeft/ArrowUp and ArrowRight/ArrowDown to move selection to the previous or
next LOGO_POSITIONS entry, wrapping at the boundaries and preventing default
scrolling.
In `@src/ui/web-server.ts`:
- Around line 1637-1638: In the async handler containing the full-episode
transcript write, replace the synchronous writeFileSync call with the
promise-based writeFile API and await it. Import writeFile from node:fs/promises
alongside unlink, while preserving the existing wordsPath and serialized
transcript content.
- Around line 1299-1304: Define shared CAPTION_POSITIONS and LOGO_POSITIONS
constants plus a normalizeCaptionFontScale() helper in the web-server module,
then replace the duplicated arrays and font-scale clamp at all five validation
sites, including the flow around the shown caption/logo validation. Ensure each
site preserves the existing accepted positions and clamps values to 60–160 with
the current default behavior.
In `@tests/test_silence_removal.py`:
- Around line 40-56: Add a test alongside
test_remap_transcript_closes_removed_gaps that passes a word entirely within a
removed interval to remap_transcript, then assert the resulting words retain
only the kept word and drop the fully cut word. Use the existing keep segments
and empty segments structure from the proposed scenario to cover the
_map_range/remap_timed_items path.
🪄 Autofix
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 Plus
Run ID: ea0d6240-31ee-428d-99b5-63d68c5290f3
📒 Files selected for processing (30)
README.mdbackend/main.pybackend/services/caption_renderer.pybackend/services/captions_burn.pybackend/services/clip_generator.pybackend/services/silence_removal.pyremotion/render-full-episode.mjsremotion/render.mjsremotion/src/CaptionedClip.tsxremotion/src/Root.tsxremotion/src/chunks.test.tsremotion/src/chunks.tsremotion/src/components/BrandedCaptions.tsxremotion/src/components/HormoziCaptions.tsxremotion/src/components/KaraokeCaptions.tsxremotion/src/components/SubtleCaptions.tsxremotion/src/types.tssrc/models/index.tssrc/ui/client/CopyButton.tsxsrc/ui/client/EpisodeWorkspace.jsxsrc/ui/client/Layout.tsxsrc/ui/client/lib.test.tssrc/ui/client/lib.tssrc/ui/public/css/styles.csssrc/ui/web-server.tssrc/utils/full-episode-export.test.tssrc/utils/full-episode-export.tssrc/utils/http-range.test.tssrc/utils/http-range.tstests/test_silence_removal.py
|
|
||
| Path(output_dir).mkdir(parents=True, exist_ok=True) | ||
| output_path = _reserve_output_path(video_path, output_dir) | ||
| work_dir = Path(tempfile.mkdtemp(prefix="podcli_silence_", dir=paths["working"] if os.path.isdir(paths["working"]) else None)) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
os.replace fails when the work directory and the output directory are on different filesystems.
Line 436 creates work_dir under paths["working"]. Line 459 moves partial from that directory to output_path, which is under the caller-supplied output_dir. backend/main.py defaults output_dir to paths["output"], and a user can configure an output directory on another volume, such as an external drive. os.replace raises OSError: [Errno 18] Invalid cross-device link in that case. The render then fails after the full encode has completed, which is the most expensive point to fail.
shutil.move falls back to a copy-and-delete across filesystems.
🐛 Proposed fix
- os.replace(partial, output_path)
+ shutil.move(str(partial), str(output_path))Also applies to: 459-459
🤖 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/silence_removal.py` at line 436, Update the final move
operation near the partial-output handling to use shutil.move instead of
os.replace, allowing the completed file to move across filesystems. Preserve the
existing source partial path, output_path destination, and cleanup behavior.
| const run = (command, args) => { | ||
| const result = spawnSync(command, args, { | ||
| encoding: "utf8", | ||
| stdio: ["ignore", "pipe", "pipe"], | ||
| }); | ||
| if (result.status !== 0) { | ||
| const detail = (result.stderr || result.stdout || "unknown error").trim().slice(-3000); | ||
| throw new Error(`${path.basename(command)} failed (${result.status}): ${detail}`); | ||
| } | ||
| return result.stdout.trim(); | ||
| }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
spawnSync has no timeout, so a stalled ffmpeg hangs the export permanently.
run blocks the Node event loop for the whole child process. If ffmpeg or ffprobe stalls, the SIGINT and SIGTERM handlers at lines 101-102 cannot run, and src/ui/web-server.ts never sees the process close. The job stays running forever and the user cannot cancel it.
Add a timeout and treat a timeout as a failure.
🛡️ Proposed fix
-const run = (command, args) => {
+const run = (command, args, timeout = 3_600_000) => {
const result = spawnSync(command, args, {
encoding: "utf8",
stdio: ["ignore", "pipe", "pipe"],
+ timeout,
});
+ if (result.error) {
+ throw new Error(`${path.basename(command)} failed: ${result.error.message}`);
+ }
if (result.status !== 0) {📝 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 run = (command, args) => { | |
| const result = spawnSync(command, args, { | |
| encoding: "utf8", | |
| stdio: ["ignore", "pipe", "pipe"], | |
| }); | |
| if (result.status !== 0) { | |
| const detail = (result.stderr || result.stdout || "unknown error").trim().slice(-3000); | |
| throw new Error(`${path.basename(command)} failed (${result.status}): ${detail}`); | |
| } | |
| return result.stdout.trim(); | |
| }; | |
| const run = (command, args, timeout = 3_600_000) => { | |
| const result = spawnSync(command, args, { | |
| encoding: "utf8", | |
| stdio: ["ignore", "pipe", "pipe"], | |
| timeout, | |
| }); | |
| if (result.error) { | |
| throw new Error(`${path.basename(command)} failed: ${result.error.message}`); | |
| } | |
| if (result.status !== 0) { | |
| const detail = (result.stderr || result.stdout || "unknown error").trim().slice(-3000); | |
| throw new Error(`${path.basename(command)} failed (${result.status}): ${detail}`); | |
| } | |
| return result.stdout.trim(); | |
| }; |
🤖 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 `@remotion/render-full-episode.mjs` around lines 31 - 41, Update the run
function’s spawnSync options to enforce a timeout for ffmpeg/ffprobe commands,
and treat a timed-out result as a failure alongside nonzero exit status. Ensure
the thrown error includes the command context and available stderr/stdout
details, while preserving normal stdout handling for successful completion.
| captionPosition: opts["caption-position"] || "auto", | ||
| captionFontScale: Number(opts["caption-font-scale"] || 100), | ||
| logoPosition: opts["logo-position"] || "top-left", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Both Remotion entry points parse --caption-font-scale without validation or clamping. Each renderer calls bare Number() on the CLI argument, so a non-numeric value yields NaN and an out-of-range value passes through unchanged. backend/services/caption_renderer.py clamps the same setting to 60-160 on the ASS path, so the two caption paths can render different output for the same request.
remotion/render.mjs#L154-L156: replaceNumber(opts["caption-font-scale"] || 100)with a finiteness check plus a 60-160 clamp, defaulting to 100.remotion/render-full-episode.mjs#L64-L64: apply the identical check toNumber(args["caption-font-scale"] || 100).
Put the clamp in one shared helper that both entry points import, so the range stays aligned with the backend.
📍 Affects 2 files
remotion/render.mjs#L154-L156(this comment)remotion/render-full-episode.mjs#L64-L64
🤖 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 `@remotion/render.mjs` around lines 154 - 156, The caption font scale parsing
in render.mjs and render-full-episode.mjs accepts NaN and values outside the
backend’s supported range. Add one shared helper for the caption font scale that
parses the CLI value, defaults non-finite values to 100, and clamps finite
values to 60–160; import and use it at remotion/render.mjs:154-156 and
remotion/render-full-episode.mjs:64-64, replacing the existing Number
expressions in both sites.
| singleLine?: boolean; | ||
| }> = ({ words, currentTime, frame, fps, style, singleLine = false }) => { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the chunk-size limits that bound single-line caption width.
rg -n -C3 'MAX_CHARS_PER_CHUNK|wordsPerChunk|maxChars' remotion/srcRepository: nmbrthirteen/podcli
Length of output: 5248
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== BrandedCaptions outline =="
ast-grep outline remotion/src/components/BrandedCaptions.tsx || true
echo
echo "== BrandedCaptions relevant lines =="
cat -n remotion/src/components/BrandedCaptions.tsx | sed -n '1,180p'
echo
echo "== types relevant lines =="
cat -n remotion/src/types.ts | sed -n '1,110p'
echo
echo "== chunks relevant lines =="
cat -n remotion/src/chunks.ts | sed -n '1,90p'Repository: nmbrthirteen/podcli
Length of output: 13194
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate caption renderer and related constants =="
rg -n -C4 'CAPTION_GAP_FILL_MAX|MAX_CHARS|wordsPerChunk|1\.6|captionFontScale|fontScale|caption font' .
echo
echo "== python shape/constant probe for BrandedCaptions bounding assumptions =="
python3 - <<'PY'
import re
from pathlib import Path
tsx = Path('remotion/src/components/BrandedCaptions.tsx').read_text()
types = Path('remotion/src/types.ts').read_text()
chunks = Path('remotion/src/chunks.ts').read_text()
constant = re.search(r'const\s+MAX_CHARS_PER_CHUNK\s*=\s*(\d+)', tsx)
branded_per_chunk = re.search(r"branded:\s*{(?P<body>.*?)wordsPerChunk:\s*(\d+)", types, re.S)
font = re.search(r"branded:\s*{(?P<body>.*?)fontSize:\s*(\d+)", types, re.S)
scales = [1]
print("MAX_CHARS_PER_CHUNK", constant.group(1) if constant else None)
print("branded_wordsPerChunk", branded_per_chunk.group(2) if branded_per_chunk else None)
print("branded_fontSize", font.group(2) if font else None)
print("reference height", re.search(r'REFERENCE_HEIGHT\s*=\s*(\d+)', types).group(1) if re.search(r'REFERENCE_HEIGHT\s*=\s*(\d+)', types) else None)
print("left/right inset pixels on 1080 canvas", 60 * 1080 / 1920)
print("left/right inset pixels on 1920 canvas", 60)
def max_chars_per_frame():
return int(constant.group(1))
def rendered_chars_at_scale(s):
return max_chars_per_frame()
for scale in scales:
left_right = 60 * scale
print({"canvas_height": 1920 * scale, "left_right_margin_px": left_right})
PYRepository: nmbrthirteen/podcli
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== remotion components after caption line =="
cat -n remotion/src/components/BrandedCaptions.tsx | sed -n '177,230p'
echo
echo "== remotion chunk split behavior =="
python3 - <<'PY'
from pathlib import Path
import re
txs = [("remotion/src/chunks.ts", re.escape("splitTail"))]
for path, needle in txs:
text = Path(path).read_text()
idx = text.find("splitTail")
start = max(0, text.rfind("export function buildChunks", 0, idx))
end = text.find("// Captions vanishing", idx)
print(text[start:end].replace(" ", " "))
PYRepository: nmbrthirteen/podcli
Length of output: 3369
Clamp single-line caption lines to the safe inset.
line 90 sets whiteSpace: "nowrap" while left/right are fixed at 60 * s; MAX_CHARS_PER_CHUNK = 18 does not constrain physical width for the captionFontScale range up to 1.6. A single chunk can exceed (width - 120 * s) / 2 and overflow the container, so keep nowrap for word breaks only or enforce a rendered-width clamp.
🤖 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 `@remotion/src/components/BrandedCaptions.tsx` around lines 76 - 77, Update the
single-line rendering in the BrandedCaptions component to prevent caption text
from exceeding the safe inset between the fixed left/right offsets, while
preserving nowrap behavior for word grouping. Apply a rendered-width constraint
or equivalent scaling/clamping for the full captionFontScale range, using the
existing caption container dimensions and style logic.
| ...(logoPosition.startsWith("top-") ? { top: 180 * s } : { bottom: 180 * s }), | ||
| ...(logoPosition.endsWith("-left") | ||
| ? { left: 108 * s } | ||
| : logoPosition.endsWith("-right") | ||
| ? { right: 108 * s } | ||
| : { left: "50%", transform: "translateX(-50%)" }), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Bottom logo placement can overlap the captions.
The logo is placed at bottom: 180 * s with height: 126 * s, so it occupies roughly 180–306 scaled units from the bottom. When captionPosition is "lower", Root.tsx sets marginBottom to 220, so the caption block starts inside that band. The logo and the captions then overlap. Consider offsetting the caption margin when logoPosition starts with bottom-.
♻️ Example guard
const baseMargin = style.marginBottom * s;
let dynamicMargin = baseMargin;
+ if (logoPosition.startsWith("bottom-")) {
+ dynamicMargin = Math.max(dynamicMargin, 330 * 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 `@remotion/src/components/BrandedCaptions.tsx` around lines 156 - 161, Update
the caption margin calculation in BrandedCaptions so lower captions receive
additional bottom offset when logoPosition starts with "bottom-". Preserve the
existing margin behavior for top logos and other caption positions, and ensure
the offset prevents the caption block from overlapping the bottom logo.
| const clearEpisode = () => { | ||
| fetch('/api/ui-state', { | ||
| method: 'POST', | ||
| headers: { 'Content-Type': 'application/json' }, | ||
| body: JSON.stringify({ | ||
| _source: 'ui', _allowClear: true, videoPath: '', filePath: '', | ||
| transcript: null, rawTranscriptText: '', suggestions: [], | ||
| silenceOriginal: null, silencePlan: null, | ||
| deselectedIndices: [], phase: 'idle', results: [], energyData: {}, | ||
| }), | ||
| }).catch(() => {}); | ||
| setVideoPath(''); setFile(null); setTranscript(null); setTranscriptText(''); | ||
| setSuggestions([]); setDeselected(new Set()); setPhase('idle'); setResults([]); | ||
| setSilenceOriginal(null); setSilencePlan(null); | ||
| setEnergyData({}); setPreviewSrc(null); setPreviewMode('clips'); | ||
| autoTranscribeRef.current = ''; | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Three reset paths leave a stale fullEpisodeResult. resetClipWorkForSource clears fullEpisodeResult at line 1508, but these three paths reset the workspace without calling it. The stale value keeps the "Preview rendered" button and the download link pointing at the previous episode.
src/ui/client/EpisodeWorkspace.jsx#L1286-L1302: addsetFullEpisodeResult(null)toclearEpisode, or callresetClipWorkForSourceinstead of the inline resets.src/ui/client/EpisodeWorkspace.jsx#L858-L873: addsetFullEpisodeResult(null)to thechangedVideobranch ofloadPreset.src/ui/client/EpisodeWorkspace.jsx#L2660-L2660: addsetFullEpisodeResult(null)to the "Start over" handler.
📍 Affects 1 file
src/ui/client/EpisodeWorkspace.jsx#L1286-L1302(this comment)src/ui/client/EpisodeWorkspace.jsx#L858-L873src/ui/client/EpisodeWorkspace.jsx#L2660-L2660
🤖 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/client/EpisodeWorkspace.jsx` around lines 1286 - 1302, Clear stale
fullEpisodeResult in all three workspace reset paths: add
setFullEpisodeResult(null) to clearEpisode, the changedVideo branch of
loadPreset, and the “Start over” handler at src/ui/client/EpisodeWorkspace.jsx
lines 1286-1302, 858-873, and 2660 respectively.
| useEffect(() => { | ||
| if (silenceRenderStream?.status === 'done') { | ||
| const rendered = silenceRenderStream.result; | ||
| const original = pendingSilenceOriginalRef.current; | ||
| if (rendered?.output_path && rendered?.transcript && original) { | ||
| setSilenceOriginal(original); | ||
| setVideoPath(rendered.output_path); | ||
| setFile({ file_path: rendered.output_path }); | ||
| setTranscript(rendered.transcript); | ||
| setSilencePlan(prev => ({ ...(prev || {}), applied: true, output_path: rendered.output_path })); | ||
| resetClipWorkForSource(); | ||
| autoTranscribeRef.current = rendered.output_path; | ||
| } | ||
| pendingSilenceOriginalRef.current = null; | ||
| setSilenceRenderJobId(null); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
A silence render that returns no output path fails silently.
Lines 1537-1545 apply the result only when output_path, transcript, and the pending original are all present. If any is missing, the handler clears the job id and shows nothing. The progress panel disappears and the user gets no explanation. Set an error in the else path.
🐛 Proposed fix
if (rendered?.output_path && rendered?.transcript && original) {
...
autoTranscribeRef.current = rendered.output_path;
+ } else {
+ setError('Silence removal finished without a compact episode.');
}📝 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.
| useEffect(() => { | |
| if (silenceRenderStream?.status === 'done') { | |
| const rendered = silenceRenderStream.result; | |
| const original = pendingSilenceOriginalRef.current; | |
| if (rendered?.output_path && rendered?.transcript && original) { | |
| setSilenceOriginal(original); | |
| setVideoPath(rendered.output_path); | |
| setFile({ file_path: rendered.output_path }); | |
| setTranscript(rendered.transcript); | |
| setSilencePlan(prev => ({ ...(prev || {}), applied: true, output_path: rendered.output_path })); | |
| resetClipWorkForSource(); | |
| autoTranscribeRef.current = rendered.output_path; | |
| } | |
| pendingSilenceOriginalRef.current = null; | |
| setSilenceRenderJobId(null); | |
| useEffect(() => { | |
| if (silenceRenderStream?.status === 'done') { | |
| const rendered = silenceRenderStream.result; | |
| const original = pendingSilenceOriginalRef.current; | |
| if (rendered?.output_path && rendered?.transcript && original) { | |
| setSilenceOriginal(original); | |
| setVideoPath(rendered.output_path); | |
| setFile({ file_path: rendered.output_path }); | |
| setTranscript(rendered.transcript); | |
| setSilencePlan(prev => ({ ...(prev || {}), applied: true, output_path: rendered.output_path })); | |
| resetClipWorkForSource(); | |
| autoTranscribeRef.current = rendered.output_path; | |
| } else { | |
| setError('Silence removal finished without a compact episode.'); | |
| } | |
| pendingSilenceOriginalRef.current = null; | |
| setSilenceRenderJobId(null); |
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 1537-1537: Avoid using the initial state variable in setState
Context: setSilenceOriginal(original)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(setstate-same-var)
🤖 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/client/EpisodeWorkspace.jsx` around lines 1533 - 1547, Update the
silenceRenderStream completion handler in the useEffect to set an appropriate
error when rendered output_path, transcript, or
pendingSilenceOriginalRef.current is missing, before clearing the pending state
and job ID. Preserve the existing success updates when all required values are
present.
| <div className="silence-timeline" aria-label={`${silencePlan.cut_count || 0} silent sections will be removed`}> | ||
| {(silencePlan.removed_ranges || []).map((range, index) => ( | ||
| <span key={index} className="silence-cut" style={{ | ||
| left: `${(range.start / silencePlan.source_duration) * 100}%`, | ||
| width: `${((range.end - range.start) / silencePlan.source_duration) * 100}%`, | ||
| }} /> | ||
| ))} | ||
| </div> |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guard the timeline division against a missing source_duration.
Line 2145 already guards with silencePlan.source_duration || 0. Lines 2153-2154 divide by the same field without a guard. A missing or zero value produces NaN% or Infinity%, and the browser drops the declarations. Reuse a single guarded value.
♻️ Proposed fix
- <div className="silence-timeline" aria-label={`${silencePlan.cut_count || 0} silent sections will be removed`}>
- {(silencePlan.removed_ranges || []).map((range, index) => (
+ <div className="silence-timeline" aria-label={`${silencePlan.cut_count || 0} silent sections will be removed`}>
+ {(silencePlan.source_duration > 0 ? silencePlan.removed_ranges || [] : []).map((range, index) => (📝 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.
| <div className="silence-timeline" aria-label={`${silencePlan.cut_count || 0} silent sections will be removed`}> | |
| {(silencePlan.removed_ranges || []).map((range, index) => ( | |
| <span key={index} className="silence-cut" style={{ | |
| left: `${(range.start / silencePlan.source_duration) * 100}%`, | |
| width: `${((range.end - range.start) / silencePlan.source_duration) * 100}%`, | |
| }} /> | |
| ))} | |
| </div> | |
| <div className="silence-timeline" aria-label={`${silencePlan.cut_count || 0} silent sections will be removed`}> | |
| {(silencePlan.source_duration > 0 ? silencePlan.removed_ranges || [] : []).map((range, index) => ( | |
| <span key={index} className="silence-cut" style={{ | |
| left: `${(range.start / silencePlan.source_duration) * 100}%`, | |
| width: `${((range.end - range.start) / silencePlan.source_duration) * 100}%`, | |
| }} /> | |
| ))} | |
| </div> |
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 2150-2155: Do not use array indexes for a list component's key
Context: (silencePlan.removed_ranges || []).map((range, index) => (
<span key={index} className="silence-cut" style={{
left: ${(range.start / silencePlan.source_duration) * 100}%,
width: ${((range.end - range.start) / silencePlan.source_duration) * 100}%,
}} />
))
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(list-component-no-index)
🤖 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/client/EpisodeWorkspace.jsx` around lines 2150 - 2157, Update the
silence timeline rendering in the map around silencePlan.removed_ranges to reuse
a single guarded source-duration value, consistent with the existing
silencePlan.source_duration || 0 handling. Use that guarded value for both the
range.start and range.end - range.start percentage calculations, preventing
unguarded division when source_duration is missing or zero.
| .logo-position-option:disabled { cursor: not-allowed; opacity: 0.42; } | ||
| .logo-position-mark { | ||
| position: absolute; width: 10px; height: 5px; border-radius: 2px; | ||
| background: currentColor; color: var(--text3); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the Stylelint value keyword.
Line 749 uses currentColor, which violates value-keyword-case. Use the configured lowercase form so the stylesheet passes lint.
Proposed fix
- background: currentColor; color: var(--text3);
+ background: currentcolor; color: var(--text3);📝 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.
| background: currentColor; color: var(--text3); | |
| background: currentcolor; color: var(--text3); |
🧰 Tools
🪛 Stylelint (17.14.0)
[error] 749-749: Expected "currentColor" to be "currentcolor" (value-keyword-case)
(value-keyword-case)
🤖 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/public/css/styles.css` at line 749, Update the background declaration
at the affected styles.css rule to use the configured lowercase spelling of the
current-color keyword, while leaving the adjacent text color declaration
unchanged.
Source: Linters/SAST tools
| if (typeof video_path !== "string" || !existsSync(video_path)) { | ||
| res.status(400).json({ error: "Select a local episode first" }); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Three new endpoints accept an arbitrary video_path and bypass the existing source allowlist. Each new route validates video_path with existsSync alone. The file already maintains allowedSourcePaths with registerSourcePath, and safePath constrains filenames to a base directory, so the shared root cause is that the new handlers do not reuse that control. A request can make the server read, probe, or re-encode any file the server user can read, and the job status discloses whether a given path exists.
src/ui/web-server.ts#L1465-L1468: gatevideo_pathin/api/analyze-silencethrough the allowlist before creating the analysis job.src/ui/web-server.ts#L1521-L1524: gatevideo_pathin/api/render-silence-removedthrough the same check before rendering and before writing the.silence.jsonmanifest.src/ui/web-server.ts#L1601-L1604: gatevideo_pathin/api/export-full-episodethrough the same check before spawning the renderer.
Extract one assertAllowedSource(video_path) helper and call it at all three sites.
📍 Affects 1 file
src/ui/web-server.ts#L1465-L1468(this comment)src/ui/web-server.ts#L1521-L1524src/ui/web-server.ts#L1601-L1604
🤖 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 1465 - 1468, The three new handlers bypass
the source allowlist by validating video_path only with existsSync. In
src/ui/web-server.ts at lines 1465-1468, 1521-1524, and 1601-1604, extract an
assertAllowedSource(video_path) helper using the existing
allowedSourcePaths/registerSourcePath controls, and call it before creating
analysis jobs, rendering or writing manifests, and spawning the renderer.
1 participant
What this does
How I tested it
Checklist
npx tsc --noEmitandnpm testpass (pluspytest tests/if you touched the backend)Summary by CodeRabbit