Fix cleanup action not working on directories #3664
Conversation
📝 WalkthroughWalkthroughAdds Helpers::remove_dir(string $dir) to App\Assets\Helpers, documents it on the Helpers facade, and replaces ad-hoc directory removals in a controller and a job to call the centralized helper; the job removes its internal remover and returns early on success. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
ildyria
changed the title
Codecov Report❌ Patch coverage is 🚀 New features to boost your workflow:
|
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
app/Facades/Helpers.php (1)
31-31: Facade doc added — consider camelCase for consistencyMost methods on Helpers use camelCase. Consider renaming to removeDir for consistency across the codebase.
Apply across files:
- * @method static void remove_dir(string $dir): void + * @method static void removeDir(string $dir): voidapp/Assets/Helpers.php (2)
279-282: Add minimal guardrails to avoid accidental top-level deletionCheap safety net; bail out early on invalid or dangerous inputs. Keeps Helper generic while avoiding obvious footguns.
public function remove_dir(string $dir): void { + // Guardrails + if ($dir === '' || $dir === '/' || !is_dir($dir)) { + return; + } $it = new \RecursiveDirectoryIterator($dir, \RecursiveDirectoryIterator::SKIP_DOTS);
279-291: Optional: method naming consistencyIf you adopt camelCase, update the method here as well.
- public function remove_dir(string $dir): void + public function removeDir(string $dir): voidapp/Http/Controllers/Admin/Maintenance/Cleaning.php (1)
49-53: Prefer getPathname() and align with optional renamegetRealPath() can return false; getPathname() is safer with DirectoryIterator when you already control the base path. Also, if you switch to removeDir (camelCase), update here.
-Helpers::remove_dir($file_info->getRealPath()); +Helpers::remove_dir($file_info->getPathname());If adopting camelCase rename:
-Helpers::remove_dir($file_info->getPathname()); +Helpers::removeDir($file_info->getPathname());And for files:
-unlink($file_info->getRealPath()); +unlink($file_info->getPathname());app/Jobs/CleanUpExtraction.php (1)
58-63: Happy path + early return — LGTM; consider optional catch for richer historyCurrent behavior will fail the job if deletion throws (which may be fine). If you prefer to record a FAILURE status with context instead of a failed job, catch FilesystemException, set status to FAILURE, and rethrow or return.
Example:
-Helpers::remove_dir($this->folder_path); +try { + Helpers::remove_dir($this->folder_path); +} catch (\Safe\Exceptions\FilesystemException $e) { + $this->history->status = JobStatus::FAILURE; + $this->history->save(); + throw $e; // or return; depending on desired semantics +}If adopting camelCase:
-Helpers::remove_dir($this->folder_path); +Helpers::removeDir($this->folder_path);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
app/Assets/Helpers.php(2 hunks)app/Facades/Helpers.php(1 hunks)app/Http/Controllers/Admin/Maintenance/Cleaning.php(2 hunks)app/Jobs/CleanUpExtraction.php(2 hunks)
🔇 Additional comments (3)
app/Assets/Helpers.php (1)
14-19: Safe filesystem imports — LGTMImporting Safe exceptions and functions here is appropriate and keeps failure modes explicit.
app/Http/Controllers/Admin/Maintenance/Cleaning.php (1)
11-11: Centralizing deletion via Helpers facade — good moveReduces duplication and keeps behavior consistent across call sites.
app/Jobs/CleanUpExtraction.php (1)
12-12: Facade import — LGTMKeeps the job lean and delegates deletion specifics to the shared helper.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/Assets/Helpers.php (2)
294-295: Release iterators before removing the root directory (Windows handle contention)On Windows, open directory handles held by iterators can prevent rmdir(). Free them explicitly before the final removal.
- rmdir($dir); + // Ensure iterators release directory handles before final removal (notably on Windows) + unset($files, $it); + rmdir($dir);
277-278: Docblock: include all thrown exceptionsIf you adopt the guards above, document the additional exception for callers.
- * @throws FilesystemException + * @throws FilesystemException + * @throws \InvalidArgumentException if $dir is root-like or invalid
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
app/Assets/Helpers.php(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.php
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.php: New PHP files must include the project license header and have a single blank line after the opening <?php tag
Use snake_case for PHP variable names
Apply PSR-4 coding standard to PHP code
Always call in_array with true as the third parameter for strict checking (in_array($needle, $haystack, true))
Only use boolean expressions in if statements; avoid integers or strings as conditions
Use strict comparison (===, !==) instead of loose comparison (==, !=)
Avoid duplicating code in both if and else branches
Do not use empty(); prefer explicit checks (e.g., === null, === '', count(...) === 0)
Files:
app/Assets/Helpers.php
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: 3️⃣ PHP dist / 8.3 - sqlite
- GitHub Check: 3️⃣ PHP dist / 8.4 - sqlite
- GitHub Check: 3️⃣ PHP dist / 8.4 - mariadb
- GitHub Check: 3️⃣ PHP dist / 8.3 - postgresql
- GitHub Check: 3️⃣ PHP dist / 8.4 - postgresql
- GitHub Check: 3️⃣ PHP dist / 8.3 - mariadb
- GitHub Check: 2️⃣ PHP tests / 8.4 - sqlite -- Unit,Feature_v2
- GitHub Check: 2️⃣ PHP tests / 8.3 - postgresql -- Unit,Feature_v2
- GitHub Check: 2️⃣ PHP tests / 8.3 - sqlite -- Unit,Feature_v2
- GitHub Check: 2️⃣ PHP tests / 8.4 - mariadb -- Unit,Feature_v2
- GitHub Check: 2️⃣ PHP tests / 8.4 - postgresql -- Unit,Feature_v2
- GitHub Check: 2️⃣ PHP tests / 8.3 - mariadb -- Unit,Feature_v2
🔇 Additional comments (2)
app/Assets/Helpers.php (2)
14-18: Safe filesystem imports: LGTMUsing Safe\rmdir and Safe\unlink is appropriate here and aligns with explicit exception handling.
285-293: Symlink handling inside the tree: well doneTreating symlinks like files (unlink without following) avoids rmdir() failures and prevents unintended traversal.
Summary by CodeRabbit
Bug Fixes
Refactor
Documentation