feat: channel flat file structure option (#258)

[dialmaster]

• Add "Flat file structure (no video subfolders)" toggle to channel settings dialog and manual download settings dialog
• Add skip_video_folder column to channels table via migration
• Update yt-dlp command builder to omit video subfolder level in output/thumbnail paths when enabled
• Update post-processor to move individual files instead of entire directories in flat mode
• Update video deletion to remove matching files individually instead of deleting the parent directory
• Update download executor cleanup to handle flat-mode in-progress files
• Plumb skipVideoFolder through channel download grouper, download module, and API route with validation
• Add skipVideoFolder to the manual download API override settings
• Document flat file structure in folder structure, usage guide, and API integration docs
• Add tests for channel settings toggle, download settings toggle, download module, and deletion module

[github-actions]

📊 Test Coverage Report

Backend Coverage

Type	Coverage	Status
Lines	79.58%	🟡
Statements	79.45%	🟡
Functions	80.70%	🟢
Branches	72.57%	🟡

Frontend Coverage

Type	Coverage	Status
Lines	87.23%	🟢
Statements	86.52%	🟢
Functions	80.47%	🟢
Branches	81.50%	🟢

Coverage Requirements

• Minimum threshold: 70% line coverage
• Backend: ✅ Passes
• Frontend: ✅ Passes

---

Coverage report generated for commit c2408fc

[claude]

Code Review: feat: channel flat file structure option (#258)

Overall this is a well-structured feature addition with good test coverage and documentation. The implementation is mostly clean, but there are a few issues worth addressing.

---

Issues

1. Potential race condition in flat-mode post-processor file movement (videoDownloadPostProcessFiles.js)

In flat mode, the post-processor iterates over all files currently in the temp channel folder and moves them:

const updatedFilesInDir = await fs.readdir(videoDirectory); // reads the entire temp channel folder
for (const file of updatedFilesInDir) {
  await moveWithRetries(srcPath, destPath, ...);
}

In flat mode, videoDirectory is the shared temp channel folder (e.g., /tmp/youtarr/Channel Name/), not an isolated video-scoped subfolder. If yt-dlp downloads multiple videos into this folder before the post-processor runs for each one (e.g., if a channel folder already has partially staged files from a prior incomplete run, or if concurrent groups are downloading the same channel), all those files will be moved on the first post-processor invocation, leaving nothing for subsequent invocations.

In the nested (default) case this is safe because each video gets its own subfolder. Consider moving only files matching the current video's ID to mirror the safer pattern used in the deletion and cleanup paths:

const youtubeId = jsonData.id; // or however the ID is available in this context
for (const file of updatedFilesInDir) {
  if (file.includes(`[${youtubeId}]`)) {
    await moveWithRetries(path.join(videoDirectory, file), path.join(targetVideoDirectory, file), ...);
  }
}

2. JSDoc parameter mismatch in videoDeletionModule.js#isFlat()

The JSDoc lists @param {string} youtubeId but the method signature only accepts filePath:

/**
 * @param {string} filePath - Full path to the video file
 * @param {string} youtubeId - YouTube video ID    <-- stale, should be removed
 */
isFlat(filePath) {

3. Ambiguous toggle-off semantics in ChannelSettingsDialog.tsx

Toggling the switch off sends null rather than false:

skip_video_folder: e.target.checked ? true : null

If the intent is that null = "inherit default" and false = "explicitly disabled", this is fine as documented, but it means the UI provides no way to set an explicit false after the setting was previously true. If null and false are semantically identical (both meaning "use nested structure"), the model and migration comment should reflect that, and the UI label might benefit from a note.

4. Inconsistent loading pattern for skip_video_folder vs other settings

Other settings use Object.prototype.hasOwnProperty.call() for missing-key detection, but skip_video_folder uses ?? null:

// existing pattern:
default_rating: Object.prototype.hasOwnProperty.call(settingsData, 'default_rating')
  ? settingsData.default_rating : null,

// new:
skip_video_folder: settingsData.skip_video_folder ?? null,

The ?? null form silently swallows a server-returned false (preserving it correctly) but would also hide a missing key from an older server version, treating it as null. The inconsistency is minor since the migration ensures the column is always present, but aligning with the established pattern improves consistency and resilience.

5. Secondary filename match pattern without brackets is overly broad

In both downloadExecutor.js and videoDeletionModule.js:

if (fileName.includes(`[${youtubeId}]`) || fileName.includes(` - ${youtubeId}`))

The bracketed form [abc123XYZ_d] is what yt-dlp actually produces. The second condition (without brackets) appears to be defensive dead code. YouTube IDs are 11 chars of [A-Za-z0-9_-], making accidental matches rare, but the intent isn't documented. Consider removing it or adding a comment explaining the scenario it guards against.

---

Minor Notes

• hasFilters() behavioral change: Adding skipVideoFolder to ChannelFilterConfig#hasFilters() is architecturally correct (channels with differing flat/nested settings must be in separate download groups), but the method's original intent was duration/regex filtering. A short comment explaining why skipVideoFolder affects grouping would help future readers.

• ChannelFilterConfig#buildFilterKey() abbreviation: "skipVF" is compact but opaque. Since this is a serialized cache key and not user-facing, it's fine—just noting it.

• DB column comment missing: The migration and model add skip_video_folder without a comment field, unlike default_rating which has comment: 'Default rating to apply to unrated videos in this channel'. Low priority but keeps the schema self-documenting.

---

Positives

• Clean separation of concerns: the setting is plumbed correctly through the DB model → grouper → command builder → post-processor → deletion module chain.
• Good test coverage across all layers (frontend component tests, download module tests, deletion module tests, executor tests).
• The safety check in videoDeletionModule.js correctly shifts from directory-path containment to file-path containment, which is the right check for flat mode.
• Documentation is clear and the folder structure diagrams accurately represent both modes.
• The migration uses allowNull: true, defaultValue: null so existing channels are unaffected by default.

[claude]

PR Review: feat: channel flat file structure option (#258)

Overall this is a well-structured feature with good test coverage. The end-to-end plumbing (DB migration → model → settings module → download builder → post-processor → deletion) is consistent and the new toggle is validated at the API boundary. A few issues worth addressing before merge:

---

Bugs / Correctness

1. JSDoc mismatch in videoDeletionModule.js — isFlat()

The JSDoc documents a second youtubeId parameter that does not exist in the function signature:

// JSDoc says: @param {string} youtubeId - YouTube video ID
isFlat(filePath) {   // <-- only one param

Minor, but it will confuse future readers.

---

2. Heuristic flat-mode detection is fragile in videoDeletionModule.js

isFlat() infers the storage mode from whether the parent directory looks like a video directory:

isFlat(filePath) {
  const parentDir = path.dirname(filePath);
  return !isVideoDirectory(parentDir);
}

This relies on a naming convention that could fail in edge cases:

• A video downloaded in nested mode whose directory was manually renamed after the fact would be deleted file-by-file instead of directory-by-directory.
• A video whose file_path in the DB points directly at a channel folder (e.g. after a failed move) would incorrectly be treated as flat mode.

A safer approach would be to store the mode on the Video DB record, or pass it explicitly through the deletion call chain.

---

3. Stale skipVideoFolder state survives disabling custom settings in DownloadSettingsDialog.tsx

The hasOverride calculation includes skipVideoFolder independently of useCustomSettings:

const hasOverride = useCustomSettings || allowRedownload ||
  (mode === 'manual' && subfolderOverride !== null) ||
  (mode === 'manual' && audioFormat !== null) ||
  (mode === 'manual' && skipVideoFolder);   // <-- always active once toggled

The test "triggers override when only skipVideoFolder is toggled (no custom settings)" explicitly tests this behavior, but the UX may surprise users: toggling flat mode on, then disabling custom settings, still sends skipVideoFolder: true. Consider resetting skipVideoFolder when useCustomSettings is disabled, or ensuring the toggle is hidden/greyed-out when custom settings are off.

---

4. File-matching pattern in flat-mode cleanup is slightly over-broad

Both downloadExecutor.js and videoDeletionModule.js match files using:

fileName.includes(`[${youtubeId}]`) || fileName.includes(` - ${youtubeId}`)

The second pattern (' - ' + youtubeId) is not part of the documented file naming convention (Channel - Title [id].ext). Unless there is a known format that produces Channel - youtubeId.ext (without brackets), this branch looks like dead code that could, in theory, match unintended files if a title or channel name happens to contain the YouTube ID. Removing the second pattern (or documenting when it is needed) would make the intent clearer.

---

Code Quality / Design

5. Null vs. false inconsistency between channel and manual-download settings

• Channel settings: toggle-off writes skip_video_folder: null (meaning "inherit default")
• Manual download: toggle-off leaves skipVideoFolder: false

This is functionally correct (null and false both mean "don't skip"), but the trichotomy (null / false / true) on the channel side vs. the boolean on the manual side makes the semantics harder to reason about at a glance. A comment in ChannelSettingsDialog.tsx explaining that null represents "inherit from default" would help.

6. hasFilters() name is misleading for skipVideoFolder

skipVideoFolder is not a filter in the traditional sense — it does not exclude videos, it changes the output path. The method now returns true for channels that only have flat mode enabled, which forces them into their own download group. That grouping is necessary for correctness (different yt-dlp --output paths), but the name hasFilters does not communicate this. A comment at the skipVideoFolder branch would clarify why it needs to influence grouping.

---

Minor / Nitpicks

• validateSkipVideoFolder in channelSettingsModule.js accepts undefined as valid, but the caller already guards with !== undefined before calling it. The function could be simplified to only validate true / false / null.
• videoDownloadPostProcessFiles.js cleans up the temp channel folder with cleanupEmptyParents(videoDirectory, ...) in flat mode. If two simultaneous flat-mode downloads run for the same channel, one cleanup might remove the temp channel folder while the other is still writing to it. This is an unlikely race condition but worth a note.
• The new flat-mode log line in downloadExecutor.js is added inside a branch that previously used to continue without setting cleanedAny = true. The fix to set cleanedAny = true and call destroy() here looks correct — the previous behavior of silently skipping these entries appears to have been a pre-existing bug, and it is good that this PR addresses it.

---

Summary

Severity	Count
Medium (correctness/fragility)	3 (items 2, 3, 4)
Low (code quality/clarity)	3 (items 1, 5, 6)
Nit	3

The feature logic is sound and test coverage is solid. The main concern is the heuristic flat-mode detection in deletion (item 2) — if a video's file_path DB record ever diverges from the actual on-disk layout, the wrong deletion path will be taken silently. The other items are lower risk but worth a look before merge.

[claude]

PR Review: Channel Flat File Structure Option

Overall this is a well-structured feature with good test coverage across both frontend and backend. The approach of plumbing skipVideoFolder through the entire stack is correct. A few issues worth addressing before merge:

---

Bug: File move filter inconsistency in post-processor

In videoDownloadPostProcessFiles.js (flat mode file move), files are filtered by the bracketed form only. But in both videoDeletionModule.js and downloadExecutor.js, the matching pattern also includes a dash fallback.

The post-processor only handles the bracketed form [ID], skipping the dash fallback - ID. If yt-dlp ever uses the dash naming pattern, those files would not be moved from temp to final in flat mode but could still be matched by the deletion code. The filter should be consistent across all three locations.

---

Fragile: isFlat() heuristic in videoDeletionModule

The isFlat() method infers flat vs. nested mode from the directory naming convention at deletion time. If skip_video_folder was changed on a channel after videos were downloaded (e.g., switched from flat to nested or vice versa), deletion will use the wrong strategy for existing records. Deleting the wrong set of files (or not deleting the right ones) is a significant correctness risk.

Consider querying the video channel skip_video_folder setting from the database, or storing skip_video_folder on the Video record at download time. The heuristic approach also relies on the assumption that channel folder names never match the isVideoDirectory pattern, which may not hold universally.

---

UX: Hidden skipVideoFolder state in DownloadSettingsDialog

When a user enables custom settings and turns on the flat file structure toggle, then disables custom settings (which hides the toggle), and confirms the download, the download still uses skipVideoFolder: true (verified by the test triggers override when only skipVideoFolder is toggled). The state persists invisibly. Users who disable custom settings would reasonably expect all custom options to be cleared. At minimum this should be called out in the UI, or skipVideoFolder should be reset when useCustomSettings is toggled off.

---

Potential race condition: concurrent flat-mode downloads for the same channel

In videoDownloadPostProcessFiles.js flat mode, files are moved individually from the temp channel directory to the final channel directory. If two downloads run concurrently for the same channel in flat mode, both post-processors will readdir the same temp directory and filter by their own [id]. This filtering should prevent them from moving each other's files, but there is still a window where one process could read files that the other is in the process of moving. The file-by-file move (vs. atomic directory move in nested mode) loses the atomicity guarantee. Worth documenting as a known caveat at minimum.

---

Minor: hasActiveFilters() naming is slightly misleading

skipVideoFolder is a file storage structure setting, not a filter. It has been added to ChannelFilterConfig.hasActiveFilters() so that channels with different structural settings get separate download groups, which is correct behavior, but the method name no longer accurately reflects its purpose. Consider requiresOwnGroup() or adding a comment explaining the broader intent.

---

Minor: Model definition missing comment

The migration adds a descriptive comment to the column (When true, videos are stored directly in the channel folder without per-video subfolders) but the corresponding field in server/models/channel.js omits it. For consistency with other fields in that model that have comments, it should be included.

---

Minor: Formatting inconsistency in ChannelSettingsDialog.tsx

The skip_video_folder hasOwnProperty call has the null fallback on the same line as the ternary, while other settings in the same block have the null on its own line. Small style nit but inconsistent with the surrounding code.

---

What is done well

• The grouper correctly separates channels with different skipVideoFolder settings, preventing output path collisions
• The DB migration and model are clean; using nullable boolean (vs. non-null false) is consistent with other optional settings
• API-layer validation for skipVideoFolder in both the route handler and channelSettingsModule is thorough
• Test coverage is solid across all layers, particularly the deletion and cleanup behavior
• Documentation (folder structure, usage guide, API integration) is clear and complete
• The safety check change from validating the directory path to validating the file path is a correct improvement for flat mode

[claude]

PR Review: Channel Flat File Structure Option

Overall this is a well-scoped feature with good test coverage and consistent patterns throughout the stack. A few issues worth addressing before merge.

---

Bug: videoDownload.destroy() not called in flat-mode cleanup

In downloadExecutor.js, the flat-mode cleanup path ends with cleanedAny = true; continue;. The continue skips the destroy() call that would normally be at the bottom of the loop iteration, yet the test asserts expect(mockVideoDownload.destroy).toHaveBeenCalled(). Either:

1. The destroy() call was added inside the flat-mode block but is missing from the diff, or
2. The implementation and the test are mismatched — destroy() never executes in flat mode, leaving stale JobVideoDownload tracking records after cleanup.

This needs verification. If destroy() is omitted in the flat-mode path, in-progress tracking entries will accumulate in the DB.

---

Concern: Heuristic-based flat detection in videoDeletionModule.isFlat()

isFlat(filePath) {
  const parentDir = path.dirname(filePath);
  return !isVideoDirectory(parentDir);
}

This infers the structure from the path naming convention rather than from the channel's skip_video_folder DB setting. This can misfire in edge cases:

• A video stored in nested mode whose directory doesn't match the expected channel - title [id] pattern would be treated as flat, causing the deletion to attempt scanning the channel folder instead of removing a directory.
• A channel with skip_video_folder = false that somehow ends up with a flat-looking path could corrupt the channel folder.

Strongly prefer looking up the channel's skip_video_folder from the DB at deletion time (the Video record's associated channel is available), rather than inferring structure from the file path heuristic.

---

Minor: null vs false semantics for disabling the toggle

In ChannelSettingsDialog.tsx:

skip_video_folder: e.target.checked ? true : null

Unchecking the toggle sends null rather than false. Both are treated as "off" (the DB column allows null with default null), but it means there is no way to explicitly store false to distinguish "user explicitly disabled" from "never configured". If that distinction is intentional (null = inherit global default someday), it should be documented. If not, using false on uncheck would be cleaner and consistent with the DownloadSettingsDialog which uses a plain boolean.

---

Minor: doDownload() parameter count

doDownload() now takes 10 positional parameters:

async doDownload(args, jobId, jobType, urlCount, originalUrls, allowRedownload, skipJobTransition, subfolderOverride, ratingOverride, skipVideoFolder)

This is pre-existing style, but adding another parameter makes call sites error-prone. Consider refactoring the trailing optional parameters into an options object at some point — this PR could be a natural opportunity since it's already touching all call sites.

---

Positive observations

• Correct grouping logic: Including skipVideoFolder in buildFilterKey() and hasGroupingCriteria() ensures channels with different flat-file settings are placed in separate download groups. This is essential for correctness.
• hasFilters() → hasGroupingCriteria() rename: More accurate now that the method covers structural settings, not just content filters.
• Concurrent-download safety: The flat-mode file moving code correctly filters by [id] before touching files, avoiding collisions with other in-progress downloads sharing the same channel folder.
• Safety check update: Changing the safety check from directory path to file path is correct for flat mode.
• Test coverage: New tests cover toggle rendering, save payload, server round-trip, reset on close, and all override/priority scenarios.
• Documentation: Folder structure doc and usage guide updated clearly.

---

Summary

The main blocking issue is the potential destroy() omission in the flat-mode executor cleanup path. The heuristic isFlat() detection is a design concern worth resolving before this goes to production. Everything else is minor polish.

[dialmaster]

Note, the callout from Claude about Bug: videoDownload.destroy() not called in flat-mode cleanup is NOT correct:
The flat-mode path sets cleanedAny = true then continue. This skips the rest of the inner for loop (the nested-mode cleanup logic on lines 177-200), not the outer logic.
After the for loop completes, if (cleanedAny) calls videoDownload.destroy().