Skip to content

feat: auto-clean empty channel directories (#443)#459

Merged
dialmaster merged 1 commit intodevfrom
feat/443-feature-request-delete-directory-with-auto-cleanup-if-directory-is-empty
Mar 15, 2026
Merged

feat: auto-clean empty channel directories (#443)#459
dialmaster merged 1 commit intodevfrom
feat/443-feature-request-delete-directory-with-auto-cleanup-if-directory-is-empty

Conversation

@dialmaster
Copy link
Collaborator

  • Add CHANNEL_CLEANUP_IGNORABLE_FILES constant for files (poster.jpg) that don't count as real content when deciding if a directory is empty
  • Add isDirectoryEffectivelyEmpty() with case-insensitive matching for ignorable files
  • Enhance cleanupEmptyChannelDirectory() with includeIgnorableFiles option that deletes ignorable files before rmdir; returns boolean; defaults to false for backward compatibility
  • Add _tryCleanupChannelDirectory() to videoDeletionModule that runs best-effort after every successful deletion (nested and flat modes), with parent subfolder cleanup when channel dir is removed
  • Replace duplicate 30-line cleanup implementation in downloadExecutor with delegation to shared directoryManager function

- Add CHANNEL_CLEANUP_IGNORABLE_FILES constant for files (poster.jpg) that
  don't count as real content when deciding if a directory is empty
- Add isDirectoryEffectivelyEmpty() with case-insensitive matching for
  ignorable files
- Enhance cleanupEmptyChannelDirectory() with includeIgnorableFiles option
  that deletes ignorable files before rmdir; returns boolean; defaults to
  false for backward compatibility
- Add _tryCleanupChannelDirectory() to videoDeletionModule that runs
  best-effort after every successful deletion (nested and flat modes),
  with parent subfolder cleanup when channel dir is removed
- Replace duplicate 30-line cleanup implementation in downloadExecutor
  with delegation to shared directoryManager function
@claude
Copy link

claude bot commented Mar 15, 2026

Code Review: feat: auto-clean empty channel directories

Overall this is a well-structured PR. The consolidation of duplicate cleanup logic is a clear improvement, and the test coverage is thorough. A few observations:

Bug / Correctness: TOCTOU race in cleanupEmptyChannelDirectory with includeIgnorableFiles: true

When includeIgnorableFiles is true, the function does:

  1. isDirectoryEffectivelyEmpty — first readdir
  2. Second readdir to get the list of files to unlink

If a real file is added between steps 1 and 2 (e.g., by a concurrent download), the function will unlink it — deleting actual video content — and then successfully rmdir the now-empty directory. The unlink loop does not filter against CHANNEL_CLEANUP_IGNORABLE_FILES; it blindly deletes everything from the second readdir result.

The simplest fix: filter the entries in the unlink loop to only files in CHANNEL_CLEANUP_IGNORABLE_FILES (case-insensitively), consistent with how the emptiness check works. This prevents the data-loss risk.

Minor: Missing explicit null-guard in _tryCleanupChannelDirectory

When filePath is null, path.dirname(null) throws a TypeError which is silently caught by the outer try/catch. This works (the test covers it), but it relies on an error for control flow. An explicit early return would be clearer and avoids a spurious warn-level log in an expected code path.

Observation: cleanupEmptyParents uses strict emptiness

cleanupEmptyParents calls isDirectoryEmpty (not isDirectoryEffectivelyEmpty), so a subfolder parent containing only poster.jpg will not be cleaned up. Likely intentional since subfolder dirs are not channel dirs — just noting it in case full cleanup was the intent.

Positive notes

  • Deduplicating the 30-line implementation in downloadExecutor is the right call.
  • The boolean return value from cleanupEmptyChannelDirectory is a clean API improvement.
  • Backward compatibility is preserved (includeIgnorableFiles defaults to false).
  • Error handling is appropriately defensive (best-effort, no propagation).
  • Test coverage is comprehensive including the edge cases.

@github-actions
Copy link
Contributor

📊 Test Coverage Report

Backend Coverage

Type Coverage Status
Lines 79.72% 🟡
Statements 79.58% 🟡
Functions 80.72% 🟢
Branches 72.65% 🟡

Frontend Coverage

Type Coverage Status
Lines 87.40% 🟢
Statements 86.71% 🟢
Functions 80.45% 🟢
Branches 81.48% 🟢

Coverage Requirements

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

Coverage report generated for commit 26f16d8

@dialmaster dialmaster merged commit e140fd6 into dev Mar 15, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant