Repository navigation
refactor: architecture cleanup — 6 deferred items from master fix list - #17
Conversation
…pe CSS, tighten CSP Six deferred architecture cleanup items from the master fix list: - Q1+Q2: Extract _do_generate (129 lines) and _do_voice_convert (155 lines) from inline closures to module-level async functions in server.py, using functools.partial to bind parameters for the generation queue protocol. - Q3: Replace 8 per-call reqwest::Client creations with a single shared HttpClient in Tauri managed state. Each call site uses tokio::time::timeout() with its original per-request duration (10s–600s). - Q4: Migrate SidecarState child/data_dir/starting fields from std::sync::Mutex to tokio::sync::Mutex. Replace the 120-iteration polling loop in ensure_running() with tokio::sync::Notify. Port stays std::sync for cheap synchronous reads. - Q8: Move ~150 lines of duplicated CSS (11 class selectors) from settings and create-voice page styles into global app.css. - S6: Route sidecar audio through new get_generation_audio Tauri command returning blob bytes, replacing direct browser HTTP fetch. Remove media-src http://127.0.0.1:* wildcard from CSP.
- Add send_with_timeout to poll_and_download's bare .send() calls (lost 600s coverage when migrating to shared client) - Revoke blob URL before reassignment in create-voice to prevent memory leak - Add log::warn to all error paths in get_generation_audio - Extract validate_generation_id helper, deduplicate from poll_generation and get_generation_audio, add 4 unit tests - Replace gen_id+generations params with set_status callback in both extracted Python functions - Add exc_info=True to generation worker outer catch - Remove dead import soundfile, unnecessary _asyncio alias, unused as e - Handle health_check client build failure with log+return instead of unwrap_or_default
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe changes refactor the TTS system to use shared HTTP client management, extract generation and voice-conversion logic into reusable backend coroutines, migrate audio URL retrieval from localhost HTTP to in-memory blob URLs, transition sidecar state management from synchronous to asynchronous, and consolidate shared CSS styling across components. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src-tauri/src/sidecar.rs (2)
231-242:⚠️ Potential issue | 🟠 MajorClear the stored sidecar state when startup dies early.
This branch removes
childbut leavesportpopulated. After that,get_sidecar_status()can still report a running sidecar on a dead port until some later restart path clears it. Reset the stored startup state before returning this error.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/sidecar.rs` around lines 231 - 242, The branch that handles an unexpected child exit removes state.child but leaves state.port set, causing get_sidecar_status() to report a running sidecar on a stale port; before returning the error in the child.try_wait() Ok(Some(status)) and Err(e) branches, also clear the stored startup state (e.g., set state.port to None and any other startup-related fields) so the port is not left populated—update the block around state.child.lock().await where child.try_wait() is handled to reset state.port (and related startup flags) prior to returning the error.
282-294:⚠️ Potential issue | 🟠 MajorUse
kill().awaitinstead ofstart_kill()to avoid leaving the process unreaped.
start_kill()only sends the SIGKILL signal without waiting for the process to exit; immediately dropping thetokio::process::Childleaves it as a zombie process on Unix. Since this is already an async function, take the child out of the mutex andkill().awaitto both send the signal and wait for process termination before clearing state.🔧 Proposed fix
async fn stop_sidecar_inner(state: &SidecarState) { let mut guard = state.child.lock().await; - if let Some(ref mut child) = *guard { + if let Some(mut child) = guard.take() { log::info!("[sidecar] Stopping child process"); - match child.start_kill() { - Ok(()) => log::info!("[sidecar] Kill signal sent to child process"), - Err(e) => log::warn!("[sidecar] Failed to send kill signal: {}", e), + match child.kill().await { + Ok(()) => log::info!("[sidecar] Child process terminated"), + Err(e) => log::warn!("[sidecar] Failed to terminate child process: {}", e), } } - *guard = None; *state.port.lock().unwrap() = None; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/sidecar.rs` around lines 282 - 294, In stop_sidecar_inner: instead of calling child.start_kill() while holding the mutex and then dropping the Child (which can leave a zombie), take the Child out of state.child (use Option::take on state.child inside the mutex), drop the mutex guard, then call child.kill().await (await termination) and handle/log any errors from the kill/await; finally clear state.port and leave state.child as None. Refer to stop_sidecar_inner, SidecarState, state.child, state.port, and variable child to locate where to change the logic.src-tauri/src/local_tts.rs (1)
403-408:⚠️ Potential issue | 🟠 MajorPass
original_durationin the/voice-convertpayload.
src-tauri/sidecar/server.pyLine 797 still parses this field, and Lines 597-603 only restore converted length when it is> 0. Because the long-input path crossfades chunks on Lines 593-595, omitting the duration here means long conversions can come back short. Either thread the known source duration into this helper or derive it frominput_wavbefore building the JSON.🤖 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 403 - 408, The payload sent to /voice-convert is missing the original_duration field, causing the server to misrestore lengths for long inputs; before building the JSON body in this function, compute or accept the source duration (e.g., derive duration from input_wav or add a parameter like original_duration) and include it in the serde_json::json! payload as "original_duration": original_duration (alongside profile_id and source_audio_path) so server.py can use it to restore converted length.
🤖 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/server.py`:
- Line 514: The log call uses an unnecessary f-string prefix causing Ruff F541;
remove the stray "f" from the logger.info call so the static message in
logger.info(" Truncated sample — omitting transcript to avoid mismatch") is a
plain string (locate the offending line by searching for the logger.info call
with the f prefix).
In `@src-tauri/src/local_tts.rs`:
- Around line 17-27: The current send_with_timeout function only wraps the
Future that yields a Response, which times out only until headers arrive and
leaves body reads unprotected; update callers to use reqwest's
RequestBuilder::timeout (or change send_with_timeout to accept a RequestBuilder
and call .timeout(Duration) before .send()) so the timeout covers the entire
request lifecycle including body reads; adjust call sites like poll_and_download
and get_generation_audio to pass the RequestBuilder (e.g.,
http.client.get(&url).timeout(Duration::from_secs(timeout_secs))) so the timeout
is enforced for connection, headers, and full response body consumption.
In `@src-tauri/src/sidecar.rs`:
- Around line 113-121: The start/launch logic is racy because start_sidecar()
flips SidecarState.starting in a separate critical section from the check in
ensure_running(), allowing two callers to concurrently spawn sidecars; fix this
by making the check-and-set atomic inside a single locked section: acquire
state.starting.lock().await, check the boolean, and if false set it to true and
immediately release (or keep locked through spawn decision) before calling
start_sidecar_inner() or spawning any background task so only one path proceeds
to spawn; update start_sidecar(), ensure_running(), and any other places that
set/read SidecarState.starting (including the background startup task and the
other block referenced) to use this single locked check-and-set pattern and
ensure child/port are only written after the atomic ownership of startup is
established.
In `@src/app.css`:
- Around line 13-148: The global selectors (.header, .section, .provider-toggle,
.provider-btn and their variants) conflict with other routes; rename/namespace
them (e.g., .create-voice-header, .create-voice-section,
.create-voice-provider-toggle, .create-voice-provider-btn and respective
.*-title/.card/.input/.small-btn variants) in this stylesheet and update the
corresponding markup/components that render the settings/create-voice UI to use
the new class names so the styles remain scoped to the intended routes.
In `@src/routes/create-voice/`+page.svelte:
- Around line 249-253: generatedAudioUrl is only revoked inside testGenerate(),
but other code paths (e.g., when the user navigates back to step 3 or leaves the
page) drop the blob URL without revoking it; update every place that clears or
replaces generatedAudioUrl to first call URL.revokeObjectURL(generatedAudioUrl)
if generatedAudioUrl is truthy (or startsWith('blob:')) — specifically add this
revoke call before clearing generatedAudioUrl in the handler that moves the UI
back to step 3 (the function that sets the step/state back) and add a component
cleanup (onDestroy) that revokes generatedAudioUrl before clearing it, ensuring
no blob: URLs are leaked anywhere generatedAudioUrl is set to '' or replaced.
---
Outside diff comments:
In `@src-tauri/src/local_tts.rs`:
- Around line 403-408: The payload sent to /voice-convert is missing the
original_duration field, causing the server to misrestore lengths for long
inputs; before building the JSON body in this function, compute or accept the
source duration (e.g., derive duration from input_wav or add a parameter like
original_duration) and include it in the serde_json::json! payload as
"original_duration": original_duration (alongside profile_id and
source_audio_path) so server.py can use it to restore converted length.
In `@src-tauri/src/sidecar.rs`:
- Around line 231-242: The branch that handles an unexpected child exit removes
state.child but leaves state.port set, causing get_sidecar_status() to report a
running sidecar on a stale port; before returning the error in the
child.try_wait() Ok(Some(status)) and Err(e) branches, also clear the stored
startup state (e.g., set state.port to None and any other startup-related
fields) so the port is not left populated—update the block around
state.child.lock().await where child.try_wait() is handled to reset state.port
(and related startup flags) prior to returning the error.
- Around line 282-294: In stop_sidecar_inner: instead of calling
child.start_kill() while holding the mutex and then dropping the Child (which
can leave a zombie), take the Child out of state.child (use Option::take on
state.child inside the mutex), drop the mutex guard, then call
child.kill().await (await termination) and handle/log any errors from the
kill/await; finally clear state.port and leave state.child as None. Refer to
stop_sidecar_inner, SidecarState, state.child, state.port, and variable child to
locate where to change the logic.
🪄 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: cb7ea487-6966-432d-b4e2-a34cfda37fd0
📒 Files selected for processing (11)
src-tauri/sidecar/server.pysrc-tauri/src/lib.rssrc-tauri/src/local_tts.rssrc-tauri/src/pipeline.rssrc-tauri/src/sidecar.rssrc-tauri/tauri.conf.jsonsrc/app.csssrc/lib/voicebox.test.tssrc/lib/voicebox.tssrc/routes/create-voice/+page.sveltesrc/routes/settings/+page.svelte
💤 Files with no reviewable changes (1)
- src/routes/settings/+page.svelte
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: rust
- GitHub Check: claude-review
🧰 Additional context used
📓 Path-based instructions (11)
src-tauri/src/**/*.rs
📄 CodeRabbit inference engine (GEMINI.md)
Secrets use OS keyring (
keyringcrate), never stored in config files
src-tauri/src/**/*.rs: Backend tests must pass viacargo testbefore task completion
No new clippy warnings should be introduced, verified viacargo clippy --manifest-path src-tauri/Cargo.toml -- -D warnings
For end-to-end feature addition: Backend agent adds Rust command in appropriate module and registers inlib.rs
When adding a new sidecar endpoint: Backend agent adds Rust wrapper inlocal_tts.rsor new module and exposes as Tauri command
src-tauri/src/**/*.rs: Uselog::info!and similar macros for logging on the Rust side
Write Rust tests using#[cfg(test)] mod testsin the same file as the code being tested
Store API keys and tokens via OS keyring using thekeyringcrate; never store secrets in config files
Ensure the TTS sidecar launches automatically on app startup; callsidecar::ensure_running()before any local TTS operation
Files:
src-tauri/src/pipeline.rssrc-tauri/src/lib.rssrc-tauri/src/sidecar.rssrc-tauri/src/local_tts.rs
src-tauri/src/**
📄 CodeRabbit inference engine (AGENTS.md)
Backend agent owns
src-tauri/src/directory and may read but not modify frontend or sidecar files
Files:
src-tauri/src/pipeline.rssrc-tauri/src/lib.rssrc-tauri/src/sidecar.rssrc-tauri/src/local_tts.rs
src-tauri/tauri.conf.json
📄 CodeRabbit inference engine (CLAUDE.md)
Define Content Security Policy (CSP) in
tauri.conf.jsonand update it when adding new external domains
Files:
src-tauri/tauri.conf.json
src/**/*.{ts,tsx,svelte}
📄 CodeRabbit inference engine (GEMINI.md)
All Tauri IPC must go through
src/lib/tauri.tstauriInvoke<T>()wrapper
src/**/*.{ts,tsx,svelte}: Type checking must pass viapnpm checkbefore task completion
For end-to-end feature addition: Frontend agent addstauriInvokecall and updates state/UI
Frontend must call sidecar endpoints viatauriInvoke, never via direct HTTP calls
src/**/*.{ts,tsx,svelte}: Route all backend calls through thetauriInvoke<T>()wrapper intauri.tsinstead of calling Tauri commands directly
Never call sidecar HTTP endpoints directly from frontend code; always route through Rust commands (sidecar_fetch,sidecar_upload)
Uselogger.tswith[VO:tag]prefix pattern for frontend logging
Files:
src/lib/voicebox.tssrc/lib/voicebox.test.tssrc/routes/create-voice/+page.svelte
src/**
📄 CodeRabbit inference engine (AGENTS.md)
Frontend agent owns
src/directory and may read but not modify Rust or Python files
Files:
src/lib/voicebox.tssrc/lib/voicebox.test.tssrc/routes/create-voice/+page.sveltesrc/app.css
src/**/*.test.{ts,tsx,svelte}
📄 CodeRabbit inference engine (AGENTS.md)
Frontend tests must pass via
pnpm testbefore task completion
Files:
src/lib/voicebox.test.ts
src/**/*.{test.ts,svelte.test.ts}
📄 CodeRabbit inference engine (CLAUDE.md)
Co-locate test files with source code as
*.test.tsor*.svelte.test.ts
Files:
src/lib/voicebox.test.ts
src/**/*.svelte
📄 CodeRabbit inference engine (GEMINI.md)
Use Svelte 5 runes only (
$state(),$derived(),$effect()). Never use Svelte 4 syntax ($:,export let,$$props)Use Svelte 5 runes syntax exclusively; do not use Svelte 4 syntax like
$:orexport let
Files:
src/routes/create-voice/+page.svelte
src-tauri/sidecar/**/*.py
📄 CodeRabbit inference engine (GEMINI.md)
Python sidecar targets Apple Silicon only (MLX, no CUDA)
Files:
src-tauri/sidecar/server.py
src-tauri/sidecar/**
📄 CodeRabbit inference engine (AGENTS.md)
Sidecar agent owns
src-tauri/sidecar/directory and may read but not modify Rust or frontend files
Files:
src-tauri/sidecar/server.py
src-tauri/sidecar/server.py
📄 CodeRabbit inference engine (AGENTS.md)
src-tauri/sidecar/server.py: For end-to-end feature addition: Sidecar agent adds FastAPI endpoint inserver.pyif TTS-related
When adding a new sidecar endpoint: implement inserver.pyfirst and test standalone
Files:
src-tauri/sidecar/server.py
🧠 Learnings (15)
📓 Common learnings
Learnt from: CR
Repo: davidbzyk/voiceover PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-02T16:02:35.934Z
Learning: Applies to src-tauri/src/**/*.rs : Ensure the TTS sidecar launches automatically on app startup; call `sidecar::ensure_running()` before any local TTS operation
Learnt from: CR
Repo: davidbzyk/voiceover PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-01T02:22:27.651Z
Learning: Applies to src-tauri/sidecar/server.py : For end-to-end feature addition: Sidecar agent adds FastAPI endpoint in `server.py` if TTS-related
📚 Learning: 2026-04-01T02:22:27.651Z
Learnt from: CR
Repo: davidbzyk/voiceover PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-01T02:22:27.651Z
Learning: Applies to src-tauri/src/**/*.rs : When adding a new sidecar endpoint: Backend agent adds Rust wrapper in `local_tts.rs` or new module and exposes as Tauri command
Applied to files:
src-tauri/src/pipeline.rssrc-tauri/src/lib.rssrc-tauri/src/sidecar.rssrc-tauri/src/local_tts.rs
📚 Learning: 2026-04-02T16:02:35.934Z
Learnt from: CR
Repo: davidbzyk/voiceover PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-02T16:02:35.934Z
Learning: Applies to src-tauri/src/**/*.rs : Ensure the TTS sidecar launches automatically on app startup; call `sidecar::ensure_running()` before any local TTS operation
Applied to files:
src-tauri/src/pipeline.rssrc-tauri/src/lib.rssrc-tauri/src/sidecar.rssrc-tauri/src/local_tts.rs
📚 Learning: 2026-04-01T02:22:13.859Z
Learnt from: CR
Repo: davidbzyk/voiceover PR: 0
File: GEMINI.md:0-0
Timestamp: 2026-04-01T02:22:13.859Z
Learning: Config changes: update `src-tauri/src/config.rs` (Rust struct + persistence)
Applied to files:
src-tauri/src/pipeline.rssrc-tauri/src/lib.rssrc-tauri/tauri.conf.jsonsrc-tauri/src/sidecar.rssrc-tauri/src/local_tts.rs
📚 Learning: 2026-04-01T02:22:13.859Z
Learnt from: CR
Repo: davidbzyk/voiceover PR: 0
File: GEMINI.md:0-0
Timestamp: 2026-04-01T02:22:13.859Z
Learning: New Tauri command: implement in Rust module, register in `src-tauri/src/lib.rs` `invoke_handler`, call via `tauriInvoke` on frontend
Applied to files:
src-tauri/src/lib.rs
📚 Learning: 2026-04-01T02:22:27.651Z
Learnt from: CR
Repo: davidbzyk/voiceover PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-01T02:22:27.651Z
Learning: Tauri commands must be registered in `lib.rs` `invoke_handler` and have corresponding `tauriInvoke` calls on the frontend
Applied to files:
src-tauri/src/lib.rs
📚 Learning: 2026-04-01T02:22:27.651Z
Learnt from: CR
Repo: davidbzyk/voiceover PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-01T02:22:27.651Z
Learning: Applies to src-tauri/src/**/*.rs : For end-to-end feature addition: Backend agent adds Rust command in appropriate module and registers in `lib.rs`
Applied to files:
src-tauri/src/lib.rssrc-tauri/src/sidecar.rs
📚 Learning: 2026-04-01T02:22:13.859Z
Learnt from: CR
Repo: davidbzyk/voiceover PR: 0
File: GEMINI.md:0-0
Timestamp: 2026-04-01T02:22:13.859Z
Learning: New sidecar endpoint: add to `src-tauri/sidecar/server.py`, wrap in Rust command, expose to frontend
Applied to files:
src-tauri/src/lib.rssrc-tauri/src/sidecar.rssrc-tauri/src/local_tts.rs
📚 Learning: 2026-04-02T16:02:35.934Z
Learnt from: CR
Repo: davidbzyk/voiceover PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-02T16:02:35.934Z
Learning: Applies to src-tauri/tauri.conf.json : Define Content Security Policy (CSP) in `tauri.conf.json` and update it when adding new external domains
Applied to files:
src-tauri/tauri.conf.json
📚 Learning: 2026-04-01T02:22:27.651Z
Learnt from: CR
Repo: davidbzyk/voiceover PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-01T02:22:27.651Z
Learning: Applies to src/**/*.{ts,tsx,svelte} : For end-to-end feature addition: Frontend agent adds `tauriInvoke` call and updates state/UI
Applied to files:
src-tauri/tauri.conf.json
📚 Learning: 2026-04-01T02:22:27.651Z
Learnt from: CR
Repo: davidbzyk/voiceover PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-01T02:22:27.651Z
Learning: Applies to src-tauri/sidecar/** : Sidecar agent owns `src-tauri/sidecar/` directory and may read but not modify Rust or frontend files
Applied to files:
src-tauri/src/sidecar.rs
📚 Learning: 2026-04-01T02:22:13.859Z
Learnt from: CR
Repo: davidbzyk/voiceover PR: 0
File: GEMINI.md:0-0
Timestamp: 2026-04-01T02:22:13.859Z
Learning: Frontend never calls sidecar HTTP directly; always through Rust Tauri commands
Applied to files:
src-tauri/src/sidecar.rssrc-tauri/src/local_tts.rs
📚 Learning: 2026-04-01T02:22:27.651Z
Learnt from: CR
Repo: davidbzyk/voiceover PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-01T02:22:27.651Z
Learning: Applies to src-tauri/src/**/*.rs : No new clippy warnings should be introduced, verified via `cargo clippy --manifest-path src-tauri/Cargo.toml -- -D warnings`
Applied to files:
src-tauri/src/sidecar.rs
📚 Learning: 2026-04-01T02:22:27.651Z
Learnt from: CR
Repo: davidbzyk/voiceover PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-01T02:22:27.651Z
Learning: Applies to src-tauri/sidecar/server.py : For end-to-end feature addition: Sidecar agent adds FastAPI endpoint in `server.py` if TTS-related
Applied to files:
src-tauri/sidecar/server.py
📚 Learning: 2026-04-02T16:02:35.934Z
Learnt from: CR
Repo: davidbzyk/voiceover PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-02T16:02:35.934Z
Learning: Applies to src/**/*.{ts,tsx,svelte} : Never call sidecar HTTP endpoints directly from frontend code; always route through Rust commands (`sidecar_fetch`, `sidecar_upload`)
Applied to files:
src-tauri/src/local_tts.rs
🪛 Ruff (0.15.7)
src-tauri/sidecar/server.py
[error] 514-514: f-string without any placeholders
Remove extraneous f prefix
(F541)
🔇 Additional comments (5)
src-tauri/tauri.conf.json (1)
26-26: CSP tightening looks good.Dropping
http://127.0.0.1:*frommedia-srcmatches the new blob-based audio path and narrows the policy. As per coding guidelines: "Define Content Security Policy (CSP) intauri.conf.jsonand update it when adding new external domains"src-tauri/src/pipeline.rs (1)
170-190: Nice: the local path still goes throughensure_running()before using the shared client.This keeps the sidecar-startup contract intact while removing per-call HTTP client churn. Based on learnings: "Ensure the TTS sidecar launches automatically on app startup; call
sidecar::ensure_running()before any local TTS operation"src-tauri/sidecar/server.py (1)
738-748: Nice endpoint slimming.
functools.partial(...)plus theset_statuscallback keeps/generateand/voice-convertfocused on queue orchestration while preserving progress updates.Also applies to: 812-820
src-tauri/src/local_tts.rs (2)
613-656: Good IPC boundary for sidecar audio.Returning raw bytes here keeps the frontend off direct
127.0.0.1media URLs while still reusing the sidecar’s/audio/{id}contract.Based on learnings: Frontend never calls sidecar HTTP directly; always through Rust Tauri commands.
870-888: Good regression coverage for the new ID validator.These tests hit the obvious traversal and shell-metacharacter inputs the helper is supposed to reject.
| elif was_truncated: | ||
| # Audio was truncated — omit transcript to avoid text/audio mismatch | ||
| all_have_text = False | ||
| logger.info(f" Truncated sample — omitting transcript to avoid mismatch") |
There was a problem hiding this comment.
Drop the stray f prefix on the static log line.
Line 514 triggers Ruff F541, so this will fail lint even though the message itself is fine.
🔧 Minimal fix
- logger.info(f" Truncated sample — omitting transcript to avoid mismatch")
+ logger.info(" Truncated sample — omitting transcript to avoid mismatch")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| logger.info(f" Truncated sample — omitting transcript to avoid mismatch") | |
| logger.info(" Truncated sample — omitting transcript to avoid mismatch") |
🧰 Tools
🪛 Ruff (0.15.7)
[error] 514-514: f-string without any placeholders
Remove extraneous f prefix
(F541)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src-tauri/sidecar/server.py` at line 514, The log call uses an unnecessary
f-string prefix causing Ruff F541; remove the stray "f" from the logger.info
call so the static message in logger.info(" Truncated sample — omitting
transcript to avoid mismatch") is a plain string (locate the offending line by
searching for the logger.info call with the f prefix).
| /// Send an HTTP request with a per-call timeout. | ||
| async fn send_with_timeout( | ||
| future: impl std::future::Future<Output = Result<reqwest::Response, reqwest::Error>>, | ||
| timeout_secs: u64, | ||
| context: &str, | ||
| ) -> Result<reqwest::Response, String> { | ||
| tokio::time::timeout(std::time::Duration::from_secs(timeout_secs), future) | ||
| .await | ||
| .map_err(|_| format!("{context}: timed out after {timeout_secs}s"))? | ||
| .map_err(|e| format!("{context}: {e}")) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In reqwest, does wrapping only RequestBuilder::send()intokio::time::timeout()also time out reading the response body from.text(), .json(), or .bytes()? How does that compare with RequestBuilder::timeout()/ClientBuilder::timeout()?
💡 Result:
No, wrapping only RequestBuilder::send in tokio::time::timeout does NOT time out reading the response body from .text, .json, or .bytes. The send future completes once the response headers are received and the Response object is returned, but reading the body (which can be streamed/chunked) happens separately in subsequent calls like .text.await, which are not covered by that timeout. This can lead to hangs if the body transfer stalls. In contrast: - RequestBuilder::timeout applies a timeout from connection start through complete response body reading, covering both send and body consumption like .text. - ClientBuilder::timeout sets a default timeout for all requests from that client, with the same coverage (connect + full response), but can be overridden per-request. To time out the full request including body reading with tokio::time::timeout, wrap the entire chain: tokio::time::timeout(duration, async { let res = builder.send.await?; res.text.await // or .json, etc. }) This ensures timeout protection for both phases. Official docs confirm RequestBuilder::timeout: "The timeout is applied from when the request starts connecting until the response body has finished." Multiple GitHub issues and Stack Overflow posts highlight body reading hangs post-send without additional timeouts (e.g., slow chunked responses).
Citations:
- 1: https://stackoverflow.com/questions/73010896/tokiotimetimeout-and-reqwests-cant-wrap-my-future-in-a-timeout
- 2: https://docs.rs/reqwest/latest/
- 3: https://docs.rs/reqwest/0.10.8/reqwest/struct.RequestBuilder.html
- 4: https://docs.rs/reqwest/0.10.5/reqwest/struct.RequestBuilder.html
- 5: https://docs.rs/reqwest/0.11.27/reqwest/struct.RequestBuilder.html
- 6: response err.is_timeout() can't satisfy reqwest::Error { kind: Body, source: TimedOut } case seanmonstar/reqwest#2076
- 7:
.json()timed out when parsing large response seanmonstar/reqwest#2153 - 8: Requests with timeouts only respect them for the first read seanmonstar/reqwest#1380
send_with_timeout only times the request head, leaving body reads unprotected.
Wrapping only .send() in tokio::time::timeout() stops once the Response exists; every later .text(), .json(), and .bytes() read is still unbounded. A stalled sidecar body will hang poll_and_download() and get_generation_audio(). This is a regression from the old reqwest request-timeout behavior, which covers both the request headers and complete response body reading.
Use RequestBuilder::timeout() instead, which applies timeout protection from connection start through entire response body consumption:
Safer implementation
-async fn send_with_timeout(
- future: impl std::future::Future<Output = Result<reqwest::Response, reqwest::Error>>,
+async fn send_with_timeout(
+ request: reqwest::RequestBuilder,
timeout_secs: u64,
context: &str,
) -> Result<reqwest::Response, String> {
- tokio::time::timeout(std::time::Duration::from_secs(timeout_secs), future)
- .await
- .map_err(|_| format!("{context}: timed out after {timeout_secs}s"))?
- .map_err(|e| format!("{context}: {e}"))
+ request
+ .timeout(std::time::Duration::from_secs(timeout_secs))
+ .send()
+ .await
+ .map_err(|e| {
+ if e.is_timeout() {
+ format!("{context}: timed out after {timeout_secs}s")
+ } else {
+ format!("{context}: {e}")
+ }
+ })
}Call sites:
let response = send_with_timeout(http.client.get(&url), 10, "Failed to fetch voice profiles").await?;🤖 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 17 - 27, The current
send_with_timeout function only wraps the Future that yields a Response, which
times out only until headers arrive and leaves body reads unprotected; update
callers to use reqwest's RequestBuilder::timeout (or change send_with_timeout to
accept a RequestBuilder and call .timeout(Duration) before .send()) so the
timeout covers the entire request lifecycle including body reads; adjust call
sites like poll_and_download and get_generation_audio to pass the RequestBuilder
(e.g., http.client.get(&url).timeout(Duration::from_secs(timeout_secs))) so the
timeout is enforced for connection, headers, and full response body consumption.
| pub async fn start_sidecar(app: &tauri::AppHandle) -> Result<u16, String> { | ||
| let state = app.state::<SidecarState>(); | ||
| *state.starting.lock().unwrap() = true; | ||
| *state.starting.lock().await = true; | ||
|
|
||
| let result = start_sidecar_inner(app).await; | ||
|
|
||
| let state = app.state::<SidecarState>(); | ||
| *state.starting.lock().unwrap() = false; | ||
| *state.starting.lock().await = false; | ||
| state.startup_done.notify_waiters(); |
There was a problem hiding this comment.
starting is still racy; two callers can launch two sidecars.
ensure_running() reads starting, but start_sidecar() flips it in a separate critical section. The background startup task in src-tauri/src/lib.rs:54-58 can overlap with a foreground ensure_running(), so both paths can observe false, both spawn, and whichever finishes last overwrites child/port. Please make the check-and-set atomic in one place before any spawn happens.
Also applies to: 304-346
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src-tauri/src/sidecar.rs` around lines 113 - 121, The start/launch logic is
racy because start_sidecar() flips SidecarState.starting in a separate critical
section from the check in ensure_running(), allowing two callers to concurrently
spawn sidecars; fix this by making the check-and-set atomic inside a single
locked section: acquire state.starting.lock().await, check the boolean, and if
false set it to true and immediately release (or keep locked through spawn
decision) before calling start_sidecar_inner() or spawning any background task
so only one path proceeds to spawn; update start_sidecar(), ensure_running(),
and any other places that set/read SidecarState.starting (including the
background startup task and the other block referenced) to use this single
locked check-and-set pattern and ensure child/port are only written after the
atomic ownership of startup is established.
| /* --------------------------------------------------------------------------- | ||
| Shared page styles — used by settings and create-voice routes | ||
| --------------------------------------------------------------------------- */ | ||
|
|
||
| .header { | ||
| display: flex; | ||
| align-items: center; | ||
| gap: 12px; | ||
| } | ||
| .header h2 { | ||
| margin: 0; | ||
| font-size: 18px; | ||
| } | ||
| .back-btn { | ||
| background: #334155; | ||
| border: none; | ||
| color: #94a3b8; | ||
| padding: 6px 12px; | ||
| border-radius: 6px; | ||
| cursor: pointer; | ||
| font-size: 13px; | ||
| } | ||
| .back-btn:hover { | ||
| background: #475569; | ||
| color: #f1f5f9; | ||
| } | ||
| .section { | ||
| display: flex; | ||
| flex-direction: column; | ||
| gap: 8px; | ||
| } | ||
| .section-title { | ||
| font-size: 13px; | ||
| font-weight: 600; | ||
| } | ||
| .card { | ||
| background: #1e293b; | ||
| border-radius: 8px; | ||
| padding: 14px; | ||
| display: flex; | ||
| flex-direction: column; | ||
| gap: 10px; | ||
| } | ||
| .field-label { | ||
| font-size: 11px; | ||
| color: #64748b; | ||
| } | ||
| .input { | ||
| background: #0f172a; | ||
| border: 1px solid #334155; | ||
| border-radius: 6px; | ||
| padding: 8px 12px; | ||
| color: #cbd5e1; | ||
| font-size: 12px; | ||
| outline: none; | ||
| width: 100%; | ||
| } | ||
| .input:focus { | ||
| border-color: #f97316; | ||
| } | ||
| select.input { | ||
| appearance: none; | ||
| background-image: url("data:image/svg+xml,%3Csvg xmlns='http://www.w3.org/2000/svg' width='12' height='12' fill='%2364748b' viewBox='0 0 16 16'%3E%3Cpath d='M8 11L3 6h10z'/%3E%3C/svg%3E"); | ||
| background-repeat: no-repeat; | ||
| background-position: right 10px center; | ||
| padding-right: 28px; | ||
| cursor: pointer; | ||
| } | ||
| select.input option { | ||
| background: #0f172a; | ||
| color: #cbd5e1; | ||
| } | ||
| .small-btn { | ||
| background: #334155; | ||
| border: none; | ||
| color: #94a3b8; | ||
| padding: 6px 10px; | ||
| border-radius: 6px; | ||
| cursor: pointer; | ||
| font-size: 12px; | ||
| white-space: nowrap; | ||
| } | ||
| .small-btn:hover { | ||
| background: #475569; | ||
| } | ||
| .small-btn:disabled { | ||
| opacity: 0.4; | ||
| cursor: not-allowed; | ||
| } | ||
| .small-btn.accent { | ||
| background: #f97316; | ||
| color: white; | ||
| } | ||
| .small-btn.accent:disabled { | ||
| opacity: 0.4; | ||
| } | ||
| .status { | ||
| font-size: 11px; | ||
| } | ||
| .status.valid { | ||
| color: #22c55e; | ||
| } | ||
| .status.invalid { | ||
| color: #ef4444; | ||
| } | ||
| .hint-text { | ||
| font-size: 11px; | ||
| color: #64748b; | ||
| line-height: 1.4; | ||
| } | ||
| .provider-toggle { | ||
| display: flex; | ||
| gap: 4px; | ||
| background: #0f172a; | ||
| border-radius: 6px; | ||
| padding: 3px; | ||
| } | ||
| .provider-btn { | ||
| flex: 1; | ||
| background: transparent; | ||
| border: none; | ||
| color: #64748b; | ||
| padding: 8px 12px; | ||
| border-radius: 4px; | ||
| cursor: pointer; | ||
| font-size: 13px; | ||
| font-weight: 500; | ||
| transition: all 0.15s; | ||
| } | ||
| .provider-btn.active { | ||
| background: #334155; | ||
| color: #f1f5f9; | ||
| } | ||
| .provider-btn:hover:not(.active) { | ||
| color: #94a3b8; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Expected: no output. Any match is a candidate global-style collision.
rg -nP --glob 'src/**/*.svelte' 'class\s*=\s*"[^"]*\b(header|back-btn|section|section-title|card|field-label|input|small-btn|status|hint-text|provider-toggle|provider-btn)\b[^"]*"' \
| grep -vE 'src/routes/(create-voice/\+page\.svelte|settings/\+page\.svelte):' || trueRepository: davidbzyk/voiceover
Length of output: 891
Namespace these selectors before applying them globally—verified collisions with existing routes.
The added CSS rules for .header, .section, .provider-toggle, and .provider-btn are already in use across other routes:
.headerin preview route.section,.provider-toggle, and.provider-btnin home route
Moving these from scoped styles to the global stylesheet will apply the settings/create-voice styling to these routes unintentionally. Prefix or scope these selectors (e.g., .create-voice-header, .settings-header) before placing them in app.css.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app.css` around lines 13 - 148, The global selectors (.header, .section,
.provider-toggle, .provider-btn and their variants) conflict with other routes;
rename/namespace them (e.g., .create-voice-header, .create-voice-section,
.create-voice-provider-toggle, .create-voice-provider-btn and respective
.*-title/.card/.input/.small-btn variants) in this stylesheet and update the
corresponding markup/components that render the settings/create-voice UI to use
the new class names so the styles remain scoped to the intended routes.
| async function testGenerate() { | ||
| generating = true; | ||
| generateError = ''; | ||
| if (generatedAudioUrl) URL.revokeObjectURL(generatedAudioUrl); | ||
| generatedAudioUrl = ''; |
There was a problem hiding this comment.
This only fixes one blob-URL leak path.
generatedAudioUrl is revoked before regeneration, but it is still cleared without cleanup when the user goes back to step 3 (Line 566) or leaves the page. Now that src/lib/voicebox.ts:123-126 returns blob: URLs, every path that drops the string needs to revoke it.
♻️ Suggested cleanup
-import { onMount } from 'svelte';
+import { onDestroy, onMount } from 'svelte';
@@
onMount(() => {
checkPrerequisites();
});
+
+onDestroy(() => {
+ if (generatedAudioUrl) URL.revokeObjectURL(generatedAudioUrl);
+});
@@
- <button class="small-btn" onclick={() => { generatedAudioUrl = ''; step = 3; }}>
+ <button class="small-btn" onclick={() => {
+ if (generatedAudioUrl) URL.revokeObjectURL(generatedAudioUrl);
+ generatedAudioUrl = '';
+ step = 3;
+ }}>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/routes/create-voice/`+page.svelte around lines 249 - 253,
generatedAudioUrl is only revoked inside testGenerate(), but other code paths
(e.g., when the user navigates back to step 3 or leaves the page) drop the blob
URL without revoking it; update every place that clears or replaces
generatedAudioUrl to first call URL.revokeObjectURL(generatedAudioUrl) if
generatedAudioUrl is truthy (or startsWith('blob:')) — specifically add this
revoke call before clearing generatedAudioUrl in the handler that moves the UI
back to step 3 (the function that sets the step/state back) and add a component
cleanup (onDestroy) that revokes generatedAudioUrl before clearing it, ensuring
no blob: URLs are leaked anywhere generatedAudioUrl is set to '' or replaced.
1 participant
Summary
_do_generate(129 lines) and_do_voice_convert(155 lines) from inline closures to module-level functions usingfunctools.partialandset_statuscallbackreqwest::Clientcreations with sharedHttpClientin Tauri state, per-requesttokio::time::timeout()SidecarStatechild/data_dir/starting totokio::sync::Mutex, replace polling loop withtokio::sync::Notifyapp.cssget_generation_audioTauri command (blob), removemedia-src http://127.0.0.1:*from CSPReview fixes (second commit)
send_with_timeouttopoll_and_downloadbare.send()callslog::warntoget_generation_audioerror pathsvalidate_generation_idhelper + 4 unit testsgen_id+generationsparams withset_statuscallbackhealth_checkclient build failure gracefullyTest plan
cargo clippy -- -D warningscleanpnpm check— 0 errors, 0 warnings