feat(stories): multi-track timeline editor, per-lane mixing, and audio export - #1007
Open
Lvigentini wants to merge 24 commits into
Open
feat(stories): multi-track timeline editor, per-lane mixing, and audio export#1007Lvigentini wants to merge 24 commits into
Lvigentini wants to merge 24 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>
…mats The story mixer flattened every source to 24 kHz mono (`load_audio` passes sr=24000, mono=True) and summed into a 1-D buffer, so an imported music bed came out at 12 kHz Nyquist with its stereo image folded flat. Imported audio also landed on track 0 and was appended after the last clip, so a music file played *after* the narration instead of underneath it. Mixing ------ Decode each source once at its native rate, derive a project rate from what the sources actually are (capped at 48 kHz), and mix into a (2, n) stereo buffer. Each lane renders to its own buffer first, which is what makes ducking possible -- a bed can be attenuated by the *finished* speech lane -- and is where track gain, mute and solo apply. Clip duration now comes from the decoded array rather than generation.duration. A clip pinned to a version of a different length was mistimed, and a NULL duration raised TypeError inside a bare `except: continue`, silently dropping the clip from the export. Tracks ------ New story_tracks table holding per-lane name, volume, mute, solo and an optional duck-under-lane. Keyed by (story_id, index) rather than owning clips: story_items.track stays a plain integer, so drag/move/reorder are untouched and a lane with no row simply mixes at unity gain. Deleting a row resets the lane; it never removes clips. Solo is evaluated globally -- once any lane is soloed the rest are silent, including lanes with no settings row. Treating a missing row as exempt rather than defaults let the un-configured voice lane play through a solo of the bed. Clips ----- Per-clip fade_in_ms/fade_out_ms as positional envelopes, deliberately not EFFECT_REGISTRY entries -- those are pedalboard plugins built as cls(**params), whereas a fade depends on where the clip starts and ends. Overlong fades scale down together instead of clipping, so a short clip stays monotonic. Splitting moves the fade-out to the tail so no dip appears at the seam. Per-clip speed via librosa.effects.time_stretch (phase vocoder, pitch preserved); pedalboard has no time-stretch. Because clips are absolutely positioned, a re-timed clip changes its own length without shifting neighbours, exactly as trim already does. Export formats -------------- MP3, OGG, Opus and FLAC alongside WAV, via the libsndfile already shipped with the app (1.2.2, with LAME/mpg123/Vorbis/Opus/FLAC compiled in). No ffmpeg, no new dependency. Default stays wav. ffmpeg ------ Optional and detected, never required: it adds EBU R128 loudness normalisation, and every path falls back cleanly. It was already a hidden dependency for m4a/aac/webm import, which libsndfile cannot open and which therefore fell through librosa to audioread; those now fail fast with a clear message instead of dying in the decoder. Availability is reported on /health. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Surfaces the timeline audio work in the Stories editor. Track mixer strip ----------------- Each lane gets mute, solo, volume and a duck-under-lane picker in the label column, which widens from 64px to fit them. The label cell is now sized from LABEL_COL_WIDTH rather than a Tailwind class, so the timeline's coordinate maths can never drift from what is rendered. A lane with no settings row renders the same controls at their defaults and creates the row on first change -- matching the mixer, which treats a missing row as defaults rather than as exempt. Solo is shown as a story-wide state: lanes silenced *because something else is soloed* are dimmed rather than marked muted, so it is clear why they went quiet. Clip fades and speed -------------------- Two popovers beside the existing volume control, following its local-state-then-commit shape so dragging a slider doesn't fire a request per pixel. getEffectiveDuration now divides by speed, matching the mixer's formula. Without it the timeline would draw a re-timed clip at its original length and the playhead would drift away from what the export actually contains. Export ------ The export button becomes a menu: WAV, MP3, OGG, Opus and FLAC, plus a loudness-normalised option that is disabled with an explanatory label when ffmpeg is absent rather than silently ignoring the request. It previously passed the click handler straight to onClick, which with the new signature would have handed a MouseEvent in as the format. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
story_tracks rows are keyed by story_id but carry no FK cascade, so delete_story left them behind as unreachable rows — found when testing against a real database, which accumulated two orphans from deleted stories. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
backend/README.md: adds the Folders domain row, corrects the endpoint count (the table said 90; the tree has had more than that for a while and is now 135), documents the new folder/duplicate/export-format examples, and adds an optional-dependencies section for ffmpeg -- which is not bundled, is never required, and is the only decoder for the m4a/aac/webm imports the endpoint advertises. Also notes that --data-dir wins over VOICEBOX_DATA_DIR, and that the environment variable is the only one that reaches a bare uvicorn. CHANGELOG.md: drafted per .agents/skills/draft-release-notes into the [Unreleased] section, preserving the existing Linux entry. CONTRIBUTING asks contributors to edit this file directly, but the file's own header forbids manual edits and points at the skill; the skill wins. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
mousedown entered drag mode immediately and mouseup committed a move, so a plain click on a clip could drop it onto a neighbouring track -- selection was destructive. A press now has to travel 4px before it becomes a drag; below that it stays a click and commits nothing. The drag maths also ignored the timeline's scrollTop, so once the track area was scrolled vertically a drag resolved to the wrong lane. Both the initial position and the move now include it. Adds, in the same area: - Per-lane import and remove. A lane could be created with + but never removed, and there was no way to place audio on a *specific* lane -- the story-level import always picks a free lane at t=0, which is wrong for a stinger partway in. Remove only offers itself on empty non-default lanes, so it can never take clips with it, and it clears that lane's mixer settings so a later lane reusing the index doesn't inherit a stale mute or duck target. - Snapping, on by default, joining a dragged clip flush to its neighbours or to zero. The radius is in screen pixels, not milliseconds, so the feel stays constant across zoom levels. Toggle in the toolbar. - A live seconds readout above a clip while trimming. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Folder headers used the same weight and colour as the rows inside them, so a list of voices and the folders holding them looked like one flat list. Headers now sit on a shaded, bordered strip in bold, with the count in a pill, and members are indented behind a rule. Applied to both the voice folders and the nested clip folder tree so the two panels stay consistent. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The drag threshold added in 0e05d41 deadlocked. The move/up handlers were attached only while draggingItem was set, but the threshold that promotes a press into a drag lives inside handleDragMove -- so the handler was never attached, the threshold never ran, and no clip could be dragged at all. Track the armed press in state as well as the ref, and attach the handlers for either an armed press or a live drag. The ref still carries the coordinates so the threshold reads them without waiting on a re-render. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ming Story folders ------------- Stories gain nested folders, matching clips. The kind discriminator grows a third value and the flat-vs-nested rule moves from a hardcoded voice check to a _FLAT_KINDS set, so only voices stay one level deep. Selecting a parent folder shows the whole subtree, since a story records only its own folder. The clip folder tree is now generic over kind rather than being copied a third time; stories reuse it with their own labels. Drag and drop ------------- Voices, clips and stories can be dragged onto a folder header, or onto Uncategorised to unfile them. Uses native HTML5 drag and drop rather than dnd-kit: these lists need a drop target, not sortable reordering, and it keeps the folder headers as plain elements. The payload carries the folder kind so a voice dropped on a clip folder is ignored rather than failing server-side. Timeline -------- Ripple move, off by default: dragging a clip carries everything later on the same track by the same delta, so inserting an intro pushes the track along instead of leaving a hole. Off by default because it rewrites clips the user did not touch. Right-click a clip for exact timing. Length and speed are two views of one number, so editing either recomputes the other and only the resulting speed is stored -- which keeps it consistent with the mixer, where speed is what actually changes the rendered length. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Clicking a clip folder listed no clips even though the count was right. The effect that cleared the accumulated list on a filter change is declared after the effect that fills it, so when React Query served the new folder's rows in the same commit the clear wiped them -- and historyData never changed again, so they never came back. The clear is gone: replacing on page 0, which the filling effect already does, is what discards the previous folder's rows. Dragging a row never started. The draggable element wrapped the row body, which is a <button>, and browsers don't reliably begin a native drag from inside one. Each row now has an explicit grip handle that is itself the draggable element -- also a visible affordance, matching the story timeline. Adds a voice preview dialog to the Generate tab, reachable from the row menu and the card. It reuses the Voices tab's MiniSamplePlayer rather than a second implementation, so playback behaves identically in both places, and stays read-only: this is for choosing a voice, not editing one. Cargo.lock carries the 0.5.1 version from the bump. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Dragging a row showed the no-drop cursor over every folder. The drop targets were correct: Tauri's dragDropEnabled defaults to true, so the webview hands drag events to the OS file-drop handler instead of the page, and no HTML5 drop target inside the app can ever accept. Turning it off is safe here because nothing uses Tauri's native drag-drop event -- every drop zone in the app is already HTML5 (dataTransfer.files in StoryContent and AudioSampleUpload). Those were subject to the same interception, so this should make dropping an audio file onto a story work too. Also cancels dragenter alongside dragover on the folder targets; some engines only treat an element as a drop target once enter has been cancelled. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis change adds folder management for clips, stories, and voice profiles. It adds story-track editing and configurable audio export. It adds voice-profile duplication and preview workflows, FFmpeg availability reporting, data-directory configuration, migrations, API contracts, and validation tests. ChangesApplication feature expansion
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
app/src/components/StoriesTab/StoryList.tsx (1)
306-309: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe empty state reports a search miss when only the folder filter is active.
filterednarrows by folder first and then by search. If a folder contains no stories and the search box is empty, this branch still rendersstories.empty.noMatcheswith an emptyquery.
HistoryTablehandles the same case at line 472 by branching onfolderSelection.kind. Match that behavior.🐛 Proposed fix
) : filtered.length === 0 ? ( <div className="px-4 py-12 text-center text-sm text-muted-foreground"> - <p>{t('stories.empty.noMatches', { query: search })}</p> + <p> + {search.trim() + ? t('stories.empty.noMatches', { query: search }) + : t('folders.story.emptyFilter')} + </p> </div>Add
folders.story.emptyFiltertoapp/src/i18n/locales/en/translation.json.🤖 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/StoriesTab/StoryList.tsx` around lines 306 - 309, Update the empty-state branch in StoryList so it distinguishes an active folder filter from a search miss, matching HistoryTable’s folderSelection.kind behavior: use the folder-filter empty message when appropriate and retain the noMatches message for searches. Add the corresponding folders.story.emptyFilter translation key to the English locale.backend/services/stories.py (2)
1138-1262: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftSynchronous work blocks the event loop across the export path. Both sites run blocking, long-running work directly inside
async defhandlers on the story export request, so a single export stalls every other request served by the process.
backend/services/stories.py#L1138-L1262: run the decode, resample, time-stretch, lane mixing andencode_audiobody in a worker thread, for example throughstarlette.concurrency.run_in_threadpool.backend/routes/stories.py#L303-L306: wrap theffmpeg.normalize_loudnesscall inrun_in_threadpool, sincesubprocess.runholds the loop for up to its 300 second timeout.🤖 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/stories.py` around lines 1138 - 1262, The story export processing in backend/services/stories.py lines 1138-1262 must run its decode, resampling, time-stretching, mixing, and encode_audio work through a worker thread, such as Starlette’s run_in_threadpool, rather than blocking the async handler; update the surrounding export function while preserving its behavior. Also update backend/routes/stories.py lines 303-306 to invoke ffmpeg.normalize_loudness via run_in_threadpool, so its blocking subprocess call does not hold the event loop.
736-755: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCopy fades and speed in
duplicate_story_item, and use speed-adjusted gap.
duplicate_story_itemleavesfade_in_msandfade_out_msat 0 andspeedat 1.0 for the copy, so duplicated clips lose per-clip fade/speed settings.effective_duration_msalso ignores clipping/speed for the gap placement, so duplicated re-timed clips do not start after the original clip duration; use the speed-adjusted trimmed duration instart_time_ms.🤖 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/stories.py` around lines 736 - 755, Update duplicate_story_item to copy the source item’s fade_in_ms, fade_out_ms, and speed values instead of resetting them. Calculate the duplicated item’s start_time_ms using the speed-adjusted duration of the source’s trimmed/clipped range, reusing effective_duration_ms or the established equivalent so the new clip begins after the original’s actual playback duration.
🧹 Nitpick comments (15)
app/src/components/VoiceProfiles/ProfileList.tsx (1)
177-183: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winFolder creation fails silently.
submitNewFoldercloses the dialog immediately and discards the mutation result. If the request fails, for example on a duplicate name, the user sees the dialog close and no new folder, with no explanation.Add an
onErrortoast, asProfileRow.handleDuplicatedoes.♻️ Proposed change
const submitNewFolder = () => { const trimmed = newFolderName.trim(); if (!trimmed) return; - createFolder.mutate({ name: trimmed }); + createFolder.mutate( + { name: trimmed }, + { + onError: (error) => + toast({ + title: t('folders.createFailed'), + description: error.message, + variant: 'destructive', + }), + }, + ); setNewFolderName(''); setNewFolderOpen(false); };Import
useToastand add thefolders.createFailedkey.🤖 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/VoiceProfiles/ProfileList.tsx` around lines 177 - 183, Update submitNewFolder to pass an onError callback to createFolder.mutate that displays the folders.createFailed toast, following the notification pattern used by ProfileRow.handleDuplicate. Import useToast and add the folders.createFailed translation key, while preserving the existing successful submission flow.app/src/stores/uiStore.ts (1)
133-139: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe rehydration guard does not cover the case the comment describes.
The comment states that a store persisted before story folders has no
storykey. The guard only replaces the whole map when it is absent. A partial map keeps its missing keys.This is safe today because
collapsedFolderIdsisPartial<Record<FolderKind, string[]>>and all readers use?? []. Align the comment with the code, or fill the missing keys.♻️ Proposed comment alignment
onRehydrateStorage: () => (state) => { if (state) applyTheme(state.theme); - // Persisted before folders existed, so an older store has no map — - // and a store persisted before story folders has no 'story' key. + // Persisted before folders existed, so an older store has no map. + // Missing per-kind keys need no backfill: the map is Partial and + // every reader falls back to an empty list. if (state && !state.collapsedFolderIds) { state.collapsedFolderIds = { voice: [], generation: [], story: [] }; } },🤖 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/stores/uiStore.ts` around lines 133 - 139, Update the rehydration handling in onRehydrateStorage so persisted collapsedFolderIds maps missing the story key are handled consistently: either revise the nearby comment to describe only the absent-map case, or ensure each missing folder key, including story, is initialized while preserving existing keys.app/src/components/History/ClipFolderTree.tsx (1)
202-204: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueThe badge count can disagree with the filtered list.
item_countcounts direct members only, per the contract note inapp/src/lib/api/types.tsat line 50.HistoryTablecallsuseHistorywithoutinclude_subfolders, and the server defaults it totrue(seebackend/routes/history.py). Selecting a parent folder therefore lists descendant clips while the badge shows a smaller number.Consider showing a rolled-up count for parents, or pass
include_subfolders: falsefromHistoryTableso the two agree.🤖 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/ClipFolderTree.tsx` around lines 202 - 204, Align the folder badge with the filtered history list by updating the HistoryTable/useHistory flow to pass include_subfolders: false, matching the direct-member item_count shown in ClipFolderTree. Preserve the existing folder selection behavior while ensuring parent-folder results and badge counts use the same scope.app/src/components/History/HistoryTable.tsx (1)
749-762: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBoth "Move to" submenus flatten a nested folder list. Clip and story folders nest, and
useFoldersreturns them as a flatFolderResponse[]carryingparent_id. Both submenus map that list directly to one menu level, so nesting is lost and folders that share a name under different parents become indistinguishable.ClipFolderTreerenders the same data as a tree. Extract one helper that renders a flat folder list as an indented tree or as parent-path labels, then use it in both places.
app/src/components/History/HistoryTable.tsx#L749-L762: replace the flatclipFolders.mapwith the shared hierarchy-aware renderer.app/src/components/StoriesTab/StoryList.tsx#L400-L410: replace the flatstoryFolders.mapwith the same renderer.🤖 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 749 - 762, Replace the flat folder mapping in app/src/components/History/HistoryTable.tsx#L749-L762 and app/src/components/StoriesTab/StoryList.tsx#L400-L410 with one shared hierarchy-aware renderer/helper that uses each folder’s parent_id to display nested folders as an indented tree or parent-path labels. Preserve each submenu item’s existing key, disabled condition, and setGenerationFolder.mutate behavior while ensuring same-named folders remain distinguishable.backend/database/models.py (2)
126-162: 🚀 Performance & Scalability | 🔵 TrivialConsider indexing the folder and lane lookup columns.
Generation.folder_id,VoiceProfile.folder_id,Story.folder_id, andFolder.parent_idare all filtered on every list request.folder_and_descendantsissues oneparent_id IN (...)query per tree level, and history filtering addsfolder_id IN (...).StoryTrack.story_idis queried per story load and deleted bystory_id. Addingindex=Trueto these columns keeps those queries cheap as data grows.Note that
UniqueConstraint("story_id", "index")already coversStoryTrack.story_idlookups on SQLite, so only the folder columns need attention.🤖 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 126 - 162, Add database indexes to the folder lookup columns Generation.folder_id, VoiceProfile.folder_id, Story.folder_id, and Folder.parent_id by setting index=True on those Column definitions. Do not add a separate index to StoryTrack.story_id because its existing UniqueConstraint already covers that lookup.
28-39: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the third folder kind.
FOLDER_KIND_PATTERNinbackend/models.pyallowsvoice,generation, andstory, and_MEMBER_MODELmaps all three. Both docstrings still describe only two kinds, so they understate the discriminator and omit that story folders nest.
backend/database/models.py#L28-L39: add the"story"case to theFolderdocstring and state that it nests, matchinggeneration.backend/routes/folders.py#L1-L13: add thestorykind to the module docstring's kind list and note that story folders nest.🤖 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 28 - 39, Update the Folder docstring in backend/database/models.py (lines 28-39) to document the story kind and state that it supports nesting like generation; update the module docstring in backend/routes/folders.py (lines 1-13) to include story in the kind list and note its nesting behavior.backend/database/migrations.py (1)
353-363: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCollapse the nested
ifstatements to satisfy Ruff SIM102.Ruff reports SIM102 three times in this function.
♻️ Proposed refactor
- if "profiles" in tables: - if "folder_id" not in _get_columns(inspector, "profiles"): - _add_column(engine, "profiles", "folder_id VARCHAR", "folder_id") - - if "generations" in tables: - if "folder_id" not in _get_columns(inspector, "generations"): - _add_column(engine, "generations", "folder_id VARCHAR", "folder_id") - - if "stories" in tables: - if "folder_id" not in _get_columns(inspector, "stories"): - _add_column(engine, "stories", "folder_id VARCHAR", "folder_id") + for table in ("profiles", "generations", "stories"): + if table in tables and "folder_id" not in _get_columns(inspector, table): + _add_column(engine, table, "folder_id VARCHAR", "folder_id")🤖 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/migrations.py` around lines 353 - 363, In the migration function, update the three table-specific blocks for profiles, generations, and stories to combine each table-existence check with its missing-folder_id-column check into a single conditional, preserving the existing _add_column calls and behavior.Source: Linters/SAST tools
backend/tests/test_folders.py (1)
324-330: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the folder filter behaviour, not only the status code.
These assertions pass even if the filter is ignored.
include_subfoldersexpansion throughfolder_and_descendantsis the most intricate new logic in this cohort, and no test covers it. Add a test that files a generation into a child folder, then requests the parent folder withinclude_subfolders=trueand withinclude_subfolders=false, and asserts the returned ids differ. Add a matching assertion thatuncategorised_only=trueexcludes filed clips.Do you want me to generate these tests?
🤖 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 324 - 330, Strengthen test_history_accepts_folder_filters to verify filtering results rather than only HTTP 200 responses: create parent and child folders, file a generation in the child, then assert /history with the parent folder and include_subfolders=true returns the generation while include_subfolders=false does not. Also assert that /history with uncategorised_only=true excludes the filed generation, using the returned item IDs.backend/routes/folders.py (1)
23-23: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExport
_profile_to_responseunder a public name.This route module imports a private helper from
backend/services/profiles.py. The leading underscore marks it as internal to that service, so any refactor there silently breaks this route. Rename it toprofile_to_responsein the service, or add a thin public wrapper and import that.🤖 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 the profile response conversion through a public symbol: rename `_profile_to_response` to `profile_to_response` in the profiles service or add a delegating public wrapper, then update the route import and usage to reference the public name while preserving the existing response behavior.backend/utils/ffmpeg.py (1)
68-90: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueGuard the caller-supplied
suffixused to build the temp filename.The static analysis hit on line 103 is a false positive for command injection:
cmdis a list,shell=Trueis absent,execomes fromshutil.which, and the numeric options are typed floats.One residual point remains.
suffixis interpolated intoin{suffix}andout{suffix}. The only current caller passesEXPORT_FORMATS[...]["ext"], which is server-controlled, so there is no issue today. A future caller that forwards an uploaded filename extension would write outside the temp directory through../. Reject asuffixthat is not a plain extension.🛡️ Proposed fix
exe = ffmpeg_path() if exe is None: return None + if not re.fullmatch(r"\.[A-Za-z0-9]{1,8}", suffix): + raise ValueError(f"Invalid audio suffix: {suffix!r}")🤖 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/utils/ffmpeg.py` around lines 68 - 90, Validate the caller-supplied suffix in normalize_loudness before constructing the temporary input and output paths, accepting only a plain file extension and rejecting values containing path separators, traversal components, or other filename content; return None for invalid suffixes while preserving the existing ffmpeg behavior for valid extensions.Source: Linters/SAST tools
backend/routes/stories.py (1)
274-274: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the parameter and keep the
formatquery name.Ruff A002 flags
formatbecause it shadows the builtin. Renaming alone would change the public query parameter, so use an alias.♻️ Proposed fix
- format: str = "wav", + fmt: str = Query("wav", alias="format"),Update the body to use
fmt, and importQueryfromfastapi.🤖 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/stories.py` at line 274, Rename the endpoint parameter from format to fmt to avoid shadowing the builtin, while preserving the public query parameter name by declaring its FastAPI Query alias as "format". Import Query from fastapi and update all references in the affected endpoint body to use fmt.Source: Linters/SAST tools
app/src/components/StoriesTab/StoryContent.tsx (1)
51-57: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the format labels into the translation catalogue.
The dropdown labels are hardcoded English strings, while every other user-facing string in this component goes through
t(). Non-English users see a translated "Format" heading above untranslated entries.Store only the format value here and resolve the label with
t('storyContent.export.formats.' + value).🤖 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/StoriesTab/StoryContent.tsx` around lines 51 - 57, Update EXPORT_FORMATS to store only each ExportAudioFormat value, then resolve each dropdown label through t('storyContent.export.formats.' + value) at the rendering/use site. Add the corresponding format keys to the translation catalogue and preserve the existing format ordering and values.backend/tests/test_ffmpeg_optional.py (1)
135-146: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe test claims MP3 but never imports an MP3.
The docstring states "WAV/FLAC/OGG/MP3 must import with ffmpeg absent", and the loop covers only WAV, FLAC and OGG. MP3 is the one format in that list that depends on the bundled libsndfile having LAME, so it is the case most worth asserting.
Add MP3 to the loop, or remove it from the docstring.
💚 Proposed fix
- for name, fmt in (("a.wav", "WAV"), ("a.flac", "FLAC"), ("a.ogg", "OGG")): + for name, fmt in ( + ("a.wav", "WAV"), + ("a.flac", "FLAC"), + ("a.ogg", "OGG"), + ("a.mp3", "MP3"), + ):🤖 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_ffmpeg_optional.py` around lines 135 - 146, Update test_libsndfile_formats_need_no_ffmpeg so its cases match the docstring: add an MP3 fixture to the format loop and exercise the same import assertion for it, or remove MP3 from the docstring if it is not intended to be covered.app/src/components/StoriesTab/StoryTrackEditor.tsx (1)
1228-1250: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUse the batch update for ripple moves.
Ripple issues one
moveItem.mutateper following clip. Each success invalidates['stories']and['stories', storyId], so a drag on a dense lane produces a burst of requests and repeated refetches. A partial failure also leaves the lane half-shifted with one toast per failed clip.
StoryItemBatchUpdateis already imported inapp/src/lib/hooks/useStories.ts. Send the moved clip and the rippled clips in one batch request instead.🤖 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/StoriesTab/StoryTrackEditor.tsx` around lines 1228 - 1250, Replace the per-clip moveItem.mutate calls in the ripple logic with one StoryItemBatchUpdate request containing the moved clip and all eligible following clips. Preserve the existing delta, track, and nonnegative start-time calculations, and route the batch failure through the existing onError handling so the operation performs one invalidation and one error notification.backend/tests/test_story_mixdown.py (1)
25-26: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBoth new test modules leak their data directory. Each module calls
tempfile.mkdtempat import time to setVOICEBOX_DATA_DIR, and neither removes the directory, so every run leaves generated audio and a SQLite database behind.
backend/tests/test_story_mixdown.py#L25-L26: register cleanup for_DATA_DIR, for example with an autouse session fixture that callsshutil.rmtree(_DATA_DIR, ignore_errors=True).backend/tests/test_ffmpeg_optional.py#L25-L26: apply the same cleanup, or move both modules onto one shared conftest fixture.🤖 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_story_mixdown.py` around lines 25 - 26, Register teardown for the module-level _DATA_DIR temporary directory in backend/tests/test_story_mixdown.py lines 25-26, using an autouse session fixture or equivalent cleanup that calls shutil.rmtree with ignore_errors=True. Apply the same cleanup to backend/tests/test_ffmpeg_optional.py lines 25-26, or replace both with one shared conftest fixture; ensure neither test module leaves generated audio or SQLite data behind.
🤖 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/ClipFolderTree.tsx`:
- Around line 181-194: Update the collapse toggle in the folder tree to use a
dedicated translated action label that distinguishes expanding from collapsing
and includes the folder name, instead of aria-label={folder.name}. Add the
folders.expand and folders.collapse translation keys to the English translations
and select the appropriate key based on collapsed state.
In `@app/src/components/StoriesTab/StoryTrackEditor.tsx`:
- Around line 367-371: Replace the w-16 width on the ruler corner spacer and
scrollbar corner spacer with the shared LABEL_COL_WIDTH constant. Update the
spacer elements in the ruler section and scrollbar section, preserving their
existing layout behavior so both align with the lane label cell and clip layer.
- Around line 2013-2097: Update the updateSpeed.mutate call in the timing
dialog’s Apply handler to provide an onError callback that displays the
component’s established error toast, while preserving the existing mutation
payload and validation. Ensure the dialog’s close behavior remains unchanged and
rejected requests give the user visible feedback.
In `@app/src/components/StoriesTab/TrackMixerControls.tsx`:
- Around line 82-90: Update the volume control in TrackMixerControls to mirror
ClipVolumePopover: maintain a localVolume state initialized from track.volume,
use it for the Slider value and readout, update only localVolume in
onValueChange, and call patch with the final localVolume on release. This must
produce a single API mutation per drag while preserving the displayed value
during the interaction.
In `@app/src/components/VoiceProfiles/FolderSection.tsx`:
- Around line 90-97: Update the drop handlers in FolderSection: call
e.preventDefault() at the start of onDrop before validating readFolderDragData,
while preserving the existing voice-payload guard and onDropItem behavior.
Change onDragLeave to clear dragOver only when the pointer leaves the header
element itself, ignoring transitions into its child elements.
In `@app/src/components/VoiceProfiles/ProfileCard.tsx`:
- Around line 143-151: Update the duplicateProfile mutation handler in
ProfileCard’s CircleButton to add an onError callback that displays a
destructive toast using the existing profiles.duplicate.failedTitle translation,
matching the failure feedback behavior of ProfileRow.
In `@app/src/components/VoiceProfiles/VoicePreviewDialog.tsx`:
- Around line 52-61: Update the VoicePreviewDialog render logic using the error
state returned by useProfileSamples. Show a load-failure message when the query
errors, and only render the noSamples empty state after a successful query
returns zero samples; keep the loading state and sample display behavior
unchanged.
In `@app/src/i18n/locales/en/translation.json`:
- Around line 696-701: Update the storyContent.export.normalized translation to
remove the hardcoded “MP3” container reference, leaving a format-neutral
loudness-normalization label that remains correct for all supported export
formats.
In `@app/src/lib/hooks/useFolders.ts`:
- Around line 55-69: Update useDeleteFolder so the member-list cache key matches
each FolderKind: retain ['profiles'] for voice, use ['history'] for generation,
and use ['stories'] for story. Also update the file docstring describing folders
to include stories alongside voices and clips.
In `@backend/models.py`:
- Around line 799-807: Update StoryTrackUpsert.duck_under_track to enforce a
non-negative value with ge=0, then in upsert_story_track validate the provided
duck_under_track against the known lane index and raise ValueError("A track
cannot duck under itself") when they match.
In `@backend/README.md`:
- Line 84: Update backend/README.md at lines 84-84 to include stories in the
/folders description. Update lines 133-133 to state that backend/config.py
defaults to the resolved local data directory when neither override is set, and
describe the OS-specific app-data path only as a value supplied by the desktop
launcher when applicable.
In `@backend/routes/__init__.py`:
- Around line 28-32: Reorder the router registration in the app initialization
so folders_router is included before profiles_router. Keep health_router
registration unchanged, ensuring PUT /profiles/{profile_id}/folder is matched
before the broader PUT /profiles/{profile_id} route.
In `@backend/routes/profiles.py`:
- Around line 153-154: Update the ValueError handler in the affected route to
explicitly chain the raised HTTPException with the caught exception using from
e, preserving the causal traceback and satisfying Ruff B904.
In `@backend/services/profiles.py`:
- Around line 139-154: Update the profile creation/import flow using
get_unique_profile_name so name selection and insertion occur in one
transaction, handling IntegrityError by rolling back and retrying with the next
suffix instead of returning a server error. Ensure each retry can allocate a new
name and remove any duplicate directory created by the failed attempt.
- Around line 750-755: Update ProfileDuplicateRequest validation to strip the
provided name before applying its length check, and reject an explicitly
supplied name that becomes empty after trimming with the existing validation
response. Ensure the duplicate-profile flow around get_unique_profile_name
receives the trimmed name while preserving the default source-name copy behavior
when no name is provided.
In `@backend/services/stories.py`:
- Around line 1242-1252: Update the ducking loop over lanes so each
_duck_envelope call reads its source from a pre-ducking snapshot rather than the
dictionary being modified in place. Apply the resulting envelope to the current
lane afterward, preserving existing track and source checks while making
mutually ducking lanes independent of dictionary insertion order.
In `@backend/tests/test_data_dir_env.py`:
- Around line 28-41: Update the reload_config fixture teardown so monkeypatch
restores VOICEBOX_DATA_DIR before the final importlib.reload(config). Preserve
the existing per-test environment patching and ensure the final config reload
observes the real process environment rather than the temporary test value.
In `@backend/utils/audio.py`:
- Around line 69-82: The soundfile dependency must match the codec guarantee
documented above EXPORT_FORMATS. Update backend/requirements.txt to pin the
exact soundfile release whose bundled libsndfile is 1.2.2 with LAME and Opus
support, preserving the existing 1.2.2 guarantee; alternatively, remove that
guarantee from the comment and ensure the documented formats remain accurate.
---
Outside diff comments:
In `@app/src/components/StoriesTab/StoryList.tsx`:
- Around line 306-309: Update the empty-state branch in StoryList so it
distinguishes an active folder filter from a search miss, matching
HistoryTable’s folderSelection.kind behavior: use the folder-filter empty
message when appropriate and retain the noMatches message for searches. Add the
corresponding folders.story.emptyFilter translation key to the English locale.
In `@backend/services/stories.py`:
- Around line 1138-1262: The story export processing in
backend/services/stories.py lines 1138-1262 must run its decode, resampling,
time-stretching, mixing, and encode_audio work through a worker thread, such as
Starlette’s run_in_threadpool, rather than blocking the async handler; update
the surrounding export function while preserving its behavior. Also update
backend/routes/stories.py lines 303-306 to invoke ffmpeg.normalize_loudness via
run_in_threadpool, so its blocking subprocess call does not hold the event loop.
- Around line 736-755: Update duplicate_story_item to copy the source item’s
fade_in_ms, fade_out_ms, and speed values instead of resetting them. Calculate
the duplicated item’s start_time_ms using the speed-adjusted duration of the
source’s trimmed/clipped range, reusing effective_duration_ms or the established
equivalent so the new clip begins after the original’s actual playback duration.
---
Nitpick comments:
In `@app/src/components/History/ClipFolderTree.tsx`:
- Around line 202-204: Align the folder badge with the filtered history list by
updating the HistoryTable/useHistory flow to pass include_subfolders: false,
matching the direct-member item_count shown in ClipFolderTree. Preserve the
existing folder selection behavior while ensuring parent-folder results and
badge counts use the same scope.
In `@app/src/components/History/HistoryTable.tsx`:
- Around line 749-762: Replace the flat folder mapping in
app/src/components/History/HistoryTable.tsx#L749-L762 and
app/src/components/StoriesTab/StoryList.tsx#L400-L410 with one shared
hierarchy-aware renderer/helper that uses each folder’s parent_id to display
nested folders as an indented tree or parent-path labels. Preserve each submenu
item’s existing key, disabled condition, and setGenerationFolder.mutate behavior
while ensuring same-named folders remain distinguishable.
In `@app/src/components/StoriesTab/StoryContent.tsx`:
- Around line 51-57: Update EXPORT_FORMATS to store only each ExportAudioFormat
value, then resolve each dropdown label through t('storyContent.export.formats.'
+ value) at the rendering/use site. Add the corresponding format keys to the
translation catalogue and preserve the existing format ordering and values.
In `@app/src/components/StoriesTab/StoryTrackEditor.tsx`:
- Around line 1228-1250: Replace the per-clip moveItem.mutate calls in the
ripple logic with one StoryItemBatchUpdate request containing the moved clip and
all eligible following clips. Preserve the existing delta, track, and
nonnegative start-time calculations, and route the batch failure through the
existing onError handling so the operation performs one invalidation and one
error notification.
In `@app/src/components/VoiceProfiles/ProfileList.tsx`:
- Around line 177-183: Update submitNewFolder to pass an onError callback to
createFolder.mutate that displays the folders.createFailed toast, following the
notification pattern used by ProfileRow.handleDuplicate. Import useToast and add
the folders.createFailed translation key, while preserving the existing
successful submission flow.
In `@app/src/stores/uiStore.ts`:
- Around line 133-139: Update the rehydration handling in onRehydrateStorage so
persisted collapsedFolderIds maps missing the story key are handled
consistently: either revise the nearby comment to describe only the absent-map
case, or ensure each missing folder key, including story, is initialized while
preserving existing keys.
In `@backend/database/migrations.py`:
- Around line 353-363: In the migration function, update the three
table-specific blocks for profiles, generations, and stories to combine each
table-existence check with its missing-folder_id-column check into a single
conditional, preserving the existing _add_column calls and behavior.
In `@backend/database/models.py`:
- Around line 126-162: Add database indexes to the folder lookup columns
Generation.folder_id, VoiceProfile.folder_id, Story.folder_id, and
Folder.parent_id by setting index=True on those Column definitions. Do not add a
separate index to StoryTrack.story_id because its existing UniqueConstraint
already covers that lookup.
- Around line 28-39: Update the Folder docstring in backend/database/models.py
(lines 28-39) to document the story kind and state that it supports nesting like
generation; update the module docstring in backend/routes/folders.py (lines
1-13) to include story in the kind list and note its nesting behavior.
In `@backend/routes/folders.py`:
- Line 23: Expose the profile response conversion through a public symbol:
rename `_profile_to_response` to `profile_to_response` in the profiles service
or add a delegating public wrapper, then update the route import and usage to
reference the public name while preserving the existing response behavior.
In `@backend/routes/stories.py`:
- Line 274: Rename the endpoint parameter from format to fmt to avoid shadowing
the builtin, while preserving the public query parameter name by declaring its
FastAPI Query alias as "format". Import Query from fastapi and update all
references in the affected endpoint body to use fmt.
In `@backend/tests/test_ffmpeg_optional.py`:
- Around line 135-146: Update test_libsndfile_formats_need_no_ffmpeg so its
cases match the docstring: add an MP3 fixture to the format loop and exercise
the same import assertion for it, or remove MP3 from the docstring if it is not
intended to be covered.
In `@backend/tests/test_folders.py`:
- Around line 324-330: Strengthen test_history_accepts_folder_filters to verify
filtering results rather than only HTTP 200 responses: create parent and child
folders, file a generation in the child, then assert /history with the parent
folder and include_subfolders=true returns the generation while
include_subfolders=false does not. Also assert that /history with
uncategorised_only=true excludes the filed generation, using the returned item
IDs.
In `@backend/tests/test_story_mixdown.py`:
- Around line 25-26: Register teardown for the module-level _DATA_DIR temporary
directory in backend/tests/test_story_mixdown.py lines 25-26, using an autouse
session fixture or equivalent cleanup that calls shutil.rmtree with
ignore_errors=True. Apply the same cleanup to
backend/tests/test_ffmpeg_optional.py lines 25-26, or replace both with one
shared conftest fixture; ensure neither test module leaves generated audio or
SQLite data behind.
In `@backend/utils/ffmpeg.py`:
- Around line 68-90: Validate the caller-supplied suffix in normalize_loudness
before constructing the temporary input and output paths, accepting only a plain
file extension and rejecting values containing path separators, traversal
components, or other filename content; return None for invalid suffixes while
preserving the existing ffmpeg behavior for valid extensions.
🪄 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: d37ac6ec-45d7-420a-944f-98e2fea7544c
⛔ Files ignored due to path filters (1)
tauri/src-tauri/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (47)
app/src/components/History/ClipFolderTree.tsxapp/src/components/History/HistoryTable.tsxapp/src/components/StoriesTab/StoryChatItem.tsxapp/src/components/StoriesTab/StoryContent.tsxapp/src/components/StoriesTab/StoryList.tsxapp/src/components/StoriesTab/StoryTrackEditor.tsxapp/src/components/StoriesTab/TrackMixerControls.tsxapp/src/components/VoiceProfiles/FolderSection.tsxapp/src/components/VoiceProfiles/ProfileCard.tsxapp/src/components/VoiceProfiles/ProfileForm.tsxapp/src/components/VoiceProfiles/ProfileList.tsxapp/src/components/VoiceProfiles/ProfileRow.tsxapp/src/components/VoiceProfiles/SampleList.tsxapp/src/components/VoiceProfiles/VoicePreviewDialog.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/lib/hooks/useStories.tsapp/src/lib/utils/folderDrag.tsapp/src/stores/uiStore.tsbackend/README.mdbackend/config.pybackend/database/migrations.pybackend/database/models.pybackend/models.pybackend/routes/__init__.pybackend/routes/folders.pybackend/routes/generations.pybackend/routes/health.pybackend/routes/history.pybackend/routes/profiles.pybackend/routes/stories.pybackend/services/export_import.pybackend/services/folders.pybackend/services/history.pybackend/services/profiles.pybackend/services/stories.pybackend/tests/test_data_dir_env.pybackend/tests/test_ffmpeg_optional.pybackend/tests/test_folders.pybackend/tests/test_profile_duplicate.pybackend/tests/test_story_mixdown.pybackend/utils/audio.pybackend/utils/ffmpeg.pytauri/src-tauri/tauri.conf.json
Comment on lines
+181
to
+194
| {children.length > 0 ? ( | ||
| <button | ||
| type="button" | ||
| onClick={() => toggleCollapsed(kind, folder.id)} | ||
| className="shrink-0 rounded p-0.5 hover:bg-accent/50" | ||
| aria-label={folder.name} | ||
| aria-expanded={!collapsed} | ||
| > | ||
| <Chevron className="h-3 w-3 text-muted-foreground" /> | ||
| </button> | ||
| ) : ( | ||
| // Keeps leaf labels aligned with their expandable siblings. | ||
| <span className="w-4 shrink-0" /> | ||
| )} |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
The collapse toggle needs an action label.
aria-label={folder.name} gives the toggle the same accessible name as the folder-selection button beside it. A screen reader user hears two controls named after the folder and cannot tell which one expands the subtree.
Use a dedicated label that names the action and the folder.
♿ Proposed fix
<button
type="button"
onClick={() => toggleCollapsed(kind, folder.id)}
className="shrink-0 rounded p-0.5 hover:bg-accent/50"
- aria-label={folder.name}
+ aria-label={t(collapsed ? 'folders.expand' : 'folders.collapse', {
+ name: folder.name,
+ })}
aria-expanded={!collapsed}
>Add folders.expand and folders.collapse to app/src/i18n/locales/en/translation.json.
🤖 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/ClipFolderTree.tsx` around lines 181 - 194, Update
the collapse toggle in the folder tree to use a dedicated translated action
label that distinguishes expanding from collapsing and includes the folder name,
instead of aria-label={folder.name}. Add the folders.expand and folders.collapse
translation keys to the English translations and select the appropriate key
based on collapsed state.
Comment on lines
+2013
to
+2097
|
|
||
| <Dialog open={lengthTargetId !== null} onOpenChange={() => setLengthTargetId(null)}> | ||
| <DialogContent className="max-w-xs"> | ||
| <DialogHeader> | ||
| <DialogTitle>Clip timing</DialogTitle> | ||
| </DialogHeader> | ||
|
|
||
| {/* Length and speed are two views of the same number: editing one | ||
| recomputes the other, and only the resulting speed is stored. */} | ||
| <div className="space-y-3"> | ||
| <div> | ||
| <span className="mb-1 block text-xs text-muted-foreground">Target length</span> | ||
| <div className="flex items-center gap-2"> | ||
| <Input | ||
| value={lengthDraft} | ||
| onChange={(e) => { | ||
| setLengthDraft(e.target.value); | ||
| const target = Number.parseFloat(e.target.value); | ||
| const clip = items.find((i) => i.id === lengthTargetId); | ||
| if (clip && target > 0) { | ||
| const natural = | ||
| clip.duration * 1000 - (clip.trim_start_ms || 0) - (clip.trim_end_ms || 0); | ||
| setSpeedDraft((natural / 1000 / target).toFixed(2)); | ||
| } | ||
| }} | ||
| className="h-8" | ||
| inputMode="decimal" | ||
| aria-label="Target length in seconds" | ||
| /> | ||
| <span className="text-xs text-muted-foreground">s</span> | ||
| </div> | ||
| </div> | ||
|
|
||
| <div> | ||
| <span className="mb-1 block text-xs text-muted-foreground">Speed</span> | ||
| <div className="flex items-center gap-2"> | ||
| <Input | ||
| value={speedDraft} | ||
| onChange={(e) => { | ||
| setSpeedDraft(e.target.value); | ||
| const rate = Number.parseFloat(e.target.value); | ||
| const clip = items.find((i) => i.id === lengthTargetId); | ||
| if (clip && rate > 0) { | ||
| const natural = | ||
| clip.duration * 1000 - (clip.trim_start_ms || 0) - (clip.trim_end_ms || 0); | ||
| setLengthDraft((natural / 1000 / rate).toFixed(2)); | ||
| } | ||
| }} | ||
| className="h-8" | ||
| inputMode="decimal" | ||
| aria-label="Speed multiplier" | ||
| /> | ||
| <span className="text-xs text-muted-foreground">x</span> | ||
| </div> | ||
| </div> | ||
| <p className="text-[10px] leading-snug text-muted-foreground"> | ||
| Pitch is preserved. 0.25x to 4x; outside that the stretch smears speech. | ||
| </p> | ||
| </div> | ||
|
|
||
| <DialogFooter> | ||
| <Button variant="outline" onClick={() => setLengthTargetId(null)}> | ||
| Cancel | ||
| </Button> | ||
| <Button | ||
| onClick={() => { | ||
| const rate = Number.parseFloat(speedDraft); | ||
| if (lengthTargetId && rate >= 0.25 && rate <= 4) { | ||
| updateSpeed.mutate({ | ||
| storyId, | ||
| itemId: lengthTargetId, | ||
| data: { speed: rate }, | ||
| }); | ||
| } | ||
| setLengthTargetId(null); | ||
| }} | ||
| disabled={ | ||
| !(Number.parseFloat(speedDraft) >= 0.25 && Number.parseFloat(speedDraft) <= 4) | ||
| } | ||
| > | ||
| Apply | ||
| </Button> | ||
| </DialogFooter> | ||
| </DialogContent> | ||
| </Dialog> |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Add error handling to the timing dialog mutation.
Every other mutation in this component passes an onError toast. This updateSpeed.mutate call does not, so a rejected request closes the dialog with no feedback and the timeline silently keeps the old speed.
♻️ Proposed fix
- updateSpeed.mutate({
- storyId,
- itemId: lengthTargetId,
- data: { speed: rate },
- });
+ updateSpeed.mutate(
+ { storyId, itemId: lengthTargetId, data: { speed: rate } },
+ {
+ onError: (error) => {
+ toast({
+ title: 'Failed to update speed',
+ description: error instanceof Error ? error.message : String(error),
+ variant: 'destructive',
+ });
+ },
+ },
+ );📝 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
| <Dialog open={lengthTargetId !== null} onOpenChange={() => setLengthTargetId(null)}> | |
| <DialogContent className="max-w-xs"> | |
| <DialogHeader> | |
| <DialogTitle>Clip timing</DialogTitle> | |
| </DialogHeader> | |
| {/* Length and speed are two views of the same number: editing one | |
| recomputes the other, and only the resulting speed is stored. */} | |
| <div className="space-y-3"> | |
| <div> | |
| <span className="mb-1 block text-xs text-muted-foreground">Target length</span> | |
| <div className="flex items-center gap-2"> | |
| <Input | |
| value={lengthDraft} | |
| onChange={(e) => { | |
| setLengthDraft(e.target.value); | |
| const target = Number.parseFloat(e.target.value); | |
| const clip = items.find((i) => i.id === lengthTargetId); | |
| if (clip && target > 0) { | |
| const natural = | |
| clip.duration * 1000 - (clip.trim_start_ms || 0) - (clip.trim_end_ms || 0); | |
| setSpeedDraft((natural / 1000 / target).toFixed(2)); | |
| } | |
| }} | |
| className="h-8" | |
| inputMode="decimal" | |
| aria-label="Target length in seconds" | |
| /> | |
| <span className="text-xs text-muted-foreground">s</span> | |
| </div> | |
| </div> | |
| <div> | |
| <span className="mb-1 block text-xs text-muted-foreground">Speed</span> | |
| <div className="flex items-center gap-2"> | |
| <Input | |
| value={speedDraft} | |
| onChange={(e) => { | |
| setSpeedDraft(e.target.value); | |
| const rate = Number.parseFloat(e.target.value); | |
| const clip = items.find((i) => i.id === lengthTargetId); | |
| if (clip && rate > 0) { | |
| const natural = | |
| clip.duration * 1000 - (clip.trim_start_ms || 0) - (clip.trim_end_ms || 0); | |
| setLengthDraft((natural / 1000 / rate).toFixed(2)); | |
| } | |
| }} | |
| className="h-8" | |
| inputMode="decimal" | |
| aria-label="Speed multiplier" | |
| /> | |
| <span className="text-xs text-muted-foreground">x</span> | |
| </div> | |
| </div> | |
| <p className="text-[10px] leading-snug text-muted-foreground"> | |
| Pitch is preserved. 0.25x to 4x; outside that the stretch smears speech. | |
| </p> | |
| </div> | |
| <DialogFooter> | |
| <Button variant="outline" onClick={() => setLengthTargetId(null)}> | |
| Cancel | |
| </Button> | |
| <Button | |
| onClick={() => { | |
| const rate = Number.parseFloat(speedDraft); | |
| if (lengthTargetId && rate >= 0.25 && rate <= 4) { | |
| updateSpeed.mutate({ | |
| storyId, | |
| itemId: lengthTargetId, | |
| data: { speed: rate }, | |
| }); | |
| } | |
| setLengthTargetId(null); | |
| }} | |
| disabled={ | |
| !(Number.parseFloat(speedDraft) >= 0.25 && Number.parseFloat(speedDraft) <= 4) | |
| } | |
| > | |
| Apply | |
| </Button> | |
| </DialogFooter> | |
| </DialogContent> | |
| </Dialog> | |
| <Dialog open={lengthTargetId !== null} onOpenChange={() => setLengthTargetId(null)}> | |
| <DialogContent className="max-w-xs"> | |
| <DialogHeader> | |
| <DialogTitle>Clip timing</DialogTitle> | |
| </DialogHeader> | |
| {/* Length and speed are two views of the same number: editing one | |
| recomputes the other, and only the resulting speed is stored. */} | |
| <div className="space-y-3"> | |
| <div> | |
| <span className="mb-1 block text-xs text-muted-foreground">Target length</span> | |
| <div className="flex items-center gap-2"> | |
| <Input | |
| value={lengthDraft} | |
| onChange={(e) => { | |
| setLengthDraft(e.target.value); | |
| const target = Number.parseFloat(e.target.value); | |
| const clip = items.find((i) => i.id === lengthTargetId); | |
| if (clip && target > 0) { | |
| const natural = | |
| clip.duration * 1000 - (clip.trim_start_ms || 0) - (clip.trim_end_ms || 0); | |
| setSpeedDraft((natural / 1000 / target).toFixed(2)); | |
| } | |
| }} | |
| className="h-8" | |
| inputMode="decimal" | |
| aria-label="Target length in seconds" | |
| /> | |
| <span className="text-xs text-muted-foreground">s</span> | |
| </div> | |
| </div> | |
| <div> | |
| <span className="mb-1 block text-xs text-muted-foreground">Speed</span> | |
| <div className="flex items-center gap-2"> | |
| <Input | |
| value={speedDraft} | |
| onChange={(e) => { | |
| setSpeedDraft(e.target.value); | |
| const rate = Number.parseFloat(e.target.value); | |
| const clip = items.find((i) => i.id === lengthTargetId); | |
| if (clip && rate > 0) { | |
| const natural = | |
| clip.duration * 1000 - (clip.trim_start_ms || 0) - (clip.trim_end_ms || 0); | |
| setLengthDraft((natural / 1000 / rate).toFixed(2)); | |
| } | |
| }} | |
| className="h-8" | |
| inputMode="decimal" | |
| aria-label="Speed multiplier" | |
| /> | |
| <span className="text-xs text-muted-foreground">x</span> | |
| </div> | |
| </div> | |
| <p className="text-[10px] leading-snug text-muted-foreground"> | |
| Pitch is preserved. 0.25x to 4x; outside that the stretch smears speech. | |
| </p> | |
| </div> | |
| <DialogFooter> | |
| <Button variant="outline" onClick={() => setLengthTargetId(null)}> | |
| Cancel | |
| </Button> | |
| <Button | |
| onClick={() => { | |
| const rate = Number.parseFloat(speedDraft); | |
| if (lengthTargetId && rate >= 0.25 && rate <= 4) { | |
| updateSpeed.mutate( | |
| { storyId, itemId: lengthTargetId, data: { speed: rate } }, | |
| { | |
| onError: (error) => { | |
| toast({ | |
| title: 'Failed to update speed', | |
| description: error instanceof Error ? error.message : String(error), | |
| variant: 'destructive', | |
| }); | |
| }, | |
| }, | |
| ); | |
| } | |
| setLengthTargetId(null); | |
| }} | |
| disabled={ | |
| !(Number.parseFloat(speedDraft) >= 0.25 && Number.parseFloat(speedDraft) <= 4) | |
| } | |
| > | |
| Apply | |
| </Button> | |
| </DialogFooter> | |
| </DialogContent> | |
| </Dialog> |
🤖 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/StoriesTab/StoryTrackEditor.tsx` around lines 2013 - 2097,
Update the updateSpeed.mutate call in the timing dialog’s Apply handler to
provide an onError callback that displays the component’s established error
toast, while preserving the existing mutation payload and validation. Ensure the
dialog’s close behavior remains unchanged and rejected requests give the user
visible feedback.
Comment on lines
+750
to
+755
| source = db.query(DBVoiceProfile).filter_by(id=profile_id).first() | ||
| if not source: | ||
| 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 length check runs before Line 755 calls strip(). The service then creates a profile named "", or " (1)" after a collision.
Trim before validation and reject an empty explicit name with a validation response.
🤖 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 750 - 755, Update
ProfileDuplicateRequest validation to strip the provided name before applying
its length check, and reject an explicitly supplied name that becomes empty
after trimming with the existing validation response. Ensure the
duplicate-profile flow around get_unique_profile_name receives the trimmed name
while preserving the default source-name copy behavior when no name is provided.
Comment on lines
+28
to
+41
| @pytest.fixture | ||
| def reload_config(monkeypatch): | ||
| """Reload ``config`` with a patched environment, restoring it afterwards.""" | ||
|
|
||
| def _reload(data_dir: str | None): | ||
| if data_dir is None: | ||
| monkeypatch.delenv("VOICEBOX_DATA_DIR", raising=False) | ||
| else: | ||
| monkeypatch.setenv("VOICEBOX_DATA_DIR", data_dir) | ||
| return importlib.reload(config) | ||
|
|
||
| yield _reload | ||
| # Leave the module matching the real process environment for later tests. | ||
| importlib.reload(config) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Restore the environment before the final reload.
Line 41 reloads config before monkeypatch restores VOICEBOX_DATA_DIR. After the final test, config._data_dir can retain the temporary from-env path. Later tests can then use an invalid or deleted temporary directory.
Proposed fix
import importlib
+import os
import sys
from pathlib import Path
@@
def reload_config(monkeypatch):
"""Reload ``config`` with a patched environment, restoring it afterwards."""
+ original_data_dir = os.environ.get("VOICEBOX_DATA_DIR")
@@
yield _reload
# Leave the module matching the real process environment for later tests.
+ if original_data_dir is None:
+ os.environ.pop("VOICEBOX_DATA_DIR", None)
+ else:
+ os.environ["VOICEBOX_DATA_DIR"] = original_data_dir
importlib.reload(config)📝 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
| @pytest.fixture | |
| def reload_config(monkeypatch): | |
| """Reload ``config`` with a patched environment, restoring it afterwards.""" | |
| def _reload(data_dir: str | None): | |
| if data_dir is None: | |
| monkeypatch.delenv("VOICEBOX_DATA_DIR", raising=False) | |
| else: | |
| monkeypatch.setenv("VOICEBOX_DATA_DIR", data_dir) | |
| return importlib.reload(config) | |
| yield _reload | |
| # Leave the module matching the real process environment for later tests. | |
| importlib.reload(config) | |
| `@pytest.fixture` | |
| def reload_config(monkeypatch): | |
| """Reload ``config`` with a patched environment, restoring it afterwards.""" | |
| original_data_dir = os.environ.get("VOICEBOX_DATA_DIR") | |
| def _reload(data_dir: str | None): | |
| if data_dir is None: | |
| monkeypatch.delenv("VOICEBOX_DATA_DIR", raising=False) | |
| else: | |
| monkeypatch.setenv("VOICEBOX_DATA_DIR", data_dir) | |
| return importlib.reload(config) | |
| yield _reload | |
| # Leave the module matching the real process environment for later tests. | |
| if original_data_dir is None: | |
| os.environ.pop("VOICEBOX_DATA_DIR", None) | |
| else: | |
| os.environ["VOICEBOX_DATA_DIR"] = original_data_dir | |
| importlib.reload(config) |
🤖 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 28 - 41, Update the
reload_config fixture teardown so monkeypatch restores VOICEBOX_DATA_DIR before
the final importlib.reload(config). Preserve the existing per-test environment
patching and ensure the final config reload observes the real process
environment rather than the temporary test value.
Two bugs found in review of jamiepine#1007. Timeline offset by 182 px ------------------------- LABEL_COL_WIDTH is 246, and the lane label cell and clip layer both use it, but the two sibling corner spacers were left at `w-16` (64 px): - the ruler corner spacer, so every time marker was drawn 182 px left of the clips it labels, and handleTimelineClick — which subtracts LABEL_COL_WIDTH — seeked to the wrong time when clicking the ruler - the scrollbar corner spacer, so the thumb no longer tracked the view Drive both from LABEL_COL_WIDTH so they cannot drift again. Deleting a story folder left its stories invisible -------------------------------------------------- FolderKind has three values, but useDeleteFolder used a two-way ternary (`kind === 'voice' ? 'profiles' : 'history'`), so deleting a *story* folder invalidated ['history'] and never ['stories'] — the key useStories reads. Member stories kept the deleted folder_id in cache, and StoryList filters on that, so they vanished from both the deleted folder and Uncategorised until some unrelated refetch. Replace the ternary with a Record<FolderKind, string> map, so adding a fourth kind is a type error rather than a silently stale list. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Lvigentini
pushed a commit
to Lvigentini/voicebox
that referenced
this pull request
Aug 8, 2026
Two bugs found in review of jamiepine#1007. Timeline offset by 182 px ------------------------- LABEL_COL_WIDTH is 246, and the lane label cell and clip layer both use it, but the two sibling corner spacers were left at `w-16` (64 px): - the ruler corner spacer, so every time marker was drawn 182 px left of the clips it labels, and handleTimelineClick — which subtracts LABEL_COL_WIDTH — seeked to the wrong time when clicking the ruler - the scrollbar corner spacer, so the thumb no longer tracked the view Drive both from LABEL_COL_WIDTH so they cannot drift again. Deleting a story folder left its stories invisible -------------------------------------------------- FolderKind has three values, but useDeleteFolder used a two-way ternary (`kind === 'voice' ? 'profiles' : 'history'`), so deleting a *story* folder invalidated ['history'] and never ['stories'] — the key useStories reads. Member stories kept the deleted folder_id in cache, and StoryList filters on that, so they vanished from both the deleted folder and Uncategorised until some unrelated refetch. Replace the ternary with a Record<FolderKind, string> map, so adding a fourth kind is a type error rather than a silently stale list. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Author
|
Thanks @coderabbitai — went through the review. Two of the findings were real and are fixed in f331d30; one I think is a false positive. Fixed
Not applying: The concern was that
The remaining comments are noted — the debounced volume slider in particular looks worth doing, and I'll follow up separately rather than grow this PR. |
Contributor
|
Tip For best results, initiate chat on the files or code changes.
I confirm that the two fixes address the reported issues. Your Starlette route analysis is correct. If you want, I can create a follow-up GitHub issue for the debounced volume-slider update. 🐇 ✅ ✏️ Learnings added
You are interacting with an AI system. |
Two problems with the clips pane once a library gets real. Folder list had no scroll container ----------------------------------- ClipFolderTree rendered every folder inline with no height constraint, so a few dozen folders pushed the clip list off-screen and there was nothing to scroll. The tree now lives in its own `max-h-[40vh]` scroll area. "All clips" and "Uncategorised" stay pinned outside it, so the two pseudo-folders are always reachable no matter how far the list is scrolled. No way to find a clip inside a folder ------------------------------------- Folders scope the list, but a folder with a few hundred clips still had to be scrolled by eye. Adds a search box using the shared ListPaneSearch, matching what the Stories and Captures panes already do — folder first, then text, so the folder is a scope and the search runs inside it. The search runs server-side: `/history` already accepted a `search` parameter and HistoryQuery already carried it, the UI just never sent one. That matters here because the list is paginated — a client-side filter would only have searched the pages already loaded. Debounced through a new useDebouncedValue hook, since otherwise every keystroke is a request. StoryList doesn't need one; it filters an in-memory list. Changing the term resets to page 0, mirroring the folder-change reset, or the first response lands at a stale offset and the list looks empty. The empty state is search-aware too — "No voice generations, yet…" is misleading when the library is full and the query simply matched nothing. 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 (2)
app/src/components/History/HistoryTable.tsx (2)
478-483: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winReset accumulated history after generation folder moves.
useSetGenerationFolderinvalidates['history'], butHistoryTabledoes not resetpage/allHistoryonsetGenerationFolder.isSuccess. After a move beyond page 0, the accumulator appends new page data without removing the moved generation, so it can remain visible after the move. ResetpageandallHistoryon mutation success, or update/remove the moved ID from the accumulated list.Also applies to the Move To handler at lines 759-797.
🤖 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 478 - 483, Update the HistoryTable folder-move flows, including ClipFolderTree’s onDropItem and the Move To handler, to reset page and allHistory when setGenerationFolder reports success. Ensure the accumulated history is cleared before refetching so moved generations cannot remain visible; preserve the existing mutation behavior.
555-556: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd localized
history.dragHandlevalues.
HistoryTablereadst('history.dragHandle'), but only the English locale defines it. Add localized values to the remaining locales so the drag handle label does not fall back to English.🤖 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 555 - 556, Add the missing history.dragHandle translation to every non-English locale resource used by HistoryTable, preserving each locale’s language and the existing translation structure so t('history.dragHandle') never falls back to English.
🤖 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 `@app/src/components/History/HistoryTable.tsx`:
- Around line 478-483: Update the HistoryTable folder-move flows, including
ClipFolderTree’s onDropItem and the Move To handler, to reset page and
allHistory when setGenerationFolder reports success. Ensure the accumulated
history is cleared before refetching so moved generations cannot remain visible;
preserve the existing mutation behavior.
- Around line 555-556: Add the missing history.dragHandle translation to every
non-English locale resource used by HistoryTable, preserving each locale’s
language and the existing translation structure so t('history.dragHandle') never
falls back to English.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1ae99fb0-b347-4b0e-96a5-1340c082ee69
📒 Files selected for processing (12)
app/src/components/History/ClipFolderTree.tsxapp/src/components/History/HistoryTable.tsxapp/src/i18n/locales/en/translation.jsonapp/src/i18n/locales/es/translation.jsonapp/src/i18n/locales/fr/translation.jsonapp/src/i18n/locales/it/translation.jsonapp/src/i18n/locales/ja/translation.jsonapp/src/i18n/locales/ko/translation.jsonapp/src/i18n/locales/pt-BR/translation.jsonapp/src/i18n/locales/zh-CN/translation.jsonapp/src/i18n/locales/zh-TW/translation.jsonapp/src/lib/hooks/useDebouncedValue.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- app/src/i18n/locales/en/translation.json
- app/src/components/History/ClipFolderTree.tsx
Follow-up to the search box. Resetting to page 0 on every query change is correct, but it made the list blink through an empty state on each keystroke, which read as "no results" rather than "loading". Keep the previous page ---------------------- `useHistory` now sets `placeholderData: keepPreviousData`. The current rows stay on screen while the new scope loads instead of unmounting. The three other callers have static query keys, so it never engages for them. Show that they're stale ----------------------- Kept rows are dimmed via `isPlaceholderData`. Without it the old results look like genuine matches for what was just typed, which is worse than a blank list — it looks wrong rather than busy. Don't claim empty before the first load --------------------------------------- The empty state is suppressed while `isLoading`. There is no previous page to keep on a cold open, so the "no clips" message used to flash before the first rows arrived. Scroll back to the top ---------------------- Changing folder or search term resets the scroll position. Otherwise you keep the offset from the previous, longer list and land mid-way through the new one — or past its end, looking at nothing, which is the same failure the empty flash caused. Clearing the search ------------------- ListPaneSearch grows a clear button when it has a value, plus Escape to clear. Shared with the Stories and Captures panes, which had no way to reset a search short of selecting the text. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Remaining review findings on jamiepine#1007. Volume slider wrote on every step --------------------------------- `onValueChange` fires continuously while dragging, so one gesture issued dozens of PUTs, each invalidating the track query, and out-of-order responses snapped the thumb backwards mid-drag. Now drives local state while dragging and persists on `onValueCommit`, which is what ClipVolumePopover already did. Ducking depended on dict order ------------------------------ The mixdown applied each envelope in place while later iterations read the same lanes. With two lanes ducking under each other, whichever was processed first read an untouched source and the second read an already-ducked one — the same project mixed differently depending on dict order. Envelopes are now computed from the pre-ducking lanes, then applied. A lane could also duck under itself, attenuating by its own envelope, which is quietest exactly where the lane is loudest. Rejected at the route. Failures that looked like normal states --------------------------------------- Duplicating from a voice card had no success or error callbacks, so a failure just left the button idle — the list view already toasts both. And the preview dialog treated a failed samples request as "this voice has no reference audio", which is the normal state for preset and designed voices; a load error now says so. Not changed: `storyContent.export.normalized` was flagged for hardcoding MP3, but the handler passes 'mp3' explicitly, so the label matches the behaviour. Offering normalisation for other formats is a feature, not a fix. 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
Remaining review findings on jamiepine#1007. Volume slider wrote on every step --------------------------------- `onValueChange` fires continuously while dragging, so one gesture issued dozens of PUTs, each invalidating the track query, and out-of-order responses snapped the thumb backwards mid-drag. Now drives local state while dragging and persists on `onValueCommit`, which is what ClipVolumePopover already did. Ducking depended on dict order ------------------------------ The mixdown applied each envelope in place while later iterations read the same lanes. With two lanes ducking under each other, whichever was processed first read an untouched source and the second read an already-ducked one — the same project mixed differently depending on dict order. Envelopes are now computed from the pre-ducking lanes, then applied. A lane could also duck under itself, attenuating by its own envelope, which is quietest exactly where the lane is loudest. Rejected at the route. Failures that looked like normal states --------------------------------------- Duplicating from a voice card had no success or error callbacks, so a failure just left the button idle — the list view already toasts both. And the preview dialog treated a failed samples request as "this voice has no reference audio", which is the normal state for preset and designed voices; a load error now says so. Not changed: `storyContent.export.normalized` was flagged for hardcoding MP3, but the handler passes 'mp3' explicitly, so the label matches the behaviour. Offering normalisation for other formats is a feature, not a fix. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two findings CodeRabbit could only post outside the diff. Moved clips could linger ------------------------ Same bug as the one fixed on the folders PR, which this branch predates, so the fix never reached here. Past page 0 the refreshed first page is appended to allHistory rather than replacing it, leaving a clip visible in a folder it no longer belongs to. All three move paths — drag-and-drop onto a folder, Move To, and Uncategorised — now go through one handler that drops to page 0. This branch has the drag-and-drop path the other one doesn't, so it needed covering here regardless. history.dragHandle was English-only ----------------------------------- Defined only in the English bundle while the `history` block exists in all nine, so every other locale silently fell back. It is an aria-label, which is a worse thing to leave untranslated than visible text. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Lvigentini
force-pushed
the
feat/stories-timeline-upstream
branch
from
August 9, 2026 13:46
1a8203d to
4530217
Compare
jamiepine#869 asks for a format parameter on /audio so callers can get MP3 without transcoding themselves. The encoder this needs already landed in this branch for story export -- EXPORT_FORMATS and encode_audio in utils/audio.py -- so the endpoints only had to reach for it. GET /audio/{generation_id} and GET /audio/version/{version_id} now take ?format=wav|mp3|ogg|opus|flac. Omitted, the stored file is streamed by FileResponse exactly as before, so existing callers and range requests are untouched; a transcode has to buffer the whole encode, so it is only paid for when asked for. A file already in the requested container is handed back as-is rather than round-tripped through the encoder. Decoding uses librosa at the file native rate and channel count, matching the story mixdown, so a transcode is a container change and not a resample. Opus is the exception -- libsndfile only encodes it at 48 kHz -- and encode_audio already handles that resample. An unsupported container is a 400 naming what is on offer, not a silent fallback to WAV. Fixes jamiepine#869 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Aug 9, 2026
…mposes jamiepine#1016 adds voice search and threads a query into ProfileList as a prop. This branch rewrites the same component for folders and a card/list toggle, so the two collide in exactly one file. Rather than leave that to whoever rebases second, ProfileList now takes the same `search` prop and does the filtering folder-aware. Filtering happens before bucketing, which is the part that only makes sense once both features exist: a match stays in the folder it belongs to instead of collapsing into one flat list. Folders with no matches are hidden while a query is active -- a wall of empty folder headers is noise -- but they come back when the query clears, since the header is also the drop target for filing a voice. The match predicate is jamiepine#1016's (name, description, language, preset_engine, default_engine), and so is the alphabetical tiebreak in the sort; without it the order inside a folder is whatever the API returned. Their selected-profile-first rule is deliberately not adopted -- it reads differently once voices are grouped, and it interacts with the scroll-into-view effect above, so that is a UX call for the author of jamiepine#1016 and the maintainer, not one to make silently here. `profiles.list.noVoicesMatch` is added in all nine locales rather than English only, so the string is not left half-translated. No search box is added here. The input, the VoicesTab filters and the layout work stay in jamiepine#1016; this is only the seam it plugs into. Refs jamiepine#1016, jamiepine#966 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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>
Two live review findings on jamiepine#1007. The rest of that review is stale -- the ruler offset, the mixer slider write-per-step, ducking determinism and the silent failures were fixed in later commits, and the whitespace-name and folder-cache findings arrived with the b74eb6a cherry-pick. `duck_under_track` had no lower bound, so a negative lane index was accepted and then silently never matched a lane at mix time -- a setting that looks saved and does nothing. Bounded with `ge=0`. Self-ducking was already rejected in the route, which is where the lane's own index is known; that half of the finding was already addressed. `duplicate_profile`'s ValueError was re-raised as an HTTPException without `from e`, dropping the original traceback. Three tests: self-ducking, a negative target, and a valid one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Author
|
@coderabbitai — triaged all 18 findings. Two were live and are fixed in Fixed now:
Already fixed, in commits after the review:
Assessed and not taken — Still open and worth doing, just not in this commit: atomic profile-name allocation (a real check-then-act, same shape as the one I just fixed in #1025), the |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
git show --no-ext-diff --unified=12 --format=fuller 3460a59 -- \
backend/models.py \
backend/routes/profiles.py \
backend/routes/folders.py
printf '\n--- route declarations ---\n'
rg -n -C 3 \
'`@router`\.(put|post|patch|delete)\("/profiles/\{profile_id\}(/folder)?"' \
backend/routes/profiles.py backend/routes/folders.py
printf '\n--- relevant model and handler structure ---\n'
ast-grep outline backend/models.py --items all --match 'StoryTrackUpsert|ProfileDuplicateRequest'
ast-grep outline backend/routes/profiles.py --items all --match 'duplicate_profile'Length of output: 5969
Your route analysis is also correct. The remaining items are valid follow-up candidates. Would you like me to create GitHub issues for atomic profile-name allocation, the ✏️ Learnings added
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! You are interacting with an AI system. |
Author
|
Follow-up note for whenever this lands: #1026 adds the backend half of #870 — What it deliberately leaves out is the part that belongs here: when a correction changes a segment's length, the following clips on that lane need to ripple by the delta, or the arrangement drifts. This PR already has ripple move, so it's the right home for it — I kept it out of #1026 rather than write timeline code against Once both are in, the flow is: edit text → regenerate → item repins to the new version → lane ripples by the duration delta, with keep-timing as the opt-out. |
…rop target Three remaining review findings on jamiepine#1007. Name allocation was check-then-act ---------------------------------- `get_unique_profile_name` only SELECTs, and the INSERT happens later, so two concurrent duplicates or imports could both be handed the same name -- the second then failed the UNIQUE constraint and surfaced as a 500 rather than falling through to the next suffix. `insert_profile_with_unique_name` settles the name by inserting it, retrying on IntegrityError with the next suffix, so the constraint arbitrates instead of erroring. `build_row` is a callback because a rolled-back commit expunges the instance and each attempt needs a fresh one. Called before any file work in `duplicate_profile`: retrying after the sample copies would mean undoing them, and the profile directory is keyed by the new id rather than the name, so a name retry does not touch it. `get_unique_profile_name` stays for callers that need a candidate name before they have a row, with its raciness documented rather than implied. Drop target ran the browser default on rejected payloads -------------------------------------------------------- `onDrop` returned before `preventDefault()` when the payload was missing or was not a voice, but `onDragOver` had already accepted the element as a drop target. In a webview a dropped file or URL then navigates the page away. Cancel first, decide second. `onDragLeave` also fired whenever the pointer crossed into the header's own children -- the toggle button, the count badge -- so the highlight flickered for the whole drag. Only a leave of the header itself counts now. soundfile floor did not match the promise ----------------------------------------- `EXPORT_FORMATS` documents MP3 and Opus writing with no ffmpeg, which holds only from the wheel bundling libsndfile 1.2.2. The floor was `>=0.12.0`, whose bundled 1.1.0 can write neither. Raised to `>=0.13.0` in both requirements files, with the reason recorded next to it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Author
|
The three findings I'd left open are now done in Name allocation was check-then-act. Two details worth flagging for review: The drop target ran the browser default on rejected payloads. Correct and the more serious of the two UI ones — The 191 backend tests pass, frontend typecheck clean, no new lint. |
Lvigentini
added a commit
to Lvigentini/voicebox
that referenced
this pull request
Aug 10, 2026
Listening rejected `<prosody rate>` as distorting the audio. The cause was the algorithm, not the idea: `librosa.effects.time_stretch` is a phase vocoder, which reconstructs from magnitudes and re-estimated phase. On speech that smears consonants and leaves the characteristic phasey ring. WSOLA never leaves the time domain. It overlap-adds real waveform segments, choosing each splice point by cross-correlation so successive pitch periods line up, so nothing is resynthesised and consonants stay intact. No new dependency -- it is numpy. Resampling remains the wrong answer for a different reason: it would transpose the voice, which is not what a rate directive means. There is a test for that now, comparing the dominant frequency before and after. A bug the tests caught: the first version folded the search offset back into the read pointer, so a run of forward-biased matches accelerated the read and returned 1.47s where 2.0s was asked for. The nominal pointer has to advance by `analysis_hop` regardless of where the search landed. Ratios are accurate now across 0.5x to 2.0x. Worth recording for whoever picks this up: `services/stories.py` applies per-clip speed with the same phase vocoder, so the story mixer has the same artefact. This function is self-contained and should move there too -- tracked separately since it belongs to jamiepine#1007 rather than here. 4 further tests: ratio accuracy across five rates, pitch preservation, and that silence stays silent rather than ringing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Per-clip speed used `librosa.effects.time_stretch`, a phase vocoder. It reconstructs from magnitudes and re-estimated phase, which on speech smears consonants and leaves a phasey ring -- rejected outright in listening tests while checking the same algorithm elsewhere. Pitch survived either way; only the artefacts differ. `time_stretch_speech` in utils/audio.py replaces it with WSOLA, which stays in the time domain and overlap-adds real waveform segments, choosing each splice by cross-correlation so pitch periods line up. Nothing is resynthesised, so consonants stay intact. numpy only, no new dependency. Stereo needed more than a loop ------------------------------ Stretching each channel separately looked obvious and was wrong twice over. The splice search is content-dependent, so left and right choose different points: the stereo image tears, and the channels come out different lengths, which `np.stack` rejects outright -- caught by a test asserting stereo keeps its shape. Splices are now planned once from the mono downmix and applied to every channel, so all channels are cut identically, the image survives, and the lengths match by construction. The mixer works in `(channels, samples)` throughout, so this is the shape it actually hits -- every story clip with a speed adjustment was going through the broken path. 6 tests: ratio accuracy across four rates, pitch preservation by comparing the dominant frequency before and after, and stereo keeping both its shape and its length. Same helper is on the prosody branch for `<prosody rate>`; whichever merges second should point at this one rather than keeping a second copy. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Author
|
Pushed Speed used a phase vocoder. Replaced with WSOLA in Stereo was broken in a way the loop hid. The obvious implementation — stretch each channel — is wrong twice, because the splice search is content-dependent:
The mixer works in Splices are now planned once from the mono downmix and applied to every channel, so all channels cut identically, the image survives, and lengths match by construction. Found by a test asserting stereo keeps its shape — the naive version failed it immediately. 6 tests: ratio accuracy across four rates, pitch preservation via dominant-frequency comparison before and after, and stereo keeping shape and length. 197 backend tests pass, no new lint. |
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 #1006.
What it does
story_trackstable with per-lane name, volume, mute, solo and optional duck-under-lane. Keyed by(story_id, index), sostory_items.trackstays a plain integer and drag/move/reorder are untouched; a lane with no row mixes at unity gain.Backend:
routes/stories.py,services/stories.py,services/history.py, plusbackend/tests/test_story_mixdown.py. Frontend:StoryTrackEditor,TrackMixerControls,StoryContent,StoryList,StoryChatItem,useStories.Also fixes #869
#869 asks for a
formatparameter on/audioso API callers can get MP3 without transcoding themselves. The encoder that needs —EXPORT_FORMATSandencode_audioinutils/audio.py— is already in this branch for story export, so the endpoints only had to reach for it.GET /audio/{generation_id}andGET /audio/version/{version_id}now take?format=wav|mp3|ogg|opus|flac. Omitted, the stored file is streamed byFileResponseexactly as before, so existing callers and range requests are untouched; the transcode buffers a whole encode, so it is only paid for when asked for. A file already in the requested container is handed back as-is rather than round-tripped. An unsupported container is a 400 naming what is on offer, not a silent fallback to WAV.backend/tests/test_audio_format_param.py— 10 tests covering the default path, the same-container shortcut, each container, sample-rate preservation, the Opus 48 kHz resample, and both rejections.Fixes #869
Possible fix for #715
#715 reports clips cut off in playback and blending on export. This changes clip duration to come from the decoded array rather than
generation.duration— a clip pinned to a version of a different length was mistimed, and a NULL duration raisedTypeErrorinside a bareexcept: continue, silently dropping the clip from the export. Same shape as the report, but I have not reproduced #715 directly, so flagging rather than claiming. Refs #715.Stacking
Carries the folder work from #1005, including its review-fix commit (
b74eb6a, cherry-picked here — this branch had drifted behind #1005 and was showing already-fixed bugs). It also carriesd5eea00, the backend half of #1004, but not #1004's two Tauri commits.So the branches aren't strict ancestors of one another — they're independent branches that share commits. Verified with
git cherry: #1007 now contains every patch in #1005. Merge order #1004 → #1005 → this still reads cleanest, but nothing breaks in another order.Notes for review
CHANGELOG.md, since releases and that file are yours to manage.Out of scope, on purpose
#870 — editing a segment's text. This PR gives a segment trim, fades, speed, split, duplicate and version-swap, but no way to fix a typo and regenerate in place. That's the most obvious remaining hole in the timeline, and I'd rather flag it than have it read as an oversight: it needs a regenerate-and-reattach path through the item's
generation_id, which is a different change from the mixing work here. Happy to take it as a follow-up if you want it.#55 — inline pause/prosody markup in stories. Same reasoning; it belongs at the text layer, not the mixer.
Screenshots
Stories list and audio export
The Stories tab with the story list, story content, and the
Export Audioformat picker: WAV, MP3, OGG Vorbis, Opus and FLAC all written by the bundled libsndfile with no ffmpeg required. The separateMasteringentry —MP3, loudness normalised— is the EBU R128 path, which is the one option that needs ffmpeg on PATH; without it the request still succeeds using peak normalisation.Multi-track timeline editor
The track editor with several lanes and clips. Note the ruler alignment:
LABEL_COL_WIDTHnow drives the ruler and scrollbar corner spacers, fixing the 182 px offset described in f331d30.Per-lane mixer and ducking
Per-lane volume, mute, solo and
Duck under track, which attenuates a lane against the finished output of another — how a music bed sits under narration.🤖 Generated with Claude Code
Summary by CodeRabbit
Summary by CodeRabbit