Dataset & preprocessing UX: trigger tags, mix-from-folders, Preprocess All - #62
Dataset & preprocessing UX: trigger tags, mix-from-folders, Preprocess All#62scragnog wants to merge 4 commits into
Conversation
…ontrol Trigger tags (from 1fe7c70, Gabriel's work): bulk apply now overwrites existing tags, respects file selection scope, saves per-file tag_position to sidecars (read during preprocessing), adds tag_position to _KNOWN_SIDECAR_KEYS, removes the confusing 'Replace' position option. Mix datasets: extends the koda-dernet#52 Windows link fix — selections may include whole folders (recursive audio expansion), output is flattened by filename, sidecars are always independent copies (never links) with copy/empty choice via new sidecar_mode API param, and a zero-created run cleans up after itself with an error summary. Co-authored-by: Gabriel <gadna166@gmail.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…lation
- Add 'Preprocess All' button to Audio Library header
- Expose getFolders() from Dataset module for trigger tag data
- Wire Preprocess All to queue all folders with their common_trigger
- Update Preprocess Selected to pass per-folder trigger tags
- queuePreprocess() now accepts objects with {path, triggerTag}
- _runNextInQueue() auto-sets trigger tag field per queue item
- Queue list shows trigger tag next to each folder name
The queue-based _runNextInQueue() was missing genre_ratio from its config object, causing it to always default to 0 regardless of the UI field value.
…able - Checkbox now immediately updates Trigger Tag column for all folder/file rows - Folder rows show their own name, file rows inherit parent folder name - Re-renders table on checkbox toggle - Also auto-fills pp-trigger-tag when navigating to Preprocess tab via folder button
📝 WalkthroughWalkthroughThe update adds folder-derived trigger tags and a Preprocess All action, extends preprocess queues and bulk tagging to selected items, adds mix sidecar copy/empty modes, and preserves tag-position metadata through sidecar parsing and prompt construction. ChangesDataset workflow updates
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant DatasetBrowser
participant WorkspaceLab
participant API
participant Backend
DatasetBrowser->>WorkspaceLab: queue folder preprocessing with trigger tags
WorkspaceLab->>API: run preprocessing with triggerTag and genre_ratio
DatasetBrowser->>API: create mix with sidecarMode
API->>Backend: create dataset and write sidecars
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 HTMLHint (1.9.2)frontend/index.html[{"file":"/frontend/index.html","messages":[{"type":"warning","message":"The type attribute must be present on elements.","raw":"<button class="btn btn--sm browse-btn" data-target="welcome-checkpoint-dir">","evidence":" <button class="btn btn--sm browse-btn" data-target="welcome-checkpoint-dir">Browse","line":53,"col":16,"rule":{"id":"button-type-require","description":"The type attribute of a element must be present with a valid value: "button", "submit", or "reset".","link":"https://htmlhint.com/rules/button-type-require"}},{"type":"warning","message":"The type attribute must be present on elements.","raw":"<button class="btn btn--sm browse-btn" data-target="welcome-audio-dir">","evidence":" <button class="btn btn--sm browse-btn" data-target="welcome-audio-dir">Browse","line":61,"col":16,"rule":{"id":"button-type-require","description":"The type attribute of a element must be present with a vali ... [truncated 144340 characters] ... . ","link":"https://htmlhint.com/rules/input-requires-label"}},{"type":"warning","message":"No matching [ label ] tag found.","raw":"<input type="checkbox" id="crt-toggle">","evidence":" <input type="checkbox" id="crt-toggle">","line":2793,"col":19,"rule":{"id":"input-requires-label","description":"All [ input ] tags must have a corresponding [ label ] tag. ","link":"https://htmlhint.com/rules/input-requires-label"}},{"type":"warning","message":"No matching [ label ] tag found.","raw":"<input type="range" id="crt-strength" min="0" max="1" step="0.05" value="0.70" class="range-styled" style="flex:1;">","evidence":" <input type="range" id="crt-strength" min="0" max="1" step="0.05" value="0.70" class="range-styled" style="flex:1;"> 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}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/js/workspace-lab.js (1)
242-257: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winQueued output directories are derived from basename only — collision risk across differently-nested folders.
outputDir: _joinPath(tensorsDir, _pathBasename(audioDir) || "tensors")uses only the last path segment. Two source folders under different parents but sharing a name (e.g.artist_a/vocalsandartist_b/vocals) resolve to the identicaloutputDir. This existed for the single-folder flow too, but this PR makes it far more likely to actually trigger, since bothdataset.js'sdataset-bulk-preprocess(multi-folder select) and the new_initPreprocessAll(queues every folder in the library) now feed multiple items into this same queue, which starts running immediately (_runNextInQueue()) with no per-item review step. A collision here silently merges/overwrites tensors from two unrelated source folders in one output directory.Disambiguate using the folder's relative path (not just basename), e.g. joining sanitized path segments, before falling back to basename-only collisions.
💡 Possible fix direction
- outputDir: _joinPath(tensorsDir, _pathBasename(audioDir) || "tensors"), + outputDir: _joinPath(tensorsDir, _relativeSlugForOutput(audioDir, root) || "tensors"),Where
_relativeSlugForOutputderives a unique slug from the folder's path relative to the audio root (e.g. joining path segments with_), falling back to basename only when unambiguous.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/js/workspace-lab.js` around lines 242 - 257, Update queuePreprocess to derive each queued outputDir from the audio folder’s path relative to the audio root, using sanitized path segments so differently nested folders remain distinct; reuse _relativeSlugForOutput if available. Preserve the tensorsDir root and only fall back to basename-based naming when the relative path cannot be determined.sidestep_engine/data/preprocess_prompt.py (1)
32-38: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRemoved "replace" handling leaves a stale, now-broken UI option and stale docstring.
The
tag_position == "replace"branch was removed, so whentag_positionisn't"prepend"or"append", theif tag:block does nothing — the trigger tag is silently dropped from the prompt rather than replacing the caption/genre text. Two consequences:
- The docstring (lines 34-35) still documents
tag_positionas accepting"prepend","append", or"replace", which is now inaccurate.frontend/index.html's standalone Preprocess tab (id="pp-tag-position", separate from the Bulk Trigger Tag modal that correctly dropped "Replace" in this PR) still offers<option value="replace">Replace caption</option>. That value is read via$("pp-tag-position")?.valueinworkspace-lab.jsand sent astag_positionin the preprocess config — if this function is what ultimately renders the training prompt for that flow, selecting "Replace caption" there now silently drops the trigger tag entirely, with no error surfaced to the user.Either restore explicit "replace" handling, or remove the now-dead
pp-tag-position"Replace caption" option infrontend/index.htmland update this docstring to match.🔧 Suggested fix (if replace should stay removed)
Args: meta: Per-sample metadata dict from the dataset JSON. - tag_position: Where to apply ``custom_tag`` (``"prepend"``, - ``"append"``, or ``"replace"``). + tag_position: Where to apply ``custom_tag`` (``"prepend"`` or + ``"append"``).- <option value="replace">Replace caption</option>(remove the option from
frontend/index.html's#pp-tag-positionselect)Also applies to: 58-63
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sidestep_engine/data/preprocess_prompt.py` around lines 32 - 38, Resolve the stale replace behavior across the preprocess flow: either restore explicit replace handling in the prompt-building function, or, if replace remains unsupported, remove the “replace” option from the pp-tag-position select in frontend/index.html and update that function’s docstring to document only the supported tag positions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/js/api.js`:
- Around line 233-243: Update bulkWriteTriggerTag so the selectedPaths branch
resolves each selected file through the underlying dataset file metadata or an
equivalent lookup, preferring its sidecar_path and falling back to deriving the
.txt path from the file path. Extend the Dataset/scanDataset result flow as
needed to expose that metadata, while preserving the existing scan-based
behavior and selection filtering.
In `@frontend/js/dataset.js`:
- Around line 663-695: The _initPreprocessAll function currently queues
aggregated parent folders alongside their descendants, causing overlapping scans
and duplicate tensor outputs. Filter _folders to only leaf audio folders—exclude
any folder that is an ancestor of another folder—before mapping them into
preprocessing items, while preserving the existing root-folder fallback when no
non-root leaves exist.
In `@sidestep_engine/gui/file_ops.py`:
- Around line 560-612: Update the flattened output naming in the audio expansion
loop so distinct source files with the same basename receive unique destination
names instead of being counted as skipped. Preserve idempotent skipping when the
existing destination corresponds to the same source, and disambiguate collisions
from different sources using a deterministic counter or relative-path-derived
suffix before calling _link_or_copy_mix_file.
In `@sidestep_engine/gui/server.py`:
- Line 107: Constrain the sidecar_mode parameter in the server API to the
supported values "copy" and "empty", rejecting any other value with HTTP 422
before invoking file operations. Preserve the existing default of "copy" and the
behavior in file_ops.py for valid values.
---
Outside diff comments:
In `@frontend/js/workspace-lab.js`:
- Around line 242-257: Update queuePreprocess to derive each queued outputDir
from the audio folder’s path relative to the audio root, using sanitized path
segments so differently nested folders remain distinct; reuse
_relativeSlugForOutput if available. Preserve the tensorsDir root and only fall
back to basename-based naming when the relative path cannot be determined.
In `@sidestep_engine/data/preprocess_prompt.py`:
- Around line 32-38: Resolve the stale replace behavior across the preprocess
flow: either restore explicit replace handling in the prompt-building function,
or, if replace remains unsupported, remove the “replace” option from the
pp-tag-position select in frontend/index.html and update that function’s
docstring to document only the supported tag positions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2bbae970-598a-4279-9b00-69925f65aba5
📒 Files selected for processing (9)
frontend/index.htmlfrontend/js/api.jsfrontend/js/dataset.jsfrontend/js/workspace-lab.jssidestep_engine/data/dataset_builder.pysidestep_engine/data/preprocess_prompt.pysidestep_engine/data/sidecar_metadata.pysidestep_engine/gui/file_ops.pysidestep_engine/gui/server.py
| async function bulkWriteTriggerTag(datasetDir, tag, position, selectedPaths) { | ||
| let paths; | ||
| if (selectedPaths && selectedPaths.length) { | ||
| // Only apply to selected files — derive sidecar paths | ||
| paths = selectedPaths.map(p => p.replace(/\.[^.]+$/, '.txt')); | ||
| } else { | ||
| // No selection: scan for ALL audio files | ||
| const scan = await scanDataset(datasetDir).catch(() => ({ files: [] })); | ||
| paths = (scan.files || []) | ||
| .map(f => f.sidecar_path || f.path.replace(/\.[^.]+$/, '.txt')); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Selected-path branch ignores sidecar_path, unlike the scan-based branch in the same function.
When selectedPaths is provided, sidecar paths are derived purely via p.replace(/\.[^.]+$/, '.txt'). The fallback ("no selection") branch right below it explicitly prefers f.sidecar_path || f.path.replace(...), implying sidecar paths aren't always a naive extension swap. The new selection-aware path silently drops that fallback, so a bulk trigger-tag write against a selection could target the wrong sidecar for any file whose real sidecar path diverges from the naive convention.
🔧 Suggested fix
- if (selectedPaths && selectedPaths.length) {
- // Only apply to selected files — derive sidecar paths
- paths = selectedPaths.map(p => p.replace(/\.[^.]+$/, '.txt'));
- } else {
+ if (selectedPaths && selectedPaths.length) {
+ // Only apply to selected files — prefer known sidecar_path when available
+ const known = new Map((Dataset?.getFiles?.() || []).map(f => [f.path, f.sidecar_path]));
+ paths = selectedPaths.map(p => known.get(p) || p.replace(/\.[^.]+$/, '.txt'));
+ } else {This requires exposing the underlying file objects (or a lookup) from Dataset, not just bare path strings, to fully resolve.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/js/api.js` around lines 233 - 243, Update bulkWriteTriggerTag so the
selectedPaths branch resolves each selected file through the underlying dataset
file metadata or an equivalent lookup, preferring its sidecar_path and falling
back to deriving the .txt path from the file path. Extend the
Dataset/scanDataset result flow as needed to expose that metadata, while
preserving the existing scan-based behavior and selection filtering.
| /* ---- Preprocess All button ---- */ | ||
| function _initPreprocessAll() { | ||
| $('btn-preprocess-all')?.addEventListener('click', () => { | ||
| const root = _scanRoot || _canonicalAudioPath(); | ||
| if (!root) { if (typeof showToast === 'function') showToast('No audio directory configured', 'warn'); return; } | ||
| // Collect all non-root folders with their trigger tags | ||
| const autoTag = document.getElementById('dataset-auto-folder-trigger')?.checked; | ||
| const items = _folders | ||
| .filter(f => (f.path || '.') !== '.') | ||
| .map(f => { | ||
| let tConfig = f.common_trigger || ''; | ||
| if (autoTag) { | ||
| tConfig = (f.path || '').split('/').pop() || tConfig; | ||
| } | ||
| return { | ||
| path: _joinPath(root, f.path), | ||
| triggerTag: tConfig, | ||
| }; | ||
| }); | ||
| if (!items.length) { | ||
| // Only root folder — preprocess the root itself | ||
| items.push({ path: root, triggerTag: _folders[0]?.common_trigger || '' }); | ||
| } | ||
| _setActiveLabPanel('preprocess'); | ||
| if (typeof WorkspaceLab !== 'undefined' && WorkspaceLab.queuePreprocess) { | ||
| WorkspaceLab.queuePreprocess(items); | ||
| showToast(items.length + ' folder' + (items.length > 1 ? 's' : '') + ' queued for preprocessing', 'ok'); | ||
| } else { | ||
| _openPreprocessForFolder('.'); | ||
| } | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether audio scanning/preprocessing walks subdirectories recursively.
rg -n "rglob|os.walk|walk\(|recursive" sidestep_engine/gui/server.py sidestep_engine/data --type py -C2Repository: koda-dernet/Side-Step
Length of output: 3725
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## dataset.js relevant sections"
fd -a 'dataset.js$' . | sed 's#^\./##'
FILE="$(fd 'dataset.js$' . | head -n1)"
wc -l "$FILE"
sed -n '1,130p' "$FILE"
sed -n '600,720p' "$FILE"
echo
echo "## preprocessing endpoints/functions and queue"
rg -n "queuePreprocess|_runNextInQueue|openPreprocess|preprocess|audio_dir|scanAudioFolder|scan.*folder|dataset.*scan|_folders|common_trigger" frontend/sidestep_engine gui server.py . -A3 -B3 || trueRepository: koda-dernet/Side-Step
Length of output: 50378
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="$(fd 'dataset.js$' . | head -n1)"
echo "## frontend/js/dataset.js outline (preprocess/folder symbols)"
ast-grep outline "$FILE" --match '_initPreprocessAll|_folders|scanAudioFolder|_openPreprocessForFolder|queuePreprocess|_runNextInQueue' --view expanded || true
echo
echo "## frontend/js/dataset.js relevant lines"
sed -n '580,710p' "$FILE"
echo
echo "## frontend/js/dataset.js queue/scan lines"
rg -n "function _openPreprocessForFolder|queuePreprocess|_runNextInQueue|scanAudioFolder|_folders\s*=|_scanRoot|Preprocess All|dataset-auto-folder-trigger" "$FILE" -C 4
echo
echo "## server preprocessing endpoints"
sed -n '130,180p' sidestep_engine/gui/server.py
sed -n '455,475p' sidestep_engine/gui/server.py
echo
echo "## file_ops scan/preprocess wrappers"
rg -n "def scan_audio_dir|scan_audio_dir|preprocess|PreprocessStartRequest|queuePreprocess|_runNextInQueue|scanAudioFolder" sidestep_engine/gui/file_ops.py sidestep_engine/gui/server.py -C 4Repository: koda-dernet/Side-Step
Length of output: 25551
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## sidestep_engine/gui/file_ops.py scan/core"
sed -n '345,520p' sidestep_engine/gui/file_ops.py
echo
echo "## sidestep_engine/data/preprocess.py audio discovery"
sed -n '120,240p' sidestep_engine/data/preprocess.py
sed -n '240,360p' sidestep_engine/data/preprocess.py
echo
echo "## WorkspaceLab queue implementation"
rg -n "class WorkspaceLab|queuePreprocess|_runNextInQueue|start_preprocess|preprocess" frontend sidestep_engine -C 3Repository: koda-dernet/Side-Step
Length of output: 50378
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="$(fd 'dataset.js$' . | head -n1)"
echo "## frontend/js/workspace-lab.js queue and output section"
sed -n '130,280p' frontend/js/workspace-lab.js
echo
echo "## frontend/js/dataset.js scan normalization and hierarchy section"
sed -n '75,250p' "$FILE"
echo
echo "## relevant preprocess discovery recursion"
sed -n '70,105p' sidestep_engine/data/preprocess_discovery.pyRepository: koda-dernet/Side-Step
Length of output: 14082
Filter "Preprocess All" to leaf audio folders.
/api/dataset/scan returns aggregated folder rows, while preprocessing and discovery use rglob("*") from the given audio_dir, so queuing both artist/ and artist/album/ scans/discovery overlaps; the queued jobs also write separate tensor output directories, creating duplicate tensor work. Derive leaf folders and only enqueue those, or document that this button intentionally duplicates raw audio across tensor outputs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/js/dataset.js` around lines 663 - 695, The _initPreprocessAll
function currently queues aggregated parent folders alongside their descendants,
causing overlapping scans and duplicate tensor outputs. Filter _folders to only
leaf audio folders—exclude any folder that is an ancestor of another
folder—before mapping them into preprocessing items, while preserving the
existing root-folder fallback when no non-root leaves exist.
| # -- Expand entries: folders → contained audio files, files pass through -- | ||
| audio_paths: List[Path] = [] | ||
| for raw in files or []: | ||
| p = _resolve_gui_path(raw) | ||
| if p.is_dir(): | ||
| audio_paths.extend( | ||
| sorted(f for f in p.rglob("*") if f.is_file() and f.suffix.lower() in _AUDIO_EXTS) | ||
| ) | ||
| elif p.is_file() and p.suffix.lower() in _AUDIO_EXTS: | ||
| audio_paths.append(p) | ||
|
|
||
| if not audio_paths: | ||
| return {"ok": False, "error": "No audio files found in the selection"} | ||
|
|
||
| out_dir.mkdir(parents=True, exist_ok=True) | ||
|
|
||
| created = 0 | ||
| skipped = 0 | ||
| errors: List[str] = [] | ||
|
|
||
| for raw in files or []: | ||
| src = _resolve_gui_path(raw) | ||
| if src.suffix.lower() not in _AUDIO_EXTS: | ||
| skipped += 1 | ||
| continue | ||
| if not src.is_file(): | ||
| skipped += 1 | ||
| continue | ||
| try: | ||
| src_res = src.resolve(strict=False) | ||
| rel = src_res.relative_to(src_root_res) | ||
| except ValueError: | ||
| skipped += 1 | ||
| continue | ||
|
|
||
| dest_audio = out_dir / rel | ||
| dest_audio.parent.mkdir(parents=True, exist_ok=True) | ||
| for src in audio_paths: | ||
| dest_audio = out_dir / src.name | ||
| if dest_audio.exists(): | ||
| skipped += 1 | ||
| continue | ||
|
|
||
| try: | ||
| _link_or_copy_mix_file(src_res, dest_audio) | ||
| _link_or_copy_mix_file(src, dest_audio) | ||
| created += 1 | ||
| except OSError as exc: | ||
| errors.append(f"{src.name}: {exc}") | ||
| continue | ||
|
|
||
| src_sidecar = src_res.with_suffix(".txt") | ||
| if src_sidecar.is_file(): | ||
| dest_sidecar = dest_audio.with_suffix(".txt") | ||
| if not dest_sidecar.exists(): | ||
| # -- Sidecar handling: always an independent file, never a link -- | ||
| dest_sidecar = dest_audio.with_suffix(".txt") | ||
| if not dest_sidecar.exists(): | ||
| src_sidecar = src.with_suffix(".txt") | ||
| if sidecar_mode == "copy" and src_sidecar.is_file(): | ||
| try: | ||
| _link_or_copy_mix_file(src_sidecar, dest_sidecar) | ||
| shutil.copy2(str(src_sidecar), str(dest_sidecar)) | ||
| except OSError: | ||
| # Sidecar failures should not fail the whole mix. | ||
| pass | ||
| dest_sidecar.write_text("", encoding="utf-8") | ||
| else: | ||
| dest_sidecar.write_text("", encoding="utf-8") | ||
|
|
||
| if created == 0: | ||
| # Clean up the empty directory we created | ||
| try: | ||
| shutil.rmtree(str(out_dir), ignore_errors=True) | ||
| except Exception: | ||
| pass | ||
| summary = "; ".join(errors[:5]) if errors else "unknown" | ||
| return {"ok": False, "error": f"Could not link any files: {summary}"} |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Flattened filenames can collide across source folders, silently dropping files from the mix.
Recursive expansion (p.rglob("*")) can pull audio files with the same basename from different subfolders (e.g. artist_a/track01.mp3 and artist_b/track01.mp3). Since output naming is basename-only (dest_audio = out_dir / src.name), the second file to reach that name sees dest_audio.exists() already True and is counted as skipped — identical to the "already linked, safe to skip" case used for idempotent re-runs. There's no way to distinguish "intentionally already present" from "a different file dropped due to a name clash," and no error is surfaced for the latter. Given this PR explicitly targets flattened output from recursive folder selections, basename clashes between differently-named source folders are a realistic scenario (generic stem/track names are common).
Disambiguate on collision (e.g. suffix with a counter or a slug derived from the file's relative path) instead of silently skipping distinct source files.
🔧 Suggested fix direction
for src in audio_paths:
- dest_audio = out_dir / src.name
- if dest_audio.exists():
- skipped += 1
- continue
+ dest_audio = out_dir / src.name
+ if dest_audio.exists():
+ # Disambiguate genuine name collisions between distinct source files
+ n = 1
+ while dest_audio.exists():
+ dest_audio = out_dir / f"{src.stem}_{n}{src.suffix}"
+ n += 1📝 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.
| # -- Expand entries: folders → contained audio files, files pass through -- | |
| audio_paths: List[Path] = [] | |
| for raw in files or []: | |
| p = _resolve_gui_path(raw) | |
| if p.is_dir(): | |
| audio_paths.extend( | |
| sorted(f for f in p.rglob("*") if f.is_file() and f.suffix.lower() in _AUDIO_EXTS) | |
| ) | |
| elif p.is_file() and p.suffix.lower() in _AUDIO_EXTS: | |
| audio_paths.append(p) | |
| if not audio_paths: | |
| return {"ok": False, "error": "No audio files found in the selection"} | |
| out_dir.mkdir(parents=True, exist_ok=True) | |
| created = 0 | |
| skipped = 0 | |
| errors: List[str] = [] | |
| for raw in files or []: | |
| src = _resolve_gui_path(raw) | |
| if src.suffix.lower() not in _AUDIO_EXTS: | |
| skipped += 1 | |
| continue | |
| if not src.is_file(): | |
| skipped += 1 | |
| continue | |
| try: | |
| src_res = src.resolve(strict=False) | |
| rel = src_res.relative_to(src_root_res) | |
| except ValueError: | |
| skipped += 1 | |
| continue | |
| dest_audio = out_dir / rel | |
| dest_audio.parent.mkdir(parents=True, exist_ok=True) | |
| for src in audio_paths: | |
| dest_audio = out_dir / src.name | |
| if dest_audio.exists(): | |
| skipped += 1 | |
| continue | |
| try: | |
| _link_or_copy_mix_file(src_res, dest_audio) | |
| _link_or_copy_mix_file(src, dest_audio) | |
| created += 1 | |
| except OSError as exc: | |
| errors.append(f"{src.name}: {exc}") | |
| continue | |
| src_sidecar = src_res.with_suffix(".txt") | |
| if src_sidecar.is_file(): | |
| dest_sidecar = dest_audio.with_suffix(".txt") | |
| if not dest_sidecar.exists(): | |
| # -- Sidecar handling: always an independent file, never a link -- | |
| dest_sidecar = dest_audio.with_suffix(".txt") | |
| if not dest_sidecar.exists(): | |
| src_sidecar = src.with_suffix(".txt") | |
| if sidecar_mode == "copy" and src_sidecar.is_file(): | |
| try: | |
| _link_or_copy_mix_file(src_sidecar, dest_sidecar) | |
| shutil.copy2(str(src_sidecar), str(dest_sidecar)) | |
| except OSError: | |
| # Sidecar failures should not fail the whole mix. | |
| pass | |
| dest_sidecar.write_text("", encoding="utf-8") | |
| else: | |
| dest_sidecar.write_text("", encoding="utf-8") | |
| if created == 0: | |
| # Clean up the empty directory we created | |
| try: | |
| shutil.rmtree(str(out_dir), ignore_errors=True) | |
| except Exception: | |
| pass | |
| summary = "; ".join(errors[:5]) if errors else "unknown" | |
| return {"ok": False, "error": f"Could not link any files: {summary}"} | |
| created = 0 | |
| skipped = 0 | |
| errors: List[str] = [] | |
| for src in audio_paths: | |
| dest_audio = out_dir / src.name | |
| if dest_audio.exists(): | |
| # Disambiguate genuine name collisions between distinct source files | |
| n = 1 | |
| while dest_audio.exists(): | |
| dest_audio = out_dir / f"{src.stem}_{n}{src.suffix}" | |
| n += 1 | |
| try: | |
| _link_or_copy_mix_file(src, dest_audio) | |
| created += 1 | |
| except OSError as exc: | |
| errors.append(f"{src.name}: {exc}") | |
| continue | |
| # -- Sidecar handling: always an independent file, never a link -- | |
| dest_sidecar = dest_audio.with_suffix(".txt") | |
| if not dest_sidecar.exists(): | |
| src_sidecar = src.with_suffix(".txt") | |
| if sidecar_mode == "copy" and src_sidecar.is_file(): | |
| try: | |
| shutil.copy2(str(src_sidecar), str(dest_sidecar)) | |
| except OSError: | |
| dest_sidecar.write_text("", encoding="utf-8") | |
| else: | |
| dest_sidecar.write_text("", encoding="utf-8") |
🧰 Tools
🪛 Ruff (0.16.0)
[warning] 607-610: Use contextlib.suppress(Exception) instead of try-except-pass
Replace try-except-pass with with contextlib.suppress(Exception): ...
(SIM105)
[error] 609-610: try-except-pass detected, consider logging the exception
(S110)
[warning] 609-609: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@sidestep_engine/gui/file_ops.py` around lines 560 - 612, Update the flattened
output naming in the audio expansion loop so distinct source files with the same
basename receive unique destination names instead of being counted as skipped.
Preserve idempotent skipping when the existing destination corresponds to the
same source, and disambiguate collisions from different sources using a
deterministic counter or relative-path-derived suffix before calling
_link_or_copy_mix_file.
| destination_root: str | ||
| mix_name: str | ||
| files: List[str] | ||
| sidecar_mode: str = "copy" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Repo files matching server/file_ops names:"
fd -a 'server.py|file_ops.py' . | sed 's#^\./##'
echo
echo "Relevant snippets:"
sed -n '80,125p' sidestep_engine/gui/server.py
sed -n '450,490p' sidestep_engine/gui/server.py
rg -n "sidecar_mode|create_mix_dataset|MixDatasetRequest" .Repository: koda-dernet/Side-Step
Length of output: 4180
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "file_ops.py create_mix_dataset implementation:"
sed -n '500,615p' sidestep_engine/gui/file_ops.py
echo
echo "Programmatic validation of str-typed field behavior with Pydantic:"
python3 - <<'PY'
from pydantic import BaseModel, ValidationError
from typing import List, Optional
class MixDatasetRequestBase(BaseModel):
source_root: str
destination_root: str
mix_name: str
files: Optional[List[str]] = None
sidecar_mode: str = "copy"
class MixDatasetRequestTyped(BaseModel):
source_root: str
destination_root: str
mix_name: str
files: Optional[List[str]] = None
sidecar_mode: str = "copy"
for s in ["copy", "empty", "EMPTY", "copyy", "42"]:
for name, model in [("BaseModel(str)", MixDatasetRequestBase), ("Pydantic str)", MixDatasetRequestTyped)]:
try:
d = model(source_root="a", destination_root="b", mix_name="x", files=[], sidecar_mode=s)
print(f"{name} input={s!r} -> sidecar_mode={d.sidecar_mode!r}")
except ValidationError as e:
print(f"{name} input={s!r} -> ValidationError")
print("sidecar_mode in file_ops defaults copy and branch:", "sidecar_mode == \"copy\"" in open("sidestep_engine/gui/file_ops.py").read())
PYRepository: koda-dernet/Side-Step
Length of output: 4695
🌐 Web query:
Pydantic v1 v2 string coercion validation enum Literal 422 invalid value BaseSettings documentation
💡 Result:
In Pydantic V2, validation behavior for Literal types changed significantly compared to V1, often leading to 422 Unprocessable Entity errors when passing inputs that would have been automatically coerced in V1 [1][2][3]. Key points regarding Literal, Enum, and coercion: 1. Strict Literal Validation: In Pydantic V2, Literal validation is strict [4]. Pydantic expects the input to match the Literal value exactly [5][6]. Unlike standard field types, Literal does not perform broad type coercion [5]. For example, if you have Literal[123], passing the string '123' will fail, even if the field type is int [5]. 2. Enum/StrEnum Interaction: When using Enum or StrEnum members within a Literal (e.g., Literal[MyEnum.VALUE]), Pydantic expects the input to be the actual enum member, not its underlying value [5][6]. Passing the string equivalent of the enum value will typically trigger a literal_error (422) because the input type does not match the expected enum member instance [1][7][5][6]. 3. Coercion Differences: While Pydantic V2 generally performs smart coercion for standard model fields, this logic does not extend to Literal fields [1][5]. If you require coercion, you must either pass the correct type (e.g., the enum member itself) or use a custom validator to perform the cast before the Literal validation occurs [1][5][6]. 4. BaseSettings Migration: BaseSettings was moved to a separate package, pydantic-settings, in V2 [3][8]. If you are migrating and encountering validation issues in your settings, note that while BaseSettings parses values from the environment as strings by default (similar to V1), complex types require JSON-encoded strings [9]. If your settings models rely on Literal constraints, you may face the same strict validation issues described above, as BaseSettings validation follows the same V2 rules as BaseModel [3][9]. For gradual migration from Pydantic V1 to V2, you can continue using the V1-compatible version of BaseSettings by importing it from pydantic.v1 while running Pydantic V2 [10].
Citations:
- 1: StrEnum isn't coerced into str when used with a Literal pydantic/pydantic#9276
- 2: https://pydantic.dev/docs/validation/latest/get-started/migration/
- 3: https://pydantic.dev/docs/validation/2.12/get-started/migration/
- 4: https://pydantic.dev/docs/validation/2.12/api/pydantic/standard_library_types/
- 5: (🐞)
LiteralwithEnums aren't validated consistently pydantic/pydantic#9968 - 6: Different behaviour when using Literal and Enum and StrEnum pydantic/pydantic#9791
- 7: Coercion of
LiteralwithStrEnumandstris not consistent pydantic/pydantic#9989 - 8: https://github.com/pydantic/pydantic/blob/main/docs/migration.md
- 9: https://pydantic.dev/docs/validation/2.12/concepts/pydantic_settings/
- 10: Is there a gradual upgrade path from Pydantic v1 to v2 for BaseSettings? pydantic/pydantic-settings#117
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Dependency hints:"
git ls-files | rg -n '(^|/)(pyproject\.toml|requirements.*\.txt|.*\.py)$' | rg -n 'pydantic|requirements|pyproject' || true
echo
echo "server imports:"
sed -n '1,80p' sidestep_engine/gui/server.py
echo
echo "Dependency/version references:"
rg -n "pydantic|require" pyproject.toml requirements*.txt 2>/dev/null || trueRepository: koda-dernet/Side-Step
Length of output: 2576
Constrain sidecar_mode to copy or empty.
file_ops.py only copies sidecars when the value is exactly "copy"; otherwise it writes blank .txt files. A mistyped API value can silently discard existing sidecar text, so reject unsupported values with a 422 instead of accepting arbitrary strings.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@sidestep_engine/gui/server.py` at line 107, Constrain the sidecar_mode
parameter in the server API to the supported values "copy" and "empty",
rejecting any other value with HTTP 422 before invoking file operations.
Preserve the existing default of "copy" and the behavior in file_ops.py for
valid values.
1 participant
Dataset & preprocessing UX: trigger tags, mix-from-folders, Preprocess All
Builds on the #52 Windows mix fix and extends the Audio Library workflow.
Trigger tags
tag_positionin the sidecar and honored during preprocessing.tag_positionadded to_KNOWN_SIDECAR_KEYS(was silently discarded by the sidecar parser).Mix datasets
sidecar_modeAPI param).Preprocess All
genre_ratio.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Updates