Add post-processing audio effects system#271
Merged
Conversation
Adds a full effects pipeline powered by Spotify's pedalboard library, enabling users to apply professional DSP effects (flanger, reverb, delay, compressor, pitch shift, filters, gain) to generated audio. Key features: - Effects chain editor with drag-and-drop reordering (dnd-kit) - Generation versions: clean copy always saved, processed versions created on top - Built-in presets (Robotic, Radio, Echo Chamber, Deep Voice) + custom user presets - Per-profile default effects chain (auto-applied to new generations) - Per-generation effects override from the generation form - Apply effects to existing generations from history (creates new version) - Ephemeral preview endpoint for auditioning effects without persisting - Dedicated Effects sidebar tab with preset management and live preview - Version switcher in history cards with expandable panel - Backward-compatible: existing generations backfilled as clean versions
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:
📝 WalkthroughWalkthroughAdds a full effects/post-processing system: frontend editors and UI, presets and preset CRUD, preview/apply/versioning of processed audio, a pedalboard-based backend effects engine, DB models for presets and generation versions, API client methods/types, and integration into generation, history, and profile flows. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant FE as Frontend
participant API as ApiClient
participant BE as Backend
participant Engine as EffectsEngine
participant DB as Database
User->>FE: Open Effects tab / edit chain
FE->>API: GET /effects/available
API->>BE: GET /effects/available
BE->>Engine: get_available_effects()
Engine-->>BE: effect types & params
BE-->>API: AvailableEffectsResponse
API-->>FE: display registry
sequenceDiagram
actor User
participant FE as Frontend
participant API as ApiClient
participant BE as Backend
participant Engine as EffectsEngine
participant DB as Database
User->>FE: Preview effects for generation
FE->>API: POST /effects/preview/{generationId} (effects_chain)
API->>BE: preview_effects endpoint
BE->>DB: fetch generation audio (clean)
BE->>Engine: apply_effects(clean_audio, effects_chain)
Engine-->>BE: processed audio blob
BE-->>API: Blob response
API-->>FE: play preview
sequenceDiagram
actor User
participant FE as Frontend
participant API as ApiClient
participant BE as Backend
participant Engine as EffectsEngine
participant DB as Database
User->>FE: Apply effects to generation
FE->>API: POST /generations/{id}/versions/apply-effects
API->>BE: apply_effects_to_generation
BE->>DB: create_version(generation_id, label="clean", audio_path=clean)
BE->>Engine: apply_effects(clean_audio, effects_chain)
Engine-->>BE: processed audio file
BE->>DB: create_version(generation_id, label="processed", audio_path=processed, effects_chain)
BE->>DB: set_default (if requested) / update generation.audio_path
BE-->>API: GenerationVersionResponse
API-->>FE: refresh history / show new version
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/history.py (1)
285-300:⚠️ Potential issue | 🟠 MajorMissing version cleanup in
delete_generations_by_profile.This function deletes generations but doesn't clean up associated
GenerationVersionrecords or their audio files, causing orphaned data. Apply the same cleanup pattern used indelete_generation.🐛 Proposed fix
async def delete_generations_by_profile( profile_id: str, db: Session, ) -> int: """ Delete all generations for a profile. ... """ + from . import versions as versions_mod + generations = db.query(DBGeneration).filter_by(profile_id=profile_id).all() count = 0 for generation in generations: + # Delete all version files and records + versions_mod.delete_versions_for_generation(generation.id, db) + - # Delete audio file - audio_path = Path(generation.audio_path) - if audio_path.exists(): - audio_path.unlink() + # Delete main audio file (if not already removed by version cleanup) + if generation.audio_path: + audio_path = Path(generation.audio_path) + if audio_path.exists(): + audio_path.unlink() # Delete from database db.delete(generation) count += 1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/history.py` around lines 285 - 300, The batch deletion in delete_generations_by_profile currently removes DBGeneration rows and their audio files but leaves GenerationVersion records and their audio files orphaned; update delete_generations_by_profile to mirror the cleanup in delete_generation by iterating each GenerationVersion for every generation (use the GenerationVersion model / generation.versions relationship or a query for GenerationVersion where generation_id == generation.id), delete any version.audio_path files on disk, delete the GenerationVersion rows from the session, then proceed to delete the DBGeneration and its audio file before committing and returning the count.app/src/lib/hooks/useGenerationForm.ts (1)
108-121:⚠️ Potential issue | 🟠 MajorPreserve
[]as an explicit per-generation override.Line 121 drops empty chains by converting them to
undefined. In the backend that means “use the profile default”, so a user cannot override a profile’s default effects with “none” for a single generation. Send[]whengetEffectsChain()returns an empty array, and reserveundefinedfor “no override provided”.🛠️ Suggested change
- effects_chain: effectsChain?.length ? effectsChain : undefined, + effects_chain: effectsChain !== undefined ? effectsChain : undefined,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/lib/hooks/useGenerationForm.ts` around lines 108 - 121, The code currently converts an empty effectsChain to undefined when calling generation.mutateAsync, which causes the backend to apply the profile default; instead, preserve an explicit empty array as a per-generation override. Change the value passed for effects_chain in the generation.mutateAsync call (where effectsChain is sourced from options.getEffectsChain()) so that if options.getEffectsChain() returns undefined you pass undefined, but if it returns an array (including []), you pass that array unchanged (e.g., use a check like Array.isArray(effectsChain) ? effectsChain : undefined or effectsChain === undefined ? undefined : effectsChain) so empty chains are sent as [] and undefined remains the “no override provided” sentinel.
🧹 Nitpick comments (11)
backend/database.py (1)
422-446: Unused loop variablekey.The loop variable
keyis not used. Per static analysis hint, rename to_keyto indicate intentional non-use.♻️ Proposed fix
- for idx, (key, preset_data) in enumerate(BUILTIN_PRESETS.items()): + for idx, (_key, preset_data) in enumerate(BUILTIN_PRESETS.items()):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/database.py` around lines 422 - 446, In _seed_builtin_presets(), the loop unpacks (key, preset_data) from BUILTIN_PRESETS but never uses key; update the loop to use a clearly intended unused variable name (e.g., rename key to _key) in the for statement (for idx, (_key, preset_data) in enumerate(BUILTIN_PRESETS.items()):) so static analysis no longer flags an unused variable while leaving all logic in _seed_builtin_presets, BUILTIN_PRESETS handling, and subsequent database operations unchanged.backend/models.py (2)
361-363: Consider ifEffectsChainwrapper is needed.This model wraps a list of
EffectConfigbut doesn't appear to be used in the API endpoints (they useList[EffectConfig]directly). Consider removing it if unused, or document its intended use case.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/models.py` around lines 361 - 363, The EffectsChain wrapper class (EffectsChain : BaseModel with effects: List[EffectConfig]) appears unused by the API which accepts List[EffectConfig] directly; either remove the EffectsChain model to avoid dead code or update usages and docs to justify it. Search for the EffectsChain symbol and any references to effects: List[EffectConfig] (and the EffectConfig model) and either (A) delete the EffectsChain class and replace any remaining references with plain List[EffectConfig], or (B) keep EffectsChain and change endpoint signatures, pydantic validators, and docstrings to accept/return EffectsChain and add a short docstring explaining its purpose. Ensure tests and imports are updated accordingly.
428-433: Consider stronger typing forparamsdict.The
paramsfield could be typed asDict[str, AvailableEffectParam]for better API documentation and type safety, since you have theAvailableEffectParammodel defined.+from typing import Dict + class AvailableEffect(BaseModel): """Description of an available effect type.""" type: str label: str description: str - params: dict # param_name -> AvailableEffectParam + params: Dict[str, AvailableEffectParam]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/models.py` around lines 428 - 433, The params field on the AvailableEffect model is currently untyped dict; update it to use a typed mapping for better docs and type safety by changing params to Dict[str, AvailableEffectParam] on the AvailableEffect class and ensure typing.Dict (or from typing import Dict) is imported and that AvailableEffectParam is referenced/imported where needed.backend/history.py (1)
23-57: Silent exception swallowing and code duplication.
Lines 43-44 silently swallow exceptions (same issue as
profiles.py). Consider logging.This helper duplicates logic from
versions._version_response. Consider importing and reusing that function.♻️ Proposed refactor using existing helper
def _get_versions_for_generation(generation_id: str, db: Session) -> tuple: """Get versions list and active version ID for a generation.""" - import json + from . import versions as versions_mod + versions_rows = ( db.query(DBGenerationVersion) .filter_by(generation_id=generation_id) .order_by(DBGenerationVersion.created_at) .all() ) if not versions_rows: return None, None - versions = [] + versions = [versions_mod._version_response(v) for v in versions_rows] active_version_id = None - for v in versions_rows: - effects_chain = None - if v.effects_chain: - try: - raw = json.loads(v.effects_chain) - effects_chain = [EffectConfig(**e) for e in raw] - except Exception: - pass - versions.append(GenerationVersionResponse( - id=v.id, - generation_id=v.generation_id, - label=v.label, - audio_path=v.audio_path, - effects_chain=effects_chain, - is_default=v.is_default, - created_at=v.created_at, - )) - if v.is_default: - active_version_id = v.id + for v in versions: + if v.is_default: + active_version_id = v.id + break return versions, active_version_id🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/history.py` around lines 23 - 57, The helper _get_versions_for_generation is silently swallowing JSON parse errors when building effects_chain and is duplicating the formatting logic present in versions._version_response; fix it by importing and calling versions._version_response (or the specific function that builds a GenerationVersionResponse) to construct each response instead of manually creating GenerationVersionResponse objects, and replace the bare except with a narrow except catching json.JSONDecodeError (or Exception if necessary) while logging the error via the module logger (or processLogger) including context (generation_id and DBGenerationVersion.id) so failures aren’t silent; ensure you still set active_version_id when v.is_default.backend/versions.py (1)
139-181: Consider consolidating commits for atomicity.The function has multiple
db.commit()calls (lines 163, 175, 179) which could leave the database in an inconsistent state if an error occurs between commits. Consider using a single commit at the end.♻️ Proposed consolidation
db.delete(version) - db.commit() + db.flush() # Make deletion visible within transaction # If this was the default, promote the first remaining version if was_default: first = ( db.query(DBGenerationVersion) .filter_by(generation_id=gen_id) .order_by(DBGenerationVersion.created_at) .first() ) if first: first.is_default = True - db.commit() gen = db.query(DBGeneration).filter_by(id=gen_id).first() if gen: gen.audio_path = first.audio_path - db.commit() + + db.commit() # Single commit for atomicity return True🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/versions.py` around lines 139 - 181, The delete_version function currently calls db.commit() multiple times; wrap the DB mutations in a single transaction so all changes are atomic: determine was_default and gen_id as before, delete the DBGenerationVersion object (db.delete(version)), if was_default find the first remaining DBGenerationVersion (query on DBGenerationVersion with generation_id) and set first.is_default = True and update the related DBGeneration.audio_path to first.audio_path, then perform one db.commit() at the end; ensure the file deletion (Path(version.audio_path).unlink()) is performed either before the DB commit or handled so failures don't leave the DB partially updated, and keep using the same Session methods (delete, queries) on DBGenerationVersion and DBGeneration to locate and update records.backend/effects.py (1)
17-27: Consider adding error handling for corrupt data.Unlike
_profile_to_responseinprofiles.py, this helper doesn't catch JSON parse errors. While validation at write time should prevent corrupt data, consider adding defensive error handling for consistency and to guard against manual DB edits or migrations.♻️ Proposed fix
def _preset_response(p: DBEffectPreset) -> EffectPresetResponse: """Convert a DB preset row to a Pydantic response.""" - effects_chain = [EffectConfig(**e) for e in json.loads(p.effects_chain)] + try: + effects_chain = [EffectConfig(**e) for e in json.loads(p.effects_chain)] + except Exception as e: + import logging + logging.warning(f"Failed to parse effects_chain for preset {p.id}: {e}") + effects_chain = [] return EffectPresetResponse(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/effects.py` around lines 17 - 27, _wrap the JSON parse and Pydantic construction in _preset_response so corrupt DB data is handled like _profile_to_response: try to json.loads(p.effects_chain) and build EffectConfig(**e) items, and catch json.JSONDecodeError, TypeError and pydantic.ValidationError; on error log a warning that includes p.id and the raw p.effects_chain, set effects_chain to an empty list (or a safe default) and continue returning EffectPresetResponse. Ensure you reference EffectConfig and EffectPresetResponse in the fix so the fallback shape matches the response schema.app/src/components/Effects/EffectsChainEditor.tsx (3)
251-272: Remove unusedindexprop from SortableEffectItem.The
indexprop is declared inSortableEffectItemPropsand passed to the component but never used withinSortableEffectItem. This is dead code.🧹 Proposed cleanup
interface SortableEffectItemProps { id: string; effect: EffectConfig; - index: number; effectDef?: AvailableEffect; isExpanded: boolean; onToggleExpand: () => void; onRemove: () => void; onToggleEnabled: () => void; onUpdateParam: (paramName: string, paramValue: number) => void; } function SortableEffectItem({ id, effect, - index, effectDef, isExpanded, ...And update the call site at line 215:
- <SortableEffectItem - key={effect._id} - id={effect._id} - effect={effect} - index={index} + <SortableEffectItem + key={effect._id} + id={effect._id} + effect={effect}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/Effects/EffectsChainEditor.tsx` around lines 251 - 272, The SortableEffectItem component declares and receives an unused index prop causing dead code: remove index from the SortableEffectItemProps interface and from the function parameter destructuring in SortableEffectItem, and then update its call site(s) (where SortableEffectItem is instantiated) to stop passing the index prop; keep all other props and handlers (effect, effectDef, isExpanded, onToggleExpand, onRemove, onToggleEnabled, onUpdateParam) unchanged so behavior remains identical.
40-43: Module-level mutable counter may cause ID collisions.The
nextIdcounter persists across component instances and won't reset during HMR. If multipleEffectsChainEditorinstances exist or during development hot reloads, this could lead to unexpected behavior when IDs are compared across different editor instances.♻️ Suggested improvement using crypto.randomUUID()
-let nextId = 0; -function makeId() { - return `fx-${++nextId}`; +function makeId() { + return `fx-${crypto.randomUUID().slice(0, 8)}`; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/Effects/EffectsChainEditor.tsx` around lines 40 - 43, The module-level mutable counter nextId used by makeId() can collide across component instances and HMR; replace it with a non-global unique-ID approach (for example use crypto.randomUUID() or React's useId) inside the EffectsChainEditor instance so IDs are generated per instance rather than from a shared nextId; update all usages that call makeId() to call the new instance-scoped ID generator (or inline crypto.randomUUID()) and remove the module-level nextId to prevent cross-instance/HMR collisions.
63-73: ID synchronization may break with external value changes.The
idsRefapproach assumes thevaluearray only changes via this component's callbacks. If the parent modifies the array externally (e.g., reordering, inserting in the middle), the IDs won't correspond to the correct effects since they're matched by index position.This is acceptable if the component contract guarantees
valueonly changes throughonChange, but worth documenting or adding a more robust identity mechanism if external mutations are expected.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/Effects/EffectsChainEditor.tsx` around lines 63 - 73, IDs drift when parent reorders/inserts effects because idsRef.current is aligned by index; update EffectsChainEditor so IDs follow effect identity not position: in the useMemo that builds items use a Map/WeakMap keyed by each effect object (or a stable property like an existing id field) to reuse existing IDs (lookup existing id via map, otherwise create with makeId and store it), and remove entries for effects no longer present; reference: useMemo block, idsRef, makeId, value, and the returned items mapping.app/src/lib/api/types.ts (1)
87-91: Redundant field declarations in HistoryResponse.
HistoryResponseextendsGenerationResponsewhich already declaresversions?: GenerationVersionResponse[]andactive_version_id?: stringat lines 76-77. The re-declaration at lines 89-90 is redundant.🧹 Remove redundant fields
export interface HistoryResponse extends GenerationResponse { profile_name: string; - versions?: GenerationVersionResponse[]; - active_version_id?: string; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/lib/api/types.ts` around lines 87 - 91, HistoryResponse currently re-declares fields that are already present on GenerationResponse (specifically versions and active_version_id); remove the redundant declarations of versions?: GenerationVersionResponse[] and active_version_id?: string from the HistoryResponse interface so it simply extends GenerationResponse and only adds profile_name (leave HistoryResponse, GenerationResponse, versions, and active_version_id identifiers intact).app/src/components/Effects/GenerationPicker.tsx (1)
20-25: Hard-coded limit of 50 may exclude recent generations.The picker fetches only the first 50 history items. Users with extensive generation history may not see all their completed generations. Consider either increasing the limit or adding pagination/load-more functionality.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/Effects/GenerationPicker.tsx` around lines 20 - 25, The history fetch uses a hard-coded limit of 50 which can omit recent completed generations; update the useHistory call (where useHistory is invoked) and the completedGenerations logic to support more results by either increasing/removing the static limit or implementing paging/load-more: modify the useHistory invocation to accept a configurable limit or cursor-based pagination, add state for page/cursor and a "load more" handler in the GenerationPicker component, and ensure completedGenerations (the useMemo that filters historyData.items) concatenates new pages rather than replacing them so all completed generations are visible.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/components/EffectsTab/EffectsDetail.tsx`:
- Around line 183-223: The UI shows editable fields for existing custom presets
but lacks a Save action; add a Save button in the same control group that
appears when !isBuiltIn && !isCreatingNew and wire it to call the existing
updateEffectPreset(...) function (or create a new handler like
handleUpdatePreset that calls updateEffectPreset), passing the current preset id
and workingChain; ensure the button mirrors disabled logic used elsewhere
(disabled when saving || workingChain.length === 0), shows saving state (e.g.,
'Saving...' when saving), and avoids affecting built-ins (keep current Delete
button for non-built-in). This provides a persistent update path for edits to
custom presets.
In `@app/src/components/Generation/FloatingGenerateBox.tsx`:
- Around line 395-398: The click handler uses the pre-toggle value of
isEffectsMode causing the inverted behavior; compute the new state first and
base the side-effect on that (e.g., derive newEffects = !isEffectsMode, call
setIsEffectsMode(newEffects), and if newEffects then call
setIsInstructMode(false)), or use the functional setter form
setIsEffectsMode(prev => { const next = !prev; if (next)
setIsInstructMode(false); return next; }) to ensure instruct mode is disabled
only when effects mode is being enabled; update the onClick in
FloatingGenerateBox to reference isEffectsMode, setIsEffectsMode, and
setIsInstructMode accordingly.
In `@app/src/components/History/HistoryTable.tsx`:
- Around line 348-381: The interactive container uses conditional role/tabIndex
but always wires onMouseDown/onKeyDown, so when isPlayable is false it still has
interactive handlers without proper semantics; change the markup to render a
semantic <button> (or a focusable element with role="button" permanently) only
when isPlayable is true, and render a non-interactive <div> otherwise, or
alternately keep a single element but always set role="button" and tabIndex and
only attach handlePlay in onMouseDown/onKeyDown when isPlayable; update the code
around isPlayable, onMouseDown/onKeyDown, and handlePlay usage and ensure
aria-label remains accurate for both states.
In `@app/src/components/VoiceProfiles/ProfileForm.tsx`:
- Around line 445-460: The effects save catch in ProfileForm.tsx currently only
shows a toast then allows execution to continue into the success/close path;
modify the handler around updateProfileEffects (the block using
apiClient.updateProfileEffects and the effectsDirty flag) to prevent falling
through on error by either re-throwing fxError or returning early from the
enclosing save/submit function so the modal remains open and the effects chain
is not lost; ensure any success toast/close logic that follows (the code that
closes the dialog and shows the "voice updated" toast) only runs when
updateProfileEffects completes without throwing.
In `@app/src/stores/effectsStore.ts`:
- Around line 24-25: The setter setIsCreatingNew currently assigns
selectedPresetId using the no-op ternary v ? null : null; update
setIsCreatingNew so it clears selectedPresetId only when entering create mode (v
=== true) and preserves the existing selectedPresetId when v is false by using
the state updater form (referencing setIsCreatingNew, isCreatingNew,
selectedPresetId and set) so the new state is set({ isCreatingNew: v,
selectedPresetId: v ? null : state.selectedPresetId }).
In `@backend/effects.py`:
- Around line 52-71: The create_preset function can raise an unhandled
IntegrityError when inserting a DBEffectPreset with a duplicate name; update
create_preset to check for existing presets by name before adding (e.g., query
DBEffectPreset where name == data.name) and raise a ValueError or a custom
exception when a duplicate exists, or alternatively catch
sqlalchemy.exc.IntegrityError around db.commit() in create_preset (or ensure
create_effect_preset handles IntegrityError) and translate it into a clear error
for the endpoint; reference DBEffectPreset, create_preset, db.commit, and the
create_effect_preset endpoint when implementing the fix.
In `@backend/main.py`:
- Around line 1588-1591: The current lookup uses the mutable display label
(v.label == "clean") to choose the source version; change that to pick the
immutable original/unprocessed version instead by using a stable field such as
v.effects_chain is None or a dedicated v.kind == "original" (where available)
when calling versions_mod.list_versions(generation_id, db) and computing
clean_version; update the same logic locations that reference v.label == "clean"
(e.g., the clean_version selection and any subsequent source-path resolution
using clean_version.audio_path or gen.audio_path) to use the immutable property
(effects_chain or kind) so processed versions named "clean" cannot be mistakenly
treated as the unprocessed baseline.
- Around line 828-857: The code currently calls versions_mod.create_version(...)
for the "clean" version before post-processing; if apply_effects, save_audio, or
the second create_version fails you leave inconsistent rows/files. Change the
flow so you do not call versions_mod.create_version (the create_version function
in backend/versions.py) until after validate_effects_chain, apply_effects,
save_audio and any processed-version creation have completed successfully (i.e.,
once final_audio_path and all files exist), or alternatively wrap the entire
post-processing block in a try/except and roll back any created versions/files
on error by deleting the created audio files and removing the earlier
create_version entry via the versions_mod API; ensure all references to
apply_effects, validate_effects_chain, save_audio and
versions_mod.create_version are updated accordingly.
- Around line 1595-1602: Synchronous audio I/O and CPU-bound processing (calls
to load_audio, apply_effects, and sf.write) are being executed directly inside
async endpoints (preview_effects, apply_effects_to_generation, transcribe_audio)
which will block the event loop; wrap those blocking calls in
asyncio.to_thread() (or run them in a dedicated threadpool) so the
decode/process/encode/write work runs off the event loop and the async handlers
remain non-blocking, replacing direct calls to load_audio, apply_effects, and
sf.write with await asyncio.to_thread(...) invocations for each blocking helper.
In `@backend/profiles.py`:
- Around line 32-50: In _profile_to_response, avoid silently swallowing errors
when deserializing profile.effects_chain: catch the exception around
_json.loads(...) / EffectConfig(...) in the try block and log a warning
including the exception details and identifying info (e.g., profile.id or
profile.name) so malformed JSON or validation errors are visible; then continue
returning VoiceProfileResponse with effects_chain set to None (as current) to
preserve behavior while surfacing the root cause for debugging.
In `@backend/utils/effects.py`:
- Around line 25-35: The hard import of pedalboard in effects.py causes startup
crashes when pedalboard is missing; modify the module so pedalboard is imported
lazily inside functions that actually need it (e.g., build_pedalboard()) and add
a guarded fallback: try importing pedalboard inside build_pedalboard() (or an
internal resolver), on ImportError log a warning and return a no-op/disabled
effects state so the app can start; change EFFECT_REGISTRY to store effect class
names/identifiers instead of direct class objects and resolve those names to
actual pedalboard classes at runtime in build_pedalboard() (used by
_seed_builtin_presets()/BUILTIN_PRESETS) so database initialization no longer
triggers a hard import.
---
Outside diff comments:
In `@app/src/lib/hooks/useGenerationForm.ts`:
- Around line 108-121: The code currently converts an empty effectsChain to
undefined when calling generation.mutateAsync, which causes the backend to apply
the profile default; instead, preserve an explicit empty array as a
per-generation override. Change the value passed for effects_chain in the
generation.mutateAsync call (where effectsChain is sourced from
options.getEffectsChain()) so that if options.getEffectsChain() returns
undefined you pass undefined, but if it returns an array (including []), you
pass that array unchanged (e.g., use a check like Array.isArray(effectsChain) ?
effectsChain : undefined or effectsChain === undefined ? undefined :
effectsChain) so empty chains are sent as [] and undefined remains the “no
override provided” sentinel.
In `@backend/history.py`:
- Around line 285-300: The batch deletion in delete_generations_by_profile
currently removes DBGeneration rows and their audio files but leaves
GenerationVersion records and their audio files orphaned; update
delete_generations_by_profile to mirror the cleanup in delete_generation by
iterating each GenerationVersion for every generation (use the GenerationVersion
model / generation.versions relationship or a query for GenerationVersion where
generation_id == generation.id), delete any version.audio_path files on disk,
delete the GenerationVersion rows from the session, then proceed to delete the
DBGeneration and its audio file before committing and returning the count.
---
Nitpick comments:
In `@app/src/components/Effects/EffectsChainEditor.tsx`:
- Around line 251-272: The SortableEffectItem component declares and receives an
unused index prop causing dead code: remove index from the
SortableEffectItemProps interface and from the function parameter destructuring
in SortableEffectItem, and then update its call site(s) (where
SortableEffectItem is instantiated) to stop passing the index prop; keep all
other props and handlers (effect, effectDef, isExpanded, onToggleExpand,
onRemove, onToggleEnabled, onUpdateParam) unchanged so behavior remains
identical.
- Around line 40-43: The module-level mutable counter nextId used by makeId()
can collide across component instances and HMR; replace it with a non-global
unique-ID approach (for example use crypto.randomUUID() or React's useId) inside
the EffectsChainEditor instance so IDs are generated per instance rather than
from a shared nextId; update all usages that call makeId() to call the new
instance-scoped ID generator (or inline crypto.randomUUID()) and remove the
module-level nextId to prevent cross-instance/HMR collisions.
- Around line 63-73: IDs drift when parent reorders/inserts effects because
idsRef.current is aligned by index; update EffectsChainEditor so IDs follow
effect identity not position: in the useMemo that builds items use a Map/WeakMap
keyed by each effect object (or a stable property like an existing id field) to
reuse existing IDs (lookup existing id via map, otherwise create with makeId and
store it), and remove entries for effects no longer present; reference: useMemo
block, idsRef, makeId, value, and the returned items mapping.
In `@app/src/components/Effects/GenerationPicker.tsx`:
- Around line 20-25: The history fetch uses a hard-coded limit of 50 which can
omit recent completed generations; update the useHistory call (where useHistory
is invoked) and the completedGenerations logic to support more results by either
increasing/removing the static limit or implementing paging/load-more: modify
the useHistory invocation to accept a configurable limit or cursor-based
pagination, add state for page/cursor and a "load more" handler in the
GenerationPicker component, and ensure completedGenerations (the useMemo that
filters historyData.items) concatenates new pages rather than replacing them so
all completed generations are visible.
In `@app/src/lib/api/types.ts`:
- Around line 87-91: HistoryResponse currently re-declares fields that are
already present on GenerationResponse (specifically versions and
active_version_id); remove the redundant declarations of versions?:
GenerationVersionResponse[] and active_version_id?: string from the
HistoryResponse interface so it simply extends GenerationResponse and only adds
profile_name (leave HistoryResponse, GenerationResponse, versions, and
active_version_id identifiers intact).
In `@backend/database.py`:
- Around line 422-446: In _seed_builtin_presets(), the loop unpacks (key,
preset_data) from BUILTIN_PRESETS but never uses key; update the loop to use a
clearly intended unused variable name (e.g., rename key to _key) in the for
statement (for idx, (_key, preset_data) in enumerate(BUILTIN_PRESETS.items()):)
so static analysis no longer flags an unused variable while leaving all logic in
_seed_builtin_presets, BUILTIN_PRESETS handling, and subsequent database
operations unchanged.
In `@backend/effects.py`:
- Around line 17-27: _wrap the JSON parse and Pydantic construction in
_preset_response so corrupt DB data is handled like _profile_to_response: try to
json.loads(p.effects_chain) and build EffectConfig(**e) items, and catch
json.JSONDecodeError, TypeError and pydantic.ValidationError; on error log a
warning that includes p.id and the raw p.effects_chain, set effects_chain to an
empty list (or a safe default) and continue returning EffectPresetResponse.
Ensure you reference EffectConfig and EffectPresetResponse in the fix so the
fallback shape matches the response schema.
In `@backend/history.py`:
- Around line 23-57: The helper _get_versions_for_generation is silently
swallowing JSON parse errors when building effects_chain and is duplicating the
formatting logic present in versions._version_response; fix it by importing and
calling versions._version_response (or the specific function that builds a
GenerationVersionResponse) to construct each response instead of manually
creating GenerationVersionResponse objects, and replace the bare except with a
narrow except catching json.JSONDecodeError (or Exception if necessary) while
logging the error via the module logger (or processLogger) including context
(generation_id and DBGenerationVersion.id) so failures aren’t silent; ensure you
still set active_version_id when v.is_default.
In `@backend/models.py`:
- Around line 361-363: The EffectsChain wrapper class (EffectsChain : BaseModel
with effects: List[EffectConfig]) appears unused by the API which accepts
List[EffectConfig] directly; either remove the EffectsChain model to avoid dead
code or update usages and docs to justify it. Search for the EffectsChain symbol
and any references to effects: List[EffectConfig] (and the EffectConfig model)
and either (A) delete the EffectsChain class and replace any remaining
references with plain List[EffectConfig], or (B) keep EffectsChain and change
endpoint signatures, pydantic validators, and docstrings to accept/return
EffectsChain and add a short docstring explaining its purpose. Ensure tests and
imports are updated accordingly.
- Around line 428-433: The params field on the AvailableEffect model is
currently untyped dict; update it to use a typed mapping for better docs and
type safety by changing params to Dict[str, AvailableEffectParam] on the
AvailableEffect class and ensure typing.Dict (or from typing import Dict) is
imported and that AvailableEffectParam is referenced/imported where needed.
In `@backend/versions.py`:
- Around line 139-181: The delete_version function currently calls db.commit()
multiple times; wrap the DB mutations in a single transaction so all changes are
atomic: determine was_default and gen_id as before, delete the
DBGenerationVersion object (db.delete(version)), if was_default find the first
remaining DBGenerationVersion (query on DBGenerationVersion with generation_id)
and set first.is_default = True and update the related DBGeneration.audio_path
to first.audio_path, then perform one db.commit() at the end; ensure the file
deletion (Path(version.audio_path).unlink()) is performed either before the DB
commit or handled so failures don't leave the DB partially updated, and keep
using the same Session methods (delete, queries) on DBGenerationVersion and
DBGeneration to locate and update records.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 251cf9b1-349c-4126-b1ea-790aee6cea09
📒 Files selected for processing (24)
app/src/components/Effects/EffectsChainEditor.tsxapp/src/components/Effects/GenerationPicker.tsxapp/src/components/EffectsTab/EffectsDetail.tsxapp/src/components/EffectsTab/EffectsList.tsxapp/src/components/EffectsTab/EffectsTab.tsxapp/src/components/Generation/FloatingGenerateBox.tsxapp/src/components/History/HistoryTable.tsxapp/src/components/Sidebar.tsxapp/src/components/VoiceProfiles/ProfileForm.tsxapp/src/components/ui/select.tsxapp/src/lib/api/client.tsapp/src/lib/api/types.tsapp/src/lib/hooks/useGenerationForm.tsapp/src/router.tsxapp/src/stores/effectsStore.tsbackend/database.pybackend/effects.pybackend/history.pybackend/main.pybackend/models.pybackend/profiles.pybackend/requirements.txtbackend/utils/effects.pybackend/versions.py
Comment on lines
+828
to
+857
| # Create clean version entry | ||
| versions_mod.create_version( | ||
| generation_id=generation_id, | ||
| label="clean", | ||
| audio_path=str(clean_audio_path), | ||
| db=bg_db, | ||
| effects_chain=None, | ||
| is_default=not has_effects, | ||
| ) | ||
|
|
||
| # Apply effects and create processed version if configured | ||
| final_audio_path = str(clean_audio_path) | ||
| if has_effects: | ||
| from .utils.effects import apply_effects, validate_effects_chain | ||
| error_msg = validate_effects_chain(effects_chain_config) | ||
| if error_msg: | ||
| print(f"Warning: invalid effects chain, skipping: {error_msg}") | ||
| else: | ||
| processed_audio = apply_effects(audio, sample_rate, effects_chain_config) | ||
| processed_path = config.get_generations_dir() / f"{generation_id}_processed.wav" | ||
| save_audio(processed_audio, str(processed_path), sample_rate) | ||
| final_audio_path = str(processed_path) | ||
| versions_mod.create_version( | ||
| generation_id=generation_id, | ||
| label="processed", | ||
| audio_path=str(processed_path), | ||
| db=bg_db, | ||
| effects_chain=effects_chain_config, | ||
| is_default=True, | ||
| ) |
Contributor
There was a problem hiding this comment.
Defer version persistence until the whole post-processing run succeeds.
backend/versions.py:create_version() commits immediately. Line 829 writes the clean version before the rest of the pipeline is known to be successful, so any later exception from processing, saving, or the second version insert leaves version rows/files behind while the generation is marked failed. Create the version records only after all output files are ready, or explicitly clean them up in the failure path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/main.py` around lines 828 - 857, The code currently calls
versions_mod.create_version(...) for the "clean" version before post-processing;
if apply_effects, save_audio, or the second create_version fails you leave
inconsistent rows/files. Change the flow so you do not call
versions_mod.create_version (the create_version function in backend/versions.py)
until after validate_effects_chain, apply_effects, save_audio and any
processed-version creation have completed successfully (i.e., once
final_audio_path and all files exist), or alternatively wrap the entire
post-processing block in a try/except and roll back any created versions/files
on error by deleting the created audio files and removing the earlier
create_version entry via the versions_mod API; ensure all references to
apply_effects, validate_effects_chain, save_audio and
versions_mod.create_version are updated accordingly.
…nc audio ops - Fix inverted effects toggle in FloatingGenerateBox - Add Save button + API method for editing custom effect presets - Return early on effects save failure in ProfileForm - Fix no-op ternary in effectsStore - Handle duplicate preset names with proper 400 response - Use effects_chain is None instead of label for clean version lookup - Move blocking audio ops to asyncio.to_thread in async endpoints - Log warnings instead of silently swallowing parse errors
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/components/Generation/FloatingGenerateBox.tsx (1)
56-65:⚠️ Potential issue | 🟠 MajorEffects toggle does not control whether effects are applied.
On Line 64, payload inclusion ignores
isEffectsMode, so a non-empty chain is still submitted after toggling effects off.🛠️ Suggested fix
const { form, handleSubmit, isPending } = useGenerationForm({ @@ - getEffectsChain: () => (effectsChain.length > 0 ? effectsChain : undefined), + getEffectsChain: () => + isEffectsMode && effectsChain.length > 0 ? effectsChain : undefined, });Also applies to: 395-399
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/Generation/FloatingGenerateBox.tsx` around lines 56 - 65, The getEffectsChain supplied to useGenerationForm currently returns effectsChain regardless of isEffectsMode, so effects still get applied when toggled off; update the getEffectsChain callback (used in useGenerationForm and the other occurrence around the block handling payload creation at lines ~395-399) to return effectsChain only when isEffectsMode is true and effectsChain.length > 0 (otherwise return undefined) so toggling off prevents submitting the chain; reference the symbols isEffectsMode, getEffectsChain, effectsChain, and useGenerationForm to locate and change both places.
♻️ Duplicate comments (1)
backend/main.py (1)
828-857:⚠️ Potential issue | 🟠 MajorVersion persistence can still be left in a partial state on post-processing failure.
Line 829 persists the clean version before the full effects pipeline is known to succeed. If later processing/save/version creation fails, rows/files can remain while the generation is marked failed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/main.py` around lines 828 - 857, The clean version is being persisted via versions_mod.create_version before the effects pipeline runs, so if apply_effects/save_audio or the processed versions_mod.create_version fails the DB/files are left in a partial state; fix by either (A) postponing the initial versions_mod.create_version(...) call for the "clean" label until after validate_effects_chain(...) and apply_effects(...) complete successfully (so both "clean" and "processed" are created together), or (B) wrap the effects processing and subsequent versions_mod.create_version/save_audio calls in a try/except and on any exception delete the previously created clean version and clean file (use versions_mod.delete_version or equivalent and remove the saved audio path) so the generation is not left partially persisted; update code around versions_mod.create_version, validate_effects_chain, apply_effects, save_audio to implement one of these approaches.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/effects.py`:
- Around line 92-107: In update_preset(), handle unique-name collisions the same
way create_preset() does: wrap the db.commit()/db.refresh(preset) block in a
try/except that catches sqlalchemy.exc.IntegrityError, calls db.rollback(), and
raises a ValueError with a clear "duplicate preset name" message so the endpoint
maps it to a 400; import IntegrityError where needed and keep raising ValueError
(not raw IntegrityError) to preserve existing error mapping for
_preset_response/update path.
In `@backend/main.py`:
- Around line 769-773: The try/except around
_json.loads(profile_obj.effects_chain) currently swallows all errors; change the
except to capture the exception (e.g., except Exception as e) and log a clear
error including the exception/traceback and profile identity (use profile_obj.id
or profile_obj.name) before applying a safe fallback for effects_chain_config
(e.g., None or an empty dict) so malformed stored defaults are visible in logs
and the application remains stable; update the block around
effects_chain_config/_json.loads(profile_obj.effects_chain) accordingly.
- Around line 1600-1602: The sf.write call is passing "WAV" positionally (going
to subtype) which is wrong for a BytesIO target; update the call in
backend/main.py (the call that uses sf.write with buf, processed, sample_rate)
to pass keyword args and set format explicitly (e.g., await
asyncio.to_thread(sf.write, buf, processed, sample_rate, format="WAV") or use
parameter names like sf.write(file=buf, data=processed, samplerate=sample_rate,
format="WAV")); if you also need a subtype, provide it via the subtype=...
keyword rather than a positional argument.
---
Outside diff comments:
In `@app/src/components/Generation/FloatingGenerateBox.tsx`:
- Around line 56-65: The getEffectsChain supplied to useGenerationForm currently
returns effectsChain regardless of isEffectsMode, so effects still get applied
when toggled off; update the getEffectsChain callback (used in useGenerationForm
and the other occurrence around the block handling payload creation at lines
~395-399) to return effectsChain only when isEffectsMode is true and
effectsChain.length > 0 (otherwise return undefined) so toggling off prevents
submitting the chain; reference the symbols isEffectsMode, getEffectsChain,
effectsChain, and useGenerationForm to locate and change both places.
---
Duplicate comments:
In `@backend/main.py`:
- Around line 828-857: The clean version is being persisted via
versions_mod.create_version before the effects pipeline runs, so if
apply_effects/save_audio or the processed versions_mod.create_version fails the
DB/files are left in a partial state; fix by either (A) postponing the initial
versions_mod.create_version(...) call for the "clean" label until after
validate_effects_chain(...) and apply_effects(...) complete successfully (so
both "clean" and "processed" are created together), or (B) wrap the effects
processing and subsequent versions_mod.create_version/save_audio calls in a
try/except and on any exception delete the previously created clean version and
clean file (use versions_mod.delete_version or equivalent and remove the saved
audio path) so the generation is not left partially persisted; update code
around versions_mod.create_version, validate_effects_chain, apply_effects,
save_audio to implement one of these approaches.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 66695b2b-a93c-4907-a1e2-45064cec3a30
📒 Files selected for processing (9)
app/src/components/EffectsTab/EffectsDetail.tsxapp/src/components/Generation/FloatingGenerateBox.tsxapp/src/components/VoiceProfiles/ProfileForm.tsxapp/src/lib/api/client.tsapp/src/lib/api/types.tsapp/src/stores/effectsStore.tsbackend/effects.pybackend/main.pybackend/profiles.py
🚧 Files skipped from review as they are similar to previous changes (3)
- app/src/components/EffectsTab/EffectsDetail.tsx
- app/src/stores/effectsStore.ts
- app/src/lib/api/client.ts
Comment on lines
+92
to
+107
| if data.name is not None: | ||
| preset.name = data.name | ||
| if data.description is not None: | ||
| preset.description = data.description | ||
| if data.effects_chain is not None: | ||
| from .utils.effects import validate_effects_chain | ||
|
|
||
| chain_dicts = [e.model_dump() for e in data.effects_chain] | ||
| error = validate_effects_chain(chain_dicts) | ||
| if error: | ||
| raise ValueError(error) | ||
| preset.effects_chain = json.dumps(chain_dicts) | ||
|
|
||
| db.commit() | ||
| db.refresh(preset) | ||
| return _preset_response(preset) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify update path lacks duplicate handling and endpoint only catches ValueError
rg -n -C4 "def update_preset|preset.name = data.name|db.commit|except IntegrityError" backend/effects.py
rg -n -C6 "async def update_effect_preset|except ValueError" backend/main.py
rg -n -C8 "class EffectPreset|name = Column\\(String.*unique=True" backend/database.pyRepository: jamiepine/voicebox
Length of output: 11912
Duplicate-name rename path can return 500.
update_preset() commits name changes without handling unique-name collisions, while the endpoint only maps ValueError to 400. Renaming a preset to an existing name will raise an unhandled IntegrityError, surfacing as a 500 error instead of 400.
The asymmetry with create_preset(), which guards against duplicates and converts IntegrityError to ValueError, creates a vulnerability in the update path.
🛠️ Suggested fix
def update_preset(preset_id: str, data: EffectPresetUpdate, db: Session) -> Optional[EffectPresetResponse]:
@@
- if data.name is not None:
+ if data.name is not None:
+ existing = db.query(DBEffectPreset).filter(
+ DBEffectPreset.name == data.name,
+ DBEffectPreset.id != preset_id,
+ ).first()
+ if existing:
+ raise ValueError(f"A preset named '{data.name}' already exists")
preset.name = data.name
@@
- db.commit()
+ try:
+ db.commit()
+ except IntegrityError as e:
+ db.rollback()
+ raise ValueError("Preset name already exists") from e
db.refresh(preset)
return _preset_response(preset)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/effects.py` around lines 92 - 107, In update_preset(), handle
unique-name collisions the same way create_preset() does: wrap the
db.commit()/db.refresh(preset) block in a try/except that catches
sqlalchemy.exc.IntegrityError, calls db.rollback(), and raises a ValueError with
a clear "duplicate preset name" message so the endpoint maps it to a 400; import
IntegrityError where needed and keep raising ValueError (not raw IntegrityError)
to preserve existing error mapping for _preset_response/update path.
Comment on lines
+769
to
+773
| try: | ||
| effects_chain_config = _json.loads(profile_obj.effects_chain) | ||
| except Exception: | ||
| pass | ||
|
|
Contributor
There was a problem hiding this comment.
Don’t silently swallow profile default effects parse failures.
Lines 771-772 currently suppress deserialization errors; this hides malformed stored defaults and makes operational debugging harder.
🛠️ Suggested fix
- if profile_obj and profile_obj.effects_chain:
- try:
- effects_chain_config = _json.loads(profile_obj.effects_chain)
- except Exception:
- pass
+ if profile_obj and profile_obj.effects_chain:
+ try:
+ effects_chain_config = _json.loads(profile_obj.effects_chain)
+ except Exception as e:
+ import logging
+ logging.warning(
+ "Failed to parse profile effects_chain for profile %s: %s",
+ data.profile_id,
+ e,
+ )🧰 Tools
🪛 Ruff (0.15.5)
[error] 771-772: try-except-pass detected, consider logging the exception
(S110)
[warning] 771-771: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/main.py` around lines 769 - 773, The try/except around
_json.loads(profile_obj.effects_chain) currently swallows all errors; change the
except to capture the exception (e.g., except Exception as e) and log a clear
error including the exception/traceback and profile identity (use profile_obj.id
or profile_obj.name) before applying a safe fallback for effects_chain_config
(e.g., None or an empty dict) so malformed stored defaults are visible in logs
and the application remains stable; update the block around
effects_chain_config/_json.loads(profile_obj.effects_chain) accordingly.
Reload the player with the version-specific audio URL when effects are applied to the currently playing generation. Also consolidate the instruct/effects buttons into a single button with the effects editor shown inline when instruct mode is open.
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/Generation/FloatingGenerateBox.tsx (2)
366-374:⚠️ Potential issue | 🟡 MinorARIA label no longer matches the control’s combined behavior.
The tooltip now says “instructions & effects” (Line 373), but the button
aria-labelstill says instructions only. This leaves screen-reader users without the updated intent.Suggested fix
- aria-label={ - isInstructMode ? 'Fine tune instructions, on' : 'Fine tune instructions' - } + aria-label={ + isInstructMode + ? 'Fine tune instructions and effects, on' + : 'Fine tune instructions and effects' + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/Generation/FloatingGenerateBox.tsx` around lines 366 - 374, Update the Button's aria-label to reflect the control's combined purpose (instructions and effects) so screen readers match the visible tooltip; modify the aria-label expression on the Button (where isInstructMode is used) to include both "instructions & effects" in both states (e.g., toggling between "Fine tune instructions & effects, on" and "Fine tune instructions & effects") so the Button component and the tooltip span remain consistent.
344-365:⚠️ Potential issue | 🟠 MajorEffects can remain active but become hidden/non-editable after engine switch.
At Line 344, the toggle/editor UI is only rendered for
qwen, but Line 63 still submitseffectsChainwhenever it is non-empty. If a user configures effects on Qwen and then switches engines, effects may still apply with no visible control to edit/disable.Suggested minimal fix
- {isExpanded && form.watch('engine') === 'qwen' && ( + {isExpanded && + (form.watch('engine') === 'qwen' || effectsChain.length > 0) && (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/Generation/FloatingGenerateBox.tsx` around lines 344 - 365, The effectsChain can remain active but become uneditable when the user switches off 'qwen'; fix by clearing or disabling effects when the engine changes: add logic that watches form.watch('engine') and if it becomes !== 'qwen' and effectsChain is non-empty, call setEffectsChain([]) (or otherwise prevent effects from being submitted) so effects are not applied invisibly; alternatively, render the effects toggle/UI for non-'qwen' engines by moving the Button block out of the form.watch('engine') === 'qwen' condition so the user can edit/disable effects — reference effectsChain, setEffectsChain, form.watch('engine'), isExpanded and the Button/isInstructMode toggle.
🧹 Nitpick comments (2)
app/src/components/History/HistoryTable.tsx (2)
543-548: UX: Clicking a version both plays and switches default simultaneously.When the user clicks a non-default version, it plays the audio and sets it as default in a single action. Consider whether users might want to preview a version without committing to it as the new default—if so, a separate "Set as default" action might be clearer.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/History/HistoryTable.tsx` around lines 543 - 548, The current onClick handler both calls handlePlayVersion(...) and, for non-default versions, handleSwitchVersion(...), causing a click to both preview and commit; change the onClick for the version row to only call handlePlayVersion(gen.id, v.id, gen.text, gen.profile_id) and remove the conditional handleSwitchVersion call, and instead add a separate UI control (e.g., a "Set as default" button or icon rendered when v.is_default is false) that calls handleSwitchVersion(gen.id, v.id) when explicitly activated; update any accessible labels and tests that reference handlePlayVersion and handleSwitchVersion to reflect the split actions.
445-449: Addrole="presentation"to clarify non-interactive container intent.Static analysis flags this div for having
onClickwithout keyboard handlers. The div exists solely to stop event propagation—child buttons handle actual interactions and are keyboard-accessible. Addingrole="presentation"clarifies this is a layout container.♻️ Suggested fix
{/* Far right - Actions */} <div + role="presentation" className="shrink-0 flex flex-col justify-center items-center gap-1" onMouseDown={(e) => e.stopPropagation()} onClick={(e) => e.stopPropagation()} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/History/HistoryTable.tsx` around lines 445 - 449, The div in HistoryTable.tsx with className "shrink-0 flex flex-col justify-center items-center gap-1" that uses onMouseDown and onClick solely to stop propagation should be marked as non-interactive by adding role="presentation" to the element; update the JSX for that container (the element using onMouseDown={(e) => e.stopPropagation()} and onClick={(e) => e.stopPropagation()}) to include role="presentation" so static analysis knows it's a layout-only container and not missing keyboard handlers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@app/src/components/Generation/FloatingGenerateBox.tsx`:
- Around line 366-374: Update the Button's aria-label to reflect the control's
combined purpose (instructions and effects) so screen readers match the visible
tooltip; modify the aria-label expression on the Button (where isInstructMode is
used) to include both "instructions & effects" in both states (e.g., toggling
between "Fine tune instructions & effects, on" and "Fine tune instructions &
effects") so the Button component and the tooltip span remain consistent.
- Around line 344-365: The effectsChain can remain active but become uneditable
when the user switches off 'qwen'; fix by clearing or disabling effects when the
engine changes: add logic that watches form.watch('engine') and if it becomes
!== 'qwen' and effectsChain is non-empty, call setEffectsChain([]) (or otherwise
prevent effects from being submitted) so effects are not applied invisibly;
alternatively, render the effects toggle/UI for non-'qwen' engines by moving the
Button block out of the form.watch('engine') === 'qwen' condition so the user
can edit/disable effects — reference effectsChain, setEffectsChain,
form.watch('engine'), isExpanded and the Button/isInstructMode toggle.
---
Nitpick comments:
In `@app/src/components/History/HistoryTable.tsx`:
- Around line 543-548: The current onClick handler both calls
handlePlayVersion(...) and, for non-default versions, handleSwitchVersion(...),
causing a click to both preview and commit; change the onClick for the version
row to only call handlePlayVersion(gen.id, v.id, gen.text, gen.profile_id) and
remove the conditional handleSwitchVersion call, and instead add a separate UI
control (e.g., a "Set as default" button or icon rendered when v.is_default is
false) that calls handleSwitchVersion(gen.id, v.id) when explicitly activated;
update any accessible labels and tests that reference handlePlayVersion and
handleSwitchVersion to reflect the split actions.
- Around line 445-449: The div in HistoryTable.tsx with className "shrink-0 flex
flex-col justify-center items-center gap-1" that uses onMouseDown and onClick
solely to stop propagation should be marked as non-interactive by adding
role="presentation" to the element; update the JSX for that container (the
element using onMouseDown={(e) => e.stopPropagation()} and onClick={(e) =>
e.stopPropagation()}) to include role="presentation" so static analysis knows
it's a layout-only container and not missing keyboard handlers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2db99732-33f6-4c02-8a49-b01ab99571c5
📒 Files selected for processing (2)
app/src/components/Generation/FloatingGenerateBox.tsxapp/src/components/History/HistoryTable.tsx
- Add /generate/{id}/regenerate endpoint that creates a new take version
- Wire regenerate into history dropdown with SSE progress + autoplay
- Add normalize_audio to regenerate path
- Remove duplicate Regenerate menu item
- Show disabled ellipsis menu during generation instead of hiding it
- Fix sf.write format kwarg broken by asyncio.to_thread migration
- Rename clean version label to 'original', effects to 'version-N'
- Show effects chain names in version list instead of 'N fx'
- Include all versions in export package
- Clean up version panel padding
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (3)
backend/main.py (3)
824-857:⚠️ Potential issue | 🟠 MajorPersist version rows only after effects processing outcome is known.
Line 829 commits the clean version before validation/processing. If downstream work fails (or validation fails), you can end with inconsistent version metadata (including no default version when
has_effects=Truebut processing is skipped) and orphan artifacts.🔧 Suggested restructuring
- has_effects = effects_chain_config and any( + has_effects = bool(effects_chain_config and any( e.get("enabled", True) for e in effects_chain_config - ) + )) - # Create clean version entry - versions_mod.create_version( - generation_id=generation_id, - label="original", - audio_path=str(clean_audio_path), - db=bg_db, - effects_chain=None, - is_default=not has_effects, - ) - - # Apply effects and create processed version if configured final_audio_path = str(clean_audio_path) + processed_path = None if has_effects: from .utils.effects import apply_effects, validate_effects_chain error_msg = validate_effects_chain(effects_chain_config) if error_msg: print(f"Warning: invalid effects chain, skipping: {error_msg}") else: processed_audio = apply_effects(audio, sample_rate, effects_chain_config) processed_path = config.get_generations_dir() / f"{generation_id}_processed.wav" save_audio(processed_audio, str(processed_path), sample_rate) final_audio_path = str(processed_path) + + # Persist versions after processing branch settles. + versions_mod.create_version( + generation_id=generation_id, + label="original", + audio_path=str(clean_audio_path), + db=bg_db, + effects_chain=None, + is_default=processed_path is None, + ) + if processed_path is not None: + versions_mod.create_version( + generation_id=generation_id, + label="version-2", + audio_path=str(processed_path), + db=bg_db, + effects_chain=effects_chain_config, + is_default=True, + ) - versions_mod.create_version( - generation_id=generation_id, - label="version-2", - audio_path=str(processed_path), - db=bg_db, - effects_chain=effects_chain_config, - is_default=True, - )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/main.py` around lines 824 - 857, The current flow calls versions_mod.create_version for the "original" before validating/applying effects, which can leave inconsistent metadata if validate_effects_chain or apply_effects fails; change the logic so effects are validated first (use validate_effects_chain) and only after a successful validation and processing (apply_effects, save_audio) create the appropriate version rows: if effects are invalid or skipped create a single "original" version with is_default=True, otherwise create the "original" version with is_default=False and then create the processed version (label "version-2") with is_default=True; ensure you also avoid writing the processed file unless apply_effects succeeds to prevent orphan artifacts and update final_audio_path accordingly.
769-772:⚠️ Potential issue | 🟡 MinorLog malformed profile default effects instead of silently ignoring parse failures.
Line 771 currently swallows bad stored JSON and hides data corruption/ops signals.
🔧 Suggested fix
if profile_obj and profile_obj.effects_chain: try: effects_chain_config = _json.loads(profile_obj.effects_chain) - except Exception: - pass + except Exception as e: + import logging + logging.getLogger(__name__).warning( + "Failed to parse profile effects_chain for profile %s: %s", + profile_obj.id, + e, + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/main.py` around lines 769 - 772, Replace the silent except-pass around _json.loads(profile_obj.effects_chain) so malformed stored JSON is logged; catch the exception as e (e.g., except Exception as e:) and call the project logger (or logging.exception) to record the error, include identifying info such as profile_obj.id or profile_obj.name and the raw profile_obj.effects_chain value plus the exception details, then allow effects_chain_config to remain None or a safe default.
1857-1871:⚠️ Potential issue | 🟠 MajorRemove processed audio file if version insert fails.
The file is written first (Line 1859), then DB insert is attempted (Line 1864). On DB failure, the endpoint leaks an orphaned file.
🔧 Suggested fix
version_id = str(uuid.uuid4()) processed_path = config.get_generations_dir() / f"{generation_id}_{version_id[:8]}.wav" await asyncio.to_thread(save_audio, processed_audio, str(processed_path), sample_rate) @@ - version = versions_mod.create_version( - generation_id=generation_id, - label=label, - audio_path=str(processed_path), - db=db, - effects_chain=chain_dicts, - is_default=data.set_as_default, - ) + try: + version = versions_mod.create_version( + generation_id=generation_id, + label=label, + audio_path=str(processed_path), + db=db, + effects_chain=chain_dicts, + is_default=data.set_as_default, + ) + except Exception: + Path(processed_path).unlink(missing_ok=True) + raise🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/main.py` around lines 1857 - 1871, The code saves the processed audio to processed_path before calling versions_mod.create_version, which can fail and leave an orphaned file; wrap the create_version call in a try/except (or try/finally) so that on any exception you remove the saved file (use Path.unlink or os.remove on processed_path) and then re-raise the exception so the error propagates; reference the processed_path variable, the save_audio call that created it, and the versions_mod.create_version call when implementing the cleanup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/components/AudioPlayer/AudioPlayer.tsx`:
- Around line 160-174: The timeupdate handler in wavesurfer.on('timeupdate') is
pausing playback when time >= duration which prevents the media 'ended' event
and thus WaveSurfer's 'finish' handler (and your onFinish callback) from
running; remove the terminal pause/loop logic from the timeupdate handler (the
block that calls wavesurfer.pause(), wavesurfer.seekTo(0)/play when isLooping,
setIsPlaying(false), and the early return) and instead let the media naturally
reach 'ended' so the existing wavesurfer.on('finish') handler can run and invoke
onFinish; keep only non-terminal updates (e.g., setCurrentTime) in the
timeupdate handler and rely on the isLooping/finish logic inside the finish
event handler to handle looping and finalization.
In `@app/src/components/History/HistoryTable.tsx`:
- Around line 562-567: The current onClick handler calls both
handlePlayVersion(gen.id, v.id, gen.text, gen.profile_id) and, if the version is
non-default, handleSwitchVersion(gen.id, v.id), causing an unexpected default
switch when users only intend to preview; change the click behavior so the
onClick only invokes handlePlayVersion with the existing arguments (gen.id,
v.id, gen.text, gen.profile_id) and remove the conditional handleSwitchVersion
call, then add a separate explicit control (e.g., a "Set as default" button or
menu item tied to handleSwitchVersion(gen.id, v.id)) adjacent to the version row
so switching defaults is an opt-in action.
In `@backend/export_import.py`:
- Around line 284-297: The export currently serializes multiple versions (see
version_entries in export_import.py) but import_generation_from_zip() only
creates a single DBGeneration and assigns audio_files[0], losing
DBGenerationVersion rows, is_default flags, effects_chain and original default
selection; update import_generation_from_zip() to recreate DBGenerationVersion
records for each exported version entry (use the exported "filename",
"is_default", "effects_chain", "label", and original version id if needed) and
attach each saved audio file to its own DBGenerationVersion record rather than
stuffing the first file into DBGeneration; ensure the code restores the
is_default flag by setting the DBGeneration.default_version (or equivalent) to
the exported default and preserve effects_chain by deserializing and saving it
on each DBGenerationVersion so re-export/import is round-trippable.
- Around line 286-297: The export currently adds every DB row into
manifest["versions"] even if the referenced audio file (v.audio_path) is missing
and the later zip write silently skips missing files; modify the loop that
builds version_entries (and the corresponding zip-writing block that iterates
versions) to verify existence of Path(v.audio_path) before appending to
version_entries or attempting to write it to the ZIP, and if a file is missing
raise/return a clear exception (fail the export) instead of continuing;
reference the loop over versions, v.audio_path, version_entries and the ZIP
write block that handles versions to locate where to add the Path(...).exists()
check and the early failure.
In `@backend/main.py`:
- Around line 1000-1003: The current code unconditionally sets gen.status =
"generating" and later may set gen.status = "failed" even if a prior completed
generation/audio exists; modify the regenerate flow in backend/main.py to first
check the existing generation state and assets (e.g., gen.status and whatever
field stores the completed audio/asset) and only set gen.status = "generating"
if there is no already-completed result to preserve, and when a regenerate
attempt fails avoid overwriting a prior completed state—only set gen.error (and
set status to "failed") when there is no previously completed asset to fall back
to; update the logic around the gen.status/gen.error mutations and db.commit
calls so they guard on gen.status == "completed" or presence of the completed
asset before downgrading.
---
Duplicate comments:
In `@backend/main.py`:
- Around line 824-857: The current flow calls versions_mod.create_version for
the "original" before validating/applying effects, which can leave inconsistent
metadata if validate_effects_chain or apply_effects fails; change the logic so
effects are validated first (use validate_effects_chain) and only after a
successful validation and processing (apply_effects, save_audio) create the
appropriate version rows: if effects are invalid or skipped create a single
"original" version with is_default=True, otherwise create the "original" version
with is_default=False and then create the processed version (label "version-2")
with is_default=True; ensure you also avoid writing the processed file unless
apply_effects succeeds to prevent orphan artifacts and update final_audio_path
accordingly.
- Around line 769-772: Replace the silent except-pass around
_json.loads(profile_obj.effects_chain) so malformed stored JSON is logged; catch
the exception as e (e.g., except Exception as e:) and call the project logger
(or logging.exception) to record the error, include identifying info such as
profile_obj.id or profile_obj.name and the raw profile_obj.effects_chain value
plus the exception details, then allow effects_chain_config to remain None or a
safe default.
- Around line 1857-1871: The code saves the processed audio to processed_path
before calling versions_mod.create_version, which can fail and leave an orphaned
file; wrap the create_version call in a try/except (or try/finally) so that on
any exception you remove the saved file (use Path.unlink or os.remove on
processed_path) and then re-raise the exception so the error propagates;
reference the processed_path variable, the save_audio call that created it, and
the versions_mod.create_version call when implementing the cleanup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 71b40d9a-d1ba-4f38-99fb-f1d98e77069d
📒 Files selected for processing (5)
app/src/components/AudioPlayer/AudioPlayer.tsxapp/src/components/History/HistoryTable.tsxapp/src/lib/api/client.tsbackend/export_import.pybackend/main.py
Comment on lines
+160
to
+174
| // Update store when time changes, stop if past duration | ||
| wavesurfer.on('timeupdate', (time) => { | ||
| const dur = usePlayerStore.getState().duration; | ||
| if (dur > 0 && time >= dur) { | ||
| setCurrentTime(dur); | ||
| const loop = usePlayerStore.getState().isLooping; | ||
| if (loop) { | ||
| wavesurfer.seekTo(0); | ||
| wavesurfer.play(); | ||
| } else { | ||
| wavesurfer.pause(); | ||
| setIsPlaying(false); | ||
| } | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In WaveSurfer.js, if pause() is called from a timeupdatecallback when currentTime >= duration, is thefinish event still guaranteed to fire? Please cite docs/issues for the current major version.
💡 Result:
No. In the current major version (WaveSurfer.js v7), the finish event is emitted only in the handler for the media element’s ended event—not when you call pause() yourself. So if you call pause() from a timeupdate callback as soon as currentTime >= duration, you can prevent the underlying ended event from occurring, and therefore finish is not guaranteed to fire. [2]
Docs describe finish as “When the audio finishes playing,” but do not state it will fire if you programmatically pause at (or near) the end. [1]
Sources: WaveSurfer events docs [1], WaveSurfer v7 source (ended ⇒ emit finish) [2].
🏁 Script executed:
# Inspect the AudioPlayer file to understand the full context
# First, check file size to ensure safe reading
wc -l app/src/components/AudioPlayer/AudioPlayer.tsxRepository: jamiepine/voicebox
Length of output: 112
🏁 Script executed:
# Read the timeupdate handler section (lines 160-174) with context
sed -n '155,180p' app/src/components/AudioPlayer/AudioPlayer.tsxRepository: jamiepine/voicebox
Length of output: 831
🏁 Script executed:
# Read the finish handler section (lines 410-440) with context
sed -n '410,440p' app/src/components/AudioPlayer/AudioPlayer.tsxRepository: jamiepine/voicebox
Length of output: 969
Move onFinish callback invocation to timeupdate handler or consolidate all terminal logic in the finish handler.
The timeupdate handler pauses at the end of track (lines 169), which prevents the media element's ended event from firing. Since WaveSurfer.js v7 emits the finish event only when ended fires, the finish handler (lines 416-430) is never reached, making the onFinish callback unreachable.
Either:
- Invoke
onFinishfrom thetimeupdatehandler alongside the pause, or - Remove pause/loop logic from
timeupdateand let the media naturally fireended→finish→onFinish.
Option 2 is cleaner and keeps terminal behavior in one place.
Proposed fix (Option 2)
- // Update store when time changes, stop if past duration
+ // Update store when time changes; clamp to duration.
+ // End-of-track loop/stop/callback behavior is handled in `finish`.
wavesurfer.on('timeupdate', (time) => {
const dur = usePlayerStore.getState().duration;
- if (dur > 0 && time >= dur) {
- setCurrentTime(dur);
- const loop = usePlayerStore.getState().isLooping;
- if (loop) {
- wavesurfer.seekTo(0);
- wavesurfer.play();
- } else {
- wavesurfer.pause();
- setIsPlaying(false);
- }
- return;
- }
- setCurrentTime(time);
+ setCurrentTime(dur > 0 ? Math.min(time, dur) : time);
});📝 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
| // Update store when time changes, stop if past duration | |
| wavesurfer.on('timeupdate', (time) => { | |
| const dur = usePlayerStore.getState().duration; | |
| if (dur > 0 && time >= dur) { | |
| setCurrentTime(dur); | |
| const loop = usePlayerStore.getState().isLooping; | |
| if (loop) { | |
| wavesurfer.seekTo(0); | |
| wavesurfer.play(); | |
| } else { | |
| wavesurfer.pause(); | |
| setIsPlaying(false); | |
| } | |
| return; | |
| } | |
| // Update store when time changes; clamp to duration. | |
| // End-of-track loop/stop/callback behavior is handled in `finish`. | |
| wavesurfer.on('timeupdate', (time) => { | |
| const dur = usePlayerStore.getState().duration; | |
| setCurrentTime(dur > 0 ? Math.min(time, dur) : time); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/components/AudioPlayer/AudioPlayer.tsx` around lines 160 - 174, The
timeupdate handler in wavesurfer.on('timeupdate') is pausing playback when time
>= duration which prevents the media 'ended' event and thus WaveSurfer's
'finish' handler (and your onFinish callback) from running; remove the terminal
pause/loop logic from the timeupdate handler (the block that calls
wavesurfer.pause(), wavesurfer.seekTo(0)/play when isLooping,
setIsPlaying(false), and the early return) and instead let the media naturally
reach 'ended' so the existing wavesurfer.on('finish') handler can run and invoke
onFinish; keep only non-terminal updates (e.g., setCurrentTime) in the
timeupdate handler and rely on the isLooping/finish logic inside the finish
event handler to handle looping and finalization.
Comment on lines
+284
to
+297
| # Build version manifest entries | ||
| version_entries = [] | ||
| for v in versions: | ||
| v_path = Path(v.audio_path) | ||
| effects_chain = None | ||
| if v.effects_chain: | ||
| effects_chain = json.loads(v.effects_chain) | ||
| version_entries.append({ | ||
| "id": v.id, | ||
| "label": v.label, | ||
| "is_default": v.is_default, | ||
| "effects_chain": effects_chain, | ||
| "filename": v_path.name, | ||
| }) |
Contributor
There was a problem hiding this comment.
Keep the new archive format round-trippable.
import_generation_from_zip() at Lines 384-445 still imports audio_files[0] into a single DBGeneration row and never recreates DBGenerationVersion records. A ZIP exported here will therefore lose extra versions, is_default, and effects_chain on re-import, and the imported audio will follow write order rather than the default version.
Also applies to: 316-330
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/export_import.py` around lines 284 - 297, The export currently
serializes multiple versions (see version_entries in export_import.py) but
import_generation_from_zip() only creates a single DBGeneration and assigns
audio_files[0], losing DBGenerationVersion rows, is_default flags, effects_chain
and original default selection; update import_generation_from_zip() to recreate
DBGenerationVersion records for each exported version entry (use the exported
"filename", "is_default", "effects_chain", "label", and original version id if
needed) and attach each saved audio file to its own DBGenerationVersion record
rather than stuffing the first file into DBGeneration; ensure the code restores
the is_default flag by setting the DBGeneration.default_version (or equivalent)
to the exported default and preserve effects_chain by deserializing and saving
it on each DBGenerationVersion so re-export/import is round-trippable.
Comment on lines
+286
to
+297
| for v in versions: | ||
| v_path = Path(v.audio_path) | ||
| effects_chain = None | ||
| if v.effects_chain: | ||
| effects_chain = json.loads(v.effects_chain) | ||
| version_entries.append({ | ||
| "id": v.id, | ||
| "label": v.label, | ||
| "is_default": v.is_default, | ||
| "effects_chain": effects_chain, | ||
| "filename": v_path.name, | ||
| }) |
Contributor
There was a problem hiding this comment.
Fail the export when a version file is missing.
Every DB row is added to manifest["versions"], but the ZIP write silently skips missing audio_paths. That can produce an archive whose manifest references files that were never bundled, and the fallback at Lines 326-330 does not help once versions is non-empty.
💡 Minimal fix
version_entries = []
for v in versions:
v_path = Path(v.audio_path)
+ if not v_path.exists():
+ raise ValueError(f"Generation version audio file not found: {v.audio_path}")
effects_chain = None
if v.effects_chain:
effects_chain = json.loads(v.effects_chain)
version_entries.append({
"id": v.id,
@@
# Add all version audio files
for v in versions:
v_path = Path(v.audio_path)
- if v_path.exists():
- zip_file.write(v_path, f"audio/{v_path.name}")
+ zip_file.write(v_path, f"audio/{v_path.name}")Also applies to: 320-330
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/export_import.py` around lines 286 - 297, The export currently adds
every DB row into manifest["versions"] even if the referenced audio file
(v.audio_path) is missing and the later zip write silently skips missing files;
modify the loop that builds version_entries (and the corresponding zip-writing
block that iterates versions) to verify existence of Path(v.audio_path) before
appending to version_entries or attempting to write it to the ZIP, and if a file
is missing raise/return a clear exception (fail the export) instead of
continuing; reference the loop over versions, v.audio_path, version_entries and
the ZIP write block that handles versions to locate where to add the
Path(...).exists() check and the early failure.
Comment on lines
+1000
to
+1003
| # Set to generating so the UI shows the loader and SSE picks it up | ||
| gen.status = "generating" | ||
| gen.error = None | ||
| db.commit() |
Contributor
There was a problem hiding this comment.
Do not downgrade an already completed generation to failed when regenerate fails.
This endpoint mutates status to generating (Line 1001) and then to failed on regenerate errors (Line 1078), even though a valid prior version/audio already exists. That can incorrectly block completed-only operations for the original item.
🔧 Suggested fix
async def regenerate_generation(generation_id: str, db: Session = Depends(get_db)):
@@
- # Set to generating so the UI shows the loader and SSE picks it up
+ previous_status = gen.status or "completed"
+ previous_audio_path = gen.audio_path
+ previous_duration = gen.duration
+
+ # Set to generating so the UI shows the loader and SSE picks it up
gen.status = "generating"
@@
except Exception as e:
import traceback
traceback.print_exc()
- await history.update_generation_status(
- generation_id=generation_id,
- status="failed",
- db=bg_db,
- error=str(e),
- )
+ if previous_audio_path:
+ await history.update_generation_status(
+ generation_id=generation_id,
+ status=previous_status,
+ db=bg_db,
+ audio_path=previous_audio_path,
+ duration=previous_duration,
+ )
+ else:
+ await history.update_generation_status(
+ generation_id=generation_id,
+ status="failed",
+ db=bg_db,
+ error=str(e),
+ )Also applies to: 1078-1083
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/main.py` around lines 1000 - 1003, The current code unconditionally
sets gen.status = "generating" and later may set gen.status = "failed" even if a
prior completed generation/audio exists; modify the regenerate flow in
backend/main.py to first check the existing generation state and assets (e.g.,
gen.status and whatever field stores the completed audio/asset) and only set
gen.status = "generating" if there is no already-completed result to preserve,
and when a regenerate attempt fails avoid overwriting a prior completed
state—only set gen.error (and set status to "failed") when there is no
previously completed asset to fall back to; update the logic around the
gen.status/gen.error mutations and db.commit calls so they guard on gen.status
== "completed" or presence of the completed asset before downgrading.
- Add is_favorited column with toggle endpoint and star button on history - Show sparkles icon on profile cards that have effects configured - Gold ring on selected profile cards - Smaller, gray action buttons with brighter hover - Clamp player time to duration to prevent runaway playback - Align profile card icon to top for wrapped names - Flush bottom corners on history card when versions expanded - Simplify .gitignore data/ rule
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
app/src/components/VoiceProfiles/ProfileCard.tsx (1)
115-121: Consider adding a tooltip for accessibility.The Sparkles icon correctly renders when
effects_chainexists and is non-empty, and the type guard properly handles the optional field. However, the icon is purely visual with no accessible context—screen reader users and sighted users unfamiliar with the UI won't know what it indicates.💡 Suggested improvement to add a title attribute
{profile.effects_chain && profile.effects_chain.length > 0 && ( - <Sparkles className="h-3.5 w-3.5 text-accent" /> + <Sparkles className="h-3.5 w-3.5 text-accent" aria-hidden="true" title="Has audio effects" /> )}Alternatively, wrap in a
<Tooltip>component if one is available in the UI library for a more polished experience.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/VoiceProfiles/ProfileCard.tsx` around lines 115 - 121, Add accessible context for the Sparkles icon in ProfileCard.tsx by providing a tooltip or title so screen readers and sighted users understand its meaning; locate the conditional that renders <Sparkles /> when profile.effects_chain && profile.effects_chain.length > 0 and either add a title attribute (e.g., title="Has audio effects") on the Sparkles element or wrap it with the project's Tooltip component (if available) so the icon conveys "has effects_chain" to assistive tech and hover users.backend/main.py (1)
1722-1728: Consider simplifying the lambda wrapper.The lambda wrapping
sf.writeis functional but slightly awkward. A direct call would be cleaner sinceasyncio.to_threadaccepts positional args.♻️ Suggested simplification
- await asyncio.to_thread(lambda: sf.write(buf, processed, sample_rate, format="WAV")) + def _write_wav(): + sf.write(buf, processed, sample_rate, format="WAV") + await asyncio.to_thread(_write_wav)Or use
functools.partial:from functools import partial await asyncio.to_thread(partial(sf.write, buf, processed, sample_rate, format="WAV"))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/main.py` around lines 1722 - 1728, Replace the lambda wrapper around sf.write when calling asyncio.to_thread with a direct call that passes sf.write and its args to asyncio.to_thread (or use functools.partial); specifically update the call using asyncio.to_thread(lambda: sf.write(buf, processed, sample_rate, format="WAV")) to call asyncio.to_thread(sf.write, buf, processed, sample_rate, format="WAV") (or asyncio.to_thread(partial(sf.write, buf, processed, sample_rate, format="WAV"))) so sf.write, buf, processed and sample_rate are passed directly without the lambda.backend/database.py (1)
439-454: Rename unused loop variablekeyto_key.The loop variable
keyis not used within the loop body. Prefix with underscore to indicate intentionally unused.♻️ Suggested fix
- for idx, (key, preset_data) in enumerate(BUILTIN_PRESETS.items()): + for idx, (_key, preset_data) in enumerate(BUILTIN_PRESETS.items()):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/database.py` around lines 439 - 454, Rename the unused loop variable key to _key in the for loop that iterates over BUILTIN_PRESETS (the header currently reads for idx, (key, preset_data) in enumerate(BUILTIN_PRESETS.items()):); update it to for idx, (_key, preset_data) in enumerate(BUILTIN_PRESETS.items()): so that the unused identifier is clearly marked, leaving the rest of the logic that creates or updates EffectPreset, uses sort_order, existing, db.add, and db.commit unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/history.py`:
- Around line 40-44: The current try/except around json.loads(v.effects_chain)
and the subsequent EffectConfig(**e) list comprehension swallows JSON parse or
validation errors (effects_chain) silently; change the except block to catch
Exception as e and emit a warning that includes the offending v.effects_chain
and the exception (e) so corrupted data is visible in logs—use the module logger
(e.g., logger.warning or logging.getLogger(__name__).warning) and keep the rest
of the logic (raw = json.loads(...), effects_chain = [EffectConfig(**e) ...])
intact.
---
Nitpick comments:
In `@app/src/components/VoiceProfiles/ProfileCard.tsx`:
- Around line 115-121: Add accessible context for the Sparkles icon in
ProfileCard.tsx by providing a tooltip or title so screen readers and sighted
users understand its meaning; locate the conditional that renders <Sparkles />
when profile.effects_chain && profile.effects_chain.length > 0 and either add a
title attribute (e.g., title="Has audio effects") on the Sparkles element or
wrap it with the project's Tooltip component (if available) so the icon conveys
"has effects_chain" to assistive tech and hover users.
In `@backend/database.py`:
- Around line 439-454: Rename the unused loop variable key to _key in the for
loop that iterates over BUILTIN_PRESETS (the header currently reads for idx,
(key, preset_data) in enumerate(BUILTIN_PRESETS.items()):); update it to for
idx, (_key, preset_data) in enumerate(BUILTIN_PRESETS.items()): so that the
unused identifier is clearly marked, leaving the rest of the logic that creates
or updates EffectPreset, uses sort_order, existing, db.add, and db.commit
unchanged.
In `@backend/main.py`:
- Around line 1722-1728: Replace the lambda wrapper around sf.write when calling
asyncio.to_thread with a direct call that passes sf.write and its args to
asyncio.to_thread (or use functools.partial); specifically update the call using
asyncio.to_thread(lambda: sf.write(buf, processed, sample_rate, format="WAV"))
to call asyncio.to_thread(sf.write, buf, processed, sample_rate, format="WAV")
(or asyncio.to_thread(partial(sf.write, buf, processed, sample_rate,
format="WAV"))) so sf.write, buf, processed and sample_rate are passed directly
without the lambda.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ef139095-ba36-40c1-b59f-944aaa95b1ce
📒 Files selected for processing (9)
.gitignoreapp/src/components/History/HistoryTable.tsxapp/src/components/VoiceProfiles/ProfileCard.tsxapp/src/lib/api/client.tsapp/src/lib/api/types.tsbackend/database.pybackend/history.pybackend/main.pybackend/models.py
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/lib/api/types.ts
Comment on lines
+40
to
+44
| try: | ||
| raw = json.loads(v.effects_chain) | ||
| effects_chain = [EffectConfig(**e) for e in raw] | ||
| except Exception: | ||
| pass |
Contributor
There was a problem hiding this comment.
Log JSON parse failures instead of silently swallowing them.
If the stored effects_chain is malformed, this silent pass makes debugging very difficult. A warning log would surface corrupted data without breaking the response.
🛠️ Suggested fix
if v.effects_chain:
try:
raw = json.loads(v.effects_chain)
effects_chain = [EffectConfig(**e) for e in raw]
- except Exception:
- pass
+ except Exception as e:
+ import logging
+ logging.warning(
+ "Failed to parse effects_chain for version %s: %s", v.id, e
+ )🧰 Tools
🪛 Ruff (0.15.5)
[error] 43-44: try-except-pass detected, consider logging the exception
(S110)
[warning] 43-43: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/history.py` around lines 40 - 44, The current try/except around
json.loads(v.effects_chain) and the subsequent EffectConfig(**e) list
comprehension swallows JSON parse or validation errors (effects_chain) silently;
change the except block to catch Exception as e and emit a warning that includes
the offending v.effects_chain and the exception (e) so corrupted data is visible
in logs—use the module logger (e.g., logger.warning or
logging.getLogger(__name__).warning) and keep the rest of the logic (raw =
json.loads(...), effects_chain = [EffectConfig(**e) ...]) intact.
- Story items can be pinned to a specific generation version via toolbar dropdown (shows when clip is selected and has >1 version) - version_id column on story_items with migration, validated against the generation's versions before saving - Split/duplicate preserve the source clip's pinned version - Export and playback resolve version-specific audio paths - Extracted _build_item_detail helper in stories.py (DRY cleanup) - Story list restyled from rounded cards to flat rows with rounded hover/active states, gradient header fade, and dynamic bottom padding that accounts for track editor + generate box
…ul with inline inspector
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a full audio post-processing effects pipeline powered by Spotify's pedalboard library, enabling users to apply professional DSP effects to generated speech.
Features
Effects Engine (
backend/utils/effects.py)Generation Versions (
backend/versions.py)Effects Presets (
backend/effects.py)API Endpoints
GET /effects/available— list all effect types with parameter definitionsGET /effects/presets— list all presets (built-in + custom)POST /effects/presets— create custom presetDELETE /effects/presets/{id}— delete custom presetPOST /effects/preview/{generation_id}— ephemeral preview (process in memory, stream WAV back without saving)POST /generations/{id}/versions/apply-effects— apply effects to existing generationGET /generations/{id}/versions— list all versionsPUT /generations/{id}/versions/{vid}/set-default— switch active versionGET /audio/version/{version_id}— serve specific version audioPUT /profiles/{id}/effects— set default effects chain on a voice profileFrontend
app/src/components/Effects/EffectsChainEditor.tsx) — drag-and-drop reorderable effect chain builder with parameter sliders, preset loading, enable/disable per effectapp/src/components/EffectsTab/) — dedicated sidebar tab with left panel (preset list) + right panel (editor with live preview via generation picker)app/src/components/Effects/GenerationPicker.tsx) — reusable popover for selecting a generation for previewIntegration Points
Dependencies
pedalboard>=0.9.0(Spotify's DSP library, pip-installable, no system deps)@dnd-kit(already in the project, used for effect chain reordering)Summary by CodeRabbit