fix: resolve Copilot review comments from PR #112 #113
+9
−2
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Addresses post-merge Copilot feedback on RBAC Phase 1 code quality: 1. TemporalRoleUser.php: - Fix: Changed $fillable array from 'team_id' to 'tenant_id' - Reason: Matches Spatie Permission config (team_foreign_key => 'tenant_id') - Impact: Ensures mass assignment uses correct column name 2. User.php: - Fix: Added type hint 'Builder $query' to where() callback - Reason: Improves type safety, IDE support, consistency with applyActiveFilter() - Impact: Better developer experience, aligns with PHPStan Level Max All tests passing (12/12 temporal role tests, 163 total)
💡 Tip: Consider Using Draft PRsBenefits of opening PRs as drafts initially:
How to convert:
This is just a friendly reminder - feel free to continue as is! 😊 |
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR addresses post-merge Copilot review comments from PR #112, focusing on two technical improvements to the RBAC Phase 1 implementation:
- Corrects a field name mismatch in the
TemporalRoleUsermodel to align with Spatie Permission configuration - Adds type safety to a callback parameter in the
Usermodel'sroles()relationship
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| app/Models/TemporalRoleUser.php | Changes $fillable array entry from 'team_id' to 'tenant_id' to match the Spatie Permission config setting (team_foreign_key => 'tenant_id') |
| app/Models/User.php | Adds Builder type hint to the where callback parameter in the roles() relationship method for improved type safety |
| CHANGELOG.md | Documents the post-merge fixes with clear descriptions of what was changed and why |
💡 Tip: Consider Using Draft PRsBenefits of opening PRs as drafts initially:
How to convert:
This is just a friendly reminder - feel free to continue as is! 😊 |
kevalyq
added a commit
that referenced
this pull request
Nov 8, 2025
Implements zero-tolerance domain policy enforcement across the codebase. CRITICAL CHANGES: - New check-domains.sh script validates only secpal.app and secpal.dev domains - Integrated into preflight.sh pre-push hook (ZERO TOLERANCE) - Smart exclusions for documentation and YAML arrays - Blocks push on any violation FIXES APPLIED (Copilot Review): - Shebang portability (#!/usr/bin/env bash) - Restored DDEV auto-detection for PHP checks - Restored Pint --test --dirty workflow - Restored eol=lf and Copyright Contributors in .gitattributes - Fixed misleading .pre-commit-config.yaml reference Closes #113
28 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix Copilot Review Comments from PR #112
Context
After merging PR #112, Copilot identified 2 valid issues in the post-merge review that require fixes.
Changes
1. TemporalRoleUser.php - Fix $fillable array
Issue: Used
'team_id'instead of'tenant_id'in mass assignable fieldsFix: Changed to
'tenant_id'(line 50)Reason:
'team_foreign_key' => 'tenant_id'(config/permission.php line 102)tenant_id, notteam_id2. User.php - Add type hint to callback
Issue: Missing type hint on
$queryparameter inwhere()callbackFix: Changed
function ($query)tofunction (Builder $query)(line 92)Reason:
TemporalRoleUser::applyActiveFilter()use Illuminate\Database\Eloquent\Builder;import3. CHANGELOG.md
Added entry under
[Unreleased] → Fixedsection documenting both fixes.Quality Checks
✅ All 163 tests passing (467 assertions)
✅ PHPStan Level Max: 0 errors
✅ Laravel Pint: PSR-12 compliant
✅ REUSE 3.3: License compliance verified
Notes
Relates to #110 (RBAC Phase 1 Tests)
Follow-up to #112