Stability pass: DB leaks, App.jsx hooks refactor, desktop bootstrap - #49
Merged
Conversation
…syncio API ## DB Connection Leaks (P0) - Convert 38 raw get_db() calls to db_conn() context manager across 14 router files - Connections are now guaranteed to close even when exceptions are raised - profiles.py create_profile: clean up orphaned audio file if DB insert fails - profiles.py lock_profile: consolidate 3 separate conn.close() error paths ## Race Condition (P1) - Add _dub_jobs_lock (threading.Lock) to protect _dub_jobs dict in dub_pipeline.py - get_job/put_job now thread-safe for concurrent dub sessions ## asyncio Deprecation (P2) - Replace 23 asyncio.get_event_loop() calls with asyncio.get_running_loop() - Prevents DeprecationWarning on Python 3.12+ and future breakage on 3.14 ## Quick Fixes - gallery.py preview_voice: remove filesystem path from error response (P2) - dub_pipeline.py parse_vtt_segments: remove redundant `import re` inside loop (P3) - gallery.py _init_gallery_db: use db_conn() context manager (P2)
## Frontend - Extract useTTS hook (150 LOC) — TTS generation, streaming, audio ingestion - Extract useProfiles hook (219 LOC) — voice profile CRUD, lock/unlock, preview - Centralize isTauri detection: dialog.js, VoiceGallery.jsx, Settings.jsx now import from utils/media.js instead of 4 different detection patterns ## Backend - Add pytest-cov to dev dependencies - Baseline coverage: 39% across backend/ (214 tests pass) - Add .coverage to .gitignore
## Frontend Testing (new) - Set up Vitest with jsdom environment + @testing-library/react - 11 tests: utils (isTauri, formatTime, constants) + Zustand store (mode, text, dubStep, pill) - Scripts: 'test' (vitest run), 'test:watch' (vitest), 'test:legacy' (node runner) ## App.jsx Decomposition (continued) - Extract useDubWorkflow hook (387 LOC) — upload, ingest, transcribe SSE, translate, generate SSE, abort, stop, cleanup - Extract useAppData hook (181 LOC) — data loading, localStorage persistence, WebSocket real-time updates, model-status pill management ## TypeScript checkJs - Enable checkJs: true in tsconfig.json for IDE-level type checking - 947 existing errors (informational, not blocking builds) - noImplicitAny remains false to avoid blocking
## CI - Add 'Run Vitest (frontend)' step — runs 11 unit tests - Override --checkJs false in CI typecheck to avoid 947 pre-existing errors - Rename legacy test step for clarity ## Hooks - Fix useProfiles to accept loadProfiles from parent (useAppData) instead of managing its own duplicate profiles array
App.jsx now delegates to extracted hooks instead of inline logic: - useAppData: data loading, localStorage, WebSocket, model pill - useProfiles: voice profile CRUD, lock/unlock, preview - useTTS: generation, streaming, audio ingestion - useDubWorkflow: upload, transcribe SSE, translate, generate SSE 988 lines removed. All handler logic lives in focused, independently testable hooks. Store selectors and render JSX stay in App.jsx as the shell. Verified: vite build clean, 11 frontend + 214 backend tests pass.
Backend: register hf_progress listener during _load_model_sync() so download/weight-loading tqdm events update _loading_detail with a progress percentage (0-99%). get_model_status() now includes a 'progress' field that the frontend polls. Frontend: useAppData reads msQuery.data.progress and calls setPillProgress() — the FloatingPill already renders the percentage text and progress bar width from this value.
transformers >=4.52 calls _can_set_experts_implementation() and _can_set_attn_implementation() during PreTrainedModel.__init__, which open the class source file via open(class_file). In a Tauri desktop bundle, module.__file__ points to a path that doesn't exist on disk, causing: FileNotFoundError: .../omnivoice/models/omnivoice.py Override both classmethods on OmniVoice to return static values without filesystem access. OmniVoice doesn't use MoE experts (return False), but does support flex/flash attn (return True).
The Tauri bootstrap previously only copied omnivoice/ and backend/ to Application Support on the first run. Subsequent app updates kept using stale source files, preventing bug fixes from landing. Now ensure_venv_ready() always syncs both directories from the bundle resources before returning, even when the venv is healthy. This fixes the FileNotFoundError crash where the old omnivoice.py lacked the _can_set_experts_implementation override.
- Primary button: solid gradient fill with hover glow + lift + press - Stepper nav: connected pills with glow ring on active step - Welcome cards: glassmorphism with stagger-in animations, lucide icons, left-border accent strip, hover translate - Preflight panel: colored icon pill backgrounds, stagger-slide entrance - Step transitions: fade+slide animation via keyed wrapper - Footnote: shortened paths (~/ notation), Reveal in Finder button - Recommendation banner: gradient background with accent glow - Compact spacing throughout for denser, professional layout
When clean_and_retry_bootstrap removes the project dir, any old uvicorn process still running from the deleted paths remains alive on port 3900. The subsequent retry_bootstrap sees the port is healthy and attaches to the zombie instead of re-bootstrapping. Now explicitly kill any process on the backend port after cleaning, before calling retry_bootstrap.
…m environment variables for subprocesses, and improve FFMPEG binary path resolution.
Contributor
📝 WalkthroughWalkthroughThis PR standardizes asyncio loop retrieval with asyncio.get_running_loop(), migrates manual DB connection lifecycle to db_conn() context managers, extracts core frontend orchestration into four hooks, refactors App.jsx, updates dub speaker-clone UI, redesigns the SetupWizard, adds Vitest tests/config, and updates Tauri/bootstrap and deployment/CI files. ChangesUnified Backend & Frontend Modernization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/services/tts_backend.py (1)
135-146:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix async-context detection logic in
_ensure_loaded.
asyncio.get_running_loop()raises RuntimeError if no loop is running and returns the running loop otherwise. This means the check at line 137 is always True when line 136 succeeds, making the custom error at line 140 always caught by theexcept RuntimeErrorblock at line 145. The subsequentasyncio.run()call then fails in async contexts with "cannot be called from a running event loop", replacing your intended diagnostic message.Invert the logic: check whether a loop is running by attempting to get it. If it succeeds, raise your diagnostic directly. If it fails, you're in a sync context and can safely call
asyncio.run().🔧 Proposed fix
- try: - loop = asyncio.get_running_loop() - if loop.is_running(): - # Already inside an async context — caller should await - # `get_model()` themselves and pass it in via the constructor. - raise RuntimeError( - "OmniVoiceBackend.generate() called inside an async context without a pre-loaded model. " - "Pass `model=await get_model()` to the constructor." - ) - self._model = loop.run_until_complete(get_model()) - except RuntimeError: - self._model = asyncio.run(get_model()) + try: + asyncio.get_running_loop() + except RuntimeError: + # No running loop in this thread: safe to bootstrap synchronously. + self._model = asyncio.run(get_model()) + return + # Running async context: require caller to pass preloaded model. + raise RuntimeError( + "OmniVoiceBackend.generate() called inside an async context without a pre-loaded model. " + "Pass `model=await get_model()` to the constructor." + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/services/tts_backend.py` around lines 135 - 146, The async-context detection in _ensure_loaded is inverted and catching your own diagnostic; change the logic to try calling asyncio.get_running_loop(), and if that call succeeds (no exception) raise the RuntimeError diagnostic about calling generate() inside an async context and instructing to pass model=await get_model(); if asyncio.get_running_loop() raises RuntimeError (meaning no loop is running), call self._model = asyncio.run(get_model()) to load the model synchronously; reference _ensure_loaded and get_model when making this change so you don't accidentally use loop.run_until_complete or catch the diagnostic you just raised.
🧹 Nitpick comments (6)
.gitignore (1)
88-88: ⚡ Quick winConsider adding additional coverage artifact patterns.
While
.coverageis the primary data file, pytest-cov can generate additional artifacts that should typically be ignored:
.coverage.*(parallel mode creates numbered data files)htmlcov/(HTML coverage reports)coverage.xml(XML reports often used in CI)📝 Suggested additions
.coverage +.coverage.* +htmlcov/ +coverage.xml🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.gitignore at line 88, .gitignore currently only ignores the single `.coverage` file; add the common pytest-cov artifacts to avoid committing coverage outputs by appending patterns `.coverage.*`, `htmlcov/`, and `coverage.xml` to the file so parallel/HTML/XML reports are also ignored. Ensure you update the existing `.coverage` entry in the .gitignore to include these additional patterns and commit the change.deploy/Dockerfile (1)
35-35: ⚡ Quick winConsider testing removal of
build-essentialfor reduced image size, but verify the build succeeds first.Most dependencies in
pyproject.toml(torch, transformers, whisperx, etc.) provide pre-built wheels for Linux. However, packages likepedalboard,soundfile, andpsutilhave C extensions and may need compilation tools during wheel installation. Sinceuvprefers pre-built wheels and the project uses a frozenuv.lock, these dependencies likely install without compilation.To safely remove
build-essential, run a test build with it removed and confirm all dependencies install successfully. If the build succeeds, you can save ~200MB per image.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/Dockerfile` at line 35, Remove the "build-essential" package from the Dockerfile and run a full test build to verify all Python dependencies in pyproject.toml (notably torch, transformers, whisperx, pedalboard, soundfile, psutil) install as wheels without C compilation; specifically, edit the Dockerfile to omit the build-essential apt package, build the image, inspect the pip install logs for compilation errors, run the project's test/startup commands inside the container to ensure runtime functionality, and if any dependency fails to install or tests fail, revert the change or add only the minimal build tools required for that failing package.frontend/src/pages/SetupWizard.jsx (1)
309-317: ⚡ Quick winAdd explicit
type="button"attribute.The button element should include
type="button"to prevent unintended form submission behavior. While this button isn't inside a form, it's a best practice to be explicit about button types.♻️ Suggested fix
<button className="setup-wizard__footnote-link" onClick={() => revealPath(cachePath)} title="Open in Finder" + type="button" >🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/pages/SetupWizard.jsx` around lines 309 - 317, The button rendered in SetupWizard.jsx (the element with className "setup-wizard__footnote-link" that calls revealPath(cachePath) onClick) should include an explicit type attribute to avoid default form-submit behavior; update that JSX button to add type="button" so it won't trigger form submission if placed inside a form in the future.backend/api/routers/dub_export.py (1)
10-10: ⚡ Quick winUnused import:
db_connis never used in this file.The import of
db_connwas added but is not referenced anywhere in the file. All database operations appear to be handled through_get_job()imported fromdub_core.🧹 Remove unused import
-from core.db import db_conn🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/api/routers/dub_export.py` at line 10, Remove the unused import db_conn from core.db in dub_export.py; locate the top-level import line containing "from core.db import db_conn" and delete it (or replace with a used symbol if db_conn was intended), leaving existing references to _get_job from dub_core unchanged so behavior remains the same.frontend/src/hooks/useAppData.js (1)
165-178: 💤 Low valueConsider memoizing the persistence object to reduce effect triggers.
The persistence effect has an extensive dependency array (lines 174-178) that will trigger on any state change. While functional, this could cause unnecessary localStorage writes. Consider memoizing the serialization object:
♻️ Optimization suggestion
+ const persistState = useMemo(() => ({ + uiScale, text, mode, vdStates, language, + isSidebarCollapsed, sidebarTab, + dubJobId, dubFilename, dubDuration, dubSegments, + dubLang, dubLangCode, dubTracks, dubStep, dubTranscript, + exportTracks, preserveBg, defaultTrack, exportHistory, + speed, steps, cfg, denoise, showOverrides + }), [uiScale, text, mode, vdStates, language, isSidebarCollapsed, sidebarTab, + dubJobId, dubFilename, dubDuration, dubSegments, dubLang, dubLangCode, + dubTracks, dubStep, dubTranscript, exportTracks, preserveBg, defaultTrack, + exportHistory, speed, steps, cfg, denoise, showOverrides + ]); + useEffect(() => { - localStorage.setItem('omni_ui', JSON.stringify({ - uiScale, text, mode, vdStates, language, - isSidebarCollapsed, sidebarTab, - dubJobId, dubFilename, dubDuration, dubSegments, - dubLang, dubLangCode, dubTracks, dubStep, dubTranscript, - exportTracks, preserveBg, defaultTrack, exportHistory, - speed, steps, cfg, denoise, showOverrides - })); - }, [uiScale, text, mode, vdStates, language, isSidebarCollapsed, sidebarTab, - dubJobId, dubFilename, dubDuration, dubSegments, dubLang, dubLangCode, - dubTracks, dubStep, dubTranscript, exportTracks, preserveBg, defaultTrack, - exportHistory, speed, steps, cfg, denoise, showOverrides - ]); + localStorage.setItem('omni_ui', JSON.stringify(persistState)); + }, [persistState]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/hooks/useAppData.js` around lines 165 - 178, The effect writes a large object to localStorage and lists every field in the dependency array, causing frequent writes; replace that with a memoized payload (e.g., compute the object or its JSON string using useMemo) that includes uiScale, text, mode, vdStates, language, isSidebarCollapsed, sidebarTab, dubJobId, dubFilename, dubDuration, dubSegments, dubLang, dubLangCode, dubTracks, dubStep, dubTranscript, exportTracks, preserveBg, defaultTrack, exportHistory, speed, steps, cfg, denoise, showOverrides as dependencies, then have useEffect only depend on that memoized value and call localStorage.setItem('omni_ui', ...) using the memoized result to avoid unnecessary writes.backend/api/routers/gallery.py (1)
469-497: 💤 Low valueSQL construction is safe but could be more explicit.
The dynamic SQL construction on Line 496 triggers static analysis warnings because column names are interpolated via f-string. While the code is currently safe (column names are validated against a fixed set), consider using an allowlist pattern to make the safety more obvious:
♻️ Refactor to explicit allowlist
`@router.patch`("/gallery/voices/{voice_id}") def update_voice(voice_id: str, body: dict): """Update voice metadata — name, tags, is_favorite.""" + ALLOWED_FIELDS = {"name", "tags", "is_favorite", "description"} with db_conn() as conn: row = conn.execute("SELECT id FROM voice_gallery WHERE id = ?", (voice_id,)).fetchone() if not row: raise HTTPException(status_code=404, detail="Voice not found") updates = [] params = [] - if "name" in body: + for field in ["name", "tags", "is_favorite", "description"]: + if field not in body or field not in ALLOWED_FIELDS: + continue + if field == "name": - updates.append("name = ?") - params.append(body["name"]) - if "tags" in body: + updates.append("name = ?") + params.append(body["name"]) + elif field == "tags": - updates.append("tags = ?") - params.append(json.dumps(body["tags"]) if isinstance(body["tags"], list) else body["tags"]) - if "is_favorite" in body: + updates.append("tags = ?") + params.append(json.dumps(body["tags"]) if isinstance(body["tags"], list) else body["tags"]) + elif field == "is_favorite": - updates.append("is_favorite = ?") - params.append(1 if body["is_favorite"] else 0) - if "description" in body: + updates.append("is_favorite = ?") + params.append(1 if body["is_favorite"] else 0) + elif field == "description": - updates.append("description = ?") - params.append(body["description"]) + updates.append("description = ?") + params.append(body["description"]) if not updates: return {"success": True, "updated": []} params.append(voice_id) conn.execute(f"UPDATE voice_gallery SET {', '.join(updates)} WHERE id = ?", params) return {"success": True, "updated": list(body.keys())}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/api/routers/gallery.py` around lines 469 - 497, The dynamic f-string used to build the UPDATE clause in update_voice currently interpolates column names (via the updates list) which triggers static analysis warnings; replace the implicit column interpolation with an explicit allowlist mapping of allowed keys to concrete column assignments (e.g., map "name","tags","is_favorite","description" to their SQL snippets) and only append SQL fragments from that allowlist when the corresponding key exists in body, then join those validated fragments to form the SET clause and keep parameterized values in params before executing the prepared statement against voice_gallery.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/api/routers/generation.py`:
- Around line 185-187: The except block catching ValueError should preserve the
original traceback by using exception chaining when re-raising as an
HTTPException; update the except ValueError as e handler that currently calls
logger.error("Validation failed: %s", e) and raise
HTTPException(status_code=400, detail=str(e)) so that it instead re-raises with
"from e" (i.e., raise HTTPException(...) from e) to keep the original exception
context.
In `@deploy/docker-compose.yml`:
- Line 29: The omnivoice service was given a profiles key which prevents it from
auto-starting by default; remove the profiles: ["cpu"] entry from the omnivoice
service definition so Docker Compose will start it with plain docker compose up
(restore default CPU-mode behavior and match the documented command). Ensure you
edit the omnivoice service block and delete the profiles line (and any trailing
commas/formatting) so the service has no profiles key.
In `@frontend/src-tauri/src/bootstrap.rs`:
- Around line 322-331: The code currently deletes backend_dir with
fs::remove_dir_all and then only logs a warning if
copy_dir_recursive(&res_backend, &backend_dir) fails but still returns
Some((venv_py, backend_dir)); change this to fail fast: either (preferred) copy
into a temp directory and only call fs::remove_dir_all + rename on successful
copy, or (simpler) if copy_dir_recursive fails after removal then log an error
and return None (do not return Some). Update the block referencing res_backend,
backend_dir, copy_dir_recursive and the final return Some((venv_py,
backend_dir)) to implement one of these fixes so a failed sync does not return
success.
In `@frontend/src/App.jsx`:
- Around line 173-204: The refactor removed the setSeed selector so later calls
like setSeed(item.seed.toString()) throw ReferenceError; re-add the selector by
grabbing setSeed from the app store (useAppStore) alongside the other
generate-slice selectors in App (so setSeed is available in this component
scope), ensuring any restore/history code can call setSeed(item.seed.toString())
without errors.
In `@frontend/src/hooks/useDubWorkflow.js`:
- Around line 371-375: handleDubStop sets the UI to 'stopping' but on
tasksCancel failure only shows a toast, leaving the workflow stuck; modify
handleDubStop to capture the previous step (e.g., const prev = current dub step
before setDubStep('stopping')) and in the catch block call setDubStep(prev) to
restore the prior state, while still showing toast.error for the failure; ensure
references to dubTaskId, tasksCancel, setDubStep, and toast.error are used to
implement this rollback so the UI is not left permanently in 'stopping'.
- Around line 321-364: The stream handler currently treats EOF as success; add
an explicit "receivedDone" flag (set true when evt.type === 'done' in the JSON
branch) and only run the success path (setDubStep('done'), loadDubHistory(),
loadProjects(), playPing(), completePill) after the read loop if receivedDone is
true and wasCancelled is false; if EOF occurs without receivedDone, mark the job
as not finished (e.g., setDubStep('editing') and set a Dub error or dismiss the
pill) so partial/aborted streams don't report success — update logic around the
reader loop, the evt.type === 'done' handling, and the post-loop conditional
that currently checks wasCancelled and dubStep.
In `@frontend/src/hooks/useTTS.js`:
- Line 72: The interval callback overwrites the full generationTime string and
discards any existing "(xx%)" suffix; update the interval to preserve the
percentage by using the functional state updater for setGenerationTime and
reusing any existing percent suffix from the previous value (e.g., read prev
inside setGenerationTime, extract trailing "(...%)" with a regex, compute the
new elapsed string from Date.now() - st, then return `${elapsed}${percentSuffix
|| ''}`), or alternatively split out percentage into its own state
(generationPercent) and only update elapsed in setGenerationTime so the percent
is never lost; make this change where timerRef.current is set and in any other
places that call setGenerationTime (e.g., the code that previously appended
"(xx%)") to ensure the percent is preserved.
In `@frontend/src/store/store.test.js`:
- Around line 4-46: Add a beforeEach that clears persisted state and resets the
Zustand store before each test: call localStorage.clear() and then import
useAppStore and invoke its getState() reset path (e.g. call an existing reset
function or call getState().setMode(...) and getState().setText(...) to restore
defaults). Reference useAppStore, useAppStore.getState(), setMode, setText (and
keep dubStep/stage expectations unchanged) so tests are order-independent.
---
Outside diff comments:
In `@backend/services/tts_backend.py`:
- Around line 135-146: The async-context detection in _ensure_loaded is inverted
and catching your own diagnostic; change the logic to try calling
asyncio.get_running_loop(), and if that call succeeds (no exception) raise the
RuntimeError diagnostic about calling generate() inside an async context and
instructing to pass model=await get_model(); if asyncio.get_running_loop()
raises RuntimeError (meaning no loop is running), call self._model =
asyncio.run(get_model()) to load the model synchronously; reference
_ensure_loaded and get_model when making this change so you don't accidentally
use loop.run_until_complete or catch the diagnostic you just raised.
---
Nitpick comments:
In @.gitignore:
- Line 88: .gitignore currently only ignores the single `.coverage` file; add
the common pytest-cov artifacts to avoid committing coverage outputs by
appending patterns `.coverage.*`, `htmlcov/`, and `coverage.xml` to the file so
parallel/HTML/XML reports are also ignored. Ensure you update the existing
`.coverage` entry in the .gitignore to include these additional patterns and
commit the change.
In `@backend/api/routers/dub_export.py`:
- Line 10: Remove the unused import db_conn from core.db in dub_export.py;
locate the top-level import line containing "from core.db import db_conn" and
delete it (or replace with a used symbol if db_conn was intended), leaving
existing references to _get_job from dub_core unchanged so behavior remains the
same.
In `@backend/api/routers/gallery.py`:
- Around line 469-497: The dynamic f-string used to build the UPDATE clause in
update_voice currently interpolates column names (via the updates list) which
triggers static analysis warnings; replace the implicit column interpolation
with an explicit allowlist mapping of allowed keys to concrete column
assignments (e.g., map "name","tags","is_favorite","description" to their SQL
snippets) and only append SQL fragments from that allowlist when the
corresponding key exists in body, then join those validated fragments to form
the SET clause and keep parameterized values in params before executing the
prepared statement against voice_gallery.
In `@deploy/Dockerfile`:
- Line 35: Remove the "build-essential" package from the Dockerfile and run a
full test build to verify all Python dependencies in pyproject.toml (notably
torch, transformers, whisperx, pedalboard, soundfile, psutil) install as wheels
without C compilation; specifically, edit the Dockerfile to omit the
build-essential apt package, build the image, inspect the pip install logs for
compilation errors, run the project's test/startup commands inside the container
to ensure runtime functionality, and if any dependency fails to install or tests
fail, revert the change or add only the minimal build tools required for that
failing package.
In `@frontend/src/hooks/useAppData.js`:
- Around line 165-178: The effect writes a large object to localStorage and
lists every field in the dependency array, causing frequent writes; replace that
with a memoized payload (e.g., compute the object or its JSON string using
useMemo) that includes uiScale, text, mode, vdStates, language,
isSidebarCollapsed, sidebarTab, dubJobId, dubFilename, dubDuration, dubSegments,
dubLang, dubLangCode, dubTracks, dubStep, dubTranscript, exportTracks,
preserveBg, defaultTrack, exportHistory, speed, steps, cfg, denoise,
showOverrides as dependencies, then have useEffect only depend on that memoized
value and call localStorage.setItem('omni_ui', ...) using the memoized result to
avoid unnecessary writes.
In `@frontend/src/pages/SetupWizard.jsx`:
- Around line 309-317: The button rendered in SetupWizard.jsx (the element with
className "setup-wizard__footnote-link" that calls revealPath(cachePath)
onClick) should include an explicit type attribute to avoid default form-submit
behavior; update that JSX button to add type="button" so it won't trigger form
submission if placed inside a form in the future.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: cd7282f6-86e1-4db9-a200-d2723d1eb317
⛔ Files ignored due to path filters (2)
bun.lockis excluded by!**/*.lockuv.lockis excluded by!**/*.lock
📒 Files selected for processing (61)
.github/workflows/ci.yml.gitignorebackend/api/routers/batch.pybackend/api/routers/capture.pybackend/api/routers/capture_ws.pybackend/api/routers/dub_core.pybackend/api/routers/dub_export.pybackend/api/routers/dub_generate.pybackend/api/routers/dub_translate.pybackend/api/routers/exports.pybackend/api/routers/gallery.pybackend/api/routers/generation.pybackend/api/routers/marketplace.pybackend/api/routers/openai_compat.pybackend/api/routers/profiles.pybackend/api/routers/projects.pybackend/api/routers/setup/download.pybackend/api/routers/setup/wizard.pybackend/api/routers/tts_stream.pybackend/main.pybackend/services/asr_backend.pybackend/services/batched_tts.pybackend/services/dub_pipeline.pybackend/services/ffmpeg_utils.pybackend/services/gpu_sandbox.pybackend/services/model_manager.pybackend/services/translator.pybackend/services/tts_backend.pybackend/services/video_context.pydeploy/Dockerfiledeploy/docker-compose.ymlfrontend/package.jsonfrontend/src-tauri/src/backend.rsfrontend/src-tauri/src/bootstrap.rsfrontend/src/App.jsxfrontend/src/components/BootstrapSplash.cssfrontend/src/components/CastingView.jsxfrontend/src/components/DubSegmentRow.jsxfrontend/src/components/DubSegmentTable.jsxfrontend/src/hooks/useAppData.jsfrontend/src/hooks/useDubWorkflow.jsfrontend/src/hooks/useProfiles.jsfrontend/src/hooks/useTTS.jsfrontend/src/pages/DubTab.jsxfrontend/src/pages/Settings.cssfrontend/src/pages/Settings.jsxfrontend/src/pages/SetupWizard.cssfrontend/src/pages/SetupWizard.jsxfrontend/src/pages/VoiceGallery.jsxfrontend/src/store/store.test.jsfrontend/src/test/setup.jsfrontend/src/ui/Button.cssfrontend/src/ui/Progress.jsxfrontend/src/utils/dialog.jsfrontend/src/utils/utils.test.jsfrontend/tsconfig.jsonfrontend/vite.config.jsomnivoice/models/omnivoice.pypyproject.tomlscripts/desktop-prod.shscripts/smoke-test.sh
Comment on lines
+185
to
+187
| except ValueError as e: | ||
| logger.error("Validation failed: %s", e) | ||
| raise HTTPException(status_code=400, detail=str(e)) |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add exception chaining to preserve the original traceback.
When re-raising as HTTPException, use from e to preserve the exception chain and make debugging easier.
🔗 Proposed fix to add exception chaining
except ValueError as e:
logger.error("Validation failed: %s", e)
- raise HTTPException(status_code=400, detail=str(e))
+ raise HTTPException(status_code=400, detail=str(e)) from eAs per coding guidelines, the static analysis tool (Ruff B904) recommends using raise ... from err to distinguish validation errors from errors in exception handling.
🧰 Tools
🪛 Ruff (0.15.12)
[warning] 187-187: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/api/routers/generation.py` around lines 185 - 187, The except block
catching ValueError should preserve the original traceback by using exception
chaining when re-raising as an HTTPException; update the except ValueError as e
handler that currently calls logger.error("Validation failed: %s", e) and raise
HTTPException(status_code=400, detail=str(e)) so that it instead re-raises with
"from e" (i.e., raise HTTPException(...) from e) to keep the original exception
context.
| context: .. | ||
| dockerfile: deploy/Dockerfile | ||
| container_name: omnivoice-studio | ||
| profiles: ["cpu"] |
Contributor
There was a problem hiding this comment.
Remove profiles: ["cpu"] to restore default behavior.
Adding profiles: ["cpu"] to the default omnivoice service is a breaking change. Docker Compose only auto-starts services that have no profiles key. With this change:
docker compose upwill start nothing (both services require explicit profiles)- The documented command on line 5 (
docker compose -f deploy/docker-compose.yml up # CPU mode) will fail
The comments clearly indicate CPU mode should be the default. Remove the profiles key from the omnivoice service to preserve backward compatibility and match the documented behavior.
🔧 Proposed fix
build:
context: ..
dockerfile: deploy/Dockerfile
container_name: omnivoice-studio
- profiles: ["cpu"]
ports:
- "127.0.0.1:3900:3900"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@deploy/docker-compose.yml` at line 29, The omnivoice service was given a
profiles key which prevents it from auto-starting by default; remove the
profiles: ["cpu"] entry from the omnivoice service definition so Docker Compose
will start it with plain docker compose up (restore default CPU-mode behavior
and match the documented command). Ensure you edit the omnivoice service block
and delete the profiles line (and any trailing commas/formatting) so the service
has no profiles key.
Comment on lines
+322
to
331
| if res_backend.is_dir() { | ||
| let _ = fs::remove_dir_all(&backend_dir); | ||
| if let Err(e) = copy_dir_recursive(&res_backend, &backend_dir) { | ||
| log::warn!("Failed to sync backend/ sources: {}", e); | ||
| } else { | ||
| log::info!("Synced backend/ from bundle"); | ||
| } | ||
| } | ||
| } | ||
| return Some((venv_py, backend_dir)); |
Contributor
There was a problem hiding this comment.
Fail fast when backend source sync fails after deletion.
At Line 323 the existing backend/ directory is removed, but if copy fails at Line 324 you only log a warning and still return success at Line 331. This can leave installs without backend sources and defer failure to backend startup.
Suggested fix
if res_backend.is_dir() {
let _ = fs::remove_dir_all(&backend_dir);
if let Err(e) = copy_dir_recursive(&res_backend, &backend_dir) {
- log::warn!("Failed to sync backend/ sources: {}", e);
+ fail(progress, &format!("Failed to sync backend/ sources: {}", e));
+ return None;
} else {
log::info!("Synced backend/ from bundle");
}
}📝 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.
Suggested change
| if res_backend.is_dir() { | |
| let _ = fs::remove_dir_all(&backend_dir); | |
| if let Err(e) = copy_dir_recursive(&res_backend, &backend_dir) { | |
| log::warn!("Failed to sync backend/ sources: {}", e); | |
| } else { | |
| log::info!("Synced backend/ from bundle"); | |
| } | |
| } | |
| } | |
| return Some((venv_py, backend_dir)); | |
| if res_backend.is_dir() { | |
| let _ = fs::remove_dir_all(&backend_dir); | |
| if let Err(e) = copy_dir_recursive(&res_backend, &backend_dir) { | |
| fail(progress, &format!("Failed to sync backend/ sources: {}", e)); | |
| return None; | |
| } else { | |
| log::info!("Synced backend/ from bundle"); | |
| } | |
| } | |
| } | |
| return Some((venv_py, backend_dir)); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src-tauri/src/bootstrap.rs` around lines 322 - 331, The code
currently deletes backend_dir with fs::remove_dir_all and then only logs a
warning if copy_dir_recursive(&res_backend, &backend_dir) fails but still
returns Some((venv_py, backend_dir)); change this to fail fast: either
(preferred) copy into a temp directory and only call fs::remove_dir_all + rename
on successful copy, or (simpler) if copy_dir_recursive fails after removal then
log an error and return None (do not return Some). Update the block referencing
res_backend, backend_dir, copy_dir_recursive and the final return Some((venv_py,
backend_dir)) to implement one of these fixes so a failed sync does not return
success.
Comment on lines
+173
to
204
| // ═══ EXTRACTED HOOKS ═══ | ||
| const { | ||
| profiles, history, dubHistory, studioProjects, exportHistory, | ||
| showOverrides, setShowOverrides, | ||
| sysStats, modelStatus, | ||
| loadProfiles, loadHistory, loadDubHistory, loadProjects, loadExportHistory, | ||
| } = useAppData(); | ||
|
|
||
| // ═══ VOICE PROFILES ═══ | ||
| const [profiles, setProfiles] = useState([]); | ||
| const [selectedProfile, setSelectedProfile] = useState(null); | ||
| const [showSaveProfile, setShowSaveProfile] = useState(false); | ||
| const [profileName, setProfileName] = useState(''); | ||
| const { | ||
| selectedProfile, setSelectedProfile, | ||
| showSaveProfile, setShowSaveProfile, | ||
| profileName, setProfileName, | ||
| previewLoading, segmentPreviewLoading, | ||
| isVoicePreviewOpen, setIsVoicePreviewOpen, | ||
| voicePreviewProfileId, setVoicePreviewProfileId, | ||
| handleSaveProfile: _handleSaveProfile, | ||
| handleDeleteProfile, handleSelectProfile, | ||
| handlePreviewVoice, handleSegmentPreview, | ||
| handleSaveHistoryAsProfile, handleLockProfile, handleUnlockProfile, | ||
| } = useProfiles({ loadHistory, loadProfiles }); | ||
|
|
||
| const { | ||
| refAudio, setRefAudio, | ||
| pendingTrimFile, setPendingTrimFile, | ||
| isGenerating, generationTime, | ||
| textAreaRef, | ||
| ingestRefAudio, insertTag, applyPreset, | ||
| handleGenerate, | ||
| } = useTTS({ selectedProfile, setSelectedProfile, loadHistory }); | ||
|
|
||
| const handleSaveProfile = () => _handleSaveProfile(refAudio, refText, instruct, language); | ||
|
|
Contributor
There was a problem hiding this comment.
Rewire setSeed after the hook extraction.
Line 731 later still calls setSeed(item.seed.toString()), but this refactor no longer defines setSeed in App. Restoring any history item with a seed will now throw a ReferenceError and abort the restore flow.
// add this back alongside the other generate-slice selectors
const setSeed = useAppStore(s => s.setSeed);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src/App.jsx` around lines 173 - 204, The refactor removed the
setSeed selector so later calls like setSeed(item.seed.toString()) throw
ReferenceError; re-add the selector by grabbing setSeed from the app store
(useAppStore) alongside the other generate-slice selectors in App (so setSeed is
available in this component scope), ensuring any restore/history code can call
setSeed(item.seed.toString()) without errors.
Comment on lines
+321
to
+364
| const streamRes = await fetch(tasksStreamUrl(data.task_id)); | ||
| const reader = streamRes.body.getReader(); | ||
| const decoder = new TextDecoder(); | ||
| let buffer = ''; | ||
| let wasCancelled = false; | ||
| while (true) { | ||
| const { done, value } = await reader.read(); | ||
| if (done) break; | ||
| buffer += decoder.decode(value, { stream: true }); | ||
| const lines = buffer.split('\n'); buffer = lines.pop(); | ||
| for (const line of lines) { | ||
| if (line.startsWith('data: ')) { | ||
| try { | ||
| const evt = JSON.parse(line.slice(6)); | ||
| if (evt.type === 'progress') { | ||
| setDubProgress({ current: evt.current + 1, total: evt.total, text: evt.text }); | ||
| useAppStore.getState().setPillProgress(Math.round(((evt.current + 1) / evt.total) * 100)); | ||
| useAppStore.getState().setPillLabel(`Generating dub… ${evt.current + 1}/${evt.total}`); | ||
| } else if (evt.type === 'done') { | ||
| setDubStep('done'); | ||
| setDubTracks(evt.tracks || []); | ||
| if (evt.sync_scores) setDubSegments(prev => prev.map((s, idx) => ({ ...s, sync_ratio: evt.sync_scores[idx] }))); | ||
| if (evt.seg_num_step && typeof evt.seg_num_step === 'object') { | ||
| const previewIds = Object.entries(evt.seg_num_step).filter(([, n]) => typeof n === 'number' && n < steps).map(([id]) => id); | ||
| setPreviewSegIds(previewIds); | ||
| } | ||
| if (evt.seg_hashes && Object.keys(evt.seg_hashes).length > 0) { | ||
| useAppStore.getState().setLastGenFingerprints?.(evt.seg_hashes); | ||
| } else { | ||
| try { const plan = await apiPost('/tools/incremental', { segments: dubSegments.map(s => ({ id: String(s.id), text: s.text, target_lang: s.target_lang, profile_id: s.profile_id, instruct: s.instruct, speed: s.speed, direction: s.direction })) }); useAppStore.getState().setLastGenFingerprints?.(plan.fingerprints || {}); } catch {} | ||
| } | ||
| } else if (evt.type === 'cancelled') { | ||
| wasCancelled = true; setDubStep('editing'); setDubError('Generation aborted.'); toast('Dubbing aborted', { icon: '⏹' }); | ||
| } else if (evt.type === 'error') setDubError(p => p + `\nSeg ${evt.segment}: ${evt.error}`); | ||
| } catch (e) {} | ||
| } | ||
| } | ||
| } | ||
| setDubTaskId(null); | ||
| if (!wasCancelled) { | ||
| if (dubStep !== 'done') setDubStep('done'); | ||
| loadDubHistory(); loadProjects(); playPing(); | ||
| useAppStore.getState().completePill('Dub complete'); | ||
| } else { useAppStore.getState().dismissPill(); } |
Contributor
There was a problem hiding this comment.
Require a terminal done event before reporting success.
Lines 360-364 currently treat any non-cancelled EOF as success. If the stream drops after partial progress, the hook still flips the job to done, plays the completion ping, and refreshes history as if generation finished.
Suggested fix
const streamRes = await fetch(tasksStreamUrl(data.task_id));
const reader = streamRes.body.getReader();
const decoder = new TextDecoder();
let buffer = '';
let wasCancelled = false;
+ let sawDone = false;
while (true) {
const { done, value } = await reader.read();
if (done) break;
buffer += decoder.decode(value, { stream: true });
const lines = buffer.split('\n'); buffer = lines.pop();
@@
if (evt.type === 'progress') {
setDubProgress({ current: evt.current + 1, total: evt.total, text: evt.text });
useAppStore.getState().setPillProgress(Math.round(((evt.current + 1) / evt.total) * 100));
useAppStore.getState().setPillLabel(`Generating dub… ${evt.current + 1}/${evt.total}`);
} else if (evt.type === 'done') {
+ sawDone = true;
setDubStep('done');
setDubTracks(evt.tracks || []);
if (evt.sync_scores) setDubSegments(prev => prev.map((s, idx) => ({ ...s, sync_ratio: evt.sync_scores[idx] })));
@@
}
setDubTaskId(null);
if (!wasCancelled) {
+ if (!sawDone) throw new Error('Generation stream ended before completion');
if (dubStep !== 'done') setDubStep('done');
loadDubHistory(); loadProjects(); playPing();
useAppStore.getState().completePill('Dub complete');
} else { useAppStore.getState().dismissPill(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src/hooks/useDubWorkflow.js` around lines 321 - 364, The stream
handler currently treats EOF as success; add an explicit "receivedDone" flag
(set true when evt.type === 'done' in the JSON branch) and only run the success
path (setDubStep('done'), loadDubHistory(), loadProjects(), playPing(),
completePill) after the read loop if receivedDone is true and wasCancelled is
false; if EOF occurs without receivedDone, mark the job as not finished (e.g.,
setDubStep('editing') and set a Dub error or dismiss the pill) so
partial/aborted streams don't report success — update logic around the reader
loop, the evt.type === 'done' handling, and the post-loop conditional that
currently checks wasCancelled and dubStep.
Comment on lines
+371
to
+375
| const handleDubStop = useCallback(async () => { | ||
| if (!dubTaskId) return; | ||
| setDubStep('stopping'); | ||
| try { await tasksCancel(dubTaskId); } catch (e) { toast.error('Failed to stop'); } | ||
| }, [dubTaskId, setDubStep]); |
Contributor
There was a problem hiding this comment.
Restore the previous step when cancellation fails.
Line 374 only shows a toast on failure, so the UI stays stuck in stopping if tasksCancel errors. That leaves the workflow in a dead state even though the task may still be running.
Suggested fix
const handleDubStop = useCallback(async () => {
if (!dubTaskId) return;
+ const prevStep = dubStep;
setDubStep('stopping');
- try { await tasksCancel(dubTaskId); } catch (e) { toast.error('Failed to stop'); }
- }, [dubTaskId, setDubStep]);
+ try {
+ await tasksCancel(dubTaskId);
+ } catch (e) {
+ setDubStep(prevStep);
+ toast.error('Failed to stop');
+ }
+ }, [dubTaskId, dubStep, setDubStep]);📝 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.
Suggested change
| const handleDubStop = useCallback(async () => { | |
| if (!dubTaskId) return; | |
| setDubStep('stopping'); | |
| try { await tasksCancel(dubTaskId); } catch (e) { toast.error('Failed to stop'); } | |
| }, [dubTaskId, setDubStep]); | |
| const handleDubStop = useCallback(async () => { | |
| if (!dubTaskId) return; | |
| const prevStep = dubStep; | |
| setDubStep('stopping'); | |
| try { | |
| await tasksCancel(dubTaskId); | |
| } catch (e) { | |
| setDubStep(prevStep); | |
| toast.error('Failed to stop'); | |
| } | |
| }, [dubTaskId, dubStep, setDubStep]); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src/hooks/useDubWorkflow.js` around lines 371 - 375, handleDubStop
sets the UI to 'stopping' but on tasksCancel failure only shows a toast, leaving
the workflow stuck; modify handleDubStop to capture the previous step (e.g.,
const prev = current dub step before setDubStep('stopping')) and in the catch
block call setDubStep(prev) to restore the prior state, while still showing
toast.error for the failure; ensure references to dubTaskId, tasksCancel,
setDubStep, and toast.error are used to implement this rollback so the UI is not
left permanently in 'stopping'.
| setIsGenerating(true); | ||
| setGenerationTime(0); | ||
| const st = Date.now(); | ||
| timerRef.current = setInterval(() => setGenerationTime(((Date.now() - st) / 1000).toFixed(1)), 100); |
Contributor
There was a problem hiding this comment.
Preserve the percentage instead of overwriting it every tick.
Line 72 rewrites generationTime to a plain elapsed string every 100 ms, so the (xx%) suffix added on Lines 121-123 is dropped on the next interval tick. The progress indicator will flicker or never stay visible.
Suggested fix
- timerRef.current = setInterval(() => setGenerationTime(((Date.now() - st) / 1000).toFixed(1)), 100);
+ timerRef.current = setInterval(() => {
+ const elapsed = ((Date.now() - st) / 1000).toFixed(1);
+ setGenerationTime(prev => {
+ const suffix = /\(\d+%\)$/.exec(String(prev))?.[0];
+ return suffix ? `${elapsed} ${suffix}` : elapsed;
+ });
+ }, 100);Also applies to: 121-124
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src/hooks/useTTS.js` at line 72, The interval callback overwrites
the full generationTime string and discards any existing "(xx%)" suffix; update
the interval to preserve the percentage by using the functional state updater
for setGenerationTime and reusing any existing percent suffix from the previous
value (e.g., read prev inside setGenerationTime, extract trailing "(...%)" with
a regex, compute the new elapsed string from Date.now() - st, then return
`${elapsed}${percentSuffix || ''}`), or alternatively split out percentage into
its own state (generationPercent) and only update elapsed in setGenerationTime
so the percent is never lost; make this change where timerRef.current is set and
in any other places that call setGenerationTime (e.g., the code that previously
appended "(xx%)") to ensure the percent is preserved.
Comment on lines
+4
to
+46
| describe('Zustand store', () => { | ||
| it('useAppStore initialises with default mode', async () => { | ||
| const { useAppStore } = await import('../store'); | ||
| const { result } = renderHook(() => useAppStore(s => s.mode)); | ||
| // Default mode should be a string (launchpad, design, clone, or dub) | ||
| expect(typeof result.current).toBe('string'); | ||
| expect(result.current.length).toBeGreaterThan(0); | ||
| }); | ||
|
|
||
| it('setMode updates mode', async () => { | ||
| const { useAppStore } = await import('../store'); | ||
| const { result, rerender } = renderHook(() => ({ | ||
| mode: useAppStore(s => s.mode), | ||
| setMode: useAppStore(s => s.setMode), | ||
| })); | ||
| result.current.setMode('dub'); | ||
| rerender(); | ||
| expect(result.current.mode).toBe('dub'); | ||
| }); | ||
|
|
||
| it('setText updates text', async () => { | ||
| const { useAppStore } = await import('../store'); | ||
| const { result, rerender } = renderHook(() => ({ | ||
| text: useAppStore(s => s.text), | ||
| setText: useAppStore(s => s.setText), | ||
| })); | ||
| result.current.setText('hello world'); | ||
| rerender(); | ||
| expect(result.current.text).toBe('hello world'); | ||
| }); | ||
|
|
||
| it('dubSlice initialises with idle step', async () => { | ||
| const { useAppStore } = await import('../store'); | ||
| const { result } = renderHook(() => useAppStore(s => s.dubStep)); | ||
| expect(result.current).toBe('idle'); | ||
| }); | ||
|
|
||
| it('pill slice starts at idle', async () => { | ||
| const { useAppStore } = await import('../store'); | ||
| const { result } = renderHook(() => useAppStore(s => s.stage)); | ||
| expect(result.current).toBe('idle'); | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Inspect store exports/state helpers:"
fd -p 'store.*' frontend/src/store --exec rg -n "getState|getInitialState|setState|reset|create"
echo
echo "Inspect this test file for lifecycle reset hooks:"
rg -n "beforeEach|afterEach|setMode|setText|describe|it" frontend/src/store/store.test.jsRepository: debpalash/OmniVoice-Studio
Length of output: 1814
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check store.js for reset/getState mechanisms ==="
cat -n frontend/src/store/store.js | head -100
echo
echo "=== Check if test file has any setup/teardown ==="
cat -n frontend/src/store/store.test.js
echo
echo "=== Check if localStorage is cleared anywhere ==="
rg -n "localStorage|sessionStorage|clear|clearAll|reset" frontend/src/store/store.test.js
echo
echo "=== Check for test setup files ==="
fd -p '(setup|teardown|beforeAll|beforeEach)' frontend/srcRepository: debpalash/OmniVoice-Studio
Length of output: 201
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== List all files in store directory ==="
ls -la frontend/src/store/
echo
echo "=== Search for useAppStore definition ==="
rg -n "export const useAppStore" frontend/src
echo
echo "=== Show complete test file ==="
cat -n frontend/src/store/store.test.jsRepository: debpalash/OmniVoice-Studio
Length of output: 2869
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Full store index.ts ==="
cat -n frontend/src/store/index.ts
echo
echo "=== Check for reset methods in slices ==="
rg -n "reset" frontend/src/store/*.tsRepository: debpalash/OmniVoice-Studio
Length of output: 5109
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== DubSlice resetDubState implementation ==="
sed -n '160,170p' frontend/src/store/dubSlice.ts
echo
echo "=== Check all slice initial states ==="
rg -n "dubStep|text|mode|stage" frontend/src/store/*.ts | head -30
echo
echo "=== Verify if getState is exported or available ==="
rg -n "getState|api\." frontend/src/store/index.tsRepository: debpalash/OmniVoice-Studio
Length of output: 3387
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Look for text field definition ==="
rg -n "text:" frontend/src/store/*.ts
echo
echo "=== Check pillSlice full definition ==="
cat -n frontend/src/store/pillSlice.ts
echo
echo "=== Check for dubStep in dubSlice ==="
rg -n "dubStep:" frontend/src/store/dubSlice.ts | head -5Repository: debpalash/OmniVoice-Studio
Length of output: 4605
Add beforeEach hook to reset store state between tests.
These tests mutate the Zustand store (which uses localStorage persistence) without restoring baseline state. The mode and text mutations in tests 2–3 persist to localStorage, creating order-dependent failures as more tests are added. Add a beforeEach hook to clear localStorage or call a store reset method before each test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src/store/store.test.js` around lines 4 - 46, Add a beforeEach that
clears persisted state and resets the Zustand store before each test: call
localStorage.clear() and then import useAppStore and invoke its getState() reset
path (e.g. call an existing reset function or call getState().setMode(...) and
getState().setText(...) to restore defaults). Reference useAppStore,
useAppStore.getState(), setMode, setText (and keep dubStep/stage expectations
unchanged) so tests are order-independent.
- deploy/docker-compose.yml: remove profiles: ["cpu"] from the default service so `docker compose up` matches the comment on line 5. With the profile present, no service auto-started. - frontend/src/App.jsx: drop the setSeed call in restoreHistory. The selector was never reintroduced after the App.jsx hooks split, and there is no seed state in the store — seeds are generated fresh per call in useTTS and only read from history items for display. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ap fail-fast - backend/services/tts_backend.py: invert async-context detection in _ensure_loaded. The previous code unconditionally caught its own diagnostic RuntimeError and then called asyncio.run() inside a running loop, masking the intended error message. - frontend/src/hooks/useDubWorkflow.js: require a terminal `done` event before reporting dub success. Without this, a dropped stream after partial progress would flip the UI to `done`, refresh history, and play the completion ping as if generation finished. - frontend/src/hooks/useDubWorkflow.js: restore the previous step when tasksCancel() fails. The UI was getting stuck in `stopping` forever on cancel errors. - frontend/src-tauri/src/bootstrap.rs: fail-fast when source sync fails after the existing directory has already been removed. The previous warn-and-continue path could leave the install with no backend/ or omnivoice/ sources and defer the failure to backend startup with a cryptic error. - backend/api/routers/generation.py: add `from e` to the ValueError → HTTPException re-raise (Ruff B904). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The 100ms timer in useTTS was rewriting generationTime to a plain elapsed-seconds string, which immediately wiped the "(xx%)" download suffix written on the next iteration of the response-body loop. The real-time percentage was flickering on/off as a result. Read the previous value inside the setter and reattach any existing percent suffix. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
frontend/src/hooks/useTTS.js (1)
37-37: 💤 Low valueClear the interval on unmount as well.
timerRefis only cleared inside thefinallyblock ofhandleGenerate. If the consumer component unmounts mid-generation (e.g., user switches view), the interval keeps firing and pushes state updates into an unmounted tree. Add a one-shot effect to clear on unmount.♻️ Suggested cleanup
-import { useState, useRef, useCallback } from 'react'; +import { useState, useRef, useCallback, useEffect } from 'react'; @@ const timerRef = useRef(null); const textAreaRef = useRef(null); + + useEffect(() => () => { + if (timerRef.current) clearInterval(timerRef.current); + }, []);Also applies to: 141-144
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/hooks/useTTS.js` at line 37, The interval stored in timerRef (defined in useTTS.js) is only cleared inside the finally block of handleGenerate, so if the component unmounts mid-generation the interval continues firing; add a useEffect cleanup (a one-shot effect with [] deps) that checks timerRef.current and clears the interval (clearInterval) and sets timerRef.current = null on unmount, and also ensure any other places that set timerRef (e.g., the code around the other timer usage referenced near the end of the file) are consistent with this cleanup to avoid state updates on an unmounted component.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/src/hooks/useDubWorkflow.js`:
- Around line 321-322: The code reads the stream without validating the response
so non-2xx or no-body responses cause NPEs or obscure errors; before calling
streamRes.body.getReader() in useDubWorkflow.js (around the
tasksStreamUrl/data.task_id logic) check streamRes.ok and that streamRes.body is
non-null, and if not, read/parse the error payload (or at least include
streamRes.status and statusText) and throw or return a descriptive error so the
downstream reader/sawDone logic isn’t fed HTML/JSON or a null body; update the
branch that creates `reader` to handle non-ok responses and surface the real
failure instead of letting the generation stream parser swallow it.
- Around line 106-111: The error handler attached to evt currently closes and
rejects on any error event (using evt, gotFinal, close(), resolve(), reject()),
which treats transient EventSource reconnects as fatal; update the handler to
only close and reject when evt.readyState === EventSource.CLOSED (mirror the
_waitForPrep approach) and otherwise ignore transient errors so the SSE can
auto-reconnect — keep the existing logic that parses e.data for m.detail and
uses gotFinal to resolve, but gate the unconditional reject behind the
readyState CLOSED check.
---
Nitpick comments:
In `@frontend/src/hooks/useTTS.js`:
- Line 37: The interval stored in timerRef (defined in useTTS.js) is only
cleared inside the finally block of handleGenerate, so if the component unmounts
mid-generation the interval continues firing; add a useEffect cleanup (a
one-shot effect with [] deps) that checks timerRef.current and clears the
interval (clearInterval) and sets timerRef.current = null on unmount, and also
ensure any other places that set timerRef (e.g., the code around the other timer
usage referenced near the end of the file) are consistent with this cleanup to
avoid state updates on an unmounted component.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6e829d0b-13a2-4949-8ee8-a870c68660e6
📒 Files selected for processing (7)
backend/api/routers/generation.pybackend/services/tts_backend.pydeploy/docker-compose.ymlfrontend/src-tauri/src/bootstrap.rsfrontend/src/App.jsxfrontend/src/hooks/useDubWorkflow.jsfrontend/src/hooks/useTTS.js
✅ Files skipped from review due to trivial changes (1)
- deploy/docker-compose.yml
🚧 Files skipped from review as they are similar to previous changes (4)
- backend/services/tts_backend.py
- frontend/src-tauri/src/bootstrap.rs
- backend/api/routers/generation.py
- frontend/src/App.jsx
Comment on lines
+106
to
+111
| evt.addEventListener('error', (e) => { | ||
| try { const m = e.data ? JSON.parse(e.data) : null; if (m && m.detail) { close(); reject(new Error(m.detail)); return; } } catch {} | ||
| if (gotFinal) { close(); resolve(); return; } | ||
| close(); | ||
| reject(new Error('Transcribe stream dropped before emitting any segments. Likely ASR backend failed to load — check backend log + Settings → Models.')); | ||
| }); |
Contributor
There was a problem hiding this comment.
Transcribe SSE rejects on transient EventSource errors.
This error handler closes the stream and rejects on any error event, regardless of readyState. EventSource fires error on transient disconnects too and auto-reconnects (readyState=CONNECTING), so a momentary network blip during transcription will surface as a fatal "ASR backend failed to load" error and abort the workflow. Compare with _waitForPrep at lines 146–152, which guards with evt.readyState === EventSource.CLOSED before rejecting — that's the correct pattern.
🛠 Suggested fix
- evt.addEventListener('error', (e) => {
- try { const m = e.data ? JSON.parse(e.data) : null; if (m && m.detail) { close(); reject(new Error(m.detail)); return; } } catch {}
- if (gotFinal) { close(); resolve(); return; }
- close();
- reject(new Error('Transcribe stream dropped before emitting any segments. Likely ASR backend failed to load — check backend log + Settings → Models.'));
- });
+ evt.addEventListener('error', (e) => {
+ try { const m = e.data ? JSON.parse(e.data) : null; if (m && m.detail) { close(); ctrl.signal.removeEventListener('abort', onAbortSignal); reject(new Error(m.detail)); return; } } catch {}
+ // Native EventSource error: only treat as fatal once the connection is permanently closed.
+ if (evt.readyState !== EventSource.CLOSED) return;
+ if (gotFinal) { close(); ctrl.signal.removeEventListener('abort', onAbortSignal); resolve(); return; }
+ close();
+ ctrl.signal.removeEventListener('abort', onAbortSignal);
+ reject(new Error('Transcribe stream dropped before emitting any segments. Likely ASR backend failed to load — check backend log + Settings → Models.'));
+ });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src/hooks/useDubWorkflow.js` around lines 106 - 111, The error
handler attached to evt currently closes and rejects on any error event (using
evt, gotFinal, close(), resolve(), reject()), which treats transient EventSource
reconnects as fatal; update the handler to only close and reject when
evt.readyState === EventSource.CLOSED (mirror the _waitForPrep approach) and
otherwise ignore transient errors so the SSE can auto-reconnect — keep the
existing logic that parses e.data for m.detail and uses gotFinal to resolve, but
gate the unconditional reject behind the readyState CLOSED check.
Comment on lines
+321
to
+322
| const streamRes = await fetch(tasksStreamUrl(data.task_id)); | ||
| const reader = streamRes.body.getReader(); |
Contributor
There was a problem hiding this comment.
Check streamRes.ok / body before reading the generation stream.
If /tasks/<id>/stream returns a non-2xx (or a non-SSE) response — e.g. backend restart, auth failure, task not found — streamRes.body.getReader() either NPEs (body can be null on some error responses) or successfully reads HTML/JSON which is then silently swallowed in the data: parse loop. The new sawDone guard eventually throws Generation stream ended before completion, which masks the real cause. A quick streamRes.ok check surfaces the real status.
🛠 Suggested fix
const data = await dubGenerate(dubJobId, body);
setDubTaskId(data.task_id);
const streamRes = await fetch(tasksStreamUrl(data.task_id));
+ if (!streamRes.ok || !streamRes.body) {
+ const detail = await streamRes.text().catch(() => '');
+ throw new Error(`Generation stream failed (${streamRes.status})${detail ? `: ${detail.slice(0, 200)}` : ''}`);
+ }
const reader = streamRes.body.getReader();📝 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.
Suggested change
| const streamRes = await fetch(tasksStreamUrl(data.task_id)); | |
| const reader = streamRes.body.getReader(); | |
| const streamRes = await fetch(tasksStreamUrl(data.task_id)); | |
| if (!streamRes.ok || !streamRes.body) { | |
| const detail = await streamRes.text().catch(() => ''); | |
| throw new Error(`Generation stream failed (${streamRes.status})${detail ? `: ${detail.slice(0, 200)}` : ''}`); | |
| } | |
| const reader = streamRes.body.getReader(); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src/hooks/useDubWorkflow.js` around lines 321 - 322, The code reads
the stream without validating the response so non-2xx or no-body responses cause
NPEs or obscure errors; before calling streamRes.body.getReader() in
useDubWorkflow.js (around the tasksStreamUrl/data.task_id logic) check
streamRes.ok and that streamRes.body is non-null, and if not, read/parse the
error payload (or at least include streamRes.status and statusText) and throw or
return a descriptive error so the downstream reader/sawDone logic isn’t fed
HTML/JSON or a null body; update the branch that creates `reader` to handle
non-ok responses and surface the real failure instead of letting the generation
stream parser swallow it.
7 tasks
debpalash
added a commit
that referenced
this pull request
May 12, 2026
#50) * chore: post-refactor cleanup — wire fingerprints, drop dead code, scope pytest Follow-up to PR #49. Fixes residual issues from the App.jsx hooks split and tightens repo hygiene so a bare `pytest` doesn't foot-gun. Real bug - frontend/src/hooks/useDubWorkflow.js: setLastGenFingerprints lives in useSegmentEditing, not on the store. The previous code called useAppStore.getState().setLastGenFingerprints?.(...) — the optional chain swallowed the missing method, so the "N segments changed" badge never updated after a fresh generate until a project save+reopen. Thread setLastGenFingerprints in from App.jsx; useSegmentEditing() now runs before useDubWorkflow() to make the setter available. Dead code from the refactor - frontend/src/App.jsx: drop unused `showAllProjects` useState and `pushUndo` from the useSegmentEditing destructure. - frontend/src/hooks/useDubWorkflow.js: drop 5 unused selectors (preserveBg, defaultTrack, exportTracks, dualSubs, burnSubs) — the dub-download logic that needs these lives in App.jsx, not the hook. Repo hygiene - backend/api/routers/setup.py.bak: delete 38 KB tracked-in-git backup. The setup/ subpackage replacement has been in place for a while. - pyproject.toml: add [tool.pytest.ini_options] with testpaths + norecursedirs. Previously a bare `pytest` would INTERNALERROR walking into research/ (1.2 GB of vendored upstream projects with their own test_*.py files that call sys.exit at module level). - .github/workflows/ci.yml: run backend/tests/ as a second pytest invocation. The 23 tests there stub core.config in sys.modules to avoid the heavy main app import chain — that pollutes import state for other tests, so they need their own session. Previously these tests existed in the repo but never ran on CI. Net effect on lint: 60 → 52 problems (-8) from dead-code removal. Test counts unchanged: pytest 214 + 23, vitest 11. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore: log silent catch failures that mask real bugs CodeRabbit nitpick on #50: empty catch on the incremental-plan fallback swallows errors. Extending the fix to the catches in this area that have the same problem (a real failure would be invisible) while leaving the genuinely non-actionable cleanup catches alone (EventSource.close(), localStorage.setItem, fire-and-forget UI promises). Logged: - useDubWorkflow.js:97 — transcribe SSE message handler - useDubWorkflow.js:347 — incremental-plan fallback (the CR finding) - useDubWorkflow.js:352 — dub generate SSE event dispatch - App.jsx:552 — exportRecord on Tauri save path - App.jsx:580 — exportRecord on browser download path Left silent (cleanup / non-actionable): - useDubWorkflow.js:68, 112 — evt.close() in SSE teardown - useDubWorkflow.js:102 — SSE error-event payload parse fallback - App.jsx:124 — localStorage.setItem (quota / privacy mode) - App.jsx:789, 901 — fire-and-forget UI promise tails Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Merged
13 tasks
debpalash
added a commit
that referenced
this pull request
May 18, 2026
…ion) User report from a Pinokio/Windows install of OmniVoice flagged three real issues that affect every device target equally: 1. Docker: `docker compose --profile gpu up` port-conflicts on 3900. The CPU and GPU services both bind to the same host port. The Linux contributor's fix (profiles: ["cpu"]) was reverted in #49 on CodeRabbit's advice — that was the wrong call. Restoring the profile and updating the header comment + README to `--profile cpu up`. 2. Argos / pip install from the UI fails inside the Docker image with "No virtual environment found; run `uv venv`...". The Dockerfile sets UV_SYSTEM_PYTHON=1 but that env var governs `uv venv`, not `uv pip install`. Add a runtime `_in_virtualenv()` check; when running on system Python (Docker, bare metal), `run_pip` now injects --system after the install/uninstall subcommand. Inside a real venv we leave it off so installs land in the active environment. 3. Speaker diarization silently falls back to the silence-gap heuristic when the pyannote pipeline isn't available (no HF_TOKEN, license not accepted, network blocked). Symptom: a man↔woman exchange is detected as one speaker because the heuristic only fires on >1.2s silences. _diarize() now returns (segments, warning); dub_core yields a `warning` SSE event when the heuristic was used; useDubWorkflow renders it as a toast so users know to set HF_TOKEN. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
debpalash
added a commit
that referenced
this pull request
May 18, 2026
First v0.3.x release on the Phase 0 cross-platform CI baseline. ## Cross-platform bug fixes (375ea4e) User-reported bugs from a Pinokio/Windows session: - Docker `compose --profile gpu up` no longer port-conflicts on 3900 — restored `profiles: ["cpu"]` that #49 wrongly reverted on CodeRabbit's advice. - Argos / pip install from the UI now works inside Docker — added `_in_virtualenv()` runtime check; `run_pip` injects `--system` automatically when on system Python. - Speaker diarization warning toast — when pyannote silently falls back to the silence-gap heuristic (missing HF_TOKEN, license not accepted, network blocked), `_diarize()` now returns `(segments, warning)`; `useDubWorkflow` renders an 8-second toast. ## Dub editor UX (d5df454) Six fixes per annotated screenshots: - Editable segment start times (`m:ss.s` or raw seconds; Esc reverts, Enter commits; rejects overlap with end). - Click a transcript row → seek the waveform/video (`WaveformTimeline` now forwardRef's `seekTo(time)`). - Speaker is datalist-backed (pulls from detected speaker clones; free text still allowed). - Scissors menu splits at cursor — uses live caret, then last caret, then sentence-boundary fallback. - Mouse-wheel scrolls the waveform; Cmd/Ctrl left alone for browser pinch-zoom. - Menu popover collision: added `avoidCollisions` + `collisionPadding=8` to Radix Content; removed `position: fixed` from `.ui-menu`. ## VRAM-aware GPU pool (73dbe18) `_gpu_pool` was hardcoded `ThreadPoolExecutor(max_workers=1)` since introduction — every TTS forward serialized through one thread. - CUDA / ROCm: `workers = clamp(1, free_GB // 2.5, 4)`. 16 GB card with ~14 GB free → 4 workers → ~4× throughput on multi-segment dubs. - MPS / CPU / unknown: 1 worker. - `OMNIVOICE_GPU_WORKERS` env var override (clamped 1..16). - Module `__getattr__` preserves the public `_gpu_pool` symbol for existing callers. ## Stories tab — wire-up + UX (f6bbc7a) The 264-line `StoriesEditor` component existed but was mounted nowhere. Now wired into NavRail + lazy-loaded on `mode === 'stories'`. Added Paste & Split panel (sentence-boundary chunking) and per-track `[pause 0.5s]` insertion. ## Stories — pauses + inline voice (edd3a1d) `frontend/src/utils/storyTokens.js` — tokenizer for `[pause X.Ys]` and `[voice:X]…[voice:default]` markers. Voice switches are stateful (carry forward). 13 new vitest cases (vitest now 24/24). ## Verified - 214 backend tests pass (3 skipped, 10 xfailed, 3 xpassed) - 23 router-smoke tests pass - 24/24 vitest cases pass (13 new) - All 7 Phase 0 CI checks green (Tauri shell + Smoke on macOS/Windows/Linux + Tests) 🤖 Generated with [Claude Code](https://claude.com/claude-code)
This was referenced May 18, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Broad stability + refactor pass. Started as a DB connection-leak fix and grew to cover desktop bootstrap reliability, an App.jsx hooks split (2067 → 1129 LOC), CI test coverage, and dubbing UX polish.
Changes
Backend
services/ffmpeg_utils.py.Frontend — hooks refactor
App.jsx: 2067 → 1129 LOC (-45%) (e876e7c)useAppData,useDubWorkflow,useProfiles,useTTSisTauricentralizedDesktop bootstrap
FileNotFoundErrorduring model init in bundled builds (4f83edb)Testing / CI
checkJsadded (c277221)useProfiles(b22f42a)pytest-covwired up (59d454a)Deploy
Test plan
pytestcleanbun run test/ Vitest cleantsc --noEmitcleanNotes
Branch name (
fix/db-connection-leaks-and-audit) understates the scope; happy to split if reviewers prefer, but the desktop bootstrap and frontend refactor changes interact with the DB-safety work in places (hook lifecycles, subprocess cleanup), so a combined review is probably faster.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores