fix: harden input validation, file safety, and error handling#227
Open
ankit1999 wants to merge 8 commits intojamiepine:mainfrom
Open
fix: harden input validation, file safety, and error handling#227ankit1999 wants to merge 8 commits intojamiepine:mainfrom
ankit1999 wants to merge 8 commits intojamiepine:mainfrom
Conversation
- New custom_models.py module for CRUD management of user-defined HF TTS models - New /custom-models API endpoints (list, add, get, delete) - Updated MLX and PyTorch backends to resolve custom model paths (custom:slug format) - Added Custom Models section to ModelManagement UI with add/remove dialogs - Updated GenerationForm and FloatingGenerateBox with grouped model selectors - Added CustomModelCreate/Response types and API client methods - Added instruct field to GenerationRequest type - Graceful actool fallback in build.rs for non-Xcode environments - Added custom_models hidden import for PyInstaller bundling Author: AJ - Kamyab (Ankit Jain)
- ModelManagement.tsx: validate model_name format before removal, guard duplicate clicks - client.ts: URL-encode modelId in removeCustomModel path - useGenerationForm.ts: constrain modelSize with regex pattern - useModelStatus.ts: extract shared hook from GenerationForm/FloatingGenerateBox - custom_models.py: atomic writes, threading lock, corrupt config backup, strict hf_repo_id regex - models.py: add regex pattern to model_size Field - config.py: platform-appropriate default data dir for PyInstaller bundles - main.py: re-raise HTTPException before broad except to preserve 202 responses - custom_models.json: remove pre-seeded entry for clean installs - requirements.txt: add platformdirs dependency - backend/README.md: document --data-dir CLI flag
Contributor
📝 WalkthroughWalkthroughThis pull request introduces custom model management functionality, enabling users to register HuggingFace TTS models via the UI. It includes persistent metadata storage, API endpoints, form validation updates, and backend model resolution logic across MLX and PyTorch backends. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User / UI
participant Frontend as Frontend API Client
participant Backend as Backend Server
participant Storage as Custom Models Storage
participant HF as HuggingFace Hub
User->>Frontend: Click "Add Custom Model"
Frontend->>User: Show add dialog
User->>Frontend: Enter repo ID + display name
Frontend->>Backend: POST /custom-models {hf_repo_id, display_name}
Backend->>Storage: add_custom_model(hf_repo_id, display_name)
Storage->>Storage: Validate HF repo ID format
Storage->>Storage: Generate slug from repo ID
Storage->>Storage: Check for duplicates
Storage->>Storage: Persist to JSON atomically
Storage-->>Backend: Return CustomModelResponse
Backend-->>Frontend: 200 with model metadata
Frontend->>User: Close dialog, refresh model list
Frontend->>Backend: GET /models/status
Backend->>Storage: list_custom_models()
Storage-->>Backend: Return custom model list
Backend->>HF: Query HF cache for model files
Backend-->>Frontend: ModelStatus[] with custom models
Frontend->>User: Display custom models in management UI
sequenceDiagram
participant User as User / UI
participant Frontend as Frontend Form
participant Schema as Validation Schema
participant Backend as Backend Server
participant ModelResolver as Model Loader (MLX/PyTorch)
participant HF as HuggingFace Hub
User->>Frontend: Select/enter custom model size
Frontend->>Schema: Validate modelSize: "custom:user/model"
Schema->>Schema: Check regex: custom:[a-z0-9]...
Schema-->>Frontend: ✓ Valid
User->>Frontend: Submit generation request
Frontend->>Backend: POST /generate {model_size: "custom:user/model", ...}
Backend->>ModelResolver: _get_model_path("custom:user/model")
ModelResolver->>Storage: get_hf_repo_id_for_custom_model("user/model")
Storage-->>ModelResolver: "user/model" (HF repo ID)
ModelResolver->>HF: Load model from "user/model"
HF-->>ModelResolver: Model weights
ModelResolver-->>Backend: Local model path
Backend->>Backend: Generate TTS with custom model
Backend-->>Frontend: Audio response
Frontend->>User: Play generated audio
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/lib/hooks/useGenerationForm.ts (1)
120-132:⚠️ Potential issue | 🟠 MajorHandle download-pending responses before using
resultasGenerationResponse.This block assumes
result.durationexists. When backend returns 202 download-pending payload, this can crash the success path.✅ Safer flow: early-return on undownloaded model
try { const modelStatus = await apiClient.getModelStatus(); const model = modelStatus.models.find((m) => m.model_name === modelName); if (model) { displayName = model.display_name; if (!model.downloaded) { // Not yet downloaded — enable progress tracking UI setDownloadingModelName(modelName); setDownloadingDisplayName(displayName); + if (!model.downloading) { + await apiClient.triggerModelDownload(modelName); + } + toast({ + title: 'Model download in progress', + description: `${displayName} is downloading. Try generating again when ready.`, + }); + return; } } } catch (error) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/lib/hooks/useGenerationForm.ts` around lines 120 - 132, The success handler assumes result is a fully available GenerationResponse and accesses result.duration, which will crash if the backend returned a 202 download-pending payload; update the flow in useGenerationForm where generation.mutateAsync is awaited (the block that constructs toast with result.duration) to first detect the download-pending response (e.g., check a status field or absence of duration/download URL on the returned object) and early-return or show a "download pending" toast instead of accessing result.duration; ensure subsequent logic only runs when result.duration exists (guard on result.duration or result.status === 'ready') so you never dereference duration on a pending response.
🧹 Nitpick comments (5)
backend/config.py (1)
50-57: Consider ensuring the data directory exists inget_data_dir().Unlike
set_data_dir()and the subdirectory getters (e.g.,get_profiles_dir()),get_data_dir()does not callmkdir(). If called beforeset_data_dir()and the directory doesn't exist, callers may encounter errors when writing files.🔧 Proposed fix
def get_data_dir() -> Path: """ Get the data directory path. Returns: Path to the data directory """ + _data_dir.mkdir(parents=True, exist_ok=True) return _data_dir🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/config.py` around lines 50 - 57, get_data_dir() should ensure the underlying directory exists like set_data_dir() and get_profiles_dir() do: before returning the module-level _data_dir Path, call _data_dir.mkdir(parents=True, exist_ok=True) (or equivalent) to create the directory if missing so callers won’t fail when writing files; update the get_data_dir() function to perform this mkdir step and then return _data_dir.backend/backends/mlx_backend.py (1)
40-65: Consider extracting shared custom model resolution logic.The custom model resolution code (lines 40-49) is nearly identical to
pytorch_backend.py(lines 65-73). While this duplication is minimal and acceptable, you could extract it to a shared utility if this pattern expands to more backends.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/backends/mlx_backend.py` around lines 40 - 65, The duplicate custom-model resolution block (checking model_size.startswith("custom:"), extracting custom_id, calling get_hf_repo_id_for_custom_model and raising ValueError) should be extracted into a shared helper so both mlx_backend and pytorch_backend reuse it; create a utility function (e.g., resolve_custom_model_hf_repo or get_hf_repo_id_from_model_size) that accepts model_size, imports/calls get_hf_repo_id_for_custom_model, returns the hf_repo_id or raises the ValueError, and replace the inline code in mlx_backend (the model_size.startswith("custom:") branch) and the corresponding code in pytorch_backend with calls to this new helper; keep existing behavior/logging (print) and preserve mlx_model_map/other logic.backend/custom_models.py (1)
47-62: Uselogger.exceptionto include traceback in error logs.Within exception handlers,
logger.exceptionautomatically includes the traceback, which is more useful for debugging thanlogger.error.♻️ Proposed fix
except json.JSONDecodeError as exc: # Back up the corrupt file so we don't lose data backup = path.with_suffix( f".json.corrupt.{datetime.utcnow().strftime('%Y%m%dT%H%M%S')}" ) try: path.rename(backup) - logger.error( + logger.exception( "Corrupt custom_models.json backed up to %s: %s", backup, exc ) except OSError as rename_err: - logger.error( + logger.exception( "Failed to back up corrupt config %s: %s (original error: %s)", path, rename_err, exc, ) raise🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/custom_models.py` around lines 47 - 62, Replace the logger.error calls inside the JSON decode and nested OSError handlers with logger.exception so the traceback is recorded: in the except json.JSONDecodeError as exc block, change the logger.error("Corrupt custom_models.json backed up to %s: %s", backup, exc) to logger.exception(...) and in the inner except OSError as rename_err block change logger.error("Failed to back up corrupt config %s: %s (original error: %s)", path, rename_err, exc) to logger.exception(...) (preserving the descriptive messages and context) so the full tracebacks for the JSONDecodeError and the rename error are included.backend/main.py (1)
1546-1552: Don’t silently swallow cache-probe exceptions in model status paths.Current
except Exception: passbehavior hides actionable failures. Log at debug level so support can diagnose cache-state inconsistencies.🧭 Minimal observability patch
+import logging +logger = logging.getLogger(__name__) @@ - except Exception: - pass + except Exception: + logger.debug("Custom model cache probe failed (incomplete check): %s", hf_repo_id, exc_info=True) @@ - except Exception: - pass + except Exception: + logger.debug("Custom model size computation failed: %s", hf_repo_id, exc_info=True) @@ - except Exception: - pass - except Exception: - pass + except Exception: + logger.debug("Custom model fallback size computation failed: %s", hf_repo_id, exc_info=True) + except Exception: + logger.debug("Custom model fallback cache probe failed: %s", hf_repo_id, exc_info=True) @@ - except Exception: - pass + except Exception: + logger.debug("Custom model loaded-state check failed: %s", model_name, exc_info=True) @@ - except Exception: + except Exception: + logger.debug("Custom model status assembly failed: %s", model_name, exc_info=True) is_downloading = model_name in active_download_namesAlso applies to: 1557-1560, 1584-1593, 1598-1601, 1618-1628
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/main.py` around lines 1546 - 1552, Replace silent except blocks that currently do "except Exception: pass" in the model status cache probes (the blocks that compute cache_dir_path, blobs_dir and has_incomplete using hf_constants.HF_HUB_CACHE and hf_repo_id) with exception handlers that call the module logger at debug level including a descriptive message and the exception details (e.g., "failed probing HF cache for repo {hf_repo_id}, cache_dir={cache_dir_path}") so failures aren't swallowed; make the same change for the other similar blocks flagged (the ones around lines computing blobs_dir/has_incomplete in the same function) to ensure consistent debug-level observability while preserving existing control flow.app/src/components/Generation/GenerationForm.tsx (1)
146-174: Extract shared model-option rendering/mapping to avoid drift.The built-in/custom select rendering and
model_name→sizeValuemapping is duplicated inFloatingGenerateBox.tsx; fallback text has already started diverging. Consider a shared helper/component for this block.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/Generation/GenerationForm.tsx` around lines 146 - 174, The built-in/custom model option rendering and the model_name→sizeValue mapping in GenerationForm (see builtInModels, customModels, SelectGroup/SelectItem usage and the sizeValue = model.model_name.replace('qwen-tts-', '')) are duplicated in FloatingGenerateBox; extract a shared helper or small presentational component (e.g., renderModelOptions or ModelSelectGroup component and a mapModelNameToSize utility) that returns the same SelectGroup/SelectItem structure and performs the model_name→sizeValue transformation, then replace the duplicated loops in both GenerationForm.tsx and FloatingGenerateBox.tsx to call the new shared function/component so fallback text and mapping remain consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/components/Generation/GenerationForm.tsx`:
- Line 37: The hook useModelStatus currently classifies builtInModels using
model_name.startsWith('qwen-tts'), which incorrectly flags repos like
"qwen-tts/my-voice" as built-in; update the predicate in useModelStatus (the
filter that builds builtInModels/customModels) to only treat true built-ins such
as the core qwen-tts identifier (e.g., model_name === 'qwen-tts' or match a
pattern that disallows a following '/'), for example replace
startsWith('qwen-tts') with a stricter check (exact equality or a regex like
/^qwen-tts($|[:@])/) so repo-qualified names with a slash go to customModels.
In `@app/src/components/ServerSettings/ModelManagement.tsx`:
- Around line 617-623: The Remove button currently disables only when
model.loaded or isUnregistering is true; update the disable condition for the
Button (the component with onClick={onRemove}) to also check the showDownloading
flag so removal is prevented while a download is active. Locate the Button that
uses onRemove and the disabled prop, and add showDownloading (or its local/state
variable) into the combined disabled expression alongside model.loaded and
isUnregistering.
In `@backend/custom_models.py`:
- Line 10: Remove the unused fcntl import from the top of custom_models.py
(delete the `import fcntl` statement), verify there are no remaining references
to fcntl in the file (the module already uses threading.Lock for
synchronization), and run tests/linting to confirm no regressions.
In `@backend/voicebox-server.spec`:
- Around line 16-19: The spec unconditionally calls collect_all('mlx') and
collect_all('mlx_audio') which fails when MLX isn't installed; wrap those
collect_all calls in a runtime presence check (e.g., try/except ImportError or
importlib.util.find_spec) before invoking collect_all so that if 'mlx' or
'mlx_audio' is missing you skip updating tmp_ret/datas/binaries/hiddenimports.
Specifically, guard the collect_all('mlx') and collect_all('mlx_audio') blocks
(the places that assign tmp_ret and then do datas += tmp_ret[0]; binaries +=
tmp_ret[1]; hiddenimports += tmp_ret[2]) so they only run when the package is
importable.
---
Outside diff comments:
In `@app/src/lib/hooks/useGenerationForm.ts`:
- Around line 120-132: The success handler assumes result is a fully available
GenerationResponse and accesses result.duration, which will crash if the backend
returned a 202 download-pending payload; update the flow in useGenerationForm
where generation.mutateAsync is awaited (the block that constructs toast with
result.duration) to first detect the download-pending response (e.g., check a
status field or absence of duration/download URL on the returned object) and
early-return or show a "download pending" toast instead of accessing
result.duration; ensure subsequent logic only runs when result.duration exists
(guard on result.duration or result.status === 'ready') so you never dereference
duration on a pending response.
---
Nitpick comments:
In `@app/src/components/Generation/GenerationForm.tsx`:
- Around line 146-174: The built-in/custom model option rendering and the
model_name→sizeValue mapping in GenerationForm (see builtInModels, customModels,
SelectGroup/SelectItem usage and the sizeValue =
model.model_name.replace('qwen-tts-', '')) are duplicated in
FloatingGenerateBox; extract a shared helper or small presentational component
(e.g., renderModelOptions or ModelSelectGroup component and a mapModelNameToSize
utility) that returns the same SelectGroup/SelectItem structure and performs the
model_name→sizeValue transformation, then replace the duplicated loops in both
GenerationForm.tsx and FloatingGenerateBox.tsx to call the new shared
function/component so fallback text and mapping remain consistent.
In `@backend/backends/mlx_backend.py`:
- Around line 40-65: The duplicate custom-model resolution block (checking
model_size.startswith("custom:"), extracting custom_id, calling
get_hf_repo_id_for_custom_model and raising ValueError) should be extracted into
a shared helper so both mlx_backend and pytorch_backend reuse it; create a
utility function (e.g., resolve_custom_model_hf_repo or
get_hf_repo_id_from_model_size) that accepts model_size, imports/calls
get_hf_repo_id_for_custom_model, returns the hf_repo_id or raises the
ValueError, and replace the inline code in mlx_backend (the
model_size.startswith("custom:") branch) and the corresponding code in
pytorch_backend with calls to this new helper; keep existing behavior/logging
(print) and preserve mlx_model_map/other logic.
In `@backend/config.py`:
- Around line 50-57: get_data_dir() should ensure the underlying directory
exists like set_data_dir() and get_profiles_dir() do: before returning the
module-level _data_dir Path, call _data_dir.mkdir(parents=True, exist_ok=True)
(or equivalent) to create the directory if missing so callers won’t fail when
writing files; update the get_data_dir() function to perform this mkdir step and
then return _data_dir.
In `@backend/custom_models.py`:
- Around line 47-62: Replace the logger.error calls inside the JSON decode and
nested OSError handlers with logger.exception so the traceback is recorded: in
the except json.JSONDecodeError as exc block, change the logger.error("Corrupt
custom_models.json backed up to %s: %s", backup, exc) to logger.exception(...)
and in the inner except OSError as rename_err block change logger.error("Failed
to back up corrupt config %s: %s (original error: %s)", path, rename_err, exc)
to logger.exception(...) (preserving the descriptive messages and context) so
the full tracebacks for the JSONDecodeError and the rename error are included.
In `@backend/main.py`:
- Around line 1546-1552: Replace silent except blocks that currently do "except
Exception: pass" in the model status cache probes (the blocks that compute
cache_dir_path, blobs_dir and has_incomplete using hf_constants.HF_HUB_CACHE and
hf_repo_id) with exception handlers that call the module logger at debug level
including a descriptive message and the exception details (e.g., "failed probing
HF cache for repo {hf_repo_id}, cache_dir={cache_dir_path}") so failures aren't
swallowed; make the same change for the other similar blocks flagged (the ones
around lines computing blobs_dir/has_incomplete in the same function) to ensure
consistent debug-level observability while preserving existing control flow.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
bun.lockis excluded by!**/*.locktauri/src-tauri/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
app/src/components/Generation/FloatingGenerateBox.tsxapp/src/components/Generation/GenerationForm.tsxapp/src/components/ServerSettings/ModelManagement.tsxapp/src/lib/api/client.tsapp/src/lib/api/types.tsapp/src/lib/hooks/useGenerationForm.tsapp/src/lib/hooks/useModelStatus.tsbackend/README.mdbackend/backends/mlx_backend.pybackend/backends/pytorch_backend.pybackend/build_binary.pybackend/config.pybackend/custom_models.pybackend/main.pybackend/models.pybackend/requirements.txtbackend/voicebox-server.specdata/custom_models.jsontauri/src-tauri/build.rs
| const { form, handleSubmit, isPending } = useGenerationForm(); | ||
|
|
||
| // Use shared hook for model status fetching and grouping | ||
| const { builtInModels, customModels } = useModelStatus(); |
Contributor
There was a problem hiding this comment.
Prevent built-in/custom option overlap from hook classification.
builtInModels from useModelStatus is based on model_name.startsWith('qwen-tts'). A custom repo like qwen-tts/my-voice can appear in both groups and be misinterpreted in built-in mapping.
🔧 Proposed fix (in app/src/lib/hooks/useModelStatus.ts)
-const builtInModels =
- modelStatus?.models.filter((m) => m.model_name.startsWith('qwen-tts')) || [];
+const builtInModels =
+ modelStatus?.models.filter((m) => !m.is_custom && m.model_name.startsWith('qwen-tts-')) || [];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/components/Generation/GenerationForm.tsx` at line 37, The hook
useModelStatus currently classifies builtInModels using
model_name.startsWith('qwen-tts'), which incorrectly flags repos like
"qwen-tts/my-voice" as built-in; update the predicate in useModelStatus (the
filter that builds builtInModels/customModels) to only treat true built-ins such
as the core qwen-tts identifier (e.g., model_name === 'qwen-tts' or match a
pattern that disallows a following '/'), for example replace
startsWith('qwen-tts') with a stricter check (exact equality or a regex like
/^qwen-tts($|[:@])/) so repo-qualified names with a slash go to customModels.
Comment on lines
+617
to
+623
| <Button | ||
| size="sm" | ||
| onClick={onRemove} | ||
| variant="ghost" | ||
| title="Remove custom model from list" | ||
| disabled={model.loaded || isUnregistering} | ||
| > |
Contributor
There was a problem hiding this comment.
Disable “remove custom model” while download is active.
Unregistering during showDownloading can create inconsistent UX/state around in-flight downloads.
🛠️ Small UI guard improvement
<Button
size="sm"
onClick={onRemove}
variant="ghost"
title="Remove custom model from list"
- disabled={model.loaded || isUnregistering}
+ disabled={model.loaded || isUnregistering || showDownloading}
>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/components/ServerSettings/ModelManagement.tsx` around lines 617 -
623, The Remove button currently disables only when model.loaded or
isUnregistering is true; update the disable condition for the Button (the
component with onClick={onRemove}) to also check the showDownloading flag so
removal is prevented while a download is active. Locate the Button that uses
onRemove and the disabled prop, and add showDownloading (or its local/state
variable) into the combined disabled expression alongside model.loaded and
isUnregistering.
| @author AJ - Kamyab (Ankit Jain) | ||
| """ | ||
|
|
||
| import fcntl |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg 'fcntl' backend/custom_models.pyRepository: jamiepine/voicebox
Length of output: 74
🏁 Script executed:
rg 'fcntl[\.(\s]' backend/custom_models.pyRepository: jamiepine/voicebox
Length of output: 44
🏁 Script executed:
head -20 backend/custom_models.pyRepository: jamiepine/voicebox
Length of output: 467
Remove unused fcntl import on line 10.
The fcntl module is imported but never used in this file. Since fcntl is Unix-only, this import will cause an ImportError on Windows. The module already uses threading.Lock for synchronization, making fcntl unnecessary.
Proposed fix
-import fcntl
import json
import logging📝 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
| import fcntl | |
| import json | |
| import logging |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/custom_models.py` at line 10, Remove the unused fcntl import from the
top of custom_models.py (delete the `import fcntl` statement), verify there are
no remaining references to fcntl in the file (the module already uses
threading.Lock for synchronization), and run tests/linting to confirm no
regressions.
Comment on lines
+16
to
+19
| tmp_ret = collect_all('mlx') | ||
| datas += tmp_ret[0]; binaries += tmp_ret[1]; hiddenimports += tmp_ret[2] | ||
| tmp_ret = collect_all('mlx_audio') | ||
| datas += tmp_ret[0]; binaries += tmp_ret[1]; hiddenimports += tmp_ret[2] |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
git ls-files | head -20Repository: jamiepine/voicebox
Length of output: 446
🏁 Script executed:
find . -name "*.spec" -o -name "*requirements*.txt" -o -name "pyproject.toml" -o -name "setup.cfg" | grep -v ".git" | head -20Repository: jamiepine/voicebox
Length of output: 169
🏁 Script executed:
ls -la backend/ 2>/dev/null || echo "backend dir not found"Repository: jamiepine/voicebox
Length of output: 1843
🏁 Script executed:
# Check if the spec file exists and examine it
if [ -f "backend/voicebox-server.spec" ]; then
echo "=== backend/voicebox-server.spec ==="
head -30 backend/voicebox-server.spec
fiRepository: jamiepine/voicebox
Length of output: 1802
🏁 Script executed:
echo "=== backend/requirements.txt ===" && cat backend/requirements.txt && echo && \
echo "=== backend/requirements-mlx.txt ===" && cat backend/requirements-mlx.txtRepository: jamiepine/voicebox
Length of output: 672
🏁 Script executed:
echo "=== Build workflows ===" && \
for f in .github/workflows/*.yml; do
echo "--- $f ---"
cat "$f" | head -50
doneRepository: jamiepine/voicebox
Length of output: 2778
🏁 Script executed:
echo "=== Check Makefile for build commands ===" && \
cat Makefile 2>/dev/null | head -80Repository: jamiepine/voicebox
Length of output: 3156
🏁 Script executed:
# Verify PyInstaller collect_all behavior and check if error handling exists in build_binary.py
echo "=== backend/build_binary.py ===" && \
head -50 backend/build_binary.py && \
echo "..." && \
tail -30 backend/build_binary.pyRepository: jamiepine/voicebox
Length of output: 2784
🏁 Script executed:
# Check if there's any error handling around collect_all in the repo
rg -n "collect_all|try.*collect" backend/Repository: jamiepine/voicebox
Length of output: 268
Guard MLX collect_all calls for environments where MLX is absent.
The spec file unconditionally calls collect_all('mlx') and collect_all('mlx_audio') (lines 16–19), but MLX is declared as Apple Silicon-only in requirements-mlx.txt and conditionally installed only on aarch64-apple-darwin. Windows and Intel Mac CI builds skip MLX dependencies, causing PyInstaller to fail when the spec file is executed on these platforms. Add package-presence guards before collecting.
Suggested fix
+from importlib.util import find_spec
+
tmp_ret = collect_all('mlx')
datas += tmp_ret[0]; binaries += tmp_ret[1]; hiddenimports += tmp_ret[2]
tmp_ret = collect_all('mlx_audio')
datas += tmp_ret[0]; binaries += tmp_ret[1]; hiddenimports += tmp_ret[2]
+if find_spec("mlx") is not None:
+ _d, _b, _h = collect_all("mlx")
+ datas += _d
+ binaries += _b
+ hiddenimports += _h
+
+if find_spec("mlx_audio") is not None:
+ _d, _b, _h = collect_all("mlx_audio")
+ datas += _d
+ binaries += _b
+ hiddenimports += _h📝 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
| tmp_ret = collect_all('mlx') | |
| datas += tmp_ret[0]; binaries += tmp_ret[1]; hiddenimports += tmp_ret[2] | |
| tmp_ret = collect_all('mlx_audio') | |
| datas += tmp_ret[0]; binaries += tmp_ret[1]; hiddenimports += tmp_ret[2] | |
| from importlib.util import find_spec | |
| if find_spec("mlx") is not None: | |
| _d, _b, _h = collect_all("mlx") | |
| datas += _d | |
| binaries += _b | |
| hiddenimports += _h | |
| if find_spec("mlx_audio") is not None: | |
| _d, _b, _h = collect_all("mlx_audio") | |
| datas += _d | |
| binaries += _b | |
| hiddenimports += _h |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/voicebox-server.spec` around lines 16 - 19, The spec unconditionally
calls collect_all('mlx') and collect_all('mlx_audio') which fails when MLX isn't
installed; wrap those collect_all calls in a runtime presence check (e.g.,
try/except ImportError or importlib.util.find_spec) before invoking collect_all
so that if 'mlx' or 'mlx_audio' is missing you skip updating
tmp_ret/datas/binaries/hiddenimports. Specifically, guard the collect_all('mlx')
and collect_all('mlx_audio') blocks (the places that assign tmp_ret and then do
datas += tmp_ret[0]; binaries += tmp_ret[1]; hiddenimports += tmp_ret[2]) so
they only run when the package is importable.
Resolved 16 merge conflicts to integrate custom model management into the new refactored architecture: Frontend: - Removed inline modelSize selectors in FloatingGenerateBox and GenerationForm in favor of the unified EngineModelSelector - Merged model_size typing (string for custom: slugs) with origin/main's GenerationRequest properties (engine, effects_chain, etc.) - Preserved custom model CRUD in ModelManagement with new modal UI - Updated useGenerationForm to handle custom model identifiers within the new multi-engine architecture Backend: - Accepted origin/main's modular app.py + routes architecture - Injected custom model status into routes/models.py get_model_status - Added custom model CRUD endpoints (/custom-models) to the models router - Merged platformdirs fallback into config.py path resolution - Extended model_size regex to accept 1B, 3B sizes alongside custom: slugs - Combined hidden imports in build_binary.py and voicebox-server.spec
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/src/lib/api/client.ts (1)
3-22:⚠️ Potential issue | 🟠 MajorDuplicate
ActiveTasksResponseimport.
ActiveTasksResponseis imported on both line 4 and line 19 (Biome flags this as a parse error). Drop the second occurrence; the other added types (CustomModelCreate,CustomModelResponse,CustomModelListResponse) should stay, ideally merged alphabetically into the existing sorted import list.🛠️ Proposed fix
import type { - ActiveTasksResponse, ApplyEffectsRequest, AvailableEffectsResponse, + ActiveTasksResponse, + CustomModelCreate, + CustomModelListResponse, + CustomModelResponse, CudaStatus, EffectConfig, EffectPresetCreate, EffectPresetResponse, GenerationRequest, GenerationResponse, GenerationVersionResponse, HealthResponse, HistoryListResponse, HistoryQuery, HistoryResponse, ModelDownloadRequest, - ActiveTasksResponse, - CustomModelCreate, - CustomModelResponse, - CustomModelListResponse, ModelStatusListResponse,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/lib/api/client.ts` around lines 3 - 22, The import list contains a duplicate ActiveTasksResponse symbol; remove the second occurrence and merge the new types (CustomModelCreate, CustomModelResponse, CustomModelListResponse) into the existing sorted import block so the list remains alphabetized and deduplicated (update the top-level import that currently declares ActiveTasksResponse, ApplyEffectsRequest, ..., GenerationResponse, etc., to include the three CustomModel* types in the correct alphabetical position and omit the duplicate ActiveTasksResponse).app/src/components/ServerSettings/ModelManagement.tsx (1)
1-1186:⚠️ Potential issue | 🔴 CriticalThis file is structurally broken and will not compile.
Biome is already flagging five parse errors, but there are far more blocking issues — this file as posted cannot type-check or run. Key problems:
Imports are corrupted (lines 3-64).
- Line 3 concatenates two imports on one line (
...} from 'react';import {) and the secondimport {never gets a matching source before intervening imports appear.- Lines 19-25 (
DialogFooter, DialogHeader, DialogTitle, } from '@/components/ui/dialog';plusInput/Label/Progress) have no openingimport {— the dialog named imports (Dialog,DialogContent,DialogDescription) are missing entirely, yet they're used at lines 714-715, 1147-1148, etc.- Line 64 (
import { usePlatform } from '@/platform/PlatformContext';) and line 65 (useServerStore) are placed inside theModelManagementfunction body opened at line 39. Imports cannot appear inside a function.
ModelManagementnever closes cleanly. Top-level declarations likefetchHuggingFaceModelInfo,MODEL_DESCRIPTIONS,formatDownloads,formatLicense,formatPipelineTag,formatBytes(lines 67-123) are nested insideModelManagement()because the function opened at line 39 has no matching close before them. Corresponding closing}then never balances.Multiple statements on one line where newlines were lost.
- Line 318: two
await queryClient.*calls on a single line.- Line 343:
title: 'Unload failed', description: error.message,— missing newline between object properties.- Line 410:
.replace('license:', ''); return (— return statement merged with the previous assignment.- Line 1030:
{pendingMigrateDir} </div>— closing tag merged with text.Undefined references.
modelStatus,isLoading(used at lines 231, 377-378, 387, 390, 496, 500, 502) —useModelStatus()is never imported or called, and nouseQuery(['modelStatus'])exists in this file either.addCustomModelMutation(lines 357, 1177, 1178) andremoveCustomModelMutation(lines 683, 694, 697, 938, 940, 948) are used but never declared.CustomModelItem(line 669) is used but neither imported nor defined.DialogContent,DialogDescription,Dialog(lines 714-715, 720, 1147-1148, 1151) are not imported.JSX mismatched. Lines 710-711 close with
</div> ) : null} </CardContent>but no<Card>/<CardContent>ever opens in this file. The<Dialog open={addDialogOpen}>block at lines 1147-1183 is emitted afterModelManagementcloses at line 1145, making it orphaned top-level JSX followed by another stray</div>,), and}at 1184-1186.Duplicated "Custom Models" UI. The sections loop at lines 502-586 already renders a "Custom Models" subsection (with
section.isCustomAdd button at 508-512), and then lines 651-707 render another full "Custom Models" section, seemingly copy-pasted from an earlier version. One of the two needs to go.This looks like the result of a messy merge/patch —
tsc --noEmitwould not pass on this file despite what the PR description claims. Please re-apply the intended hunks cleanly (reconstruct imports, ensure the function body/JSX balances, wire upuseModelStatus, declare the missing mutations, and either define or importCustomModelItem) and re-runtsc --noEmitbefore merging.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/ServerSettings/ModelManagement.tsx` around lines 1 - 1186, The file is syntactically broken due to mangled imports, misplaced top-level declarations inside the ModelManagement function, missing hook/mutation declarations, duplicate UI blocks, and several merged lines — fix by: 1) move and reconstruct all import statements to the top (include Dialog, DialogContent, DialogDescription, DialogFooter/DialogHeader/DialogTitle, CustomModelItem, useModelStatus/usePlatform/useServerStore), removing the stray concatenated import fragments; 2) ensure ModelManagement() opens and closes correctly so helper functions (fetchHuggingFaceModelInfo, MODEL_DESCRIPTIONS, formatDownloads/formatLicense/formatPipelineTag/formatBytes) are declared outside the component or moved above the component and not nested inside it; 3) declare or create the missing React Query mutations/variables (addCustomModelMutation, removeCustomModelMutation, modelStatus/isLoading via useModelStatus or a useQuery for ['modelStatus']), and wire up CustomModelItem prop types; 4) remove the duplicated "Custom Models" UI block (keep only the sections-based rendering) and fix merged statements/newlines (split combined await/queryClient calls and object properties like onError descriptions) so JSX tags (CardContent/Dialog/DialogContent/etc.) are balanced; 5) run tsc --noEmit and eslint/formatter to catch any remaining parse/type errors before committing.
🧹 Nitpick comments (4)
backend/backends/pytorch_backend.py (1)
54-63: Custom model resolution looks correct.Logic correctly handles the
custom:prefix before falling through to the built-in map, and raisesValueErrorconsistent with the unknown-size error path below, so_load_model_sync/_is_model_cachedcallers see a uniform failure mode. The lazy import of..custom_modelsavoids potential circular-import risk at module load.One minor nit: the
#@ModifiedAJ - Kamyab (Ankit Jain)attribution comment on line 55 is better tracked via VCS (git blame) than inline; consider dropping it to keep the source clean.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/backends/pytorch_backend.py` around lines 54 - 63, Remove the inline attribution comment "# `@modified` AJ - Kamyab (Ankit Jain) — Added custom model path resolution" to keep the source clean and rely on VCS history; leave the surrounding logic intact (the model_size.startswith("custom:") branch that computes custom_id, calls get_hf_repo_id_for_custom_model(custom_id), and raises ValueError when not found) so callers like _load_model_sync and _is_model_cached continue to see the same failure behavior.app/src/lib/api/client.ts (1)
459-463: Minor: indentation insideremoveCustomModelbody.Line 460 is indented at 6 spaces (new code) but lines 461-463 retain the surrounding 4-space indentation, making the closing brace misalign with the return. Not a correctness issue, but worth normalizing before merge.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/lib/api/client.ts` around lines 459 - 463, The indentation inside the removeCustomModel method is inconsistent: adjust the indentation so the return statement and the closing brace align with the surrounding 4-space indentation used in this file; locate the async removeCustomModel(modelId: string) method and change the line(s) currently indented with 6 spaces to use 4 spaces so the return and the closing brace line up with other methods.app/src/lib/hooks/useModelStatus.ts (1)
22-29: Consider a defensive fallback forcustomModels.Filtering
customModelsstrictly onm.is_customcouples this hook tightly to the backend always populating the flag. As noted on the backend change, the current/models/statusimplementation forgets to setis_custom=Trueon injected custom entries, so this hook will currently return an emptycustomModelslist. Once that's fixed server-side this works, but a combined predicate (e.g.m.is_custom || m.model_name.startsWith('custom:')) would make the hook resilient to future drift and match the convention used elsewhere (ModelManagement.tsx).🛠️ Proposed fix
const customModels = useMemo( - () => modelStatus?.models.filter((m) => m.is_custom) ?? [], + () => modelStatus?.models.filter((m) => m.is_custom || m.model_name.startsWith('custom:')) ?? [], [modelStatus], );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/lib/hooks/useModelStatus.ts` around lines 22 - 29, The customModels filter in useModelStatus currently only checks m.is_custom which can be missing; update the useMemo for customModels in useModelStatus to defensively include models whose model_name follows the custom convention (e.g., m.model_name.startsWith('custom:')) in addition to m.is_custom (i.e., filter by m.is_custom || m.model_name?.startsWith('custom:')), preserving the null-safety fallback (?? []) so the hook remains resilient to backend omissions and matches the convention used in ModelManagement.tsx.backend/routes/models.py (1)
389-428: Duplicate custom-model scanning logic.This block re-implements cache directory presence / weights detection / size computation that already exists in the built-in loop at lines 312-345. Since custom entries differ only in where their config comes from, consider extracting the cache-probe into a helper (e.g.
_probe_repo_cache(repo_id) -> (downloaded, size_mb)) and reusing it for both paths. Not a blocker, but given the PR explicitly defersprobe_model_cache_statusrefactoring, this is the natural spot to consolidate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/routes/models.py` around lines 389 - 428, The custom-model block duplicates the repo cache probing/size logic already used in the built-in model loop (the code around the existing probe_model_cache_status logic at lines ~312-345); extract that logic into a small helper (e.g. _probe_repo_cache(repo_id) -> (downloaded: bool, size_mb: Optional[float])) and call it from both the custom entries loop and the built-in loop so the code in the custom_models.list_custom_models() handling only maps config fields and uses _probe_repo_cache(hf_repo) to set downloaded and size_mb; preserve use of models.ModelStatus, hf_repo in active_download_repos for downloading flag, and keep exception handling around the per-entry probe.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/components/Generation/FloatingGenerateBox.tsx`:
- Around line 11-13: The import list in FloatingGenerateBox.tsx includes unused
symbols SelectGroup and SelectLabel; remove these two from the named imports
(the import that currently lists SelectGroup, SelectItem, SelectLabel) so only
used symbols like SelectItem remain, then run a quick type-check/ESLint to
ensure no other unused imports remain.
In `@app/src/components/Generation/GenerationForm.tsx`:
- Around line 18-20: The imports SelectGroup and SelectLabel in the
GenerationForm component are unused; remove them from the import list that
currently includes SelectGroup, SelectItem, SelectLabel so only the needed
symbols (e.g., SelectItem) remain, updating the import statement at the top of
GenerationForm.tsx (the component/function named GenerationForm) to eliminate
SelectGroup and SelectLabel.
In `@backend/routes/models.py`:
- Around line 495-509: Add a short docstring or inline comment at the start of
the custom model cache-delete branch (the code that checks
model_name.startswith("custom:"), extracts custom_id and deletes the HF repo
cache_dir) stating that this endpoint only removes the HF cache directory and
does not unregister or unload the model (it intentionally does not call
remove_custom_model(custom_id) or perform any unload), and that callers must
also call the DELETE /custom-models/{model_id} endpoint to remove the registry
entry and fully delete the model; reference the variables model_name, custom_id
and the helper remove_custom_model in the comment to make the divergence
explicit for future maintainers.
- Around line 415-425: The appended ModelStatus entries for injected custom
models are missing the is_custom flag; update the models.ModelStatus
construction inside the statuses.append call that builds entries for
cm_name/display/hf_repo so it includes is_custom=True for these injected custom
entries (leave other ModelStatus constructions unchanged); this ensures the
useModelStatus hook (and consumers like GenerationForm) can detect custom models
via the is_custom field.
---
Outside diff comments:
In `@app/src/components/ServerSettings/ModelManagement.tsx`:
- Around line 1-1186: The file is syntactically broken due to mangled imports,
misplaced top-level declarations inside the ModelManagement function, missing
hook/mutation declarations, duplicate UI blocks, and several merged lines — fix
by: 1) move and reconstruct all import statements to the top (include Dialog,
DialogContent, DialogDescription, DialogFooter/DialogHeader/DialogTitle,
CustomModelItem, useModelStatus/usePlatform/useServerStore), removing the stray
concatenated import fragments; 2) ensure ModelManagement() opens and closes
correctly so helper functions (fetchHuggingFaceModelInfo, MODEL_DESCRIPTIONS,
formatDownloads/formatLicense/formatPipelineTag/formatBytes) are declared
outside the component or moved above the component and not nested inside it; 3)
declare or create the missing React Query mutations/variables
(addCustomModelMutation, removeCustomModelMutation, modelStatus/isLoading via
useModelStatus or a useQuery for ['modelStatus']), and wire up CustomModelItem
prop types; 4) remove the duplicated "Custom Models" UI block (keep only the
sections-based rendering) and fix merged statements/newlines (split combined
await/queryClient calls and object properties like onError descriptions) so JSX
tags (CardContent/Dialog/DialogContent/etc.) are balanced; 5) run tsc --noEmit
and eslint/formatter to catch any remaining parse/type errors before committing.
In `@app/src/lib/api/client.ts`:
- Around line 3-22: The import list contains a duplicate ActiveTasksResponse
symbol; remove the second occurrence and merge the new types (CustomModelCreate,
CustomModelResponse, CustomModelListResponse) into the existing sorted import
block so the list remains alphabetized and deduplicated (update the top-level
import that currently declares ActiveTasksResponse, ApplyEffectsRequest, ...,
GenerationResponse, etc., to include the three CustomModel* types in the correct
alphabetical position and omit the duplicate ActiveTasksResponse).
---
Nitpick comments:
In `@app/src/lib/api/client.ts`:
- Around line 459-463: The indentation inside the removeCustomModel method is
inconsistent: adjust the indentation so the return statement and the closing
brace align with the surrounding 4-space indentation used in this file; locate
the async removeCustomModel(modelId: string) method and change the line(s)
currently indented with 6 spaces to use 4 spaces so the return and the closing
brace line up with other methods.
In `@app/src/lib/hooks/useModelStatus.ts`:
- Around line 22-29: The customModels filter in useModelStatus currently only
checks m.is_custom which can be missing; update the useMemo for customModels in
useModelStatus to defensively include models whose model_name follows the custom
convention (e.g., m.model_name.startsWith('custom:')) in addition to m.is_custom
(i.e., filter by m.is_custom || m.model_name?.startsWith('custom:')), preserving
the null-safety fallback (?? []) so the hook remains resilient to backend
omissions and matches the convention used in ModelManagement.tsx.
In `@backend/backends/pytorch_backend.py`:
- Around line 54-63: Remove the inline attribution comment "# `@modified` AJ -
Kamyab (Ankit Jain) — Added custom model path resolution" to keep the source
clean and rely on VCS history; leave the surrounding logic intact (the
model_size.startswith("custom:") branch that computes custom_id, calls
get_hf_repo_id_for_custom_model(custom_id), and raises ValueError when not
found) so callers like _load_model_sync and _is_model_cached continue to see the
same failure behavior.
In `@backend/routes/models.py`:
- Around line 389-428: The custom-model block duplicates the repo cache
probing/size logic already used in the built-in model loop (the code around the
existing probe_model_cache_status logic at lines ~312-345); extract that logic
into a small helper (e.g. _probe_repo_cache(repo_id) -> (downloaded: bool,
size_mb: Optional[float])) and call it from both the custom entries loop and the
built-in loop so the code in the custom_models.list_custom_models() handling
only maps config fields and uses _probe_repo_cache(hf_repo) to set downloaded
and size_mb; preserve use of models.ModelStatus, hf_repo in
active_download_repos for downloading flag, and keep exception handling around
the per-entry probe.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7947d167-e950-4785-8b1b-10f1289f738a
📒 Files selected for processing (17)
.github/workflows/release.ymlapp/src/components/Generation/FloatingGenerateBox.tsxapp/src/components/Generation/GenerationForm.tsxapp/src/components/ServerSettings/ModelManagement.tsxapp/src/lib/api/client.tsapp/src/lib/api/types.tsapp/src/lib/hooks/useGenerationForm.tsapp/src/lib/hooks/useModelStatus.tsbackend/backends/mlx_backend.pybackend/backends/pytorch_backend.pybackend/build_binary.pybackend/config.pybackend/main.pybackend/models.pybackend/requirements.txtbackend/routes/models.pybackend/voicebox-server.spec
💤 Files with no reviewable changes (1)
- .github/workflows/release.yml
✅ Files skipped from review due to trivial changes (3)
- backend/requirements.txt
- backend/voicebox-server.spec
- backend/main.py
🚧 Files skipped from review as they are similar to previous changes (4)
- backend/build_binary.py
- backend/backends/mlx_backend.py
- app/src/lib/hooks/useGenerationForm.ts
- backend/models.py
Comment on lines
+11
to
+13
| SelectGroup, | ||
| SelectItem, | ||
| SelectLabel, |
Contributor
There was a problem hiding this comment.
Remove unused imports.
SelectGroup and SelectLabel are imported but never used in this component.
🧹 Proposed fix to remove unused imports
import {
Select,
SelectContent,
- SelectGroup,
SelectItem,
- SelectLabel,
SelectTrigger,
SelectValue,
} from '@/components/ui/select';📝 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
| SelectGroup, | |
| SelectItem, | |
| SelectLabel, | |
| import { | |
| Select, | |
| SelectContent, | |
| SelectItem, | |
| SelectTrigger, | |
| SelectValue, | |
| } from '@/components/ui/select'; |
🧰 Tools
🪛 Biome (2.4.11)
[error] 11-13: Several of these imports are unused.
(lint/correctness/noUnusedImports)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/components/Generation/FloatingGenerateBox.tsx` around lines 11 - 13,
The import list in FloatingGenerateBox.tsx includes unused symbols SelectGroup
and SelectLabel; remove these two from the named imports (the import that
currently lists SelectGroup, SelectItem, SelectLabel) so only used symbols like
SelectItem remain, then run a quick type-check/ESLint to ensure no other unused
imports remain.
Comment on lines
+18
to
+20
| SelectGroup, | ||
| SelectItem, | ||
| SelectLabel, |
Contributor
There was a problem hiding this comment.
Remove unused imports.
SelectGroup and SelectLabel are imported but never used in this component.
🧹 Proposed fix to remove unused imports
import {
Select,
SelectContent,
- SelectGroup,
SelectItem,
- SelectLabel,
SelectTrigger,
SelectValue,
} from '@/components/ui/select';📝 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
| SelectGroup, | |
| SelectItem, | |
| SelectLabel, | |
| import { | |
| Select, | |
| SelectContent, | |
| SelectItem, | |
| SelectTrigger, | |
| SelectValue, | |
| } from '@/components/ui/select'; |
🧰 Tools
🪛 Biome (2.4.11)
[error] 18-20: Several of these imports are unused.
(lint/correctness/noUnusedImports)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/components/Generation/GenerationForm.tsx` around lines 18 - 20, The
imports SelectGroup and SelectLabel in the GenerationForm component are unused;
remove them from the import list that currently includes SelectGroup,
SelectItem, SelectLabel so only the needed symbols (e.g., SelectItem) remain,
updating the import statement at the top of GenerationForm.tsx (the
component/function named GenerationForm) to eliminate SelectGroup and
SelectLabel.
Comment on lines
+415
to
+425
| statuses.append( | ||
| models.ModelStatus( | ||
| model_name=cm_name, | ||
| display_name=display, | ||
| hf_repo_id=hf_repo, | ||
| downloaded=downloaded, | ||
| downloading=hf_repo in active_download_repos, | ||
| size_mb=cm_size_mb, | ||
| loaded=False, | ||
| ) | ||
| ) |
Contributor
There was a problem hiding this comment.
Set is_custom=True on injected custom entries.
The new useModelStatus hook derives customModels via m.is_custom (see app/src/lib/hooks/useModelStatus.ts line 27), and ModelStatus in app/src/lib/api/types.ts (and, per the PR summary, backend/models.py) exposes an is_custom field specifically for this purpose. Omitting it here means the shared hook will always return an empty customModels list, breaking any consumer (GenerationForm / FloatingGenerateBox) that was migrated to the hook. The in-file ModelManagement.tsx still works only because it falls back to a startsWith('custom:') filter — that inconsistency is precisely what the is_custom flag is meant to remove.
🛠️ Proposed fix
statuses.append(
models.ModelStatus(
model_name=cm_name,
display_name=display,
hf_repo_id=hf_repo,
downloaded=downloaded,
downloading=hf_repo in active_download_repos,
size_mb=cm_size_mb,
loaded=False,
+ is_custom=True,
)
)📝 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
| statuses.append( | |
| models.ModelStatus( | |
| model_name=cm_name, | |
| display_name=display, | |
| hf_repo_id=hf_repo, | |
| downloaded=downloaded, | |
| downloading=hf_repo in active_download_repos, | |
| size_mb=cm_size_mb, | |
| loaded=False, | |
| ) | |
| ) | |
| statuses.append( | |
| models.ModelStatus( | |
| model_name=cm_name, | |
| display_name=display, | |
| hf_repo_id=hf_repo, | |
| downloaded=downloaded, | |
| downloading=hf_repo in active_download_repos, | |
| size_mb=cm_size_mb, | |
| loaded=False, | |
| is_custom=True, | |
| ) | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/routes/models.py` around lines 415 - 425, The appended ModelStatus
entries for injected custom models are missing the is_custom flag; update the
models.ModelStatus construction inside the statuses.append call that builds
entries for cm_name/display/hf_repo so it includes is_custom=True for these
injected custom entries (leave other ModelStatus constructions unchanged); this
ensures the useModelStatus hook (and consumers like GenerationForm) can detect
custom models via the is_custom field.
Comment on lines
+495
to
+509
| # Handle custom models | ||
| if model_name.startswith("custom:"): | ||
| custom_id = model_name[len("custom:"):] | ||
| cm = custom_models.get_custom_model(custom_id) | ||
| if not cm: | ||
| raise HTTPException(status_code=404, detail=f"Custom model '{custom_id}' not found") | ||
| hf_repo_id = cm.get("hf_repo_id", "") | ||
| try: | ||
| cache_dir = hf_constants.HF_HUB_CACHE | ||
| repo_cache_dir = Path(cache_dir) / ("models--" + hf_repo_id.replace("/", "--")) | ||
| if repo_cache_dir.exists(): | ||
| shutil.rmtree(repo_cache_dir) | ||
| return {"message": f"Custom model {model_name} cache deleted successfully"} | ||
| except Exception as e: | ||
| raise HTTPException(status_code=500, detail=f"Failed to delete custom model cache: {str(e)}") from e |
Contributor
There was a problem hiding this comment.
Custom-model cache delete leaves registry intact and skips unload — intentional, but document the divergence.
This branch deletes the HF cache directory but does not call remove_custom_model(custom_id) or attempt to unload the model. The separate DELETE /custom-models/{model_id} endpoint handles unregistration. That's a reasonable split, but the frontend has to call both endpoints to fully remove a custom model; otherwise the entry reappears as "not downloaded" after cache clear. Consider a short docstring note so callers don't assume this endpoint removes the registration too.
🧰 Tools
🪛 Ruff (0.15.10)
[warning] 509-509: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/routes/models.py` around lines 495 - 509, Add a short docstring or
inline comment at the start of the custom model cache-delete branch (the code
that checks model_name.startswith("custom:"), extracts custom_id and deletes the
HF repo cache_dir) stating that this endpoint only removes the HF cache
directory and does not unregister or unload the model (it intentionally does not
call remove_custom_model(custom_id) or perform any unload), and that callers
must also call the DELETE /custom-models/{model_id} endpoint to remove the
registry entry and fully delete the model; reference the variables model_name,
custom_id and the helper remove_custom_model in the comment to make the
divergence explicit for future maintainers.
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
Addresses code review findings across frontend and backend — input validation, file integrity, error handling, and code deduplication.
Changes
Frontend
model_nameformat before removal; disable Remove button during pending mutationmodelIdinremoveCustomModelpathmodelSizewith regex (built-in sizes + custom slugs)GenerationForm/FloatingGenerateBoxBackend
owner/reporegex forhf_repo_idmodel_sizeField--onefilebundles (viaplatformdirs)HTTPExceptionbefore broadexcept Exceptionso 202 download-in-progress responses aren't swallowed as 500splatformdirs>=4.0.0--data-dirCLI flagSkipped
main.pyprobe_model_cache_status refactoring (pure refactoring, deferred to reduce regression risk)Verification
tsc --noEmit— zero errorspy_compile— all modified files passSummary by CodeRabbit
New Features
Bug Fixes
Chores