feat: folders for voices and clips, list view, and profile duplication - #1005
Open
Lvigentini wants to merge 4 commits into
Open
feat: folders for voices and clips, list view, and profile duplication#1005Lvigentini wants to merge 4 commits into
Lvigentini wants to merge 4 commits into
Conversation
backend/README.md documents VOICEBOX_DATA_DIR alongside --data-dir as a way to override the data directory, but config.py never read it -- only VOICEBOX_MODELS_DIR was wired up, so the default stayed hardcoded to ./data. This matters in development: `just dev` starts the backend with a bare `uvicorn backend.main:app`, which never reaches main.py's argparse, so --data-dir is unavailable there and the dev server always writes to ./data inside the repo. Pointing a dev server at the packaged app's data directory meant editing the justfile. Read the variable when computing the default. --data-dir still takes precedence, since it calls set_data_dir() after import. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two related additions, sharing changes to the profile models and service.
Folders
-------
A `folders` table plus a folder_id column on profiles and generations. One
table serves both, discriminated by kind:
- voice — flat. Voices are a small stable set that reads better as
one level, so the routes reject a parent_id.
- generation — nests to arbitrary depth, since clips accumulate per
project and need real hierarchy.
Nesting is enforced in the routes rather than the schema so the constraint
can relax later without a migration. Reparenting is guarded against cycles:
moving a folder inside its own descendant would detach the subtree from the
root and strand it in the tree UI.
Deleting a folder never deletes its contents. Members become uncategorised
and child folders rise to the deleted folder's parent, so a folder is only
ever a view over items, not an owner of them.
The folders table itself is created by Base.metadata.create_all(), which
runs right after migrations; only the columns on pre-existing tables need
the hand-rolled migration. Those are declared without a REFERENCES clause,
matching story_items.version_id, because SQLite cannot add a column with a
foreign key to a table that does not exist yet.
Duplication
-----------
POST /profiles/{id}/duplicate, deliberately not implemented as an
export-then-import round-trip. The transfer format carries only
name/description/language, so a round-trip silently drops personality,
effects_chain, default_engine and every preset/designed field -- and export
rejects profiles with no samples, which is every preset voice. Copying the
row directly keeps all of it. Sample audio is copied byte-for-byte rather
than re-encoded through add_profile_sample(), so a duplicate sounds like its
original instead of being resampled.
_get_unique_profile_name moves from export_import to services.profiles so
duplication can reuse it without importing the export/import layer; the old
name stays as an alias.
Also threads a folder filter through GET /history. Its limit/offset bounds
are now declared as Query constraints: building HistoryQuery from raw ints
inside the handler surfaced its ValidationError as an opaque 500 instead of
a 422.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Voices as a list ---------------- Two-line rows: name, language and trait badges on the first line, the description on the second — enough to tell apart clones that differ only by a suffix (v2_slower_deeper vs v3_podcast). Cards remain available behind a toggle, remembered across restarts. Row actions sit behind a menu rather than always-visible icon buttons: at list density a row is ~48px, too tight for four buttons beside the text. The selectable area is a real <button> rather than a div with role=button, because the actions menu is itself a button and nesting interactive elements is invalid. Folders ------- Voices group into flat folders; clips get a nested tree. Both use one collapsible-section pattern, with an Uncategorised bucket for anything unfiled. Collapse state persists per kind. The clip folder tree renders outside the history empty-state branch — if it did not, filtering to an empty folder would remove the only control able to clear that filter. Changing the filter drops the accumulated infinite-scroll pages, which would otherwise leave clips from the previous filter on screen beneath the new results. Deleting the folder currently being filtered resets the filter to All, rather than leaving it pointed at something gone. Both tree builders tolerate a dangling parent/folder id — another window can delete a folder between renders, and the item should surface at the root rather than vanish. Duplicate --------- A duplicate action on both the row menu and the card, wired to the new endpoint. Copies samples, avatar, personality, effects and preset fields — the export/import round-trip users previously had to do by hand keeps only name, description and language. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughThis change adds voice and generation folders with nesting, CRUD operations, membership assignment, history filtering, persisted UI state, voice list/card views, and voice-profile duplication with copied metadata and samples. ChangesFolders and profile duplication
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ProfileList
participant useFolders
participant folders_api
participant Database
ProfileList->>useFolders: create or update voice folder
useFolders->>folders_api: send folder mutation
folders_api->>Database: persist folder change
Database-->>folders_api: return folder response
folders_api-->>useFolders: invalidate folder query
useFolders-->>ProfileList: refresh grouped profiles
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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: 6
🧹 Nitpick comments (3)
backend/routes/folders.py (1)
23-23: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExport a public serializer instead of importing
_profile_to_response.
_profile_to_responseis named as module-internal tobackend/services/profiles.py. Importing it from a route module couples the route to a private symbol, so a rename inside the service breaks this file with no signal at the boundary.Add a public alias or a thin public function in
backend/services/profiles.py, then import that name here.🤖 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/routes/folders.py` at line 23, Expose a public profile serializer in the profiles service by adding a public alias or thin wrapper around _profile_to_response, then update the folders route import to use that public symbol instead of the private name. Preserve the serializer’s existing behavior and response shape.backend/database/models.py (1)
115-116: 🚀 Performance & Scalability | 🔵 TrivialConsider indexing
generations.folder_id.
list_generationsfilters onfolder_idwith equality,IS NULL, andIN (...). Thegenerationstable grows with every clip, so this filter runs against the largest table in the schema. An index onfolder_idkeeps folder filtering and the grouped_member_countsquery cheap as history grows.If you add the index, also add it in
_migrate_foldersfor existing databases, sincecreate_all()only builds indexes for new tables.⚙️ Proposed index
- folder_id = Column(String, ForeignKey("folders.id"), nullable=True) + folder_id = Column(String, ForeignKey("folders.id"), nullable=True, index=True)🤖 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/database/models.py` around lines 115 - 116, Add an index for generations.folder_id at the column definition, and update _migrate_folders to create the same index for existing databases. Ensure the index supports list_generations filtering and the grouped _member_counts query without changing their behavior.backend/tests/test_folders.py (1)
265-271: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the
include_subfoldersbranch.These assertions only check that the endpoint returns 200. The descendant branch in
backend/services/history.pylines 202-204 is the one folder filter with real logic: it callsfolder_and_descendantsand matchesfolder_id IN (...). Nothing here proves a parent folder returns a child folder's clips, and nothing provesinclude_subfolders=falseexcludes them.That branch can break silently — a wrong traversal still returns 200 with the wrong rows. Add a test that creates a parent folder and a child folder, assigns a generation to the child through
PUT /history/{generation_id}/folder, then asserts the clip appears when querying the parent with the default and disappears withinclude_subfolders=false.🤖 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/tests/test_folders.py` around lines 265 - 271, Expand test_history_accepts_folder_filters to create parent and child folders, assign a generation to the child via PUT /history/{generation_id}/folder, and verify the generation appears when querying the parent with default include_subfolders behavior. Add a matching assertion that querying the parent with include_subfolders=false excludes the child generation, covering the folder_and_descendants branch rather than only checking status codes.
🤖 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 `@app/src/components/History/HistoryTable.tsx`:
- Around line 716-740: Update the setGenerationFolder mutation calls in the
folder-selection handlers to reset page to zero and clear allHistory in the
mutation success callback after a successful move. Apply this to both the
uncategorised and mapped-folder actions so the active filter reloads from page 0
without retaining the moved clip.
In `@app/src/lib/hooks/useFolders.ts`:
- Around line 44-52: Update the onSuccess handler in useDetachFolder to
invalidate the ['history'] query whenever kind === 'generation', while
preserving the existing ['folders', kind] invalidation for all detach
operations.
In `@backend/models.py`:
- Around line 21-22: Update the name fields in FolderCreate and FolderUpdate to
strip surrounding whitespace before validation, then enforce min_length=1 and
max_length=100 on the stripped value so whitespace-only names are rejected and
stored names remain normalized.
In `@backend/services/profiles.py`:
- Line 755: Update ProfileDuplicateRequest to normalize the provided name before
validation and reject values that become blank after stripping whitespace. Keep
the service call’s get_unique_profile_name behavior unchanged, ensuring
whitespace-only names fail request validation instead of reaching the duplicate
route and producing a 404.
- Line 205: Update create_profile to validate data.folder_id through the shared
folder-validation logic before inserting the profile, requiring the folder to
exist and have kind == "voice". Return the established client error for invalid
or non-voice folders, and only persist folder_id after validation succeeds.
In `@backend/tests/test_data_dir_env.py`:
- Around line 23-25: Update test_data_dir_env.py to import and reload the config
module through the backend package, matching the suite’s established
backend.config module identity; remove the sys.path insertion and top-level
config import, and ensure fixture mutations and reloads target backend.config
consistently.
---
Nitpick comments:
In `@backend/database/models.py`:
- Around line 115-116: Add an index for generations.folder_id at the column
definition, and update _migrate_folders to create the same index for existing
databases. Ensure the index supports list_generations filtering and the grouped
_member_counts query without changing their behavior.
In `@backend/routes/folders.py`:
- Line 23: Expose a public profile serializer in the profiles service by adding
a public alias or thin wrapper around _profile_to_response, then update the
folders route import to use that public symbol instead of the private name.
Preserve the serializer’s existing behavior and response shape.
In `@backend/tests/test_folders.py`:
- Around line 265-271: Expand test_history_accepts_folder_filters to create
parent and child folders, assign a generation to the child via PUT
/history/{generation_id}/folder, and verify the generation appears when querying
the parent with default include_subfolders behavior. Add a matching assertion
that querying the parent with include_subfolders=false excludes the child
generation, covering the folder_and_descendants branch rather than only checking
status codes.
🪄 Autofix
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: d3a01aa5-a5bb-4d46-90f4-7ea3553057c5
📒 Files selected for processing (27)
app/src/components/History/ClipFolderTree.tsxapp/src/components/History/HistoryTable.tsxapp/src/components/VoiceProfiles/FolderSection.tsxapp/src/components/VoiceProfiles/ProfileCard.tsxapp/src/components/VoiceProfiles/ProfileList.tsxapp/src/components/VoiceProfiles/ProfileRow.tsxapp/src/i18n/locales/en/translation.jsonapp/src/lib/api/client.tsapp/src/lib/api/types.tsapp/src/lib/hooks/useFolders.tsapp/src/lib/hooks/useProfiles.tsapp/src/stores/uiStore.tsbackend/config.pybackend/database/migrations.pybackend/database/models.pybackend/models.pybackend/routes/__init__.pybackend/routes/folders.pybackend/routes/history.pybackend/routes/profiles.pybackend/services/export_import.pybackend/services/folders.pybackend/services/history.pybackend/services/profiles.pybackend/tests/test_data_dir_env.pybackend/tests/test_folders.pybackend/tests/test_profile_duplicate.py
Comment on lines
+716
to
+740
| <DropdownMenuItem | ||
| disabled={!gen.folder_id} | ||
| onClick={() => | ||
| setGenerationFolder.mutate({ | ||
| generationId: gen.id, | ||
| folderId: null, | ||
| }) | ||
| } | ||
| > | ||
| {t('folders.uncategorised')} | ||
| </DropdownMenuItem> | ||
| {(clipFolders ?? []).map((folder) => ( | ||
| <DropdownMenuItem | ||
| key={folder.id} | ||
| disabled={folder.id === gen.folder_id} | ||
| onClick={() => | ||
| setGenerationFolder.mutate({ | ||
| generationId: gen.id, | ||
| folderId: folder.id, | ||
| }) | ||
| } | ||
| > | ||
| {folder.name} | ||
| </DropdownMenuItem> | ||
| ))} |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reset accumulated history after a successful folder move.
When page is greater than zero, this mutation invalidates the current history query but does not remove moved clips from allHistory. The history effect appends the refreshed page and preserves a clip that no longer belongs to the selected folder or uncategorised filter.
Reset page and allHistory in the mutation success callback. This forces a fresh page-0 result for the active filter.
Proposed fix
+ const handleMoveToFolder = (generationId: string, folderId: string | null) => {
+ setGenerationFolder.mutate(
+ { generationId, folderId },
+ {
+ onSuccess: () => {
+ setPage(0);
+ setAllHistory([]);
+ },
+ },
+ );
+ };
+
- onClick={() => setGenerationFolder.mutate({ generationId: gen.id, folderId: null })}
+ onClick={() => handleMoveToFolder(gen.id, null)}
- onClick={() => setGenerationFolder.mutate({ generationId: gen.id, folderId: folder.id })}
+ onClick={() => handleMoveToFolder(gen.id, folder.id)}📝 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
| <DropdownMenuItem | |
| disabled={!gen.folder_id} | |
| onClick={() => | |
| setGenerationFolder.mutate({ | |
| generationId: gen.id, | |
| folderId: null, | |
| }) | |
| } | |
| > | |
| {t('folders.uncategorised')} | |
| </DropdownMenuItem> | |
| {(clipFolders ?? []).map((folder) => ( | |
| <DropdownMenuItem | |
| key={folder.id} | |
| disabled={folder.id === gen.folder_id} | |
| onClick={() => | |
| setGenerationFolder.mutate({ | |
| generationId: gen.id, | |
| folderId: folder.id, | |
| }) | |
| } | |
| > | |
| {folder.name} | |
| </DropdownMenuItem> | |
| ))} | |
| const handleMoveToFolder = (generationId: string, folderId: string | null) => { | |
| setGenerationFolder.mutate( | |
| { generationId, folderId }, | |
| { | |
| onSuccess: () => { | |
| setPage(0); | |
| setAllHistory([]); | |
| }, | |
| }, | |
| ); | |
| }; | |
| <DropdownMenuItem | |
| disabled={!gen.folder_id} | |
| onClick={() => handleMoveToFolder(gen.id, null)} | |
| > | |
| {t('folders.uncategorised')} | |
| </DropdownMenuItem> | |
| {(clipFolders ?? []).map((folder) => ( | |
| <DropdownMenuItem | |
| key={folder.id} | |
| disabled={folder.id === gen.folder_id} | |
| onClick={() => handleMoveToFolder(gen.id, folder.id)} | |
| > | |
| {folder.name} | |
| </DropdownMenuItem> | |
| ))} |
🤖 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 `@app/src/components/History/HistoryTable.tsx` around lines 716 - 740, Update
the setGenerationFolder mutation calls in the folder-selection handlers to reset
page to zero and clear allHistory in the mutation success callback after a
successful move. Apply this to both the uncategorised and mapped-folder actions
so the active filter reloads from page 0 without retaining the moved clip.
| raise ValueError(f"Profile {profile_id} not found") | ||
|
|
||
| new_id = str(uuid.uuid4()) | ||
| new_name = get_unique_profile_name(name.strip() if name else f"{source.name} (copy)", db) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject whitespace-only duplicate names.
ProfileDuplicateRequest accepts " " because its raw length is nonzero. Line 755 then converts it to "", and the duplicate can persist with an empty name.
Normalize and reject blank names in ProfileDuplicateRequest before this service call. This also avoids converting a validation error into the route's current 404 response. Based on the provided model context, ProfileDuplicateRequest validates raw length only.
🤖 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/profiles.py` at line 755, Update ProfileDuplicateRequest to
normalize the provided name before validation and reject values that become
blank after stripping whitespace. Keep the service call’s
get_unique_profile_name behavior unchanged, ensuring whitespace-only names fail
request validation instead of reaching the duplicate route and producing a 404.
Comment on lines
+23
to
+25
| sys.path.insert(0, str(Path(__file__).parent.parent)) | ||
|
|
||
| import config # noqa: E402 |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Compare sys.path bootstrapping and config import forms across backend tests.
set -euo pipefail
fd -e py . backend/tests --exec rg -n 'sys\.path\.insert|^\s*(import|from)\s+(backend\.)?config' {} \; -x true 2>/dev/null || true
echo '--- direct config imports ---'
rg -n --type=py -C2 '^\s*(import config|from config import|from backend import config|from \.\.? import config|import backend\.config)' backendRepository: jamiepine/voicebox
Length of output: 9308
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- backend/tests/test_data_dir_env.py ---'
cat -n backend/tests/test_data_dir_env.py
echo '--- config source outline/imports ---'
fd -e py '^(config|__init__|conftest|server\.py)$' backend tests -x sh -c 'echo "### $1"; wc -l "$1"; ast-grep outline "$1" 2>/dev/null | head -80 || true' sh {}
echo '--- reload references ---'
rg -n --type=py 'importlib\.reload|reload\(' backend testsRepository: jamiepine/voicebox
Length of output: 4369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- backend tests list ---'
git ls-files backend/tests | sort
echo '--- config.py ---'
cat -n backend/config.py
echo '--- all config import lines across repository ---'
rg -n --type=py '(^|[^A-Za-z0-9_.])(import (config|backend)|from (config|backend) import|from \.\.?backend import|from \. import config|import backend\.config|from backend import config)' || true
echo '--- pytest configuration ---'
fd -e ini -e toml -e cfg -e json -e yaml -e yml . backend --exec sh -c 'echo "### $1"; sed -n "1,160p" "$1"' sh {}Repository: jamiepine/voicebox
Length of output: 7825
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
"""
Read-only semantic probe of the module identity produced by the test's bootstrapping.
The probe only prints names and attributes from the bootstrap environment; it does
not import backend/config or run repository code.
"""
import sys
from pathlib import Path
ROOT = Path(__file__).resolve().parent
backend = ROOT / "backend"
assert backend.exists(), "sandbox path not present"
sys.path.insert(0, str(backend))
top_level_import = any(
part == "backend" and next_part == "config"
for part, next_part in zip(("backend", "config"), ("backend", "config"))
)
print("backend_dir_exists:", backend.exists())
print("backend_on_path:", (sys.path[0] == str(backend)))
print("config_file:", (backend / "config.py").exists())
print("current_module_name:", __name__)
print("module_attribute_name:", "module" in globals())
PY
echo '--- pytest discovery settings ---'
rg -n --type=ini -C3 'pythonpath|testpaths|addopts|configfile|asyncio' . || trueRepository: jamiepine/voicebox
Length of output: 346
Import and reload backend.config consistently.
test_data_dir_env.py adds backend/ to sys.path and reloads the top-level config module, while the rest of the code/test suite imports config through the backend package (from backend import config, from .. import config, or relative ..imports). In a single pytest process, use the same module identity for all reloads to avoid keeping one backend.config object stale while this fixture mutates another config object.
🤖 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/tests/test_data_dir_env.py` around lines 23 - 25, Update
test_data_dir_env.py to import and reload the config module through the backend
package, matching the suite’s established backend.config module identity; remove
the sys.path insertion and top-level config import, and ensure fixture mutations
and reloads target backend.config consistently.
Five findings from the review of jamiepine#1005. Folder assignment on profile creation ------------------------------------- `create_profile` persisted `folder_id` without checking it. The folder-assignment endpoint enforces that a voice only lands in a voice folder, but creation bypassed that entirely — a client could file a new profile straight into a clip folder, or reference a folder that does not exist. Same two checks, raised as ValueError so the route returns 400 like its neighbours. Whitespace-only names --------------------- `" "` passed `min_length=1` because the raw string is three characters long, and the route then stored `.strip()` — an empty label. Same for the duplicate-profile override, which could produce a copy with no name at all. Both now use a `TrimmedName` type that strips first and length-checks the result. Left `AudioChannelUpdate` and `EffectPresetUpdate` alone; they have the same gap but are not this PR's concern. Moved clips lingering in the list --------------------------------- Moving a clip to another folder invalidated the history query but left `allHistory` intact. Past page 0 the refreshed first page is appended rather than replacing the list, so the clip stayed visible in a folder it no longer belonged to. Folder moves now go through one handler that drops back to page 0, which is what makes the next response replace the list. Detaching a folder ------------------ `useDetachFolder` invalidated only the folder tree. Member lists include subfolders, so anyone filtered by the former parent kept seeing the detached child's items until an unrelated refetch. The sixth finding — the flat `config` import in test_data_dir_env — is fixed on the jamiepine#1004 branch and resolves here on merge. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Lvigentini
pushed a commit
to Lvigentini/voicebox
that referenced
this pull request
Aug 9, 2026
Five findings from the review of jamiepine#1005. Folder assignment on profile creation ------------------------------------- `create_profile` persisted `folder_id` without checking it. The folder-assignment endpoint enforces that a voice only lands in a voice folder, but creation bypassed that entirely — a client could file a new profile straight into a clip folder, or reference a folder that does not exist. Same two checks, raised as ValueError so the route returns 400 like its neighbours. Whitespace-only names --------------------- `" "` passed `min_length=1` because the raw string is three characters long, and the route then stored `.strip()` — an empty label. Same for the duplicate-profile override, which could produce a copy with no name at all. Both now use a `TrimmedName` type that strips first and length-checks the result. Left `AudioChannelUpdate` and `EffectPresetUpdate` alone; they have the same gap but are not this PR's concern. Moved clips lingering in the list --------------------------------- Moving a clip to another folder invalidated the history query but left `allHistory` intact. Past page 0 the refreshed first page is appended rather than replacing the list, so the clip stayed visible in a folder it no longer belonged to. Folder moves now go through one handler that drops back to page 0, which is what makes the next response replace the list. Detaching a folder ------------------ `useDetachFolder` invalidated only the folder tree. Member lists include subfolders, so anyone filtered by the former parent kept seeing the detached child's items until an unrelated refetch. The sixth finding — the flat `config` import in test_data_dir_env — is fixed on the jamiepine#1004 branch and resolves here on merge. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/services/profiles.py (1)
140-157: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winHandle duplicate-name conflicts during profile duplication.
DBVoiceProfile.namehasunique=True, butget_unique_profile_name()can pass the same candidate name to multiple concurrentduplicate_profile()calls. Those calls can add rows at the same name and then raiseIntegrityErroratdb.commit(); the route currently catches onlyValueError, so duplicate-profile requests become 500. Catch and retry the conflict with a fresh allocation induplicate_profile(), or serialize the candidate check insert into one transaction.🤖 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/profiles.py` around lines 140 - 157, Update duplicate_profile to handle concurrent DBVoiceProfile.name uniqueness conflicts: catch IntegrityError from the commit, roll back the session, allocate a fresh name via get_unique_profile_name, and retry the duplication without converting the request to a 500. Preserve the existing ValueError handling and ensure retries do not leave the failed transaction state active.
🧹 Nitpick comments (1)
backend/models.py (1)
5-6: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the standard-library
Annotatedimport.The Python target is now
>=3.12, so importAnnotatedfromtyping.Diff
-from typing_extensions import Annotated +from typing import Annotated🤖 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/models.py` around lines 5 - 6, Update the Annotated import used by the model definitions to come from the standard-library typing module, while retaining the existing Pydantic imports and usage.Source: Linters/SAST tools
🤖 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.
Outside diff comments:
In `@backend/services/profiles.py`:
- Around line 140-157: Update duplicate_profile to handle concurrent
DBVoiceProfile.name uniqueness conflicts: catch IntegrityError from the commit,
roll back the session, allocate a fresh name via get_unique_profile_name, and
retry the duplication without converting the request to a 500. Preserve the
existing ValueError handling and ensure retries do not leave the failed
transaction state active.
---
Nitpick comments:
In `@backend/models.py`:
- Around line 5-6: Update the Annotated import used by the model definitions to
come from the standard-library typing module, while retaining the existing
Pydantic imports and usage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 13f7d311-fafb-4a42-8693-b6d906299c2e
📒 Files selected for processing (4)
app/src/components/History/HistoryTable.tsxapp/src/lib/hooks/useFolders.tsbackend/models.pybackend/services/profiles.py
🚧 Files skipped from review as they are similar to previous changes (2)
- app/src/lib/hooks/useFolders.ts
- app/src/components/History/HistoryTable.tsx
Lvigentini
force-pushed
the
feat/ui-organisation
branch
from
August 9, 2026 13:46
cbfb69d to
b74eb6a
Compare
Lvigentini
added a commit
to Lvigentini/voicebox
that referenced
this pull request
Aug 9, 2026
Five findings from the review of jamiepine#1005. Folder assignment on profile creation ------------------------------------- `create_profile` persisted `folder_id` without checking it. The folder-assignment endpoint enforces that a voice only lands in a voice folder, but creation bypassed that entirely — a client could file a new profile straight into a clip folder, or reference a folder that does not exist. Same two checks, raised as ValueError so the route returns 400 like its neighbours. Whitespace-only names --------------------- `" "` passed `min_length=1` because the raw string is three characters long, and the route then stored `.strip()` — an empty label. Same for the duplicate-profile override, which could produce a copy with no name at all. Both now use a `TrimmedName` type that strips first and length-checks the result. Left `AudioChannelUpdate` and `EffectPresetUpdate` alone; they have the same gap but are not this PR's concern. Moved clips lingering in the list --------------------------------- Moving a clip to another folder invalidated the history query but left `allHistory` intact. Past page 0 the refreshed first page is appended rather than replacing the list, so the clip stayed visible in a folder it no longer belonged to. Folder moves now go through one handler that drops back to page 0, which is what makes the next response replace the list. Detaching a folder ------------------ `useDetachFolder` invalidated only the folder tree. Member lists include subfolders, so anyone filtered by the former parent kept seeing the detached child's items until an unrelated refetch. The sixth finding — the flat `config` import in test_data_dir_env — is fixed on the jamiepine#1004 branch and resolves here on merge. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Implements the folder organisation asked for in #821.
What it adds
Backend: new
folderstable and migration,backend/routes/folders.py, plus folder fields on the profile and history routes. Frontend:ClipFolderTree,FolderSection,ProfileRow, auseFoldershook, and uiStore state for the view mode.Stacking
This branch carries one of #1004's three commits —
d5eea00, the backend half ofVOICEBOX_DATA_DIR. The two Tauri commits that make the variable reach the packaged app are not here, so on this branch the override still only works for a bareuvicorn, which is the half that doesn't fix #981. That's deliberate: they're Rust changes with nothing to do with folders. #1004 remains the PR to merge for the actual fix.Merge in either order; they don't conflict.
Notes for review
ProfileListas a prop. This branch rewrites the same component for folders, so we collided in exactly one file.ProfileListhere now accepts the samesearch/onClearSearchprops and filters before folder bucketing, so a match stays in its folder rather than collapsing into a flat list; folders with no matches hide while a query is active. Match predicate and the alphabetical sort tiebreak are feat(ui): add responsive search & filter to Voice Library and Main Editor (fixes #966) #1016's, andprofiles.list.noVoicesMatchis translated in all nine locales. No search box is added here — the input, VoicesTab filters and layout work stay in feat(ui): add responsive search & filter to Voice Library and Main Editor (fixes #966) #1016. If this lands first, that PR'sProfileListhunk drops out and the rest applies clean. Details in my comment there; the one thing that still needs your call is the responsive layout rework feat(ui): add responsive search & filter to Voice Library and Main Editor (fixes #966) #1016 bundles in, which can't be mechanically composed with folder sections.HUGGINGFACE_HUB_CACHE), not UI folders, so I've dropped the claim — fix: honor VOICEBOX_DATA_DIR in both the backend and the desktop app #1004 is the PR that speaks to it.Closes #821
Screenshots
Voices tab — folders and list view
Folder sections with member counts (
PODCAST VOICES 6,AUDITIONS 4), anUNCATEGORISEDbucket for unfiled voices, and theNew folderaction. This is the list view added in this PR — the toggle at top right switches back to the existing card view.Row actions — duplicate and file into a folder
The per-row action menu, showing
Duplicate profile(copies samples, personality and effects) andMove to folderalongside the existing actions.Clips tab — folder tree
The same folder model applied to generated clips, with per-folder counts, a selected folder highlighted, and
All clips/Uncategorisedpseudo-folders. Clips can also be dragged onto a folder header to file them.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Accessibility