Repository navigation
fix: misc bugs, library rename, TTS timing improvements - #20
Conversation
…rror CosyVoice3 0.5B was missing from /models/status and /models/download endpoints, so the Create Voice wizard never showed it as a required download. Added it to status, download map, and health check. Model download crashed with "SyntaxError: JSON Parse error: Unexpected identifier data" because downloadModel() used the generic sidecar_fetch proxy + JSON.parse() on an SSE stream response. Switched to the existing download_model Rust command which correctly parses SSE and streams progress via Tauri Channel. Removed the now-unnecessary polling loop.
Fix model count assertion (2 → 3) and add cosyvoice key checks to health endpoint tests.
Set codesign_identity to "Developer ID Application" and add a sidecar-specific entitlements.plist (JIT, unsigned memory, disable-library-validation for PyInstaller + MLX, no audio/camera).
Fix model list join to use commas for 3+ models and update size estimate from ~4GB to ~5GB to account for CosyVoice3.
Voice Conversion was missing video_duration — the sidecar received original_duration=0 so audio padding was skipped, producing audio shorter than the video. Added apad + -shortest to ffmpeg replace_audio as a safety net: if generated audio is shorter than video for any reason, silence-pad to match. This prevents early cutoff in both modes.
Allow users to rename recordings from the library via an inline edit interaction. Renaming updates the .mp4 and all companion files (.meta.json, .txt, cached thumbnail) atomically.
- probe_duration: check both video stream AND format/container duration, take the max (WebM often lacks per-stream duration metadata) - Blended fade-out (300ms) before silence padding in both VC and TTS fallback paths for smooth audio-to-silence transitions - Interpolate degenerate Whisper timestamps (zero-duration words get estimated timing from average valid word duration) - Crossfade word boundaries (25ms fade-in/out) in silence insertion to eliminate audible stops/starts - Remove hallucination filter — was too aggressive, filtered valid segments with repetitive content (compression_ratio > 2.4) - Disable Whisper repetition suppression (compression_ratio_threshold, no_speech_threshold, condition_on_previous_text) so repeated phrases are transcribed faithfully - Revert apad/-shortest from ffmpeg (made clipping worse)
Falls back to the ELEVENLABS_API_KEY env var when no key is saved in the app config or keychain. Avoids re-entering the key every time during development.
- Add Voice Creation Wizard screenshots (5 steps) - Fix Preview.png casing (Preview → preview) - Update project structure: add missing Rust modules (library.rs, models.rs, secrets.rs, tts_provider.rs), frontend components, sidecar build files, scripts directory - Fix routes to show (app) group and library page - Add 9 missing Tauri commands to reference table (poll_generation, get_generation_audio, library CRUD, rename, open, reveal) - Update architecture diagram with all backend modules - Document ELEVENLABS_API_KEY env var auto-detection - Update model download info (all 3 via Create Voice wizard) - Add rename to video library feature list
Excalidraw-generated architecture diagram showing three-column layout (Frontend, Backend, Sidecar) with connection arrows, ML models, external services, and processing pipeline flows.
Each model now gets its own status dot (green/gray) so all three models are always visible, even if some are already downloaded.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR removes two Claude GitHub Actions workflows, adds recording rename support (UI + Tauri command + Rust logic + tests), enhances local TTS timing and fades, adds CosyVoice3 model support, adds macOS sidecar entitlements and signing, updates several sidecar tests/docs, and wires progress/evented model downloads in the frontend. Changes
Sequence Diagram(s)mermaid mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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: 4
🧹 Nitpick comments (10)
src-tauri/src/library.rs (2)
287-293: No-op detection triggers expensive full directory scan.When the user renames to the exact same name,
scan_recordingsscans the entire output directory just to return the existingRecordingInfo. Consider constructing theRecordingInfodirectly (similar to the success path at Lines 329-370) to avoid the overhead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/library.rs` around lines 287 - 293, The no-op branch that checks if new_path == old_path currently calls scan_recordings(&output_dir) and searches for the RecordingInfo, which is expensive; instead, construct and return the RecordingInfo directly without scanning by building the same struct fields used in the successful rename path (use file_path, output_dir, new_path/old_path values and any metadata available) and return it as Ok(RecordingInfo) from the new_path == old_path branch, avoiding the scan_recordings call.
272-285: Redundant validation ofold_pathon Line 273.
validate_in_output_dir(&old_path, &output_dir)was already called at Line 252. This second call on Line 273 is redundant.♻️ Remove redundant validation
// Prevent traversal via ".." in the name - validate_in_output_dir(&old_path, &output_dir) - .and_then(|_| { - // new_path may not exist yet, so validate its parent - let parent = new_path.parent().ok_or("Invalid path")?; - let canonical_parent = std::fs::canonicalize(parent) - .map_err(|e| format!("Invalid path: {e}"))?; - let canonical_dir = std::fs::canonicalize(&output_dir) - .map_err(|e| format!("Invalid output directory: {e}"))?; - if !canonical_parent.starts_with(&canonical_dir) { - return Err("Access denied: target is outside the output directory".to_string()); - } - Ok(()) - })?; + // new_path may not exist yet, so validate its parent + let parent = new_path.parent().ok_or("Invalid path")?; + let canonical_parent = std::fs::canonicalize(parent) + .map_err(|e| format!("Invalid path: {e}"))?; + let canonical_dir = std::fs::canonicalize(&output_dir) + .map_err(|e| format!("Invalid output directory: {e}"))?; + if !canonical_parent.starts_with(&canonical_dir) { + return Err("Access denied: target is outside the output directory".to_string()); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/library.rs` around lines 272 - 285, Remove the redundant call to validate_in_output_dir(&old_path, &output_dir) in the block that validates the target path; instead only validate the new_path's parent as intended: get new_path.parent(), canonicalize the parent and the output_dir, compare canonical_parent.starts_with(&canonical_dir) and return the same access-denied error if it fails. Keep the same error messages and use the existing map_err/...? propagation so the function behavior and types remain unchanged; reference the validate_in_output_dir function, new_path, old_path, output_dir, parent, canonical_parent, canonical_dir and std::fs::canonicalize when locating the code to edit.src-tauri/src/config.rs (1)
188-196: Environment variable value may be persisted to keychain on next save.When the API key is auto-populated from
ELEVENLABS_API_KEY, a subsequentsave_configcall (e.g., when the user modifies any setting) will persist that value to the OS keychain viasave_secrets(). This is likely intentional as a bootstrap mechanism, but users expecting the env var to remain ephemeral may be surprised.Consider either:
- Documenting this behavior in the README tip, or
- Adding a flag to track that the key came from an env var if you want to avoid persisting it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/config.rs` around lines 188 - 196, The code auto-populates config.elevenlabs_api_key from the ELEVENLABS_API_KEY env var, which will then be persisted to the OS keychain on the next save_config/save_secrets call; to avoid unintentionally persisting an env-derived secret add a provenance flag (e.g., elevenlabs_api_key_source or elevenlabs_api_key_from_env) to the Config struct when loading in the function that sets config.elevenlabs_api_key, set that flag to true when sourcing from std::env::var, and update save_config/save_secrets to skip persisting keys whose source flag indicates they came from the environment (or clear the flag if the user edits the field to intentionally persist it); alternatively, update documentation to clearly state this bootstrap persistence behavior if you prefer to keep current logic.src/lib/RecordingCard.svelte (1)
201-212: Consider adding explicit save/cancel affordances for better discoverability.The rename workflow commits on
Enterand cancels onEscapeor blur. While this is efficient for keyboard users, there's no visual indication of how to confirm the rename. Consider adding:
- A checkmark button to commit, or
- Placeholder text like "Press Enter to save"
This is a minor UX polish and the current implementation is functional.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/RecordingCard.svelte` around lines 201 - 212, The rename input currently relies solely on keyboard events (handleRenameKeydown) and blur (cancelRename); add an explicit visual affordance to improve discoverability by updating the editing branch that renders the input (the block using editing, editName, editInput, and class="rename-input") to include either a clickable commit control (e.g., a small checkmark button that calls the same commit/rename handler as Enter) and a cancel button (that calls cancelRename), or at minimum add a placeholder/title on the input like "Press Enter to save" to signal the behavior; ensure the new button(s) call the existing rename commit logic and keep accessibility attributes (aria-label) consistent with recording.filename display.src-tauri/sidecar/voiceover-tts.spec (1)
159-160: Parameterize the signing identity for CI/dev portability.Line 159 hardcodes
"Developer ID Application", which is fragile when developers or CI environments have multiple matching certificates. Use an environment variable with fallback:⚙️ Proposed refactor
+import os from PyInstaller.utils.hooks import collect_all, copy_metadata, collect_submodules @@ - codesign_identity="Developer ID Application", + codesign_identity=os.environ.get("VOICEOVER_CODESIGN_IDENTITY", "Developer ID Application"), entitlements_file="entitlements.plist",This allows developers and CI to set
VOICEOVER_CODESIGN_IDENTITYwhen needed, while preserving the existing default for local builds.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/sidecar/voiceover-tts.spec` around lines 159 - 160, Replace the hardcoded codesign_identity value with a lookup of the VOICEOVER_CODESIGN_IDENTITY environment variable (falling back to "Developer ID Application") so the spec becomes configurable for CI/dev; update the place where codesign_identity is set in the voiceover-tts.spec (the codesign_identity key) to read process/env-style env var with a default, leaving entitlements_file unchanged.src-tauri/sidecar/test_server_integration.py (1)
211-217: Prefer asserting required model IDs overlen(...) == 3.This will fail the next time a supported model is added even if
/models/statusis still correct. Checking for the expected entries keeps the test tied to the endpoint contract instead of the current catalog size.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/sidecar/test_server_integration.py` around lines 211 - 217, Replace the brittle length check in test_models_status with assertions that the required model IDs are present: keep the GET call to client.get("/models/status"), parse data = response.json(), then assert "models" in data and that data["models"] contains the expected model identifiers (e.g., check for specific IDs you require) rather than asserting len(data["models"]) == 3; reference the test_models_status function and the data["models"] list when making these membership assertions.src-tauri/sidecar/server.py (2)
427-431: Potential dtype mismatch in in-place fade multiplication.The fade array is explicitly
dtype=np.float32, butaudiomay have a different dtype depending on upstream generation functions. Ifaudioisfloat64or another type, the in-place multiplicationaudio[-fade_samples:] *= fadecould cause precision loss or unexpected behavior.Consider ensuring consistent dtype:
♻️ Ensure dtype consistency
# Fade out last 300ms before silence padding fade_samples = min(int(0.3 * sample_rate), len(audio) // 2) if fade_samples > 0: - fade = np.linspace(1.0, 0.0, fade_samples, dtype=np.float32) + fade = np.linspace(1.0, 0.0, fade_samples, dtype=audio.dtype) audio[-fade_samples:] *= fade🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/sidecar/server.py` around lines 427 - 431, The in-place multiplication audio[-fade_samples:] *= fade can cause dtype mismatch because fade is created as np.float32 while audio may be a different dtype; update the fade creation and/or perform safe casting so types match: compute fade with dtype=audio.dtype (use np.linspace(..., dtype=audio.dtype)) or cast audio slice to float32 before applying fade and then cast back, ensuring operations reference the variables fade, fade_samples, audio and sample_rate so the resulting multiplication does not lose precision or raise casting errors.
605-609: Same dtype consideration as TTS path.Apply the same dtype consistency fix here to match the audio array's dtype.
♻️ Ensure dtype consistency
# Fade out last 300ms before silence padding fade_samples = min(int(0.3 * sample_rate), len(audio) // 2) if fade_samples > 0: - fade = np.linspace(1.0, 0.0, fade_samples, dtype=np.float32) + fade = np.linspace(1.0, 0.0, fade_samples, dtype=audio.dtype) audio[-fade_samples:] *= fade🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/sidecar/server.py` around lines 605 - 609, The fade array currently uses dtype=np.float32 which can mismatch the existing audio array; modify the fade creation so its dtype matches audio.dtype (e.g., create fade with dtype=audio.dtype or cast it) and ensure the in-place multiplication audio[-fade_samples:] *= fade uses a matching dtype; update the block around fade_samples, fade and audio to construct or cast the fade to audio.dtype to maintain dtype consistency.src/routes/(app)/create-voice/+page.svelte (2)
323-328: Size estimate missing for single-model case.When only one model needs downloading, the size estimate
(~5GB total)is not shown, but it is shown for multiple models. Consider adding size info for consistency, or clarify if this is intentional due to varying single-model sizes.♻️ Add size estimate for single model
{`#if` missing.length === 1} - {missing[0].display_name} needs to be downloaded. + {missing[0].display_name} needs to be downloaded (~1-2GB). {:else}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/`(app)/create-voice/+page.svelte around lines 323 - 328, The single-model branch for rendering missing models currently omits the size estimate while the multiple-model branch appends "(~5GB total)"; update the single-model branch that renders {missing[0].display_name} to also include the size text (e.g., "needs to be downloaded (~5GB total)") or compute and insert a per-model size if available, so the UI is consistent with the names/names.slice(...) branch that uses (~5GB total).
114-116: Status messages displayed with error styling during download.Setting
downloadError = statuscauses status messages like "Starting download of whisper..." to be displayed with theclass="status invalid"styling (Line 346), which likely renders them in red/error colors. This creates confusing UX where progress updates appear as errors.Consider using a separate state variable for download status vs errors:
♻️ Separate download status from error state
Add a new state variable for download status:
let downloading = $state(false); let downloadError = $state(''); +let downloadStatus = $state(''); let prerequisiteError = $state('');Update the callback to use the new status variable:
await client.downloadModel(model.model_name, (_progress, status) => { - downloadError = status; + downloadStatus = status; });Clear status on completion:
downloadError = ''; +downloadStatus = '';Then in the template, display status separately from errors:
{`#if` downloading && downloadStatus} <div class="hint-text">{downloadStatus}</div> {/if} {`#if` downloadError} <div class="status invalid">{downloadError}</div> {/if}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/`(app)/create-voice/+page.svelte around lines 114 - 116, The code sets downloadError = status inside the client.downloadModel callback so transient progress messages (e.g., "Starting download of whisper...") are shown with the error styling; introduce a separate state variable (e.g., downloadStatus) and use that in the callback instead of downloadError, keep downloadError reserved for real failures, clear downloadStatus on successful completion and set downloadError only when an actual error occurs, and update the template to render downloadStatus with non-error styling (while keeping downloadError displayed with class="status invalid"); look for client.downloadModel, downloadError, and downloading in the file to make the changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src-tauri/sidecar/chunked_tts.py`:
- Around line 175-181: The repair loop currently extends degenerate
zero-duration words unboundedly; change it to first find the next non-degenerate
word's start (scan word_timing after the current index for the next entry where
end-start >= 0.01) as next_true_start, and if found clamp and distribute the
repaired run to lie entirely before next_true_start: compute the
available_window = next_true_start - (fixed[-1]["end"] if fixed else
word_timing[0]["start"]) and evenly space the remaining repaired words inside
that window using avg_gap/avg_duration as targets (or fall back to the original
avg_gap/avg_duration when no next_true_start), ensuring each fw["end"] =
min(fw["start"] + avg_duration, next_true_start - tiny_epsilon) and adjusting
fw["start"] for subsequent repaired words so they do not cross next_true_start;
this prevents spilling past the next real timestamp (affects variables
word_timing, fw, fixed, avg_gap, avg_duration and the downstream
pad_audio_to_match_timing logic).
In `@src-tauri/src/library.rs`:
- Around line 264-268: The current suffix check uses new_name.ends_with(".mp4")
which is case-sensitive and can produce filenames like "video.MP4.mp4"; change
the condition to perform a case-insensitive check (e.g., check
new_name.to_lowercase().ends_with(".mp4")) before appending ".mp4" so the code
only appends the extension when the name does not already have a .mp4 extension
in any case; keep the original new_name casing when returning the value.
In `@src-tauri/src/local_tts.rs`:
- Around line 406-410: The payload currently always sets "original_duration" to
video_duration.unwrap_or(0.0) which hides unknown probe failures; update the
body construction in process_recording() so you only include "original_duration"
when ffmpeg::probe_duration() returned Some(value) (or else derive the WAV/audio
duration from the input_path_str and include that), otherwise omit the
"original_duration" field entirely so the sidecar can apply its own
audio-duration fallback; look for the JSON assembly using variables
video_duration, input_path_str and profile_id and conditionally add the field
instead of unwrapping to 0.0.
In `@src/lib/voicebox.ts`:
- Around line 58-77: The frontend downloadModel uses a Tauri Channel/onEvent for
live SSE progress, but the Rust command handler (the download_model command in
src-tauri/src/models.rs) currently calls response.text().await which buffers the
whole SSE stream and delays events; change the handler to stream the HTTP
response body (e.g., use reqwest's byte/line stream or async read of
response.bytes_stream()) and parse SSE chunks incrementally, emitting each
parsed progress/complete/error event immediately to the Tauri event/channel
sender the frontend expects (so the Channel in downloadModel receives onmessage
calls in real time).
---
Nitpick comments:
In `@src-tauri/sidecar/server.py`:
- Around line 427-431: The in-place multiplication audio[-fade_samples:] *= fade
can cause dtype mismatch because fade is created as np.float32 while audio may
be a different dtype; update the fade creation and/or perform safe casting so
types match: compute fade with dtype=audio.dtype (use np.linspace(...,
dtype=audio.dtype)) or cast audio slice to float32 before applying fade and then
cast back, ensuring operations reference the variables fade, fade_samples, audio
and sample_rate so the resulting multiplication does not lose precision or raise
casting errors.
- Around line 605-609: The fade array currently uses dtype=np.float32 which can
mismatch the existing audio array; modify the fade creation so its dtype matches
audio.dtype (e.g., create fade with dtype=audio.dtype or cast it) and ensure the
in-place multiplication audio[-fade_samples:] *= fade uses a matching dtype;
update the block around fade_samples, fade and audio to construct or cast the
fade to audio.dtype to maintain dtype consistency.
In `@src-tauri/sidecar/test_server_integration.py`:
- Around line 211-217: Replace the brittle length check in test_models_status
with assertions that the required model IDs are present: keep the GET call to
client.get("/models/status"), parse data = response.json(), then assert "models"
in data and that data["models"] contains the expected model identifiers (e.g.,
check for specific IDs you require) rather than asserting len(data["models"]) ==
3; reference the test_models_status function and the data["models"] list when
making these membership assertions.
In `@src-tauri/sidecar/voiceover-tts.spec`:
- Around line 159-160: Replace the hardcoded codesign_identity value with a
lookup of the VOICEOVER_CODESIGN_IDENTITY environment variable (falling back to
"Developer ID Application") so the spec becomes configurable for CI/dev; update
the place where codesign_identity is set in the voiceover-tts.spec (the
codesign_identity key) to read process/env-style env var with a default, leaving
entitlements_file unchanged.
In `@src-tauri/src/config.rs`:
- Around line 188-196: The code auto-populates config.elevenlabs_api_key from
the ELEVENLABS_API_KEY env var, which will then be persisted to the OS keychain
on the next save_config/save_secrets call; to avoid unintentionally persisting
an env-derived secret add a provenance flag (e.g., elevenlabs_api_key_source or
elevenlabs_api_key_from_env) to the Config struct when loading in the function
that sets config.elevenlabs_api_key, set that flag to true when sourcing from
std::env::var, and update save_config/save_secrets to skip persisting keys whose
source flag indicates they came from the environment (or clear the flag if the
user edits the field to intentionally persist it); alternatively, update
documentation to clearly state this bootstrap persistence behavior if you prefer
to keep current logic.
In `@src-tauri/src/library.rs`:
- Around line 287-293: The no-op branch that checks if new_path == old_path
currently calls scan_recordings(&output_dir) and searches for the RecordingInfo,
which is expensive; instead, construct and return the RecordingInfo directly
without scanning by building the same struct fields used in the successful
rename path (use file_path, output_dir, new_path/old_path values and any
metadata available) and return it as Ok(RecordingInfo) from the new_path ==
old_path branch, avoiding the scan_recordings call.
- Around line 272-285: Remove the redundant call to
validate_in_output_dir(&old_path, &output_dir) in the block that validates the
target path; instead only validate the new_path's parent as intended: get
new_path.parent(), canonicalize the parent and the output_dir, compare
canonical_parent.starts_with(&canonical_dir) and return the same access-denied
error if it fails. Keep the same error messages and use the existing
map_err/...? propagation so the function behavior and types remain unchanged;
reference the validate_in_output_dir function, new_path, old_path, output_dir,
parent, canonical_parent, canonical_dir and std::fs::canonicalize when locating
the code to edit.
In `@src/lib/RecordingCard.svelte`:
- Around line 201-212: The rename input currently relies solely on keyboard
events (handleRenameKeydown) and blur (cancelRename); add an explicit visual
affordance to improve discoverability by updating the editing branch that
renders the input (the block using editing, editName, editInput, and
class="rename-input") to include either a clickable commit control (e.g., a
small checkmark button that calls the same commit/rename handler as Enter) and a
cancel button (that calls cancelRename), or at minimum add a placeholder/title
on the input like "Press Enter to save" to signal the behavior; ensure the new
button(s) call the existing rename commit logic and keep accessibility
attributes (aria-label) consistent with recording.filename display.
In `@src/routes/`(app)/create-voice/+page.svelte:
- Around line 323-328: The single-model branch for rendering missing models
currently omits the size estimate while the multiple-model branch appends "(~5GB
total)"; update the single-model branch that renders {missing[0].display_name}
to also include the size text (e.g., "needs to be downloaded (~5GB total)") or
compute and insert a per-model size if available, so the UI is consistent with
the names/names.slice(...) branch that uses (~5GB total).
- Around line 114-116: The code sets downloadError = status inside the
client.downloadModel callback so transient progress messages (e.g., "Starting
download of whisper...") are shown with the error styling; introduce a separate
state variable (e.g., downloadStatus) and use that in the callback instead of
downloadError, keep downloadError reserved for real failures, clear
downloadStatus on successful completion and set downloadError only when an
actual error occurs, and update the template to render downloadStatus with
non-error styling (while keeping downloadError displayed with class="status
invalid"); look for client.downloadModel, downloadError, and downloading in the
file to make the changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f1972669-44e5-4048-a11e-c57832e52e75
⛔ Files ignored due to path filters (6)
images/Create_1.pngis excluded by!**/*.pngimages/Create_2.pngis excluded by!**/*.pngimages/Create_3.pngis excluded by!**/*.pngimages/Create_4.pngis excluded by!**/*.pngimages/Create_5.pngis excluded by!**/*.pngimages/architecture.pngis excluded by!**/*.png
📒 Files selected for processing (22)
.github/workflows/claude-code-review.yml.github/workflows/claude.ymlREADME.mdimages/architecture.excalidrawsrc-tauri/sidecar/chunked_tts.pysrc-tauri/sidecar/entitlements.plistsrc-tauri/sidecar/server.pysrc-tauri/sidecar/test_binary.shsrc-tauri/sidecar/test_server_integration.pysrc-tauri/sidecar/test_tts.pysrc-tauri/sidecar/tts.pysrc-tauri/sidecar/voiceover-tts.specsrc-tauri/src/config.rssrc-tauri/src/ffmpeg.rssrc-tauri/src/lib.rssrc-tauri/src/library.rssrc-tauri/src/local_tts.rssrc-tauri/src/pipeline.rssrc/lib/RecordingCard.sveltesrc/lib/library.svelte.tssrc/lib/voicebox.tssrc/routes/(app)/create-voice/+page.svelte
💤 Files with no reviewable changes (2)
- .github/workflows/claude.yml
- .github/workflows/claude-code-review.yml
📜 Review details
🔇 Additional comments (17)
images/architecture.excalidraw (1)
1-628: LGTM!Well-structured architecture diagram documenting the system's three-tier design (Frontend, Backend, Sidecar) with clear data flow arrows and component breakdowns. Good addition for onboarding and documentation.
README.md (3)
39-50: LGTM!Clear documentation of the Voice Creation Wizard with screenshot references and workflow steps.
238-239: LGTM!The environment variable tip correctly documents the new auto-detection feature implemented in
config.rs.
396-404: LGTM!New Tauri commands are properly documented, matching the registrations in
lib.rs.src-tauri/src/lib.rs (1)
94-94: LGTM!Command registration for
rename_recordingcorrectly added to the invoke handler.src/lib/library.svelte.ts (1)
81-85: LGTM!The
renameRecordingmethod correctly invokes the backend command and updates the in-memory state by finding the old entry and replacing it with the updatedRecordingInfo(which contains the new path).src/lib/RecordingCard.svelte (1)
147-186: LGTM!The rename workflow is well-implemented with proper input focus/selection, keyboard handling (Enter to commit, Escape to cancel), and error handling.
src-tauri/src/library.rs (2)
241-371: Overall implementation is solid with good security controls.The rename logic correctly:
- Validates paths are within the output directory (traversal prevention)
- Rejects path separators and null bytes in filenames
- Checks for destination conflicts before renaming
- Renames companion files atomically with the main file
- Returns properly reconstructed
RecordingInfoUnit tests provide good coverage of edge cases.
556-700: LGTM!Comprehensive unit tests covering the rename functionality: companions, extension handling, validation failures, and duplicate detection.
src-tauri/sidecar/entitlements.plist (1)
9-12: PyInstaller's DYLD override requirement: Consider architectural alternatives.The sidecar uses PyInstaller to bundle native dependencies (MLX, qwen-tts, etc.) as dylibs. PyInstaller's macOS bootloader internally requires
DYLD_LIBRARY_PATHoverrides to locate bundled libraries at runtime, makingallow-dyld-environment-variablesa functional requirement for the current architecture. Combined withdisable-library-validation, this does weaken hardening significantly.Rather than removing the entitlement (which would break the binary), consider:
- Re-architecting the sidecar distribution: Use properly signed/notarized dylibs and avoid PyInstaller's DYLD dependency, or
- Documenting the trade-off: If PyInstaller remains, record that the hardening reduction is an accepted cost for this build approach.
If you do pursue option 1 (signed dylibs), you could remove
allow-dyld-environment-variablesand retain proper security posture.src-tauri/sidecar/tts.py (1)
158-167: Non-empty false positives from silence and noise may bypass filtering.We disable
no_speech_thresholdandcompression_ratio_threshold, and skip the hallucination filter for word-timestamp branches. The phrase-grouping and segment-filtering functions remove empty text only (viaif not phrase_text:andif not text: continue). Downstream inlocal_tts.rs, only an empty-transcript check is performed. This means non-empty false positives—such as repeated single words from noise or brief hallucinations—will be synthesized without further validation. If the repetitive-content fix requires these thresholds disabled, consider adding a lightweight semantic gate (e.g., minimum phrase length, confidence score, or spectral entropy check) to filter false positives before synthesis.Also applies to: 202-210
src-tauri/sidecar/test_tts.py (1)
20-27: LGTM! Test correctly updated to reflect new filtering behavior.The renamed test and updated assertions accurately reflect that
_filter_segmentsnow keeps all segments with non-empty text, regardless ofno_speech_proborcompression_ratiovalues. The test fixture effectively covers the implementation's behavior with varying feature values (0.1/0.9 forno_speech_prob, 1.2/3.0 forcompression_ratio).src-tauri/sidecar/server.py (3)
662-662: LGTM! Consistent health check pattern for CosyVoice.The health check correctly reports
cosyvoiceavailability using the same pattern as other models (loaded OR downloaded).
1039-1044: LGTM! CosyVoice3 model status properly integrated.The model status entry follows the established pattern and correctly ties
loadedto_vc_model is not None.
1062-1064: LGTM! Download aliases provide case-insensitive model selection.Good addition of lowercase variant for usability.
src/routes/(app)/create-voice/+page.svelte (2)
312-318: LGTM! Clear visual feedback for individual model download status.The per-model status dots provide helpful visibility into which models are ready vs pending.
120-124: LGTM! Clean model status refresh after downloads.Single refresh after downloads complete is more efficient than the previous polling approach. The step advancement is correctly gated on all models being downloaded.
| for w in word_timing: | ||
| fw = dict(w) | ||
| dur = w["end"] - w["start"] | ||
| if dur < 0.01: # degenerate zero-duration word | ||
| if fixed: | ||
| fw["start"] = fixed[-1]["end"] + avg_gap | ||
| fw["end"] = fw["start"] + avg_duration |
There was a problem hiding this comment.
Clamp repaired zero-duration words to the next real timestamp.
This rebuilds degenerate words by repeatedly adding avg_gap + avg_duration, but it never caps the run against the next non-degenerate start. A zero-duration cluster in the middle of a phrase can therefore spill past the next anchored word, and pad_audio_to_match_timing() will then clamp that negative gap to zero and lose the pause entirely. Distribute the repaired run inside the available window up to the next real timestamp instead of extending it unboundedly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src-tauri/sidecar/chunked_tts.py` around lines 175 - 181, The repair loop
currently extends degenerate zero-duration words unboundedly; change it to first
find the next non-degenerate word's start (scan word_timing after the current
index for the next entry where end-start >= 0.01) as next_true_start, and if
found clamp and distribute the repaired run to lie entirely before
next_true_start: compute the available_window = next_true_start -
(fixed[-1]["end"] if fixed else word_timing[0]["start"]) and evenly space the
remaining repaired words inside that window using avg_gap/avg_duration as
targets (or fall back to the original avg_gap/avg_duration when no
next_true_start), ensuring each fw["end"] = min(fw["start"] + avg_duration,
next_true_start - tiny_epsilon) and adjusting fw["start"] for subsequent
repaired words so they do not cross next_true_start; this prevents spilling past
the next real timestamp (affects variables word_timing, fw, fixed, avg_gap,
avg_duration and the downstream pad_audio_to_match_timing logic).
| let new_name = if new_name.ends_with(".mp4") { | ||
| new_name | ||
| } else { | ||
| format!("{new_name}.mp4") | ||
| }; |
There was a problem hiding this comment.
Case-sensitive extension check may cause .MP4.mp4 suffix for uppercase extensions.
The ends_with(".mp4") check is case-sensitive, but the frontend strips the extension case-insensitively (/\.mp4$/i). If a filename is video.MP4, the frontend passes "video", and this works. However, if a user types video.MP4 directly (with uppercase), it won't match ends_with(".mp4") and will become video.MP4.mp4.
🐛 Proposed fix for case-insensitive extension check
// Ensure .mp4 extension
- let new_name = if new_name.ends_with(".mp4") {
+ let new_name = if new_name.to_lowercase().ends_with(".mp4") {
new_name
} else {
format!("{new_name}.mp4")
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src-tauri/src/library.rs` around lines 264 - 268, The current suffix check
uses new_name.ends_with(".mp4") which is case-sensitive and can produce
filenames like "video.MP4.mp4"; change the condition to perform a
case-insensitive check (e.g., check new_name.to_lowercase().ends_with(".mp4"))
before appending ".mp4" so the code only appends the extension when the name
does not already have a .mp4 extension in any case; keep the original new_name
casing when returning the value.
| let body = serde_json::json!({ | ||
| "profile_id": profile_id, | ||
| "source_audio_path": input_path_str, | ||
| "original_duration": video_duration.unwrap_or(0.0), | ||
| }); |
There was a problem hiding this comment.
Don't serialize an unknown VC duration as 0.0.
When ffmpeg::probe_duration() fails, process_recording() explicitly says it will fall back to audio duration, but this payload sends original_duration: 0.0 instead. That means the VC branch drops the padding/clipping fallback on exactly the files that could not be probed. Either derive the WAV duration here or omit original_duration when it's unknown so the sidecar can fall back itself.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src-tauri/src/local_tts.rs` around lines 406 - 410, The payload currently
always sets "original_duration" to video_duration.unwrap_or(0.0) which hides
unknown probe failures; update the body construction in process_recording() so
you only include "original_duration" when ffmpeg::probe_duration() returned
Some(value) (or else derive the WAV/audio duration from the input_path_str and
include that), otherwise omit the "original_duration" field entirely so the
sidecar can apply its own audio-duration fallback; look for the JSON assembly
using variables video_duration, input_path_str and profile_id and conditionally
add the field instead of unwrapping to 0.0.
| /** Trigger model download via the dedicated Rust command (handles SSE parsing). */ | ||
| async downloadModel( | ||
| modelName: string, | ||
| onProgress?: (progress: number, status: string) => void | ||
| ): Promise<void> { | ||
| const { invoke, Channel } = await import('@tauri-apps/api/core'); | ||
|
|
||
| type ModelDownloadEvent = | ||
| | { event: 'progress'; data: { progress: number; status: string } } | ||
| | { event: 'complete'; data: { model: string } } | ||
| | { event: 'error'; data: { message: string } }; | ||
|
|
||
| const onEvent = new Channel<ModelDownloadEvent>(); | ||
| onEvent.onmessage = (msg) => { | ||
| if (msg.event === 'progress' && onProgress) { | ||
| onProgress(msg.data.progress, msg.data.status); | ||
| } | ||
| }; | ||
|
|
||
| await invoke('download_model', { model: modelName, onEvent }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that `download_model` buffers the full SSE response before emitting events.
sed -n '58,138p' src-tauri/src/models.rsRepository: davidbzyk/voiceover
Length of output: 2672
Backend SSE buffering prevents live progress updates.
The frontend downloadModel() now relies entirely on Channel callbacks, but the backend at src-tauri/src/models.rs buffers the entire SSE response with response.text().await before processing it. This means all progress events are queued and sent only after the download finishes, causing the UI to hang silently until completion. The Rust handler must stream the response body incrementally rather than buffering it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/voicebox.ts` around lines 58 - 77, The frontend downloadModel uses a
Tauri Channel/onEvent for live SSE progress, but the Rust command handler (the
download_model command in src-tauri/src/models.rs) currently calls
response.text().await which buffers the whole SSE stream and delays events;
change the handler to stream the HTTP response body (e.g., use reqwest's
byte/line stream or async read of response.bytes_stream()) and parse SSE chunks
incrementally, emitting each parsed progress/complete/error event immediately to
the Tauri event/channel sender the frontend expects (so the Channel in
downloadModel receives onmessage calls in real time).
Summary
ELEVENLABS_API_KEYfrom environmentTest plan
cargo test— 126 passedpnpm test— 90 passedsvelte-check— 0 errors