-
Notifications
You must be signed in to change notification settings - Fork 67
Dev #1717
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughBuilding model relationship methods renamed from verbose internal names to simplified aliases. BuildingController updated to use the new method names. Subnetwork::contains now accepts nullable strings with a guard clause. Build script conditional logic prevents unnecessary git operations when no asset changes detected. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/Models/Building.php (1)
70-73: Remove duplicate relationship methodbuildingPhysicalSwitch().This method is a duplicate of
physicalSwitches()at line 106. Both define identicalHasManyrelationships toPhysicalSwitchwith the same foreign key, local key, and ordering. Keeping both methods creates ambiguity and can lead to confusion about which one should be used.Apply this diff to remove the duplicate:
- /** @return HasMany<PhysicalSwitch, $this> */ - public function buildingPhysicalSwitch(): HasMany - { - return $this->hasMany(PhysicalSwitch::class, 'building_id', 'id')->orderBy('name'); - } -
🧹 Nitpick comments (1)
app/Models/Subnetwork.php (1)
214-218: Add braces to the if statement for consistency with coding standards.While PHP allows single-line if statements without braces, PSR-2 coding standards recommend always using braces for control structures to improve readability and prevent future bugs.
Apply this diff:
public function contains(?string $ip): bool { - if ($ip === null) + if ($ip === null) { return false; + } $cidr = $this->address;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (4)
app/Http/Controllers/Admin/BuildingController.php(1 hunks)app/Models/Building.php(2 hunks)app/Models/Subnetwork.php(1 hunks)build.sh(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
build.sh (1)
34-41: LGTM! Good improvement to prevent unnecessary commits.The conditional logic correctly prevents git operations when there are no changes in
public/build/assets/, avoiding empty commits.app/Http/Controllers/Admin/BuildingController.php (1)
141-141: LGTM! Relationship names are consistent with the Building model.The eager-loaded relationships correctly reference the renamed methods in the Building model.
app/Models/Building.php (1)
58-61: LGTM! Relationship method names simplified effectively.The renamed relationship methods maintain correct
HasManyconfigurations with proper foreign keys, local keys, and ordering. The new names are cleaner and more consistent.Also applies to: 76-79, 82-85, 88-91, 94-97, 100-103, 106-109
Summary by CodeRabbit
Refactor
Bug Fixes
Chores