Initial AI handbrake implementation

OPUS_45_UPDATES.md

@@ -0,0 +1,316 @@
+# HandBrake Integration - Comprehensive Code Review & Proposed Updates
+
+**Branch:** `Handbrake` vs `master`  
+**Date:** November 27, 2025  
+**Review Model:** Claude Opus 4.5  
+**Files Changed:** 18 files, +2,128 lines, -36 lines
+
+---
+
+## Executive Summary
+
+This PR introduces **HandBrake post-processing integration** to the MakeMKV Auto Rip tool, allowing automatic compression of ripped MKV files to MP4/M4V format. The implementation is well-structured with proper error handling, security considerations, and retry logic. Test coverage meets the 80% threshold at **80.51%**.
+
+---
+
+## 1. Summary of Changes
+
+### 1.1 New Features
+
+| Feature | Files | Description |
+|---------|-------|-------------|
+| **HandBrake Service** | `src/services/handbrake.service.js` | 528-line service with validation, conversion, retry logic, and security sanitization |
+| **Config Integration** | `src/config/index.js` | New `handbrake` config getter with validation at startup |
+| **Constants** | `src/constants/index.js` | `HANDBRAKE_CONSTANTS` for presets, timeouts, file headers, retry limits |
+| **Config Validation** | `src/utils/handbrake-config.js` | Schema validation utility for HandBrake configuration |
+| **Filesystem Utils** | `src/utils/filesystem.js` | New `readdir()` and `unlink()` async methods |
+| **Rip Integration** | `src/services/rip.service.js` | HandBrake processing after successful rips, result tracking arrays |
+
+### 1.2 Test Coverage Additions
+
+| Test File | Tests | Coverage Target |
+|-----------|-------|-----------------|
+| `tests/unit/handbrake.service.test.js` | 12 | HandBrake service unit tests |
+| `tests/unit/handbrake-error.test.js` | 22 | HandBrakeError class & sanitization |
+| `tests/integration/handbrake-integration.test.js` | 4 | Integration with real filesystem |
+| `tests/unit/cli-commands.test.js` | 10 | CLI command coverage (0% → 80%) |
+| `tests/unit/rip.service.extended.test.js` | 21 | Extended rip service coverage (75% → 94.65%) |
+
+### 1.3 Documentation Updates
+
+- **README.md**: HandBrake configuration section, error handling documentation, troubleshooting table
+- **config.yaml**: Complete HandBrake configuration block with extensive comments
+
+---
+
+## 2. Clean Code & Architecture Review
+
+### 2.1 ✅ Strengths
+
+#### Separation of Concerns
+```
+HandBrakeService (conversion logic)
+    ↓ calls
+AppConfig.handbrake (configuration)
+    ↓ uses
+validateHandBrakeConfig (validation utility)
+    ↓ references
+HANDBRAKE_CONSTANTS (magic numbers extracted)
+```
+
+- **Single Responsibility**: `HandBrakeService` handles only conversion; `RipService` handles orchestration
+- **Configuration Centralized**: All HandBrake config flows through `AppConfig.handbrake` getter
+- **Constants Extracted**: No magic numbers in service code; all in `HANDBRAKE_CONSTANTS`
+
+#### Security Measures
+- **Path Sanitization**: `sanitizePath()` removes null bytes, control characters, detects path traversal
+- **Shell Injection Prevention**: `buildCommand()` validates additional_args for dangerous characters `[;&|`$()<>\n\r]`
+- **Conflicting Args Check**: Prevents user from overriding `--input`, `--output`, `--preset`
+
+#### Error Handling
+- **Custom Error Class**: `HandBrakeError` with `details` property for rich debugging
+- **Retry Logic**: 3-tier fallback preset system (1080p30 → 720p30 → 480p30)
+- **Cleanup on Failure**: Partial/corrupt output files automatically removed
+- **Timeout Management**: Dynamic timeout based on file size (min 2hr, max 12hr)
+
+### 2.2 ⚠️ Areas for Improvement
+
+#### 2.2.1 Duplicate Validation Logic
+```javascript
+// In AppConfig.validate():
+if (!['mp4', 'm4v'].includes(handbrakeConfig.output_format.toLowerCase())) { ... }
+
+// In HandBrakeService.validateConfig():
+if (!validFormats.includes(config.output_format.toLowerCase())) { ... }
+```
+**Issue**: Output format validation duplicated in two places.  
+**Recommendation**: Single source of truth in `validateHandBrakeConfig()`.
+
+#### 2.2.2 Verbose Logging in Production
+```javascript
+// rip.service.js - ~20 Logger.info() calls in handleRipCompletion
+Logger.info("Analyzing MakeMKV output for completion status...");
+Logger.info("Found relevant MakeMKV output lines:");
+relevantLines.forEach(line => Logger.info(`- ${line}`));
+Logger.info(`Rip completion check result: ${success ? 'successful' : 'failed'}`);
+Logger.info(`HandBrake enabled status: ${...}`);
+// ... and more
+```
+**Issue**: Excessive debug logging clutters console output.  
+**Recommendation**: Add log levels (DEBUG vs INFO) or a `verbose` config option.
+
+#### 2.2.3 Hardcoded Timeout Calculation
+```javascript
+// handbrake.service.js:434
+const timeoutMs = Math.max(
+  HANDBRAKE_CONSTANTS.MIN_TIMEOUT_HOURS * 60 * 60 * 1000,
+  Math.min(fileSizeGB * 60 * 1000, HANDBRAKE_CONSTANTS.MAX_TIMEOUT_HOURS * 60 * 60 * 1000)
+);
+```
+**Issue**: Timeout formula hardcoded; `* 60 * 1000` repeated.  
+**Recommendation**: Use `HANDBRAKE_CONSTANTS.TIMEOUT.MS_PER_MINUTE` already defined.
+
+#### 2.2.4 Regex Complexity for MakeMKV Output Parsing
+```javascript
+const outputMatch = stdout.match(/MSG:5014[^"]*"Saving \d+ titles into directory ([^"]*)"/) ||
+  stdout.match(/MSG:5014[^"]*"[^"]*","[^"]*","[^"]*","([^"]*)"/) ||
+  stdout.match(/Saving \d+ titles into directory ([^"\s]+)/);
+```
+**Issue**: Three fallback regex patterns are fragile; difficult to maintain.  
+**Recommendation**: Create a dedicated `MakeMKVParser` utility with explicit pattern handling.
+
+#### 2.2.5 Synchronous File Operations in Async Context
+```javascript
+// handbrake.service.js:339
+if (!fs.existsSync(outputPath)) { ... }
+const stats = fs.statSync(outputPath);
+const fd = fs.openSync(outputPath, 'r');
+fs.readSync(fd, buffer, 0, 1024, 0);
+fs.closeSync(fd);
+```
+**Issue**: Mixing sync/async file operations; blocks event loop during validation.  
+**Recommendation**: Convert to fully async (`fs.promises.*`) for consistency.
+
+---
+
+## 3. Test Coverage Analysis
+
+### 3.1 Current Coverage
+| Metric | Value | Target | Status |
+|--------|-------|--------|--------|
+| Overall Statements | 80.51% | ≥80% | ✅ Pass |
+| Overall Branches | 83.87% | ≥80% | ✅ Pass |
+| Overall Functions | 91.00% | ≥80% | ✅ Pass |
+| Overall Lines | 80.51% | ≥80% | ✅ Pass |
+
+### 3.2 Module-Specific Coverage
+
+| Module | Before | After | Change |
+|--------|--------|-------|--------|
+| `commands.js` | 0% | 80% | +80% ✅ |
+| `rip.service.js` | 75% | 94.65% | +19.65% ✅ |
+| `handbrake.service.js` | N/A | 65.56% | New ⚠️ |
+| `api.routes.js` | 15% | 15% | No change ⚠️ |
+
+### 3.3 ⚠️ Coverage Gaps
+
+#### 3.3.1 `handbrake.service.js` at 65.56%
+**Uncovered Lines**: 277-295, 309-320, 417-433, 464-477, 493-512, 514-524
+
+Missing test coverage for:
+- `retryConversion()` success path with fallback presets
+- `parseHandBrakeOutput()` progress/warning extraction
+- `convertFile()` success path with actual execution
+- Timeout handling code path
+- Cleanup logic on partial failures
+
+#### 3.3.2 `api.routes.js` at 15%
+**Status**: Intentionally deprioritized due to stateful module-level variables requiring significant refactoring.
+
+### 3.4 Test Quality Assessment
+
+| Aspect | Rating | Notes |
+|--------|--------|-------|
+| **Mock Isolation** | ⭐⭐⭐⭐⭐ | Proper `vi.clearAllMocks()` / `vi.restoreAllMocks()` |
+| **Edge Cases** | ⭐⭐⭐⭐ | Good error/failure path coverage |
+| **Integration Tests** | ⭐⭐⭐ | Limited to filesystem; no actual HandBrake execution |
+| **Security Tests** | ⭐⭐⭐⭐⭐ | Path traversal, shell injection well covered |
+| **Async Handling** | ⭐⭐⭐⭐ | Proper async/await in all test cases |
+
+---
+
+## 4. Meeting User Needs
+
+### 4.1 ✅ User Requirements Met
+
+| Requirement | Status | Implementation |
+|-------------|--------|----------------|
+| Enable/disable HandBrake | ✅ | `handbrake.enabled` config flag |
+| Auto-detect HandBrakeCLI | ✅ | Platform-specific path scanning |
+| Custom CLI path | ✅ | `handbrake.cli_path` option |
+| Preset selection | ✅ | `handbrake.preset` with validation |
+| Output format choice | ✅ | MP4/M4V support |
+| Delete original option | ✅ | `handbrake.delete_original` flag |
+| Additional args | ✅ | `handbrake.additional_args` with security validation |
+| Error recovery | ✅ | 3-tier fallback preset retry |
+| Progress feedback | ✅ | Console logging of conversion status |
+
+### 4.2 ⚠️ Potential User Friction Points
+
+#### 4.2.1 No Progress Bar
+Users converting large files (50GB+) see only periodic log messages. A progress percentage would improve UX.
+
+#### 4.2.2 CLI vs GUI Confusion
+Documentation explains HandBrakeCLI is separate from GUI, but users may still download wrong package.
+
+#### 4.2.3 No Preset Listing
+Users must know valid HandBrake presets; no `--list-presets` equivalent exposed.
+
+#### 4.2.4 No Queue Visualization
+When processing multiple discs with HandBrake, users can't see what's queued vs completed.
+
+---
+
+## 5. Speed & Ease of Use Concerns
+
+### 5.1 Performance Considerations
+
+| Concern | Current State | Impact |
+|---------|---------------|--------|
+| **Timeout Calculation** | Dynamic based on file size | ✅ Good |
+| **Buffer Size** | 10MB for stdout/stderr | ✅ Adequate |
+| **Sync File Operations** | Used in `validateOutput()` | ⚠️ Blocks event loop |
+| **Sequential HandBrake Processing** | One file at a time | ⚠️ Could parallelize for multi-disc |
+| **Retry Overhead** | Up to 3 full re-encodes on failure | ⚠️ Potentially hours of extra time |
+
+### 5.2 Startup Validation Overhead
+```javascript
+// AppConfig.validate() now also validates HandBrake
+if (config.handbrake?.enabled) {
+  // Validates format, preset, and checks cli_path exists
+}
+```
+**Impact**: Minimal (~50ms extra) but could fail startup if HandBrake path is temporarily unavailable.
+
+### 5.3 Memory Usage
+HandBrake conversion of large files (50GB+) combined with 10MB stdout buffer could cause memory pressure on low-memory systems.
+
+---
+
+## 6. Proposed Updates
+
+### 6.1 High Priority (Should Do Before Merge)
+
+| # | Update | Effort | Rationale |
+|---|--------|--------|-----------|
+| 1 | **Add log verbosity control** | 2hr | Reduce console clutter; add `handbrake.verbose` option |
+| 2 | **Convert sync file ops to async** | 1hr | `validateOutput()` uses sync I/O in async function |
+| 3 | **Consolidate validation logic** | 1hr | Remove duplicate format validation |
+| 4 | **Increase handbrake.service.js coverage to 75%+** | 3hr | Cover retry success path and timeout handling |
+
+### 6.2 Medium Priority (Should Do Soon)
+
+| # | Update | Effort | Rationale |
+|---|--------|--------|-----------|
+| 5 | **Add progress percentage parsing** | 2hr | Parse HandBrake's `Encoding: task X of Y, Y.YY %` output |
+| 6 | **Create MakeMKVParser utility** | 2hr | Centralize regex patterns for MSG parsing |
+| 7 | **Add `--list-presets` command** | 1hr | Help users discover valid presets |
+| 8 | **Add HandBrake queue status to web UI** | 4hr | Show pending/completed conversions |
+
+### 6.3 Low Priority (Nice to Have)
+
+| # | Update | Effort | Rationale |
+|---|--------|--------|-----------|
+| 9 | **Parallel HandBrake processing** | 4hr | Process multiple files concurrently (with CPU limit) |
+| 10 | **Add estimated time remaining** | 2hr | Based on progress percentage and elapsed time |
+| 11 | **Hardware acceleration detection** | 3hr | Auto-detect NVENC/QSV/VCE and adjust presets |
+| 12 | **Pre-flight HandBrake test** | 1hr | Quick test encode of 1 second to verify setup |
+| 13 | **Refactor api.routes.js for testability** | 6hr | Extract state into injectable service |
+
+### 6.4 Documentation Updates
+
+| # | Update | Effort | Rationale |
+|---|--------|--------|-----------|
+| 14 | **Add video walkthrough** | 2hr | Show complete setup flow |
+| 15 | **Add troubleshooting flowchart** | 1hr | Visual decision tree for common errors |
+| 16 | **Document preset benchmarks** | 2hr | Speed/quality tradeoffs for common presets |
+
+---
+
+## 7. Implementation Priority Matrix
+
+```
+                    IMPACT
+                    High    Medium    Low
+              ┌─────────┬─────────┬─────────┐
+         High │  1, 4   │  5, 7   │  10     │
+   EFFORT     ├─────────┼─────────┼─────────┤
+       Medium │  2, 3   │  6, 8   │  9, 11  │
+              ├─────────┼─────────┼─────────┤
+          Low │         │  12     │  14-16  │
+              └─────────┴─────────┴─────────┘
+
+Recommended Order: 1 → 2 → 3 → 4 → 5 → 7 → 6 → 8 → 12 → rest
+```
+
+---
+
+## 8. Conclusion
+
+The HandBrake integration is **production-ready** with the following caveats:
+
+| Aspect | Status |
+|--------|--------|
+| Core Functionality | ✅ Complete |
+| Error Handling | ✅ Robust |
+| Security | ✅ Well-considered |
+| Test Coverage | ✅ Meets 80% threshold |
+| Documentation | ✅ Comprehensive |
+| Code Quality | ⚠️ Minor improvements needed |
+| User Experience | ⚠️ Progress feedback could improve |
+
+**Recommendation**: Merge after addressing items 1-4 from High Priority list (estimated 7 hours total).
+
+---
+
+*Generated by Claude Opus 4.5 code review on November 27, 2025*