Feat: Affected files #51
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThe GitHub webhook push-event handling in Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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. Comment |
![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 (1)
tests/VCS/Adapter/GitHubTest.php (1)
150-153: Consider order-independent assertions for robustness.The test assertions validate both the count and specific order of affected files. While the current implementation maintains insertion order, this creates a dependency on the order of commits in the webhook payload.
For more robust testing, consider verifying the presence of files without assuming order:
$this->assertCount(3, $pushResult['affectedFiles']); - $this->assertSame('src/lib.js', $pushResult['affectedFiles'][0]); - $this->assertSame('README.md', $pushResult['affectedFiles'][1]); - $this->assertSame('src/main.js', $pushResult['affectedFiles'][2]); + $this->assertContains('src/lib.js', $pushResult['affectedFiles']); + $this->assertContains('README.md', $pushResult['affectedFiles']); + $this->assertContains('src/main.js', $pushResult['affectedFiles']);Alternatively, if order is intentional and important, document this behavior explicitly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/VCS/Adapter/Git/GitHub.php(2 hunks)tests/VCS/Adapter/GitHubTest.php(2 hunks)
🔇 Additional comments (2)
src/VCS/Adapter/Git/GitHub.php (1)
695-695: LGTM!The addition of
affectedFilesto the return payload is appropriate and consistent with the structure of the push event result.tests/VCS/Adapter/GitHubTest.php (1)
57-87: LGTM!The test payload correctly includes a commits array with realistic file changes (added, removed, and modified), which aligns with the GitHub webhook format for push events.
There was a problem hiding this comment.
Pull request overview
This PR adds support for tracking affected files in GitHub push webhook events. The implementation collects all files that were added, removed, or modified across all commits in a push event and exposes them through a new affectedFiles array in the event response.
Key Changes:
- Added logic to collect and deduplicate affected files from all commits in a push event
- Updated test fixtures to include commit details with file changes
- Added assertions to verify the affected files are correctly extracted and returned
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/VCS/Adapter/Git/GitHub.php | Implements logic to collect affected files from commits and adds affectedFiles to the push event response |
| tests/VCS/Adapter/GitHubTest.php | Adds test fixture data with commit file changes and assertions to validate affected files tracking |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/VCS/Adapter/Git/GitHub.php (1)
654-667: Good progress on performance optimization; consider completing the previous review suggestions.The associative array approach successfully addresses the O(n²) performance concern raised in previous reviews. The null coalescing operators add basic validation.
However, the previous review suggested explicit
is_array()checks for defensive programming. In PHP 8+, if$payload['commits']is set to a non-array value (e.g., a string), theforeachwill throw aTypeError. While GitHub's webhook payloads should be well-formed, adding the explicit check makes the code more resilient to malformed input.Additionally, the previous review recommended using a generic variable name like
$fileinstead of$added,$removed,$modified, since all three loops are simply adding file paths to the same deduplicated set.Consider applying the approach from the previous review to strengthen validation and improve clarity:
$affectedFiles = []; -foreach (($payload['commits'] ?? []) as $commit) { - foreach (($commit['added'] ?? []) as $added) { - $affectedFiles[$added] = true; +$commits = $payload['commits'] ?? []; +if (is_array($commits)) { + foreach ($commits as $commit) { + foreach (($commit['added'] ?? []) as $file) { + $affectedFiles[$file] = true; + } + foreach (($commit['removed'] ?? []) as $file) { + $affectedFiles[$file] = true; + } + foreach (($commit['modified'] ?? []) as $file) { + $affectedFiles[$file] = true; + } } - - foreach (($commit['removed'] ?? []) as $removed) { - $affectedFiles[$removed] = true; - } - - foreach (($commit['modified'] ?? []) as $modified) { - $affectedFiles[$modified] = true; - } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/VCS/Adapter/Git/GitHub.php(2 hunks)
🔇 Additional comments (1)
src/VCS/Adapter/Git/GitHub.php (1)
689-689: LGTM!The
array_keys()call correctly extracts the deduplicated file paths from the associative array.
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.