From 0d0b62ea933aba844682650e9b99fc941c28ccac Mon Sep 17 00:00:00 2001 From: James Pine Date: Mon, 16 Mar 2026 03:12:01 -0700 Subject: [PATCH] address CodeRabbit review: fix 4 critical + 12 major issues Critical: - Remove dead backend.utils.validation PyInstaller hidden import - Fix story_items table rebuild to preserve track/trim/version columns - Guard cache migration against same source/destination path - Fix regeneration audio overwrite (use random uuid suffix per take) Major: - Engine selector: validate language on Qwen switch, clear stale modelSize - Sync language validation regex between profile create and generate (22 langs) - Guard CUDA download against duplicate concurrent requests - Only set model_size for engines that support multiple sizes - Fix 404 swallowed by generic except in history export - Validate audio_path before FileResponse in export-audio - Transcription: stream uploads in 1MB chunks, use robust cache check, call complete_download() on Whisper download success - Set clean version as default when effects chain validation fails - Return explicit error when Windows port occupied by non-voicebox process --- .../Generation/EngineModelSelector.tsx | 7 ++++ backend/build_binary.py | 2 +- backend/database/migrations.py | 10 ++++-- backend/models.py | 2 +- backend/routes/cuda.py | 5 +++ backend/routes/generations.py | 2 +- backend/routes/history.py | 35 ++++++++++--------- backend/routes/models.py | 3 ++ backend/routes/transcription.py | 23 +++++------- backend/services/generation.py | 10 ++++-- tauri/src-tauri/src/main.rs | 8 ++++- 11 files changed, 69 insertions(+), 38 deletions(-) diff --git a/app/src/components/Generation/EngineModelSelector.tsx b/app/src/components/Generation/EngineModelSelector.tsx index fc58e828..77dac03f 100644 --- a/app/src/components/Generation/EngineModelSelector.tsx +++ b/app/src/components/Generation/EngineModelSelector.tsx @@ -42,8 +42,15 @@ function handleEngineChange(form: UseFormReturn, value: st const [, modelSize] = value.split(':'); form.setValue('engine', 'qwen'); form.setValue('modelSize', modelSize as '1.7B' | '0.6B'); + // Validate language is supported by Qwen + const currentLang = form.getValues('language'); + const available = getLanguageOptionsForEngine('qwen'); + if (!available.some((l) => l.value === currentLang)) { + form.setValue('language', available[0]?.value ?? 'en'); + } } else { form.setValue('engine', value as GenerationFormValues['engine']); + form.setValue('modelSize', undefined as unknown as '1.7B' | '0.6B'); if (ENGLISH_ONLY_ENGINES.has(value)) { form.setValue('language', 'en'); } else { diff --git a/backend/build_binary.py b/backend/build_binary.py index 6f8b1736..005bbe25 100644 --- a/backend/build_binary.py +++ b/backend/build_binary.py @@ -66,7 +66,7 @@ def build_server(cuda=False): '--hidden-import', 'backend.utils.cache', '--hidden-import', 'backend.utils.progress', '--hidden-import', 'backend.utils.hf_progress', - '--hidden-import', 'backend.utils.validation', + '--hidden-import', 'backend.services.cuda', '--hidden-import', 'backend.services.effects', '--hidden-import', 'backend.utils.effects', diff --git a/backend/database/migrations.py b/backend/database/migrations.py index 56e0bf2b..158b38c6 100644 --- a/backend/database/migrations.py +++ b/backend/database/migrations.py @@ -92,14 +92,20 @@ def _migrate_story_items(engine, inspector, tables: set[str]) -> None: story_id VARCHAR NOT NULL, generation_id VARCHAR NOT NULL, start_time_ms INTEGER NOT NULL DEFAULT 0, + track INTEGER NOT NULL DEFAULT 0, + trim_start_ms INTEGER NOT NULL DEFAULT 0, + trim_end_ms INTEGER NOT NULL DEFAULT 0, + version_id VARCHAR, created_at DATETIME, FOREIGN KEY (story_id) REFERENCES stories(id), FOREIGN KEY (generation_id) REFERENCES generations(id) ) """)) conn.execute(text(""" - INSERT INTO story_items_new (id, story_id, generation_id, start_time_ms, created_at) - SELECT id, story_id, generation_id, start_time_ms, created_at FROM story_items + INSERT INTO story_items_new (id, story_id, generation_id, start_time_ms, track, trim_start_ms, trim_end_ms, version_id, created_at) + SELECT id, story_id, generation_id, start_time_ms, + COALESCE(track, 0), COALESCE(trim_start_ms, 0), COALESCE(trim_end_ms, 0), version_id, created_at + FROM story_items """)) conn.execute(text("DROP TABLE story_items")) conn.execute(text("ALTER TABLE story_items_new RENAME TO story_items")) diff --git a/backend/models.py b/backend/models.py index 4814c7e5..ef8e196d 100644 --- a/backend/models.py +++ b/backend/models.py @@ -64,7 +64,7 @@ class GenerationRequest(BaseModel): profile_id: str text: str = Field(..., min_length=1, max_length=50000) - language: str = Field(default="en", pattern="^(zh|en|ja|ko|de|fr|ru|pt|es|it|he)$") + language: str = Field(default="en", pattern="^(zh|en|ja|ko|de|fr|ru|pt|es|it|he|ar|da|el|fi|hi|ms|nl|no|pl|sv|sw|tr)$") seed: Optional[int] = Field(None, ge=0) model_size: Optional[str] = Field(default="1.7B", pattern="^(1\\.7B|0\\.6B)$") instruct: Optional[str] = Field(None, max_length=500) diff --git a/backend/routes/cuda.py b/backend/routes/cuda.py index 9c6e1cd3..cd9d5766 100644 --- a/backend/routes/cuda.py +++ b/backend/routes/cuda.py @@ -29,6 +29,11 @@ async def download_cuda_backend(): if cuda.get_cuda_binary_path() is not None: raise HTTPException(status_code=409, detail="CUDA backend already downloaded") + progress_manager = get_progress_manager() + existing = progress_manager.get_progress(cuda.PROGRESS_KEY) + if existing and existing.get("status") == "downloading": + raise HTTPException(status_code=409, detail="CUDA backend download already in progress") + async def _download(): try: await cuda.download_cuda_binary() diff --git a/backend/routes/generations.py b/backend/routes/generations.py index d9f4ef29..160b0aa1 100644 --- a/backend/routes/generations.py +++ b/backend/routes/generations.py @@ -33,7 +33,7 @@ async def generate_speech( from ..backends import engine_has_model_sizes engine = data.engine or "qwen" - model_size = data.model_size or "1.7B" + model_size = (data.model_size or "1.7B") if engine_has_model_sizes(engine) else None generation = await history.create_generation( profile_id=data.profile_id, diff --git a/backend/routes/history.py b/backend/routes/history.py index 5ae438b2..5435e299 100644 --- a/backend/routes/history.py +++ b/backend/routes/history.py @@ -126,28 +126,28 @@ async def export_generation( db: Session = Depends(get_db), ): """Export a generation as a ZIP archive.""" + generation = db.query(DBGeneration).filter_by(id=generation_id).first() + if not generation: + raise HTTPException(status_code=404, detail="Generation not found") + try: - generation = db.query(DBGeneration).filter_by(id=generation_id).first() - if not generation: - raise HTTPException(status_code=404, detail="Generation not found") - zip_bytes = export_import.export_generation_to_zip(generation_id, db) - - safe_text = "".join(c for c in generation.text[:30] if c.isalnum() or c in (" ", "-", "_")).strip() - if not safe_text: - safe_text = "generation" - filename = f"generation-{safe_text}.voicebox.zip" - - return StreamingResponse( - io.BytesIO(zip_bytes), - media_type="application/zip", - headers={"Content-Disposition": safe_content_disposition("attachment", filename)}, - ) except ValueError as e: raise HTTPException(status_code=400, detail=str(e)) except Exception as e: raise HTTPException(status_code=500, detail=str(e)) + safe_text = "".join(c for c in generation.text[:30] if c.isalnum() or c in (" ", "-", "_")).strip() + if not safe_text: + safe_text = "generation" + filename = f"generation-{safe_text}.voicebox.zip" + + return StreamingResponse( + io.BytesIO(zip_bytes), + media_type="application/zip", + headers={"Content-Disposition": safe_content_disposition("attachment", filename)}, + ) + @router.get("/history/{generation_id}/export-audio") async def export_generation_audio( @@ -159,8 +159,11 @@ async def export_generation_audio( if not generation: raise HTTPException(status_code=404, detail="Generation not found") + if not generation.audio_path: + raise HTTPException(status_code=404, detail="Generation has no audio file") + audio_path = Path(generation.audio_path) - if not audio_path.exists(): + if not audio_path.is_file(): raise HTTPException(status_code=404, detail="Audio file not found") safe_text = "".join(c for c in generation.text[:30] if c.isalnum() or c in (" ", "-", "_")).strip() diff --git a/backend/routes/models.py b/backend/routes/models.py index 5706c7ad..573c3c53 100644 --- a/backend/routes/models.py +++ b/backend/routes/models.py @@ -129,6 +129,9 @@ async def migrate_models(request: models.ModelMigrateRequest): if not source.exists(): raise HTTPException(status_code=404, detail="Current model cache directory not found") + if source.resolve() == destination.resolve(): + raise HTTPException(status_code=400, detail="Source and destination are the same directory") + model_dirs = [d for d in source.iterdir() if d.name.startswith("models--") and d.is_dir()] if not model_dirs: return {"moved": 0, "errors": [], "source": str(source), "destination": str(destination)} diff --git a/backend/routes/transcription.py b/backend/routes/transcription.py index b5008c82..90cb1c95 100644 --- a/backend/routes/transcription.py +++ b/backend/routes/transcription.py @@ -13,6 +13,8 @@ from ..utils.tasks import get_task_manager router = APIRouter() +UPLOAD_CHUNK_SIZE = 1024 * 1024 # 1MB + @router.post("/transcribe", response_model=models.TranscriptionResponse) async def transcribe_audio( @@ -21,8 +23,8 @@ async def transcribe_audio( ): """Transcribe audio file to text.""" with tempfile.NamedTemporaryFile(suffix=".wav", delete=False) as tmp: - content = await file.read() - tmp.write(content) + while chunk := await file.read(UPLOAD_CHUNK_SIZE): + tmp.write(chunk) tmp_path = tmp.name try: @@ -32,27 +34,20 @@ async def transcribe_audio( duration = len(audio) / sr whisper_model = transcribe.get_whisper_model() - model_size = whisper_model.model_size - whisper_hf_repos = { - "large": "openai/whisper-large-v3", - "turbo": "openai/whisper-large-v3-turbo", - } - model_name = whisper_hf_repos.get(model_size, f"openai/whisper-{model_size}") - from huggingface_hub import constants as hf_constants - - repo_cache = Path(hf_constants.HF_HUB_CACHE) / ("models--" + model_name.replace("/", "--")) - if not repo_cache.exists(): + if not whisper_model.is_loaded() and not whisper_model._is_model_cached(model_size): progress_model_name = f"whisper-{model_size}" + task_manager = get_task_manager() async def download_whisper_background(): try: await whisper_model.load_model_async(model_size) + task_manager.complete_download(progress_model_name) except Exception as e: - get_task_manager().error_download(progress_model_name, str(e)) + task_manager.error_download(progress_model_name, str(e)) - get_task_manager().start_download(progress_model_name) + task_manager.start_download(progress_model_name) create_background_task(download_whisper_background()) raise HTTPException( diff --git a/backend/services/generation.py b/backend/services/generation.py index 8ffca53e..ff86a0de 100644 --- a/backend/services/generation.py +++ b/backend/services/generation.py @@ -176,7 +176,11 @@ def _save_generate( error_msg = validate_effects_chain(effects_chain) if error_msg: - print(f"Warning: invalid effects chain, skipping: {error_msg}") + import logging + logging.getLogger(__name__).warning("invalid effects chain, skipping: %s", error_msg) + versions_mod.set_default_version( + versions_mod.list_versions(generation_id, db)[0].id, db + ) else: processed_audio = apply_effects(audio, sample_rate, effects_chain) processed_path = config.get_generations_dir() / f"{generation_id}_processed.wav" @@ -225,7 +229,9 @@ def _save_regenerate( """ from . import versions as versions_mod - suffix = version_id[:8] if version_id else generation_id[:8] + import uuid as _uuid + + suffix = _uuid.uuid4().hex[:8] audio_path = config.get_generations_dir() / f"{generation_id}_{suffix}.wav" save_audio(audio, str(audio_path), sample_rate) diff --git a/tauri/src-tauri/src/main.rs b/tauri/src-tauri/src/main.rs index 209b4d90..559a36d3 100644 --- a/tauri/src-tauri/src/main.rs +++ b/tauri/src-tauri/src/main.rs @@ -113,11 +113,17 @@ async fn start_server( &format!("127.0.0.1:{}", SERVER_PORT).parse().unwrap(), std::time::Duration::from_secs(1), ).is_ok() { - // Port is in use — check if it's a voicebox process via tasklist + // Port is in use — check if it's a voicebox process if let Some(pid) = find_voicebox_pid_on_port(SERVER_PORT) { println!("Found existing voicebox-server on port {} (PID: {}), reusing it", SERVER_PORT, pid); *state.server_pid.lock().unwrap() = Some(pid); return Ok(format!("http://127.0.0.1:{}", SERVER_PORT)); + } else { + return Err(format!( + "Port {} is already in use by another application. \ + Close the other application or change the Voicebox port.", + SERVER_PORT + )); } } }