feat: config profiles, integrations system, and DaVinci Resolve export#1
Merged
Merged
Conversation
Replace ad-hoc `os.path.join(__file__, "..", "..", ".podcli", ...)` patterns across the Python backend with a single `config.paths` module that resolves home/data/cache locations from env vars (PODCLI_HOME, PODCLI_DATA) and a `.podcli-home` marker file. The TypeScript side mirrors the same precedence in `src/config/paths.ts`, and `python-executor.ts` propagates both env vars to the Python subprocess so the two layers always agree. Also: ignore the `.podcli-home` profile marker to prevent absolute paths from leaking into commits.
Three independent but related additions, plus the ProRes 4444 alpha caption overlay that the editor exporter needs. Config profiles - `podcli config <export|import|use|status|migrate>` packages a config home (knowledge, presets, assets, settings) into a portable zip and restores it elsewhere. Asset paths in registries/presets are rewritten to point at the imported copies. Imports back up the existing home before clobbering. - Idempotent legacy-cache migration runs once on startup to move stale `project/.podcli/cache/` content into `data/cache/`. - Equivalent MCP tool `manage_config` and HTTP endpoints under `/api/config`. - Bundle extraction rejects symlinks, absolute paths, and parent-traversal members to prevent zip-slip and out-of-tree writes. Output integrations - New `backend/services/integrations/` package with a registry/manager and a shared FCPXML/IR helper module for editor exporters. - DaVinci Resolve integration (experimental, opt-in via `manage_integrations action=enable name=davinci_resolve`) emits an FCPXML 1.10 project: one compound clip per short on the master timeline, V1 source plus optional V2 alpha caption overlay so cuts and captions remain editable in free or Studio Resolve 20.x. - MCP tools: `manage_integrations`, `export_to_davinci_resolve`. UI page at `/integrations.html` toggles per-integration state. ProRes alpha caption overlay - `remotion/render.mjs --keep-overlay` writes a ProRes 4444 alpha overlay alongside the baked output instead of deleting it. - `clip_generator.generate_clip` exposes `keep_caption_overlay` and returns `caption_overlay_path` + `cropped_source_path` for editor handoff. It also auto-enables the overlay when the davinci_resolve integration is on, so enabling the editor integration is the only knob users have to flip. Also fixes a pre-existing bug exposed by the new `from config.paths import paths` import in cli.py: a local `paths = _thumb_gen(...)` in the thumbnail loop shadowed the module-level dict, so `cmd_process` crashed with UnboundLocalError on any clip that triggered earlier `paths[...]` lookups. Renamed the local to `thumb_paths`.
After Claude/Codex picks moments in `[3/4] Selecting moments...`, persist the result to `<home>/sessions/clips-<cache_hash>.json` keyed by the same video hash used for the transcript cache. A subsequent `podcli process` on the same video reloads the saved suggestions instantly instead of re-running the AI step (which can take several minutes on long episodes and bills tokens each time). `--no-resume` forces regeneration. Session files are only loaded when `top_n` matches; mismatches fall through to a fresh AI call.
|
Warning Review limit reached
More reviews will be available in 44 minutes. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (13)
📝 WalkthroughWalkthroughThis PR centralizes filesystem paths, adds an integrations framework and DaVinci Resolve exporter, implements portable config bundle export/import with safe extraction and legacy migration, adds Remotion caption-overlay export and wiring, updates MCP/Web UI routes and tools, and includes tests and docs. ChangesPath Resolution and Configuration Infrastructure
Integration Framework and DaVinci Resolve
Caption Overlay Export for DaVinci Resolve
Config Bundle Export, Import, and Legacy Migration
Transcript Cache Migration and Suggestion Resume
Web API Routes and Web UI
Documentation, tests, and misc
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
backend/services/clip_generator.py (1)
900-913:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPersist the overlay artifacts outside
work_dir.
caption_overlay_pathandcropped_source_pathpoint intowork_dir, but thefinallyblock deletes that directory wheneveroutput_diris set. In the normal CLI flow the caller gets paths to files that no longer exist, so the Resolve exporter can't consume them.💡 Proposed fix
- out = { + persisted_overlay_path = None + persisted_cropped_path = None + if keep_caption_overlay and caption_overlay_path and os.path.exists(caption_overlay_path): + base, _ = os.path.splitext(final_path) + persisted_overlay_path = f"{base}_captions.mov" + persisted_cropped_path = f"{base}_cropped.mp4" + shutil.copy2(caption_overlay_path, persisted_overlay_path) + shutil.copy2(cropped_path, persisted_cropped_path) + + out = { "output_path": final_path, "duration": round(duration, 2), "file_size_mb": file_size_mb, "title": title, "start_second": start_second, "end_second": end_second, "caption_style": caption_style, "crop_strategy": crop_strategy, } - if keep_caption_overlay and caption_overlay_path and os.path.exists(caption_overlay_path): - out["caption_overlay_path"] = caption_overlay_path - out["cropped_source_path"] = cropped_path + if persisted_overlay_path and persisted_cropped_path: + out["caption_overlay_path"] = persisted_overlay_path + out["cropped_source_path"] = persisted_cropped_path return outAlso applies to: 915-918
🤖 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/clip_generator.py` around lines 900 - 913, The caption overlay and cropped source artifacts (caption_overlay_path and cropped_path) are currently inside work_dir which is removed in the finally block, so persist them to a safe location before work_dir is deleted: if output_dir (or final_path's parent) is set, copy/move caption_overlay_path and cropped_path to a durable location (e.g., the same directory as final_path or a subfolder under output_dir), update the out dict keys ("caption_overlay_path" and "cropped_source_path") to point to the new persisted paths, and only include the persisted paths in the returned out; ensure this logic is applied where out is constructed (the block that builds out and the conditional that adds caption_overlay_path/cropped_source_path) so callers receive valid, non-temporary file paths.src/server.ts (1)
771-824:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the resolved batch inputs in the direct fallback.
You compute
resolvedClips,resolvedVideoPath, andresolvedTranscriptWordsforexport_selected/clip_numbers, but the non-Web-UI fallback still callshandleBatchClips(params). If the UI server is down, this path can export the wrong clips or fail with missing inputs even though resolution already succeeded.Suggested fix
- finalResult = await handleBatchClips(params); + finalResult = await handleBatchClips({ + ...params, + video_path: resolvedVideoPath, + clips: resolvedClips, + transcript_words: resolvedTranscriptWords, + });Also applies to: 895-900
🤖 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/server.ts` around lines 771 - 824, The resolved batch inputs (resolvedClips, resolvedVideoPath, resolvedTranscriptWords) computed for export_selected/clip_numbers are not used in the non‑UI fallback; instead handleBatchClips(params) is called which can use stale/missing params. Update the fallback(s) that call handleBatchClips (and the similar block around the 895-900 region) to call handleBatchClips with the resolved values or to replace params' fields before calling it — i.e., pass an object where clips = resolvedClips, video_path = resolvedVideoPath, and transcript_words = resolvedTranscriptWords (falling back to original params only if those resolved variables are still undefined) so the exported batch uses the resolved inputs.src/ui/web-server.ts (1)
1737-1747:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
/api/mcp/exportis stripping clip fields the new export flow depends on.This mapper only preserves a small subset of clip properties and only translates
segments -> keep_segments. That drops already-normalizedkeep_segmentsfromsrc/server.tsand also dropskeep_caption_overlay, so MCP-driven multi-cut exports collapse to single ranges and overlay exports silently lose their sidecar assets.Suggested fix
const styledClips = clips.map((c: any) => ({ + ...c, start_second: c.start_second, end_second: c.end_second, title: (c.title || "clip").slice(0, 40), caption_style: c.caption_style || captionStyle, crop_strategy: c.crop_strategy || cropStrategy, allow_ass_fallback: c.allow_ass_fallback === true || allowAssFallback, - // Preserve multi-cut segments from suggestions - ...(Array.isArray(c.segments) && - c.segments.length > 0 && { keep_segments: c.segments }), + ...(Array.isArray(c.keep_segments) && + c.keep_segments.length > 0 && { keep_segments: c.keep_segments }), + ...(Array.isArray(c.segments) && + c.segments.length > 0 && { keep_segments: c.segments }), + keep_caption_overlay: c.keep_caption_overlay === true, })); @@ { video_path: videoPath, clips: styledClips, transcript_words: transcriptWords, output_dir: paths.output, logo_path: logoPath, outro_path: outroPath, + keep_caption_overlay: req.body.keep_caption_overlay === true, face_map: uiState.transcript?.face_map, },Also applies to: 1771-1778
🤖 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 1737 - 1747, The mapper in styledClips is dropping clip properties the export flow needs (it converts segments -> keep_segments but overwrites or omits already-normalized keep_segments and omits keep_caption_overlay and other passthrough fields), so update the clips.map transformation (styledClips and the similar block around the other occurrence) to: 1) preserve an existing c.keep_segments if present and only derive keep_segments from c.segments when c.keep_segments is absent, 2) include c.keep_caption_overlay (or default) in the output, and 3) pass-through any other important sidecar/overlay fields rather than stripping them—use the existing c.* values (e.g. keep_segments, keep_caption_overlay, allow_ass_fallback) when available instead of always replacing them.
🧹 Nitpick comments (6)
backend/services/integrations/davinci_resolve/__init__.py (1)
8-8: 💤 Low valueConsider alphabetically sorting
__all__.Ruff suggests sorting
__all__entries alphabetically. While the current order matches import order, alphabetical sorting improves consistency and maintainability.♻️ Proposed fix
-__all__ = ["integration", "emitter"] +__all__ = ["emitter", "integration"]🤖 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/integrations/davinci_resolve/__init__.py` at line 8, The __all__ export list is unsorted; update the __all__ variable in the module to list its names in alphabetical order (e.g., put "emitter" before "integration") so the __all__ = [...] tuple/ list is alphabetically ordered for consistency and to satisfy linters like Ruff.backend/services/integrations/_shared/media_probe.py (1)
13-50: ⚡ Quick winConsider combining the two ffprobe calls.
Running
ffprobetwice (once for video, once for audio) is less efficient than running once with both stream selections. While this works correctly, combining them could reduce overhead, especially when probing many files.♻️ Example combined approach
cmd = [ "ffprobe", "-v", "error", "-select_streams", "v:0,a:0", "-show_entries", "stream=width,height,r_frame_rate,nb_frames,duration,channels:format=duration", "-of", "json", p, ] data = json.loads(subprocess.check_output(cmd, text=True, timeout=30)) streams = data.get("streams", []) v_stream = next((s for s in streams if s.get("width")), {}) a_stream = next((s for s in streams if s.get("channels")), {})🤖 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/integrations/_shared/media_probe.py` around lines 13 - 50, The two separate ffprobe invocations in media_probe.py should be merged into a single ffprobe call: call ffprobe once with "-select_streams", "v:0,a:0" and "-show_entries", "stream=width,height,r_frame_rate,nb_frames,duration,channels:format=duration", parse the single JSON result (replace v_data/a_data usage with a single data variable), then locate v_stream as the stream with width/height/r_frame_rate and a_stream as the stream with channels (e.g., using next over data.get("streams", [])); keep the existing logic that computes width, height, fps, fmt_duration and duration_frames but read those values from v_stream/a_stream in the consolidated data and include a timeout on subprocess.check_output as appropriate.backend/services/integrations/davinci_resolve/cli.py (2)
70-72: 💤 Low valueDefault output directory may not exist.
The default output path assumes
data/export/davinci_resolve/exists. If it doesn't,emitter.emit()will fail when writing the file. Consider ensuring the directory exists or documenting that users should create it.🛡️ Proposed fix
out = args.out or ( REPO_ROOT / "data" / "export" / "davinci_resolve" / f"{args.title.replace(' ', '_')}.fcpxml" ) + out.parent.mkdir(parents=True, exist_ok=True)🤖 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/integrations/davinci_resolve/cli.py` around lines 70 - 72, The default output path assigned to out (out = args.out or (REPO_ROOT / "data" / "export" / "davinci_resolve" / f"{args.title.replace(' ', '_')}.fcpxml")) assumes the parent directory exists and will cause emitter.emit() to fail; before calling emitter.emit() ensure the output file's parent directory exists by creating out.parent with parents=True and exist_ok=True (or document that users must create data/export/davinci_resolve), referencing the variables out, REPO_ROOT, args.title, and the emitter.emit call so the directory creation happens immediately prior to writing the file.
16-18: ⚡ Quick winFragile path calculation using fixed parent depth.
The REPO_ROOT calculation assumes this file is exactly 4 directories deep. If the file is moved or the structure changes, this will break silently.
♻️ More robust alternative
-REPO_ROOT = Path(__file__).resolve().parents[4] +# Find repo root by looking for a marker file +def _find_repo_root(): + p = Path(__file__).resolve() + for parent in [p] + list(p.parents): + if (parent / "backend").is_dir() and (parent / "package.json").is_file(): + return parent + raise RuntimeError("Could not locate repository root") + +REPO_ROOT = _find_repo_root()🤖 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/integrations/davinci_resolve/cli.py` around lines 16 - 18, The REPO_ROOT calculation is fragile because parents[4] assumes fixed depth; change it to walk up the directory tree from Path(__file__).resolve() until you find a repository marker (e.g., a ".git" directory or the "backend" folder) and set REPO_ROOT to that parent instead of using parents[4]; then use that computed REPO_ROOT when checking/inserting into sys.path (symbols to update: REPO_ROOT, Path(__file__).resolve(), and the sys.path.insert call) so the import path logic continues to work if files are moved.backend/services/integrations/davinci_resolve/emitter.py (2)
30-31: 💤 Low valueDocument the rationale for ID starting values.
The asset IDs start at 2 and media IDs at 1000 without explanation. A brief comment would help future maintainers understand whether these specific values are significant or arbitrary.
🤖 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/integrations/davinci_resolve/emitter.py` around lines 30 - 31, Add a short explanatory comment above the next_asset and next_media initializations in emitter.py that documents why asset IDs start at 2 and media IDs at 1000 (e.g., reserved ID 1 for root/placeholder, gap for legacy IDs, or arbitrary chosen to avoid collision), or mark them as intentionally arbitrary if no special meaning; reference the variables next_asset and next_media and update their nearby comment or module docstring to capture this rationale for future maintainers.
23-33: ⚡ Quick winConsider validating that
project.shortsis non-empty.The function proceeds directly to building the library even if
project.shortsis empty, which would produce valid FCPXML with no clips. Adding an early check would provide clearer feedback.🛡️ Proposed validation
def emit(project: Project, out_path: Path) -> Path: + if not project.shorts: + raise ValueError("Project must contain at least one short") fmt_id = "r1"🤖 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/integrations/davinci_resolve/emitter.py` around lines 23 - 33, The emit function should validate that project.shorts is non-empty before building the FCPXML; add an early check at the top of emit (before building fmt_id/resources/compounds) that tests if not project.shorts and then raise a clear exception (e.g., ValueError with a descriptive message) or return an explicit error so callers know there are no clips to emit; update any callers or tests that expect this behavior.
🤖 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/cli.py`:
- Around line 405-410: The session key currently only uses
compute_cache_hash(video_path) so passing a custom transcript (args.transcript)
can reuse stale suggestions; update the resume logic in the code that sets
cache_hash (uses services.transcript_packer.compute_cache_hash) to either (a)
disable resume when args.transcript is present by leaving cache_hash empty/None,
or (b) include the transcript and relevant config in the session key (e.g.,
compute a combined hash of video_path plus the transcript file contents/mtime
and any config flags) so functions that rely on cache_hash will never reuse
suggestions from a different transcript; apply the same change to the other
resume spots noted (the blocks around where cache_hash is computed at the other
occurrences).
In `@backend/config_bundle.py`:
- Around line 496-497: Activating a new home writes the marker but never
refreshes the module-level in-memory paths cache, so functions like
get_active_home() and later home-relative operations still return the old home;
after writing the marker in set_active_home() and any code path that calls
_marker_path().write_text(...) (including import_config(..., activate=True)),
invalidate or reload the cached paths (e.g., update the module-level paths
variable or call the existing path-loading routine) so
config.paths/get_active_home() reflect the new marker immediately in-process;
alternatively remove the import-time caching and make get_active_home() read the
marker dynamically. Ensure you update all activation sites referenced (the
functions that call _marker_path().write_text) to perform this refresh.
In `@backend/main.py`:
- Around line 476-483: The auto-migration call in _maybe_auto_migrate_backend
currently runs inline and will propagate exceptions to request handling; wrap
the import and call to auto_migrate_legacy_if_pending(quiet=True) in a
try/except that catches Exception, logs the exception (contextual message
referencing auto-migrate) and continues without re-raising so migration failures
are best-effort and non-blocking; apply the same try/except pattern to the other
identical call referenced at the other location (the occurrence around line 498)
and keep the existing early-return logic for "manage_config" tasks unchanged.
- Around line 453-454: The current except block returns the full traceback in
the API response; instead, log the full traceback on the server and emit a
sanitized error string to the client. In the except block for emit_result use
logging.exception(...) or a server logger to record traceback.format_exc() (or
call logger.exception(e)) and then call emit_result(task_id, "error",
error=f"{type(e).__name__}: {e}") or a generic message like "Internal server
error" so no stack/paths are sent to clients; update the block around
emit_result and the exception handler to perform server-side logging and a
sanitized emit_result call.
In `@backend/services/encoder.py`:
- Line 173: The docstring comment still hardcodes "data/cache/encoder.json" but
the cache path is now resolved dynamically via paths["cache"]; update the
docstring in backend/services/encoder.py (the comment at the Cached at ... line)
to reference the dynamic cache location (e.g. paths["cache"] joined with the
encoder filename) and note it is keyed by the ffmpeg binary fingerprint so the
documentation matches the actual behavior in this module.
In `@backend/services/integrations/_shared/media_probe.py`:
- Around line 13-21: The subprocess.check_output calls that run ffprobe
(building v_cmd and invoking subprocess.check_output to produce v_data) need a
timeout to avoid hanging; update the call to include a timeout parameter (e.g.,
timeout=30) and handle subprocess.TimeoutExpired if desired, and apply the same
change to the audio probe call that builds a_cmd / produces a_data so both
ffprobe invocations use a timeout.
In `@backend/services/integrations/base.py`:
- Around line 46-47: The register method on IntegrationBase currently overwrites
any existing entry in cls._instances; change register (def register(cls,
integration: IntegrationBase)) to first check if integration.name already exists
in cls._instances and if so raise a clear exception (e.g., ValueError or
RuntimeError) preventing silent overwrite; otherwise proceed to assign
cls._instances[integration.name] = integration so duplicate registrations fail
fast and surface the conflict.
In `@backend/services/integrations/davinci_resolve/integration.py`:
- Around line 64-66: The code currently allows an empty params["shorts"] to
proceed and emit a project with no clips; before calling probe_media and
building shorts (the block that starts with shorts: list[Short] = [] and the
later similar block around the probe_media loop), validate that params["shorts"]
is present and not empty (or that the resulting shorts list after filtering is
non-empty) and raise a clear validation error (e.g., ValueError or a
ValidationError) instead of continuing; apply this check both where
params["shorts"] is consumed and in the second similar loop to fail fast and
return an error response rather than success metadata.
In `@backend/utils/prompt_files.py`:
- Line 25: The module docstring and any function comments referring to
“.podcli/tmp/” are out of date—update them to reflect that temporary files are
now written to os.path.join(paths["home"], "tmp") (the variable base in this
file), so change wording to indicate the temp directory is under the configured
paths["home"]/tmp location (and adjust any troubleshooting/manual cleanup
instructions accordingly).
In `@src/config/paths.ts`:
- Around line 11-22: The resolveHome() function currently calls
readFileSync(homeMarker) without error handling which can crash the app; wrap
the readFileSync(homeMarker, "utf-8") call in a try-catch inside the
existsSync(homeMarker) block (keeping the isAbsolute(marker) check) and on any
exception treat it as a missing/invalid marker by falling back to return
resolve(projectRoot, ".podcli"); reference resolveHome(), homeMarker,
readFileSync, existsSync, and projectRoot when making the change.
In `@src/handlers/integrations.handler.ts`:
- Around line 127-133: The handler currently calls handleManageIntegrations({
action, name }) even when action is "enable" or "disable" and name is empty; add
an upfront guard in the async handler (the anonymous async ({ action, name }) =>
{ ... } block) that checks if action === "enable" || action === "disable" and if
name is falsy, then return a clear MCP error via mcpText (e.g., a descriptive
message like "Missing required 'name' for enable/disable action") instead of
calling handleManageIntegrations; this will fail fast and avoid sending an empty
name to Python.
In `@src/handlers/integrations.routes.ts`:
- Around line 44-57: The export route that calls
executor.execute("manage_config", ...) and then res.download(file, ...) must
delete the temporary bundle file after the response completes—use the
res.download callback or res.on('finish'/'close') to unlink the bundlePath/file
so the ZIPs don't accumulate; likewise, for the import route that reads
req.file.path, ensure you remove the uploaded file in a finally block after
processing (regardless of success/failure) to guarantee cleanup; reference the
export handler around executor.execute and res.download and the import handler
using req.file.path when adding the unlink logic.
In `@src/server.ts`:
- Around line 509-515: The export path in src/server.ts is not forwarding the
keep_caption_overlay flag to the /api/mcp/export endpoint, so Web-UI exports
ignore overlay-enabled renders; update the code that constructs the export
request payload for the Web-UI export path so it includes the
keep_caption_overlay property (preserve its boolean value/default), propagate it
through any intermediate structures or typed interfaces used when calling
/api/mcp/export, and ensure the server handler reading /api/mcp/export consumes
that field; specifically add keep_caption_overlay to the JSON/body sent to
/api/mcp/export wherever export requests are assembled.
In `@src/ui/public/integrations.html`:
- Around line 169-170: The current integration toggle is a non-focusable div
("toggle" element with role="switch" and data-name="${escapeHtml(int.name)}")
and must be replaced or augmented with a keyboard-accessible control: change the
element to a semantic, focusable control (e.g., a <button> or an <input
type="checkbox">) that maintains role="switch" or native semantics, ensure
aria-checked reflects int.enabled, include keyboard event handling (Space/Enter)
to toggle the state, keep the data-name attribute for identification, and ensure
visual classes ("on") are applied when toggled; update any click-only listeners
attached to the old ".toggle" to handle keyboard activation as well.
In `@src/ui/web-server.ts`:
- Around line 1643-1645: The field transcriptWordCount is returning the words
array itself due to misplaced parentheses around (uiState.transcript?.words ??
[].length); change it to return a numeric length instead by using
uiState.transcript?.words.length (or (uiState.transcript?.words ?? []).length)
so transcriptWordCount is always a number; update the expression that sets
transcriptWordCount in web-server.ts to reference
uiState.transcript?.words.length and fall back to 0 when missing.
- Around line 909-919: The current isUploadedFile check uses startsWith on
path.resolve(join(paths.working, "uploads")), which mistakenly matches sibling
directories like "uploads-backup"; change the guard in the web-server logic that
computes resolvedPath and isUploadedFile so you compare against the uploads
directory with a directory boundary (e.g., compute uploadsDir =
path.resolve(join(paths.working, "uploads")) and ensure resolvedPath ===
uploadsDir or resolvedPath.startsWith(uploadsDir + path.sep)) or use
path.relative to verify the file is inside uploads (relative does not begin with
'..'); update the isUploadedFile boolean accordingly where resolvedPath,
isSessionVideo, and paths.working are used.
---
Outside diff comments:
In `@backend/services/clip_generator.py`:
- Around line 900-913: The caption overlay and cropped source artifacts
(caption_overlay_path and cropped_path) are currently inside work_dir which is
removed in the finally block, so persist them to a safe location before work_dir
is deleted: if output_dir (or final_path's parent) is set, copy/move
caption_overlay_path and cropped_path to a durable location (e.g., the same
directory as final_path or a subfolder under output_dir), update the out dict
keys ("caption_overlay_path" and "cropped_source_path") to point to the new
persisted paths, and only include the persisted paths in the returned out;
ensure this logic is applied where out is constructed (the block that builds out
and the conditional that adds caption_overlay_path/cropped_source_path) so
callers receive valid, non-temporary file paths.
In `@src/server.ts`:
- Around line 771-824: The resolved batch inputs (resolvedClips,
resolvedVideoPath, resolvedTranscriptWords) computed for
export_selected/clip_numbers are not used in the non‑UI fallback; instead
handleBatchClips(params) is called which can use stale/missing params. Update
the fallback(s) that call handleBatchClips (and the similar block around the
895-900 region) to call handleBatchClips with the resolved values or to replace
params' fields before calling it — i.e., pass an object where clips =
resolvedClips, video_path = resolvedVideoPath, and transcript_words =
resolvedTranscriptWords (falling back to original params only if those resolved
variables are still undefined) so the exported batch uses the resolved inputs.
In `@src/ui/web-server.ts`:
- Around line 1737-1747: The mapper in styledClips is dropping clip properties
the export flow needs (it converts segments -> keep_segments but overwrites or
omits already-normalized keep_segments and omits keep_caption_overlay and other
passthrough fields), so update the clips.map transformation (styledClips and the
similar block around the other occurrence) to: 1) preserve an existing
c.keep_segments if present and only derive keep_segments from c.segments when
c.keep_segments is absent, 2) include c.keep_caption_overlay (or default) in the
output, and 3) pass-through any other important sidecar/overlay fields rather
than stripping them—use the existing c.* values (e.g. keep_segments,
keep_caption_overlay, allow_ass_fallback) when available instead of always
replacing them.
---
Nitpick comments:
In `@backend/services/integrations/_shared/media_probe.py`:
- Around line 13-50: The two separate ffprobe invocations in media_probe.py
should be merged into a single ffprobe call: call ffprobe once with
"-select_streams", "v:0,a:0" and "-show_entries",
"stream=width,height,r_frame_rate,nb_frames,duration,channels:format=duration",
parse the single JSON result (replace v_data/a_data usage with a single data
variable), then locate v_stream as the stream with width/height/r_frame_rate and
a_stream as the stream with channels (e.g., using next over data.get("streams",
[])); keep the existing logic that computes width, height, fps, fmt_duration and
duration_frames but read those values from v_stream/a_stream in the consolidated
data and include a timeout on subprocess.check_output as appropriate.
In `@backend/services/integrations/davinci_resolve/__init__.py`:
- Line 8: The __all__ export list is unsorted; update the __all__ variable in
the module to list its names in alphabetical order (e.g., put "emitter" before
"integration") so the __all__ = [...] tuple/ list is alphabetically ordered for
consistency and to satisfy linters like Ruff.
In `@backend/services/integrations/davinci_resolve/cli.py`:
- Around line 70-72: The default output path assigned to out (out = args.out or
(REPO_ROOT / "data" / "export" / "davinci_resolve" / f"{args.title.replace(' ',
'_')}.fcpxml")) assumes the parent directory exists and will cause
emitter.emit() to fail; before calling emitter.emit() ensure the output file's
parent directory exists by creating out.parent with parents=True and
exist_ok=True (or document that users must create data/export/davinci_resolve),
referencing the variables out, REPO_ROOT, args.title, and the emitter.emit call
so the directory creation happens immediately prior to writing the file.
- Around line 16-18: The REPO_ROOT calculation is fragile because parents[4]
assumes fixed depth; change it to walk up the directory tree from
Path(__file__).resolve() until you find a repository marker (e.g., a ".git"
directory or the "backend" folder) and set REPO_ROOT to that parent instead of
using parents[4]; then use that computed REPO_ROOT when checking/inserting into
sys.path (symbols to update: REPO_ROOT, Path(__file__).resolve(), and the
sys.path.insert call) so the import path logic continues to work if files are
moved.
In `@backend/services/integrations/davinci_resolve/emitter.py`:
- Around line 30-31: Add a short explanatory comment above the next_asset and
next_media initializations in emitter.py that documents why asset IDs start at 2
and media IDs at 1000 (e.g., reserved ID 1 for root/placeholder, gap for legacy
IDs, or arbitrary chosen to avoid collision), or mark them as intentionally
arbitrary if no special meaning; reference the variables next_asset and
next_media and update their nearby comment or module docstring to capture this
rationale for future maintainers.
- Around line 23-33: The emit function should validate that project.shorts is
non-empty before building the FCPXML; add an early check at the top of emit
(before building fmt_id/resources/compounds) that tests if not project.shorts
and then raise a clear exception (e.g., ValueError with a descriptive message)
or return an explicit error so callers know there are no clips to emit; update
any callers or tests that expect this 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: f56e140a-d39d-4b4f-a887-30b5a0c3746f
📒 Files selected for processing (50)
.gitignoreCONTRIBUTING.mdREADME.mdbackend/cli.pybackend/config/paths.pybackend/config_bundle.pybackend/main.pybackend/presets.pybackend/services/asset_store.pybackend/services/claude_suggest.pybackend/services/clip_generator.pybackend/services/content_generator.pybackend/services/corrections.pybackend/services/encoder.pybackend/services/integrations/__init__.pybackend/services/integrations/_shared/__init__.pybackend/services/integrations/_shared/fcpxml.pybackend/services/integrations/_shared/media_probe.pybackend/services/integrations/_shared/timeline_ir.pybackend/services/integrations/base.pybackend/services/integrations/davinci_resolve/README.mdbackend/services/integrations/davinci_resolve/__init__.pybackend/services/integrations/davinci_resolve/cli.pybackend/services/integrations/davinci_resolve/emitter.pybackend/services/integrations/davinci_resolve/integration.pybackend/services/integrations/manager.pybackend/services/thumbnail_ai.pybackend/services/thumbnail_generator.pybackend/services/thumbnail_html.pybackend/services/transcript_packer.pybackend/utils/prompt_files.pyremotion/render.mjsremotion/test-render.mjssrc/config/paths.test.tssrc/config/paths.tssrc/handlers/batch-clips.handler.tssrc/handlers/create-clip.handler.tssrc/handlers/integrations.handler.tssrc/handlers/integrations.routes.tssrc/models/index.tssrc/server.tssrc/services/python-executor.tssrc/ui/public/config.htmlsrc/ui/public/index.htmlsrc/ui/public/integration.htmlsrc/ui/public/integrations.htmlsrc/ui/web-server.tssrc/utils/logger.test.tstests/test_clip_generator.pytests/test_config_bundle.py
- cli: key resume sessions on selection signature (ai_select, duration bounds); make --no-cache bypass resume; clear stale session on regenerate - fcpxml: emit canonical NTSC frame durations and derive DF/NDF timecode instead of limit_denominator(1000) and hardcoded NDF - media_probe: wrap ffprobe with actionable errors; prefer duration*fps over unreliable nb_frames - config_bundle: drop imported manifest.json (leaks exporter abs paths), clean it on rollback; validate zip directory entries; fix asset_count - davinci: reject empty shorts export - routes: delete temp export/import zips
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/config_bundle.py (1)
509-515:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't return the raw imported manifest to callers.
manifeststill containssource_homeand the absolute-pathpath_mapfrom the exporting machine. Returning it here leaks the same host-specific data you just removed from disk, and this result is surfaced through the config action entrypoints.🤖 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/config_bundle.py` around lines 509 - 515, The returned dict exposes host-specific data because the raw manifest (variable manifest) still contains source_home and absolute paths in path_map; sanitize before returning by creating a deep copy of manifest and removing or redacting source_home and converting or removing absolute entries in path_map (or replace them with relative keys/URIs) so callers do not receive host-specific paths; then return the cleaned copy in the "manifest" key instead of the original manifest.backend/services/integrations/_shared/fcpxml.py (1)
47-48:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
Path.as_uri()forfile://URIs infile_uri()The current
file_uri()manually builds URIs and produces incorrect Windows-style output (file://C%3A%5C...) instead of a valid local file URI. Usep.resolve().as_uri()(it formats correctly on both POSIX and Windows).def file_uri(p: Path) -> str: return p.resolve().as_uri()🤖 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/integrations/_shared/fcpxml.py` around lines 47 - 48, The file_uri function currently builds a file URI by concatenating "file://" with urllib.parse.quote of p.resolve(), which produces incorrect Windows-style URIs; update the implementation of file_uri to return a proper file URI using Path.as_uri on the resolved path (i.e., call p.resolve().as_uri()) so it formats correctly on both POSIX and Windows and remove the manual urllib.parse.quote usage.
♻️ Duplicate comments (2)
backend/services/integrations/_shared/media_probe.py (1)
10-18:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBound the shared
ffprobecall with a timeout.
_run_ffprobe()still uses an unboundedsubprocess.check_output, so one hungffprobeprocess can stall the entire export path indefinitely. Add a timeout here and translateTimeoutExpiredinto the sameRuntimeErrorsurface.Proposed fix
def _run_ffprobe(cmd: list[str], path: str) -> dict[str, Any]: try: - return json.loads(subprocess.check_output(cmd, text=True, stderr=subprocess.PIPE)) - except FileNotFoundError: - raise RuntimeError("ffprobe not found — install ffmpeg to use editor-export integrations") + return json.loads(subprocess.check_output(cmd, text=True, stderr=subprocess.PIPE, timeout=30)) + except FileNotFoundError as e: + raise RuntimeError("ffprobe not found — install ffmpeg to use editor-export integrations") from e + except subprocess.TimeoutExpired as e: + raise RuntimeError(f"ffprobe timed out for {path}") from e except subprocess.CalledProcessError as e: - raise RuntimeError(f"ffprobe failed for {path}: {(e.stderr or '').strip() or e}") - except json.JSONDecodeError: - raise RuntimeError(f"ffprobe returned unexpected output for {path}") + raise RuntimeError(f"ffprobe failed for {path}: {(e.stderr or '').strip() or e}") from e + except json.JSONDecodeError as e: + raise RuntimeError(f"ffprobe returned unexpected output for {path}") from e🤖 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/integrations/_shared/media_probe.py` around lines 10 - 18, The _run_ffprobe function currently calls subprocess.check_output without a timeout and can hang; update the call in _run_ffprobe(cmd: list[str], path: str) to pass a reasonable timeout (e.g., 10s) and add an except subprocess.TimeoutExpired block that raises a RuntimeError with a message consistent with the existing errors (include the path and useful details), so timeouts are surfaced the same way as CalledProcessError and FileNotFoundError.backend/config_bundle.py (1)
502-503:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRefresh the in-memory path map after switching homes.
Both activation paths only rewrite the marker file. In the same Python process,
get_active_home()andconfig statusstill read the oldpaths["home"]until restart, so the new home is not actually active for subsequent operations.Also applies to: 518-526, 536-543
🤖 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/config_bundle.py` around lines 502 - 503, After writing the marker file with _marker_path().write_text(...), also update the in-memory paths map so subsequent calls to get_active_home() and "config status" see the new home; specifically set paths["home"] = str(target) (or the same normalized value written to the marker) and, if a reload function exists (e.g. _load_paths() / reload_paths()), call it to refresh any cached state; apply the same change to the other activation sites around the regions mentioned (the blocks around lines 518-526 and 536-543) so the process-wide state matches the marker file immediately.
🤖 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.
Outside diff comments:
In `@backend/config_bundle.py`:
- Around line 509-515: The returned dict exposes host-specific data because the
raw manifest (variable manifest) still contains source_home and absolute paths
in path_map; sanitize before returning by creating a deep copy of manifest and
removing or redacting source_home and converting or removing absolute entries in
path_map (or replace them with relative keys/URIs) so callers do not receive
host-specific paths; then return the cleaned copy in the "manifest" key instead
of the original manifest.
In `@backend/services/integrations/_shared/fcpxml.py`:
- Around line 47-48: The file_uri function currently builds a file URI by
concatenating "file://" with urllib.parse.quote of p.resolve(), which produces
incorrect Windows-style URIs; update the implementation of file_uri to return a
proper file URI using Path.as_uri on the resolved path (i.e., call
p.resolve().as_uri()) so it formats correctly on both POSIX and Windows and
remove the manual urllib.parse.quote usage.
---
Duplicate comments:
In `@backend/config_bundle.py`:
- Around line 502-503: After writing the marker file with
_marker_path().write_text(...), also update the in-memory paths map so
subsequent calls to get_active_home() and "config status" see the new home;
specifically set paths["home"] = str(target) (or the same normalized value
written to the marker) and, if a reload function exists (e.g. _load_paths() /
reload_paths()), call it to refresh any cached state; apply the same change to
the other activation sites around the regions mentioned (the blocks around lines
518-526 and 536-543) so the process-wide state matches the marker file
immediately.
In `@backend/services/integrations/_shared/media_probe.py`:
- Around line 10-18: The _run_ffprobe function currently calls
subprocess.check_output without a timeout and can hang; update the call in
_run_ffprobe(cmd: list[str], path: str) to pass a reasonable timeout (e.g., 10s)
and add an except subprocess.TimeoutExpired block that raises a RuntimeError
with a message consistent with the existing errors (include the path and useful
details), so timeouts are surfaced the same way as CalledProcessError and
FileNotFoundError.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8466ba07-4daa-4a12-8a01-842a842d0ba7
📒 Files selected for processing (7)
backend/cli.pybackend/config_bundle.pybackend/services/integrations/_shared/fcpxml.pybackend/services/integrations/_shared/media_probe.pybackend/services/integrations/davinci_resolve/emitter.pybackend/services/integrations/davinci_resolve/integration.pysrc/handlers/integrations.routes.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- backend/services/integrations/davinci_resolve/integration.py
- backend/services/integrations/davinci_resolve/emitter.py
- src/handlers/integrations.routes.ts
- backend/cli.py
- cli: disable suggestion resume when a custom --transcript is supplied - config/paths: add reload_paths(); refresh in-memory paths on home activate (set_active_home + import_config activate) so config status reflects it live - main: log traceback server-side, return sanitized API error; isolate auto-migration failures from task dispatch - integrations/base: fail fast on duplicate integration name registration - media_probe: add 30s ffprobe timeout - paths.ts: guard resolveHome() marker read; fall back to default on error - integrations.handler: require name for enable/disable - server: forward keep_caption_overlay through the Web-UI export path - web-server: tighten uploads boundary check (path.relative, not startsWith); return transcript word count instead of the words array - integrations.html: keyboard-accessible <button> toggle with focus ring - docs: correct encoder/prompt_files cache + tmp path wording
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Three connected additions plus the paths cleanup they ride on top of:
os.path.join(__file__, "..", "..", ".podcli", ...)across the Python backend with a singleconfig.pathsmodule. TypeScript mirrors the same env-var precedence (PODCLI_HOME,PODCLI_DATA) and propagates both vars to the Python subprocess. Also ignores the.podcli-homeprofile marker so absolute paths can't leak into commits.podcli config <export|import|use|status|migrate>packages the config home (knowledge, presets, assets, settings) into a portable zip and restores it elsewhere. Asset paths in registries/presets are rewritten to point at the imported copies. Imports back up the existing home before clobbering. MCP toolmanage_configand/api/config/*HTTP endpoints expose the same actions. Idempotent migration moves legacyproject/.podcli/cache/content intodata/cache/on first run. Zip extraction rejects symlinks, absolute paths, and parent-traversal members.backend/services/integrations/introduces a registry/manager for editor exporters. DaVinci Resolve is the first integration (opt-in viamanage_integrations action=enable name=davinci_resolve): emits an FCPXML 1.10 project with one compound clip per short on the master timeline, V1 source + optional V2 alpha caption overlay, editable in free or Studio Resolve 20.x. Configure via/integrations.html. Real-clip flow tested end-to-end against 19 rendered shorts.remotion/render.mjs --keep-overlaykeeps a transparent caption overlay alongside the baked output instead of deleting it.clip_generator.generate_clipreturnscaption_overlay_path+cropped_source_pathfor editor handoff and auto-enables the overlay when the DaVinci integration is on, so enabling the integration is the only toggle users need.[3/4] Selecting moments...now persists results to<home>/sessions/clips-<hash>.json. Subsequent runs ofpodcli processon the same video instant-resume instead of re-running Claude/Codex (saves several minutes + tokens per retry on long episodes).--no-resumeopts out.Also fixes a pre-existing
pathsvariable shadow incmd_processthat the new module-levelfrom config.paths import pathsimport would otherwise unmask (localpaths = _thumb_gen(...)in the thumbnail loop turned the dict access into anUnboundLocalError).Test plan
pytest tests/— 298 passing (incl. newtest_config_bundle.pycovering round-trip, backup-on-failure, zip-slip rejection, legacy cache + presets migration)npx tsc --noEmitcleanexport_to_davinci_resolve→ 17 KB FCPXML written → import path documented inbackend/services/integrations/davinci_resolve/README.md.fcpxmlinto free DaVinci Resolve 20.x and confirm the 19 compound clips render identically to the baked outputsdavinci_resolveintegration → runcreate_clip→ verify_captions.movProRes 4444 alpha overlay lands next to the MP4podcli config export ~/test.zip→podcli config import ~/test.zip --home ~/.podcli-alt --activate→ verify knowledge/presets/assets paths resolve correctly under the new homeNotes on scope
DaVinci Resolve integration is marked experimental in code (
default_enabled = False) and in docs. The FCPXML emitter is intentionally constrained to free-tier Resolve structures — no Studio-only effects, noadjust-blend, no transform keyframes (Resolve mis-translates those). Future editor integrations (Premiere, FCP, CapCut) can subclassIntegrationBaseand reuse the_sharedFCPXML/IR helpers.Summary by CodeRabbit
New Features
Documentation
Bug Fixes
Refactor
Tests