Merge pull request #459 from DialmasterOrg/feat/443-feature-request-delete-directory-with-auto-cleanup-if-directory-is-empty

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

Author: dialmaster

server/modules/__tests__/videoDeletionModule.test.js

@@ -39,10 +39,17 @@ describe('VideoDeletionModule', () => {
       promises: mockFs
     }));
 
-    // Mock the filesystem module (isVideoDirectory)
+    // Mock the filesystem module (isVideoDirectory, cleanupEmptyChannelDirectory, cleanupEmptyParents)
     // Default to true (nested structure) for backwards compatibility with existing tests
     jest.doMock('../filesystem', () => ({
-      isVideoDirectory: jest.fn(() => true)
+      isVideoDirectory: jest.fn(() => true),
+      cleanupEmptyChannelDirectory: jest.fn().mockResolvedValue(false),
+      cleanupEmptyParents: jest.fn().mockResolvedValue()
+    }));
+
+    // Mock configModule for _tryCleanupChannelDirectory
+    jest.doMock('../configModule', () => ({
+      directoryPath: '/test/output'
     }));
 
     // Require the module after mocks are in place
@@ -270,6 +277,158 @@ describe('VideoDeletionModule', () => {
     });
   });
 
+  describe('_tryCleanupChannelDirectory', () => {
+    let mockFilesystem;
+
+    beforeEach(() => {
+      mockFilesystem = require('../filesystem');
+    });
+
+    test('should call cleanupEmptyChannelDirectory with grandparent path for nested deletion', async () => {
+      const mockVideoRecord = {
+        id: 1,
+        youtubeId: 'abc123',
+        filePath: '/test/output/Channel Name/Channel Name - Video Title - abc123/video.mp4',
+        removed: false,
+        update: jest.fn().mockResolvedValue()
+      };
+
+      mockVideo.findByPk.mockResolvedValue(mockVideoRecord);
+      mockFs.rm.mockResolvedValue();
+
+      await VideoDeletionModule.deleteVideoById(1);
+
+      expect(mockFilesystem.cleanupEmptyChannelDirectory).toHaveBeenCalledWith(
+        '/test/output/Channel Name',
+        '/test/output',
+        { includeIgnorableFiles: true }
+      );
+    });
+
+    test('should call cleanupEmptyChannelDirectory with parent path for flat deletion', async () => {
+      // Override isVideoDirectory to return false for flat mode
+      mockFilesystem.isVideoDirectory.mockReturnValue(false);
+
+      const mockVideoRecord = {
+        id: 1,
+        youtubeId: 'abc123',
+        filePath: '/test/output/Channel Name/Channel Name - Video Title [abc123].mp4',
+        removed: false,
+        update: jest.fn().mockResolvedValue()
+      };
+
+      mockVideo.findByPk.mockResolvedValue(mockVideoRecord);
+      mockFs.readdir.mockResolvedValue([]);
+
+      await VideoDeletionModule.deleteVideoById(1);
+
+      expect(mockFilesystem.cleanupEmptyChannelDirectory).toHaveBeenCalledWith(
+        '/test/output/Channel Name',
+        '/test/output',
+        { includeIgnorableFiles: true }
+      );
+    });
+
+    test('should NOT call cleanup when video has no filePath', async () => {
+      const mockVideoRecord = {
+        id: 1,
+        youtubeId: 'abc123',
+        filePath: null,
+        removed: false,
+        update: jest.fn().mockResolvedValue()
+      };
+
+      mockVideo.findByPk.mockResolvedValue(mockVideoRecord);
+
+      await VideoDeletionModule.deleteVideoById(1);
+
+      expect(mockFilesystem.cleanupEmptyChannelDirectory).not.toHaveBeenCalled();
+    });
+
+    test('should NOT call cleanup when file deletion fails', async () => {
+      const mockVideoRecord = {
+        id: 1,
+        youtubeId: 'abc123',
+        filePath: '/test/output/Channel/Channel - Video - abc123/video.mp4',
+        removed: false
+      };
+
+      mockVideo.findByPk.mockResolvedValue(mockVideoRecord);
+
+      const permissionError = new Error('EACCES');
+      permissionError.code = 'EACCES';
+      mockFs.rm.mockRejectedValue(permissionError);
+
+      await VideoDeletionModule.deleteVideoById(1);
+
+      expect(mockFilesystem.cleanupEmptyChannelDirectory).not.toHaveBeenCalled();
+    });
+
+    test('should not affect success response when cleanup throws', async () => {
+      mockFilesystem.cleanupEmptyChannelDirectory.mockRejectedValueOnce(new Error('cleanup failed'));
+
+      const mockVideoRecord = {
+        id: 1,
+        youtubeId: 'abc123',
+        filePath: '/test/output/Channel Name/Channel Name - Video Title - abc123/video.mp4',
+        removed: false,
+        update: jest.fn().mockResolvedValue()
+      };
+
+      mockVideo.findByPk.mockResolvedValue(mockVideoRecord);
+      mockFs.rm.mockResolvedValue();
+
+      const result = await VideoDeletionModule.deleteVideoById(1);
+
+      expect(result.success).toBe(true);
+      expect(mockLogger.warn).toHaveBeenCalledWith(
+        expect.objectContaining({ err: expect.any(Error) }),
+        expect.stringContaining('non-fatal')
+      );
+    });
+
+    test('should call cleanupEmptyParents when channel dir was removed', async () => {
+      mockFilesystem.cleanupEmptyChannelDirectory.mockResolvedValueOnce(true);
+
+      const mockVideoRecord = {
+        id: 1,
+        youtubeId: 'abc123',
+        filePath: '/test/output/__subfolder/Channel Name/Channel Name - Video Title - abc123/video.mp4',
+        removed: false,
+        update: jest.fn().mockResolvedValue()
+      };
+
+      mockVideo.findByPk.mockResolvedValue(mockVideoRecord);
+      mockFs.rm.mockResolvedValue();
+
+      await VideoDeletionModule.deleteVideoById(1);
+
+      expect(mockFilesystem.cleanupEmptyParents).toHaveBeenCalledWith(
+        '/test/output/__subfolder',
+        '/test/output'
+      );
+    });
+
+    test('should NOT call cleanupEmptyParents when channel dir was not removed', async () => {
+      mockFilesystem.cleanupEmptyChannelDirectory.mockResolvedValueOnce(false);
+
+      const mockVideoRecord = {
+        id: 1,
+        youtubeId: 'abc123',
+        filePath: '/test/output/Channel Name/Channel Name - Video Title - abc123/video.mp4',
+        removed: false,
+        update: jest.fn().mockResolvedValue()
+      };
+
+      mockVideo.findByPk.mockResolvedValue(mockVideoRecord);
+      mockFs.rm.mockResolvedValue();
+
+      await VideoDeletionModule.deleteVideoById(1);
+
+      expect(mockFilesystem.cleanupEmptyParents).not.toHaveBeenCalled();
+    });
+  });
+
   describe('deleteVideos', () => {
     test('should successfully delete multiple videos', async () => {
       const mockVideo1 = {

server/modules/download/__tests__/downloadExecutor.test.js

@@ -86,7 +86,8 @@ jest.mock('../../filesystem', () => ({
   isMainVideoFile: jest.fn(),
   isVideoDirectory: jest.fn(),
   isChannelDirectory: jest.fn(),
-  isDirectoryEmpty: jest.fn()
+  isDirectoryEmpty: jest.fn(),
+  cleanupEmptyChannelDirectory: jest.fn().mockResolvedValue(false)
 }));
 
 const DownloadExecutor = require('../downloadExecutor');

server/modules/download/downloadExecutor.js

@@ -69,38 +69,9 @@ class DownloadExecutor {
   }
 
   // Helper function to remove a channel directory if it's empty
-  // This is called after removing a video directory to clean up empty channel folders
+  // Delegates to the shared directoryManager implementation
   async cleanupEmptyChannelDirectory(channelDir) {
-    const fsPromises = require('fs').promises;
-
-    try {
-      // Verify this is actually a channel directory (not root or subfolder)
-      if (!filesystem.isChannelDirectory(channelDir, configModule.directoryPath)) {
-        logger.debug({ channelDir }, 'Not a channel directory, skipping cleanup');
-        return;
-      }
-
-      // Check if directory exists
-      const exists = await fsPromises.access(channelDir).then(() => true).catch(() => false);
-      if (!exists) {
-        logger.debug({ channelDir }, 'Channel directory does not exist');
-        return;
-      }
-
-      // Check if directory is empty
-      const isEmpty = await filesystem.isDirectoryEmpty(channelDir);
-      if (!isEmpty) {
-        logger.debug({ channelDir }, 'Channel directory not empty, keeping it');
-        return;
-      }
-
-      // Remove empty channel directory
-      await fsPromises.rmdir(channelDir);
-      logger.info({ channelDir }, 'Removed empty channel directory');
-    } catch (error) {
-      logger.error({ err: error, channelDir }, 'Error cleaning up empty channel directory');
-      // Don't throw - this is a best-effort cleanup
-    }
+    await filesystem.cleanupEmptyChannelDirectory(channelDir, configModule.directoryPath);
   }
 
   // Cleanup function for in-progress videos based on database tracking

server/modules/filesystem/__tests__/constants.test.js

@@ -9,7 +9,8 @@ const {
   YOUTUBE_ID_DASH_PATTERN,
   YOUTUBE_ID_PATTERN,
   MAIN_VIDEO_FILE_PATTERN,
-  FRAGMENT_FILE_PATTERN
+  FRAGMENT_FILE_PATTERN,
+  CHANNEL_CLEANUP_IGNORABLE_FILES
 } = require('../constants');
 
 describe('filesystem/constants', () => {
@@ -136,4 +137,14 @@ describe('filesystem/constants', () => {
       expect(FRAGMENT_FILE_PATTERN.test('Channel - Title [dQw4w9WgXcQ].mp4')).toBe(false);
     });
   });
+
+  describe('CHANNEL_CLEANUP_IGNORABLE_FILES', () => {
+    it('should exist and be an array', () => {
+      expect(Array.isArray(CHANNEL_CLEANUP_IGNORABLE_FILES)).toBe(true);
+    });
+
+    it('should contain poster.jpg', () => {
+      expect(CHANNEL_CLEANUP_IGNORABLE_FILES).toContain('poster.jpg');
+    });
+  });
 });